Use crypto/rand instead of math/rand for random string generation#145
Use crypto/rand instead of math/rand for random string generation#145kblissett wants to merge 1 commit intoSecureAuthCorp:masterfrom
Conversation
This adheres to security best practices by using cryptographically secure random values for OAuth2 state parameters and PKCE code verifiers.
kblissett
requested review from
ikawalec,
jdabrowski and
mbilski
as code owners
There was a problem hiding this comment.
Pull request overview
This PR migrates from math/rand to crypto/rand for generating random strings used in OAuth2 security-critical contexts (PKCE code verifiers and JWT assertion JTI claims), addressing a security best practice issue raised in #144.
Changes:
- Replaced
math/randwithcrypto/randfor cryptographically secure random generation - Simplified the implementation by removing the global random generator and init() function
- Changed the character set from
[]runeto[]bytefor direct byte manipulation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func RandomString(n int) string { | ||
| b := make([]rune, n) | ||
| b := make([]byte, n) | ||
| rand.Read(b) |
There was a problem hiding this comment.
The error return value from rand.Read() is being ignored. According to the crypto/rand documentation, rand.Read can return an error if the underlying system fails to provide sufficient randomness. This function should handle the error appropriately, either by returning an error to the caller or by panicking if random generation fails, since generating predictable values would be a security issue for OAuth2 state parameters and PKCE code verifiers.
| rand.Read(b) | |
| if _, err := rand.Read(b); err != nil { | |
| panic("oauth2: failed to generate secure random string: " + err.Error()) | |
| } |
| for i := range b { | ||
| b[i] = letter[r.Intn(len(letter))] | ||
| b[i] = letter[b[i]%byte(len(letter))] |
There was a problem hiding this comment.
The modulo operation creates a slight bias in character selection. Since 256 % 62 = 8, the first 8 characters of the alphabet have a slightly higher probability of being selected. While the practical security impact is minimal for the current use cases (43-character PKCE verifiers and 20-character JTI claims), consider using a rejection sampling approach: generate a random byte, and if it's >= 248 (the largest multiple of 62 that fits in a byte), discard it and generate a new one. This ensures uniform distribution across all characters.
|
I made the linter suggested fixes in #148 |
3 participants
This adheres to security best practices by using cryptographically secure random values for OAuth2 state parameters and PKCE code verifiers. Resolves #144