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

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

Closed
leshi opened this issue Apr 24, 2017 · 5 comments
Closed

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

leshi opened this issue Apr 24, 2017 · 5 comments

Comments

@leshi
Copy link
Contributor

leshi commented Apr 24, 2017

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.

@leshi leshi added this to the WD-05 milestone Apr 24, 2017
@equalsJeffH
Copy link
Contributor

@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 #378 (review)

@AngeloKai
Copy link
Contributor

AngeloKai commented Apr 24, 2017

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
Copy link
Contributor Author

leshi commented Apr 24, 2017

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
Copy link
Contributor

@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
Copy link
Contributor Author

leshi commented Apr 24, 2017

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.

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

No branches or pull requests

3 participants