-
Notifications
You must be signed in to change notification settings - Fork 259
CNTRLPLANE-2241: Add KMS encryption mode into library-go encryption controllers #2086
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
|
@ardaguclu: This pull request references CNTRLPLANE-2241 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
@ardaguclu: This pull request references CNTRLPLANE-2241 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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: ardaguclu 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 |
|
@ardaguclu: This pull request references CNTRLPLANE-2241 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
377c3cb to
0b007f9
Compare
|
/hold
|
|
/uncc @dgrisonnet |
ok, thanks, i will start reviewing soon. |
p0lyn0mial
left a comment
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.
I did a quick first pass.
Looked mostly at key_controller.go.
I posted my question / observation below. Let me know what you think.
| } | ||
|
|
||
| if len(ks.KMSConfig) > 0 { | ||
| s.Annotations[EncryptionSecretKMSConfig] = string(ks.KMSConfig) |
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 v1 assumes the KMS configuration is always the same and static, and we don’t know what future v2 will look like.
I’m wondering if we could slightly simplify the code by always putting the well-known configuration in the annotation when the current mode is KMS. This could simplify the code and potential places we would have to change.
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.
I'll investigate the possible implications tomorrow and get back to you. I think it would be simpler as you said.
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.
Initially I tried to adopt generic approach that would work also for v2 (except the API changes). But I think starting simple by dropping the unsupported parts in v1 alleviates both of us cognitive load.
Thus, generic KMSConfig is dropped, instead default endpoint is embedded. There isn't KMSConfig comparison anymore, because we don't support this already.
| if currentMode == state.KMS { | ||
| // We are here because Encryption Mode is not changed | ||
| secret := crypto.ModeToNewKeyFunc[state.KMS](kmsConfig) | ||
| if latestKey.Key.Secret != base64.StdEncoding.EncodeToString(secret) { |
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.
i'm not sure that all changes/updates to a kms config should trigger a new key generation. rolling out a new key is expensive in terms of required shutdowns. also changing a timeout seems like an update that should not require a new key. something we need to figure out for v2.
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.
I completely agree with you. From the perspective of this line, it doesn't matter what secret includes. This line in any case need to compare the secret to detect new key or not.
https://github.com/openshift/library-go/pull/2086/changes#diff-de68f84fb611b486200c440adbdd1997edb7ab5ffbc520e485b39f73b18d339eR296-R304 this is where we decide which data should trigger new key or not (by forming the source of the secret)
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.
However, in v1, we don't support kms configurational changes. We always embed the same well-known endpoint. In order to keep the simplicity, I've removed secret comparison for KMS. Latest version of this PR embeds well known endpoint and keeps tracks of it.
|
|
||
| func (c *keyController) generateKeySecret(keyID uint64, currentMode state.Mode, internalReason, externalReason string) (*corev1.Secret, error) { | ||
| bs := crypto.ModeToNewKeyFunc[currentMode]() | ||
| kmsConfig := []byte(kms.DefaultEndpoint) |
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.
This should include the selective set of kms fields from APIServer config that will trigger new key. For now it is statically set.
|
|
||
| func NewKMSKey(externalKey []byte) []byte { | ||
| // Used as fixed-length identifier in EncryptionConfiguration provider names. | ||
| // Example: kms-secrets-1-a7K9mJ2xH4nQ8pL5vR3wTg== |
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.
We can use hexadecimal instead base64
| }, | ||
| }) | ||
| case state.KMS: | ||
| kmsConfig := string(key.KMSConfig) |
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.
In v1, key.KMSConfig equals to well known endpoint.
p0lyn0mial
left a comment
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.
thanks, left a few additional comments. ptal when you have time.
| // the user via unsupportConfigOverrides.encryption.reason triggered this key. | ||
| ExternalReason string | ||
| // Encoded KMSConfig that stores the KMS related fields | ||
| KMSConfig []byte |
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.
nit could this be renamed to KMSConfiguration ?
| // the user via unsupportConfigOverrides.encryption.reason triggered this key. | ||
| ExternalReason string | ||
| // Encoded KMSConfig that stores the KMS related fields | ||
| KMSConfig []byte |
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.
could KMSConfiguration be a struct ? why []bytes ?
could KMSConfiguration be:
KMSConfiguration *apiserverconfigv1.KMSConfiguration
?
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.
I tried to answer in #2086 (comment). KMSConfiguration refers to plugin specific configuration rather than apiserverconfigv1.KMSConfiguration
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.
I store as bytes array to keep the generality. Because each plugin type will have their own specific fields.
| ExternalReason: externalReason, | ||
| } | ||
| if currentMode == state.KMS { | ||
| ks.KMSConfig = kmsConfig |
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.
could this be:
ks.KMSConfiguration = &apiserverv1.KMSConfiguration{
APIVersion: "v2",
Name: fmt.Sprintf("%d", keyID),
Endpoint: kms.DefaultEndpoint,
Timeout: &metav1.Duration{Duration: kms.DefaultTimeout},
}
?
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.
KMSConfig stores Plugin specific configurations (although this is obsolete, I just reference https://github.com/openshift/api/blob/81371d13d1fcad175a48627cf11524a94a80c377/config/v1/types_kmsencryption.go#L36 to show an example of a configuration). So it is bound to APIServer config rather than apiserverv1.KMSConfiguration.
For instance KMS config will likely store AppRole, etc. for Vault in v2.
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.
We need to trigger a new migration, if keyArn (or appRole, etc.) is changed, even though these are not represented in apiserverv1.KMSConfiguration.
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.
would it make sense to focus on v1 in this PR ?
we might change the approach for v2 once we start prototyping.
as far as i can tell a KMSConfig is actually apiserverconfigv1.KMSConfiguration
xref: https://github.com/openshift/library-go/pull/2086/changes#r2773184416
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.
keyArn (or appRole, etc.) is changed, even though these are not represented in apiserverv1.KMSConfiguration.
aren't these changes for a kms plugin ?
i think we need to distinguish two configs: one for a kms plugin and the other one for kms configuration (encryption-cfg)
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.
You are right actually. We may even need a third one to track key_id from Status endpoint. But for v1, I think we can have one.
| func (c *keyController) generateKeySecret(keyID uint64, currentMode state.Mode, internalReason, externalReason string) (*corev1.Secret, error) { | ||
| bs := crypto.ModeToNewKeyFunc[currentMode]() | ||
| kmsConfig := []byte(kms.DefaultEndpoint) | ||
| bs := crypto.ModeToNewKeyFunc[currentMode](kmsConfig) |
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 the key is not used why it can't be 0 as for the identity ?
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.
It will definitely be used in v2. You mean we can remove it for v1, right?
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 the key is not used why it can't be 0 as for the identity ?
I think, for v1, we can remove indeed.
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.
I would like to keep this pr simple for v1.
Once we start developing v2 we might change the approach.
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.
yeah, agree. It makes sense. I'm updating.
| provider := apiserverconfigv1.ProviderConfiguration{ | ||
| // This generated KMSConfiguration can be convertible to KeyState, | ||
| // since provider name contains the key id and secret. | ||
| KMS: &apiserverconfigv1.KMSConfiguration{ |
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.
here, as far as i can tell a KMSConfig is actually apiserverconfigv1.KMSConfiguration.
as far as i can tell we will never store any additional data beyond apiserverconfigv1.KMSConfiguration .
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.
I'm marking this PR as WIP to make necessary changes based on this ^^.
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.
PR has been updated to store apiserverconfigv1.KMSConfiguration.
|
/retitle WIP: CNTRLPLANE-2241: Add KMS encryption mode into library-go encryption controllers |
d0321f6 to
b8208c8
Compare
|
/retest |
|
/retitle CNTRLPLANE-2241: Add KMS encryption mode into library-go encryption controllers |
p0lyn0mial
left a comment
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.
left a few more questions.
overall it lgtm
| case provider.KMS != nil: | ||
| // Since in v1 we don't support kms -> kms migrations, we can use empty key. | ||
| // Because we don't need to compare secrets to detect change. | ||
| secret := crypto.ModeToNewKeyFunc[state.KMS]() |
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.
would it make sense to define a var for it similar to emptyStaticIdentityKey ?(defined at the top of this file)
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.
yes, it would be nicer. Updated.
| Name: keyId, | ||
| Secret: base64.StdEncoding.EncodeToString(secret), | ||
| }, | ||
| Mode: state.KMS, |
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.
shouldn't we also populate ks.KMSConfiguration ?
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.
Once we set correct keyId and secret to KeyState, backed secret will be found which includes any other field we need (including the KMSConfiguration). KeyState is enriched in here
library-go/pkg/operator/encryption/encryptionconfig/config.go
Lines 115 to 120 in b6adacb
| for _, k := range backedKeys { | |
| if state.EqualKeyAndEqualID(&ks, &k) { | |
| ks = k | |
| break | |
| } | |
| } |
| // Since in v1 we don't support kms -> kms migrations, we can use empty key. | ||
| // Because we don't need to compare secrets to detect change. | ||
| secret := crypto.ModeToNewKeyFunc[state.KMS]() | ||
| keyId := strings.Split(provider.KMS.Name, "-")[0] |
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.
could we create a private function for adding/removing the suffix ?
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.
That would be better. Updated.
| state.AESGCM: NewAES256Key, | ||
| state.SecretBox: NewAES256Key, // secretbox requires a 32 byte key so we can reuse the same function here | ||
| state.Identity: NewIdentityKey, | ||
| state.KMS: NewIdentityKey, |
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.
would it make sense to rename NewIdentityKey to something more generic ?(now used by state.Identity and state.KSM)
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 point. Updated.
| // Since in v1 we don't support kms -> kms migrations, we can use empty key. | ||
| // Because we don't need to compare secrets to detect change. | ||
| secret := crypto.ModeToNewKeyFunc[state.KMS]() | ||
| keyId := strings.Split(provider.KMS.Name, "-")[0] |
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.
I’m still wondering about encapsulation here.
I think other parts of the code won’t even be aware that the name has changed.
Only the code in encryptionconfig/config.go will encode and decode the new name, right?
Do we need to ensure that the name doesn’t already contain "-"?
For simplicity, we could add a comment in the key generation function describing this requirement.
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.
Only the code in encryptionconfig/config.go will encode and decode the new name, right?
That is correct. Rest of the code only deals with KeyState. They do not even know about the details of EncryptionConfiguration.
Do we need to ensure that the name doesn’t already contain "-"?
Kube-apiserver already fails, if it encounters a provider name without -. Because uniqueness has broken.
For simplicity, we could add a comment in the key generation function describing this requirement.
I've added a comment about the reasons behind this provider name generation mechanism. Please let me know your thoughts. Thanks.
| func getKeyIDFromProviderName(providerName string) string { | ||
| // We just need to obtain the keyID to find our backed secret | ||
| // e.g. "1-secrets" | ||
| return strings.Split(providerName, "-")[0] |
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.
How should this handle an unexpected provider name?
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.
Although it may not be possible, it is better to handle the invalid provider name gracefully. I've updated the code. Please let me know your thoughts.
| }, | ||
| }) | ||
| case state.KMS: | ||
| // In order to preserve the uniqueness, we should insert resource name |
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.
OK, "kms v2 providers name must always be unique across all kms providers (v1 and v2)":
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.
Are there going to be side effects from this naming scheme visible in things like metric labels?
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.
All the metrics that is about provider_name will be affected https://github.com/kubernetes/kubernetes/blob/90a76aaa9afbe9cdb3df5f0a4356018b6637648b/staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/metrics/metrics.go#L169.
| func createKMSProviderName(keyID, resource string) string { | ||
| // Ideally we should have used keyId simply in kms provider name. | ||
| // However, this is an upstream constraint that every provider name must be unique. | ||
| // To maintain uniqueness while still allowing access to the keyId, we generate provider name in this format. |
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.
OK, this is at least less strange than smuggling identity names in phony AES-GCM provider configs. Later, I would be interested in coming up with alternatives that let the controllers correlate provider configs to their source secret without smuggling.
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.
I agree with you. It was error-prone and not nice. Since now we carry KMSConfigurations in backed Secrets, I think we won't need to carry anything in provider_names.
| // Moreover, we don't trigger new key when external reason is changed. | ||
| // Because it would lead to duplicate providers which is not allowed. |
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.
IIUC, even if we did roll out a new encryption config in response to the "reason" update, we're not even capable of triggering a KEK rotation unless a specific provider happened to expose that capability independently.
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 point. That is correct.
| const ( | ||
| DefaultEndpoint = "unix:///var/run/kmsplugin/kms.sock" | ||
| DefaultTimeout = 10 * time.Second | ||
| ) |
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.
Is there a real need to export these constants? IMO, duplicating the values in tests is good because it requires that changes in observable behavior are accompanied by changes to test expectations. And other code (doing related things like setting up volume mounts) should be observing effective values rather than using hardcoded defaults.
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.
The only reason is to be accessed by key_controller. If we move these constants in there, we don't need to export them.
|
@ardaguclu: 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. |
This PR adds the implementation proposed in openshift/enhancements#1900.
This PR introduces KMS as a new mode in encryption controllers to be functioning along side with the other modes. Basically, the idea is to track the KMS configuration to detect any configuration changes. So that key_controller can create new Secret to initiate migration. KMS Secret will not contain any confidential data. It will simply store the KMS configuration.
For Tech Preview v1, we will only support static unix domain socket (i.e.
unix:///var/run/kmsplugin/kms.sock). Consequently, in this version, we don't support KMS -> KMS migrations.Ref: Previous attemps:
Notes: