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

remove "required" on ScopedCredentialDescriptor.id #245

Closed
AngeloKai opened this issue Nov 2, 2016 · 10 comments
Closed

remove "required" on ScopedCredentialDescriptor.id #245

AngeloKai opened this issue Nov 2, 2016 · 10 comments

Comments

@AngeloKai
Copy link
Contributor

AngeloKai commented Nov 2, 2016

Although our current spec only has one credential type: “ScopedCred”, we may add more types to the spec in the future. In case more types were added in the future and developer hasn’t done works to adopt their server to accept newer types, they would want to only accept credentials of a certain type. However, our current design doesn’t provide a way for them to just search by type. Developer has to provide both credential type and credential id in allowList and excludeList to specify what credentials they find acceptable.

To provide more flexibility to the developer to search, I propose we remove the “required” keyword from the id parameter in ScopedCredentialDescriptor and revise the language in the spec about allowList and excludeList being lists of acceptable credentials. Instead, let’s change the language to a “list of search criteria to find credentials acceptable to the callers.”

@equalsJeffH equalsJeffH added this to the CR milestone Nov 4, 2016
@equalsJeffH equalsJeffH changed the title remove id requirement in ScopedCredentialDescriptor remove "required" on ScopedCredentialDescriptor.id Nov 4, 2016
@equalsJeffH equalsJeffH modified the milestones: WD-05, CR May 3, 2017
@AngeloKai AngeloKai modified the milestones: WD-06, WD-05 May 3, 2017
@jcjones
Copy link
Contributor

jcjones commented May 18, 2017

Making id optional for future expandability seems reasonable.

I think the language updates around usage are already handled. For example, using a credential right now says (emphasis mine):

This optional member contains a list of PublicKeyCredentialDescriptor object representing public key credentials acceptable to the caller, in decending order of the caller’s preference (the first item in the list is the most preferred credential, and so on down the line).

