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.

Sign up for GitHub

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

Jump to bottom

Initial proposal for modifications to resident key credential requirements #1191

Merged
merged 25 commits into from May 8, 2019
Merged

Initial proposal for modifications to resident key credential requirements #1191

merged 25 commits into from May 8, 2019

Conversation

sbweeden
Copy link
Contributor

@sbweeden sbweeden commented Mar 25, 2019 •
edited by pr-preview bot

Attempts to fix #991 #1149

Outstanding issues/questions:

In the user-agent processing algorithm described in section 5.1.3, please check if the modifications in step 20 are possible w.r.t. CTAP. Specifically is it possible for a user-agent (client) to determine in an authenticator is capable of resident (or non-resident) key credentials?

In the IANA considerations section (11.2) I have added the credProps extension. How do I really register this with WebAuthn-Registries?

Preview | Diff

@sbweeden sbweeden mentioned this pull request Mar 25, 2019
Closed
emlun
emlun requested changes Mar 25, 2019
View reviewed changes
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, Shane! It looks pretty good overall to me, but could use some polish.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
emlun
emlun requested changes Mar 26, 2019
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
emlun
emlun previously requested changes Mar 26, 2019
View reviewed changes
index.bs Outdated Show resolved Hide resolved
This was referenced Mar 26, 2019
Closed
Merged
@emlun emlun mentioned this pull request Mar 27, 2019
Closed
@equalsJeffH
Copy link
Contributor

thanks for submitting this @sbweeden! I'm at IETF this week, wont be able to submit a review until next week...

@nadalin nadalin requested review from agl and akshayku April 3, 2019 07:12
@nadalin nadalin added this to the L2-WD-01 milestone Apr 3, 2019
@akshayku
Copy link
Contributor

akshayku commented Apr 3, 2019

This requires modifications according to latest discussion in the issue. See #991 (comment)

@sbweeden
Based on comments from WG call on 2019-04-03 (captured in w3c#991) I …
af5383d 
…have changed the set of permitted ResidentKeyRequirement values back to one of discouraged, preferred, required.
@sbweeden
Copy link
Contributor Author

@akshayku, @emlun, @agl I have changed the ResidentKeyRequirement enumeration back to my original proposal based on the comments from the 2019-04-03 WG call. Hoping this can be reviewed now.

akshayku
akshayku approved these changes Apr 17, 2019
View reviewed changes
Copy link
Contributor

@akshayku akshayku left a comment

Choose a reason for hiding this comment

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

LGTM

emlun
emlun reviewed Apr 17, 2019
View reviewed changes
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

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

LGTM, but I still think there is value in an "indifferent" option - not instead of "discouraged", but in addition to it. Even if there turns out to be no practical difference in how clients implement it, it expresses a different RP intent.

@emlun emlun dismissed their stale review April 17, 2019 14:09

Outdated

@emlun
Copy link
Member

emlun commented Apr 17, 2019

@akshayku talked earlier about having the client retry the authenticator invocation with different requireResidentKey arguments in the "preferred" and "discouraged" cases. Would it be useful to capture that retry logic here, or would that be too prescriptive?

@akshayku
Copy link
Contributor

@emlun That seems too prescriptive to me. Platforms/Browsers handling of these parameters to CTAP parameters is complex and I would not go over prescribing it.

I would concentrate on defining the intent from the RP's perspective correctly and its use cases.

@equalsJeffH
Copy link
Contributor

Hi @sbweeden -- please see: sbweeden#3

sbweeden and others added 2 commits May 1, 2019 14:12
@sbweeden
Copy link
Contributor Author

sbweeden commented May 1, 2019

I merged sbweeden#3 however made two small subsequent changes. The first was to fix a compile error related to indenting, and the other a small grammatical error of "a" vs "an" before a word starting with a vowel.

@equalsJeffH
Copy link
Contributor

@sbweeden -- great, thx, this is now ready for some re-review from others: @emlun @agl @akshayku others?

akshayku
akshayku approved these changes May 1, 2019
View reviewed changes
Copy link
Contributor

@akshayku akshayku left a comment •
edited

Choose a reason for hiding this comment

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

Overall looks good to me. Have some questions about this extension..

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
akshayku
akshayku requested changes May 1, 2019
View reviewed changes
Copy link
Contributor

@akshayku akshayku left a comment

Choose a reason for hiding this comment

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

Shane, are you proposing an extension to be implemented by external authenticators?? And Why?

index.bs Show resolved Hide resolved
@sbweeden
Copy link
Contributor Author

sbweeden commented May 1, 2019

Ready for re-review following removal of authenticator extension component of the credProps extension per WG call on May 1, 2019.

emlun
emlun reviewed May 2, 2019
View reviewed changes
index.bs Outdated Show resolved Hide resolved
equalsJeffH
equalsJeffH suggested changes May 2, 2019
View reviewed changes
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 LGTM overall, thx @sbweeden! there's one thing in the extension's IANA registration we need to fix.

index.bs Outdated Show resolved Hide resolved
@sbweeden
Copy link
Contributor Author

sbweeden commented May 2, 2019

Thanks for latest review @emlun and @equalsJeffH. I have addressed both the pick-ups, and found one additional dfn reference error which I fixed as well.

@emlun
Copy link
Member

emlun commented May 3, 2019

With the changes @agl proposes in fido-alliance/fido-2-specs#661 it looks to me like we actually could support forbidden here. Should we? (We could do that in a separate PR if we don't want to hold this one up for it)

emlun
emlun reviewed May 3, 2019
View reviewed changes
index.bs Outdated Show resolved Hide resolved
@equalsJeffH
Copy link
Contributor

@sbweeden

I have addressed both the pick-ups, and found one additional dfn reference error which I fixed as well.

Yep, looks good. changing 'credential' to 'pk cred source' is good catch, thx :)
Would be good to add the comma and fix grammar error that @emlun caught.

