5 ms·

I've been using `math/rand` instead of `crypto/rand` where `crypto/rand` was absolutely needed. This resulted in static keys being used in early versions of dns

I've been using `math/rand` instead of `crypto/rand` where `crypto/rand` was absolutely needed. This resulted in static keys being used in early versions of dnscrypt-proxy2. The reason is that I'm using the VSCode extension that automatically adds imports. In all the source files requiring secure randomness, I carefully imported `crypto/rand` manually, but I forgot to do it in one of the files. Everything compiled and worked file, and I didn't notice that in that specific file, the extension silently added a `math/rand` import. Since then, I import `crypto/rand` as `cryptorand` to avoid the wrong `rand` to be automatically imported. By the way, Zig also uses a ChaCha8-based random number generator, and for cryptographic operations, people can't supply their own generator; the secure one is always used. For testing, some functions accept an explicit seed. For constrained environments, the standard library also includes a smaller one based on the Ascon permutation and the Reverie construction.

edflsafoiewq 2y ago
Someone else mentioned this same thing. Tbh the practice of automatically adding imports seems totally crazy and defeats the whole point of segregating names into different namespaces.
- SamWhited 2y ago
  I have a distinct memory from the first Go contributor summit where I brought this up (I have no idea what we were discussing or what it was in response to) and the attitude of every other developer at the table was "yah, but we special cased this after it caused problems with crypto/rand, math/rand so goimports is fixed now and it's fine". And then it happened again with every IDE. And with other packages that haven't been special cased yet. And with… I dunno, I never could convince anyone but it just seems like a terrible idea to me. Please just write out your imports, it doesn't take that long. :(
  - arp242 2y ago
    > Please just write out your imports, it doesn't take that long. I have to disagree with that; add debug fmt.Println() → add fmt import. Okay, found it, so remove debug → remove fmt import. Okay so the problem was that we need to use filepath.EvalSymlinks() → add path/filepath import. Wait, that still didn't work; let's add back that debug Println()... etc. etc. Other people may have other dev cycles, but I hugely miss it when it's not available (I sometimes do some work on VMs for cross-platform work where this is the case). Same with gofmt really; these days much of the time I just write: if foo==bar{fun()}else{panic("oh noes")} And let gofmt sort it out. Of course I can write all the spaces and whatnot but why bother? I do agree the whole package shadow thing is a right pain.
  - kbolino 2y ago
    The "write out your imports" ship sailed with modules. Nobody wants to write out "code.internal.corporate.domain/bureaucratic/hierarchy/of/orgs/foo" when they're looking for "foo". Even GitHub-hosted modules have fairly long names. The tools need to get better though. I'd rather they fall back to asking me than guessing when the import is ambiguous. I also think they should only ever autoimport from the stdlib or what's directly referenced in go.mod.
    - SamWhited 2y ago
      I mean, copy/paste is a thing, I'm not suggesting you have to always type out every single character. Just know what's being imported.
      - kbolino 2y ago
        I don't see a functional difference between copy-paste and autoimport. They have more or less the same risks. Imports should rarely be manually managed directly in source. Doing that is the epitome of tedium and most imports are just boilerplate, so your eyes will glaze over and you'll miss the finer points unless a specific file warrants deeper scrutiny. That doesn't mean imports should never be reviewed, and again, I think the tools need a lot of improvement in this regard.
        tedunangst 2y ago
        I've never copy and pasted crypto/rand and gotten math/rand.
        kbolino 2y ago
        Yeah that's a legit gripe but I wasn't really talking about the standard library which uses nice short import paths.
- jedisct1 2y ago
  It's convenient, and probably fine when imports are unambiguous. The problem here is that there were multiple possible imports for the same name, and in such situation, the extension picks an arbitrary one. It feels rather like a bug (or a design issue) in the extension, that can be fixed, than a conceptual issue in automatic imports.
  - rsc 2y ago
    It doesn't pick an arbitrary one. It prefers crypto/rand, and has since 2016 (https://go-review.googlesource.com/24847 https://go-review.googlesource.com/24847).
- amenhotep 2y ago
  This is a pet theory borne of no research at all, but from my fiddling around with Go I feel like this is a necessary feature for one reason: Go will refuse to compile if you have unused imports. When I'd be playing around trying to get things to work, the import management thing was a lifesaver, I'd delete a line of code that was only a temporary exploration and its imports would disappear, I'd write something similar and the tool would add the imports back again. The annoyance of having to add them back constantly or always make sure there was some dummy "use" somewhere or manually comment them out and back in every time would be incredible to me. It seems like it would be a lot better to simply warn on used imports and maybe refuse in release mode, but Go apparently thinks religious purity in one aspect is worth the price of making standard tooling remarkably lax in another?
- rsc 2y ago
  In general there is very little overlap. crypto/rand vs math/rand don't even overlap except for rand.Read, and that was a mistake. We also corrected the import fixer in 2016 to prefer crypto/rand, so no one should have run into this problem in a very long time. https://go-review.googlesource.com/24847 https://go-review.googlesource.com/24847
rsc 2y ago
I'm not sure what happened in your case, but it probably wasn't what you describe. We changed goimports in 2016 to prefer crypto/rand over math/rand (https://go-review.googlesource.com/24847 https://go-review.googlesource.com/24847), and that was before there was VSCode support for Go.
- rsc 2y ago
  Filippo and I dug into this a bit more, and it is possible that VSCode auto-complete (which also adds imports for the auto-completed things) is the culprit here. Apologies if that's what happened to you. We will look into fixing that. Now that math/rand.Read is marked deprecated, at least if it does get selected, you get a nice strikethrough rendering as well.
  - meling 2y ago
    It is probably not related, but gopls has a known issue with its heuristic for picking imports: https://github.com/golang/go/issues/61208 https://github.com/golang/go/issues/61208

edflsafoiewq 2y ago

Someone else mentioned this same thing. Tbh the practice of automatically adding imports seems totally crazy and defeats the whole point of segregating names into different namespaces.

SamWhited 2y ago
I have a distinct memory from the first Go contributor summit where I brought this up (I have no idea what we were discussing or what it was in response to) and the attitude of every other developer at the table was "yah, but we special cased this after it caused problems with crypto/rand, math/rand so goimports is fixed now and it's fine". And then it happened again with every IDE. And with other packages that haven't been special cased yet. And with… I dunno, I never could convince anyone but it just seems like a terrible idea to me. Please just write out your imports, it doesn't take that long. :(
- arp242 2y ago
  > Please just write out your imports, it doesn't take that long. I have to disagree with that; add debug fmt.Println() → add fmt import. Okay, found it, so remove debug → remove fmt import. Okay so the problem was that we need to use filepath.EvalSymlinks() → add path/filepath import. Wait, that still didn't work; let's add back that debug Println()... etc. etc. Other people may have other dev cycles, but I hugely miss it when it's not available (I sometimes do some work on VMs for cross-platform work where this is the case). Same with gofmt really; these days much of the time I just write: if foo==bar{fun()}else{panic("oh noes")} And let gofmt sort it out. Of course I can write all the spaces and whatnot but why bother? I do agree the whole package shadow thing is a right pain.
- kbolino 2y ago
  The "write out your imports" ship sailed with modules. Nobody wants to write out "code.internal.corporate.domain/bureaucratic/hierarchy/of/orgs/foo" when they're looking for "foo". Even GitHub-hosted modules have fairly long names. The tools need to get better though. I'd rather they fall back to asking me than guessing when the import is ambiguous. I also think they should only ever autoimport from the stdlib or what's directly referenced in go.mod.
  - SamWhited 2y ago
    I mean, copy/paste is a thing, I'm not suggesting you have to always type out every single character. Just know what's being imported.
    - kbolino 2y ago
      I don't see a functional difference between copy-paste and autoimport. They have more or less the same risks. Imports should rarely be manually managed directly in source. Doing that is the epitome of tedium and most imports are just boilerplate, so your eyes will glaze over and you'll miss the finer points unless a specific file warrants deeper scrutiny. That doesn't mean imports should never be reviewed, and again, I think the tools need a lot of improvement in this regard.
      - tedunangst 2y ago
        I've never copy and pasted crypto/rand and gotten math/rand.
        kbolino 2y ago
        Yeah that's a legit gripe but I wasn't really talking about the standard library which uses nice short import paths.
jedisct1 2y ago
It's convenient, and probably fine when imports are unambiguous. The problem here is that there were multiple possible imports for the same name, and in such situation, the extension picks an arbitrary one. It feels rather like a bug (or a design issue) in the extension, that can be fixed, than a conceptual issue in automatic imports.
- rsc 2y ago
  It doesn't pick an arbitrary one. It prefers crypto/rand, and has since 2016 (https://go-review.googlesource.com/24847 https://go-review.googlesource.com/24847).
amenhotep 2y ago
This is a pet theory borne of no research at all, but from my fiddling around with Go I feel like this is a necessary feature for one reason: Go will refuse to compile if you have unused imports. When I'd be playing around trying to get things to work, the import management thing was a lifesaver, I'd delete a line of code that was only a temporary exploration and its imports would disappear, I'd write something similar and the tool would add the imports back again. The annoyance of having to add them back constantly or always make sure there was some dummy "use" somewhere or manually comment them out and back in every time would be incredible to me. It seems like it would be a lot better to simply warn on used imports and maybe refuse in release mode, but Go apparently thinks religious purity in one aspect is worth the price of making standard tooling remarkably lax in another?
rsc 2y ago
In general there is very little overlap. crypto/rand vs math/rand don't even overlap except for rand.Read, and that was a mistake. We also corrected the import fixer in 2016 to prefer crypto/rand, so no one should have run into this problem in a very long time. https://go-review.googlesource.com/24847 https://go-review.googlesource.com/24847

rsc 2y ago

I'm not sure what happened in your case, but it probably wasn't what you describe. We changed goimports in 2016 to prefer crypto/rand over math/rand (https://go-review.googlesource.com/24847 https://go-review.googlesource.com/24847), and that was before there was VSCode support for Go.

rsc 2y ago
Filippo and I dug into this a bit more, and it is possible that VSCode auto-complete (which also adds imports for the auto-completed things) is the culprit here. Apologies if that's what happened to you. We will look into fixing that. Now that math/rand.Read is marked deprecated, at least if it does get selected, you get a nice strikethrough rendering as well.
- meling 2y ago
  It is probably not related, but gopls has a known issue with its heuristic for picking imports: https://github.com/golang/go/issues/61208 https://github.com/golang/go/issues/61208