-
Notifications
You must be signed in to change notification settings - Fork 39.5k
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
feat:(kms) encrypt data with DEK using AES-GCM instead of AES-CBC #111119
Conversation
/sig auth |
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 wondering how we'll document this.
The text to change in the docs is something like:
Uses envelope encryption scheme: Data is encrypted by data encryption keys (DEKs) that use AES-CBC with PKCS#7 padding; DEKs are encrypted by key encryption keys (KEKs) according to configuration in Key Management Service (KMS)
That text is already kind of complicated. It'll become even more complicated with the change in this PR.
- Should this revised encryption be behind a feature gate? (IMO: yes)
- When will we drop support for reading the previous envelope encryption scheme
- or will we support readback via AES-CBC indefinitely
- What tasks should a cluster operator perform to get ready for that removal (if planned)
I think I'd prefer to let the cluster operator specify a DEK encryption mechanism for writes and configure which encryption mechanisms are supported for readback. That's similar to how a cluster operator can configure eg aesgcm
for writes and aesgcm
, kms
or aescbc
for reads.
staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go
Show resolved
Hide resolved
In the PR description, “Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:” is blank. This is a significant change for some cluster operators and we should include a link to relevant docs. Ideally there is also a KEP that we should link to. Having a KEP also triggers docs tracking; without it, SIG Release won't know to nag folks about getting the docs updated. |
@aramase this is held - can you add a comment to clarify why and under what circumstances you'd expect to unhold? |
/triage accepted |
staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go
Show resolved
Hide resolved
perhaps update release note:
|
From sig-auth: We consider this to be an implementation detail of the envelope encryption storage transformer. There are no feature gates or user knobs and the user should never be aware of the migration. Let's make sure the doc is accurate but I don't think we need to stretch this out with a KEP / features gates / configuration at this time. |
/priority important-soon |
/hold cancel |
/milestone v1.25 |
@aramase: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Milestone Maintainers Team and have them propose you as an additional delegate for this responsibility. In 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 kubernetes/test-infra repository. |
For the release note
then could we link to https://kubernetes.io/docs/tasks/administer-cluster/kms-provider/? That saves readers a job. |
We might want to document how to ensure all relevant objects use an AES-GCM DEK (which I think is: read them all, and then rewrite them - same as if you migrate to / or from KMS encryption at rest) |
@enj @mikedanese This PR is ready for review. Could you take a look when you get a chance? |
@mikedanese @enj Could we get this PR reviewed soon so we don't miss the v1.25 code freeze? |
The v1.25 code freeze is tomorrow. @mikedanese Could you review this so we can get it in for v1.25? |
/milestone v1.25 |
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 code change itself LGTM.
staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Anish Ramasekar <[email protected]>
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aramase, enj The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Anish Ramasekar [email protected]
What type of PR is this?
/kind feature
What this PR does / why we need it:
Follow-up to #108745. This change updates the KMS envelope encryption to be use AES-GCM to encrypt data using the DEK instead of AES-CBC. To allow for downgrades and HA upgrades, this functionality allows reads with AES-GCM and AES-CBC.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: