Allow to set keystore password in Certificate #6657

rquinio1A · 2024-01-19T15:03:40Z

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

Allow to set JKS/PKCS12 keystore password used for encryption as a literal value in the Certificate. The passwordSecretRef field is now optional, passwordSecretRef and password fields are exclusive. If not password is explicitly set, it defaults to `changeit`. The literal password value is stored inside the generated certificate secret, under the secret key `keystorePassword`.

Signed-off-by: Romain QUINIO <romain.quinio@amadeus.com>

jetstack-bot · 2024-01-19T15:03:51Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

jetstack-bot · 2024-01-19T15:04:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sgtcodfish for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

erikgb

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.....

deploy/crds/crd-certificates.yaml

pkg/apis/certmanager/v1/types_certificate.go

erikgb · 2024-01-21T16:39:06Z

pkg/controller/certificates/issuing/internal/secret.go

+		} 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")


I think this should be a user-facing error, ref. the comment about webhook validation above.

Pheraps if no password is provided you can set as default to 'changeit'
same as keytool

+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.

it's also the default used here: https://github.com/SSLMate/go-pkcs12/blob/master/pkcs12.go#L37

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).

@erikgb one could argue that supporting more inputs is not breaking behavior.
eg. not failing when no password is provided.

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) ?

pkg/controller/certificates/issuing/internal/secret.go

rquinio1A · 2024-01-24T21:48:45Z

Following review comments, I've pushed a 2nd commit to:

add webhook validation that fields are exclusive
put the password in secret only if hardcoded
use a password default value (changeit)

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>

erikgb · 2024-01-25T06:36:54Z

/ok-to-test

jetstack-bot · 2024-01-25T06:41:04Z

@rquinio1A: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cert-manager-master-make-test	`84c0f47`	link	true	`/test pull-cert-manager-master-make-test`
pull-cert-manager-master-e2e-v1-28	`84c0f47`	link	true	`/test pull-cert-manager-master-e2e-v1-28`
pull-cert-manager-master-e2e-v1-28-upgrade	`84c0f47`	link	true	`/test pull-cert-manager-master-e2e-v1-28-upgrade`

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.

SgtCoDFish · 2024-01-25T11:19:12Z

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!

inteon · 2024-02-05T13:30:23Z

internal/apis/certmanager/validation/certificate.go

+		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."))
+		}
+	}


@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.

@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

@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.

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).

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.

@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.

fadecore · 2024-02-06T17:16:24Z

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.
As a result all the containers only have to mount one secret to have needed information.
If we wanted to automate these steps and use cert-manager we would need a CR at vendor side two support keystore and password to be in different secrets. :(
Looking forward for the upcoming releases and the option to have the possibility to choose which way to go :)
Thanks to everyone for contributing!

jetstack-bot · 2024-03-08T03:29:03Z

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.

schedin · 2024-04-23T14:03:28Z

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)?

cert-manager-prow · 2024-05-14T18:02:17Z

@rquinio1A: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cert-manager-master-make-verify	`84c0f47`	link	true	`/test pull-cert-manager-master-make-verify`
pull-cert-manager-master-e2e-v1-30-upgrade	`84c0f47`	link	true	`/test pull-cert-manager-master-e2e-v1-30-upgrade`
pull-cert-manager-master-e2e-v1-30	`84c0f47`	link	true	`/test pull-cert-manager-master-e2e-v1-30`

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.

Allow to set keystore password in Certificate

774e4f3

Signed-off-by: Romain QUINIO <romain.quinio@amadeus.com>

jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/api Indicates a PR directly modifies the 'pkg/apis' directory labels Jan 19, 2024

jetstack-bot added area/deploy Indicates a PR modifies deployment configuration area/testing Issues relating to testing labels Jan 19, 2024

rquinio1A mentioned this pull request Jan 19, 2024

Allow hardcoded JKS and PKCS#12 passwords #6269

Open

erikgb suggested changes Jan 21, 2024

View reviewed changes

Validate keystore cr and default password

84c0f47

Signed-off-by: Romain QUINIO <romain.quinio@amadeus.com>

rquinio1A force-pushed the feature/keystore-password-litteral branch from e83115e to 84c0f47 Compare January 24, 2024 21:53

jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 25, 2024

inteon reviewed Feb 5, 2024

View reviewed changes

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to set keystore password in Certificate #6657

Allow to set keystore password in Certificate #6657

rquinio1A commented Jan 19, 2024 •

edited

jetstack-bot commented Jan 19, 2024

jetstack-bot commented Jan 19, 2024

erikgb left a comment

erikgb Jan 21, 2024

YvesZelros Jan 24, 2024 •

edited

rquinio1A Jan 24, 2024

rquinio1A Jan 24, 2024

erikgb Jan 25, 2024 •

edited

inteon Jan 25, 2024

rquinio1A Jan 25, 2024

rquinio1A commented Jan 24, 2024

erikgb commented Jan 25, 2024

jetstack-bot commented Jan 25, 2024

SgtCoDFish commented Jan 25, 2024

inteon Feb 5, 2024 •

edited

YvesZelros Feb 5, 2024 •

edited

erikgb Feb 5, 2024

inteon Feb 5, 2024 •

edited

inteon Feb 5, 2024 •

edited

YvesZelros Feb 5, 2024 •

edited

fadecore commented Feb 6, 2024

jetstack-bot commented Mar 8, 2024

schedin commented Apr 23, 2024

cert-manager-prow bot commented May 14, 2024

Allow to set keystore password in Certificate #6657

Are you sure you want to change the base?

Allow to set keystore password in Certificate #6657

Conversation

rquinio1A commented Jan 19, 2024 • edited

Pull Request Motivation

Kind

Release Note

jetstack-bot commented Jan 19, 2024

jetstack-bot commented Jan 19, 2024

erikgb left a comment

Choose a reason for hiding this comment

erikgb Jan 21, 2024

Choose a reason for hiding this comment

YvesZelros Jan 24, 2024 • edited

Choose a reason for hiding this comment

rquinio1A Jan 24, 2024

Choose a reason for hiding this comment

rquinio1A Jan 24, 2024

Choose a reason for hiding this comment

erikgb Jan 25, 2024 • edited

Choose a reason for hiding this comment

inteon Jan 25, 2024

Choose a reason for hiding this comment

rquinio1A Jan 25, 2024

Choose a reason for hiding this comment

rquinio1A commented Jan 24, 2024

erikgb commented Jan 25, 2024

jetstack-bot commented Jan 25, 2024

SgtCoDFish commented Jan 25, 2024

inteon Feb 5, 2024 • edited

Choose a reason for hiding this comment

YvesZelros Feb 5, 2024 • edited

Choose a reason for hiding this comment

erikgb Feb 5, 2024

Choose a reason for hiding this comment

inteon Feb 5, 2024 • edited

Choose a reason for hiding this comment

inteon Feb 5, 2024 • edited

Choose a reason for hiding this comment

YvesZelros Feb 5, 2024 • edited

Choose a reason for hiding this comment

fadecore commented Feb 6, 2024

jetstack-bot commented Mar 8, 2024

schedin commented Apr 23, 2024

cert-manager-prow bot commented May 14, 2024

rquinio1A commented Jan 19, 2024 •

edited

YvesZelros Jan 24, 2024 •

edited

erikgb Jan 25, 2024 •

edited

inteon Feb 5, 2024 •

edited

YvesZelros Feb 5, 2024 •

edited

inteon Feb 5, 2024 •

edited

inteon Feb 5, 2024 •

edited

YvesZelros Feb 5, 2024 •

edited