Skip to content

fix(protocol): conditional create fail #434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(protocol): conditional create fail #434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions protocol/assertion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (car CredentialAssertionResponse) Parse() (par *ParsedCredentialAssertionDa
// documentation.
//
// Specification: §7.2 Verifying an Authentication Assertion (https://www.w3.org/TR/webauthn/#sctn-verifying-assertion)
func (p *ParsedCredentialAssertionData) Verify(storedChallenge string, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, appID string, verifyUser bool, credentialBytes []byte) error {
func (p *ParsedCredentialAssertionData) Verify(storedChallenge string, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, appID string, verifyUser bool, verifyUserPresence bool, credentialBytes []byte) error {
// Steps 4 through 6 in verifying the assertion data (https://www.w3.org/TR/webauthn/#verifying-assertion) are
// "assertive" steps, i.e. "Let JSONtext be the result of running UTF-8 decode on the value of cData."
// We handle these steps in part as we verify but also beforehand
Expand All @@ -160,7 +160,7 @@ func (p *ParsedCredentialAssertionData) Verify(storedChallenge string, relyingPa
}

// Handle steps 11 through 14, verifying the authenticator data.
validError = p.Response.AuthenticatorData.Verify(rpIDHash[:], appIDHash[:], verifyUser)
validError = p.Response.AuthenticatorData.Verify(rpIDHash[:], appIDHash[:], verifyUser, verifyUserPresence)
if validError != nil {
return validError
}
Expand Down
2 changes: 1 addition & 1 deletion protocol/assertion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func TestParsedCredentialAssertionData_Verify(t *testing.T) {
Raw: tt.fields.Raw,
}

if err := p.Verify(tt.args.storedChallenge.String(), tt.args.relyingPartyID, tt.args.relyingPartyOrigin, nil, TopOriginIgnoreVerificationMode, "", tt.args.verifyUser, tt.args.credentialBytes); (err != nil) != tt.wantErr {
if err := p.Verify(tt.args.storedChallenge.String(), tt.args.relyingPartyID, tt.args.relyingPartyOrigin, nil, TopOriginIgnoreVerificationMode, "", tt.args.verifyUser, false, tt.args.credentialBytes); (err != nil) != tt.wantErr {
t.Errorf("ParsedCredentialAssertionData.Verify() error = %v, wantErr %v", err, tt.wantErr)
}
})
Expand Down
4 changes: 2 additions & 2 deletions protocol/attestation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,11 @@ func (ccr *AuthenticatorAttestationResponse) Parse() (p *ParsedAttestationRespon
//
// Steps 13 through 15 are verified against the auth data. These steps are identical to 15 through 18 for assertion so we
// handle them with AuthData.
func (a *AttestationObject) Verify(relyingPartyID string, clientDataHash []byte, userVerificationRequired bool, mds metadata.Provider, credParams []CredentialParameter) (err error) {
func (a *AttestationObject) Verify(relyingPartyID string, clientDataHash []byte, userVerificationRequired bool, userPresenceRequired bool, mds metadata.Provider, credParams []CredentialParameter) (err error) {
rpIDHash := sha256.Sum256([]byte(relyingPartyID))

// Begin Step 13 through 15. Verify that the rpIdHash in authData is the SHA-256 hash of the RP ID expected by the RP.
if err = a.AuthData.Verify(rpIDHash[:], nil, userVerificationRequired); err != nil {
if err = a.AuthData.Verify(rpIDHash[:], nil, userVerificationRequired, userPresenceRequired); err != nil {
return err
}

Expand Down
2 changes: 1 addition & 1 deletion protocol/attestation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func TestAttestationVerify(t *testing.T) {

pcc.Response = *parsedAttestationResponse

_, err = pcc.Verify(options.Response.Challenge.String(), false, options.Response.RelyingParty.ID, []string{options.Response.RelyingParty.Name}, nil, TopOriginIgnoreVerificationMode, nil, options.Response.Parameters)
_, err = pcc.Verify(options.Response.Challenge.String(), false, false, options.Response.RelyingParty.ID, []string{options.Response.RelyingParty.Name}, nil, TopOriginIgnoreVerificationMode, nil, options.Response.Parameters)

require.NoError(t, err)
})
Expand Down
10 changes: 5 additions & 5 deletions protocol/authenticator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ func ResidentKeyNotRequired() *bool {

// Verify on AuthenticatorData handles Steps 13 through 15 & 17 for Registration
// and Steps 15 through 18 for Assertion.
func (a *AuthenticatorData) Verify(rpIdHash []byte, appIDHash []byte, userVerificationRequired bool) error {
func (a *AuthenticatorData) Verify(rpIdHash []byte, appIDHash []byte, userVerificationRequired bool, userPresenceRequired bool) error {

// Registration Step 13 & Assertion Step 15
// Verify that the RP ID hash in authData is indeed the SHA-256
Expand All @@ -394,17 +394,17 @@ func (a *AuthenticatorData) Verify(rpIdHash []byte, appIDHash []byte, userVerifi
return ErrVerification.WithInfo(fmt.Sprintf("RP Hash mismatch. Expected %x and Received %x", a.RPIDHash, rpIdHash))
}

// Registration Step 14 & Assertion Step 16
// Registration Step 15 & Assertion Step 16
// Verify that the User Present bit of the flags in authData is set.
if !a.Flags.UserPresent() {
return ErrVerification.WithInfo(fmt.Sprintf("User presence flag not set by authenticator"))
if userPresenceRequired && !a.Flags.UserPresent() {
return ErrVerification.WithInfo("User presence required but flag not set by authenticator")
}

// Registration Step 15 & Assertion Step 17
// If user verification is required for this assertion, verify that
// the User Verified bit of the flags in authData is set.
if userVerificationRequired && !a.Flags.UserVerified() {
return ErrVerification.WithInfo(fmt.Sprintf("User verification required but flag not set by authenticator"))
return ErrVerification.WithInfo("User verification required but flag not set by authenticator")
}

// Registration Step 17 & Assertion Step 18
Expand Down
9 changes: 6 additions & 3 deletions protocol/authenticator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,7 @@ func TestAuthenticatorData_Verify(t *testing.T) {
type args struct {
rpIdHash []byte
userVerificationRequired bool
userPresenceRequired bool
}

tests := []struct {
Expand Down Expand Up @@ -473,12 +474,13 @@ func TestAuthenticatorData_Verify(t *testing.T) {
Flags: AuthenticatorFlags(0x04),
},
args: args{
rpIdHash: []byte{1, 2, 3},
rpIdHash: []byte{1, 2, 3},
userPresenceRequired: true,
},
errString: "Error validating the authenticator response",
errType: "verification_error",
errDetails: "Error validating the authenticator response",
errInfo: "User presence flag not set by authenticator",
errInfo: "User presence required but flag not set by authenticator",
},
{
name: "User verification required",
Expand All @@ -489,6 +491,7 @@ func TestAuthenticatorData_Verify(t *testing.T) {
args: args{
rpIdHash: []byte{1, 2, 3},
userVerificationRequired: true,
userPresenceRequired: true,
},
errString: "Error validating the authenticator response",
errType: "verification_error",
Expand All @@ -506,7 +509,7 @@ func TestAuthenticatorData_Verify(t *testing.T) {
AttData: tt.fields.AttData,
ExtData: tt.fields.ExtData,
}
err := a.Verify(tt.args.rpIdHash, nil, tt.args.userVerificationRequired)
err := a.Verify(tt.args.rpIdHash, nil, tt.args.userVerificationRequired, tt.args.userPresenceRequired)
if tt.errString != "" {
assert.EqualError(t, err, tt.errString)

Expand Down
4 changes: 2 additions & 2 deletions protocol/credential.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func (ccr CredentialCreationResponse) Parse() (pcc *ParsedCredentialCreationData
// Verify the Client and Attestation data.
//
// Specification: §7.1. Registering a New Credential (https://www.w3.org/TR/webauthn/#sctn-registering-a-new-credential)
func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter) (clientDataHash []byte, err error) {
func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, verifyUserPresence bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter) (clientDataHash []byte, err error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider parameter placement for less disruptive API changes.

Adding the new verifyUserPresence parameter in the middle of the parameter list creates a more disruptive breaking change than necessary. Consider placing new parameters at the end of the parameter list in future API changes to minimize disruption.

Additionally, the method documentation should be updated to explain the new parameter's purpose and its relationship to the WebAuthn Level 3 conditional mediation behavior.

-func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, verifyUserPresence bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter) (clientDataHash []byte, err error) {
+func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter, verifyUserPresence bool) (clientDataHash []byte, err error) {

Update the method documentation:

 // Verify the Client and Attestation data.
+// The verifyUserPresence parameter controls whether the User Present (UP) flag is verified,
+// allowing conditional mediation behavior as per WebAuthn Level 3 specification.
 //
 // Specification: §7.1. Registering a New Credential (https://www.w3.org/TR/webauthn/#sctn-registering-a-new-credential)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, verifyUserPresence bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter) (clientDataHash []byte, err error) {
// Verify the Client and Attestation data.
// The verifyUserPresence parameter controls whether the User Present (UP) flag is verified,
// allowing conditional mediation behavior as per WebAuthn Level 3 specification.
//
// Specification: §7.1. Registering a New Credential (https://www.w3.org/TR/webauthn/#sctn-registering-a-new-credential)
func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUser bool, relyingPartyID string, rpOrigins, rpTopOrigins []string, rpTopOriginsVerify TopOriginVerificationMode, mds metadata.Provider, credParams []CredentialParameter, verifyUserPresence bool) (clientDataHash []byte, err error) {
// existing implementation...
}
🤖 Prompt for AI Agents 
In protocol/credential.go at line 148, the new parameter verifyUserPresence is
inserted in the middle of the Verify method's parameter list, causing a
disruptive API change. To fix this, move verifyUserPresence to the end of the
parameter list to minimize breaking changes. Also, update the method's
documentation to describe the purpose of verifyUserPresence and how it relates
to WebAuthn Level 3 conditional mediation behavior.

// Handles steps 3 through 6 - Verifying the Client Data against the Relying Party's stored data
if err = pcc.Response.CollectedClientData.Verify(storedChallenge, CreateCeremony, rpOrigins, rpTopOrigins, rpTopOriginsVerify); err != nil {
return nil, err
Expand All @@ -161,7 +161,7 @@ func (pcc *ParsedCredentialCreationData) Verify(storedChallenge string, verifyUs

// We do the above step while parsing and decoding the CredentialCreationResponse
// Handle steps 9 through 14 - This verifies the attestation object.
if err = pcc.Response.AttestationObject.Verify(relyingPartyID, clientDataHash, verifyUser, mds, credParams); err != nil {
if err = pcc.Response.AttestationObject.Verify(relyingPartyID, clientDataHash, verifyUser, verifyUserPresence, mds, credParams); err != nil {
return clientDataHash, err
}

Expand Down
2 changes: 1 addition & 1 deletion protocol/credential_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ func TestParsedCredentialCreationData_Verify(t *testing.T) {
Response: tt.fields.Response,
Raw: tt.fields.Raw,
}
if _, err := pcc.Verify(tt.args.storedChallenge.String(), tt.args.verifyUser, tt.args.relyingPartyID, tt.args.relyingPartyOrigin, nil, TopOriginIgnoreVerificationMode, nil, tt.args.credParams); (err != nil) != tt.wantErr {
if _, err := pcc.Verify(tt.args.storedChallenge.String(), tt.args.verifyUser, false, tt.args.relyingPartyID, tt.args.relyingPartyOrigin, nil, TopOriginIgnoreVerificationMode, nil, tt.args.credParams); (err != nil) != tt.wantErr {
t.Errorf("ParsedCredentialCreationData.Verify() error = %+v, wantErr %v", err, tt.wantErr)
}
})
Expand Down
3 changes: 2 additions & 1 deletion webauthn/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedRe
}

shouldVerifyUser := session.UserVerification == protocol.VerificationRequired
shouldVerifyUserPresence := true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implement conditional user presence verification logic.

The hardcoded shouldVerifyUserPresence := true doesn't align with the WebAuthn Level 3 specification and the fix described in the PR objectives. According to the spec, user presence should only be verified when mediation is not "conditional".

Apply this diff to implement proper conditional logic:

-	shouldVerifyUserPresence := true
+	shouldVerifyUserPresence := session.Mediation != protocol.MediationConditional

This matches the implementation in webauthn/registration.go line 235 and ensures consistency across login and registration flows.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
shouldVerifyUserPresence := true
- shouldVerifyUserPresence := true
+ shouldVerifyUserPresence := session.Mediation != protocol.MediationConditional
🤖 Prompt for AI Agents 
In webauthn/login.go at line 360, replace the hardcoded assignment of
shouldVerifyUserPresence to true with conditional logic that sets it to true
only when the mediation value is not "conditional". This aligns with the
WebAuthn Level 3 specification and matches the approach used in
webauthn/registration.go at line 235, ensuring consistent user presence
verification behavior between login and registration flows.


rpID := webauthn.Config.RPID
rpOrigins := webauthn.Config.RPOrigins
Expand All @@ -367,7 +368,7 @@ func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedRe
}

// Handle steps 4 through 16.
if err = parsedResponse.Verify(session.Challenge, rpID, rpOrigins, rpTopOrigins, webauthn.Config.RPTopOriginVerificationMode, appID, shouldVerifyUser, credential.PublicKey); err != nil {
if err = parsedResponse.Verify(session.Challenge, rpID, rpOrigins, rpTopOrigins, webauthn.Config.RPTopOriginVerificationMode, appID, shouldVerifyUser, shouldVerifyUserPresence, credential.PublicKey); err != nil {
return nil, err
}

Expand Down
4 changes: 3 additions & 1 deletion webauthn/registration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ func (webauthn *WebAuthn) BeginMediatedRegistration(user User, mediation protoco
UserID: user.WebAuthnID(),
UserVerification: creation.Response.AuthenticatorSelection.UserVerification,
CredParams: creation.Response.Parameters,
Mediation: creation.Mediation,
}

if webauthn.Config.Timeouts.Registration.Enforce {
Expand Down Expand Up @@ -231,10 +232,11 @@ func (webauthn *WebAuthn) CreateCredential(user User, session SessionData, parse
}

shouldVerifyUser := session.UserVerification == protocol.VerificationRequired
shouldVerifyUserPresence := session.Mediation != protocol.MediationConditional

var clientDataHash []byte

if clientDataHash, err = parsedResponse.Verify(session.Challenge, shouldVerifyUser, webauthn.Config.RPID, webauthn.Config.RPOrigins, webauthn.Config.RPTopOrigins, webauthn.Config.RPTopOriginVerificationMode, webauthn.Config.MDS, session.CredParams); err != nil {
if clientDataHash, err = parsedResponse.Verify(session.Challenge, shouldVerifyUser, shouldVerifyUserPresence, webauthn.Config.RPID, webauthn.Config.RPOrigins, webauthn.Config.RPTopOrigins, webauthn.Config.RPTopOriginVerificationMode, webauthn.Config.MDS, session.CredParams); err != nil {
return nil, err
}

Expand Down
7 changes: 4 additions & 3 deletions webauthn/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,8 @@ type SessionData struct {
AllowedCredentialIDs [][]byte `json:"allowed_credentials,omitempty"`
Expires time.Time `json:"expires"`

UserVerification protocol.UserVerificationRequirement `json:"userVerification"`
Extensions protocol.AuthenticationExtensions `json:"extensions,omitempty"`
CredParams []protocol.CredentialParameter `json:"credParams,omitempty"`
UserVerification protocol.UserVerificationRequirement `json:"userVerification"`
Extensions protocol.AuthenticationExtensions `json:"extensions,omitempty"`
CredParams []protocol.CredentialParameter `json:"credParams,omitempty"`
Mediation protocol.CredentialMediationRequirement `json:"mediation,omitempty"`
}