fix(util/proxy): fix tls.config when secret.spec.caBundle is nil #4371

CharlesQQ · 2023-12-05T10:47:11Z

What type of PR is this?

What this PR does / why we need it:

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmadactl`: Fixed return err in case of  secret.spec. caBundle is nil

codecov-commenter · 2023-12-05T11:00:03Z

Codecov Report

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

Comparison is base (09a43c6) 51.87% compared to head (fca22c2) 51.89%.
Report is 10 commits behind head on master.

Files	Patch %	Lines
pkg/util/proxy/proxy.go	0.00%	5 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4371      +/-   ##
==========================================
+ Coverage   51.87%   51.89%   +0.01%     
==========================================
  Files         243      243              
  Lines       24114    24112       -2     
==========================================
+ Hits        12509    12512       +3     
+ Misses      10922    10918       -4     
+ Partials      683      682       -1

Flag	Coverage Δ
unittests	`51.89% <0.00%> (+0.01%)`	⬆️

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

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

XiShanYongYe-Chang · 2023-12-05T12:47:01Z

/assign @jwcesign

jwcesign · 2023-12-05T14:28:10Z

pkg/util/proxy/proxy.go

@@ -124,7 +124,7 @@ func NewThrottledUpgradeAwareProxyHandler(location *url.URL, transport http.Roun

 func GetTlsConfigForCluster(ctx context.Context, cluster *clusterapis.Cluster, secretGetter SecretGetterFunc) (*tls.Config, error) {
 	// 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 {
+	if cluster.Spec.SecretRef == nil || cluster.Spec.InsecureSkipTLSVerification {


Should we set the RootCAs even if InsecureSkipTLSVerification is true(If the caBundle exists)?

Should we set the RootCAs even if InsecureSkipTLSVerification is true(If the caBundle exists)?

en.., sure, It's a batter choice

/cc @jwcesign

jwcesign

lgtm

cc @chaosi-zju for double checking

chaosi-zju · 2023-12-06T03:44:12Z

pkg/util/proxy/proxy.go

 	caBundle, err := getClusterCABundle(cluster.Name, caSecret)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get CA bundle for cluster %s: %v", cluster.Name, err)
+		return &tls.Config{
+			MinVersion: tls.VersionTLS13,
+			// Ignore false positive warning: "TLS InsecureSkipVerify may be true. (gosec)"
+			// Whether to skip server certificate verification depends on the
+			// configuration(.spec.insecureSkipTLSVerification, defaults to false) in a Cluster object.
+			InsecureSkipVerify: cluster.Spec.InsecureSkipTLSVerification, //nolint:gosec
+		}, nil


hi, in your case, cluster.Spec.InsecureSkipTLSVerification=true and caBundle="", you code works fine.

But, if cluster.Spec.InsecureSkipTLSVerification=false and caBundle="", this case may be we should report error if caBundle is empty?

Otherwise, you see, caBundle is empty and we return &tls.Config{InsecureSkipVerify: false}, this is a invalid config.

if cluster.Spec.InsecureSkipTLSVerification=false and caBundle="", this case may be we should report error if caBundle is empty?

I think, if the ca in the container is correct, it will still work fine

hi, in your case, cluster.Spec.InsecureSkipTLSVerification=true and caBundle="", you code works fine.

But, if cluster.Spec.InsecureSkipTLSVerification=false and caBundle="", this case may be we should report error if caBundle is empty?

Otherwise, you see, caBundle is empty and we return &tls.Config{InsecureSkipVerify: false}, this is a invalid config.

got it!!!

jwcesign · 2023-12-06T06:14:14Z

pkg/util/proxy/proxy.go

+				InsecureSkipVerify: cluster.Spec.InsecureSkipTLSVerification, //nolint:gosec
+			}, nil
+		}
+		return nil, fmt.Errorf("insecureSkipTLSVerification is false, but failed to get CA bundle for cluster %s: %v", cluster.Name, err)


How about this? @chaosi-zju @CharlesQQ

func getClusterCABundle(clusterName string, secret *corev1.Secret) []byte { caBundle, found := secret.Data[clusterapis.SecretCADataKey] if !found { return []byte{} } return caBundle } func GetTlsConfigForCluster(ctx context.Context, cluster *clusterapis.Cluster, secretGetter SecretGetterFunc) (*tls.Config, error) { ... caBundle := getClusterCABundle(cluster.Name, caSecret) caCertPool := x509.NewCertPool() caCertPool.AppendCertsFromPEM(caBundle) return &tls.Config{ RootCAs: caCertPool, MinVersion: tls.VersionTLS13, // Ignore false positive warning: "TLS InsecureSkipVerify may be true. (gosec)" // Whether to skip server certificate verification depends on the // configuration(.spec.insecureSkipTLSVerification, defaults to false) in a Cluster object. InsecureSkipVerify: cluster.Spec.InsecureSkipTLSVerification, //nolint:gosec }, nil

I think, if the ca in the container is correct, it will still work fine

I got your point.

How about this?

I think perfect.

Hi, @CharlesQQ
Can you help update this?

How about this? @chaosi-zju @CharlesQQ

func getClusterCABundle(clusterName string, secret *corev1.Secret) []byte { caBundle, found := secret.Data[clusterapis.SecretCADataKey] if !found { return []byte{} } return caBundle } func GetTlsConfigForCluster(ctx context.Context, cluster *clusterapis.Cluster, secretGetter SecretGetterFunc) (*tls.Config, error) { ... caBundle := getClusterCABundle(cluster.Name, caSecret) caCertPool := x509.NewCertPool() caCertPool.AppendCertsFromPEM(caBundle) return &tls.Config{ RootCAs: caCertPool, MinVersion: tls.VersionTLS13, // Ignore false positive warning: "TLS InsecureSkipVerify may be true. (gosec)" // Whether to skip server certificate verification depends on the // configuration(.spec.insecureSkipTLSVerification, defaults to false) in a Cluster object. InsecureSkipVerify: cluster.Spec.InsecureSkipTLSVerification, //nolint:gosec }, nil

motified!!!

Signed-off-by: chang.qiangqiang <[email protected]>

jwcesign · 2023-12-06T07:42:30Z

Nice work! Thanks for your contribution.

/lgtm

jwcesign · 2023-12-06T08:03:43Z

/cc @RainbowMango

XiShanYongYe-Chang

Thanks @CharlesQQ
LGMT

I think this should affect user behavior. Can we add a release note to explain it?

XiShanYongYe-Chang · 2023-12-08T08:31:14Z

/kind bug

CharlesQQ · 2023-12-08T10:39:02Z

Thanks @CharlesQQ LGMT

I think this should affect user behavior. Can we add a release note to explain it?

Of cause

XiShanYongYe-Chang · 2023-12-09T06:15:27Z

/approve

karmada-bot · 2023-12-09T06:15:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: XiShanYongYe-Chang

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:

~~pkg/util/OWNERS~~ [XiShanYongYe-Chang]

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

…k-of-#4371-upstream-release-1.8 Automated cherry pick of #4371: fix(util/proxy): fix tls.config when secret.spec.caBundle is

karmada-bot requested review from chaunceyjiang and XiShanYongYe-Chang December 5, 2023 10:47

karmada-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 5, 2023

CharlesQQ force-pushed the proxy-fix branch from 1bafd90 to 42cd284 Compare December 5, 2023 10:48

CharlesQQ force-pushed the proxy-fix branch from 42cd284 to a22626f Compare December 5, 2023 11:27

karmada-bot assigned jwcesign Dec 5, 2023

jwcesign reviewed Dec 5, 2023

View reviewed changes

CharlesQQ force-pushed the proxy-fix branch from a22626f to b7b78ad Compare December 6, 2023 02:36

CharlesQQ requested a review from jwcesign December 6, 2023 02:45

jwcesign reviewed Dec 6, 2023

View reviewed changes

chaosi-zju reviewed Dec 6, 2023

View reviewed changes

CharlesQQ force-pushed the proxy-fix branch from b7b78ad to 66668b5 Compare December 6, 2023 04:15

karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 6, 2023

karmada-bot assigned chaosi-zju Dec 6, 2023

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

jwcesign reviewed Dec 6, 2023

View reviewed changes

fix(util/proxy): fix tls.config when secret.spec.caBundle is nil

fca22c2

Signed-off-by: chang.qiangqiang <[email protected]>

CharlesQQ force-pushed the proxy-fix branch from 66668b5 to fca22c2 Compare December 6, 2023 07:19

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2023

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

karmada-bot requested a review from RainbowMango December 6, 2023 08:03

XiShanYongYe-Chang reviewed Dec 8, 2023

View reviewed changes

karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 8, 2023

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

karmada-bot merged commit e2c6ece into karmada-io:master Dec 9, 2023
12 checks passed

XiShanYongYe-Chang mentioned this pull request Dec 9, 2023

Automated cherry pick of #4371: fix(util/proxy): fix tls.config when secret.spec.caBundle is #4391

Merged

karmada-bot added a commit that referenced this pull request Dec 11, 2023

Merge pull request #4391 from XiShanYongYe-Chang/automated-cherry-pic…

bf69c28

…k-of-#4371-upstream-release-1.8 Automated cherry pick of #4371: fix(util/proxy): fix tls.config when secret.spec.caBundle is

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(util/proxy): fix tls.config when secret.spec.caBundle is nil #4371

fix(util/proxy): fix tls.config when secret.spec.caBundle is nil #4371

CharlesQQ commented Dec 5, 2023 •

edited

Loading

codecov-commenter commented Dec 5, 2023 •

edited

Loading

XiShanYongYe-Chang commented Dec 5, 2023

jwcesign Dec 5, 2023

CharlesQQ Dec 6, 2023

CharlesQQ Dec 6, 2023

jwcesign left a comment

chaosi-zju Dec 6, 2023

jwcesign Dec 6, 2023 •

edited

Loading

CharlesQQ Dec 6, 2023

jwcesign Dec 6, 2023

chaosi-zju Dec 6, 2023 •

edited

Loading

jwcesign Dec 6, 2023

CharlesQQ Dec 6, 2023

jwcesign commented Dec 6, 2023

jwcesign commented Dec 6, 2023

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang commented Dec 8, 2023

CharlesQQ commented Dec 8, 2023 •

edited

Loading

XiShanYongYe-Chang commented Dec 9, 2023

karmada-bot commented Dec 9, 2023

fix(util/proxy): fix tls.config when secret.spec.caBundle is nil #4371

fix(util/proxy): fix tls.config when secret.spec.caBundle is nil #4371

Conversation

CharlesQQ commented Dec 5, 2023 • edited Loading

codecov-commenter commented Dec 5, 2023 • edited Loading

Codecov Report

XiShanYongYe-Chang commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwcesign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwcesign Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaosi-zju Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwcesign commented Dec 6, 2023

jwcesign commented Dec 6, 2023

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Dec 8, 2023

CharlesQQ commented Dec 8, 2023 • edited Loading

XiShanYongYe-Chang commented Dec 9, 2023

karmada-bot commented Dec 9, 2023

CharlesQQ commented Dec 5, 2023 •

edited

Loading

codecov-commenter commented Dec 5, 2023 •

edited

Loading

jwcesign Dec 6, 2023 •

edited

Loading

chaosi-zju Dec 6, 2023 •

edited

Loading

CharlesQQ commented Dec 8, 2023 •

edited

Loading