Skip to content
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.

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

Issue #9372 Enhance Kafka Spec with cert algorithm management #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

steffen-karlsson
Copy link
Contributor

No description provided.

Signed-off-by: Steffen Karlsson <[email protected]>
@scholzj
Copy link
Member

scholzj commented May 7, 2024

Thanks for the proposal. I think we need to be careful how does this fit with #116.

CC @katheris

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

I think the API looks reasonable. But I also thing that this is a bit more tricky than just adding the API. I think it would be worth elaborating on the following details:

  • What will be the suported values?
  • How will they be validated?
  • How do you ensure that they are supported by both the OpenSSL (where the certificates are issues) as well as by Java (where the certificates are used)
  • How will this handle FIPS where different algs might be available than in the non-FIPS mode?
  • How will this information be passed to the User Operator?
  • It is also worth thinking about how this would relate to when we (possibly) use tools such as Cert Manager in the future

@steffen-karlsson
Copy link
Contributor Author

I think the API looks reasonable. But I also thing that this is a bit more tricky than just adding the API. I think it would be worth elaborating on the following details:

  • What will be the suported values?
  • How will they be validated?
  • How do you ensure that they are supported by both the OpenSSL (where the certificates are issues) as well as by Java (where the certificates are used)
  • How will this handle FIPS where different algs might be available than in the non-FIPS mode?
  • How will this information be passed to the User Operator?
  • It is also worth thinking about how this would relate to when we (possibly) use tools such as Cert Manager in the future

Sure that makes sense, let me do an FAQ and answer some of these concerns in the proposal it self.

@katheris
Copy link
Contributor

I agree with Jakub that some more details in the PR are needed. The original issue links here for where it is "hardcoded", but that is actually just system tests. Implementing this will be more involved than just changing those few lines of code, as the use of RSA is assumed in a few places. E.g. in several methods in the OpenSslCertManager class. As a result I suspect it might be challenging to let the user specify whatever they like, so having a set list of supported values makes more sense to me.

Looking at how this fits with future plans to make use of Cert Manager, you can specify the key algorithm and size when requesting certificates, so I think that's fine. The tricky part is the signature hashing algorithm because as far as I can tell it cannot be specified when using Cert Manager, instead it looks like it gets determined by the key size and algorithm cert-manager/cert-manager#3040

@katheris
Copy link
Contributor

Hey @steffen-karlsson did you have any time to think about what the supported values would be and Jakub's other questions?

Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal. It would be nice to give users this level of control, but I think we need to define the API and behaviour in a little more detail.

Comment on lines +17 to +19
* `keyAlgorithm`: The algorithm for generating the private key.
* `keySize`: The size of the private key generated.
* `signatureAlgorithm`: The hashing algorithm used for signing.
Copy link
Member

Choose a reason for hiding this comment

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

We need more detail about these:

*Are they validated via the CRD Schema, or the Cluster Operator itself? Or do we just pass them to openssl and treat algorithms that are unknown to it as any other openssl failure?

  • Are we going to document some key algorithms which we guarantee will be available? How might we, in the future, go about deprecating and removing support for some combination of these parameters?
  • Documenting allowed key sizes is tricky because they depend on the key algorithm.
  • Do the same settings apply for both CA and end-entity certificates?
  • Are these settings only use for new certificates, or does changing them imply that certificates need to be immediately regenerated?

@im-konge
Copy link
Member

Triaged on 27.6.2024: @steffen-karlsson are you waiting for Kate's proposal to proceed? Did you have time to look at the comments from Kate and Jakub?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants