Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add min TLS version for stream server #3764

Merged
merged 1 commit into from
Apr 16, 2022
Merged

add min TLS version for stream server #3764

merged 1 commit into from
Apr 16, 2022

Conversation

snstaberah
Copy link
Contributor

Signed-off-by: snstaberah yangyonglc@inspur.com

What type of PR is this?

/kind feature

What this PR does / why we need it:
add MinVersion: tls.VersionTLS12 in streamserver,this can improve security of cloudcore,and pass some security test
Which issue(s) this PR fixes:

Fixes #3753

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@kubeedge-bot kubeedge-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 2, 2022
@kubeedge-bot kubeedge-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 2, 2022
ClientAuth: tls.RequestClientCert,
ClientAuth: tls.RequestClientCert,
MinVersion: tls.VersionTLS12,
CipherSuites: []uint16{tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stream server is used to serve the connection from apiserver, not sure whether the logs/exec works if adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a test on kubernetes 1.20.9 and kubeedge 1.8.1,logs/exec works well;
In fact,apiserver use client-go to get a TLSConfig,and client-go also define the TLS min version to 1.2 for security;
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, the MinVersion looks good, but for the CipherSuites, in kubeedge we use the ECDSA keys, so only set one tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, not sure if it's correct for k8s, myabe k8s can use rsa keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read kubernetes apiserver code(client-side)and kubelet code (server-side) for logs/exec,in kubernetes,kubelet can use a flag to define tls-cipher-suites(https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/),and apiserver didn't set cipher-suites in client request tls config,so apiserver will use default go lib (crypto/tls) cipher suite list to compare with server side;
image

to keep consistent with kubelet,we can just use default tls cipher suite config for stream server to receive request from apiserver, I have modified my commit and did a test on kubernetes 1.20.9 and kubeedge 1.8.1,logs/exec works well;

but in practice,kubelet often set only one or two prefered cipher suite to improve security,less than the default cipher suite list,so i think it is better to add a flag for streamserver to let user chose a cipher suite in the future;

this has no effect for the apiserver to request stream server because apiserver didn't set cipher-suites in client request ,so it can use a full list to dial with stream server;

image
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snstaberah Thanks for the explanation! Looks good to use the default value in go. I found in my cluster installed by kubeadm, the kubelet haven't set the cipher-suite flag, so wondering the pic showed above is your cluster?

Copy link
Member

@fisherxu fisherxu Apr 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now for the cipher-suite in kubeedge cloud-edge connection, we only set one is tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, which means we can't compatible the rsa certs, but we support user to set their own certs. So do you think it's needed to add tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 in the list?

Another point I read from the docs is in tls1.3, it's not needed to set the cipher-suite...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my company,we set both apiserver and kubelet
with TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,according to this link https://www.ibm.com/docs/en/cloud-private/3.1.2?topic=installation-specifying-tls-ciphers-etcd-kubernetes,to fix SSL Medium Strength Cipher Suites Supported(SWEET32) problem;there are some similar reference with https://docs.openshift.com/container-platform/4.8/nodes/nodes/nodes-nodes-tls.html;
it is special config for TLS1.2;
if we support user to set their own certs, to connect from edgecore to tunnel server,i think add tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 in the tunnel server cipher suite list,or add a flag in cloudcore for user to set their prefered cipher suite is better;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. And now the best choice for cipher suite seems is ECDHE_ECDSA, with more short key and high security ;-)

Signed-off-by: snstaberah <yangyonglc@inspur.com>
Copy link
Member

@fisherxu fisherxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2022
@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fisherxu

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:

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

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2022
@kubeedge-bot kubeedge-bot merged commit 7773345 into kubeedge:master Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update TLS default version in streamserver ?
3 participants