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.

Sign up for GitHub

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

Jump to bottom

Add TLS min version flag #58528

Merged
merged 2 commits into from
Jan 23, 2018
Merged

Add TLS min version flag #58528

merged 2 commits into from
Jan 23, 2018

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jan 19, 2018 •
edited
Loading

Adds a flag for controlling the minimum TLS level allowed.

/assign liggitt

@kubernetes/sig-node-pr-reviews @k8s-mirror-api-machinery-pr-reviews

--tls-min-version on kubelet and kube-apiserver allow for configuring minimum TLS versions

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 19, 2018
liggitt
liggitt reviewed Jan 19, 2018
View reviewed changes
cmd/kubelet/app/options/options.go Outdated
fs.StringVar(&c.TLSMinVersion, "tls-min-version", c.TLSMinVersion,
"Minimum TLS version supported. "+
"Value must match version names from https://golang.org/pkg/crypto/tls/#pkg-constants. "+
"If omitted, the default Go cipher suites will be used")
Copy link
Member

Choose a reason for hiding this comment

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

don't promise a default... we want the freedom to change this as needed

liggitt
liggitt reviewed Jan 19, 2018
View reviewed changes
cmd/kubelet/app/server.go Outdated
// Can't use TLSv1.1 because of RC4 cipher usage
minTLSVersion := uint16(tls.VersionTLS12)
if len(kc.TLSMinVersion) > 0 {
minTLSVersion, err = flag.TLSCipherSuite(kc.TLSMinVersion)
Copy link
Member

Choose a reason for hiding this comment

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

this isn't a cipher suite, it's a TLS version... need a new TLSVersion func

liggitt
liggitt reviewed Jan 19, 2018
View reviewed changes
staging/src/k8s.io/apiserver/pkg/util/flag/ciphersuites_flag.go Outdated
}
ciphersIntSlice = append(ciphersIntSlice, intValue)
}
return ciphersIntSlice, nil
}

func TLSCipherSuite(cipherName string) (uint16, error) {
Copy link
Member

Choose a reason for hiding this comment

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

this should be TLSVersion(tlsVersion string) (uint16, error) that deals with these constants:

        VersionSSL30 = 0x0300
        VersionTLS10 = 0x0301
        VersionTLS11 = 0x0302
        VersionTLS12 = 0x0303

@deads2k
Copy link
Contributor Author

deads2k commented Jan 19, 2018

Ah, the second magic block in openshift crypto.

@deads2k deads2k force-pushed the kubelet-02-mincipher branch from b00d80a to c01ff0e Compare January 19, 2018 17:18
@deads2k
Copy link
Contributor Author

deads2k commented Jan 19, 2018

fixed

liggitt
liggitt reviewed Jan 19, 2018
View reviewed changes
staging/src/k8s.io/apiserver/pkg/util/flag/ciphersuites_flag.go
@@ -62,3 +62,26 @@ func TLSCipherSuites(cipherNames []string) ([]uint16, error) {
}
return ciphersIntSlice, nil
}

var versions = map[string]uint16{
Copy link
Member

Choose a reason for hiding this comment

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

update TestConstantMaps to keep this in sync

@liggitt
Copy link
Member

liggitt commented Jan 19, 2018

nit on test, LGTM otherwise. can clean up flag help (listing valid values) in followup along with the valid values for ciphers

@liggitt
Copy link
Member

liggitt commented Jan 19, 2018

and needs a release note

@deads2k deads2k force-pushed the kubelet-02-mincipher branch 2 times, most recently from 740c4b0 to 11b7980 Compare January 19, 2018 17:55
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 19, 2018
@deads2k deads2k force-pushed the kubelet-02-mincipher branch from 11b7980 to 4ce7bcc Compare January 19, 2018 19:08
@deads2k
Copy link
Contributor Author

deads2k commented Jan 19, 2018

test and release note added.

@liggitt
Copy link
Member

liggitt commented Jan 19, 2018

/retest
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2018
@liggitt liggitt added this to the v1.10 milestone Jan 19, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Jan 19, 2018

/approve no-issue

@derekwaynecarr
Copy link
Member

kubelet changes LGTM

/approve no-issue

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr, liggitt

Associated issue requirement bypassed by: deads2k, derekwaynecarr

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@deads2k
Copy link
Contributor Author

deads2k commented Jan 22, 2018

/retest

2 similar comments
@deads2k
Copy link
Contributor Author

deads2k commented Jan 22, 2018

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Jan 22, 2018

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 58547, 57228, 58528, 58499, 58618). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 3550551 into kubernetes:master Jan 23, 2018
@deads2k deads2k deleted the kubelet-02-mincipher branch July 3, 2018 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.
6 participants
@deads2k @liggitt @derekwaynecarr @k8s-ci-robot @k8s-github-robot @fejta-bot