Skip to content

Commit e28abcb

Browse files
committed
fix(provision): guard chown target against filesystem root in init-container
In a k8s init container only the workspace dir is mounted (subPath), so filepath.Dir(HostPath) resolves to "/" for HostPath=/workspace — the chown would recurse over the entire container root (chown -R 1000:1000 /). Today it's masked by dropped capabilities + a non-root security context (chown fails silently, provisioning still succeeds), but it's a correctness defect and a latent security hazard if that security context is ever relaxed. Extract the target computation into a testable chownTarget() helper that falls back to the workspace dir itself when the parent is "/" or ".", preserving broker-side behavior (chown the project root). Add unit coverage. Review finding from PR #172 (medium severity, required fix).
1 parent 6b7c6b3 commit e28abcb

2 files changed

Lines changed: 43 additions & 3 deletions

File tree

pkg/provision/provision.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -215,9 +215,8 @@ func ProvisionShared(in ProvisionInput) error {
215215

216216
// Chown to stable NFS UID/GID (design §9.1). This is a ONE-TIME operation
217217
// under the advisory lock — per-start chown is skipped for NFS (see N1-5).
218-
// Chown the workspace dir itself (sentinelDir may differ from project root
219-
// when running inside a k8s init container with a subPath mount).
220-
chownRoot := filepath.Dir(in.Resolved.HostPath)
218+
//
219+
chownRoot := chownTarget(in.Resolved.HostPath)
221220
uid, gid := resolveUID(in), resolveGID(in)
222221
if err := chownProjectTree(chownRoot, uid, gid); err != nil {
223222
slog.Warn("nfsBackend.Provision: chown failed (non-fatal, may lack privileges)",
@@ -405,6 +404,23 @@ func sanitizeBranchName(name string) string {
405404
return result
406405
}
407406

407+
// chownTarget returns the directory to recursively chown for a freshly
408+
// provisioned workspace.
409+
//
410+
// Broker-side, the project root is the parent of the workspace dir (it also
411+
// holds the shared-dirs siblings), so we chown the parent. But inside a k8s
412+
// init container only the workspace dir itself is mounted (subPath), so its
413+
// parent resolves to the filesystem root "/". Chowning "/" recursively is
414+
// wrong — and a latent security hazard if the pod's security context is ever
415+
// relaxed — so fall back to chowning the workspace dir itself in that case.
416+
func chownTarget(hostPath string) string {
417+
parent := filepath.Dir(hostPath)
418+
if parent == "/" || parent == "." {
419+
return hostPath
420+
}
421+
return parent
422+
}
423+
408424
// chownProjectTree sets ownership of the project root and its contents to the
409425
// given UID/GID. This is a ONE-TIME operation done under the advisory lock
410426
// during first provisioning (design §9.1). Per-start chown is NOT done for

pkg/provision/provision_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,30 @@ func TestSanitizeBranchName(t *testing.T) {
9898
}
9999
}
100100

101+
func TestChownTarget(t *testing.T) {
102+
tests := []struct {
103+
name string
104+
hostPath string
105+
want string
106+
}{
107+
// Broker-side: chown the project root (parent of the workspace dir).
108+
{"broker project root", "/srv/nfs/share1/proj-abc/workspace", "/srv/nfs/share1/proj-abc"},
109+
// k8s init container subPath mount: parent is "/", fall back to the
110+
// workspace dir itself rather than chown -R the whole container root.
111+
{"k8s workspace mount", "/workspace", "/workspace"},
112+
// Relative path has no real parent ("."); fall back to the path itself.
113+
{"relative path", "workspace", "workspace"},
114+
}
115+
116+
for _, tt := range tests {
117+
t.Run(tt.name, func(t *testing.T) {
118+
if got := chownTarget(tt.hostPath); got != tt.want {
119+
t.Errorf("chownTarget(%q) = %q, want %q", tt.hostPath, got, tt.want)
120+
}
121+
})
122+
}
123+
}
124+
101125
// --- writeSentinel ---
102126

103127
func TestWriteSentinel_Atomic(t *testing.T) {

0 commit comments

Comments
 (0)