Skip to content

Commit f9ce228

Browse files
authored
Fix secrets redaction for CSI volumes to avoid state corruption (#27176)
In #12583 we changed the serialization code for CSI volumes so that we were using the extension method we use for topology and nodes. This reduces an enormous amount of boilerplate code, but we introduced a state store corruption bug in the process. The extension method sanitizes the volume without copying it, so a read of the volume (such as getting an event from the event stream) can cause the volume's secrets to be redacted in subsequent requests to publish or mount the volume. Move the sanitization code into a testable method on the volume, and add a copy to the method. Ref: #12583 Fixes: #26766
1 parent e17d688 commit f9ce228

File tree

4 files changed

+49
-11
lines changed

4 files changed

+49
-11
lines changed

.changelog/27176.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
```release-note:bug
2+
csi: Fixed a bug where reading a volume from the API or event stream could erase its secrets
3+
```

nomad/structs/csi.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,6 +578,27 @@ func (v *CSIVolume) Copy() *CSIVolume {
578578
return out
579579
}
580580

581+
// Sanitize returns a deep copy of the volume, with sensitive fields redacted
582+
func (v *CSIVolume) Sanitize() *CSIVolume {
583+
if v == nil {
584+
return nil
585+
}
586+
587+
clean := v.Copy()
588+
589+
// would be better not to have at all but left in and redacted for backwards
590+
// compatibility with the existing API
591+
clean.Secrets = nil
592+
593+
// MountFlags can contain secrets, so we always redact it but want to show
594+
// the user that we have the value
595+
if v.MountOptions != nil {
596+
clean.MountOptions = clean.MountOptions.Sanitize()
597+
}
598+
599+
return clean
600+
}
601+
581602
// Claim updates the allocations and changes the volume state
582603
func (v *CSIVolume) Claim(claim *CSIVolumeClaim, alloc *Allocation) error {
583604
// COMPAT: volumes registered prior to 1.1.0 will be missing caps for the

nomad/structs/csi_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1093,6 +1093,30 @@ func TestTaskCSIPluginConfig_Equal(t *testing.T) {
10931093
}})
10941094
}
10951095

1096+
func TestCSIVolumeSanitize(t *testing.T) {
1097+
ci.Parallel(t)
1098+
1099+
orig := &CSIVolume{
1100+
ID: "foo",
1101+
MountOptions: &CSIMountOptions{
1102+
FSType: "ext4",
1103+
MountFlags: []string{"ro", "noatime"},
1104+
},
1105+
Secrets: CSISecrets{
1106+
"foo": "bar",
1107+
"baz": "qux",
1108+
},
1109+
Parameters: map[string]string{"example": "unchanged"},
1110+
}
1111+
1112+
sanitized := orig.Sanitize()
1113+
must.Eq(t, []string{"[REDACTED]"}, sanitized.MountOptions.MountFlags)
1114+
must.Nil(t, sanitized.Secrets)
1115+
1116+
orig.Parameters["example"] = "different"
1117+
must.Eq(t, "unchanged", sanitized.Parameters["example"])
1118+
}
1119+
10961120
func TestCSISecretsSanitize(t *testing.T) {
10971121
ci.Parallel(t)
10981122

nomad/structs/extensions.go

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ func nodeExt(v interface{}) interface{} {
6969
}
7070

7171
func csiVolumeExt(v interface{}) interface{} {
72-
vol := v.(*CSIVolume)
72+
vol := v.(*CSIVolume).Sanitize()
7373
type EmbeddedCSIVolume CSIVolume
7474

7575
allocCount := len(vol.ReadAllocs) + len(vol.WriteAllocs)
@@ -100,15 +100,5 @@ func csiVolumeExt(v interface{}) interface{} {
100100
}
101101
}
102102

103-
// MountFlags can contain secrets, so we always redact it but want
104-
// to show the user that we have the value
105-
if vol.MountOptions != nil && len(vol.MountOptions.MountFlags) > 0 {
106-
apiVol.MountOptions.MountFlags = []string{"[REDACTED]"}
107-
}
108-
109-
// would be better not to have at all but left in and redacted for
110-
// backwards compatibility with the existing API
111-
apiVol.Secrets = nil
112-
113103
return apiVol
114104
}

0 commit comments

Comments
 (0)