feat(protocol): enhance mds errors #341
Conversation
WalkthroughThe changes primarily focus on enhancing the error handling and validation processes within the Changes
Possibly related PRs
Poem
Warning Rate limit exceeded@james-d-elliott has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
protocol/errors.go (2)
43-46: Add a default details message for consistencyUnlike other error definitions in this file,
ErrMetadatahas an empty Details field. Consider adding a default message to maintain consistency and provide immediate context when the error is used without callingWithDetails().ErrMetadata = &Error{ Type: "invalid_metadata", - Details: "", + Details: "Error validating metadata statement", }
43-46: Consider maintaining alphabetical ordering of error definitionsThe new
ErrMetadatais placed betweenErrInvalidAttestationandErrAttestationFormat, breaking the apparent alphabetical ordering of error variables. Consider moving it afterErrInvalidAttestationbut beforeErrNotImplementedto maintain consistent ordering.protocol/metadata.go (3)
22-22: Typographical Error in Error MessageThe word "retrieving" is misspelled as "retreiving" in the error message. Please correct the spelling to improve clarity.
Apply this diff to fix the typo:
- return nil, ErrMetadata.WithInfo(fmt.Sprintf("Failed to validate authenticator metadata for Authenticator Attestation GUID '%s'. Error occurred retreiving the metadata entry: %+v", aaguid, err)) + return nil, ErrMetadata.WithInfo(fmt.Sprintf("Failed to validate authenticator metadata for Authenticator Attestation GUID '%s'. Error occurred retrieving the metadata entry: %+v", aaguid, err))
49-49: Inconsistent Return Value on Attestation Type MismatchWhen the attestation type is not found, the function returns
(entry, ErrMetadata.WithInfo(...)). In other error cases where validation fails, the function returns(nil, error). For consistency and to avoid potential confusion, consider returning(nil, error)when an error occurs.Apply this diff to make the return value consistent:
- return entry, ErrMetadata.WithInfo(fmt.Sprintf("Failed to validate authenticator metadata for Authenticator Attestation GUID '%s'. The attestation type '%s' is not known to be used by this authenticator.", aaguid.String(), attestationType)) + return nil, ErrMetadata.WithInfo(fmt.Sprintf("Failed to validate authenticator metadata for Authenticator Attestation GUID '%s'. The attestation type '%s' is not known to be used by this authenticator.", aaguid.String(), attestationType))
55-55: Inconsistent Return Value on Status Validation ErrorSimilarly, when
mds.ValidateStatusReportsreturns an error, the function returns(entry, error). To maintain consistency across the function, consider returning(nil, error)unless there is a specific reason to return theentryalongside the error.Apply this diff to align the return values:
- return entry, ErrMetadata.WithInfo(fmt.Sprintf("Failed to validate authenticator metadata for Authenticator Attestation GUID '%s'. Error occurred validating the authenticator status: %+v", aaguid, err)) + return nil, ErrMetadata.WithInfo(fmt.Sprintf("Failed to validate authenticator metadata for Authenticator Attestation GUID '%s'. Error occurred validating the authenticator status: %+v", aaguid, err))protocol/attestation.go (1)
190-190: Use the declared context variablectxThe context variable
ctxis declared but not used. Consider usingctxin the call toValidateMetadatafor consistency.Apply this diff to utilize the
ctxvariable:var protoErr *Error ctx := context.Background() - if entry, protoErr = ValidateMetadata(context.Background(), aaguid, attestationType, mds); protoErr != nil { + if entry, protoErr = ValidateMetadata(ctx, aaguid, attestationType, mds); protoErr != nil {webauthn/login.go (1)
320-323: Consider passing a context instead of usingcontext.Background()Using
context.Background()may limit the ability to handle cancellations and timeouts effectively. Consider passing acontext.Contextfrom the caller to allow for better control over the execution flow and resource management.Apply the following changes:
Modify the call to
ValidateMetadata:- if _, protoErr = protocol.ValidateMetadata(context.Background(), aaguid, credential.AttestationType, webauthn.Config.MDS); protoErr != nil { + if _, protoErr = protocol.ValidateMetadata(ctx, aaguid, credential.AttestationType, webauthn.Config.MDS); protoErr != nil {Update the function signature to accept a
context.Context:- func (webauthn *WebAuthn) validateLogin(user User, session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) (*Credential, error) { + func (webauthn *WebAuthn) validateLogin(ctx context.Context, user User, session SessionData, parsedResponse *protocol.ParsedCredentialAssertionData) (*Credential, error) {Ensure that all callers of
validateLoginpass the appropriate context. For example:- return webauthn.validateLogin(user, session, parsedResponse) + return webauthn.validateLogin(ctx, user, session, parsedResponse)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
protocol/attestation.go(1 hunks)protocol/errors.go(1 hunks)protocol/metadata.go(1 hunks)webauthn/login.go(1 hunks)
🔇 Additional comments (2)
protocol/errors.go (1)
43-46: Verify error handling consistency across metadata validation
Let's ensure this new error is properly integrated with appropriate error details where used.
✅ Verification successful
Error handling is consistent but uses WithInfo instead of WithDetails
The error is properly integrated across the codebase in protocol/metadata.go with detailed error messages, but it consistently uses WithInfo() method instead of WithDetails(). All usages provide specific error context including the AAGUID and detailed failure reasons. The error is used in appropriate scenarios:
- When metadata entry retrieval fails
- When authenticator has no registered metadata
- When attestation type validation fails
- When status report validation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how ErrMetadata is used across the codebase
# Expect: Usage in ValidateMetadata with WithDetails() calls
# Check for direct ErrMetadata usage
rg "ErrMetadata" -B 2 -A 2
# Check for "invalid_metadata" type usage
rg "invalid_metadata" -B 2 -A 2
# Look for potential error wrapping patterns
ast-grep --pattern 'WithDetails($$$)'
Length of output: 1982
protocol/metadata.go (1)
14-14: Verify the Handling of Nil Metadata Provider
When mds == nil, the function returns (nil, nil). Ensure that this behavior is intentional and that the calling code correctly handles a nil entry and a nil error. If a nil mds should be considered an error, you might want to return an appropriate error to avoid potential nil pointer dereferences.
Run the following script to check how ValidateMetadata is called in the codebase and whether mds can be nil:
✅ Verification successful
Returning nil, nil for a nil Provider is Correct
The current behavior is intentional and safe. From the code analysis:
- The
webauthn.Config.MDSis explicitly checked fornilbefore callingValidateMetadata - The
ValidateMetadatafunction gracefully handles bothnilprovider andnilentry cases - The metadata validation is an optional feature, as evidenced by the conditional checks in
login.go
The function correctly follows the pattern where a nil provider indicates that metadata validation is disabled, which is a valid configuration state. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to ValidateMetadata and check for nil `mds`.
# Search for all usages of ValidateMetadata
rg 'ValidateMetadata\('
# Check if `mds` is ever passed as `nil`
rg -A2 -B2 'ValidateMetadata\([^,]+,[^,]+,[^,]+,\s*nil\)'
Length of output: 552
Script:
#!/bin/bash
# Let's check how webauthn.Config.MDS is used and initialized
rg -A 5 -B 5 'Config.*MDS'
# Let's also check the metadata.Provider interface definition
ast-grep --pattern 'type Provider interface {
$$$
}'
# And check if there are any tests that verify nil provider behavior
rg -A 5 -B 5 'TestValidateMetadata'
Length of output: 4107
Script:
#!/bin/bash
# Let's check the implementation of ValidateMetadata to understand the nil handling logic
rg -A 20 "func ValidateMetadata"
# Let's also check if there are any tests in protocol package
fd ".*_test.go" protocol/ --exec rg -l "TestValidate"
Length of output: 1340
| if string(atype) == attestationType { | ||
| found = true |
There was a problem hiding this comment.
Possible Type Conversion Issue in Attestation Type Comparison
In the comparison string(atype) == attestationType, there may be a type mismatch. If atype is not inherently a string, converting it using string(atype) might not yield the expected result. Ensure that atype is properly converted to a string that represents the attestation type.
Apply this diff if atype is of a custom type that implements String():
- if string(atype) == attestationType {
+ if atype.String() == attestationType {Alternatively, if attestationType can be converted to the same type as atype, consider:
- if string(atype) == attestationType {
+ if atype == metadata.AttestationType(attestationType) {Please verify that the comparison accurately reflects the intended logic.
Committable suggestion skipped: line range outside the PR's diff.
james-d-elliott
force-pushed
the
feat-enhance-errors
branch
from
bf6866e
to
493a377
Compare
This adds significant improvements to the MDS3 errors helping identify the root cause of a validation error.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
protocol/metadata.go (2)
90-90: Fix spelling error in error messageThe word "agaisnt" is misspelled and should be "against".
- return ErrMetadata.WithDetails(fmt.Sprintf("Failed to validate attestation statement signature during attestation validation for Authenticator Attestation GUID '%s'. The attestation certificate could not be verified due to an error validating the trust chain agaisnt the Metadata Service.", aaguid)) + return ErrMetadata.WithDetails(fmt.Sprintf("Failed to validate attestation statement signature during attestation validation for Authenticator Attestation GUID '%s'. The attestation certificate could not be verified due to an error validating the trust chain against the Metadata Service.", aaguid))🧰 Tools
🪛 golangci-lint (1.62.2)
90-90:
agaisntis a misspelling ofagainst(misspell)
72-91: Consider using error constants for common error messagesThe error messages for certificate validation are quite long and repeated. Consider extracting them into constants for better maintainability and consistency.
Example:
const ( errNoCertificates = "The attestation had no certificates" errCertificateTypeMismatch = "The first certificate in the attestation was type '%T' but '[]byte' was expected" errFullFormatUnsupported = "Attestation was provided in the full format but the authenticator doesn't support the full attestation format" // ... more constants )🧰 Tools
🪛 golangci-lint (1.62.2)
90-90:
agaisntis a misspelling ofagainst(misspell)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
protocol/attestation.go(1 hunks)protocol/errors.go(1 hunks)protocol/metadata.go(2 hunks)webauthn/login.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- protocol/attestation.go
- protocol/errors.go
- webauthn/login.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
protocol/metadata.go
13-13: cyclomatic complexity 21 of func ValidateMetadata is high (> 15)
(gocyclo)
90-90: agaisnt is a misspelling of against
(misspell)
🔇 Additional comments (3)
protocol/metadata.go (3)
Line range hint 23-38: LGTM! Enhanced error messages provide better context
The entry validation logic has been improved with detailed error messages that include the AAGUID, making it easier to diagnose issues.
🧰 Tools
🪛 golangci-lint (1.62.2)
90-90: agaisnt is a misspelling of against
(misspell)
42-48: Ensure safe type conversion in attestation type comparison
The string(atype) conversion might not yield the expected result if atype is not inherently a string.
Consider using the type's String() method or proper type conversion:
- if string(atype) == attestationType {
+ if atype.String() == attestationType {54-59: LGTM! Clear error handling for status validation
The status validation error handling has been improved with detailed context.
james-d-elliott
force-pushed
the
feat-enhance-errors
branch
from
493a377
to
9ac95ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
protocol/metadata.go (2)
96-96: Fix spelling error in error messageThere's a spelling error in the error message: "agaisnt" should be "against".
Apply this diff:
- return ErrMetadata.WithDetails(fmt.Sprintf("Failed to validate attestation statement signature during attestation validation for Authenticator Attestation GUID '%s'. The attestation certificate could not be verified due to an error validating the trust chain agaisnt the Metadata Service.", aaguid)) + return ErrMetadata.WithDetails(fmt.Sprintf("Failed to validate attestation statement signature during attestation validation for Authenticator Attestation GUID '%s'. The attestation certificate could not be verified due to an error validating the trust chain against the Metadata Service.", aaguid))🧰 Tools
🪛 golangci-lint (1.62.2)
96-96:
agaisntis a misspelling ofagainst(misspell)
76-82: Consider additional error handling for certificate parsingWhile the current error handling is good, consider adding validation for the certificate data length before parsing:
if data, ok = x5cs[0].([]byte); !ok { return ErrMetadata.WithDetails(fmt.Sprintf("Failed to parse attestation certificate from x5c during attestation validation for Authenticator Attestation GUID '%s'.", aaguid)).WithInfo(fmt.Sprintf("The first certificate in the attestation was type '%T' but '[]byte' was expected", x5cs[0])) } +if len(data) == 0 { + return ErrMetadata.WithDetails(fmt.Sprintf("Failed to parse attestation certificate from x5c during attestation validation for Authenticator Attestation GUID '%s'.", aaguid)).WithInfo("The certificate data is empty") +} + if x5c, err = x509.ParseCertificate(data); err != nil { return ErrMetadata.WithDetails(fmt.Sprintf("Failed to parse attestation certificate from x5c during attestation validation for Authenticator Attestation GUID '%s'.", aaguid)).WithInfo(fmt.Sprintf("Error returned from x509.ParseCertificate: %+v", err)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
protocol/attestation.go(1 hunks)protocol/errors.go(1 hunks)protocol/metadata.go(2 hunks)webauthn/login.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- protocol/attestation.go
- protocol/errors.go
- webauthn/login.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
protocol/metadata.go
13-13: cyclomatic complexity 23 of func ValidateMetadata is high (> 15)
(gocyclo)
96-96: agaisnt is a misspelling of against
(misspell)
🔇 Additional comments (4)
protocol/metadata.go (4)
Line range hint 23-38: LGTM! Error handling improvements
The error handling has been enhanced with more descriptive messages using ErrMetadata.WithInfo. The special case handling for zero AAGUID is well-implemented.
🧰 Tools
🪛 golangci-lint (1.62.2)
96-96: agaisnt is a misspelling of against
(misspell)
42-48: Possible Type Conversion Issue in Attestation Type Comparison
The comparison string(atype) == attestationType might not yield the expected result if atype is not inherently a string type.
Let's verify the type definition:
#!/bin/bash
# Search for AttestationType definition
ast-grep --pattern 'type AttestationType'54-59: LGTM! Clear status validation
The status validation logic is well-structured with proper error handling and descriptive messages.
13-21: 🛠️ Refactor suggestion
Consider decomposing the function to reduce complexity
The function's cyclomatic complexity has increased to 23 (> 15), making it harder to maintain and test. Consider breaking it down into smaller, focused functions.
Consider splitting into these functions:
func validateEntry(ctx context.Context, mds metadata.Provider, aaguid uuid.UUID) *Error
func validateAttestationType(ctx context.Context, entry *metadata.Entry, attestationType string) *Error
func validateStatus(ctx context.Context, mds metadata.Provider, entry *metadata.Entry) *Error
func validateTrustAnchor(ctx context.Context, mds metadata.Provider, entry *metadata.Entry, attestationType string, x5cs []any) *Error🧰 Tools
🪛 golangci-lint (1.62.2)
13-13: cyclomatic complexity 23 of func ValidateMetadata is high (> 15)
(gocyclo)
This adds significant improvements to the MDS3 errors helping identify the root cause of a validation error.