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.

Sign up for GitHub

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

Jump to bottom

MSC2970: Remove pusher path requirement #2970

Open
wants to merge 4 commits into
base: old_master
Choose a base branch
from
Open

MSC2970: Remove pusher path requirement #2970

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c5f8479
add proposal
Sorunome Jan 17, 2021
0c051a2
Add more detail to the MSC
richvdh Apr 21, 2021
0ac32dc
Allow query strings
richvdh Apr 21, 2021
96bee63
Apply suggestions from code review
richvdh Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions proposals/2970-remove-pusher-path.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# MSC2970: Remove pusher path requirement
richvdh marked this conversation as resolved.
Show resolved Hide resolved

Recently synapse [removed the ability to set arbitrary pusher paths](https://github.com/matrix-org/synapse/pull/8865),
in order to follow the spec. During the time that synapse has allowed any pusher paths, it has shown
itself to be very useful, to the point where this fix for synapse to follow the spec has become a
real hindrance in some areas.

With the need of push notifications without FCM or apples push system getting greater and greater, an
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
With the need of push notifications without FCM or apples push system getting greater and greater, an
With the need of push notifications without [FCM](https://firebase.google.com/docs/cloud-messaging) or [APNs](https://developer.apple.com/library/archive/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/APNSOverview.html) getting greater and greater, an

elegant approach is to self-host push for your own devices and completely remove the need of any gateway.
This has been experimented with before, in projects such as [matrix-gotify](https://gitlab.com/Sorunome/matrix-gotify).
These setups, however, typically have a URL specific to a device. The requirement of pushing to
`/_matrix/push/v1/notify` makes this requirement impossible.
richvdh marked this conversation as resolved.
Show resolved Hide resolved

Additionally p2p push could push directly to an ipv6 address of a phone, if it is known. Depending on
implementation a custom path is not possible here, either.

In general, having a path requirement for the pushers basically forces app developers to implement
and host matrix-specific gateways, instead of just being able to push general json blobs directly to
your phone. The removal of a pusher path requirement would *greatly* increase flexibility here.

## Proposal

The requirement for pushers with a kind `http` to have the path `/_matrix/push/v1/notify` is removed.
The homeserver is expected to just push notifications in the specified format to the URL as given,
including host, path and query part. The requirement that pusher URLs MUST be https remains.

### Versioning

In the future a new, incompatible, push format may be introduced. As a client typically dictates what
format of push it wants, the new format could be indicated by setting the pusher kind to e.g. `httpv2`.
The client is then also expected to use a pusher URL which is capable of that new format. That way,
client, server and wherever the pusher URL points to all know what payload to expect.

## Potential issues

None
richvdh marked this conversation as resolved.
Show resolved Hide resolved

## Alternatives

Instead of allowing the pusher path to be anything, it could be introduced that the pusher path may
be prefixed with custom path fragments, but must still end in `/_matrix/push/v1/notify`. This approach
tries to solve future versioning of different payloads by setting the version within the path. However,
it would make matrix harder to integrate into existing push systems without the need for a gateway,
as a push server might not expect paths to be suffixed. The versioning via introducing a new pusher
kind seems appropriate either way, if the payload significantly changes. As such, it seems that allowing
any pusher paths is the less restrictive approach.

## Security considerations
Copy link
Member

Choose a reason for hiding this comment

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

In addition to @clokep's comment (here) there's an added concern of being able to register pushers which point at random web services. This not only can point at internal infrastructure, but also a random webserver out in the wild. A handshake process may be in order.

Copy link
Member

Choose a reason for hiding this comment

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

Is the same as @clokep's comment, or somehow different?

@clokep wrote:

targeting arbitrary URLs.

@turt2live wrote

being able to register pushers which point at random web services

in any case, IMHO: this is no different from any other service that provides webhooks: there is no requirement on the target path for a Github event notifications, for example.

Pivoting into internal infrastructure should be handled with IP blacklists etc (an effective IP blacklist is already required for URL previews.)

Other services that provide webhooks don't require a handshake, and I'm unconvinced that we need one. It sounds like an an annoyance for people to set up.

Concerns over traffic amplification should be handled with rate-limiting (and making sure that pushers that don't get the correct response get disabled), which is orthogonal to the URL path.

Copy link
Member

Choose a reason for hiding this comment

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

Is the same as @clokep's comment, or somehow different?

@clokep wrote:

targeting arbitrary URLs.

My intention from that blurb is pretty much the same as what @turt2live wrote.

in any case, IMHO: this is no different from any other service that provides webhooks: there is no requirement on the target path for a Github event notifications, for example.

Yes, that's true. We should at least document that this does have security implications to it.

(P.S. Sorry for forgetting to use a thread!)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I think the security implications of sending less restricted HTTP requests from the homeserver are important to note but worth the minor risk.

Copy link
Member

Choose a reason for hiding this comment

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

I hope I've now described the security considerations in question.

Copy link
Member

Choose a reason for hiding this comment

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

Seems reasonable to me! 👍


None