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 Kubernetes PKI certificate file permissions #11069

Conversation

bmelbourne
Copy link
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

Align Kubernetes PKI certificate file permissions with key file permissions and reduce exposure of Kubernetes PKI certificates to non-root users, hence improving cluster security, specifically CIS Kubernetes benchmark compliance.

Summary Report for compliance: CIS Kubernetes Benchmarks v1.23
┌────────┬──────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────┬────────┬────────┐
│   ID   │ Severity │                                                  Control Name                                                   │ Status │ Issues │
├────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────┼────────┤
│ 1.1.20 │ CRITICAL │ Ensure that the Kubernetes PKI certificate file permissions are set to 600 or more restrictive                  │  FAIL  │   3    │
└────────┴──────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────┴────────┴────────┘

Which issue(s) this PR fixes:

None.

Special notes for your reviewer:

None.

Does this PR introduce a user-facing change?:

None

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 9, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @bmelbourne. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 9, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bmelbourne
Once this PR has been reviewed and has the lgtm label, please assign oomichi 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/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 9, 2024
@yankay
Copy link
Member

yankay commented Apr 10, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 10, 2024
@bmelbourne
Copy link
Contributor Author

@yankay
Hi, can you restart the CI pipeline please as it appears to be stuck?

@MrFreezeex
Copy link
Member

Hi, thanks for your contribution however I am not entirely sure this is the right fix.

First of all the severity score "critical" of the benchmark you provided looks kind of dubious to me, restricting a certificate access looks weird I don't really see the security issue here. However there is indeed no point that other users access this, so I would suggest to try changing the permission of the cert dir in kubernetes/preinstall/tasks/0050-create_directories.yml instead which should solve potential miss configuration there as well.

@bmelbourne bmelbourne force-pushed the update-pki-cert-file-permissions branch from a5727c8 to b1146be Compare April 18, 2024 14:04
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 18, 2024
@bmelbourne
Copy link
Contributor Author

bmelbourne commented Apr 18, 2024

@MrFreezeex
Based on your recommendation, I've updated kubernetes/preinstall/tasks/0050-create_directories.yml to limit the Kubernetes PKI directory permissions {{ kube_cert_dir }} to root user only. I've tested the code updates and as the updated directory permissions have no effect, the previous code snippet to update the Kubernetes PKI certificate file permissions needs to remain in place.

I've also refactored some of the other tasks to improve consistency.

@bmelbourne
Copy link
Contributor Author

bmelbourne commented Apr 18, 2024

@yankay
Do we know why the Kubespray CI - packet_centos7-calico-ha-once-localhost job is failing as no error is being reported? Are other Kubespray PRs impacted by the same issue?

Running after_script
Running after script...
$ chronic ./tests/scripts/testcases_cleanup.sh
Uploading artifacts for failed job
Uploading artifacts...
cluster-dump: found 3 matching artifact files and directories 
Uploading artifacts as "archive" to coordinator... 201 Created  id=6660609784 responseStatus=201 Created token=glcbt-65
Cleaning up project directory and file based variables
ERROR: Job failed: command terminated with exit code 1

@bmelbourne bmelbourne force-pushed the update-pki-cert-file-permissions branch from b1146be to 7b6c6bf Compare April 23, 2024 11:20
@MrFreezeex
Copy link
Member

@MrFreezeex Based on your recommendation, I've updated kubernetes/preinstall/tasks/0050-create_directories.yml to limit the Kubernetes PKI directory permissions {{ kube_cert_dir }} to root user only. I've tested the code updates and as the updated directory permissions have no effect, the previous code snippet to update the Kubernetes PKI certificate file permissions needs to remain in place.

I've also refactored some of the other tasks to improve consistency.

Thanks! Could you only do keep the changes in the preinstall role and not in roles/kubernetes/kubeadm/tasks/main.yml though?

@bmelbourne
Copy link
Contributor Author

@MrFreezeex Based on your recommendation, I've updated kubernetes/preinstall/tasks/0050-create_directories.yml to limit the Kubernetes PKI directory permissions {{ kube_cert_dir }} to root user only. I've tested the code updates and as the updated directory permissions have no effect, the previous code snippet to update the Kubernetes PKI certificate file permissions needs to remain in place.
I've also refactored some of the other tasks to improve consistency.

Thanks! Could you only do keep the changes in the preinstall role and not in roles/kubernetes/kubeadm/tasks/main.yml though?

The code changes are still required in order to restrict certificate file permissions to the root user only and pass the CIS Kubernetes benchmark compliance report.

@bmelbourne bmelbourne force-pushed the update-pki-cert-file-permissions branch from 7b6c6bf to 50546c5 Compare May 3, 2024 10:11
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 3, 2024
@bmelbourne bmelbourne force-pushed the update-pki-cert-file-permissions branch from 50546c5 to 7489554 Compare May 3, 2024 10:22
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 3, 2024
@bmelbourne bmelbourne force-pushed the update-pki-cert-file-permissions branch from 7489554 to b5e1477 Compare May 3, 2024 10:24
@bmelbourne bmelbourne force-pushed the update-pki-cert-file-permissions branch from b5e1477 to 706d7f9 Compare May 3, 2024 10:25
@VannTen
Copy link
Contributor

VannTen commented May 3, 2024

Two things:

  • the .crt do not contain secret material, do they (if they do, that's different) ? What it the point to restricting their permissions, according to CIS ?
  • Those are created by kubeadm, correct ? In that case kubeadm should handle those permissions, preferably.

@bmelbourne bmelbourne closed this May 3, 2024
@bmelbourne bmelbourne deleted the update-pki-cert-file-permissions branch May 3, 2024 11:03
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. kind/bug Categorizes issue or PR as related to a bug. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants