Skip to content
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

Jump to bottom

feat:(kms) encrypt data with DEK using AES-GCM instead of AES-CBC #111119

Merged
merged 1 commit into from Aug 2, 2022
Merged

feat:(kms) encrypt data with DEK using AES-GCM instead of AES-CBC #111119

merged 1 commit into from Aug 2, 2022

Conversation

aramase
Copy link
Member

@aramase aramase commented Jul 13, 2022 •
edited

Signed-off-by: Anish Ramasekar anish.ramasekar@gmail.com

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?

Encrypt data with DEK using AES-GCM instead of AES-CBC for kms data encryption. No user action required. Reads with AES-GCM and AES-CBC will continue to be allowed.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

https://kubernetes.io/docs/tasks/administer-cluster/kms-provider/

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 13, 2022
@aramase
Copy link
Member Author

aramase commented Jul 13, 2022

/sig auth
/hold

@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 13, 2022
@aramase
Copy link
Member Author

aramase commented Jul 13, 2022

/cc @enj @ritazh

@k8s-ci-robot k8s-ci-robot requested review from enj and ritazh July 13, 2022 18:04
sftim
sftim reviewed Jul 14, 2022
View reviewed changes
Copy link
Contributor

@sftim sftim left a 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
@sftim sftim mentioned this pull request Jul 14, 2022
Open
@sftim
Copy link
Contributor

sftim commented Jul 14, 2022 •
edited

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.

@sftim
Copy link
Contributor

sftim commented Jul 14, 2022

@aramase this is held - can you add a comment to clarify why and under what circumstances you'd expect to unhold?

@leilajal
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 14, 2022
@ritazh
Copy link
Member

ritazh commented Jul 20, 2022

perhaps update release note:

Encrypt data with DEK using AES-GCM instead of AES-CBC for kms data encryption. Not user action required. Reads with AES-GCM and AES-CBC will continue to be allowed.

@mikedanese
Copy link
Member

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.

@aramase
Copy link
Member Author

aramase commented Jul 20, 2022

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 20, 2022
@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 20, 2022
@aramase
Copy link
Member Author

aramase commented Jul 20, 2022

  • I've modified some of the tests we added in the previous release to simulate downgrade (write with AES CBC). @enj LMK what you think.
  • Updated the release note

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2022
@aramase aramase mentioned this pull request Jul 20, 2022
Closed
@aramase
Copy link
Member Author

aramase commented Jul 20, 2022

/milestone v1.25

@k8s-ci-robot
Copy link
Contributor

@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:

/milestone v1.25

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.

@sftim
Copy link
Contributor

sftim commented Jul 20, 2022

For the release note

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

then could we link to https://kubernetes.io/docs/tasks/administer-cluster/kms-provider/? That saves readers a job.

@sftim
Copy link
Contributor

sftim commented Jul 20, 2022

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)

@aramase aramase requested a review from mikedanese July 22, 2022 18:03
@aramase
Copy link
Member Author

aramase commented Jul 25, 2022

@enj @mikedanese This PR is ready for review. Could you take a look when you get a chance?

@aramase
Copy link
Member Author

aramase commented Jul 28, 2022

@mikedanese @enj Could we get this PR reviewed soon so we don't miss the v1.25 code freeze?

@aramase
Copy link
Member Author

aramase commented Aug 1, 2022

The v1.25 code freeze is tomorrow.

@mikedanese Could you review this so we can get it in for v1.25?

@jeremyrickard
Copy link
Contributor

/milestone v1.25

@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Aug 1, 2022
enj
enj suggested changes Aug 1, 2022
View reviewed changes
Copy link
Member

@enj enj left a 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
@aramase
feat:(kms) encrypt data with DEK using AES-GCM instead of AES-CBC
d54631a 
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@aramase aramase requested a review from enj August 2, 2022 01:14
@enj
Copy link
Member

enj commented Aug 2, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2022
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2022
@k8s-ci-robot k8s-ci-robot merged commit cdc6011 into kubernetes:master Aug 2, 2022
@aramase aramase deleted the aes-gcm-part-2 branch August 2, 2022 02:23
@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Aug 4, 2022
@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@liggitt liggitt Awaiting requested review from liggitt

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@ritazh ritazh Awaiting requested review from ritazh

@mikedanese mikedanese Awaiting requested review from mikedanese

@enj enj Awaiting requested review from enj

Assignees

@enj enj

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@aramase @sftim @leilajal @ritazh @mikedanese @k8s-ci-robot @jeremyrickard @enj @liggitt