fix: use syscall.Chmod to correctly handle setgid/setuid/sticky bits in mountPermissions#1106
Conversation
…in mountPermissions os.Chmod with os.FileMode does not correctly map Unix setgid (02000), setuid (04000), or sticky (01000) bits because Go os.FileMode uses different bit positions for these flags. For example, mountPermissions of 02770 (setgid + rwxrwx) would only apply 0770 via os.Chmod, silently dropping the setgid bit. Switch to syscall.Chmod which takes raw Unix mode bits directly. Also use chmodIfPermissionMismatch in CreateVolume for consistency, and use 0777 for MkdirAll in NodePublishVolume since the subsequent chmodIfPermissionMismatch handles the actual permission setting. Ref 940
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix mountPermissions handling so that special mode bits (setuid/setgid/sticky) are correctly applied when provisioning/mounting NFS volumes.
Changes:
- Switch
chmodIfPermissionMismatchto use a raw-mode chmod implementation intended to preserve setuid/setgid/sticky bits. - Reuse
chmodIfPermissionMismatchfromCreateVolumefor consistent permission setting. - Create mountpoint directories with
0777prior to mount (relying on the subsequent chmod step for final permissions).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/nfs/utils.go | Updates chmod logic to use a raw-mode chmod call and adds rationale in comments. |
| pkg/nfs/controllerserver.go | Routes CreateVolume’s permission reset through chmodIfPermissionMismatch. |
| pkg/nfs/nodeserver.go | Creates target mount directory with 0777 instead of mountPermissions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a44de06 to
c514194
Compare
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
42c4d44 to
97a5632
Compare
97a5632 to
7537bb6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7537bb6 to
e978534
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…n mask, add special-bit tests - Split syscall.Chmod into platform-specific files (chmod_unix.go, chmod_windows.go) with build tags to avoid breaking Windows builds - Fix permission comparison mask in chmodIfPermissionMismatch to include setuid/setgid/sticky bits, not just os.ModePerm (0777) - Add fileModeToUnixMode helper to correctly convert raw Unix mode values to Go's os.FileMode representation - Add unit tests for special bits (02770, 01777, 04755) in chmod_unix_test.go
e978534 to
f213ed7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/retest |
|
/retest |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/cherrypick release-4.13 |
|
@andyzhangx: new pull request created: #1110 DetailsIn response to this:
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. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
When
mountPermissionsincludes setgid (e.g.02770), setuid, or sticky bits,os.Chmodwithos.FileModesilently drops these special bits because Goos.FileModeuses different bit positions than raw Unix mode bits.For example,
mountPermissions: "2770"(setgid + rwxrwx---) parsed as octal gives02770, butos.Chmod(path, os.FileMode(02770))only applies0770— the setgid bit is lost.This PR switches to
syscall.Chmodwhich takes raw Unix mode bits directly and correctly applies setgid/setuid/sticky bits.Changes:
pkg/nfs/chmod_unix.go: Platform-specificchmod()usingsyscall.Chmodfor correct special bit handlingpkg/nfs/chmod_windows.go: Windows fallback usingos.Chmod(special bits not supported on Windows)pkg/nfs/utils.go:unixModeToFileMode()to convert raw Unix mode to Goos.FileModefor accurate permission comparisonchmodIfPermissionMismatch()to useuint32parameter (raw Unix mode_t) and compare including setuid/setgid/sticky bitspkg/nfs/controllerserver.go: UsechmodIfPermissionMismatchin CreateVolume for consistencypkg/nfs/chmod_unix_test.go: Add tests for setgid (02770), sticky (01777), and setuid (04755) bitshack/boilerplate/boilerplate.py: Update regex to support//go:buildconstraint syntaxWhich issue(s) this PR fixes:
Ref #940
Does this PR introduce a user-facing change?