From d50cb86cc7b7d59033f9182431ea44bcdbb9ceb1 Mon Sep 17 00:00:00 2001 From: Etienne Stalmans Date: Sun, 29 Mar 2026 13:56:32 +0200 Subject: [PATCH] feat: add alphanumeric OTPs --- internal/api/mail.go | 50 +++++++++++++++++---- internal/api/mfa.go | 8 +++- internal/api/phone.go | 6 ++- internal/conf/configuration.go | 15 ++++--- internal/crypto/crypto.go | 17 ++++++++ internal/crypto/crypto_test.go | 79 ++++++++++++++++++++++++++++++++++ 6 files changed, 158 insertions(+), 17 deletions(-) diff --git a/internal/api/mail.go b/internal/api/mail.go index 87c4841c34..6256cbe44b 100644 --- a/internal/api/mail.go +++ b/internal/api/mail.go @@ -90,9 +90,13 @@ func (a *API) adminGenerateLink(w http.ResponseWriter, r *http.Request) error { } } - var url string + var url, otp string now := time.Now() - otp := crypto.GenerateOtp(config.Mailer.OtpLength) + if config.Mailer.OtpAlphaNumeric { + otp = crypto.GenerateAlphanumericOtp(config.Mailer.OtpLength) + } else { + otp = crypto.GenerateOtp(config.Mailer.OtpLength) + } hashedToken := crypto.GenerateTokenHash(params.Email, otp) @@ -326,7 +330,12 @@ func (a *API) sendConfirmation(r *http.Request, tx *storage.Connection, u *model return err } oldToken := u.ConfirmationToken - otp := crypto.GenerateOtp(otpLength) + var otp string + if config.Mailer.OtpAlphaNumeric { + otp = crypto.GenerateAlphanumericOtp(otpLength) + } else { + otp = crypto.GenerateOtp(otpLength) + } token := crypto.GenerateTokenHash(u.GetEmail(), otp) u.ConfirmationToken = addFlowPrefixToToken(token, flowType) @@ -361,7 +370,12 @@ func (a *API) sendInvite(r *http.Request, tx *storage.Connection, u *models.User otpLength := config.Mailer.OtpLength var err error oldToken := u.ConfirmationToken - otp := crypto.GenerateOtp(otpLength) + var otp string + if config.Mailer.OtpAlphaNumeric { + otp = crypto.GenerateAlphanumericOtp(otpLength) + } else { + otp = crypto.GenerateOtp(otpLength) + } u.ConfirmationToken = crypto.GenerateTokenHash(u.GetEmail(), otp) now := time.Now() @@ -403,7 +417,12 @@ func (a *API) sendPasswordRecovery(r *http.Request, tx *storage.Connection, u *m } oldToken := u.RecoveryToken - otp := crypto.GenerateOtp(otpLength) + var otp string + if config.Mailer.OtpAlphaNumeric { + otp = crypto.GenerateAlphanumericOtp(otpLength) + } else { + otp = crypto.GenerateOtp(otpLength) + } token := crypto.GenerateTokenHash(u.GetEmail(), otp) u.RecoveryToken = addFlowPrefixToToken(token, flowType) @@ -445,7 +464,12 @@ func (a *API) sendReauthenticationOtp(r *http.Request, tx *storage.Connection, u } oldToken := u.ReauthenticationToken - otp := crypto.GenerateOtp(otpLength) + var otp string + if config.Mailer.OtpAlphaNumeric { + otp = crypto.GenerateAlphanumericOtp(otpLength) + } else { + otp = crypto.GenerateOtp(otpLength) + } u.ReauthenticationToken = crypto.GenerateTokenHash(u.GetEmail(), otp) now := time.Now() @@ -488,7 +512,12 @@ func (a *API) sendMagicLink(r *http.Request, tx *storage.Connection, u *models.U } oldToken := u.RecoveryToken - otp := crypto.GenerateOtp(otpLength) + var otp string + if config.Mailer.OtpAlphaNumeric { + otp = crypto.GenerateAlphanumericOtp(otpLength) + } else { + otp = crypto.GenerateOtp(otpLength) + } token := crypto.GenerateTokenHash(u.GetEmail(), otp) u.RecoveryToken = addFlowPrefixToToken(token, flowType) @@ -528,7 +557,12 @@ func (a *API) sendEmailChange(r *http.Request, tx *storage.Connection, u *models return err } - otpNew := crypto.GenerateOtp(otpLength) + var otpNew string + if config.Mailer.OtpAlphaNumeric { + otpNew = crypto.GenerateAlphanumericOtp(otpLength) + } else { + otpNew = crypto.GenerateOtp(otpLength) + } u.EmailChange = email token := crypto.GenerateTokenHash(u.EmailChange, otpNew) diff --git a/internal/api/mfa.go b/internal/api/mfa.go index 4b6467eadb..06d72ca2f9 100644 --- a/internal/api/mfa.go +++ b/internal/api/mfa.go @@ -392,8 +392,12 @@ func (a *API) challengePhoneFactor(w http.ResponseWriter, r *http.Request) error return apierrors.NewTooManyRequestsError(apierrors.ErrorCodeOverSMSSendRateLimit, "%s", generateFrequencyLimitErrorMessage(factor.LastChallengedAt, config.MFA.Phone.MaxFrequency)) } } - - otp := crypto.GenerateOtp(config.MFA.Phone.OtpLength) + var otp string + if config.MFA.Phone.OtpAlphaNumeric { + otp = crypto.GenerateAlphanumericOtp(config.MFA.Phone.OtpLength) + } else { + otp = crypto.GenerateOtp(config.MFA.Phone.OtpLength) + } challenge, err := factor.CreatePhoneChallenge(ipAddress, otp, config.Security.DBEncryption.Encrypt, config.Security.DBEncryption.EncryptionKeyID, config.Security.DBEncryption.EncryptionKey) if err != nil { diff --git a/internal/api/phone.go b/internal/api/phone.go index 77f46ca294..b4fe1770ee 100644 --- a/internal/api/phone.go +++ b/internal/api/phone.go @@ -92,7 +92,11 @@ func (a *API) sendPhoneConfirmation(r *http.Request, tx *storage.Connection, use return "", apierrors.NewTooManyRequestsError(apierrors.ErrorCodeOverSMSSendRateLimit, "SMS rate limit exceeded") } } - otp = crypto.GenerateOtp(config.Sms.OtpLength) + if config.Sms.OtpAlphaNumeric { + otp = crypto.GenerateAlphanumericOtp(config.Sms.OtpLength) + } else { + otp = crypto.GenerateOtp(config.Sms.OtpLength) + } if config.Hook.SendSMS.Enabled { input := v0hooks.NewSendSMSInput( diff --git a/internal/conf/configuration.go b/internal/conf/configuration.go index ad7b59a022..b16aa5fcb8 100644 --- a/internal/conf/configuration.go +++ b/internal/conf/configuration.go @@ -159,10 +159,11 @@ type TOTPFactorTypeConfiguration struct { type PhoneFactorTypeConfiguration struct { // Default to false in order to ensure Phone MFA is opt-in MFAFactorTypeConfiguration - OtpLength int `json:"otp_length" split_words:"true"` - SMSTemplate *template.Template `json:"-"` - MaxFrequency time.Duration `json:"max_frequency" split_words:"true"` - Template string `json:"template"` + OtpLength int `json:"otp_length" split_words:"true"` + OtpAlphaNumeric bool `json:"otp_alpha_numeric" split_words:"true"` + SMSTemplate *template.Template `json:"-"` + MaxFrequency time.Duration `json:"max_frequency" split_words:"true"` + Template string `json:"template"` } // MFAConfiguration holds all the MFA related Configuration @@ -572,8 +573,9 @@ type MailerConfiguration struct { SecureEmailChangeEnabled bool `json:"secure_email_change_enabled" split_words:"true" default:"true"` - OtpExp uint `json:"otp_exp" split_words:"true"` - OtpLength int `json:"otp_length" split_words:"true"` + OtpExp uint `json:"otp_exp" split_words:"true"` + OtpLength int `json:"otp_length" split_words:"true"` + OtpAlphaNumeric bool `json:"otp_alpha_numeric" split_words:"true"` ExternalHosts []string `json:"external_hosts" split_words:"true"` @@ -657,6 +659,7 @@ type SmsProviderConfiguration struct { MaxFrequency time.Duration `json:"max_frequency" split_words:"true"` OtpExp uint `json:"otp_exp" split_words:"true"` OtpLength int `json:"otp_length" split_words:"true"` + OtpAlphaNumeric bool `json:"otp_alpha_numeric" split_words:"true"` Provider string `json:"provider"` Template string `json:"template"` TestOTP map[string]string `json:"test_otp" split_words:"true"` diff --git a/internal/crypto/crypto.go b/internal/crypto/crypto.go index 35bf252126..ee5fc12dfe 100644 --- a/internal/crypto/crypto.go +++ b/internal/crypto/crypto.go @@ -23,6 +23,11 @@ func GenerateOtp(digits int) string { return generateOtp(rand.Reader, digits) } +// GenerateAlphanumericOtp generates a random n digit otp with extended charset +func GenerateAlphanumericOtp(digits int) string { + return generateAlphanumericOtp(rand.Reader, digits) +} + func generateOtp(r io.Reader, digits int) string { // TODO(cstockton): Change the code to be below and propagate errors so we // can have non-panicing bounds checks. This is just a defensive change so @@ -42,6 +47,18 @@ func generateOtp(r io.Reader, digits int) string { return otp } +func generateAlphanumericOtp(r io.Reader, length int) string { + const charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + charsetLen := big.NewInt(int64(len(charset))) + + otp := make([]byte, length) + for i := range otp { + val := must(rand.Int(r, charsetLen)) + otp[i] = charset[val.Int64()] + } + return string(otp) +} + func GenerateTokenHash(emailOrPhone, otp string) string { return fmt.Sprintf("%x", sha256.Sum224([]byte(emailOrPhone+otp))) } diff --git a/internal/crypto/crypto_test.go b/internal/crypto/crypto_test.go index 0344a1b95f..ec06c7dfb3 100644 --- a/internal/crypto/crypto_test.go +++ b/internal/crypto/crypto_test.go @@ -1,9 +1,13 @@ package crypto import ( + "crypto/rand" + "math" + "strings" "testing" mrand "math/rand" + mathrand "math/rand/v2" "github.com/gofrs/uuid" "github.com/stretchr/testify/assert" @@ -77,6 +81,81 @@ func TestGenerateOtp(t *testing.T) { } } +func TestGenerateAlphanumericOtp(t *testing.T) { + const charset = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" + charsetSet := make(map[rune]bool) + for _, c := range charset { + charsetSet[c] = true + } + + t.Run("correct length", func(t *testing.T) { + for _, length := range []int{1, 4, 6, 8, 12} { + otp := generateAlphanumericOtp(rand.Reader, length) + if len(otp) != length { + t.Errorf("length=%d: got OTP of length %d: %q", length, len(otp), otp) + } + } + }) + + t.Run("only valid characters", func(t *testing.T) { + for range 100 { + otp := generateAlphanumericOtp(rand.Reader, 10) + for _, c := range otp { + if !charsetSet[c] { + t.Errorf("invalid character %q in OTP %q", c, otp) + } + } + } + }) + + t.Run("uppercase only", func(t *testing.T) { + for range 100 { + otp := generateAlphanumericOtp(rand.Reader, 10) + if otp != strings.ToUpper(otp) { + t.Errorf("OTP contains lowercase characters: %q", otp) + } + } + }) + + t.Run("deterministic with fixed reader", func(t *testing.T) { + seed := [32]byte{} + r1 := mathrand.NewChaCha8(seed) + r2 := mathrand.NewChaCha8(seed) + otp1 := generateAlphanumericOtp(r1, 8) + otp2 := generateAlphanumericOtp(r2, 8) + if otp1 != otp2 { + t.Errorf("same seed produced different OTPs: %q vs %q", otp1, otp2) + } + }) + + t.Run("different seeds produce different OTPs", func(t *testing.T) { + r1 := mathrand.NewChaCha8([32]byte{0}) + r2 := mathrand.NewChaCha8([32]byte{1}) + otp1 := generateAlphanumericOtp(r1, 16) + otp2 := generateAlphanumericOtp(r2, 16) + if otp1 == otp2 { + t.Errorf("different seeds produced the same OTP: %q", otp1) + } + }) + + t.Run("character distribution is roughly uniform", func(t *testing.T) { + counts := make(map[rune]int) + iterations := 36 * 1000 + for range iterations { + otp := generateAlphanumericOtp(rand.Reader, 1) + counts[rune(otp[0])]++ + } + expected := float64(iterations) / float64(len(charset)) + tolerance := expected * 0.15 + for _, c := range charset { + diff := math.Abs(float64(counts[c]) - expected) + if diff > tolerance { + t.Errorf("character %q count %d deviates too far from expected %.0f", c, counts[c], expected) + } + } + }) +} + func TestEncryptedStringPositive(t *testing.T) { id := uuid.Must(uuid.NewV4()).String()