Add browser binding to the secure payment confirmation spec #286
Conversation
pejic
force-pushed
the
main
branch
2 times, most recently
from
c5e83b6 to
aac5dad
Compare
pejic
force-pushed
the
main
branch
2 times, most recently
from
a9bdc90 to
7a9c1c3
Compare
pejic
force-pushed
the
main
branch
3 times, most recently
from
007c502 to
f68533d
Compare
There was a problem hiding this comment.
Some more minor nits, but overall this LGTM as a solid foundation for BBKs in the spec. There is still more to add, notably we need to add Security & Privacy Considerations for BBKs, but I think we can do that in a separate PR in the future.
Note that we should not land this yet. Firstly, I'm sure Dominic may have input as a spec mentor (thanks in advance Dominic!), as may Ian as a reviewer. However on top of that, we should give the Working Group the chance to review the content before this lands. @ianbjacobs - I would prefer not to wait until the 24th to do that, do you think it would suffice to send the preview to the Working Group mailing list and invite feedback that way?
(Even after this lands we can always come back and change it, of course!!)
Thank you Slobo for your work here!
This change adds browser bound keys to the secure payment confirmation spec. See issue 271, w3c#271.
There was a problem hiding this comment.
Looking better. There are still lots of strange instances where you describe a step and then actually make it technical by running algorithms and passing arguments as an afterthought. Those need to be cleaned up to just read like natural spec code. See the last review comment in this review.
| user agent. | ||
|
|
||
| : <dfn>Key Pair</dfn> | ||
| :: A pair of asymmetric cryptographic keys. These are algorithm specific |
There was a problem hiding this comment.
Might be worth making this a struct, with two members: "public key" and "private key". Then move the text describing when they are populated / returned to non-normative Note text maybe. That seems to be what you're trying to get at with "public portion" and "private portion" anyways. And I think the text below is non-normative too.
| 1. [=Bind a key pair=] using |bbk_id| and <var | ||
| ignore>pubKeyCred</var>.{{PublicKeyCredential/[[identifier]]}}. | ||
|
|
||
| 1. If [=binding a key pair=] failed, return an error, {{UnknownError}}. |
There was a problem hiding this comment.
For items like this, I would make this look like:
1. Do operation. If that returned failure, then [...]
The point is, I'd move explicit failure detection to the same line as the operation and reference it implicitly (with "that") instead of linking again, and on a separate line.
(See whatwg/infra#545 for "returning failure". This is a common thing to do in algorithms that need to return some failed sentinel value. We just call it, literally "failure").
| To <dfn>bind a key pair</dfn> given the |bbk_id|, the byte array containing the | ||
| key pair identifier, and |credential_id|, the byte array of the SPC | ||
| credential_id (i.e. passkey id), store the identifiers in the | ||
| [=browser_bound_map=] as follows: |
There was a problem hiding this comment.
Make the types come before the params. Example: ... given a byte array |bbk_id|, etc.. Short descriptive text should come as a note before the actual algorithm begins, or in a first step or something.
| credential_id (i.e. passkey id), store the identifiers in the | ||
| [=browser_bound_map=] as follows: | ||
|
|
||
| 1. Insert the browser bound key id into the map: |
There was a problem hiding this comment.
Use [=map/exists=] and [=map/set=] for insertion and assignment. And reference the map explicitly when doing it. See https://dom.spec.whatwg.org/#ref-for-map-set for example, which looks more like code than English, which is good.
|
|
||
| 1. Return true if the map insertion succeeded. | ||
|
|
||
| 1. Return false if the map insertion failed. |
There was a problem hiding this comment.
We definitely need to define the conditions under which this can succeed and fail, as I would have no idea what this means if I was another implementer coming along trying to implement this. At the very least, add a non-normative Note describing the expected conditions under which a failed insertion might take place, and what that means.
| <dl class="switch"> | ||
|
|
||
| : [=map/exists=] | ||
| :: Retrieve the existing key pair |
There was a problem hiding this comment.
This line isn't needed. Just put the two spec steps that actually do the work.
| 1. Let |bbk_and_algorithm| be [=keypair_map=][|bbk_id|]. | ||
|
|
||
| : Does not [=map/exist=] | ||
| :: Create and bind a new key pair |
There was a problem hiding this comment.
Same here. It's fine for comments or "Note"s, but I don't think we should have another layer of nesting just to include this line of text, when the real work is below.
| ## Getting the browser bound public key ## {#sctn-getting-the-bbk-public-key} | ||
|
|
||
| To <dfn>get a browser bound public key</dfn> given the |bbk_and_algorithm|, the | ||
| [=tuple=] of key pair and {{COSEAlgorithmIdentifier}} returned by the precedures |
There was a problem hiding this comment.
| [=tuple=] of key pair and {{COSEAlgorithmIdentifier}} returned by the precedures | |
| [=tuple=] of key pair and {{COSEAlgorithmIdentifier}} returned by the procedures |
|
|
||
| 1. Let |algorithm| be |bbk_and_algorithm|[1]. | ||
|
|
||
| 1. Generate |signature|, a byte array, by performing the cryptographic |
There was a problem hiding this comment.
Same here. Don't describe it as "Generating |signature|". Just do the work:
1. Let |signature| be the result of performing |algorithm| with |bbk|, on |client_data_json|.
Note: This generates a cryptographic signature that [....]
|
|
||
| This component internal to the the user agent manages platform-dependant | ||
| cryptographic key pairs including their association to [=SPC credentials=] | ||
| (i.e. passkeys). This section explains the <dfn>browser bound key store</dfn> |
There was a problem hiding this comment.
Who owns the key store? It feels like it's global to the browser, and if you think that's right, I'd make it owned by "the [=user agent=] for clarity. Do you think that's the right model?
See #271
Preview | Diff