fix(webauthn): ask for exactly the user verification that will be enforced - #204
Merged
Merged
Conversation
…orced Registration advertised userVerification 'preferred', telling the authenticator verification was optional, then rejected a response that skipped it, because SimpleWebAuthn's requireUserVerification defaults to true and this server never set it. A user on an authenticator that skips verification completed the whole ceremony and failed at the last step, having never been asked to verify. Authentication separately asked for 'required', so the two halves of the same product disagreed. authenticator_policy.userVerification now drives both the ask and the enforcement, on registration and authentication, from one value. Defaults to required, which does not change what is accepted, since that was already enforced. It changes what is asked for. Step-up deliberately keeps requiring verification regardless of the policy. It exists to re-verify the human, and without verification it is a second signature from a key the session already proved it holds. Its verification is now pinned explicitly rather than relying on the same library default. Closes #167
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 #167. Requires
@seamless-auth/types0.13.0, already released. FinishesSession 4 of #155.
The issue's premise needs one correction
#167 says a credential enrolled without user verification "can be created and can
then never be used to sign in." That is not what happens, and the truth is worse
in a more visible way.
requireUserVerificationdefaults totrueon both SimpleWebAuthn verifiers,and this server never set it. So registration was:
userVerification: 'preferred', telling the authenticatorverification was optional
required, via the library defaultA user on an authenticator that skips verification therefore completed the entire
ceremony, tapped the key, answered the prompt, and was rejected at the last step,
having never been asked to verify in the first place. The credential is not
created-then-unusable; the enrolment is wasted with a confusing failure.
Authentication separately asked for
required, so the two halves of the sameproduct also disagreed with each other.
The change
authenticator_policy.userVerificationdrives the ask and the enforcement,on registration and authentication. One value, so they cannot drift.
The default of
requireddoes not change what is accepted, because that wasalready enforced. It changes what is asked for, so an authenticator is told to
verify rather than allowed to skip and be rejected afterwards.
One judgement call, flagged for review
Step-up deliberately still requires verification regardless of the policy.
Step-up exists to re-verify the human. Without verification it is a second
signature from a key the session already proved it holds, which proves nothing
the session did not already prove. A deployment that relaxes verification
generally should still get a real check when elevating.
The counter-argument is that a deployment on authenticators that cannot verify
would find step-up unusable. I judged a step-up that cannot verify to be worse
than one that is unavailable, but this is a product call rather than a technical
one and I would change it on request. Its verification is now pinned explicitly
rather than relying on the same library default that caused this issue.
Verification
92 test files, 1016 passed, 1 skipped. Coverage 98.78% lines.
typecheck,lint,format:checkclean. Contract regenerated, documented inconfiguration.mdand.env.example.Four new tests: registration asks for what it enforces, a relaxed deployment is
followed, and the policy reaches both verifiers in both directions.
Note on the test diff
Nine
getSystemConfigstubs gained the policy field. They stub config wholesaleand so bypass the schema default, producing a state production cannot reach. I
completed the stubs rather than adding a defensive read in the controller,
because the schema guarantees the field and a fallback would be dead code that
hides a real misconfiguration.
Unrelated flake, noted
One run failed on an unrelated PRF assertion in the same file. It passed on the
next five consecutive runs of that spec in isolation, and the full sequential
suite is green. That is #190, not this change.