From 09523340e93df083ad2a971c6cc6f5436b199d09 Mon Sep 17 00:00:00 2001 From: Michael Weibel Date: Wed, 27 May 2026 11:29:39 +0200 Subject: [PATCH 1/2] fix(luks): safely verify and reuse existing LUKS mappings NodeStageVolume on a host that still had a stale /dev/mapper/ from an earlier failed or racing stage would reuse the mapping without verifying which backing device it pointed at. Two staging paths could end up sharing the same device-mapper minor, after which GetDeviceMountRefs returned non-empty for every subsequent unstage. If the pod was assigned to another Node on next execution (CronJobs showed this problem), the logs reported "Multi-Attach error" until resolved manually. Verify the mapping before reuse: * cryptsetupStatus parses `cryptsetup status`. * validateExistingLuksMapping is extracted from luksOpen with an injectable statusFn so the match / mismatch / inactive / malformed / status-error paths are unit-tested without exec or a real device-mapper entry. * Both sides of the backing-device equality check go through EvalSymlinks to ensure we got the right device. * if luksClose calls return errors, return them instead of just logging them - this ensures they are actually surfaced --- driver/luks_util.go | 254 ++++++++++++++++++++++++++++++--------- driver/luks_util_test.go | 165 +++++++++++++++++++++++++ 2 files changed, 359 insertions(+), 60 deletions(-) create mode 100644 driver/luks_util_test.go diff --git a/driver/luks_util.go b/driver/luks_util.go index d0f2595d..e23d9e30 100644 --- a/driver/luks_util.go +++ b/driver/luks_util.go @@ -18,10 +18,13 @@ limitations under the License. package driver import ( + "bufio" + "bytes" "errors" "fmt" "os" "os/exec" + "path/filepath" "strings" "github.com/sirupsen/logrus" @@ -117,7 +120,7 @@ func getLuksContext(secrets map[string]string, context map[string]string, lifecy } } -func luksFormat(source string, mkfsCmd string, mkfsArgs []string, ctx LuksContext, log *logrus.Entry) error { +func luksFormat(source string, mkfsCmd string, mkfsArgs []string, ctx LuksContext, log *logrus.Entry) (err error) { cryptsetupCmd, err := getCryptsetupCmd() if err != nil { return err @@ -128,8 +131,7 @@ func luksFormat(source string, mkfsCmd string, mkfsArgs []string, ctx LuksContex } defer func() { - e := os.Remove(filename) - if e != nil { + if e := os.Remove(filename); e != nil { log.Errorf("cannot delete temporary file %s: %s", filename, e.Error()) } }() @@ -156,21 +158,22 @@ func luksFormat(source string, mkfsCmd string, mkfsArgs []string, ctx LuksContex err, cryptsetupCmd, strings.Join(cryptsetupArgs, " "), string(out)) } - // format the disk with the desired filesystem - // open the luks partition and set up a mapping - err = luksOpen(source, filename, ctx, log) + opened, err := luksOpen(source, filename, ctx, log) if err != nil { - return fmt.Errorf("cryptsetup luksOpen failed: %v cmd: '%s %s' output: %q", - err, cryptsetupCmd, strings.Join(cryptsetupArgs, " "), string(out)) + return fmt.Errorf("luksOpen during format failed: %w", err) } - defer func() { - e := luksClose(ctx.VolumeName, log) - if e != nil { - log.Errorf("cannot close luks device: %s", e.Error()) - } - }() + if opened { + defer func() { + if e := luksClose(ctx.VolumeName, log); e != nil { + log.Errorf("cannot close luks device: %s", e.Error()) + if err == nil { + err = fmt.Errorf("luksClose after format failed: %w", e) + } + } + }() + } // replace the source volume with the mapped one in the arguments to mkfs mkfsNewArgs := make([]string, len(mkfsArgs)) @@ -187,10 +190,10 @@ func luksFormat(source string, mkfsCmd string, mkfsArgs []string, ctx LuksContex "args": mkfsArgs, }).Info("executing format command") - out, err = exec.Command(mkfsCmd, mkfsArgs...).CombinedOutput() + mkfsOut, err := exec.Command(mkfsCmd, mkfsArgs...).CombinedOutput() if err != nil { return fmt.Errorf("formatting disk failed: %v cmd: '%s %s' output: %q", - err, mkfsCmd, strings.Join(mkfsArgs, " "), string(out)) + err, mkfsCmd, strings.Join(mkfsArgs, " "), string(mkfsOut)) } return nil @@ -203,14 +206,13 @@ func luksPrepareMount(source string, ctx LuksContext, log *logrus.Entry) (string return "", err } defer func() { - err := os.Remove(filename) - if err != nil { + if err := os.Remove(filename); err != nil { log.Errorf("cannot delete temporary file %s: %s", filename, err.Error()) } }() - err = luksOpen(source, filename, ctx, log) - if err != nil { + // The mapping is intentionally kept open until NodeUnstageVolume. + if _, err := luksOpen(source, filename, ctx, log); err != nil { return "", err } return "/dev/mapper/" + ctx.VolumeName, nil @@ -238,7 +240,7 @@ func luksClose(volume string, log *logrus.Entry) error { // checks if the given volume is formatted by checking if it is a luks volume and // if the luks volume, once opened, contains a filesystem -func isLuksVolumeFormatted(volume string, ctx LuksContext, log *logrus.Entry) (bool, error) { +func isLuksVolumeFormatted(volume string, ctx LuksContext, log *logrus.Entry) (formatted bool, err error) { isLuks, err := isLuks(volume) if err != nil { return false, err @@ -252,38 +254,67 @@ func isLuksVolumeFormatted(volume string, ctx LuksContext, log *logrus.Entry) (b return false, err } defer func() { - e := os.Remove(filename) - if e != nil { + if e := os.Remove(filename); e != nil { log.Errorf("cannot delete temporary file %s: %s", filename, e.Error()) } }() - err = luksOpen(volume, filename, ctx, log) + opened, err := luksOpen(volume, filename, ctx, log) if err != nil { return false, err } - defer func() { - e := luksClose(ctx.VolumeName, log) - if e != nil { - log.Errorf("cannot close luks device: %s", e.Error()) - } - }() + if opened { + defer func() { + if e := luksClose(ctx.VolumeName, log); e != nil { + log.Errorf("cannot close luks device: %s", e.Error()) + if err == nil { + err = fmt.Errorf("luksClose after format check failed: %w", e) + } + } + }() + } return isVolumeFormatted(volume, log) } -func luksOpen(volume string, keyFile string, ctx LuksContext, log *logrus.Entry) error { - // check if the luks volume is already open - if _, err := os.Stat("/dev/mapper/" + ctx.VolumeName); !os.IsNotExist(err) { - log.WithFields(logrus.Fields{ - "volume": volume, - }).Info("luks volume is already open") - return nil +// luksOpen ensures that /dev/mapper/ exists and is backed by +// the device referenced by `volume`. The boolean return value reports whether +// this call actually opened the mapping (true) or found an existing mapping +// it validated and reused (false). Callers that registered a deferred +// luksClose should gate it on this flag so they do not close a mapping they +// did not open. +func luksOpen(volume string, keyFile string, ctx LuksContext, log *logrus.Entry) (bool, error) { + mapperPath := "/dev/mapper/" + ctx.VolumeName + if _, statErr := os.Stat(mapperPath); statErr == nil { + // A mapping with this name already exists. Confirm that it is + // backed by the device we just resolved before reusing it. + // Without this check, a stale mapping left over from a + // previously-attached cloudscale volume can end up mounted over a + // freshly attached volume. Once two staging paths share one + // device-mapper minor, GetDeviceMountRefs refuses every subsequent + // unstage and the node accumulates unrecoverable state. + inactive, backing, err := validateExistingLuksMapping(ctx.VolumeName, volume, cryptsetupStatus) + if err != nil { + return false, err + } + if inactive { + // The mapping was closed between Stat and status (another + // goroutine raced us). Fall through to open it fresh. + log.WithField("volume", volume).Info("luks mapping vanished between stat and status, opening fresh") + } else { + log.WithFields(logrus.Fields{ + "volume": volume, + "backing": backing, + }).Info("luks volume is already open and backing device matches") + return false, nil + } + } else if !os.IsNotExist(statErr) { + return false, fmt.Errorf("stat %s: %w", mapperPath, statErr) } cryptsetupCmd, err := getCryptsetupCmd() if err != nil { - return err + return false, err } cryptsetupArgs := []string{ "--batch-mode", @@ -297,10 +328,10 @@ func luksOpen(volume string, keyFile string, ctx LuksContext, log *logrus.Entry) }).Info("executing cryptsetup luksOpen command") out, err := exec.Command(cryptsetupCmd, cryptsetupArgs...).CombinedOutput() if err != nil { - return fmt.Errorf("cryptsetup luksOpen failed: %v cmd: '%s %s' output: %q", + return false, fmt.Errorf("cryptsetup luksOpen failed: %v cmd: '%s %s' output: %q", err, cryptsetupCmd, strings.Join(cryptsetupArgs, " "), string(out)) } - return nil + return true, nil } // runs cryptsetup resize for a given volume (/dev/mapper/pvc-xyz) @@ -332,38 +363,141 @@ func isLuks(volume string) (bool, error) { return true, nil } -// check is a given mapping under /dev/mapper is a luks volume -func isLuksMapping(volume string) (bool, string, error) { - if strings.HasPrefix(volume, "/dev/mapper/") { - mappingName := volume[len("/dev/mapper/"):] - cryptsetupCmd, err := getCryptsetupCmd() - if err != nil { - return false, mappingName, err - } - cryptsetupArgs := []string{"status", mappingName} +// cryptsetupStatusInfo holds the fields we parse from `cryptsetup status `. +type cryptsetupStatusInfo struct { + backing string + isLuks bool + isInactive bool +} - out, err := exec.Command(cryptsetupCmd, cryptsetupArgs...).CombinedOutput() - if err != nil { - return false, mappingName, nil +// parseCryptsetupStatus extracts the LUKS-ness, backing device, and +// active/inactive state from the output of `cryptsetup status `. +// The first line is the state header, always one of: +// +// "/dev/mapper/ is inactive." +// "/dev/mapper/ is active and is in use." +// "/dev/mapper/ is active." +// +// Subsequent lines (active case only) are " key: value", parsed in the +// loop below. +func parseCryptsetupStatus(out []byte) cryptsetupStatusInfo { + var info cryptsetupStatusInfo + scanner := bufio.NewScanner(bytes.NewReader(out)) + if !scanner.Scan() { + return info + } + if strings.Contains(scanner.Text(), " is inactive") { + info.isInactive = true + return info + } + for scanner.Scan() { + key, value, ok := strings.Cut(scanner.Text(), ":") + if !ok { + continue } - for _, statusLine := range strings.Split(string(out), "\n") { - if strings.Contains(statusLine, "type:") { - if strings.Contains(strings.ToLower(statusLine), "luks") { - return true, mappingName, nil - } - return false, mappingName, nil + switch strings.TrimSpace(key) { + case "type": + if strings.Contains(strings.ToLower(value), "luks") { + info.isLuks = true } + case "device": + info.backing = strings.TrimSpace(value) } + } + return info +} + +// validateExistingLuksMapping confirms that the existing /dev/mapper/ +// is backed by the same block device as `volume`. It returns: +// - (true, "", nil) when the mapping is no longer active (a concurrent +// close raced the stat); the caller should re-open. +// - (false, backing, nil) when the mapping is safe to reuse; `backing` is +// the device path as reported by cryptsetup (useful for logging). +// - (false, "", err) when cryptsetup status fails, the mapping is +// active but reports no backing device, or the backing device does not +// match the resolved `volume`. +// +// The statusFn parameter exists so tests can substitute a fake without +// actually shelling out to cryptsetup. +func validateExistingLuksMapping( + mapperName, volume string, + statusFn func(string) (cryptsetupStatusInfo, error), +) (isInactive bool, backing string, err error) { + info, err := statusFn(mapperName) + if err != nil { + return false, "", fmt.Errorf("luks mapping %s exists but cryptsetup status failed: %w", + mapperName, err) + } + if info.isInactive { + return true, "", nil + } + if info.backing == "" { + return false, "", fmt.Errorf("luks mapping %s is active but cryptsetup status reported no backing device", + mapperName) + } + expected, err := filepath.EvalSymlinks(volume) + if err != nil { + return false, "", fmt.Errorf("cannot resolve volume %s for luks mapping validation: %w", + volume, err) + } + // Resolve both sides through EvalSymlinks: the cryptsetup we ship today + // canonicalises the device path via realpath() at open time, but a future + // cryptsetup could preserve the caller-supplied symlink. Comparing + // resolved-against-resolved keeps the check correct either way. + backingResolved, evalErr := filepath.EvalSymlinks(info.backing) + if evalErr != nil { + backingResolved = info.backing + } + if backingResolved != expected { + return false, "", fmt.Errorf("luks mapping %s is backed by %s (resolved: %s), expected %s, refusing to reuse stale mapping", + mapperName, info.backing, backingResolved, expected) + } + return false, info.backing, nil +} +// cryptsetupStatus runs `cryptsetup status ` and returns the parsed +// info. A mapping that is reported as inactive (the normal "no such mapping" +// case) is returned with info.isInactive == true and a nil error so callers +// can distinguish it from real failures — mirroring the sentinel pattern in +// ceph-csi's DeviceEncryptionStatus. +func cryptsetupStatus(name string) (cryptsetupStatusInfo, error) { + cryptsetupCmd, err := getCryptsetupCmd() + if err != nil { + return cryptsetupStatusInfo{}, err + } + out, err := exec.Command(cryptsetupCmd, "status", name).CombinedOutput() + info := parseCryptsetupStatus(out) + if err != nil { + if info.isInactive { + return info, nil + } + return info, fmt.Errorf("cryptsetup status %s failed: %v output: %q", + name, err, string(out)) + } + return info, nil +} + +// check is a given mapping under /dev/mapper is a luks volume +func isLuksMapping(volume string) (bool, string, error) { + if !strings.HasPrefix(volume, "/dev/mapper/") { + return false, "", nil + } + mappingName := volume[len("/dev/mapper/"):] + info, err := cryptsetupStatus(mappingName) + if err != nil { + return false, mappingName, err + } + if info.isInactive { + return false, "", nil } - return false, "", nil + return info.isLuks, mappingName, nil } func getCryptsetupCmd() (string, error) { cryptsetupCmd := "cryptsetup" _, err := exec.LookPath(cryptsetupCmd) if err != nil { - if err == exec.ErrNotFound { + if errors.Is(err, exec.ErrNotFound) { return "", fmt.Errorf("%q executable not found in $PATH", cryptsetupCmd) } return "", err diff --git a/driver/luks_util_test.go b/driver/luks_util_test.go new file mode 100644 index 00000000..b316bd0f --- /dev/null +++ b/driver/luks_util_test.go @@ -0,0 +1,165 @@ +/* +Copyright cloudscale.ch + +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 +*/ + +package driver + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestParseCryptsetupStatus(t *testing.T) { + tests := []struct { + name string + out string + wantBacking string + wantIsLuks bool + wantIsInactive bool + }{ + { + name: "active LUKS1 mapping", + out: `/dev/mapper/pvc-foo is active. + type: LUKS1 + cipher: aes-xts-plain64 + keysize: 512 bits + device: /dev/sdb + offset: 4096 sectors + size: 1048576 sectors + mode: read/write`, + wantBacking: "/dev/sdb", + wantIsLuks: true, + }, + { + name: "active non-LUKS mapping", + out: `/dev/mapper/foo is active. + type: PLAIN + cipher: aes-cbc-essiv:sha256 + device: /dev/sdc`, + wantBacking: "/dev/sdc", + wantIsLuks: false, + }, + { + name: "inactive mapping", + out: `/dev/mapper/pvc-foo is inactive.`, + wantIsInactive: true, + }, + { + name: "malformed (no type or device line)", + out: `/dev/mapper/pvc-foo is active.`, + }, + { + name: "empty output", + out: ``, + wantIsInactive: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := parseCryptsetupStatus([]byte(tt.out)) + if got.backing != tt.wantBacking { + t.Errorf("backing = %q, want %q", got.backing, tt.wantBacking) + } + if got.isLuks != tt.wantIsLuks { + t.Errorf("isLuks = %v, want %v", got.isLuks, tt.wantIsLuks) + } + if got.isInactive != tt.wantIsInactive { + t.Errorf("isInactive = %v, want %v", got.isInactive, tt.wantIsInactive) + } + }) + } +} + +func TestValidateExistingLuksMapping(t *testing.T) { + // EvalSymlinks on a regular file returns the file's absolute path. That is + // enough to exercise the match / mismatch branches without needing a real + // block device or /dev/mapper entry. + tmp, err := os.CreateTemp(t.TempDir(), "fake-backing-*") + if err != nil { + t.Fatalf("CreateTemp: %v", err) + } + _ = tmp.Close() + resolved, err := filepath.EvalSymlinks(tmp.Name()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + + tests := []struct { + name string + status cryptsetupStatusInfo + statusErr error + volume string + wantInactive bool + wantBacking string + wantErrSub string + }{ + { + name: "backing matches", + status: cryptsetupStatusInfo{backing: resolved}, + volume: tmp.Name(), + wantBacking: resolved, + }, + { + name: "backing mismatches", + status: cryptsetupStatusInfo{backing: "/dev/nonexistent-other"}, + volume: tmp.Name(), + wantErrSub: "refusing to reuse stale mapping", + }, + { + name: "mapping inactive", + status: cryptsetupStatusInfo{isInactive: true}, + volume: tmp.Name(), + wantInactive: true, + }, + { + name: "no backing device reported", + status: cryptsetupStatusInfo{}, + volume: tmp.Name(), + wantErrSub: "reported no backing device", + }, + { + name: "status call errors", + statusErr: errors.New("boom"), + volume: tmp.Name(), + wantErrSub: "cryptsetup status failed", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fake := func(string) (cryptsetupStatusInfo, error) { + return tt.status, tt.statusErr + } + inactive, backing, err := validateExistingLuksMapping("mapper", tt.volume, fake) + + if tt.wantErrSub != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.wantErrSub) + } + if !strings.Contains(err.Error(), tt.wantErrSub) { + t.Fatalf("expected error containing %q, got %q", tt.wantErrSub, err.Error()) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if inactive != tt.wantInactive { + t.Errorf("inactive = %v, want %v", inactive, tt.wantInactive) + } + if backing != tt.wantBacking { + t.Errorf("backing = %q, want %q", backing, tt.wantBacking) + } + }) + } +} From 2d6d189612e5f56d03fcfdcf6a9a1305e960d27e Mon Sep 17 00:00:00 2001 From: Michael Weibel Date: Wed, 27 May 2026 16:03:53 +0200 Subject: [PATCH 2/2] fix(luks): log cryptsetup resize command luksResize previously discarded both the command invocation and the combined output, so a failing resize bubbled up as a bare exit-status error with no way to tell what cryptsetup actually said. --- driver/luks_util.go | 15 ++++++++++++--- driver/node.go | 4 ++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/driver/luks_util.go b/driver/luks_util.go index e23d9e30..0e18e0e5 100644 --- a/driver/luks_util.go +++ b/driver/luks_util.go @@ -335,15 +335,24 @@ func luksOpen(volume string, keyFile string, ctx LuksContext, log *logrus.Entry) } // runs cryptsetup resize for a given volume (/dev/mapper/pvc-xyz) -func luksResize(volume string) error { +func luksResize(volume string, log *logrus.Entry) error { cryptsetupCmd, err := getCryptsetupCmd() if err != nil { return err } cryptsetupArgs := []string{"--batch-mode", "resize", volume} - _, err = exec.Command(cryptsetupCmd, cryptsetupArgs...).CombinedOutput() - return err + log.WithFields(logrus.Fields{ + "cmd": cryptsetupCmd, + "args": cryptsetupArgs, + }).Info("executing cryptsetup resize command") + + out, err := exec.Command(cryptsetupCmd, cryptsetupArgs...).CombinedOutput() + if err != nil { + return fmt.Errorf("cryptsetup resize failed: %v cmd: '%s %s' output: %q", + err, cryptsetupCmd, strings.Join(cryptsetupArgs, " "), string(out)) + } + return nil } // runs cryptsetup isLuks for a given volume diff --git a/driver/node.go b/driver/node.go index ec3e0a10..3205268c 100644 --- a/driver/node.go +++ b/driver/node.go @@ -207,7 +207,7 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe } if isLuks { ll.Info("resizing LUKS container before filesystem resize") - if err := luksResize(devicePath); err != nil { + if err := luksResize(devicePath, ll); err != nil { return nil, status.Errorf(codes.Internal, "failed to resize LUKS container on %s: %v", devicePath, err) } } @@ -595,7 +595,7 @@ func (d *Driver) NodeExpandVolume(ctx context.Context, req *csi.NodeExpandVolume // the luks container must be resized if the volume was resized while the disk was mounted if isLuks { ll.Info("resizing luks container") - err := luksResize(devicePath) + err := luksResize(devicePath, ll) if err != nil { return nil, status.Errorf(codes.Internal, "NodeExpandVolume unable resize luks container for volume %q at %q: %v", volumePath, devicePath, err) }