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: Implement setting common TLS parameters in ClientTrafficPolicy #2297

Merged
merged 16 commits into from
Jan 9, 2024

Conversation

liorokman
Copy link
Contributor

What this PR does / why we need it:

Implementation for setting Ciphers, ECDHCurves, ALPN protocols, Signature Algorithms, and TLS protocol versions in ClientTrafficPolicy.

Which issue(s) this PR fixes:
Implements #2286
Based on #2287

Related to #1846

@liorokman liorokman requested a review from a team as a code owner December 12, 2023 08:09
Copy link

🚀 Thank you for contributing to the Envoy Gateway project! 🚀

Before merging, please ensure to follow the process below:

  1. Requesting Reviews:
  • cc @envoyproxy/gateway-reviewers team for an initial review.
  • After the initial review, reviewers should request the @envoyproxy/gateway-maintainers team for further review.
  1. Review Approval:
  • Your PR needs to receive at least two approvals.
  • At least one approval must come from a member of the gateway-maintainers team.

NOTE: Once your PR is under review, please do not rebase and force push it. Otherwise, it will force your reviewers to review the PR from scratch rather than simply look at your latest changes.

What's more, you can help expedite the processing of your PR by
  • Ensuring you have self-reviewed your work according to the project's Contribution Guidelines.
  • If your PR addresses a specific issue, make sure to mention it in the PR description.
  • Respond promptly if there are any test failures or suggestions for improvements that we comment on.

@liorokman
Copy link
Contributor Author

@envoyproxy/gateway-reviewers please review

Copy link

github-actions bot commented Dec 12, 2023

@github-actions github-actions bot temporarily deployed to pull request December 12, 2023 08:13 Inactive
@zirain
Copy link
Contributor

zirain commented Dec 13, 2023

please don't try to implement before API accepted, maybe a waste of your time.

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Attention: 62 lines in your changes are missing coverage. Please review.

Comparison is base (3812cb5) 64.66% compared to head (3ca7a9a) 64.63%.
Report is 5 commits behind head on main.

Files Patch % Lines
internal/ir/zz_generated.deepcopy.go 5.08% 52 Missing and 4 partials ⚠️
internal/ir/xds.go 79.16% 4 Missing and 1 partial ⚠️
internal/xds/translator/listener.go 98.07% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2297      +/-   ##
==========================================
- Coverage   64.66%   64.63%   -0.04%     
==========================================
  Files         115      115              
  Lines       16844    17326     +482     
==========================================
+ Hits        10893    11199     +306     
- Misses       5256     5415     +159     
- Partials      695      712      +17     

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

@github-actions github-actions bot temporarily deployed to pull request December 13, 2023 06:22 Inactive
Copy link
Contributor

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

This looks like a really good start. I'd like some more attention paid to the defaults (at minimum, we absolutely must state what the defaults are!), and I kind of think that if the version stanza sets the minimum and maximum it should be named versions plural, but that's a really minor nit.

I'm marking this as "request changes" because the defaults thing is important, I think. Thanks for digging into this!

