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

Define Public Key Credential Source and Credential ID. #620

Merged
merged 5 commits into from
Nov 2, 2017

Conversation

jyasskin
Copy link
Member

@jyasskin jyasskin commented Oct 9, 2017

This also broadens the definition of "Public Key Credential" to include the several ways it's used, including the private key, attested public key, and assertion. This is a willful violation of RFC4949.

Credential ID is defined to explicitly include the possibility that it's the encrypted Credential Source.


Preview (#credential-i…) (#public-key-c…) (#public-key-c…) | Diff

@emlun
Copy link
Member

emlun commented Oct 11, 2017

You're right @jyasskin, #393 does define credentialId which would kind-of conflict with this one. I'll narrow the scope on the one in #393 somehow.

@jyasskin
Copy link
Member Author

@rlin1 You've got a bunch of comments on #623 that are actually about this change.

The basic issue is about what to call the 3 things involved with using a credential:

  1. The secret or capability that the client possesses.
  2. The proof that the client has the secret/capability.
  3. The thing that lets the RP verify the proof.

We have a couple different kinds of credentials, and I want the terms to be consistent across them. I think I'm fine with your suggestion of:

  1. Credential
  2. Assertion (We'd probably have to call this a "Credential Assertion" in general, but we could use the shorthand within this spec.)
  3. No suggestion, but I'd say maybe "Credential Verifier"?

I think those work for passwords (the password is all three) and for SMS auth (1: SIM card; 2&3: OTP).

One difficulty is that credentials.get() returns a Credential rather than a CredentialAssertion, but we could just live with that inconsistency.

@mikewest, how do you feel about these names if I were to send a patch to Credential Manager?

@selfissued selfissued added this to the WD-07 milestone Oct 12, 2017
@nadalin
Copy link
Contributor

nadalin commented Oct 14, 2017

@jyasskin any reason why this can't be merged ?

@jyasskin
Copy link
Member Author

@nadalin: Yes. @rlin1 objected to the particular definitions I wrote, and I haven't yet modified the definitions to match his preference. (The group could also say to override his preference, but I don't expect that outcome.)

@mikewest
Copy link
Member

@mikewest, how do you feel about these names if I were to send a patch to Credential Manager?

Today, the CM API calls the thing that's handed to an origin to authenticate a user a Credential. It sounds like you'd like to change that to Credential Assertion or Assertion? And call the underlying secret thing that enables generation of the assertion Credential? I think I'm ok with that distinction, but we'll have to figure out what the web-facing implementation would look like (we're using Credential today for the thing I think you want to call Assertion, for instance).

@nadalin
Copy link
Contributor

nadalin commented Oct 17, 2017

@jyasskin This seems like a breaking change if we change at this point, not sure this is a wise thing to do as we don't gain much from this

@jyasskin
Copy link
Member Author

@mikewest That's right, and now that I'm thinking about this again, I'm not sure why I was generally positive on @rlin1's suggested naming (although I still don't hate it). We (Mike and I) talked about this during the merge, and decided that we should use the short name ("Credential") for the concept that's actually used by Javascript. Using Assertion in Javascript is probably not ok, since there can be other kinds of assertions, and using CredentialAssertion in JS when JS never sees Credential is kinda confusing.

I won't be at the WebAuthn meeting Wednesday, but I'd appreciate if the group could develop an opinion about the naming choices and report back. The choices so far are, for the capability/proof-of-possession/verifier:

  1. My "Credential Source"/"Credential"/"Credential Verifier"
  2. @rlin1's "Credential"/"Credential Assertion"/"Credential Verifier"

We could also use the "Credential Assertion" terminology in WebAuthn, but still call the type Credential in Javascript, although that might be confusing.

@nadalin I'm not sure what changes in here look breaking. This patch is just a bunch of definitions. Renaming the Credential type is breaking, but that's not proposed yet.

@mikewest
Copy link
Member

We could also use the "Credential Assertion" terminology in WebAuthn, but still call the type Credential in Javascript, although that might be confusing.

That does seem confusing.

I'd be happiest with your #1 ("Credential Source"/"Credential"/"Credential Verifier"), given that I'm reluctant to change the web-exposed JS interface at this point.

@emlun
Copy link
Member

emlun commented Oct 18, 2017

I think "Credential Source" is technically correct but intuitively feels more like something that creates private keys. How about "Identity"? As in, an authenticator contains/represents an identity and generates credentials proving that identity. That also happens to match up neatly with "credential ID".

@nadalin
Copy link
Contributor

nadalin commented Oct 18, 2017

@jyasskin OK, I thought I was reading that from what @mikewest was posting

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.

Looks good to me.

@equalsJeffH
Copy link
Contributor

I am reviewing this and PR #623

@equalsJeffH
Copy link
Contributor

It looks like this branch needs a merge-from-master -- will it help if I do that? I think i have contributor perms to jyassking/webauthn.

@jyasskin
Copy link
Member Author

@emlun If I understand correctly, "identity" has other connotations that we don't want to apply to the private key owned by an authenticator, even though I agree that the private key can be used to authenticate as an identity.

@equalsJeffH I've rebased, but I haven't gone through @rlin1's comments to see what other terminology we should update if we stick with "Credential Source" as the name of the private key and associated information. If you can endorse particular items, I'll update their names in a new commit.

  • Credential ID -> Credential Source ID
  • assertion -> credential globally
  • PublicKeyCredentialDescriptor -> PublicKeyCredentialSourceDescriptor

Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

This looks OK to me.

@equalsJeffH
Copy link
Contributor

@jyasskin: if I understand correctly, you are suggesting these further changes to add to this PR..

  • Credential ID -> Credential Source ID
  • assertion -> credential globally
  • PublicKeyCredentialDescriptor -> PublicKeyCredentialSourceDescriptor

..yes?

Those above suggestions would be breaking changes. It might be possible to convince folks to do "Credential ID -> Credential Source ID" -- it would only (breaking) change credentialidlength and credentialid in section #sec-attested-credential-data.

Otherwise, IIUC, I do not support the other two changes at this time, and am not sure they would buy us that much in general. Those latter two changes, especially renaming "assertion", would be quite invasive, both editorially and technically (ie, in renaming API components, section names, terminology, etc, senses).

In any case, I have detail-level comments that I'm working on entering in that I generally support this PR's present substance.

@jyasskin
Copy link
Member Author

jyasskin commented Oct 24, 2017

@equalsJeffH Sounds good. I'm happy not adding any extra changes to this PR too; just wanted to get an explicit opinion on @rlin1's apparent suggestions.

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.

Thanks for taking a stab at this @jyasskin. I think I understand and generally agree with your desire to reconcile terminology between webauthn and credential-management -- especially the notion of credential source.

But overall it seems ok for there to be a bit of a terminology impedance mismatch between webauthn and credential-management: e.g., a webauthn assertion is an example what credman terms a single-use credential.

I have a bunch of detail-level comments below but I have given up on them and propose I submit a proposed in-whole build on these Credential ID, Credential public key, public key cred source, and public key cred definitions. (so the comments below are sort of hints wrt what I'm thinking)

Though, if we truly are not going to make any breaking renaming changes between this and PR #623 (I have comments to also submit on that), then I think we can punt this to CR and I can craft up the proposal after completing what I need to yet do for WD-07.

WDYT?

index.bs Outdated
:: A probabilistically-unique [=byte sequence=] identifying a [=public key credential source=].

Credential IDs are generated by [=authenticators=] in two forms:
1. Bytes with at least 100 bits of entropy, or
Copy link
Contributor

Choose a reason for hiding this comment

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

what would the be min length for 100 bits of entropy? I am thinking there ought to be a not-less-than-this-number-of-bytes stipulation here along with the "with at least 100 bits of entropy" stipulation.

Copy link
Member Author

Choose a reason for hiding this comment

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

"At least 16 bytes"?

I don't think this actually gains us much vs the obvious 13 bytes needed to store 100 bits. RPs can't optimize based on knowing they'll always use at least 16 bytes, and a broken authenticator that gives fewer bits of entropy wouldn't also return fewer bytes to flag that.

index.bs Outdated
Credential IDs are generated by [=authenticators=] in two forms:
1. Bytes with at least 100 bits of entropy, or
1. The [=public key credential source=], without its [=public key credential source/id=] item, encrypted so only the owning
authenticator can decrypt it.
Copy link
Contributor

Choose a reason for hiding this comment

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

hm. this seems to be suggesting to wrap (ie, bundle up & encrypt) a whole passel of stuff to form the Credential ID, given what all is listed in the [=public key credential source=] definition.

In U2F, only the private key and appID are wrapped. In UAF, the wrapped tuple is ( KHAccessToken, cred private key (UAuth.priv), userhandle size, userhandle ).

We need to explicitly decide what is wrapped for "webauthn authenticators".

Also, we ought to provide some guidance on key wrapping algs and such. Either we should cite e.g. [SP800-57] and [SP800-38F] as UAF does, or perhaps just cite FIDO Authenticator Allowed Cryptography List

Copy link
Contributor

@jovasco jovasco Oct 25, 2017

Choose a reason for hiding this comment

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

In U2F, there is no definition of what is inside a key handle. It is defined as an opaque blob of up to 256 bytes (but private key and AppID are a functional minimum). It just says these fields MAY be included.

I believe webauthn should not try to define what is inside this blob. The contents will depend on the authenticator needs (with or without display, first or second factor, local storage, supported extensions, ...). It's pretty obvious what to include based on what the vendor chooses to support. I believe trying to define it will either impose unnecessary limits or just be stating the obvious. It can't be verified anyway.

As for guidance of how it is wrapped: make sure it cannot collide with the requirements of the FIDO security requirements working group.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fundamentally, the list here is the private key, the rpId (which is equivalent to the appID and I think also to the KHAccessToken), the user handle, and optional extra information to inform any UI the authenticator might expose. I think the user handle should also be optional to accommodate U2F. Are there any other fields we should explicitly call out here?

I'd rather have a separate PR to constrain the key-wrapping algorithms. For that change ... I'm not confident we should constrain those, although non-normatively referring to the Allowed Cryptography list seems fine. Is that list going to keep updating in the future as new vulnerabilities are discovered and new algorithms are invented? What's the stable reference to it? Will it be an IANA registry?

index.bs Outdated
: <dfn>Credential ID</dfn>
:: A probabilistically-unique [=byte sequence=] identifying a [=public key credential source=].

Credential IDs are generated by [=authenticators=] in two forms:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have more description of the purposful difference between the two forms. the 2nd form allowing an authnr to be more or less stateless by storing the state back with the RP server. We ought to say this explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a rough description of this benefit of the second form.

index.bs Outdated
1. The [=public key credential source=], without its [=public key credential source/id=] item, encrypted so only the owning
authenticator can decrypt it.

[=[RPS]=] don't need to distinguish these two kinds of Credential IDs.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest:
[=[RPS]=] do not need to distinguish these two [=redential ID=] forms.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
: <dfn>otherUI</dfn>
:: Other information used by the authenticator to inform its UI. For example, this might include the user's
{{displayName}}.
</dl>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. We've been trying to keep the explanatory/intro text free of gnarly code-like details, and link to the latter as appropriate. I note that the above struct layout is duplicated down in section authenticatorMakeCredential (#op-make-cred) over in PR #623. I'm thinking that the latter (or nearby there) is the correct place for it, rather than here in terminology section.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, done. Thanks for pushing back when explanatory sections are too code-like.

index.bs Outdated
{{displayName}}.
</dl>

The [=authenticatorMakeCredential=] operation creates [=public key credential sources=] and returns the matching
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest:
The [=authenticatorMakeCredential=] operation creates [=public key credential sources=] bound to the [=authenticator=] and returns the associated

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reworded this a little more, but included your suggestion.

index.bs Outdated
: <dfn>Public Key Credential</dfn>
:: An instance of {{PublicKeyCredential}} in which {{PublicKeyCredential/response}} holds an {{AuthenticatorAssertionResponse}}
proving possession of a particular [=credential private key=].

Copy link
Contributor

Choose a reason for hiding this comment

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

well, there's two flavors of PK Cred returned -- an attested one from #createCredential aka [[Create]](), and an asserted proof-of-possession one (i.e., an assertion) from #getAssertion aka [[DiscoverFromExternalSource]]().

I'm wondering whether we can use the qualified terms "attested public key credential" and "public key credential assertion" (aka: an assertion in short form) to distinguish them.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have terms for both of those uses ("attestation object" and "authentication assertion"), so I'd rather not define additional synonyms.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

index.bs Outdated
:: An instance of {{PublicKeyCredential}} in which {{PublicKeyCredential/response}} holds an {{AuthenticatorAssertionResponse}}
proving possession of a particular [=credential private key=].

Note: This is a [=willful violation=] of [[RFC4949]]. In English, a "credential" is both a) the thing presented to prove a
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is important to note, thanks. tho, RFC4949 is only "Informational", so not strictly necessary to call out, but I think it is a good idea to do so.

index.bs Outdated
proving possession of a particular [=credential private key=].

Note: This is a [=willful violation=] of [[RFC4949]]. In English, a "credential" is both a) the thing presented to prove a
statement and b) intended to be used multiple times. It's impossible to achieve both criteria with a single piece of data in
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest:
s/criteria/criteria securely/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
: <dfn>Credential Public Key</dfn>
:: The public key portion of an [=[RP]=]-specific <dfn>credential key pair</dfn>, generated by an [=authenticator=] and
returned to an [=[RP]=] at [=registration=] time (see also [=public key credential=]). The private key portion of the
[=credential key pair=] is known as the <dfn>credential private key</dfn>. Note that in the case of [=self
attestation=], the [=credential key pair=] is also used as the [=attestation key pair=], see [=self attestation=]
for details.

: <dfn>Public Key Credential Source</dfn>
:: A [=credential source=] that can generate [=public key credentials=].
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we can at this time reasonably change "assertion" to be "public key credential", which is what the above statement is attempting, IIUC.

Plus, we have designed the PublicKeyCredential object such that it may be used to convey attested credential data (at credential source creation time) or not (at assertion generation time).

suggest:
A [=credential source=] used by an [=authenticator=] to generate [=authentication assertions=].

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Per 2017-10-25 meeting discussion, I've updated the whole change to use "assertion" where it had been using "credential", and to treat "credential" as the union of "assertions" and "credential sources".

@equalsJeffH
Copy link
Contributor

reviewed: #620 (review)

@equalsJeffH
Copy link
Contributor

also on pr #620, i would like to find a way to retain a fair bit of the current public key credential terminology definition rather than just supplanting it, thanks.

Copy link
Member Author

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

@equalsJeffH I've tried to keep more of the current "public key credential" definition.

index.bs Outdated
: <dfn>Credential ID</dfn>
:: A probabilistically-unique [=byte sequence=] identifying a [=public key credential source=].

Credential IDs are generated by [=authenticators=] in two forms:
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a rough description of this benefit of the second form.

index.bs Outdated
:: A probabilistically-unique [=byte sequence=] identifying a [=public key credential source=].

Credential IDs are generated by [=authenticators=] in two forms:
1. Bytes with at least 100 bits of entropy, or
Copy link
Member Author

Choose a reason for hiding this comment

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

"At least 16 bytes"?

I don't think this actually gains us much vs the obvious 13 bytes needed to store 100 bits. RPs can't optimize based on knowing they'll always use at least 16 bytes, and a broken authenticator that gives fewer bits of entropy wouldn't also return fewer bytes to flag that.

index.bs Outdated
Credential IDs are generated by [=authenticators=] in two forms:
1. Bytes with at least 100 bits of entropy, or
1. The [=public key credential source=], without its [=public key credential source/id=] item, encrypted so only the owning
authenticator can decrypt it.
Copy link
Member Author

Choose a reason for hiding this comment

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

Fundamentally, the list here is the private key, the rpId (which is equivalent to the appID and I think also to the KHAccessToken), the user handle, and optional extra information to inform any UI the authenticator might expose. I think the user handle should also be optional to accommodate U2F. Are there any other fields we should explicitly call out here?

I'd rather have a separate PR to constrain the key-wrapping algorithms. For that change ... I'm not confident we should constrain those, although non-normatively referring to the Allowed Cryptography list seems fine. Is that list going to keep updating in the future as new vulnerabilities are discovered and new algorithms are invented? What's the stable reference to it? Will it be an IANA registry?

index.bs Outdated
1. The [=public key credential source=], without its [=public key credential source/id=] item, encrypted so only the owning
authenticator can decrypt it.

[=[RPS]=] don't need to distinguish these two kinds of Credential IDs.
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
: <dfn>Credential Public Key</dfn>
:: The public key portion of an [=[RP]=]-specific <dfn>credential key pair</dfn>, generated by an [=authenticator=] and
returned to an [=[RP]=] at [=registration=] time (see also [=public key credential=]). The private key portion of the
[=credential key pair=] is known as the <dfn>credential private key</dfn>. Note that in the case of [=self
attestation=], the [=credential key pair=] is also used as the [=attestation key pair=], see [=self attestation=]
for details.

: <dfn>Public Key Credential Source</dfn>
:: A [=credential source=] that can generate [=public key credentials=].
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Per 2017-10-25 meeting discussion, I've updated the whole change to use "assertion" where it had been using "credential", and to treat "credential" as the union of "assertions" and "credential sources".

index.bs Outdated
: <dfn>otherUI</dfn>
:: Other information used by the authenticator to inform its UI. For example, this might include the user's
{{displayName}}.
</dl>
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, done. Thanks for pushing back when explanatory sections are too code-like.

index.bs Outdated
{{displayName}}.
</dl>

The [=authenticatorMakeCredential=] operation creates [=public key credential sources=] and returns the matching
Copy link
Member Author

Choose a reason for hiding this comment

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

I've reworded this a little more, but included your suggestion.

index.bs Outdated
proving possession of a particular [=credential private key=].

Note: This is a [=willful violation=] of [[RFC4949]]. In English, a "credential" is both a) the thing presented to prove a
statement and b) intended to be used multiple times. It's impossible to achieve both criteria with a single piece of data in
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
: <dfn>Public Key Credential</dfn>
:: An instance of {{PublicKeyCredential}} in which {{PublicKeyCredential/response}} holds an {{AuthenticatorAssertionResponse}}
proving possession of a particular [=credential private key=].

