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

AESGCM gateway #65

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

AESGCM gateway #65

wants to merge 5 commits into from

Conversation

karmanyaahm
Copy link
Member

This is an explicit gateway, taking in the URL and using discovery like @p1gp1g suggested. @quqkuk what do you think about this?

Comment on lines +20 to +24
type Aesgcm struct {
Enabled bool `env:"UP_GATEWAY_AESGCM_ENABLE"`
path string
discovery []byte
}
Copy link
Member Author

Choose a reason for hiding this comment

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

for posterity, this file is practically copied and pasted from generic.go, which has existed in common proxies for a while, just without discovery and without documentation.

@karmanyaahm karmanyaahm requested review from p1gp1g and removed request for p1gp1g December 28, 2023 03:31
@karmanyaahm
Copy link
Member Author

@quqkuk no hurry if you're away for the holidays or busy, just wanted to ping you to let you know that I'll merge this if this approach works for you. Or, if there are changes that need to be made for this to work with telegram I can do that too.

@quqkuk
Copy link

quqkuk commented Jan 6, 2024

Sorry for not replying earlier, as you thought I have been away for the holidays.
Everything in this patch seems good, the only thing that bothers me (and is why I wrote a transparent proxy) is gateway discovery - We cannot count on the server to know what gateway to use, do you think it's best to put that logic in the application by getting the gateway endpoint from the user (with, eventually, a fallback)? I have other ideas, but all of them are trade-offs between getting the gateway from the user and getting it from the push provider.
A Transparent proxy would require no configuration, and could be made discoverable by replying to GET requests with an apt JSON object, but deployment is not trivial.
To be clear, I don't want to defend the transparent proxy as the superior solution, I'm just probing what do you think is the best way to use the gateway you propose

@p1gp1g
Copy link
Member

p1gp1g commented Jan 9, 2024

Sorry for not replying earlier, for the same reason :)

@quqkuk:
I assume you want to use this gateway for Telegram. They seem pretty open to push protocols (https://core.telegram.org/api/push-updates, they have wrongly stated that UnifiedPush is compatible with Firefox's simple push), I'm pretty sure they will consider supporting webpush (RFC) if we ask for.

About the transparent proxy, we have already think about it (UnifiedPush/wishlist#15 (comment)), we had some discussion about this topic outside this issue, which doesn't help to follow everything. Basically, this could be a good solution if we were pushing for its adoption, but there are many downsides:

  • We want to push the webpush standard (RFC) (WebPush over UnifiedPush wishlist#15 (comment)), pushing the draft adoption goes against this goal and will just add inertia to the migration.
  • A transparent proxy doesn't follow our current specifications. This would require to adapt it and probably update the push servers.
  • We try to keep users setup as less divided as possible. This gateway will very probably not be implemented by push providers, and this will add division in the setups.
  • The protocol aims to be as simple as possible, we have decided to also follow the webpush standard, because we think that protocol and sync-on-push are the way to go.
  • A transparent proxy can introduce some security consideration, like smuggling, but I think this is pretty limited in this context.

It is technically easy to do an automatic push gateway discovery request, like we have done for matrix (most push providers have a built-in gateway). But I think this gateway (webpush-draft4 -> UnifiedPush) can be useful only with centralized services like Telegram, so having a default one (for instance on unifiedpush.org) which can be manually changed by the user on their application if they want to change it (without an auto discovery request).

I am personally not even for a webpush-draft4 gateway because it pushes in the wrong direction, and because the draft allows to do sync-on-push which is also something we recommend. But I'm OK to review my point of view on this. But the transparent proxy has too many downsides to adopt it.


@karmanyaahm, if we continue with this gateway, is it possible to use WebPush-Draft4 (or something like this) instead of AESGCM to refer to this gateway ?

@karmanyaahm
Copy link
Member Author

I made the name AESGCM because that's what the Content-Encoding for this is. It's also prettier than WebPushDraft4.

@quqkuk
Copy link

quqkuk commented Jan 12, 2024

I'm pretty sure they will consider supporting webpush (RFC) if we ask for.

https://bugs.telegram.org/c/35218

  • A transparent proxy doesn't follow our current specifications. This would require to adapt it and probably update the push servers.

Wait - how? Because the spec doesn't say that the message may be transformed?

having a default one (for instance on unifiedpush.org) which can be manually changed by the user on their application if they want to change it (without an auto discovery request).

I am ok with that, thank you for answering

@p1gp1g
Copy link
Member

p1gp1g commented Jan 12, 2024

I'm pretty sure they will consider supporting webpush (RFC) if we ask for.

https://bugs.telegram.org/c/35218

👍 We can also say that this protocol can be introduced as the 13th supported protocol, to leave the legacy webpush with aesgcm available to be backward compatible.

  • A transparent proxy doesn't follow our current specifications. This would require to adapt it and probably update the push servers.

Wait - how? Because the spec doesn't say that the message may be transformed?

Yep, we have to update the discovery request, to add feature discovery (which I wish will never happen). Then we should state that the current spec should be followed when the draft4/aesgcm is not available OR the header Content-Encoding: aesgcm is not available. Then specify the behavior with the draft4/aesgcm feature when there is this header. Which adds a lot of complexity for something that should be abandoned :(

having a default one (for instance on unifiedpush.org) which can be manually changed by the user on their application if they want to change it (without an auto discovery request).

I am ok with that, thank you for answering

If you're OK, let's see how telegram answer the request first then we may go for it then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants