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 key rotation guidelines for encryption at rest #39747

Closed
wants to merge 1 commit into from

Conversation

dgrisonnet
Copy link
Member

Add general guidelines for keys rotations and update the write-based requirements for aesgcm from 200 000 to 2^32. In addition to that, the documentation now includes a complete explanation of the logic behind these numbers.

Fixes #39477

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 1, 2023
@dgrisonnet
Copy link
Member Author

/sig auth
/cc @enj @ibihim @tkashem

@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. language/en Issues or PRs related to English language labels Mar 1, 2023
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Mar 1, 2023
@sftim
Copy link
Contributor

sftim commented Mar 1, 2023

/sig security

@k8s-ci-robot k8s-ci-robot added the sig/security Categorizes an issue or PR as relevant to SIG Security. label Mar 1, 2023
@sftim
Copy link
Contributor

sftim commented Mar 1, 2023

Hi. Please rebase this PR against main - there was a breaking change to our CVE feed.

Comment on lines 369 to 373
The smallest power of 2 for which this equation is satified is $2^{32}$

$$1-e^{-\frac{2^{32}}{2^{97}}} \approx 1.164\times{10}^{-10} \leq \frac{1}{2^{32}} \approx 2.32\times{10}^{-10} \leq 1-e^{-\frac{2^{33}}{2^{97}}} \approx 4.656\times{10}^{-10}$$
Copy link
Member Author

@dgrisonnet dgrisonnet Mar 1, 2023

Choose a reason for hiding this comment

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

I tried to solve the formula with k as the unknown and find its value based on p(k) <= 2^32, but couldn't get the result I wanted.
For the record, the logic to solve the birthday paradox with that kind of equation is explained in https://en.wikipedia.org/wiki/Birthday_problem under An upper bound on the probability and a lower bound on the number of people.

@netlify
Copy link

netlify bot commented Mar 1, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 99dc737
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/640f4f592fedff0008da8a75
😎 Deploy Preview https://deploy-preview-39747--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 settings.

@dgrisonnet
Copy link
Member Author

@sftim the math formulas are broken in the preview, do you know how I can fix them?

https://deploy-preview-39747--kubernetes-io-main-staging.netlify.app/docs/tasks/administer-cluster/encrypt-data/

@sftim
Copy link
Contributor

sftim commented Mar 1, 2023

@sftim the math formulas are broken in the preview, do you know how I can fix them?

https://deploy-preview-39747--kubernetes-io-main-staging.netlify.app/docs/tasks/administer-cluster/encrypt-data/

We don't support maths formulas. Consider filing a feature request (label it area/web-development)? I don't think enabling these should be hard, as upstream Docsy does have support.

@dgrisonnet
Copy link
Member Author

Could you point me to the part of the code I need to modify to support that? I might as well add the feature, while I am at it

@sftim
Copy link
Contributor

sftim commented Mar 1, 2023

Could you point me to the part of the code I need to modify to support that? I might as well add the feature, while I am at it

I'm afraid I haven't looked. Try https://www.google.com/search?q="docsy"+math+support

@dgrisonnet
Copy link
Member Author

I meant on the k/website side. But I can try to find my way in the repo.

@dgrisonnet
Copy link
Member Author

dgrisonnet commented Mar 1, 2023

Found it, I will update the PR, thanks for pointing out to docsy

@dgrisonnet dgrisonnet force-pushed the aesgcm branch 2 times, most recently from 125aeb2 to 4c6e33b Compare March 1, 2023 17:51
@dgrisonnet
Copy link
Member Author

@sftim adding support for scientific formulas is a bit more complex than what I had anticipated.

I originally tried enabling KaTeX in config.toml following this docsy documentation: https://www.docsy.dev/docs/adding-content/diagrams-and-formulae/, but for some reasons it didn't work for Kubernetes. I tried on a brand new hugo website and following the steps from the doc worked as expected, so it seems to be specific to Kubernetes.

The first thing I noticed is that the website is using an old version of docsy and KaTeX format as changed a bit since then. Here the correct doc for what we are trying to achieve: https://github.com/google/docsy/blob/v0.2.0/userguide/content/en/docs/Adding%20content/diagrams-and-formulae/index.md.

But even when following this v0.2.0 doc, the formulas weren't rendered properly and there wasn't any logs/info in the web console that I could help my investigation.

While looking into that, I also noticed that Kubernetes website supports mermaid and since the docsy configuration is very similar, I thought I would've a look there to help me.
To my surprise mermaid is not currently enabled via docsy but rather there is a custom shortocode for it: https://github.com/kubernetes/website/blob/main/layouts/shortcodes/mermaid.html.