if tlsParams.Version != nil {
var minVersion, maxVersion int
var err error
if minVersion, err = tlsVersionToNumericVersion(tlsParams.Version.Min); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to imply that settings like this:

version:
  min: TLSv1_2

will not work, since it seems that the default for max is TLS_Auto.

I think that "auto" should mean "the earliest supported version" for minimum and "the latest supported version" for maximum. (While we're at it, I also don't see any value in forcing people to type "TLS_" everywhere: just let the versions be strings like "v1.3", and "auto".)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was resolved by adopting defaults of minimum TLS v1.2 and maximum TLS v1.3.

The validation that min <= max was implemented as a CEL rule.

@kflynn
Copy link
Contributor

kflynn commented Dec 20, 2023

D'oh!! I should've read #2287 before commenting here. Mea culpa.

@liorokman
Copy link
Contributor Author

D'oh!! I should've read #2287 before commenting here. Mea culpa.

Continuing the discussion on the API in #2287.

@arkodg arkodg marked this pull request as draft December 20, 2023 18:58
@liorokman liorokman marked this pull request as ready for review December 26, 2023 22:42
@arkodg
Copy link
Contributor

arkodg commented Jan 3, 2024

hey @liorokman can you rebase ?

internal/ir/xds.go Outdated Show resolved Hide resolved
internal/ir/xds.go Outdated Show resolved Hide resolved

func buildTLSVersion(version *gwv1a1.TLSVersion) tlsv3.TlsParameters_TlsProtocol {
lookup := map[gwv1a1.TLSVersion]tlsv3.TlsParameters_TlsProtocol{
gwv1a1.TLSv10: tlsv3.TlsParameters_TLSv1_0,
Copy link
Contributor

Choose a reason for hiding this comment

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

thinking out loud, about the API, should the user strings be 1.2, 1.3 instead of v1_2 and v1_3 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it matters that much. I'll change to whatever is requested.

Copy link
Contributor

Choose a reason for hiding this comment

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

FTR I think "1.2" and "1.3" are great. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

@liorokman sorry for churn, can you go ahead and update the strings to 1.2 & 1.3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

One small thing to notice: when setting the TLS protocol version to the string 1.0, sometimes YAML/JSON parsers detect the value as a number and not as a string and remove the .0 from the value. It's important to quote the value in order to keep the .0 in it as part of a string.

This can happen when the YAML text is unmarshaled into untyped maps. I ran into this when updating the YAML files that comprise the tests. It also happens with the tool that generates the CRD, which is why the validation enum comment contains quotes.

Example:

spec:
    tls:
       # This sometimes gets parsed wrong, since it becomes a number and 
       # loses the fractional part.
       minVersion: 1.0
       # This always gets parsed correctly as a string.
       minVersion: "1.0"

@@ -995,6 +996,9 @@ func (t *Translator) processTCPRouteParentRefs(tcpRoute *TCPRouteContext, resour
},
TLS: &ir.TLS{Terminate: irTLSConfigs(listener.tlsSecrets)},
}
if irListener.TLS.Terminate != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice !
a comment in here saying enable http/1.1 & http/2 alpn protocols by default would help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

just realized this is the logic for TCPRoute, what about HTTPRoute and GRPCRoute ?

Copy link
Contributor

@arkodg arkodg Jan 5, 2024

Choose a reason for hiding this comment

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

rethinking about this logic, here's my sugession

  1. dont parse/set ALPN here
  2. in the ClientTrafficPolicy layer, if ALPN is set, then set it in the IR
  3. In the xds layer, if ALPN is nil, set to http/1.1 and http/2, if set, set to the value in ALPN field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@arkodg
Copy link
Contributor

arkodg commented Jan 3, 2024

@liorokman curious why the permissions was changed for so many files

@liorokman
Copy link
Contributor Author

curious why the permissions was changed for so many files

When I started with the tests, it stood out like a sore thumb that so many yaml files were marked as executable.

if httpIR.HTTP3 != nil {
httpIR.TLS.ALPNProtocols = []string{"h3"}
} else if tlsParams != nil {
httpIR.TLS.ALPNProtocols = make([]string, len(tlsParams.ALPNProtocols))
Copy link
Contributor

@arkodg arkodg Jan 4, 2024

Choose a reason for hiding this comment

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

shouldnt this line

httpIR.TLS.ALPNProtocols = make([]string, len(tlsParams.ALPNProtocols))

be under if tlsParams.ALPNProtocols != nil

else it will end up resetting the value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@arkodg
Copy link
Contributor

arkodg commented Jan 5, 2024

hey @liorokman one more thing I noticed was the ALPN string for http/2 - lets set it to h2 in the API
its easier to define h2c in the future https://developer.mozilla.org/en-US/docs/Glossary/ALPN

apart from this, LGTM

this PR also needs a rebase

@liorokman
Copy link
Contributor Author

liorokman commented Jan 5, 2024

hey @liorokman one more thing I noticed was the ALPN string for http/2 - lets set it to h2 in the API its easier to define h2c in the future https://developer.mozilla.org/en-US/docs/Glossary/ALPN

ALPN can never be used for h2c, by the very definition of h2c (HTTP/2 over cleartext). ALPN is a TLS extension, while h2c is only possible if TLS is not being used and HTTP/2 is not negotiated but rather used because of pre-knowledge. Without TLS there is no ALPN, but with TLS there is no h2c.

If EG supports h2c, it will not be a good fit to enable it by configuring TLS or ALPN, which means that these constants could never mean h2c.

I suggest keeping the http/2 string in the ALPN constants, since if http/2 is being used it's never going to be anything other than h2.

Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
      Depends on envoyproxy#2287.

Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
* Make the new tests pass

Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
* Use non API types in the IR structure
* Rename TLSListenerConfig to TLSConfig
* Use string slices instead of Enum slices for ALPN in the IR structure

Signed-off-by: Lior Okman <lior.okman@sap.com>
underscore separator.

Signed-off-by: Lior Okman <lior.okman@sap.com>
  translates to the required defaults in the XDS layer
* Fixed translation issues between the ALPN constants and the values
  required on the XDS level

Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
@arkodg
Copy link
Contributor

arkodg commented Jan 5, 2024

@liorokman I understand that h2c in the context of TLS doesnt make much sense, which is also why we shouldnt allow it in the API
but there is an existing RFC definition for ALPN protocols here https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids and we should reuse it imo

@arkodg arkodg removed their assignment Jan 5, 2024
@liorokman
Copy link
Contributor Author

Is it OK to assume that valid Enum values for the ALPN field will always be from the IANA registry? Or should I keep the lookup function that currently allows the enum to be different than the technical values required on the wire?

@arkodg
Copy link
Contributor

arkodg commented Jan 5, 2024

Is it OK to assume that valid Enum values for the ALPN field will always be from the IANA registry? Or should I keep the lookup function that currently allows the enum to be different than the technical values required on the wire?

yeah 1:1 works, and we can do w/o a lookup func for alpn for now imo

  strings.
* Replace the user-facing "http/2" ALPN string with "h2".

Signed-off-by: Lior Okman <lior.okman@sap.com>
Copy link
Contributor

@arkodg arkodg 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, appreciate you addressing all the comments !

@arkodg arkodg requested review from kflynn and a team January 8, 2024 23:41
@@ -5,8 +5,8 @@

package v1alpha1

// +kubebuilder:validation:XValidation:rule="has(self.minVersion) && self.minVersion == 'v1_3' ? !has(self.ciphers) : true", message="setting ciphers has no effect if the minimum possible TLS version is 1.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add some test for these in a separated PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added #2424

@arkodg arkodg merged commit 17c57fc into envoyproxy:main Jan 9, 2024
20 checks passed
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.

4 participants