pkg/crypto: Add missing cipher suites to OpenSSL-to-IANA ciphers mapping

[damdo]

Summary

Expands the openSSLToIANACiphersMap in pkg/crypto/crypto.go
to include additional TLS cipher suites that were previously missing.
This enables the OpenSSLToIANACipherSuites() function to translate
a broader set of OpenSSL cipher names to their IANA equivalents.

Why

The existing map was missing several commonly used cipher suites,
particularly DHE (Diffie-Hellman Ephemeral) variants.
This caused OpenSSLToIANACipherSuites() to drop these ciphers
when translating cipher lists from OpenSSL format to IANA format.
Causing operators using this pkg to log these suites as unsupported.

[damdo]

/assign @richardsonnick @joelanford @rvanderp3 @sanchezl @mrunalp @rhmdnd

/hold

For consensus

[damdo]

/cc @JoelSpeed

[damdo]

/assign @p0lyn0mial

[joelanford]

FWIW, it definitely seems like every known cipher suite should have a mapping, so I'm +1 on adding the missing mappings.

[p0lyn0mial]

@damdo is this pr still relevant ?

[damdo]

It is still relevant yes, I need to look into the test failures..

[damdo]

@p0lyn0mial test are passing now, this is ready for review. Thanks!

[damdo]

/unhold

[p0lyn0mial]

OK, it looks like most of the added ciphers also appear in the TLSProfiles (Old, Intermediate, Modern), so these should be added to the mapping.

[p0lyn0mial]

However, I cannot find this particular cipher. Could you explain why we would like to add it? I think this cipher is generally considered weak.

[damdo]

Good question.

The reason I'm adding these is to allow a translation between one format (OpenSSL) and the other (IANA).

The ciphers I put here do not imply these are safe or not weak, it just means that whomever uses crypto.OpenSSLToIANACipherSuites() is able to translate as many formats as possible from one domain to the other.

I did a scan of both formats and added all the ciphers that are in the intersection of the two.

IMO it shouldn't be the responsibility of the OpenSSLToIANACipherSuites() translation function to decide which ciphers are vulnerable and which ones are not, but it should be at the specific profile level.

This is also specified in the godoc signature of this function, which says // Unknown ciphers are left out.

If we don't want users to be able to specify a certain ciphers in the Custom profile, we should block them at the API level, or error at some equivalent level.

[p0lyn0mial]

ok, thanks, this makes sense.

[p0lyn0mial]

Also, does it make sense to add a new unit test (if we don’t already have one) to verify that all cipher suites defined in the LSProfiles have a corresponding mapping in openSSLToIANACiphersMap?

[damdo]

Also, does it make sense to add a new unit test (if we don’t already have one) to verify that all cipher suites defined in the LSProfiles have a corresponding mapping in openSSLToIANACiphersMap?

This is a good point! We can add that so we don't regress on not being able to translate them if they change in the API spec/type.

[damdo]

Also, does it make sense to add a new unit test (if we don’t already have one) to verify that all cipher suites defined in the LSProfiles have a corresponding mapping in openSSLToIANACiphersMap?

This is a good point! We can add that so we don't regress on not being able to translate them if they change in the API spec/type.

Pushed a new test for this.

[p0lyn0mial]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damdo, p0lyn0mial

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [p0lyn0mial]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@damdo: all tests passed!

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

[ardaguclu]

