set golang's secure cipher suites as etcd's cipher suites #4253

zhzhuang-zju · 2023-11-16T07:23:46Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
set golang's default secure cipher suites as etcd's cipher suites to fix CVE-2016-2183.
Which issue(s) this PR fixes:
Parts of #4191

Special notes for your reviewer:
none
Does this PR introduce a user-facing change?:

Fixed CVE-2016-2183 by setting golang's secure cipher suites to etcd's cipher suites

codecov-commenter · 2023-11-16T07:36:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4d6e9d7) 52.83% compared to head (6c7d34f) 52.79%.
Report is 12 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    #4253      +/-   ##
==========================================
- Coverage   52.83%   52.79%   -0.04%     
==========================================
  Files         240      240              
  Lines       23595    23620      +25     
==========================================
+ Hits        12466    12470       +4     
- Misses      10453    10471      +18     
- Partials      676      679       +3

Flag	Coverage Δ
unittests	`52.79% <100.00%> (-0.04%)`	⬇️

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.

RainbowMango · 2023-11-16T08:55:38Z

I guess the CLI part also needs this.

zhzhuang-zju · 2023-11-16T08:59:15Z

I guess the CLI part also needs this.

You are absolutely right! I'll add this part in this pr later.

yanfeng1992 · 2023-11-16T09:53:16Z

I have some suggestions #4191 (comment) @zhzhuang-zju

RainbowMango · 2023-11-17T04:03:17Z

artifacts/deploy/karmada-etcd.yaml

@@ -82,6 +82,7 @@ spec:
            - --trusted-ca-file=/etc/karmada/pki/etcd-ca.crt
            - --data-dir=/var/lib/etcd
            - --snapshot-count=10000
+            - --cipher-suites=TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256,TLS_AES_256_GCM_SHA384,TLS_CHACHA20_POLY1305_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305


It'd be great to have a comment to describe where these suites come from.

It'd be great to have a comment to describe where these suites come from.

+1

Other parts, lgtm

Referring to go/src/crypto/tls/cipher_suites.go, golang supports 17 security algorithms.

On this basis, k8s adds map mapping to two of the security algorithms. Finally, these 19 cipher suites were formed.

karmada/vendor/k8s.io/component-base/cli/flag/ciphersuites_flag.go

Lines 33 to 44 in 98e655f

func init() {

for _, suite := range tls.CipherSuites() {

ciphers[suite.Name] = suite.ID

}

// keep legacy names for backward compatibility

ciphers["TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305"] = tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256

ciphers["TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305"] = tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256

for _, suite := range tls.InsecureCipherSuites() {

insecureCiphers[suite.Name] = suite.ID

}

}

I mean comment in the code, so that when people read the code they can get where these cipher suites come from.

Got it~, I'll comment in the code right away

Signed-off-by: zhzhuang-zju <[email protected]>

RainbowMango

/approve

@yanfeng1992 would you like to take another look?

karmada-bot · 2023-11-20T07:12:46Z

[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

yanfeng1992 · 2023-11-20T12:38:05Z

operator/pkg/controlplane/etcd/mainfests.go

@@ -44,7 +44,8 @@ spec:
        - --key-file=/etc/karmada/pki/etcd/etcd-server.key
        - --data-dir=/var/lib/etcd
        - --snapshot-count=10000
-        - --log-level=debug
+        - --log-level=debug=


Does add = here have any effect? @zhzhuang-zju

Thank you for finding this problem. I fixed it in PR #4286 . Can you help with review?

karmada-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Nov 16, 2023

karmada-bot requested review from jwcesign, lonelyCZ and Poor12 November 16, 2023 07:23

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

zhzhuang-zju mentioned this pull request Nov 16, 2023

Set MinVersion to VersionTLS13 for tlsconfig to fix CVE-2016-2183 #4191

Closed

25 tasks

zhzhuang-zju force-pushed the tls-etcd branch from be037c5 to 1742a8f Compare November 16, 2023 10:44

RainbowMango reviewed Nov 17, 2023

View reviewed changes

zhzhuang-zju changed the title ~~set golang's default secure cipher suites as etcd's cipher suites~~ set golang's secure cipher suites as etcd's cipher suites Nov 17, 2023

zhzhuang-zju force-pushed the tls-etcd branch 2 times, most recently from 3914017 to 6c7d34f Compare November 17, 2023 10:57

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 Nov 17, 2023

set golang's secure cipher suites as etcd's cipher suites

9b1e18b

Signed-off-by: zhzhuang-zju <[email protected]>

zhzhuang-zju force-pushed the tls-etcd branch from 6c7d34f to 9b1e18b Compare November 20, 2023 03:51

karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 20, 2023

RainbowMango approved these changes Nov 20, 2023

View reviewed changes

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

karmada-bot assigned yanfeng1992 Nov 20, 2023

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

karmada-bot merged commit d1598a9 into karmada-io:master Nov 20, 2023
13 checks passed

yanfeng1992 reviewed Nov 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set golang's secure cipher suites as etcd's cipher suites #4253

set golang's secure cipher suites as etcd's cipher suites #4253

zhzhuang-zju commented Nov 16, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Nov 16, 2023 •

edited

Loading

RainbowMango commented Nov 16, 2023

zhzhuang-zju commented Nov 16, 2023

yanfeng1992 commented Nov 16, 2023 •

edited

Loading

RainbowMango Nov 17, 2023

yanfeng1992 Nov 17, 2023 •

edited

Loading

zhzhuang-zju Nov 17, 2023

RainbowMango Nov 17, 2023

zhzhuang-zju Nov 17, 2023

RainbowMango left a comment

karmada-bot commented Nov 20, 2023

yanfeng1992 Nov 20, 2023 •

edited

Loading

zhzhuang-zju Nov 21, 2023

	func init() {
	for _, suite := range tls.CipherSuites() {
	ciphers[suite.Name] = suite.ID
	}
	// keep legacy names for backward compatibility
	ciphers["TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305"] = tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
	ciphers["TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305"] = tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256

	for _, suite := range tls.InsecureCipherSuites() {
	insecureCiphers[suite.Name] = suite.ID
	}
	}

set golang's secure cipher suites as etcd's cipher suites #4253

set golang's secure cipher suites as etcd's cipher suites #4253

Conversation

zhzhuang-zju commented Nov 16, 2023 • edited by RainbowMango Loading

codecov-commenter commented Nov 16, 2023 • edited Loading

Codecov Report

RainbowMango commented Nov 16, 2023

zhzhuang-zju commented Nov 16, 2023

yanfeng1992 commented Nov 16, 2023 • edited Loading

RainbowMango Nov 17, 2023

Choose a reason for hiding this comment

yanfeng1992 Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

zhzhuang-zju Nov 17, 2023

Choose a reason for hiding this comment

RainbowMango Nov 17, 2023

Choose a reason for hiding this comment

zhzhuang-zju Nov 17, 2023

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Nov 20, 2023

yanfeng1992 Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

zhzhuang-zju Nov 21, 2023

Choose a reason for hiding this comment

zhzhuang-zju commented Nov 16, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Nov 16, 2023 •

edited

Loading

yanfeng1992 commented Nov 16, 2023 •

edited

Loading

yanfeng1992 Nov 17, 2023 •

edited

Loading

yanfeng1992 Nov 20, 2023 •

edited

Loading