From 9f80de3772b41553770f1d0654d3e291a5bec50c Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Wed, 23 May 2018 10:15:12 +0200 Subject: [PATCH] Split NsEnterMounter and Mounter implementation of doBindSubpath nsenter implementation needs to mount different thing in the end and do different checks on the result. --- pkg/util/mount/BUILD | 1 + pkg/util/mount/mount_linux.go | 22 ++-- pkg/util/mount/mount_linux_test.go | 2 +- pkg/util/mount/nsenter_mount.go | 123 +++++++++++++++------ pkg/util/mount/nsenter_mount_test.go | 156 ++++++++++----------------- 5 files changed, 167 insertions(+), 137 deletions(-) diff --git a/pkg/util/mount/BUILD b/pkg/util/mount/BUILD index 6d2e65cbc96..a12f1397bf5 100644 --- a/pkg/util/mount/BUILD +++ b/pkg/util/mount/BUILD @@ -106,6 +106,7 @@ go_test( ] + select({ "@io_bazel_rules_go//go/platform:linux": [ "//vendor/github.com/golang/glog:go_default_library", + "//vendor/golang.org/x/sys/unix:go_default_library", "//vendor/k8s.io/utils/exec:go_default_library", ], "@io_bazel_rules_go//go/platform:windows": [ diff --git a/pkg/util/mount/mount_linux.go b/pkg/util/mount/mount_linux.go index 0671099b727..27bb91249ce 100644 --- a/pkg/util/mount/mount_linux.go +++ b/pkg/util/mount/mount_linux.go @@ -720,7 +720,7 @@ func getSELinuxSupport(path string, mountInfoFilename string) (bool, error) { } func (mounter *Mounter) PrepareSafeSubpath(subPath Subpath) (newHostPath string, cleanupAction func(), err error) { - newHostPath, err = doBindSubPath(mounter, subPath, os.Getpid()) + newHostPath, err = doBindSubPath(mounter, subPath) // There is no action when the container starts. Bind-mount will be cleaned // when container stops by CleanSubPaths. @@ -742,6 +742,11 @@ func safeOpenSubPath(mounter Interface, subpath Subpath) (int, error) { // prepareSubpathTarget creates target for bind-mount of subpath. It returns // "true" when the target already exists and something is mounted there. +// Given Subpath must have all paths with already resolved symlinks and with +// paths relevant to kubelet (when it runs in a container). +// This function is called also by NsEnterMounter. It works because +// /var/lib/kubelet is mounted from the host into the container with Kubelet as +// /var/lib/kubelet too. func prepareSubpathTarget(mounter Interface, subpath Subpath) (bool, string, error) { // Early check for already bind-mounted subpath. bindPathTarget := getSubpathBindTarget(subpath) @@ -793,10 +798,12 @@ func getSubpathBindTarget(subpath Subpath) string { return filepath.Join(subpath.PodDir, containerSubPathDirectoryName, subpath.VolumeName, subpath.ContainerName, strconv.Itoa(subpath.VolumeMountIndex)) } -// This implementation is shared between Linux and NsEnterMounter -// kubeletPid is PID of kubelet in the PID namespace where bind-mount is done, -// i.e. pid on the *host* if kubelet runs in a container. -func doBindSubPath(mounter Interface, subpath Subpath, kubeletPid int) (hostPath string, err error) { +func doBindSubPath(mounter Interface, subpath Subpath) (hostPath string, err error) { + // Linux, kubelet runs on the host: + // - safely open the subpath + // - bind-mount /proc//fd/ to subpath target + // User can't change /proc//fd/ to point to a bad place. + // Evaluate all symlinks here once for all subsequent functions. newVolumePath, err := filepath.EvalSymlinks(subpath.VolumePath) if err != nil { @@ -810,7 +817,6 @@ func doBindSubPath(mounter Interface, subpath Subpath, kubeletPid int) (hostPath subpath.VolumePath = newVolumePath subpath.Path = newPath - // Check the subpath is correct and open it fd, err := safeOpenSubPath(mounter, subpath) if err != nil { return "", err @@ -836,6 +842,7 @@ func doBindSubPath(mounter Interface, subpath Subpath, kubeletPid int) (hostPath } }() + kubeletPid := os.Getpid() mountSource := fmt.Sprintf("/proc/%d/fd/%v", kubeletPid, fd) // Do the bind mount @@ -1193,6 +1200,9 @@ func findExistingPrefix(base, pathname string) (string, []string, error) { // Symlinks are disallowed (pathname must already resolve symlinks), // and the path must be within the base directory. func doSafeOpen(pathname string, base string) (int, error) { + pathname = filepath.Clean(pathname) + base = filepath.Clean(base) + // Calculate segments to follow subpath, err := filepath.Rel(base, pathname) if err != nil { diff --git a/pkg/util/mount/mount_linux_test.go b/pkg/util/mount/mount_linux_test.go index 86fb25597c6..36972c67c29 100644 --- a/pkg/util/mount/mount_linux_test.go +++ b/pkg/util/mount/mount_linux_test.go @@ -1291,7 +1291,7 @@ func TestBindSubPath(t *testing.T) { } _, subpathMount := getTestPaths(base) - bindPathTarget, err := doBindSubPath(fm, subpath, 1) + bindPathTarget, err := doBindSubPath(fm, subpath) if test.expectError { if err == nil { t.Errorf("test %q failed: expected error, got success", test.name) diff --git a/pkg/util/mount/nsenter_mount.go b/pkg/util/mount/nsenter_mount.go index dfc690a64bc..e1e02b7d82b 100644 --- a/pkg/util/mount/nsenter_mount.go +++ b/pkg/util/mount/nsenter_mount.go @@ -22,13 +22,12 @@ import ( "fmt" "os" "path/filepath" - "regexp" - "strconv" "strings" + "syscall" "github.com/golang/glog" + "golang.org/x/sys/unix" utilfile "k8s.io/kubernetes/pkg/util/file" - utilio "k8s.io/kubernetes/pkg/util/io" "k8s.io/kubernetes/pkg/util/nsenter" ) @@ -37,13 +36,6 @@ const ( hostProcMountsPath = "/rootfs/proc/1/mounts" // hostProcMountinfoPath is the default mount info path for rootfs hostProcMountinfoPath = "/rootfs/proc/1/mountinfo" - // hostProcSelfStatusPath is the default path to /proc/self/status on the host - hostProcSelfStatusPath = "/rootfs/proc/self/status" -) - -var ( - // pidRegExp matches "Pid: " in /proc/self/status - pidRegExp = regexp.MustCompile(`\nPid:\t([0-9]*)\n`) ) // Currently, all docker containers receive their own mount namespaces. @@ -297,29 +289,9 @@ func (mounter *NsenterMounter) CleanSubPaths(podDir string, volumeName string) e return doCleanSubPaths(mounter, podDir, volumeName) } -// getPidOnHost returns kubelet's pid in the host pid namespace -func (mounter *NsenterMounter) getPidOnHost(procStatusPath string) (int, error) { - // Get the PID from /rootfs/proc/self/status - statusBytes, err := utilio.ConsistentRead(procStatusPath, maxListTries) - if err != nil { - return 0, fmt.Errorf("error reading %s: %s", procStatusPath, err) - } - matches := pidRegExp.FindSubmatch(statusBytes) - if len(matches) < 2 { - return 0, fmt.Errorf("cannot parse %s: no Pid:", procStatusPath) - } - return strconv.Atoi(string(matches[1])) -} - func (mounter *NsenterMounter) PrepareSafeSubpath(subPath Subpath) (newHostPath string, cleanupAction func(), err error) { - hostPid, err := mounter.getPidOnHost(hostProcSelfStatusPath) - if err != nil { - return "", nil, err - } - glog.V(4).Infof("Kubelet's PID on the host is %d", hostPid) - // Bind-mount the subpath to avoid using symlinks in subpaths. - newHostPath, err = doBindSubPath(mounter, subPath, hostPid) + newHostPath, err = doNsEnterBindSubPath(mounter, subPath) // There is no action when the container starts. Bind-mount will be cleaned // when container stops by CleanSubPaths. @@ -352,6 +324,95 @@ func (mounter *NsenterMounter) GetMountRefs(pathname string) ([]string, error) { return getMountRefsByDev(mounter, hostpath) } +func doNsEnterBindSubPath(mounter *NsenterMounter, subpath Subpath) (hostPath string, err error) { + // Linux, kubelet runs in a container: + // - safely open the subpath + // - bind-mount the subpath to target (this can be unsafe) + // - check that we mounted the right thing by comparing device ID and inode + // of the subpath (via safely opened fd) and the target (that's under our + // control) + + // Evaluate all symlinks here once for all subsequent functions. + evaluatedHostVolumePath, err := mounter.ne.EvalSymlinks(subpath.VolumePath, true /*mustExist*/) + if err != nil { + return "", fmt.Errorf("error resolving symlinks in %q: %v", subpath.VolumePath, err) + } + evaluatedHostSubpath, err := mounter.ne.EvalSymlinks(subpath.Path, true /*mustExist*/) + if err != nil { + return "", fmt.Errorf("error resolving symlinks in %q: %v", subpath.Path, err) + } + glog.V(5).Infof("doBindSubPath %q (%q) for volumepath %q", subpath.Path, evaluatedHostSubpath, subpath.VolumePath) + subpath.VolumePath = mounter.ne.KubeletPath(evaluatedHostVolumePath) + subpath.Path = mounter.ne.KubeletPath(evaluatedHostSubpath) + + // Check the subpath is correct and open it + fd, err := safeOpenSubPath(mounter, subpath) + if err != nil { + return "", err + } + defer syscall.Close(fd) + + alreadyMounted, bindPathTarget, err := prepareSubpathTarget(mounter, subpath) + if err != nil { + return "", err + } + if alreadyMounted { + return bindPathTarget, nil + } + + success := false + defer func() { + // Cleanup subpath on error + if !success { + glog.V(4).Infof("doNsEnterBindSubPath() failed for %q, cleaning up subpath", bindPathTarget) + if cleanErr := cleanSubPath(mounter, subpath); cleanErr != nil { + glog.Errorf("Failed to clean subpath %q: %v", bindPathTarget, cleanErr) + } + } + }() + + // Leap of faith: optimistically expect that nobody has modified previously + // expanded evalSubPath with evil symlinks and bind-mount it. + // Mount is done on the host! don't use kubelet path! + glog.V(5).Infof("bind mounting %q at %q", evaluatedHostSubpath, bindPathTarget) + if err = mounter.Mount(evaluatedHostSubpath, bindPathTarget, "" /*fstype*/, []string{"bind"}); err != nil { + return "", fmt.Errorf("error mounting %s: %s", evaluatedHostSubpath, err) + } + + // Check that the bind-mount target is the same inode and device as the + // source that we keept open, i.e. we mounted the right thing. + err = checkDeviceInode(fd, bindPathTarget) + if err != nil { + return "", fmt.Errorf("error checking bind mount for subpath %s: %s", subpath.VolumePath, err) + } + + success = true + glog.V(3).Infof("Bound SubPath %s into %s", subpath.Path, bindPathTarget) + return bindPathTarget, nil +} + +// checkDeviceInode checks that opened file and path represent the same file. +func checkDeviceInode(fd int, path string) error { + var srcStat, dstStat unix.Stat_t + err := unix.Fstat(fd, &srcStat) + if err != nil { + return fmt.Errorf("error running fstat on subpath FD: %v", err) + } + + err = unix.Stat(path, &dstStat) + if err != nil { + return fmt.Errorf("error running fstat on %s: %v", path, err) + } + + if srcStat.Dev != dstStat.Dev { + return fmt.Errorf("different device number") + } + if srcStat.Ino != dstStat.Ino { + return fmt.Errorf("different inode") + } + return nil +} + func (mounter *NsenterMounter) GetFSGroup(pathname string) (int64, error) { hostPath, err := mounter.ne.EvalSymlinks(pathname, true /* mustExist */) if err != nil { diff --git a/pkg/util/mount/nsenter_mount_test.go b/pkg/util/mount/nsenter_mount_test.go index 2aee4ad5f9d..2edb6febf97 100644 --- a/pkg/util/mount/nsenter_mount_test.go +++ b/pkg/util/mount/nsenter_mount_test.go @@ -21,9 +21,11 @@ package mount import ( "io/ioutil" "os" - "path" - "strconv" + "path/filepath" + "strings" "testing" + + "golang.org/x/sys/unix" ) func TestParseFindMnt(t *testing.T) { @@ -72,120 +74,76 @@ func TestParseFindMnt(t *testing.T) { } } -func TestGetPidOnHost(t *testing.T) { - tempDir, err := ioutil.TempDir("", "get_pid_on_host_tests") +func TestCheckDeviceInode(t *testing.T) { + testDir, err := ioutil.TempDir("", "nsenter-mounter-device-") if err != nil { - t.Fatalf(err.Error()) + t.Fatalf("Cannot create temporary directory: %s", err) } - defer os.RemoveAll(tempDir) + defer os.RemoveAll(testDir) tests := []struct { name string - procFile string - expectedPid int - expectError bool + srcPath string + dstPath string + expectError string }{ { - name: "valid status file", - procFile: `Name: cat -Umask: 0002 -State: R (running) -Tgid: 15041 -Ngid: 0 -Pid: 15041 -PPid: 22699 -TracerPid: 0 -Uid: 1000 1000 1000 1000 -Gid: 1000 1000 1000 1000 -FDSize: 256 -Groups: 10 135 156 157 158 973 984 1000 1001 -NStgid: 15041 -NSpid: 15041 -NSpgid: 15041 -NSsid: 22699 -VmPeak: 115016 kB -VmSize: 115016 kB -VmLck: 0 kB -VmPin: 0 kB -VmHWM: 816 kB -VmRSS: 816 kB -RssAnon: 64 kB -RssFile: 752 kB -RssShmem: 0 kB -VmData: 312 kB -VmStk: 136 kB -VmExe: 32 kB -VmLib: 2060 kB -VmPTE: 44 kB -VmPMD: 12 kB -VmSwap: 0 kB -HugetlbPages: 0 kB -Threads: 1 -SigQ: 2/60752 -SigPnd: 0000000000000000 -ShdPnd: 0000000000000000 -SigBlk: 0000000000000000 -SigIgn: 0000000000000000 -SigCgt: 0000000000000000 -CapInh: 0000000000000000 -CapPrm: 0000000000000000 -CapEff: 0000000000000000 -CapBnd: 0000003fffffffff -CapAmb: 0000000000000000 -NoNewPrivs: 0 -Seccomp: 0 -Cpus_allowed: ff -Cpus_allowed_list: 0-7 -Mems_allowed: 00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000001 -Mems_allowed_list: 0 -voluntary_ctxt_switches: 0 -nonvoluntary_ctxt_switches: 0 -`, - expectedPid: 15041, + name: "the same file", + srcPath: filepath.Join(testDir, "1"), + dstPath: filepath.Join(testDir, "1"), + expectError: "", }, { - name: "no Pid:", - procFile: `Name: cat -Umask: 0002 -State: R (running) -Tgid: 15041 -Ngid: 0 -PPid: 22699 -`, - expectedPid: 0, - expectError: true, + name: "different file on the same FS", + srcPath: filepath.Join(testDir, "2.1"), + dstPath: filepath.Join(testDir, "2.2"), + expectError: "different inode", }, { - name: "invalid Pid:", - procFile: `Name: cat -Umask: 0002 -State: R (running) -Tgid: 15041 -Ngid: 0 -Pid: invalid -PPid: 22699 -`, - expectedPid: 0, - expectError: true, + name: "different file on different device", + srcPath: filepath.Join(testDir, "3"), + // /proc is always on a different "device" than /tmp (or $TEMP) + dstPath: "/proc/self/status", + expectError: "different device", }, } - for i, test := range tests { - filename := path.Join(tempDir, strconv.Itoa(i)) - err := ioutil.WriteFile(filename, []byte(test.procFile), 0666) + for _, test := range tests { + if err := ioutil.WriteFile(test.srcPath, []byte{}, 0644); err != nil { + t.Errorf("Test %q: cannot create srcPath %s: %s", test.name, test.srcPath, err) + continue + } + + // Don't create dst if it exists + if _, err := os.Stat(test.dstPath); os.IsNotExist(err) { + if err := ioutil.WriteFile(test.dstPath, []byte{}, 0644); err != nil { + t.Errorf("Test %q: cannot create dstPath %s: %s", test.name, test.dstPath, err) + continue + } + } else if err != nil { + t.Errorf("Test %q: cannot check existence of dstPath %s: %s", test.name, test.dstPath, err) + continue + } + + fd, err := unix.Open(test.srcPath, unix.O_CREAT, 0644) if err != nil { - t.Fatalf(err.Error()) + t.Errorf("Test %q: cannot open srcPath %s: %s", test.name, test.srcPath, err) + continue } - mounter := NsenterMounter{} - pid, err := mounter.getPidOnHost(filename) - if err != nil && !test.expectError { - t.Errorf("Test %q: unexpected error: %s", test.name, err) + + err = checkDeviceInode(fd, test.dstPath) + + if test.expectError == "" && err != nil { + t.Errorf("Test %q: expected no error, got %s", test.name, err) } - if err == nil && test.expectError { - t.Errorf("Test %q: expected error, got none", test.name) - } - if pid != test.expectedPid { - t.Errorf("Test %q: expected pid %d, got %d", test.name, test.expectedPid, pid) + if test.expectError != "" { + if err == nil { + t.Errorf("Test %q: expected error, got none", test.name) + } else { + if !strings.Contains(err.Error(), test.expectError) { + t.Errorf("Test %q: expected error %q, got %q", test.name, test.expectError, err) + } + } } } }