@emlun

With the changes @agl proposes in fido-alliance/fido-2-specs#661 it looks to me like we actually could support forbidden here. Should we? (We could do that in a separate PR if we don't want to hold this one up for it)

you mean add "forbidden" as a fourth value of ResidentKeyRequirement ?

@emlun
Copy link
Member

emlun commented May 3, 2019

@equalsJeffH: you mean add "forbidden" as a fourth value of ResidentKeyRequirement ?

Yes.

@equalsJeffH
Copy link
Contributor

I think we should merge this and separately consider possibly adding "forbidden" as a fourth value of ResidentKeyRequirement.

emlun
emlun approved these changes May 7, 2019
View reviewed changes
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

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

Thanks @sbweeden!

@akshayku akshayku merged commit 9315c0a into w3c:master May 8, 2019
WebAuthnBot pushed a commit that referenced this pull request May 8, 2019
Built by Travis-CI: 9315c0a Initial proposal for modifications to res…
635ad8e 
…ident key credential requirements (#1191)
WebAuthnBot pushed a commit that referenced this pull request May 8, 2019
Built by Travis-CI: 9315c0a Initial proposal for modifications to res…
d848f54 
…ident key credential requirements (#1191)
@equalsJeffH equalsJeffH mentioned this pull request May 16, 2019
Closed
@Kieun Kieun mentioned this pull request May 17, 2019
Closed
@lgarron lgarron mentioned this pull request Oct 13, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agl agl agl approved these changes

@equalsJeffH equalsJeffH equalsJeffH approved these changes

@emlun emlun emlun approved these changes

@akshayku akshayku akshayku approved these changes

Assignees

@sbweeden sbweeden

Labels
type:technical
Projects
None yet
Milestone
L2-WD-01
Development

Successfully merging this pull request may close these issues.

Indicate resident key credential "preferred" during registration and find out what the authenticator offered
6 participants
@sbweeden @equalsJeffH @akshayku @emlun @agl @nadalin