From 3991bdb269f72756becfacee5bd9e540c5b71250 Mon Sep 17 00:00:00 2001 From: Stojan Dimitrovski Date: Tue, 28 Nov 2023 18:58:27 +0100 Subject: [PATCH] feat: add required characters password strength check (#1323) Adds the `GOTRUE_PASSWORD_REQUIRED_CHARACTERS` config option, which if set, will reject passwords that do not contain at least one character of each set of characters. It is defined like so: `abc...xyz:0123...89`. This means that at least one lowercase and one digit has to be present in the password to be accepted. All other characters are also allowed. To include the `:` character, escape it with `\:`. When a weak password is detected, the HTTP 429 error is sent with an additional JSON field `weak_password` that includes a `reasons` property -- an array of the strings: - `length` if the password is not long enough - `characters` if the password does not use all required character sets --------- Co-authored-by: Kang Ming --- README.md | 2 + internal/api/admin_test.go | 4 +- internal/api/errors.go | 20 ++++-- internal/api/password.go | 43 ++++++++++- internal/api/password_test.go | 107 ++++++++++++++++++++++++++++ internal/conf/configuration.go | 65 +++++++++++++---- internal/conf/configuration_test.go | 69 ++++++++++++++++++ 7 files changed, 286 insertions(+), 24 deletions(-) create mode 100644 internal/api/password_test.go diff --git a/README.md b/README.md index 413e7f9178..92e9594dfa 100644 --- a/README.md +++ b/README.md @@ -217,6 +217,8 @@ Rate limit the number of emails sent per hr on the following endpoints: `/signup Minimum password length, defaults to 6. +`GOTRUE_PASSWORD_REQUIRED_CHARACTERS` - a string of character sets separated by `:`. A password must contain at least one character of each set to be accepted. To use the `:` character escape it with `\`. + `GOTRUE_SECURITY_REFRESH_TOKEN_ROTATION_ENABLED` - `bool` If refresh token rotation is enabled, gotrue will automatically detect malicious attempts to reuse a revoked refresh token. When a malicious attempt is detected, gotrue immediately revokes all tokens that descended from the offending token. diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go index ea2fab8953..f886ca88cc 100644 --- a/internal/api/admin_test.go +++ b/internal/api/admin_test.go @@ -456,7 +456,7 @@ func (ts *AdminTestSuite) TestAdminUserUpdatePasswordFailed() { require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user") var updateEndpoint = fmt.Sprintf("/admin/users/%s", u.ID) - ts.Config.PasswordMinLength = 6 + ts.Config.Password.MinLength = 6 ts.Run("Password doesn't meet minimum length", func() { var buffer bytes.Buffer require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ @@ -479,7 +479,7 @@ func (ts *AdminTestSuite) TestAdminUserUpdateBannedUntilFailed() { require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user") var updateEndpoint = fmt.Sprintf("/admin/users/%s", u.ID) - ts.Config.PasswordMinLength = 6 + ts.Config.Password.MinLength = 6 ts.Run("Incorrect format for ban_duration", func() { var buffer bytes.Buffer require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{ diff --git a/internal/api/errors.go b/internal/api/errors.go index 8efdbe811e..bb7ba4870f 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -65,10 +65,6 @@ func (e *OAuthError) Cause() error { return e } -func invalidPasswordLengthError(passwordMinLength int) *HTTPError { - return unprocessableEntityError(fmt.Sprintf("Password should be at least %d characters", passwordMinLength)) -} - func invalidSignupError(config *conf.GlobalConfiguration) *HTTPError { var msg string if config.External.Email.Enabled && config.External.Phone.Enabled { @@ -245,6 +241,22 @@ func handleError(err error, w http.ResponseWriter, r *http.Request) { log := observability.GetLogEntry(r) errorID := getRequestID(r.Context()) switch e := err.(type) { + case *WeakPasswordError: + var output struct { + HTTPError + Payload struct { + Reasons []string `json:"reasons,omitempty"` + } `json:"weak_password,omitempty"` + } + + output.Code = http.StatusUnprocessableEntity + output.Message = e.Message + output.Payload.Reasons = e.Reasons + + if jsonErr := sendJSON(w, output.Code, output); jsonErr != nil { + handleError(jsonErr, w, r) + } + case *HTTPError: if e.Code >= http.StatusInternalServerError { e.ErrorID = errorID diff --git a/internal/api/password.go b/internal/api/password.go index 7b98d4eea9..a614d67bd2 100644 --- a/internal/api/password.go +++ b/internal/api/password.go @@ -1,12 +1,49 @@ package api -import "context" +import ( + "context" + "fmt" + "strings" +) + +// WeakPasswordError encodes an error that a password does not meet strength +// requirements. It is handled specially in errors.go as it gets transformed to +// a HTTPError with a special weak_password field that encodes the Reasons +// slice. +type WeakPasswordError struct { + Message string + Reasons []string +} + +func (e *WeakPasswordError) Error() string { + return e.Message +} func (a *API) checkPasswordStrength(ctx context.Context, password string) error { config := a.config - if len(password) < config.PasswordMinLength { - return invalidPasswordLengthError(config.PasswordMinLength) + var messages, reasons []string + + if len(password) < config.Password.MinLength { + reasons = append(reasons, "length") + messages = append(messages, fmt.Sprintf("Password should be at least %d characters.", config.Password.MinLength)) + } + + for _, characterSet := range config.Password.RequiredCharacters { + if characterSet != "" && !strings.ContainsAny(password, characterSet) { + reasons = append(reasons, "characters") + + messages = append(messages, fmt.Sprintf("Password should contain at least one character of each: %s.", strings.Join(config.Password.RequiredCharacters, ", "))) + + break + } + } + + if len(reasons) > 0 { + return &WeakPasswordError{ + Message: strings.Join(messages, " "), + Reasons: reasons, + } } return nil diff --git a/internal/api/password_test.go b/internal/api/password_test.go new file mode 100644 index 0000000000..a35fe2d05e --- /dev/null +++ b/internal/api/password_test.go @@ -0,0 +1,107 @@ +package api + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "github.com/supabase/gotrue/internal/conf" +) + +func TestPasswordStrengthChecks(t *testing.T) { + examples := []struct { + MinLength int + RequiredCharacters []string + + Password string + Reasons []string + }{ + { + MinLength: 6, + Password: "12345", + Reasons: []string{ + "length", + }, + }, + { + MinLength: 6, + RequiredCharacters: []string{ + "a", + "b", + "c", + }, + Password: "123", + Reasons: []string{ + "length", + "characters", + }, + }, + { + MinLength: 6, + RequiredCharacters: []string{ + "a", + "b", + "c", + }, + Password: "a123", + Reasons: []string{ + "length", + "characters", + }, + }, + { + MinLength: 6, + RequiredCharacters: []string{ + "a", + "b", + "c", + }, + Password: "ab123", + Reasons: []string{ + "length", + "characters", + }, + }, + { + MinLength: 6, + RequiredCharacters: []string{ + "a", + "b", + "c", + }, + Password: "c123", + Reasons: []string{ + "length", + "characters", + }, + }, + { + MinLength: 6, + RequiredCharacters: []string{ + "a", + "b", + "c", + }, + Password: "abc123", + Reasons: nil, + }, + } + + for i, example := range examples { + api := &API{ + config: &conf.GlobalConfiguration{ + Password: conf.PasswordConfiguration{ + MinLength: example.MinLength, + RequiredCharacters: conf.PasswordRequiredCharacters(example.RequiredCharacters), + }, + }, + } + + err := api.checkPasswordStrength(context.Background(), example.Password) + if example.Reasons == nil { + require.NoError(t, err, "Example %d failed with error", i) + } else { + require.Equal(t, err.(*WeakPasswordError).Reasons, example.Reasons, "Example %d failed with wrong reasons", i) + } + } +} diff --git a/internal/conf/configuration.go b/internal/conf/configuration.go index a3778a5c0c..4025bd53a7 100644 --- a/internal/conf/configuration.go +++ b/internal/conf/configuration.go @@ -131,6 +131,41 @@ func (c *SessionsConfiguration) Validate() error { return nil } +type PasswordRequiredCharacters []string + +func (v *PasswordRequiredCharacters) Decode(value string) error { + parts := strings.Split(value, ":") + + for i := 0; i < len(parts)-1; i += 1 { + part := parts[i] + + if part == "" { + continue + } + + // part ended in escape character, so it should be joined with the next one + if part[len(part)-1] == '\\' { + parts[i] = part[0:len(part)-1] + ":" + parts[i+1] + parts[i+1] = "" + continue + } + } + + for _, part := range parts { + if part != "" { + *v = append(*v, part) + } + } + + return nil +} + +type PasswordConfiguration struct { + MinLength int `json:"min_length" split_words:"true"` + + RequiredCharacters PasswordRequiredCharacters `json:"required_characters" split_words:"true"` +} + // GlobalConfiguration holds all the configuration that applies to all instances. type GlobalConfiguration struct { API APIConfiguration @@ -149,19 +184,19 @@ type GlobalConfiguration struct { RateLimitTokenRefresh float64 `split_words:"true" default:"150"` RateLimitSso float64 `split_words:"true" default:"30"` - SiteURL string `json:"site_url" split_words:"true" required:"true"` - URIAllowList []string `json:"uri_allow_list" split_words:"true"` - URIAllowListMap map[string]glob.Glob - PasswordMinLength int `json:"password_min_length" split_words:"true"` - JWT JWTConfiguration `json:"jwt"` - Mailer MailerConfiguration `json:"mailer"` - Sms SmsProviderConfiguration `json:"sms"` - DisableSignup bool `json:"disable_signup" split_words:"true"` - Webhook WebhookConfig `json:"webhook" split_words:"true"` - Security SecurityConfiguration `json:"security"` - Sessions SessionsConfiguration `json:"sessions"` - MFA MFAConfiguration `json:"MFA"` - Cookie struct { + SiteURL string `json:"site_url" split_words:"true" required:"true"` + URIAllowList []string `json:"uri_allow_list" split_words:"true"` + URIAllowListMap map[string]glob.Glob + Password PasswordConfiguration `json:"password"` + JWT JWTConfiguration `json:"jwt"` + Mailer MailerConfiguration `json:"mailer"` + Sms SmsProviderConfiguration `json:"sms"` + DisableSignup bool `json:"disable_signup" split_words:"true"` + Webhook WebhookConfig `json:"webhook" split_words:"true"` + Security SecurityConfiguration `json:"security"` + Sessions SessionsConfiguration `json:"sessions"` + MFA MFAConfiguration `json:"MFA"` + Cookie struct { Key string `json:"key"` Domain string `json:"domain"` Duration int `json:"duration"` @@ -516,8 +551,8 @@ func (config *GlobalConfiguration) ApplyDefaults() error { } } - if config.PasswordMinLength < defaultMinPasswordLength { - config.PasswordMinLength = defaultMinPasswordLength + if config.Password.MinLength < defaultMinPasswordLength { + config.Password.MinLength = defaultMinPasswordLength } if config.MFA.ChallengeExpiryDuration < defaultChallengeExpiryDuration { config.MFA.ChallengeExpiryDuration = defaultChallengeExpiryDuration diff --git a/internal/conf/configuration_test.go b/internal/conf/configuration_test.go index 6931753749..a9178085c9 100644 --- a/internal/conf/configuration_test.go +++ b/internal/conf/configuration_test.go @@ -26,3 +26,72 @@ func TestGlobal(t *testing.T) { require.NotNil(t, gc) assert.Equal(t, "X-Request-ID", gc.API.RequestIDHeader) } + +func TestPasswordRequiredCharactersDecode(t *testing.T) { + examples := []struct { + Value string + Result []string + }{ + { + Value: "a:b:c", + Result: []string{ + "a", + "b", + "c", + }, + }, + { + Value: "a\\:b:c", + Result: []string{ + "a:b", + "c", + }, + }, + { + Value: "a:b\\:c", + Result: []string{ + "a", + "b:c", + }, + }, + { + Value: "\\:a:b:c", + Result: []string{ + ":a", + "b", + "c", + }, + }, + { + Value: "a:b:c\\:", + Result: []string{ + "a", + "b", + "c:", + }, + }, + { + Value: "::\\::", + Result: []string{ + ":", + }, + }, + { + Value: "", + Result: nil, + }, + { + Value: " ", + Result: []string{ + " ", + }, + }, + } + + for i, example := range examples { + var into PasswordRequiredCharacters + require.NoError(t, into.Decode(example.Value), "Example %d failed with error", i) + + require.Equal(t, []string(into), example.Result, "Example %d got unexpected result", i) + } +}