Copy link
Member Author

Choose a reason for hiding this comment

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

We have terms for both of those uses ("attestation object" and "authentication assertion"), so I'd rather not define additional synonyms.

This also redefines "Public Key Credential" to be the thing presented to the RP,
as a willful violation of RFC4949.

Credential ID is defined to explicitly include the possibility that it's the
encrypted Credential Source.
…d assertions.

Also fix some comments from equalsJeffH.
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.

overall LGTM -- I do have some detail-level comments, thanks :)

index.bs Outdated

Credential IDs are generated by [=authenticators=] in two forms:
1. At least 16 bytes that include at least 100 bits of entropy, or
1. The [=public key credential source=], without its [=Credential ID=], encrypted so only the owning authenticator can
Copy link
Contributor

Choose a reason for hiding this comment

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

wrt "encrypted": we need to specify/reference key-wrapping criteria. E.g., see the "Cryptographic (Crypto) Kernel", "KeyHandle", and "Wrap.sym" sections of the table at https://fidoalliance.org/specs/fido-uaf-v1.1-id-20170202/fido-uaf-authnr-cmds-v1.1-id-20170202.html#security-guidelines

Once these webauthn mods are merged-to-master, we need to submit an issue regarding this.

Copy link
Contributor

Choose a reason for hiding this comment

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

There could also be a third form: some hash value derived from the public key or some encrypted data. We might want to rephrase the option 1 to include such approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or do you consider such option being included in 1. already?

Copy link
Member Author

Choose a reason for hiding this comment

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

@equalsJeffH I've added a note referring to those guidelines for now.

@rlin1 I changed item 1 in f1f9a07 to talk about bits of entropy instead of random numbers in order to include hashes.

index.bs Outdated
* A [=Credential ID=].
* A [=credential private key=].
* The [=Relying Party Identifier=] for the [=[RP]=] that created this credential source.
* An optional [=user handle=] in which the [=[RP]'s=] describes the person who created this credential source.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/[=[RP]'s=]/[=[RP]=]/

the user handle is specifically intended to be a RP user account database primary key--i.e., "A user handle is an opaque byte sequence..."--we should be consistently describing it as such.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, maybe "user handle for the person who"?

index.bs Outdated
* Optional other information used by the authenticator to inform its UI. For example, this might include the user's
{{displayName}}.

The [=authenticatorMakeCredential=] operation creates a [=public key credential source=] bound to an [=authenticator=] and
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, ok, so we can define "owning authenticator" here:
s/bound to an [=authenticator=]/bound to an <dfn>owning authenticator</dfn>/ -- yes?

[aside: I personally do not like the prevalent use of "own"/"owning"/etc in the computer&network world (it has connotations that do not hold in cyberspace), and would personally prefer to us "managing", though can grit teeth and live with "owning" if no one else cares to join Sisyphus here...]

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not attached to "owning", and I'd be introducing the first use in this spec. Done.

