-
Notifications
You must be signed in to change notification settings - Fork 259
NO-JIRA: Add ECDSA P-256 key generation support #2116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@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. |
|
[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 |
8dd03c3 to
724c6f6
Compare
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 Testing: - Added TestECDSACertificateGeneration with 5 test cases - Verifies RSA (default and explicit), ECDSA, and invalid inputs - All existing tests pass with no regressions Depends on: openshift/library-go#2116 Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Fabien Dupont <fdupont@redhat.com>
724c6f6 to
b5ed9f4
Compare
| } | ||
|
|
||
| // MakeServerCertWithAlgorithm creates a server certificate with the specified key algorithm | ||
| func (ca *CA) MakeServerCertWithAlgorithm(hostnames sets.Set[string], lifetime time.Duration, algorithm KeyAlgorithm, fns ...CertificateExtensionFunc) (*TLSCertificateConfig, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I might be wrong, but it looks like MakeServerCert and MakeServerCertWithAlgorithm are very similar. There are two differences. The first is the key algorithm, and the second is that MakeServerCertWithAlgorithm sets the signature algorithm.
I’m wondering if it would make sense to create a private common function for both. I think something like the following could work:
func (ca *CA) MakeServerCertWithAlgorithm(hostnames sets.Set[string], lifetime time.Duration, algorithm KeyAlgorithm, fns ...CertificateExtensionFunc) (*TLSCertificateConfig, error) {
sigFn := func(template *x509.Certificate) error {
template.SignatureAlgorithm = signatureAlgorithmForKey(ca.Config.Key)
return nil
}
fns = append([]CertificateExtensionFunc{sigFn}, fns...)
return ca.makeServerCertWithAlgorithm(hostnames, lifetime, algorithm, fns...)
}
and
func (ca *CA) MakeServerCert(hostnames sets.Set[string], lifetime time.Duration, fns ...CertificateExtensionFunc) (*TLSCertificateConfig, error) {
return ca.makeServerCertWithAlgorithm(hostnames, lifetime, AlgorithmRSA, fns...)
}
pkg/crypto/crypto.go
Outdated
| case AlgorithmRSA: | ||
| return newKeyPairWithHash() | ||
| default: | ||
| return newKeyPairWithHash() // Default to RSA for backwards compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a private method I think we could return an error for unsupported algorithms.
pkg/crypto/crypto.go
Outdated
| } | ||
|
|
||
| // MakeServerCertForDurationWithAlgorithm creates a server certificate with specified duration and algorithm | ||
| func (ca *CA) MakeServerCertForDurationWithAlgorithm(hostnames sets.Set[string], lifetime time.Duration, algorithm KeyAlgorithm, fns ...CertificateExtensionFunc) (*TLSCertificateConfig, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am I right that the differences between MakeServerCertForDuration and MakeServerCertForDurationWithAlgorithm mirror those between MakeServerCert and MakeServerCertWithAlgorithm (key algorithm and signature algorithm)? If so, could we refactor them in the same way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good feedback. Makes sense.
Extends the crypto package to generate ECDSA P-256 key pairs in addition to existing RSA support, enabling OpenShift components to use modern elliptic curve cryptography. Adds: - KeyAlgorithm type for algorithm selection (RSA, ECDSA) - newECDSAKeyPair() and newECDSAKeyPairWithHash() functions using P-256 curve - newKeyPairWithAlgorithm() for unified key generation - signatureAlgorithmForKey() for automatic algorithm detection - CA.MakeServerCertWithAlgorithm() and CA.MakeServerCertForDurationWithAlgorithm() All existing APIs remain unchanged, preserving 100% backwards compatibility. New functionality is opt-in through *WithAlgorithm functions. ECDSA P-256 provides equivalent security to 3072-bit RSA with smaller keys (~87% smaller), faster operations, and better performance. This prepares OpenShift for modern TLS deployments and aligns with industry best practices. Test coverage includes: - Unit tests for key generation, signature algorithm detection, and encoding - Integration tests for RSA CA + ECDSA server and vice versa - Backwards compatibility tests verifying existing RSA functionality Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Fabien Dupont <fdupont@redhat.com>
b5ed9f4 to
854a902
Compare
|
@fabiendupont: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
Extends the crypto package to generate ECDSA P-256 key pairs in addition to existing RSA support, enabling OpenShift components to use modern elliptic curve cryptography.
Adds:
All existing APIs remain unchanged, preserving 100% backwards compatibility. New functionality is opt-in through *WithAlgorithm functions.
ECDSA P-256 provides equivalent security to 3072-bit RSA with smaller keys (~87% smaller), faster operations, and better performance. This prepares OpenShift for modern TLS deployments and aligns with industry best practices.
Test coverage includes: