From 6f0bb85900713bb549fc3b9938b430d9d259eeb9 Mon Sep 17 00:00:00 2001 From: Joel Lee Date: Wed, 31 Jul 2024 15:07:32 +0200 Subject: [PATCH] fix: MFA NewFactor to default to creating unverfied factors (#1692) ## What kind of change does this PR introduce? - Split `NewFactor` into `NewPhoneFactor()` and `NewTOTPFactor()`. - All `NewFactor` methods will now create unverified factors. - Additionally, also guards the `Challenge` endpoint when verification is disabled for a factor. The hope is to reduce cognitive load and the chance of creating a factor in an undesired state Should one wish to obtain a Verified Factor (say for tests) they can call `UpdateStatus`. It is unlikely for this to be a common use case though. Someone might have brought this up prior but only getting to it now --- internal/api/admin_test.go | 7 ++++--- internal/api/mfa.go | 21 +++++++++++++++++---- internal/api/mfa_test.go | 8 ++++---- internal/models/factor.go | 16 ++++++---------- internal/models/factor_test.go | 2 +- 5 files changed, 32 insertions(+), 22 deletions(-) diff --git a/internal/api/admin_test.go b/internal/api/admin_test.go index e1b2c0328..e2904d8bf 100644 --- a/internal/api/admin_test.go +++ b/internal/api/admin_test.go @@ -769,7 +769,8 @@ func (ts *AdminTestSuite) TestAdminUserDeleteFactor() { require.NoError(ts.T(), err, "Error making new user") require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user") - f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateVerified) + f := models.NewTOTPFactor(u, "testSimpleName") + require.NoError(ts.T(), f.UpdateStatus(ts.API.db, models.FactorStateVerified)) require.NoError(ts.T(), f.SetSecret("secretkey", ts.Config.Security.DBEncryption.Encrypt, ts.Config.Security.DBEncryption.EncryptionKeyID, ts.Config.Security.DBEncryption.EncryptionKey)) require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor") @@ -793,7 +794,7 @@ func (ts *AdminTestSuite) TestAdminUserGetFactors() { require.NoError(ts.T(), err, "Error making new user") require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user") - f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified) + f := models.NewTOTPFactor(u, "testSimpleName") require.NoError(ts.T(), f.SetSecret("secretkey", ts.Config.Security.DBEncryption.Encrypt, ts.Config.Security.DBEncryption.EncryptionKeyID, ts.Config.Security.DBEncryption.EncryptionKey)) require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor") @@ -815,7 +816,7 @@ func (ts *AdminTestSuite) TestAdminUserUpdateFactor() { require.NoError(ts.T(), err, "Error making new user") require.NoError(ts.T(), ts.API.db.Create(u), "Error creating user") - f := models.NewFactor(u, "testSimpleName", models.TOTP, models.FactorStateUnverified) + f := models.NewTOTPFactor(u, "testSimpleName") require.NoError(ts.T(), f.SetSecret("secretkey", ts.Config.Security.DBEncryption.Encrypt, ts.Config.Security.DBEncryption.EncryptionKeyID, ts.Config.Security.DBEncryption.EncryptionKey)) require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor") diff --git a/internal/api/mfa.go b/internal/api/mfa.go index 126f33545..a3d4d9f55 100644 --- a/internal/api/mfa.go +++ b/internal/api/mfa.go @@ -108,7 +108,7 @@ func (a *API) enrollPhoneFactor(w http.ResponseWriter, r *http.Request, params * if numVerifiedFactors > 0 && !session.IsAAL2() { return forbiddenError(ErrorCodeInsufficientAAL, "AAL2 required to enroll a new factor") } - factor := models.NewPhoneFactor(user, phone, params.FriendlyName, params.FactorType, models.FactorStateUnverified) + factor := models.NewPhoneFactor(user, phone, params.FriendlyName) err = db.Transaction(func(tx *storage.Connection) error { if terr := tx.Create(factor); terr != nil { pgErr := utilities.NewPostgresError(terr) @@ -222,7 +222,7 @@ func (a *API) EnrollFactor(w http.ResponseWriter, r *http.Request) error { } svgData.End() - factor = models.NewFactor(user, params.FriendlyName, params.FactorType, models.FactorStateUnverified) + factor = models.NewTOTPFactor(user, params.FriendlyName) if err := factor.SetSecret(key.Secret(), config.Security.DBEncryption.Encrypt, config.Security.DBEncryption.EncryptionKeyID, config.Security.DBEncryption.EncryptionKey); err != nil { return err } @@ -352,10 +352,23 @@ func (a *API) ChallengeFactor(w http.ResponseWriter, r *http.Request) error { user := getUser(ctx) factor := getFactor(ctx) + ipAddress := utilities.GetIPAddress(r) - if factor.IsPhoneFactor() { + switch factor.FactorType { + case models.Phone: + if !config.MFA.Phone.VerifyEnabled { + return unprocessableEntityError(ErrorCodeMFAPhoneEnrollDisabled, "MFA verification is disabled for Phone") + } return a.challengePhoneFactor(w, r) + + case models.TOTP: + if !config.MFA.TOTP.VerifyEnabled { + return unprocessableEntityError(ErrorCodeMFATOTPEnrollDisabled, "MFA verification is disabled for TOTP") + } + default: + return badRequestError(ErrorCodeValidationFailed, "factor_type needs to be TOTP or Phone") } + challenge := factor.CreateChallenge(ipAddress) if err := db.Transaction(func(tx *storage.Connection) error { if terr := tx.Create(challenge); terr != nil { @@ -626,7 +639,7 @@ func (a *API) VerifyFactor(w http.ResponseWriter, r *http.Request) error { return terr } } - if shouldReEncrypt && config.Security.DBEncryption.Encrypt && factor.IsTOTPFactor() { + if shouldReEncrypt && config.Security.DBEncryption.Encrypt { es, terr := crypto.NewEncryptedString(factor.ID.String(), []byte(secret), config.Security.DBEncryption.EncryptionKeyID, config.Security.DBEncryption.EncryptionKey) if terr != nil { return terr diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go index 4a8316c1e..e08f20514 100644 --- a/internal/api/mfa_test.go +++ b/internal/api/mfa_test.go @@ -62,7 +62,7 @@ func (ts *MFATestSuite) SetupTest() { require.NoError(ts.T(), err, "Error creating test user model") require.NoError(ts.T(), ts.API.db.Create(u), "Error saving new test user") // Create Factor - f := models.NewFactor(u, "test_factor", models.TOTP, models.FactorStateUnverified) + f := models.NewTOTPFactor(u, "test_factor") require.NoError(ts.T(), f.SetSecret("secretkey", ts.Config.Security.DBEncryption.Encrypt, ts.Config.Security.DBEncryption.EncryptionKeyID, ts.Config.Security.DBEncryption.EncryptionKey)) require.NoError(ts.T(), ts.API.db.Create(f), "Error saving new test factor") // Create corresponding session @@ -277,7 +277,7 @@ func (ts *MFATestSuite) TestChallengeSMSFactor() { phone := "+1234567" friendlyName := "testchallengesmsfactor" - f := models.NewPhoneFactor(ts.TestUser, phone, friendlyName, models.Phone, models.FactorStateUnverified) + f := models.NewPhoneFactor(ts.TestUser, phone, friendlyName) require.NoError(ts.T(), ts.API.db.Create(f), "Error creating new SMS factor") token := ts.generateAAL1Token(ts.TestUser, &ts.TestSession.ID) @@ -369,7 +369,7 @@ func (ts *MFATestSuite) TestMFAVerifyFactor() { if v.factorType == models.TOTP { friendlyName := uuid.Must(uuid.NewV4()).String() - f = models.NewFactor(ts.TestUser, friendlyName, models.TOTP, models.FactorStateUnverified) + f = models.NewTOTPFactor(ts.TestUser, friendlyName) sharedSecret = ts.TestOTPKey.Secret() f.Secret = sharedSecret require.NoError(ts.T(), ts.API.db.Create(f), "Error updating new test factor") @@ -379,7 +379,7 @@ func (ts *MFATestSuite) TestMFAVerifyFactor() { otp, err := crypto.GenerateOtp(numDigits) require.NoError(ts.T(), err) phone := fmt.Sprintf("+%s", otp) - f = models.NewPhoneFactor(ts.TestUser, phone, friendlyName, models.Phone, models.FactorStateUnverified) + f = models.NewPhoneFactor(ts.TestUser, phone, friendlyName) require.NoError(ts.T(), ts.API.db.Create(f), "Error creating new SMS factor") } diff --git a/internal/models/factor.go b/internal/models/factor.go index 1a618df8e..4776ab972 100644 --- a/internal/models/factor.go +++ b/internal/models/factor.go @@ -148,8 +148,12 @@ func NewFactor(user *User, friendlyName string, factorType string, state FactorS return factor } -func NewPhoneFactor(user *User, phone, friendlyName string, factorType string, state FactorState) *Factor { - factor := NewFactor(user, friendlyName, factorType, state) +func NewTOTPFactor(user *User, friendlyName string) *Factor { + return NewFactor(user, friendlyName, TOTP, FactorStateUnverified) +} + +func NewPhoneFactor(user *User, phone, friendlyName string) *Factor { + factor := NewFactor(user, friendlyName, Phone, FactorStateUnverified) factor.Phone = storage.NullString(phone) return factor } @@ -238,14 +242,6 @@ func (f *Factor) UpdateFactorType(tx *storage.Connection, factorType string) err return tx.UpdateOnly(f, "factor_type", "updated_at") } -func (f *Factor) IsTOTPFactor() bool { - return f.FactorType == TOTP -} - -func (f *Factor) IsPhoneFactor() bool { - return f.FactorType == Phone -} - func (f *Factor) DowngradeSessionsToAAL1(tx *storage.Connection) error { sessions, err := FindSessionsByFactorID(tx, f.ID) if err != nil { diff --git a/internal/models/factor_test.go b/internal/models/factor_test.go index 1ca782ce6..614cff239 100644 --- a/internal/models/factor_test.go +++ b/internal/models/factor_test.go @@ -37,7 +37,7 @@ func (ts *FactorTestSuite) SetupTest() { require.NoError(ts.T(), err) require.NoError(ts.T(), ts.db.Create(user)) - factor := NewFactor(user, "asimplename", TOTP, FactorStateUnverified) + factor := NewTOTPFactor(user, "asimplename") require.NoError(ts.T(), factor.SetSecret("topsecret", false, "", "")) require.NoError(ts.T(), ts.db.Create(factor)) ts.TestFactor = factor