index.bs Outdated
times (the public key), while this specification gives "credential" the English term's flexibility. This specification
uses more specific terms to identify the data related to an [[RFC4949]] credential:

: "Authentication information", including the private key
Copy link
Contributor

Choose a reason for hiding this comment

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

s/, including the private key/ (possibly including a private key)/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated

: <dfn>Public Key Credential</dfn>

:: A WebAuthn [=public key credential=] is either a [=public key credential source=], the
Copy link
Contributor

Choose a reason for hiding this comment

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

overall I like this "public key credential" definintion, thanks for bearing with me :)

I do remain partial, tho, to retaining the opening sentence of "Generically, a credential is data one entity presents to another in order to authenticate the former to the latter [RFC4949].", if that's ok with folks -- i think it appropriately sets the overall context for the reader.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand this: here public key credential could be equal to a public key credential source. That confuses me.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, got it: The term "public key credential" is used for both, the public key and the assertion.

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps:
s/A webauthn/The term/
s/is either/refers to/
..?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Subsequently, only that [=[RP]=], as identified by its [=RP ID=], is able to employ the [=public key credential=] in
[=authentication|authentication ceremonies=], via the {{CredentialsContainer/get()}} method. The [=[RP]=] uses its stored
copy of the [=credential public key=] to verify the resultant [=authentication assertion=].

