add support for ecdsa keys #28

vinny-sabatini · 2023-06-01T18:11:55Z

This upgrades cert-manager/cert-manager to v1.12.1 in order to use the PrivateKeyAlgorithmAnnotationKey released in v1.9.0.

I believe this would close out #15

maelvls · 2023-06-02T14:16:46Z

Hi. Thanks for the contribution!! This looks great! May I suggest a few other unit tests:

When the annotation cert-manager.io/private-key-algorithm isn't present, it should default to the RSA algorithm (right now, there is no test that doesn't use the annotation).
When the annotation cert-manager.io/private-key-algorithm is present but isn't a valid value, it should show an event on the Route.

I used gocover to discover these missing cases, see coverage.html.

Creating coverage.html for a PR

go install github.com/Azure/gocover@latest
gh pr checkout 28
go test ./... -coverprofile=cover.out                                                     
gocover diff --repository-path=$PWD --cover-profile=cover.out --compare-branch=origin/main

vinny-sabatini · 2023-06-02T14:25:06Z

Thanks for the feedback @maelvls ! I will work on addressing those things today.

vinny-sabatini · 2023-06-05T17:22:04Z

I added the events in 6c628c9 and updated the tests to ensure we are getting the expected private key format in 5790859

I noticed there are no tests for createNextCR. I wasn't sure if you wanted me to add tests for that with this PR since I am changing that method or if just adding test cases to generateNextPrivateKey was sufficient for now. Let me know if you want those tests included in this PR, otherwise, I was hoping to add test cases for that in a separate PR.

vinny-sabatini · 2023-06-12T19:42:08Z

Hi @maelvls I believe I have addressed the requested changes for this PR.
Please give this another review when you have time and let me know if you have any thoughts about my previous comment about tests for createNextCR. Thanks 😄

maelvls · 2023-06-23T17:56:47Z

Hey, sorry for the delay! I will take a look on Monday (it's Friday 19:56 now and I want to go home lol)

Note: I'll be on PTO from Tue 27 June to Sun 23 July. Please ping another maintainer (such as @inteon) if you need a quicker review.

maelvls · 2023-07-31T14:46:45Z

Hey, sorry for the lack of unit tests on createNextCR. 😞

Preferably, we would first write the missing unit tests for createNextCR (without the changes from this PR), merge these new unit tests to master, and then come back to this PR to add one or two unit tests.

Given that you have already spent a lot of time working on this PR, I'd understand if you would prefer merging this PR first and then write the new unit tests in another PR.

What do you think?

Signed-off-by: Vinny Sabatini <[email protected]>

vinny-sabatini · 2023-08-01T22:13:29Z

No problem about the missing tests. I have no opposition to adding additional test coverage for that before this PR is merged.

It looks like #27 added some tests I think I can build on top of which helps a lot. I will work on adding coverage for my changes, making sure the PR checks pass, and then will reach out for a review. Thanks!

Signed-off-by: Vinny Sabatini <[email protected]>

I am not positive what is causing the failed test case at this point, still looking into it Signed-off-by: Vinny Sabatini <[email protected]>

maelvls · 2023-09-15T16:36:43Z

internal/controller/sync_test.go

+				} else if tt.wantCSR.PublicKeyAlgorithm == x509.RSA {
+					privateKey = rsaKey
+				}
+				csr, err := x509.CreateCertificateRequest(rand.Reader, tt.wantCSR, privateKey)


It was working well with the RSA signature algorithm, but comparing the two signatures will fail with ECDSA since the signatures depend on a random number.

It is probably why the test TestRoute_buildNextCR/With_ECDSA_private_key_algorithm_annotation is failing 😅

I imagine that (for the ECDSA test specifically) we can simply check that the "rest" of the CSR is the same without checking the signature

What do you think?

Thanks for the tip @maelvls! It does seem like that is the cause of that test failing. I'll try to update that test accordingly

I think I updated the test to cover the ECDSA use case and the rest of the PR is ready for review whenever you have time @maelvls, thanks again for the suggestion 😄

Signed-off-by: Vinny Sabatini <[email protected]>

maelvls · 2023-10-05T16:59:52Z

Excellent work, thank you so much for contributing this feature!! 🙏 Let's create a new release with you feature on Monday! I think there are a few changes that haven't been released yet too.

/lgtm
/approve

jetstack-bot · 2023-10-05T16:59:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maelvls]

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

jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 1, 2023

vinny-sabatini force-pushed the support-additional-keys branch from 6f6f36e to 362411d Compare June 1, 2023 18:15

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jun 1, 2023

maelvls self-requested a review June 23, 2023 17:57

This was referenced Jul 31, 2023

Additional support for subject annotations #29

Merged

Support Route.spec.subdomain #27

Merged

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2023

vinny-sabatini added 4 commits August 1, 2023 16:13

Upgrade cert-manager/cert-manager to v1.12.1

07e44f2

Signed-off-by: Vinny Sabatini <[email protected]>

add support for ecdsa keys

f2b3fba

Signed-off-by: Vinny Sabatini <[email protected]>

emit event for invalid private key algorithm

14f7a09

Signed-off-by: Vinny Sabatini <[email protected]>

test private key header matches expected format

7949fd4

Signed-off-by: Vinny Sabatini <[email protected]>

vinny-sabatini force-pushed the support-additional-keys branch from 5790859 to 7949fd4 Compare August 1, 2023 22:06

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2023

jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Aug 29, 2023

fix return for invalid private key algorithm in buildNextCR

d79e75a

Signed-off-by: Vinny Sabatini <[email protected]>

vinny-sabatini force-pushed the support-additional-keys branch from f302ded to d79e75a Compare August 29, 2023 14:56

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Aug 29, 2023

wip: add ecdsa test cases for buildNextCR

f5dc55d

I am not positive what is causing the failed test case at this point, still looking into it Signed-off-by: Vinny Sabatini <[email protected]>

jetstack-bot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 30, 2023

jetstack-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 30, 2023

ctrought mentioned this pull request Sep 13, 2023

Support latest cert-manager operator for openshift #39

Closed

maelvls reviewed Sep 15, 2023

View reviewed changes

fix ECDSA tests for buildNextCR

1509964

Signed-off-by: Vinny Sabatini <[email protected]>

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2023

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2023

jetstack-bot merged commit d401251 into cert-manager:main Oct 5, 2023
1 check passed

vinny-sabatini deleted the support-additional-keys branch October 5, 2023 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for ecdsa keys #28

add support for ecdsa keys #28

vinny-sabatini commented Jun 1, 2023

maelvls commented Jun 2, 2023 •

edited

Loading

vinny-sabatini commented Jun 2, 2023

vinny-sabatini commented Jun 5, 2023

vinny-sabatini commented Jun 12, 2023

maelvls commented Jun 23, 2023 •

edited

Loading

maelvls commented Jul 31, 2023 •

edited

Loading

vinny-sabatini commented Aug 1, 2023

maelvls Sep 15, 2023 •

edited

Loading

vinny-sabatini Sep 18, 2023

vinny-sabatini Sep 18, 2023

maelvls commented Oct 5, 2023

jetstack-bot commented Oct 5, 2023

add support for ecdsa keys #28

add support for ecdsa keys #28

Conversation

vinny-sabatini commented Jun 1, 2023

maelvls commented Jun 2, 2023 • edited Loading

vinny-sabatini commented Jun 2, 2023

vinny-sabatini commented Jun 5, 2023

vinny-sabatini commented Jun 12, 2023

maelvls commented Jun 23, 2023 • edited Loading

maelvls commented Jul 31, 2023 • edited Loading

vinny-sabatini commented Aug 1, 2023

maelvls Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

vinny-sabatini Sep 18, 2023

Choose a reason for hiding this comment

vinny-sabatini Sep 18, 2023

Choose a reason for hiding this comment

maelvls commented Oct 5, 2023

jetstack-bot commented Oct 5, 2023

maelvls commented Jun 2, 2023 •

edited

Loading

maelvls commented Jun 23, 2023 •

edited

Loading

maelvls commented Jul 31, 2023 •

edited

Loading

maelvls Sep 15, 2023 •

edited

Loading