fixes #7506: enable configurable max key/cert sizes, defaulting to original safe values introduced in #7401#7642
Conversation
|
Hi @robertlestak. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
f5b7c90 to
d9134c0
Compare
|
👋 Hiya @robertlestak I'm not sure we would want this configured via environment variables, all other cert-manager config is done via flags or via the config file. |
|
@ThatsMrTalbot no worries, i didn't want to overstep in changing too much, i'll modify to use config file and push update later today! |
|
+1 waiting for this feature. |
|
+1 also looking forward to this due to #7506. Thanks for taking this on, @robertlestak! |
d9134c0 to
8bd4396
Compare
…iginal safe values introduced in #7401 Signed-off-by: robertlestak <robert.lestak@umusic.com>
8bd4396 to
ba61c53
Compare
|
/ok-to-test |
|
@ThatsMrTalbot brought this to my attention in standup this morning. For my 2 cents, I'm totally fine with increasing the max size limits across the board with no need for configuration (for context, I wrote the PR adding these limits to fix a security issue and the current limits are my best guesses) We're already prepared for certs to grow much (much) larger with post-quantum signatures and we'd need to bump the size limits for that at some point anyway! If you were able to provide examples of the cert chains you're talking about which exceed these size limits, I'd be comfortable with increasing the limits accordingly. (I understand that certs can have private org data in them, so you could just provide the size in bytes of a cert + the max number of certs in a chain that you see if you're not comfortable sharing the actual certs!) |
|
@SgtCoDFish Ack my use case may be "non-standard" but certainly not unique. Due to reasons out of scope of cert-manager, I have business use cases where a single cert can have >250 SANs on it. This is with "conventional" public CA+Intermediate (eg Sectigo, DigiCert, etc) - so the majority of the "size" comes from the leaf w/ over 200 SANs. I would hesitate to say "X bytes should suffice", since business case is dynamic and while i have 250 SANs today, I had ~150 last year, and may have ~350 next year. For highly regulated environments managed entirely through IaC, any manifest changes must first go through layers of review/approval, and are gated with granular RBAC preventing even operators from performing certain actions. If I have a user with the ability to create secrets/certs inside my environment, I've got much bigger issues than theoretical DoS of cluster-local |
|
Thank you for the context, that's really helpful! I did a bit of digging and I saw that adding about 10 longish example DNS names (e.g. I think it's fair to say that it'd be hard for us to anticipate how these size limits might need to grow. You've convinced me that configurable size limits are worthwhile! Configurable size limits probably aren't likely to be backported, though - so the soonest they'd be usable would be with cert-manager 1.19 in early Oct (or in an alpha/beta release done before that). I'm on paternity leave for 2 months starting next week, so I won't be able to review this and drive it through, but I'd encourage you to chase for reviews in #cert-manager-dev on slack once tests are passing! |
c9a357b to
fc37bc5
Compare
- Resolved conflicts between PEM size limits and CertificateRequestMinimumBackoffDuration - Fixed MaxChainLength semantics (bytes not count) - Updated Helm chart documentation and schema - Fixed validation logic - Added missing test imports - All tests passing Signed-off-by: robert lestak <robert.lestak@umusic.com>
27c2221 to
463f848
Compare
463f848 to
27c2221
Compare
27c2221 to
65cb1f0
Compare
65cb1f0 to
27c2221
Compare
|
@robertlestak: The following tests failed, say
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. 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. |
|
@SgtCoDFish hope you had a happy and restful holiday with the growing family 😀 Picking this back up, I made the requested changes and all tests are passing 🎉 I've tested my version in one of our environments which is currently facing the issue (currently on v1.19.1) and it worked as expected - default config continued to fail (as expected) but after increasing the size limit values in the config, the cert was properly issued and sync'ed into k8s. Just joined the #cert-manager-dev slack channel, thanks for the guidance and assistance, looking forward to continuing to contribute to this great project! |
Signed-off-by: robert lestak <robert.lestak@umusic.com>
ThatsMrTalbot
left a comment
There was a problem hiding this comment.
Hiya @robertlestak, I've only got a couple of comments, on the whole this looks good
- Add sync.RWMutex to protect concurrent access to globalSizeLimits - Use Lock/Unlock in SetGlobalSizeLimits for write operations - Use RLock/RUnlock in GetGlobalSizeLimits for read operations - Replace all direct globalSizeLimits accesses with GetGlobalSizeLimits() calls Addresses review feedback from ThatsMrTalbot on PR #7642 Signed-off-by: robert lestak <robert.lestak@umusic.com>
|
Thanks for making those changes! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ThatsMrTalbot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Pull Request Motivation
While 'non-standard', there are valid technical situations where a key/pem would be expected to exceed the 'safe' values implemented in #7401.
Issue #7506 shows that multiple users are facing this issue.
This PR enables operators to override the default values in the controller configuration. "Default" usage will continue to use the values implemented in #7401, but for operators who need higher max values, they can set those on a per-deployment basis.
Kind
/kind bug feature
Release Note