Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
device public key extension #1663
device public key extension #1663
Changes from 74 commits
6719e05
4a6b8fe
5d1662d
1622df2
5e684aa
71afdbe
8040d13
38131e6
ad71ff1
094d385
e66eb2d
618b2de
1e97952
8f0d66d
43e03c8
503a027
87340d7
cbb066f
1e72a00
99a6b79
e9db523
68ebaa2
8b5702c
da82c2e
f84069b
f6663cb
22e325d
a3ed05b
6382444
768d900
fd9ea00
a34b489
2832b5e
e47c5f8
75c8f25
4515d63
c208e19
591cded
59260f0
c3487a2
0e8d3b3
6b216db
25b07e6
2da4504
f943bbc
e23ccfe
7a1e2ee
8a420eb
d2b529b
666718a
f9e861c
e1a4383
9182fa1
89e2660
54eb767
f6fcee8
1747dff
12ec079
59f2909
8b4d51c
c5f3b2d
4ebd028
4f18790
66e67bd
9ac274a
fcc6a68
73cc7ff
7c5393c
3d16662
aee534c
7c3e2e8
90593b9
d0bef33
89cec45
db63d69
9a78683
d52342c
e23c4b9
b8ec5b8
0bb9aaa
3237896
d652787
55e64c9
41ffcbf
f131d68
e1e6d94
23ea3ef
683ad4d
17f3aa2
b4e8d0e
619ebb9
2730294
f0fe8f2
f145234
b8d8567
d92bad2
6d45aba
eb598ff
b7289e1
dcfb392
cbb6b5d
ccfd0b4
f3315b5
27ef223
bfce0cf
20dd35c
27d0895
e30cdb1
38fb4e1
0c7fad0
844cff7
6fbfccf
4e67faa
832c2e8
6940a43
7b531a8
04ddb48
3cba94c
47017e4
16a846a
2ec8861
5c1cd98
5c6c23d
a026a5b
ec03d4d
88be1a6
3430c95
5af393d
fe333fe
ece61f0
4279e6e
d25fd53
8966fe6
d671894
ca1b0c6
6112877
ed0b779
9bd0e3d
759ce04
8aa160c
bff403d
fba2725
f780870
6ae32a0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is it worth calling out that this is for authenticators where the [=credential public key=] might not be hardware-bound? I think in the minds of readers for some time they're probably coming to WebAuthn with that assumption and might be unmoored by this description without that context.
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.
Yes, good catch, I've submitted #1665 re explicitly defining the "synced | hardware-bound" credential notions.
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.
Is it fair to say this extension is only useful in cases where the credential public key registration is performed with direct attestation, and further that this not be a self attestation?
Really, what's the point in declaring a device public key if there is no real verifiable root of trust of the authenticator attestation of the associated credential public key?
In the case of "none" attestation, there is no AAGUID anyway, so you can't even provide a meaningful value for aaguid in devicePubKey extension output. I would argue that for "self" attestation it's meaningless as well since self attestation can be spoofed by any client.
If the above assertions are agreed, I think this needs to be made more explicit, and that there should be guidance to suggest that this extension only be used with authenticators and in contexts providing a direct attestation using a verifiable root of trust.
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.
The DPK is providing a device continuity signal whose usefulness is arguably independent of what sort of attestation of the user credential may have been supplied at the time the user credential public key was originally registered.
The devicePubKey extension output in conjunction with create() or get() output demonstrates authenticator's possession of both the (potentially synced) user credential private key and the device private key, simultaneously, by employing a nested set of signatures beginning with the authenticator's attestation private key.
"none" is an *attestation conveyance" preference (also having a corresponding "attestation statement format"), not an attestation type, and signals the client platform to modify the attestation returned by the authenticator before returning the attstn object to the RP. Thus the authnr may indeed have an AAGUID to employ with the device public key extension.
"self attestation" is an "attestation type" and (offhand) seems not applicable/appropriate for use in attesting to device public keys. "Basic" attestation type (aka "batch attestation") is the primarily applicable attestation type. The AttCA and AnonCA types require further thinking.
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.
As an RP, I can certainly receive a "none" attestation statement format, and that can and does happen at times regardless of the RP-requested attestation conveyance preference. One such example is the current use of Apple passkey credentials. I can't see any point in accepting values of this extension containing a "none" attestation statement format. I think when the TODO part of this PR is being done (
how to verify a attObjForDevicePublicKey
) that we should mention specifically what to accept/reject for allowed attestation statement formats.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.
Yes, though, the attestation you're referring to there is that of the user credential. As @agl notes in his above comment: "So the two attestations (user cred's, and DPK's) could happen to be equal, but that wouldn't generally be true.", and so the DPK attestation in this case could be tied to hardware roots of trust even though the user credential's attestation is not.
It will also depend on what DPK attestation types/formats/conveyances we determine are appropriate for DPKs spec-wise (i.e., on a technical basis), and then what a given DPK-supporting authenticator actually implements.
Is it not up to RPs to decide what to trust? E.g., presently in Step 20 of section 7.1. Registering a New Credential assessing "trustworthiness" is done by the RP according to the RP's policies, and the webauthn spec does not attempt to stipulate such policies.
Nominally, if an RP receives a "none" DPK attestation from an authenticator (which I suspect could potentially happen regardless of what the spec ends up saying), then it seems it is up to the RP to decide what to do, which could vary, e.g., based on how sophisticated (or not) the RP's session risk scoring is.
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.
Yes it is ultimately up to the RP to decide what to do, however practical guidance can and should be provided as to the verification rules for an RP. I do not find the use of the DPK extension intuitive, and the stronger the guardrails here the better.
For example I cannot think of any practical scenario in which an RP, having requested the DPK extension, should then accept a DPK attestation that cannot be verified back to a hardware root of trust. After all, that's the entire point of the extension in the first place - provide a device-bound continuity check. A DPK attestation containing attestation statement format of "none" or "self" provides no such assurance.
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.
Hardware may not be available all the time on every device user has. Which means that there may not be hardware root of trust all the time. Then it is up to the RP to decide what to do in its risk analysis. What DPK is telling that it is a new device in that case, which is still a requirement to figure out even when there may not be a hardware root of trust.
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.
I can logically "infer" or fallback to the posture of it being a new device if no DPK is present. For example, how does DPK provides any better or more reliable a trust signal over a persistent cookie if it isn't attested?
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.
The DPK lives in the authenticator, so it can be involved in a sign-in request on a new device as long as a known authenticator is used. It's also likely longer lived than a cookie.
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.
Seem feeling here about giving context. Most authenticators are capable of creating a hardware-bound key pair but won't honour this extension :)
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.
Agreed. To be addressed along with issue #1665.
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.
“synced credential” is mentioned here but is probably the first mention of such a thing in the spec?
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.
Agreed. To be addressed along with issue #1665.