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

GZIP Compression #199

Open
danielnehrig opened this issue Oct 7, 2022 · 15 comments
Open

GZIP Compression #199

danielnehrig opened this issue Oct 7, 2022 · 15 comments
Labels
author-willing-to-impl The author of this issue is willing to try to implement the solution themselves. C-enhancement Category: enhancement tribble-reported This issue was reported through Tribble.

Comments

@danielnehrig
Copy link

This issue is requesting an enhancement to Perseus. Details of the scope will be available in issue labels.
The user described the problem related to this request as follows:

server responses to the client can be quite big and adding another proxy infront of the app to gzip compress the response introduces additional cost on the infrastructure if might not needed.

The user described the issue as follows:

I recommend adding gzip compression to perseus responses

  • The author is willing to attempt an implementation: true
Tribble internal data

dHJpYmJsZS1yZXBvcnRlZCxDLWVuaGFuY2VtZW50LGF1dGhvci13aWxsaW5nLXRvLWltcGw=

@github-actions github-actions bot added author-willing-to-impl The author of this issue is willing to try to implement the solution themselves. C-enhancement Category: enhancement tribble-reported This issue was reported through Tribble. labels Oct 7, 2022
@arctic-hen7
Copy link
Member

arctic-hen7 commented Oct 7, 2022

This is perfectly reasonable, but should be done at the level of server integrations. Serving exported apps with gzip should be the responsibility of the file host.

This should just be a matter of using the right middleware for each integration.

(PRs are welcome!)

@danielnehrig
Copy link
Author

warp already supports it
what do you think about adding the .with(warp::filters::compression:gzip()) to the perseus::dflt_server ?
@arctic-hen7

@arctic-hen7
Copy link
Member

Yeah certainly! Do you want to submit a PR for that?

@danielnehrig
Copy link
Author

yes

@ITwrx
Copy link

ITwrx commented Oct 12, 2022

i think this should be opt-in for security reasons.

What i would like is if pre-rendered pages (and any other relevant assets) were automatically, statically gzipped by Perseus, so nginx could serve that without anything using on-the-fly gzip compression.

@danielnehrig
Copy link
Author

danielnehrig commented Oct 13, 2022

could you elaborate how the dynamic warp compression filter would be exploitable in that way?

also i'll build it into the perseus builder pattern

@arctic-hen7
Copy link
Member

The idea of static gzipping is attractive, but only in certain cases. Overall, I think it should be done at request time, simply because Perseus needs to read and modify the stuff on disk extensively when being run aa a server. When exported, I think it should be solely the responsibility of the server the user chooses, just to make things simple.

@ITwrx
Copy link

ITwrx commented Oct 13, 2022

@danielnehrig I don't know anything about Warp's implementation and was only referring to on-the-fly compression side channel attacks, in general; like BREACH
I'm also not trying to discourage the development of the feature, as on-the-fly compression makes a noticeable difference and some projects may determine that compression side channel attacks are beyond their security concerns. I only think that it should not be enabled/activated by default, especially since this is something that is usually decided by the server admin and sometimes the developer and the server admin(s) are not the same people.
I'm not familiar with the Perseus builder pattern. Maybe that means the user would have to opt-in?

@arctic-hen7 I only mention static gzipping as a safe alternative way to get some compression benefits when feasible, not suggesting as a complete replacement for every project. I can also believe that on-the-fly compression is more convenient to implement and is certainly more comprehensive in it's coverage. FWICT, (and IIRC) it is also fundamentally insecure.

When exported, I think it should be solely the responsibility of the server the user chooses, just to make things simple.

would that also include the manual enabling of the feature? because that's what i'm mainly concerned with. I think any on-the-fly compression option should be documented as a possible security risk and made opt-in to protect users who may not know about the risks.

@danielnehrig
Copy link
Author

danielnehrig commented Oct 13, 2022

I'm not familiar with the Perseus builder pattern. Maybe that means the user would have to opt-in?

yes it would be opt-in

@arctic-hen7
Copy link
Member

@ITwrx understood. With static exporting, the user uses some third-party program to serve their files, which should handle any compression independently of Perseus.

I think what @danielnehrig means by the builder pattern is the PerseusApp struct that allows setting options for the app in a builder style. (Do correct me if that's not what you mean though!) Adding an option to signal to server integrations that compression should be used could work, or it might be simpler to use feature flags on the integrations themselves. That is, the perseus-warp package's default server would have compression added only if the dflt-server-gzip flag were set. That would make it opt-in, and would allow the user full control when working with a custom server. This would also allow individual server integrations to support other types of compression in future if needed, without modifying the core. I would prefer this over integration with PerseusApp to be honest.

@danielnehrig
Copy link
Author

@ITwrx understood. With static exporting, the user uses some third-party program to serve their files, which should handle any compression independently of Perseus.

I think what @danielnehrig means by the builder pattern is the PerseusApp struct that allows setting options for the app in a builder style. (Do correct me if that's not what you mean though!) Adding an option to signal to server integrations that compression should be used could work, or it might be simpler to use feature flags on the integrations themselves. That is, the perseus-warp package's default server would have compression added only if the dflt-server-gzip flag were set. That would make it opt-in, and would allow the user full control when working with a custom server. This would also allow individual server integrations to support other types of compression in future if needed, without modifying the core. I would prefer this over integration with PerseusApp to be honest.

Correct that's what I meant what would you prefer though over feature flag or the builder pattern ?

@arctic-hen7
Copy link
Member

@danielnehrig I think the feature flag approach would work best.

@danielnehrig
Copy link
Author

alright then

@arctic-hen7
Copy link
Member

Having looked into all this further, I think compression should be the default for the Wasm bundle in particular (since I've had reports on Discord of up to seven point Lighthouse improvements, to 100, with vs. without this), and shoudl be done statically. To my understanding, BREACH is not a concern here because it's a static file with no secrets in it. As for page states, that should be completely opt-in if we even decide to support it at all. But brotli compression of the Wasm bundle should be done automatically by the CLI (with an argument to disable this behavior). There should also be gzip compression, and server integrations should be responsible for figuring out what to send. I recall this article had some nice explanations of this.

@danielnehrig are you still thinking of working on this?

@danielnehrig
Copy link
Author

i would tackle this next after the cookies #158 implementation but probably only for warp for now since i've already implemented that for my project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author-willing-to-impl The author of this issue is willing to try to implement the solution themselves. C-enhancement Category: enhancement tribble-reported This issue was reported through Tribble.
Projects
None yet
Development

No branches or pull requests

3 participants