@damdo When operators are bumped with latest library-go (openshift/cluster-kube-apiserver-operator#2027), CI jobs fail during cluster bootstrap. I think, changes in here create discrepancy between the operator and oauth-server that causes the failure (ref: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-authentication-operator/834/pull-ci-openshift-cluster-authentication-operator-master-e2e-agnostic/2019289972250513408/artifacts/e2e-agnostic/gather-extra/artifacts/pods/openshift-authentication_oauth-openshift-58ff9bf4db-bsngh_oauth-openshift.log).

Currently, library-go can not be importable. We may need to revert this change to make oauth-server compatible first and merge this again.

[ardaguclu]

I've opened revert PR #2118

[damdo]

@damdo When operators are bumped with latest library-go (openshift/cluster-kube-apiserver-operator#2027), CI jobs fail during cluster bootstrap. I think, changes in here create discrepancy between the operator and oauth-server that causes the failure (ref: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-authentication-operator/834/pull-ci-openshift-cluster-authentication-operator-master-e2e-agnostic/2019289972250513408/artifacts/e2e-agnostic/gather-extra/artifacts/pods/openshift-authentication_oauth-openshift-58ff9bf4db-bsngh_oauth-openshift.log).

Currently, library-go can not be importable. We may need to revert this change to make oauth-server compatible first and merge this again.

@ardaguclu Thanks for the details here. I'm going to open a draft revert of the revert so we don't forget about this.
Do you have a bug to track this issue for the KAS operator? So we know when this is resolved and we can reintroduce the change?

Thanks!

Update opened the draft un-revert here: #2119

[ardaguclu]

@damdo cluster-kube-apiserver-operator is not the only one experiencing the issue, cluster-authentication-operator and cluster-openshift-apiserver-operator too.
I think you can test your PR by opening a fake bump in one of these repositories. This is an example openshift/cluster-authentication-operator#832 how to do that. Unfortunately I don't have a Jira bug for this.

[damdo]

I see, thanks @ardaguclu it sounds like those operators have similar logic in how they treat unknown ciphers.

It feels to me that we should treat it as a bug in those operators as these are actually the ciphers specified in the profiles defined at the APIServer level, which are part of the desired configuration for operators secure serving. (cc. @joelanford as the PQC Staff Eng. for awareness)

If/When you have the bug (or bugs if you are creating more) for these components, could you please post it here, so we can track it? :)

Thanks!

[ardaguclu]

This is kubernetes limitation. You can only add the cipher suites defined in https://github.com/kubernetes/kubernetes/blob/dc1ec1211e4f54064ba6dafd8aac46ac3d4379b4/staging/src/k8s.io/component-base/cli/flag/ciphersuites_flag.go#L34-L43 kubernetes. API Server won't allow the others.

[damdo]

Ack thanks for the context. Please do keep us posted on how it goes, ty.

[ardaguclu]

Ack thanks for the context. Please do keep us posted on how it goes, ty.

Actually I'm done with respect to this issue :). Aggregated API servers use the same logic of kube-apiserver. So I'm not sure what can be done.

[damdo]

Ack thanks for the context. Please do keep us posted on how it goes, ty.

Actually I'm done with respect to this issue :). Aggregated API servers use the same logic of kube-apiserver. So I'm not sure what can be done.

@ardaguclu If there are unsupported ciphers by the APIServer then I feel like there should probably be a point raised to either drop the cipher from the centralized APIServer TLS profiles here: https://github.com/openshift/api/blob/81371d13d1fcad175a48627cf11524a94a80c377/config/v1/types_tlssecurityprofile.go#L257
or make a point upstream to update the kube-apiserver to play well with these.

As I mentioned above

this PR is just directed at extending the mapping to allow a wider number of translations between one format (OpenSSL) and the other (IANA).
The ciphers listed in the mapping do not imply these are safe or not weak, it just means that whomever uses crypto.OpenSSLToIANACipherSuites() is able to translate as many formats as possible from one domain to the other.
IMO it shouldn't be the responsibility of the OpenSSLToIANACipherSuites() translation function to decide which ciphers are vulnerable and which ones are not, but it should be at the specific profile level.

So without this PR we are just hiding the problem by not translating "unknown" ciphers, but we aren't really fully honoring the centralized TLS configuration with respect to ciphers as we are only accepting a subset of those, for example, defined in the Intermediate and Old profiles.

Hence my suggestion to follow up on this, as I think it is our priority to try and fully honor the centralized TLS configuration.

Thanks!

[ardaguclu]

@damdo I agree with you. This needs to be fixed by either modifying in upstream or openshift/api. I assume that there is a Jira epic and dedicated team in control plane group for this?

[damdo]

👍 @ardaguclu Let's bring this up in the TLS PQC forum channel. I'll create a thread there.

--
Edit: thread is here