fix(): reduce minimum tls version for osm controller, verifier, health #5292

whitneygriffith · 2023-03-13T13:20:41Z

Description:
Backport the following fix for #5282 into release-v1.2

Part of #5282

Testing done:
Confirmed demo, health check and verifier worked as expected when OSM control plane has the following configuration:

Tls Max Protocol Version:  TLSv1_2 
Tls Min Protocol Version:  TLSv1_2

Affected area:

Functional Area
New Functionality	[ ]
CI System	[ ]
CLI Tool	[ ]
Certificate Management	[ ]
Control Plane	[x]
Demo	[ ]
Documentation	[ ]
Egress	[ ]
Ingress	[ ]
Install	[ ]
Networking	[ ]
Observability	[ ]
Performance	[ ]
SMI Policy	[ ]
Security	[ ]
Sidecar Injection	[ ]
Tests	[ ]
Upgrade	[ ]
Other	[ ]

keithmattix

Changes look good, but could you add an E2E test to make sure we continuously verify that setting the sidecar to maxTLSVersion v1.2 does not break anything?

codecov-commenter · 2023-03-16T20:33:37Z

Codecov Report

Merging #5292 (4881735) into release-v1.2 (899fdb8) will increase coverage by 0.01%.
The diff coverage is 33.33%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@               Coverage Diff                @@
##           release-v1.2    #5292      +/-   ##
================================================
+ Coverage         68.83%   68.84%   +0.01%     
================================================
  Files               210      210              
  Lines             15679    15679              
================================================
+ Hits              10792    10794       +2     
+ Misses             4836     4834       -2     
  Partials             51       51

Flag	Coverage Δ
unittests	`68.84% <33.33%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/cli/verifier/pod.go	`52.55% <0.00%> (ø)`
pkg/health/health.go	`52.11% <0.00%> (ø)`
pkg/utils/mtls.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Whitney Griffith <whgriffi@microsoft.com>

whitneygriffith · 2023-03-17T19:40:12Z

Changes look good, but could you add an E2E test to make sure we continuously verify that setting the sidecar to maxTLSVersion v1.2 does not break anything?

Added an e2e test

keithmattix

Good work; thanks for the tests! I assume there will be a follow-up PR removing unsupported versions (or just marking them as unsupported?

whitneygriffith · 2023-03-20T18:11:09Z

Good work; thanks for the tests! I assume there will be a follow-up PR removing unsupported versions (or just marking them as unsupported?

Yes! I will be creating a PR that will be removing TLSv1_0 and TLSv1_1 for the envoy max TLS Version.

jaellio · 2023-03-20T20:27:13Z

tests/e2e/e2e_envoy_max_mtls_version_test.go

+
+// Prior iterations of OSM supported a wide range of min and max MTLS versions for the envoy sidecar (TLS_AUTO, TLSv1_0, TLSv1_1, TLSv1_2 and TLSv1_3)
+// even though the OSM Control Plane's minimum version has been upgraded to TLSv1_2
+// This test verifies that the envoy sidecar maxTLSVersion is compatible with the current osm control plane's minTLSVersion


nit:

Suggested change

// This test verifies that the envoy sidecar maxTLSVersion is compatible with the current osm control plane's minTLSVersion

// This test verifies that the envoy sidecar maxTLSVersion is compatible with the current OSM control plane's minTLSVersion

jaellio · 2023-03-20T20:44:59Z

tests/e2e/e2e_envoy_max_mtls_version_test.go

+// Prior iterations of OSM supported a wide range of min and max MTLS versions for the envoy sidecar (TLS_AUTO, TLSv1_0, TLSv1_1, TLSv1_2 and TLSv1_3)
+// even though the OSM Control Plane's minimum version has been upgraded to TLSv1_2
+// This test verifies that the envoy sidecar maxTLSVersion is compatible with the current osm control plane's minTLSVersion
+var _ = OSMDescribe("Test envoy maxTLSVersion is compatible with osm control plane's minTLSVersion",


nit:

Suggested change

var _ = OSMDescribe("Test envoy maxTLSVersion is compatible with osm control plane's minTLSVersion",

var _ = OSMDescribe("Test envoy maxTLSVersion is compatible with OSM control plane's minTLSVersion",

openservicemesh#5292) Addresses potentially incompatible envoy max tls version and OSM control plane min tls version by updating the OSM control plane min tls version from TLSv1_3 to TLSv1_2. Fixes openservicemesh#5282. Signed-off-by: Whitney Griffith <whgriffi@microsoft.com>

openservicemesh#5292) Addresses potentially incompatible envoy max tls version and OSM control plane min tls version by updating the OSM control plane min tls version from TLSv1_3 to TLSv1_2. Fixes openservicemesh#5282. Signed-off-by: Whitney Griffith <whgriffi@microsoft.com> Signed-off-by: jaellio <jaellio@microsoft.com>

#5292) Addresses potentially incompatible envoy max tls version and OSM control plane min tls version by updating the OSM control plane min tls version from TLSv1_3 to TLSv1_2. Fixes #5282. Signed-off-by: Whitney Griffith <whgriffi@microsoft.com> Signed-off-by: jaellio <jaellio@microsoft.com>

whitneygriffith requested review from shashankram, nojnhuh, jaellio, trstringer, keithmattix, steeling and shalier as code owners March 13, 2023 13:20

keithmattix suggested changes Mar 13, 2023

View reviewed changes

whitneygriffith force-pushed the release-v1.2.3 branch from e40c43d to 4b82112 Compare March 16, 2023 23:12

fix(): reduce minimum tls version for osm controller, verifier, health

6e66bfb

Signed-off-by: Whitney Griffith <whgriffi@microsoft.com>

whitneygriffith force-pushed the release-v1.2.3 branch from 4b82112 to 6e66bfb Compare March 16, 2023 23:49

keithmattix approved these changes Mar 20, 2023

View reviewed changes

jaellio approved these changes Mar 20, 2023

View reviewed changes

jaellio mentioned this pull request Mar 20, 2023

fix(): reduce minimum tls version for control plane #5291

Merged

jaellio merged commit 1a9b067 into openservicemesh:release-v1.2 Mar 20, 2023

This was referenced Apr 3, 2023

Cut release v1.2.4 #5308

Closed

Backport fixes to v1.2 #5311

Closed

Backport fixes to v1.1 #5312

Closed

jaellio mentioned this pull request Apr 5, 2023

[backport] cherry-pick 1a9b067 and 00fd7e3 to release-v1.1 #5318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(): reduce minimum tls version for osm controller, verifier, health #5292

fix(): reduce minimum tls version for osm controller, verifier, health #5292

Uh oh!

whitneygriffith commented Mar 13, 2023 •

edited

Loading

Uh oh!

keithmattix left a comment

Uh oh!

codecov-commenter commented Mar 16, 2023 •

edited

Loading

Uh oh!

whitneygriffith commented Mar 17, 2023

Uh oh!

keithmattix left a comment

Uh oh!

whitneygriffith commented Mar 20, 2023

Uh oh!

jaellio Mar 20, 2023

Uh oh!

jaellio Mar 20, 2023

Uh oh!

Uh oh!

	// This test verifies that the envoy sidecar maxTLSVersion is compatible with the current osm control plane's minTLSVersion
	// This test verifies that the envoy sidecar maxTLSVersion is compatible with the current OSM control plane's minTLSVersion

	var _ = OSMDescribe("Test envoy maxTLSVersion is compatible with osm control plane's minTLSVersion",
	var _ = OSMDescribe("Test envoy maxTLSVersion is compatible with OSM control plane's minTLSVersion",

fix(): reduce minimum tls version for osm controller, verifier, health #5292

fix(): reduce minimum tls version for osm controller, verifier, health #5292

Uh oh!

Conversation

whitneygriffith commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keithmattix left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

whitneygriffith commented Mar 17, 2023

Uh oh!

keithmattix left a comment

Choose a reason for hiding this comment

Uh oh!

whitneygriffith commented Mar 20, 2023

Uh oh!

jaellio Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

jaellio Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whitneygriffith commented Mar 13, 2023 •

edited

Loading

codecov-commenter commented Mar 16, 2023 •

edited

Loading