CSI staging directories are named by a hash of the volume ID by ericcurtin · Pull Request #3180 · agent-substrate/substrate

Eric Curtin (ericcurtin) · 2026-10-23T20:40:53Z

A CSI volume ID with .. or a leading / made atelet create and remove directories outside the staging directory. Staging now uses <staging>/<driver>/<sha256(ID)>/mount, so any ID maps to one safe path.

Fixes #2935

AI use: AI-assisted (opencode). I reviewed and tested it.

Tests pass
Docs updated (none needed)

Davanum Srinivas (dims) · 2026-10-23T22:04:34Z

-	stagingPath := filepath.Join(p.stagingDirPrefix, volumeID)
+	stagingPath, err := p.stagingDir(volumeID)
+	if err != nil {
+		return err


Can you check this please?

unstageVolume now errors on a rejected ID, and atelet forwards that error, so Terminate fails for that actor (volumes.go:86, main.go:2018). Without a change, an actor whose template carries such an ID crashes at start and then can never be terminated: it stays in the cluster holding its slot, and every retry fails the same way.

Thank you, fixed in 19caa8a.

Done in 19caa8a, with an upgrade note in bbf82ef.

Eric Curtin (ericcurtin) · 2026-10-23T22:27:01Z

Davanum Srinivas (@dims) Tim Hockin (@thockin) PTAL when you get a chance, feedback addressed. Thank you!

Abhishek Sharma (abhisheksharma2411)

I looked at this one because #3024 is the same class of fix on the image-volume side.

I threw 28 adversarial IDs at stagingDir rather than reading it — "", ., .., ./, /, //, /etc/shadow, ../../etc/shadow, a/../../etc, a/.., ok/.., ....//....//etc, backslash and C: forms, a 50-deep path — and checked each result both for escaping the prefix and for resolving to the prefix itself. Nothing escapes, and nothing lands on the staging dir itself. Multi-element IDs like a/b/c are kept, which matches what the helper's comment says it wants.

"" is worth calling out as covered: filepath.IsLocal("") is already false, so the empty ID is rejected without needing its own clause.

./internal/volume/csi passes.

Two things, neither a blocker.

The unstageVolume skip assumes "stageVolume rejects this ID too, so nothing was staged". That holds for anything staged by this build, but a volume staged by an earlier build — where stageVolume did a bare filepath.Join with no validation — could be on disk with an ID this now rejects. After an upgrade that directory would never be unstaged, because the skip returns nil before NodeUnstageVolume is called. I think it is still the right call, since failing there strands the actor, and the leaked directory is outside the staging prefix anyway. But the comment reads as if the case cannot exist, and on an upgrade it can. Maybe worth a word so the next reader does not have to re-derive it.

Second, a curiosity rather than a problem: an ID containing a NUL byte is accepted by the check and then fails in os.MkdirAll with invalid argument, since Go rejects NUL in syscall paths. Safe, just an opaque error if a driver ever produces one.

LGTM.

Claude assisted with this review; the probe results are from runs on my own machine.

Eric Curtin (ericcurtin) · 2026-10-24T08:47:46Z

Added the upgrade note in bbf82ef. Leaving the NUL case, it fails safely.

Eric Curtin (ericcurtin) · 2026-10-30T15:08:51Z

Benjamin Elder (@BenTheElder) Dmitry Berkovich (@dberkov) It has one approval and feedback is addressed. PTAL when you get a chance. Thank you!

Michelle Au (msau42) · 2026-11-09T17:37:15Z

+// stagingDir returns the staging directory for volumeID. The ID is the
+// driver's own string and may contain slashes, but it must name a directory
+// below stagingDirPrefix.
+func (p *Plugin) stagingDir(volumeID string) (string, error) {
+	if !filepath.IsLocal(volumeID) || filepath.Clean(volumeID) == "." {
+		return "", fmt.Errorf("volume ID %q does not name a directory below the staging directory", volumeID)
+	}
+	return filepath.Join(p.stagingDirPrefix, volumeID), nil
+}


🤖 Since ExternalVolumes is still a preview gate, I'd suggest hashing the ID rather than validating it. Stage at <BasePath>/staging/<driver>/<sha256(volume ID)>/mount, like kubelet's .../kubernetes.io/csi/<driver>/<sha256(volume handle)>/globalmount. Every volume then gets one fixed-length directory, whatever its ID contains, so nothing needs rejecting. That fixes:

The leading-/ regression. filepath.IsLocal rejects every ID that starts with /, but those never escaped.

The unstage skip and its caveats go away, because every ID has a safe path.

Collisions and nesting. Today a/b, a//b, a/./b and x/../a/b share one directory, and a and a/b nest, so MkdirAll writes into another volume's mounted filesystem.

Long IDs and leftovers. A 256-byte ID with no slashes (the API allows 256) fails MkdirAll with ENAMETOOLONG. And removeStagingDir only removes the leaf, so multi-segment IDs like PD's leave empty parent directories behind.

Clashes between drivers. All drivers share <BasePath>/staging, but CSI only guarantees an ID is unique within one driver. The driver name is already validated as a DNS subdomain at atelet's request edge and checked against GetPluginInfo, so it's safe as a path element.

Thanks, done in the latest commit. The ID is hashed under a per-driver directory, and the reject and unstage skip are gone.

Michelle Au (msau42) · 2026-11-10T02:58:56Z

+			// One directory per driver and volume, below the prefix.
+			wantDepth := 3
+			rel, err := filepath.Rel(plugin.stagingDirPrefix, stagedAt)
+			if err != nil || !filepath.IsLocal(rel) || len(strings.Split(rel, string(filepath.Separator))) != wantDepth {
+				t.Errorf("staging path %q is not <prefix>/<driver>/<hash>/mount", stagedAt)
+			}


🤖 This layout is on-disk state shared across atelet builds, so could the test pin it exactly instead of checking the depth? For example:

p := &Plugin{stagingDirPrefix: "/staging", driverName: "d.example.com"} want := "/staging/d.example.com/d2e8363faaac7ae76def3b14091d8eb5755f6b92e9531627aeec833a8731cc49/mount" if got := p.stagingDir("vol-1"); got != want { t.Errorf("stagingDir(%q) = %q, want %q", "vol-1", got, want) }

The table could also record the paths sent to NodePublishVolume and NodeUnstageVolume and require them to equal the staged path. The previous revision checked that NodeUnstageVolume was called; this one doesn't. A case with a non-empty mount leaf would also show that it and its parent are kept. Today the other tests compute their expected path with stagingDir itself, so swapping the driver and hash, renaming the leaf, truncating the hash, skipping NodeUnstageVolume, or cleaning up with os.RemoveAll all still pass.

Smaller gaps nearby: nothing checks that the driver directory is kept after unmount (the comment on L470 says it is), the Unimplemented branches don't check that the per-volume directory is removed, and nothing checks that newCSIPlugin passes the driver name to NewPlugin; passing "" still passes every test.

Michelle Au (msau42) · 2026-11-10T02:58:56Z

+// stagingDir returns the staging directory for volumeID. The directory is named
+// by a hash of the ID, which is the driver's own string, and is scoped to the
+// driver because IDs are only unique within one.
+func (p *Plugin) stagingDir(volumeID string) string {
+	sum := sha256.Sum256([]byte(volumeID))
+	return filepath.Join(p.stagingDirPrefix, p.driverName, hex.EncodeToString(sum[:]), "mount")
+}
+
+// removeStagingDir removes an empty staging directory and its per-volume
+// parent, logging any failure.
 func removeStagingDir(ctx context.Context, stagingPath string) {
-	if err := os.Remove(stagingPath); err != nil && !os.IsNotExist(err) {
-		slog.WarnContext(ctx, "failed to remove staging directory", slog.String("path", stagingPath), slog.Any("error", err))
+	for _, dir := range []string{stagingPath, filepath.Dir(stagingPath)} {
+		if err := os.Remove(dir); err != nil && !os.IsNotExist(err) {
+			slog.WarnContext(ctx, "failed to remove staging directory", slog.String("path", dir), slog.Any("error", err))
+		}
 	}
 }


🤖 With hashed names, nothing records which volume a staging directory belongs to, so logging staging_path with volume_id when staging would help debug leftovers.

removeStagingDir could skip the parent when removing the leaf fails, to avoid a second ENOTEMPTY warning.

Michelle Au (msau42) · 2026-11-10T04:22:28Z

Also follow the instructions in the DCO check to fix the failing check

NodeStage and NodeUnstage joined the raw ID onto the staging prefix, so an ID like ../x made atelet create and remove directories outside it. Signed-off-by: Eric Curtin <eric.curtin@docker.com>

Signed-off-by: Eric Curtin <eric.curtin@docker.com>

Eric Curtin (ericcurtin) · 2026-11-10T09:27:37Z

Michelle Au (@msau42) The test now pins the exact layout and checks the stage, publish and unstage paths. Staging now logs volume_id with staging_path, and DCO is fixed.

Davanum Srinivas (dims) reviewed Oct 23, 2026

View reviewed changes

Abhishek Sharma (abhisheksharma2411) approved these changes Oct 23, 2026

View reviewed changes

Bowei Du (bowei) added area/node area/security Security related issue/pr area/storage kind/bug Something isn't working / bugfixes labels Oct 27, 2026

Benjamin Elder (BenTheElder) mentioned this pull request Nov 6, 2026

kubectl ate create actor-template creates every document in a manifest #2157

Open

2 tasks done

Michelle Au (msau42) reviewed Nov 9, 2026

View reviewed changes

Michelle Au (msau42) self-assigned this Nov 9, 2026

Eric Curtin (ericcurtin) changed the title ~~CSI volume IDs can no longer escape the staging directory~~ CSI staging directories are named by a hash of the volume ID Nov 9, 2026

Michelle Au (msau42) reviewed Nov 10, 2026

View reviewed changes

Michelle Au (msau42) mentioned this pull request Nov 10, 2026

[Epic] External Volume features and bug fixes for v1.1 #3757

Open

33 tasks

Eric Curtin (ericcurtin) added 5 commits November 10, 2026 09:27

csi: reject a volume ID that escapes the staging directory

c641ca6

NodeStage and NodeUnstage joined the raw ID onto the staging prefix, so an ID like ../x made atelet create and remove directories outside it. Signed-off-by: Eric Curtin <eric.curtin@docker.com>

csi: unstaging a rejected volume ID is a no-op, so Terminate still works

7322fad

Signed-off-by: Eric Curtin <eric.curtin@docker.com>

csi: note an older build may have staged a rejected ID

0c81411

Signed-off-by: Eric Curtin <eric.curtin@docker.com>

csi: stage at a per-driver hash of the volume ID

1c6737e

Signed-off-by: Eric Curtin <eric.curtin@docker.com>

csi: pin staging layout in tests, log staged path per volume

3eee9c4

Signed-off-by: Eric Curtin <eric.curtin@docker.com>

Eric Curtin (ericcurtin) force-pushed the csi-staging-id-local branch from 710c794 to 3eee9c4 Compare November 10, 2026 09:27

Conversation

Eric Curtin (ericcurtin) commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Davanum Srinivas (dims) Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

Eric Curtin (ericcurtin) Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

Eric Curtin (ericcurtin) Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

Eric Curtin (ericcurtin) commented Oct 23, 2026

Uh oh!

Abhishek Sharma (abhisheksharma2411) left a comment

Choose a reason for hiding this comment

Uh oh!

Eric Curtin (ericcurtin) commented Oct 24, 2026

Uh oh!

Eric Curtin (ericcurtin) commented Oct 30, 2026

Uh oh!

Michelle Au (msau42) Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eric Curtin (ericcurtin) Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

Michelle Au (msau42) Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

Michelle Au (msau42) Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

Michelle Au (msau42) commented Nov 10, 2026

Uh oh!

Eric Curtin (ericcurtin) commented Nov 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Eric Curtin (ericcurtin) commented Oct 23, 2026 •

edited

Loading

Michelle Au (msau42) Nov 9, 2026 •

edited

Loading