feat: Implement setting common TLS parameters in ClientTrafficPolicy #2297

liorokman · 2023-12-12T08:09:50Z

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

github-actions · 2023-12-12T08:10:02Z

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

Before merging, please ensure to follow the process below:

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.

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 · 2023-12-12T08:11:30Z

@envoyproxy/gateway-reviewers please review

github-actions · 2023-12-12T08:13:03Z

🚀 Deployed on https://gateway-pr-2297-preview--eg-preview.netlify.app

zirain · 2023-12-13T00:15:29Z

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

codecov · 2023-12-13T00:20:37Z

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.

kflynn

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!

kflynn · 2023-12-20T17:46:55Z

internal/gatewayapi/clienttrafficpolicy.go

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


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

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 · 2023-12-20T17:59:40Z

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

liorokman · 2023-12-20T18:55:32Z

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

Continuing the discussion on the API in #2287.

arkodg · 2024-01-03T00:43:45Z

hey @liorokman can you rebase ?

internal/xds/translator/listener.go

internal/ir/xds.go

internal/xds/translator/listener.go

arkodg · 2024-01-03T23:27:08Z

internal/xds/translator/listener.go

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


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

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

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

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

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"

internal/xds/translator/listener.go

arkodg · 2024-01-03T23:32:06Z

internal/gatewayapi/route.go

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


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

Added a comment.

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

rethinking about this logic, here's my sugession

dont parse/set ALPN here

in the ClientTrafficPolicy layer, if ALPN is set, then set it in the IR

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

internal/gatewayapi/clienttrafficpolicy.go

arkodg · 2024-01-03T23:38:09Z

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

liorokman · 2024-01-04T05:18:31Z

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.

arkodg · 2024-01-04T22:56:21Z

internal/gatewayapi/clienttrafficpolicy.go

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


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

arkodg · 2024-01-05T21:02:04Z

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 · 2024-01-05T22:23:47Z

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>

Depends on envoyproxy#2287. 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 · 2024-01-05T22:44:04Z

@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

liorokman · 2024-01-05T22:52:21Z

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 · 2024-01-05T22:53:56Z

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>

arkodg

LGTM thanks, appreciate you addressing all the comments !

zirain · 2024-01-09T00:37:37Z

api/v1alpha1/tls_types.go

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


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

Added #2424

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

github-actions bot temporarily deployed to pull request December 12, 2023 08:13 Inactive

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

kflynn suggested changes Dec 20, 2023

View reviewed changes

arkodg marked this pull request as draft December 20, 2023 18:58

liorokman force-pushed the ctp-tls-impl branch from 2409454 to bd0d64e Compare December 23, 2023 13:26

liorokman marked this pull request as ready for review December 26, 2023 22:42

liorokman force-pushed the ctp-tls-impl branch from bd0d64e to f75f911 Compare January 3, 2024 06:18