diff --git a/internal/api/mfa.go b/internal/api/mfa.go index b5d1368ca..d29abe40d 100644 --- a/internal/api/mfa.go +++ b/internal/api/mfa.go @@ -42,11 +42,6 @@ type TOTPObject struct { URI string `json:"uri,omitempty"` } - -type CredentialOptions[T wbnprotocol.CredentialCreation | wbnprotocol.CredentialAssertion] struct { - PublicKey T `json:"publicKey"` -} - type EnrollFactorResponse struct { ID uuid.UUID `json:"id"` Type string `json:"type"` @@ -70,7 +65,8 @@ type ChallengeFactorResponse struct { ID uuid.UUID `json:"id"` Type string `json:"type"` ExpiresAt int64 `json:"expires_at"` - Options CredentialOptions[T] `json:"options"` + CredentialRequestOptions *wbnprotocol.CredentialAssertion `json:"credential_request_options"` + CredentialCreationOptions *wbnprotocol.CredentialCreation `json:"credential_creation_options"` } type UnenrollFactorResponse struct { @@ -79,7 +75,7 @@ type UnenrollFactorResponse struct { type WebAuthnParams struct { RPID string `json:"rp_id,omitempty"` - // TODO: reconcile this later, revert it back to a string array + // TODO: reconcile this later RPOrigins string `json:"rp_origins,omitempty"` AssertionResponse *wbnprotocol.CredentialAssertionResponse `json:"assertion_response,omitempty"` CreationResponse *wbnprotocol.CredentialCreationResponse `json:"creation_response,omitempty"` @@ -547,10 +543,9 @@ func (a *API) challengeWebAuthnFactor(w http.ResponseWriter, r *http.Request) er challenge = ws.ToChallenge(factor.ID, ipAddress) response = &ChallengeFactorResponse{ + CredentialCreationOptions: options, Type: factor.FactorType, ID: challenge.ID, - Options: CredentialOptions[wbnprotocol.PublicKeyCredentialCreationOptions]{PublicKey: options}, - } } else if factor.IsVerified() { @@ -563,9 +558,9 @@ func (a *API) challengeWebAuthnFactor(w http.ResponseWriter, r *http.Request) er } challenge = ws.ToChallenge(factor.ID, ipAddress) response = &ChallengeFactorResponse{ + CredentialRequestOptions: options, Type: factor.FactorType, ID: challenge.ID, - Options: CredentialOptions[wbnprotocol.PublicKeyCredentialRequestOptions]{PublicKey: options}, } } @@ -586,6 +581,32 @@ func (a *API) challengeWebAuthnFactor(w http.ResponseWriter, r *http.Request) er } +func (a *API) validateChallenge(r *http.Request, db *storage.Connection, factor *models.Factor, challengeID uuid.UUID) (*models.Challenge, error) { + config := a.config + currentIP := utilities.GetIPAddress(r) + + challenge, err := factor.FindChallengeByID(db, challengeID) + if err != nil { + if models.IsNotFoundError(err) { + return nil, notFoundError(ErrorCodeMFAFactorNotFound, "MFA factor with the provided challenge ID not found") + } + return nil, internalServerError("Database error finding Challenge").WithInternalError(err) + } + + if challenge.VerifiedAt != nil || challenge.IPAddress != currentIP { + return nil, unprocessableEntityError(ErrorCodeMFAIPAddressMismatch, "Challenge and verify IP addresses mismatch") + } + + if challenge.HasExpired(config.MFA.ChallengeExpiryDuration) { + if err := db.Destroy(challenge); err != nil { + return nil, internalServerError("Database error deleting challenge").WithInternalError(err) + } + return nil, unprocessableEntityError(ErrorCodeMFAChallengeExpired, "MFA challenge %v has expired, verify against another challenge or create a new challenge.", challenge.ID) + } + + return challenge, nil +} + func (a *API) ChallengeFactor(w http.ResponseWriter, r *http.Request) error { ctx := r.Context() config := a.config @@ -621,25 +642,10 @@ func (a *API) verifyTOTPFactor(w http.ResponseWriter, r *http.Request, params *V factor := getFactor(ctx) config := a.config db := a.db.WithContext(ctx) - currentIP := utilities.GetIPAddress(r) - challenge, err := factor.FindChallengeByID(db, params.ChallengeID) - if err != nil && models.IsNotFoundError(err) { - return notFoundError(ErrorCodeMFAFactorNotFound, "MFA factor with the provided challenge ID not found") - } else if err != nil { - return internalServerError("Database error finding Challenge").WithInternalError(err) - } - - // Ambiguous so as not to leak whether there is a verified challenge - if challenge.VerifiedAt != nil || challenge.IPAddress != currentIP { - return unprocessableEntityError(ErrorCodeMFAIPAddressMismatch, "Challenge and verify IP addresses mismatch") - } - - if challenge.HasExpired(config.MFA.ChallengeExpiryDuration) { - if err := db.Destroy(challenge); err != nil { - return internalServerError("Database error deleting challenge").WithInternalError(err) - } - return unprocessableEntityError(ErrorCodeMFAChallengeExpired, "MFA challenge %v has expired, verify against another challenge or create a new challenge.", challenge.ID) + challenge, err := a.validateChallenge(r, db, factor, params.ChallengeID) + if err != nil { + return err } secret, shouldReEncrypt, err := factor.GetSecret(config.Security.DBEncryption.DecryptionKeys, config.Security.DBEncryption.Encrypt, config.Security.DBEncryption.EncryptionKeyID) @@ -756,25 +762,12 @@ func (a *API) verifyPhoneFactor(w http.ResponseWriter, r *http.Request, params * user := getUser(ctx) factor := getFactor(ctx) db := a.db.WithContext(ctx) - currentIP := utilities.GetIPAddress(r) - challenge, err := factor.FindChallengeByID(db, params.ChallengeID) - if err != nil && models.IsNotFoundError(err) { - return notFoundError(ErrorCodeMFAFactorNotFound, "MFA factor with the provided challenge ID not found") - } else if err != nil { - return internalServerError("Database error finding Challenge").WithInternalError(err) - } - - if challenge.VerifiedAt != nil || challenge.IPAddress != currentIP { - return unprocessableEntityError(ErrorCodeMFAIPAddressMismatch, "Challenge and verify IP addresses mismatch") + challenge, err := a.validateChallenge(r, db, factor, params.ChallengeID) + if err != nil { + return err } - if challenge.HasExpired(config.MFA.ChallengeExpiryDuration) { - if err := db.Destroy(challenge); err != nil { - return internalServerError("Database error deleting challenge").WithInternalError(err) - } - return unprocessableEntityError(ErrorCodeMFAChallengeExpired, "MFA challenge %v has expired, verify against another challenge or create a new challenge.", challenge.ID) - } otpCode, shouldReEncrypt, err := challenge.GetOtpCode(config.Security.DBEncryption.DecryptionKeys, config.Security.DBEncryption.Encrypt, config.Security.DBEncryption.EncryptionKeyID) if err != nil { return internalServerError("Database error verifying MFA TOTP secret").WithInternalError(err) @@ -870,10 +863,14 @@ func (a *API) verifyWebAuthnFactor(w http.ResponseWriter, r *http.Request, param user := getUser(ctx) factor := getFactor(ctx) db := a.db.WithContext(ctx) + var webAuthn *webauthn.WebAuthn var credential *webauthn.Credential - var err error + challenge, err := a.validateChallenge(r, db, factor, params.ChallengeID) + if err != nil { + return err + } switch { case params.WebAuthn == nil: @@ -889,25 +886,6 @@ func (a *API) verifyWebAuthnFactor(w http.ResponseWriter, r *http.Request, param } } - challenge, err := factor.FindChallengeByID(db, params.ChallengeID) - if err != nil && models.IsNotFoundError(err) { - return notFoundError(ErrorCodeMFAFactorNotFound, "MFA factor with the provided challenge ID not found") - } else if err != nil { - return internalServerError("Database error finding Challenge").WithInternalError(err) - } - - // Ambiguous so as not to leak whether there is a verified challenge - if challenge.VerifiedAt != nil || challenge.IPAddress != currentIP { - return unprocessableEntityError(ErrorCodeMFAIPAddressMismatch, "Challenge and verify IP addresses mismatch") - } - - if challenge.HasExpired(config.MFA.ChallengeExpiryDuration) { - if err := db.Destroy(challenge); err != nil { - return internalServerError("Database error deleting challenge").WithInternalError(err) - } - return unprocessableEntityError(ErrorCodeMFAChallengeExpired, "MFA challenge %v has expired, verify against another challenge or create a new challenge.", challenge.ID) - } - // TODO: change so we don't handle pointer webAuthnSession := *challenge.WebAuthnSessionData.SessionData @@ -922,6 +900,8 @@ func (a *API) verifyWebAuthnFactor(w http.ResponseWriter, r *http.Request, param return badRequestError(ErrorCodeValidationFailed, "Invalid credential creation response") } + // Destroy right before use + // TODO: Consider wrapping into function if err := db.Destroy(challenge); err != nil { return internalServerError("Database error deleting challenge").WithInternalError(err) } diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go index bba3bf04f..ceabc37be 100644 --- a/internal/api/mfa_test.go +++ b/internal/api/mfa_test.go @@ -718,8 +718,8 @@ func (ts *MFATestSuite) TestMFAFollowedByPasswordSignIn() { func (ts *MFATestSuite) TestChallengeWebAuthnFactor() { factor := models.NewWebAuthnFactor(ts.TestUser, "WebAuthnfactor") validWebAuthnConfiguration := &WebAuthnParams{ - RPID: "localhost", - RPOrigins: "http://localhost:3000", + RPID: "localhost", + RPOrigins: "http://localhost:3000", } require.NoError(ts.T(), ts.API.db.Create(factor), "Error saving new test factor") token := ts.generateAAL1Token(ts.TestUser, &ts.TestSession.ID) diff --git a/internal/models/user.go b/internal/models/user.go index cdcede319..a289977b0 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -948,7 +948,6 @@ func (user *User) WebAuthnDisplayName() string { return string(user.Email) } - func (user *User) WebAuthnCredentials() []webauthn.Credential { var credentials []webauthn.Credential