golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
120.42k stars 17.29k forks source link

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

Closed FiloSottile closed 1 week ago

FiloSottile commented 1 month ago

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

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

zephyrtronium commented 1 month ago

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

neild commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

rsc commented 3 weeks ago

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

The proposal is to make the ChaCha8 type implement io.Reader. It always returns the requested number of bytes and never returns an error.

rsc commented 2 weeks ago

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

The proposal is to make the ChaCha8 type implement io.Reader. It always returns the requested number of bytes and never returns an error.

gopherbot commented 2 weeks ago

Change https://go.dev/cl/586316 mentions this issue: math/rand/v2: implement io.Reader for ChaCha8

gopherbot commented 1 week ago

Change https://go.dev/cl/586615 mentions this issue: math/rand/v2: add Read method to ChaCha8 source

gopherbot commented 1 week ago

Change https://go.dev/cl/587280 mentions this issue: math/rand/v2: add ChaCha8.Read

rolandshoemaker commented 1 week ago

Re-opening for proposal acceptance.

rolandshoemaker commented 1 week ago

Uh, this is not the right issue.