Fix #720: Align user handle management with CTAP #730

emlun · 2017-12-20T16:40:06Z

As stated in #558 (comment). This resolves #720.

…inition This resolves #720.

As stated in #558 (comment) and #558 (comment) the user handle should not be returned when operating in 2nd factor mode (i.e., when given a non-empty `allowCredentials` list).

emlun · 2017-12-20T16:43:22Z

index.bs

@@ -392,7 +392,7 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S
    * A [=Credential ID=].
    * A [=credential private key=].
    * The [=Relying Party Identifier=] for the [=[RP]=] that created this credential source.
-    * An optional [=user handle=] for the person who created this credential source.
+    * A [=user handle=] for the person who created this credential source.


This change is already merged into master via #721, but GitHub doesn't seem to detect it because I based this PR off the issue-720-user-handle-optional branch whose commits were rebased onto master instead of merging.

emlun · 2017-12-20T16:48:41Z

Assigned this to the CR milestone since the associated issue is assigned to CR.

emlun · 2017-12-20T20:12:45Z

@akshayku pointed out on the 2017-12-20 WG call that the issues this was meant to solve are probably non-issues. We agreed but decided to ask @christiaanbrand to take one last look at this.

akshayku · 2017-12-21T00:37:44Z

index.bs

        - |authenticatorData|
        - |signature|
-        - The [=user handle=] associated with |selectedCredential|.
+        - The [=user handle=] associated with |selectedCredential|, if no [=list=] of credentials was supplied by the client, or
+            no such [=list=] was supplied. Otherwise, do not return this value.


I don't see any reason on why not.

Its not a private information and RP may need it even if RP is supplying a set of credentialIDs as userHandle is helping RP for the lookup even in second factor case.

akshayku · 2017-12-21T00:39:23Z

index.bs

+
+      Note: If the client supplies a [=list=] of exactly one credential and it was successfully employed, then its
+          [=credential ID=] and [=user handle=] are not returned since the [=[RP]=], having provided the [=credential ID=] to the
+          client, already knows the [=credential ID=] and the identity of the user. This saves transmitting these bytes over what


That's not true.

CredentialID and userHandle are providing different information and RP knowing CredentialID to information that is stored in userHandle is not accurate.

akshayku

Returning userHandle even in second factor case provides a value to the RP.

As we have already established that userHandle is not private information, I don't see any reason on why we are directing authenticators to not return back that information.

emlun · 2017-12-21T14:02:58Z

I now agree that the user handle is not private information, but I don't see how the RP could not know the user's identity if the RP has already looked up a list of credential IDs for that user.

Either way, I see now that CTAP's getAssertion method always returns the user.id even in the 2nd factor case, so I support closing this. Sorry for wasting everyone's time on yesterday's call.

akshayku · 2017-12-21T17:14:08Z

Let me give you our example.

We have a distributed server environment and is doing credentialID based lookup. We use user id as a helper for lookup in this distributed environment for scalability purposes. The response server is not the same as request server. And that is why, userId is a helpful to us even in a second factor case.

As this is not optional in CTAP spec, I see no reason for WebAuthN to hide this information.

I am closing this. Please reopen if someone does not agree.

akshayku · 2017-12-22T17:30:23Z

I am reopening this PR.

The confusion is coming because 2nd factor != server credentials in all the cases.

CTAP spec needs clarification as there is no user information returned in authenticatorGetAssertion for server credentials and U2F devices as it is not available to the authenticator. For device resident keys, userID MUST be returned (irrespective of whether credentialID list is provided or not). I will open the clarification PR for CTAP spec.

Regarding this PR, it needs more work. Irrespective of allow credential ID list is present or not, if authenticator is giving userID back, it should be returned back to the RP.

So the sections in this PR which talks about whether user ID returned from authenticator is null or not is correct. Sections which talks about removing this information when credential ID list is present in not correct.

akshayku · 2017-12-22T17:34:23Z

index.bs

@@ -1245,7 +1245,8 @@ When this method is invoked, the user agent MUST execute the following algorithm
                ::  whose value is the bytes of the signature value returned by the [=authenticator=].

                :   <code><dfn for="assertionCreationData">userHandleResult</dfn></code>
-                ::  whose value is the bytes of the [=user handle=] returned by the [=authenticator=].
+                ::  If |savedCredentialId| exists, set the value of [=userHandleResult=] to be the bytes of the [=user handle=]


Even if savedCredentialID exists or a list of credentialID exists, if authenticator is returning userID, it should be returned back to the RP.

akshayku · 2017-12-22T17:35:02Z

index.bs

@@ -1278,7 +1279,9 @@ When this method is invoked, the user agent MUST execute the following algorithm
                            <code>|assertionCreationData|.[=assertionCreationData/signatureResult=]</code>.

                        :   {{AuthenticatorAssertionResponse/userHandle}}
-                        ::  A new {{ArrayBuffer}}, created using |global|'s [=%ArrayBuffer%=], containing the bytes of
+                        ::  If <code>|assertionCreationData|.[=assertionCreationData/userHandleResult=]</code> is null, set this


This part is correct.

akshayku · 2017-12-22T17:39:34Z

index.bs

+        - The [=user handle=] associated with |selectedCredential|, if no [=list=] of credentials was supplied by the client, or
+            no such [=list=] was supplied. Otherwise, do not return this value.
+
+      Note: If the client supplies a [=list=] of exactly one credential and it was successfully employed, then its


I would suggest this note section to be deleted. If user handle is returned from the authenticator, it should be returned back to the RP.

Regarding only one CredentialID being sent, not returning it back does not work for distributed server scenarios like ours where request processing server is not the same as response processing server.

emlun · 2017-12-24T11:09:17Z

What do you mean by "server credentials"?

Ah yes, I see now that CTAP's authenticatorMakeCredential method stores the user data only for resident keys. However, even with that limitation I think your use case can be solved by embedding a user ID in the challenge parameter. Since challenge is an opaque byte array of unspecified length, and is returned in the assertion response, you could set challenge to the UTF-8 bytes of for example {"userId":"ff642b","random":"1/BjTenZNLAw9l03J2J2BcpgXP5Ic7gEuoVfVVrf7Bg="}. This should allow you to embed the user ID while maintaining challenge as a valid nonce, correct?

So I don't think changing CTAP is strictly necessary, but I'll update this PR either way just in case we need it.

emlun · 2017-12-24T22:59:21Z

Wait - actually, the response processing server (ResPS) needs to be able to verify that the returned challenge equals that sent to the client, so there needs to be some trusted communication path between the request processing server (ReqPS) and the ResPS so the ResPS can obtain the challenge from the ReqPS. Could that message then not also contain the user ID (if known, i.e. in 2nd factor mode)?

CTAP's [authenticatorMakeCredential][1] method stores the `user` parameter only for resident credentials. [1]: https://fidoalliance.org/specs/fido-v2.0-rd-20170927/fido-client-to-authenticator-protocol-v2.0-rd-20170927.html#h3_authenticatorMakeCredential

…urce definition" This reverts commit d448eb3.

emlun · 2017-12-25T14:44:12Z

Summary of currently proposed changes:

AuthenticatorAssertionResponse.userHandle is now nullable.
The authenticator MAY now skip storing the user handle for credentials that do not have a client-side-resident credential private key. (CTAP currently does this)
The authenticator now always returns the user handle if it is available.
The client now returns userHandle: null if the authenticator did not return the user handle.

However it looks like CTAP is internally inconsistent: the user argument to authenticatorMakeCredential is stored only for resident keys, but the user attribute is required in the authenticatorGetAssertion response...

akshayku

Looks good to me.

akshayku · 2017-12-25T19:28:35Z

@emlun: Thanks for taking this on.

You are correct, even if response server is different request server (rare case), user information is still available in addition to credentialID as it is a second factor case and user has already entered user information in first step. I was incorrect earlier and when I relooked at the whole scenario, I realized that the confusion came because earlier we were equating second factor case with server credentials case which is not always correct.

For server credentials, it is of the same semantics as of U2F meaning authenticators MAY drop all user information while returning the credentialID. Some authenticators MAY also store this information in encrypted form in future however there is no need and I am not aware of any such authenticators as of now. In any case, this is authenticator specific. CTAP spec needs a clarification as it says user ID is required in authenticatorGetAssertion response which is inconsistent for U2F devices or for server credentials where there is no such information available. I will make that clarification.

Platform logic is simple, if it returned from authenticator (for resident keys, irrespective of whether credential list if present or not), return it to the RP, otherwise set it to NULL.

Thanks

emlun · 2017-12-25T20:45:36Z

@akshayku Ok, thanks!

equalsJeffH · 2017-12-29T15:43:29Z

@akshayku please clarify what you mean by "server credentials"? thx.

akshayku · 2017-12-29T22:16:50Z

@equalsJeffH : By "server credentials", I meant credentials which are resident on the server instead of FIDO device. Or in other words, credentials which are created via authenticatorMakeCredential API where "rk" is not set to TRUE and credentialID returned by the API contains encrypted private credential.

These credentials created by FIDO 2 device has the same characteristics as credentials which would have been created via U2F devices.

agl · 2018-01-02T17:28:50Z

I asked internally about cbrand's concerns here and I understand them to be centered around the case where a 2nd-factor token is found by someone. It would be good for that not to essentially disclose their username in the 2nd-factor case.

Such a concern would thus have to be focused on CTAP2 rather than webauthn. So I don't think we have any unexpected concerns here.

ve7jtb · 2018-01-02T17:52:40Z

all

the concern seems more around how resident credentials are protected. It shouldn't matter if the resident credential is being used as first or second factor. Both need to be protected.
I think any sort of resident credential needs to be protected with at least a device pin.

I am not sure that a resident credential with no userID is sufficient protection, without some device unlock. I think from a CTAP point of view a resident credential is resident credential.

John B.

equalsJeffH

this seems nominally OK IIUC, modulo results of discussing @ve7jtb's #730 (comment)

equalsJeffH · 2018-01-02T22:30:35Z

index.bs

@@ -1245,7 +1245,8 @@ When this method is invoked, the user agent MUST execute the following algorithm
                ::  whose value is the bytes of the signature value returned by the [=authenticator=].

                :   <code><dfn for="assertionCreationData">userHandleResult</dfn></code>
-                ::  whose value is the bytes of the [=user handle=] returned by the [=authenticator=].
+                ::  If the authenticator returned a [=user handle=], set the value of [=userHandleResult=] to be the bytes of that
+                    [=user handle=] returned by the [=authenticator=]. Otherwise, set the value of [=userHandleResult=] to null.


suggest:
If the [=authenticator=] returned a [=user handle=], set the value of [=userHandleResult=] to be the bytes of the returned [=user handle=]. Otherwise, ...

equalsJeffH · 2018-01-02T22:46:24Z

index.bs

@@ -1278,7 +1279,9 @@ When this method is invoked, the user agent MUST execute the following algorithm
                            <code>|assertionCreationData|.[=assertionCreationData/signatureResult=]</code>.

                        :   {{AuthenticatorAssertionResponse/userHandle}}
-                        ::  A new {{ArrayBuffer}}, created using |global|'s [=%ArrayBuffer%=], containing the bytes of
+                        ::  If <code>|assertionCreationData|.[=assertionCreationData/userHandleResult=]</code> is null, set this
+                            field to null. Otherwise, set this field to a new {{ArrayBuffer}}, created using |global|'s


nit: I'm not sure these two sentences are technically correct given that the dfn of ArrayBuffer is: "An object that holds a pointer (which may be null) to a buffer of a fixed number of bytes". I.e., may need to only instantiate the arraybuffer and assign the bytes of userHandleResult to it, and if userHandleResult is null, then the buffer pointer is automagically set to null.

However, the present language may be close enough to pass muster with reviewers.

As far as I could tell when I looked into it, it seemed like ArrayBuffer is a nullable type, if that's what you mean. Did I get that wrong?

Or, rather, it looked to me making a field of type ArrayBuffer nullable is legal. In that case I suppose the value could be null in two ways, so you may be right in that it's more correct to keep it not nullable, but I feel like making the field itself explicitly nullable better communicates the intent. What do you think?

emlun added 2 commits December 8, 2017 10:39

Don't say user handle is optional in Public Key Credential Source def…

d448eb3

…inition This resolves #720.

Don't return user handle in getAssertion in 2nd factor mode

3b2a1d1

As stated in #558 (comment) and #558 (comment) the user handle should not be returned when operating in 2nd factor mode (i.e., when given a non-empty `allowCredentials` list).

