There are a few crypto APIs that take an io.Reader as a source of random bytes, but that don't commit to how those bytes are used. This caused issues over and over, for example any time we wanted to change the algorithm. These days we both document that they are not deterministic, and use randutil.MaybeReadByte to somewhat enforce it. See #58637.

Now that we have GODEBUGs, it might be time to rip the band-aid off. I propose we start ignoring the random io.Reader parameter of the following APIs, and always use the system random source (crypto/internal/sysrand.Read, not crypto/rand.Reader which may be overridden by the application).

  • rsa.GenerateKey and rsa.GenerateMultiPrimeKey
  • rsa.EncryptPKCS1v15
  • ecdsa.GenerateKey
  • ecdsa.SignASN1, ecdsa.Sign, and ecdsa.PrivateKey.Sign
  • ecdh.Curve.GenerateKey

Using GODEBUG=cryptocustomrand=1 restores the old behavior. (Suggestions for a better name welcome.) This is a GODEBUG that I would like to remove in a few releases.

rsa.SignPKCS1v15 is not randomized, while rsa.SignPSS and rsa.EncryptOAEP have a fairly well-specified way to use random bytes. Aside from those and ed25519.GenerateKey (see below), I think I listed all APIs in non-deprecated packages that take a random io.Reader.

This might be an issue for the crypto/tls tests, which defuse MaybeReadByte by producing a stream of identical bytes. That's an abuse of GenerateKey anyway, because there is no guarantee that algorithms that expect random inputs will work with constant repeating streams. See for example #70643.

ed25519.GenerateKey is a weird exception in that it is well defined, but also documented to use crypto/rand.Reader if nil is passed. This is annoying because it forces a dependency on crypto/rand and therefore on math/big. We can't just use crypto/internal/sysrand.Read because the user might have overridden crypto/rand.Reader. I am tempted to also propose replacing "crypto/rand.Reader" with "the system random source" but it's probably not worth the risk.

/cc @golang/security

Comment From: gabyhelp

Related Issues

Related Code Changes

Related Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

Comment From: seankhliao

it seems quite rare to override rand.Reader outside of tests https://github.com/search?q=language%3Ago+%2Frand.Reader+%3D+%2F+-path%3A*_test.go&type=code

Comment From: mateusz834

Are there any real world use-cases where it is currently necessary to provide a different random source, than crypto/rand.Reader? seccomp?

Comment From: FiloSottile

This might be an issue for the crypto/tls tests, which defuse MaybeReadByte by producing a stream of identical bytes. That's an abuse of GenerateKey anyway, because there is no guarantee that algorithms that expect random inputs will work with constant repeating streams. See for example #70643.

This is actually a pretty common requirement in tests, and it feels wrong to get tests forever stuck to a combination of GODEBUG=cryptocustomrand=1 and defusing MaybeReadByte just to end up with breakage any time we change the algorithm.

We discussed a solution with @rsc: let's just acknowledge that tests need this, and that in tests it might make sense to take the tradeoff of losing backwards compatibility, and provide an explicit way to get to the same result, which only works in tests and which is also useful for other things.

package testing

// RandReader returns a Reader that produces a deterministic pseudo-random
// stream based on the seed.
//
// The returned reader can also be used as the rand parameter to
// rsa.GenerateKey, rsa.EncryptPKCS1v15, ecdsa.GenerateKey,
// ecdsa.SignASN1, ecdsa.Sign, and ecdsa.PrivateKey.Sign,
// and ecdh.Curve.GenerateKey to cause them to behave deterministically.
// Note that the output of these functions may and will change across Go versions,
// so any tests using this affordance must be prepared to
// update vectors based on the Go version (e.g. with a build tag).
//
// It can only be called in a test (as reported by [Testing]),
// otherwise it panics.
func RandReader(seed int64) io.Reader

Comment From: aclements

Overall this seems reasonable, though it will be a difficult transition.

We've been trying to keep niche things out of the testing package proper. How about testing/cryptotest? Then we can also call it InsecureRandReader to really hammer home that you shouldn't use this.

Comment From: FiloSottile

I proposed it as a generically named thing because "a reproducible sequence of pseudo-tandom bytes, for testing" is something I've needed quite a few times, not only when testing crypto, and I jury-rigged it with AES-CTR or ChaCha8.

I wonder if that's my selection bias, but I've heard from others that they would have needed this for generic purposes.

With the testing.Testing check and such a small seed, I am not really worried about folks using it thinking it's secure.

Ultimately, no strong opinion, although a new package feels like a big lift.

Comment From: aclements

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — aclements for the proposal review group

Comment From: aclements

There are a lot of other APIs that take random io.Readers, too, like EncryptOAEP, EncryptPKCS1v15, etc. Though I also see there are a lot of functions that take a random io.Reader but are documented to ignore it. What's your ideal final state here?

