chore(sec): unify usage of crypto/rand.Read (go-gitea#7453)

Gusted · Earl Warren · commit 53df0bf9a4a1 · 2025-04-04T03:31:37.000Z
- Unify the usage of [`crypto/rand.Read`](https://pkg.go.dev/crypto/rand#Read) to `util.CryptoRandomBytes`. - Refactor `util.CryptoRandomBytes` to never return an error. It is documented by Go, https://go.dev/issue/66821, to always succeed. So if we still receive a error or if the returned bytes read is not equal to the expected bytes to be read we panic (just to be on the safe side). - This simplifies a lot of code to no longer care about error handling. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7453 Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org> Co-authored-by: Gusted <postmaster@gusted.xyz> Co-committed-by: Gusted <postmaster@gusted.xyz>
diff --git a/cmd/generate.go b/cmd/generate.go
@@ -70,10 +70,7 @@ func runGenerateInternalToken(c *cli.Context) error {
 }
 
 func runGenerateLfsJwtSecret(c *cli.Context) error {
-	_, jwtSecretBase64, err := generate.NewJwtSecret()
-	if err != nil {
-		return err
-	}
+	_, jwtSecretBase64 := generate.NewJwtSecret()
 
 	fmt.Printf("%s", jwtSecretBase64)
 
diff --git a/models/actions/runner.go b/models/actions/runner.go
@@ -168,12 +168,7 @@ func (r *ActionRunner) GenerateToken() (err error) {
 // UpdateSecret updates the hash based on the specified token. It does not
 // ensure that the runner's UUID matches the first 16 bytes of the token.
 func (r *ActionRunner) UpdateSecret(token string) error {
-	saltBytes, err := util.CryptoRandomBytes(16)
-	if err != nil {
-		return fmt.Errorf("CryptoRandomBytes %v", err)
-	}
-
-	salt := hex.EncodeToString(saltBytes)
+	salt := hex.EncodeToString(util.CryptoRandomBytes(16))
 
 	r.Token = token
 	r.TokenSalt = salt
diff --git a/models/actions/utils.go b/models/actions/utils.go
@@ -22,11 +22,7 @@ func generateSaltedToken() (string, string, string, string, error) {
 	if err != nil {
 		return "", "", "", "", err
 	}
-	buf, err := util.CryptoRandomBytes(20)
-	if err != nil {
-		return "", "", "", "", err
-	}
-	token := hex.EncodeToString(buf)
+	token := hex.EncodeToString(util.CryptoRandomBytes(20))
 	hash := auth_model.HashToken(token, salt)
 	return token, salt, hash, token[len(token)-8:], nil
 }
diff --git a/models/auth/access_token.go b/models/auth/access_token.go
@@ -111,12 +111,8 @@ func generateAccessToken(t *AccessToken) error {
 	if err != nil {
 		return err
 	}
-	token, err := util.CryptoRandomBytes(20)
-	if err != nil {
-		return err
-	}
 	t.TokenSalt = salt
-	t.Token = hex.EncodeToString(token)
+	t.Token = hex.EncodeToString(util.CryptoRandomBytes(20))
 	t.TokenHash = HashToken(t.Token, t.TokenSalt)
 	t.TokenLastEight = t.Token[len(t.Token)-8:]
 	return nil
diff --git a/models/auth/auth_token.go b/models/auth/auth_token.go
@@ -63,10 +63,7 @@ func (authToken *AuthorizationToken) IsExpired() bool {
 func GenerateAuthToken(ctx context.Context, userID int64, expiry timeutil.TimeStamp, purpose AuthorizationPurpose) (lookupKey, validator string, err error) {
 	// Request 64 random bytes. The first 32 bytes will be used for the lookupKey
 	// and the other 32 bytes will be used for the validator.
-	rBytes, err := util.CryptoRandomBytes(64)
-	if err != nil {
-		return "", "", err
-	}
+	rBytes := util.CryptoRandomBytes(64)
 	hexEncoded := hex.EncodeToString(rBytes)
 	validator, lookupKey = hexEncoded[64:], hexEncoded[:64]
 
diff --git a/models/auth/oauth2.go b/models/auth/oauth2.go
@@ -184,13 +184,9 @@ var base32Lower = base32.NewEncoding(lowerBase32Chars).WithPadding(base32.NoPadd
 
 // GenerateClientSecret will generate the client secret and returns the plaintext and saves the hash at the database
 func (app *OAuth2Application) GenerateClientSecret(ctx context.Context) (string, error) {
-	rBytes, err := util.CryptoRandomBytes(32)
-	if err != nil {
-		return "", err
-	}
 	// Add a prefix to the base32, this is in order to make it easier
 	// for code scanners to grab sensitive tokens.
-	clientSecret := "gto_" + base32Lower.EncodeToString(rBytes)
+	clientSecret := "gto_" + base32Lower.EncodeToString(util.CryptoRandomBytes(32))
 
 	hashedSecret, err := bcrypt.GenerateFromPassword([]byte(clientSecret), bcrypt.DefaultCost)
 	if err != nil {
@@ -475,13 +471,9 @@ func (grant *OAuth2Grant) TableName() string {
 
 // GenerateNewAuthorizationCode generates a new authorization code for a grant and saves it to the database
 func (grant *OAuth2Grant) GenerateNewAuthorizationCode(ctx context.Context, redirectURI, codeChallenge, codeChallengeMethod string) (code *OAuth2AuthorizationCode, err error) {
-	rBytes, err := util.CryptoRandomBytes(32)
-	if err != nil {
-		return &OAuth2AuthorizationCode{}, err
-	}
 	// Add a prefix to the base32, this is in order to make it easier
 	// for code scanners to grab sensitive tokens.
-	codeSecret := "gta_" + base32Lower.EncodeToString(rBytes)
+	codeSecret := "gta_" + base32Lower.EncodeToString(util.CryptoRandomBytes(32))
 
 	code = &OAuth2AuthorizationCode{
 		Grant:               grant,
diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go
@@ -61,17 +61,13 @@ func init() {
 }
 
 // GenerateScratchToken recreates the scratch token the user is using.
-func (t *TwoFactor) GenerateScratchToken() (string, error) {
-	tokenBytes, err := util.CryptoRandomBytes(6)
-	if err != nil {
-		return "", err
-	}
+func (t *TwoFactor) GenerateScratchToken() string {
 	// these chars are specially chosen, avoid ambiguous chars like `0`, `O`, `1`, `I`.
 	const base32Chars = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789"
-	token := base32.NewEncoding(base32Chars).WithPadding(base32.NoPadding).EncodeToString(tokenBytes)
+	token := base32.NewEncoding(base32Chars).WithPadding(base32.NoPadding).EncodeToString(util.CryptoRandomBytes(6))
 	t.ScratchSalt, _ = util.CryptoRandomString(10)
 	t.ScratchHash = HashToken(token, t.ScratchSalt)
-	return token, nil
+	return token
 }
 
 // HashToken return the hashable salt
diff --git a/models/organization/org.go b/models/organization/org.go
@@ -289,12 +289,8 @@ func CreateOrganization(ctx context.Context, org *Organization, owner *user_mode
 	}
 
 	org.LowerName = strings.ToLower(org.Name)
-	if org.Rands, err = user_model.GetUserSalt(); err != nil {
-		return err
-	}
-	if org.Salt, err = user_model.GetUserSalt(); err != nil {
-		return err
-	}
+	org.Rands = user_model.GetUserSalt()
+	org.Salt = user_model.GetUserSalt()
 	org.UseCustomAvatar = true
 	org.MaxRepoCreation = -1
 	org.NumTeams = 1
diff --git a/models/user/email_address.go b/models/user/email_address.go
@@ -266,9 +266,7 @@ func updateActivation(ctx context.Context, email *EmailAddress, activate bool) e
 	if err != nil {
 		return err
 	}
-	if user.Rands, err = GetUserSalt(); err != nil {
-		return err
-	}
+	user.Rands = GetUserSalt()
 	email.IsActivated = activate
 	if _, err := db.GetEngine(ctx).ID(email.ID).Cols("is_activated").Update(email); err != nil {
 		return err
@@ -403,9 +401,7 @@ func ActivateUserEmail(ctx context.Context, userID int64, email string, activate
 		// The user's activation state should be synchronized with the primary email
 		if user.IsActive != activate {
 			user.IsActive = activate
-			if user.Rands, err = GetUserSalt(); err != nil {
-				return fmt.Errorf("unable to generate salt: %w", err)
-			}
+			user.Rands = GetUserSalt()
 			if err = UpdateUserCols(ctx, user, "is_active", "rands"); err != nil {
 				return fmt.Errorf("unable to updateUserCols() for user ID: %d: %w", userID, err)
 			}
diff --git a/models/user/user.go b/models/user/user.go
@@ -387,9 +387,7 @@ func (u *User) SetPassword(passwd string) (err error) {
 		return err
 	}
 
-	if u.Salt, err = GetUserSalt(); err != nil {
-		return err
-	}
+	u.Salt = GetUserSalt()
 	if u.Passwd, err = hash.Parse(setting.PasswordHashAlgo).Hash(passwd, u.Salt); err != nil {
 		return err
 	}
@@ -559,13 +557,9 @@ func IsUserExist(ctx context.Context, uid int64, name string) (bool, error) {
 const SaltByteLength = 16
 
 // GetUserSalt returns a random user salt token.
-func GetUserSalt() (string, error) {
-	rBytes, err := util.CryptoRandomBytes(SaltByteLength)
-	if err != nil {
-		return "", err
-	}
+func GetUserSalt() string {
 	// Returns a 32 bytes long string.
-	return hex.EncodeToString(rBytes), nil
+	return hex.EncodeToString(util.CryptoRandomBytes(SaltByteLength))
 }
 
 // Note: The set of characters here can safely expand without a breaking change,
@@ -772,9 +766,7 @@ func createUser(ctx context.Context, u *User, createdByAdmin bool, overwriteDefa
 
 	u.LowerName = strings.ToLower(u.Name)
 	u.AvatarEmail = u.Email
-	if u.Rands, err = GetUserSalt(); err != nil {
-		return err
-	}
+	u.Rands = GetUserSalt()
 	if u.Passwd != "" {
 		if err = u.SetPassword(u.Passwd); err != nil {
 			return err
diff --git a/modules/generate/generate.go b/modules/generate/generate.go
@@ -5,10 +5,8 @@
 package generate
 
 import (
-	"crypto/rand"
 	"encoding/base64"
 	"fmt"
-	"io"
 	"time"
 
 	"forgejo.org/modules/util"
@@ -18,18 +16,11 @@ import (
 
 // NewInternalToken generate a new value intended to be used by INTERNAL_TOKEN.
 func NewInternalToken() (string, error) {
-	secretBytes := make([]byte, 32)
-	_, err := io.ReadFull(rand.Reader, secretBytes)
-	if err != nil {
-		return "", err
-	}
-
-	secretKey := base64.RawURLEncoding.EncodeToString(secretBytes)
+	secretKey := base64.RawURLEncoding.EncodeToString(util.CryptoRandomBytes(32))
 
 	now := time.Now()
 
-	var internalToken string
-	internalToken, err = jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
+	internalToken, err := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
 		"nbf": now.Unix(),
 	}).SignedString([]byte(secretKey))
 	if err != nil {
@@ -54,14 +45,9 @@ func DecodeJwtSecret(src string) ([]byte, error) {
 }
 
 // NewJwtSecret generates a new base64 encoded value intended to be used for JWT secrets.
-func NewJwtSecret() ([]byte, string, error) {
-	bytes := make([]byte, 32)
-	_, err := rand.Read(bytes)
-	if err != nil {
-		return nil, "", err
-	}
-
-	return bytes, base64.RawURLEncoding.EncodeToString(bytes), nil
+func NewJwtSecret() ([]byte, string) {
+	bytes := util.CryptoRandomBytes(32)
+	return bytes, base64.RawURLEncoding.EncodeToString(bytes)
 }
 
 // NewSecretKey generate a new value intended to be used by SECRET_KEY.
diff --git a/modules/generate/generate_test.go b/modules/generate/generate_test.go
@@ -26,8 +26,7 @@ func TestDecodeJwtSecret(t *testing.T) {
 }
 
 func TestNewJwtSecret(t *testing.T) {
-	secret, encoded, err := NewJwtSecret()
-	require.NoError(t, err)
+	secret, encoded := NewJwtSecret()
 	assert.Len(t, secret, 32)
 	decoded, err := DecodeJwtSecret(encoded)
 	require.NoError(t, err)
diff --git a/modules/keying/keying.go b/modules/keying/keying.go
@@ -17,10 +17,11 @@ package keying
 
 import (
 	"crypto/hkdf"
-	"crypto/rand"
 	"crypto/sha256"
 	"encoding/binary"
 
+	"forgejo.org/modules/util"
+
 	"golang.org/x/crypto/chacha20poly1305"
 )
 
@@ -95,10 +96,7 @@ func (k *Key) Encrypt(plaintext, additionalData []byte) []byte {
 	}
 
 	// Generate a random nonce.
-	nonce := make([]byte, aeadNonceSize)
-	if n, err := rand.Read(nonce); err != nil || n != aeadNonceSize {
-		panic(err)
-	}
+	nonce := util.CryptoRandomBytes(aeadNonceSize)
 
 	// Returns the ciphertext of this plaintext.
 	return e.Seal(nonce, nonce, plaintext, additionalData)
diff --git a/modules/setting/lfs.go b/modules/setting/lfs.go
@@ -80,10 +80,7 @@ func loadLFSFrom(rootCfg ConfigProvider) error {
 	jwtSecretBase64 := loadSecret(rootCfg.Section("server"), "LFS_JWT_SECRET_URI", "LFS_JWT_SECRET")
 	LFS.JWTSecretBytes, err = generate.DecodeJwtSecret(jwtSecretBase64)
 	if err != nil {
-		LFS.JWTSecretBytes, jwtSecretBase64, err = generate.NewJwtSecret()
-		if err != nil {
-			return fmt.Errorf("error generating JWT Secret for custom config: %v", err)
-		}
+		LFS.JWTSecretBytes, jwtSecretBase64 = generate.NewJwtSecret()
 
 		// Save secret
 		saveCfg, err := rootCfg.PrepareSaving()
diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go
@@ -138,10 +138,7 @@ func loadOAuth2From(rootCfg ConfigProvider) {
 	if InstallLock {
 		jwtSecretBytes, err := generate.DecodeJwtSecret(jwtSecretBase64)
 		if err != nil {
-			jwtSecretBytes, jwtSecretBase64, err = generate.NewJwtSecret()
-			if err != nil {
-				log.Fatal("error generating JWT secret: %v", err)
-			}
+			jwtSecretBytes, jwtSecretBase64 = generate.NewJwtSecret()
 			saveCfg, err := rootCfg.PrepareSaving()
 			if err != nil {
 				log.Fatal("save oauth2.JWT_SECRET failed: %v", err)
@@ -161,10 +158,7 @@ var generalSigningSecret atomic.Pointer[[]byte]
 func GetGeneralTokenSigningSecret() []byte {
 	old := generalSigningSecret.Load()
 	if old == nil || len(*old) == 0 {
-		jwtSecret, _, err := generate.NewJwtSecret()
-		if err != nil {
-			log.Fatal("Unable to generate general JWT secret: %v", err)
-		}
+		jwtSecret, _ := generate.NewJwtSecret()
 		if generalSigningSecret.CompareAndSwap(old, &jwtSecret) {
 			return jwtSecret
 		}
diff --git a/modules/util/util.go b/modules/util/util.go
@@ -88,10 +88,16 @@ func CryptoRandomString(length int64) (string, error) {
 // CryptoRandomBytes generates `length` crypto bytes
 // This differs from CryptoRandomString, as each byte in CryptoRandomString is generated by [0,61] range
 // This function generates totally random bytes, each byte is generated by [0,255] range
-func CryptoRandomBytes(length int64) ([]byte, error) {
+func CryptoRandomBytes(length int64) []byte {
+	// crypto/rand.Read is documented to never return a error.
+	// https://go.dev/issue/66821
 	buf := make([]byte, length)
-	_, err := rand.Read(buf)
-	return buf, err
+	n, err := rand.Read(buf)
+	if err != nil || n != int(length) {
+		panic(err)
+	}
+
+	return buf
 }
 
 // ToUpperASCII returns s with all ASCII letters mapped to their upper case.
diff --git a/modules/util/util_test.go b/modules/util/util_test.go
@@ -163,20 +163,18 @@ func Test_RandomString(t *testing.T) {
 }
 
 func Test_RandomBytes(t *testing.T) {
-	bytes1, err := util.CryptoRandomBytes(32)
-	require.NoError(t, err)
-
-	bytes2, err := util.CryptoRandomBytes(32)
-	require.NoError(t, err)
+	bytes1 := util.CryptoRandomBytes(32)
+	bytes2 := util.CryptoRandomBytes(32)
 
+	assert.Len(t, bytes1, 32)
+	assert.Len(t, bytes2, 32)
 	assert.NotEqual(t, bytes1, bytes2)
 
-	bytes3, err := util.CryptoRandomBytes(256)
-	require.NoError(t, err)
-
-	bytes4, err := util.CryptoRandomBytes(256)
-	require.NoError(t, err)
+	bytes3 := util.CryptoRandomBytes(256)
+	bytes4 := util.CryptoRandomBytes(256)
 
+	assert.Len(t, bytes3, 256)
+	assert.Len(t, bytes4, 256)
 	assert.NotEqual(t, bytes3, bytes4)
 }
 
diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json
@@ -20,6 +20,5 @@
     "error.not_found.title": "Page not found",
     "alert.asset_load_failed": "Failed to load asset files from {path}. Please make sure the asset files can be accessed.",
     "alert.range_error": " must be a number between %[1]s and %[2]s.",
-    "install.invalid_lfs_path": "Unable to create the LFS root at the specified path: %[1]s",
-    "install.lfs_jwt_secret_failed": "Unable to generate a LFS JWT secret: %[1]s"
-}
+    "install.invalid_lfs_path": "Unable to create the LFS root at the specified path: %[1]s"
+}
diff --git a/routers/install/install.go b/routers/install/install.go
@@ -408,11 +408,7 @@ func SubmitInstall(ctx *context.Context) {
 	if form.LFSRootPath != "" {
 		cfg.Section("server").Key("LFS_START_SERVER").SetValue("true")
 		cfg.Section("lfs").Key("PATH").SetValue(form.LFSRootPath)
-		var lfsJwtSecret string
-		if _, lfsJwtSecret, err = generate.NewJwtSecret(); err != nil {
-			ctx.RenderWithErr(ctx.Tr("install.lfs_jwt_secret_failed", err), tplInstall, &form)
-			return
-		}
+		_, lfsJwtSecret := generate.NewJwtSecret()
 		cfg.Section("server").Key("LFS_JWT_SECRET").SetValue(lfsJwtSecret)
 	} else {
 		cfg.Section("server").Key("LFS_START_SERVER").SetValue("false")
@@ -483,11 +479,7 @@ func SubmitInstall(ctx *context.Context) {
 	// FIXME: at the moment, no matter oauth2 is enabled or not, it must generate a "oauth2 JWT_SECRET"
 	// see the "loadOAuth2From" in "setting/oauth2.go"
 	if !cfg.Section("oauth2").HasKey("JWT_SECRET") && !cfg.Section("oauth2").HasKey("JWT_SECRET_URI") {
-		_, jwtSecretBase64, err := generate.NewJwtSecret()
-		if err != nil {
-			ctx.RenderWithErr(ctx.Tr("install.secret_key_failed", err), tplInstall, &form)
-			return
-		}
+		_, jwtSecretBase64 := generate.NewJwtSecret()
 		cfg.Section("oauth2").Key("JWT_SECRET").SetValue(jwtSecretBase64)
 	}
 
diff --git a/routers/web/auth/2fa.go b/routers/web/auth/2fa.go
diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go
diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go
diff --git a/routers/web/user/setting/security/2fa.go b/routers/web/user/setting/security/2fa.go
diff --git a/tests/e2e/utils_e2e_test.go b/tests/e2e/utils_e2e_test.go
diff --git a/tests/integration/api_packages_test.go b/tests/integration/api_packages_test.go

Original file line number	Diff line number	Diff line change
`@@ -70,10 +70,7 @@ func runGenerateInternalToken(c *cli.Context) error {`
`70`	`70`	`}`
`71`	`71`
`72`	`72`	`func runGenerateLfsJwtSecret(c *cli.Context) error {`
`73`		`- _, jwtSecretBase64, err := generate.NewJwtSecret()`
`74`		`- if err != nil {`
`75`		`- return err`
`76`		`- }`
	`73`	`+ _, jwtSecretBase64 := generate.NewJwtSecret()`
`77`	`74`
`78`	`75`	`fmt.Printf("%s", jwtSecretBase64)`
`79`	`76`
Original file line number	Diff line number	Diff line change
`@@ -22,11 +22,7 @@ func generateSaltedToken() (string, string, string, string, error) {`
`22`	`22`	`if err != nil {`
`23`	`23`	`return "", "", "", "", err`
`24`	`24`	`}`
`25`		`- buf, err := util.CryptoRandomBytes(20)`
`26`		`- if err != nil {`
`27`		`- return "", "", "", "", err`
`28`		`- }`
`29`		`- token := hex.EncodeToString(buf)`
	`25`	`+ token := hex.EncodeToString(util.CryptoRandomBytes(20))`
`30`	`26`	`hash := auth_model.HashToken(token, salt)`
`31`	`27`	`return token, salt, hash, token[len(token)-8:], nil`
`32`	`28`	`}`