Copy link
Contributor

Choose a reason for hiding this comment

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

great, thanks for including the above :)

@equalsJeffH equalsJeffH dismissed their stale review November 1, 2017 17:19

because revised version LGTM

index.bs Outdated
@@ -324,13 +324,60 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S
:: A user agent implementing, in conjunction with the underlying platform, the [=Web Authentication API=] and algorithms
given in this specification, and handling communication between [=authenticators=] and [=[RPS]=].

: <dfn>Credential ID</dfn>
:: A probabilistically-unique [=byte sequence=] identifying a [=public key credential source=].
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this term (public key credetial source) is a good fit here.
When I look at the def of pk cred source below, this sounds pretty much like the authenticator itself.
But the Credential ID should identify a specific credential, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the term credential for the authentication key (pair).

Copy link
Contributor

Choose a reason for hiding this comment

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

The current definition of Credential Public Key also suggests that meaning (IMHO)

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that you were reviewing an old version here.

index.bs Outdated
:: A probabilistically-unique [=byte sequence=] identifying a [=public key credential source=].

Credential IDs are generated by [=authenticators=] in two forms:
1. At least 128 random bits, or
Copy link
Contributor

Choose a reason for hiding this comment

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

would propose 100 bits.

index.bs Outdated
: <dfn>Credential Public Key</dfn>
:: The public key portion of an [=[RP]=]-specific <dfn>credential key pair</dfn>, generated by an [=authenticator=] and
returned to an [=[RP]=] at [=registration=] time (see also [=public key credential=]). The private key portion of the
[=credential key pair=] is known as the <dfn>credential private key</dfn>. Note that in the case of [=self
attestation=], the [=credential key pair=] is also used as the [=attestation key pair=], see [=self attestation=]
for details.

: <dfn>Public Key Credential Source</dfn>
:: A [=credential source=] that can generate [=public key credentials=].
Copy link
Contributor

Choose a reason for hiding this comment

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

sounds like authenticator. Is that intentional?

index.bs Outdated
: <dfn>rpId</dfn>
:: The [=Relying Party Identifier=] for the [=[RP]=] that created this credential source.
: <dfn>userHandle</dfn>
:: The [=user handle=] associated when this credential source was created.
Copy link
Contributor

Choose a reason for hiding this comment

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

why credential source here and not credential?

index.bs Outdated
a public key system. [[RFC4949]] chooses to define a credential as the thing that can be used multiple times (the public
key), while this specification chooses to define a credential as the proof.

A [=public key credential=] is created by the [=authenticatorGetAssertion=] operation using a [=public key credential
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, got it now. Until now I thought we used the term credential for the authentication public/private key (and not for the signature assertion).
Here it seems you want to use the term public key credential for the assertion itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am quite sure we would have to check all other occurences of public key credential to be in-line with this re-defined meaning.

Copy link
Contributor

Choose a reason for hiding this comment

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

This terminology is not aligned with the authenticatorMakeCredential call.
MakeCredential only makes sense if the key pair is the credential.

index.bs Outdated
:: An instance of {{PublicKeyCredential}} in which {{PublicKeyCredential/response}} holds an {{AuthenticatorAssertionResponse}}
proving possession of a particular [=credential private key=].

Note: This is a [=willful violation=] of [[RFC4949]]. In English, a "credential" is both a) the thing presented to prove a
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the term credential for the authentication key pair and assertion for the data we send to the RP.

