fix(webauthn): advertise every algorithm the FIDO specification requires - #203
Merged
Conversation
Registration took the SimpleWebAuthn default of [-8, -7, -257], which omits RS1. FIDO Server Requirements v2.3 requires RS1, RS256, ES256 and EdDSA, so a conformance run would have flagged it. The set is now explicit and ordered by preference. pubKeyCredParams is an ordered list, and RS1 is RSASSA-PKCS1-v1_5 with SHA-1, so it sits last: offered because the specification requires support for it, ordered so nothing picks it while a better option is available. Verification is pinned to the same set. It previously defaulted to every algorithm the library knows, which accepted credentials using algorithms this server never advertised. Closes #166
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #166. No cross-repo work, no release round trip.
The gap
generateRegistrationOptionsnever setsupportedAlgorithmIDs, so theSimpleWebAuthn default applied:
FIDO Server Requirements v2.3 requires a server to implement
RS1,RS256,ES256andEdDSA. RS1 was missing, and a conformance run would have flaggedit.
Ordering is part of the fix, not decoration
pubKeyCredParamsis an ordered preference list, so where RS1 sits mattersas much as whether it is there. RS1 is RSASSA-PKCS1-v1_5 with SHA-1:
Advertised because the specification requires support for it, ordered last so no
authenticator with a better option available will choose it. Both facts are
asserted, including a separate assertion that RS1 is specifically the last entry
rather than merely present.
A second gap found while fixing the first
verifyRegistrationResponsetakes the same option, and its default is everyalgorithm the library knows. So the server was accepting credentials using
algorithms it never advertised. Verification is now pinned to the same constant,
so what is offered and what is accepted cannot drift apart.
Why state the set at all
Leaving it to a library default means a minor upgrade can silently change what
this server advertises. That is how RS1 came to be missing in the first place.
The test pins the exact array, so the next upgrade that changes it fails loudly.
Mutation checked
Removing RS1 from the constant fails exactly the two new tests and nothing else,
so they are real guards rather than assertions that happen to pass.
Verification
92 test files, 1012 passed, 1 skipped. Coverage 98.78% lines.
typecheck,lint,format:checkclean. Contract regenerated, no change to it: thealgorithm set is inside the generated credential options rather than in the
documented schema.