-
Notifications
You must be signed in to change notification settings - Fork 22
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
Allow to specify private key algorithm and size #168
Conversation
/invite @timuthy |
@MartinWeindel Command "/invite @timuthy " failed with "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the gardener/cert-management repository.". Additional Information
|
/assign |
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.
Nice feature 🙂
README.md
Outdated
@@ -352,6 +354,33 @@ spec: | |||
|
|||
In this case the secret `my-secret` will contains the labels. | |||
|
|||
### Specifying private key algorithm and size | |||
|
|||
By default, the certificate uses `RSA` with a key size of 2048 bits for the private key |
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.
By default, the certificate uses `RSA` with a key size of 2048 bits for the private key | |
By default, the certificate uses `RSA` with a key size of 2048 bits for the private key. |
pkg/apis/cert/v1alpha1/types.go
Outdated
// key size of 2048 will be used for `RSA` key algorithm and | ||
// key size of 256 will be used for `ECDSA` key algorithm. | ||
// +optional | ||
Algorithm PrivateKeyAlgorithm `json:"algorithm,omitempty"` |
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.
Algorithm PrivateKeyAlgorithm `json:"algorithm,omitempty"` | |
Algorithm *PrivateKeyAlgorithm `json:"algorithm,omitempty"` |
pkg/apis/cert/v1alpha1/types.go
Outdated
// and will default to `256` if not specified. | ||
// No other values are allowed. | ||
// +optional | ||
Size int `json:"size,omitempty"` |
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.
Size int `json:"size,omitempty"` | |
Size *int32 `json:"size,omitempty"` |
pkg/cert/legobridge/certificate.go
Outdated
return keyType, nil | ||
} | ||
|
||
// FromKeyType converts key type back to |
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.
Doc string incomplete.
pkg/cert/legobridge/certificate.go
Outdated
} | ||
return client.Certificate.Obtain(request) | ||
} | ||
|
||
func obtainForCSR(client *lego.Client, csr []byte, deactivateAuthz bool, preferredChain string) (*certificate.Resource, error) { | ||
// ToKeyType extracts the key type from the |
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.
Doc string incomplete.
// key size of 256 will be used for `ECDSA` key algorithm. | ||
// +optional |
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.
Can we work with // +kubebuilder:validation:Enum=RSA;ECDSA
here, maybe also for the size
?
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.
Interestingly, you can really define an enumeration for numbers.🤩
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.
One doubt left, the rest looks good to me ✅
} | ||
newPrivateKey := createPrivateKey(info.PrivateKeyAlgorithm, info.PrivateKeySize) | ||
if !reflect.DeepEqual(spec.PrivateKey, newPrivateKey) { | ||
spec.PrivateKey = newPrivateKey |
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.
Not sure if I understand the code correctly: Does this mean that the controller writes back the defaulted values to the certificate.spec.privateKey
if it's not set before? If yes, won't this cause all existing certificates to be re-generated when this feature is rolled out because the spec checksum changes?
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.
If the source object has no private key related annotations, the function createPrivateKey
returns nil
. Even if you would add these annotations with the default values, the generated certificate resource will be updated, but no new certificate would be requested, as the hash code is unchanged.
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.
/lgtm
@timuthy Thanks for taking the time for a review! |
What this PR does / why we need it:
The algorithm and size for the private key can now be specified in the certificate spec section to override the default algorithm
RSA
with key size 2048.Supported algorithms are
RSA
andECDSA
. ForRSA
the allowed key sizes are2048
,3072
, and4096
with2048
as default is not specified explicitly. ForECDSA
the allowed key sizes are256
and384
with256
as default.These algorithms and key sizes are supported by Let's encrypt. For other ACME servers please check their documentation for information about supported combinations.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: