Skip to content

Plumb and allow customizing ciphers and tls version #13167

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
Merged
merged 2 commits into from
Mar 3, 2017
Merged

Plumb and allow customizing ciphers and tls version #13167

merged 2 commits into from
Mar 3, 2017

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Mar 1, 2017 •
edited
Loading

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1427919
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1387789
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1425941

  • default to existing secure cipher list
  • switch config to human readable names, add validation
  • plumb to kubelet server init
  • integration test

Allows the following in all servingInfo stanzas:

bindAddress: ...
...
minTLSVersion: VersionTLS10
cipherSuites:
- TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
...

Valid tls versions are "VersionTLS10", "VersionTLS11", "VersionTLS12"

Valid cipher suites are https://golang.org/pkg/crypto/tls/#pkg-constants for the golang version used to build origin, taken from descriptions of http://www.iana.org/assignments/tls-parameters/tls-parameters.xml#tls-parameters-4

sttts
sttts reviewed Mar 1, 2017
View reviewed changes
pkg/cmd/server/api/v1/types.go Outdated
// TLS 1.0 = 0x0301
// TLS 1.1 = 0x0302
// TLS 1.2 = 0x0303
MinTLSVersion uint16 `json:"minTLSVersion,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

If these types are visible to the user and edited by him, some text representation would be better: "tls1.2"

sttts
sttts reviewed Mar 1, 2017
View reviewed changes
pkg/cmd/server/api/v1/types.go Outdated
MinTLSVersion uint16 `json:"minTLSVersion,omitempty"`
// CipherSuites contains an overridden list of ciphers for the server to support.
// Values must match ciphers from http://www.iana.org/assignments/tls-parameters/tls-parameters.xml#tls-parameters-4
CipherSuites []uint16 `json:"cipherSuites,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

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 think maintaining a mapping of human readable names to IANA versions across go versions will drift

Copy link
Contributor

Choose a reason for hiding this comment

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

I hoped that there is a standard for human readable names of ciphers.

@sttts
Copy link
Contributor

sttts commented Mar 1, 2017

Looks good. An integration test would make sense.

@smarterclayton
Copy link
Contributor

LGTM

@liggitt liggitt added this to the 1.5.0 milestone Mar 1, 2017
@liggitt
Copy link
Contributor Author

liggitt commented Mar 1, 2017

[test]

@liggitt liggitt mentioned this pull request Mar 1, 2017
Closed
@liggitt
Copy link
Contributor Author

liggitt commented Mar 1, 2017

[merge]

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2017
@mfojtik mfojtik mentioned this pull request Mar 2, 2017
Merged
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to be01706

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/752/) (Base Commit: 29e3a46)

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2017
@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to be01706

@openshift-bot
Copy link
Contributor

openshift-bot commented Mar 3, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/791/) (Base Commit: a86ef13) (Image: devenv-rhel7_6027)

@openshift-bot openshift-bot merged commit ad8ea70 into openshift:master Mar 3, 2017
@liggitt liggitt deleted the ciphers branch March 6, 2017 16:24
@liggitt liggitt mentioned this pull request Mar 6, 2017
Merged
@martineg martineg mentioned this pull request Mar 8, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@liggitt @sttts @smarterclayton @openshift-bot