Fix #644: Add UV parameter to getAssertion #672

emlun · 2017-11-06T13:03:10Z

This resolves #644, and possibly #524 and #645.

Summary:

The definition of "user present" now states that if the user is "verified" they are also "present". Background: credentials.get() should have optional parameters for userVerification and userPresence #629 Reverted after review
A UV parameter has been added to PublicKeyCredentialRequestOptions.
A Boolean UV parameter is now passed to authenticatorGetAssertion().
The type of the UV parameter of the client operations has changed from Boolean to UserVerificationRequirement.
- Background: #getAssertion alg needs to pass authenticator selection requirements to authenticatorGetAssertion operation #644, credentials.get() should have optional parameters for userVerification and userPresence #629.
- The enum type UserVerificationRequirement has been added, with values required, ~~wanted~~ preferred and ~~not-wanted~~ discouraged.
- The type of the member requireUserVerification of AuthenticatorSelectionCriteria has changed from Boolean to UserVerificationRequirement.
- A member requireUserVerification of type UserVerificationRequirement has been added to PublicKeyCredentialRequestOptions.
- This value is used by the client operations to compute a Boolean value to pass to the authenticator operations.
~~Authenticator operations are now specified to always require UP or UV.~~ This was already stated in prose in §5.1.4 Use an existing credential to make an assertion, but not in the algorithms:

Since this specification requires an authorization gesture to create any credentials, [...]

Silent authenticator operations has also raised objections (credentials.get() should have optional parameters for userVerification and userPresence #629, #getAssertion alg needs to pass authenticator selection requirements to authenticatorGetAssertion operation #644), so we should state this requirement clearly until it's decided to support silent operations. This probably breaks compatibility with UAF Silent Authenticators (see WebAuthn available to Workers? aka "silent authentication" #199 (comment)).

EDIT: This logic has moved to the client level instead.
~~authenticatorMakeCredential now always uses UV if the authenticator supports it. If requireUserVerification is true the client will only send the command to capable authenticators.~~ Reverted after review
Authenticator operations' UP/UV behaviour has been aligned to agree with CTAP2.
- A Boolean UP parameter is now passed to authenticatorMakeCredential() and authenticatorGetAssertion(). The client always sets this to the inverse of the UV parameter.
CTAP compatibility should not be affected by any of the above. If it is, that's a bug.

…r standalone It's just confusing to send the whole `AuthenticatorSelectionCriteria` structure but ignore of forbid 2 of the 3 members. If those additional parameters should be added in the future, that will probably be a breaking change anyway.

…tion

emlun · 2017-11-06T13:04:01Z

@Kieun may have something to say about how this affects UAF.

emlun · 2017-11-06T13:26:59Z

I would also like to rename the requireUserVerification member to userVerification, but left it out for now to avoid unnecessary breakage.

Kieun · 2017-11-07T12:33:58Z

This modification indeed affects UAF. But I'm not sure that how many number of UAF silent authenticators are deployed in the market. I would also think that at least user presence is needed during authentication in web space for privacy reason.
But for native application, it would be worth to provide silent authenticators for authentication since a lot of applications already tend to use automated login(authenticate) approach.

emlun · 2017-11-07T13:26:54Z

By "native application", do you mean platform authenticators (i.e. TPMs integrated in laptops/phones)? There was an exception for them, but it was in authenticatorMakeCredential which has been agreed should for now not be allowed to be silent. Do you request a similar exception for platform authenticators in authenticatorGetAssertion?

Kieun · 2017-11-07T13:37:00Z

I just mean that "native application" is a platform specific RP application such as Android or iOS app that can leverage FIDO silent authenticators for automated login use case.

emlun · 2017-11-07T13:54:27Z

Ok. That sounds like it falls outside the scope of the WebAuthn spec.

equalsJeffH · 2017-11-15T07:50:22Z

thanks @emlun -- this now looks nominally OK to me, but I think it really needs explicit buy-in from Chrome, FF, and @rlin1: in that my understanding of the nominal agreement had been that the default interaction case would be 2nd factor -- this seems to be changing it to 1st factor. this is fine by me, however we ought to explcitly get buy-in from other working group participants on it. thx, HTH.

cc: @jcjones @rlin1 @balfanz @leshi @AngeloKai

emlun · 2017-11-15T07:58:25Z

@equalsJeffH Thanks!

Hm, I did not intend to change the default interaction case. What parts of the changes are suggesting that?

rlin1

see my comments

rlin1 · 2017-11-15T12:46:13Z

index.bs

@@ -1383,7 +1407,7 @@ attributes.
    dictionary AuthenticatorSelectionCriteria {
        AuthenticatorAttachment      authenticatorAttachment;
        boolean                      requireResidentKey = false;
-        boolean                      requireUserVerification = false;
+        UserVerificationRequirement  requireUserVerification = "preferred";


Don't have a strong opinion on the default value - as longh as the RP can override it (which is the case IMHO).

rlin1 · 2017-11-15T12:46:44Z

index.bs

@@ -1538,6 +1591,7 @@ an assertion. Its {{PublicKeyCredentialRequestOptions/challenge}} member must be
        unsigned long                        timeout;
        USVString                            rpId;
        sequence<PublicKeyCredentialDescriptor> allowCredentials = [];
+        UserVerificationRequirement          userVerification = "preferred";


Don't have a strong opinion on the default value - as longh as the RP can override it (which is the case IMHO).

rlin1 · 2017-11-15T12:47:48Z

index.bs

@@ -1560,7 +1618,12 @@ an assertion. Its {{PublicKeyCredentialRequestOptions/challenge}} member must be
    ::  This optional member contains a list of {{PublicKeyCredentialDescriptor}} object representing [=public key credentials=]


objects - need plural here.

Good catch, thanks.

rlin1 · 2017-11-15T12:59:06Z

index.bs

-    by the
-    [=authenticator=] if it has its own output capability, or by the user agent otherwise.
+
+1. Obtain [=user consent=] for using |selectedCredential|. The prompt for obtaining this [=user consent|consent=] may be shown by


We SHOULD NOT require additional user consent gathering by the platform for using an authenticator that is already registered!

rlin1 · 2017-11-15T13:08:00Z

index.bs

 1. If |allowCredentialDescriptorList| is now empty, return an error code equivalent to "{{NotAllowedError}}" and terminate the
    operation.
+
 1. Prompt the user to select a [=public key credential|credential=] |selectedCredential| from |allowCredentialDescriptorList|.


this is only required if the list has more than 1 entry.

so we should say: If the |allowCredentialDescriptorList| contains multiple entries, then ...

rlin1 · 2017-11-15T13:09:54Z

index.bs

-    by the
-    [=authenticator=] if it has its own output capability, or by the user agent otherwise.
+
+1. Obtain [=user consent=] for using |selectedCredential|. The prompt for obtaining this [=user consent|consent=] may be shown by


We SHOULD be clear that user verification, e.g. a fingerprint swipe IS a method for obtaining user consent - we don't want additional dialog boxes!

@rlin1

Per review comment by @rlin1.

jcjones · 2017-11-15T22:32:49Z

@emlun said:

Hm, I did not intend to change the default interaction case. What parts of the changes are suggesting that?

I think using "preferred" as the default option is what @equalsJeffH is referring to, and I agree that it does change the nature of the default interaction.

I added a comment where I was pinged (https://github.com/w3c/webauthn/pull/672/files#r151271242). I won't oppose the will of the working group if there's consensus on this, but my preference would be to have this default to "discouraged" to keep the common case simple.

emlun · 2017-11-15T22:50:26Z

@jcjones Ah, I see. Those are good points. I don't have a strong opinion either way. As far as I can tell it seems like most are more in favor of "discouraged", but only @akshayku has voiced a strong opinion. I'm fine with changing the default to "discouraged" if noone objects.

jcjones · 2017-11-15T23:19:47Z

@emlun: It looks to me like there's broad agreement on that. I don't see anything problematic with this change otherwise, and I think if you swap those defaults then this should be free to merge (after we get confirmation from the other 3 reviewers)

jcjones · 2017-11-15T23:22:26Z

Oops, I spoke too soon. I missed the embedded threads. Disregard.

Still, I think adjusting that default would help. But the points about presence vs. user verification are better articulated by Rolf, Akshay, Jeff, et. al than I can do.

akshayku · 2017-11-16T18:05:45Z

@jcjones : What is the common case in your opinion? Does not it maps to RP supporting all kind of authenticators?? and Isn't that the direction we should be going to?

emlun · 2017-11-16T18:08:39Z

"preferred" indicates that the RP supports all authenticators, yes, but so does "discouraged". I think it's also fair to say "discouraged" maps closer to the previous default requireUserVerification: false (in makeCredential).

akshayku · 2017-11-16T18:16:52Z

@emlun

No. In discouraged case, even if authenticator supports user verification, RP probably will not get it and RP has no way to know/take advantage of that feature. Similarly, Authenticator who has done extra work to make user verification possible in their product, will unfairly suffer because by default that feature is not used. preferred maps to default behavior of authenticator and RP has a better chance of supporting all kind of authenticators.

emlun · 2017-11-16T19:50:37Z

@akshayku Ok. I guess you and I use "support" to mean different things - neither the client nor the RP will refuse authenticators with UV in the "discouraged" case, so they'll still support them in that sense.

Anyway, with that it looks like consensus is possible for "preferred" but not for "discouraged". Still waiting for responses from @balfanz and @leshi.

jcjones · 2017-11-16T23:27:21Z

I think @akshayku and I are on different pages, and maybe I'm on the same page as @emlun, but to be honest: I'm fine with "preferred", and arguing that letting the common case promote UV authenticators is fine and in keeping with the rest of the model IMO.

christiaanbrand · 2017-11-17T21:59:50Z

I don't really care that much. As long as we can override it.

akshayku · 2017-11-17T22:58:41Z

@emlun : I think we have an agreement for preferred. If you agree, lets check this in so that we can move on.

emlun · 2017-11-17T23:14:30Z

Great! I'll go ahead and merge if noone objects by Monday morning CET.

nadalin · 2017-11-20T16:38:10Z

@jcjones @balfanz @AngeloKai Please review asap so try to get closure before meeting this week

equalsJeffH · 2017-11-20T17:05:10Z

@nadalin -- this was merged 7.5 hr before your comment above :)

emlun added 11 commits November 4, 2017 15:22

Add UV parameter to getAssertion operation

1c1a111

Fix indentation

1d2d0e5

Always require UP or UV for authenticator operations

48226c7

Make requireUserVerification enum instead of Boolean

d0b43a6

Improve formulation of UP/UV requirement in authenticator operations

ef015fa

Remove duplicated error return instruction

81f9445

Add dfn links to occurences of "user consent"

76d6e0d

Move UserVerificationRequirement to supporting data structures

af7da2d

Fix reference to |rpEntity| and |userEntity| in authenticatorGetAsser…

20e5a52

…tion

Fix typo

dbca08d

emlun added priority:implementation stat:breaking subtype:algorithms/WebIDL type:technical labels Nov 6, 2017

emlun added this to the WD-07 milestone Nov 6, 2017

emlun self-assigned this Nov 6, 2017

emlun requested review from equalsJeffH, balfanz, jcjones, leshi, AngeloKai and akshayku November 6, 2017 13:03

Remove redundant "if present" from requireUserVerification description

c6f07a5

equalsJeffH requested a review from rlin1 November 15, 2017 06:46

more consistency: add Get name to section 5.1.4

46d6c51

rlin1 reviewed Nov 15, 2017

View reviewed changes

emlun added 4 commits November 15, 2017 15:15

Fix plural

ea30a33

Prompt for credential selection only if more than one eligible

3d352c7

Per review comment by @rlin1.

Use Infra list terms in authenticatorGetAssertion

f6358cf

Use Infra list terms in authenticatorGetAssertion

05f4b23

akshayku approved these changes Nov 17, 2017

View reviewed changes

equalsJeffH approved these changes Nov 19, 2017

View reviewed changes

emlun merged commit e517264 into master Nov 20, 2017

emlun deleted the issue-644 branch November 20, 2017 09:21

emlun mentioned this pull request Dec 13, 2017

Bug: getAssertion accidentally calls authenticatorGetAssertion in first factor mode #685

Closed

emlun mentioned this pull request Dec 18, 2018

Clarify relationships between "uv"/"up" of the CTAP spec and "userVerification"/"userPresence" #1123

Closed

emlun mentioned this pull request Jul 4, 2019

The user-verification default of “preferred” is catching most sites out. #1253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #644: Add UV parameter to getAssertion #672

Fix #644: Add UV parameter to getAssertion #672

emlun commented Nov 6, 2017 •

edited by pr-preview bot

Loading

emlun commented Nov 6, 2017

emlun commented Nov 6, 2017

Kieun commented Nov 7, 2017

emlun commented Nov 7, 2017

Kieun commented Nov 7, 2017

emlun commented Nov 7, 2017

equalsJeffH commented Nov 15, 2017

emlun commented Nov 15, 2017

rlin1 left a comment

rlin1 Nov 15, 2017

rlin1 Nov 15, 2017

rlin1 Nov 15, 2017

emlun Nov 15, 2017

rlin1 Nov 15, 2017

rlin1 Nov 15, 2017

rlin1 Nov 15, 2017

emlun Nov 15, 2017

rlin1 Nov 15, 2017

jcjones commented Nov 15, 2017

emlun commented Nov 15, 2017

jcjones commented Nov 15, 2017

jcjones commented Nov 15, 2017

akshayku commented Nov 16, 2017

emlun commented Nov 16, 2017

akshayku commented Nov 16, 2017

emlun commented Nov 16, 2017

jcjones commented Nov 16, 2017

christiaanbrand commented Nov 17, 2017

akshayku commented Nov 17, 2017

emlun commented Nov 17, 2017

nadalin commented Nov 20, 2017

equalsJeffH commented Nov 20, 2017

		@@ -1560,7 +1618,12 @@ an assertion. Its {{PublicKeyCredentialRequestOptions/challenge}} member must be
		:: This optional member contains a list of {{PublicKeyCredentialDescriptor}} object representing [=public key credentials=]

Fix #644: Add UV parameter to getAssertion #672

Fix #644: Add UV parameter to getAssertion #672

Conversation

emlun commented Nov 6, 2017 • edited by pr-preview bot Loading

emlun commented Nov 6, 2017

emlun commented Nov 6, 2017

Kieun commented Nov 7, 2017

emlun commented Nov 7, 2017

Kieun commented Nov 7, 2017

emlun commented Nov 7, 2017

equalsJeffH commented Nov 15, 2017

emlun commented Nov 15, 2017

rlin1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcjones commented Nov 15, 2017

emlun commented Nov 15, 2017

jcjones commented Nov 15, 2017

jcjones commented Nov 15, 2017

akshayku commented Nov 16, 2017

emlun commented Nov 16, 2017

akshayku commented Nov 16, 2017

emlun commented Nov 16, 2017

jcjones commented Nov 16, 2017

christiaanbrand commented Nov 17, 2017

akshayku commented Nov 17, 2017

emlun commented Nov 17, 2017

nadalin commented Nov 20, 2017

equalsJeffH commented Nov 20, 2017

emlun commented Nov 6, 2017 •

edited by pr-preview bot

Loading