> Go aims to help developers write code that is secure by default. When we observe a common mistake with security consequences, we look for ways to reduce the risk of that mistake or eliminate it entirely. In this case, math/rand’s global generator was far too predictable, leading to serious problems in a variety of contexts.
> For example, when Go 1.20 deprecated math/rand’s Read, we heard from developers who discovered (thanks to tooling pointing out use of deprecated functionality) they had been using it in places where crypto/rand’s Read was definitely needed, like generating key material.
I made exactly this mistake in rclone. I refactored some code which was using the Read function from crypt/rand and during the process the import got automatically changed (probably by goimports when mixing code which did use math/rand) to math/rand. So it changed from using a secure random number generator to a deterministic one rclone seeded with the time of day. I didn't notice in the diffs :-( Hence
Ouch, apologies for that. We changed goimports to prefer crypto/rand back in 2016, so I'm not entirely sure what happened during your refactoring. Perhaps code that used other math/rand-only APIs ended up in the same file. https://go-review.googlesource.com/24847
Ever since 2016 I actually do a named import for "math/rand" now by calling it "mathrand" or "weakrand" just to be sure I don't accidentally use it for anything secure.
Go keeps getting better. Thanks for your hard work!
Yes I'm pretty sure that is what happened. I was consolidating random functions into one file which was already using math/rand so when I moved the function which was using crypto/rand there it picked up the math/rand import.
So not goimports problem, my problem for expecting goimports to magically do the right thing like it usually does!
I've also had some report a vulnerability because they thought math/rand was being used when it wasn't. They just mixed something up with a few different files – not a big deal – but it just goes to show how confusing the entire thing is.
text/template and html/template are similar. In hindsight this package name shadowing was a bad idea.
This is one of my bigger remaining issues with day-to-day use of Go. I love how neat Go `package.Symbol` usually looks -- that objective was met -- but it has at least these problems:
* It is syntactically identical to `variable.Member` so even at a glance it's ambiguous. After shadowing occurs, diagnostics get really confused. Go could at least include more error information when this is a possible cause.
* The best package names have a lot of overlap with the best local variable names and in some cases good unexported type names [1]. Single words, almost exclusively nouns, usually singular form (prior to `slices`/`maps`), etc. so if you follow these idioms for both packages and variables you risk a lot of shadows. Who among us hasn't wanted to shadow the name `url` or `path`? To this day I feel I waste a lot of time trying to choose good package names without burning the namespace of good variable names.
Outside the standard library, the official gRPC library is an arguably even better example of terrible package naming: `codes`, `peer`, `status`, and `resolver` are all likely variable names too. The only properly namespaced package is `grpclog`.
* Even if you accept that lowercase is unexported and uppercase is exported, the lowercase side of this makes package-level unexported type names more likely to shadow package names. You can nest those type names inside functions, unless they're generic functions, and these nested types can't have methods so they're very rarely useful. If intra-package namespacing and privacy were more refined, at least we'd have more workarounds to avoid type names shadowing package names.
* Semi-related, with packages being the only way to enforce privacy, it seems like we're encouraged to create a lot of packages if we want to enforce privacy a lot. But the more you create, the more globally unique names you have to choose, creating more pressure on that namespace shared with variables and unexported types.
* You can't nest `pkg1.pkg2.Symbol` even though you can nest `var1.var2.Member`[2]. This could get ugly fast and I don't actually want this in the language, but if it had existed then it would have been a useful tool to resolve ambiguity and separate namespaces without aliases. In any case it's another inconsistency between syntax and semantics.
* Import aliases can solve a lot of problems on the spot, but trades them for other problems. When used inconsistently, humans can get confused jumping around between files, but there's no tooling to enforce consistent use, not even a linter. Even when used consistently, tooling can still get confused, e.g. when you move code to another file that doesn't have the import yet then your tooling has to make its best guess and even the state of the art makes a lot of mistakes. In general, having even a single import alias makes code snippets less "portable" even within a project and even more so across projects, so in practice they should be avoided.
* Package names are tied to file organization while still also being tied to privacy. When you're perfectly happy with a project structure, this is very elegant and you forget all about it. When you want to refactor a bit, especially to avoid a circular dependency or adjust privacy control, you have to update all callers at best or are permanently limited by your exported API at worst [3]. I observe that most libraries out there prefer to have just a couple of huge exported packages, giving up privacy on their end in exchange for simplicity for users.
* Dot imports were designed in a way that ensures they never get used. You can only dot-import an entire package, not individual symbols from it, so any dot-import is a semver hazard and discouraged. It didn't have to be this way, importing individual symbols (like C++, Python, Java, Rust, and probably many more [4]) would have still reduced clutter without trading it for a semver hazard. It's a strange oversight in a language otherwise so well suited to writing future-proof code. Of my gripes in this comment, I think this is the main one I feel could be resolved by backwards-compatible language extensions, but it's also the least relevant to the original issue of name shadowing.
These are all manageable issues when you carefully structure your own projects and pick all of your own package names. Though, I'm sure anyone who has worked in Go long enough has had to contribute to (or outright inherit) a project written with a very different approach that has a lot more shadowing hazards, showing how these individually simple rules can combine to serious quality of life issues on a project. Exported package names often become unfixable, so when you inherit a situation like that, you're going to get routine papercuts with no real way to avoid them.
[1] If the separator wasn't the same `.` then this wouldn't be a problem, though I admit any other separator in this position is automatically uglier.
[2] Assume the type is in the same package so the middle token can be lowercase.
[3] Rust goes a bit far in the other direction by always requiring an explicit module structure, but once you have it, it's actually decoupled from file organization and privacy. That said, my overall experience has been that managing `mod` and `use` hierarchies in Rust is still far more clunky and manual than packages in Go. The good parts are fine privacy control, separate namespaces, and consistent nesting, the bad part is needing boilerplate even in what should be simple cases.
[4] I respect that Go doesn't copy other languages and does its own thing, but when a feature like importing individual symbols is common to languages as different as Python and Rust, there might be real value in solving the same problem in Go too.
This is a great analysis that gave me a lot to think about.
I've always loved the way Go's packages work. With care, I can organize large codebases that end up feeling so clean and nice to deal with.
I usually get to pick all my own package names, so the issues you bring up are manageable for me. But you explained them well, and I can imagine the frustration of inheriting public API packages that were not carefully designed.
And this one is a perennial little naming hiccup:
> Who among us hasn't wanted to shadow the name `url` or `path`?
I still prefer Go's packages to other languages' ways of code organization, but I think you convinced me that giving packages their own separator (and not sharing '.') might have been a better design. Though strangely, as you mentioned, any other separator is uglier.
I also noticed when I tried to search for "secure password generation golang" nearly all examples use math/rand. And to make things worse all of them initialize seed with current time just before generating the password.
This was discovered after I found in our code someone used math rand and was curious where they copied it from :)
goimports has special-cased math/rand.Read vs crypto/rand.Read from basically the beginning. But https://github.com/golang/tools/commit/0835c735343e0d8e375f0... in 2016 references a time window where it could resolve "rand.Read" as "math/rand". Maybe you were in that time window?
I think this was my fault. I refactored some code using crypto/rand into a file which was already using math/rand and failed to notice that the imports were wrong and not magically fixed by goimports like they usually are!
So goimports rocks and my code review skills suck!
Or SemiRand, UnsafeRand, that kinda thing. Bit more explicit naming. I feel like a lot of Go's naming is down to legacy / what people are used to, which on the one side is good because familiarity, but on the other it opens it up to the same mistakes and confusion that has already been made in the past.
The main issue was math/rand having a `Read` utility function with the same signature as crypto/rand, compounded by packages being namespaced but the namespacing not being preserved by importing. So a call to `rand.Read()` could be cryptographic or not you'd have to check the import to be sure.
Would not have happened in C# which uses distinct Random (and Random.Shared) as PRNG and RandomNumberGenerator[0] as CSPRNG even when mixing namespaces.
Also has corresponding analyzer rule if you want to enforce this project-wide[1].
If I had to guess which of Random and RandomNumberGenerator was the cryptographically secure one, I would have guessed wrong. It's not clear either way.
> Go aims to help developers write code that is secure by default. When we observe a common mistake with security consequences, we look for ways to reduce the risk of that mistake or eliminate it entirely. In this case, math/rand’s global generator was far too predictable, leading to serious problems in a variety of contexts.
> For example, when Go 1.20 deprecated math/rand’s Read, we heard from developers who discovered (thanks to tooling pointing out use of deprecated functionality) they had been using it in places where crypto/rand’s Read was definitely needed, like generating key material.
I made exactly this mistake in rclone. I refactored some code which was using the Read function from crypt/rand and during the process the import got automatically changed (probably by goimports when mixing code which did use math/rand) to math/rand. So it changed from using a secure random number generator to a deterministic one rclone seeded with the time of day. I didn't notice in the diffs :-( Hence
https://www.cvedetails.com/cve/CVE-2020-28924/
So this change gets a big :+1: from me.