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

quic: enable certificate compression/decompression #35999

Merged
merged 7 commits into from
Sep 10, 2024

Conversation

ggreenway
Copy link
Contributor

@ggreenway ggreenway commented Sep 5, 2024

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

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 <ggreenway@apple.com>
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #35999 was opened by ggreenway.

see: more, trace.

@ggreenway
Copy link
Contributor Author

@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.

@RyanTheOptimist
Copy link
Contributor

@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?

@ggreenway
Copy link
Contributor Author

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?

@RyanTheOptimist
Copy link
Contributor

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.

Makes sense.

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?

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).

@ggreenway
Copy link
Contributor Author

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.

@ggreenway
Copy link
Contributor Author

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 <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Copy link

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #35999 was synchronize by ggreenway.

see: more, trace.

@ggreenway ggreenway marked this pull request as ready for review September 9, 2024 20:27
RyanTheOptimist
RyanTheOptimist previously approved these changes Sep 9, 2024
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a 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 <ggreenway@apple.com>
@ggreenway
Copy link
Contributor Author

/wait

One more leak, and coverage is too low

Signed-off-by: Greg Greenway <ggreenway@apple.com>
@ggreenway
Copy link
Contributor Author

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 <ggreenway@apple.com>
Copy link

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

🐱

Caused by: #35999 was synchronize by ggreenway.

see: more, trace.

@RyanTheOptimist RyanTheOptimist merged commit 627187b into envoyproxy:main Sep 10, 2024
40 checks passed
unicell added a commit to unicell/envoy that referenced this pull request Sep 11, 2024
* 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 <qiuyu@apple.com>
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.

2 participants