From 331e37c1dc2ffaa5e53f27771809e7bccda8c20c Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Fri, 6 Sep 2024 18:12:43 +0200 Subject: [PATCH 01/10] Added function for random pass generator --- internal/utilities/password.go | 66 ++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 internal/utilities/password.go diff --git a/internal/utilities/password.go b/internal/utilities/password.go new file mode 100644 index 000000000..6123b44e3 --- /dev/null +++ b/internal/utilities/password.go @@ -0,0 +1,66 @@ +package utilities + +import ( + "fmt" + "math/rand" + "strings" + "time" +) + +func parseGroups(requiredChars string) []string { + var groups []string + var currentGroup strings.Builder + inEscape := false + + for _, ch := range requiredChars { + if inEscape { + if ch == ':' { + currentGroup.WriteRune(ch) + inEscape = false + } else { + currentGroup.WriteRune('\\') + currentGroup.WriteRune(ch) + inEscape = false + } + } else if ch == '\\' { + inEscape = true + } else if ch == ':' { + groups = append(groups, currentGroup.String()) + currentGroup.Reset() + } else { + currentGroup.WriteRune(ch) + } + } + + if currentGroup.Len() > 0 { + groups = append(groups, currentGroup.String()) + } + + return groups +} + +func generatePassword(requiredChars string, length int) string { + rand.Seed(time.Now().UnixNano()) + + groups := parseGroups(requiredChars) + passwordBuilder := strings.Builder{} + + for _, group := range groups { + if len(group) > 0 { + passwordBuilder.WriteString(string(group[rand.Intn(len(group))])) + } + } + + allChars := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + for passwordBuilder.Len() < length { + passwordBuilder.WriteString(string(allChars[rand.Intn(len(allChars))])) + } + + password := passwordBuilder.String() + passwordBytes := []byte(password) + rand.Shuffle(len(passwordBytes), func(i, j int) { + passwordBytes[i], passwordBytes[j] = passwordBytes[j], passwordBytes[i] + }) + + return string(passwordBytes) +} \ No newline at end of file From 1ee831fdca8b7c2bc8718ad9e4eaf6f01c1f2f24 Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Fri, 6 Sep 2024 19:32:12 +0000 Subject: [PATCH 02/10] Fixed type mismatch --- internal/api/magic_link.go | 7 +-- internal/utilities/password.go | 108 +++++++++++++-------------------- 2 files changed, 44 insertions(+), 71 deletions(-) diff --git a/internal/api/magic_link.go b/internal/api/magic_link.go index 44f9fc88f..791ce0327 100644 --- a/internal/api/magic_link.go +++ b/internal/api/magic_link.go @@ -8,9 +8,9 @@ import ( "net/http" "strings" - "github.com/sethvargo/go-password/password" "github.com/supabase/auth/internal/models" "github.com/supabase/auth/internal/storage" + "github.com/supabase/auth/internal/utilities" ) // MagicLinkParams holds the parameters for a magic link request @@ -83,10 +83,7 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error { if isNewUser { // User either doesn't exist or hasn't completed the signup process. // Sign them up with temporary password. - password, err := password.Generate(64, 10, 1, false, true) - if err != nil { - return internalServerError("error creating user").WithInternalError(err) - } + password := utilities.GeneratePassword(config.Password.RequiredCharacters, 33) signUpParams := &SignupParams{ Email: params.Email, diff --git a/internal/utilities/password.go b/internal/utilities/password.go index 6123b44e3..786ff8163 100644 --- a/internal/utilities/password.go +++ b/internal/utilities/password.go @@ -1,66 +1,42 @@ -package utilities - -import ( - "fmt" - "math/rand" - "strings" - "time" -) - -func parseGroups(requiredChars string) []string { - var groups []string - var currentGroup strings.Builder - inEscape := false - - for _, ch := range requiredChars { - if inEscape { - if ch == ':' { - currentGroup.WriteRune(ch) - inEscape = false - } else { - currentGroup.WriteRune('\\') - currentGroup.WriteRune(ch) - inEscape = false - } - } else if ch == '\\' { - inEscape = true - } else if ch == ':' { - groups = append(groups, currentGroup.String()) - currentGroup.Reset() - } else { - currentGroup.WriteRune(ch) - } - } - - if currentGroup.Len() > 0 { - groups = append(groups, currentGroup.String()) - } - - return groups -} - -func generatePassword(requiredChars string, length int) string { - rand.Seed(time.Now().UnixNano()) - - groups := parseGroups(requiredChars) - passwordBuilder := strings.Builder{} - - for _, group := range groups { - if len(group) > 0 { - passwordBuilder.WriteString(string(group[rand.Intn(len(group))])) - } - } - - allChars := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" - for passwordBuilder.Len() < length { - passwordBuilder.WriteString(string(allChars[rand.Intn(len(allChars))])) - } - - password := passwordBuilder.String() - passwordBytes := []byte(password) - rand.Shuffle(len(passwordBytes), func(i, j int) { - passwordBytes[i], passwordBytes[j] = passwordBytes[j], passwordBytes[i] - }) - - return string(passwordBytes) -} \ No newline at end of file +package utilities + +import ( + "math/rand" + "strings" + "time" +) + +// parseGroups processes the required character groups from a slice of strings. +func parseGroups(requiredChars []string) []string { + var groups []string + groups = append(groups, requiredChars...) + return groups +} + +func GeneratePassword(requiredChars []string, length int) string { + rand.Seed(time.Now().UnixNano()) + + groups := parseGroups(requiredChars) + passwordBuilder := strings.Builder{} + + for _, group := range groups { + if len(group) > 0 { + passwordBuilder.WriteString(string(group[rand.Intn(len(group))])) + } + } + + // Define a default character set for random generation (if needed) + allChars := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + + for passwordBuilder.Len() < length { + passwordBuilder.WriteString(string(allChars[rand.Intn(len(allChars))])) + } + + password := passwordBuilder.String() + passwordBytes := []byte(password) + rand.Shuffle(len(passwordBytes), func(i, j int) { + passwordBytes[i], passwordBytes[j] = passwordBytes[j], passwordBytes[i] + }) + + return string(passwordBytes) +} From 943cba65ce8e6738b669b374d64c0fa48c7ddd04 Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Fri, 6 Sep 2024 19:47:11 +0000 Subject: [PATCH 03/10] error test --- internal/api/magic_link.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/api/magic_link.go b/internal/api/magic_link.go index 791ce0327..ac27d98ba 100644 --- a/internal/api/magic_link.go +++ b/internal/api/magic_link.go @@ -84,6 +84,7 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error { // User either doesn't exist or hasn't completed the signup process. // Sign them up with temporary password. password := utilities.GeneratePassword(config.Password.RequiredCharacters, 33) + return internalServerError(password) signUpParams := &SignupParams{ Email: params.Email, From 655c5bdadd986d44373db352a7addf0ad88ccaaa Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Fri, 6 Sep 2024 20:33:13 +0000 Subject: [PATCH 04/10] Removed debug --- internal/api/magic_link.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/api/magic_link.go b/internal/api/magic_link.go index ac27d98ba..791ce0327 100644 --- a/internal/api/magic_link.go +++ b/internal/api/magic_link.go @@ -84,7 +84,6 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error { // User either doesn't exist or hasn't completed the signup process. // Sign them up with temporary password. password := utilities.GeneratePassword(config.Password.RequiredCharacters, 33) - return internalServerError(password) signUpParams := &SignupParams{ Email: params.Email, From db57285e6aacf9b1c4ddbfb8e8d48f32834ee147 Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Mon, 9 Sep 2024 11:13:33 +0200 Subject: [PATCH 05/10] Removed depcrecated function --- internal/utilities/password.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/utilities/password.go b/internal/utilities/password.go index 786ff8163..08965ac77 100644 --- a/internal/utilities/password.go +++ b/internal/utilities/password.go @@ -14,7 +14,6 @@ func parseGroups(requiredChars []string) []string { } func GeneratePassword(requiredChars []string, length int) string { - rand.Seed(time.Now().UnixNano()) groups := parseGroups(requiredChars) passwordBuilder := strings.Builder{} From 07a44bcc40403d943831393462e86861d4d0767e Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Tue, 10 Sep 2024 22:10:30 +0200 Subject: [PATCH 06/10] fix: "time" imported and not used --- internal/utilities/password.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/utilities/password.go b/internal/utilities/password.go index 08965ac77..52585caf3 100644 --- a/internal/utilities/password.go +++ b/internal/utilities/password.go @@ -3,7 +3,6 @@ package utilities import ( "math/rand" "strings" - "time" ) // parseGroups processes the required character groups from a slice of strings. From fb1a58c3ca0845285da6fb6f91f11b8ddbd311bd Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Tue, 10 Sep 2024 20:36:12 +0000 Subject: [PATCH 07/10] fix: reworked GeneratePassword to use secure crypt random generator --- internal/api/magic_link.go | 6 ++++- internal/utilities/password.go | 45 ++++++++++++++++++++++++++-------- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/internal/api/magic_link.go b/internal/api/magic_link.go index 791ce0327..36840da0a 100644 --- a/internal/api/magic_link.go +++ b/internal/api/magic_link.go @@ -83,7 +83,11 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error { if isNewUser { // User either doesn't exist or hasn't completed the signup process. // Sign them up with temporary password. - password := utilities.GeneratePassword(config.Password.RequiredCharacters, 33) + password, err := utilities.GeneratePassword(config.Password.RequiredCharacters, 33) + if err != nil { + // password generation must succeed + panic(err) + } signUpParams := &SignupParams{ Email: params.Email, diff --git a/internal/utilities/password.go b/internal/utilities/password.go index 52585caf3..6077b80aa 100644 --- a/internal/utilities/password.go +++ b/internal/utilities/password.go @@ -1,7 +1,8 @@ package utilities import ( - "math/rand" + "crypto/rand" + "math/big" "strings" ) @@ -12,29 +13,53 @@ func parseGroups(requiredChars []string) []string { return groups } -func GeneratePassword(requiredChars []string, length int) string { - +func GeneratePassword(requiredChars []string, length int) (string, error) { groups := parseGroups(requiredChars) passwordBuilder := strings.Builder{} + passwordBuilder.Grow(length) + // Add required characters for _, group := range groups { if len(group) > 0 { - passwordBuilder.WriteString(string(group[rand.Intn(len(group))])) + randomIndex, err := secureRandomInt(len(group)) + if err != nil { + return "", err + } + passwordBuilder.WriteByte(group[randomIndex]) } } // Define a default character set for random generation (if needed) allChars := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + // Fill the rest of the password for passwordBuilder.Len() < length { - passwordBuilder.WriteString(string(allChars[rand.Intn(len(allChars))])) + randomIndex, err := secureRandomInt(len(allChars)) + if err != nil { + return "", err + } + passwordBuilder.WriteByte(allChars[randomIndex]) } - password := passwordBuilder.String() - passwordBytes := []byte(password) - rand.Shuffle(len(passwordBytes), func(i, j int) { + // Convert to byte slice for shuffling + passwordBytes := []byte(passwordBuilder.String()) + + // Secure shuffling + for i := len(passwordBytes) - 1; i > 0; i-- { + j, err := secureRandomInt(i + 1) + if err != nil { + return "", err + } passwordBytes[i], passwordBytes[j] = passwordBytes[j], passwordBytes[i] - }) + } + + return string(passwordBytes), nil +} - return string(passwordBytes) +func secureRandomInt(max int) (int, error) { + randomInt, err := rand.Int(rand.Reader, big.NewInt(int64(max))) + if err != nil { + return 0, err + } + return int(randomInt.Int64()), nil } From f33ed1c1c7c4e94c1e6b743d9979ecff91ec4354 Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Tue, 24 Sep 2024 10:50:58 +0200 Subject: [PATCH 08/10] Apply suggestions from code review Co-authored-by: Chris Stockton <180184+cstockton@users.noreply.github.com> --- internal/utilities/password.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/internal/utilities/password.go b/internal/utilities/password.go index 6077b80aa..49dc03748 100644 --- a/internal/utilities/password.go +++ b/internal/utilities/password.go @@ -6,20 +6,12 @@ import ( "strings" ) -// parseGroups processes the required character groups from a slice of strings. -func parseGroups(requiredChars []string) []string { - var groups []string - groups = append(groups, requiredChars...) - return groups -} - func GeneratePassword(requiredChars []string, length int) (string, error) { - groups := parseGroups(requiredChars) passwordBuilder := strings.Builder{} passwordBuilder.Grow(length) // Add required characters - for _, group := range groups { + for _, group := range requiredChars { if len(group) > 0 { randomIndex, err := secureRandomInt(len(group)) if err != nil { @@ -30,7 +22,7 @@ func GeneratePassword(requiredChars []string, length int) (string, error) { } // Define a default character set for random generation (if needed) - allChars := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + const allChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" // Fill the rest of the password for passwordBuilder.Len() < length { From d2fe8f9da7c5a8e11f7e16301dcb488ceb917c2d Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Tue, 24 Sep 2024 10:15:00 +0000 Subject: [PATCH 09/10] chore: moved function to appropriate file and generated tests --- internal/api/magic_link.go | 4 +- internal/crypto/crypto.go | 9 +++++ internal/crypto/password.go | 42 +++++++++++++++++++++ internal/crypto/password_test.go | 65 ++++++++++++++++++++++++++++++++ internal/utilities/password.go | 57 ---------------------------- 5 files changed, 118 insertions(+), 59 deletions(-) delete mode 100644 internal/utilities/password.go diff --git a/internal/api/magic_link.go b/internal/api/magic_link.go index 36840da0a..337a00d60 100644 --- a/internal/api/magic_link.go +++ b/internal/api/magic_link.go @@ -8,9 +8,9 @@ import ( "net/http" "strings" + "github.com/supabase/auth/internal/crypto" "github.com/supabase/auth/internal/models" "github.com/supabase/auth/internal/storage" - "github.com/supabase/auth/internal/utilities" ) // MagicLinkParams holds the parameters for a magic link request @@ -83,7 +83,7 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error { if isNewUser { // User either doesn't exist or hasn't completed the signup process. // Sign them up with temporary password. - password, err := utilities.GeneratePassword(config.Password.RequiredCharacters, 33) + password, err := crypto.GeneratePassword(config.Password.RequiredCharacters, 33) if err != nil { // password generation must succeed panic(err) diff --git a/internal/crypto/crypto.go b/internal/crypto/crypto.go index be6a2b5df..56cf4d5c8 100644 --- a/internal/crypto/crypto.go +++ b/internal/crypto/crypto.go @@ -51,6 +51,15 @@ func GenerateTokenHash(emailOrPhone, otp string) string { return fmt.Sprintf("%x", sha256.Sum224([]byte(emailOrPhone+otp))) } +// Generated a random secure integer from [0, max[ +func secureRandomInt(max int) (int, error) { + randomInt, err := rand.Int(rand.Reader, big.NewInt(int64(max))) + if err != nil { + return 0, errors.WithMessage(err, "Error generating random integer") + } + return int(randomInt.Int64()), nil +} + func GenerateSignatures(secrets []string, msgID uuid.UUID, currentTime time.Time, inputPayload []byte) ([]string, error) { SymmetricSignaturePrefix := "v1," // TODO(joel): Handle asymmetric case once library has been upgraded diff --git a/internal/crypto/password.go b/internal/crypto/password.go index bce145a45..451ab121c 100644 --- a/internal/crypto/password.go +++ b/internal/crypto/password.go @@ -234,3 +234,45 @@ func GenerateFromPassword(ctx context.Context, password string) (string, error) return string(hash), nil } + +func GeneratePassword(requiredChars []string, length int) (string, error) { + passwordBuilder := strings.Builder{} + passwordBuilder.Grow(length) + + // Add required characters + for _, group := range requiredChars { + if len(group) > 0 { + randomIndex, err := secureRandomInt(len(group)) + if err != nil { + return "", err + } + passwordBuilder.WriteByte(group[randomIndex]) + } + } + + // Define a default character set for random generation (if needed) + const allChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + + // Fill the rest of the password + for passwordBuilder.Len() < length { + randomIndex, err := secureRandomInt(len(allChars)) + if err != nil { + return "", err + } + passwordBuilder.WriteByte(allChars[randomIndex]) + } + + // Convert to byte slice for shuffling + passwordBytes := []byte(passwordBuilder.String()) + + // Secure shuffling + for i := len(passwordBytes) - 1; i > 0; i-- { + j, err := secureRandomInt(i + 1) + if err != nil { + return "", err + } + passwordBytes[i], passwordBytes[j] = passwordBytes[j], passwordBytes[i] + } + + return string(passwordBytes), nil +} diff --git a/internal/crypto/password_test.go b/internal/crypto/password_test.go index c3091975b..59eb0a08b 100644 --- a/internal/crypto/password_test.go +++ b/internal/crypto/password_test.go @@ -2,6 +2,7 @@ package crypto import ( "context" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -19,3 +20,67 @@ func TestArgon2(t *testing.T) { assert.NoError(t, CompareHashAndPassword(context.Background(), example, "test")) } } + +func TestGeneratePassword(t *testing.T) { + tests := []struct { + name string + requiredChars []string + length int + wantErr bool + }{ + { + name: "Valid password generation", + requiredChars: []string{"ABC", "123", "@#$"}, + length: 12, + wantErr: false, + }, + { + name: "Empty required chars", + requiredChars: []string{}, + length: 8, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := GeneratePassword(tt.requiredChars, tt.length) + + if (err != nil) != tt.wantErr { + t.Errorf("GeneratePassword() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if len(got) != tt.length { + t.Errorf("GeneratePassword() returned password of length %d, want %d", len(got), tt.length) + } + + // Check if all required characters are present + for _, chars := range tt.requiredChars { + found := false + for _, c := range got { + if strings.ContainsRune(chars, c) { + found = true + break + } + } + if !found && len(chars) > 0 { + t.Errorf("GeneratePassword() missing required character from set %s", chars) + } + } + }) + } + + // Check for duplicates passwords + passwords := make(map[string]bool) + for i := 0; i < 30; i++ { + p, err := GeneratePassword([]string{"ABC", "123", "@#$"}, 30) + if err != nil { + t.Errorf("GeneratePassword() unexpected error: %v", err) + } + if passwords[p] { + t.Errorf("GeneratePassword() generated duplicate password: %s", p) + } + passwords[p] = true + } +} diff --git a/internal/utilities/password.go b/internal/utilities/password.go deleted file mode 100644 index 49dc03748..000000000 --- a/internal/utilities/password.go +++ /dev/null @@ -1,57 +0,0 @@ -package utilities - -import ( - "crypto/rand" - "math/big" - "strings" -) - -func GeneratePassword(requiredChars []string, length int) (string, error) { - passwordBuilder := strings.Builder{} - passwordBuilder.Grow(length) - - // Add required characters - for _, group := range requiredChars { - if len(group) > 0 { - randomIndex, err := secureRandomInt(len(group)) - if err != nil { - return "", err - } - passwordBuilder.WriteByte(group[randomIndex]) - } - } - - // Define a default character set for random generation (if needed) - const allChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" - - // Fill the rest of the password - for passwordBuilder.Len() < length { - randomIndex, err := secureRandomInt(len(allChars)) - if err != nil { - return "", err - } - passwordBuilder.WriteByte(allChars[randomIndex]) - } - - // Convert to byte slice for shuffling - passwordBytes := []byte(passwordBuilder.String()) - - // Secure shuffling - for i := len(passwordBytes) - 1; i > 0; i-- { - j, err := secureRandomInt(i + 1) - if err != nil { - return "", err - } - passwordBytes[i], passwordBytes[j] = passwordBytes[j], passwordBytes[i] - } - - return string(passwordBytes), nil -} - -func secureRandomInt(max int) (int, error) { - randomInt, err := rand.Int(rand.Reader, big.NewInt(int64(max))) - if err != nil { - return 0, err - } - return int(randomInt.Int64()), nil -} From a23885566c737b41442a1b6dae3f7756f12e068b Mon Sep 17 00:00:00 2001 From: klajdi369 <21219484+klajdi369@users.noreply.github.com> Date: Tue, 24 Sep 2024 12:24:50 +0000 Subject: [PATCH 10/10] fix: properly returning error --- internal/api/magic_link.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/api/magic_link.go b/internal/api/magic_link.go index 337a00d60..15030bde2 100644 --- a/internal/api/magic_link.go +++ b/internal/api/magic_link.go @@ -85,8 +85,7 @@ func (a *API) MagicLink(w http.ResponseWriter, r *http.Request) error { // Sign them up with temporary password. password, err := crypto.GeneratePassword(config.Password.RequiredCharacters, 33) if err != nil { - // password generation must succeed - panic(err) + return internalServerError("error creating user").WithInternalError(err) } signUpParams := &SignupParams{