diff --git a/cmd/forgejo/actions.go b/cmd/forgejo/actions.go index c445d1aa38..f520924c20 100644 --- a/cmd/forgejo/actions.go +++ b/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) } diff --git a/cmd/generate.go b/cmd/generate.go index f39f6f1aef..5df10fa4e3 100644 --- a/cmd/generate.go +++ b/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) diff --git a/models/actions/runner.go b/models/actions/runner.go index 7cdc83af93..99ae7629a8 100644 --- a/models/actions/runner.go +++ b/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 diff --git a/models/actions/runner_token.go b/models/actions/runner_token.go index a59304d8e8..ece15b55e8 100644 --- a/models/actions/runner_token.go +++ b/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 }) } diff --git a/models/actions/task.go b/models/actions/task.go index acf2c11bdd..8038e82e90 100644 --- a/models/actions/task.go +++ b/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 { diff --git a/models/actions/utils.go b/models/actions/utils.go index d8e053b0ba..ba4a58bbb5 100644 --- a/models/actions/utils.go +++ b/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:] } /* diff --git a/models/auth/access_token.go b/models/auth/access_token.go index 2bdb5357c5..ceade7dbad 100644 --- a/models/auth/access_token.go +++ b/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() diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go index 9a53ad30e0..5c9cf45f97 100644 --- a/models/auth/twofactor.go +++ b/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 } diff --git a/models/gitea_migrations/v1_6/v71.go b/models/gitea_migrations/v1_6/v71.go index c9e96980d3..9d2128090f 100644 --- a/models/gitea_migrations/v1_6/v71.go +++ b/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) diff --git a/models/gitea_migrations/v1_9/v85.go b/models/gitea_migrations/v1_9/v85.go index c54a006f4c..acb0316c32 100644 --- a/models/gitea_migrations/v1_9/v85.go +++ b/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 { diff --git a/models/organization/team_invite.go b/models/organization/team_invite.go index 45be6c4c64..3fdabcb433 100644 --- a/models/organization/team_invite.go +++ b/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, diff --git a/models/packages/package_blob_upload.go b/models/packages/package_blob_upload.go index ddffb6c305..19546afed6 100644 --- a/models/packages/package_blob_upload.go +++ b/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 } diff --git a/modules/generate/generate.go b/modules/generate/generate.go index 16a797ef5a..2df808fe9e 100644 --- a/modules/generate/generate.go +++ b/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) } diff --git a/modules/git/fetch.go b/modules/git/fetch.go index af8bb34d38..52907e6dfb 100644 --- a/modules/git/fetch.go +++ b/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) diff --git a/modules/util/util.go b/modules/util/util.go index dd84281cc2..f21936fb5f 100644 --- a/modules/util/util.go +++ b/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 diff --git a/modules/util/util_test.go b/modules/util/util_test.go index 21988fd0f8..a85113b2f4 100644 --- a/modules/util/util_test.go +++ b/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) { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 5baf2fcfe8..83f4e21683 100644 --- a/options/locale/locale_en-US.ini +++ b/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 diff --git a/routers/api/actions/runner/runner.go b/routers/api/actions/runner/runner.go index 30dda946be..83a93cb1c9 100644 --- a/routers/api/actions/runner/runner.go +++ b/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 { diff --git a/routers/api/v1/repo/mirror.go b/routers/api/v1/repo/mirror.go index fa2d5abb11..0ff8993b5a 100644 --- a/routers/api/v1/repo/mirror.go +++ b/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 { diff --git a/routers/install/install.go b/routers/install/install.go index eaede217d3..63a3f965f4 100644 --- a/routers/install/install.go +++ b/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) } diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index e3335ef077..7fbc8b8d55 100644 --- a/routers/web/auth/oauth.go +++ b/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 } diff --git a/routers/web/auth/openid.go b/routers/web/auth/openid.go index 1d7bdcd1ce..b2c4f3b68e 100644 --- a/routers/web/auth/openid.go +++ b/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 diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index 1f54ba353d..7047af9196 100644 --- a/routers/web/repo/setting/setting.go +++ b/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 { diff --git a/tests/integration/api_issue_label_test.go b/tests/integration/api_issue_label_test.go index c6706d6ce2..7199985d4e 100644 --- a/tests/integration/api_issue_label_test.go +++ b/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"))