feat: Allow Fileownership change through FSGroup and VOLUME_MOUNT_GROUP - #1841
feat: Allow Fileownership change through FSGroup and VOLUME_MOUNT_GROUP#1841mytreya-rh wants to merge 1 commit into
Conversation
k8s-ci-robot
added
the
kind/feature
|
|
k8s-ci-robot
added
cncf-cla: no
|
Welcome @mytreya-rh! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @mytreya-rh. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
k8s-ci-robot
added
size/XXL
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
898943d to
b797f9d
Compare
|
/retest |
There was a problem hiding this comment.
The windows job failures are related to this PR.
E0630 16:56:06.282028 10108 atomic_writer.go:419] "unable to change file with owner" err="chown c:\\var\\lib\\kubelet\\pods\\ff425598-c3fa-480d-a6af-814831673629\\volumes\\kubernetes.io~csi\\secrets-store-inline\\mount\\..2025_06_30_16_56_06.1168464543\\secretalias: not supported by windows" logContext="secrets-store-csi-driver" fullPath="c:\\var\\lib\\kubelet\\pods\\ff425598-c3fa-480d-a6af-814831673629\\volumes\\kubernetes.io~csi\\secrets-store-inline\\mount\\..2025_06_30_16_56_06.1168464543\\secretalias" owner=-1
github-project-automation
Bot
moved this from In Review
to Changes Requested
in SIG Auth
b797f9d to
cbac857
Compare
Thanks @aramase ! |
|
/retest |
dobsonj
left a comment
dobsonj
left a comment
There was a problem hiding this comment.
I have one comment on test/bats/e2e-provider.bats, but otherwise LGTM. It's a useful fix, implementation looks correct, good test coverage, and passing CI tests. Netlify is warning about a line unrelated from your changes.
| kubectl wait -n rotation --for=condition=Ready --timeout=60s pod ${curl_pod_name} | ||
| local pod_ip=$(kubectl get pod -n kube-system -l app=csi-secrets-store-e2e-provider -o jsonpath="{.items[0].status.podIP}") | ||
| run kubectl exec ${curl_pod_name} -n rotation -- curl http://${pod_ip}:8080/rotation?rotated=true | ||
| sleep 35 # 30 is poll interval, 5 second grace should be enough |
There was a problem hiding this comment.
I worry that 35 seconds may not be enough to prevent flakes. In @test "Test auto rotation of mount contents and K8s secrets" (line 472) it used to sleep 60 seconds, but now it only sleeps 35 seconds? Is it possible for a reconcile loop to be delayed for some reason that would cause this to take longer than 35?
I would probably not reduce this below 60, we had one similar case in vault.bats waiting on secret rotation where we had to increase it to 120 to improve the pass rate.
|
/retest |
k8s-ci-robot
added
the
sig/storage
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1841 +/- ##
==========================================
+ Coverage 21.47% 22.31% +0.83%
==========================================
Files 57 57
Lines 3269 3218 -51
==========================================
+ Hits 702 718 +16
+ Misses 2476 2407 -69
- Partials 91 93 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8618850 to
40f47db
Compare
|
/retest
|
| "k8s.io/klog/v2" | ||
| mount "k8s.io/mount-utils" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| internalerrors "sigs.k8s.io/secrets-store-csi-driver/pkg/errors" |
There was a problem hiding this comment.
nit: group commits
import (
stdlib
internal
external
)There was a problem hiding this comment.
Done, makes it better organized. Thanks
| if len(fsGroupStr) == 0 { | ||
| return NoGID, nil | ||
| } | ||
| return strconv.Atoi(fsGroupStr) |
There was a problem hiding this comment.
strconv.Atoi accepts negative values. The test even validates -23 as a valid GID. A negative GID other than -1 passed to os.Chown is undefined behavior on Linux. Kubelet should never send a negative value, but we should still reject it here.
func ParseFSGroup(fsGroupStr string) (int, error) {
if len(fsGroupStr) == 0 {
return NoGID, nil
}
gid, err := strconv.Atoi(fsGroupStr)
if err != nil {
return NoGID, err
}
if gid < 0 {
return NoGID, fmt.Errorf("invalid FSGroup: %d must be non-negative", gid)
}
return gid, nil
}Update the negative gid test case to expect an error.
There was a problem hiding this comment.
Agree, and done.
Earlier, my intention was to keep complete type compatibility/extensibility, and let the implementation (os.Chown) handle the full range that it supports, but as fsGroup is validated at API to be in range: 0 to 2147483647, disallowing the negative values like you suggested.
| # On Windows, the failed unmount calls from: https://github.com/kubernetes-sigs/secrets-store-csi-driver/pull/545 | ||
| # do not prevent the pod from being deleted. Search through the driver logs | ||
| # for the error. | ||
| run bash -c "kubectl -n $NAMESPACE logs -l app=$POD_NAME --tail -1 -c secrets-store | grep '^E.*failed to clean and unmount target path.*$'" |
There was a problem hiding this comment.
-l app=$POD_NAME doesn't match anything — the test pods don't have that label. This means kubectl logs returns empty, grep always fails, and assert_failure always passes. The unmount error check is effectively a no-op.
Should be the driver DaemonSet pods:
| run bash -c "kubectl -n $NAMESPACE logs -l app=$POD_NAME --tail -1 -c secrets-store | grep '^E.*failed to clean and unmount target path.*$'" | |
| run bash -c "kubectl logs -l app=secrets-store-csi-driver --tail -1 -c secrets-store -n kube-system | grep '^E.*failed to clean and unmount target path.*$'" |
There was a problem hiding this comment.
oops, that was such bad refactoring. Thanks for catching it, corrected.
| csiPodName: "pod1", | ||
| csiPodNamespace: "default", | ||
| csiPodUID: "poduid1", | ||
| }, |
There was a problem hiding this comment.
nit: default VolumeCapability has nil AccessType. Works today because GetMount() on nil returns nil and GetVolumeMountGroup() on nil returns "". Fragile if we ever add a nil guard.
VolumeCapability: &csi.VolumeCapability{
AccessType: &csi.VolumeCapability_Mount{
Mount: &csi.VolumeCapability_MountVolume{},
},
},| # enable rotation response in mock server | ||
| local curl_pod_name=curl-$(openssl rand -hex 5) | ||
| kubectl run ${curl_pod_name} -n rotation --image=curlimages/curl:7.75.0 --labels="test=rotation" -- tail -f /dev/null > /dev/null | ||
| kubectl wait -n rotation --for=condition=Ready --timeout=60s pod ${curl_pod_name} > /dev/null |
There was a problem hiding this comment.
| kubectl wait -n rotation --for=condition=Ready --timeout=60s pod ${curl_pod_name} > /dev/null | |
| kubectl wait -n rotation --for=condition=Ready --timeout=60s pod ${curl_pod_name} |
There was a problem hiding this comment.
i think we need the redirection to /dev/null so that the function just returns the curl_pod_name right?
ex:
$k wait -n kube-system --for=condition=Ready --timeout=60s pod coredns-6f6b679f8f-6vqqg
pod/coredns-6f6b679f8f-6vqqg condition met
$
$ k wait -n kube-system --for=condition=Ready --timeout=60s pod coredns-6f6b679f8f-6vqqg >/dev/null
$
| kubectl run ${curl_pod_name} -n rotation --image=curlimages/curl:7.75.0 --labels="test=rotation" -- tail -f /dev/null > /dev/null | ||
| kubectl wait -n rotation --for=condition=Ready --timeout=60s pod ${curl_pod_name} > /dev/null |
There was a problem hiding this comment.
If kubectl run or kubectl wait fails, the function silently continues and you get a confusing downstream failure. Add || return 1 after the critical commands.
There was a problem hiding this comment.
Agree, now returning 1 on kubectl errors, so that the function call results in error in the caller's scope
| FsUser *int64 | ||
| Data []byte | ||
| Mode int32 | ||
| FsGroup *int |
There was a problem hiding this comment.
nit: upstream uses FsUser *int64. This changes both the name and type — both intentional. Add a short comment noting the divergence so future readers don't think it drifted by accident.
There was a problem hiding this comment.
Actually added the comment in the file header. and now added reasoning for type change as well.
Shall i move it to the struct definition instead?
|
/retest Error: INSTALLATION FAILED: Kubernetes cluster unreachable: Get "https://sscsi-e2e--sscsi-e2e-86ed-46678f-t4lfap2y.hcp.uksouth.azmk8s.io:443/version": dial tcp: lookup sscsi-e2e--sscsi-e2e-86ed-46678f-t4lfap2y.hcp.uksouth.azmk8s.io on 172.20.0.10:53: no such host |
aramase
left a comment
aramase
left a comment
There was a problem hiding this comment.
Hopefully last set of comments.
| # On Windows, the failed unmount calls from: https://github.com/kubernetes-sigs/secrets-store-csi-driver/pull/545 | ||
| # do not prevent the pod from being deleted. Search through the driver logs | ||
| # for the error. | ||
| run bash -c "kubectl -n $NAMESPACE logs -l app=secrets-store-csi-driver --tail -1 -c secrets-store | grep '^E.*failed to clean and unmount target path.*$'" |
There was a problem hiding this comment.
-n $NAMESPACE queries the test namespace (e.g. default, test-v1alpha1), but the driver DaemonSet pods run in kube-system. This means kubectl logs finds no pods in the test namespace, grep fails, and assert_failure always passes — making this
check a no-op.
The original code had -n kube-system:
| run bash -c "kubectl -n $NAMESPACE logs -l app=secrets-store-csi-driver --tail -1 -c secrets-store | grep '^E.*failed to clean and unmount target path.*$'" | |
| run bash -c "kubectl logs -l app=secrets-store-csi-driver --tail -1 -c secrets-store -n kube-system | grep '^E.*failed to clean and unmount target path.*$'" |
There was a problem hiding this comment.
done, thanks again for catching this oversight.
Also simplified the semantics of passing file permissions to create_spc function in this file.
| if err != nil || mode > 511 { | ||
| return nil, fmt.Errorf("invalid filePermission: %s, error: %w for file: %s", mockSecretsStoreObject.FilePermission, err, mockSecretsStoreObject.ObjectName) | ||
| } |
There was a problem hiding this comment.
When mode > 511 but err == nil, this wraps a nil error with %w which prints <nil> in the message. Split the conditions:
if err != nil {
return nil, fmt.Errorf("invalid filePermission: %s, error: %w for file: %s", mockSecretsStoreObject.FilePermission, err, mockSecretsStoreObject.ObjectName)
}
if mode > 511 {
return nil, fmt.Errorf("invalid filePermission: %s exceeds 0777 for file: %s", mockSecretsStoreObject.FilePermission, mockSecretsStoreObject.ObjectName)
}There was a problem hiding this comment.
Done, also changed mode > 511 to mode > 0o777 for better readability
| } | ||
|
|
||
| klog.V(2).InfoS("node publish volume", "target", targetPath, "volumeId", volumeID, "mount flags", mountFlags) | ||
| klog.V(2).InfoS("node publish volume", "target", targetPath, "volumeId", volumeID, "mount flags", mountFlags, "volumeCapabilities", req.VolumeCapability.String()) |
There was a problem hiding this comment.
nit: req.VolumeCapability.String() dumps the entire proto including mount flags, access mode, etc. If the intent is just to log the FSGroup, consider logging mountVol.GetVolumeMountGroup() after parsing it instead. The full capability proto can be noisy
in production logs.
There was a problem hiding this comment.
Done, now only logging VolumeMountGroup.
However, not using the parsed value but the value obtained in the NodePublishVolume arguments, as it could help in better debugging if for some reason the parse function is not working as expected.
| // * tag: v1.20.6, | ||
| // * commit: 8a62859e515889f07e3e3be6a1080413f17cf2c3 | ||
| // * link: https://github.com/kubernetes/kubernetes/blob/8a62859e515889f07e3e3be6a1080413f17cf2c3/pkg/volume/util/atomic_writer.go | ||
| // In addition, FileProjection::FSUser has been changed to FileProjection::FSGroup |
There was a problem hiding this comment.
The header comment is fine but could you also add a one-liner at the struct itself? That's where people will look when they see FsGroup *int and wonder why it doesn't match upstream's FsUser *int64:
// FileProjection contains file Data and access Mode.
// FsGroup diverges from upstream's FsUser (*int64) — see file header for rationale.
type FileProjection struct {
|
/retest
|
|
/retest
|
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
43f6852 to
e08b33d
Compare
|
/unhold
|
k8s-ci-robot
removed
the
do-not-merge/hold
740df77 to
b4862da
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds FSGroup-based file ownership support for mounted secret contents by advertising the CSI VOLUME_MOUNT_GROUP capability and applying the provided GID when writing files. It also extends the e2e mock provider and Bats tests to validate non-root pods reading non-world-readable secrets when fsGroup is set.
Changes:
- Advertise
csi.NodeServiceCapability_RPC_VOLUME_MOUNT_GROUP, parseVolumeMountGroup, and propagate it through the mount/write pipeline toos.Chown(..., gid). - Extend the e2e provider to accept per-object
filePermissionand update Bats fixtures/tests to cover non-root + FSGroup scenarios. - Add
ParseFSGroup/NoGIDhelpers and update unit tests for the new parameters/signatures.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pkg/secrets-store/nodeserver.go |
Parses VolumeMountGroup, passes GID through mount flow, and advertises VOLUME_MOUNT_GROUP. |
pkg/secrets-store/provider_client.go |
Threads GID into WritePayloads when writing mounted files. |
pkg/util/fileutil/writer.go |
Extends WritePayloads to accept a GID and attach it to projections for atomic writer. |
pkg/util/fileutil/atomic_writer.go |
Applies group ownership (os.Chown(..., -1, gid)) to written files (non-Windows). |
pkg/util/fileutil/filesystem.go / filesystem_test.go |
Adds NoGID and ParseFSGroup with tests. |
pkg/util/fileutil/writer_test.go |
Updates tests for new WritePayloads(..., gid) signature. |
pkg/secrets-store/provider_client_test.go |
Updates tests for new MountContent(..., gid) signature. |
pkg/secrets-store/nodeserver_test.go |
Refactors setup helpers and adds FSGroup parsing/error test coverage. |
test/e2eprovider/types/types.go |
Adds filePermission to mock object schema for e2e provider. |
test/e2eprovider/server/server.go |
Parses filePermission and sets returned file mode for mounted files. |
test/bats/tests/e2e_provider/pod-secrets-store-inline-volume-crd.yaml |
Parameterizes pod/SPC names and pod/container security contexts for FSGroup scenarios. |
test/bats/tests/e2e_provider/e2e_provider_secretproviderclass.yaml |
Parameterizes SPC name and per-object file modes used by the e2e provider. |
test/bats/tests/e2e_provider/e2e_provider_v1alpha1_secretproviderclass.yaml |
Removed; replaced by parameterized SPC manifest usage. |
test/bats/e2e-provider.bats |
Adds helper functions and new non-root/FSGroup e2e tests; refactors repeated logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function disable_secret_rotation() { | ||
| local curl_pod_name=$1 | ||
| local pod_ip=$(kubectl get pod -n kube-system -l app=csi-secrets-store-e2e-provider -o jsonpath="{.items[0].status.podIP}") | ||
| run kubectl exec ${curl_pod_name} -n rotation -- curl http://${pod_ip}:8080/rotation?rotated=false | ||
| } |
There was a problem hiding this comment.
Thanks for the catch, @copilot
The pod creation is now moved to setup_file and the same POD will be reused across the various rotation tests. The pod will be deleted in teardown when the namespace gets deleted.
This way we wont create and delete the POD multiple times.
Instead of run kubectl, the disable_secret_rotation function just executes kubectl directly to not let the error go unnoticed.
| POD_NAME="non-root-with-no-fsgroup" delete_pod | ||
| } | ||
|
|
||
| @test "Non-root POD with FSGroup - create" { |
There was a problem hiding this comment.
Missing one case here: mode 0600 with fsGroup should still fail to read. That's the test that proves the chown only touches the group bit and not the user bit — the exact property that would silently break if a future refactor mis-orders the args to os.Chown(fullPath, -1, *fileProjection.FsGroup). Without it, swapping those args goes unnoticed: mode 0640 still works (group readable either way once you own it), and there's no negative test catching the regression.
Suggested addition:
@test "deploy e2e-provider v1 secretproviderclass crd with owner-only permissions" {
SPC_NAME="e2e-provider-600" SECRET_MODE=0600 KEY_MODE=0600 create_spc
}
@test "Non-root POD with FSGroup - should fail to read owner-only secret" {
SPC_NAME="e2e-provider-600" POD_NAME="non-root-with-fsgroup-600" POD_SECURITY_CONTEXT='"runAsNonRoot": true, "runAsUser": 1004, "runAsGroup": 1004, "fsGroup": 1004' create_pod
POD_NAME="non-root-with-fsgroup-600" run read_secret
assert_failure
}
@test "Non-root POD with FSGroup - owner-only unmount succeeds" {
POD_NAME="non-root-with-fsgroup-600" delete_pod
}There was a problem hiding this comment.
Thanks @aramase included the above test and its working as expected.
aramase
left a comment
aramase
left a comment
There was a problem hiding this comment.
@mytreya-rh just one comment and also PTAL at the copilot review comment.
b4862da to
4e3ef66
Compare
Implements the CSI NodeServiceCapability RPC_VOLUME_MOUNT_GROUP so that mounted secret files are chown'd to the pod's FSGroup. This allows secrets to be not world-readable in non-root containers. 1. nodeServer::NodeGetCapabilities() advertise VOLUME_MOUNT_GROUP 2. nodeServer::NodePublishVolume() get POD's FSGroup if any from: req.VolumeCapability.GetMount().GetVolumeMountGroup() 3. pass the fsgroup onto (writer.go) WritePayloads() 4. include the FSGroup in the FileProjection struct (rename FileProjection::FSUser as FSGroup) 5. change AtomicWriter::writePayloadToDir() to chown the group based on FSGroup 6. Add relevant Unit tests, and e2eprovider tests 7. Bit of refactoring in the unit and e2eprovider tests to make them more terse
4e3ef66 to
a917b86
Compare
|
Hi, what's the status of this PR that seems to help a lot for files that need 600 ownership |
hi @Nemric This PR helps non-root containers mount and use non world-readable secrets. But, as of now, the FSUser which is derived from the runAsUser field of the POD/Container spec is NOT passed to CSIDrivers and changing the USER ownership will require changes in kubelet and perhaps in the NodePublishVolume API. |
|
would that help ? kubernetes/kubernetes#139764 |
surely.. thanks for sharing |
|
Would be great to see this merged, it looks ready to go - but let me know if I can help with anything! |
What type of PR is this?
/kind feature
What this PR does / why we need it:
(As of now pls consider it as a draft PR to discuss the solution further.)
Allows the secrets to be mounted with FSGroup as specified in the POD spec.
Thus, A pod with a non-root user should be able to read a secret, and that secret need not be world-readable.
Which issue(s) this PR fixes :
Fixes #858
Is this a chart or deployment yaml update?
There is a yaml update for secrets-store.csi.x-k8s.io_secretproviderclasspodstatuses.yaml (generated through make manifests).
It is added in the manifest_staging/deploy
But if this PR merges after: #1622, the change in SecretProviderClassPodStatusStatus won't be required anymore and we can revert the changes related to reconciler.
Special notes for your reviewer:
Problem:
Solution:
Notes:
tests added in e2e-provider:
unit tests:
TODOs: