crypto: document that GenerateKey functions are not deterministic #58637
In Go 1.20 we finally made crypto/ecdsa.GenerateKey call MaybeReadByte, so that now all GenerateKey implementation are non-deterministic, avoiding applications relying on the internals of our implementation, which would stop us from ever changing them. We should document this.
The text was updated successfully, but these errors were encountered:
This change seems to have broken user recovery of lost keys in Upspin (https://upspin.io/doc/faq.md#lost-key) and I'm trying to figure out the best way to keep our app's promise. Any suggestions?
By the way, I'm not the only developer that misunderstood the intent of
Upspin's design and implementation did pass review by Google's AppSec and ISE teams at the time and this key recovery design was not a buried detail, so I conjecture other applications may discover a similar catastrophic problem in the future. I'd like to document the best solution here to save their app users from panic. It doesn't need to be high performance or constant-time, just a rarely used standalone compatibility tool, ideally fitting into the rest of your work as well as possible.
(By the way, thanks very much for your Go 1.20 cryptography changes!)
A few more points not evident from the quoted Upspin doc:
Thank you for the feedback and sorry for the churn.
Deterministic key generation absolutely has its place, and is a reasonable design to adopt in your case. However, relying on Hyrum's Law and the unspecified behavior exposed by an implementation is problematic. If you need the same input to always produce the same output, you want a precise specification and test vectors. Kinda surprisingly, the cryptography community has not produced that yet for deterministic key generation. (I do plan to try to fix that at some point.)
Still, this was not a change made out of ideological purity. The other issue with relying on the implementation details of GenerateKey is that it ties our hands in making GenerateKey and its backend better. What happened in Go 1.20 (so that ship has sailed, we're just talking about documentation here) is that we rewrote the backend to be constant time and safer, and a different key generation strategy became a much better choice. (Rejection sampling doesn't require a wide reduction, which we didn't have to implement in the new backend. There are alternatives but they add complexity.)
By the way, I regret GenerateKey taking an io.Reader at all. There is no correct answer for what to pass to it except crypto/rand.Reader.
I do have that for you though! https://pkg.go.dev/filippo.io/keygen#ECDSALegacy is Go 1.19's GenerateKey, implemented in non-constant time, just like it was in Go 1.19. (Note that timing side channels rarely matter for key generation, as a single observation is not enough to leak significant parts of the key, except when doing deterministic key generation. For your use case you should be fine, because it's a recovery path, but it was important to make GenerateKey constant time especially if it was being used deterministically.)
I am always reticent to point to my own third-party package in official docs, but is there some place I could have linked that which would have helped you find it?
Thank you, excellent answer!
I would have voted to remove the io.Reader parameter as the clearest fix, but I understand why you were reluctant.
Happy to use your ECDSALegacy while waiting for an eventual deterministic key generation in the standard library. Also, thanks for linking here from the reddit discussion.
As Filippo points out in golang/go#58637, "There is no correct answer for what to pass to it except crypto/rand.Reader". Since we aren't bound by any sort of API stability promise, do the right thing.
in case it helps anyone else coming here in the future:
Sure, the difference is in randFieldElement. The "compatible one" that I'm using is:
whereas the one you're perhaps checking against from ecdsa_legacy.go is:
There are various things one could be "compatible" with, so I'm not asking for any changes in ECDSALegacy but only recording for anyone else in my situation that this is what I needed to do so that Upspin's keygen_test.go would pass, and users' old secretseeds would continue to create the same keys as from years ago.
Hmm, no, ECDSALegacy implements the first algorithm you excerpted
and it's tested against Go 1.19 in GitHub Actions
ECDSALegacy is meant to and documented to be compatible with Go 1.19, that's what it's for, so if it didn't work for you I'd like to know more about how.
Fixes golang#58637 Change-Id: I9eb3905d5b35ea22e22e1d8eb8c33594eac487fc Reviewed-on: https://go-review.googlesource.com/c/go/+/505155 TryBot-Result: Gopher Robot <firstname.lastname@example.org> Run-TryBot: Filippo Valsorda <email@example.com> Reviewed-by: Roland Shoemaker <firstname.lastname@example.org> Reviewed-by: Dmitri Shuralyov <email@example.com> Auto-Submit: Filippo Valsorda <firstname.lastname@example.org>
Refs: #3770 Closes: #3761 Here we upgrade all libp2p libraries to the recent versions. To make it possible, we were also forced to bump the Go version from 1.18 to 1.20. This is the minimum version supported by recent libp2p packages. I recommend reviewing commit by commit where specific changes are described in detail. Here is a brief summary of what has been done: ### Upgrade Go from 1.18 to 1.20 Upgrade of Go resulted in a need to: - Adjust the return type of the `slices.SortFunc` compare function we are using in one unit test. This is because the version of the `golang.org/x/exp` package had to be bumped up as well. The returned type of the compare function used in `slices.SortFunc` was changed from `bool` to `int` somewhere between (5a980c7) - Fix the `TestCoordinationExecutor_Coordinate` which started to be flaky due to a changed behavior of `ecdsa.GenerateKey`. [Since Go 1.20](golang/go#58637), the returned key no longer depends deterministically on the bytes read from the provided RNG, and may change between calls and/or between versions (2ed7179) - Fix the `TestWalletRegistry_getWalletByPublicKeyHash_NotFound` which used a dummy curve point. Since Go 1.19, such a behavior leads to a panic (50b6bd6) - Reformat code using the new `gofmt` version (3c2274e) - Adjust the Dockerfile (8e07451) - Bump `staticcheck` version used by CI and fix the new warnings about deprecated standard library functions by replacing them as recommended (a87eea3) ### Upgrade of libp2p libraries Upgrade of libp2p packages forced us to: - Adjust `go-libp2p-core` imports to be `go-libp/core` as this package was moved to the `go-libp2p` monorepo (95d60b8) - Adjust our `transport` and `authenticatedConnection` implementations to expose some additional functions required by libp2p interfaces (ac01765) - Set up our `transport` differently due to the changes around libp2p `Security` option (110fbb3, 6953b79)