Skip to content
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

MSC4174: webpush push kind #4174

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

p1gp1g
Copy link

@p1gp1g p1gp1g commented Aug 1, 2024

rendered

Client implementation:
synapse impl: element-hq/synapse#17987
updated hydrogen impl: element-hq/hydrogen-web#1201

@p1gp1g p1gp1g force-pushed the webpush-pushkind branch from 0f3f077 to 6ccf76d Compare August 1, 2024 15:53
@p1gp1g p1gp1g force-pushed the webpush-pushkind branch from 6ccf76d to 24473e5 Compare August 1, 2024 15:54
@p1gp1g p1gp1g changed the title MSC webpush push kind MSC4174: webpush push kind Aug 1, 2024
@turt2live turt2live added push proposal A matrix spec change proposal client-server Client-Server API needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Aug 1, 2024
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation requirements:

  • Client
  • Server

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turt2live These requirements are now met

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client (Hydrogen) and Server (Synapse) implementations noted at #4174 (comment)

are defined in the Push Gateway Specification. Currently the only format available is ’event_id_only'.
- `url`: Required if `kind` is `http`, not used else. The URL to use to send notifications to. MUST be an
HTTPS URL with a path of /_matrix/push/v1/notify
- `endpoint`: Required if `kind` is `webpush`, not used else. The URL to send notification to, as defined as a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a stupid question but could you not just reuse the url field?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to mention it in the alternative part. Clients that already use webpush and must use a push gateway uses endpoint because url is the one used for the gateway.

So, defining that way would make their current PusherData spec compliant

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it would have been better to set pushKey and url in the PusherData too, but that's out of this proposal scope

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that re-using the current field seems simpler.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-using url as the http kind do ? Or Re-using endpoint like hydrogen and sygnal do ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reusing the endpoint -- this definitely needs a different kind.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I resolve this "conversation" then ?