emlun added stat:breaking subtype:algorithms/WebIDL type:technical labels Dec 20, 2017

emlun added this to the CR milestone Dec 20, 2017

emlun self-assigned this Dec 20, 2017

emlun requested a review from equalsJeffH December 20, 2017 16:40

emlun commented Dec 20, 2017

View reviewed changes

nadalin requested a review from akshayku December 20, 2017 18:07

equalsJeffH requested review from akshayku and jcjones and removed request for akshayku December 20, 2017 18:10

akshayku reviewed Dec 21, 2017

View reviewed changes

akshayku requested changes Dec 21, 2017

View reviewed changes

akshayku closed this Dec 21, 2017

emlun mentioned this pull request Dec 21, 2017

Contradiction in whether user handle is required #720

Closed

akshayku reopened this Dec 22, 2017

akshayku reviewed Dec 22, 2017

View reviewed changes

akshayku requested changes Dec 22, 2017

View reviewed changes

emlun added 2 commits December 25, 2017 14:14

Address review comments by @akshayku

3938fc1

Revert "Don't say user handle is optional in Public Key Credential So…

684581f

…urce definition" This reverts commit d448eb3.

akshayku approved these changes Dec 25, 2017

View reviewed changes

emlun changed the title ~~Fix #720: Don't return user handle in 2nd factor mode~~ Fix #720: Align user handle management with CTAP Dec 25, 2017

equalsJeffH reviewed Jan 2, 2018

View reviewed changes

Address one of @equalsJeffH's review comments

f3e8afb

selfissued approved these changes Jan 3, 2018

View reviewed changes

jcjones approved these changes Jan 3, 2018

View reviewed changes

emlun merged commit 5948f3b into master Jan 3, 2018

emlun deleted the issue-720-user-handle-optional branch January 3, 2018 19:13

emlun mentioned this pull request Jan 12, 2018

FIDO U2F Attestation Statement Format needs to clarify that user handle will be empty #715

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #720: Align user handle management with CTAP #730

Fix #720: Align user handle management with CTAP #730

emlun commented Dec 20, 2017 •

edited by pr-preview bot

Loading

emlun Dec 20, 2017

emlun commented Dec 20, 2017

emlun commented Dec 20, 2017

akshayku Dec 21, 2017

akshayku Dec 21, 2017

akshayku left a comment

emlun commented Dec 21, 2017

akshayku commented Dec 21, 2017

akshayku commented Dec 22, 2017

akshayku Dec 22, 2017

emlun Dec 25, 2017

akshayku Dec 22, 2017

akshayku Dec 22, 2017

emlun Dec 25, 2017

emlun commented Dec 24, 2017

emlun commented Dec 24, 2017

emlun commented Dec 25, 2017

akshayku left a comment

akshayku commented Dec 25, 2017 •

edited

Loading

emlun commented Dec 25, 2017

equalsJeffH commented Dec 29, 2017

akshayku commented Dec 29, 2017

agl commented Jan 2, 2018

ve7jtb commented Jan 2, 2018

equalsJeffH left a comment

equalsJeffH Jan 2, 2018

emlun Jan 3, 2018

equalsJeffH Jan 2, 2018

emlun Jan 3, 2018

emlun Jan 3, 2018

Fix #720: Align user handle management with CTAP #730

Fix #720: Align user handle management with CTAP #730

Conversation

emlun commented Dec 20, 2017 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

emlun commented Dec 20, 2017

emlun commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayku left a comment

Choose a reason for hiding this comment

emlun commented Dec 21, 2017

akshayku commented Dec 21, 2017

akshayku commented Dec 22, 2017

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

emlun commented Dec 24, 2017

emlun commented Dec 24, 2017

emlun commented Dec 25, 2017

akshayku left a comment

Choose a reason for hiding this comment

akshayku commented Dec 25, 2017 • edited Loading

emlun commented Dec 25, 2017

equalsJeffH commented Dec 29, 2017

akshayku commented Dec 29, 2017

agl commented Jan 2, 2018

ve7jtb commented Jan 2, 2018

equalsJeffH 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

emlun commented Dec 20, 2017 •

edited by pr-preview bot

Loading

akshayku commented Dec 25, 2017 •

edited

Loading