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

Update encryption-at-rest task page #33285

Closed

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Apr 28, 2022

Revise Encrypting Secret Data at Rest [preview]

This is an overall PR and I am splitting out smaller PRs - scroll down to see more

  • Use HTML table [preview] in place of harder-to-read Markdown version.
  • assume that encryption at rest is available; no need to check cluster version is compatible
  • clarify purpose of task
  • use more glossary tooltips
  • improve page headings
  • style tweaks

Split PRs (current / recent set):

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. language/en Issues or PRs related to English language labels Apr 28, 2022
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Apr 28, 2022
@netlify
Copy link

netlify bot commented Apr 28, 2022

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit c91ebf4
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6635077f5fd5b7000936e697
😎 Deploy Preview https://deploy-preview-33285--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@divya-mohan0209 divya-mohan0209 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor grammatical nit.

content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from ae0db33 to 98312e7 Compare May 21, 2022 21:27
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2022
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 98312e7 to 0320672 Compare June 26, 2022 20:47
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2022
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 0320672 to 98d87a4 Compare June 26, 2022 20:48
@sftim sftim changed the title [WIP] Update encryption-at-rest task page Update encryption-at-rest task page Jun 26, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2022
@sftim
Copy link
Contributor Author

sftim commented Jun 27, 2022

@divya-mohan0209 are you happy with the changes I made here?

@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 98d87a4 to dd1681a Compare June 27, 2022 08:33
Copy link
Contributor

@divya-mohan0209 divya-mohan0209 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few suggestions, @sftim . Hope they're helpful!

content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
is the first provider, the first key is used for encryption.
You can configure multiple providers, and each provider (other than `identity`, which does not encrypt) supports multiple keys.
For encryption, the API server uses the first configured key from the first provider.
For decryption, the API server tries each key in order for decryption, stopping when decryption succeeds.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we intend to say sequential order? I think making that clear might help.

Suggested change
For decryption, the API server tries each key in order for decryption, stopping when decryption succeeds.
For decryption, the API server tries each key in order for decryption, stopping when decryption succeeds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the different wording you're suggesting @divya-mohan0209 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what I intended to ask was "For decryption, the API server tries each key in order for decryption, stopping when decryption succeeds." is the order sequential?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope I covered this.

content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
@sftim sftim marked this pull request as draft July 12, 2022 21:48
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2022
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 7944b8a to 1e91951 Compare August 6, 2022 15:14
@sftim
Copy link
Contributor Author

sftim commented Aug 6, 2022

I'll rebase this against current main.

@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 1e91951 to 700d4b5 Compare August 6, 2022 15:15
@sftim sftim marked this pull request as ready for review August 6, 2022 15:15
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2022
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 700d4b5 to e0e9537 Compare August 6, 2022 15:47

Some of the APIs in Kubernetes, such as {{< glossary_tooltip text="Secret" term_id="secret" >}},
support at-rest encryption. This at-rest encryption is additional to any system-level
encryption for the etcd cluster or hosts where the kube-apiserver stores data persistently.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
encryption for the etcd cluster or hosts where the kube-apiserver stores data persistently.
encryption for the etcd cluster or hosts where etcd stores data persistently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sftim we're trying to make a distinction between remote & in-house (for lack of a better word) etcd clusters here, yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't think this is about stacked etcd vs a separate cluster.

I think the point is that encryption at rest might also apply to, for example, audit logs written to the host filesystem by a static API server Pod.

If we make that change, then we're changing the meaning and would be leaving out that other scenario (logs, etc).

content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
Comment on lines 50 to 51
Check whether the `kube-apiserver` process is running with the `--encryption-provider-config`
command line argument is set. If it is not, you do not have encryption at rest is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sentences like these could be simplified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yes. If we can merge this as an improvement without rewriting, that's handy.

content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
Then the first-listed provider for a resource is something **other** than `identity`,
then any new information written to resources of that type will be encrypted as configured.

If you are not sure about the progress of any previous migration to encrypt data at rest,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph doesn't flow. I don't know why we suddenly jump to this sub-topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you able to recommend a different place to put this advice @tengqm ? If so, I can move it there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this warrants a dedicated subsection, "migrating to encryption at rest".

content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
Comment on lines 242 to 243
Consider whether you need to make a backup of that configuration. If you do,
also think about how you will ensure that your backup is secure.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer reading simple clear text over this. I mean:

You may want to create a secure backup of the configuration file.

```

…and then restart each API server in turn. This change prevents the API server
from accessing a plain-text Secret, even by accident.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may and may not be a good practice. It depends.
If I'm using one provider today and I'm migrating to a different provider tomorrow, keeping identity there may be convenient. I don't know. My gut feeling is that an assertion like this should be made carefully.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is it not a good practice to remove the identity provider?


When running a single `kube-apiserver` instance, step 2 may be skipped.
1. Generate a new key and add it as the second key entry for the current provider on all
control plane hosts (strictly speaking: on all hosts that run a `kube-apiserver`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
control plane hosts (strictly speaking: on all hosts that run a `kube-apiserver`).
nodes that run a `kube-apiserver` process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My wording avoids assuming that the kube-apiserver is running inside Pods. I will try to find another wording that also works for the common case where there are control plane nodes.

content/en/docs/tasks/administer-cluster/encrypt-data.md Outdated Show resolved Hide resolved
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 3e76938 to 6a235f3 Compare January 17, 2024 23:12
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 6a235f3 to 5673fc1 Compare January 19, 2024 00:31
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 5673fc1 to d9756d9 Compare January 28, 2024 12:59
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from d9756d9 to b65a904 Compare January 30, 2024 15:05
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 30, 2024
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from b65a904 to 2c40443 Compare January 30, 2024 15:14
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 30, 2024
@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 2c40443 to 1ecc17d Compare January 30, 2024 15:27
@Pxyzi3c

This comment was marked as spam.

@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 1ecc17d to 734cff8 Compare February 14, 2024 11:46
@sftim
Copy link
Contributor Author

sftim commented Mar 31, 2024

To move this forward, please review #44622, or suggest another part of the page that I can split out.

@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 734cff8 to 19ce31d Compare April 1, 2024 14:43
@sftim
Copy link
Contributor Author

sftim commented Apr 1, 2024

I'd be happy to see these changes land in one batch, if that's feasible; if not, please see PR #44622

Comment on lines +77 to +79
{{< note >}}
For cluster configurations with two or more control plane nodes, the encryption configuration
should be identical across each control plane node.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is repeated as a caution block later in the file (under the heading ”Reconfigure other control plane hosts”).

I think the advice bears repeating. Get this wrong and you can really end up regretting it.

Comment on lines 596 to 598
When you are planning to update the encryption configuration of your cluster, plan this
so that the API servers in your control plane can always decrypt the stored data
(even part way through rolling out the change).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This advice is new as of today.

@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 19ce31d to 8bba96e Compare May 1, 2024 11:30
@sftim
Copy link
Contributor Author

sftim commented May 1, 2024

See #44622 which I recommend reviewing before this one.

@sftim sftim force-pushed the 20220428_revise_encryption_at_rest branch from 8bba96e to c91ebf4 Compare May 3, 2024 15:49
@sftim sftim marked this pull request as draft May 16, 2024 00:57
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 16, 2024
@sftim
Copy link
Contributor Author

sftim commented Jul 25, 2024

Doubt I'll find time to finish this.

/close

@k8s-ci-robot
Copy link
Contributor

@sftim: Closed this PR.

In response to this:

Doubt I'll find time to finish this.

/close

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/security Categorizes an issue or PR as relevant to SIG Security. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants