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 intokubernetes-sigs:mainfrom
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
mytreya-rh
force-pushed
the
allow_file_ownership
branch
from
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
mytreya-rh
force-pushed
the
allow_file_ownership
branch
from
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.
test/bats/e2e-provider.bats
Outdated
| 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
mytreya-rh
force-pushed
the
allow_file_ownership
branch
from
e47407a to
9495080
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/retest Looks like infra issues:
|
mytreya-rh
force-pushed
the
allow_file_ownership
branch
from
9495080 to
da34a62
Compare
|
/retest as failure in windows job seems to be infra related:
|
|
/retest
|
|
/retest
|
pkg/constants/constants.go
Outdated
| /* | ||
| Copyright 2025 The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ |
There was a problem hiding this comment.
got rid of this package altogether as per later suggestion.
| } | ||
|
|
||
| if fileProjection.FsUser == nil { | ||
| if fileProjection.FsGroup == nil || runtimeutil.IsRuntimeWindows() { |
There was a problem hiding this comment.
FsGroup is always set to &gid even when gid == constants.NoGID (-1). This means FsGroup is never nil, so the nil check never triggers on Linux -- every mount without FSGroup will still call os.Chown(path, -1, -1) on every file. This is unnecessary syscall overhead.
maybe something like:
fp := FileProjection{
Data: payload.GetContents(),
Mode: payload.GetMode(),
}
if gid != constants.NoGID {
fp.FsGroup = &gid
}
files[payload.GetPath()] = fpThere was a problem hiding this comment.
Yes, now Updated the FSGroup population such that only valid GIDs get assinged
pkg/util/fileutil/filesystem.go
Outdated
| return constants.NoGID, nil | ||
| } | ||
| // Non-sentinel negative GID is invalid and thus we use ParseUint here. | ||
| gid, err := strconv.ParseUint(fsGroupStr, 10, 63) |
There was a problem hiding this comment.
strconv.ParseUint(fsGroupStr, 10, 63) accepts GIDs up to 2^63 - 1, but valid Linux GIDs max out at 2^32 - 1. Consider using bit size 32 for tighter validation or add a comment explaining why 63 was chosen.
There was a problem hiding this comment.
Changed the type for FSGroup/GID to be generic 'int' through out.
Didn't switch to 32 bit as the kubelet makes allowance for 64bit values: https://github.com/kubernetes/kubernetes/blob/b910026535af2d8a64d45efefeb8d9efb75a4817/pkg/volume/csi/csi_client.go#L64
This way, we are not assuming anything about the valid size as the chown API also considers a gid to be generic int type.
| } | ||
| customize(request) | ||
| return request | ||
| } |
| }, | ||
| }, | ||
| { | ||
| name: "volume mount with rotation but skipped", |
There was a problem hiding this comment.
Thanks for catching this. Guess i lost it during rebase.
Re-included the test.
test/bats/e2e-provider.bats
Outdated
| # 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 -n kube-system | grep '^E.*failed to clean and unmount target path.*$'" |
There was a problem hiding this comment.
| run bash -c "kubectl -n $NAMESPACE logs -l app=$POD_NAME --tail -1 -c secrets-store -n kube-system | grep '^E.*failed to clean and unmount target path.*$'" | |
| 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.*$'" |
the second -n wins, so -n $NAMESPACE would be dead code.
test/bats/e2e-provider.bats
Outdated
| # default key value returned by mock provider. | ||
| # base64 encoded content comparision is easier in case of very long multiline string. | ||
| export KEY_VALUE_CONTAINS=${KEY_VALUE:-"LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0KVGhpcyBpcyBtb2NrIGtleQotLS0tLUVORCBQVUJMSUMgS0VZLS0tLS0K"} | ||
| # defualt version value returned by mock provider |
There was a problem hiding this comment.
| # defualt version value returned by mock provider | |
| # default version value returned by mock provider |
There was a problem hiding this comment.
corrected all the three occurrences of this typo, thanks!
pkg/constants/constants.go
Outdated
There was a problem hiding this comment.
This package contains a single constant (NoGID) used only in the context of file operations. Consider placing it in pkg/util/fileutil instead to avoid a new package for one constant.
| assert_failure | ||
| } | ||
|
|
||
| function enable_secret_rotation() { |
There was a problem hiding this comment.
This function creates a local curl_pod_name but never echos it. Callers do curl_pod_name=$(enable_secret_rotation), which captures all stdout from kubectl run, kubectl wait, and curl -- not just the pod name. Then disable_secret_rotation $curl_pod_name receives garbage.
Add echo "$curl_pod_name" at the end of the function and suppress stdout on the intermediate commands.
There was a problem hiding this comment.
Thanks for catching this.
Redirected the stdout of other commands to /dev/null
echoed the curl_pod_name
(Verified with a local test that the return value is just the pod name. Didn't check it in though)
mytreya-rh
force-pushed
the
allow_file_ownership
branch
2 times, most recently
from
871e2b8 to
8618850
Compare
|
/retest looks like a transient error on the failed jobs:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
pkg/util/fileutil/atomic_writer.go
Outdated
| } | ||
| if err := os.Chown(fullPath, int(*fileProjection.FsUser), -1); err != nil { | ||
| klog.ErrorS(err, "unable to change file with owner", "logContext", w.logContext, "fullPath", fullPath, "owner", int(*fileProjection.FsUser)) | ||
| if err := os.Chown(fullPath, -1, int(*fileProjection.FsGroup)); err != nil { |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types
pkg/util/fileutil/atomic_writer.go
Outdated
| if err := os.Chown(fullPath, int(*fileProjection.FsUser), -1); err != nil { | ||
| klog.ErrorS(err, "unable to change file with owner", "logContext", w.logContext, "fullPath", fullPath, "owner", int(*fileProjection.FsUser)) | ||
| if err := os.Chown(fullPath, -1, int(*fileProjection.FsGroup)); err != nil { | ||
| klog.ErrorS(err, "unable to change file with owner", "logContext", w.logContext, "fullPath", fullPath, "owner", int(*fileProjection.FsGroup)) |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types
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:
|
mytreya-rh
force-pushed
the
allow_file_ownership
branch
from
8618850 to
40f47db
Compare
|
/retest
|
pkg/secrets-store/nodeserver.go
Outdated
| "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
pkg/util/fileutil/filesystem.go
Outdated
| 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.
test/bats/e2e-provider.bats
Outdated
| # 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{},
},
},
test/bats/e2e-provider.bats
Outdated
| # 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
$
test/bats/e2e-provider.bats
Outdated
| 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.
test/bats/e2e-provider.bats
Outdated
| # 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.
test/e2eprovider/server/server.go
Outdated
| 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
pkg/secrets-store/nodeserver.go
Outdated
| } | ||
|
|
||
| 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
mytreya-rh
force-pushed
the
allow_file_ownership
branch
2 times, most recently
from
43f6852 to
e08b33d
Compare
|
/unhold
|
k8s-ci-robot
removed
the
do-not-merge/hold
mytreya-rh
force-pushed
the
allow_file_ownership
branch
2 times, most recently
from
41dd89b to
740df77
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
mytreya-rh
force-pushed
the
allow_file_ownership
branch
from
740df77 to
b4862da
Compare
8 participants
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: