Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Acceptable UV caching time affordance extension added #2021

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rlin1
Copy link
Contributor

@rlin1 rlin1 commented Feb 12, 2024

The current behavior of some Passkey Providers to “cache” the user verification status cannot be modeled in FIDO/WebAuthn today and hence might surprise relying parties.

This PR adds the user verification extension that allows the RP to indicate its willingness to accept cached user verification for a given time.


Preview | Diff

@timcappalli
Copy link
Member

@rlin1 we talked about this extensively at the WebAuthn F2F last year. The direction we were discussing was adding an ability for an authenticator to convey when it last performed UV and the authenticator would be truthful in its UV response in authData. This would provide the balance of an RP knowing the UV response is truthful along with some additional context, and the client and/or provider being able to make the best decision based on context they know.

This extension could potentially be part of that solution, but it is unclear whether the authenticator would reply truthfully about UV for the assertion or would lie based on the caching being acceptable to the RP.

@rlin1
Copy link
Contributor Author

rlin1 commented Feb 12, 2024

With this approach, the maxUVC included in the authenticator output could be defined differently to be less or equal to the maxUVC as provided by the RP - so that the authenticator could set it to the elapsed time.
The Authenticator today already indicates whether UV was performed. The current expectation is that this is fresh user verification. I think it would be good to let the RP indicate whether UV caching is acceptable for the specific use case.

@rlin1 rlin1 self-assigned this Feb 12, 2024
@yackermann yackermann changed the title user verification caching etension added user verification caching extension added Feb 13, 2024
@rlin1
Copy link
Contributor Author

rlin1 commented Feb 14, 2024

Close #2023

@rlin1 rlin1 linked an issue Feb 14, 2024 that may be closed by this pull request
@rlin1
Copy link
Contributor Author

rlin1 commented Feb 14, 2024

Should this extension cover userPresence as well? If yes: we might want to rename it to userGestureCaching...

@emlun emlun self-requested a review February 14, 2024 19:18
@nadalin nadalin requested a review from agl February 14, 2024 19:21
@nadalin nadalin added the @Risk Items that are at risk for L3 label Feb 14, 2024
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Comment on lines 9255 to 9270
"RFC9052": {
"authors": ["Jim Schaad"],
"title": "CBOR Object Signing and Encryption (COSE): Structures and Process",
"href": "https://datatracker.ietf.org/doc/rfc9052/",
"status": "IETF Internet Standard",
"date": "August 2022"
},

"RFC9053": {
"authors": ["Jim Schaad"],
"title": "CBOR Object Signing and Encryption (COSE): Initial Algorithms",
"href": "https://datatracker.ietf.org/doc/rfc9053/",
"status": "RFC Informational",
"date": "August 2022"
},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These don't seem to be referenced anywhere in the added text; was this added by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was required to make bikeshed run successfully

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you running Bikeshed? You may need to run bikeshed update to update your snapshot of the bibliography index.

index.bs Outdated Show resolved Hide resolved
@nadalin nadalin added this to the Futures (catch-all) milestone Feb 14, 2024
@emlun
Copy link
Member

emlun commented Mar 19, 2024

I force-pushed the branch to remove some unrelated whitespace changes from the commit history; you may need to hard-reset any local clones to the new HEAD commit.

