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

feat(translator): client tls session resumption #4293

Merged
merged 25 commits into from
Oct 15, 2024

Conversation

guydc
Copy link
Contributor

@guydc guydc commented Sep 20, 2024

What type of PR is this?

What this PR does / why we need it:

Introduces an API for TLS session management:

  • Disable TLS resumption by default. Allow users to opt-in.
  • Adds E2E Tests and Docs for ClientTrafficPolicy TLS settings

Which issue(s) this PR fixes:
Fixes #4268, #2422, #2421

@guydc guydc requested a review from a team as a code owner September 20, 2024 19:04
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.79%. Comparing base (e0455ac) to head (bffd67f).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4293      +/-   ##
==========================================
- Coverage   65.81%   65.79%   -0.02%     
==========================================
  Files         200      200              
  Lines       24177    24190      +13     
==========================================
+ Hits        15911    15917       +6     
- Misses       7129     7134       +5     
- Partials     1137     1139       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@guydc
Copy link
Contributor Author

guydc commented Sep 25, 2024

Following yesterday's community meeting, some additional industry context.

Currently, Mozilla's SSL config generator recommends disabling stateless resumption (session tickets) for nginx, apache and haproxy for security reasons.

This is mostly due to insufficient rotation of session ticket encryption keys in these projects. There is an ongoing discussion on changing the recommendation for newer versions of nginx where keys are rotated: mozilla/server-side-tls#135.

BoringSSL rotates encryption keys by default every 48 hours: https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#Session-tickets. Envoy doesn't change this behavior or make the rotation schedule configurable. Industry leaders like cloudflare rotate session ticket encryption keys every hour: https://blog.cloudflare.com/keyless-ssl-the-nitty-gritty-technical-details/.

There is no security recommendation to disable stateful session resumption. However:

nginx ingress by default disables stateful and stateless session resumption, but makes it possible to opt-in:

I propose that we change the current behavior and disable both resumption methods by default, but allow users to opt-in similar to nginx ingress.

Signed-off-by: Guy Daich <[email protected]>
@guydc guydc changed the title api: client tls session resumption feat(translator): client tls session resumption Oct 7, 2024
guydc added 2 commits October 7, 2024 15:20
Signed-off-by: Guy Daich <[email protected]>
Signed-off-by: Guy Daich <[email protected]>
@@ -69,3 +79,56 @@ spec:
- kind: "Secret"
group: ""
name: "client-mtls-certificate"
---
Copy link
Member

Choose a reason for hiding this comment

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

need to record how this is generated for future maintenance.

// of different resumption methods. Performance gains from resumption are diminished when
// Envoy proxy is deployed with more than one replica.
// +optional
SessionResumptionSettings *SessionResumptionSettings `json:"sessionResumption,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer SessionResumption

Comment on lines 23 to 31
SessionTimeout *gwapiv1.Duration `json:"sessionTimeout,omitempty"`

// SessionResumptionSettings determine the proxy's supported TLS session resumption option.
// By default, Envoy Gateway does not enable session resumption. Use sessionResumption to
// enable stateful and stateless session resumption. Users should consider security impacts
// of different resumption methods. Performance gains from resumption are diminished when
// Envoy proxy is deployed with more than one replica.
// +optional
SessionResumptionSettings *SessionResumption `json:"sessionResumption,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

can we use following structure?

session:
  timeout:
  resumption:

if there're more than 1 property with session prefix, prefer to move them into a struct.

Copy link
Member

@zhaohuabing zhaohuabing Oct 9, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it seems that different implementations have different meanings for session timeout.

BoringSSL seems to treat this as a hard timeout after which a new handshake is required:

This is how long we are willing to use the secret to encrypt traffic without fresh key material.

In envoy, this is also used as a hint on session tickets.

OpenSSL seems to treat this as a soft timeout after which resumption cannot occur, but an active session may still continue (?):

Whenever a new session is negotiated, it is assigned a timeout value, after which it will not be accepted for session reuse.

@arkodg requested that we leave sessionTimeout out of this PR. I will change the API to @zirain's proposal, to support for future addition of session-related settings that are not specific to resumption.

@guydc guydc requested a review from arkodg October 9, 2024 23:27
@@ -15,6 +15,10 @@ type ClientTLSSettings struct {
// +optional
ClientValidation *ClientValidationContext `json:"clientValidation,omitempty"`
TLSSettings `json:",inline"`

// Session defines setting related to TLS session management.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Session defines setting related to TLS session management.
// Session defines settings related to TLS session management.


// Session defines setting related to TLS session management.
type Session struct {
// Resumption determine the proxy's supported TLS session resumption option.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Resumption determine the proxy's supported TLS session resumption option.
// Resumption determines the proxy's supported TLS session resumption option.

@@ -30,6 +30,8 @@ http:
minVersion: "1.0"
alpnProtocols:
- some-other-protocol
sessionTimeout: 30s
Copy link
Contributor

Choose a reason for hiding this comment

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

sessionTimeout clean up needed here

@guydc guydc requested a review from arkodg October 11, 2024 10:36
Copy link
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

require user-faced doc.

Copy link
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

@guydc guydc added the release-note Indicates a required release note label Oct 15, 2024
@guydc guydc merged commit 1f29518 into envoyproxy:main Oct 15, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Indicates a required release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support TLS Session Resumption settings
4 participants