Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Set pinProtocol for UV tokens when supported #197

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BryanJacobs
Copy link

Let's say we have the following situation:

  • A CTAP authenticator supports PIN protocols 1 and 2
  • The authenticator currently does not have a PIN set
  • The user makes a request for a credential with the hmac-secret extension

Because of some... let's call it interesting design... in the way extensions are handled, the user will pass HmacSecretExtension (the type) to the client. They won't pass an INSTANCE of HmacSecretExtension. This means pin_protocol on the extension object will always be null.

The extension currently contains code that chooses the PIN protocol based on what the authenticator supports. So if the authenticator supports PIN protocol 2, the extension will use that.

But back up in the CTAP2 Client, we didn't choose a PIN protocol. Net effect:

  1. HMAC secrets use PIN protocol 2, with 16-byte IVs (okay, sure, the authenticator supports it)
  2. getAssertion call is made with pinUvAuthParam unset (good)
  3. getAssertion call is made with pinProtool unset (BAD - this causes a default of PIN protocol version 1)

There's a circular dependency in how the extensions are set up right now, where the decision of what PIN protocol to use depends on the permissions coming back from the extensions, but the extensions also need to know the PIN protocol.

My chosen solution is simple: if the authenticator supports UV tokens, always pass it the PIN protocol explicitly - even when not doing PIN auth. This doesn't break any of the existing tests in the repository, but does repair the untested path where the hmac-secret extension is being used without a PIN set on a PIN-protocol-2-supporting authenticator.

This avoids situations where the authenticator is being used with
extensions that require a pinProtocol, and the pinProtocol defaults to
1.
@dainnilsson
Copy link
Member

Was this resolved by the changes in 1.1.2, or is this still an issue? Since the extension itself has a field for pinProtocol version in the authenticator input, I don't see any issue with not having it set for the outer getAssertion call.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants