search

w3c / webauthn

Web Authentication: An API for accessing Public Key Credentials
https://w3c.github.io/webauthn/
Other
1.16k stars 166 forks source link

Why was PR #409 (UV bit) merged? #424

Closed leshi closed 7 years ago

leshi commented 7 years ago

Hi all,

I really don't think this PR (#409) should have been merged.

First, the corresponding PR in CTAP has not been merged or is it not a dependency?

Second, I don't think this is needed. Why can't you specify what type of user presence/verification check is needed during key (credential) creation? The attestation will tell you what type of key was made by this authenticator and then subsequent signatures coming from that credential will tell you that the UV was enforced.

equalsJeffH commented 7 years ago

@leshi wrote:

I really don't think this PR (#409) should have been merged.

I tend to agree with @leshi. i note that his comment on PR #409 was never answered/addressed.

Also, it appears that this sentence was crafted in response to one of my comments on the PR:

If the authenticator's user verification procedure also obtained a positive [=Test of User Presence] result, the TUP flag would be set as well.

..but that sentence is suboptimal from a spec perspective. It is implying that testing user presence could be different than obtaining user verification. Is it? Or is it always the case that if one has verified the user, you've also verified presence, by definition? also, we should probably be using MUST in the bit 0 & bit 1 descriptions wrt being set.

@leshi wrote:

Second, I don't think this is needed. Why can't you specify what type of user presence/verification check is needed during key (credential) creation?

Ah, and this would be yet another authenticator attribute RP-driven authenticator selection, further begging the question of having a framework to express and handle that, or continuing to approach it in an ad-hoc manner. See https://github.com/w3c/webauthn/pull/378#pullrequestreview-34141186

AngeloKai commented 7 years ago

i note that his comment on PR #409 was never answered/addressed.

It never addressed because the PR is to the CTAP spec, which is part of FIDO. The on-going CTAP spec work is not public so I don't believe I can share it publicly on GitHub. I will follow up with Alexei to share it. But I don't think it matters given that a number of parts of the Web API already predicate the CTAP spec.

Second, I don't think this is needed. Why can't you specify what type of user presence/verification check is needed during key (credential) creation? The attestation will tell you what type of key was made by this authenticator and then subsequent signatures coming from that credential will tell you that the UV was enforced.

I am perfectly ok with adding a new optional parameter to help authenticator selection. This just adds an optional bit for developers know whether user verification was performed. I don't know how you would know UV is performed without this bit in. The create method returns clientDataJSON and attestationObject. Neither of which will let the developer knows this. Are you suggesting that the RP can figure this out by looking at AAGUID? That way adds much more work. Plus the AAGUID database is not up-and-running yet.

Finally, the reason why I merge the PR is because I haven't seen strong objection to the idea of adding this bit but rather editorial changes which are proposed by Jeff and Jeffrey. I have incorporated all the editorial asks by them. Understandably, one sentence may not be optimal per Jeff's comment above. I do apologize for that sentence and I am perfectly happy with opening up a new PR to address this. Finally, we have said on last week's call that if you have an objection, you will review the PR by the end of the week and let us know if you don't think the idea is good.

leshi commented 7 years ago

I don't know how you would know UV is performed without this bit in.

We should add it to the attestation -- that this type of authenticator is the kind that verifies the user :)

AngeloKai commented 7 years ago

@leshi Are you talking about attestation object? As shown in this graph, authenticator data is part of attestation. The PR adds one bit to the authenticator data, therefore adding one bit to the attestation object.

I did forget about adding that bit in the attestation graph. I will open up a PR to do so in a minute.

leshi commented 7 years ago

I'm saying that I would like to be very cautions about burning those bits, there are only 7 of them left -- there are other ways of expressing this like putting it in the attestation or as an extension.