Support custom ACME account key type. by ldlb9527 · Pull Request #7646 · cert-manager/cert-manager

ldlb9527 · 2025-03-30T19:13:06Z

Pull Request Motivation

Kind

/kind feature

Release Note

Add a new parameter `keyType` to support specifying the key type for ACME account.

cert-manager-prow · 2025-03-30T19:13:10Z

[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 jakexks for approval. For more information see the Code Review Process.

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

Details

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

cert-manager-prow · 2025-03-30T19:13:16Z

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

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.

Catman100 · 2025-05-18T19:50:13Z

Hello @ldlb9527 can you please add the Release Note Block to your MR?
Best regards

wallrj · 2025-06-03T21:21:28Z

/ok-to-test

Copilot

Pull Request Overview

This PR adds support for a custom ACME account key type by introducing a new parameter “keyType” and updating the codebase to use the generic crypto.Signer interface rather than a specific RSA implementation. It also updates tests, API type definitions, and CRDs to reflect this change.

Updated ACME key generation with key type support and error handling improvements.
Modified function signatures and type conversions to use crypto.Signer across the codebase.
Adjusted API conversions and CRD definitions to include the new keyType field.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/util/pki/generate.go	Introduced MarshalPrivateKey using crypto.Signer
pkg/issuer/acme/setup_test.go	Updated test mocks to use crypto.Signer instead of *rsa.PrivateKey
pkg/issuer/acme/setup.go	Adjusted key creation and client registration to use crypto.Signer
pkg/apis/acme/v1/types_issuer.go	Added keyType field to ACMEIssuer schema
pkg/acme/accounts/test/registry.go	Updated FakeRegistry function signatures for crypto.Signer
pkg/acme/accounts/registry.go	Refactored client registry to support crypto.Signer in stableOptions
pkg/acme/accounts/client.go	Updated client creation function signature for crypto.Signer
internal/apis/acme/v1/zz_generated.conversion.go	Included keyType conversion
internal/apis/acme/types_issuer.go	Added keyType field in internal ACMEIssuer type
deploy/crds/crd-issuers.yaml	Documented new keyType field in issuer CRD
deploy/crds/crd-clusterissuers.yaml	Documented new keyType field in cluster issuer CRD

Copilot · 2025-06-03T21:21:56Z

+func MarshalPrivateKey(privateKey crypto.Signer) []byte {
+	if privateKey == nil {
+		return nil
+	}
+	var privateKeyBytes []byte
+	switch privateKey.(type) {
+	case *rsa.PrivateKey:
+		privateKeyBytes = x509.MarshalPKCS1PrivateKey(privateKey.(*rsa.PrivateKey))
+	case *ecdsa.PrivateKey:
+		privateKeyBytes, _ = x509.MarshalECPrivateKey(privateKey.(*ecdsa.PrivateKey))
+	default:
+		return nil
+	}
+	return privateKeyBytes


Consider handling the error returned by x509.MarshalECPrivateKey rather than discarding it to avoid silent failures during EC private key marshalling.

Suggested change

func MarshalPrivateKey(privateKey crypto.Signer) []byte {

if privateKey == nil {

return nil

}

var privateKeyBytes []byte

switch privateKey.(type) {

case *rsa.PrivateKey:

privateKeyBytes = x509.MarshalPKCS1PrivateKey(privateKey.(*rsa.PrivateKey))

case *ecdsa.PrivateKey:

privateKeyBytes, _ = x509.MarshalECPrivateKey(privateKey.(*ecdsa.PrivateKey))

default:

return nil

}

return privateKeyBytes

func MarshalPrivateKey(privateKey crypto.Signer) ([]byte, error) {

if privateKey == nil {

return nil, fmt.Errorf("private key is nil")

}

var privateKeyBytes []byte

var err error

switch privateKey.(type) {

case *rsa.PrivateKey:

privateKeyBytes = x509.MarshalPKCS1PrivateKey(privateKey.(*rsa.PrivateKey))

case *ecdsa.PrivateKey:

privateKeyBytes, err = x509.MarshalECPrivateKey(privateKey.(*ecdsa.PrivateKey))

if err != nil {

return nil, fmt.Errorf("failed to marshal ECDSA private key: %w", err)

}

default:

return nil, fmt.Errorf("unsupported private key type: %T", privateKey)

}

return privateKeyBytes, nil

Copilot · 2025-06-03T21:21:56Z

+		exponent = 0
+
+	default:
+		return stableOptions{}


In newStableOptions, the default case silently returns an empty stableOptions. Consider explicitly handling unsupported crypto.Signer types, for example by returning an error or logging a warning, to make the failure mode clearer.

Suggested change

return stableOptions{}

return stableOptions{}, errors.New("unsupported crypto.Signer type")

ldlb9527 · 2025-06-04T03:13:10Z

I will try to handle failed jobs. Please let me know if you find any problems with the code.

Catman100 · 2025-07-20T08:30:21Z

Hello @ldlb9527,
is there a status update for the Issue?

Signed-off-by: xiong.chen <1249843194@qq.com>

ldlb9527 · 2025-07-22T16:42:51Z

Hello @Catman100 , I have rebased my changes on the latest code.

Catman100 · 2025-10-11T17:57:55Z

Hi, is there any change? Greetings

ldlb9527 · 2025-10-13T03:42:03Z

Hi @wallrj ,

I see @Catman100 has been following up. Could you confirm if this feature (custom ACME account key type) is something the team wants to support?

Catman100 · 2025-12-14T22:09:58Z

@inteon @erikgb can you check this Request please. Thanks for your Time

cert-manager-prow · 2025-12-23T18:54:22Z

PR needs rebase.

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.

cert-manager-prow · 2025-12-30T08:34:22Z

@ldlb9527: 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-e2e-v1-35-upgrade	`097829c`	link	true	`/test pull-cert-manager-master-e2e-v1-35-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.

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.

marie-j · 2026-01-20T11:03:56Z

Hello @ldlb9527

Do you need a hand on this topic ? I am willing to help you on this matter to get this contribution moved forward if needed

cert-manager-prow Bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/acme Indicates a PR directly modifies the ACME Issuer code labels Mar 30, 2025

cert-manager-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 30, 2025

wallrj requested a review from Copilot June 3, 2025 21:21

cert-manager-prow 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 Jun 3, 2025

Copilot AI reviewed Jun 3, 2025

View reviewed changes

cert-manager-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2025

ldlb9527 force-pushed the feat-account-key branch from 6f2b738 to 5167900 Compare June 7, 2025 18:02

ldlb9527 force-pushed the feat-account-key branch 2 times, most recently from 91e3f15 to eaf496e Compare June 7, 2025 18:36

cert-manager-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2025

ldlb9527 force-pushed the feat-account-key branch from eaf496e to bd5136d Compare July 22, 2025 14:51

cert-manager-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2025

ldlb9527 force-pushed the feat-account-key branch from bd5136d to acbbe4f Compare July 22, 2025 15:01

Support custom ACME account key type.

097829c

Signed-off-by: xiong.chen <1249843194@qq.com>

ldlb9527 force-pushed the feat-account-key branch from acbbe4f to 097829c Compare July 22, 2025 16:27

cert-manager-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2025

	return stableOptions{}
	return stableOptions{}, errors.New("unsupported crypto.Signer type")

Conversation

ldlb9527 commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Motivation

Kind

Release Note

Uh oh!

cert-manager-prow Bot commented Mar 30, 2025

Uh oh!

cert-manager-prow Bot commented Mar 30, 2025

Uh oh!

Catman100 commented May 18, 2025

Uh oh!

wallrj commented Jun 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

ldlb9527 commented Jun 4, 2025

Uh oh!

Catman100 commented Jul 20, 2025

Uh oh!

ldlb9527 commented Jul 22, 2025

Uh oh!

Catman100 commented Oct 11, 2025

Uh oh!

ldlb9527 commented Oct 13, 2025

Uh oh!

Catman100 commented Dec 14, 2025

Uh oh!

cert-manager-prow Bot commented Dec 23, 2025

Uh oh!

cert-manager-prow Bot commented Dec 30, 2025

Uh oh!

marie-j commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldlb9527 commented Mar 30, 2025 •

edited

Loading

marie-j commented Jan 20, 2026 •

edited

Loading