index.bs Outdated

Credential IDs are generated by [=authenticators=] in two forms:
1. At least 16 bytes that include at least 100 bits of entropy, or
1. The [=public key credential source=], without its [=Credential ID=], encrypted so only the owning authenticator can
Copy link
Contributor

Choose a reason for hiding this comment

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

There could also be a third form: some hash value derived from the public key or some encrypted data. We might want to rephrase the option 1 to include such approach.

index.bs Outdated

Credential IDs are generated by [=authenticators=] in two forms:
1. At least 16 bytes that include at least 100 bits of entropy, or
1. The [=public key credential source=], without its [=Credential ID=], encrypted so only the owning authenticator can
Copy link
Contributor

Choose a reason for hiding this comment

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

Or do you consider such option being included in 1. already?

index.bs Outdated

: <dfn>Public Key Credential</dfn>

:: A WebAuthn [=public key credential=] is either a [=public key credential source=], the
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand this: here public key credential could be equal to a public key credential source. That confuses me.

index.bs Outdated

: <dfn>Public Key Credential</dfn>

:: A WebAuthn [=public key credential=] is either a [=public key credential source=], the
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, got it: The term "public key credential" is used for both, the public key and the assertion.

@equalsJeffH
Copy link
Contributor

LGTM! thanks @jyasskin!

@jyasskin jyasskin merged commit c647b70 into w3c:master Nov 2, 2017
@jyasskin jyasskin deleted the define-credential-source branch November 2, 2017 19:38
WebAuthnBot pushed a commit that referenced this pull request Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants