Conversation
0f3f077 to
6ccf76d
Compare
Signed-off-by: S1m <[email protected]>
6ccf76d to
24473e5
Compare
There was a problem hiding this comment.
Implementation requirements:
- Client
- Server
There was a problem hiding this comment.
Client (Hydrogen) and Server (Synapse) implementations noted at #4174 (comment)
There was a problem hiding this comment.
This MSC has changed since the implementations were made. I'm not sure what all the changes were. If they are primarily minor changes such as naming changes (e.g. renaming properties), or changes of that sort, then the implementations should still be fine as a demonstration that the MSC works. If there are any substantial changes, then the implementations may need to be updated.
There was a problem hiding this comment.
The homeserver implementation for Synapse is actively being updated in element-hq/synapse#17987. It has yet to be merged.
The client implementation for Hydrogen has not been updated since Dec 2nd, 2024: element-hq/hydrogen-web#1201. The changes to the MSC since Dec 2nd, 2024 are https://github.com/matrix-org/matrix-spec-proposals/pull/4174/files/0346332a1f802425b579d2b4cdf98fe8a462d48b..f59cb97551216812d1ca492fafba05d057cc1298.
I've left a review on the Hydrogen PR with what appears to be missing from a cursory glance. As such, I think we're still missing an up-to-date client implementation.
There was a problem hiding this comment.
@mscbot concern The client implementation has grown stale with respect to the MSC. See this PR review for the outstanding changes.
|
Thank you for your feedback @clokep, I'm updated the proposal soon |
Reword some sentences, add links
|
Client implementation: p1gp1g/hydrogen-web@e7cc488 |
Add capability and VAPID
|
synapse impl: element-hq/synapse#17987 |
|
Implementations seem to implement what's described in the MSC, so removing the needs-checking label |
There was a problem hiding this comment.
The homeserver implementation for Synapse is actively being updated in element-hq/synapse#17987. It has yet to be merged.
The client implementation for Hydrogen has not been updated since Dec 2nd, 2024: element-hq/hydrogen-web#1201. The changes to the MSC since Dec 2nd, 2024 are https://github.com/matrix-org/matrix-spec-proposals/pull/4174/files/0346332a1f802425b579d2b4cdf98fe8a462d48b..f59cb97551216812d1ca492fafba05d057cc1298.
I've left a review on the Hydrogen PR with what appears to be missing from a cursory glance. As such, I think we're still missing an up-to-date client implementation.
Co-authored-by: Andrew Morgan <[email protected]>
| @@ -0,0 +1,177 @@ | |||
| # MSC4174: Web push | |||
|
|
|||
| This MSC supersedes and replaces [MSC3013](https://github.com/matrix-org/matrix-spec-proposals/pull/3013), which introduced push notification encryption first. | |||
There was a problem hiding this comment.
I think that we only want to use the encryption defined in here for WebPush (and only because it isn't easy to replace the encryption in WebPush with something else). If we want to encrypt push notifictions over other systems, we should use a different mechanism, which will probably be something different from what 3013 defines. So 3013 will be probably replaced, but not by this MSC.
Basically, this first sentence should be removed.
proposals/4174-webpush-pushkind.md
Outdated
| `POST /_matrix/client/v3/pushers/set` now supports a new `kind`: `webpush`. | ||
| - `kind`: is updated to introduce `webpush` which makes a | ||
| pusher that sends Web Push encrypted messages. | ||
| - `pushkey`: is updated, if the `kind` is `webpush`, this is the user agent public key in the uncompressed form ([SEC 1](https://www.secg.org/sec1-v2.pdf), section 2.3.3, replicated from X9.62), encoded in URL-safe Base64 without padding. The public key comes from a ECDH |
There was a problem hiding this comment.
Is the intention here that the pushkey is still a unique identifier for the pusher (as it is currently defined in the spec), and is also the public key? In other words, the client needs to use a unique P-256 key per pusher? If so, I think this should be stated explicity.
There was a problem hiding this comment.
The spec isn't very clear for FCM/APNS push key too
| @@ -0,0 +1,177 @@ | |||
| # MSC4174: Web push | |||
There was a problem hiding this comment.
[non-blocking] This MSC refers to the feature inconsistently as "WebPush" or "Web Push" (with or without the space), but it's hard to fault this MSC for that, since the RFCs do the same thing. However, when the spec gets written, we should pick one and stick with it.
proposals/4174-webpush-pushkind.md
Outdated
| Web Push is a standard for (E2EE) push notifications, defined with 3 RFC: | ||
| - [RFC8030](https://www.rfc-editor.org/rfc/rfc8030) defines the application server to push server communications | ||
| - [RFC8291](https://www.rfc-editor.org/rfc/rfc8291) defines the encryption: | ||
| - the subscribing client (user agent in RFC8030) generates a P-256 key pair the *auth* secret, and sends the P-256 public key and the *auth* secret to the home server during registration |
There was a problem hiding this comment.
Are the auth secret and P-256 key pair different things?
(Also, why is auth in italics?)
| ``` | ||
| "m.webpush": { | ||
| "enabled": true, | ||
| "vapid": "BNbXV88MfMI0fSxB7cDngopoviZRTbxIS0qSS-O7BZCtG04khMOn-PP2ueb_X7Aeci42n02kJ0-JJJ0uQ4ELRTs" |
There was a problem hiding this comment.
I slightly feel that it might be more appropriate to expose this on a separate endpoint, but it's not a strong opinion.
|
|
||
| ``` | ||
| "m.webpush": { | ||
| "enabled": true, |
There was a problem hiding this comment.
is enabled necessary here, or is the presence of an m.webpush entry sufficient for clients to determine that their server supports webpush?
| safelist a private IP. (Synapse already implements [`ip_range_whitelist`](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#ip_range_whitelist)) | ||
| It is also recommended to not follow redirection, to avoid implementation issue where the destination is checked | ||
| before sending the request but not for redirection. | ||
|
|
There was a problem hiding this comment.
does the sample implementation follow these recommendations? Do they work ok in practice (especially not following redirects)?
| - Until this proposal is considered stable, implementations must use | ||
| `org.matrix.msc4174.webpush` instead of `m.webpush`. |
There was a problem hiding this comment.
for the capabilities? There doesn't seem much point specifying an unstable prefix for that without also doing so for Pusher.kind and the new API endpoint.
|
RE: P-256 concern (can't find the comment anymore) RFC8291, web push AES-128-GCM encryption: the key is derived from the auth_secret and the ECDH secret. If somehow the P-256 curve is broken, you will still have to break the auth_secret too. And if the notification contains the content of the event, the content is encrypted with Olm/Megolm on top of that I think we should start working with the IETF to make a new AES-256-GCM encryption for web push, that works the same way, maybe using X25519 at the same time; using a 32B pre-shared key as |
I would say that this is a good idea, though I would suggest that we attempt to reuse HPKE and its single shot API if such an attempt is mounted. HPKE also has some drafts around which add PQ-safe algorithms to the mix. |
Co-authored-by: Richard van der Hoff <[email protected]>
There should be different way to do it, and would be discussed with IETF. The limited size of push notifications is important, so the pre-shared key solution may be a good thing. I'm surprised they offer AES-128, I thought it wasn't considered PQ-safe |
Co-authored-by: Richard van der Hoff <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]> Co-authored-by: Hubert Chathi <[email protected]> Co-authored-by: S1m <[email protected]>
Co-authored-by: S1m <[email protected]>
rendered
Client implementation:
synapse impl: element-hq/synapse#17987
updated hydrogen impl: element-hq/hydrogen-web#1201
Note: in addition to this MSC, it would be very useful to have MSC4173, if someone is interested in implementing this MSC as well
SCT Stuff:
MSC checklist
FCP tickyboxes