CMP-3606: Retire old TLS Cipher Checks and homogenize the active ones - #12749
Conversation
|
Hi @sluetze. Thanks for your PR. I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
|
||
| ocil: |- | ||
| Run the following comman on the kubelete nodes(s): | ||
| {{% raw %}}<pre>oc patch kubeapiservers.operator.openshift.io cluster --type merge -p '{"spec":{"unsupportedConfigOverrides":{"servingInfo":{"cipherSuites":["TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256","TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256","TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305","TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384","TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305","TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384","TLS_RSA_WITH_AES_256_GCM_SHA384","TLS_RSA_WITH_AES_128_GCM_SHA256"]} } } }'</pre>{{% endraw %}} |
There was a problem hiding this comment.
Interesting that in CIS v1.6.0 item 1.2.33, the use of unsupportedConfigOverrides is not recommended anymore.
We don't have a rule for that though.
| status: automated | ||
| rules: | ||
| - kubelet_configure_tls_cipher_suites | ||
| - kubelet_configure_tls_cipher_suites_ingresscontroller |
There was a problem hiding this comment.
This rename and move makes sense to me.
@rhmdnd @Vincent056 thoughts?
126cc75 to
c9231f7
Compare
c9231f7 to
df1ee83
Compare
|
/lgtm Premerge verification steps: Objective: Ensure the old rules (
Run compliance scan and verify remediation gets applied: |
|
@sluetze Please resolve the conflicts and rebase on the latest upstream master branch. |
63176ce to
30ce4ed
Compare
|
Code Climate has analyzed commit 30ce4ed and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 61.9% (0.0% change). View more on Code Climate. |
|
@yuumasato can we get this merged? I will happily rebase and resolve conflicts again, but having done this a couple of times now, I want a "will get merged shortly" commitment beforehand. |
|
@sluetze yes, we can get this merged. |
30ce4ed to
c51dc1a
Compare
|
/lgtm The operator is correctly using the replacement rule (ingress-controller-tls-cipher-suites).
|
Description:
Removing
kubelet_configure_tls_cipher_suites_openshiftapiserver_operatorandkubelet_configure_tls_cipher_suites_kubeapiserver_operatorand renamingkubelet_configure_tls_cipher_suites_ingresscontrollerto create a more concise structurethis is part of a larger effort to make all TLS Cipher Suites and their remediations configurable with variables (see https://issues.redhat.com/browse/RFE-6859 )
Rationale:
The current state of the TLS Cipher Checks is a little bit heterogenous. There are currently 6 of them:
while 1-4 are used in multiple profiles,
kubelet_configure_tls_cipher_suites_openshiftapiserver_operatorandkubelet_configure_tls_cipher_suites_kubeapiserver_operatorare only used in the default profile. they also do not really have anything to do withkubeletand also have remediations which are unsupported or defect. There are nowadays better ways to configure TLS profiles and these are used by the newer rules.Furthermore the
kubelet_configure_tls_cipher_suites_ingresscontrollerdoesnt have anything to do with thekubelet. There are more rules regarding the ingresscontroller undernetworking. I believe this to be an artifact, since you can see, that the newapi_server_tls_cipher_suitesis underapiserver. Movingkubelet_configure_tls_cipher_suites_ingresscontrollertonetworkingbundles the ingresscontroller tls rules together and makes it easier to get a grasp of what is existing.These changes should prevent people creating new profiles to use the wrong/outdated rules and also more easily find relevant rules.
Review Hints:
I checked for occurences of the rules by grepping the repository
I replaced the occurence in
controls/cis_ocp_1_4_0/section-4.ymlwith the new name.
IMHO this rule does not match the requirement, as the ingresscontroller has nothing to do with the kubelet. I would recommend to remove it, as the requirement is addressed by the kubelet rule.
Furthermore I wonder, why this rule is not used in more places.
one could also go one step further and unify the naming of the cipher rules, but I think this is creating no value.