Allow ECDSA for ACME client keys

Lego uses ec256 by default, which I hadn't realized previously, so when I asked Let's Encrypt for a bump on limits on an account, it was using ECDSA. The problem is I can't use this account in cert-manager, since it's explicitly rejected.

golang/x/crypto already accepts EDCSA anyway, so the limitation is solely in cert-manager.

https://pkg.go.dev/golang.org/x/crypto/acme#Client https://github.com/go-acme/lego/blob/0e614c1d0bd56884d6e4c8a05f97e4e653bf8db8/cmd/flags.go#L49

Pull Request Motivation

As in the commit message, I have an account with increased limits that is tied to an ECDSA key. I also think it'd be pretty nice to support this anyway

Kind

feature

Issue report

https://github.com/cert-manager/cert-manager/issues/5357

Release Note

 * ECDSA support for ACME accounts
     - We now support ECDSA created keys in privateKeySecretRef for existing ACME accounts in Issuer/ClusterIssuers 

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jmleddy To complete the pull request process, please assign wallrj after the PR has been reviewed. You can assign the PR to them by writing /assign @wallrj in a comment when ready.

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

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

/ok-to-test

@jmleddy: The following test 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	817ad4f06e802981353079b897f4dbd15005c5d1	link	true	/test pull-cert-manager-master-make-test

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.

So taking a look here, it seems this adds support for cert-manager to read ECDSA keys for ACME accounts, but it doesn't teach cert-manager how to generate them (nor does it give the user an option to configure what kind should be generated).

I think we should also include an algorithm field in the ACMEIssuer spec for this, as we may run into weird states in future if we merge this PR and then later add that (for example, if you start using this with an EC key and we then add an algorithm field (which would default to RSA for backward compatibility), what would cert-manager do if it finds an EC key?

We'd have a few choices:

1. refuse to continue to operate (which in your instance would cause renewals to stop!)
2. regenerate the key so it is RSA, aka creating a new account
3. continue anyway and throw a warning

The issue with (3) is that if a user does want to switch from say, RSA to ECDSA, we could actually implement (1) or (2) in a safe way (as we currently know all keys referenced by Issuers in cert-manager are RSA, so this migration is safe).

So to summarise, I think we should add a corresponding algorithm field with this, and either go for option (1) or (2). I am leaning towards option (1) as this avoids people accidentally deleting their account keys (which in some cases could be really problematic)

I'll get to work on generating the keys as well. In that case we would need algorithm field to support generating ECDSA keys.

what would cert-manager do if it finds an EC key?

In our case we are using disableAccountKeyGeneration so there isn't this issue of regenerating keys. As long as the key exists (say from a prior registration) and we don't specify algorithm, I think we should use the key that exists and only ensure it is either RSA or ECDSA as this code does. If you do specify algorithm, you probably know enough to be aware that this causes new account creation, so I don't think it is that big of an issue and so I favor option #2. I also see a level of operational pain for people deleting k8s secrets in all their clusters that want to switch keys if we implement #1. We can also document how this works and warn that changing algorithms triggers key and thus account creation.

I think this feature would be far safer if it didn't include deletion of account keys, given how critical they are to maintain. For that reason I think no matter what configuration people have, to move from RSA->ECDSA (or vice versa) should require an explicit deletion of the Secret (or if this isn't favourable, the user can also change the secretName in the same update call).

The question of whether we permit mismatching keys is influenced by 1) how we handle changes to the algorithm field (i.e. how we inform the user of the change so they know to take action) and 2) whether disableAccountKeyGeneration is enabled.

Ideally, having the algorithm field default to RSA makes it clear to our users what algorithm they are using when they run kubectl get/describe. The downside of this is it means we can't distinguish 'not set' from the default value, which means we can't know if the user hasn't set the field (and therefore relax validation).

We could relax the validation if disableAccountKeyGeneration is set to true, though it may be confusing when they get/describe their issuer and see algorithm is RSA despite their key actually being ECDSA (this could also be problematic from a security standpoint, as people might expect this validation to be in place and rely on it for security/audit purposes).

Given the current state of our users is that nobody can have an EC key at all, it seems simpler to opt for the stricter approach today - those who do want to switch to EC keys can do so by updating the algorithm field (and deleting their existing secret/changing the secretName). This is operationally simple and safe for new users, and doesn't have any potentially nasty surprises built in (for example deleting an existing account key, or not realising they are using an RSA key when their algorithm is set to EC).

Thanks for the discussion on this 😄

/kind feature

Sorry I've been unable to work on this in the past month. I've added an algorithm field so you can generate EC keys if you like, and validation that whatever key does in fact match that algorithm if it exists. Thus if people want to switch to using an account with an EC key after having already generated an RSA key, they will have to also specify privateSecretKeyRef. For our case it is not too much trouble to add the account: ec config for our existing account.

/retest

@munnerz does this make more sense to you?

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jmleddy To complete the pull request process, please assign maelvls after the PR has been reviewed. You can assign the PR to them by writing /assign @maelvls in a comment when ready.

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

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

@jmleddy Are you still planning to work on this one? Would you perhaps like assistance? If so I could open a PR building on your branch and get it rebased/cleaned up.

Are you still planning to work on this one? Would you perhaps like assistance? If so I could open a PR building on your branch and get it rebased/cleaned up.

If you'd be happy to open a PR to build on this please do feel free and tag me as a reviewer! This has been stale for long enough that it might be worth doing.

@cblecker @SgtCoDFish thank you for your comments I was out on leave in February and missed the initial review in December. I can get started on rebasing and addressing your comments if you haven't already added this feature in the intervening time.

I can get started on rebasing and addressing your comments if you haven't already added this feature in the intervening time.

If you'd be able to do that it'd be awesome, thanks! I've got some leave coming up so if you do make changes I'd encourage you to post in #cert-manager-dev on Kubernetes slack to see if anyone else could take a look!

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Send feedback to jetstack. /lifecycle stale

@jmleddy: 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-e2e-v1-27-upgrade	8473917bd44a8a616251fb19214f74a7486eca51	link	true	/test pull-cert-manager-master-e2e-v1-27-upgrade
pull-cert-manager-master-e2e-v1-27	8473917bd44a8a616251fb19214f74a7486eca51	link	true	/test pull-cert-manager-master-e2e-v1-27
pull-cert-manager-master-e2e-v1-28-upgrade	8473917bd44a8a616251fb19214f74a7486eca51	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.

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close. Send feedback to jetstack. /lifecycle rotten /remove-lifecycle stale

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten. Send feedback to jetstack. /close

@jetstack-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten. Send feedback to jetstack. /close

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.