So I tried my chance at configuring mermaid via docsy to see if my problem was specific to katex. However after following the doc and enabling mermaid in config.toml. I was greeted by a webpage not rendereding the diagram and the following error in the console:

Uncaught ReferenceError: mermaid is not defined
    at main.js:197:25
    at main.js:200:3
197: var settings = norm(mermaid.mermaidAPI.defaultConfig, params);

From that observation, my current theory is that enabling docsy plugins such as katex and mermaid in the main config.toml doesn't work in Kubernetes' website special case because the pieces of JS that should be imported in the web pages are not. Do you perhaps have any idea what could cause a conflict that would prevent importing these libraries?

As a temporary solution, would it perhaps make sense to create a new shortcode for katex similarly to what we are already doing for mermaid?

@sftim
Copy link
Contributor

sftim commented Mar 10, 2023

The Mermaid bug is #31960

Perhaps we can omit the formulas for this PR and open another PR to add those details as a follow-up?

@dgrisonnet
Copy link
Member Author

dgrisonnet commented Mar 10, 2023

The Mermaid bug is #31960

👍 it seems a bit different than what I experienced with katex, but the issue might be related.

Perhaps we can omit the formulas for this PR and open another PR to add those details as a follow-up?

I am not a fan of omitting the formulas since the goal of this PR is to update the numbers and be transparent about them. Would replacing the raw formulas by screenshots of the markdown rendering be considered good enough for now?

@sftim
Copy link
Contributor

sftim commented Mar 10, 2023

Try using SVG. Did you file an issue about adding support for maths? If not, please do that!

@dgrisonnet
Copy link
Member Author

Not yet since I wanted to solve it myself but it turns out to be a bit too complex. I'll do that once I've updated this PR.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2023
@k8s-ci-robot k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 13, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign krol3 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2023
Signed-off-by: Damien Grisonnet <dgrisonn@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2023
@dgrisonnet
Copy link
Member Author

/retest

@dgrisonnet
Copy link
Member Author

I fixed the rendering of the scientific formulas. The latest version of the doc can be found at https://deploy-preview-39747--kubernetes-io-main-staging.netlify.app/docs/tasks/administer-cluster/encrypt-data/.

@sftim @enj @ibihim can you have a look please.

@sftim
Copy link
Contributor

sftim commented Mar 14, 2023

Thanks for getting this PR ready.

Overall, this information about cryptoperiod is not directly part of the task, and there's enough of it that I think we've passed a threshold. I recommend making a new reference page and moving two things there from the existing task page:

  • the providers table
  • the new cryptoperiod information including equations

That will keep the task page focused on the task. It's OK to retain a summary of the providers in the task page; in fact I recommend it.

The URL of the new page could be https://kubernetes.io/docs/reference/encryption/at-rest-api/

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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 Apr 12, 2023

@dgrisonnet we'd like to have these changes, if you've got the capacity to update given the v1.27 release

@dgrisonnet
Copy link
Member Author

A couple of things piled up, but I'll try to finish that work asap

@aramase
Copy link
Member

aramase commented May 1, 2023

/assign @enj @smarterclayton

could you review this when you get a chance?

@kbhawkey
Copy link
Contributor

@dgrisonnet ,
Can you rebase your pull request or can I close it?
Thanks

@divya-mohan0209
Copy link
Contributor

Hello @dgrisonnet , we'd like to see this change merged. Would you have the capacity to work on this PR? Please let us know by 25th August, 2023, failing which we shall be closing the PR.

@dgrisonnet
Copy link
Member Author

Hi @divya-mohan0209, thank you for the reminder, I'll try to work on it again in the upcoming weeks.

@divya-mohan0209
Copy link
Contributor

Thank you for confirming @dgrisonnet

@reylejano
Copy link
Member

Hi @dgrisonnet , do you have some time to rebase this PR as there is a merge conflict.
If there is no rebase, a PR wrangler may close this PR

@kbhawkey
Copy link
Contributor

Hi @dgrisonnet .
I plan to close this pull request. When you have time to rebase, feel free to reopen the PR.
Thanks
/close

@k8s-ci-robot
Copy link
Contributor

@kbhawkey: Closed this PR.

In response to this:

Hi @dgrisonnet .
I plan to close this pull request. When you have time to rebase, feel free to reopen the PR.
Thanks
/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/test-infra 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. language/en Issues or PRs related to English language needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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
Archived in project
Development

Successfully merging this pull request may close these issues.

Security information about AES-GCM key rotation are misleading
9 participants