Honour and verify the WebAuthn user handle (user.id) - #764
Open
jnbdz wants to merge 1 commit into
Open
Conversation
Fixes eclipse-vertx#580 Fixes eclipse-vertx#581 createCredentialsOptions() ignored the id of the given user object and always generated a random UUID as user.id, so authenticators could not recognise an existing account and happily created duplicate credentials for the same user. The id is now used when present (validated as a base64url string of 1..64 bytes, per the spec's user handle definition) and only generated when absent. Since the user handle is the stable, non user identifiable, key of an account, it is now carried through the whole flow: - Authenticator gains a userId property (stored by CredentialStorage along with the rest of the record and therefore part of the principal of the authenticated User); - WebAuthn4JCredentials gains a userId property: at registration it is the user.id that was sent to the browser and is stored on the new authenticator; at authentication it is optional and, when set, the credential must belong to that user; - at authentication the userHandle returned in the assertion is checked against the stored userId of the credential (verifying-assertion step 6 of the spec). Authenticators registered before this change have no userId and are not subject to the check, so existing deployments keep working; empty user handles (non discoverable credentials) are ignored. The CredentialStorage interface is unchanged: implementations that want to index by user id can do so from the Authenticator handed to storeCredential.
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.
Fixes #580
Fixes #581
Both issues were filed against the old
vertx-auth-webauthnmodule, but the behaviour carried over tovertx-auth-webauthn4j:WebAuthn4JImpl.createCredentialsOptions()ignored theidof theuserobject and always put a random UUID inuser.id, even though the docs/example describeidas the caller-provided base64url user handle. As a result authenticators cannot recognise an existing account and create duplicate credentials for the same user (#580), and there is no stable id to key authenticators on or to check the assertionuserHandleagainst (#581).#580 —
user.idis honouredcreateCredentialsOptions(user)usesuser.idwhen present; it is validated as a base64url string of 1..64 bytes (the spec's user handle definition) and rejected with a clear error otherwise. When absent, a random UUID is generated as before.#581 — the user handle is carried through the flow
AuthenticatorgainsuserId(base64url). It is stored via the existingCredentialStorage.storeCredentialand, since the principal isauthenticator.toJson(), exposed asuserIdin the authenticatedUser's principal.WebAuthn4JCredentialsgainsuserId:webauthn.create: the relying party passes theuser.idit sent to the browser (same way it passesusername); it is stored on the new authenticator.webauthn.get: optional; when the relying party identified the user before the ceremony, the credential must belong to that user.webauthn.getalso verifies that the assertion'sresponse.userHandle, when non-empty, matches the storeduserIdof the credential (verifying an authentication assertion, step 6). Comparison is on decoded bytes so padded/unpadded encodings are equivalent.Compatibility
userId == nulland are not subject to the check, so existing deployments keep working unchanged; empty user handles (non-discoverable credentials) are ignored.CredentialStorageis not changed (it is@VertxGenand implemented by users). Implementations that want to index by user id can do so from theAuthenticatorhanded tostoreCredential, and for discoverable-credential logins (find(null, credentialId)) the returnedAuthenticatornow carries the owner'suserId. This deviates from the issue's suggestion of adding the id to the storage query, deliberately, to avoid a breaking interface change — happy to revisit if you'd rather extend the interface.AuthenticatorConverter,WebAuthn4JCredentialsConverter).Tests
UserHandleTest(13 tests, offline): given/generated/invalid (non-base64url, empty, >64 bytes)user.id; registration stores the id and exposes it in the principal (with and without id); login with matching / mismatchinguserHandle, matching / mismatching expecteduserId, legacy authenticator withoutuserId, and emptyuserHandle. Existing suite unchanged (the only local failure wasEmulatorTest.testMetadata, which hits the live FIDO MDS endpoint and was returning HTTP 429 to my IP — same onmaster).Docs: registration section of the asciidoc,
WebAuthn4J#createCredentialsOptionsjavadoc and the examples updated to describe the user handle.