feat: Add ca to check the validation of the member clusters' server certificate #4183

jwcesign · 2023-10-26T07:31:25Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #4182

Special notes for your reviewer:
If I change the following line to false:

karmada/pkg/util/proxy/proxy.go

Line 79 in 3267155

TLS: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec

It will have the following error in karmada-aggregated-apiserver:

And karmadactl couldn't list the pods in member clusters:

With this PR, everything works fine

Does this PR introduce a user-facing change?:

`karmada-aggregated-apiserver`: Add ca to check the validation of the member clusters' server certificate.

jwcesign · 2023-10-26T07:33:28Z

/cc @chaosi-zju @XiShanYongYe-Chang @RainbowMango

zishen

Good for validating the member clusters' server certificate

RainbowMango · 2023-10-26T10:07:43Z

The unit test is failing, @jwcesign :
https://github.com/karmada-io/karmada/actions/runs/6651924401/job/18075272929?pr=4183#step:4:5247

RainbowMango · 2023-10-26T10:16:50Z

pkg/util/proxy/proxy.go

@@ -94,14 +102,39 @@ func NewThrottledUpgradeAwareProxyHandler(location *url.URL, transport http.Roun
 	return proxy.NewUpgradeAwareHandler(location, transport, wrapTransport, upgradeRequired, proxy.NewErrorResponder(responder))
 }

+func GetTlsConfigForCluster(ctx context.Context, cluster *clusterapis.Cluster, secretGetter SecretGetterFunc) (*tls.Config, error) {
+	if cluster.Spec.SecretRef == nil {


Can you remind me when the secret would not exist?

From my point of view, it couldn't be empty. But I saw it's an optional field in the struct comment. Or I delete this field with kubectl.

For Pull mode cluster, the karmada-agent may not set the secret, see the usage from karmada-agent:

--report-secrets strings The secrets that are allowed to be reported to the Karmada control plane during registering. Valid values are 'KubeCredentials', 'KubeImpersonator' and 'None'. e.g 'KubeCredentials,KubeImpersonator' or 'None'. (default [KubeCredentials,KubeImpersonator])

Suggested change

if cluster.Spec.SecretRef == nil {

// The secret is optional for a pull-mode cluster, if no secret just returns a config with root CA unset.

if cluster.Spec.SecretRef == nil {

Maybe we can have a comment here.

RainbowMango · 2023-10-26T10:17:33Z

pkg/util/proxy/proxy.go

+	return &tls.Config{
+		RootCAs:            caCertPool,
+		MinVersion:         tls.VersionTLS13,
+		InsecureSkipVerify: cluster.Spec.InsecureSkipTLSVerification, //nolint:gosec


Can we remove the nolint:gosec now?

Same as above.

If delete it, there is a lint error: cluster.Spec.InsecureSkipTLSVerification may be true.

But it's allowed to be true(if users want to do it).

Before:

pkg/util/proxy/proxy.go:109:24: G402: TLS InsecureSkipVerify set true. (gosec) InsecureSkipVerify: true,

After:

pkg/util/proxy/proxy.go:109:24: G402: TLS InsecureSkipVerify may be true. (gosec) InsecureSkipVerify: cluster.Spec.InsecureSkipTLSVerification,

we might need a comment to explain it.

Signed-off-by: jwcesign <[email protected]>

pkg/util/proxy/proxy.go

RainbowMango · 2023-10-26T12:47:04Z

pkg/util/proxy/proxy.go

@@ -94,14 +102,39 @@ func NewThrottledUpgradeAwareProxyHandler(location *url.URL, transport http.Roun
 	return proxy.NewUpgradeAwareHandler(location, transport, wrapTransport, upgradeRequired, proxy.NewErrorResponder(responder))
 }

+func GetTlsConfigForCluster(ctx context.Context, cluster *clusterapis.Cluster, secretGetter SecretGetterFunc) (*tls.Config, error) {
+	if cluster.Spec.SecretRef == nil {


Suggested change

if cluster.Spec.SecretRef == nil {

// The secret is optional for a pull-mode cluster, if no secret just returns a config with root CA unset.

if cluster.Spec.SecretRef == nil {

Maybe we can have a comment here.

codecov-commenter · 2023-10-26T13:14:35Z

Codecov Report

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

Comparison is base (3267155) 52.91% compared to head (473b2be) 52.86%.
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4183      +/-   ##
==========================================
- Coverage   52.91%   52.86%   -0.06%     
==========================================
  Files         239      239              
  Lines       23526    23571      +45     
==========================================
+ Hits        12449    12460      +11     
- Misses      10401    10434      +33     
- Partials      676      677       +1

Flag	Coverage Δ
unittests	`52.86% <38.98%> (-0.06%)`	⬇️

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

Files	Coverage Δ
pkg/registry/cluster/storage/storage.go	`0.00% <0.00%> (ø)`
pkg/util/proxy/proxy.go	`74.14% <46.00%> (-15.76%)`	⬇️

... and 1 file with indirect coverage changes

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

RainbowMango

/lgtm
/approve

karmada-bot · 2023-10-26T13:34:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 26, 2023

karmada-bot requested review from Garrybest and XiShanYongYe-Chang October 26, 2023 07:31

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 26, 2023

karmada-bot requested review from chaosi-zju and RainbowMango October 26, 2023 07:33

jwcesign force-pushed the fix-insecure branch 2 times, most recently from 8ce3bd7 to 56e9649 Compare October 26, 2023 08:25

zishen reviewed Oct 26, 2023

View reviewed changes

jwcesign force-pushed the fix-insecure branch from 56e9649 to 1362e7b Compare October 26, 2023 09:08

RainbowMango reviewed Oct 26, 2023

View reviewed changes

feat: add ca to check the validation of the server certificate

473b2be

Signed-off-by: jwcesign <[email protected]>

jwcesign force-pushed the fix-insecure branch from 1362e7b to d891cd0 Compare October 26, 2023 11:28

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 26, 2023

jwcesign force-pushed the fix-insecure branch 2 times, most recently from a360866 to 4e89504 Compare October 26, 2023 12:36

RainbowMango reviewed Oct 26, 2023

View reviewed changes

jwcesign force-pushed the fix-insecure branch from 4e89504 to 473b2be Compare October 26, 2023 12:51

RainbowMango approved these changes Oct 26, 2023

View reviewed changes

karmada-bot assigned RainbowMango Oct 26, 2023

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2023

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2023

karmada-bot merged commit edeb94b into karmada-io:master Oct 26, 2023
12 checks passed

jwcesign mentioned this pull request Oct 31, 2023

them command karmadactl exec does not work well #4197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ca to check the validation of the member clusters' server certificate #4183

feat: Add ca to check the validation of the member clusters' server certificate #4183

jwcesign commented Oct 26, 2023 •

edited

Loading

jwcesign commented Oct 26, 2023

zishen left a comment

RainbowMango commented Oct 26, 2023

RainbowMango Oct 26, 2023

jwcesign Oct 26, 2023 •

edited

Loading

RainbowMango Oct 26, 2023

RainbowMango Oct 26, 2023

RainbowMango Oct 26, 2023

jwcesign Oct 26, 2023 •

edited

Loading

RainbowMango Oct 26, 2023

RainbowMango Oct 26, 2023

codecov-commenter commented Oct 26, 2023

RainbowMango left a comment

karmada-bot commented Oct 26, 2023

	if cluster.Spec.SecretRef == nil {
	// The secret is optional for a pull-mode cluster, if no secret just returns a config with root CA unset.
	if cluster.Spec.SecretRef == nil {

feat: Add ca to check the validation of the member clusters' server certificate #4183

feat: Add ca to check the validation of the member clusters' server certificate #4183

Conversation

jwcesign commented Oct 26, 2023 • edited Loading

jwcesign commented Oct 26, 2023

zishen left a comment

Choose a reason for hiding this comment

RainbowMango commented Oct 26, 2023

RainbowMango Oct 26, 2023

Choose a reason for hiding this comment

jwcesign Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

RainbowMango Oct 26, 2023

Choose a reason for hiding this comment

RainbowMango Oct 26, 2023

Choose a reason for hiding this comment

RainbowMango Oct 26, 2023

Choose a reason for hiding this comment

jwcesign Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

RainbowMango Oct 26, 2023

Choose a reason for hiding this comment

RainbowMango Oct 26, 2023

Choose a reason for hiding this comment

codecov-commenter commented Oct 26, 2023

Codecov Report

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Oct 26, 2023

jwcesign commented Oct 26, 2023 •

edited

Loading

jwcesign Oct 26, 2023 •

edited

Loading

jwcesign Oct 26, 2023 •

edited

Loading