feat: support backend tls settings with envoyproxy #3218

alexwo · 2024-04-18T19:25:25Z

What this PR does / why we need it:
This change allows to enhance the security and adaptability of connections to backend services. By allowing administrators to specify TLS settings, such as which cryptographic ciphers and TLS protocol versions to use, it allows to better align their security posture with industry best practices and regulatory requirements.

Approach:

Enable specification of default values through a TLS settings section in the EnvoyProxy backend.
Use these settings as the default TLS configurations whenever a BackendTLS policy is specified for a backend, applying the EnvoyProxy defaults to affected backends.

Once the BackendTLS policy or an alternative method for providing these settings is implemented/accepted in upstream, opt for more specific settings than those provided by envoyproxy backendtls settings.

Which issue(s) this PR fixes:
#2901

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

guydc · 2024-04-18T22:37:57Z

cc @zhaohuabing who was also looking into backend MTLS in #2984

guydc

Thanks for this!
Some comments on reuse of existing types.

guydc · 2024-04-18T22:52:34Z

api/v1alpha1/envoyproxy_types.go

+	ClientCertificateRef *gwapiv1.SecretObjectReference `json:"clientCertificateRef,omitempty"`
+	// Ciphers defines only support the specified cipher list when negotiating BackendTLS 1.0-1.2 (this setting has no effect when negotiating BackendTLS 1.3).
+	// +optional
+	Ciphers []string `json:"ciphers,omitempty"`


Maybe we reuse some of the types in tls_types.go that we use in client traffic policy, like TLSVersion? Maybe we can just embed the top-level TLSSettings struct here?

We can, it currently contain also ClientValidation and ALPNProtocol, this is less interesting in this context.
Would it make sense to re-structure TLSSettings and include the commons here?

we can create a struct that represents the TLSParams and embed that struct in both place and handle the ClientCertificateRef and ClientValidation fields seperately

guydc · 2024-04-18T22:58:27Z

internal/gatewayapi/backendtlspolicy.go

@@ -78,7 +78,26 @@ func (t *Translator) processBackendTLSPolicy(
 	}

 	status.SetAcceptedForPolicyAncestors(&policy.Status, ancestorRefs, t.GatewayControllerName)
-
+	// apply defaults as per envoyproxy
+	if resources.EnvoyProxy != nil {


Should we apply this logic outside of backendtlspolicy translation, since it's not directly originating from that policy? It's probably most efficient and user-friendly to do it here. But, in the future TLS settings may originate from other resources, as described in kubernetes-sigs/gateway-api#2910

Right, it can make sense but maybe we can do it after support for additional resources is implemented in upstream. I guess we can move / extend this logic to other areas as well at any time.

Do you see an advantage in making this change now? Maybe If we want to support a use case of applying this on a gateway level already.

wdyt?

guydc · 2024-04-18T23:00:29Z

internal/xds/translator/utils.go

@@ -161,3 +161,17 @@ func createExtServiceXDSCluster(rd *ir.RouteDestination, tCtx *types.ResourceVer
 	}
 	return nil
 }
+
+func ParseTLSProtocol(tlsVersion ir.TLSVersion) (tlsv3.TlsParameters_TlsProtocol, error) {


can we reuse the util that we have in CTP for this mapping?

yes, i think we should be able i will check it.

guydc · 2024-04-18T23:03:32Z

internal/gatewayapi/testdata/envoyproxy-tls-settings.in.yaml

@@ -0,0 +1,133 @@
+gateways:


Can we also add MTLS + TLS Settings tests for ext-auth and ext-proc, demonstrating how this applied to these sorts of backends?

arkodg · 2024-04-19T02:36:58Z

thanks for working on this @alexwo !
this PR is doing two things

added support for specifying clientCert which doesnt exist in upstream today
added support for specifying tls params such as cipher_suites

both 1. and 2. are added into the global EnvoyProxy API
should 2. live inside BTP, so it can be modified on a per route (per upstream) basis ?

alexwo · 2024-04-19T05:54:25Z

thanks for working on this @alexwo ! this PR is doing two things

added support for specifying clientCert which doesnt exist in upstream today

added support for specifying tls params such as cipher_suites

both 1. and 2. are added into the global EnvoyProxy API should 2. live inside BTP, so it can be modified on a per route (per upstream) basis ?

Hi @arkodg ,

It can allow for better flexibility, perhaps it can make more sense for (2) settings, @guydc WDYT?

thanks for working on this @alexwo ! this PR is doing two things

added support for specifying clientCert which doesnt exist in upstream today

added support for specifying tls params such as cipher_suites

both 1. and 2. are added into the global EnvoyProxy API should 2. live inside BTP, so it can be modified on a per route (per upstream) basis ?

guydc · 2024-04-19T11:35:22Z

It can allow for better flexibility, perhaps it can make more sense for (2) settings, @guydc WDYT?

The major downside I see in using BTP is that it can only attach to xRoute backends. So, if we need to apply TLS settings for other backends (extproc, extauth, OTEL, ALS ... ), we will need to add TLS setting in the scope of of #3069.

I'm fine with both options, as long as we can support TLS settings for both xRoute and non-xRoute backends. @arkodg - WDYT?

arkodg · 2024-04-19T20:31:51Z

It can allow for better flexibility, perhaps it can make more sense for (2) settings, @guydc WDYT?

The major downside I see in using BTP is that it can only attach to xRoute backends. So, if we need to apply TLS settings for other backends (extproc, extauth, OTEL, ALS ... ), we will need to add TLS setting in the scope of of #3069.

I'm fine with both options, as long as we can support TLS settings for both xRoute and non-xRoute backends. @arkodg - WDYT?

yeah for non xRoute we'll need to embed it into our backendRef

by moving 2. into BTP, we add the ability to fine tune settings, but now tls settings are spread out in 3 places, which can get confusing

clientCertificateRef in EnvoyProxy.Spec.BackendTLS
tlsParams in BackendTrafficPolicy.Spec.TLS
caCertificateRefs in BackendTLSPolicy.Spec.Validation

alexwo · 2024-04-21T15:29:13Z

yeah for non xRoute we'll need to embed it into our backendRef

by moving 2. into BTP, we add the ability to fine tune settings, but now tls settings are spread out in 3 places, which can get confusing

clientCertificateRef in EnvoyProxy.Spec.BackendTLS

tlsParams in BackendTrafficPolicy.Spec.TLS

caCertificateRefs in BackendTLSPolicy.Spec.Validation

If we go with the BTP direction, does it make sense to move clientCertificateRef, to BTP as well?
This way we can have things less spread.

guydc · 2024-04-22T13:32:50Z

If we go with the BTP direction, does it make sense to move clientCertificateRef, to BTP as well? This way we can have things less spread.

I think so. It's possible that different backends will require a different client cert. It's not uncommon for organizations to use private PKI(s) for client certs, and so backends may have different trust stores and CAs. If we go with EnvoyProxy-level config, we basically assume that all backends can be configured to trust the EG client certificate.

arkodg · 2024-04-22T19:55:43Z

@guydc @alexwo as platform administrators, would you say that the feature is necessary, because you are trying to enforce specific tls parameters (ciphers, tls versions). If this is true, then the argument of putting it in EnvoyProxy.Spec.BackendTLS can be made for 2. since for this use case, we'd want the platform admin to enforce these settings w/o allowing the app dev (route level BTP owner) to override it.

guydc · 2024-04-22T21:23:17Z

as platform administrators, would you say that the feature is necessary, because you are trying to enforce specific tls parameters (ciphers, tls versions)

Yes, our use case is related to defining system-wide upstream TLS settings, and so EnvoyProxy makes a lot of sense to me.

arkodg · 2024-04-23T01:03:12Z

as platform administrators, would you say that the feature is necessary, because you are trying to enforce specific tls parameters (ciphers, tls versions)

Yes, our use case is related to defining system-wide upstream TLS settings, and so EnvoyProxy makes a lot of sense to me.

cool, i'm a +1 for this as well

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo · 2024-04-24T10:54:37Z

/retest

api/v1alpha1/envoyproxy_types.go

guydc

LGTM, Thanks!

arkodg · 2024-04-24T16:17:25Z

internal/xds/translator/translator.go

@@ -768,10 +768,29 @@ func buildXdsUpstreamTLSCASecret(tlsConfig *ir.TLSUpstreamConfig) *tlsv3.Secret
 func buildXdsUpstreamTLSSocketWthCert(tlsConfig *ir.TLSUpstreamConfig) (*corev3.TransportSocket, error) {

 	var tlsCtx *tlsv3.UpstreamTlsContext
+	var tlsParams *tlsv3.TlsParameters


can we reuse

gateway/internal/xds/translator/listener.go

Line 572 in 27a770f

func buildTLSParams(tlsConfig *ir.TLSConfig) *tlsv3.TlsParameters {

?

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo · 2024-04-25T05:07:33Z

/retest

alexwo · 2024-04-25T05:56:14Z

/retest

alexwo · 2024-04-25T06:20:29Z

/retest

guydc · 2024-04-25T11:52:51Z

/retest

alexwo · 2024-04-25T16:30:06Z

/retest

alexwo · 2024-04-25T17:01:08Z

/retest

arkodg

LGTM thanks !

alexwo added 5 commits April 18, 2024 21:17

adding backend tls settings support via envoyproxy

9868422

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

updating

7ecca6f

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

update docs

ccdae56

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

fix generates

5ce2994

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

updating

09f2f0e

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo changed the title ~~feat: support backend tls settings in envoyproxy~~ feat: support backend tls settings via envoyproxy Apr 18, 2024

alexwo changed the title ~~feat: support backend tls settings via envoyproxy~~ feat: support backend tls settings with envoyproxy Apr 18, 2024

updating

01b3bcc

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo marked this pull request as ready for review April 18, 2024 20:15

alexwo requested a review from a team as a code owner April 18, 2024 20:15

alexwo added 2 commits April 18, 2024 23:49

adding translation

77fab65

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

updating

69c25b8

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

guydc reviewed Apr 18, 2024

View reviewed changes

guydc mentioned this pull request Apr 19, 2024

API for client cert #2984

Closed

alexwo added 5 commits April 24, 2024 10:58

merge with main

93e8382

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

code review changes

8022085

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

updating

c6ed523

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

regenerate

b9d6c8f

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

fix generate

7755982

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

guydc reviewed Apr 24, 2024

View reviewed changes

api/v1alpha1/envoyproxy_types.go Show resolved Hide resolved

Merge branch 'main' into backend_tls

27a770f

guydc previously approved these changes Apr 24, 2024

View reviewed changes

arkodg reviewed Apr 24, 2024

View reviewed changes

alexwo added 2 commits April 25, 2024 00:26

reuse tls builder

6d98016

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

Merge branch 'backend_tls' of https://github.com/alexwo/gateway

c17d15b

Signed-off-by: Alexander Volchok <alex.volchok@sap.com>

alexwo dismissed guydc’s stale review via c17d15b April 24, 2024 22:26

Merge branch 'main' into backend_tls

12f7063

Merge branch 'main' into backend_tls

949a5d7

Merge branch 'main' into backend_tls

5ed2666

guydc approved these changes Apr 25, 2024

View reviewed changes

arkodg approved these changes Apr 25, 2024

View reviewed changes

arkodg merged commit a81291e into envoyproxy:main Apr 25, 2024
23 checks passed

alexwo deleted the backend_tls branch April 26, 2024 05:56

guydc mentioned this pull request May 1, 2024

Backend TLS Settings #2901

Closed

arkodg mentioned this pull request May 8, 2024

Envoy Client Certs for Ext Auth and Backend TLS #2536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support backend tls settings with envoyproxy #3218

feat: support backend tls settings with envoyproxy #3218

alexwo commented Apr 18, 2024 •

edited

Loading

guydc commented Apr 18, 2024

guydc left a comment

guydc Apr 18, 2024

alexwo Apr 19, 2024 •

edited

Loading

arkodg Apr 22, 2024

guydc Apr 18, 2024

alexwo Apr 19, 2024 •

edited

Loading

guydc Apr 18, 2024

alexwo Apr 19, 2024

guydc Apr 18, 2024

arkodg commented Apr 19, 2024

alexwo commented Apr 19, 2024 •

edited

Loading

guydc commented Apr 19, 2024

arkodg commented Apr 19, 2024

alexwo commented Apr 21, 2024 •

edited

Loading

guydc commented Apr 22, 2024

arkodg commented Apr 22, 2024

guydc commented Apr 22, 2024

arkodg commented Apr 23, 2024

alexwo commented Apr 24, 2024

guydc left a comment

arkodg Apr 24, 2024

alexwo commented Apr 25, 2024

alexwo commented Apr 25, 2024

alexwo commented Apr 25, 2024

guydc commented Apr 25, 2024

alexwo commented Apr 25, 2024

alexwo commented Apr 25, 2024

arkodg left a comment

feat: support backend tls settings with envoyproxy #3218

feat: support backend tls settings with envoyproxy #3218

Conversation

alexwo commented Apr 18, 2024 • edited Loading

guydc commented Apr 18, 2024

guydc left a comment

Choose a reason for hiding this comment

guydc Apr 18, 2024

Choose a reason for hiding this comment

alexwo Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

arkodg Apr 22, 2024

Choose a reason for hiding this comment

guydc Apr 18, 2024

Choose a reason for hiding this comment

alexwo Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

guydc Apr 18, 2024

Choose a reason for hiding this comment

alexwo Apr 19, 2024

Choose a reason for hiding this comment

guydc Apr 18, 2024

Choose a reason for hiding this comment

arkodg commented Apr 19, 2024

alexwo commented Apr 19, 2024 • edited Loading

guydc commented Apr 19, 2024

arkodg commented Apr 19, 2024

alexwo commented Apr 21, 2024 • edited Loading

guydc commented Apr 22, 2024

arkodg commented Apr 22, 2024

guydc commented Apr 22, 2024

arkodg commented Apr 23, 2024

alexwo commented Apr 24, 2024

guydc left a comment

Choose a reason for hiding this comment

arkodg Apr 24, 2024

Choose a reason for hiding this comment

alexwo commented Apr 25, 2024

alexwo commented Apr 25, 2024

alexwo commented Apr 25, 2024

guydc commented Apr 25, 2024

alexwo commented Apr 25, 2024

alexwo commented Apr 25, 2024

arkodg left a comment

Choose a reason for hiding this comment

alexwo commented Apr 18, 2024 •

edited

Loading

alexwo Apr 19, 2024 •

edited

Loading

alexwo Apr 19, 2024 •

edited

Loading

alexwo commented Apr 19, 2024 •

edited

Loading

alexwo commented Apr 21, 2024 •

edited

Loading