From ca965e48d658f2642e736b75672d05d5a67e61bb Mon Sep 17 00:00:00 2001
From: Julian Sommer <115516918+juliansommer@users.noreply.github.com>
Date: Fri, 1 May 2026 11:15:58 +1000
Subject: [PATCH] fix(daemon): deny cross tenant attach on empty tenant id
 volumes

---
 spinifex/daemon/daemon_handlers_test.go   |  2 +
 spinifex/daemon/daemon_handlers_volume.go |  6 +-
 spinifex/daemon/ownership.go              | 12 ++++
 spinifex/daemon/ownership_test.go         | 68 +++++++++++++++++++++++
 4 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/spinifex/daemon/daemon_handlers_test.go b/spinifex/daemon/daemon_handlers_test.go
index 227f1511c..507d54b44 100644
--- a/spinifex/daemon/daemon_handlers_test.go
+++ b/spinifex/daemon/daemon_handlers_test.go
@@ -1608,6 +1608,7 @@ func TestAttachVolume_ZoneMismatch(t *testing.T) {
 				SizeGiB:          10,
 				State:            "available",
 				AvailabilityZone: "us-west-2a",
+				TenantID:         testAccountID,
 			},
 		},
 	}
@@ -3416,6 +3417,7 @@ func TestAttachVolume_VolumeInUse(t *testing.T) {
 				VolumeID: volumeID,
 				SizeGiB:  10,
 				State:    "in-use",
+				TenantID: testAccountID,
 			},
 		},
 	}
diff --git a/spinifex/daemon/daemon_handlers_volume.go b/spinifex/daemon/daemon_handlers_volume.go
index 371ed070c..6961d7d2b 100644
--- a/spinifex/daemon/daemon_handlers_volume.go
+++ b/spinifex/daemon/daemon_handlers_volume.go
@@ -54,9 +54,11 @@ func (d *Daemon) handleAttachVolume(msg *nats.Msg, command types.EC2InstanceComm
 		return
 	}
 
-	// Account scoping: verify the caller owns this volume
+	// Account scoping: verify the caller owns this volume.
+	// Pre-Phase4 volumes (empty TenantID) are root-only; otherwise the caller
+	// must match the recorded tenant exactly.
 	callerAccountID := utils.AccountIDFromMsg(msg)
-	if callerAccountID != "" && volCfg.VolumeMetadata.TenantID != "" && volCfg.VolumeMetadata.TenantID != callerAccountID {
+	if !volumeVisibleTo(volCfg.VolumeMetadata.TenantID, callerAccountID) {
 		slog.Warn("AttachVolume: account does not own volume", "volumeId", volumeID, "callerAccount", callerAccountID, "ownerAccount", volCfg.VolumeMetadata.TenantID)
 		respondWithError(msg, awserrors.ErrorInvalidVolumeNotFound)
 		return
diff --git a/spinifex/daemon/ownership.go b/spinifex/daemon/ownership.go
index 2c4a74772..e9d6ae645 100644
--- a/spinifex/daemon/ownership.go
+++ b/spinifex/daemon/ownership.go
@@ -43,3 +43,15 @@ func isInstanceVisible(callerAccountID, ownerAccountID string) bool {
 	}
 	return callerAccountID == ownerAccountID
 }
+
+// volumeVisibleTo reports whether callerAccountID may operate on a volume with
+// the given tenantID. Pre-Phase4 volumes (empty tenantID) are root-only —
+// without this, the legacy short-circuit `tenantID != "" && tenantID != caller`
+// matched every caller and let any tenant attach a legacy/migration volume by
+// ID alone.
+func volumeVisibleTo(tenantID, callerAccountID string) bool {
+	if tenantID == "" {
+		return callerAccountID == utils.GlobalAccountID
+	}
+	return callerAccountID == tenantID
+}
diff --git a/spinifex/daemon/ownership_test.go b/spinifex/daemon/ownership_test.go
index d8e29a41e..cdd4f80c8 100644
--- a/spinifex/daemon/ownership_test.go
+++ b/spinifex/daemon/ownership_test.go
@@ -61,3 +61,71 @@ func TestIsInstanceVisible(t *testing.T) {
 		})
 	}
 }
+
+func TestVolumeVisibleTo(t *testing.T) {
+	tests := []struct {
+		name            string
+		tenantID        string
+		callerAccountID string
+		expected        bool
+	}{
+		{
+			name:            "same tenant",
+			tenantID:        "123456789012",
+			callerAccountID: "123456789012",
+			expected:        true,
+		},
+		{
+			name:            "different tenant denied",
+			tenantID:        "123456789012",
+			callerAccountID: "999999999999",
+			expected:        false,
+		},
+		{
+			name:            "pre-Phase4 volume visible to root",
+			tenantID:        "",
+			callerAccountID: utils.GlobalAccountID,
+			expected:        true,
+		},
+		{
+			name:            "pre-Phase4 volume hidden from non-root tenant",
+			tenantID:        "",
+			callerAccountID: "123456789012",
+			expected:        false,
+		},
+		{
+			name:            "pre-Phase4 volume denied to empty caller",
+			tenantID:        "",
+			callerAccountID: "",
+			expected:        false,
+		},
+		{
+			name:            "tenant volume denied to empty caller",
+			tenantID:        "123456789012",
+			callerAccountID: "",
+			expected:        false,
+		},
+		{
+			name:            "root cannot impersonate tenant volume",
+			tenantID:        "123456789012",
+			callerAccountID: utils.GlobalAccountID,
+			expected:        false,
+		},
+		{
+			name:            "root accessing root-owned volume",
+			tenantID:        utils.GlobalAccountID,
+			callerAccountID: utils.GlobalAccountID,
+			expected:        true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := volumeVisibleTo(tt.tenantID, tt.callerAccountID)
+			if got != tt.expected {
+				t.Errorf("volumeVisibleTo(%q, %q) = %v, want %v",
+					tt.tenantID, tt.callerAccountID, got, tt.expected)
+			}
+		})
+	}
+}