NO-JIRA: Add ECDSA certificate generation support via annotation#315
Draft
fabiendupont wants to merge 1 commit intoopenshift:mainfrom
Draft
NO-JIRA: Add ECDSA certificate generation support via annotation#315fabiendupont wants to merge 1 commit intoopenshift:mainfrom
fabiendupont wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@fabiendupont: This pull request explicitly references no jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds per-service key algorithm selection for serving certificates (rsa or ecdsa) via a new service annotation and threads algorithm choice through CA creation, rotation, secret initialization, controller certificate generation, and tests; includes validation and error-annotation handling for invalid values. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Service (Annotation)
participant Controller as ServingCert Controller
participant Validator as Algorithm Validator
participant CA as CA / Operator
participant CertGen as Certificate Generator
Service->>Controller: Service created/updated (serving cert + algorithm annotation)
Controller->>Validator: Read annotation, validate (rsa | ecdsa)
alt invalid
Validator-->>Controller: validation error
Controller->>Service: updateServiceFailure (add error annotation)
else valid
Validator-->>Controller: validated algorithm
Controller->>CA: request CA/secret creation or use existing (pass algorithm)
CA->>CA: map algorithm -> crypto.KeyAlgorithm, create/init signing secret
Controller->>CertGen: Make server cert with selected algorithm
CertGen-->>Controller: return cert + key
Controller->>Service: create/update secret with cert/key
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fabiendupont The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
fabiendupont
force-pushed
the
add-ecdsa-annotation-support
branch
from
dd98880 to
070ae62
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
pkg/operator/config.go (1)
31-33: Normalize override input before algorithm comparison.Line 32 should trim whitespace; values like
" ecdsa "currently fall back to RSA silently.♻️ Proposed tweak
func (c caConfig) cryptoKeyAlgorithm() crypto.KeyAlgorithm { - if strings.EqualFold(c.KeyAlgorithm, "ecdsa") { + if strings.EqualFold(strings.TrimSpace(c.KeyAlgorithm), "ecdsa") { return crypto.AlgorithmECDSA } return crypto.AlgorithmRSA }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/config.go` around lines 31 - 33, Normalize c.KeyAlgorithm by trimming surrounding whitespace before comparing in caConfig.cryptoKeyAlgorithm; e.g., compute a trimmed value (use strings.TrimSpace on c.KeyAlgorithm) and then use strings.EqualFold on that trimmed value when checking for "ecdsa" so inputs like " ecdsa " are recognized instead of falling back to RSA.test/e2e/e2e.go (2)
315-315: Prefer constant over raw TLS key string.Line 315 uses
"tls.crt"directly; usev1.TLSCertKeyfor consistency and typo safety.🧹 Suggested tweak
- err = editServingSecretDataGinkgo(t, adminClient, testSecretName, ns.Name, "tls.crt") + err = editServingSecretDataGinkgo(t, adminClient, testSecretName, ns.Name, v1.TLSCertKey)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e.go` at line 315, Replace the raw TLS key string literal "tls.crt" with the Kubernetes constant v1.TLSCertKey when calling editServingSecretDataGinkgo (i.e., change the argument passed in the call editServingSecretDataGinkgo(t, adminClient, testSecretName, ns.Name, "tls.crt") to use v1.TLSCertKey) to ensure consistency and avoid typos.
97-113: Expand ECDSA e2e coverage to include headless services.This new context only exercises non-headless services, while the base serving-cert path already validates both modes. Adding
headless=true/falsehere would better protect SAN/DNS behavior parity for ECDSA paths.♻️ Suggested change
g.Context("serving-cert-annotation-ecdsa", func() { - g.It("[Operator][Serial] should provision ECDSA certificates for services", func() { - testServingCertAnnotationWithAlgorithm(g.GinkgoTB(), "ecdsa") - }) + for _, headless := range []bool{false, true} { + g.It(fmt.Sprintf("[Operator][Serial] should provision ECDSA certificates for services with headless=%v", headless), func() { + testServingCertAnnotationWithAlgorithm(g.GinkgoTB(), "ecdsa", headless) + }) + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e.go` around lines 97 - 113, The ECDSA e2e contexts only run non-headless cases; update the three ECDSA-related specs to exercise both headless modes by invoking the existing helpers for headless=true and headless=false (e.g., call testServingCertAnnotationWithAlgorithm(..., headless) and testServingCertSecretDeleteDataWithAlgorithm(..., headless)); also run testServingCertAnnotationInvalidAlgorithm with both headless values so SAN/DNS parity is validated for invalid-algorithm paths. Locate the g.Context blocks and the helper calls (testServingCertAnnotationWithAlgorithm, testServingCertAnnotationInvalidAlgorithm, testServingCertSecretDeleteDataWithAlgorithm) and change them to iterate or add two It cases that pass headless=true and headless=false into the helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/servingcert/controller/secret_creating_controller_test.go`:
- Around line 812-815: The test currently calls crypto.EnsureCA with hard-coded
/tmp paths which is flaky; change the test to create a unique temp dir via
t.TempDir(), build CA file paths with filepath.Join(tempDir,
"test-ca.crt"/"test-ca.key"/"test-ca.serial"), and pass those joined paths into
crypto.EnsureCA (the call that assigns ca, _, err := crypto.EnsureCA(...)); keep
signerName and caLifetime as-is and remove any reliance on fixed /tmp paths so
the test is hermetic and safe for concurrent runs.
In `@pkg/operator/rotate.go`:
- Around line 110-115: In rotateSigningCA, do not default unknown private-key
types to RSA: check the result of algorithmFromKey(currentKey) and if it
indicates an unsupported/unknown algorithm return a clear error instead of
proceeding to call crypto.MakeSelfSignedCAConfigForDurationWithAlgorithm;
propagate that error out of rotateSigningCA so rotation fails fast. Apply the
same change to the other CA-rotation code path that also calls algorithmFromKey
and MakeSelfSignedCAConfigForDurationWithAlgorithm so unknown key types are
rejected there as well.
In `@pkg/operator/sync_common_test.go`:
- Line 48: The test currently calls initializeSigningSecret(secret, 0,
tc.duration, crypto.AlgorithmRSA) and ignores its returned error; update the
test setup to capture the returned error and fail the test on error (e.g.,
assign err := initializeSigningSecret(...) and call t.Fatalf or require.NoError
on err) so initialization failures are surfaced; reference the
initializeSigningSecret call in the test and ensure any returned error is
asserted before proceeding.
In `@test/e2e/e2e.go`:
- Around line 255-262: Replace the single immediate secret Get check with a
short polling loop that repeatedly calls
adminClient.CoreV1().Secrets(ns.Name).Get(..., testSecretName,
metav1.GetOptions{}) for a configured timeout (e.g., 5s) and interval (e.g.,
200ms), failing if any call succeeds (err == nil) or if the final state is not
errors.IsNotFound(err); ensure you reference testSecretName, ns.Name,
errors.IsNotFound and metav1.GetOptions in the loop and return a clear t.Fatalf
on unexpected errors or if the secret appears during the polling window.
- Around line 171-214: The testServingCertAnnotationWithAlgorithm function
ignores its algorithm parameter when asserting the certificate public key
algorithm; update the final assertion to derive the expected
x509.PublicKeyAlgorithm from the algorithm parameter (e.g., map "rsa" ->
x509.RSA, "ecdsa" -> x509.ECDSA, etc.), then compare cert.PublicKeyAlgorithm to
that expected constant and fail with a clear message including both expected and
actual values; adjust the check in the block after x509.ParseCertificate (where
cert.PublicKeyAlgorithm is inspected) to use a switch or map on the algorithm
variable and assert accordingly.
---
Nitpick comments:
In `@pkg/operator/config.go`:
- Around line 31-33: Normalize c.KeyAlgorithm by trimming surrounding whitespace
before comparing in caConfig.cryptoKeyAlgorithm; e.g., compute a trimmed value
(use strings.TrimSpace on c.KeyAlgorithm) and then use strings.EqualFold on that
trimmed value when checking for "ecdsa" so inputs like " ecdsa " are recognized
instead of falling back to RSA.
In `@test/e2e/e2e.go`:
- Line 315: Replace the raw TLS key string literal "tls.crt" with the Kubernetes
constant v1.TLSCertKey when calling editServingSecretDataGinkgo (i.e., change
the argument passed in the call editServingSecretDataGinkgo(t, adminClient,
testSecretName, ns.Name, "tls.crt") to use v1.TLSCertKey) to ensure consistency
and avoid typos.
- Around line 97-113: The ECDSA e2e contexts only run non-headless cases; update
the three ECDSA-related specs to exercise both headless modes by invoking the
existing helpers for headless=true and headless=false (e.g., call
testServingCertAnnotationWithAlgorithm(..., headless) and
testServingCertSecretDeleteDataWithAlgorithm(..., headless)); also run
testServingCertAnnotationInvalidAlgorithm with both headless values so SAN/DNS
parity is validated for invalid-algorithm paths. Locate the g.Context blocks and
the helper calls (testServingCertAnnotationWithAlgorithm,
testServingCertAnnotationInvalidAlgorithm,
testServingCertSecretDeleteDataWithAlgorithm) and change them to iterate or add
two It cases that pass headless=true and headless=false into the helpers.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/library-go/pkg/crypto/crypto.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (9)
pkg/controller/api/api.gopkg/controller/servingcert/controller/secret_creating_controller.gopkg/controller/servingcert/controller/secret_creating_controller_test.gopkg/operator/config.gopkg/operator/rotate.gopkg/operator/sync_common.gopkg/operator/sync_common_test.gotest/e2e/e2e.gotest/e2e/e2e_test.go
pkg/controller/servingcert/controller/secret_creating_controller_test.go
Outdated
Show resolved
Hide resolved
Adds support for generating ECDSA P-256 certificates by specifying the key algorithm via the new service annotation: service.beta.openshift.io/serving-cert-key-algorithm: ecdsa When the annotation is not specified or set to "rsa", the operator generates RSA certificates for full backwards compatibility. This enables services to opt into modern elliptic curve cryptography, which provides equivalent security to 3072-bit RSA with significantly smaller keys (~87% smaller) and better performance. Implementation: - Added ServingCertKeyAlgorithmAnnotation constant in api.go - Modified MakeServingCert() to check annotation and select algorithm - Uses library-go's new MakeServerCertWithAlgorithm() API - Validates annotation values (rsa, ecdsa) with helpful error messages - Case-insensitive algorithm matching - Wired ECDSA CA support: caConfig now accepts a KeyAlgorithm field, initializeSigningSecret passes algorithm to CA creation - CA rotation supports both RSA and ECDSA keys: removed RSA-only type assertion, rotateSigningCA and createIntermediateCACert accept crypto.PrivateKey, algorithm is detected and preserved on rotation Testing: - Added TestECDSACertificateGeneration with 5 test cases - Verifies RSA (default and explicit), ECDSA, and invalid inputs - Added e2e tests: ECDSA cert provisioning, invalid algorithm rejection, and ECDSA cert regeneration after corruption - All existing tests pass with no regressions Depends on: openshift/library-go#2116 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Fabien Dupont <fdupont@redhat.com>
fabiendupont
force-pushed
the
add-ecdsa-annotation-support
branch
from
070ae62 to
7b40982
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/operator/sync_common.go (1)
151-155: Handle unknown algorithms explicitly instead of defaulting viaelse.The current
elsepath treats any non-ECDSA value as RSA. Aswitchwith an explicit error path avoids silent fallback if an unsupported value slips in.Suggested hardening
- if algorithm == crypto.AlgorithmECDSA { - ca, err = crypto.MakeSelfSignedCAConfigForDurationWithAlgorithm(name, lifetime, algorithm) - } else { - ca, err = crypto.MakeSelfSignedCAConfig(name, lifetime) - } + switch algorithm { + case crypto.AlgorithmECDSA: + ca, err = crypto.MakeSelfSignedCAConfigForDurationWithAlgorithm(name, lifetime, algorithm) + case crypto.AlgorithmRSA: + ca, err = crypto.MakeSelfSignedCAConfig(name, lifetime) + default: + return fmt.Errorf("unsupported signing key algorithm %q", algorithm) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/sync_common.go` around lines 151 - 155, The code currently treats any non-ecdsa algorithm as RSA via an else branch; change the conditional to an explicit switch on `algorithm` that handles `crypto.AlgorithmECDSA` and `crypto.AlgorithmRSA` (calling `crypto.MakeSelfSignedCAConfigForDurationWithAlgorithm` for ECDSA and `crypto.MakeSelfSignedCAConfig` for RSA) and add a default case that returns or propagates a clear error indicating an unsupported algorithm value instead of silently falling back; ensure the error is returned from the surrounding function so callers can handle it.pkg/operator/rotate.go (1)
209-209: Prefer empty algorithm value when returning an error.Returning
AlgorithmRSAtogether with an error can mislead future callers that accidentally ignoreerr.Small clarity tweak
- return crypto.AlgorithmRSA, fmt.Errorf("unsupported private key type %T", key) + return "", fmt.Errorf("unsupported private key type %T", key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/rotate.go` at line 209, The return in rotate.go currently returns crypto.AlgorithmRSA alongside an error (return crypto.AlgorithmRSA, fmt.Errorf(...)), which can mislead callers that ignore the error; instead return the zero/empty algorithm value (the zero value for the Algorithm type) when returning an error—replace crypto.AlgorithmRSA with the empty/zero algorithm value in that error return in the function that determines the algorithm (the code path that produces fmt.Errorf("unsupported private key type %T", key)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/rotate.go`:
- Line 209: The return in rotate.go currently returns crypto.AlgorithmRSA
alongside an error (return crypto.AlgorithmRSA, fmt.Errorf(...)), which can
mislead callers that ignore the error; instead return the zero/empty algorithm
value (the zero value for the Algorithm type) when returning an error—replace
crypto.AlgorithmRSA with the empty/zero algorithm value in that error return in
the function that determines the algorithm (the code path that produces
fmt.Errorf("unsupported private key type %T", key)).
In `@pkg/operator/sync_common.go`:
- Around line 151-155: The code currently treats any non-ecdsa algorithm as RSA
via an else branch; change the conditional to an explicit switch on `algorithm`
that handles `crypto.AlgorithmECDSA` and `crypto.AlgorithmRSA` (calling
`crypto.MakeSelfSignedCAConfigForDurationWithAlgorithm` for ECDSA and
`crypto.MakeSelfSignedCAConfig` for RSA) and add a default case that returns or
propagates a clear error indicating an unsupported algorithm value instead of
silently falling back; ensure the error is returned from the surrounding
function so callers can handle it.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/library-go/pkg/crypto/crypto.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (9)
pkg/controller/api/api.gopkg/controller/servingcert/controller/secret_creating_controller.gopkg/controller/servingcert/controller/secret_creating_controller_test.gopkg/operator/config.gopkg/operator/rotate.gopkg/operator/sync_common.gopkg/operator/sync_common_test.gotest/e2e/e2e.gotest/e2e/e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/e2e_test.go
- pkg/operator/config.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds support for generating ECDSA P-256 certificates by specifying the key algorithm via the new service annotation:
service.beta.openshift.io/serving-cert-key-algorithm: ecdsa
When the annotation is not specified or set to "rsa", the operator generates RSA certificates for full backwards compatibility.
This enables services to opt into modern elliptic curve cryptography, which provides equivalent security to 3072-bit RSA with significantly smaller keys (~87% smaller) and better performance.
Implementation:
Testing:
Depends on: openshift/library-go#2116
Summary by CodeRabbit
New Features
Bug Fixes
Tests