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 ScopedCredentialInfo.publicKey; use "credential public key" term consistently #235

Merged
merged 8 commits into from Nov 30, 2016

Conversation

equalsJeffH
Copy link
Contributor

Fixes #94

@@ -531,7 +537,6 @@ authorizing an authenticator with which to complete the operation.
[SecureContext]
interface ScopedCredentialInfo {
readonly attribute ScopedCredential credential;
readonly attribute CryptoKey publicKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also delete lines 165-168 since there are no longer any references to CryptoKey or JsonWebKey left in this spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doh. thx :) See new commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we remove CryptoKey as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry. yes. see new commit. thx.

@equalsJeffH equalsJeffH added this to the WD-03 milestone Oct 18, 2016
@equalsJeffH equalsJeffH self-assigned this Oct 18, 2016
@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Oct 25, 2016

I've made some updates to this PR #235 and have also stashed a textual diff of this PR from the current state of the master branch here..

http://kingsmountain.com/doc/diff/diff-webauthn-index-master-1eebeed-from-index-jeffh-pubkey-94-4b3d958.pdf

please review.

@@ -1670,8 +1676,8 @@ with the fields of the attestation certificate's extension data.
- Verify that {{AndroidKeyAttestation/signature}} is a valid certificate chain, consisting of a time-valid X.509 certificate
chaining up to a trusted attestation root key.

- Verify that the public key in the attestation certificate matches the credential public key in the attestation data field
of the given <a>authenticatorData</a>.
- Verify that the public key in the attestation certificate matches the <a>credential public key</a> in the attestation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion based on earlier email thread: "attestation certificate" could be replaced with something like "leaf certificate in the chain represented by {{AndroidKeyAttestation/signature}}".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thx, will address this.

@vijaybh
Copy link
Contributor

vijaybh commented Nov 2, 2016

One editorial suggestion which captures something we also discussed before. Other than that this LGTM.

@bifurcation
Copy link
Contributor

This PR removes important functionality. Right now, it is possible for a RP to use this API without any knowledge of attestation formats (as long as it's willing to believe the browser that the key is good enough). With this PR, the RP can now only interoperate with an authenticator if it understand the attestation format produced by that authenticator, and it has to have code to process all the attestation formats on its critical path, before it can even look at the public key. Those seem like pretty serious constraints on interoperability and usability, so I'm inclined to close this PR.

@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Nov 4, 2016

Hi @bifurcation (Richard), thx for the review.

@bifurcation wrote:

With this PR, the RP can now only interoperate with an authenticator if it understand the attestation format produced by that authenticator

actually, that is no longer the case since PR #161 "make attestation more modular" which added this section..

{#generating-an-attestation-statement}

This section specifies the algorithm for generating an attestation statement, independent
of <a>attestation format</a>.

..and which specifies (via the table therein) a common format for conveying AAGUID and CredID and pubkey alg & encoding and attested public key, across all attstn formats.

Thus an RP may still, if it accepts the risk, simply pluck the attested public key from the attestation statement without understanding the various attstn formats (which remains necessary if the RP wishes to verify the attstn signature).

Additionally, this PR normalizes terminology, which we would want/need to do in any case.

@equalsJeffH
Copy link
Contributor Author

Ok, have addressed @vijaybh 's comment above. Would be good to have @rlin1 check the new language in {#android-key-attestation}.

The generated index.html likely has some dangling hyperlinks, at least it does when processed via the 'cloud (i.e., latest) bikeshed' -- we can fix these later i trust (they also exist in other branches I'm working on).

@equalsJeffH
Copy link
Contributor Author

I have fixed the dangling hyperlinks I've experienced in other branches I'm working on.

@equalsJeffH
Copy link
Contributor Author

webauthn 9-Nov meeting minutes https://www.w3.org/2016/11/09-webauthn-minutes.html has this regarding this PR:

We are now discussing PR #235

It'd be good to have Rolf double-check on the language for PR #235
Vijay is ok with the change
There are some contentious points about #235. Alexei will respond to that.
There is no clear documentation for CBOR mapping. This issue will addressed in a separate conversation.
Alexei will take a look at PR #235 before we merge it
Is any issue on WD-03 that is not addressed by a open pull request?

Alexei has been busy and not gotten to this, and we wish to get WD-03 published, and @vijaybh is ok with merging this, so am doing so. If there's issues with it, please file new issues, and we can address in another WD update.

@equalsJeffH equalsJeffH merged commit f891d0c into master Nov 30, 2016
@equalsJeffH equalsJeffH deleted the jeffh-pubkey-94 branch November 30, 2016 19:42
WebAuthnBot pushed a commit that referenced this pull request Nov 30, 2016
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.

None yet

3 participants