Repository navigation
Add "Credential Manager Trust Group (CMTG) Key" extension - #2377
timcappalli wants to merge 50 commits into
Conversation
nsatragno
left a comment
There was a problem hiding this comment.
Thank you so much for doing this work! Here's a first pass at a review.
Co-authored-by: Nina Satragno <nsatragno@gmail.com>
Co-authored-by: Nina Satragno <nsatragno@gmail.com>
|
|
||
| 1. Let |cmtgPrivateKey| be the newly created or existing [=Credential Manager Trust Group private key=]. | ||
|
|
||
| 1. Let |cmtgKeySig| be the result of signing the [=assertion signature=] [input](#fig-signature) with |cmtgPrivateKey|, |
There was a problem hiding this comment.
This is ambiguous in a subtle way. The assertion signature input is the authenticatorData + clientDataHash. However, the authenticatorData is not the same for the authenticator get assertion and make credential operations.
I think for make credential, you mean to use the computed authenticatorData with the attestedCredentialData. This makes the most sense to me.
So, saying "assertion signature input" for make credential seems subtly wrong. Why not spell out what is signed exactly?
Add basic virtual authenticator support for CMTG keys at the device layer. This lets us write tests without needing the enclave. Later, we'll wire this to devtools and the chromedriver testing API. See w3c/webauthn#2377. Bug: 485888879 Change-Id: I19330587a3246b49e0c533d8c7915085e675b0ce Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8041160 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Commit-Queue: Nina Satragno <nsatragno@chromium.org> Cr-Commit-Position: refs/heads/main@{#1657412}
nsatragno
left a comment
There was a problem hiding this comment.
Approved with two nits.
Thank you so much for taking on this feature. I really appreciate it!
| }; | ||
| partial dictionary AuthenticationExtensionsClientInputsJSON { | ||
| boolean cmtgKey; | ||
| } ; |
There was a problem hiding this comment.
nit: remove the extraneous whitespace.
|
|
||
| 1. Let |extensionOutput| be the value of <code>|clientExtensionResults|.{{AuthenticationExtensionsClientOutputs/cmtgKey}}</code>. | ||
|
|
||
| 1. If |extensionOutput| does not contain the required members of {{AuthenticationExtensionsCmtgKeyOutputs}}, |
There was a problem hiding this comment.
nit: extensionOutput may not be present.
- If |extensionOutput| is not present or |extensionOutput| does not contain...
| 1. If this algorithm is being run as part of [=authenticatorMakeCredential=]: | ||
|
|
||
| 1. Let |cmtgKeySig| be the result of signing the concatenation of |authenticatorData| (as constructed in [[#sctn-generating-an-attestation-object]], | ||
| which includes |attestedCredentialData|) and |clientDataHash|, with |cmtgPrivateKey|, | ||
| using the same signature algorithm as that used by the [=user credential=]'s [=credential key pair=]. | ||
|
|
There was a problem hiding this comment.
Related to https://github.com/w3c/webauthn/pull/2377/changes#r3624840460, this might be a good place to add an additional step, after this existing one, to iterate over existing CMTG keys and ensure that the authenticator is still managing a credential source for each CMTG key's corresponding ([=public key credential source/id|Credential ID=], [=public key credential source/rpId|RP ID=], [=public key credential source/userHandle|User Handle=]) tuple. If not, delete the CMTG key.
|
Why does the PR description not have "Affects privacy" checked? This would allow distinguishing between multiple devices of the same user without the user explicitly consenting to it. That is a fingerprintable signal, right? |
|
|
||
| NOTE: [=[RPS]=] MUST ignore any keys in |extensionOutput| that are not defined in the {{AuthenticationExtensionsCmtgKeyOutputs}} dictionary. | ||
|
|
||
| 1. Parse |cmtgKey| as a COSE_Key. |
There was a problem hiding this comment.
[nit] Add explicit extraction steps before step 4:
Let |cmtgKey| be |extensionOutput|.cmtgKey.
Let |signature| be |extensionOutput|.signature.
Verify |cmtgKey| and |signature| are valid ArrayBuffers.
| 1. Verify that the algorithm used for |signature| matches the COSE algorithm identifier of the [=credentialPublicKey=] | ||
| within the [=attestedCredentialData=] of |authData|. | ||
|
|
||
| 1. Verify that |signature| is a valid signature over the [=assertion signature=] [input](#fig-signature) |
There was a problem hiding this comment.
Registration produces an attestation object, not an assertion. Referring to the "assertion signature input" during registration might be confusing.
|
|
||
| 1. Verify that |signature| is a valid signature over the [=assertion signature=] [input](#fig-signature) | ||
| (i.e. |authData| and |hash|) by the [=Credential Manager Trust Group Key=] |cmtgKey|. | ||
|
|
There was a problem hiding this comment.
Maybe it would be worth explicitly calling out the case if verification fails.
| 1. Verify that the algorithm used for |signature| matches the algorithm of the public key | ||
| previously stored for the |credentialRecord|. | ||
|
|
||
| 1. Verify that |signature| is a valid signature over the [=assertion signature=] [input](#fig-signature) |
There was a problem hiding this comment.
Maybe it would be worth explicitly calling out the case if verification fails.
| 1. Let |newEntry| be a new [=CMTG Key Record=] where its [$CMTG Key Record/publicKey$] is |cmtgKey| | ||
| and its [$CMTG Key Record/lastUsed$] is the current time. | ||
|
|
||
| 1. Append |newEntry| to |credentialRecord|.[$credential record/cmtgKeyRecords$]. |
There was a problem hiding this comment.
Would it make sense to add that appending |newEntry| to cmtgKeyRecords should only occur if the RP's policy allows accepting the new trust group (e.g., following successful step-up or risk checks) is passed.
| 1. Let |cmtgKeySig| be the result of signing the [=assertion signature|assertion signature input=] with |cmtgPrivateKey|, | ||
| using the same signature algorithm as that used by the [=user credential=]'s [=credential key pair=]. | ||
|
|
||
| Note: The [=assertion signature=] [input](#fig-signature) and |cmtgKeySig| covers the [=[RP]=]'s {{PublicKeyCredentialCreationOptions/challenge}} |
There was a problem hiding this comment.
Maybe it should be PublicKeyCredentialRequestOptions/challenge given it is assertion.
| ##### Registration (`create()`) ##### {#sctn-cmtg-key-extension-verification-create} | ||
|
|
||
| If a {{AuthenticationExtensionsClientOutputs/cmtgKey}} extension output is present in a {{CredentialsContainer/create()|navigator.credentials.create()}} call, | ||
| the below verification steps are performed in the context of <a href=#reg-ceremony-verify-extension-outputs>this step</a> of [[#sctn-registering-a-new-credential]] |
There was a problem hiding this comment.
For the sake of the text making sense even if the hyperlinks get dropped (perhaps unlikely, but might be relevant for plain text quotations if nothing else), I think we should refer to this as a "step X" number. That's also the style currently used throughout the document.
|
|
||
| 1. Parse |cmtgKey| as a COSE_Key. | ||
|
|
||
| 1. Verify that the algorithm used for |signature| matches the COSE algorithm identifier of the [=credentialPublicKey=] |
There was a problem hiding this comment.
|signature| is undefined here. I think this is meant to be <code>|extensionOutput|.{{AuthenticationExtensionsCmtgKeyOutputs/signature}}</code>?
| 1. Verify that the algorithm used for |signature| matches the COSE algorithm identifier of the [=credentialPublicKey=] | |
| 1. Verify that the algorithm used for <code>|extensionOutput|.{{AuthenticationExtensionsCmtgKeyOutputs/signature}}</code> matches the COSE algorithm identifier of the [=credentialPublicKey=] |
(This should then also apply to further references to |signature| below.)
|
|
||
| 1. Terminate these validation steps as inconclusive. | ||
|
|
||
| NOTE: [=[RPS]=] MUST ignore any keys in |extensionOutput| that are not defined in the {{AuthenticationExtensionsCmtgKeyOutputs}} dictionary. |
There was a problem hiding this comment.
Per #1979, the normative MUST should not be in a NOTE block.
| (The signature algorithm is the same as for the [=user credential=].) | ||
|
|
||
| 1. If |credentialRecord|.[$credential record/cmtgKeyRecords$] contains an entry |e| | ||
| where |e|.[$CMTG Key Record/publicKey$] matches |cmtgKey|: |
There was a problem hiding this comment.
"matches" might be a dangerous word here as it could be interpreted several ways (for example, a COSE_Key with kid might "match" a COSE_Key without kid but other members equal). But since §2.4 requires CTAP2 canonical CBOR for everything, and §6.5.1 further restricts the encoding of these public keys to forbid optional values (like kid), it should be safe to assume that all encodings of the same key should be byte-identical. So it should be safe to specify here that publicKey and cmtgKey should be equal byte arrays, which eliminates any ambiguity.
| The extension output includes the public key and a signature generated using the | ||
| [=Credential Manager Trust Group private key=] (`cmtgKey1`). | ||
|
|
||
| <div class="example"> |
There was a problem hiding this comment.
Both of these two examples look like most of it is the usual base WebAuthn stuff, just with the addition of a cmtgKey: true extension input, right? If so, then I don't think these examples seem very helpful as is. They give an initial impression that you need a lot of code to use this extension, while actually the cmtgKey part is a very small fraction of the example volume (at least on the client side). It might be more helpful to reduce the example to concentrate on the extension parts specifically, and perhaps also include some abbreviated pseudocode for what needs to happen on the server side too.
There was a problem hiding this comment.
@emlun what do you think of the updated example?
| standard [=authentication=] verification: | ||
|
|
||
| ``` | ||
| cmtgOutput = response.clientExtensionResults.cmtgKey |
There was a problem hiding this comment.
Perhaps we should tell relying parties that they need to obtain the CMTG key from the extensions section of the authenticator data rather than from the client extension results. Otherwise there's little point in having the CMTG key be signed by the passkey.
(This makes me think: is there a point then in copying the public key to the extensions output?)
…MTGK, a=testonly Automatic update from web-platform-tests [webauthn] Renderer implementation for CMTGK Implement the renderer side of Credential Manager Trust Group (CMTG) Keys. See w3c/webauthn#2377. Fuchsia-Binary-Size: Size increase is unavoidable. Bug: 485888879 Change-Id: I3a75a7d7d598ec8078d5424e292ec724eaf497b9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7841424 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Auto-Submit: Nina Satragno <nsatragno@chromium.org> Commit-Queue: Nina Satragno <nsatragno@chromium.org> Cr-Commit-Position: refs/heads/main@{#1633722} -- wpt-commits: 2f1307421d80b0ef84dfe4b8fd26f3f8421feb70 wpt-pr: 59982
Closes #2338
The following tasks have been completed:
Implementation commitment:
Documentation and checks
Preview | Diff