File 0005-Improve-non-root-path-handling of Package kubevirt.31631
From 249ca794b302ad3cc5dba104d634d496171d0557 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:00:03 +0200
Subject: [PATCH 01/20] Add a safepath package to encapsule path operations
Privileged virt-handler operations inside non-root virt-launcher pods
need extra precautions to ensure that symbolic links do not lead to
pointing to unintended locations.
The safepath package introduces path operations based on `*At` unit file
syscall (OpenAt, MkDirAt, ...) which have built-in support for not
following symlink and therefore allow atomic check- and modify
operations.
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit ba30ed6a883d6801c3c79544d6282e5fd3ab0434)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/safepath/BUILD.bazel | 30 ++
pkg/safepath/safepath.go | 433 ++++++++++++++++++++++++++++
pkg/safepath/safepath_linux.go | 103 +++++++
pkg/safepath/safepath_suite_test.go | 30 ++
pkg/safepath/safepath_test.go | 300 +++++++++++++++++++
5 files changed, 896 insertions(+)
create mode 100644 pkg/safepath/BUILD.bazel
create mode 100644 pkg/safepath/safepath.go
create mode 100644 pkg/safepath/safepath_linux.go
create mode 100644 pkg/safepath/safepath_suite_test.go
create mode 100644 pkg/safepath/safepath_test.go
diff --git a/pkg/safepath/BUILD.bazel b/pkg/safepath/BUILD.bazel
new file mode 100644
index 000000000..c521acc9b
--- /dev/null
+++ b/pkg/safepath/BUILD.bazel
@@ -0,0 +1,30 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+ name = "go_default_library",
+ srcs = [
+ "safepath.go",
+ "safepath_linux.go",
+ ],
+ importpath = "kubevirt.io/kubevirt/pkg/safepath",
+ visibility = ["//visibility:public"],
+ deps = [
+ "//pkg/unsafepath:go_default_library",
+ "//vendor/golang.org/x/sys/unix:go_default_library",
+ ],
+)
+
+go_test(
+ name = "go_default_test",
+ srcs = [
+ "safepath_suite_test.go",
+ "safepath_test.go",
+ ],
+ embed = [":go_default_library"],
+ deps = [
+ "//pkg/unsafepath:go_default_library",
+ "//staging/src/kubevirt.io/client-go/testutils:go_default_library",
+ "//vendor/github.com/onsi/ginkgo/v2:go_default_library",
+ "//vendor/github.com/onsi/gomega:go_default_library",
+ ],
+)
diff --git a/pkg/safepath/safepath.go b/pkg/safepath/safepath.go
new file mode 100644
index 000000000..99822067b
--- /dev/null
+++ b/pkg/safepath/safepath.go
@@ -0,0 +1,433 @@
+package safepath
+
+import (
+ "container/list"
+ "fmt"
+ "net"
+ "os"
+ "path/filepath"
+ "strings"
+ "syscall"
+
+ "kubevirt.io/kubevirt/pkg/unsafepath"
+
+ "golang.org/x/sys/unix"
+)
+
+// JoinAndResolveWithRelativeRoot joins an absolute relativeRoot base path with
+// additional elements which have to be kept below the relativeRoot base.
+// Relative and absolute links will be resolved relative to the provided rootBase
+// and can not escape it.
+func JoinAndResolveWithRelativeRoot(rootBase string, elems ...string) (*Path, error) {
+ // ensure that rootBase is absolute
+ if !filepath.IsAbs(rootBase) {
+ return nil, fmt.Errorf("basepath is not absolute: %q", rootBase)
+ }
+
+ path := pathRoot
+ fifo := newLimitedFifo(256)
+ for i := len(elems) - 1; i >= 0; i-- {
+ if err := fifo.push(strings.Split(filepath.Clean(elems[i]), pathSeparator)); err != nil {
+ return nil, err
+ }
+ }
+
+ for !fifo.empty() {
+ child := fifo.pop()
+ var link string
+ var err error
+
+ path, link, err = advance(rootBase, path, child)
+ if err != nil {
+ return nil, err
+ }
+ if link != "" {
+ if err := fifo.push(strings.Split(link, pathSeparator)); err != nil {
+ return nil, err
+ }
+ }
+ }
+
+ // Assert that the result is indeed a clean path in the expected format
+ // at this point in time.
+ finalPath := newPath(rootBase, path)
+ fd, err := OpenAtNoFollow(finalPath)
+ if err != nil {
+ return nil, err
+ }
+ _ = fd.Close()
+
+ return finalPath, nil
+}
+
+type fifo struct {
+ ops uint
+ store *list.List
+ maxOps uint
+}
+
+func (f *fifo) push(pathElements []string) error {
+ for i := len(pathElements) - 1; i >= 0; i-- {
+ if f.ops > f.maxOps {
+ return fmt.Errorf("more than %v path elements evaluated", f.maxOps)
+ }
+ if pathElements[i] == "" {
+ continue
+ }
+ f.ops++
+ f.store.PushFront(pathElements[i])
+ }
+ return nil
+}
+
+func (f *fifo) pop() string {
+ if val := f.store.Front(); val != nil {
+ f.store.Remove(val)
+ return val.Value.(string)
+ }
+ return ""
+}
+
+func (f *fifo) empty() bool {
+ return f.store.Len() == 0
+}
+
+// newLimitedFifo creates a fifo with a maximum enqueue limit to
+// avoid abuse on filepath operations.
+func newLimitedFifo(maxOps uint) *fifo {
+ return &fifo{
+ store: list.New(),
+ maxOps: maxOps,
+ }
+}
+
+// OpenAtNoFollow safely opens a filedescriptor to a path relative to
+// rootBase. Any symlink encountered will be treated as invalid and the operation will be aborted.
+// This works best together with a path first resolved with JoinAndResolveWithRelativeRoot
+// which can resolve relative paths and symlinks.
+func OpenAtNoFollow(path *Path) (file *File, err error) {
+ fd, err := open(path.rootBase)
+ if err != nil {
+ return nil, err
+ }
+ for _, child := range strings.Split(filepath.Clean(path.relativePath), pathSeparator) {
+ if child == "" {
+ continue
+ }
+ newfd, err := openat(fd, child)
+ _ = syscall.Close(fd) // always close the parent after the lookup
+ if err != nil {
+ return nil, err
+ }
+ fd = newfd
+ }
+ return &File{fd: fd, path: path}, nil
+}
+
+func ChmodAtNoFollow(path *Path, mode os.FileMode) error {
+ f, err := OpenAtNoFollow(path)
+ if err != nil {
+ return err
+ }
+ defer f.Close()
+ return os.Chmod(f.SafePath(), mode)
+}
+
+func ChownAtNoFollow(path *Path, uid, gid int) error {
+ f, err := OpenAtNoFollow(path)
+ if err != nil {
+ return err
+ }
+ defer f.Close()
+ return os.Chown(f.SafePath(), uid, gid)
+}
+
+func ChpermAtNoFollow(path *Path, uid, gid int, mode os.FileMode) error {
+ // first set the ownership, to avoid that someone may change back the file mode
+ // after we set it. This is necessary if the file got somehow created without
+ // the right owners, maybe with malicious intent.
+ if err := ChownAtNoFollow(path, uid, gid); err != nil {
+ return err
+ }
+ if err := ChmodAtNoFollow(path, mode); err != nil {
+ return err
+ }
+ return nil
+}
+
+func MkdirAtNoFollow(path *Path, dirName string, mode os.FileMode) error {
+ if err := isSingleElement(dirName); err != nil {
+ return err
+ }
+ f, err := OpenAtNoFollow(path)
+ if err != nil {
+ return err
+ }
+ defer f.Close()
+ if err := unix.Mkdirat(f.fd, dirName, uint32(mode)); err != nil {
+ return err
+ }
+ return nil
+}
+
+// TouchAtNoFollow safely touches a file relative to
+// rootBase. The additional elements form the relative path. Any symlink
+// encountered will be treated as invalid and the operation will be aborted.
+// This works best together with a path first resolved with JoinAndResolveWithRelativeRoot
+// which can resolve relative paths to their real path without symlinks.
+// If the target file exists already, the function will fail.
+func TouchAtNoFollow(path *Path, fileName string, mode os.FileMode) (err error) {
+ if err := isSingleElement(fileName); err != nil {
+ return err
+ }
+ parent, err := OpenAtNoFollow(path)
+ if err != nil {
+ return err
+ }
+ defer parent.Close()
+ fd, err := touchat(parent.fd, fileName, uint32(mode))
+ if err != nil {
+ return err
+ }
+ _ = syscall.Close(fd)
+ return nil
+}
+
+func MknodAtNoFollow(path *Path, fileName string, mode os.FileMode, dev uint64) (err error) {
+ if err := isSingleElement(fileName); err != nil {
+ return err
+ }
+ parent, err := OpenAtNoFollow(path)
+ if err != nil {
+ return err
+ }
+ defer parent.Close()
+ return mknodat(parent.fd, fileName, uint32(mode), dev)
+}
+
+func StatAtNoFollow(path *Path) (os.FileInfo, error) {
+ pathFd, err := OpenAtNoFollow(path)
+ if err != nil {
+ return nil, err
+ }
+ defer pathFd.Close()
+ return os.Stat(pathFd.SafePath())
+}
+
+type File struct {
+ fd int
+ path *Path
+}
+
+func (f *File) Close() error {
+ return syscall.Close(f.fd)
+}
+
+func (f *File) String() string {
+ return f.Path().String()
+}
+
+// SafePath returns a path pointing to the associated file descriptor.
+// It is safe to reuse this path without additional checks. The kernel
+// will ensure that this path always points to the resolved file.
+// To operate on the file just use os.Open and related calls.
+func (f *File) SafePath() string {
+ return path(f.fd)
+}
+
+func (f *File) Path() *Path {
+ return f.path
+}
+
+// Path is a path which was at the time of creation a real path
+// re
+type Path struct {
+ rootBase string
+ relativePath string
+}
+
+// Raw returns an "unsafe" path. It's properties are not safe to use without certain precautions.
+// It exposes no access functions. All access happens via functions in the "unsafepath" package.
+func (p *Path) Raw() *unsafepath.Path {
+ return unsafepath.New(p.rootBase, p.relativePath)
+}
+
+func (p *Path) IsRoot() bool {
+ return unsafepath.UnsafeAbsolute(p.Raw()) == pathRoot
+}
+
+// AppendAndResolveWithRelativeRoot returns a new path with the passed elements resolve relative
+// to the current absolute path.
+func (p *Path) AppendAndResolveWithRelativeRoot(relativeRootElems ...string) (*Path, error) {
+ tmpPath, err := JoinAndResolveWithRelativeRoot(unsafepath.UnsafeAbsolute(p.Raw()), relativeRootElems...)
+ if err != nil {
+ return nil, err
+ }
+
+ newPath := newPath(p.rootBase, filepath.Join(p.relativePath, tmpPath.relativePath))
+ fd, err := OpenAtNoFollow(newPath)
+ if err != nil {
+ return nil, err
+ }
+ _ = fd.Close()
+
+ return newPath, err
+}
+
+func (p *Path) String() string {
+ return fmt.Sprintf("root: %v, relative: %v", p.rootBase, p.relativePath)
+}
+
+// ExecuteNoFollow opens the file in question and provides the file descriptor path as safePath string.
+// This safePath string can be (re)opened with normal os.* operations. The file descriptor path is
+// managed by the kernel and there is no way to inject malicious symlinks.
+func (p *Path) ExecuteNoFollow(callback func(safePath string) error) error {
+ f, err := OpenAtNoFollow(p)
+ if err != nil {
+ return err
+ }
+ defer f.Close()
+ return callback(f.SafePath())
+}
+
+// DirNoFollow returns the parent directory of the safepath.Path as safepath.Path.
+func (p *Path) DirNoFollow() (*Path, error) {
+ if len(p.relativePath) == 0 {
+ return nil, fmt.Errorf("already at relative root, can't get parent")
+ }
+ newPath := newPath(p.rootBase, filepath.Dir(p.relativePath))
+ return newPath, nil
+}
+
+// Base returns the basename of the relative untrusted part of the safepath.
+func (p *Path) Base() (string, error) {
+ if len(p.relativePath) == 0 {
+ return "", fmt.Errorf("already at relative root, can't get parent")
+ }
+ return filepath.Base(p.relativePath), nil
+}
+
+func newPath(rootBase, relativePath string) *Path {
+ return &Path{
+ rootBase: rootBase,
+ relativePath: filepath.Join("/", relativePath),
+ }
+}
+
+// NewFileNoFollow assumes that a real path to a file is given. It will validate that
+// the file is indeed absolute by doing the following checks:
+// * ensure that the path is absolute
+// * ensure that the path does not container relative path elements
+// * ensure that no symlinks are provided
+// It will return the opened file which contains a link to a safe-to-use path
+// to the file, which can't be tampered with. To operate on the file just use os.Open and related calls.
+func NewFileNoFollow(path string) (*File, error) {
+ if filepath.Clean(path) != path || !filepath.IsAbs(path) {
+ return nil, fmt.Errorf("path %q must be absolute and must not contain relative elements", path)
+ }
+ p := newPath("/", path)
+ return OpenAtNoFollow(p)
+}
+
+// NewPathNoFollow is a convenience method to get out of a supposedly link-free path a safepath.Path.
+// If there is a symlink included the command will fail.
+func NewPathNoFollow(path string) (*Path, error) {
+ fd, err := NewFileNoFollow(path)
+ if err != nil {
+ return nil, err
+ }
+ defer fd.Close()
+ return fd.Path(), nil
+}
+
+// JoinNoFollow joins the root path with the given additional path.
+// If the additional path element is not a real path (like containing symlinks), it fails.
+func JoinNoFollow(rootPath *Path, path string) (*Path, error) {
+ if filepath.Clean(path) != path || path == "" {
+ return nil, fmt.Errorf("path %q must not contain relative elements and must not be empty", path)
+ }
+ p := newPath(unsafepath.UnsafeAbsolute(rootPath.Raw()), path)
+ f, err := OpenAtNoFollow(p)
+ if err != nil {
+ return nil, err
+ }
+ return f.Path(), f.Close()
+}
+
+func isSingleElement(path string) error {
+ cleanedPath := filepath.Clean(path)
+ if cleanedPath != path || strings.ContainsAny(path, pathSeparator) {
+ return fmt.Errorf("path %q must be a single non-relative path segment", path)
+ }
+ switch path {
+ case "", "..", ".":
+ return fmt.Errorf("path %q must be a single non-relative path segment", path)
+ default:
+ return nil
+ }
+}
+
+// UnlinkAtNoFollow allows deleting the specified file or directory (directory must be empty to succeed).
+func UnlinkAtNoFollow(path *Path) error {
+ parent, err := path.DirNoFollow()
+ if err != nil {
+ return err
+ }
+ basename, err := path.Base()
+ if err != nil {
+ return nil
+ }
+ info, err := StatAtNoFollow(path)
+ if err != nil {
+ return err
+ }
+ fd, err := OpenAtNoFollow(parent)
+ if err != nil {
+ return err
+ }
+ defer fd.Close()
+ if info.IsDir() {
+ // if dir is empty we can delete it with AT_REMOVEDIR
+ return unlinkat(fd.fd, basename, unix.AT_REMOVEDIR)
+ } else {
+ return unlinkat(fd.fd, basename, 0)
+ }
+}
+
+// ListenUnixNoFollow safely creates a socket in user-owned path
+// Since there exists no socketat on unix, first a safe delete is performed,
+// then the socket is created.
+func ListenUnixNoFollow(socketDir *Path, socketName string) (net.Listener, error) {
+ if err := isSingleElement(socketName); err != nil {
+ return nil, err
+ }
+
+ addr, err := net.ResolveUnixAddr("unix", filepath.Join(unsafepath.UnsafeAbsolute(socketDir.Raw()), socketName))
+ if err != nil {
+ return nil, err
+ }
+
+ socketPath, err := JoinNoFollow(socketDir, socketName)
+ if err == nil {
+ // This ensures that we don't allow unlinking arbitrary files
+ if err := UnlinkAtNoFollow(socketPath); err != nil {
+ return nil, err
+ }
+ } else if !os.IsNotExist(err) {
+ return nil, err
+ }
+
+ listener, err := net.ListenUnix("unix", addr)
+ if err != nil {
+ return nil, err
+ }
+
+ // Ensure that the socket path is a real path
+ // this does not 100% remove the chance of
+ // having a socket created at the wrong place, but it makes it unlikely
+ _, err = JoinNoFollow(socketDir, socketName)
+ if err != nil {
+ return nil, err
+ }
+ return listener, nil
+}
diff --git a/pkg/safepath/safepath_linux.go b/pkg/safepath/safepath_linux.go
new file mode 100644
index 000000000..96bb3378f
--- /dev/null
+++ b/pkg/safepath/safepath_linux.go
@@ -0,0 +1,103 @@
+//go:build linux
+
+package safepath
+
+import (
+ "fmt"
+ "io/fs"
+ "os"
+ "path/filepath"
+ "strings"
+ "syscall"
+
+ "golang.org/x/sys/unix"
+)
+
+const pathSeparator = string(os.PathSeparator)
+const pathRoot = string(os.PathSeparator)
+
+// advance will try to add the child to the parent. If it is a relative symlink it will resolve it
+// and return the parent with the new symlink. If it is an absolute symlink, parent will be reset to '/'
+// and returned together with the absolute symlink. If the joined result is no symlink, the joined result will
+// be returned as the new parent.
+func advance(rootBase string, parent string, child string) (string, string, error) {
+ // Ensure parent is absolute and never empty
+ parent = filepath.Clean(parent)
+ if !filepath.IsAbs(parent) {
+ return "", "", fmt.Errorf("parent path %v must be absolute", parent)
+ }
+
+ if strings.Contains(child, pathSeparator) {
+ return "", "", fmt.Errorf("child %q must not contain a path separator", child)
+ }
+
+ // Deal with relative path elements like '.', '//' and '..'
+ // Since parent is absolute, worst case we get '/' as result
+ path := filepath.Join(parent, child)
+
+ if path == rootBase {
+ // don't evaluate the root itself, since rootBase is allowed to be a symlink
+ return path, "", nil
+ }
+
+ fi, err := os.Lstat(filepath.Join(rootBase, path))
+ if err != nil {
+ return "", "", err
+ }
+
+ if fi.Mode()&fs.ModeSymlink == 0 {
+ // no symlink, we are done, return the joined result of parent and child
+ return filepath.Clean(path), "", nil
+ }
+
+ link, err := os.Readlink(filepath.Join(rootBase, path))
+ if err != nil {
+ return "", "", err
+ }
+
+ if filepath.IsAbs(link) {
+ // the link is absolute, let's reset the parent and the discovered link path
+ return pathRoot, filepath.Clean(link), nil
+ } else {
+ // on relative links, don't advance parent and return the link
+ return parent, filepath.Clean(link), nil
+ }
+}
+
+// openat helps traversing a path without following symlinks
+// to ensure safe path references on user-owned paths by privileged processes
+func openat(dirfd int, path string) (fd int, err error) {
+ if err := isSingleElement(path); err != nil {
+ return -1, err
+ }
+ return unix.Openat(dirfd, path, unix.O_NOFOLLOW|unix.O_PATH, 0)
+}
+
+func unlinkat(dirfd int, path string, flags int) error {
+ if err := isSingleElement(path); err != nil {
+ return err
+ }
+ return unix.Unlinkat(dirfd, path, flags)
+}
+
+func touchat(dirfd int, path string, mode uint32) (fd int, err error) {
+ if err := isSingleElement(path); err != nil {
+ return -1, err
+ }
+ return unix.Openat(dirfd, path, unix.O_NOFOLLOW|syscall.O_CREAT|syscall.O_EXCL, mode)
+}
+
+func mknodat(dirfd int, path string, mode uint32, dev uint64) (err error) {
+ if err := isSingleElement(path); err != nil {
+ return err
+ }
+ return unix.Mknodat(dirfd, path, mode, int(dev))
+}
+
+func open(path string) (fd int, err error) {
+ return syscall.Open(path, unix.O_PATH, 0)
+}
+
+func path(fd int) string {
+ return fmt.Sprintf("/proc/self/fd/%d", fd)
+}
diff --git a/pkg/safepath/safepath_suite_test.go b/pkg/safepath/safepath_suite_test.go
new file mode 100644
index 000000000..92f1da11f
--- /dev/null
+++ b/pkg/safepath/safepath_suite_test.go
@@ -0,0 +1,30 @@
+/*
+ * This file is part of the KubeVirt project
+ *
+ * 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
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Copyright 2018 Red Hat, Inc.
+ *
+ */
+
+package safepath
+
+import (
+ "testing"
+
+ "kubevirt.io/client-go/testutils"
+)
+
+func TestSafePath(t *testing.T) {
+ testutils.KubeVirtTestSuiteSetup(t)
+}
diff --git a/pkg/safepath/safepath_test.go b/pkg/safepath/safepath_test.go
new file mode 100644
index 000000000..bfaae7ea6
--- /dev/null
+++ b/pkg/safepath/safepath_test.go
@@ -0,0 +1,300 @@
+package safepath
+
+import (
+ "fmt"
+ "io/fs"
+ "os"
+ "os/user"
+ "path/filepath"
+ "strconv"
+ "syscall"
+
+ "kubevirt.io/kubevirt/pkg/unsafepath"
+
+ . "github.com/onsi/ginkgo/v2"
+ . "github.com/onsi/gomega"
+)
+
+type pathBuilder struct {
+ segments [][]string
+ relativeRoot string
+ systemRoot string
+}
+
+// Path adds a new path segment to the final path to construct
+func (p *pathBuilder) Path(path string) *pathBuilder {
+ p.segments = append(p.segments, []string{path})
+ return p
+}
+
+// Link adds a path segemtn and a link location to the final path to construct.
+// The link target can be an absolute or a relative path.
+func (p *pathBuilder) Link(path string, target string) *pathBuilder {
+ p.segments = append(p.segments, []string{path, target})
+ return p
+}
+
+// new returns a new path builder with the given relative root prefix.
+func new(root string) *pathBuilder {
+ return &pathBuilder{segments: [][]string{}, relativeRoot: root}
+}
+
+// RelativeRoot returns the final full relative root path.
+// Must be called after Builder() to be valid.
+func (p *pathBuilder) RelativeRoot() string {
+ return filepath.Join(p.systemRoot, p.relativeRoot)
+}
+
+// SystemRoot returns the emulated system root path, where the
+// RelativeRoot path is a child of.
+// Must be called after Builder() to be valid.
+func (p *pathBuilder) SystemRoot() string {
+ return p.systemRoot
+}
+
+// Build the defined path. Absolute links are prefixed wit the SystemRoot which
+// will be the base of a ginkgo managed tmp directory.
+func (p *pathBuilder) Build() (string, error) {
+ p.systemRoot = GinkgoT().TempDir()
+ relativeRoot := filepath.Join(p.systemRoot, p.relativeRoot)
+ parent := relativeRoot
+ if err := os.MkdirAll(parent, os.ModePerm); err != nil {
+ return "", err
+ }
+ for _, elem := range p.segments {
+ parent = filepath.Join(parent, elem[0])
+ if len(elem) == 2 {
+ link := elem[1]
+ if err := os.Symlink(link, parent); err != nil {
+ return "", err
+ }
+ } else {
+ if err := os.MkdirAll(parent, os.ModePerm); err != nil {
+ return "", err
+ }
+ }
+ }
+
+ relativePath := ""
+ for _, elem := range p.segments {
+ relativePath = filepath.Join(relativePath, elem[0])
+ }
+
+ return relativePath, nil
+}
+
+var _ = Describe("safepath", func() {
+
+ DescribeTable("should prevent an escape via", func(builder *pathBuilder, expectedPath string) {
+ path, err := builder.Build()
+ Expect(err).ToNot(HaveOccurred())
+ constructedPath, err := JoinAndResolveWithRelativeRoot(builder.RelativeRoot(), path)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(unsafepath.UnsafeAbsolute(constructedPath.Raw())).To(Equal(filepath.Join(builder.RelativeRoot(), expectedPath)))
+ },
+ Entry("an absolute link to root subdirectory", new("/var/lib/rel/root").Path("link/back/to").Link("link", "/link"),
+ "/link",
+ ),
+ Entry("an absolute link to root", new("/var/lib/rel/root").Path("link/back/to").Link("link", "/"),
+ "/",
+ ),
+ Entry("a relative link", new("/var/lib/rel/root").Path("link/back/to").Link("var", "../../../../../"),
+ "/",
+ ),
+ )
+
+ DescribeTable("should be able to", func(builder *pathBuilder, expectedPath string) {
+ path, err := builder.Build()
+ Expect(err).ToNot(HaveOccurred())
+ constructedPath, err := JoinAndResolveWithRelativeRoot(builder.RelativeRoot(), path)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(unsafepath.UnsafeAbsolute(constructedPath.Raw())).To(Equal(filepath.Join(builder.RelativeRoot(), expectedPath)))
+ },
+ Entry("handle relative paths by cutting off at the relative root", new("/var/lib/rel/root").Path("link/back/to").Path("../../../../../"),
+ `/`,
+ ),
+ Entry("handle relative legitimate paths", new("/var/lib/rel/root").Path("link/back/to").Path("../../"),
+ `/link`,
+ ),
+ Entry("handle legitimate paths with relative symlinks", new("/var/lib/rel/root").Path("link/back/to").Link("test", "../../"),
+ `/link`,
+ ),
+ Entry("handle multiple legitimate symlink redirects", new("/var/lib/rel/root").Path("link/back/to").Link("test", "../../").Path("b/c").Link("yeah", "../"),
+ `/link/b`,
+ ),
+ )
+
+ It("should detect self-referencing links", func() {
+ builder := new("/var/lib/rel/root").Path("link/back/to").Link("test", "../test")
+ path, err := builder.Build()
+ Expect(err).ToNot(HaveOccurred())
+ _, err = JoinAndResolveWithRelativeRoot(builder.RelativeRoot(), path)
+ Expect(err).To(HaveOccurred())
+ })
+
+ It("should follow a sequence of linked links", func() {
+ root := GinkgoT().TempDir()
+ relativeRoot := filepath.Join(root, "testroot")
+ path := "some/path/to/follow"
+ Expect(os.MkdirAll(filepath.Join(relativeRoot, path, "test3", "test4"), os.ModePerm))
+ Expect(os.Symlink("test3", filepath.Join(relativeRoot, path, "test2"))).To(Succeed())
+ Expect(os.Symlink("test2", filepath.Join(relativeRoot, path, "test1"))).To(Succeed())
+ // try to reach the test4 directory over the test1 link
+ pp, err := JoinAndResolveWithRelativeRoot(relativeRoot, path, "/test1/test4")
+ Expect(err).ToNot(HaveOccurred())
+ // don't use join to avoid any clean operations
+ Expect(unsafepath.UnsafeAbsolute(pp.Raw())).To(Equal(relativeRoot + "/some/path/to/follow/test3/test4"))
+ })
+
+ It("should detect too many redirects", func() {
+ root := GinkgoT().TempDir()
+ relativeRoot := filepath.Join(root, "testroot")
+ path := "some/path/to/follow"
+ Expect(os.MkdirAll(filepath.Join(relativeRoot, path, "test3", "test4"), os.ModePerm))
+ Expect(os.Symlink("test3", filepath.Join(relativeRoot, path, "test100"))).To(Succeed())
+ for i := 101; i < 401+50; i++ {
+ Expect(os.Symlink(fmt.Sprintf("test%d", i-1), filepath.Join(relativeRoot, path, fmt.Sprintf("test%d", i)))).To(Succeed())
+ }
+
+ // try to reach the test4 directory over the test1 link
+ _, err := JoinAndResolveWithRelativeRoot(relativeRoot, path, "/test435/test4")
+ Expect(err).To(HaveOccurred())
+ Expect(err.Error()).To(Equal("more than 256 path elements evaluated"))
+ })
+
+ It("should not resolve symlinks in the root path", func() {
+ root := GinkgoT().TempDir()
+ relativeRoot := filepath.Join(root, "testroot")
+ path := "some/path/to/follow"
+ Expect(os.MkdirAll(filepath.Join(relativeRoot, path, "test3", "test4"), os.ModePerm))
+ Expect(os.Symlink("test3", filepath.Join(relativeRoot, path, "test2"))).To(Succeed())
+ Expect(os.Symlink("test2", filepath.Join(relativeRoot, path, "test1"))).To(Succeed())
+ // include the symlink in the root path
+ pp, err := JoinAndResolveWithRelativeRoot(filepath.Join(relativeRoot, path, "test1"), "test4")
+ Expect(err).ToNot(HaveOccurred())
+ // don't use join to avoid any clean operations
+ Expect(unsafepath.UnsafeAbsolute(pp.Raw())).To(Equal(relativeRoot + "/some/path/to/follow/test1/test4"))
+ })
+
+ It("should create a socket repeatedly the safe way", func() {
+ root, err := JoinAndResolveWithRelativeRoot("/", GinkgoT().TempDir())
+ Expect(err).ToNot(HaveOccurred())
+ l, err := ListenUnixNoFollow(root, "my.sock")
+ Expect(err).ToNot(HaveOccurred())
+ l.Close()
+ l, err = ListenUnixNoFollow(root, "my.sock")
+ Expect(err).ToNot(HaveOccurred())
+ l.Close()
+ })
+
+ It("should open a safepath and provide its filedescriptor path with execute", func() {
+ root, err := JoinAndResolveWithRelativeRoot("/", GinkgoT().TempDir())
+ Expect(err).ToNot(HaveOccurred())
+ Expect(os.MkdirAll(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "test"), os.ModePerm)).To(Succeed())
+
+ Expect(root.ExecuteNoFollow(func(safePath string) error {
+ Expect(safePath).To(ContainSubstring("/proc/self/fd/"))
+ _, err := os.Stat(filepath.Join(safePath, "test"))
+ Expect(err).ToNot(HaveOccurred())
+ return nil
+ })).To(Succeed())
+ })
+
+ It("should create a child directory", func() {
+ root, err := JoinAndResolveWithRelativeRoot("/", GinkgoT().TempDir())
+ Expect(err).ToNot(HaveOccurred())
+ Expect(MkdirAtNoFollow(root, "test", os.ModePerm)).To(Succeed())
+ _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "test"))
+ Expect(err).ToNot(HaveOccurred())
+ })
+
+ It("should set owner and file permissions", func() {
+ root, err := JoinAndResolveWithRelativeRoot("/", GinkgoT().TempDir())
+ Expect(err).ToNot(HaveOccurred())
+ u, err := user.Current()
+ Expect(err).ToNot(HaveOccurred())
+ uid, err := strconv.Atoi(u.Uid)
+ Expect(err).ToNot(HaveOccurred())
+ gid, err := strconv.Atoi(u.Gid)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(ChpermAtNoFollow(root, uid, gid, 0777)).To(Succeed())
+ stat, err := StatAtNoFollow(root)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(stat.Sys().(*syscall.Stat_t).Gid).To(Equal(uint32(gid)))
+ Expect(stat.Sys().(*syscall.Stat_t).Uid).To(Equal(uint32(uid)))
+ Expect(stat.Mode() & 0777).To(Equal(fs.FileMode(0777)))
+ Expect(ChpermAtNoFollow(root, uid, gid, 0770)).To(Succeed())
+ stat, err = StatAtNoFollow(root)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(stat.Mode() & 0777).To(Equal(fs.FileMode(0770)))
+ Expect(stat.Sys().(*syscall.Stat_t).Gid).To(Equal(uint32(gid)))
+ Expect(stat.Sys().(*syscall.Stat_t).Uid).To(Equal(uint32(uid)))
+ })
+
+ It("should unlink files and directories", func() {
+ root, err := JoinAndResolveWithRelativeRoot("/", GinkgoT().TempDir())
+ Expect(err).ToNot(HaveOccurred())
+ Expect(TouchAtNoFollow(root, "test", os.ModePerm)).To(Succeed())
+ Expect(MkdirAtNoFollow(root, "testdir", os.ModePerm)).To(Succeed())
+ _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "test"))
+ Expect(err).ToNot(HaveOccurred())
+ _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "testdir"))
+ Expect(err).ToNot(HaveOccurred())
+ p, err := JoinNoFollow(root, "test")
+ Expect(err).ToNot(HaveOccurred())
+ dir, err := JoinNoFollow(root, "testdir")
+ Expect(err).ToNot(HaveOccurred())
+ Expect(UnlinkAtNoFollow(p)).To(Succeed())
+ Expect(UnlinkAtNoFollow(dir)).To(Succeed())
+ _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "test"))
+ Expect(err).To(HaveOccurred())
+ _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "testdir"))
+ Expect(err).To(HaveOccurred())
+ })
+
+ It("should return base and relative paths correctly", func() {
+ baseDir := GinkgoT().TempDir()
+ root, err := JoinAndResolveWithRelativeRoot(baseDir)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(MkdirAtNoFollow(root, "test", os.ModePerm)).To(Succeed())
+ child, err := JoinNoFollow(root, "test")
+ Expect(err).ToNot(HaveOccurred())
+ Expect(unsafepath.UnsafeRoot(child.Raw())).To(Equal(baseDir))
+ Expect(unsafepath.UnsafeRelative(child.Raw())).To(Equal("/test"))
+ })
+
+ It("should append new relative root components to the relative path", func() {
+ baseDir := GinkgoT().TempDir()
+ root, err := JoinAndResolveWithRelativeRoot(baseDir)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(MkdirAtNoFollow(root, "test", os.ModePerm)).To(Succeed())
+ child, err := root.AppendAndResolveWithRelativeRoot("test")
+ Expect(err).ToNot(HaveOccurred())
+ Expect(unsafepath.UnsafeRoot(child.Raw())).To(Equal(baseDir))
+ Expect(unsafepath.UnsafeRelative(child.Raw())).To(Equal("/test"))
+ })
+
+ It("should detect absolute root", func() {
+ p, err := NewPathNoFollow("/")
+ Expect(err).ToNot(HaveOccurred())
+ Expect(p.IsRoot()).To(BeTrue())
+ tmpDir, err := JoinAndResolveWithRelativeRoot("/", GinkgoT().TempDir())
+ Expect(err).ToNot(HaveOccurred())
+ Expect(tmpDir.IsRoot()).To(BeFalse())
+ })
+
+ It("should be possible to use os.ReadDir on a safepath", func() {
+ baseDir := GinkgoT().TempDir()
+ root, err := JoinAndResolveWithRelativeRoot(baseDir)
+ Expect(err).ToNot(HaveOccurred())
+
+ Expect(os.MkdirAll(filepath.Join(baseDir, "test"), os.ModePerm)).To(Succeed())
+
+ var files []os.DirEntry
+ Expect(root.ExecuteNoFollow(func(safePath string) (err error) {
+ files, err = os.ReadDir(safePath)
+ return err
+ })).To(Succeed())
+ Expect(files).To(HaveLen(1))
+ })
+})
--
2.37.1
From e14b71261d31d0a28a981e1c5ae968c75af521e2 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:02:29 +0200
Subject: [PATCH 02/20] Add unsafepath package
Under some circumstances it is necessary for virt-handler to get
"unsafe" path segments which potentially cross mount-namespaces or cross
directories where at some point in the path the path suffixes are owned
by users and not by privileged entities anymore.
To make such unsafe access visible, encapsulate them in an unsafe
package. In the future imports can be allow-listed to avoid
unintentional wrong use.
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 1a8c0367230a907f368f3b7d4d5b1034dcd2f764)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/unsafepath/BUILD.bazel | 8 ++++++++
pkg/unsafepath/unsafepath.go | 27 +++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
create mode 100644 pkg/unsafepath/BUILD.bazel
create mode 100644 pkg/unsafepath/unsafepath.go
diff --git a/pkg/unsafepath/BUILD.bazel b/pkg/unsafepath/BUILD.bazel
new file mode 100644
index 000000000..1f3ec55b6
--- /dev/null
+++ b/pkg/unsafepath/BUILD.bazel
@@ -0,0 +1,8 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+ name = "go_default_library",
+ srcs = ["unsafepath.go"],
+ importpath = "kubevirt.io/kubevirt/pkg/unsafepath",
+ visibility = ["//visibility:public"],
+)
diff --git a/pkg/unsafepath/unsafepath.go b/pkg/unsafepath/unsafepath.go
new file mode 100644
index 000000000..e1ca82fdd
--- /dev/null
+++ b/pkg/unsafepath/unsafepath.go
@@ -0,0 +1,27 @@
+package unsafepath
+
+import "path/filepath"
+
+type Path struct {
+ rootBase string
+ relativePath string
+}
+
+func New(rootBase string, relativePath string) *Path {
+ return &Path{
+ rootBase: rootBase,
+ relativePath: relativePath,
+ }
+}
+
+func UnsafeAbsolute(path *Path) string {
+ return filepath.Join(path.rootBase, path.relativePath)
+}
+
+func UnsafeRelative(path *Path) string {
+ return path.relativePath
+}
+
+func UnsafeRoot(path *Path) string {
+ return path.rootBase
+}
--
2.37.1
From f619e7e45b68d817c94a0d238705a1aa117f647f Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:04:44 +0200
Subject: [PATCH 03/20] Move isolation detection code over to safepath
(cherry picked from commit 7de70e9a17eb0ce951a9ef05d526752aa2aeb88c)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/virt-handler/isolation/BUILD.bazel | 4 +
pkg/virt-handler/isolation/detector_test.go | 6 +-
.../isolation/generated_mock_isolation.go | 9 +-
pkg/virt-handler/isolation/isolation.go | 82 +++++++++++++------
pkg/virt-handler/isolation/isolation_test.go | 70 +++++++---------
pkg/virt-handler/isolation/validation.go | 11 ---
6 files changed, 102 insertions(+), 80 deletions(-)
diff --git a/pkg/virt-handler/isolation/BUILD.bazel b/pkg/virt-handler/isolation/BUILD.bazel
index 09c01594a..8f4d270fa 100644
--- a/pkg/virt-handler/isolation/BUILD.bazel
+++ b/pkg/virt-handler/isolation/BUILD.bazel
@@ -14,6 +14,8 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/container-disk:go_default_library",
+ "//pkg/safepath:go_default_library",
+ "//pkg/unsafepath:go_default_library",
"//pkg/util:go_default_library",
"//pkg/virt-controller/services:go_default_library",
"//pkg/virt-handler/cmd-client:go_default_library",
@@ -39,6 +41,8 @@ go_test(
data = glob(["testdata/**"]),
embed = [":go_default_library"],
deps = [
+ "//pkg/safepath:go_default_library",
+ "//pkg/unsafepath:go_default_library",
"//pkg/util:go_default_library",
"//pkg/virt-handler/cmd-client:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
diff --git a/pkg/virt-handler/isolation/detector_test.go b/pkg/virt-handler/isolation/detector_test.go
index 0fbb0fff7..241c81c47 100644
--- a/pkg/virt-handler/isolation/detector_test.go
+++ b/pkg/virt-handler/isolation/detector_test.go
@@ -35,6 +35,8 @@ import (
"github.com/mitchellh/go-ps"
v1 "kubevirt.io/api/core/v1"
+
+ "kubevirt.io/kubevirt/pkg/unsafepath"
cmdclient "kubevirt.io/kubevirt/pkg/virt-handler/cmd-client"
)
@@ -113,7 +115,9 @@ var _ = Describe("Isolation Detector", func() {
It("Should detect the Mount root of the test suite", func() {
result, err := NewSocketBasedIsolationDetector(tmpDir).Allowlist([]string{"devices"}).Detect(vm)
Expect(err).ToNot(HaveOccurred())
- Expect(result.MountRoot()).To(Equal(fmt.Sprintf("/proc/%d/root", os.Getpid())))
+ root, err := result.MountRoot()
+ Expect(err).ToNot(HaveOccurred())
+ Expect(unsafepath.UnsafeAbsolute(root.Raw())).To(Equal(fmt.Sprintf("/proc/%d/root", os.Getpid())))
})
})
})
diff --git a/pkg/virt-handler/isolation/generated_mock_isolation.go b/pkg/virt-handler/isolation/generated_mock_isolation.go
index a97358648..0f788fe71 100644
--- a/pkg/virt-handler/isolation/generated_mock_isolation.go
+++ b/pkg/virt-handler/isolation/generated_mock_isolation.go
@@ -6,6 +6,8 @@ package isolation
import (
gomock "github.com/golang/mock/gomock"
mountinfo "github.com/moby/sys/mountinfo"
+
+ safepath "kubevirt.io/kubevirt/pkg/safepath"
)
// Mock of IsolationResult interface
@@ -59,10 +61,11 @@ func (_mr *_MockIsolationResultRecorder) PIDNamespace() *gomock.Call {
return _mr.mock.ctrl.RecordCall(_mr.mock, "PIDNamespace")
}
-func (_m *MockIsolationResult) MountRoot() string {
+func (_m *MockIsolationResult) MountRoot() (*safepath.Path, error) {
ret := _m.ctrl.Call(_m, "MountRoot")
- ret0, _ := ret[0].(string)
- return ret0
+ ret0, _ := ret[0].(*safepath.Path)
+ ret1, _ := ret[1].(error)
+ return ret0, ret1
}
func (_mr *_MockIsolationResultRecorder) MountRoot() *gomock.Call {
diff --git a/pkg/virt-handler/isolation/isolation.go b/pkg/virt-handler/isolation/isolation.go
index 27dfa65e5..650b956c3 100644
--- a/pkg/virt-handler/isolation/isolation.go
+++ b/pkg/virt-handler/isolation/isolation.go
@@ -28,12 +28,15 @@ package isolation
import (
"fmt"
"os"
- "path/filepath"
"sort"
"strings"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
+
mount "github.com/moby/sys/mountinfo"
+ "kubevirt.io/kubevirt/pkg/safepath"
+
"kubevirt.io/client-go/log"
"kubevirt.io/kubevirt/pkg/util"
)
@@ -47,7 +50,7 @@ type IsolationResult interface {
// full path to the process namespace
PIDNamespace() string
// full path to the process root mount
- MountRoot() string
+ MountRoot() (*safepath.Path, error)
// full path to the mount namespace
MountNamespace() string
// mounts for the process
@@ -71,27 +74,33 @@ func (r *RealIsolationResult) MountNamespace() string {
return fmt.Sprintf("/proc/%d/ns/mnt", r.pid)
}
-// IsMounted checks if the given path is a mount point or not. Works with symlinks.
-func (r *RealIsolationResult) IsMounted(mountPoint string) (isMounted bool, err error) {
- mountPoint, err = filepath.Abs(mountPoint)
+// IsMounted checks if the given path is a mount point or not.
+func IsMounted(mountPoint *safepath.Path) (isMounted bool, err error) {
+ // Ensure that the path is still a valid absolute path without symlinks
+ f, err := safepath.OpenAtNoFollow(mountPoint)
if err != nil {
- return false, fmt.Errorf("failed to resolve %v to an absolute path: %v", mountPoint, err)
+ // treat os.IsNotExist() as error too
+ // since the inherent property of a safepath.Path is that the path must
+ // have existed at the point of object creation
+ return false, err
}
- mountPoint, err = filepath.EvalSymlinks(mountPoint)
- if err != nil {
- if os.IsNotExist(err) {
- return false, nil
- }
- return false, fmt.Errorf("could not resolve symlinks in path %v: %v", mountPoint, err)
+ defer f.Close()
+ if mountPoint.IsRoot() {
+ // mount.Mounted has purely string matching based special logic on how to treat "/".
+ // Emulating this for safepath here without ever having to call an unsafe method on our
+ // safepath.
+ return true, nil
+ } else {
+ // TODO: Unsafe full path is required, and not a fd, since otherwise mount table lookups and such would not work.
+ return mount.Mounted(unsafepath.UnsafeAbsolute(mountPoint.Raw()))
}
- return mount.Mounted(mountPoint)
}
// AreMounted checks if given paths are mounted by calling IsMounted.
// If error occurs, the first error is returned.
-func (r *RealIsolationResult) AreMounted(mountPoints ...string) (isMounted bool, err error) {
+func (r *RealIsolationResult) AreMounted(mountPoints ...*safepath.Path) (isMounted bool, err error) {
for _, mountPoint := range mountPoints {
- isMounted, err = r.IsMounted(mountPoint)
+ isMounted, err = IsMounted(mountPoint)
if !isMounted || err != nil {
return
}
@@ -101,19 +110,27 @@ func (r *RealIsolationResult) AreMounted(mountPoints ...string) (isMounted bool,
}
// IsBlockDevice checks if the given path is a block device or not.
-func (r *RealIsolationResult) IsBlockDevice(path string) (bool, error) {
- fileInfo, err := os.Stat(path)
+func IsBlockDevice(path *safepath.Path) (bool, error) {
+ fileInfo, err := safepath.StatAtNoFollow(path)
if err != nil {
return false, fmt.Errorf("error checking for block device: %v", err)
}
if fileInfo.IsDir() || (fileInfo.Mode()&os.ModeDevice) == 0 {
- return false, fmt.Errorf("found %v, but it's not a block device", path)
+ return false, nil
}
return true, nil
}
-func (r *RealIsolationResult) MountRoot() string {
- return fmt.Sprintf("/proc/%d/root", r.pid)
+func (r *RealIsolationResult) MountRoot() (*safepath.Path, error) {
+ return safepath.JoinAndResolveWithRelativeRoot(fmt.Sprintf("/proc/%d/root", r.pid))
+}
+
+func (r *RealIsolationResult) MountRootRelative(relativePath string) (*safepath.Path, error) {
+ mountRoot, err := r.MountRoot()
+ if err != nil {
+ return nil, err
+ }
+ return mountRoot.AppendAndResolveWithRelativeRoot(relativePath)
}
func (r *RealIsolationResult) Pid() int {
@@ -177,14 +194,31 @@ func parentMountInfoFor(parent IsolationResult, mountInfo *mount.Info) (*mount.I
// ParentPathForRootMount takes a container (child) and composes a path to
// the root mount point in the context of the parent.
-func ParentPathForRootMount(parent IsolationResult, child IsolationResult) (string, error) {
+func ParentPathForRootMount(parent IsolationResult, child IsolationResult) (*safepath.Path, error) {
childRootMountInfo, err := MountInfoRoot(child)
if err != nil {
- return "", err
+ return nil, err
}
parentMountInfo, err := parentMountInfoFor(parent, childRootMountInfo)
if err != nil {
- return "", err
+ return nil, err
+ }
+ parentMountRoot, err := parent.MountRoot()
+ if err != nil {
+ return nil, err
+ }
+ path := parentMountRoot
+ path, err = path.AppendAndResolveWithRelativeRoot(parentMountInfo.Mountpoint)
+ if err != nil {
+ return nil, err
+ }
+ return path.AppendAndResolveWithRelativeRoot(strings.TrimPrefix(childRootMountInfo.Root, parentMountInfo.Root))
+}
+
+func SafeJoin(res IsolationResult, elems ...string) (*safepath.Path, error) {
+ mountRoot, err := res.MountRoot()
+ if err != nil {
+ return nil, err
}
- return filepath.Join(parent.MountRoot(), parentMountInfo.Mountpoint, strings.TrimPrefix(childRootMountInfo.Root, parentMountInfo.Root)), nil
+ return mountRoot.AppendAndResolveWithRelativeRoot(elems...)
}
diff --git a/pkg/virt-handler/isolation/isolation_test.go b/pkg/virt-handler/isolation/isolation_test.go
index 5c4a1c969..1255e9aa7 100644
--- a/pkg/virt-handler/isolation/isolation_test.go
+++ b/pkg/virt-handler/isolation/isolation_test.go
@@ -2,7 +2,6 @@ package isolation
import (
"fmt"
- "io/ioutil"
"os"
"path/filepath"
@@ -11,6 +10,9 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
+ "kubevirt.io/kubevirt/pkg/safepath"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
+
"kubevirt.io/kubevirt/pkg/util"
)
@@ -28,41 +30,12 @@ var _ = Describe("IsolationResult", func() {
})
It("Should have root mounted", func() {
- mounted, err := isolationResult.IsMounted("/")
+ root, err := safepath.NewPathNoFollow("/")
Expect(err).ToNot(HaveOccurred())
- Expect(mounted).To(BeTrue())
- })
-
- It("Should resolve absolute paths with relative navigation", func() {
- mounted, err := isolationResult.IsMounted("/var/..")
+ mounted, err := IsMounted(root)
Expect(err).ToNot(HaveOccurred())
Expect(mounted).To(BeTrue())
})
-
- It("Should resolve relative paths", func() {
- _, err := isolationResult.IsMounted(".")
- Expect(err).ToNot(HaveOccurred())
- })
-
- It("Should resolve symlinks", func() {
- tmpDir, err := ioutil.TempDir("", "kubevirt")
- Expect(err).ToNot(HaveOccurred())
- defer os.RemoveAll(tmpDir)
-
- symlinkPath := filepath.Join(tmpDir, "mysymlink")
- err = os.Symlink("/", symlinkPath)
- Expect(err).ToNot(HaveOccurred())
-
- mounted, err := isolationResult.IsMounted(symlinkPath)
- Expect(err).ToNot(HaveOccurred())
- Expect(mounted).To(BeTrue())
- })
-
- It("Should regard a non-existent path as not mounted, not as an error", func() {
- mounted, err := isolationResult.IsMounted("/aasdfjhk")
- Expect(err).ToNot(HaveOccurred())
- Expect(mounted).To(BeFalse())
- })
})
Context("Container IsolationResult", func() {
@@ -82,9 +55,13 @@ var _ = Describe("IsolationResult", func() {
var ctrl *gomock.Controller
var mockIsolationResultNode *MockIsolationResult
var mockIsolationResultContainer *MockIsolationResult
+ var tmpDir string
BeforeEach(func() {
ctrl = gomock.NewController(GinkgoT())
+ tmpDir = GinkgoT().TempDir()
+ root, err := safepath.JoinAndResolveWithRelativeRoot(filepath.Join("/proc/self/root", tmpDir))
+ Expect(err).ToNot(HaveOccurred())
mockIsolationResultNode = NewMockIsolationResult(ctrl)
mockIsolationResultNode.EXPECT().
@@ -93,7 +70,7 @@ var _ = Describe("IsolationResult", func() {
AnyTimes()
mockIsolationResultNode.EXPECT().
MountRoot().
- Return("/proc/1/root").
+ Return(root, nil).
AnyTimes()
mockIsolationResultContainer = NewMockIsolationResult(ctrl)
@@ -117,17 +94,17 @@ var _ = Describe("IsolationResult", func() {
mountDriver: "overlay",
hostMountInfoFile: "overlay_host",
launcherMountInfoFile: "overlay_launcher",
- expectedPathToRootOnNode: "/proc/1/root/var/lib/docker/overlay2/f15d9ce07df72e80d809aa99ab4a171f2f3636f65f0653e75db8ca0befd8ae02/merged",
+ expectedPathToRootOnNode: "/var/lib/docker/overlay2/f15d9ce07df72e80d809aa99ab4a171f2f3636f65f0653e75db8ca0befd8ae02/merged",
}, {
mountDriver: "devicemapper",
hostMountInfoFile: "devicemapper_host",
launcherMountInfoFile: "devicemapper_launcher",
- expectedPathToRootOnNode: "/proc/1/root/var/lib/docker/devicemapper/mnt/d0990551ba8254871a449b2ff0d9063061ae96a2c195d7a850b62f030eae1710/rootfs",
+ expectedPathToRootOnNode: "/var/lib/docker/devicemapper/mnt/d0990551ba8254871a449b2ff0d9063061ae96a2c195d7a850b62f030eae1710/rootfs",
}, {
mountDriver: "btrfs",
hostMountInfoFile: "btrfs_host",
launcherMountInfoFile: "btrfs_launcher",
- expectedPathToRootOnNode: "/proc/1/root/var/lib/containers/storage/btrfs/subvolumes/e9a94e2cde75c54834378d4835d4eda6bebb56b02068b9254780de6f9344ad0e",
+ expectedPathToRootOnNode: "/var/lib/containers/storage/btrfs/subvolumes/e9a94e2cde75c54834378d4835d4eda6bebb56b02068b9254780de6f9344ad0e",
},
}
@@ -146,6 +123,7 @@ var _ = Describe("IsolationResult", func() {
Context(fmt.Sprintf("Using storage driver %v", dataset.mountDriver), func() {
BeforeEach(func() {
+ Expect(os.MkdirAll(filepath.Join(tmpDir, dataset.expectedPathToRootOnNode), os.ModePerm)).To(Succeed())
mockIsolationResultNode.EXPECT().
Mounts(gomock.Any()).
DoAndReturn(func(f mount.FilterFunc) ([]*mount.Info, error) {
@@ -170,7 +148,7 @@ var _ = Describe("IsolationResult", func() {
It("Should detect the full path to the root mount point of a container on the node", func() {
path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer)
Expect(err).ToNot(HaveOccurred())
- Expect(path).To(Equal(dataset.expectedPathToRootOnNode))
+ Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(filepath.Join("/proc/self/root", tmpDir, dataset.expectedPathToRootOnNode)))
})
})
}
@@ -221,6 +199,9 @@ var _ = Describe("IsolationResult", func() {
})
It("Should match the correct device", func() {
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/12"), os.ModePerm)).To(Succeed())
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/21"), os.ModePerm)).To(Succeed())
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/11"), os.ModePerm)).To(Succeed())
initMountsMock(mockIsolationResultContainer, []*mount.Info{rootMountPoint})
initMountsMock(mockIsolationResultNode, []*mount.Info{
{
@@ -243,10 +224,12 @@ var _ = Describe("IsolationResult", func() {
path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer)
Expect(err).ToNot(HaveOccurred())
- Expect(path).To(Equal("/proc/1/root/11"))
+ Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(filepath.Join("/proc/self/root", tmpDir, "/11")))
})
It("Should construct a valid path when the node mountpoint does not match the filesystem path", func() {
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/some/path"), os.ModePerm)).To(Succeed())
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/other/location"), os.ModePerm)).To(Succeed())
initMountsMock(mockIsolationResultContainer, []*mount.Info{
{
Major: 1,
@@ -266,10 +249,12 @@ var _ = Describe("IsolationResult", func() {
path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer)
Expect(err).ToNot(HaveOccurred())
- Expect(path).To(Equal("/proc/1/root/other/location"))
+ Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(filepath.Join("/proc/self/root", tmpDir, "/other/location")))
})
It("Should find the longest match for a filesystem path", func() {
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/some/path/quite/deeply/located/on/the/filesystem"), os.ModePerm)).To(Succeed())
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/long/filesystem"), os.ModePerm)).To(Succeed())
initMountsMock(mockIsolationResultContainer, []*mount.Info{
{
Major: 1,
@@ -299,7 +284,7 @@ var _ = Describe("IsolationResult", func() {
path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer)
Expect(err).ToNot(HaveOccurred())
- Expect(path).To(Equal("/proc/1/root/long/filesystem"))
+ Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(filepath.Join("/proc/self/root/", tmpDir, "long/filesystem")))
})
It("Should fail when the device does not exist", func() {
@@ -318,6 +303,7 @@ var _ = Describe("IsolationResult", func() {
})
It("Should fail if the target filesystem path is not mounted", func() {
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/other/path"), os.ModePerm)).To(Succeed())
initMountsMock(mockIsolationResultContainer, []*mount.Info{rootMountPoint})
initMountsMock(mockIsolationResultNode, []*mount.Info{
{
@@ -333,6 +319,8 @@ var _ = Describe("IsolationResult", func() {
})
It("Should not fail for duplicate mountpoints", func() {
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/mymounts/first"), os.ModePerm)).To(Succeed())
+ Expect(os.MkdirAll(filepath.Join(tmpDir, "/mymounts/second"), os.ModePerm)).To(Succeed())
initMountsMock(mockIsolationResultContainer, []*mount.Info{rootMountPoint})
initMountsMock(mockIsolationResultNode, []*mount.Info{
@@ -351,7 +339,7 @@ var _ = Describe("IsolationResult", func() {
path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer)
Expect(err).ToNot(HaveOccurred())
- Expect(path).To(HavePrefix("/proc/1/root/mymounts/"))
+ Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(HavePrefix(filepath.Join("/proc/self/root", tmpDir, "/mymounts/")))
})
})
})
diff --git a/pkg/virt-handler/isolation/validation.go b/pkg/virt-handler/isolation/validation.go
index 47cce570e..7c2728c84 100644
--- a/pkg/virt-handler/isolation/validation.go
+++ b/pkg/virt-handler/isolation/validation.go
@@ -3,9 +3,7 @@ package isolation
import (
"encoding/json"
"fmt"
- "os"
"os/exec"
- "path/filepath"
v1 "kubevirt.io/api/core/v1"
"kubevirt.io/client-go/log"
@@ -45,12 +43,3 @@ func GetImageInfo(imagePath string, context IsolationResult, config *v1.DiskVeri
}
return info, err
}
-
-func GetFileSize(imagePath string, context IsolationResult) (int64, error) {
- path := filepath.Join(context.MountRoot(), imagePath)
- info, err := os.Stat(path)
- if err != nil {
- return -1, err
- }
- return info.Size(), nil
-}
--
2.37.1
From 092c0e59ebb12ccfe9bade6661f2c82e25962133 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:07:09 +0200
Subject: [PATCH 04/20] Move ephemeral-disk-utils over to safepath
Change `SetFileOwnership` to use a safepath.Path and introduce
`UnsafeSetFileOwnership` as temporary solution for all places where a
safepath.Path does not have to be used at the moment to limit the
refactor scope.
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 76d446f7968eae8b2a4b6d153bdab61f4292a033)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/ephemeral-disk-utils/BUILD.bazel | 1 +
.../generated_mock_utils.go | 14 +++++++++++-
pkg/ephemeral-disk-utils/utils.go | 22 ++++++++++++++++---
3 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/pkg/ephemeral-disk-utils/BUILD.bazel b/pkg/ephemeral-disk-utils/BUILD.bazel
index de0379a63..1838443a7 100644
--- a/pkg/ephemeral-disk-utils/BUILD.bazel
+++ b/pkg/ephemeral-disk-utils/BUILD.bazel
@@ -9,6 +9,7 @@ go_library(
importpath = "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils",
visibility = ["//visibility:public"],
deps = [
+ "//pkg/safepath:go_default_library",
"//pkg/virt-launcher/virtwrap/api:go_default_library",
"//vendor/github.com/golang/mock/gomock:go_default_library",
],
diff --git a/pkg/ephemeral-disk-utils/generated_mock_utils.go b/pkg/ephemeral-disk-utils/generated_mock_utils.go
index b3b5ea8dc..e5403e3d4 100644
--- a/pkg/ephemeral-disk-utils/generated_mock_utils.go
+++ b/pkg/ephemeral-disk-utils/generated_mock_utils.go
@@ -5,6 +5,8 @@ package ephemeraldiskutils
import (
gomock "github.com/golang/mock/gomock"
+
+ safepath "kubevirt.io/kubevirt/pkg/safepath"
)
// Mock of OwnershipManagerInterface interface
@@ -28,7 +30,17 @@ func (_m *MockOwnershipManagerInterface) EXPECT() *_MockOwnershipManagerInterfac
return _m.recorder
}
-func (_m *MockOwnershipManagerInterface) SetFileOwnership(file string) error {
+func (_m *MockOwnershipManagerInterface) UnsafeSetFileOwnership(file string) error {
+ ret := _m.ctrl.Call(_m, "UnsafeSetFileOwnership", file)
+ ret0, _ := ret[0].(error)
+ return ret0
+}
+
+func (_mr *_MockOwnershipManagerInterfaceRecorder) UnsafeSetFileOwnership(arg0 interface{}) *gomock.Call {
+ return _mr.mock.ctrl.RecordCall(_mr.mock, "UnsafeSetFileOwnership", arg0)
+}
+
+func (_m *MockOwnershipManagerInterface) SetFileOwnership(file *safepath.Path) error {
ret := _m.ctrl.Call(_m, "SetFileOwnership", file)
ret0, _ := ret[0].(error)
return ret0
diff --git a/pkg/ephemeral-disk-utils/utils.go b/pkg/ephemeral-disk-utils/utils.go
index db2e38137..e4719bcf7 100644
--- a/pkg/ephemeral-disk-utils/utils.go
+++ b/pkg/ephemeral-disk-utils/utils.go
@@ -28,6 +28,7 @@ import (
"strconv"
"syscall"
+ "kubevirt.io/kubevirt/pkg/safepath"
"kubevirt.io/kubevirt/pkg/virt-launcher/virtwrap/api"
)
@@ -42,7 +43,11 @@ func MockDefaultOwnershipManager() {
type nonOpManager struct {
}
-func (no *nonOpManager) SetFileOwnership(file string) error {
+func (no *nonOpManager) UnsafeSetFileOwnership(file string) error {
+ return nil
+}
+
+func (no *nonOpManager) SetFileOwnership(file *safepath.Path) error {
return nil
}
@@ -50,7 +55,16 @@ type OwnershipManager struct {
user string
}
-func (om *OwnershipManager) SetFileOwnership(file string) error {
+func (om *OwnershipManager) SetFileOwnership(file *safepath.Path) error {
+ fd, err := safepath.OpenAtNoFollow(file)
+ if err != nil {
+ return err
+ }
+ defer fd.Close()
+ return om.UnsafeSetFileOwnership(fd.SafePath())
+}
+
+func (om *OwnershipManager) UnsafeSetFileOwnership(file string) error {
owner, err := user.Lookup(om.user)
if err != nil {
return fmt.Errorf("failed to look up user %s: %v", om.user, err)
@@ -104,7 +118,9 @@ func FileExists(path string) (bool, error) {
}
type OwnershipManagerInterface interface {
- SetFileOwnership(file string) error
+ // Deprecated: UnsafeSetFileOwnership should not be used. Use SetFileOwnership instead.
+ UnsafeSetFileOwnership(file string) error
+ SetFileOwnership(file *safepath.Path) error
}
func GetEphemeralBackingSourceBlockDevices(domain *api.Domain) map[string]bool {
--
2.37.1
From d9537af4582bea095d1deeae0e7347b5b824a9e3 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:19:45 +0200
Subject: [PATCH 05/20] Move virt-chroot over to safepath usage
Pass fully resolved paths to virt-chroot and ensure that virt-chroot
will re-create safepath.Paths out of the input before trying to use it,
to ensure no path escapes.
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 21ee498079cfee9cacc646ac91340a3384e60ce2)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
cmd/virt-chroot/BUILD.bazel | 1 +
cmd/virt-chroot/main.go | 40 +++++++++++++++++++--
cmd/virt-chroot/selinux.go | 40 +++++++++++++++++++--
pkg/virt-handler/selinux/BUILD.bazel | 2 ++
pkg/virt-handler/selinux/labels.go | 6 ++--
pkg/virt-handler/virt-chroot/BUILD.bazel | 4 +++
pkg/virt-handler/virt-chroot/virt-chroot.go | 28 +++++++++++++--
7 files changed, 111 insertions(+), 10 deletions(-)
diff --git a/cmd/virt-chroot/BUILD.bazel b/cmd/virt-chroot/BUILD.bazel
index 29cdc81b3..fd26041a0 100644
--- a/cmd/virt-chroot/BUILD.bazel
+++ b/cmd/virt-chroot/BUILD.bazel
@@ -12,6 +12,7 @@ go_library(
importpath = "kubevirt.io/kubevirt/cmd/virt-chroot",
visibility = ["//visibility:private"],
deps = [
+ "//pkg/safepath:go_default_library",
"//pkg/virt-handler/cgroup:go_default_library",
"//vendor/github.com/opencontainers/runc/libcontainer/cgroups/fs:go_default_library",
"//vendor/github.com/opencontainers/runc/libcontainer/cgroups/fs2:go_default_library",
diff --git a/cmd/virt-chroot/main.go b/cmd/virt-chroot/main.go
index e934420a2..e28daa07c 100644
--- a/cmd/virt-chroot/main.go
+++ b/cmd/virt-chroot/main.go
@@ -11,6 +11,8 @@ import (
"github.com/spf13/cobra"
"golang.org/x/sys/unix"
+
+ "kubevirt.io/kubevirt/pkg/safepath"
)
var (
@@ -149,7 +151,25 @@ func main() {
}
}
- return syscall.Mount(args[0], args[1], fsType, uintptr(mntOpts), "")
+ // Ensure that sourceFile is a real path. It will be kept open until used
+ // by the syscall via the file descriptor path in proc (SafePath) to ensure
+ // that no symlink injection can happen after the check.
+ sourceFile, err := safepath.NewFileNoFollow(args[0])
+ if err != nil {
+ return fmt.Errorf("mount source invalid: %v", err)
+ }
+ defer sourceFile.Close()
+
+ // Ensure that targetFile is a real path. It will be kept open until used
+ // by the syscall via the file descriptor path in proc (SafePath) to ensure
+ // that no symlink injection can happen after the check.
+ targetFile, err := safepath.NewFileNoFollow(args[1])
+ if err != nil {
+ return fmt.Errorf("mount target invalid: %v", err)
+ }
+ defer targetFile.Close()
+
+ return syscall.Mount(sourceFile.SafePath(), targetFile.SafePath(), fsType, uintptr(mntOpts), "")
},
}
mntCmd.Flags().StringP("options", "o", "", "comma separated list of mount options")
@@ -160,7 +180,23 @@ func main() {
Short: "unmount in a specific mount namespace",
Args: cobra.MinimumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
- return syscall.Unmount(args[0], 0)
+ // Ensure that targetFile is a real path. It will be kept open until used
+ // by the syscall via the file descriptor path in proc (SafePath) to ensure
+ // that no symlink injection can happen after the check.
+ targetFile, err := safepath.NewPathNoFollow(args[0])
+ if err != nil {
+ return fmt.Errorf("mount target invalid: %v", err)
+ }
+ err = targetFile.ExecuteNoFollow(func(safePath string) error {
+ // we actively hold an open reference to the mount point,
+ // we have to lazy unmount, to not block ourselves
+ // with the active file-descriptor.
+ return syscall.Unmount(safePath, unix.MNT_DETACH)
+ })
+ if err != nil {
+ return fmt.Errorf("umount failed: %v", err)
+ }
+ return nil
},
}
diff --git a/cmd/virt-chroot/selinux.go b/cmd/virt-chroot/selinux.go
index af9174c1a..29cc03712 100644
--- a/cmd/virt-chroot/selinux.go
+++ b/cmd/virt-chroot/selinux.go
@@ -4,11 +4,20 @@ import (
"bytes"
"fmt"
"io/ioutil"
+ "os"
+ "path/filepath"
"github.com/opencontainers/selinux/go-selinux"
"github.com/spf13/cobra"
+ "golang.org/x/sys/unix"
+
+ "kubevirt.io/kubevirt/pkg/safepath"
)
+const xattrNameSelinux = "security.selinux"
+
+var root string
+
// NewGetEnforceCommand determines if selinux is enabled in the kernel (enforced or permissive)
func NewGetEnforceCommand() *cobra.Command {
cmd := &cobra.Command{
@@ -31,7 +40,7 @@ func NewGetEnforceCommand() *cobra.Command {
}
func RelabelCommand() *cobra.Command {
- return &cobra.Command{
+ relabelCommad := &cobra.Command{
Use: "relabel",
Short: "relabel a file with the given selinux label, if the path is not labeled like this already",
Example: "virt-chroot selinux relabel <new-label> <file-path>",
@@ -39,15 +48,38 @@ func RelabelCommand() *cobra.Command {
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
label := args[0]
- filePath := args[1]
+ if root == "" {
+ root = "/"
+ }
+
+ rootPath, err := safepath.JoinAndResolveWithRelativeRoot(root)
+ if err != nil {
+ return fmt.Errorf("failed to open root path %v: %v", rootPath, err)
+ }
+ safePath, err := safepath.JoinNoFollow(rootPath, args[1])
+ if err != nil {
+ return fmt.Errorf("failed to open final path %v: %v", filepath.Join(root, args[1]), err)
+ }
+ fd, err := safepath.OpenAtNoFollow(safePath)
+ if err != nil {
+ return fmt.Errorf("could not open file %v. Reason: %v", safePath, err)
+ }
+ defer fd.Close()
+ filePath := fd.SafePath()
currentFileLabel, err := selinux.FileLabel(filePath)
if err != nil {
return fmt.Errorf("could not retrieve label of file %s. Reason: %v", filePath, err)
}
+ writeableFD, err := os.OpenFile(filePath, os.O_APPEND|unix.S_IWRITE, os.ModePerm)
+ if err != nil {
+ return fmt.Errorf("error reopening file %s to write label %s. Reason: %v", filePath, label, err)
+ }
+ defer writeableFD.Close()
+
if currentFileLabel != label {
- if err := selinux.Chcon(filePath, label, false); err != nil {
+ if err := unix.Fsetxattr(int(writeableFD.Fd()), xattrNameSelinux, []byte(label), 0); err != nil {
return fmt.Errorf("error relabeling file %s with label %s. Reason: %v", filePath, label, err)
}
}
@@ -55,4 +87,6 @@ func RelabelCommand() *cobra.Command {
return nil
},
}
+ relabelCommad.Flags().StringVar(&root, "root", "/", "safe root path which will be prepended to passed in files")
+ return relabelCommad
}
diff --git a/pkg/virt-handler/selinux/BUILD.bazel b/pkg/virt-handler/selinux/BUILD.bazel
index e7c0fd083..3872f4e3f 100644
--- a/pkg/virt-handler/selinux/BUILD.bazel
+++ b/pkg/virt-handler/selinux/BUILD.bazel
@@ -11,6 +11,8 @@ go_library(
importpath = "kubevirt.io/kubevirt/pkg/virt-handler/selinux",
visibility = ["//visibility:public"],
deps = [
+ "//pkg/safepath:go_default_library",
+ "//pkg/unsafepath:go_default_library",
"//pkg/virt-handler/virt-chroot:go_default_library",
"//staging/src/kubevirt.io/client-go/log:go_default_library",
"//vendor/github.com/golang/mock/gomock:go_default_library",
diff --git a/pkg/virt-handler/selinux/labels.go b/pkg/virt-handler/selinux/labels.go
index 01f17db26..36c02e066 100644
--- a/pkg/virt-handler/selinux/labels.go
+++ b/pkg/virt-handler/selinux/labels.go
@@ -8,6 +8,8 @@ import (
"path/filepath"
"strings"
+ "kubevirt.io/kubevirt/pkg/safepath"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
virt_chroot "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot"
"kubevirt.io/client-go/log"
@@ -164,10 +166,10 @@ type SELinux interface {
IsPermissive() bool
}
-func RelabelFiles(newLabel string, continueOnError bool, files ...string) error {
+func RelabelFiles(newLabel string, continueOnError bool, files ...*safepath.Path) error {
relabelArgs := []string{"selinux", "relabel", newLabel}
for _, file := range files {
- cmd := exec.Command("virt-chroot", append(relabelArgs, file)...)
+ cmd := exec.Command("virt-chroot", append(relabelArgs, "--root", unsafepath.UnsafeRoot(file.Raw()), unsafepath.UnsafeRelative(file.Raw()))...)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
err := cmd.Run()
diff --git a/pkg/virt-handler/virt-chroot/BUILD.bazel b/pkg/virt-handler/virt-chroot/BUILD.bazel
index 776719e08..1c59d345c 100644
--- a/pkg/virt-handler/virt-chroot/BUILD.bazel
+++ b/pkg/virt-handler/virt-chroot/BUILD.bazel
@@ -5,4 +5,8 @@ go_library(
srcs = ["virt-chroot.go"],
importpath = "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot",
visibility = ["//visibility:public"],
+ deps = [
+ "//pkg/safepath:go_default_library",
+ "//pkg/unsafepath:go_default_library",
+ ],
)
diff --git a/pkg/virt-handler/virt-chroot/virt-chroot.go b/pkg/virt-handler/virt-chroot/virt-chroot.go
index e00dbc54d..4160212b7 100644
--- a/pkg/virt-handler/virt-chroot/virt-chroot.go
+++ b/pkg/virt-handler/virt-chroot/virt-chroot.go
@@ -19,7 +19,13 @@
package virt_chroot
-import "os/exec"
+import (
+ "os/exec"
+ "strings"
+
+ "kubevirt.io/kubevirt/pkg/safepath"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
+)
const (
binaryPath = "/usr/bin/virt-chroot"
@@ -38,7 +44,13 @@ func GetChrootMountNamespace() string {
return mountNamespace
}
-func MountChroot(sourcePath, targetPath string, ro bool) *exec.Cmd {
+func MountChroot(sourcePath, targetPath *safepath.Path, ro bool) *exec.Cmd {
+ return UnsafeMountChroot(trimProcPrefix(sourcePath), trimProcPrefix(targetPath), ro)
+}
+
+// Deprecated: UnsafeMountChroot is used to connect to code which needs to be refactored
+// to handle mounts securely.
+func UnsafeMountChroot(sourcePath, targetPath string, ro bool) *exec.Cmd {
args := append(getBaseArgs(), "mount", "-o")
optionArgs := "bind"
@@ -50,7 +62,13 @@ func MountChroot(sourcePath, targetPath string, ro bool) *exec.Cmd {
return exec.Command(binaryPath, args...)
}
-func UmountChroot(path string) *exec.Cmd {
+func UmountChroot(path *safepath.Path) *exec.Cmd {
+ return UnsafeUmountChroot(trimProcPrefix(path))
+}
+
+// Deprecated: UnsafeUmountChroot is used to connect to code which needs to be refactored
+// to handle mounts securely.
+func UnsafeUmountChroot(path string) *exec.Cmd {
args := append(getBaseArgs(), "umount", path)
return exec.Command(binaryPath, args...)
}
@@ -71,3 +89,7 @@ func RemoveMDEVType(mdevUUID string) *exec.Cmd {
func ExecChroot(args ...string) *exec.Cmd {
return exec.Command(binaryPath, args...)
}
+
+func trimProcPrefix(path *safepath.Path) string {
+ return strings.TrimPrefix(unsafepath.UnsafeAbsolute(path.Raw()), "/proc/1/root")
+}
--
2.37.1
From 3a79923310de2861547448aee73207f85de7a639 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:09:15 +0200
Subject: [PATCH 06/20] Move containerdisk path resolution over to use the
safepath package
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit f0da2cc5778178ec3de85e66239579fc373ad8d9)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/container-disk/BUILD.bazel | 3 +
pkg/container-disk/container-disk.go | 70 ++++++++++++-----------
pkg/container-disk/container-disk_test.go | 31 +++++-----
pkg/ephemeral-disk/ephemeral-disk.go | 2 +-
4 files changed, 54 insertions(+), 52 deletions(-)
diff --git a/pkg/container-disk/BUILD.bazel b/pkg/container-disk/BUILD.bazel
index 13c4d7ecc..138db3b70 100644
--- a/pkg/container-disk/BUILD.bazel
+++ b/pkg/container-disk/BUILD.bazel
@@ -11,6 +11,8 @@ go_library(
deps = [
"//pkg/ephemeral-disk:go_default_library",
"//pkg/ephemeral-disk-utils:go_default_library",
+ "//pkg/safepath:go_default_library",
+ "//pkg/unsafepath:go_default_library",
"//pkg/util:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
"//staging/src/kubevirt.io/client-go/log:go_default_library",
@@ -28,6 +30,7 @@ go_test(
],
embed = [":go_default_library"],
deps = [
+ "//pkg/unsafepath:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
"//staging/src/kubevirt.io/client-go/api:go_default_library",
"//staging/src/kubevirt.io/client-go/testutils:go_default_library",
diff --git a/pkg/container-disk/container-disk.go b/pkg/container-disk/container-disk.go
index e0ff3038b..2240877e3 100644
--- a/pkg/container-disk/container-disk.go
+++ b/pkg/container-disk/container-disk.go
@@ -30,6 +30,9 @@ import (
"kubevirt.io/client-go/log"
+ "kubevirt.io/kubevirt/pkg/safepath"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
+
kubev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
@@ -64,7 +67,7 @@ func GetVolumeMountDirOnGuest(vmi *v1.VirtualMachineInstance) string {
return filepath.Join(mountBaseDir, string(vmi.UID))
}
-func GetVolumeMountDirOnHost(vmi *v1.VirtualMachineInstance) (string, bool, error) {
+func GetVolumeMountDirOnHost(vmi *v1.VirtualMachineInstance) (*safepath.Path, error) {
basepath := ""
foundEntries := 0
foundBasepath := ""
@@ -72,7 +75,7 @@ func GetVolumeMountDirOnHost(vmi *v1.VirtualMachineInstance) (string, bool, erro
basepath = fmt.Sprintf("%s/%s/volumes/kubernetes.io~empty-dir/container-disks", podsBaseDir, string(podUID))
exists, err := diskutils.FileExists(basepath)
if err != nil {
- return "", false, err
+ return nil, err
} else if exists {
foundEntries++
foundBasepath = basepath
@@ -80,37 +83,29 @@ func GetVolumeMountDirOnHost(vmi *v1.VirtualMachineInstance) (string, bool, erro
}
if foundEntries == 1 {
- return foundBasepath, true, nil
+ return safepath.JoinAndResolveWithRelativeRoot("/", foundBasepath)
} else if foundEntries > 1 {
// Don't mount until outdated pod environments are removed
// otherwise we might stomp on a previous cleanup
- return "", false, fmt.Errorf("Found multiple pods active for vmi %s/%s. Waiting on outdated pod directories to be removed", vmi.Namespace, vmi.Name)
+ return nil, fmt.Errorf("Found multiple pods active for vmi %s/%s. Waiting on outdated pod directories to be removed", vmi.Namespace, vmi.Name)
}
- return "", false, nil
+ return nil, os.ErrNotExist
}
-func GetDiskTargetDirFromHostView(vmi *v1.VirtualMachineInstance) (string, error) {
- basepath, found, err := GetVolumeMountDirOnHost(vmi)
+func GetDiskTargetDirFromHostView(vmi *v1.VirtualMachineInstance) (*safepath.Path, error) {
+ basepath, err := GetVolumeMountDirOnHost(vmi)
if err != nil {
- return "", err
- } else if !found {
- return "", fmt.Errorf("container disk volume for vmi not found")
+ return nil, err
}
-
return basepath, nil
}
-func GetDiskTargetPathFromHostView(vmi *v1.VirtualMachineInstance, volumeIndex int) (string, error) {
- basepath, err := GetDiskTargetDirFromHostView(vmi)
- if err != nil {
- return "", err
- }
-
- return fmt.Sprintf("%s/disk_%d.img", basepath, volumeIndex), nil
+func GetDiskTargetName(volumeIndex int) string {
+ return fmt.Sprintf("disk_%d.img", volumeIndex)
}
func GetDiskTargetPathFromLauncherView(volumeIndex int) string {
- return filepath.Join(mountBaseDir, fmt.Sprintf("disk_%d.img", volumeIndex))
+ return filepath.Join(mountBaseDir, GetDiskTargetName(volumeIndex))
}
func GetKernelBootArtifactPathFromLauncherView(artifact string) string {
@@ -184,28 +179,35 @@ func NewKernelBootSocketPathGetter(baseDir string) KernelBootSocketPathGetter {
}
}
-func GetImage(root string, imagePath string) (string, error) {
- fallbackPath := filepath.Join(root, DiskSourceFallbackPath)
+func GetImage(root *safepath.Path, imagePath string) (*safepath.Path, error) {
if imagePath != "" {
- imagePath = filepath.Join(root, imagePath)
- if _, err := os.Stat(imagePath); err != nil {
- if os.IsNotExist(err) {
- return "", fmt.Errorf("No image on path %s", imagePath)
- } else {
- return "", fmt.Errorf("Failed to check if an image exists at %s", imagePath)
- }
+ var err error
+ resolvedPath, err := root.AppendAndResolveWithRelativeRoot(imagePath)
+ if err != nil {
+ return nil, fmt.Errorf("failed to determine custom image path %s: %v", imagePath, err)
}
+ return resolvedPath, nil
} else {
- files, err := os.ReadDir(fallbackPath)
+ fallbackPath, err := root.AppendAndResolveWithRelativeRoot(DiskSourceFallbackPath)
if err != nil {
- return "", fmt.Errorf("Failed to check %s for disks: %v", fallbackPath, err)
+ return nil, fmt.Errorf("failed to determine default image path %v: %v", fallbackPath, err)
}
- if len(files) > 1 {
- return "", fmt.Errorf("More than one file found in folder %s, only one disk is allowed", DiskSourceFallbackPath)
+ files, err := os.ReadDir(unsafepath.UnsafeAbsolute(fallbackPath.Raw()))
+ if err != nil {
+ return nil, fmt.Errorf("failed to check default image path %s: %v", fallbackPath, err)
+ }
+ if len(files) == 0 {
+ return nil, fmt.Errorf("no file found in folder %s, no disk present", DiskSourceFallbackPath)
+ } else if len(files) > 1 {
+ return nil, fmt.Errorf("more than one file found in folder %s, only one disk is allowed", DiskSourceFallbackPath)
+ }
+ fileName := files[0].Name()
+ resolvedPath, err := root.AppendAndResolveWithRelativeRoot(DiskSourceFallbackPath, fileName)
+ if err != nil {
+ return nil, fmt.Errorf("failed to check default image path %s: %v", imagePath, err)
}
- imagePath = filepath.Join(fallbackPath, files[0].Name())
+ return resolvedPath, nil
}
- return imagePath, nil
}
func GenerateInitContainers(vmi *v1.VirtualMachineInstance, imageIDs map[string]string, podVolumeName string, binVolumeName string) []kubev1.Container {
diff --git a/pkg/container-disk/container-disk_test.go b/pkg/container-disk/container-disk_test.go
index 7fe82787d..a9766c5d6 100644
--- a/pkg/container-disk/container-disk_test.go
+++ b/pkg/container-disk/container-disk_test.go
@@ -27,6 +27,8 @@ import (
"path/filepath"
"strings"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
+
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
k8sv1 "k8s.io/api/core/v1"
@@ -102,28 +104,26 @@ var _ = Describe("ContainerDisk", func() {
}
// should not be found if dir doesn't exist
- path, found, err := GetVolumeMountDirOnHost(vmi)
- Expect(err).ToNot(HaveOccurred())
- Expect(found).To(BeFalse())
- Expect(path).To(Equal(""))
+ path, err := GetVolumeMountDirOnHost(vmi)
+ Expect(err).To(HaveOccurred())
+ Expect(os.IsNotExist(err)).To(BeTrue())
// should be found if dir does exist
expectedPath := fmt.Sprintf("%s/1234/volumes/kubernetes.io~empty-dir/container-disks", tmpDir)
os.MkdirAll(expectedPath, 0755)
- path, found, err = GetVolumeMountDirOnHost(vmi)
+ path, err = GetVolumeMountDirOnHost(vmi)
Expect(err).ToNot(HaveOccurred())
- Expect(found).To(BeTrue())
- Expect(path).To(Equal(expectedPath))
+ Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(expectedPath))
// should be able to generate legacy socket path dir
legacySocket := GetLegacyVolumeMountDirOnHost(vmi)
Expect(legacySocket).To(Equal(filepath.Join(tmpDir, "6789")))
- // should return error if disk target doesn't exist
- targetPath, err := GetDiskTargetPathFromHostView(vmi, 1)
- expectedPath = fmt.Sprintf("%s/1234/volumes/kubernetes.io~empty-dir/container-disks/disk_1.img", tmpDir)
+ // should return error if disk target dir doesn't exist
+ targetPath, err := GetDiskTargetDirFromHostView(vmi)
+ expectedPath = fmt.Sprintf("%s/1234/volumes/kubernetes.io~empty-dir/container-disks", tmpDir)
Expect(err).ToNot(HaveOccurred())
- Expect(targetPath).To(Equal(expectedPath))
+ Expect(unsafepath.UnsafeAbsolute(targetPath.Raw())).To(Equal(expectedPath))
})
@@ -138,18 +138,15 @@ var _ = Describe("ContainerDisk", func() {
// should not return error if only one dir exists
expectedPath := fmt.Sprintf("%s/1234/volumes/kubernetes.io~empty-dir/container-disks", tmpDir)
os.MkdirAll(expectedPath, 0755)
- path, found, err := GetVolumeMountDirOnHost(vmi)
+ path, err := GetVolumeMountDirOnHost(vmi)
Expect(err).ToNot(HaveOccurred())
- Expect(found).To(BeTrue())
- Expect(path).To(Equal(expectedPath))
+ Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(expectedPath))
// return error if two dirs exist
secondPath := fmt.Sprintf("%s/5678/volumes/kubernetes.io~empty-dir/container-disks", tmpDir)
os.MkdirAll(secondPath, 0755)
- path, found, err = GetVolumeMountDirOnHost(vmi)
+ path, err = GetVolumeMountDirOnHost(vmi)
Expect(err).To(HaveOccurred())
- Expect(found).To(BeFalse())
- Expect(path).To(Equal(""))
})
It("by verifying launcher directory locations", func() {
diff --git a/pkg/ephemeral-disk/ephemeral-disk.go b/pkg/ephemeral-disk/ephemeral-disk.go
index 03dafed17..526049d5d 100644
--- a/pkg/ephemeral-disk/ephemeral-disk.go
+++ b/pkg/ephemeral-disk/ephemeral-disk.go
@@ -118,7 +118,7 @@ func (c *ephemeralDiskCreator) CreateBackedImageForVolume(volume v1.Volume, back
}
// We need to ensure that the permissions are setup correctly.
- err = diskutils.DefaultOwnershipManager.SetFileOwnership(imagePath)
+ err = diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(imagePath)
return err
}
--
2.37.1
From 058470e36ccd5a345e1193a059e54e4ac573a2c8 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:26:47 +0200
Subject: [PATCH 07/20] Move containerdisk mount operations to safepath
operations
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 0f817a789cdbcb770a401a32b11af5653987d585)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/container-disk/BUILD.bazel | 1 -
pkg/container-disk/container-disk.go | 11 +-
pkg/virt-handler/container-disk/BUILD.bazel | 2 +
pkg/virt-handler/container-disk/mount.go | 170 +++++++++-----------
4 files changed, 88 insertions(+), 96 deletions(-)
diff --git a/pkg/container-disk/BUILD.bazel b/pkg/container-disk/BUILD.bazel
index 138db3b70..2958637b4 100644
--- a/pkg/container-disk/BUILD.bazel
+++ b/pkg/container-disk/BUILD.bazel
@@ -12,7 +12,6 @@ go_library(
"//pkg/ephemeral-disk:go_default_library",
"//pkg/ephemeral-disk-utils:go_default_library",
"//pkg/safepath:go_default_library",
- "//pkg/unsafepath:go_default_library",
"//pkg/util:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
"//staging/src/kubevirt.io/client-go/log:go_default_library",
diff --git a/pkg/container-disk/container-disk.go b/pkg/container-disk/container-disk.go
index 2240877e3..d0cdce20f 100644
--- a/pkg/container-disk/container-disk.go
+++ b/pkg/container-disk/container-disk.go
@@ -30,12 +30,11 @@ import (
"kubevirt.io/client-go/log"
- "kubevirt.io/kubevirt/pkg/safepath"
- "kubevirt.io/kubevirt/pkg/unsafepath"
-
kubev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
+ "kubevirt.io/kubevirt/pkg/safepath"
+
ephemeraldisk "kubevirt.io/kubevirt/pkg/ephemeral-disk"
v1 "kubevirt.io/api/core/v1"
@@ -192,7 +191,11 @@ func GetImage(root *safepath.Path, imagePath string) (*safepath.Path, error) {
if err != nil {
return nil, fmt.Errorf("failed to determine default image path %v: %v", fallbackPath, err)
}
- files, err := os.ReadDir(unsafepath.UnsafeAbsolute(fallbackPath.Raw()))
+ var files []os.DirEntry
+ err = fallbackPath.ExecuteNoFollow(func(safePath string) (err error) {
+ files, err = os.ReadDir(safePath)
+ return err
+ })
if err != nil {
return nil, fmt.Errorf("failed to check default image path %s: %v", fallbackPath, err)
}
diff --git a/pkg/virt-handler/container-disk/BUILD.bazel b/pkg/virt-handler/container-disk/BUILD.bazel
index ebc265c83..476389a08 100644
--- a/pkg/virt-handler/container-disk/BUILD.bazel
+++ b/pkg/virt-handler/container-disk/BUILD.bazel
@@ -11,6 +11,8 @@ go_library(
deps = [
"//pkg/container-disk:go_default_library",
"//pkg/ephemeral-disk-utils:go_default_library",
+ "//pkg/safepath:go_default_library",
+ "//pkg/unsafepath:go_default_library",
"//pkg/util:go_default_library",
"//pkg/virt-config:go_default_library",
"//pkg/virt-handler/isolation:go_default_library",
diff --git a/pkg/virt-handler/container-disk/mount.go b/pkg/virt-handler/container-disk/mount.go
index 41359bbdc..710bd51c3 100644
--- a/pkg/virt-handler/container-disk/mount.go
+++ b/pkg/virt-handler/container-disk/mount.go
@@ -9,6 +9,9 @@ import (
"sync"
"time"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
+
+ "kubevirt.io/kubevirt/pkg/safepath"
"kubevirt.io/kubevirt/pkg/util"
virtconfig "kubevirt.io/kubevirt/pkg/virt-config"
virt_chroot "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot"
@@ -207,7 +210,18 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co
for i, volume := range vmi.Spec.Volumes {
if volume.ContainerDisk != nil {
- targetFile, err := containerdisk.GetDiskTargetPathFromHostView(vmi, i)
+ diskTargetDir, err := containerdisk.GetDiskTargetDirFromHostView(vmi)
+ if err != nil {
+ return nil, err
+ }
+ diskName := containerdisk.GetDiskTargetName(i)
+ // If diskName is a symlink it will fail if the target exists.
+ if err := safepath.TouchAtNoFollow(diskTargetDir, diskName, os.ModePerm); err != nil {
+ if err != nil && !os.IsExist(err) {
+ return nil, fmt.Errorf("failed to create mount point target: %v", err)
+ }
+ }
+ targetFile, err := safepath.JoinNoFollow(diskTargetDir, diskName)
if err != nil {
return nil, err
}
@@ -218,7 +232,7 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co
}
record.MountTargetEntries = append(record.MountTargetEntries, vmiMountTargetEntry{
- TargetFile: targetFile,
+ TargetFile: unsafepath.UnsafeAbsolute(targetFile.Raw()),
SocketFile: sock,
})
}
@@ -238,14 +252,19 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co
for i, volume := range vmi.Spec.Volumes {
if volume.ContainerDisk != nil {
- targetFile, err := containerdisk.GetDiskTargetPathFromHostView(vmi, i)
+ diskTargetDir, err := containerdisk.GetDiskTargetDirFromHostView(vmi)
+ if err != nil {
+ return nil, err
+ }
+ diskName := containerdisk.GetDiskTargetName(i)
+ targetFile, err := safepath.JoinNoFollow(diskTargetDir, diskName)
if err != nil {
return nil, err
}
nodeRes := isolation.NodeIsolationResult()
- if isMounted, err := nodeRes.IsMounted(targetFile); err != nil {
+ if isMounted, err := isolation.IsMounted(targetFile); err != nil {
return nil, fmt.Errorf("failed to determine if %s is already mounted: %v", targetFile, err)
} else if !isMounted {
sock, err := m.socketPathGetter(vmi, i)
@@ -265,14 +284,9 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co
if err != nil {
return nil, fmt.Errorf("failed to find a sourceFile in containerDisk %v: %v", volume.Name, err)
}
- f, err := os.Create(targetFile)
- if err != nil {
- return nil, fmt.Errorf("failed to create mount point target %v: %v", targetFile, err)
- }
- f.Close()
- log.DefaultLogger().Object(vmi).Infof("Bind mounting container disk at %s to %s", strings.TrimPrefix(sourceFile, nodeRes.MountRoot()), targetFile)
- out, err := virt_chroot.MountChroot(strings.TrimPrefix(sourceFile, nodeRes.MountRoot()), targetFile, true).CombinedOutput()
+ log.DefaultLogger().Object(vmi).Infof("Bind mounting container disk at %s to %s", sourceFile, targetFile)
+ out, err := virt_chroot.MountChroot(sourceFile, targetFile, true).CombinedOutput()
if err != nil {
return nil, fmt.Errorf("failed to bindmount containerDisk %v: %v : %v", volume.Name, string(out), err)
}
@@ -296,40 +310,6 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co
return disksInfo, nil
}
-// Legacy Unmount unmounts all container disks of a given VMI when the hold HostPath method was in use.
-// This exists for backwards compatibility for VMIs running before a KubeVirt update occurs.
-func (m *mounter) legacyUnmount(vmi *v1.VirtualMachineInstance) error {
- mountDir := containerdisk.GetLegacyVolumeMountDirOnHost(vmi)
-
- files, err := os.ReadDir(mountDir)
- if err != nil && !os.IsNotExist(err) {
- return fmt.Errorf("failed to list container disk mounts: %v", err)
- }
-
- if vmi.UID != "" {
- for _, file := range files {
- path := filepath.Join(mountDir, file.Name())
- if strings.HasSuffix(path, ".sock") {
- continue
- }
- if mounted, err := isolation.NodeIsolationResult().IsMounted(path); err != nil {
- return fmt.Errorf(failedCheckMountPointFmt, path, err)
- } else if mounted {
- // #nosec No risk for attacket injection. Parameters are predefined strings
- out, err := virt_chroot.UmountChroot(path).CombinedOutput()
- if err != nil {
- return fmt.Errorf(failedUnmountFmt, path, string(out), err)
- }
- }
- }
-
- if err := os.RemoveAll(mountDir); err != nil {
- return fmt.Errorf("failed to remove containerDisk files: %v", err)
- }
- }
- return nil
-}
-
// Unmount unmounts all container disks of a given VMI.
func (m *mounter) Unmount(vmi *v1.VirtualMachineInstance) error {
if vmi.UID == "" {
@@ -341,13 +321,6 @@ func (m *mounter) Unmount(vmi *v1.VirtualMachineInstance) error {
return fmt.Errorf("error unmounting kernel artifacts: %v", err)
}
- // this will catch unmounting a vmi's container disk when
- // an old VMI is left over after a KubeVirt update
- err = m.legacyUnmount(vmi)
- if err != nil {
- return err
- }
-
record, err := m.getMountTargetRecord(vmi)
if err != nil {
return err
@@ -360,19 +333,22 @@ func (m *mounter) Unmount(vmi *v1.VirtualMachineInstance) error {
log.DefaultLogger().Object(vmi).Infof("Found container disk mount entries")
for _, entry := range record.MountTargetEntries {
- path := entry.TargetFile
- log.DefaultLogger().Object(vmi).Infof("Looking to see if containerdisk is mounted at path %s", path)
- if mounted, err := isolation.NodeIsolationResult().IsMounted(path); err != nil {
+ log.DefaultLogger().Object(vmi).Infof("Looking to see if containerdisk is mounted at path %s", entry.TargetFile)
+ path, err := safepath.NewFileNoFollow(entry.TargetFile)
+ if err != nil {
+ return fmt.Errorf(failedCheckMountPointFmt, path, err)
+ }
+ _ = path.Close()
+ if mounted, err := isolation.IsMounted(path.Path()); err != nil {
return fmt.Errorf(failedCheckMountPointFmt, path, err)
} else if mounted {
log.DefaultLogger().Object(vmi).Infof("unmounting container disk at path %s", path)
// #nosec No risk for attacket injection. Parameters are predefined strings
- out, err := virt_chroot.UmountChroot(path).CombinedOutput()
+ out, err := virt_chroot.UmountChroot(path.Path()).CombinedOutput()
if err != nil {
return fmt.Errorf(failedUnmountFmt, path, string(out), err)
}
}
-
}
err = m.deleteMountTargetRecord(vmi)
if err != nil {
@@ -417,7 +393,19 @@ func (m *mounter) mountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo
if err != nil {
return fmt.Errorf("failed to get disk target dir: %v", err)
}
- targetDir = filepath.Join(targetDir, containerdisk.KernelBootName)
+ if err := safepath.MkdirAtNoFollow(targetDir, containerdisk.KernelBootName, 0755); err != nil {
+ if !os.IsExist(err) {
+ return err
+ }
+ }
+
+ targetDir, err = safepath.JoinNoFollow(targetDir, containerdisk.KernelBootName)
+ if err != nil {
+ return err
+ }
+ if err := safepath.ChpermAtNoFollow(targetDir, 0, 0, 0755); err != nil {
+ return err
+ }
socketFilePath, err := m.kernelBootSocketPathGetter(vmi)
if err != nil {
@@ -426,7 +414,7 @@ func (m *mounter) mountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo
record := vmiMountTargetRecord{
MountTargetEntries: []vmiMountTargetEntry{{
- TargetFile: targetDir,
+ TargetFile: unsafepath.UnsafeAbsolute(targetDir.Raw()),
SocketFile: socketFilePath,
}},
}
@@ -438,11 +426,23 @@ func (m *mounter) mountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo
nodeRes := isolation.NodeIsolationResult()
- targetInitrdPath := filepath.Join(targetDir, filepath.Base(kb.InitrdPath))
- targetKernelPath := filepath.Join(targetDir, filepath.Base(kb.KernelPath))
+ if err := safepath.TouchAtNoFollow(targetDir, filepath.Base(kb.InitrdPath), 0655); err != nil && !os.IsExist(err) {
+ return err
+ }
+ if err := safepath.TouchAtNoFollow(targetDir, filepath.Base(kb.KernelPath), 0655); err != nil && !os.IsExist(err) {
+ return err
+ }
+ targetInitrdPath, err := safepath.JoinNoFollow(targetDir, filepath.Base(kb.InitrdPath))
+ if err != nil {
+ return err
+ }
+ targetKernelPath, err := safepath.JoinNoFollow(targetDir, filepath.Base(kb.KernelPath))
+ if err != nil {
+ return err
+ }
- areKernelArtifactsMounted := func(artifactsDir string, artifactFiles ...string) (bool, error) {
- if _, err = os.Stat(artifactsDir); os.IsNotExist(err) {
+ areKernelArtifactsMounted := func(artifactsDir *safepath.Path, artifactFiles ...*safepath.Path) (bool, error) {
+ if _, err = safepath.StatAtNoFollow(artifactsDir); os.IsNotExist(err) {
return false, nil
} else if err != nil {
return false, err
@@ -455,7 +455,7 @@ func (m *mounter) mountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo
if isMounted, err := areKernelArtifactsMounted(targetDir, targetInitrdPath, targetKernelPath); err != nil {
return fmt.Errorf("failed to determine if %s is already mounted: %v", targetDir, err)
} else if !isMounted {
- log.Log.Object(vmi).Infof("mounting kernel artifacts are not mounted - mounting...")
+ log.Log.Object(vmi).Infof("kernel artifacts are not mounted - mounting...")
res, err := m.podIsolationDetector.DetectForSocket(vmi, socketFilePath)
if err != nil {
@@ -466,27 +466,13 @@ func (m *mounter) mountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo
return fmt.Errorf("failed to detect root mount point of %v on the node: %v", kernelBootName, err)
}
- err = os.Mkdir(targetDir, 0755)
- if err != nil {
- return fmt.Errorf("failed to create mount point target %v: %v", targetDir, err)
- }
-
- mount := func(artifactPath, targetPath string) error {
- if artifactPath == "" {
- return nil
- }
+ mount := func(artifactPath string, targetPath *safepath.Path) error {
sourcePath, err := containerdisk.GetImage(mountRootPath, artifactPath)
if err != nil {
return err
}
- file, err := os.Create(targetPath)
- if err != nil {
- return err
- }
- file.Close()
-
out, err := virt_chroot.MountChroot(sourcePath, targetPath, true).CombinedOutput()
if err != nil {
return fmt.Errorf("failed to bindmount %v: %v : %v", kernelBootName, string(out), err)
@@ -532,14 +518,17 @@ func (m *mounter) unmountKernelArtifacts(vmi *v1.VirtualMachineInstance) error {
return nil
}
- unmount := func(targetDir string, artifactPaths ...string) error {
+ unmount := func(targetDir *safepath.Path, artifactPaths ...string) error {
for _, artifactPath := range artifactPaths {
if artifactPath == "" {
continue
}
- targetPath := filepath.Join(targetDir, filepath.Base(artifactPath))
- if mounted, err := isolation.NodeIsolationResult().IsMounted(targetPath); err != nil {
+ targetPath, err := safepath.JoinNoFollow(targetDir, filepath.Base(artifactPath))
+ if err != nil {
+ return fmt.Errorf(failedCheckMountPointFmt, targetPath, err)
+ }
+ if mounted, err := isolation.IsMounted(targetPath); err != nil {
return fmt.Errorf(failedCheckMountPointFmt, targetPath, err)
} else if mounted {
log.DefaultLogger().Object(vmi).Infof("unmounting container disk at targetDir %s", targetPath)
@@ -554,23 +543,22 @@ func (m *mounter) unmountKernelArtifacts(vmi *v1.VirtualMachineInstance) error {
}
for idx, entry := range record.MountTargetEntries {
- targetDir := entry.TargetFile
- if !strings.Contains(targetDir, containerdisk.KernelBootName) {
+ if !strings.Contains(entry.TargetFile, containerdisk.KernelBootName) {
continue
}
- log.DefaultLogger().Object(vmi).Infof("unmounting kernel artifacts in path: %s", targetDir)
+ targetDir, err := safepath.NewFileNoFollow(entry.TargetFile)
+ if err != nil {
+ return fmt.Errorf("failed to obtaining a reference to the target directory %q: %v", targetDir, err)
+ }
+ _ = targetDir.Close()
+ log.DefaultLogger().Object(vmi).Infof("unmounting kernel artifacts in path: %v", targetDir)
- if err = unmount(targetDir, kb.InitrdPath, kb.KernelPath); err != nil {
+ if err = unmount(targetDir.Path(), kb.InitrdPath, kb.KernelPath); err != nil {
// Not returning here since even if unmount wasn't successful it's better to keep
// cleaning the mounted files.
log.Log.Object(vmi).Reason(err).Error("unable to unmount kernel artifacts")
}
- err = os.RemoveAll(targetDir)
- if err != nil {
- log.DefaultLogger().Object(vmi).Infof("cannot delete dir %s. err: %v", targetDir, err)
- }
-
removeSliceElement := func(s []vmiMountTargetEntry, idxToRemove int) []vmiMountTargetEntry {
// removes slice element efficiently
s[idxToRemove] = s[len(s)-1]
--
2.37.1
From f359a4c60a258dd32e651ea4c6e1cb256d268e58 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:11:45 +0200
Subject: [PATCH 08/20] Only accept absolute paths for containerdisks
Let virt-api reject relative paths on containerDisk, kernel or initrd
path fields. A relative path would not cause any harm due to the
safepath usage, but it is not clear what it means either.
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit b465bffbf7ea86cbc15dad68118eea0721c6af87)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/testutils/mock_config.go | 2 +-
.../admitters/vmi-create-admitter.go | 52 +++++++++++++++++--
.../admitters/vmi-create-admitter_test.go | 46 +++++++++++++++-
3 files changed, 94 insertions(+), 6 deletions(-)
diff --git a/pkg/testutils/mock_config.go b/pkg/testutils/mock_config.go
index 9a640c314..f5c2cdd79 100644
--- a/pkg/testutils/mock_config.go
+++ b/pkg/testutils/mock_config.go
@@ -55,7 +55,7 @@ func NewFakeContainerDiskSource() *KVv1.ContainerDiskSource {
return &KVv1.ContainerDiskSource{
Image: "fake-image",
ImagePullSecret: "fake-pull-secret",
- Path: "fake-path",
+ Path: "/fake-path",
}
}
diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
index e0381d8cb..e0463101d 100644
--- a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
+++ b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
@@ -23,6 +23,7 @@ import (
"encoding/base64"
"fmt"
"net"
+ "path/filepath"
"regexp"
"strings"
@@ -194,6 +195,7 @@ func ValidateVirtualMachineInstanceSpec(field *k8sfield.Path, spec *v1.VirtualMa
causes = append(causes, validateDomainSpec(field.Child("domain"), &spec.Domain)...)
causes = append(causes, validateVolumes(field.Child("volumes"), spec.Volumes, config)...)
+ causes = append(causes, validateContainerDisks(field, spec)...)
causes = append(causes, validateAccessCredentials(field.Child("accessCredentials"), spec.AccessCredentials, spec.Volumes)...)
@@ -1462,6 +1464,43 @@ func validateRealtime(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec)
return causes
}
+func validateContainerDisks(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) (causes []metav1.StatusCause) {
+ for idx, volume := range spec.Volumes {
+ if volume.ContainerDisk == nil || volume.ContainerDisk.Path == "" {
+ continue
+ }
+ causes = append(causes, validatePath(field.Child("volumes").Index(idx).Child("conatinerDisk"), volume.ContainerDisk.Path)...)
+ }
+ return causes
+}
+
+func validatePath(field *k8sfield.Path, path string) (causes []metav1.StatusCause) {
+ if path == "/" {
+ causes = append(causes, metav1.StatusCause{
+ Type: metav1.CauseTypeFieldValueInvalid,
+ Message: fmt.Sprintf("%s must not point to root",
+ field.String(),
+ ),
+ Field: field.String(),
+ })
+ } else {
+ cleanedPath := filepath.Join("/", path)
+ providedPath := strings.TrimSuffix(path, "/") // Join trims suffix slashes
+
+ if cleanedPath != providedPath {
+ causes = append(causes, metav1.StatusCause{
+ Type: metav1.CauseTypeFieldValueInvalid,
+ Message: fmt.Sprintf("%s must be an absolute path to a file without relative components",
+ field.String(),
+ ),
+ Field: field.String(),
+ })
+ }
+ }
+ return causes
+
+}
+
func validateCPURealtime(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) (causes []metav1.StatusCause) {
if !spec.Domain.CPU.DedicatedCPUPlacement {
causes = append(causes, metav1.StatusCause{
@@ -2410,13 +2449,13 @@ func validateKernelBoot(field *k8sfield.Path, kernelBoot *v1.KernelBoot) (causes
}
container := kernelBoot.Container
- containerField := field.Child("container").String()
+ containerField := field.Child("container")
if container.Image == "" {
causes = append(causes, metav1.StatusCause{
Type: metav1.CauseTypeFieldValueRequired,
Message: fmt.Sprintf("%s must be defined with an image", containerField),
- Field: containerField,
+ Field: containerField.Child("image").String(),
})
}
@@ -2424,10 +2463,17 @@ func validateKernelBoot(field *k8sfield.Path, kernelBoot *v1.KernelBoot) (causes
causes = append(causes, metav1.StatusCause{
Type: metav1.CauseTypeFieldValueRequired,
Message: fmt.Sprintf("%s must be defined with at least one of the following: kernelPath, initrdPath", containerField),
- Field: containerField,
+ Field: containerField.String(),
})
}
+ if container.KernelPath != "" {
+ causes = append(causes, validatePath(containerField.Child("kernelPath"), container.KernelPath)...)
+ }
+ if container.InitrdPath != "" {
+ causes = append(causes, validatePath(containerField.Child("initrdPath"), container.InitrdPath)...)
+ }
+
return
}
diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
index 8f30589bd..889403ddd 100644
--- a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
+++ b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
@@ -135,6 +135,21 @@ var _ = Describe("Validating VMICreate Admitter", func() {
Expect(resp.Result.Message).To(ContainSubstring("no memory requested"))
})
+ DescribeTable("path validation should fail", func(path string) {
+ Expect(validatePath(k8sfield.NewPath("fake"), path)).To(HaveLen(1))
+ },
+ Entry("if path is not absolute", "a/b/c"),
+ Entry("if path contains relative elements", "/a/b/c/../d"),
+ Entry("if path is root", "/"),
+ )
+
+ DescribeTable("path validation should succeed", func(path string) {
+ Expect(validatePath(k8sfield.NewPath("fake"), path)).To(BeEmpty())
+ },
+ Entry("if path is absolute", "/a/b/c"),
+ Entry("if path is absolute and has trailing slash", "/a/b/c/"),
+ )
+
Context("tolerations with eviction policies given", func() {
var vmi *v1.VirtualMachineInstance
var policyMigrate = v1.EvictionStrategyLiveMigrate
@@ -2249,10 +2264,12 @@ var _ = Describe("Validating VMICreate Admitter", func() {
validImage = "image"
withoutImage = ""
- validInitrd = "initrd"
+ invalidInitrd = "initrd"
+ validInitrd = "/initrd"
withoutInitrd = ""
- validKernel = "kernel"
+ invalidKernel = "kernel"
+ validKernel = "/kernel"
withoutKernel = ""
)
@@ -2280,10 +2297,35 @@ var _ = Describe("Validating VMICreate Admitter", func() {
createKernelBoot(validKernelArgs, validInitrd, withoutKernel, validImage), true),
Entry("with kernel args, with container that has only image defined - should reject",
createKernelBoot(validKernelArgs, withoutInitrd, withoutKernel, validImage), false),
+ Entry("with invalid kernel path - should reject",
+ createKernelBoot(validKernelArgs, validInitrd, invalidKernel, validImage), false),
+ Entry("with invalid initrd path - should reject",
+ createKernelBoot(validKernelArgs, invalidInitrd, validKernel, validImage), false),
Entry("with kernel args, with container that has initrd and kernel defined but without image - should reject",
createKernelBoot(validKernelArgs, validInitrd, validKernel, withoutImage), false),
)
})
+
+ It("should detect invalid containerDisk paths", func() {
+ spec := &v1.VirtualMachineInstanceSpec{}
+ disk := v1.Disk{
+ Name: "testdisk",
+ Serial: "SN-1_a",
+ }
+ spec.Domain.Devices.Disks = []v1.Disk{disk}
+ volume := v1.Volume{
+ Name: "testdisk",
+ VolumeSource: v1.VolumeSource{
+ ContainerDisk: testutils.NewFakeContainerDiskSource(),
+ },
+ }
+ volume.ContainerDisk.Path = "invalid"
+
+ spec.Volumes = []v1.Volume{volume}
+ spec.Domain.Devices.Disks = []v1.Disk{disk}
+ causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), spec, config)
+ Expect(causes).To(HaveLen(1))
+ })
})
Context("with cpu pinning", func() {
--
2.37.1
From e627a2401c3c161fe76091e433e8e1ab1fbe2588 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:15:15 +0200
Subject: [PATCH 09/20] Use UnsafeSetFileOwnership on virt-launcher code
When code is executed inside virt-launcher, no special path
considerations are necessary. This can be refactored later to remove the
`Unsafe` trigger from these code paths.
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit d7e01de4a3e3abc11a0e525f0d1b2eecdeb7138c)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/cloud-init/cloud-init.go | 4 ++--
pkg/config/config-map.go | 2 +-
pkg/config/downwardapi.go | 2 +-
pkg/config/secret.go | 2 +-
pkg/config/service-account.go | 2 +-
pkg/config/sysprep.go | 2 +-
pkg/emptydisk/emptydisk.go | 2 +-
pkg/network/cache/cache.go | 2 +-
pkg/virt-handler/migration-proxy/migration-proxy.go | 2 +-
pkg/virt-launcher/monitor.go | 4 ++--
pkg/virt-launcher/virtwrap/live-migration-target.go | 2 +-
11 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/pkg/cloud-init/cloud-init.go b/pkg/cloud-init/cloud-init.go
index 2811cecb4..f07aaa94d 100644
--- a/pkg/cloud-init/cloud-init.go
+++ b/pkg/cloud-init/cloud-init.go
@@ -524,7 +524,7 @@ func GenerateEmptyIso(vmiName string, namespace string, data *CloudInitData, siz
return err
}
- if err := diskutils.DefaultOwnershipManager.SetFileOwnership(isoStaging); err != nil {
+ if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(isoStaging); err != nil {
return err
}
err = os.Rename(isoStaging, iso)
@@ -642,7 +642,7 @@ func GenerateLocalData(vmiName string, namespace string, instanceType string, da
return err
}
- if err := diskutils.DefaultOwnershipManager.SetFileOwnership(isoStaging); err != nil {
+ if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(isoStaging); err != nil {
return err
}
diff --git a/pkg/config/config-map.go b/pkg/config/config-map.go
index cd5f7a181..9ed328a71 100644
--- a/pkg/config/config-map.go
+++ b/pkg/config/config-map.go
@@ -55,7 +55,7 @@ func CreateConfigMapDisks(vmi *v1.VirtualMachineInstance, emptyIso bool) error {
return err
}
- if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil {
+ if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil {
return err
}
}
diff --git a/pkg/config/downwardapi.go b/pkg/config/downwardapi.go
index 8b921cf8d..00e9d60d9 100644
--- a/pkg/config/downwardapi.go
+++ b/pkg/config/downwardapi.go
@@ -56,7 +56,7 @@ func CreateDownwardAPIDisks(vmi *v1.VirtualMachineInstance, emptyIso bool) error
return err
}
- if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil {
+ if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil {
return err
}
}
diff --git a/pkg/config/secret.go b/pkg/config/secret.go
index 5061c6942..f3f238110 100644
--- a/pkg/config/secret.go
+++ b/pkg/config/secret.go
@@ -56,7 +56,7 @@ func CreateSecretDisks(vmi *v1.VirtualMachineInstance, emptyIso bool) error {
return err
}
- if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil {
+ if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil {
return err
}
}
diff --git a/pkg/config/service-account.go b/pkg/config/service-account.go
index 46506abeb..26e2dbb3a 100644
--- a/pkg/config/service-account.go
+++ b/pkg/config/service-account.go
@@ -50,7 +50,7 @@ func CreateServiceAccountDisk(vmi *v1.VirtualMachineInstance, emptyIso bool) err
return err
}
- if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil {
+ if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil {
return err
}
}
diff --git a/pkg/config/sysprep.go b/pkg/config/sysprep.go
index fdf3402e6..d77415007 100644
--- a/pkg/config/sysprep.go
+++ b/pkg/config/sysprep.go
@@ -102,7 +102,7 @@ func createIsoImageAndSetFileOwnership(volumeName string, filesPath []string, si
if err := createIsoConfigImage(disk, sysprepVolumeLabel, filesPath, size); err != nil {
return err
}
- if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil {
+ if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil {
return err
}
diff --git a/pkg/emptydisk/emptydisk.go b/pkg/emptydisk/emptydisk.go
index ad72a7004..4389a43d5 100644
--- a/pkg/emptydisk/emptydisk.go
+++ b/pkg/emptydisk/emptydisk.go
@@ -45,7 +45,7 @@ func (c *emptyDiskCreator) CreateTemporaryDisks(vmi *v1.VirtualMachineInstance)
} else if err != nil {
return err
}
- if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(file); err != nil {
+ if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(file); err != nil {
return err
}
}
diff --git a/pkg/network/cache/cache.go b/pkg/network/cache/cache.go
index cc0e40652..734a3249c 100644
--- a/pkg/network/cache/cache.go
+++ b/pkg/network/cache/cache.go
@@ -100,7 +100,7 @@ func writeToCachedFile(fs cacheFS, obj interface{}, fileName string) error {
if err != nil {
return fmt.Errorf("error writing cached object: %v", err)
}
- return dutils.DefaultOwnershipManager.SetFileOwnership(fileName)
+ return dutils.DefaultOwnershipManager.UnsafeSetFileOwnership(fileName)
}
func readFromCachedFile(fs cacheFS, obj interface{}, fileName string) error {
diff --git a/pkg/virt-handler/migration-proxy/migration-proxy.go b/pkg/virt-handler/migration-proxy/migration-proxy.go
index 557c65f14..0a0112a7c 100644
--- a/pkg/virt-handler/migration-proxy/migration-proxy.go
+++ b/pkg/virt-handler/migration-proxy/migration-proxy.go
@@ -416,7 +416,7 @@ func (m *migrationProxy) createUnixListener() error {
m.logger.Reason(err).Error("failed to create unix socket for proxy service")
return err
}
- if err := diskutils.DefaultOwnershipManager.SetFileOwnership(m.unixSocketPath); err != nil {
+ if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(m.unixSocketPath); err != nil {
log.Log.Reason(err).Error("failed to change ownership on migration unix socket")
return err
}
diff --git a/pkg/virt-launcher/monitor.go b/pkg/virt-launcher/monitor.go
index 9524144b2..b70b33bff 100644
--- a/pkg/virt-launcher/monitor.go
+++ b/pkg/virt-launcher/monitor.go
@@ -57,7 +57,7 @@ func InitializePrivateDirectories(baseDir string) error {
if err := util.MkdirAllWithNosec(baseDir); err != nil {
return err
}
- if err := diskutils.DefaultOwnershipManager.SetFileOwnership(baseDir); err != nil {
+ if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(baseDir); err != nil {
return err
}
return nil
@@ -74,7 +74,7 @@ func InitializeDisksDirectories(baseDir string) error {
if err != nil {
return err
}
- err = diskutils.DefaultOwnershipManager.SetFileOwnership(baseDir)
+ err = diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(baseDir)
if err != nil {
return err
}
diff --git a/pkg/virt-launcher/virtwrap/live-migration-target.go b/pkg/virt-launcher/virtwrap/live-migration-target.go
index 4e4d107e8..b787a768d 100644
--- a/pkg/virt-launcher/virtwrap/live-migration-target.go
+++ b/pkg/virt-launcher/virtwrap/live-migration-target.go
@@ -105,7 +105,7 @@ func (l *LibvirtDomainManager) prepareMigrationTarget(
logger.Reason(err).Error("failed to create the migration sockets directory")
return err
}
- if err := diskutils.DefaultOwnershipManager.SetFileOwnership(migrationSocketsPath); err != nil {
+ if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(migrationSocketsPath); err != nil {
logger.Reason(err).Error("failed to change ownership on migration sockets directory")
return err
}
--
2.37.1
From 2ea069fe179b3bafa55fba921f42a30cb48014bf Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:22:33 +0200
Subject: [PATCH 10/20] Don't change selinux labels on hostDisk mounts
When virt-launcher runs in non-root mode, don't change selinux labels on
the target disk to be accessible for the user. This task should be
performed as a preparation step by admins on selinux enabled nodes which
"allow" this path to be accessed by VMIs.
This way we avoid working against the selinux security intent.
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 61d6dbe24d2808fa5c55ea0d83096c36729d91f9)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/virt-handler/BUILD.bazel | 1 +
pkg/virt-handler/non-root.go | 85 ++++++++++++++++++++++++------------
2 files changed, 58 insertions(+), 28 deletions(-)
diff --git a/pkg/virt-handler/BUILD.bazel b/pkg/virt-handler/BUILD.bazel
index a9923c4ef..0a0aefa3b 100644
--- a/pkg/virt-handler/BUILD.bazel
+++ b/pkg/virt-handler/BUILD.bazel
@@ -24,6 +24,7 @@ go_library(
"//pkg/network/errors:go_default_library",
"//pkg/network/setup:go_default_library",
"//pkg/network/vmispec:go_default_library",
+ "//pkg/safepath:go_default_library",
"//pkg/util:go_default_library",
"//pkg/util/hardware:go_default_library",
"//pkg/util/migrations:go_default_library",
diff --git a/pkg/virt-handler/non-root.go b/pkg/virt-handler/non-root.go
index 9795bc1c6..875518c82 100644
--- a/pkg/virt-handler/non-root.go
+++ b/pkg/virt-handler/non-root.go
@@ -13,9 +13,9 @@ import (
v1 "kubevirt.io/api/core/v1"
diskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils"
hostdisk "kubevirt.io/kubevirt/pkg/host-disk"
+ "kubevirt.io/kubevirt/pkg/safepath"
"kubevirt.io/kubevirt/pkg/util/types"
"kubevirt.io/kubevirt/pkg/virt-handler/isolation"
- "kubevirt.io/kubevirt/pkg/virt-handler/selinux"
)
func changeOwnershipOfBlockDevices(vmi *v1.VirtualMachineInstance, res isolation.IsolationResult) error {
@@ -40,9 +40,11 @@ func changeOwnershipOfBlockDevices(vmi *v1.VirtualMachineInstance, res isolation
if !types.IsPVCBlock(volumeMode) {
continue
}
-
- devPath := filepath.Join(string(filepath.Separator), "dev", volume.Name)
- if err := diskutils.DefaultOwnershipManager.SetFileOwnership(filepath.Join(res.MountRoot(), devPath)); err != nil {
+ devPath, err := isolation.SafeJoin(res, string(filepath.Separator), "dev", vmi.Spec.Volumes[i].Name)
+ if err != nil {
+ return nil
+ }
+ if err := diskutils.DefaultOwnershipManager.SetFileOwnership(devPath); err != nil {
return err
}
@@ -50,22 +52,12 @@ func changeOwnershipOfBlockDevices(vmi *v1.VirtualMachineInstance, res isolation
return nil
}
-func changeOwnershipAndRelabel(path string) error {
+func changeOwnership(path *safepath.Path) error {
err := diskutils.DefaultOwnershipManager.SetFileOwnership(path)
if err != nil {
return err
}
-
- seLinux, selinuxEnabled, err := selinux.NewSELinux()
- if err == nil && selinuxEnabled {
- unprivilegedContainerSELinuxLabel := "system_u:object_r:container_file_t:s0"
- err = selinux.RelabelFiles(unprivilegedContainerSELinuxLabel, seLinux.IsPermissive(), filepath.Join(path))
- if err != nil {
- return (fmt.Errorf("error relabeling %s: %v", path, err))
- }
-
- }
- return err
+ return nil
}
// changeOwnershipOfHostDisks needs unmodified vmi (not passed to ReplacePVCByHostDisk function)
@@ -79,7 +71,11 @@ func changeOwnershipOfHostDisks(vmiWithAllPVCs *v1.VirtualMachineInstance, res i
if err != nil {
if os.IsNotExist(err) {
diskDir := hostdisk.GetMountedHostDiskDir(volumeName)
- if err := changeOwnershipAndRelabel(filepath.Join(res.MountRoot(), diskDir)); err != nil {
+ path, err := isolation.SafeJoin(res, diskDir)
+ if err != nil {
+ return fmt.Errorf("Failed to change ownership of HostDisk dir %s, %s", volumeName, err)
+ }
+ if err := changeOwnership(path); err != nil {
return fmt.Errorf("Failed to change ownership of HostDisk dir %s, %s", volumeName, err)
}
continue
@@ -87,7 +83,11 @@ func changeOwnershipOfHostDisks(vmiWithAllPVCs *v1.VirtualMachineInstance, res i
return fmt.Errorf("Failed to recognize if hostdisk contains image, %s", err)
}
- err = changeOwnershipAndRelabel(filepath.Join(res.MountRoot(), diskPath))
+ path, err := isolation.SafeJoin(res, diskPath)
+ if err != nil {
+ return fmt.Errorf("Failed to change ownership of HostDisk image: %s", err)
+ }
+ err = changeOwnership(path)
if err != nil {
return fmt.Errorf("Failed to change ownership of HostDisk image: %s", err)
}
@@ -125,18 +125,31 @@ func getTapDevices(vmi *v1.VirtualMachineInstance) []string {
func (d *VirtualMachineController) prepareTap(vmi *v1.VirtualMachineInstance, res isolation.IsolationResult) error {
tapDevices := getTapDevices(vmi)
for _, tap := range tapDevices {
- path := filepath.Join(res.MountRoot(), "sys", "class", "net", tap, "ifindex")
- b, err := ioutil.ReadFile(path)
+ path, err := isolation.SafeJoin(res, "sys", "class", "net", tap, "ifindex")
if err != nil {
- return fmt.Errorf("Failed to read if index, %v", err)
+ return err
}
+ index, err := func(path *safepath.Path) (int, error) {
+ df, err := safepath.OpenAtNoFollow(path)
+ if err != nil {
+ return 0, err
+ }
+ defer df.Close()
+ b, err := ioutil.ReadFile(df.SafePath())
+ if err != nil {
+ return 0, fmt.Errorf("Failed to read if index, %v", err)
+ }
- index, err := strconv.Atoi(strings.TrimSpace(string(b)))
+ return strconv.Atoi(strings.TrimSpace(string(b)))
+ }(path)
if err != nil {
return err
}
- pathToTap := filepath.Join(res.MountRoot(), "dev", fmt.Sprintf("tap%d", index))
+ pathToTap, err := isolation.SafeJoin(res, "dev", fmt.Sprintf("tap%d", index))
+ if err != nil {
+ return err
+ }
if err := diskutils.DefaultOwnershipManager.SetFileOwnership(pathToTap); err != nil {
return err
@@ -147,25 +160,41 @@ func (d *VirtualMachineController) prepareTap(vmi *v1.VirtualMachineInstance, re
}
func (*VirtualMachineController) prepareVFIO(vmi *v1.VirtualMachineInstance, res isolation.IsolationResult) error {
- vfioPath := filepath.Join(res.MountRoot(), "dev", "vfio")
- err := os.Chmod(filepath.Join(vfioPath, "vfio"), 0666)
+ vfioBasePath, err := isolation.SafeJoin(res, "dev", "vfio")
if err != nil {
if os.IsNotExist(err) {
return nil
}
+ }
+ vfioPath, err := safepath.JoinNoFollow(vfioBasePath, "vfio")
+ if err != nil {
+ if os.IsNotExist(err) {
+ return nil
+ }
+ }
+ err = safepath.ChmodAtNoFollow(vfioPath, 0666)
+ if err != nil {
return err
}
- groups, err := ioutil.ReadDir(vfioPath)
+ var files []os.DirEntry
+ err = vfioBasePath.ExecuteNoFollow(func(safePath string) (err error) {
+ files, err = os.ReadDir(safePath)
+ return err
+ })
if err != nil {
return err
}
- for _, group := range groups {
+ for _, group := range files {
if group.Name() == "vfio" {
continue
}
- if err := diskutils.DefaultOwnershipManager.SetFileOwnership(filepath.Join(vfioPath, group.Name())); err != nil {
+ groupPath, err := safepath.JoinNoFollow(vfioBasePath, group.Name())
+ if err != nil {
+ return err
+ }
+ if err := diskutils.DefaultOwnershipManager.SetFileOwnership(groupPath); err != nil {
return err
}
}
--
2.37.1
From 93d056cd2c4c50c9fb60c60bdaedfcd0d07f680c Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:25:11 +0200
Subject: [PATCH 11/20] Let host-disk continue using unsafe path manipulations
host-disk has not further restrictions on what to access. Users can
already freely decide what path they want to access.
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 0e28639da59d97c9cfcaf28843fe48738f959362)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/host-disk/BUILD.bazel | 3 +++
pkg/host-disk/host-disk.go | 12 +++++++-----
pkg/host-disk/host-disk_test.go | 9 +++++++--
3 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/pkg/host-disk/BUILD.bazel b/pkg/host-disk/BUILD.bazel
index f373f585f..4f662417d 100644
--- a/pkg/host-disk/BUILD.bazel
+++ b/pkg/host-disk/BUILD.bazel
@@ -7,6 +7,8 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/ephemeral-disk-utils:go_default_library",
+ "//pkg/safepath:go_default_library",
+ "//pkg/unsafepath:go_default_library",
"//pkg/util:go_default_library",
"//pkg/util/types:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
@@ -25,6 +27,7 @@ go_test(
embed = [":go_default_library"],
deps = [
"//pkg/ephemeral-disk-utils:go_default_library",
+ "//pkg/safepath:go_default_library",
"//pkg/testutils:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
"//staging/src/kubevirt.io/client-go/api:go_default_library",
diff --git a/pkg/host-disk/host-disk.go b/pkg/host-disk/host-disk.go
index 33d13f828..32db85c2a 100644
--- a/pkg/host-disk/host-disk.go
+++ b/pkg/host-disk/host-disk.go
@@ -28,6 +28,8 @@ import (
"kubevirt.io/client-go/log"
ephemeraldiskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils"
+ "kubevirt.io/kubevirt/pkg/safepath"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
k8sv1 "k8s.io/api/core/v1"
"k8s.io/client-go/tools/record"
@@ -192,10 +194,10 @@ type DiskImgCreator struct {
recorder record.EventRecorder
lessPVCSpaceToleration int
minimumPVCReserveBytes uint64
- mountRoot string
+ mountRoot *safepath.Path
}
-func NewHostDiskCreator(recorder record.EventRecorder, lessPVCSpaceToleration int, minimumPVCReserveBytes uint64, mountRoot string) DiskImgCreator {
+func NewHostDiskCreator(recorder record.EventRecorder, lessPVCSpaceToleration int, minimumPVCReserveBytes uint64, mountRoot *safepath.Path) DiskImgCreator {
return DiskImgCreator{
dirBytesAvailableFunc: dirBytesAvailable,
recorder: recorder,
@@ -225,8 +227,8 @@ func shouldMountHostDisk(hostDisk *v1.HostDisk) bool {
}
func (hdc *DiskImgCreator) mountHostDiskAndSetOwnership(vmi *v1.VirtualMachineInstance, volumeName string, hostDisk *v1.HostDisk) error {
- diskPath := GetMountedHostDiskPathFromHandler(hdc.mountRoot, volumeName, hostDisk.Path)
- diskDir := GetMountedHostDiskDirFromHandler(hdc.mountRoot, volumeName)
+ diskPath := GetMountedHostDiskPathFromHandler(unsafepath.UnsafeAbsolute(hdc.mountRoot.Raw()), volumeName, hostDisk.Path)
+ diskDir := GetMountedHostDiskDirFromHandler(unsafepath.UnsafeAbsolute(hdc.mountRoot.Raw()), volumeName)
fileExists, err := ephemeraldiskutils.FileExists(diskPath)
if err != nil {
return err
@@ -237,7 +239,7 @@ func (hdc *DiskImgCreator) mountHostDiskAndSetOwnership(vmi *v1.VirtualMachineIn
}
}
// Change file ownership to the qemu user.
- if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(diskPath); err != nil {
+ if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(diskPath); err != nil {
log.Log.Reason(err).Errorf("Couldn't set Ownership on %s: %v", diskPath, err)
return err
}
diff --git a/pkg/host-disk/host-disk_test.go b/pkg/host-disk/host-disk_test.go
index 280bf54a7..92282ebdd 100644
--- a/pkg/host-disk/host-disk_test.go
+++ b/pkg/host-disk/host-disk_test.go
@@ -35,6 +35,8 @@ import (
"k8s.io/client-go/kubernetes/fake"
"k8s.io/client-go/tools/record"
+ "kubevirt.io/kubevirt/pkg/safepath"
+
"kubevirt.io/client-go/api"
v1 "kubevirt.io/api/core/v1"
@@ -98,8 +100,11 @@ var _ = Describe("HostDisk", func() {
recorder = record.NewFakeRecorder(100)
recorder.IncludeObject = true
- hostDiskCreator = NewHostDiskCreator(recorder, 0, 0, "")
- hostDiskCreatorWithReserve = NewHostDiskCreator(recorder, 10, 1048576, "")
+ root, err := safepath.JoinAndResolveWithRelativeRoot("/")
+ Expect(err).NotTo(HaveOccurred())
+
+ hostDiskCreator = NewHostDiskCreator(recorder, 0, 0, root)
+ hostDiskCreatorWithReserve = NewHostDiskCreator(recorder, 10, 1048576, root)
})
--
2.37.1
From ad525698f37f8a45a0d517d745c0569a2d8bf753 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:28:46 +0200
Subject: [PATCH 12/20] Move hotplug code over to safepath usage
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 59d54ee7f23d20ce17e6ec08b47458e78715811b)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/hotplug-disk/BUILD.bazel | 3 +-
pkg/hotplug-disk/hotplug-disk.go | 53 +-
pkg/hotplug-disk/hotplug-disk_test.go | 9 +-
pkg/virt-handler/hotplug-disk/BUILD.bazel | 8 +-
.../hotplug-disk/hotplug-disk_suite_test.go | 2 +-
pkg/virt-handler/hotplug-disk/mount.go | 437 ++++++++------
pkg/virt-handler/hotplug-disk/mount_test.go | 555 ++++++++++--------
7 files changed, 591 insertions(+), 476 deletions(-)
diff --git a/pkg/hotplug-disk/BUILD.bazel b/pkg/hotplug-disk/BUILD.bazel
index 1a91a3ced..af49b5ab2 100644
--- a/pkg/hotplug-disk/BUILD.bazel
+++ b/pkg/hotplug-disk/BUILD.bazel
@@ -6,7 +6,7 @@ go_library(
importpath = "kubevirt.io/kubevirt/pkg/hotplug-disk",
visibility = ["//visibility:public"],
deps = [
- "//pkg/ephemeral-disk-utils:go_default_library",
+ "//pkg/safepath:go_default_library",
"//pkg/util:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
],
@@ -21,6 +21,7 @@ go_test(
embed = [":go_default_library"],
deps = [
"//pkg/ephemeral-disk-utils:go_default_library",
+ "//pkg/unsafepath:go_default_library",
"//staging/src/kubevirt.io/client-go/testutils:go_default_library",
"//vendor/github.com/onsi/ginkgo/v2:go_default_library",
"//vendor/github.com/onsi/gomega:go_default_library",
diff --git a/pkg/hotplug-disk/hotplug-disk.go b/pkg/hotplug-disk/hotplug-disk.go
index 4e0749b70..99a944bf0 100644
--- a/pkg/hotplug-disk/hotplug-disk.go
+++ b/pkg/hotplug-disk/hotplug-disk.go
@@ -26,7 +26,8 @@ import (
"k8s.io/apimachinery/pkg/types"
- diskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils"
+ "kubevirt.io/kubevirt/pkg/safepath"
+
"kubevirt.io/kubevirt/pkg/util"
)
@@ -44,9 +45,9 @@ var (
)
type HotplugDiskManagerInterface interface {
- GetHotplugTargetPodPathOnHost(virtlauncherPodUID types.UID) (string, error)
- GetFileSystemDiskTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (string, error)
- GetFileSystemDirectoryTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (string, error)
+ GetHotplugTargetPodPathOnHost(virtlauncherPodUID types.UID) (*safepath.Path, error)
+ GetFileSystemDiskTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (*safepath.Path, error)
+ GetFileSystemDirectoryTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (*safepath.Path, error)
}
func NewHotplugDiskManager() *hotplugDiskManager {
@@ -69,51 +70,39 @@ type hotplugDiskManager struct {
}
// GetHotplugTargetPodPathOnHost retrieves the target pod (virt-launcher) path on the host.
-func (h *hotplugDiskManager) GetHotplugTargetPodPathOnHost(virtlauncherPodUID types.UID) (string, error) {
+func (h *hotplugDiskManager) GetHotplugTargetPodPathOnHost(virtlauncherPodUID types.UID) (*safepath.Path, error) {
podpath := TargetPodBasePath(h.podsBaseDir, virtlauncherPodUID)
- exists, _ := diskutils.FileExists(podpath)
- if exists {
- return podpath, nil
- }
-
- return "", fmt.Errorf("Unable to locate target path: %s", podpath)
+ return safepath.JoinAndResolveWithRelativeRoot("/", podpath)
}
// GetFileSystemDirectoryTargetPathFromHostView gets the directory path in the target pod (virt-launcher) on the host.
-func (h *hotplugDiskManager) GetFileSystemDirectoryTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (string, error) {
+func (h *hotplugDiskManager) GetFileSystemDirectoryTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (*safepath.Path, error) {
targetPath, err := h.GetHotplugTargetPodPathOnHost(virtlauncherPodUID)
if err != nil {
- return "", err
+ return nil, err
}
- directoryPath := filepath.Join(targetPath, volumeName)
- exists, err := diskutils.FileExists(directoryPath)
- if err != nil {
- return "", err
- }
- if !exists && create {
- if err := os.Mkdir(directoryPath, 0750); err != nil {
- return "", err
+ _, err = safepath.JoinNoFollow(targetPath, volumeName)
+ if os.IsNotExist(err) && create {
+ if err := safepath.MkdirAtNoFollow(targetPath, volumeName, 0750); err != nil {
+ return nil, err
}
+ } else if err != nil {
+ return nil, err
}
- return directoryPath, nil
+ return safepath.JoinNoFollow(targetPath, volumeName)
}
// GetFileSystemDiskTargetPathFromHostView gets the disk image file in the target pod (virt-launcher) on the host.
-func (h *hotplugDiskManager) GetFileSystemDiskTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (string, error) {
+func (h *hotplugDiskManager) GetFileSystemDiskTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (*safepath.Path, error) {
targetPath, err := h.GetHotplugTargetPodPathOnHost(virtlauncherPodUID)
if err != nil {
return targetPath, err
}
- diskFile := filepath.Join(targetPath, fmt.Sprintf("%s.img", volumeName))
- exists, _ := diskutils.FileExists(diskFile)
- if !exists && create {
- file, err := os.Create(diskFile)
- if err != nil {
- return diskFile, err
- }
- defer file.Close()
+ diskName := fmt.Sprintf("%s.img", volumeName)
+ if err := safepath.TouchAtNoFollow(targetPath, diskName, 0666); err != nil && !os.IsExist(err) {
+ return nil, err
}
- return diskFile, err
+ return safepath.JoinNoFollow(targetPath, diskName)
}
// SetLocalDirectory creates the base directory where disk images will be mounted when hotplugged. File system volumes will be in
diff --git a/pkg/hotplug-disk/hotplug-disk_test.go b/pkg/hotplug-disk/hotplug-disk_test.go
index 4af3ae5ee..bbf96c23e 100644
--- a/pkg/hotplug-disk/hotplug-disk_test.go
+++ b/pkg/hotplug-disk/hotplug-disk_test.go
@@ -29,6 +29,7 @@ import (
"k8s.io/apimachinery/pkg/types"
diskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
)
var _ = Describe("HotplugDisk", func() {
@@ -80,7 +81,7 @@ var _ = Describe("HotplugDisk", func() {
testPath := filepath.Join(TargetPodBasePath(podsBaseDir, testUID), "testvolume")
exists, _ := diskutils.FileExists(testPath)
Expect(exists).To(BeTrue())
- Expect(res).To(Equal(testPath))
+ Expect(unsafepath.UnsafeAbsolute(res.Raw())).To(Equal(testPath))
})
It("GetFileSystemDirectoryTargetPathFromHostView should return the volume directory", func() {
@@ -90,7 +91,7 @@ var _ = Describe("HotplugDisk", func() {
err = os.MkdirAll(testPath, os.FileMode(0755))
res, err := hotplug.GetFileSystemDirectoryTargetPathFromHostView(testUID, "testvolume", false)
Expect(err).ToNot(HaveOccurred())
- Expect(res).To(Equal(testPath))
+ Expect(unsafepath.UnsafeAbsolute(res.Raw())).To(Equal(testPath))
})
It("GetFileSystemDirectoryTargetPathFromHostView should fail on invalid UID", func() {
@@ -107,7 +108,7 @@ var _ = Describe("HotplugDisk", func() {
targetPath := filepath.Join(TargetPodBasePath(podsBaseDir, testUID), "testvolume.img")
exists, _ := diskutils.FileExists(targetPath)
Expect(exists).To(BeTrue())
- Expect(res).To(Equal(targetPath))
+ Expect(unsafepath.UnsafeAbsolute(res.Raw())).To(Equal(targetPath))
})
It("GetFileSystemDiskTargetPathFromHostView should return the disk image file", func() {
@@ -117,7 +118,7 @@ var _ = Describe("HotplugDisk", func() {
err = os.MkdirAll(targetPath, os.FileMode(0755))
res, err := hotplug.GetFileSystemDiskTargetPathFromHostView(testUID, "testvolume", false)
Expect(err).ToNot(HaveOccurred())
- Expect(res).To(Equal(targetPath))
+ Expect(unsafepath.UnsafeAbsolute(res.Raw())).To(Equal(targetPath))
})
It("GetFileSystemDiskTargetPathFromHostView should fail on invalid UID", func() {
diff --git a/pkg/virt-handler/hotplug-disk/BUILD.bazel b/pkg/virt-handler/hotplug-disk/BUILD.bazel
index 3bded5278..450e66127 100644
--- a/pkg/virt-handler/hotplug-disk/BUILD.bazel
+++ b/pkg/virt-handler/hotplug-disk/BUILD.bazel
@@ -12,7 +12,8 @@ go_library(
deps = [
"//pkg/ephemeral-disk-utils:go_default_library",
"//pkg/hotplug-disk:go_default_library",
- "//pkg/util:go_default_library",
+ "//pkg/safepath:go_default_library",
+ "//pkg/unsafepath:go_default_library",
"//pkg/virt-handler/cgroup:go_default_library",
"//pkg/virt-handler/isolation:go_default_library",
"//pkg/virt-handler/virt-chroot:go_default_library",
@@ -22,6 +23,7 @@ go_library(
"//vendor/github.com/opencontainers/runc/libcontainer/configs:go_default_library",
"//vendor/github.com/opencontainers/runc/libcontainer/devices:go_default_library",
"//vendor/github.com/pkg/errors:go_default_library",
+ "//vendor/golang.org/x/sys/unix:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
],
@@ -38,17 +40,19 @@ go_test(
deps = [
"//pkg/ephemeral-disk-utils:go_default_library",
"//pkg/hotplug-disk:go_default_library",
+ "//pkg/safepath:go_default_library",
+ "//pkg/unsafepath:go_default_library",
"//pkg/virt-handler/cgroup:go_default_library",
"//pkg/virt-handler/isolation:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
"//staging/src/kubevirt.io/client-go/api:go_default_library",
- "//staging/src/kubevirt.io/client-go/log:go_default_library",
"//staging/src/kubevirt.io/client-go/testutils:go_default_library",
"//vendor/github.com/golang/mock/gomock:go_default_library",
"//vendor/github.com/onsi/ginkgo/v2:go_default_library",
"//vendor/github.com/onsi/gomega:go_default_library",
"//vendor/github.com/opencontainers/runc/libcontainer/configs:go_default_library",
"//vendor/github.com/opencontainers/runc/libcontainer/devices:go_default_library",
+ "//vendor/golang.org/x/sys/unix:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
diff --git a/pkg/virt-handler/hotplug-disk/hotplug-disk_suite_test.go b/pkg/virt-handler/hotplug-disk/hotplug-disk_suite_test.go
index 9c4b60760..321195557 100644
--- a/pkg/virt-handler/hotplug-disk/hotplug-disk_suite_test.go
+++ b/pkg/virt-handler/hotplug-disk/hotplug-disk_suite_test.go
@@ -25,6 +25,6 @@ import (
"kubevirt.io/client-go/testutils"
)
-func TestHostDisk(t *testing.T) {
+func TestHotplugDisk(t *testing.T) {
testutils.KubeVirtTestSuiteSetup(t)
}
diff --git a/pkg/virt-handler/hotplug-disk/mount.go b/pkg/virt-handler/hotplug-disk/mount.go
index 56e1ec2a4..b80bad033 100644
--- a/pkg/virt-handler/hotplug-disk/mount.go
+++ b/pkg/virt-handler/hotplug-disk/mount.go
@@ -3,20 +3,21 @@ package hotplug_volume
import (
"encoding/json"
"fmt"
- "io"
"io/ioutil"
"os"
- "os/exec"
"path/filepath"
- "strconv"
- "strings"
"sync"
+ "syscall"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
+
+ "golang.org/x/sys/unix"
+
+ "kubevirt.io/kubevirt/pkg/safepath"
virt_chroot "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot"
diskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils"
hotplugdisk "kubevirt.io/kubevirt/pkg/hotplug-disk"
- "kubevirt.io/kubevirt/pkg/util"
"kubevirt.io/kubevirt/pkg/virt-handler/cgroup"
"kubevirt.io/kubevirt/pkg/virt-handler/isolation"
@@ -24,7 +25,6 @@ import (
"github.com/opencontainers/runc/libcontainer/devices"
k8sv1 "k8s.io/api/core/v1"
-
"k8s.io/apimachinery/pkg/types"
v1 "kubevirt.io/api/core/v1"
@@ -39,42 +39,83 @@ const (
)
var (
- deviceBasePath = func(podUID types.UID) string {
- return fmt.Sprintf("/proc/1/root/var/lib/kubelet/pods/%s/volumes/kubernetes.io~empty-dir/hotplug-disks", string(podUID))
+ nodeIsolationResult = func() isolation.IsolationResult {
+ return isolation.NodeIsolationResult()
+ }
+ deviceBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return safepath.JoinAndResolveWithRelativeRoot("/proc/1/root", fmt.Sprintf("/var/lib/kubelet/pods/%s/volumes/kubernetes.io~empty-dir/hotplug-disks", string(podUID)))
}
- sourcePodBasePath = func(podUID types.UID) string {
- return fmt.Sprintf("/proc/1/root/var/lib/kubelet/pods/%s/volumes", string(podUID))
+ sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return safepath.JoinAndResolveWithRelativeRoot("/proc/1/root", fmt.Sprintf("root/var/lib/kubelet/pods/%s/volumes", string(podUID)))
}
socketPath = func(podUID types.UID) string {
return fmt.Sprintf("pods/%s/volumes/kubernetes.io~empty-dir/hotplug-disks/hp.sock", string(podUID))
}
- statCommand = func(fileName string) ([]byte, error) {
- return exec.Command("/usr/bin/stat", fileName, "-L", "-c%t,%T,%a,%F").CombinedOutput()
+ statDevice = func(fileName *safepath.Path) (os.FileInfo, error) {
+ info, err := safepath.StatAtNoFollow(fileName)
+ if err != nil {
+ return nil, err
+ }
+ if info.Mode()&os.ModeDevice == 0 {
+ return info, fmt.Errorf("%v is not a block device", fileName)
+ }
+ return info, nil
+ }
+
+ statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) {
+ // we don't know the device name, we only know that it is the only
+ // device in a specific directory, let's look it up
+ var devName string
+ err := fileName.ExecuteNoFollow(func(safePath string) error {
+ entries, err := os.ReadDir(safePath)
+ if err != nil {
+ return err
+ }
+ for _, entry := range entries {
+ info, err := entry.Info()
+ if err != nil {
+ return err
+ }
+ if info.Mode()&os.ModeDevice == 0 {
+ // not a device
+ continue
+ }
+ devName = entry.Name()
+ return nil
+ }
+ return fmt.Errorf("no device in %v", fileName)
+ })
+ if err != nil {
+ return nil, err
+ }
+ devPath, err := safepath.JoinNoFollow(fileName, devName)
+ if err != nil {
+ return nil, err
+ }
+ return statDevice(devPath)
}
- mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) {
- output, err := exec.Command("/usr/bin/mknod", "--mode", fmt.Sprintf("0%s", blockDevicePermissions), deviceName, "b", strconv.FormatInt(major, 10), strconv.FormatInt(minor, 10)).CombinedOutput()
- log.Log.V(3).Infof("running mknod. err: %v, output: %s", err, string(output))
- return output, err
+ mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error {
+ return safepath.MknodAtNoFollow(basePath, deviceName, blockDevicePermissions|syscall.S_IFBLK, dev)
}
- mountCommand = func(sourcePath, targetPath string) ([]byte, error) {
- return virt_chroot.MountChroot(strings.TrimPrefix(sourcePath, isolation.NodeIsolationResult().MountRoot()), targetPath, false).CombinedOutput()
+ mountCommand = func(sourcePath, targetPath *safepath.Path) ([]byte, error) {
+ return virt_chroot.MountChroot(sourcePath, targetPath, false).CombinedOutput()
}
- unmountCommand = func(diskPath string) ([]byte, error) {
+ unmountCommand = func(diskPath *safepath.Path) ([]byte, error) {
return virt_chroot.UmountChroot(diskPath).CombinedOutput()
}
- isMounted = func(path string) (bool, error) {
- return isolation.NodeIsolationResult().IsMounted(path)
+ isMounted = func(path *safepath.Path) (bool, error) {
+ return isolation.IsMounted(path)
}
- isBlockDevice = func(path string) (bool, error) {
- return isolation.NodeIsolationResult().IsBlockDevice(path)
+ isBlockDevice = func(path *safepath.Path) (bool, error) {
+ return isolation.IsBlockDevice(path)
}
isolationDetector = func(path string) isolation.PodIsolationDetector {
@@ -230,11 +271,9 @@ func (m *volumeMounter) setMountTargetRecord(vmi *v1.VirtualMachineInstance, rec
}
func (m *volumeMounter) writePathToMountRecord(path string, vmi *v1.VirtualMachineInstance, record *vmiMountTargetRecord) error {
- if path != "" {
- record.MountTargetEntries = append(record.MountTargetEntries, vmiMountTargetEntry{
- TargetFile: path,
- })
- }
+ record.MountTargetEntries = append(record.MountTargetEntries, vmiMountTargetEntry{
+ TargetFile: path,
+ })
if err := m.setMountTargetRecord(vmi, record); err != nil {
return err
}
@@ -248,12 +287,12 @@ func (m *volumeMounter) mountHotplugVolume(vmi *v1.VirtualMachineInstance, volum
if m.isBlockVolume(&vmi.Status, volumeName) {
logger.V(4).Infof("Mounting block volume: %s", volumeName)
if err := m.mountBlockHotplugVolume(vmi, volumeName, sourceUID, record); err != nil {
- return err
+ return fmt.Errorf("failed to mount block hotplug volume %s: %v", volumeName, err)
}
} else {
logger.V(4).Infof("Mounting file system volume: %s", volumeName)
if err := m.mountFileSystemHotplugVolume(vmi, volumeName, sourceUID, record, mountDirectory); err != nil {
- return err
+ return fmt.Errorf("failed to mount filesystem hotplug volume %s: %v", volumeName, err)
}
}
}
@@ -335,41 +374,54 @@ func (m *volumeMounter) mountBlockHotplugVolume(vmi *v1.VirtualMachineInstance,
return err
}
- deviceName := filepath.Join(targetPath, volume)
-
- isMigrationInProgress := vmi.Status.MigrationState != nil && !vmi.Status.MigrationState.Completed
-
cgroupsManager, err := getCgroupManager(vmi)
if err != nil {
return fmt.Errorf(failedToCreateCgroupManagerErrTemplate, err)
}
- if isBlockExists, _ := isBlockDevice(deviceName); !isBlockExists {
- sourceMajor, sourceMinor, permissions, err := m.getSourceMajorMinor(sourceUID, volume)
+ if _, err := safepath.JoinNoFollow(targetPath, volume); os.IsNotExist(err) {
+ dev, permissions, err := m.getSourceMajorMinor(sourceUID, volume)
if err != nil {
return err
}
- if err := m.writePathToMountRecord(deviceName, vmi, record); err != nil {
- return err
- }
- // allow block devices
- if err := m.allowBlockMajorMinor(sourceMajor, sourceMinor, cgroupsManager); err != nil {
+
+ if err := m.writePathToMountRecord(filepath.Join(unsafepath.UnsafeAbsolute(targetPath.Raw()), volume), vmi, record); err != nil {
return err
}
- if _, err = m.createBlockDeviceFile(deviceName, sourceMajor, sourceMinor, permissions); err != nil {
+
+ if err := m.createBlockDeviceFile(targetPath, volume, dev, permissions); err != nil && !os.IsExist(err) {
return err
}
- } else if isBlockExists && (!m.volumeStatusReady(volume, vmi) || isMigrationInProgress) {
- // Block device exists already, but the volume is not ready yet, ensure that the device is allowed.
- sourceMajor, sourceMinor, _, err := m.getSourceMajorMinor(sourceUID, volume)
+ log.DefaultLogger().V(1).Infof("successfully created block device %v", volume)
+ } else if err != nil {
+ return err
+ }
+
+ devicePath, err := safepath.JoinNoFollow(targetPath, volume)
+ if err != nil {
+ return err
+ }
+ if isBlockExists, err := isBlockDevice(devicePath); err != nil {
+ return err
+ } else if !isBlockExists {
+ return fmt.Errorf("target device %v exists but it is not a block device", devicePath)
+ }
+
+ isMigrationInProgress := vmi.Status.MigrationState != nil && !vmi.Status.MigrationState.Completed
+ volumeNotReady := !m.volumeStatusReady(volume, vmi)
+
+ if isMigrationInProgress || volumeNotReady {
+ dev, _, err := m.getSourceMajorMinor(sourceUID, volume)
if err != nil {
return err
}
- if err := m.allowBlockMajorMinor(sourceMajor, sourceMinor, cgroupsManager); err != nil {
+ // allow block devices
+ if err := m.allowBlockMajorMinor(dev, cgroupsManager); err != nil {
return err
}
}
- return m.ownershipManager.SetFileOwnership(deviceName)
+
+ return m.ownershipManager.SetFileOwnership(devicePath)
}
func (m *volumeMounter) volumeStatusReady(volumeName string, vmi *v1.VirtualMachineInstance) bool {
@@ -385,109 +437,42 @@ func (m *volumeMounter) volumeStatusReady(volumeName string, vmi *v1.VirtualMach
return true
}
-func (m *volumeMounter) getSourceMajorMinor(sourceUID types.UID, volumeName string) (int64, int64, string, error) {
- result := make([]int64, 2)
- perms := ""
- if sourceUID != types.UID("") {
- basepath := filepath.Join(deviceBasePath(sourceUID), volumeName)
- err := filepath.Walk(basepath, func(filePath string, info os.FileInfo, err error) error {
- if info != nil && !info.IsDir() {
- // Walk doesn't follow symlinks which is good because I need to massage symlinks
- linkInfo, err := os.Lstat(filePath)
- if err != nil {
- return err
- }
- path := filePath
- if linkInfo.Mode()&os.ModeSymlink != 0 {
- // Its a symlink, follow it
- link, err := os.Readlink(filePath)
- if err != nil {
- return err
- }
- if !strings.HasPrefix(link, util.HostRootMount) {
- path = filepath.Join(util.HostRootMount, link)
- } else {
- path = link
- }
- }
- if m.isBlockFile(path) {
- result[0], result[1], perms, err = m.getBlockFileMajorMinor(path)
- // Err != nil means not a block device or unable to determine major/minor, try next file
- if err == nil {
- // Successfully located
- return io.EOF
- }
- }
- return nil
- }
- return nil
- })
- if err != nil && err != io.EOF {
- return -1, -1, "", err
- }
- }
- if perms == "" {
- return -1, -1, "", fmt.Errorf("Unable to find block device")
- }
- return result[0], result[1], perms, nil
-}
-
-func (m *volumeMounter) isBlockFile(fileName string) bool {
- // Stat the file and see if there is no error
- out, err := statCommand(fileName)
+func (m *volumeMounter) getSourceMajorMinor(sourceUID types.UID, volumeName string) (uint64, os.FileMode, error) {
+ basePath, err := deviceBasePath(sourceUID)
if err != nil {
- // Not a block device skip to next file
- return false
+ return 0, 0, err
}
- split := strings.Split(string(out), ",")
- // Verify I got 4 strings
- if len(split) != 4 {
- return false
+ devicePath, err := basePath.AppendAndResolveWithRelativeRoot(volumeName)
+ if err != nil {
+ return 0, 0, err
}
- return strings.TrimSpace(split[3]) == "block special file"
+ return m.getBlockFileMajorMinor(devicePath, statSourceDevice)
}
-func (m *volumeMounter) getBlockFileMajorMinor(fileName string) (int64, int64, string, error) {
- result := make([]int, 2)
- // Stat the file and see if there is no error
- out, err := statCommand(fileName)
+func (m *volumeMounter) getBlockFileMajorMinor(devicePath *safepath.Path, getter func(fileName *safepath.Path) (os.FileInfo, error)) (uint64, os.FileMode, error) {
+ fileInfo, err := getter(devicePath)
if err != nil {
- // Not a block device skip to next file
- return -1, -1, "", err
- }
- split := strings.Split(string(out), ",")
- // Verify I got 4 strings
- if len(split) != 4 {
- return -1, -1, "", fmt.Errorf("Output invalid")
+ return 0, 0, err
}
- if strings.TrimSpace(split[3]) != "block special file" {
- return -1, -1, "", fmt.Errorf("Not a block device")
- }
- // Verify that both values are ints.
- for i := 0; i < 2; i++ {
- val, err := strconv.ParseInt(split[i], 16, 32)
- if err != nil {
- return -1, -1, "", err
- }
- result[i] = int(val)
- }
- return int64(result[0]), int64(result[1]), split[2], nil
+ info := fileInfo.Sys().(*syscall.Stat_t)
+ return info.Rdev, fileInfo.Mode(), nil
}
-func (m *volumeMounter) removeBlockMajorMinor(major, minor int64, manager cgroup.Manager) error {
- return m.updateBlockMajorMinor(major, minor, false, manager)
+func (m *volumeMounter) removeBlockMajorMinor(dev uint64, manager cgroup.Manager) error {
+ return m.updateBlockMajorMinor(dev, false, manager)
}
-func (m *volumeMounter) allowBlockMajorMinor(major, minor int64, manager cgroup.Manager) error {
- return m.updateBlockMajorMinor(major, minor, true, manager)
+func (m *volumeMounter) allowBlockMajorMinor(dev uint64, manager cgroup.Manager) error {
+ return m.updateBlockMajorMinor(dev, true, manager)
}
-func (m *volumeMounter) updateBlockMajorMinor(major, minor int64, allow bool, manager cgroup.Manager) error {
+func (m *volumeMounter) updateBlockMajorMinor(dev uint64, allow bool, manager cgroup.Manager) error {
+
var err error
deviceRule := &devices.Rule{
Type: devices.BlockDevice,
- Major: major,
- Minor: minor,
+ Major: int64(unix.Major(dev)),
+ Minor: int64(unix.Minor(dev)),
Permissions: "rwm",
Allow: allow,
}
@@ -505,19 +490,12 @@ func (m *volumeMounter) updateBlockMajorMinor(major, minor int64, allow bool, ma
return err
}
-func (m *volumeMounter) createBlockDeviceFile(deviceName string, major, minor int64, blockDevicePermissions string) (string, error) {
- exists, err := diskutils.FileExists(deviceName)
- if err != nil {
- return "", err
- }
- if !exists {
- out, err := mknodCommand(deviceName, major, minor, blockDevicePermissions)
- if err != nil {
- log.DefaultLogger().Errorf("Error creating block device file: %s, %v", out, err)
- return "", err
- }
+func (m *volumeMounter) createBlockDeviceFile(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error {
+ if _, err := safepath.JoinNoFollow(basePath, deviceName); os.IsNotExist(err) {
+ return mknodCommand(basePath, deviceName, dev, blockDevicePermissions)
+ } else {
+ return err
}
- return deviceName, nil
}
func (m *volumeMounter) mountFileSystemHotplugVolume(vmi *v1.VirtualMachineInstance, volume string, sourceUID types.UID, record *vmiMountTargetRecord, mountDirectory bool) error {
@@ -526,12 +504,12 @@ func (m *volumeMounter) mountFileSystemHotplugVolume(vmi *v1.VirtualMachineInsta
// This is not the node the pod is running on.
return nil
}
- var target string
+ var target *safepath.Path
var err error
if mountDirectory {
- target, err = m.hotplugDiskManager.GetFileSystemDirectoryTargetPathFromHostView(virtlauncherUID, volume, false)
+ target, err = m.hotplugDiskManager.GetFileSystemDirectoryTargetPathFromHostView(virtlauncherUID, volume, true)
} else {
- target, err = m.hotplugDiskManager.GetFileSystemDiskTargetPathFromHostView(virtlauncherUID, volume, false)
+ target, err = m.hotplugDiskManager.GetFileSystemDiskTargetPathFromHostView(virtlauncherUID, volume, true)
}
if err != nil {
return err
@@ -547,7 +525,7 @@ func (m *volumeMounter) mountFileSystemHotplugVolume(vmi *v1.VirtualMachineInsta
// to get mounted on the node, and this will error until the volume is mounted.
return nil
}
- if err := m.writePathToMountRecord(target, vmi, record); err != nil {
+ if err := m.writePathToMountRecord(unsafepath.UnsafeAbsolute(target.Raw()), vmi, record); err != nil {
return err
}
if mountDirectory {
@@ -563,11 +541,17 @@ func (m *volumeMounter) mountFileSystemHotplugVolume(vmi *v1.VirtualMachineInsta
if err != nil {
return err
}
- if out, err := mountCommand(filepath.Join(sourcePath, "disk.img"), target); err != nil {
- return fmt.Errorf("failed to bindmount hotplug-disk %v: %v : %v", volume, string(out), err)
+ sourcePath, err = sourcePath.AppendAndResolveWithRelativeRoot("disk.img")
+ if err != nil {
+ return err
+ }
+ if out, err := mountCommand(sourcePath, target); err != nil {
+ return fmt.Errorf("failed to bindmount hotplug-disk %v to %v: %v : %v", sourcePath, target, string(out), err)
}
+ log.DefaultLogger().V(1).Infof("successfully mounted %v", volume)
}
}
+
return m.ownershipManager.SetFileOwnership(target)
}
@@ -587,43 +571,58 @@ func (m *volumeMounter) findVirtlauncherUID(vmi *v1.VirtualMachineInstance) (uid
return types.UID("")
}
-func (m *volumeMounter) getSourcePodFilePath(sourceUID types.UID, vmi *v1.VirtualMachineInstance, volume string) (string, error) {
+func (m *volumeMounter) getSourcePodFilePath(sourceUID types.UID, vmi *v1.VirtualMachineInstance, volume string) (*safepath.Path, error) {
iso := isolationDetector("/path")
isoRes, err := iso.DetectForSocket(vmi, socketPath(sourceUID))
if err != nil {
- return "", err
+ return nil, err
}
findmounts, err := LookupFindmntInfoByVolume(volume, isoRes.Pid())
if err != nil {
- return "", err
+ return nil, err
+ }
+ mountRoot, err := nodeIsolationResult().MountRoot()
+ if err != nil {
+ return nil, err
}
+
for _, findmnt := range findmounts {
if filepath.Base(findmnt.Target) == volume {
source := findmnt.GetSourcePath()
- isBlock, _ := isBlockDevice(filepath.Join(util.HostRootMount, source))
- if _, err := os.Stat(filepath.Join(util.HostRootMount, source)); os.IsNotExist(err) || isBlock {
+ path, err := mountRoot.AppendAndResolveWithRelativeRoot(source)
+ exists := !os.IsNotExist(err)
+ if err != nil && !os.IsNotExist(err) {
+ return nil, err
+ }
+
+ isBlock := false
+ if exists {
+ isBlock, _ = isBlockDevice(path)
+ }
+
+ if !exists || isBlock {
// file not found, or block device, or directory check if we can find the mount.
deviceFindMnt, err := LookupFindmntInfoByDevice(source)
if err != nil {
// Try the device found from the source
deviceFindMnt, err = LookupFindmntInfoByDevice(findmnt.GetSourceDevice())
if err != nil {
- return "", err
+ return nil, err
}
// Check if the path was relative to the device.
- if _, err := os.Stat(filepath.Join(util.HostRootMount, source)); err != nil {
- return filepath.Join(deviceFindMnt[0].Target, source), nil
+ if !exists {
+ return mountRoot.AppendAndResolveWithRelativeRoot(deviceFindMnt[0].Target, source)
}
- return "", err
+ return nil, err
}
- return deviceFindMnt[0].Target, nil
+ return mountRoot.AppendAndResolveWithRelativeRoot(deviceFindMnt[0].Target)
} else {
- return source, nil
+ return path, nil
}
}
}
// Did not find the disk image file, return error
- return "", fmt.Errorf("unable to find source disk image path for pod %s", sourceUID)
+ return nil, fmt.Errorf("unable to find source disk image path for pod %s", sourceUID)
}
// Unmount unmounts all hotplug disk that are no longer part of the VMI
@@ -645,6 +644,13 @@ func (m *volumeMounter) Unmount(vmi *v1.VirtualMachineInstance) error {
basePath, err := m.hotplugDiskManager.GetHotplugTargetPodPathOnHost(virtlauncherUID)
if err != nil {
+ if os.IsNotExist(err) {
+ // no mounts left, the base path does not even exist anymore
+ if err := m.deleteMountTargetRecord(vmi); err != nil {
+ return fmt.Errorf("failed to delete mount target records: %v", err)
+ }
+ return nil
+ }
return err
}
for _, volumeStatus := range vmi.Status.VolumeStatus {
@@ -652,40 +658,57 @@ func (m *volumeMounter) Unmount(vmi *v1.VirtualMachineInstance) error {
continue
}
if m.isBlockVolume(&vmi.Status, volumeStatus.Name) {
- path := filepath.Join(basePath, volumeStatus.Name)
- currentHotplugPaths[path] = virtlauncherUID
+ path, err := safepath.JoinNoFollow(basePath, volumeStatus.Name)
+ if err != nil {
+ return err
+ }
+ currentHotplugPaths[unsafepath.UnsafeAbsolute(path.Raw())] = virtlauncherUID
} else if m.isDirectoryMounted(&vmi.Status, volumeStatus.Name) {
path, err := m.hotplugDiskManager.GetFileSystemDirectoryTargetPathFromHostView(virtlauncherUID, volumeStatus.Name, false)
+ if os.IsExist(err) {
+ // already unmounted or never mounted
+ continue
+ }
if err != nil {
return err
}
- currentHotplugPaths[path] = virtlauncherUID
+ currentHotplugPaths[unsafepath.UnsafeAbsolute(path.Raw())] = virtlauncherUID
} else {
path, err := m.hotplugDiskManager.GetFileSystemDiskTargetPathFromHostView(virtlauncherUID, volumeStatus.Name, false)
+ if os.IsNotExist(err) {
+ // already unmounted or never mounted
+ continue
+ }
if err != nil {
return err
}
- currentHotplugPaths[path] = virtlauncherUID
+ currentHotplugPaths[unsafepath.UnsafeAbsolute(path.Raw())] = virtlauncherUID
}
}
newRecord := vmiMountTargetRecord{
MountTargetEntries: make([]vmiMountTargetEntry, 0),
}
for _, entry := range record.MountTargetEntries {
- diskPath := entry.TargetFile
- if _, ok := currentHotplugPaths[diskPath]; !ok {
- if m.isBlockFile(diskPath) {
+ fd, err := safepath.NewFileNoFollow(entry.TargetFile)
+ if err != nil {
+ return err
+ }
+ fd.Close()
+ diskPath := fd.Path()
+
+ if _, ok := currentHotplugPaths[unsafepath.UnsafeAbsolute(diskPath.Raw())]; !ok {
+ if blockDevice, err := isBlockDevice(diskPath); err != nil {
+ return err
+ } else if blockDevice {
if err := m.unmountBlockHotplugVolumes(diskPath, vmi); err != nil {
return err
}
- } else {
- if err := m.unmountFileSystemHotplugVolumes(diskPath); err != nil {
- return err
- }
+ } else if err := m.unmountFileSystemHotplugVolumes(diskPath); err != nil {
+ return err
}
} else {
newRecord.MountTargetEntries = append(newRecord.MountTargetEntries, vmiMountTargetEntry{
- TargetFile: diskPath,
+ TargetFile: unsafepath.UnsafeAbsolute(diskPath.Raw()),
})
}
}
@@ -701,7 +724,7 @@ func (m *volumeMounter) Unmount(vmi *v1.VirtualMachineInstance) error {
return nil
}
-func (m *volumeMounter) unmountFileSystemHotplugVolumes(diskPath string) error {
+func (m *volumeMounter) unmountFileSystemHotplugVolumes(diskPath *safepath.Path) error {
if mounted, err := isMounted(diskPath); err != nil {
return fmt.Errorf("failed to check mount point for hotplug disk %v: %v", diskPath, err)
} else if mounted {
@@ -709,7 +732,7 @@ func (m *volumeMounter) unmountFileSystemHotplugVolumes(diskPath string) error {
if err != nil {
return fmt.Errorf("failed to unmount hotplug disk %v: %v : %v", diskPath, string(out), err)
}
- err = os.Remove(diskPath)
+ err = safepath.UnlinkAtNoFollow(diskPath)
if err != nil {
return fmt.Errorf("failed to remove hotplug disk directory %v: %v : %v", diskPath, string(out), err)
}
@@ -718,23 +741,22 @@ func (m *volumeMounter) unmountFileSystemHotplugVolumes(diskPath string) error {
return nil
}
-func (m *volumeMounter) unmountBlockHotplugVolumes(diskPath string, vmi *v1.VirtualMachineInstance) error {
+func (m *volumeMounter) unmountBlockHotplugVolumes(diskPath *safepath.Path, vmi *v1.VirtualMachineInstance) error {
cgroupsManager, err := getCgroupManager(vmi)
if err != nil {
return fmt.Errorf(failedToCreateCgroupManagerErrTemplate, err)
}
// Get major and minor so we can deny the container.
- major, minor, _, err := m.getBlockFileMajorMinor(diskPath)
+ dev, _, err := m.getBlockFileMajorMinor(diskPath, statDevice)
if err != nil {
return err
}
// Delete block device file
- err = os.Remove(diskPath)
- if err != nil {
+ if err := safepath.UnlinkAtNoFollow(diskPath); err != nil {
return err
}
- if err := m.removeBlockMajorMinor(major, minor, cgroupsManager); err != nil {
+ if err := m.removeBlockMajorMinor(dev, cgroupsManager); err != nil {
return err
}
return nil
@@ -755,14 +777,25 @@ func (m *volumeMounter) UnmountAll(vmi *v1.VirtualMachineInstance) error {
}
for _, entry := range record.MountTargetEntries {
- diskPath := entry.TargetFile
- if m.isBlockFile(diskPath) {
- if err := m.unmountBlockHotplugVolumes(diskPath, vmi); err != nil {
+ diskPath, err := safepath.NewFileNoFollow(entry.TargetFile)
+ if err != nil {
+ if os.IsNotExist(err) {
+ logger.Infof("Device %v is not mounted anymore, continuing.", entry.TargetFile)
+ continue
+ }
+ logger.Infof("Unable to unmount volume at path %s: %v", entry.TargetFile, err)
+ continue
+ }
+ diskPath.Close()
+ if isBlock, err := isBlockDevice(diskPath.Path()); err != nil {
+ logger.Infof("Unable to remove block device at path %s: %v", diskPath, err)
+ } else if isBlock {
+ if err := m.unmountBlockHotplugVolumes(diskPath.Path(), vmi); err != nil {
logger.Infof("Unable to remove block device at path %s: %v", diskPath, err)
// Don't return error, try next.
}
} else {
- if err := m.unmountFileSystemHotplugVolumes(diskPath); err != nil {
+ if err := m.unmountFileSystemHotplugVolumes(diskPath.Path()); err != nil {
logger.Infof("Unable to unmount volume at path %s: %v", diskPath, err)
// Don't return error, try next.
}
@@ -784,16 +817,38 @@ func (m *volumeMounter) IsMounted(vmi *v1.VirtualMachineInstance, volume string,
}
targetPath, err := m.hotplugDiskManager.GetHotplugTargetPodPathOnHost(virtlauncherUID)
if err != nil {
+ if os.IsNotExist(err) {
+ return false, nil
+ }
return false, err
}
if m.isBlockVolume(&vmi.Status, volume) {
- deviceName := filepath.Join(targetPath, volume)
+ deviceName, err := safepath.JoinNoFollow(targetPath, volume)
+ if err != nil {
+ if os.IsNotExist(err) {
+ return false, nil
+ }
+ return false, err
+ }
isBlockExists, _ := isBlockDevice(deviceName)
return isBlockExists, nil
}
-
if m.isDirectoryMounted(&vmi.Status, volume) {
- return isMounted(filepath.Join(targetPath, volume))
+ path, err := safepath.JoinNoFollow(targetPath, volume)
+ if err != nil {
+ if os.IsNotExist(err) {
+ return false, nil
+ }
+ return false, err
+ }
+ return isMounted(path)
+ }
+ path, err := safepath.JoinNoFollow(targetPath, fmt.Sprintf("%s.img", volume))
+ if err != nil {
+ if os.IsNotExist(err) {
+ return false, nil
+ }
+ return false, err
}
- return isMounted(filepath.Join(targetPath, fmt.Sprintf("%s.img", volume)))
+ return isMounted(path)
}
diff --git a/pkg/virt-handler/hotplug-disk/mount_test.go b/pkg/virt-handler/hotplug-disk/mount_test.go
index 9e8ecd0cf..461315f34 100644
--- a/pkg/virt-handler/hotplug-disk/mount_test.go
+++ b/pkg/virt-handler/hotplug-disk/mount_test.go
@@ -22,11 +22,18 @@ package hotplug_volume
import (
"encoding/json"
"fmt"
+ "io/fs"
"io/ioutil"
"os"
"path/filepath"
+ "strings"
+ "syscall"
+ "time"
- "kubevirt.io/client-go/log"
+ "golang.org/x/sys/unix"
+
+ "kubevirt.io/kubevirt/pkg/safepath"
+ "kubevirt.io/kubevirt/pkg/unsafepath"
"github.com/golang/mock/gomock"
. "github.com/onsi/ginkgo/v2"
@@ -55,18 +62,21 @@ const (
)
var (
- tempDir string
- orgIsoDetector = isolationDetector
- orgDeviceBasePath = deviceBasePath
- orgStatCommand = statCommand
- orgMknodCommand = mknodCommand
- orgSourcePodBasePath = sourcePodBasePath
- orgMountCommand = mountCommand
- orgUnMountCommand = unmountCommand
- orgIsMounted = isMounted
- orgIsBlockDevice = isBlockDevice
- orgFindMntByVolume = findMntByVolume
- orgFindMntByDevice = findMntByDevice
+ tempDir string
+ tmpDirSafe *safepath.Path
+ orgIsoDetector = isolationDetector
+ orgDeviceBasePath = deviceBasePath
+ orgStatSourceCommand = statSourceDevice
+ orgStatCommand = statDevice
+ orgMknodCommand = mknodCommand
+ orgSourcePodBasePath = sourcePodBasePath
+ orgMountCommand = mountCommand
+ orgUnMountCommand = unmountCommand
+ orgIsMounted = isMounted
+ orgIsBlockDevice = isBlockDevice
+ orgFindMntByVolume = findMntByVolume
+ orgFindMntByDevice = findMntByDevice
+ orgNodeIsolationResult = nodeIsolationResult
)
var _ = Describe("HotplugVolume", func() {
@@ -128,6 +138,10 @@ var _ = Describe("HotplugVolume", func() {
cgroupManagerMock.EXPECT().GetCgroupVersion().AnyTimes()
expectedCgroupRule = nil
ownershipManager = diskutils.NewMockOwnershipManagerInterface(ctrl)
+
+ nodeIsolationResult = func() isolation.IsolationResult {
+ return isolation.NewIsolationResult(os.Getpid(), os.Getppid())
+ }
})
Context("mount target records", func() {
@@ -141,6 +155,8 @@ var _ = Describe("HotplugVolume", func() {
BeforeEach(func() {
tempDir, err = ioutil.TempDir("", "hotplug-volume-test")
Expect(err).ToNot(HaveOccurred())
+ tmpDirSafe, err = safepath.JoinAndResolveWithRelativeRoot(tempDir)
+ Expect(err).ToNot(HaveOccurred())
vmi = api.NewMinimalVMI("fake-vmi")
vmi.UID = "1234"
@@ -241,13 +257,15 @@ var _ = Describe("HotplugVolume", func() {
BeforeEach(func() {
tempDir, err = ioutil.TempDir("", "hotplug-volume-test")
Expect(err).ToNot(HaveOccurred())
+ tmpDirSafe, err = safepath.JoinAndResolveWithRelativeRoot(tempDir)
+ Expect(err).ToNot(HaveOccurred())
vmi = api.NewMinimalVMI("fake-vmi")
vmi.UID = "1234"
activePods := make(map[types.UID]string, 0)
activePods["abcd"] = "host"
vmi.Status.ActivePods = activePods
- targetPodPath = filepath.Join(tempDir, "abcd/volumes/kubernetes.io~empty-dir/hotplug-disks/testvolume")
+ targetPodPath = filepath.Join(tempDir, "abcd/volumes/kubernetes.io~empty-dir/hotplug-disks/")
err = os.MkdirAll(targetPodPath, 0755)
Expect(err).ToNot(HaveOccurred())
@@ -261,11 +279,11 @@ var _ = Describe("HotplugVolume", func() {
ownershipManager: ownershipManager,
}
- deviceBasePath = func(sourceUID types.UID) string {
- return filepath.Join(tempDir, string(sourceUID), "volumes")
+ deviceBasePath = func(sourceUID types.UID) (*safepath.Path, error) {
+ return newDir(tempDir, string(sourceUID), "volumes")
}
- statCommand = func(fileName string) ([]byte, error) {
- return []byte("6,6,0777,block special file"), nil
+ statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) {
+ return fakeStat(true, 0777, 123456), nil
}
})
@@ -273,9 +291,10 @@ var _ = Describe("HotplugVolume", func() {
AfterEach(func() {
_ = os.RemoveAll(tempDir)
deviceBasePath = orgDeviceBasePath
- statCommand = orgStatCommand
+ statSourceDevice = orgStatSourceCommand
mknodCommand = orgMknodCommand
isBlockDevice = orgIsBlockDevice
+ nodeIsolationResult = orgNodeIsolationResult
})
It("isBlockVolume should determine if we have a block volume", func() {
@@ -329,30 +348,39 @@ var _ = Describe("HotplugVolume", func() {
targetPodPath := hotplugdisk.TargetPodBasePath(tempDir, m.findVirtlauncherUID(vmi))
err = os.MkdirAll(targetPodPath, 0755)
Expect(err).ToNot(HaveOccurred())
- deviceFile := filepath.Join(tempDir, string(blockSourcePodUID), "volumes", "testvolume", "file")
- err = os.MkdirAll(filepath.Dir(deviceFile), 0755)
- Expect(err).ToNot(HaveOccurred())
- err = ioutil.WriteFile(deviceFile, []byte("test"), 0644)
+ deviceFile, err := newFile(tempDir, string(blockSourcePodUID), "volumes", "testvolume", "file")
Expect(err).ToNot(HaveOccurred())
+ Expect(ioutil.WriteFile(unsafepath.UnsafeAbsolute(deviceFile.Raw()), []byte("test"), 0644))
- targetDevicePath := filepath.Join(targetPodPath, "testvolume")
+ vmi.Status.VolumeStatus = []v1.VolumeStatus{{Name: "testvolume", HotplugVolume: &v1.HotplugVolumeStatus{}}}
+ targetDevicePath, err := newFile(targetPodPath, "testvolume")
+ Expect(err).ToNot(HaveOccurred())
ownershipManager.EXPECT().SetFileOwnership(targetDevicePath)
+ statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) {
+ return fakeStat(true, 0666, 123456), nil
+ }
+ statDevice = func(fileName *safepath.Path) (os.FileInfo, error) {
+ return fakeStat(true, 0666, 123456), nil
+ }
+ isBlockDevice = func(fileName *safepath.Path) (bool, error) {
+ return true, nil
+ }
By("Mounting and validating expected rule is set")
setExpectedCgroupRuns(2)
- expectCgroupRule(devices.BlockDevice, 6, 6, true)
+ expectCgroupRule(devices.BlockDevice, 482, 64, true)
err = m.mountBlockHotplugVolume(vmi, "testvolume", blockSourcePodUID, record)
Expect(err).ToNot(HaveOccurred())
By("Unmounting, we verify the reverse process happens")
- expectCgroupRule(devices.BlockDevice, 6, 6, false)
- err = m.unmountBlockHotplugVolumes(targetDevicePath, vmi)
+ expectCgroupRule(devices.BlockDevice, 482, 64, false)
+ err = m.unmountBlockHotplugVolumes(deviceFile, vmi)
Expect(err).ToNot(HaveOccurred())
})
It("getSourceMajorMinor should return an error if no uid", func() {
vmi.UID = ""
- _, _, _, err := m.getSourceMajorMinor("fghij", "invalid")
+ _, _, err := m.getSourceMajorMinor("fghij", "invalid")
Expect(err).To(HaveOccurred())
})
@@ -362,161 +390,121 @@ var _ = Describe("HotplugVolume", func() {
Expect(err).ToNot(HaveOccurred())
err = ioutil.WriteFile(deviceFile, []byte("test"), 0644)
Expect(err).ToNot(HaveOccurred())
- major, minor, perm, err := m.getSourceMajorMinor("fghij", "test-volume")
+ numbers, perm, err := m.getSourceMajorMinor("fghij", "test-volume")
Expect(err).ToNot(HaveOccurred())
- Expect(major).To(Equal(int64(6)))
- Expect(minor).To(Equal(int64(6)))
- Expect(perm).To(Equal("0777"))
+ Expect(unix.Major(numbers)).To(Equal(uint32(482)))
+ Expect(unix.Minor(numbers)).To(Equal(uint32(64)))
+ Expect(perm & 0777).To(Equal(os.FileMode(0777)))
})
It("getSourceMajorMinor should return error if file doesn't exists", func() {
deviceFile := filepath.Join(tempDir, "fghij", "volumes", "file")
err = os.MkdirAll(filepath.Dir(deviceFile), 0755)
Expect(err).ToNot(HaveOccurred())
- major, minor, perm, err := m.getSourceMajorMinor("fghij", "test-volume")
+ _, _, err := m.getSourceMajorMinor("fghij", "test-volume")
Expect(err).To(HaveOccurred())
- Expect(major).To(Equal(int64(-1)))
- Expect(minor).To(Equal(int64(-1)))
- Expect(perm).To(Equal(""))
+ Expect(os.IsNotExist(err)).To(BeTrue())
})
- It("isBlockFile should return proper value based on stat command", func() {
- testFileName := "test-file"
- statCommand = func(fileName string) ([]byte, error) {
- Expect(testFileName).To(Equal(fileName))
- return []byte("6,6,0777,block special file"), nil
- }
- Expect(m.isBlockFile(testFileName)).To(BeTrue())
- statCommand = func(fileName string) ([]byte, error) {
- Expect(testFileName).To(Equal(fileName))
- return []byte("6,6,0777,block special file"), fmt.Errorf("Error")
- }
- Expect(m.isBlockFile(testFileName)).To(BeFalse())
- statCommand = func(fileName string) ([]byte, error) {
- Expect(testFileName).To(Equal(fileName))
- return []byte("6,6,0777"), nil
- }
- Expect(m.isBlockFile(testFileName)).To(BeFalse())
- statCommand = func(fileName string) ([]byte, error) {
- Expect(testFileName).To(Equal(fileName))
- return []byte("6,6,0777,block special"), nil
- }
- Expect(m.isBlockFile(testFileName)).To(BeFalse())
- })
-
- DescribeTable("Should return proper values", func(stat func(fileName string) ([]byte, error), major, minor int, perm string, expectErr bool) {
- testFileName := "test-file"
- statCommand = stat
- majorRes, minorRes, permRes, err := m.getBlockFileMajorMinor(testFileName)
+ DescribeTable("Should return proper values", func(stat func(safePath *safepath.Path) (os.FileInfo, error), major, minor uint32, perm os.FileMode, expectErr bool) {
+ statSourceDevice = stat
+ testFileName, err := newFile(tempDir, "test-file")
+ Expect(err).ToNot(HaveOccurred())
+ numbers, permRes, err := m.getBlockFileMajorMinor(testFileName, statSourceDevice)
if expectErr {
Expect(err).To(HaveOccurred())
} else {
Expect(err).ToNot(HaveOccurred())
}
- // Values are translated to hex (245->580, 32->50)
- Expect(int64(major)).To(Equal(majorRes))
- Expect(int64(minor)).To(Equal(minorRes))
- Expect(perm).To(Equal(permRes))
+ Expect(unix.Major(numbers)).To(Equal(major))
+ Expect(unix.Minor(numbers)).To(Equal(minor))
+ Expect(perm).To(Equal(permRes & 0777))
},
- Entry("Should return values if stat command successful", func(fileName string) ([]byte, error) {
- return []byte("245,32,0664,block special file"), nil
- }, 581, 50, "0664", false),
- Entry("Should not return values if stat command errors", func(fileName string) ([]byte, error) {
- return []byte("245,32,0664,block special file"), fmt.Errorf("Error")
- }, -1, -1, "", true),
- Entry("Should not return values if stat command doesn't return 4 fields", func(fileName string) ([]byte, error) {
- return []byte("245,32,0664"), nil
- }, -1, -1, "", true),
- Entry("Should not return values if stat command doesn't return block special file", func(fileName string) ([]byte, error) {
- return []byte("245,32,0664, block file"), nil
- }, -1, -1, "", true),
- Entry("Should not return values if stat command doesn't int major", func(fileName string) ([]byte, error) {
- return []byte("kk,32,0664,block special file"), nil
- }, -1, -1, "", true),
- Entry("Should not return values if stat command doesn't int minor", func(fileName string) ([]byte, error) {
- return []byte("254,gg,0664,block special file"), nil
- }, -1, -1, "", true),
+ Entry("Should return values if stat command successful", func(safePath *safepath.Path) (os.FileInfo, error) {
+ return fakeStat(true, 0664, 123456), nil
+ }, uint32(482), uint32(64), os.FileMode(0664), false),
+ Entry("Should not return error if stat command errors", func(safePath *safepath.Path) (os.FileInfo, error) {
+ return fakeStat(true, 0664, 123456), fmt.Errorf("Error")
+ }, uint32(0), uint32(0), os.FileMode(0), true),
)
It("should write properly to allow/deny files if able", func() {
setExpectedCgroupRuns(2)
- expectCgroupRule(devices.BlockDevice, 34, 53, true)
- err = m.allowBlockMajorMinor(34, 53, cgroupManagerMock)
+ expectCgroupRule(devices.BlockDevice, 482, 64, true)
+ err = m.allowBlockMajorMinor(123456, cgroupManagerMock)
Expect(err).ToNot(HaveOccurred())
- expectCgroupRule(devices.BlockDevice, 34, 53, false)
- err = m.removeBlockMajorMinor(34, 53, cgroupManagerMock)
+ expectCgroupRule(devices.BlockDevice, 482, 64, false)
+ err = m.removeBlockMajorMinor(123456, cgroupManagerMock)
Expect(err).ToNot(HaveOccurred())
})
It("Should remove the block device and permissions on unmount", func() {
By("Initializing cgroup mock files")
- statCommand = func(fileName string) ([]byte, error) {
- return []byte("245,32,0664,block special file"), nil
+ statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) {
+ return fakeStat(true, 0664, 123456), nil
}
- deviceFileName := filepath.Join(tempDir, "devicefile")
- _, err := os.Create(deviceFileName)
+ deviceFileName, err := newFile(tempDir, "devicefile")
Expect(err).ToNot(HaveOccurred())
By("Mounting and validating expected rule is set")
setExpectedCgroupRuns(1)
- expectCgroupRule(devices.BlockDevice, 581, 50, false)
+ expectCgroupRule(devices.BlockDevice, 482, 64, false)
err = m.unmountBlockHotplugVolumes(deviceFileName, vmi)
Expect(err).ToNot(HaveOccurred())
})
It("Should return error if deviceFile doesn' exist", func() {
- statCommand = func(fileName string) ([]byte, error) {
- return []byte("245,32,0664,block special file"), nil
+ statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) {
+ return fakeStat(true, 0664, 123456), nil
}
- deviceFileName := filepath.Join(tempDir, "devicefile")
+ deviceFileName, err := newFile(tempDir, "devicefile")
+ Expect(err).ToNot(HaveOccurred())
+ os.Remove(unsafepath.UnsafeAbsolute(deviceFileName.Raw()))
err = m.unmountBlockHotplugVolumes(deviceFileName, vmi)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("no such file or directory"))
})
It("Should attempt to create a block device file if it doesn't exist", func() {
- testFile := filepath.Join(tempDir, "testfile")
- testMajor := int64(100)
- testMinor := int64(53)
- testPerm := "0664"
- mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) {
- Expect(deviceName).To(Equal(testFile))
- Expect(major).To(Equal(testMajor))
- Expect(minor).To(Equal(testMinor))
+ testMajor := uint32(482)
+ testMinor := uint32(64)
+ testPerm := os.FileMode(0664)
+ mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error {
+ Expect(basePath).To(Equal(tmpDirSafe))
+ Expect(deviceName).To(Equal("testfile"))
+ Expect(unix.Major(dev)).To(Equal(testMajor))
+ Expect(unix.Minor(dev)).To(Equal(testMinor))
Expect(blockDevicePermissions).To(Equal(testPerm))
- return []byte("Yay"), nil
+ return nil
}
- res, err := m.createBlockDeviceFile(testFile, testMajor, testMinor, testPerm)
+ err := m.createBlockDeviceFile(tmpDirSafe, "testfile", 123456, testPerm)
Expect(err).ToNot(HaveOccurred())
- Expect(res).To(Equal(testFile))
- mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) {
- Expect(deviceName).To(Equal(testFile))
- Expect(major).To(Equal(testMajor))
- Expect(minor).To(Equal(testMinor))
+ mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error {
+ Expect(basePath).To(Equal(tmpDirSafe))
+ Expect(deviceName).To(Equal("testfile"))
+ Expect(unix.Major(dev)).To(Equal(testMajor))
+ Expect(unix.Minor(dev)).To(Equal(testMinor))
Expect(blockDevicePermissions).To(Equal(testPerm))
- return []byte("Yay"), fmt.Errorf("Error creating block file")
+ return fmt.Errorf("Error creating block file")
}
- _, err = m.createBlockDeviceFile(testFile, testMajor, testMinor, testPerm)
+ err = m.createBlockDeviceFile(tmpDirSafe, "testfile", 123456, testPerm)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("Error creating block file"))
})
It("Should not attempt to create a block device file if it exists", func() {
testFile := filepath.Join(tempDir, "testfile")
- testMajor := int64(100)
- testMinor := int64(53)
- testPerm := "0664"
+ testPerm := os.FileMode(0664)
_, err = os.Create(testFile)
Expect(err).ToNot(HaveOccurred())
- mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) {
+ mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error {
Fail("Should not get called")
- return nil, nil
+ return nil
}
- res, err := m.createBlockDeviceFile(testFile, testMajor, testMinor, testPerm)
+ err := m.createBlockDeviceFile(tmpDirSafe, "testfile", 123456, testPerm)
Expect(err).ToNot(HaveOccurred())
- Expect(res).To(Equal(testFile))
})
})
@@ -527,20 +515,24 @@ var _ = Describe("HotplugVolume", func() {
err error
vmi *v1.VirtualMachineInstance
record *vmiMountTargetRecord
- targetPodPath string
+ targetPodPath *safepath.Path
)
BeforeEach(func() {
tempDir, err = ioutil.TempDir("", "hotplug-volume-test")
Expect(err).ToNot(HaveOccurred())
+ tmpDirSafe, err = safepath.JoinAndResolveWithRelativeRoot(tempDir)
+ Expect(err).ToNot(HaveOccurred())
+
+ volumeDir, err := newDir(tempDir, "volumes")
+ Expect(err).ToNot(HaveOccurred())
vmi = api.NewMinimalVMI("fake-vmi")
vmi.UID = "1234"
activePods := make(map[types.UID]string, 0)
activePods["abcd"] = "host"
vmi.Status.ActivePods = activePods
- targetPodPath = filepath.Join(tempDir, "abcd/volumes/kubernetes.io~empty-dir/hotplug-disks")
- err = os.MkdirAll(targetPodPath, 0755)
+ targetPodPath, err = newDir(tempDir, "abcd/volumes/kubernetes.io~empty-dir/hotplug-disks")
Expect(err).ToNot(HaveOccurred())
record = &vmiMountTargetRecord{}
@@ -552,12 +544,13 @@ var _ = Describe("HotplugVolume", func() {
ownershipManager: ownershipManager,
}
- deviceBasePath = func(sourceUID types.UID) string {
- return filepath.Join(tempDir, string(sourceUID), "volumes")
+ deviceBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return volumeDir, nil
+
}
isolationDetector = func(path string) isolation.PodIsolationDetector {
return &mockIsolationDetector{
- pid: 1,
+ pid: os.Getpid(),
}
}
})
@@ -574,20 +567,19 @@ var _ = Describe("HotplugVolume", func() {
})
It("getSourcePodFile should find the disk.img file, if it exists", func() {
- path := filepath.Join(tempDir, "ghfjk", "volumes")
- err = os.MkdirAll(path, 0755)
- sourcePodBasePath = func(podUID types.UID) string {
- return path
+ path, err := newDir(tempDir, "ghfjk", "volumes")
+ Expect(err).ToNot(HaveOccurred())
+ sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return path, nil
}
findMntByVolume = func(volumeName string, pid int) ([]byte, error) {
- return []byte(fmt.Sprintf(findmntByVolumeRes, "pvc", path)), nil
+ return []byte(fmt.Sprintf(findmntByVolumeRes, "pvc", unsafepath.UnsafeAbsolute(path.Raw()))), nil
}
- diskFile := filepath.Join(path, "disk.img")
- _, err := os.Create(diskFile)
+ _, err = newFile(unsafepath.UnsafeAbsolute(path.Raw()), "disk.img")
Expect(err).ToNot(HaveOccurred())
file, err := m.getSourcePodFilePath("ghfjk", vmi, "pvc")
Expect(err).ToNot(HaveOccurred())
- Expect(file).To(Equal(path))
+ Expect(unsafepath.UnsafeRelative(file.Raw())).To(Equal(unsafepath.UnsafeAbsolute(path.Raw())))
})
It("getSourcePodFile should return error if no UID", func() {
@@ -596,82 +588,77 @@ var _ = Describe("HotplugVolume", func() {
})
It("getSourcePodFile should return error if disk.img doesn't exist", func() {
- path := filepath.Join(tempDir, "ghfjk", "volumes")
- err = os.MkdirAll(path, 0755)
- sourcePodBasePath = func(podUID types.UID) string {
- return path
- }
+ path, err := newDir(tempDir, "ghfjk", "volumes")
Expect(err).ToNot(HaveOccurred())
- _, err := m.getSourcePodFilePath("ghfjk", vmi, "")
+ sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return path, nil
+ }
+ _, err = m.getSourcePodFilePath("ghfjk", vmi, "")
Expect(err).To(HaveOccurred())
})
It("getSourcePodFile should return error if iso detection returns error", func() {
- expectedPath := filepath.Join(tempDir, "ghfjk", "volumes")
- err = os.MkdirAll(expectedPath, 0755)
- sourcePodBasePath = func(podUID types.UID) string {
- return expectedPath
+ expectedPath, err := newDir(tempDir, "ghfjk", "volumes")
+ Expect(err).ToNot(HaveOccurred())
+ sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return expectedPath, nil
}
isolationDetector = func(path string) isolation.PodIsolationDetector {
return &mockIsolationDetector{
- pid: 40,
+ pid: 9999,
}
}
- Expect(err).ToNot(HaveOccurred())
- _, err := m.getSourcePodFilePath("ghfjk", vmi, "")
+ _, err = m.getSourcePodFilePath("ghfjk", vmi, "")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("isolation error"))
})
It("getSourcePodFile should return error if find mounts returns error", func() {
- expectedPath := filepath.Join(tempDir, "ghfjk", "volumes")
- err = os.MkdirAll(expectedPath, 0755)
- sourcePodBasePath = func(podUID types.UID) string {
- return expectedPath
+ expectedPath, err := newDir(tempDir, "ghfjk", "volumes")
+ Expect(err).ToNot(HaveOccurred())
+ sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return expectedPath, nil
}
findMntByVolume = func(volumeName string, pid int) ([]byte, error) {
return []byte(""), fmt.Errorf("findmnt error")
}
-
- Expect(err).ToNot(HaveOccurred())
- _, err := m.getSourcePodFilePath("ghfjk", vmi, "")
+ _, err = m.getSourcePodFilePath("ghfjk", vmi, "")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("findmnt error"))
})
It("getSourcePodFile should return the findmnt value", func() {
- expectedPath := filepath.Join(tempDir, "ghfjk", "volumes")
- err = os.MkdirAll(expectedPath, 0755)
- sourcePodBasePath = func(podUID types.UID) string {
- return expectedPath
+ expectedPath, err := newDir(tempDir, "ghfjk", "volumes")
+ Expect(err).ToNot(HaveOccurred())
+ sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return expectedPath, nil
}
findMntByVolume = func(volumeName string, pid int) ([]byte, error) {
- return []byte(fmt.Sprintf(findmntByVolumeRes, "pvc", expectedPath)), nil
+ return []byte(fmt.Sprintf(findmntByVolumeRes, "pvc", unsafepath.UnsafeAbsolute(expectedPath.Raw()))), nil
}
- Expect(err).ToNot(HaveOccurred())
res, err := m.getSourcePodFilePath("ghfjk", vmi, "pvc")
Expect(err).ToNot(HaveOccurred())
- Expect(res).To(Equal(expectedPath))
+ Expect(unsafepath.UnsafeRelative(res.Raw())).To(Equal(unsafepath.UnsafeAbsolute(expectedPath.Raw())))
})
It("should properly mount and unmount filesystem", func() {
sourcePodUID := "ghfjk"
- path := filepath.Join(tempDir, sourcePodUID, "volumes", "disk.img")
- err = os.MkdirAll(path, 0755)
- sourcePodBasePath = func(podUID types.UID) string {
- return path
+ path, err := newDir(tempDir, sourcePodUID, "volumes")
+ Expect(err).ToNot(HaveOccurred())
+ sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return path, nil
}
- diskFile := filepath.Join(path, "disk.img")
- _, err := os.Create(diskFile)
+ diskFile, err := newFile(unsafepath.UnsafeAbsolute(path.Raw()), "disk.img")
Expect(err).ToNot(HaveOccurred())
findMntByVolume = func(volumeName string, pid int) ([]byte, error) {
- return []byte(fmt.Sprintf(findmntByVolumeRes, "testvolume", path)), nil
+ return []byte(fmt.Sprintf(findmntByVolumeRes, "testvolume", unsafepath.UnsafeAbsolute(path.Raw()))), nil
}
- targetFilePath := filepath.Join(targetPodPath, "testvolume.img")
- mountCommand = func(sourcePath, targetPath string) ([]byte, error) {
- Expect(sourcePath).To(Equal(diskFile))
+ targetFilePath, err := newFile(unsafepath.UnsafeAbsolute(targetPodPath.Raw()), "testvolume.img")
+ Expect(err).ToNot(HaveOccurred())
+ mountCommand = func(sourcePath, targetPath *safepath.Path) ([]byte, error) {
+ Expect(unsafepath.UnsafeRelative(sourcePath.Raw())).To(Equal(unsafepath.UnsafeAbsolute(diskFile.Raw())))
Expect(targetPath).To(Equal(targetFilePath))
return []byte("Success"), nil
}
@@ -680,28 +667,27 @@ var _ = Describe("HotplugVolume", func() {
err = m.mountFileSystemHotplugVolume(vmi, "testvolume", types.UID(sourcePodUID), record, false)
Expect(err).ToNot(HaveOccurred())
Expect(record.MountTargetEntries).To(HaveLen(1))
- Expect(record.MountTargetEntries[0].TargetFile).To(Equal(targetFilePath))
+ Expect(record.MountTargetEntries[0].TargetFile).To(Equal(unsafepath.UnsafeAbsolute(targetFilePath.Raw())))
- unmountCommand = func(diskPath string) ([]byte, error) {
+ unmountCommand = func(diskPath *safepath.Path) ([]byte, error) {
Expect(targetFilePath).To(Equal(diskPath))
return []byte("Success"), nil
}
- isMounted = func(diskPath string) (bool, error) {
+ isMounted = func(diskPath *safepath.Path) (bool, error) {
Expect(targetFilePath).To(Equal(diskPath))
return true, nil
}
- err = m.unmountFileSystemHotplugVolumes(record.MountTargetEntries[0].TargetFile)
+ err = m.unmountFileSystemHotplugVolumes(targetFilePath)
Expect(err).ToNot(HaveOccurred())
- _, err = os.Stat(targetFilePath)
- Expect(err).To(HaveOccurred())
})
It("unmountFileSystemHotplugVolumes should return error if isMounted returns error", func() {
- testPath := "test"
- isMounted = func(diskPath string) (bool, error) {
- Expect(testPath).To(Equal(diskPath))
+ testPath, err := newFile(tempDir, "test")
+ Expect(err).ToNot(HaveOccurred())
+ isMounted = func(path *safepath.Path) (bool, error) {
+ Expect(testPath).To(Equal(path))
return false, fmt.Errorf("isMounted error")
}
@@ -711,8 +697,9 @@ var _ = Describe("HotplugVolume", func() {
})
It("unmountFileSystemHotplugVolumes should return nil if isMounted returns false", func() {
- testPath := "test"
- isMounted = func(diskPath string) (bool, error) {
+ testPath, err := newFile(tempDir, "test")
+ Expect(err).ToNot(HaveOccurred())
+ isMounted = func(diskPath *safepath.Path) (bool, error) {
Expect(testPath).To(Equal(diskPath))
return false, nil
}
@@ -722,12 +709,13 @@ var _ = Describe("HotplugVolume", func() {
})
It("unmountFileSystemHotplugVolumes should return error if unmountCommand returns error", func() {
- testPath := "test"
- isMounted = func(diskPath string) (bool, error) {
+ testPath, err := newFile(tempDir, "test")
+ Expect(err).ToNot(HaveOccurred())
+ isMounted = func(diskPath *safepath.Path) (bool, error) {
Expect(testPath).To(Equal(diskPath))
return true, nil
}
- unmountCommand = func(diskPath string) ([]byte, error) {
+ unmountCommand = func(diskPath *safepath.Path) ([]byte, error) {
return []byte("Failure"), fmt.Errorf("unmount error")
}
@@ -748,6 +736,8 @@ var _ = Describe("HotplugVolume", func() {
BeforeEach(func() {
tempDir, err = ioutil.TempDir("", "hotplug-volume-test")
Expect(err).ToNot(HaveOccurred())
+ tmpDirSafe, err = safepath.JoinAndResolveWithRelativeRoot(tempDir)
+ Expect(err).ToNot(HaveOccurred())
vmi = api.NewMinimalVMI("fake-vmi")
vmi.UID = "1234"
activePods := make(map[types.UID]string, 0)
@@ -766,15 +756,18 @@ var _ = Describe("HotplugVolume", func() {
ownershipManager: ownershipManager,
}
- deviceBasePath = func(sourceUID types.UID) string {
- return filepath.Join(tempDir, string(sourceUID), "volumes")
+ deviceBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return newDir(tempDir, string(podUID), "volumes")
}
- statCommand = func(fileName string) ([]byte, error) {
- return []byte("6,6,0777,block special file"), nil
+ statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) {
+ return fakeStat(true, 0777, 123456), nil
+ }
+ statDevice = func(fileName *safepath.Path) (os.FileInfo, error) {
+ return fakeStat(true, 0777, 123456), nil
}
isolationDetector = func(path string) isolation.PodIsolationDetector {
return &mockIsolationDetector{
- pid: 1,
+ pid: os.Getpid(),
}
}
})
@@ -786,14 +779,15 @@ var _ = Describe("HotplugVolume", func() {
mountCommand = orgMountCommand
unmountCommand = orgUnMountCommand
isMounted = orgIsMounted
- statCommand = orgStatCommand
+ statSourceDevice = orgStatSourceCommand
+ statDevice = orgStatCommand
mknodCommand = orgMknodCommand
isBlockDevice = orgIsBlockDevice
findMntByVolume = orgFindMntByVolume
})
It("mount and umount should work for filesystem volumes", func() {
- setExpectedCgroupRuns(3)
+ setExpectedCgroupRuns(2)
sourcePodUID := types.UID("klmno")
volumeStatuses := make([]v1.VolumeStatus, 0)
@@ -823,52 +817,53 @@ var _ = Describe("HotplugVolume", func() {
},
})
vmi.Status.VolumeStatus = volumeStatuses
- deviceBasePath = func(sourceUID types.UID) string {
- return filepath.Join(tempDir, string(sourceUID), "volumeDevices")
+ deviceBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return newDir(tempDir, string(podUID), "volumeDevices")
}
- blockDevicePath := filepath.Join(tempDir, string(sourcePodUID), "volumeDevices", "blockvolume")
- fileSystemPath := filepath.Join(tempDir, string(sourcePodUID), "volumes", "disk.img")
- By(fmt.Sprintf("Creating block path: %s", blockDevicePath))
- err = os.MkdirAll(blockDevicePath, 0755)
+ blockDevicePath, err := newDir(tempDir, string(sourcePodUID), "volumeDevices", "blockvolume")
Expect(err).ToNot(HaveOccurred())
- By(fmt.Sprintf("Creating filesystem path: %s", fileSystemPath))
- err = os.MkdirAll(fileSystemPath, 0755)
+ fileSystemPath, err := newDir(tempDir, string(sourcePodUID), "volumes")
Expect(err).ToNot(HaveOccurred())
- deviceFile := filepath.Join(blockDevicePath, "blockvolumefile")
- err = ioutil.WriteFile(deviceFile, []byte("test"), 0644)
+ deviceFile, err := newFile(unsafepath.UnsafeAbsolute(blockDevicePath.Raw()), "blockvolumefile")
+ Expect(err).ToNot(HaveOccurred())
+ err = ioutil.WriteFile(unsafepath.UnsafeAbsolute(deviceFile.Raw()), []byte("test"), 0644)
Expect(err).ToNot(HaveOccurred())
- sourcePodBasePath = func(podUID types.UID) string {
+ sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) {
if podUID == sourcePodUID {
- return blockDevicePath
+ return blockDevicePath, nil
}
- return fileSystemPath
+ return fileSystemPath, nil
}
findMntByVolume = func(volumeName string, pid int) ([]byte, error) {
- return []byte(fmt.Sprintf(findmntByVolumeRes, "filesystemvolume", fileSystemPath)), nil
+ return []byte(fmt.Sprintf(findmntByVolumeRes, "filesystemvolume", unsafepath.UnsafeAbsolute(fileSystemPath.Raw()))), nil
}
- diskFile := filepath.Join(fileSystemPath, "disk.img")
- _, err = os.Create(diskFile)
+ diskFile, err := newFile(unsafepath.UnsafeAbsolute(fileSystemPath.Raw()), "disk.img")
Expect(err).ToNot(HaveOccurred())
- mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) {
- Expect(os.MkdirAll(deviceName, 0755)).To(Succeed())
- return []byte("Yay"), nil
+ mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error {
+ _, err := newFile(unsafepath.UnsafeAbsolute(basePath.Raw()), deviceName)
+ Expect(err).ToNot(HaveOccurred())
+ return nil
}
+
blockVolume := filepath.Join(targetPodPath, "blockvolume")
targetFilePath := filepath.Join(targetPodPath, "filesystemvolume.img")
- mountCommand = func(sourcePath, targetPath string) ([]byte, error) {
- Expect(sourcePath).To(Equal(filepath.Join(fileSystemPath, "disk.img")))
- Expect(targetPath).To(Equal(targetFilePath))
+ isBlockDevice = func(path *safepath.Path) (bool, error) {
+ return strings.Contains(unsafepath.UnsafeAbsolute(path.Raw()), "blockvolume"), nil
+ }
+ mountCommand = func(sourcePath, targetPath *safepath.Path) ([]byte, error) {
+ Expect(unsafepath.UnsafeRelative(sourcePath.Raw())).To(Equal(unsafepath.UnsafeAbsolute(diskFile.Raw())))
+ Expect(unsafepath.UnsafeAbsolute(targetPath.Raw())).To(Equal(targetFilePath))
return []byte("Success"), nil
}
expectedPaths := []string{targetFilePath, blockVolume}
capturedPaths := []string{}
- ownershipManager.EXPECT().SetFileOwnership(gomock.Any()).Times(2).DoAndReturn(func(path string) error {
- capturedPaths = append(capturedPaths, path)
+ ownershipManager.EXPECT().SetFileOwnership(gomock.Any()).Times(2).DoAndReturn(func(path *safepath.Path) error {
+ capturedPaths = append(capturedPaths, unsafepath.UnsafeAbsolute(path.Raw()))
return nil
})
@@ -922,12 +917,9 @@ var _ = Describe("HotplugVolume", func() {
It("unmountAll should cleanup regardless of vmi volumestatuses", func() {
setExpectedCgroupRuns(2)
- log.DefaultLogger().Infof("tempdir: %s", tempDir)
+
sourcePodUID := types.UID("klmno")
volumeStatuses := make([]v1.VolumeStatus, 0)
- mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) {
- return []byte("Yay"), nil
- }
volumeStatuses = append(volumeStatuses, v1.VolumeStatus{
Name: "permanent",
})
@@ -954,43 +946,53 @@ var _ = Describe("HotplugVolume", func() {
},
})
vmi.Status.VolumeStatus = volumeStatuses
- deviceBasePath = func(sourceUID types.UID) string {
- return filepath.Join(tempDir, string(sourceUID), "volumeDevices")
+ deviceBasePath = func(podUID types.UID) (*safepath.Path, error) {
+ return newDir(tempDir, string(podUID), "volumeDevices")
}
- blockDevicePath := filepath.Join(tempDir, string(sourcePodUID), "volumeDevices", "blockvolume")
- fileSystemPath := filepath.Join(tempDir, string(sourcePodUID), "volumes")
- err = os.MkdirAll(blockDevicePath, 0755)
+ blockDevicePath, err := newDir(tempDir, string(sourcePodUID), "volumeDevices", "blockvolume")
Expect(err).ToNot(HaveOccurred())
- err = os.MkdirAll(fileSystemPath, 0755)
+ fileSystemPath, err := newDir(tempDir, string(sourcePodUID), "volumes")
Expect(err).ToNot(HaveOccurred())
- findMntByVolume = func(volumeName string, pid int) ([]byte, error) {
- return []byte(fmt.Sprintf(findmntByVolumeRes, "filesystemvolume", fileSystemPath)), nil
- }
- deviceFile := filepath.Join(blockDevicePath, "file")
- err = ioutil.WriteFile(deviceFile, []byte("test"), 0644)
+ deviceFile, err := newFile(unsafepath.UnsafeAbsolute(blockDevicePath.Raw()), "blockvolumefile")
+ Expect(err).ToNot(HaveOccurred())
+ err = ioutil.WriteFile(unsafepath.UnsafeAbsolute(deviceFile.Raw()), []byte("test"), 0644)
Expect(err).ToNot(HaveOccurred())
- sourcePodBasePath = func(podUID types.UID) string {
+
+ sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) {
if podUID == sourcePodUID {
- return blockDevicePath
+ return blockDevicePath, nil
}
- return fileSystemPath
+ return fileSystemPath, nil
}
- diskFile := filepath.Join(fileSystemPath, "disk.img")
- _, err = os.Create(diskFile)
+ findMntByVolume = func(volumeName string, pid int) ([]byte, error) {
+ return []byte(fmt.Sprintf(findmntByVolumeRes, "filesystemvolume", unsafepath.UnsafeAbsolute(fileSystemPath.Raw()))), nil
+ }
+
+ diskFile, err := newFile(unsafepath.UnsafeAbsolute(fileSystemPath.Raw()), "disk.img")
Expect(err).ToNot(HaveOccurred())
+ mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error {
+ _, err := newFile(unsafepath.UnsafeAbsolute(basePath.Raw()), deviceName)
+ Expect(err).ToNot(HaveOccurred())
+ return nil
+ }
+
blockVolume := filepath.Join(targetPodPath, "blockvolume")
targetFilePath := filepath.Join(targetPodPath, "filesystemvolume.img")
- mountCommand = func(sourcePath, targetPath string) ([]byte, error) {
- Expect(sourcePath).To(Equal(filepath.Join(fileSystemPath, "disk.img")))
- Expect(targetPath).To(Equal(targetFilePath))
+ isBlockDevice = func(path *safepath.Path) (bool, error) {
+ return strings.Contains(unsafepath.UnsafeAbsolute(path.Raw()), "blockvolume"), nil
+ }
+ mountCommand = func(sourcePath, targetPath *safepath.Path) ([]byte, error) {
+ Expect(unsafepath.UnsafeRelative(sourcePath.Raw())).To(Equal(unsafepath.UnsafeAbsolute(diskFile.Raw())))
+ Expect(unsafepath.UnsafeAbsolute(targetPath.Raw())).To(Equal(targetFilePath))
return []byte("Success"), nil
}
- expectedPaths := []string{blockVolume, targetFilePath}
+ expectedPaths := []string{targetFilePath, blockVolume}
capturedPaths := []string{}
- ownershipManager.EXPECT().SetFileOwnership(gomock.Any()).Times(2).DoAndReturn(func(path string) error {
- capturedPaths = append(capturedPaths, path)
+
+ ownershipManager.EXPECT().SetFileOwnership(gomock.Any()).Times(2).DoAndReturn(func(path *safepath.Path) error {
+ capturedPaths = append(capturedPaths, unsafepath.UnsafeAbsolute(path.Raw()))
return nil
})
@@ -1043,7 +1045,7 @@ func (i *mockIsolationDetector) Detect(_ *v1.VirtualMachineInstance) (isolation.
}
func (i *mockIsolationDetector) DetectForSocket(_ *v1.VirtualMachineInstance, _ string) (isolation.IsolationResult, error) {
- if i.pid == 1 {
+ if i.pid != 9999 {
return isolation.NewIsolationResult(i.pid, i.ppid), nil
}
return nil, fmt.Errorf("isolation error")
@@ -1056,3 +1058,66 @@ func (i *mockIsolationDetector) Allowlist(_ []string) isolation.PodIsolationDete
func (i *mockIsolationDetector) AdjustResources(_ *v1.VirtualMachineInstance) error {
return nil
}
+
+func newFile(baseDir string, elems ...string) (*safepath.Path, error) {
+ targetPath := filepath.Join(append([]string{baseDir}, elems...)...)
+ err := os.MkdirAll(filepath.Dir(targetPath), os.ModePerm)
+ if err != nil {
+ return nil, err
+ }
+ f, err := os.Create(targetPath)
+ if err != nil {
+ return nil, err
+ }
+ f.Close()
+ return safepath.JoinAndResolveWithRelativeRoot(baseDir, elems...)
+}
+func newDir(baseDir string, elems ...string) (*safepath.Path, error) {
+ targetPath := filepath.Join(append([]string{baseDir}, elems...)...)
+ err := os.MkdirAll(targetPath, os.ModePerm)
+ if err != nil {
+ return nil, err
+ }
+ return safepath.JoinAndResolveWithRelativeRoot(baseDir, elems...)
+}
+
+func fakeStat(isDevice bool, mode os.FileMode, dev uint64) os.FileInfo {
+ return fakeFileInfo{isDevice: isDevice, mode: mode, dev: dev}
+}
+
+type fakeFileInfo struct {
+ isDevice bool
+ mode os.FileMode
+ dev uint64
+}
+
+func (f fakeFileInfo) Name() string {
+ //TODO implement me
+ panic("implement me")
+}
+
+func (f fakeFileInfo) Size() int64 {
+ //TODO implement me
+ panic("implement me")
+}
+
+func (f fakeFileInfo) Mode() fs.FileMode {
+ if f.isDevice {
+ return f.mode | fs.ModeDevice
+ }
+ return f.mode
+}
+
+func (f fakeFileInfo) ModTime() time.Time {
+ //TODO implement me
+ panic("implement me")
+}
+
+func (f fakeFileInfo) IsDir() bool {
+ //TODO implement me
+ panic("implement me")
+}
+
+func (f fakeFileInfo) Sys() interface{} {
+ return &syscall.Stat_t{Rdev: f.dev}
+}
--
2.37.1
From 056632932acf5740ff34487384f411dbb82c4066 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:29:46 +0200
Subject: [PATCH 13/20] Let virt-handler set up required devices with safepath
operations
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit ddb060e2ed8d582191eb634b56655b10d109e045)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
cmd/virt-handler/BUILD.bazel | 1 +
cmd/virt-handler/virt-handler.go | 14 +++++++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/cmd/virt-handler/BUILD.bazel b/cmd/virt-handler/BUILD.bazel
index fa4fa0c5c..99f8115be 100644
--- a/cmd/virt-handler/BUILD.bazel
+++ b/cmd/virt-handler/BUILD.bazel
@@ -17,6 +17,7 @@ go_library(
"//pkg/monitoring/profiler:go_default_library",
"//pkg/monitoring/reflector/prometheus:go_default_library",
"//pkg/monitoring/workqueue/prometheus:go_default_library",
+ "//pkg/safepath:go_default_library",
"//pkg/service:go_default_library",
"//pkg/util:go_default_library",
"//pkg/util/ratelimiter:go_default_library",
diff --git a/cmd/virt-handler/virt-handler.go b/cmd/virt-handler/virt-handler.go
index 7123d51ac..5ded05de3 100644
--- a/cmd/virt-handler/virt-handler.go
+++ b/cmd/virt-handler/virt-handler.go
@@ -45,6 +45,8 @@ import (
"k8s.io/client-go/util/certificate"
"k8s.io/client-go/util/flowcontrol"
+ "kubevirt.io/kubevirt/pkg/safepath"
+
"kubevirt.io/kubevirt/pkg/util/ratelimiter"
"kubevirt.io/kubevirt/pkg/virt-handler/node-labeller/api"
@@ -387,7 +389,17 @@ func (app *virtHandlerApp) Run() {
// relabel tun device
unprivilegedContainerSELinuxLabel := "system_u:object_r:container_file_t:s0"
- err = selinux.RelabelFiles(unprivilegedContainerSELinuxLabel, se.IsPermissive(), "/dev/net/tun", "/dev/null")
+
+ devTun, err := safepath.JoinAndResolveWithRelativeRoot("/", "/dev/net/tun")
+ if err != nil {
+ panic(err)
+ }
+ devNull, err := safepath.JoinAndResolveWithRelativeRoot("/", "/dev/null")
+ if err != nil {
+ panic(err)
+ }
+
+ err = selinux.RelabelFiles(unprivilegedContainerSELinuxLabel, se.IsPermissive(), devTun, devNull)
if err != nil {
panic(fmt.Errorf("error relabeling required files: %v", err))
}
--
2.37.1
From 2649b302d4a78e1da343ce2e2bcc8c1f4af91b32 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 19 Jul 2022 11:30:25 +0200
Subject: [PATCH 14/20] Move virt-handler controller over to safepath usage
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 11120f94acbdd0110529b9e046d9881cde66bca2)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/virt-handler/BUILD.bazel | 2 +-
pkg/virt-handler/vm.go | 69 ++++++++++++++++++++++++++----------
pkg/virt-handler/vm_test.go | 13 +++++--
3 files changed, 63 insertions(+), 21 deletions(-)
diff --git a/pkg/virt-handler/BUILD.bazel b/pkg/virt-handler/BUILD.bazel
index 0a0aefa3b..07493c9b1 100644
--- a/pkg/virt-handler/BUILD.bazel
+++ b/pkg/virt-handler/BUILD.bazel
@@ -40,7 +40,6 @@ go_library(
"//pkg/virt-handler/isolation:go_default_library",
"//pkg/virt-handler/migration-proxy:go_default_library",
"//pkg/virt-handler/node-labeller/api:go_default_library",
- "//pkg/virt-handler/selinux:go_default_library",
"//pkg/virt-launcher/virtwrap/api:go_default_library",
"//pkg/watchdog:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
@@ -76,6 +75,7 @@ go_test(
"//pkg/handler-launcher-com/cmd/v1:go_default_library",
"//pkg/network/cache:go_default_library",
"//pkg/network/errors:go_default_library",
+ "//pkg/safepath:go_default_library",
"//pkg/testutils:go_default_library",
"//pkg/util:go_default_library",
"//pkg/virt-config:go_default_library",
diff --git a/pkg/virt-handler/vm.go b/pkg/virt-handler/vm.go
index c4cad5cae..425cd5955 100644
--- a/pkg/virt-handler/vm.go
+++ b/pkg/virt-handler/vm.go
@@ -34,6 +34,7 @@ import (
"time"
hotplugdisk "kubevirt.io/kubevirt/pkg/hotplug-disk"
+ "kubevirt.io/kubevirt/pkg/safepath"
"kubevirt.io/kubevirt/pkg/virt-handler/cgroup"
"kubevirt.io/kubevirt/pkg/config"
@@ -382,20 +383,24 @@ func (d *VirtualMachineController) startDomainNotifyPipe(domainPipeStopChan chan
}
// inject the domain-notify.sock into the VMI pod.
- socketPath := filepath.Join(res.MountRoot(), d.virtShareDir, "domain-notify-pipe.sock")
-
- os.RemoveAll(socketPath)
- err = util.MkdirAllWithNosec(filepath.Dir(socketPath))
+ root, err := res.MountRoot()
+ if err != nil {
+ return err
+ }
+ socketDir, err := root.AppendAndResolveWithRelativeRoot(d.virtShareDir)
if err != nil {
- log.Log.Reason(err).Error("unable to create directory for unix socket")
return err
}
- listener, err := net.Listen("unix", socketPath)
+ listener, err := safepath.ListenUnixNoFollow(socketDir, "domain-notify-pipe.sock")
if err != nil {
log.Log.Reason(err).Error("failed to create unix socket for proxy service")
return err
}
+ socketPath, err := safepath.JoinNoFollow(socketDir, "domain-notify-pipe.sock")
+ if err != nil {
+ return err
+ }
if util.IsNonRootVMI(vmi) {
err := diskutils.DefaultOwnershipManager.SetFileOwnership(socketPath)
@@ -507,7 +512,10 @@ func (d *VirtualMachineController) setupNetwork(vmi *v1.VirtualMachineInstance)
if err != nil {
return fmt.Errorf(failedDetectIsolationFmt, err)
}
- rootMount := isolationRes.MountRoot()
+ rootMount, err := isolationRes.MountRoot()
+ if err != nil {
+ return err
+ }
return d.netConf.Setup(vmi, isolationRes.Pid(), func() error {
if virtutil.WantVirtioNetDevice(vmi) {
@@ -1105,16 +1113,30 @@ func (d *VirtualMachineController) updateIsoSizeStatus(vmi *v1.VirtualMachineIns
}
for _, volume := range vmi.Spec.Volumes {
+
volPath, found := IsoGuestVolumePath(vmi, &volume)
if !found {
continue
}
+
res, err := d.podIsolationDetector.Detect(vmi)
if err != nil {
- log.DefaultLogger().V(2).Warningf("failed to detect VMI %s", vmi.Name)
+ log.DefaultLogger().V(2).Reason(err).Warningf("failed to detect VMI %s", vmi.Name)
+ continue
+ }
+
+ rootPath, err := res.MountRoot()
+ if err != nil {
+ log.DefaultLogger().V(2).Reason(err).Warningf("failed to detect VMI %s", vmi.Name)
+ continue
+ }
+
+ safeVolPath, err := rootPath.AppendAndResolveWithRelativeRoot(volPath)
+ if err != nil {
+ log.DefaultLogger().V(2).Warningf("failed to determine file size for volume %s", volPath)
continue
}
- size, err := isolation.GetFileSize(volPath, res)
+ fileInfo, err := safepath.StatAtNoFollow(safeVolPath)
if err != nil {
log.DefaultLogger().V(2).Warningf("failed to determine file size for volume %s", volPath)
continue
@@ -1122,7 +1144,7 @@ func (d *VirtualMachineController) updateIsoSizeStatus(vmi *v1.VirtualMachineIns
for i, _ := range vmi.Status.VolumeStatus {
if vmi.Status.VolumeStatus[i].Name == volume.Name {
- vmi.Status.VolumeStatus[i].Size = size
+ vmi.Status.VolumeStatus[i].Size = fileInfo.Size()
continue
}
}
@@ -2475,7 +2497,10 @@ func (d *VirtualMachineController) vmUpdateHelperMigrationTarget(origVMI *v1.Vir
if err != nil {
return fmt.Errorf(failedDetectIsolationFmt, err)
}
- virtLauncherRootMount := isolationRes.MountRoot()
+ virtLauncherRootMount, err := isolationRes.MountRoot()
+ if err != nil {
+ return err
+ }
err = d.claimDeviceOwnership(virtLauncherRootMount, "kvm")
if err != nil {
@@ -2560,7 +2585,10 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach
if err != nil {
return fmt.Errorf(failedDetectIsolationFmt, err)
}
- virtLauncherRootMount := isolationRes.MountRoot()
+ virtLauncherRootMount, err := isolationRes.MountRoot()
+ if err != nil {
+ return err
+ }
err = d.claimDeviceOwnership(virtLauncherRootMount, "kvm")
if err != nil {
@@ -2578,7 +2606,10 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach
}
if virtutil.IsSEVVMI(vmi) {
- sevDevice := path.Join(virtLauncherRootMount, "dev", "sev")
+ sevDevice, err := safepath.JoinNoFollow(virtLauncherRootMount, filepath.Join("dev", "sev"))
+ if err != nil {
+ return err
+ }
if err := diskutils.DefaultOwnershipManager.SetFileOwnership(sevDevice); err != nil {
return fmt.Errorf("failed to set SEV device owner: %v", err)
}
@@ -2904,11 +2935,13 @@ func (d *VirtualMachineController) isHostModelMigratable(vmi *v1.VirtualMachineI
return nil
}
-func (d *VirtualMachineController) claimDeviceOwnership(virtLauncherRootMount, deviceName string) error {
- devicePath := filepath.Join(virtLauncherRootMount, "dev", deviceName)
-
- softwareEmulation, err := util.UseSoftwareEmulationForDevice(devicePath, d.clusterConfig.AllowEmulation())
- if err != nil || softwareEmulation {
+func (d *VirtualMachineController) claimDeviceOwnership(virtLauncherRootMount *safepath.Path, deviceName string) error {
+ softwareEmulation := d.clusterConfig.AllowEmulation()
+ devicePath, err := safepath.JoinNoFollow(virtLauncherRootMount, filepath.Join("dev", deviceName))
+ if err != nil {
+ if softwareEmulation {
+ return nil
+ }
return err
}
diff --git a/pkg/virt-handler/vm_test.go b/pkg/virt-handler/vm_test.go
index 06f7b44fd..1da5f688e 100644
--- a/pkg/virt-handler/vm_test.go
+++ b/pkg/virt-handler/vm_test.go
@@ -32,6 +32,8 @@ import (
"k8s.io/utils/pointer"
+ "kubevirt.io/kubevirt/pkg/safepath"
+
k8sruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/testing"
@@ -181,9 +183,16 @@ var _ = Describe("VirtualMachineInstance", func() {
mockGracefulShutdown = &MockGracefulShutdown{shareDir}
config, _, _ := testutils.NewFakeClusterConfigUsingKVConfig(&v1.KubeVirtConfiguration{})
+ Expect(os.MkdirAll(filepath.Join(vmiShareDir, "dev"), 0755)).To(Succeed())
+ f, err := os.OpenFile(filepath.Join(vmiShareDir, "dev", "kvm"), os.O_CREATE, 0755)
+ Expect(err).ToNot(HaveOccurred())
+ f.Close()
+
mockIsolationResult = isolation.NewMockIsolationResult(ctrl)
mockIsolationResult.EXPECT().Pid().Return(1).AnyTimes()
- mockIsolationResult.EXPECT().MountRoot().Return(vmiShareDir).AnyTimes()
+ rootDir, err := safepath.JoinAndResolveWithRelativeRoot(vmiShareDir)
+ Expect(err).ToNot(HaveOccurred())
+ mockIsolationResult.EXPECT().MountRoot().Return(rootDir, nil).AnyTimes()
mockIsolationDetector = isolation.NewMockPodIsolationDetector(ctrl)
mockIsolationDetector.EXPECT().Detect(gomock.Any()).Return(mockIsolationResult, nil).AnyTimes()
@@ -221,7 +230,7 @@ var _ = Describe("VirtualMachineInstance", func() {
podTestUUID = uuid.NewUUID()
sockFile = cmdclient.SocketFilePathOnHost(string(podTestUUID))
Expect(os.MkdirAll(filepath.Dir(sockFile), 0755)).To(Succeed())
- f, err := os.Create(sockFile)
+ f, err = os.Create(sockFile)
Expect(err).ToNot(HaveOccurred())
f.Close()
--
2.37.1
From 3b026a0a3895effed983eb9efca6c9e1024d40f7 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Mon, 25 Jul 2022 12:54:15 +0200
Subject: [PATCH 15/20] Ensure compatibility with old mount paths of old
KubeVirt installs
virt-handler needs to detect if mount record files contain old records
which may not have been resolved yet. In that case it has to resolve
them once to ensure that that symlinks, which were valid until now,
would not break unmount operations on already running VMIs.
After an initial load and write, the paths are clean and safe to use.
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 5cbc2736c8655c8f500b035fb37b08f54cec9aee)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
pkg/virt-handler/container-disk/mount.go | 17 +++++++++++++++++
pkg/virt-handler/hotplug-disk/mount.go | 20 +++++++++++++++++++-
pkg/virt-handler/hotplug-disk/mount_test.go | 4 +++-
3 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/pkg/virt-handler/container-disk/mount.go b/pkg/virt-handler/container-disk/mount.go
index 710bd51c3..d28fe46fd 100644
--- a/pkg/virt-handler/container-disk/mount.go
+++ b/pkg/virt-handler/container-disk/mount.go
@@ -58,6 +58,7 @@ type vmiMountTargetEntry struct {
type vmiMountTargetRecord struct {
MountTargetEntries []vmiMountTargetEntry `json:"mountTargetEntries"`
+ UsesSafePaths bool `json:"usesSafePaths"`
}
func NewMounter(isoDetector isolation.PodIsolationDetector, mountStateDir string, clusterConfig *virtconfig.ClusterConfig) Mounter {
@@ -142,6 +143,19 @@ func (m *mounter) getMountTargetRecord(vmi *v1.VirtualMachineInstance) (*vmiMoun
return nil, err
}
+ // XXX: backward compatibility for old unresolved paths, can be removed in July 2023
+ // After a one-time convert and persist, old records are safe too.
+ if !record.UsesSafePaths {
+ record.UsesSafePaths = true
+ for i, entry := range record.MountTargetEntries {
+ safePath, err := safepath.JoinAndResolveWithRelativeRoot("/", entry.TargetFile)
+ if err != nil {
+ return nil, fmt.Errorf("failed converting legacy path to safepath: %v", err)
+ }
+ record.MountTargetEntries[i].TargetFile = unsafepath.UnsafeAbsolute(safePath.Raw())
+ }
+ }
+
m.mountRecords[vmi.UID] = &record
return &record, nil
}
@@ -162,6 +176,9 @@ func (m *mounter) setAddMountTargetRecordHelper(vmi *v1.VirtualMachineInstance,
if string(vmi.UID) == "" {
return fmt.Errorf("unable to set container disk mounted directories for vmi without uid")
}
+ // XXX: backward compatibility for old unresolved paths, can be removed in July 2023
+ // After a one-time convert and persist, old records are safe too.
+ record.UsesSafePaths = true
recordFile := filepath.Join(m.mountStateDir, string(vmi.UID))
fileExists, err := diskutils.FileExists(recordFile)
diff --git a/pkg/virt-handler/hotplug-disk/mount.go b/pkg/virt-handler/hotplug-disk/mount.go
index b80bad033..a0a35f1d4 100644
--- a/pkg/virt-handler/hotplug-disk/mount.go
+++ b/pkg/virt-handler/hotplug-disk/mount.go
@@ -155,6 +155,7 @@ type vmiMountTargetEntry struct {
type vmiMountTargetRecord struct {
MountTargetEntries []vmiMountTargetEntry `json:"mountTargetEntries"`
+ UsesSafePaths bool `json:"usesSafePaths"`
}
// NewVolumeMounter creates a new VolumeMounter
@@ -234,12 +235,25 @@ func (m *volumeMounter) getMountTargetRecord(vmi *v1.VirtualMachineInstance) (*v
return nil, err
}
+ // XXX: backward compatibility for old unresolved paths, can be removed in July 2023
+ // After a one-time convert and persist, old records are safe too.
+ if !record.UsesSafePaths {
+ for i, path := range record.MountTargetEntries {
+ record.UsesSafePaths = true
+ safePath, err := safepath.JoinAndResolveWithRelativeRoot("/", path.TargetFile)
+ if err != nil {
+ return nil, fmt.Errorf("failed converting legacy path to safepath: %v", err)
+ }
+ record.MountTargetEntries[i].TargetFile = unsafepath.UnsafeAbsolute(safePath.Raw())
+ }
+ }
+
m.mountRecords[vmi.UID] = &record
return &record, nil
}
// not found
- return &vmiMountTargetRecord{}, nil
+ return &vmiMountTargetRecord{UsesSafePaths: true}, nil
}
func (m *volumeMounter) setMountTargetRecord(vmi *v1.VirtualMachineInstance, record *vmiMountTargetRecord) error {
@@ -247,6 +261,10 @@ func (m *volumeMounter) setMountTargetRecord(vmi *v1.VirtualMachineInstance, rec
return fmt.Errorf(unableFindHotplugMountedDir)
}
+ // XXX: backward compatibility for old unresolved paths, can be removed in July 2023
+ // After a one-time convert and persist, old records are safe too.
+ record.UsesSafePaths = true
+
recordFile := filepath.Join(m.mountStateDir, string(vmi.UID))
m.mountRecordsLock.Lock()
diff --git a/pkg/virt-handler/hotplug-disk/mount_test.go b/pkg/virt-handler/hotplug-disk/mount_test.go
index 461315f34..980eeb7e7 100644
--- a/pkg/virt-handler/hotplug-disk/mount_test.go
+++ b/pkg/virt-handler/hotplug-disk/mount_test.go
@@ -229,7 +229,7 @@ var _ = Describe("HotplugVolume", func() {
Expect(err).ToNot(HaveOccurred())
res, err := m.getMountTargetRecord(vmi)
Expect(err).ToNot(HaveOccurred())
- Expect(res).To(Equal(&vmiMountTargetRecord{}))
+ Expect(res).To(Equal(&vmiMountTargetRecord{UsesSafePaths: true}))
})
It("deleteMountTargetRecord should remove both record file and entry file", func() {
@@ -879,6 +879,7 @@ var _ = Describe("HotplugVolume", func() {
TargetFile: blockVolume,
},
},
+ UsesSafePaths: true,
}
expectedBytes, err := json.Marshal(record)
Expect(err).ToNot(HaveOccurred())
@@ -1009,6 +1010,7 @@ var _ = Describe("HotplugVolume", func() {
TargetFile: blockVolume,
},
},
+ UsesSafePaths: true,
}
expectedBytes, err := json.Marshal(record)
Expect(err).ToNot(HaveOccurred())
--
2.37.1
From 0a76d3f9ad8d4ea1b9a508dacbac3f0bbff697fa Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Thu, 21 Jul 2022 12:44:53 +0200
Subject: [PATCH 16/20] Temoprary fork the banncheck analyzer to allow
embedding the analyzer config
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit fe187d33947cc1ed72c7c1b064a18e7169a1be25)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
.../analyzers/banncheck/banncheck/BUILD.bazel | 12 ++
.../banncheck/banncheck/banned_api.go | 138 ++++++++++++++++++
tools/analyzers/banncheck/config/BUILD.bazel | 8 +
tools/analyzers/banncheck/config/config.go | 91 ++++++++++++
4 files changed, 249 insertions(+)
create mode 100644 tools/analyzers/banncheck/banncheck/BUILD.bazel
create mode 100644 tools/analyzers/banncheck/banncheck/banned_api.go
create mode 100644 tools/analyzers/banncheck/config/BUILD.bazel
create mode 100644 tools/analyzers/banncheck/config/config.go
diff --git a/tools/analyzers/banncheck/banncheck/BUILD.bazel b/tools/analyzers/banncheck/banncheck/BUILD.bazel
new file mode 100644
index 000000000..c5166c8e9
--- /dev/null
+++ b/tools/analyzers/banncheck/banncheck/BUILD.bazel
@@ -0,0 +1,12 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+ name = "go_default_library",
+ srcs = ["banned_api.go"],
+ importpath = "kubevirt.io/kubevirt/tools/analyzers/banncheck/banncheck",
+ visibility = ["//visibility:public"],
+ deps = [
+ "//tools/analyzers/banncheck/config:go_default_library",
+ "@org_golang_x_tools//go/analysis:go_default_library",
+ ],
+)
diff --git a/tools/analyzers/banncheck/banncheck/banned_api.go b/tools/analyzers/banncheck/banncheck/banned_api.go
new file mode 100644
index 000000000..cd6b8af7c
--- /dev/null
+++ b/tools/analyzers/banncheck/banncheck/banned_api.go
@@ -0,0 +1,138 @@
+// Copyright 2020 Google LLC
+//
+// 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
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package bannedapi provides the tools for doing static analysis
+// and checking for usage of banned APIs.
+
+// TODO: Temporary fork of banncheck from https://github.com/google/go-safeweb/tree/eb79df54b8bc1910ac3bc3fc3328da7c0fb016e1/cmd/bancheck
+package bannedapi
+
+import (
+ "errors"
+ "flag"
+ "fmt"
+ "go/token"
+ "go/types"
+ "path/filepath"
+ "strings"
+
+ "kubevirt.io/kubevirt/tools/analyzers/banncheck/config"
+
+ "golang.org/x/tools/go/analysis"
+)
+
+// NewAnalyzer returns an analyzer that checks for usage of banned APIs.
+func NewAnalyzer() *analysis.Analyzer {
+ fs := flag.NewFlagSet("", flag.ExitOnError)
+ fs.String("configs", "", "Config files with banned APIs separated by a comma")
+
+ a := &analysis.Analyzer{
+ Name: "bannedAPI",
+ Doc: "Checks for usage of banned APIs",
+ Run: checkBannedAPIs,
+ Flags: *fs,
+ }
+
+ return a
+}
+
+func checkBannedAPIs(pass *analysis.Pass) (interface{}, error) {
+ cfgFiles := pass.Analyzer.Flags.Lookup("configs").Value.String()
+ if cfgFiles == "" {
+ return nil, errors.New("missing config files")
+ }
+
+ cfg, err := config.ReadConfigs(strings.Split(cfgFiles, ","))
+ if err != nil {
+ return nil, err
+ }
+
+ checkBannedImports(pass, bannedAPIMap(cfg.Imports))
+ checkBannedFunctions(pass, bannedAPIMap(cfg.Functions))
+
+ return nil, nil
+}
+
+func checkBannedImports(pass *analysis.Pass, bannedImports map[string][]config.BannedAPI) (interface{}, error) {
+ for _, f := range pass.Files {
+ for _, i := range f.Imports {
+ importName := strings.Trim(i.Path.Value, "\"")
+ err := reportIfBanned(importName, bannedImports, i.Pos(), pass)
+ if err != nil {
+ return false, err
+ }
+ }
+ }
+ return nil, nil
+}
+
+func checkBannedFunctions(pass *analysis.Pass, bannedFns map[string][]config.BannedAPI) (interface{}, error) {
+ for id, obj := range pass.TypesInfo.Uses {
+ fn, ok := obj.(*types.Func)
+ if !ok {
+ continue
+ }
+
+ fnName := fmt.Sprintf("%s.%s", fn.Pkg().Path(), fn.Name())
+ err := reportIfBanned(fnName, bannedFns, id.Pos(), pass)
+ if err != nil {
+ return false, err
+ }
+ }
+ return nil, nil
+}
+
+func reportIfBanned(apiName string, bannedAPIs map[string][]config.BannedAPI, position token.Pos, pass *analysis.Pass) error {
+ for _, banCfg := range bannedAPIs[apiName] {
+ if apiName != banCfg.Name {
+ return nil
+ }
+ pkgAllowed, err := isPkgAllowed(pass.Pkg, banCfg)
+ if err != nil {
+ return err
+ }
+ if pkgAllowed {
+ continue
+ }
+ pass.Report(analysis.Diagnostic{
+ Pos: position,
+ Message: fmt.Sprintf("Banned API found %q. Additional info: %s", apiName, banCfg.Msg),
+ })
+ }
+ return nil
+}
+
+// isPkgAllowed checks if the Go package should be exempted from reporting banned API usages.
+func isPkgAllowed(pkg *types.Package, bannedAPI config.BannedAPI) (bool, error) {
+ for _, e := range bannedAPI.Exemptions {
+ match, err := filepath.Match(e.AllowedPkg, pkg.Path())
+ if err != nil {
+ return false, err
+ }
+ if match {
+ return true, nil
+ }
+ }
+ return false, nil
+}
+
+// bannedAPIMap builds a mapping of fully qualified API name to a list of
+// all its config.BannedAPI entries.
+func bannedAPIMap(bannedAPIs []config.BannedAPI) map[string][]config.BannedAPI {
+ m := make(map[string][]config.BannedAPI)
+ for _, API := range bannedAPIs {
+ m[API.Name] = append(m[API.Name], API)
+ }
+ return m
+}
diff --git a/tools/analyzers/banncheck/config/BUILD.bazel b/tools/analyzers/banncheck/config/BUILD.bazel
new file mode 100644
index 000000000..8076cbb79
--- /dev/null
+++ b/tools/analyzers/banncheck/config/BUILD.bazel
@@ -0,0 +1,8 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+ name = "go_default_library",
+ srcs = ["config.go"],
+ importpath = "kubevirt.io/kubevirt/tools/analyzers/banncheck/config",
+ visibility = ["//visibility:public"],
+)
diff --git a/tools/analyzers/banncheck/config/config.go b/tools/analyzers/banncheck/config/config.go
new file mode 100644
index 000000000..366c1ad7b
--- /dev/null
+++ b/tools/analyzers/banncheck/config/config.go
@@ -0,0 +1,91 @@
+// Copyright 2020 Google LLC
+//
+// 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
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// TODO: Temporary fork of banncheck from https://github.com/google/go-safeweb/tree/eb79df54b8bc1910ac3bc3fc3328da7c0fb016e1/cmd/bancheck
+package config
+
+import (
+ "encoding/json"
+ "errors"
+ "os"
+)
+
+// Config represents a configuration passed to the linter.
+type Config struct {
+ Imports []BannedAPI `json:"imports"`
+ Functions []BannedAPI `json:"functions"`
+}
+
+// BannedAPI represents an identifier (e.g. import, function call) that should not be used.
+type BannedAPI struct {
+ Name string `json:"name"` // fully qualified identifier name
+ Msg string `json:"msg"` // additional information e.g. rationale for banning
+ Exemptions []Exemption `json:"exemptions"`
+}
+
+// Exemption represents a location that should be exempted from checking for banned APIs.
+type Exemption struct {
+ Justification string `json:"justification"`
+ AllowedPkg string `json:"allowedPkg"` // Uses Go RegExp https://golang.org/pkg/regexp/syntax
+}
+
+// ReadConfigs reads banned APIs from all files.
+func ReadConfigs(files []string) (*Config, error) {
+ var imports []BannedAPI
+ var fns []BannedAPI
+
+ for _, file := range files {
+ config, err := readCfg(file)
+ if err != nil {
+ return nil, err
+ }
+
+ imports = append(imports, config.Imports...)
+ fns = append(fns, config.Functions...)
+ }
+
+ return &Config{Imports: imports, Functions: fns}, nil
+}
+
+func readCfg(filename string) (*Config, error) {
+ f, err := openFile(filename)
+ if err != nil {
+ return nil, err
+ }
+ defer f.Close()
+
+ return decodeCfg(f)
+}
+
+func openFile(filename string) (*os.File, error) {
+ info, err := os.Stat(filename)
+ if os.IsNotExist(err) {
+ return nil, errors.New("file does not exist")
+ }
+ if info.IsDir() {
+ return nil, errors.New("file is a directory")
+ }
+
+ return os.Open(filename)
+}
+
+func decodeCfg(f *os.File) (*Config, error) {
+ var cfg Config
+ err := json.NewDecoder(f).Decode(&cfg)
+ if err != nil {
+ return nil, err
+ }
+
+ return &cfg, nil
+}
--
2.37.1
From a2a23176279a60dc3c81de17e195ca0113d3d217 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Thu, 21 Jul 2022 12:47:12 +0200
Subject: [PATCH 17/20] The name of the builtin package is not populated
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit 17fa7de73f4e7809aaa9f2313aee6660925eaf03)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
tools/analyzers/banncheck/banncheck/banned_api.go | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/analyzers/banncheck/banncheck/banned_api.go b/tools/analyzers/banncheck/banncheck/banned_api.go
index cd6b8af7c..44219971f 100644
--- a/tools/analyzers/banncheck/banncheck/banned_api.go
+++ b/tools/analyzers/banncheck/banncheck/banned_api.go
@@ -84,7 +84,13 @@ func checkBannedFunctions(pass *analysis.Pass, bannedFns map[string][]config.Ban
continue
}
- fnName := fmt.Sprintf("%s.%s", fn.Pkg().Path(), fn.Name())
+ // "builtin" package is nil
+ pkgName := "builtin"
+ if fn.Pkg() != nil {
+ pkgName = fn.Pkg().Name()
+ }
+
+ fnName := fmt.Sprintf("%s.%s", pkgName, fn.Name())
err := reportIfBanned(fnName, bannedFns, id.Pos(), pass)
if err != nil {
return false, err
--
2.37.1
From 7beae6d84651fbd197324b119dddc37dd0e65e11 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Thu, 21 Jul 2022 12:52:17 +0200
Subject: [PATCH 18/20] Modify banncheck analyzer to be able to consume an
embedded config
Signed-off-by: Roman Mohr <rmohr@google.com>
(cherry picked from commit a1ad0315a840063188cc38b9e429524a6f75ed71)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
tools/analyzers/banncheck/BUILD.bazel | 13 +++++++
tools/analyzers/banncheck/banncheck.go | 19 ++++++++++
.../banncheck/banncheck/banned_api.go | 37 ++++++++++++-------
tools/analyzers/banncheck/config/config.go | 27 ++++++++------
4 files changed, 70 insertions(+), 26 deletions(-)
create mode 100644 tools/analyzers/banncheck/BUILD.bazel
create mode 100644 tools/analyzers/banncheck/banncheck.go
diff --git a/tools/analyzers/banncheck/BUILD.bazel b/tools/analyzers/banncheck/BUILD.bazel
new file mode 100644
index 000000000..761f649f1
--- /dev/null
+++ b/tools/analyzers/banncheck/BUILD.bazel
@@ -0,0 +1,13 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+ name = "go_default_library",
+ srcs = ["banncheck.go"],
+ embedsrcs = ["banncheck.json"],
+ importpath = "kubevirt.io/kubevirt/tools/analyzers/banncheck",
+ visibility = ["//visibility:public"],
+ deps = [
+ "//tools/analyzers/banncheck/banncheck:go_default_library",
+ "@org_golang_x_tools//go/analysis:go_default_library",
+ ],
+)
diff --git a/tools/analyzers/banncheck/banncheck.go b/tools/analyzers/banncheck/banncheck.go
new file mode 100644
index 000000000..09f340ea2
--- /dev/null
+++ b/tools/analyzers/banncheck/banncheck.go
@@ -0,0 +1,19 @@
+package banncheck
+
+import (
+ "embed"
+
+ banncheck "kubevirt.io/kubevirt/tools/analyzers/banncheck/banncheck"
+
+ "golang.org/x/tools/go/analysis"
+)
+
+//go:embed banncheck.json
+var configFS embed.FS
+
+var Analyzer *analysis.Analyzer
+
+func init() {
+ Analyzer = banncheck.NewAnalyzerWithFS(configFS)
+ Analyzer.Flags.Lookup("configs").Value.Set("banncheck.json")
+}
diff --git a/tools/analyzers/banncheck/banncheck/banned_api.go b/tools/analyzers/banncheck/banncheck/banned_api.go
index 44219971f..820174506 100644
--- a/tools/analyzers/banncheck/banncheck/banned_api.go
+++ b/tools/analyzers/banncheck/banncheck/banned_api.go
@@ -24,6 +24,8 @@ import (
"fmt"
"go/token"
"go/types"
+ "io/fs"
+ "os"
"path/filepath"
"strings"
@@ -33,35 +35,42 @@ import (
)
// NewAnalyzer returns an analyzer that checks for usage of banned APIs.
-func NewAnalyzer() *analysis.Analyzer {
+func NewAnalyzer(configFS fs.FS) *analysis.Analyzer {
+ return NewAnalyzerWithFS(os.DirFS("/"))
+}
+
+// NewAnalyzer returns an analyzer that checks for usage of banned APIs.
+func NewAnalyzerWithFS(configFS fs.FS) *analysis.Analyzer {
fs := flag.NewFlagSet("", flag.ExitOnError)
fs.String("configs", "", "Config files with banned APIs separated by a comma")
a := &analysis.Analyzer{
Name: "bannedAPI",
Doc: "Checks for usage of banned APIs",
- Run: checkBannedAPIs,
+ Run: checkBannedAPIsWithFS(configFS),
Flags: *fs,
}
return a
}
-func checkBannedAPIs(pass *analysis.Pass) (interface{}, error) {
- cfgFiles := pass.Analyzer.Flags.Lookup("configs").Value.String()
- if cfgFiles == "" {
- return nil, errors.New("missing config files")
- }
+func checkBannedAPIsWithFS(configFS fs.FS) func(pass *analysis.Pass) (interface{}, error) {
+ return func(pass *analysis.Pass) (interface{}, error) {
+ cfgFiles := pass.Analyzer.Flags.Lookup("configs").Value.String()
+ if cfgFiles == "" {
+ return nil, errors.New("missing config files")
+ }
- cfg, err := config.ReadConfigs(strings.Split(cfgFiles, ","))
- if err != nil {
- return nil, err
- }
+ cfg, err := config.ReadConfigs(configFS, strings.Split(cfgFiles, ","))
+ if err != nil {
+ return nil, err
+ }
- checkBannedImports(pass, bannedAPIMap(cfg.Imports))
- checkBannedFunctions(pass, bannedAPIMap(cfg.Functions))
+ checkBannedImports(pass, bannedAPIMap(cfg.Imports))
+ checkBannedFunctions(pass, bannedAPIMap(cfg.Functions))
- return nil, nil
+ return nil, nil
+ }
}
func checkBannedImports(pass *analysis.Pass, bannedImports map[string][]config.BannedAPI) (interface{}, error) {
diff --git a/tools/analyzers/banncheck/config/config.go b/tools/analyzers/banncheck/config/config.go
index 366c1ad7b..f11c77d5e 100644
--- a/tools/analyzers/banncheck/config/config.go
+++ b/tools/analyzers/banncheck/config/config.go
@@ -18,7 +18,7 @@ package config
import (
"encoding/json"
"errors"
- "os"
+ "io/fs"
)
// Config represents a configuration passed to the linter.
@@ -41,12 +41,12 @@ type Exemption struct {
}
// ReadConfigs reads banned APIs from all files.
-func ReadConfigs(files []string) (*Config, error) {
+func ReadConfigs(configFS fs.FS, files []string) (*Config, error) {
var imports []BannedAPI
var fns []BannedAPI
for _, file := range files {
- config, err := readCfg(file)
+ config, err := readCfg(configFS, file)
if err != nil {
return nil, err
}
@@ -58,8 +58,8 @@ func ReadConfigs(files []string) (*Config, error) {
return &Config{Imports: imports, Functions: fns}, nil
}
-func readCfg(filename string) (*Config, error) {
- f, err := openFile(filename)
+func readCfg(configFS fs.FS, filename string) (*Config, error) {
+ f, err := openFile(configFS, filename)
if err != nil {
return nil, err
}
@@ -68,19 +68,22 @@ func readCfg(filename string) (*Config, error) {
return decodeCfg(f)
}
-func openFile(filename string) (*os.File, error) {
- info, err := os.Stat(filename)
- if os.IsNotExist(err) {
- return nil, errors.New("file does not exist")
+func openFile(configFS fs.FS, filename string) (fs.File, error) {
+ file, err := configFS.Open(filename)
+ if err != nil {
+ return nil, err
+ }
+ info, err := file.Stat()
+ if err != nil {
+ return nil, err
}
if info.IsDir() {
return nil, errors.New("file is a directory")
}
-
- return os.Open(filename)
+ return file, nil
}
-func decodeCfg(f *os.File) (*Config, error) {
+func decodeCfg(f fs.File) (*Config, error) {
var cfg Config
err := json.NewDecoder(f).Decode(&cfg)
if err != nil {
--
2.37.1
From 0205aae14a7d32e15a3ecd76cb1c54e92e315fb1 Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Thu, 21 Jul 2022 12:52:46 +0200
Subject: [PATCH 19/20] Enable banncheck analyzer
(cherry picked from commit 257354834ad71006026e414330cd06c4354c8bee)
Signed-off-by: Roman Mohr <rmohr@google.com>
---
BUILD.bazel | 1 +
nogo_config.json | 8 +++++
tools/analyzers/banncheck/banncheck.json | 38 ++++++++++++++++++++++++
3 files changed, 47 insertions(+)
create mode 100644 tools/analyzers/banncheck/banncheck.json
diff --git a/BUILD.bazel b/BUILD.bazel
index 2104c8d4d..cf4e13c90 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -54,6 +54,7 @@ nogo(
# that are always correct).
# You can see the what `go vet` does by running `go doc cmd/vet`.
deps = [
+ "//tools/analyzers/banncheck:go_default_library",
"//vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign:go_default_library",
"@org_golang_x_tools//go/analysis/passes/asmdecl:go_default_library",
"@org_golang_x_tools//go/analysis/passes/assign:go_default_library",
diff --git a/nogo_config.json b/nogo_config.json
index 46a32563a..0f07f7997 100644
--- a/nogo_config.json
+++ b/nogo_config.json
@@ -125,5 +125,13 @@
"vendor/": "vendor doesn't pass vet",
"external/": "externaldoesn't pass vet"
}
+ },
+ "bannedAPI": {
+ "only_files": {
+ "pkg/": "only production code must not use banned imports"
+ },
+ "exclude_files": {
+ "pkg/.*_test\\.go": "unit tests are allowed to use banned imports"
+ }
}
}
diff --git a/tools/analyzers/banncheck/banncheck.json b/tools/analyzers/banncheck/banncheck.json
new file mode 100644
index 000000000..077cc8a66
--- /dev/null
+++ b/tools/analyzers/banncheck/banncheck.json
@@ -0,0 +1,38 @@
+{
+ "imports": [
+ {
+ "name": "kubevirt.io/kubevirt/pkg/unsafepath",
+ "msg": "unsafepath functions allows accessing a safepath.Path in an unsafe way and needs do be used carefully",
+ "exemptions": [
+ {
+ "justification ": "safepath needs access to unsafepath, to construct an unsafepath.Path for unsafe usage",
+ "allowedPkg": "kubevirt.io/kubevirt/pkg/safepath"
+ },
+ {
+ "justification ": "virt-chroot needs to deconstruct safepaths to pass them over to the virt-chroot binary",
+ "allowedPkg": "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot"
+ },
+ {
+ "justification ": "TODO: can get away from unsafepath but has no additional security risk since host-disk is insecure by default",
+ "allowedPkg": "kubevirt.io/kubevirt/pkg/host-disk"
+ },
+ {
+ "justification ": "TODO: checking if a path is mounted requires a path-string to check against the mount table, can be improved, exploitability risk is very low.",
+ "allowedPkg": "kubevirt.io/kubevirt/pkg/virt-handler/isolation"
+ },
+ {
+ "justification ": "XXX: mount entries are recoreded in a separate file with absolute files. No additional risk, but confusing from an audit perspective, must be changed at some point.",
+ "allowedPkg": "kubevirt.io/kubevirt/pkg/virt-handler/container-disk"
+ },
+ {
+ "justification ": "XXX: mount entries are recoreded in a separate file with absolute files. No additional risk, but confusing from an audit perspective, must be changed at some point.",
+ "allowedPkg": "kubevirt.io/kubevirt/pkg/virt-handler/hotplug-disk"
+ },
+ {
+ "justification ": "for selinux labeling safepaths need to be deconstructed to hand them over to the virt-chroot binary",
+ "allowedPkg": "kubevirt.io/kubevirt/pkg/virt-handler/selinux"
+ }
+ ]
+ }
+ ]
+}
\ No newline at end of file
--
2.37.1
From 3df49b187d5d88493bd85f50f26e5d76c69133ee Mon Sep 17 00:00:00 2001
From: Roman Mohr <rmohr@google.com>
Date: Tue, 5 Jul 2022 19:33:14 +0200
Subject: [PATCH 20/20] Let nogo linters use the right golang.org/x/tools
dependency
Instruct gazelle with an explicit mapping directive to use the right
golang.org/x/tools dependency for the Analyzer interface which is also
used by nogo to load the custom analyzers.
Since gazelle now does the correct mapping, all special handling of
files with a `#gazelle: ignore` annotation is no longer necessary.
The BUILD.bazel files for analyzers are now generated like all the other
build files.
Signed-off-by: Roman Mohr <rmohr@google.com>
---
BUILD.bazel | 1 +
hack/bazel-generate.sh | 15 ---------------
tools/analyzers/BUILD.bazel | 13 +++++--------
.../ineffassign/pkg/ineffassign/BUILD.bazel | 1 -
4 files changed, 6 insertions(+), 24 deletions(-)
diff --git a/BUILD.bazel b/BUILD.bazel
index cf4e13c90..32cbb4373 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -83,6 +83,7 @@ nogo(
# gazelle:prefix kubevirt.io/kubevirt
# gazelle:build_tags selinux
+# gazelle:resolve go golang.org/x/tools/go/analysis @org_golang_x_tools//go/analysis:go_default_library
gazelle(
name = "gazelle",
build_tags = ["selinux"],
diff --git a/hack/bazel-generate.sh b/hack/bazel-generate.sh
index 0180130fd..76bdcfd53 100755
--- a/hack/bazel-generate.sh
+++ b/hack/bazel-generate.sh
@@ -1,20 +1,5 @@
#!/usr/bin/env bash
-# first ensure this file, so that sandbox bootstrapping has a working nogo setup
-# without this sourcing hack/bootstraph.sh will fail
-cat >vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign/BUILD.bazel <<EOT
-# gazelle:ignore
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
-go_library(
- name = "go_default_library",
- srcs = ["ineffassign.go"],
- importmap = "kubevirt.io/kubevirt/vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign",
- importpath = "github.com/gordonklaus/ineffassign/pkg/ineffassign",
- visibility = ["//visibility:public"],
- deps = ["@org_golang_x_tools//go/analysis:go_default_library"],
-)
-EOT
-
source hack/common.sh
source hack/bootstrap.sh
source hack/config.sh
diff --git a/tools/analyzers/BUILD.bazel b/tools/analyzers/BUILD.bazel
index ac86bbfff..4aa636b7d 100644
--- a/tools/analyzers/BUILD.bazel
+++ b/tools/analyzers/BUILD.bazel
@@ -1,12 +1,9 @@
-# gazelle:ignore
-load("@io_bazel_rules_go//go:def.bzl", "go_tool_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
-go_tool_library(
- name = "ineffassign",
+go_library(
+ name = "go_default_library",
srcs = ["analyzer.go"],
- importpath = "kubevirt.io/kubevirt/analyzer",
+ importpath = "kubevirt.io/kubevirt/tools/analyzers",
visibility = ["//visibility:public"],
- deps = [
- "//vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign:go_tool_library",
- ],
+ deps = ["//vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign:go_default_library"],
)
diff --git a/vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign/BUILD.bazel b/vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign/BUILD.bazel
index a405fe687..300fe8ef7 100644
--- a/vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign/BUILD.bazel
+++ b/vendor/github.com/gordonklaus/ineffassign/pkg/ineffassign/BUILD.bazel
@@ -1,4 +1,3 @@
-# gazelle:ignore
load("@io_bazel_rules_go//go:def.bzl", "go_library")
go_library(
--
2.37.1
