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

Add getPublicKey method. #1395

Merged
merged 5 commits into from May 14, 2020
Merged

Add getPublicKey method. #1395

merged 5 commits into from May 14, 2020

Conversation

agl
Copy link
Contributor

@agl agl commented Mar 25, 2020

This change adds a getPublicKey method to the
AuthenticatorAttestationResponse to save some users from having to parse
out and handle COSE keys.

(See linked issue for background.)

Fixes #1363


Preview | Diff

This change adds a getPublicKey method to the
AuthenticatorAttestationResponse to save some users from having to parse
out and handle COSE keys.

(See linked issue for background.)

Fixes w3c#1363
@agl
Copy link
Contributor Author

agl commented Mar 25, 2020

For discussion: if we want to avoid uses having to grub around in the authenticator data, should we also provide methods to get the signature count and UP/UV flags?

@agl
Copy link
Contributor Author

agl commented Mar 25, 2020

From the call: yes, we should add more accessors for other stuff that people are likely to need.

@equalsJeffH
Copy link
Contributor

on 2020-03-25 call: noted that RPs that come to rely upon these accessors will need to do feature detection appropriately in case they end up running in older browsers -- call this out in spec in a Note at least. also, add accessors for select other portions of the attestationObject -- tho how many/which ones?

@equalsJeffH
Copy link
Contributor

on 2020-04-01 call: @agl to update, then reviewers to review :)

@agl
Copy link
Contributor Author

agl commented Apr 3, 2020

  • Added note about feature detection.
  • Added getAuthenticatorData to extract that from the attestation object.
  • Added note about why accessors for the flags and signature counter would not be helpful.

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, thx @agl! some comments to discuss...

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 self-requested a review April 8, 2020 19:14
@equalsJeffH
Copy link
Contributor

on 2020-04-08 call: please review!

@agl
Copy link
Contributor Author

agl commented Apr 15, 2020

(Just adding a note here that, while people should review this, I think I should sketch an implementation before we land this in the spec to ensure that there aren't any unexpected gotchas.)

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
agl and others added 2 commits April 29, 2020 09:49
(Some suggestions collide with others and GitHub can't cope with that. Will apply those manually in a sec.)

Co-Authored-By: J.C. Jones <james.jc.jones@gmail.com>
Co-Authored-By: =JeffH <jdhodges@google.com>
Co-Authored-By: Emil Lundberg <emil@emlun.se>
A SubjectPublicKeyInfo encodes only the public key, but COSE Key
structures also include a signature algorithm. Since RPs will need this
information too, this change adds getPublicKeyAlgorithm to return it.

(This change also includes some suggestions from the review that GitHub
couldn't automatically apply because they collided with other
suggestions.)
@agl
Copy link
Contributor Author

agl commented Apr 29, 2020

Latest pushes switch to ArrayBuffer for the public key, apply all suggestions, and add getPublicKeyAlgorithm to convey the COSE algorithm that's in a COSE Key, but not a SubjectPublicKeyInfo.

@equalsJeffH
Copy link
Contributor

on 2020-04-29 call, @agl still working on this and an implementation, so remains a moving target...

@agl
Copy link
Contributor Author

agl commented May 6, 2020

I've fleshed out an implementation of this to the point where I believe it'll work and thus think this PR is in good shape.

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.

LGTM, thx @agl !

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request May 12, 2020
In preparation for w3c/webauthn#1395, transform
credential public keys into SPKI form.

Change-Id: Iabbadd7802788bb323711ca754d1f04634ecfb6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2194374
Commit-Queue: Adam Langley <agl@chromium.org>
Auto-Submit: Adam Langley <agl@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#767583}
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, just one minor suggestion...

index.bs Outdated Show resolved Hide resolved
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request May 14, 2020
RSA keys could always pass through generically, but this change adds
support to the virtual authenticator and for parsing them so that they
can be exposed via getPublicKey[1] in the future.

[1] w3c/webauthn#1395

Change-Id: If927385dccd8c9bea9f1cc27f015c9cb3da2d8e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2197668
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#768493}
Co-authored-by: Emil Lundberg <emil@emlun.se>
@jcjones
Copy link
Contributor

jcjones commented May 14, 2020

Per the 13 May call, merging!

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

Successfully merging this pull request may close these issues.

Provide the public key in AuthenticatorAttestationResponse
7 participants