Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: math/rand/v2: add ChaCha8.Read #67059

Open
FiloSottile opened this issue Apr 26, 2024 · 7 comments
Open

proposal: math/rand/v2: add ChaCha8.Read #67059

FiloSottile opened this issue Apr 26, 2024 · 7 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@FiloSottile
Copy link
Contributor

Currently, if an application needs to generate a reproducible sequence of random bytes, it can choose amongst

  • math/rand.(*Rand).Read, which uses the predictable Go 1 random number generator; or
  • making a zeroReader that returns a sequence of zero bytes and using cipher.StreamReader to combine it with a *chacha20.Cipher, which is pretty inconvenient.

Since we now have a pretty good secure, deterministic, and specified RNG as math/rand/v2.ChaCha8, it makes sense to add a Read method to it.

func (*ChaCha8) Read(p []byte) (n int, err error)

Since it's on the *ChaCha8 value, it's not going to be accidentally misused in place of crypto/rand.

/cc @golang/security @golang/proposal-review

@FiloSottile FiloSottile added Proposal Proposal-Crypto Proposal related to crypto packages or other security issues labels Apr 26, 2024
@FiloSottile FiloSottile added this to the Proposal milestone Apr 26, 2024
@zephyrtronium
Copy link
Contributor

Compare #65562 which proposes adding Read to Rand instead, apparently on the assumption that it will have a ChaCha8 inside.

@neild
Copy link
Contributor

neild commented Apr 26, 2024

Another possibility:

// Bytes fills p with pseudo-random bytes from the default source.
func Bytes(p []byte)

// Bytes fills p with pseudo-random bytes.
func (*Rand) Bytes(p []byte)

Using the name Bytes avoids any possible confusion with crypto/rand.Read.

Also, it's simpler than implementing io.Reader; why return an error when there's no way the operation can fail?

@FiloSottile
Copy link
Contributor Author

Ah, yes, this is pretty much a #65562 duplicate, thanks @zephyrtronium. /cc @lukechampine

Even if it solves the name collision, I am not convinced by a package-level Bytes: without Seed, what's the point of having both that and crypto/rand.Read?

Eliding the error return is nice, but I think that having *ChaCha8 implement io.Reader is convenient. For example it can be swapped in for crypto/rand.Reader.

@neild
Copy link
Contributor

neild commented Apr 26, 2024

without Seed, what's the point of having both that and crypto/rand.Read?

Sometimes you want some random bytes for non-security purposes, and you don't want to juggle two rand packages or deal with a Read function that returns an error?

@mateusz834
Copy link
Member

mateusz834 commented Apr 27, 2024

I think this proposal offers a performance advantage compared to #65562. The Read method on per-source RNGs can be optimized, whereas with #65562, Rand.Read would need to call each time the Source.Uint64 method, but for consistency with the rest of package the #65562 seems better.

@hagemt
Copy link

hagemt commented Apr 29, 2024

I'm in favor of #67059 (this proposal) over #65562 because I am often generating keys with crypto/ed25519 for tests.

I want the key to be from a potentially-random seed, but using math/rand/v2 for GenerateKey is awkward for the reasons that @FiloSottile mentions. For others who may stumble upon this, I found it nice that I can do this:

import (
	"crypto/ed25519"
	rand1 "math/rand"
	rand2 "math/rand/v2"
)

type RNG struct {
	rand2.Source
}

func newRNG(seed int64) *RNG {
	r := rand1.New(rand1.NewSource(seed))
	return &RNG{r}
}

But, in order use ed25519.GenerateKey(newRNG(seed)) I need to make my type implement Read.

@lukechampine
Copy link
Contributor

I'm also in favor of this proposal over my own. I stand by my position regarding the security of ChaCha8, but ultimately what matters is exposing a Read method somewhere, and directly tying it to ChaCha8 ensures that callers will know what they're getting wrt security.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Development

No branches or pull requests

6 participants