-
Notifications
You must be signed in to change notification settings - Fork 4.9k
quic: enable certificate compression/decompression #35999
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
quic: enable certificate compression/decompression #35999
Conversation
QUIC servers are limited in how much data they can send to clients in the ServerHello before the client is validated (see https://www.rfc-editor.org/rfc/rfc9000.html#section-8). If too much data needs to be sent, an extra network round trip is needed. One way to reduce the size of the ServerHello data is to compress the certificates. This can, in some situations, remove an extra round trip. Signed-off-by: Greg Greenway <[email protected]>
@RyanTheOptimist this PR isn't ready for full review yet, but I wanted to get an early opinion on if there are any issues with how I'm implementing this, in terms of if this is a reasonable way to interact with the SSL_CTX that QUICHE lets us access. Assuming this looks ok, I'll add config knob, tests, cleanup code, etc before further review. |
Yeah, I think this looks like a solid approach. Thanks for doing this! I ran this past one of my colleagues who pointed out that Chrome only supports brotli for cert compression, not zlib. So we might want to add support for brotli too? |
As it turns out, the clients I care about only support zlib for now. Once this is added, it should be straightforward to also add brotli. I think it's probably sufficient to just add a single config knob for "support cert compression" and let the two sides negotiate it (which BoringSSL does for us). Do you think we need to add config support for supporting one algorithm but not the other? |
Makes sense.
I think that sounds reasonable, but I have some vague recollection about brotli not being supported in every Envoy build configuration. But that being said, we can just #ifdef the brotli variant out and that's fine. Honestly, I'm not sure if we need a config knob here at all. I kinda wonder if we should just always do this (or maybe have a knob to opt-out, if we really think we need some config for this). |
Sounds good to me. I'll add a runtime flag to disable in case of issues. |
I did a speedtest, and depending on the host run on, got between 30us and 70us per compression. I think that's fast enough (at least for now) to not bother with a compression cache. The rest of the TLS handshake should take quite a bit longer. |
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
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.
Sweet! Thanks for doing this!
Signed-off-by: Greg Greenway <[email protected]>
/wait One more leak, and coverage is too low |
Signed-off-by: Greg Greenway <[email protected]>
Coverage is lower because I don't know any way to trigger the ENVOY_BUG cases: https://storage.googleapis.com/envoy-pr/82efd1a/coverage/source/common/quic/cert_compression.cc.gcov.html |
Signed-off-by: Greg Greenway <[email protected]>
CC @envoyproxy/coverage-shephards: FYI only for changes made to |
* upstream/main: (21 commits) Add a CPU utilization resource monitor for overload manager (envoyproxy#34713) jwks: Add UA string to headers (envoyproxy#35977) exceptions: cleaning up macros (envoyproxy#35694) coverage: ratcheting (envoyproxy#36058) runtime: load rtds bool correctly as true/false instead of 1/0 (envoyproxy#36044) Typo in documentation of http original_src filter (envoyproxy#36060) docs: updating meeting info (envoyproxy#36052) quic: removes more references to spdy::Http2HeaderBlock. (envoyproxy#36057) json: add null support to the streamer (envoyproxy#36051) json: make the streamer a template class (envoyproxy#36001) docs: Add `apt.envoyproxy.io` install information (envoyproxy#36050) ext_proc: elide redundant copy in ext_proc filter factory callback (envoyproxy#36015) build(deps): bump yarl from 1.11.0 to 1.11.1 in /tools/base (envoyproxy#36049) build(deps): bump multidict from 6.0.5 to 6.1.0 in /tools/base (envoyproxy#36048) quic: enable certificate compression/decompression (envoyproxy#35999) Geoip fix asan failure (envoyproxy#36043) mobile: Fix missing logging output in Swift integration tests (envoyproxy#36040) http: minor code clean up to the http filter manager (envoyproxy#36027) ci/example: Dont build/test the filter example in Envoy CI (envoyproxy#36038) ci/codeql: Fix build setup (envoyproxy#36021) ... Signed-off-by: Qiu Yu <[email protected]>
QUIC servers are limited in how much data they can send to clients in the ServerHello before the client is validated (see https://www.rfc-editor.org/rfc/rfc9000.html#section-8). If too much data needs to be sent, an extra network round trip is needed.
One way to reduce the size of the ServerHello data is to compress the certificates. This can, in some situations, remove an extra round trip.
Risk Level: Low
Testing: Added unit and integration tests
Docs Changes:
Release Notes: Added
Platform Specific Features:
Runtime guard:
envoy.reloadable_features.quic_support_certificate_compression