From 7b01f648a62a38815a1a6a02cfdcdfc1bc150d5b Mon Sep 17 00:00:00 2001 From: Connor Catlett Date: Thu, 21 Dec 2023 16:49:25 +0000 Subject: [PATCH] Use lsblk to safeguard against outdated symlinks Signed-off-by: Connor Catlett --- pkg/driver/node_linux.go | 42 ++++++++++++++++++++++- pkg/driver/node_linux_test.go | 64 +++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/pkg/driver/node_linux.go b/pkg/driver/node_linux.go index a61486b297..5d0981ee72 100644 --- a/pkg/driver/node_linux.go +++ b/pkg/driver/node_linux.go @@ -22,7 +22,9 @@ package driver import ( "fmt" "os" + "os/exec" "path/filepath" + "regexp" "strconv" "strings" @@ -47,6 +49,7 @@ func (d *nodeService) appendPartition(devicePath, partition string) string { // if the device is not nvme, return the path directly // if the device is nvme, finds and returns the nvme device path eg. /dev/nvme1n1 func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (string, error) { + strippedVolumeName := strings.Replace(volumeID, "-", "", -1) canonicalDevicePath := "" // If the given path exists, the device MAY be nvme. Further, it MAY be a @@ -76,6 +79,9 @@ func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (st } klog.V(5).InfoS("[Debug] The canonical device path was resolved", "devicePath", devicePath, "cacanonicalDevicePath", canonicalDevicePath) + if err = verifyVolumeSerialMatch(canonicalDevicePath, strippedVolumeName, execRunner); err != nil { + return "", err + } return d.appendPartition(canonicalDevicePath, partition), nil } @@ -87,13 +93,16 @@ func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (st // which AWS presents NVME devices under /dev/disk/by-id/. For example, // vol-0fab1d5e3f72a5e23 creates a symlink at // /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol0fab1d5e3f72a5e23 - nvmeName := "nvme-Amazon_Elastic_Block_Store_" + strings.Replace(volumeID, "-", "", -1) + nvmeName := "nvme-Amazon_Elastic_Block_Store_" + strippedVolumeName nvmeDevicePath, err := findNvmeVolume(d.deviceIdentifier, nvmeName) if err == nil { klog.V(5).InfoS("[Debug] successfully resolved", "nvmeName", nvmeName, "nvmeDevicePath", nvmeDevicePath) canonicalDevicePath = nvmeDevicePath + if err = verifyVolumeSerialMatch(canonicalDevicePath, strippedVolumeName, execRunner); err != nil { + return "", err + } return d.appendPartition(canonicalDevicePath, partition), nil } else { klog.V(5).InfoS("[Debug] error searching for nvme path", "nvmeName", nvmeName, "err", err) @@ -116,6 +125,37 @@ func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (st return canonicalDevicePath, nil } +// Helper to inject exec.Comamnd().CombinedOutput() for verifyVolumeSerialMatch +// Tests use a mocked version that does not actually execute any binaries +func execRunner(name string, arg ...string) ([]byte, error) { + return exec.Command(name, arg...).CombinedOutput() +} + +func verifyVolumeSerialMatch(canonicalDevicePath string, strippedVolumeName string, execRunner func(string, ...string) ([]byte, error)) error { + // In some rare cases, a race condition can lead to the /dev/disk/by-id/ symlink becoming out of date + // See https://github.com/kubernetes-sigs/aws-ebs-csi-driver/issues/1224 for more info + // Attempt to use lsblk to double check that the nvme device selected was the correct volume + output, err := execRunner("lsblk", "--noheadings", "--ascii", "--nodeps", "--output", "SERIAL", canonicalDevicePath) + + if err == nil { + // Look for an EBS volume ID in the output, compare all matches against what we expect + // (in some rare cases there may be multiple matches due to lsblk printing partitions) + // If no volume ID is in the output (non-Nitro instances, SBE devices, etc) silently proceed + volumeRegex := regexp.MustCompile(`vol[a-z0-9]+`) + for _, volume := range volumeRegex.FindAllString(string(output), -1) { + klog.V(6).InfoS("Comparing volume serial", "canonicalDevicePath", canonicalDevicePath, "expected", strippedVolumeName, "actual", volume) + if volume != strippedVolumeName { + return fmt.Errorf("Refusing to mount %s because it claims to be %s but should be %s", canonicalDevicePath, volume, strippedVolumeName) + } + } + } else { + // If the command fails (for example, because lsblk is not available), silently ignore the error and proceed + klog.V(5).ErrorS(err, "Ignoring lsblk failure", "canonicalDevicePath", canonicalDevicePath, "strippedVolumeName", strippedVolumeName) + } + + return nil +} + func errNoDevicePathFound(devicePath, volumeID string) error { return fmt.Errorf("no device path for device %q volume %q found", devicePath, volumeID) } diff --git a/pkg/driver/node_linux_test.go b/pkg/driver/node_linux_test.go index 8a7c780f8b..f9eec9efc9 100644 --- a/pkg/driver/node_linux_test.go +++ b/pkg/driver/node_linux_test.go @@ -20,6 +20,7 @@ limitations under the License. package driver import ( + "fmt" "io/fs" "os" "testing" @@ -190,6 +191,69 @@ func TestFindDevicePath(t *testing.T) { } } +const fakeVolumeName = "vol11111111111111111" +const fakeIncorrectVolumeName = "vol21111111111111111" + +func TestVerifyVolumeSerialMatch(t *testing.T) { + type testCase struct { + name string + execOutput string + execError error + expectError bool + } + testCases := []testCase{ + { + name: "success: empty", + execOutput: "", + }, + { + name: "success: single", + execOutput: fakeVolumeName, + }, + { + name: "success: multiple", + execOutput: fakeVolumeName + "\n" + fakeVolumeName + "\n" + fakeVolumeName, + }, + { + name: "success: whitespace", + execOutput: "\t " + fakeVolumeName + " \n \t ", + }, + { + name: "success: extra output", + execOutput: "extra output without name in it\n" + fakeVolumeName, + }, + { + name: "success: failed command", + execError: fmt.Errorf("Exec failed"), + }, + { + name: "failure: wrong volume", + execOutput: fakeIncorrectVolumeName, + expectError: true, + }, + { + name: "failure: mixed", + execOutput: fakeVolumeName + "\n" + fakeIncorrectVolumeName, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + mockExecRunner := func(_ string, _ ...string) ([]byte, error) { + return []byte(tc.execOutput), tc.execError + } + + result := verifyVolumeSerialMatch("path", fakeVolumeName, mockExecRunner) + if tc.expectError { + assert.Error(t, result) + } else { + assert.NoError(t, result) + } + }) + } +} + type fakeFileInfo struct { name string mode os.FileMode