-
Notifications
You must be signed in to change notification settings - Fork 382
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
MSC2970: Remove pusher path requirement #2970
base: old_master
Are you sure you want to change the base?
Conversation
@mscbot fcp merge |
This FCP proposal has been cancelled by #2970 (comment). Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
@mscbot concern we need to come up with a versioning scheme that works before introducing this change |
matrix-org/matrix-spec#677 suggests a versioning scheme that works |
I believe that this MSC is missing some security concerns from allowing arbitrary URLs. It could allow pivoting into internal infrastructure (e.g. CVE-2021-21273 in Synapse) or targeting arbitrary URLs. |
@mscbot concern #2970 (comment) |
this concern was really only placeholder for a different concern: @mscbot resolve we need to come up with a versioning scheme that works before introducing this change |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me! 👍
duplicate concern: @mscbot resolve #2970 (comment) |
proposals/2970-remove-pusher-path.md
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 |
There was a problem hiding this comment.
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.
This isn't really ready for FCP: it needs more info in the "Security considerations" section, and an "unstable prefixes" section of some kind. Sadly I'm failing to get round to it currently and it's not really my highest priority. @mscbot fcp cancel |
preceded by a `user@` specifcication). | ||
* The URL must consist solely of ASCII characters. (Unicode hostnames should | ||
be punicode-encoded. Non-ASCII bytes in the path should be percent-encoded.) | ||
* The URL may not exceed 8000 characters in length. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8000 was picked fairly randomly based on https://tools.ietf.org/html/rfc7230#section-3.1.1.
* We could require that the push endpoint respond to an empty `ping` request - | ||
and in particular that it must respond at the time that the pusher is | ||
configured. This would significantly reduce the scope for abuse by directing | ||
pushes to inappropriate endpoints. | ||
|
||
However, it is considered out-of-scope for this MSC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this, I think it would be quite a beneficial addition. However, it would be a backwards-incompatible change to the pusher API (currently, there is no way to do a "test notification" other than by making up an event of some form), and generally needs a bit more time to write into an MSC than I have time for right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest something like this, though I thought about requiring either that the pusher URL ends with /_matrix/push/v1/notify
or that it responds to GET
requests with a predetermined response that proves it's expecting pushes.
Agreed this should be out-of-scope for this MSC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed above this could be done pretty easily by adding an httpv2
pusher type? I guess this would require clients to know the types that a server supports though, which they might not currently.
Also -- would this be compatible with the proposed projects above (matrix-gotify, etc.)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkasak Another way to verify the pusher URL is how JMAP does it like a phone number verification code.
when a subscription is created, the JMAP server immediately sends a PushVerification object to that URL
NOT make any further requests to the URL until the client receives the push and updates the subscription with the correct verification code
Co-authored-by: Denis Kasak <dkasak@termina.org.uk>
HTTPS is preferable for obvious reasons, but We don't consider it the job of | ||
the pusher API to enforce this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTPS is preferable for obvious reasons, but We don't consider it the job of | |
the pusher API to enforce this. | |
HTTPS is preferable for obvious reasons, but we don't consider it the job of | |
the pusher API to enforce this. |
Why does this have the needs-implementations path if old synapse versions already worked just that way, and thus we can see that it was tried and tested well? |
Rendered
Signed-off-by: Sorunome mail@sorunome.de