Allow specifying haproxy SSL Cipher list #14505
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
From https://wiki.mozilla.org/Security/Server_Side_TLS update the three cipher suites: # Modern cipher suite (no legacy browser support) - not currently used # Intermediate cipher suite (default) - currently used # Old cipher suite (maximum compatibility but insecure) - not currently used
|
@knobunc PTAL |
|
@openshift/networking PTAL |
knobunc
reviewed
Jun 8, 2017
| ssl-default-bind-ciphers ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-DSS-AES128-GCM-SHA256:kEDH+AESGCM:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-DSS-AES128-SHA256:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA:DHE-RSA-AES256-SHA:ECDHE-RSA-DES-CBC3-SHA:ECDHE-ECDSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:AES:DES-CBC3-SHA:HIGH:SEED:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:!RSAPSK:!aDH:!aECDH:!EDH-DSS-DES-CBC3-SHA:!KRB5-DES-CBC3-SHA:!SRP | ||
|
|
||
| {{- else }} | ||
| # user provided list of ciphers (Colon separated list as seen above) |
There was a problem hiding this comment.
It might be worth noting in the comment that the env here can never use the default since if they had not passed one, then the default of "intermediate" would have been used already.
knobunc
reviewed
Jun 8, 2017
pkg/cmd/admin/router/router.go
Outdated
| @@ -305,6 +309,7 @@ func NewCmdRouter(f *clientcmd.Factory, parentName, name string, out, errout io. | |||
| cmd.Flags().StringVar(&cfg.ExternalHostPartitionPath, "external-host-partition-path", cfg.ExternalHostPartitionPath, "If the underlying router implementation uses partitions for control boundaries, this is the path to use for that partition.") | |||
| cmd.Flags().BoolVar(&cfg.DisableNamespaceOwnershipCheck, "disable-namespace-ownership-check", cfg.DisableNamespaceOwnershipCheck, "Disables the namespace ownership check and allows different namespaces to claim either different paths to a route host or overlapping host names in case of a wildcard route. The default behavior (false) to restrict claims to the oldest namespace that has claimed either the host or the subdomain. Please be aware that if namespace ownership checks are disabled, routes in a different namespace can use this mechanism to 'steal' sub-paths for existing domains. This is only safe if route creation privileges are restricted, or if all the users can be trusted.") | |||
| cmd.Flags().StringVar(&cfg.MaxConnections, "max-connections", cfg.MaxConnections, "Specifies the maximum number of concurrent connections. Not supported for F5.") | |||
| cmd.Flags().StringVar(&cfg.Ciphers, "ciphers", cfg.Ciphers, "Specifies the cipher set (modern|intermediate|old) or the : separated list of ciphers to be used by bind. Not supported for F5.") | |||
There was a problem hiding this comment.
Perhaps:
"Specifies the cipher suites to use. You can choose a predefined cipher set ('modern', 'intermediate', or 'old') or specify exact cipher suites by passing a : separated list. Not supported for F5."
|
@openshift/networking PTAL |
rajatchopra
approved these changes
Jun 8, 2017
|
Code looks good |
The user can select from among 3 predefined cipher lists: modern, intermediate, or old. Alternatively the use may provide a custom cipher list see "man 1 ciphers". The list is used to negotiate a cipher between a user and haproxyi during bind. The predefined lists are from: https://wiki.mozilla.org/Security/Server_Side_TLS A new option to "oc adm router", --ciphers, is added to specify the cipher list. The values are modern|intermediate|old, or a ":" separated list of ciphers from "man 1 ciphers" Option --ciphers creates an environmen variable, ROUTER_CIPHERS, which is passed to the router pod. See https://trello.com/c/oeP7vrTZ
|
@rajatchopra we don't validate the ENV since those have to be set by the router admin and they can do "bad things" anyway. It's any annotations we use that we need to be scrupulous about validating. |
|
@smarterclayton Any concerns with this? |
|
No concerns as long as this is admin, not end user. |
|
@knobunc Is this ready to merge? |
knobunc
approved these changes
Jun 12, 2017
|
[merge] |
|
Evaluated for origin merge up to f2896fc |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to f2896fc |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2143/) (Base Commit: acb8636) |
|
continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/973/) (Base Commit: acb8636) |
|
Flaked in queue, no conflicts. |
pecameron
added a commit
to pecameron/openshift-docs
that referenced
this pull request
Jun 27, 2017
Openshift 3.6 The user can select from among 3 predefined cipher lists: modern, intermediate, or old. Alternatively the use may provide a custom cipher list see "openssl ciphers". The list is used to negotiate a cipher between a user and haproxyi during bind. The predefined lists are from: https://wiki.mozilla.org/Security/Server_Side_TLS A new option to "oc adm router", --ciphers, is added to specify the cipher list. The values are modern|intermediate|old, or a ":" separated list of ciphers from "man 1 ciphers" Option --ciphers creates an environment variable, ROUTER_CIPHERS, which is passed to the router pod. ---------------------- General cleanup: "oadm router" changed to "oc adm router" Code changes are in: Openshift/origin PR 14505 openshift/origin#14505 Trello oeP7vrTZ https://trello.com/c/oeP7vrTZ/285-3-allow-modification-of-haproxys-ssl-cipher-preference-ingress
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The user can select from among 3 predefined cipher lists: modern,
intermediate, or old. Alternatively the use may provide a custom
cipher list see "openssl ciphers". The list is used to negotiate a
cipher between a user and haproxyi during bind.
The predefined lists are from:
https://wiki.mozilla.org/Security/Server_Side_TLS
A new option to "oc adm router", --ciphers, is added to specify
the cipher list. The values are modern|intermediate|old, or a
":" separated list of ciphers from "man 1 ciphers"
Option --ciphers creates an environment variable, ROUTER_CIPHERS,
which is passed to the router pod.
See https://trello.com/c/oeP7vrTZ
openshift/openshift-docs PR 4587
bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1465572