proposals/4174-webpush-pushkind.md Outdated Show resolved Hide resolved
proposals/4174-webpush-pushkind.md Outdated Show resolved Hide resolved
proposals/4174-webpush-pushkind.md Outdated Show resolved Hide resolved
Having a webpush push kind would provide push notifications without gateway to
- Web app and desktop app
- Android apps using UnifiedPush (MSC2970 was open for this and won't be required anymore)
- Maybe other ? We have seen apple moving a lot into web push support
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there links to this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be added to the MSC ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://gist.github.com/mar-v-in/2a054e3a4c0a508656549fc7d0aaeb74#webpush Shows how to push to FCM using webpush, without a gateway

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be added to the MSC ?

It is helpful to add links for folks who aren't very familiar with the topic, yes. People can always ignore links if they know!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've added the 2 links


Having a webpush push kind would provide push notifications without gateway to
- Web app and desktop app
- Android apps using UnifiedPush (MSC2970 was open for this and won't be required anymore)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Links showing UnifiedPush supports this would be helpful!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec will be updated next month, and will be defined as RFC8030+RFC8291+RFC8292 aka webpush

Else, there is this comment: UnifiedPush/wishlist#15 (comment)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://unifiedpush.org/ Images shows Web Push now

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please inline these into the proposal instead of adding them as comments.

are defined in the Push Gateway Specification. Currently the only format available is ’event_id_only'.
- `url`: Required if `kind` is `http`, not used else. The URL to use to send notifications to. MUST be an
HTTPS URL with a path of /_matrix/push/v1/notify
- `endpoint`: Required if `kind` is `webpush`, not used else. The URL to send notification to, as defined as a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that re-using the current field seems simpler.

Comment on lines +53 to +54
If the `kind` is `webpush`, this is the user agent public key encoded in base64 url. The public key comes from a ECDH
keypair using the P-256 (prime256v1, cf. FIPS186) curve.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Links off to the sections of the RFCs that define how to do this would probably be good.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see link to 25519 curve when ed25519 is mentioned, P-256 is a known curve. And this is already implemented in libraries

Comment on lines +69 to +70
Security considerations are listed by RFC8030 [4], there are mainly resolved with RFC8291 (Encryption) and
RFC8292 (VAPID).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear -- Matrix would require all of these to be implemented then?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WebPush is defined by those 3 RFC, so it would require these RFC

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was unclear to me that "webpush" is all 3 vs. just the first one with optional additions.

[2] https://github.com/matrix-org/sygnal/blob/main/sygnal/webpushpushkin.py#L152
[3] https://spec.matrix.org/v1.9/client-server-api/#post_matrixclientv3pushersset (search for PusherData)

## Proposal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Examples of the request would be useful!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client side, this is already ~ implemented by hydrogen just changing http to webpush

Server side, this has to be merged into the server: https://github.com/matrix-org/sygnal/blob/main/sygnal/webpushpushkin.py#L351

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the whole point to bypass sygnal though?

Regardless, having examples in the MSC is important to show a full request/response.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, I just wanted to say that most of the code in sygnal webpushpushkin can be used to write a synapse webpushpusher

Comment on lines 5 to 10
Push notifications have the problem that they typically go through third-party gateways in order to
be delivered, e.g. FCM (Google) or APNs (Apple) and an app-specific gateway (sygnal). In order to
prevent these push gateways from being able to read any sensitive information the `event_id_only` format
was introduced, which only pushes the `event_id` and `room_id` of an event down the push. After
receiving the push message the client can hit the `GET /_matrix/client/r0/rooms/{roomId}/event/{eventId}`
to fetch the full event, and then create the notification based on that.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it isn't quite spelled out, but the goal here seems to be to avoid the push gateway, which avoids the application author seeing anything about the pushed information. So it allows going directly from homeserver -> push server.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 things:

  • it avoids using a push gateway
  • it adds E2EE to the notifications, so the push server don't see the content of the notifications

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reworded this §

@p1gp1g
Copy link
Author

p1gp1g commented Aug 7, 2024

Thank you for your feedback @clokep, I'm updated the proposal soon

Reword some sentences, add links
@p1gp1g
Copy link
Author

p1gp1g commented Nov 21, 2024

Client implementation: p1gp1g/hydrogen-web@e7cc488

Comment on lines +59 to +70
A VAPID (Voluntary Application Server Identification, cf RFC8292) is often needed to be able to register with a push
server.
It is proposed to add a `m.webpush` capability to the `/capabilities` endpoint with this format:
```
"m.webpush": {
"enabled": true,
"vapid": "BNbXV88MfMI0fSxB7cDngopoviZRTbxIS0qSS-O7BZCtG04khMOn-PP2ueb_X7Aeci42n02kJ0-JJJ0uQ4ELRTs"
}
```
It is also useful to decide if the client should register a pusher using `http` kind and and old style
Sygnal WebPush semantic. A client that supports this kind of pusher should use it if the server supports it too, and
not register another `http` pusher to avoid duplicate pushes.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following implementation experimentation this part has been added.

@MatMaul
Copy link

MatMaul commented Dec 2, 2024

synapse impl: element-hq/synapse#17987
updated hydrogen impl: element-hq/hydrogen-web#1201

@uhoreg uhoreg added implementation-needs-checking The MSC has an implementation, but the SCT has not yet checked it. and removed needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Dec 13, 2024

## Proposal

`PusherData` fields are now define as follow:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be clearer if the MSC only described the things it's adding, rather than redefining PusherData. Right now it requires some effort to parse what's actually new

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not very handy to not include the previous content. Every or webpush and lignes containing if kind is webpush have been added. The other things haven't been touched

@tulir
Copy link
Member

tulir commented Jan 21, 2025

Implementations seem to implement what's described in the MSC, so removing the needs-checking label

@tulir tulir removed the implementation-needs-checking The MSC has an implementation, but the SCT has not yet checked it. label Jan 21, 2025
- Web app and desktop app
- Android apps using UnifiedPush (MSC2970 was open for this and won't be required anymore)
- Android apps using FCM (It is possible to push to FCM with webpush standard [7])
- Maybe other ? We have seen apple moving a lot into web push support [8]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like this is a potential issue?


Having a webpush push kind would provide push notifications without gateway to
- Web app and desktop app
- Android apps using UnifiedPush (MSC2970 was open for this and won't be required anymore)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please inline these into the proposal instead of adding them as comments.

are already available and robuste: they are reviewed, and acknowledge by experts.

Having a webpush push kind would provide push notifications without gateway to
- Web app and desktop app
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So not really desktop apps, right? Not unless they're Electron or similar?

Comment on lines +69 to +70
Security considerations are listed by RFC8030 [4], there are mainly resolved with RFC8291 (Encryption) and
RFC8292 (VAPID).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was unclear to me that "webpush" is all 3 vs. just the first one with optional additions.

[7] https://gist.github.com/mar-v-in/2a054e3a4c0a508656549fc7d0aaeb74#webpush
[8] https://developer.apple.com/documentation/usernotifications/sending-web-push-notifications-in-web-apps-and-browsers

## Proposal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a requirement, but this is changing the architecture of Matrix, correct? By removing push gateways. Having an updated version of the diagram could be useful to show the simplification proposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API proposal A matrix spec change proposal push
Projects
Status: Proposed for FCP readiness
Development

Successfully merging this pull request may close these issues.

7 participants