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
Add getPublicKey method. #1395
Conversation
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
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? |
From the call: yes, we should add more accessors for other stuff that people are likely to need. |
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? |
on 2020-04-01 call: @agl to update, then reviewers to review :) |
|
There was a problem hiding this 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...
on 2020-04-08 call: please review! |
(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.) |
(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.)
Latest pushes switch to ArrayBuffer for the public key, apply all suggestions, and add |
on 2020-04-29 call, @agl still working on this and an implementation, so remains a moving target... |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx @agl !
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}
There was a problem hiding this 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...
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>
Per the 13 May call, merging! |
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