ca: change the PrivateKey type/bits validation #12267
Merged
Conversation
This file contains 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
Before adding more test cases
This commit makes two changes to the validation. Previously we would call this validation in GenerateRoot, which happens both on initialization (when a follower becomes leader), and when a configuration is updated. We only want to do this validation during config update so the logic was moved to the UpdateConfiguration function. Previously we would compare the config values against the actual cert. This caused problems when the cert was created manually in Vault (not created by Consul). Now we compare the new config against the previous config. Using a already created CA cert should never error now. Adding the key bit and types to the config should only error when the previous values were not the defaults.
dnephin
added
type/bug
github-actions
bot
added
theme/connect
kisunji
reviewed
Feb 3, 2022
kisunji
reviewed
Feb 3, 2022
kisunji
reviewed
Feb 3, 2022
kisunji
reviewed
Feb 3, 2022
| // ValidateConfigurationUpdate will be called when a user attempts to change the | ||
| // CA configuration, and the provider type has not changed from the previous | ||
| // configuration. | ||
| type ValidateConfigUpdater interface { |
There was a problem hiding this comment.
aside: I think ConfigUpdateValidator is less ambiguous but ConfigUpdateValidate() OTOH is less clear
There was a problem hiding this comment.
Ya, this interface could probably use a better name. It is hard to name things in Consul because we have way too many things in these packages. It forces us to include many terms in the name to remove ambiguity, which leads to unfortunate names like this
And fix typo
kisunji
approved these changes
Feb 4, 2022
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/576482. |
|
🍒✅ Cherry pick of commit 74ee46e onto |
hc-github-team-consul-core
pushed a commit
that referenced
this pull request
Feb 4, 2022
…idation ca: change the PrivateKey type/bits validation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #12246
This commit makes two changes to the validation.
Previously we would call this validation in GenerateRoot, which happens
both on initialization (when a follower becomes leader), and when a
configuration is updated. We only want to do this validation during
config update so the logic was moved to the
UpdateConfigurationmethod.
Previously we would compare the config values against the actual cert.
This caused problems when the cert was created manually in Vault (not
created by Consul). Now we compare the new config against the previous
config. Using an already created CA cert should never error now.
Adding the key bit and types to the config should only error when
the previous values were not the defaults.
This does mean that changing these values away from the defaults without
changing the root_pki_path will continue to silently do nothing, but I think
that is a better trade-off than return an error incorrectly at times.
In the future we should look to better structure the CA config to indicate which
values are supposed to rotate the root cert, and which ones are not.
I believe the original change only went into 1.11.x, so I am only backporting to one
release branch.