fix(webauthn): skip mds validation for none format #497
Conversation
|
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 15 minutes and 2 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. 📒 Files selected for processing (1)
WalkthroughAdds attestationFormat to protocol.ValidateMetadata and updates callers in attestation verification and login. Introduces an early return in metadata validation when attestation format is none. Minor comment correction in login.go. No other control-flow or error-handling changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant RP as RelyingParty
participant Login as validateLogin
participant Proto as protocol.ValidateMetadata
participant MDS as metadata.Provider
RP->>Login: Login request (credential)
Login->>Proto: ValidateMetadata(ctx, mds, aaguid, attestationType="", attestationFormat, x5cs=nil)
alt attestationFormat == "none"
Proto-->>Login: return nil (skip)
else attestationFormat != "none"
Proto->>MDS: Lookup AAGUID / entries
MDS-->>Proto: Metadata entries
Proto-->>Login: Validation result
end
Login-->>RP: Continue login flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
james-d-elliott
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #497 +/- ##
==========================================
- Coverage 45.01% 44.97% -0.05%
==========================================
Files 37 37
Lines 3328 3331 +3
==========================================
Hits 1498 1498
- Misses 1625 1628 +3
Partials 205 205
🚀 New features to boost your workflow:
|
james-d-elliott
force-pushed
the
fix-att-format-none
branch
from
4d4b7ba to
d66415b
Compare
The attestation format none does not include attestation data and therefore cannot be validated against MDS3 metadata. Previously the login flow attempted to validate it, which caused unnecessary failures for authenticators using the none format. This change ensures that none is explicitly exempt from MDS3 validation, allowing logins with such authenticators to succeed as expected. The registration flow already implements this measure. Fixes #387
james-d-elliott
force-pushed
the
fix-att-format-none
branch
from
d66415b to
b2ac688
Compare
![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
🧹 Nitpick comments (4)
protocol/attestation.go (1)
213-219: Consider threading through a caller-supplied context instead of context.Background().Not urgent, but using
context.Background()here prevents upstream request cancellation/timeouts from propagating into MDS calls. If feasible, extend the API to accept acontext.Contextand pass it down.protocol/metadata.go (2)
27-27: Typo in user-facing error message ("retreiving" → "retrieving").Minor but user-visible in logs; worth correcting:
- return ErrMetadata.WithInfo(fmt.Sprintf("Failed to validate authenticator metadata for Authenticator Attestation GUID '%s'. Error occurred retreiving the metadata entry: %+v", aaguid, err)) + return ErrMetadata.WithInfo(fmt.Sprintf("Failed to validate authenticator metadata for Authenticator Attestation GUID '%s'. Error occurred retrieving the metadata entry: %+v", aaguid, err))
13-21: Document parameter semantics to avoid future misuse.Given two adjacent
stringparams whose meanings are easy to mix up, add a brief doc comment clarifying expected values (e.g., attestationType: "basic|self|attca|anonca|none"; attestationFormat: "packed|tpm|android-key|android-safetynet|fido-u2f|apple|none"). This will reduce future footguns.webauthn/login.go (1)
354-364: Minor readability: redundant assignment to found=false in loop.
foundis already false before the loop; resetting it on each non-match is unnecessary and slightly obscures intent.- for _, credential = range credentials { - if bytes.Equal(credential.ID, parsedResponse.RawID) { - found = true - break - } - found = false - } + for _, credential = range credentials { + if bytes.Equal(credential.ID, parsedResponse.RawID) { + found = true + break + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
protocol/attestation.go(1 hunks)protocol/metadata.go(1 hunks)webauthn/login.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
protocol/metadata.go (1)
protocol/options.go (2)
AttestationFormat(194-194)AttestationFormatNone(224-224)
protocol/attestation.go (1)
protocol/metadata.go (1)
ValidateMetadata(13-110)
webauthn/login.go (2)
protocol/metadata.go (1)
ValidateMetadata(13-110)webauthn/types.go (1)
Config(29-76)
🔇 Additional comments (4)
protocol/attestation.go (1)
217-219: Passes attestation format into metadata validation — aligns with intent to gate on "none".Good call to include
a.Formatwhen invokingValidateMetadata. This enables the callee to short-circuit for the "none" case and keeps attestation-type checks orthogonal to attestation-format handling.protocol/metadata.go (1)
1-110: ValidateMetadata signature change acknowledged – internal updates completeThe
ValidateMetadatafunction signature inprotocol/metadata.gowas modified to:func ValidateMetadata( ctx context.Context, mds metadata.Provider, aaguid uuid.UUID, attestationType string, attestationFormat string, x5cs []any, ) *ErrorAll internal call sites have been updated to match the new parameter order:
webauthn/login.go:385
protocol.ValidateMetadata(context.Background(), webauthn.Config.MDS, aaguid, "", credential.AttestationType, nil)protocol/attestation.go:217
ValidateMetadata(context.Background(), mds, aaguid, attestationType, a.Format, x5cs)No further code changes are required within this codebase. However, because this is a public API break, please:
• Add a clear note to the upcoming release’s breaking changes section, calling out the updated signature and its parameter order.
• Verify any downstream consumers (external projects or clients) update their call sites accordingly.webauthn/login.go (2)
375-375: Grammar nit fixed.“are not used” reads better. Thanks for tightening the comment.
376-388: Security posture check: intentionally skipping MDS on "none".This change prevents status checks for “none” credentials as well (revocations, compromised reports). That matches the issue’s objective and registration behavior, but do verify this is acceptable for your deployment and threat model.
Would you like a targeted unit test that ensures:
- login skips MDS when attestation is “none” and succeeds, and
- login still enforces MDS status for non-“none” credentials?
I can draft it using a stubbedmetadata.Provider.
The attestation format none does not include attestation data and therefore cannot be validated against MDS3 metadata. Previously the login flow attempted to validate it, which caused unnecessary failures for authenticators using the none format. This change ensures that none is explicitly exempt from MDS3 validation, allowing logins with such authenticators to succeed as expected. The registration flow already implements this measure.
Fixes #387