-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Allow to set keystore password in Certificate #6657
base: master
Are you sure you want to change the base?
Allow to set keystore password in Certificate #6657
Conversation
Signed-off-by: Romain QUINIO <romain.quinio@amadeus.com>
Hi @rquinio1A. Thanks for your PR. I'm waiting for a cert-manager 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. 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, @rquinio1A! I did an initial review, and I think there are a few things that needs attention:
- Webhook validation of literal password vs. password from secret.
- Decide if we need a feature flag for this new feature, or at least part of it. I am pretty sure some users might not like that we put the keystore password just next to the keystore in the certificate secret.....
} else if crt.Spec.Keystores.PKCS12.Password != nil { | ||
pw = []byte(*crt.Spec.Keystores.PKCS12.Password) | ||
} else { | ||
return fmt.Errorf("either PasswordSecretRef or Password is mandatory") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a user-facing error, ref. the comment about webhook validation above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pheraps if no password is provided you can set as default to 'changeit'
same as keytool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, having a default password would make the config even simpler, and matches the idea is we shouldn't concern the user with keystore encryption given it's not relevant in kubernetes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's also the default used here: https://github.com/SSLMate/go-pkcs12/blob/master/pkcs12.go#L37
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need (or can) set a default password in this case. We should have a validating webhook to validate that either passwordSecretRef
or password
is set. If not, we will change the behavior if/when a user (not aware of this new feature) forgets to set passwordSecretRef
. Cert-manager API is GA (v1) - which means we should in general not do breaking changes.
Also, having a default password on keystores is uncommon. Truststores may have a default password, as the JDK default cacerts
has (changeit
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikgb one could argue that supporting more inputs is not breaking behavior.
eg. not failing when no password is provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing how the CI is failing, I'm not clear if it's even possible to make the mandatory field PasswordSecretRef become optional (cmmeta.SecretKeySelector -> *cmmeta.SecretKeySelector) ?
Following review comments, I've pushed a 2nd commit to:
With PasswordSecretRef becoming optional and a pointer, I had to manually change zz_generated.conversion.go, as I couldn't get the re-generation script to work. |
Signed-off-by: Romain QUINIO <romain.quinio@amadeus.com>
e83115e
to
84c0f47
Compare
/ok-to-test |
@rquinio1A: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
Hey, @erikgb just pointed me at this - thanks for raising this, it's great to see 😁 I don't have time this week to review anything, but I just wanted to add my support for this. We discussed this a bit in the cert-manager open source standup on 2024-01-25 and the consensus was to avoid setting a default value, and therefore require the user to give either a secretRef or an explicitly defined password. We can add a default later down the road without a problem if we want! |
if crt.Keystores.PKCS12.Password != nil && crt.Keystores.PKCS12.PasswordSecretRef != nil { | ||
el = append(el, field.Invalid(fldPath.Child("keystores", "pkcs12", "password"), crt.Keystores.PKCS12.Password, "When providing a `PasswordSecretRef` no `Password` properties may be provided.")) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rquinio1A
Could you add a check here to make sure that Password
OR PasswordSecretRef
is set?
As @SgtCoDFish mentioned, we discussed in our standup that we would like to start with a solution without default passwords.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inteon
@SgtCoDFish
CertManager don't provide an option to set password in all kind of certificate except for JKS, right ?
Then why do you want enforce users to set a password only for JKS ?
On your FAQ you write explicitly a section named
Why are passwords on JKS or PKCS#12 files not helpful?
- Why is it OK to hard code these passwords?
- Do I need to keep these passwords secure?
- Are these passwords used in a secure way?
We recommend that you treat these passwords as legacy implementation details, and use short hard-coded strings for these passwords when you're forced to use one. Don't spend time trying to generate or handle "secure" passwords for these files - simply choose a constant such as changeit or notapassword123 and use that for every PKCS#12 or JKS bundle you generate.
You propose to use changeit as password, then I don't see any reason to not set it as default according to your FAQ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YvesZelros There might be users who disagree with the referred statement in our FAQ. We are just suggesting to progress slowly on this change (new feature). Nothing blocks you from setting the password to the literal value changeit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, right now, you have to set a secret reference for both the PKCS12 and JKS target.
This PR would allow you to also just set a static password as a string (instead of the secret reference).
From @SgtCoDFish's comment: "We discussed this a bit in the cert-manager open source standup on 2024-01-25 and the consensus was to avoid setting a default value, and therefore require the user to give either a secretRef or an explicitly defined password. We can add a default later down the road without a problem if we want!"
As explained by @SgtCoDFish, we were thinking that we could start without default and add a default later if we can agree on a good default value (one for PKCS12 and one for JKS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity my original PR review is not about just JKS. I would like us to remove the default for PKCS12 from this PR too, requiring a password to be provided for both JKS and PKCS12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikgb People that disagree statement on your FAQ can still define a password ;-)
Adding a default value isn't a breaking change ... but you have the final world on this change IT ;-)
I agree that allowing to set it as literal value is a good first step to simplify the deployment of JKS/PKCS12.
I'm currently working with some software of a vendor that states in the manual to manually create a keystores and then create one secret via kubectl that contains both the keystore and the password. |
PR needs rebase. 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. |
Question: does this pull request also implement #6783? Or at least to allow for the empty string as a password (I don't know if empty string and no password is the same in this context)? |
@rquinio1A: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
Pull Request Motivation
The fact of encrypting keystores with a password in Java keytool predates the Kubernetes era.
Since the generated keystore is stored in a Kubernetes secret, the fact of encrypting it has become irrelevant (if you can access secrets, then you can also access the secret containing the encryption password ...)
Even worse, since Kubernetes Secret resources shouldn't be stored in Gitops repositories, this forces to synchronize from a vault, making things very complex for something than doesn't actually need it.
Implements #5665
Implements #6269
Kind
feature
Release Note