Skip to content

exclude crypto/rand.Read by default#269

Merged
dtcaciuc merged 1 commit intokisielk:masterfrom
pitku:master
Apr 6, 2025
Merged

exclude crypto/rand.Read by default#269
dtcaciuc merged 1 commit intokisielk:masterfrom
pitku:master

Conversation

@pitku
Copy link
Copy Markdown
Contributor

@pitku pitku commented Apr 3, 2025

Also mentioned in docs with examples. See - https://pkg.go.dev/crypto/rand#Read

@pitku pitku changed the title exclude crypto/rand.Read by default and bump minimum go version to 1.23.0 exclude crypto/rand.Read by default and bump minimum go version to 1.24.0 Apr 3, 2025
@kisielk
Copy link
Copy Markdown
Owner

kisielk commented Apr 3, 2025

What's the reason the minimum version needs to be bumped for this?

@pitku
Copy link
Copy Markdown
Contributor Author

pitku commented Apr 3, 2025

Opps! a bit of a rabbit hole with this one,

rand.Read is documented to not return errors in go 1.24 and later.
but in 1.23 or later it will not return errors but it is just not documented.
99522de1c38e4915e061cd2dac7d34ee888c8318
switched to arc4random (which is included in 1.23)

@pitku
Copy link
Copy Markdown
Contributor Author

pitku commented Apr 3, 2025

If user is using Go 1.22 there are still some codepaths in some exotic configurations which may return error for rand.Read. Ideally go mod should specify 1.24 as minimum, as only 1.24 and above guarantee that rand.Read does not return errors. But it could be left to 1.23 as well as there are no code paths which return error on rand.Read.

I would be happy to put this on hold if you/your downstream dependencies are not comfortable bumping the minimum go version.

@kisielk
Copy link
Copy Markdown
Owner

kisielk commented Apr 3, 2025 via email

@pitku
Copy link
Copy Markdown
Contributor Author

pitku commented Apr 4, 2025

I have removed go mod changes.

@pitku pitku changed the title exclude crypto/rand.Read by default and bump minimum go version to 1.24.0 exclude crypto/rand.Read by default Apr 4, 2025
@dtcaciuc dtcaciuc merged commit e3eb51b into kisielk:master Apr 6, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants