Bug 1970147: jsonnet: disable insecure cypher suites for prometheus-adapter #1234
Conversation
openshift-ci
bot
added
the
approved
fpetkovski
changed the title
|
/bugzilla refresh |
|
@fpetkovski: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
fpetkovski
changed the title
openshift-ci
bot
added
bugzilla/severity-low
|
@fpetkovski: This pull request references Bugzilla bug 1970147, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/bugzilla refresh |
|
@fpetkovski: This pull request references Bugzilla bug 1970147, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
bugzilla/valid-bug
jsonnet/prometheus-adapter.libsonnet
Outdated
| @@ -94,6 +94,7 @@ function(params) | |||
| '--metrics-relist-interval=1m', | |||
| '--prometheus-url=' + cfg.prometheusURL, | |||
| '--secure-port=6443', | |||
| '--tls-cipher-suites=TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,TLS_RSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_AES_256_GCM_SHA384,TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA', | |||
There was a problem hiding this comment.
We should have one set of ciphersuites for all components and not store them in different places.
This list should either be taken from any kube-rbac-proxy sidecar settings. Or it should be configured in main.jsonnet and propagated everywhere from there.
/hold
There was a problem hiding this comment.
note: kube-rbac-proxy component allows easy configuration of ciphersuites in https://github.com/prometheus-operator/kube-prometheus/blob/main/jsonnet/kube-prometheus/components/kube-rbac-proxy.libsonnet#L11-L38. We might want to surface that in components that use KRP.
There was a problem hiding this comment.
We already have an epic for 4.10 to configure TLS settings from a central OpenShift API, so I wouldn't try to solve that problem now. I think through that epic we will remove the tls ciphers arg completely since it will be managed dynamically by CMO.
I changed the PR to take the ciphers from the thanos querier: https://github.com/openshift/cluster-monitoring-operator/blob/master/jsonnet/thanos-querier.libsonnet#L450
There was a problem hiding this comment.
We already have an epic for 4.10 to configure TLS settings from a central OpenShift API, so I wouldn't try to solve that problem now.
This is a different problem. Applying any configuration from a central API is a runtime issue, jsonnet code and manifest generation is done before runtime.
I changed the PR to take the ciphers from the thanos querier
This is taking (copy-pasting) current ciphers and do not allow any simple changes. As such it is a stop-gap and not a proper solution.
There was a problem hiding this comment.
I am not sure if this is really necessary. I would expect the static tls-cipher-suites to be removed as an argument from static jsonnet files once we move from configuring them at runtime. I would also be mindful of the scope of the Bugzilla this PR addresses.
In any case, I've added a variable in main.jsonnet and propagated it downstream to all components.
There was a problem hiding this comment.
I would expect the static tls-cipher-suites to be removed as an argument from static jsonnet files once we move from configuring them at runtime.
Those assumptions may be valid for 4.10 and we are in 4.9 development cycle. This OpenShift version will need to be supported for ~1 year or more. During that time there is no guarantee we won't need to change cipher suites for all components and if that is the case I would prefer to change it in one place instead of searching other implementations.
There was a problem hiding this comment.
Ah ok that makes sense. I didn't take into account the longer support cycle 👍
openshift-ci
bot
added
the
do-not-merge/hold
fpetkovski
force-pushed
the
prometheus-adapter-ciphers
branch
2 times, most recently
from
4b051cc
to
4a768f2
Compare
Running sslscan against the prometheus adapter secure port reports two insecure SSL ciphers, ECDHE-RSA-DES-CBC3-SHA and DES-CBC3-SHA. This commit removes those ciphers from the list.
fpetkovski
force-pushed
the
prometheus-adapter-ciphers
branch
from
4a768f2
to
0a73e0a
Compare
|
/lgtm |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fpetkovski, jan--f, paulfantom 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@fpetkovski: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@fpetkovski: All pull requests linked via external trackers have merged: Bugzilla bug 1970147 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Running sslscan against the prometheus adapter secure port reports two
insecure SSL ciphers, ECDHE-RSA-DES-CBC3-SHA and DES-CBC3-SHA.
This commit removes those ciphers from the list.
It is not enough to pull in changes from the kube-prometheus PR since we override the prometheus-adapter args completely: