api: Add TLS configuration attributes in ClientTrafficPolicy #2287

liorokman · 2023-12-10T09:58:43Z

What this PR does / why we need it:
This PR adds the relevant optional TLS configuration attributes to ClientTrafficPolicy

Which issue(s) this PR fixes:

Related to #2286

github-actions · 2023-12-10T09:58:55Z

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

github-actions · 2023-12-10T10:02:07Z

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

shawnh2 · 2023-12-10T13:10:08Z

api/v1alpha1/tls_types.go

+type TLSVersions struct {
+	// Min specifies the minimal TLS verison to use
+	// +optional
+	Min TLSVersion `json:"min,omitempty"`


using *TLSVersion, optional fields are always ptr type

shawnh2 · 2023-12-10T13:10:19Z

api/v1alpha1/tls_types.go

+
+	// Max specifies the maximal TLS verison to use
+	// +optional
+	Max TLSVersion `json:"max,omitempty"`


shawnh2 · 2023-12-10T13:11:11Z

api/v1alpha1/tls_types.go

+	// Version details the minimum/maximum TLS protocol verison that
+	// should be supported by this listener.
+	// +optional
+	Version *TLSVersions `json:"version,omitempty"`


do we need to setup a default value for this field in EG?

Today EG simply doesn't specify any default, leaving the default up to Envoy itself. Maybe keep that behaviour, so that there will be backwards compatibility with previous EG versions?

The default min version is TLSv1_2
https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/common.proto
Using TLS protocol versions below TLSv1_2 has serious security considerations and risks.
what is the reason to use version below TLSv1_2 ?

In some cases there are legacy clients that don't support TLS 1.2 but require an older version.

I would vote for being explicit here. Setting the default to "whatever Envoy wants" leaves behavior interesting undefined as the Envoy version changes.

As @arkodg notes, Envoy's current default minimum version is TLS 1.2, and there are some very good reasons not to go below that. I'd vote to define Envoy Gateway's minimum as TLS 1.2, while still allowing users to select 1.0 or 1.1 if they need to.

(Allowing the maximum to float with Envoy is OK with me, FTR.)

(I'd also vote to call this stanza versions plural rather than version singular.)

I would vote for being explicit here. Setting the default to "whatever Envoy wants" leaves behavior interesting undefined as the Envoy version changes.

"Whatever Envoy wants" is the current default, and is also what would happen if there is no ClientTrafficPolicy attached at all.

The TLS_Auto constant maps all the way to the Envoy Proxy's enum, which is defined as ⁣Envoy will choose the optimal TLS version. Setting the minimum in EG to something other than TLS_Auto means that if Envoy Proxy fixes its default, maybe for solving some future CVE, then Envoy Gateway doesn't automatically benefit from the fix in Envoy Proxy.

(I'd also vote to call this stanza versions plural rather than version singular.)

I'm thinking to maybe get rid of the versions intermediate level, and align more closely with how Envoy Proxy models it. I think this would make it clearer.

WDYT?

yeah, we could rm versions in favor of minVersion & maxVersion , thoughts @kflynn ?

minVersion and maxVersion work for me.

I would definitely prefer having Envoy Gateway dictate a particular minimum version for predictability, but I don't feel all that strongly there.

Modified the version specification to remove the intermediate level.

I also set the default minimal version default to be 1.2 and the default maximal version to be 1.3. This is actually exactly the same as it was before, since this is Envoy Proxy's default. I agree that it makes sense to document it more clearly.

shawnh2 · 2023-12-10T13:37:23Z

api/v1alpha1/tls_types.go

+	// ALPNProtocols supplies the list of ALPN protocols that should be
+	// exposed by the listener. If left empty, ALPN will not be exposed.
+	// +optional
+	ALPNProtocols []string `json:"alpnProtocols,omitempty"`


suggest a enumerate type instead of string, since envoyproxy TLS only support two values: h2,http/1.1 or http/1.1.

IMO, we can add http protocol version enumerate type under api/v1alpha1/shared_types.go, like:

HTTPProtocolVersion1_1 = "http/1.1" HTTPProtocolVersion2 = "http/2"

and we can translate their combination into envoyproxy supported values:

["http/1.1", "http/2"] will be seen as h2,http/1.1

["http/1.1"] will be seen as http/1.1

invalid otherwise

WDYT?

I think maybe it's better to keep this as strings. I tried to create a configuration in Envoy where the ALPN was something else and Envoy had no issue with that - I was able to use openssl s_client to negotiate that different protocol string.

If somebody uses a custom version of Envoy that is aware of a different protocol, it should be possible to configure it from EG.

fair enough, since alpn_protocols is also a string type in envoyproxy

+1, need to use a custom type to enforce kube builder validations
check out

gateway/api/v1alpha1/ratelimit_types.go

Line 136 in 52946be

// +kubebuilder:validation:Enum=Exact;RegularExpression;Distinct

But ALPN is more than just "h2" and "http/1.1". If I define this as an enum, it becomes impossible to use anything else. See @guydc's comment #2287 (comment) below.

do you have a specific use case where you'd like to set a specific ALPN apart from h2 and http/1.1 ?
its safer to use enums and validate synchronously imo

My point is that validation the values to just "h2" and "http/1.1" is limiting. If this is moved to enums, I think it would make sense to have all of the possible values in the IANA registry available as accepted enums.

EG is not there yet, but at some point it would make sense to have ClientTrafficPolicy documents attached also to TLS routes. When that happens, it would be possible to have other protocols supported. Examples would include SMTP, MQTT, or IMAP with EG terminating the TLS and routing to a service running in the cluster. If ALPN is limited to just the two possibilities - this won't be possible.

It honestly seems better to me to not set this as an enum. The default is set to [ "h2", "http/1.1" ], meaning that most people wouldn't touch this. But when more control over this is needed, it makes more sense to me to make it possible to control this setting.

Updated to be an enum, as agreed in the meeting.

codecov · 2023-12-10T14:55:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a06377d) 64.75% compared to head (6af1ad1) 64.74%.
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2287      +/-   ##
==========================================
- Coverage   64.75%   64.74%   -0.01%     
==========================================
  Files         113      113              
  Lines       16567    16567              
==========================================
- Hits        10728    10727       -1     
- Misses       5165     5168       +3     
+ Partials      674      672       -2

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

shawnh2

lgtm, thanks, defer to @envoyproxy/gateway-maintainers

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

zhaohuabing · 2023-12-13T06:09:43Z

api/v1alpha1/tls_types.go

+	// ALPNProtocols supplies the list of ALPN protocols that should be
+	// exposed by the listener. If left empty, ALPN will not be exposed.
+	// +optional
+	ALPNProtocols []string `json:"alpnProtocols,omitempty"`


Should the alpn_protocols be exposed to the API? Are there any particular use cases that need other ALPNs than the default "h2", "http/1.1 ?

In some cases we have customers that want to disable ALPN altogether, or disable negotiating HTTP/2 with ALPN.

Generally, there are cases where it makes sense to have a custom implemented protocol. For example, Istio uses a custom ALPN protocol to distinguish between types of traffic (e.g. istio/istio#40680 ).

so what the real case for? can you summarize it?

In some cases we have customers that want to disable ALPN altogether, or disable negotiating HTTP/2 with ALPN.

AFAIK, normally the server side wants to let the user side choose between HTTP 1.1 and HTTP2 for back compatibility.

Generally, there are cases where it makes sense to have a custom implemented protocol. For example, Istio uses a custom ALPN protocol to distinguish between types of traffic (e.g. istio/istio#40680 ).

The Istio use case is legitimate, but Istio just uses ALPN internally without exposing it as an API, and it's not a Gateway scenario.

I suggest removing this from the API if we don't have a concrete use case right now. We can always add it later, but it will be really hard to remove an existing feature from API.

Envoy disables ALPN by default. Envoy Gateway enables ALPN and sets it to [h2, http/1.1] by default. It's a reasonable behavior, but perhaps users should have the option to control this parameter.

In general, other protos are supported in the IANA registry. In rare cases, it could be necessary to support other protos, e.g. there was a period where curl was spec-compliant and used http/1.0 as ALPN for HTTP/1.0.

Other Gateways like Emissary-Ingress do support configuration of alpn settings. The Emissary docs imply that it could be necessary to explicitly set ALPN to [h2] for gRPC. I'm not sure if this generally mandatory or required for specific grpc clients. I assume that this was also the considered when ALPN was included in CTP. Maybe @AliceProxy can provide some context here as well.

Another reason to control ALPN is related to HTTP version preservation and compatibility.

Protocol downgrades can create certain vulnerabilities, e.g. as mentioned here. If a particular backend only supports HTTP/1, the default EG configuration will make protocol downgrades possible. HTTP2 => HTTP1 is very common, but some operators may prefer to use the same protocol version on both backend and proxy, which will require customizing ALPN.

When Envoy is used as a Gateway and it's proxying traffic to an HTTP/1 backend, the backend owners may prefer that clients use HTTP/1. Some industry examples: Akamai doesn't support HTTP/2 by default, Cloudflare support HTTP/2 Disablement, CloudFront support HTTP/2 Disablement.

defer to @envoyproxy/gateway-maintainers for more inputs.

zhaohuabing · 2023-12-13T06:15:52Z

api/v1alpha1/tls_types.go

+	// SignatureAlgorithms specifies which signature algorithms the listener should
+	// support.
+	// +optional
+	SignatureAlgorithms []string `json:"signatureAlgorithms,omitempty"`


Also, are there any use cases that need to configure CipherSuites,ECDHCurves, and SignatureAlgorithms? I'm not an expert on TLS, I'm just curious.

If TLS protocol versions below 1.2 are used, the default cipher suite is not going to work. See the documentation here.

The ability to control the supported curves and signature algorithms is needed for some scenarios where additional hardening is needed - the customer wants to specify exactly what should be used.

While most use-cases shouldn't need to touch the defaults, sometimes it's required.

defer to @envoyproxy/gateway-maintainers for more inputs.

(This is a rare use case, but it is a real use case.)

does this need to be an enum ?

This field is so much of a moving target that even the Envoy Proxy documentation doesn't specify what the up-to-date values are. I'm not sure it makes sense to have an enum here.

cool, lets keep it as is

arkodg · 2023-12-16T01:52:52Z

should these be defined under tls.options within the Gateway ?
https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.GatewayTLSConfig

zhaohuabing · 2023-12-16T01:58:41Z

should these be defined under tls.options within the Gateway ? https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.GatewayTLSConfig

Yes, if upstream them to Gateway API.

arkodg · 2023-12-16T02:09:05Z

@zhaohuabing referring to the options field which is a map, which is a tls specific extension provided by upstream

zhaohuabing · 2023-12-16T02:11:12Z

@zhaohuabing referring to the options field which is a map, which is a tls specific extension provided by upstream

Ha, I haven't noticed that :-)

arkodg · 2023-12-16T02:13:50Z

api/v1alpha1/tls_types.go

+	Version *TLSVersions `json:"version,omitempty"`
+
+	// CipherSuites specifies the set of cipher suites supported when
+	// negotiating TLS 1.0 - 1.2. This setting has no effect for TLS 1.3.


needs to be enforced with CEL validation

arkodg · 2023-12-16T02:14:05Z

api/v1alpha1/tls_types.go

+	// +optional
+	CipherSuites []string `json:"ciphers,omitempty"`
+
+	// ECDHCurves specifies the set of supported ECDH curves.


default needs to be specified

arkodg · 2023-12-16T02:15:44Z

api/v1alpha1/tls_types.go

+	SignatureAlgorithms []string `json:"signatureAlgorithms,omitempty"`
+
+	// ALPNProtocols supplies the list of ALPN protocols that should be
+	// exposed by the listener. If left empty, ALPN will not be exposed.


Suggested change

// exposed by the listener. If left empty, ALPN will not be exposed.

// exposed by the listener. By default http/1.1 and http/2 are enabled .

guydc · 2023-12-18T15:57:13Z

should these be defined under tls.options within the Gateway ? https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.GatewayTLSConfig

@arkodg That's a good point. The last activity related to Gateway TLS options took place in late 2021: kubernetes-sigs/gateway-api#886. I don't think that any common TLS options were introduced in Gateway-API and I'm not sure if any of the other Gateway-API implementations leverage this part of the spec.

More recently, the Policy Attachment GEP mentioned Listener TLS configuration as an example for policy use cases: https://gateway-api.sigs.k8s.io/geps/gep-713/#direct-policy-attachment_1. Backend TLS settings are being implemented with a Policy resource: https://gateway-api.sigs.k8s.io/geps/gep-1897/. The situation here is a bit different, since it's not likely that the K8s Service resource will be extended to support backend TLS settings. So, a Policy resource is necessary.

I feel that both alternatives (policy and options) are equally valid from the Gateway-API perspective. Policies are more flexible (e.g. supporting both Gateway and Listener attachment) and provide better API Lifecycle Management (kubebuilder validation, deprecation process, decoupling from restriction applied to k8s annotations, etc.). So, I prefer a Policy-based approach here. WDYT ?

arkodg · 2023-12-18T18:37:21Z

should these be defined under tls.options within the Gateway ? https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.GatewayTLSConfig

@arkodg That's a good point. The last activity related to Gateway TLS options took place in late 2021: kubernetes-sigs/gateway-api#886. I don't think that any common TLS options were introduced in Gateway-API and I'm not sure if any of the other Gateway-API implementations leverage this part of the spec.

More recently, the Policy Attachment GEP mentioned Listener TLS configuration as an example for policy use cases: https://gateway-api.sigs.k8s.io/geps/gep-713/#direct-policy-attachment_1. Backend TLS settings are being implemented with a Policy resource: https://gateway-api.sigs.k8s.io/geps/gep-1897/. The situation here is a bit different, since it's not likely that the K8s Service resource will be extended to support backend TLS settings. So, a Policy resource is necessary.

I feel that both alternatives (policy and options) are equally valid from the Gateway-API perspective. Policies are more flexible (e.g. supporting both Gateway and Listener attachment) and provide better API Lifecycle Management (kubebuilder validation, deprecation process, decoupling from restriction applied to k8s annotations, etc.). So, I prefer a Policy-based approach here. WDYT ?

jotting down some more pros and cons of each approach

Using the tls.options within Gateway resource
Pros

config lives close to the tls listener config
easier to debug since all tls config lives in one place
dont need to create and maintain another resource (ClientTrafficPolicy) for just this case

Cons

field and values are not strongly typed so can only be validated asynchronously and surfaced in the Gateway/Listener status
cannot add CEL validation

Creating a new field within ClientTrafficPolicy
Pros

Strongly typed config
CEL validations can be added to provide instantaneous feedback

Cons

tls config lives a little far away from the listener tls config, increasing cognitive load
debugging involves examining multiple resources

liorokman · 2023-12-18T20:01:23Z

jotting down some more pros and cons of each approach

Using the tls.options within Gateway resource Pros

config lives close to the tls listener config

easier to debug since all tls config lives in one place

dont need to create and maintain another resource (ClientTrafficPolicy) for just this case

Cons

field and values are not strongly typed so can only be validated asynchronously and surfaced in the Gateway/Listener status

cannot add CEL validation

Creating a new field within ClientTrafficPolicy Pros

Strongly typed config

CEL validations can be added to provide instantaneous feedback

Cons

tls config lives a little far away from the listener tls config, increasing cognitive load

debugging involves examining multiple resources

I think that the strongly typed config is significantly more important than having the configuration close to the listener.

Just the ciphers are complicated enough that having them in an untyped string is a bad idea. It's a small mini-language that needs to be specified in exactly the right way, and having to also condense it into a string makes it harder to use.

liorokman · 2023-12-19T09:23:56Z

jotting down some more pros and cons of each approach

More pros for the policy approach:

A policy can be applied to all listeners in a Gateway resource in one place, so there's less copy-n-paste required when a gateway has multiple listeners that need to be hardened.
Allows for a separation of concerns - the person that configures a Gateway is not necessarily the same person that is responsible for fine-tuning TLS parameters (in some cases this is done by a security expert operating from a central perspective).

kflynn

Sorry for commenting on the wrong PR! 🤦‍♂️ I've added some here, and yes, I'd like to request some changes here. Thanks!

kflynn · 2023-12-20T18:11:26Z

api/v1alpha1/tls_types.go

+	// Version details the minimum/maximum TLS protocol verison that
+	// should be supported by this listener.
+	// +optional
+	Version *TLSVersions `json:"version,omitempty"`


(I'd also vote to call this stanza versions plural rather than version singular.)

kflynn · 2023-12-20T18:12:59Z

api/v1alpha1/tls_types.go

+	TLSAuto TLSVersion = "TLS_Auto"
+	// TLSv1_0 specifies TLS version 1.0
+	TLSv10 TLSVersion = "TLSv1_0"
+	// TLSv1_1 specifies TLS version 1.1
+	TLSv11 TLSVersion = "TLSv1_1"
+	// TLSv1.2 specifies TLS version 1.2
+	TLSv12 TLSVersion = "TLSv1_2"
+	// TLSv1.3 specifies TLS version 1.3
+	TLSv13 TLSVersion = "TLSv1_3"


As noted on the wrong PR 🤦‍♂️, I'd rather these just be "v1.2" etc. Forcing the user to type "TLS_" all the type feels kind of pointless.

I removed the TLS prefixes from the enum values.

(I'd also vote to call this stanza versions plural rather than version singular.)

I think maybe it would be better to get rid of the intermediate version level to make it clearer.

That would make it:

tls: minTLSVersion: v1_2 maxTLSVersion: v1_3

I'd go with @arkodg's take here:

tls: minVersion: v1.2 maxVersion: v1.3

(I don't care terribly if it's v1.2 or v1_2, though I'd slightly prefer the .).

I removed the intermediate version structure.

kflynn · 2023-12-20T18:14:18Z

api/v1alpha1/tls_types.go

+	Min *TLSVersion `json:"min,omitempty"`
+
+	// Max specifies the maximal TLS version to use
+	// +optional
+	Max *TLSVersion `json:"max,omitempty"`


Also as noted on the wrong PR 🤦‍♂️, I think it's important to allow setting either of these without being forced to set the other.

OTOH if - as I recommend - the default minimum is v1.2, then setting the maximum to e.g. v1.1 without also setting the minimum lower should be an error.

This is currently the case. That's why both of these fields are optional. If you set one but not the other, then Envoy Proxy will use its default, (which is TLS_AUTO) and is documented as (DEFAULT) ⁣Envoy will choose the optimal TLS version. This works correctly regardless of which of the fields was set or unset.

https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/common.proto#envoy-v3-api-enum-extensions-transport-sockets-tls-v3-tlsparameters-tlsprotocol

The implementation itself verifies that the minimum version is not greater than the maximum version. Since these are string constants, it's not easy to concisely validate correct ordering with CEL.

In the other PR, it would actually throw an error if you set min and left max at AUTO, since AUTO is defined to have the value -1. 🙂 That's what prompted me to look at this.

I agree that expressing this in CEL is tricky. 🙁 I'm not actually sure it's possible...

Once the API is agreed upon, I'll make sure to add tests (as part of the other PR) to verify that the verification works correctly.

Added CEL validation to verify that tls.minVersion <= tls.maxVersion

arkodg · 2023-12-20T19:22:59Z

removing the decision label, this was discussed in the community meeting yesterday, and @zhaohuabing @zirain @guydc and I agreed on introducing this API in ClientTrafficPolicy instead of reusing the tls.options field within Gateway for the below reasons

stronger validation of fields
apply default settings to all listeners

This API can be deprecated once these fields are natively introduced upstream

arkodg · 2023-12-20T19:31:37Z

api/v1alpha1/tls_types.go

+	// - ECDHE-RSA-AES256-GCM-SHA384
+	//
+	// +optional
+	CipherSuites []string `json:"ciphers,omitempty"`


should this be made into an enum for better validation ?

It's not possible to make it into an enum, because the values in here are not always singular values.

See the cipher list documentation from Envoy Proxy's underlying library.

The values in this array are expressions, allowing things like [ECDHE-ECDSA-AES128-GCM-SHA256|ECDHE-ECDSA-CHACHA20-POLY1305], which is an expression that says "either the first cipher or the second".

Additional possible values are to use ! or '+' or '-' as the prefix for a cipher name. This cannot be expressed as an enum in Envoy Gateway.

cool, lets keep it as is

arkodg · 2023-12-20T19:31:49Z

api/v1alpha1/tls_types.go

+	// In builds using BoringSSL FIPS the default curve is:
+	// - P-256
+	// +optional
+	ECDHCurves []string `json:"ecdhCurves,omitempty"`


should this be made into an enum for better validation ?

The set of valid values are not clear.

Envoy Proxy doesn't define them - the underlying BoringSSL library does. If Envoy Proxy is compiled against a different version of BoringSSL, there may be different valid values than the ones the EnvoyGateway code base uses.

I agree with @liorokman that curves & ciphers should not be enums – there's not going to be a great way to validate, especially in BYOEnvoy cases. 🙁 This is a case where we should be thinking carefully about the behavior when there's an error, and documenting it. (Personally I'd like to see a status on the Gateway itself, recognizing that that's somewhat fraught with peril...)

cool lets keep as is

arkodg · 2023-12-21T00:53:51Z

api/v1alpha1/tls_types.go

+	// - ECDHE-RSA-AES256-GCM-SHA384
+	//
+	// +optional
+	CipherSuites []string `json:"ciphers,omitempty"`


go field name and json field name dont match - cipherSuites and ciphers

arkodg

LGTM, appreciate addressing all the review comments and enhancing the doc strings

arkodg · 2023-12-22T22:30:27Z

@liorokman seeing some merge conflicts, can you rebase ?

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

Documented the defaults used for fields. Signed-off-by: Lior Okman <lior.okman@sap.com>

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

protocol version. Defined the default minimal TLS version to be 1.2 and the default maximal TLS version to 1.3. Signed-off-by: Lior Okman <lior.okman@sap.com>

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

to tls.maxVersion Signed-off-by: Lior Okman <lior.okman@sap.com>

was specified. Fixed a CEL issue where setting tls.maxVersion without setting tls.minVersion was not correctly validated Signed-off-by: Lior Okman <lior.okman@sap.com>

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

Require that if HTTP/3 is enabled then ALPN protocols are not specified, since HTTP/3 sets a custom "h3" protocol in ALPN. Signed-off-by: Lior Okman <lior.okman@sap.com>

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

…2297) * api: Add TLS configuration attributes in ClientTrafficPolicy Signed-off-by: Lior Okman <lior.okman@sap.com> * Fixed typos in the comments. Signed-off-by: Lior Okman <lior.okman@sap.com> * Updated the comment for `TLSSettings`. Signed-off-by: Lior Okman <lior.okman@sap.com> * Regenerated after rebasing Signed-off-by: Lior Okman <lior.okman@sap.com> * api: Add TLS configuration attributes in ClientTrafficPolicy Signed-off-by: Lior Okman <lior.okman@sap.com> * feat: Implement setting common TLS parameters in ClientTrafficPolicy. Depends on #2287. Signed-off-by: Lior Okman <lior.okman@sap.com> * Fixed linter and gen-check errors Signed-off-by: Lior Okman <lior.okman@sap.com> * Implement the changes required after the reviewed API Signed-off-by: Lior Okman <lior.okman@sap.com> * Set ALPN protocols in QUIC in the same way as in HTTPS Signed-off-by: Lior Okman <lior.okman@sap.com> * * Regenerated and recreated the manifests after rebase * Make the new tests pass Signed-off-by: Lior Okman <lior.okman@sap.com> * Alphabetize the output test yamls to make "gen-check" pass Signed-off-by: Lior Okman <lior.okman@sap.com> * * Remove references to the API from the XDS layer * 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> * Changed the TLS protocol version constants to have a dot instead of an underscore separator. Signed-off-by: Lior Okman <lior.okman@sap.com> * * Reworked the ALPN logic so that an empty array in the IR layer 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> * Make gen-check pass Signed-off-by: Lior Okman <lior.okman@sap.com> * * Require that ALPN constants are also valid ALPN identification strings. * Replace the user-facing "http/2" ALPN string with "h2". Signed-off-by: Lior Okman <lior.okman@sap.com> --------- Signed-off-by: Lior Okman <lior.okman@sap.com>

liorokman requested a review from a team as a code owner December 10, 2023 09:58

github-actions bot temporarily deployed to pull request December 10, 2023 10:02 Inactive

shawnh2 reviewed Dec 10, 2023

View reviewed changes

github-actions bot temporarily deployed to pull request December 10, 2023 15:52 Inactive

github-actions bot temporarily deployed to pull request December 11, 2023 13:32 Inactive

github-actions bot temporarily deployed to pull request December 12, 2023 07:06 Inactive

shawnh2 approved these changes Dec 12, 2023

View reviewed changes

liorokman added a commit to liorokman/gateway that referenced this pull request Dec 12, 2023

feat: Implement setting common TLS parameters in ClientTrafficPolicy.

73b36c6

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

liorokman mentioned this pull request Dec 12, 2023

feat: Implement setting common TLS parameters in ClientTrafficPolicy #2297

Merged

zhaohuabing reviewed Dec 13, 2023

View reviewed changes

arkodg reviewed Dec 16, 2023

View reviewed changes

github-actions bot temporarily deployed to pull request December 16, 2023 08:38 Inactive

arkodg added the kind/decision A record of a decision made by the community. label Dec 20, 2023

kflynn suggested changes Dec 20, 2023

View reviewed changes

arkodg removed the kind/decision A record of a decision made by the community. label Dec 20, 2023

arkodg reviewed Dec 20, 2023

View reviewed changes

arkodg reviewed Dec 21, 2023

View reviewed changes

arkodg previously approved these changes Dec 22, 2023

View reviewed changes

arkodg requested a review from kflynn December 22, 2023 22:30

liorokman added 12 commits December 23, 2023 10:26

api: Add TLS configuration attributes in ClientTrafficPolicy

4a4a8f8

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

Fixed typos in the comments.

b9e0e97

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

Added missing TLS 1.1 enum value for TLS Versions

b969b71

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

Regenerated the ClientTrafficPolicy CRD to include the TLS 1.1 enum.

c85c394

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

Added CEL validation for ciphers if minimum TLS version is 1.3.

3f9a22d

Documented the defaults used for fields. Signed-off-by: Lior Okman <lior.okman@sap.com>

Modified ALPN to be an enum array.

323a0d4

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

Removed the 'TLS_' prefix from the TLS protocol version enum values.

2ba678a

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

Updated the comment for TLSSettings.

647b29d

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

Removed the intermediate "version" level when specifying the TLS

74f2b21

protocol version. Defined the default minimal TLS version to be 1.2 and the default maximal TLS version to 1.3. Signed-off-by: Lior Okman <lior.okman@sap.com>

Aligned the Golang and JSON name for ciphers.

5d885c7

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

Added CEL validation to verify that tls.minVersion is less than or equal

ff622b4

to tls.maxVersion Signed-off-by: Lior Okman <lior.okman@sap.com>

Fixed a CEL issue where tls.minVersion was checked before verifying it

a959866

was specified. Fixed a CEL issue where setting tls.maxVersion without setting tls.minVersion was not correctly validated Signed-off-by: Lior Okman <lior.okman@sap.com>

liorokman dismissed arkodg’s stale review via a959866 December 23, 2023 08:31

liorokman force-pushed the clienttrafficpolicy-tls branch from 918a2d2 to a959866 Compare December 23, 2023 08:31

liorokman added 2 commits December 23, 2023 10:32

Regenerated after rebasing

7f34bfa

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

Added CEL validation tests.

6af1ad1

Require that if HTTP/3 is enabled then ALPN protocols are not specified, since HTTP/3 sets a custom "h3" protocol in ALPN. Signed-off-by: Lior Okman <lior.okman@sap.com>

liorokman added a commit to liorokman/gateway that referenced this pull request Dec 23, 2023

feat: Implement setting common TLS parameters in ClientTrafficPolicy.

48148e2

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

arkodg approved these changes Dec 26, 2023

View reviewed changes

zirain approved these changes Dec 27, 2023

View reviewed changes

zirain merged commit 3e220ef into envoyproxy:main Dec 27, 2023
18 checks passed

liorokman added a commit to liorokman/gateway that referenced this pull request Jan 3, 2024

feat: Implement setting common TLS parameters in ClientTrafficPolicy.

6423e25

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

liorokman added a commit to liorokman/gateway that referenced this pull request Jan 5, 2024

feat: Implement setting common TLS parameters in ClientTrafficPolicy.

586930f

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

liorokman mentioned this pull request Jan 9, 2024

fix: Add a CEL validation unit test to verify that no ciphers can be specified if the minimal TLS version is 1.3 #2424

Merged

	// exposed by the listener. If left empty, ALPN will not be exposed.
	// exposed by the listener. By default http/1.1 and http/2 are enabled .

api: Add TLS configuration attributes in ClientTrafficPolicy #2287

api: Add TLS configuration attributes in ClientTrafficPolicy #2287

Conversation

liorokman commented Dec 10, 2023

github-actions bot commented Dec 10, 2023

github-actions bot commented Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 10, 2023 • edited Loading

Codecov Report

shawnh2 left a comment

Choose a reason for hiding this comment

zhaohuabing Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaohuabing Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

guydc Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

zhaohuabing Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

zhaohuabing Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkodg commented Dec 16, 2023

zhaohuabing commented Dec 16, 2023

arkodg commented Dec 16, 2023

zhaohuabing commented Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guydc commented Dec 18, 2023

arkodg commented Dec 18, 2023

liorokman commented Dec 18, 2023

liorokman commented Dec 19, 2023

kflynn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liorokman Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liorokman Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 10, 2023 •

edited

Loading

arkodg Dec 16, 2023 •

edited

Loading

arkodg Dec 16, 2023 •

edited

Loading

codecov bot commented Dec 10, 2023 •

edited

Loading

zhaohuabing Dec 13, 2023 •

edited

Loading

zhaohuabing Dec 13, 2023 •

edited

Loading

guydc Dec 13, 2023 •

edited

Loading

zhaohuabing Dec 14, 2023 •

edited

Loading

zhaohuabing Dec 13, 2023 •

edited

Loading

zhaohuabing commented Dec 16, 2023 •

edited

Loading

liorokman Dec 20, 2023 •

edited

Loading

liorokman Dec 21, 2023 •

edited

Loading

kflynn Dec 20, 2023 •

edited

Loading