:: The maxTimeSinceLastUV denotes the maximum acceptable number of milliseconds elapsed since the last time the user was successfully verified.
<xmp class="idl">
partial dictionary AuthenticationExtensionsClientInputs {
unsigned long long maxTimeSinceLastUV;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This member name needs to be the same as the extension identifier.

Suggested change
unsigned long long maxTimeSinceLastUV;
unsigned long long userVerificationCaching;

Copy link
Member

@timcappalli timcappalli Mar 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should long be there twice? I guess it needs to be if we're not going to bound it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, unsigned long long is the WebIDL type for a 64-bit integer.

...oh, I was about to say we use the same type for timeout, but it turns out it's just unsigned long (32-bit). So yeah, let's make it just unsigned long here too for consistency. 264 milliseconds is almost 50 days, I can't imagine anyone needing more range than that.

:: Returns the number of milliseconds elapsed since the last time the user was successfully verified as returned by the [=authenticator=].
<xmp class="idl">
partial dictionary AuthenticationExtensionsClientOutputs {
unsigned long long timeSinceLastUV;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This member name needs to be the same as the extension identifier.

Suggested change
unsigned long long timeSinceLastUV;
unsigned long long userVerificationCaching;


```
$$extensionInput //= (
mtslUV: uint .size 4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This member name needs to be the same as the extension identifier.

Suggested change
mtslUV: uint .size 4
userVerificationCaching: uint .size 4


```
$$extensionOutput //= (
tslUV: uint .size 4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This member name needs to be the same as the extension identifier.

Suggested change
tslUV: uint .size 4
userVerificationCaching: uint .size 4

:: When user verification is requested, the [=authenticator=] triggers user verification only if more milliseconds have elapsed since the last time the user was verified than indicated by the maxTimeSinceLastUV value in the extension.

: Authenticator extension output
:: If no fresh user verification needed to be triggered triggered, the authenticator reports the time last last user verification time back to the [=[RP]=] to ensure the [=[RP]=] is aware that no fresh user verification was triggered. It is up to the authenticator to decide whether to return the real elapsed time, or a "rounded" value. If user verification was requested, this value SHALL not exceed the value originally provided in the extension input.
Copy link
Contributor

@sbweeden sbweeden Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the extension output if UV is done as part of the current ceremony? I know that the UV bit should be set in authenticatorData, but should the extension output also be set with a value of 0 perhaps? The purpose of returning that is to at least indicate to the RP that the extension was acknowledge and processed. The RP could infer that UV was done, but might otherwise not have been if the maxTimeSinceLastUV had been shorter. This would differentiate it from an authenticator that doesn't support the extension and just did UV because it always does UV when preferred is set for userVerification in the initial authentication request.

Copy link
Contributor

@MasterKale MasterKale Mar 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, the suggestion here is that extension output should be set to 0 if UV takes place during the ceremony? Which naturally means the extension output should never be greater than the duration specified as maxTimeSinceLastUV. That sounds sensible to me.

@timcappalli
Copy link
Member

I'm largely good with this one, as long as it is restricted for use with UV=preferred, similar to #2052.

@MasterKale
Copy link
Contributor

I've been convinced that this proposal enables RP's to gracefully handle more use cases than the simpler reporting mechanism proposed in #2052. I'd say let's try and get this one into the spec instead.

@timcappalli
Copy link
Member

I'd also add that one of the two open questions for #2052 also applies here:

Technically an out of band vault unlock for passkey provider doesn't satisfy user verification as it was not part of a WebAuthn ceremony. How do we want to handle that?

: Extension identifier
:: `userVerificationCaching`

: Operation applicability
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the user verification selection criteria should go IMO.

Also shouldn't this apply to both creation and authentication?

If so, suggest the following:

:   Operation applicability
::  [=registration extension|Registration=] and [=authentication extension|Authentication=] when {{AuthenticatorSelectionCriteria/userVerification}} is set to {{UserVerificationRequirement/preferred}}.

:: The maxTimeSinceLastUV denotes the maximum acceptable number of milliseconds elapsed since the last time the user was successfully verified.
<xmp class="idl">
partial dictionary AuthenticationExtensionsClientInputs {
unsigned long long maxTimeSinceLastUV;
Copy link
Member

@timcappalli timcappalli Mar 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should long be there twice? I guess it needs to be if we're not going to bound it.

:: [=authentication extension|authentication=]

: Client extension input
:: The maxTimeSinceLastUV denotes the maximum acceptable number of milliseconds elapsed since the last time the user was successfully verified.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we bound this with a min and max? (#2034 (comment))

:: None, except creating the [=authenticator extension input=] from the client extension input.

: Client extension output
:: Returns the number of milliseconds elapsed since the last time the user was successfully verified as returned by the [=authenticator=].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/verified as/verified, as

```

: Authenticator extension processing
:: When user verification is requested, the [=authenticator=] triggers user verification only if more milliseconds have elapsed since the last time the user was verified than indicated by the maxTimeSinceLastUV value in the extension.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If more milliseconds have elapsed since the last time the user was verified than indicated by the maxTimeSinceLastUV value in the extension, request [=user verification=]."

@timcappalli timcappalli changed the title user verification caching extension added Acceptable UV caching time affordance extension added Mar 27, 2024
</xmp>

: Authenticator extension input
:: The maximum acceptable time in milliseconds elapsed since last user verification, encoded in CBOR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's missing here is the negative case - if this extension is not present, do not allow caching at all. This needs to be clearly stated in the spec.

@timcappalli
Copy link
Member

2024-04-19 WebAuthn F2F meeting: consensus to put this PR (and its alternative #2052) on hold until some details around attestation and conformance in FIDO are ironed out.

@timcappalli timcappalli added stat:Blocked Prerequisites are not yet satisfied and removed type:technical @Risk Items that are at risk for L3 labels Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat:Blocked Prerequisites are not yet satisfied
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reflect caching of user verification in WebAuthn assertion
7 participants