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

Add User Verification (UV) bit to authenticator data #409

Merged
merged 19 commits into from Apr 24, 2017

Conversation

AngeloKai
Copy link
Contributor

This is a split from PR 348. This is based on the changes in PR 384 The corresponding change in the CTAP spec has already been merged.

@leshi
Copy link
Contributor

leshi commented Apr 19, 2017

Angelo, could you point me to the corresponding merge in CTAP? I don't see any references to TUI in the merged CTAP.

@equalsJeffH
Copy link
Contributor

we should not use the term "identity" in this spec -- this is peer-entity authentication

@AngeloKai
Copy link
Contributor Author

@equalsJeffH should we consider the word "verification" in replacement of "identity"?

@jyasskin
Copy link
Member

2¢: both "user verification" and "user presence" are ambiguous when read as English: do they verify that any user is present or that a particular user is present?

Despite @equalsJeffH' correct point that the API can't return anything about the RP's notion of identity, I think this bit really is asking the authenticator to return something about its notion of user identity, and so that may be the right word to use.

@equalsJeffH
Copy link
Contributor

@AngeloKai

should we consider the word "verification" in replacement of "identity"?

i would suggest we simply use the term "user verification" instead of "test of user identity". Note the definition of user verification -- "test" is implied.

@equalsJeffH
Copy link
Contributor

@jyasskin

...this bit really is asking the authenticator to return something about its notion of user identity...

I disagree -- at nav.creds.get() (aka getAssn()) time, the authnr is only saying "the entity I've interacted with now is AFAICT the same entity that I did when creating the credential". At nav.creds.create() time, it is saying "this public key & credId are mapped to my notion of the entity I've just interacted with, and I'm going to remember that, and require that same entity's presence during future nav.creds.get() invocations". Any notion of "identity" is in the eye of the RP, and depends on whatever collection of attributes the RP is mapping to these interactions and also how the RP models its notions of "identity" -- but the webauthn protocol and API are not cognizant of RPs' notions of "identity" (even tho some of the RP's "user identity attributes" may be established by and conveyed via the protocol). I.e., webauthn only provides one little piece of an RP's identity puzzle -- peer-entity authn mapped to a couple of identifiers and a public key -- managing the overall notion of "user identity" is a much broader problem and is up to the RP to figure out.

index.bs Outdated
@@ -1263,6 +1263,7 @@ The [=authenticator data=] structure is a byte array of 37 bytes or more, as fol
<td>
Flags (bit 0 is the least significant bit):
- Bit 0: [=Test of User Presence=] (`TUP`) result.
- Bit 1: [=Test of User Identity=] (`TUI`) result.
- Bits 1-5: Reserved for future use (`RFU`).
Copy link
Member

Choose a reason for hiding this comment

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

s/1-5/2-5/

@jyasskin
Copy link
Member

@equalsJeffH I think it'd be good to capture the idea of "the same entity" in the name of this bit, even if that's captured in a word other than "identity". It could be the "same user" or "consistent user" bit?

Notionally, if we have 3 RPs, a browser, and an authenticator, they can have 5 independent ideas of users' identities, and that's ok: this API is just talking about the authenticator's idea. But I'm happy to use a word other than "identity" if we can find one.

And, all that said, I've noticed that the bit's name doesn't actually appear in the API, so it doesn't matter that much, and if you're not convinced, I won't object to "verification" any more.

@AngeloKai
Copy link
Contributor Author

@equalsJeffH @jyasskin I just changed the name to "User Verification". Please review and tell me which name you like.

@AngeloKai AngeloKai changed the title Add Test of User Identity (TUI) bit to authenticator data Add User Verification (UV) bit to authenticator data Apr 20, 2017
Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

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

This PR is overall OK given that it constitutes a signed attestation that user verification occurred or did not. It harmonizes with the existing TUP bit (which I suggest we rename UP - 'user present', fwiw)

Note that after allocating this flag bit, we have four unalloc'd bits remaining.

index.bs Outdated
@@ -1263,6 +1263,7 @@ The [=authenticator data=] structure is a byte array of 37 bytes or more, as fol
<td>
Flags (bit 0 is the least significant bit):
- Bit 0: [=Test of User Presence=] (`TUP`) result.
- Bit 1: [=User Verification=] (`UV`) result.
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @AngeloKai considering feedback and migrating this to the "user verification" terminology.

Tho, actually, in thinking about the naming of both TUP and UV bits, I would use a present or past simple statement to name the above bits 0 and 1:

  • Bit 0: User Present Flag. This flag bit is set if the [=authenticator=] obtained a positive [=Test of User Presence=] result during either an [=authenticatorMakeCredential=] or an [=authenticatorGetAssertion=] operation.
  • Bit 1: User Verified Flag. This flag bit is set if the [=authenticator=] is both capable of [=user verification=], and obtained a positive [=user verification=] result during either an [=authenticatorMakeCredential=] or an [=authenticatorGetAssertion=] operation.

index.bs Outdated
@@ -1294,7 +1295,8 @@ does not change between operations but instead remains the same for the lifetime
by the authenticator during the [=authenticatorGetAssertion=] operation, by verifying that the RP ID associated with the
requested credential exactly matches the RP ID supplied by the client.

The `TUP` flag SHALL be set if and only if the authenticator detected a user through an authenticator specific gesture. The
The `TUP` flag SHALL be set if and only if the authenticator detected a user through an authenticator specific gesture. The
Copy link
Contributor

Choose a reason for hiding this comment

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

s/TUP/UP/ ? (ie, "User Present")

Also, given the language here, we seem to be saying that if UV has been accomplished, BOTH the UP (nee 'TUP') and UV flags would be set, yes?

I.e., UP is set if the authenticator is able to confirm user presence in any fashion, and UV is also set if that fashion is one able to verify that it is the "same user" as in prior interactions (e.g., credential creation).

Yes?

If so, this language ought to be polished appropriately (in both the above and below lines).

BTW, this is the way the FIDO User Verification Methods flags work -- USER_VERIFY_PRESENCE is always set, and then other flags MAY be set in order to signal finer-grained attributes of the interaction, see: https://fidoalliance.org/specs/fido-uaf-v1.1-id-20170202/fido-registry-v1.1-id-20170202.html#user-verification-methods

index.bs Outdated
@@ -1263,6 +1263,7 @@ The [=authenticator data=] structure is a byte array of 37 bytes or more, as fol
<td>
Flags (bit 0 is the least significant bit):
- Bit 0: [=Test of User Presence=] (`TUP`) result.
- Bit 1: [=User Verification=] (`UV`) result.
- Bits 1-5: Reserved for future use (`RFU`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this needs to be updated [per @jyasskin's earlier comment] to be "Bits 2-5" !

@equalsJeffH
Copy link
Contributor

@jyasskin wrote earlier:

I think it'd be good to capture the idea of "the same entity" in the name of this bit, even if that's captured in a word other than "identity". It could be the "same user" or "consistent user" bit?

Sure, if we name this bit "User Verified" per #409 (comment) -- then does that, in combo with this existing statement in the User Verification definition "The intent is to be able to distinguish individual users", satisfy your concern?

@selfissued
Copy link
Contributor

Jeff's proposed clarifications seem fine to me. Let's apply those and then merge it!

@selfissued
Copy link
Contributor

This looks like it does the job. Were we also going to rename "Test of User Present" to "User Present", as suggested? The naming would be more consistent that way.

@jyasskin
Copy link
Member

jyasskin commented Apr 24, 2017

@equalsJeffH Since the name of this bit doesn't actually show up in uses of the API, it's not that important, and I don't object to merging this PR. I do still think that the clear definitions of "verification" and "presence" don't fully mitigate that the words themselves are ambiguous.

@AngeloKai
Copy link
Contributor Author

@selfissued I'd prefer not to keep spreading out the changes. Once we merge this PR in, we can open a new PR to change TUP to UP.

@selfissued
Copy link
Contributor

Renaming TUP in a separate PR is fine too. In which case, I believe this is ready to merge.

@AngeloKai AngeloKai merged commit 55cd330 into w3c:master Apr 24, 2017
@AngeloKai
Copy link
Contributor Author

@selfissued merged as requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants