allow configuration of upstream TLS connection settings #5828

KauzClay · 2023-10-11T21:16:13Z

Just as you can configure TLS settings for downstream connections, like setting min/max TLS versions and cipher suites, you should be able to do that for upstream connections too.

For upstream connections, I would like the default Max TLS version to be 1.3, instead of the current Envoy default of 1.2

release-note/minor

Fixes #5501
Fixes #3574

tsaarni · 2023-10-12T03:54:20Z

Hi @KauzClay 👋 Linking some previous discussions related to upstream TLS just for reference info: #5501, #5666

KauzClay · 2023-10-12T22:02:44Z

@sunjayBhatia , it is still pretty rough, but I've attempted to wire everything up for this new configuartion.

One question though: how do I enforce defaults for min and max TLS version in CRD and the ConfigMap? I would like the Contour default for max version to be different than Envoy's default.

EDIT: I ended up just sticking the default max in the code where the creation of UpstreamTLSContext happens

KauzClay · 2023-10-13T17:05:38Z

/test test-linux

codecov · 2023-10-13T20:54:10Z

Codecov Report

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

Comparison is base (eece587) 78.69% compared to head (c7dddae) 78.70%.
Report is 15 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5828   +/-   ##
=======================================
  Coverage   78.69%   78.70%           
=======================================
  Files         138      138           
  Lines       19687    19717   +30     
=======================================
+ Hits        15493    15518   +25     
- Misses       3890     3895    +5     
  Partials      304      304

Files	Coverage Δ
cmd/contour/servecontext.go	`83.84% <100.00%> (+0.18%)`	⬆️
internal/contourconfig/contourconfiguration.go	`98.18% <100.00%> (+0.05%)`	⬆️
internal/dag/dag.go	`98.70% <ø> (ø)`
internal/dag/extension_processor.go	`95.90% <100.00%> (+0.03%)`	⬆️
internal/dag/httpproxy_processor.go	`91.92% <100.00%> (+0.01%)`	⬆️
internal/dag/ingress_processor.go	`97.60% <100.00%> (+<0.01%)`	⬆️
internal/envoy/v3/auth.go	`100.00% <100.00%> (ø)`
internal/envoy/v3/cluster.go	`96.36% <100.00%> (+0.04%)`	⬆️
internal/featuretests/v3/envoy.go	`99.08% <100.00%> (+<0.01%)`	⬆️
pkg/config/parameters.go	`86.39% <100.00%> (+0.32%)`	⬆️
... and 1 more

... and 2 files with indirect coverage changes

tsaarni

Added some comments inline, but looks good to me in general!

I think it would be good to have some reasoning here about the corner cases that have led Envoy not to enable TLSv1.3 for upstream, and why it is OK for Contour to do so - something that answers to #5666 (review)

One more thought: It would be nice to have e2e test for the protocol version. The echoserver used for e2e suite returns a JSON response that includes the negotiated TLS version (see here). We could check that TLSv1.3 gets returned. Check test/e2e/httpproxy/backend_tls_test.go on similar assertion checks done for the response. In responseTLSDetails the client certificate was parsed from response document and compared to what we had configured.

cmd/contour/serve.go

pkg/config/parameters.go

internal/envoy/v3/cluster.go

changelogs/unreleased/5828-KauzClay-minor.md

github-actions · 2023-11-07T00:19:17Z

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 14d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

Mark this PR as fresh by commenting or pushing a commit
Close this PR
Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

test/e2e/httpproxy/httpproxy_test.go

KauzClay · 2023-11-17T15:45:34Z

@sunjayBhatia @tsaarni looks like the tests are passing now, thanks for your feedback so far.

I tried linking the upstream tls config into extension services and dnsservices as well, but I'm not so sure I got it correct. I'd appreciate if you all took a closer look in that area next time you look through this 🙇

tsaarni · 2023-11-22T09:33:04Z

internal/dag/httpproxy_processor.go

@@ -1026,6 +1030,7 @@ func (p *HTTPProxyProcessor) computeRoutes(
 				SlowStartConfig:               slowStart,
 				MaxRequestsPerConnection:      p.MaxRequestsPerConnection,
 				PerConnectionBufferLimitBytes: p.PerConnectionBufferLimitBytes,
+				UpstreamTLS:                   determineUpstreamTLS(p.UpstreamTLS),


Just an observation about small inconsistency:

It seems we have previously in some cases avoided setting TLS parameters if TLS is not used by upstream in the first place: Cluster.UpstreamValidation is set to nil. We also have set TLS parameters unconditionally: Cluster.SNI will be set even if TLS is not used. Now Cluster.UpstreamTLS will also be set even if cluster.Protocol is not implying TLS.

Just setting the values unconditionally can be seen as simplification of the logic here. In reality the TLS parameters are ignored later in internal/envoy/v3/cluster.go when constructing envoy.Cluster when Cluster.protocol does not indicate TLS. So at the end, it does not matter if Cluster is filled in with TLS settings, even when TLS is not used.

internal/envoy/v3/cluster.go

tsaarni · 2023-11-22T13:53:43Z

pkg/config/parameters.go

@@ -429,6 +429,9 @@ type ClusterParameters struct {
 	//
 	// +optional
 	PerConnectionBufferLimitBytes *uint32 `yaml:"per-connection-buffer-limit-bytes,omitempty"`
+
+	// UpstreamTLS contains the TLS policy parameters for upstream connections
+	UpstreamTLS TLSParameters `yaml:"upstream-tls,omitempty"`


Should we avoid reusing the TLSParameters struct here since it contains some parameters that can be meaningful only for downstream or upstream TSL?

For example field TLSParameters.FallbackCertificate is used as server certificate, which appears as configuration file field tls.fallback-certificate. When reusing the struct for upstream, it could be also defined as cluster.upstream-tls.fallback-certificate according purely to syntax, but if defined there, it would be ignored.

Similarly, config file field tls.envoy-client-certificate is used as optional client cert in upstream TLS. Reusing the struct would mean field cluster.upstream-tls.envoy-client-certificate is syntactically correct, but actually it will not get used.

hmmm I see. I went with TLSParameters originally because it had what I needed and I didn't want to create a similar-but-not-quite-the-same struct.

What would you say to creating a new struct that just contains {MinimumProtocolVersion,MaximumProtocolVersion,CipherSuites}, and then making that struct a part of TLSParameters?

Then cluster.upstream-tls would only contain options that can actually be used.

👍 that sounds good to me

Okay, I ended up creating a new smaller struct called ProtocolParameters. I can always change it if you think a different name is better

tsaarni · 2023-11-22T14:20:38Z

apis/projectcontour/v1alpha1/contourconfig.go

+	// UpstreamTLS contains the TLS policy parameters for upstream connections
+	//
+	// +optional
+	UpstreamTLS *EnvoyTLS `json:"upstream-tls,omitempty"`


Question: What do people prefer: nesting TLS config in ContourConfiguration.spec.envoy.cluster.upstream-tls (like suggested here) or bumping it one level ContourConfiguration.spec.envoy.upstream-tls?

Maybe bit unfortunate but there is existing upstream TLS parameter directly under ContourConfiguration.spec.envoy, which is clientCertificate. It's bit unfortunate that the upstream TLS settings will be split in two places, but hopefully not too far from each other.

The corresponding downstream TLS setting is ContourConfiguration.spec.envoy.listener.tls. I guess perfect symmetry across TLS settings is not achievable anymore, not even in CRD config which was the fresh start :)

Given dowstream TLS settings are nested under envoy.listener, I think putting upstream settings under envoy.cluster is logical.

Re: clientCertificate -- the CRD is still v1alpha1, so we could make a breaking change if we wanted to reorganize. Seems like moving clientCertificate into UpstreamTLS might make sense?

I could move clientCertificate into UpstreamTLS. OR would you prefer that be in a separate MR?

Let's hold off for now to see what others think about that change, we can do it in a separate PR if we decide to proceed.

+1 for keeping it under cluster

apis/projectcontour/v1alpha1/contourconfig.go

skriss · 2023-12-06T20:48:24Z

apis/projectcontour/v1alpha1/contourconfig.go

+	// UpstreamTLS contains the TLS policy parameters for upstream connections
+	//
+	// +optional
+	UpstreamTLS *EnvoyTLS `json:"upstream-tls,omitempty"`


Given dowstream TLS settings are nested under envoy.listener, I think putting upstream settings under envoy.cluster is logical.

Re: clientCertificate -- the CRD is still v1alpha1, so we could make a breaking change if we wanted to reorganize. Seems like moving clientCertificate into UpstreamTLS might make sense?

Signed-off-by: Clay Kauzlaric <[email protected]>

* change json key style * re-run make generate Co-authored-by: Steve Kriss <[email protected]> Signed-off-by: Clay Kauzlaric <[email protected]>

Signed-off-by: Clay Kauzlaric <[email protected]>

sunjayBhatia · 2023-12-12T22:40:24Z

test/e2e/httpproxy/httpproxy_test.go

+				Namespace: namespace,
+			}
+		})
+		Context("with backend tls version configured via Contour ConfigMap", func() {


I think we can combine these two contexts (configmap and contourconfig-specific wrappers) and just set the same options in the BeforeEach just as we do here:

contour/test/e2e/httpproxy/httpproxy_test.go

Lines 115 to 118 in 4d48db7

BeforeEach(func() {

contourConfig.EnableExternalNameService = true

contourConfiguration.Spec.EnableExternalNameService = ref.To(true)

})

we have different test runs for using each type of config so we should only need one instance of the test

okay I combined them into 1 before each. Does it look how you're imagining now?

sunjayBhatia

Functionality looks good and change overall looks good modulo whatever existing nits there are and questions about API types

Once those are resolved I'm good to go with this change 👍🏽

changelogs/unreleased/5828-KauzClay-minor.md

internal/dag/extension_processor.go

internal/dag/httpproxy_processor.go

skriss · 2023-12-13T17:47:02Z

pkg/config/parameters.go

@@ -429,6 +429,9 @@ type ClusterParameters struct {
 	//
 	// +optional
 	PerConnectionBufferLimitBytes *uint32 `yaml:"per-connection-buffer-limit-bytes,omitempty"`
+
+	// UpstreamTLS contains the TLS policy parameters for upstream connections
+	UpstreamTLS TLSParameters `yaml:"upstream-tls,omitempty"`


👍 that sounds good to me

Signed-off-by: Clay Kauzlaric <[email protected]>

* because the two structs are identical, we can cast between them apparently Signed-off-by: Clay Kauzlaric <[email protected]>

Signed-off-by: Clay Kauzlaric <[email protected]>

pkg/config/parameters.go

Signed-off-by: Clay Kauzlaric <[email protected]>

skriss · 2023-12-15T15:36:28Z

I'll take another pass today but I think this is looking pretty good

skriss

Sorry, a couple last things, but I think that's it from me.

cmd/contour/serve.go

internal/featuretests/v3/upstreamprotocol_test.go

Signed-off-by: Clay Kauzlaric <[email protected]>

skriss

This LGTM, will leave for another maintainer to take another pass, thanks @KauzClay!

Signed-off-by: Clay Kauzlaric <[email protected]>

tsaarni

Looks good to me too! Just one more small request. If I'm not mistaken, I think we can avoid the typecasts to dag.UpstreamTLS

cmd/contour/serve.go

* gets rid of need to typecast Signed-off-by: Clay Kauzlaric <[email protected]>

skriss · 2023-12-20T16:12:22Z

Looks like we're good to merge here, thanks @KauzClay! Will merge later today.

KauzClay requested a review from a team as a code owner October 11, 2023 21:16

KauzClay requested review from skriss and sunjayBhatia and removed request for a team October 11, 2023 21:16

KauzClay mentioned this pull request Oct 12, 2023

[wip] support system-internal-encryption after recent changes knative-extensions/net-contour#980

Closed

KauzClay force-pushed the ck-upstream-tls-config branch 2 times, most recently from 069fd5f to 1fc65cd Compare October 13, 2023 14:24

sunjayBhatia added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Oct 13, 2023

KauzClay force-pushed the ck-upstream-tls-config branch from 9b98cd9 to dbd25ad Compare October 13, 2023 17:38

KauzClay changed the title ~~[wip] allow configuration of upstream TLS connection settings~~ allow configuration of upstream TLS connection settings Oct 13, 2023

tsaarni reviewed Oct 16, 2023

View reviewed changes

cmd/contour/serve.go Outdated Show resolved Hide resolved

pkg/config/parameters.go Outdated Show resolved Hide resolved

internal/envoy/v3/cluster.go Outdated Show resolved Hide resolved

changelogs/unreleased/5828-KauzClay-minor.md Outdated Show resolved Hide resolved

KauzClay force-pushed the ck-upstream-tls-config branch from efaffd3 to 741590d Compare October 23, 2023 16:59

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 7, 2023

skriss removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 7, 2023

sunjayBhatia requested review from tsaarni and clayton-gonsalves November 15, 2023 20:51

sunjayBhatia reviewed Nov 15, 2023

View reviewed changes

test/e2e/httpproxy/httpproxy_test.go Outdated Show resolved Hide resolved

sunjayBhatia reviewed Nov 15, 2023

View reviewed changes

test/e2e/httpproxy/httpproxy_test.go Outdated Show resolved Hide resolved

KauzClay force-pushed the ck-upstream-tls-config branch from 21e08ee to 803416a Compare November 15, 2023 22:55

tsaarni reviewed Nov 22, 2023

View reviewed changes

skriss requested changes Dec 6, 2023

View reviewed changes

add config options for min max tls version on upstream connections

5077619

Signed-off-by: Clay Kauzlaric <[email protected]>

KauzClay force-pushed the ck-upstream-tls-config branch from 6b15f75 to f09bcc9 Compare December 8, 2023 18:21

KauzClay and others added 2 commits December 8, 2023 13:27

Update apis/projectcontour/v1alpha1/contourconfig.go

381f66f

* change json key style * re-run make generate Co-authored-by: Steve Kriss <[email protected]> Signed-off-by: Clay Kauzlaric <[email protected]>

set upstream tls for extension and dnsname clusters

2177ee7

Signed-off-by: Clay Kauzlaric <[email protected]>

KauzClay force-pushed the ck-upstream-tls-config branch from a1cf66a to 2177ee7 Compare December 8, 2023 18:28

sunjayBhatia self-requested a review December 12, 2023 19:03

sunjayBhatia reviewed Dec 12, 2023

View reviewed changes

skriss requested changes Dec 13, 2023

View reviewed changes

KauzClay added 2 commits December 14, 2023 08:26

address nits in changelog

de87dce

Signed-off-by: Clay Kauzlaric <[email protected]>

use casting instead of helper function

edc6550

* because the two structs are identical, we can cast between them apparently Signed-off-by: Clay Kauzlaric <[email protected]>

sunjayBhatia mentioned this pull request Dec 14, 2023

Should globally configurable TLS options should be set on upstream TLS contexts? #3574

Closed

KauzClay added 2 commits December 14, 2023 12:31

config: break tls protocol params into own struct

1baae55

Signed-off-by: Clay Kauzlaric <[email protected]>

test: combine contexts

c5230ab

Signed-off-by: Clay Kauzlaric <[email protected]>

skriss requested changes Dec 14, 2023

View reviewed changes

pkg/config/parameters.go Outdated Show resolved Hide resolved

config: embed protocolparameters into tlsconfig

c9b4c92

Signed-off-by: Clay Kauzlaric <[email protected]>

skriss requested changes Dec 15, 2023

View reviewed changes

cmd/contour/serve.go Show resolved Hide resolved

internal/featuretests/v3/upstreamprotocol_test.go Outdated Show resolved Hide resolved

KauzClay added 3 commits December 15, 2023 12:04

feed upstream tls into ingress and gatewayapi processors

7719cd3

Signed-off-by: Clay Kauzlaric <[email protected]>

test: add featuretest for upstream tls

732d47d

Signed-off-by: Clay Kauzlaric <[email protected]>

don't use upstreamTLS in gatewayAPI processor

7d64501

Signed-off-by: Clay Kauzlaric <[email protected]>

skriss mentioned this pull request Dec 15, 2023

Gateway API: Implement BackendTLSPolicy #5929

Closed

KauzClay force-pushed the ck-upstream-tls-config branch from 89ee43e to 7d64501 Compare December 15, 2023 20:54

skriss approved these changes Dec 15, 2023

View reviewed changes

featuretest: add case for ingress in upstreamtls test

5e3f18e

Signed-off-by: Clay Kauzlaric <[email protected]>

tsaarni requested changes Dec 20, 2023

View reviewed changes

cmd/contour/serve.go Outdated Show resolved Hide resolved

use dag upstreamtls struct in processors

c7dddae

* gets rid of need to typecast Signed-off-by: Clay Kauzlaric <[email protected]>

tsaarni approved these changes Dec 20, 2023

View reviewed changes

skriss merged commit b57fa06 into projectcontour:main Dec 20, 2023
26 checks passed

tsaarni mentioned this pull request Dec 20, 2023

Promote upstream TLS changelog entry from minor to major #6033

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow configuration of upstream TLS connection settings #5828

allow configuration of upstream TLS connection settings #5828

KauzClay commented Oct 11, 2023 •

edited by sunjayBhatia

Loading

tsaarni commented Oct 12, 2023

KauzClay commented Oct 12, 2023 •

edited

Loading

KauzClay commented Oct 13, 2023

codecov bot commented Oct 13, 2023 •

edited

Loading

tsaarni left a comment

github-actions bot commented Nov 7, 2023

KauzClay commented Nov 17, 2023

tsaarni Nov 22, 2023

tsaarni Nov 22, 2023

KauzClay Dec 8, 2023 •

edited

Loading

skriss Dec 13, 2023

KauzClay Dec 14, 2023

tsaarni Nov 22, 2023

skriss Dec 6, 2023

KauzClay Dec 8, 2023

skriss Dec 11, 2023

sunjayBhatia Dec 12, 2023

skriss Dec 6, 2023

sunjayBhatia Dec 12, 2023

KauzClay Dec 14, 2023

sunjayBhatia left a comment •

edited

Loading

skriss Dec 13, 2023

skriss commented Dec 15, 2023

skriss left a comment

skriss left a comment

tsaarni left a comment

skriss commented Dec 20, 2023

	BeforeEach(func() {
	contourConfig.EnableExternalNameService = true
	contourConfiguration.Spec.EnableExternalNameService = ref.To(true)
	})

allow configuration of upstream TLS connection settings #5828

allow configuration of upstream TLS connection settings #5828

Conversation

KauzClay commented Oct 11, 2023 • edited by sunjayBhatia Loading

tsaarni commented Oct 12, 2023

KauzClay commented Oct 12, 2023 • edited Loading

KauzClay commented Oct 13, 2023

codecov bot commented Oct 13, 2023 • edited Loading

Codecov Report

tsaarni left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2023

KauzClay commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KauzClay Dec 8, 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

sunjayBhatia left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Dec 15, 2023

skriss left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

tsaarni left a comment

Choose a reason for hiding this comment

skriss commented Dec 20, 2023

KauzClay commented Oct 11, 2023 •

edited by sunjayBhatia

Loading

KauzClay commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

KauzClay Dec 8, 2023 •

edited

Loading

sunjayBhatia left a comment •

edited

Loading