Skip to content

Commit

Permalink
fix: put challenge into function
Browse files Browse the repository at this point in the history
  • Loading branch information
@J0
J0 committed Sep 28, 2024
1 parent c5df5d6 commit bee17da
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 67 deletions.
108 changes: 44 additions & 64 deletions internal/api/mfa.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand All @@ -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 {
Expand All @@ -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"`
Expand Down Expand Up @@ -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() {
Expand All @@ -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},
}

}
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand All @@ -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

Expand All @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions internal/api/mfa_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion internal/models/user.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,6 @@ func (user *User) WebAuthnDisplayName() string {
return string(user.Email)
}


func (user *User) WebAuthnCredentials() []webauthn.Credential {
var credentials []webauthn.Credential

Expand Down

0 comments on commit bee17da

Please sign in to comment.