Define and use the p256dh/auth internal slots #414
Conversation
annevk
left a comment
annevk
left a comment
There was a problem hiding this comment.
Nice cleanup! Looks good modulo a number of nits.
| authentication secret in accordance with [[RFC8291]]. These slots MUST be populated when | ||
| creating the <a>push subscription</a>. | ||
| A [=push subscription=] has an associated <dfn>P-256 ECDH key pair</dfn> and an | ||
| <dfn>authentication secret</dfn> in accordance with [[RFC8291]]. |
There was a problem hiding this comment.
Would be nice to (eventually) define their types as well.
There was a problem hiding this comment.
Yeah, it's somehow opaque right now. Could try byte sequences and make it always serialized. Thoughts?
I think a separate PR would be nicer for that
There was a problem hiding this comment.
Yeah, probably best as a follow-up. It could better explain getKey and such.
| authentication secret in accordance with [[RFC8291]]. These slots MUST be populated when | ||
| creating the <a>push subscription</a>. | ||
| A [=push subscription=] has an associated <dfn>P-256 ECDH key pair</dfn> and an | ||
| <dfn>authentication secret</dfn> in accordance with [[RFC8291]]. |
There was a problem hiding this comment.
Yeah, probably best as a follow-up. It could better explain getKey and such.
| </li> | ||
| </ol> | ||
| <li>Set |keys|["p256dh"] to the URL-safe base64 encoding without padding [[RFC4648]] of | ||
| the value as returned by {{PushSubscription/getKey("p256dh")}}, as a {{USVString}}. |
There was a problem hiding this comment.
I guess this isn't entirely a regression, but using the public API is not great. We should have an internal "get key" for this.
There was a problem hiding this comment.
Hmm, I took a bit of look and I think it would be nice to fix this with key types. If we make the keys always serialized, then this step can simply retrieve that without an extra algorithm.
| {{PushSubscription/getKey()}} method of the {{PushSubscription}} with an argument of | ||
| {{PushEncryptionKeyName/"p256dh"}}. | ||
| <li>Set |subscription|'s [=P-256 ECDH key pair=] to the result of generating a new P-256 | ||
| [=ECDH=] key pair [[ANSI-X9-62]]. |
There was a problem hiding this comment.
If at all possible, I wonder if it's possible to link to the particular section of ANSI-X9-62 (or even if such a section exists?... I haven't checked).
There was a problem hiding this comment.
But I don't understand tihs. ANSI X9.62 doesn't seem to be about ECDH, it's about ECDSA per what I see. X9.63 is about ECDH. Something to fix in a separate patch?
| <a>application server</a>. | ||
| <li>Otherwise: | ||
| <ol> | ||
| <li>[=/Assert=]: |name| is {{PushEncryptionKeyName/"auth"}}</li> |
There was a problem hiding this comment.
I think you can just do:
| <li>[=/Assert=]: |name| is {{PushEncryptionKeyName/"auth"}}</li> | |
| <li>Assert: |name| is {{PushEncryptionKeyName/"auth"}}</li> |
And if that should link to WebIDL's assert, we should probably fix that in ReSpec (can't remember if it links already or not).
There was a problem hiding this comment.
No, doesn't seem to work. Just a plaintext.
marcoscaceres
left a comment
marcoscaceres
left a comment
There was a problem hiding this comment.
Some editorial suggestions... great cleanup. Coming along nicely!
Co-authored-by: Marcos Cáceres <marcos@marcosc.com>
Closes #396.
The current spec assumes random kinds of keys may exist, even though there has been only two kinds of keys, which is unlikely to change. Given that adds more prose for no use, this patch defines separate internal slots for each key.
Technically this change can be done without the dictionary change. I can split it if desired.
The following tasks have been completed:
Implementation commitment:
objectPreview | Diff