It seems really odd that these APIs are going to take an io.Reader that they nearly always ignore. Another option is that we introduce new APIs that don't take the io.Reader and we deprecate the APIs that do. This would let us keep the old APIs working the way they do today, at least for another release or two, but give a strong signal that people need to move off the old APIs. I'd be much more comfortable with further changing the behavior of these APIs if people had a strong signal that this was coming. Though this wouldn't help with the testing issue.

This may be retreading old ground, but another option would be a vet check that requires the reader to be crypto/rand.Reader except in a test.

Can you give more context, or point to a past discussion, on why we decided it was okay to break backwards compatibility for these random sources?

Comment From: neild

I proposed it as a generically named thing because "a reproducible sequence of pseudo-tandom bytes, for testing" is something I've needed quite a few times, not only when testing crypto, and I jury-rigged it with AES-CTR or ChaCha8.

If testing.RandReader is worth adding on its own merits, I think that should be a separate proposal. (My main question there would be: Have we made it too difficult to acquire non-secure bytes out of math/rand/v2? Could this be rand/v2.Rand.InsecureReader?)

Comment From: aclements

Can you give more context, or point to a past discussion, on why we decided it was okay to break backwards compatibility for these random sources?

To clarify my question, I mean that, if I understand correctly, originally we were using the bytes from these readers in a semi-deterministic way: if you always provided the same bytes, you'd get the same results, unless you upgraded Go and there was an algorithm implementation change. At some point we moved to using those bytes but randomly shifting the stream in MaybeReadByte. I'm curious where we had the discussion in which we decided that was okay and not a violation of backwards compatibility.

Comment From: seankhliao

I believe that was #21915 / CL 64451

Comment From: rolandshoemaker

I think generally deprecation should be reserved for times when people should really stop using something, whereas in this case none of the underlying code would change, we'd just be dropping a argument. In an ideal world where we completely redesigned these APIs I think we'd drop the argument, but I don't think there is a reasonable way to do that now (also losing some good names, like GenerateKey, would be an overall UX loss).

One possible additional option is to steal @neild's concept of bubbles from the synctest package, and provide a way within tests to set the global crypto/rand to a deterministic reader. In this option we'd document that these functions always use the global source of randomness (unless you set the relevant GODEBUG). We'd then provide a function (perhaps in cryptotest?) which takes a testing.T and sets the global source of crypto randomness to a deterministic reader, say func Test(t *testing.T, seed int64, f func(*testing.T)), for the current goroutine and all of its children.

All methods which depend on the global source of randomness would then become ~deterministic, allowing for testing without the need to construct a special reader.

The major win here would be (a) the simplicity (from the user perspective, the implementation may be somewhat complex) and (b) that we could explicitly document that the rand argument to various functions is always ignored, and don't need to worry about the semantics of when we use a special value that is passed in or not (and if we need to add checks for people passing in non-test readers etc).

Comment From: aclements

Expanding the "bubble" concept just for this feels like a heavy lift. There may be something there (bubble-local storage? 🤔), but I think it'd need to be much better motivated.

A simpler alternative would be a function like testing.WithCryptoReader(f func()) that simply sets a global deterministic random source for the duration of f, without attempting any sort of bubble-like isolation. There are hazards to that, like it wouldn't work in parallel tests, but there's precedent for such hazards, like T.Chdir.

Comment From: aclements

@rolandshoemaker points out that if we do WithCryptoReader and include the testing.T, it could fail in a parallel test. It could also fail if the global rand is already set to a testing rand. @rolandshoemaker will post a concrete API.

Comment From: rolandshoemaker

// WithCryptoReader runs function f with the global cryptographic random source
// replaced by a deterministic random source, seeded with seed. This can be used
// to test cryptographic APIs when determinism is needed.
//
// Because WithCryptoReader affects the whole process, it cannot be used in parallel
// tests or tests with parallel ancestors.
func WithCryptoReader(t testing.T, seed uint64, f func())

Comment From: qmuntal

@rolandshoemaker what about renaming your WithCryptoReader function to WithReader and placing it in the testing/cryptotest package. This functions looks like too niche to be in testing.

Comment From: rolandshoemaker

Ah yeah, I should've noted that in the comment, I think we'd want this in cryptotest. I think WithRandReader might be a better name, or WithDeterministicGlobalRand to be extremely clear about what it does.

Comment From: aclements

It sounds like we're converging on:

Always ignore the rand io.Reader argument to crypto functions (with a GODEBUG to undo that), and add cryptotest.WithReader(t *testing.T, seed uint64, f func()) to temporarily set a global deterministic random reader for testing. WithReader would have safeguards (e.g., panicking if !testing.Testing()) and documented caveats that changes to crypto implementations can change how they use randomness and thus change their results.

There are also several things that use crypto/rand.Reader, which is a global variable that can in principle be rebound to some other reader. Is the proposal to only change functions that currently take an io.Reader to use sysrand, or to also replace all uses of crypto/rand.Reader and crypto/rand.Read?