chore: unify the usage of CryptoRandomString (#10110)

- Similair spirit of forgejo/forgejo!7453.
- Refactor the code in such a way that it always succeeds.
- To avoid doing mathematics if you use this function, define three security level (64, 128 and 256 bits) that correspond to a specific length which has that a security guarantee. I picked them as they fit the need for the existing usages of the code.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10110
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Reviewed-by: Lucas <sclu1034@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

cmd/forgejo/actions.go

@@ -212,9 +212,7 @@ func RunRegister(ctx context.Context, cli *cli.Command) error {
 
 func RunGenerateSecret(ctx context.Context, cli *cli.Command) error {
 	runner := actions_model.ActionRunner{}
-	if err := runner.GenerateToken(); err != nil {
-		return err
-	}
+	runner.GenerateToken()
 	if _, err := fmt.Fprintf(ContextGetStdout(ctx), "%s", runner.Token); err != nil {
 		panic(err)
 	}

cmd/generate.go

@@ -94,10 +94,7 @@ func runGenerateLfsJwtSecret(ctx context.Context, c *cli.Command) error {
 }
 
 func runGenerateSecretKey(ctx context.Context, c *cli.Command) error {
-	secretKey, err := generate.NewSecretKey()
-	if err != nil {
-		return err
-	}
+	secretKey := generate.NewSecretKey()
 
 	fmt.Printf("%s", secretKey)
 

models/actions/runner.go

@@ -161,9 +161,8 @@ func (r *ActionRunner) LoadAttributes(ctx context.Context) error {
 	return nil
 }
 
-func (r *ActionRunner) GenerateToken() (err error) {
-	r.Token, r.TokenSalt, r.TokenHash, _, err = generateSaltedToken()
-	return err
+func (r *ActionRunner) GenerateToken() {
+	r.Token, r.TokenSalt, r.TokenHash, _ = generateSaltedToken()
 }
 
 // UpdateSecret updates the hash based on the specified token. It does not

models/actions/runner_token.go

@@ -77,10 +77,7 @@ func NewRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerTo
 		ownerID = 0
 	}
 
-	token, err := util.CryptoRandomString(40)
-	if err != nil {
-		return nil, err
-	}
+	token := util.CryptoRandomString(util.RandomStringHigh)
 	runnerToken := &ActionRunnerToken{
 		OwnerID:  ownerID,
 		RepoID:   repoID,
@@ -95,7 +92,7 @@ func NewRunnerToken(ctx context.Context, ownerID, repoID int64) (*ActionRunnerTo
 			return err
 		}
 
-		_, err = db.GetEngine(ctx).Insert(runnerToken)
+		_, err := db.GetEngine(ctx).Insert(runnerToken)
 		return err
 	})
 }

models/actions/task.go

@@ -143,9 +143,8 @@ func (task *ActionTask) LoadAttributes(ctx context.Context) error {
 	return nil
 }
 
-func (task *ActionTask) GenerateToken() (err error) {
-	task.Token, task.TokenSalt, task.TokenHash, task.TokenLastEight, err = generateSaltedToken()
-	return err
+func (task *ActionTask) GenerateToken() {
+	task.Token, task.TokenSalt, task.TokenHash, task.TokenLastEight = generateSaltedToken()
 }
 
 // Retrieve all the attempts from the same job as the target `ActionTask`.  Limited fields are queried to avoid loading
@@ -367,9 +366,7 @@ func CreateTaskForRunner(ctx context.Context, runner *ActionRunner) (*ActionTask
 		CommitSHA:         job.CommitSHA,
 		IsForkPullRequest: job.IsForkPullRequest,
 	}
-	if err := task.GenerateToken(); err != nil {
-		return nil, false, err
-	}
+	task.GenerateToken()
 
 	var workflowJob *jobparser.Job
 	if gots, err := jobparser.Parse(job.WorkflowPayload, false); err != nil {

models/actions/utils.go

@@ -17,14 +17,11 @@ import (
 	"forgejo.org/modules/util"
 )
 
-func generateSaltedToken() (string, string, string, string, error) {
-	salt, err := util.CryptoRandomString(10)
-	if err != nil {
-		return "", "", "", "", err
-	}
+func generateSaltedToken() (string, string, string, string) {
+	salt := util.CryptoRandomString(util.RandomStringMedium)
 	token := hex.EncodeToString(util.CryptoRandomBytes(20))
 	hash := auth_model.HashToken(token, salt)
-	return token, salt, hash, token[len(token)-8:], nil
+	return token, salt, hash, token[len(token)-8:]
 }
 
 /*

models/auth/access_token.go

@@ -98,24 +98,17 @@ func init() {
 
 // NewAccessToken creates new access token.
 func NewAccessToken(ctx context.Context, t *AccessToken) error {
-	err := generateAccessToken(t)
-	if err != nil {
-		return err
-	}
-	_, err = db.GetEngine(ctx).Insert(t)
+	generateAccessToken(t)
+	_, err := db.GetEngine(ctx).Insert(t)
 	return err
 }
 
-func generateAccessToken(t *AccessToken) error {
-	salt, err := util.CryptoRandomString(10)
-	if err != nil {
-		return err
-	}
+func generateAccessToken(t *AccessToken) {
+	salt := util.CryptoRandomString(util.RandomStringMedium)
 	t.TokenSalt = salt
 	t.Token = hex.EncodeToString(util.CryptoRandomBytes(20))
 	t.TokenHash = HashToken(t.Token, t.TokenSalt)
 	t.TokenLastEight = t.Token[len(t.Token)-8:]
-	return nil
 }
 
 // DisplayPublicOnly whether to display this as a public-only token.
@@ -251,10 +244,7 @@ func RegenerateAccessTokenByID(ctx context.Context, id, userID int64) (*AccessTo
 		return nil, ErrAccessTokenNotExist{}
 	}
 
-	err = generateAccessToken(t)
-	if err != nil {
-		return nil, err
-	}
+	generateAccessToken(t)
 
 	// Reset the creation time, token is unused
 	t.UpdatedUnix = timeutil.TimeStampNow()

models/auth/twofactor.go

@@ -65,7 +65,7 @@ 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(util.CryptoRandomBytes(6))
-	t.ScratchSalt, _ = util.CryptoRandomString(10)
+	t.ScratchSalt = util.CryptoRandomString(util.RandomStringMedium)
 	t.ScratchHash = HashToken(token, t.ScratchSalt)
 	return token
 }

models/gitea_migrations/v1_6/v71.go

@@ -51,10 +51,7 @@ func AddScratchHash(x *xorm.Engine) error {
 
 		for _, tfa := range tfas {
 			// generate salt
-			salt, err := util.CryptoRandomString(10)
-			if err != nil {
-				return err
-			}
+			salt := util.CryptoRandomString(util.RandomStringMedium)
 			tfa.ScratchSalt = salt
 			tfa.ScratchHash = base.HashToken(tfa.ScratchToken, salt)
 

models/gitea_migrations/v1_9/v85.go

@@ -65,10 +65,7 @@ func HashAppToken(x *xorm.Engine) error {
 
 		for _, token := range tokens {
 			// generate salt
-			salt, err := util.CryptoRandomString(10)
-			if err != nil {
-				return err
-			}
+			salt := util.CryptoRandomString(util.RandomStringMedium)
 			token.TokenSalt = salt
 			token.TokenHash = base.HashToken(token.Sha1, salt)
 			if len(token.Sha1) < 8 {

models/organization/team_invite.go

@@ -116,10 +116,7 @@ func CreateTeamInvite(ctx context.Context, doer *user_model.User, team *Team, em
 		}
 	}
 
-	token, err := util.CryptoRandomString(25)
-	if err != nil {
-		return nil, err
-	}
+	token := util.CryptoRandomString(util.RandomStringMedium)
 
 	invite := &TeamInvite{
 		Token:     token,

models/packages/package_blob_upload.go

@@ -31,16 +31,11 @@ type PackageBlobUpload struct {
 
 // CreateBlobUpload inserts a blob upload
 func CreateBlobUpload(ctx context.Context) (*PackageBlobUpload, error) {
-	id, err := util.CryptoRandomString(25)
-	if err != nil {
-		return nil, err
-	}
-
 	pbu := &PackageBlobUpload{
-		ID: strings.ToLower(id),
+		ID: strings.ToLower(util.CryptoRandomString(util.RandomStringMedium)),
 	}
 
-	_, err = db.GetEngine(ctx).Insert(pbu)
+	_, err := db.GetEngine(ctx).Insert(pbu)
 	return pbu, err
 }
 

modules/generate/generate.go

@@ -51,11 +51,6 @@ func NewJwtSecret() ([]byte, string) {
 }
 
 // NewSecretKey generate a new value intended to be used by SECRET_KEY.
-func NewSecretKey() (string, error) {
-	secretKey, err := util.CryptoRandomString(64)
-	if err != nil {
-		return "", err
-	}
-
-	return secretKey, nil
+func NewSecretKey() string {
+	return util.CryptoRandomString(util.RandomStringHigh)
 }

modules/git/fetch.go

@@ -32,11 +32,7 @@ func (repo *Repository) Fetch(remoteURL, refspec string) (string, error) {
 	if SupportFetchPorcelain {
 		cmd.AddArguments("--porcelain")
 	} else if !strings.Contains(refspec, ":") {
-		randomString, err := util.CryptoRandomString(8)
-		if err != nil {
-			return "", err
-		}
-		refspec += ":refs/tmp/" + randomString
+		refspec += ":refs/tmp/" + util.CryptoRandomString(util.RandomStringLow)
 	}
 
 	cmd.AddArguments("--end-of-options").AddDynamicArguments(remoteURL, refspec)

modules/util/util.go

@@ -10,7 +10,6 @@ import (
 	"crypto/rand"
 	"encoding/pem"
 	"fmt"
-	"math/big"
 	"strconv"
 	"strings"
 
@@ -60,34 +59,43 @@ func NormalizeEOL(input []byte) []byte {
 	return tmp[:pos]
 }
 
-// CryptoRandomInt returns a crypto random integer between 0 and limit, inclusive
-func CryptoRandomInt(limit int64) (int64, error) {
-	rInt, err := rand.Int(rand.Reader, big.NewInt(limit))
-	if err != nil {
-		return 0, err
-	}
-	return rInt.Int64(), nil
-}
+type RandomStringSecurityLevel int64
 
-const alphanumericalChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
+const (
+	base64alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_"
 
-// CryptoRandomString generates a crypto random alphanumerical string, each byte is generated by [0,61] range
-func CryptoRandomString(length int64) (string, error) {
-	buf := make([]byte, length)
-	limit := int64(len(alphanumericalChars))
+	// If you merely need a string that has a low chance of collisions and do not
+	// need any security guarantees.
+	//
+	// Calculation: ⌈log₆₄(2⁶⁴)⌉
+	RandomStringLow RandomStringSecurityLevel = 11
+	// Can be used for transactions. Can be used for salts.
+	//
+	// Calculation: ⌈log₆₄(2¹²⁸)⌉
+	RandomStringMedium RandomStringSecurityLevel = 22
+	// You can use this for long-term storage that will have very low chances of
+	// collisions and withstand online brute-force attempts for a long time.
+	//
+	// Calculation: ⌈log₆₄(2²⁵⁶)⌉
+	RandomStringHigh RandomStringSecurityLevel = 43
+)
+
+// CryptoRandomString generates a string that is of `securityLevel`. Do not
+// expect a fixed length, it might change in the future. In doubt add a unit
+// test to catch this when it happens. The result is URL safe and contains
+// characters from the base64 alphabet.
+func CryptoRandomString(securityLevel RandomStringSecurityLevel) string {
+	buf := CryptoRandomBytes(int64(securityLevel))
 	for i := range buf {
-		num, err := CryptoRandomInt(limit)
-		if err != nil {
-			return "", err
-		}
-		buf[i] = alphanumericalChars[num]
+		buf[i] = base64alphabet[buf[i]%64]
 	}
-	return string(buf), nil
+	return string(buf)
 }
 
-// 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
+// CryptoRandomBytes generates `length` crypto bytes.
+// This function generates totally random bytes, each byte is generated by
+// the [0,255] range. If you need a human readable (ASCII safe) string, then
+// use `CryptoRandomString`.
 func CryptoRandomBytes(length int64) []byte {
 	// crypto/rand.Read is documented to never return a error.
 	// https://go.dev/issue/66821

modules/util/util_test.go

@@ -7,7 +7,6 @@ package util_test
 import (
 	"bytes"
 	"crypto/rand"
-	"regexp"
 	"strings"
 	"testing"
 
@@ -125,41 +124,36 @@ func Test_NormalizeEOL(t *testing.T) {
 	assert.Equal(t, []byte("mix\nand\nmatch\n."), util.NormalizeEOL([]byte("mix\r\nand\rmatch\n.")))
 }
 
-func Test_RandomInt(t *testing.T) {
-	randInt, err := util.CryptoRandomInt(255)
-	assert.GreaterOrEqual(t, randInt, int64(0))
-	assert.LessOrEqual(t, randInt, int64(255))
-	require.NoError(t, err)
-}
-
 func Test_RandomString(t *testing.T) {
-	str1, err := util.CryptoRandomString(32)
-	require.NoError(t, err)
-	matches, err := regexp.MatchString(`^[a-zA-Z0-9]{32}$`, str1)
-	require.NoError(t, err)
-	assert.True(t, matches)
+	t.Run("Low", func(t *testing.T) {
+		str1 := util.CryptoRandomString(util.RandomStringLow)
+		assert.Regexp(t, "^[a-zA-Z0-9_-]{11}$", str1)
 
-	str2, err := util.CryptoRandomString(32)
-	require.NoError(t, err)
-	matches, err = regexp.MatchString(`^[a-zA-Z0-9]{32}$`, str1)
-	require.NoError(t, err)
-	assert.True(t, matches)
+		str2 := util.CryptoRandomString(util.RandomStringLow)
+		assert.Regexp(t, "^[a-zA-Z0-9_-]{11}$", str2)
 
-	assert.NotEqual(t, str1, str2)
+		assert.NotEqual(t, str1, str2)
+	})
 
-	str3, err := util.CryptoRandomString(256)
-	require.NoError(t, err)
-	matches, err = regexp.MatchString(`^[a-zA-Z0-9]{256}$`, str3)
-	require.NoError(t, err)
-	assert.True(t, matches)
+	t.Run("Medium", func(t *testing.T) {
+		str1 := util.CryptoRandomString(util.RandomStringMedium)
+		assert.Regexp(t, "^[a-zA-Z0-9_-]{22}$", str1)
 
-	str4, err := util.CryptoRandomString(256)
-	require.NoError(t, err)
-	matches, err = regexp.MatchString(`^[a-zA-Z0-9]{256}$`, str4)
-	require.NoError(t, err)
-	assert.True(t, matches)
+		str2 := util.CryptoRandomString(util.RandomStringMedium)
+		assert.Regexp(t, "^[a-zA-Z0-9_-]{22}$", str2)
 
-	assert.NotEqual(t, str3, str4)
+		assert.NotEqual(t, str1, str2)
+	})
+
+	t.Run("High", func(t *testing.T) {
+		str1 := util.CryptoRandomString(util.RandomStringHigh)
+		assert.Regexp(t, "^[a-zA-Z0-9_-]{43}$", str1)
+
+		str2 := util.CryptoRandomString(util.RandomStringHigh)
+		assert.Regexp(t, "^[a-zA-Z0-9_-]{43}$", str2)
+
+		assert.NotEqual(t, str1, str2)
+	})
 }
 
 func Test_RandomBytes(t *testing.T) {

options/locale/locale_en-US.ini

@@ -354,7 +354,6 @@ invalid_repo_path = The repository root path is invalid: %v
 invalid_app_data_path = The app data path is invalid: %v
 run_user_not_match = The "user to run as" username is not the current username: %s -> %s
 internal_token_failed = Failed to generate internal token: %v
-secret_key_failed = Failed to generate secret key: %v
 save_config_failed = Failed to save configuration: %v
 enable_update_checker_helper_forgejo = It will periodically check for new Forgejo versions by checking a TXT DNS record at release.forgejo.org.
 invalid_admin_setting = Administrator account setting is invalid: %v

routers/api/actions/runner/runner.go

@@ -80,9 +80,7 @@ func (s *Service) Register(
 		Version:     req.Msg.Version,
 		AgentLabels: labels,
 	}
-	if err := runner.GenerateToken(); err != nil {
-		return nil, connect.NewError(connect.CodeInternal, errors.New("can't generate token"))
-	}
+	runner.GenerateToken()
 
 	// create new runner
 	if err := actions_model.CreateRunner(ctx, runner); err != nil {

routers/api/v1/repo/mirror.go

@@ -370,11 +370,7 @@ func CreatePushMirror(ctx *context.APIContext, mirrorOption *api.CreatePushMirro
 		return
 	}
 
-	remoteSuffix, err := util.CryptoRandomString(10)
-	if err != nil {
-		ctx.ServerError("CryptoRandomString", err)
-		return
-	}
+	remoteSuffix := util.CryptoRandomString(util.RandomStringLow)
 
 	remoteAddress, err := util.SanitizeURL(address)
 	if err != nil {

routers/install/install.go

@@ -485,11 +485,7 @@ func SubmitInstall(ctx *context.Context) {
 
 	// if there is already a SECRET_KEY, we should not overwrite it, otherwise the encrypted data will not be able to be decrypted
 	if setting.SecretKey == "" {
-		var secretKey string
-		if secretKey, err = generate.NewSecretKey(); err != nil {
-			ctx.RenderWithErr(ctx.Tr("install.secret_key_failed", err), tplInstall, &form)
-			return
-		}
+		secretKey := generate.NewSecretKey()
 		cfg.Section("security").Key("SECRET_KEY").SetValue(secretKey)
 	}
 

routers/web/auth/oauth.go

@@ -1364,10 +1364,7 @@ func generateCodeChallenge(ctx *context.Context, provider string) (codeChallenge
 		// a code_challenge can be generated
 	}
 
-	codeVerifier, err := util.CryptoRandomString(43) // 256/log2(62) = 256 bits of entropy (each char having log2(62) of randomness)
-	if err != nil {
-		return "", err
-	}
+	codeVerifier := util.CryptoRandomString(util.RandomStringHigh)
 	if err = ctx.Session.Set("CodeVerifier", codeVerifier); err != nil {
 		return "", err
 	}

routers/web/auth/openid.go

@@ -12,10 +12,10 @@ import (
 	auth_model "forgejo.org/models/auth"
 	user_model "forgejo.org/models/user"
 	"forgejo.org/modules/auth/openid"
+	"forgejo.org/modules/auth/password"
 	"forgejo.org/modules/base"
 	"forgejo.org/modules/log"
 	"forgejo.org/modules/setting"
-	"forgejo.org/modules/util"
 	"forgejo.org/modules/web"
 	"forgejo.org/services/auth"
 	"forgejo.org/services/context"
@@ -387,11 +387,7 @@ func RegisterOpenIDPost(ctx *context.Context) {
 		context.VerifyCaptcha(ctx, tplSignUpOID, form)
 	}
 
-	length := setting.MinPasswordLength
-	if length < 256 {
-		length = 256
-	}
-	password, err := util.CryptoRandomString(int64(length))
+	password, err := password.Generate(max(256, setting.MinPasswordLength))
 	if err != nil {
 		ctx.RenderWithErr(err.Error(), tplSignUpOID, form)
 		return
@@ -409,7 +405,7 @@ func RegisterOpenIDPost(ctx *context.Context) {
 
 	// add OpenID for the user
 	userOID := &user_model.UserOpenID{UID: u.ID, URI: oid}
-	if err = user_model.AddUserOpenID(ctx, userOID); err != nil {
+	if err := user_model.AddUserOpenID(ctx, userOID); err != nil {
 		if user_model.IsErrOpenIDAlreadyUsed(err) {
 			ctx.RenderWithErr(ctx.Tr("form.openid_been_used", oid), tplSignUpOID, &form)
 			return

routers/web/repo/setting/setting.go

@@ -688,11 +688,7 @@ func SettingsPost(ctx *context.Context) {
 			return
 		}
 
-		remoteSuffix, err := util.CryptoRandomString(10)
-		if err != nil {
-			ctx.ServerError("RandomString", err)
-			return
-		}
+		remoteSuffix := util.CryptoRandomString(util.RandomStringLow)
 
 		remoteAddress, err := util.SanitizeURL(address)
 		if err != nil {

tests/integration/api_issue_label_test.go

@@ -161,7 +161,7 @@ func TestAPIRemoveIssueLabel(t *testing.T) {
 	task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47})
 	task.RepoID = repo.ID
 	task.OwnerID = repo.OwnerID
-	require.NoError(t, task.GenerateToken())
+	task.GenerateToken()
 	actions_model.UpdateTask(t.Context(), task)
 
 	deleteURL := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/labels/%d", owner.Name, repo.Name, issue.Index, issueLabel.LabelID)
@@ -190,7 +190,7 @@ func TestAPIRemoveIssueLabelByName(t *testing.T) {
 	task := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: 47})
 	task.RepoID = repo.ID
 	task.OwnerID = repo.OwnerID
-	require.NoError(t, task.GenerateToken())
+	task.GenerateToken()
 	actions_model.UpdateTask(t.Context(), task)
 
 	deleteURL := fmt.Sprintf("/api/v1/repos/%s/%s/issues/%d/labels/%s", owner.Name, repo.Name, issue.Index, repoLabel.Name)
@@ -384,7 +384,7 @@ func TestAPIReplaceIssueLabelsActionsToken(t *testing.T) {
 	task.RepoID = repo.ID
 	task.OwnerID = owner.ID
 	task.IsForkPullRequest = true // Read permission.
-	require.NoError(t, task.GenerateToken())
+	task.GenerateToken()
 
 	// Explicitly need "is_fork_pull_request".
 	require.NoError(t, actions_model.UpdateTask(t.Context(), task, "repo_id", "owner_id", "is_fork_pull_request", "token", "token_salt", "token_hash", "token_last_eight"))