(ExcludeList looks fine as it's written now, too)

So I'd support a PR that marks PublicKeyCredentialDescriptor.id optional. I could go either way about adding the note to PublicKeyCredentialType to the effect that PublicKeyCredentialDescriptor.id is mandatory for "public-key" types, because I feel like it's self-evident when you have a key handle.

@equalsJeffH
Copy link
Contributor

@jcjones wrote:

I could go either way about adding the note to PublicKeyCredentialType to the effect that PublicKeyCredentialDescriptor.id is mandatory for "public-key" types

I'm confused -- which note? By "the note", perhaps you are referring to something said in conversation (eg on the webauthn call last week?) ?

Where you say "PublicKeyCredentialType" above, did you actually mean "PublicKeyCredentialDescriptor" ?

is mandatory for "public-key" types, because I feel like it's self-evident when you have a key handle.

So what you are suggesting here is that if the RP does have a credential ID for a cred of type "public-key", it must supply it in the PublicKeyCredentialDescriptor.id, regardless of authnr capabilities ?

Is this because some authnrs supporting "public-key" creds must be presented with the credential ID to do anything (eg U2F authnrs) ?

This is not the case for all authnrs supporting "public-key" creds, yes?

also, "key handle" is confusing -- that term only occurs in S 7.6. FIDO U2F Attestation Statement Format -- did you intend to say "credential ID" ?

@AngeloKai
Copy link
Contributor Author

AngeloKai commented Jun 15, 2017

My original post proposes two changes to the spec:

  1. Remove the "required" keyword from PublicKeyCredentialDescriptor.id, which is a member of the allowCredentials and excludeCredentials dictionary.
  2. Change the relevant prose to say that allowCredentials and excludeCredentials are "lists of search criteria to find credentials acceptable to the callers."

Reading from J.C.'s comment, I believe he's supportive of the first change and the second change is not necessary. In addition, he proposes another change based upon the previous two:

  1. Add requirements to the algorithm that "if the credential type is 'PublicKeyCredential', the id is required."

If we go with what J.C. proposes, there won't be any real API behavioral change except that we keep a door open for future credential types. Of course, this is @jcjones's comment and my interpretation could be off.

@AngeloKai
Copy link
Contributor Author

Attempted to be addressed by #500

@equalsJeffH
Copy link
Contributor

@AngeloKai wrote

Reading from J.C.'s comment, I believe he's supportive of the first change and the second change is not necessary

agreed.

In addition, [@jcjones] proposes another change based upon the previous two:
3. Add requirements to the algorithm that "if the credential type is 'PublicKeyCredential', the id is required."

Actually, @jcjones says "I could go either way" about the above.

Though, in reviewing PR #500, I am wondering whether the change proposed by this issue is actually necessary. You wrote:

In case more types were added in the future and developer hasn’t done works to adopt their server to accept newer types, they would want to only accept credentials of a certain type. However, our current design doesn’t provide a way for them to just search by type

..but an RP would in the above case restrict credential creation & registration to the cred type(s) it understands, yes? That can be accomplished today in the call to navigator.credentials.create() (which invokes the #createCredential alg) via appropriately setting the type value(s) in element(s) of the options.publicKey.parameters[] sequence, yes?

And so in subsequent calls to navigator.credentials.get() (which invoke the #getAssertion alg), a user's creds for this RP, on their authenticator(s), will already be of the type stipulated by the RP (unless the RP has altered which PK cred types it supports, and deprecated registered credentials, which is yet another use case, yes?).

The purpose of the options.allowCredentials[] parameter to #getAssertion, AIUI, is to identify specific creds "acceptable to the caller". Making options.allowCredentials[].id optional alters the semantics to be "any cred of this type, mapped to this RP ID, is 'acceptable'", if options.allowCredentials[].id is not present. Is that the desired behavior? What effect might it have on the user experience?

Additionally, I do not understand why we would stipulate that options.allowCredentials[n].id must be present only if options.allowCredentials[n].type's value is "public-key", and that not be the case if options.allowCredentials[n].type's value is something other than the latter.

Is this issue premised on an expectation that some new, different cred type may not have a notion of a "credential ID" ? Can you cite example(s) if so?

fwiw, I'm supportive of the overall theme of this issue -- facilitating other flavors of public key creds. Where "other flavors" consists of differences in assertion signature formats and differences in authenticator data formats, etc (eg webauthn as compared to UAF). However, all cred types supported by this spec are public key-based, and I tend to think that all types will have a notion of "credential ID" (webauthn, U2F, and UAF all do).

Perhaps I do not understand the problem you are trying to address?

@equalsJeffH
Copy link
Contributor

equalsJeffH commented Jun 28, 2017

we moved the relevant PR #500 to milestone CR on the webauthn call today 28-Jun-2017, so doing same with this issue.
update: fixed call date

@equalsJeffH equalsJeffH modified the milestones: CR, WD-06 Jun 28, 2017
@AngeloKai
Copy link
Contributor Author

Given that we can always remove 'require' (go from more restrictive to less restrictive) and the goal of the issue is to address other flavors of public key credentials (not in scope for v1), I am moving the issue to v2. @equalsJeffH @jcjones please let me know if you disagree.

@AngeloKai AngeloKai modified the milestones: L2-WD-00, CR Sep 6, 2017
@nadalin nadalin added this to the WD-07 milestone Sep 6, 2017
@nadalin nadalin modified the milestones: L2-WD-00, WD-07 Sep 6, 2017
@AngeloKai AngeloKai removed their assignment Jun 19, 2018
@nadalin
Copy link
Contributor

nadalin commented Feb 20, 2019

@equalsJeffH Can we close this as it looks like its obsolete

@jyasskin
Copy link
Member

As @AngeloKai said, you can always remove "required" later when you encounter a credential type that doesn't want to require the id field. Until then, it seems confusing to remove it from the dictionary but still require the field from prose. So +1 for closing this issue and #500.

@equalsJeffH
Copy link
Contributor

Closing this given #245 (comment), #245 (comment), #245 (comment)

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

5 participants