Skip to content

Security: Harden file and directory permissions for webhook certificates#2420

Open
rakshaak29 wants to merge 2 commits intoopenkruise:masterfrom
rakshaak29:fix/webhook-cert-permissions
Open

Security: Harden file and directory permissions for webhook certificates#2420
rakshaak29 wants to merge 2 commits intoopenkruise:masterfrom
rakshaak29:fix/webhook-cert-permissions

Conversation

@rakshaak29
Copy link
Copy Markdown
Contributor

Ⅰ. Describe what this PR does

This PR hardens the security of the webhook certificate writer by reducing overly permissive file and directory permissions in pkg/webhook/util/writer/fs.go. It also addresses TODO comments left by developers regarding whether the permissions could be reduced.

Changes made:

  • Reduced webhook certificate directory permissions from 0777 (world-writable) to 0750.
  • Reduced generated private key permissions (CAKey, ServerKey, ServerKey2) from 0666 (world-readable/writable) to 0600.
  • Reduced generated public certificate permissions (CACert, ServerCert, ServerCert2) from 0666 to 0640.

Ⅱ. Does this pull request fix one issue?

NONE

(Note: If you ended up creating an issue with the previous template, replace NONE with fixes #<YOUR_ISSUE_NUMBER>)

Ⅲ. Describe how to verify it

  1. Run the package unit tests to verify they are still successfully able to read/write the certs:
    go test ./pkg/webhook/util/writer/...

Copilot AI review requested due to automatic review settings April 23, 2026 09:13
@kruise-bot kruise-bot requested review from FillZpp and furykerry April 23, 2026 09:13
@kruise-bot
Copy link
Copy Markdown

[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 furykerry for approval by writing /assign @furykerry in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@kruise-bot kruise-bot added the size/S size/S 10-29 label Apr 23, 2026
@rakshaak29 rakshaak29 force-pushed the fix/webhook-cert-permissions branch from fec72bd to fa08c65 Compare April 23, 2026 09:17
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Hardens the webhook certificate filesystem writer by reducing overly permissive directory and file modes used when creating/writing certificates.

Changes:

  • Reduce certificate directory creation mode from 0777 to 0750.
  • Reduce private key file modes from 0666 to 0600.
  • Reduce certificate file modes from 0666 to 0640.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/webhook/util/writer/fs.go
@rakshaak29 rakshaak29 force-pushed the fix/webhook-cert-permissions branch from fa08c65 to 54d88fb Compare April 23, 2026 09:30
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.42%. Comparing base (749e8f2) to head (3ffc324).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2420      +/-   ##
==========================================
+ Coverage   48.77%   49.42%   +0.64%     
==========================================
  Files         324      325       +1     
  Lines       27928    27950      +22     
==========================================
+ Hits        13623    13814     +191     
+ Misses      12775    12553     -222     
- Partials     1530     1583      +53     
Flag Coverage Δ
unittests 49.42% <100.00%> (+0.64%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rakshaak29 rakshaak29 force-pushed the fix/webhook-cert-permissions branch from 54d88fb to 9952137 Compare April 23, 2026 16:45
@kruise-bot kruise-bot added size/L size/L: 100-499 and removed size/S size/S 10-29 labels Apr 23, 2026
@rakshaak29 rakshaak29 force-pushed the fix/webhook-cert-permissions branch from af9fb62 to 491e174 Compare April 23, 2026 17:52
Resolves TODO comments in pkg/webhook/util/writer/fs.go by tightening
overly permissive file and directory permissions for webhook certificates:

- Directory: 0777 -> 0750
- Private keys (CAKey, ServerKey): 0666 -> 0600
- Certificates (CACert, ServerCert): 0666 -> 0640
- Wrap underlying error in directory creation failure message

Also adds unit tests for prepareToWrite, certToProjectionMap, and
FSCertWriter to improve test coverage.

Fixes Ginkgo version mismatch in CI by pinning Ginkgo CLI to v2.27.5
in the Makefile to match the go.mod version.

Signed-off-by: rakshaak <rakshaak29@gmail.com>
Signed-off-by: rakshaak29 <rakshaak29@gmail.com>
@rakshaak29 rakshaak29 force-pushed the fix/webhook-cert-permissions branch from 491e174 to 0e5e23b Compare April 24, 2026 07:29
Signed-off-by: rakshaak29 <rakshaak29@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L size/L: 100-499

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants