Check for valid paths in create-symlinks hook

This change updates the create-symlinks hook to always evaluate link paths in the container's root filesystem. In addition the executable is updated to return an error if a link could not be created. Signed-off-by: Christopher Desiniotis <[email protected]>
NVIDIA · Oct 29, 2024 · 7e0cd45 · 7e0cd45
1 parent a04e3ac
commit 7e0cd45
Show file tree

Hide file tree

Showing 12 changed files with 944 additions and 8 deletions.
diff --git a/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks.go b/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks.go
@@ -23,6 +23,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/moby/sys/symlink"
 	"github.com/urfave/cli/v2"
 
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/logger"
@@ -96,13 +97,12 @@ func (m command) run(c *cli.Context, cfg *config) error {
 		}
 		parts := strings.Split(l, "::")
 		if len(parts) != 2 {
-			m.logger.Warningf("Invalid link specification %v", l)
-			continue
+			return fmt.Errorf("invalid symlink specification %v", l)
 		}
 
 		err := m.createLink(containerRoot, parts[0], parts[1])
 		if err != nil {
-			m.logger.Warningf("Failed to create link %v: %v", parts, err)
+			return fmt.Errorf("failed to create link %v: %w", parts, err)
 		}
 		created[l] = true
 	}
@@ -132,12 +132,17 @@ func (m command) createLink(containerRoot string, targetPath string, link string
 		return nil
 	}
 
-	m.logger.Infof("Symlinking %v to %v", linkPath, targetPath)
-	err = os.MkdirAll(filepath.Dir(linkPath), 0755)
+	resolvedLinkPath, err := symlink.FollowSymlinkInScope(linkPath, containerRoot)
+	if err != nil {
+		return fmt.Errorf("failed to follow path for link %v relative to %v: %w", link, containerRoot, err)
+	}
+
+	m.logger.Infof("Symlinking %v to %v", resolvedLinkPath, targetPath)
+	err = os.MkdirAll(filepath.Dir(resolvedLinkPath), 0755)
 	if err != nil {
 		return fmt.Errorf("failed to create directory: %v", err)
 	}
-	err = os.Symlink(targetPath, linkPath)
+	err = os.Symlink(targetPath, resolvedLinkPath)
 	if err != nil {
 		return fmt.Errorf("failed to create symlink: %v", err)
 	}

diff --git a/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks_test.go b/cmd/nvidia-cdi-hook/create-symlinks/create-symlinks_test.go
@@ -3,18 +3,17 @@ package symlinks
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	testlog "github.com/sirupsen/logrus/hooks/test"
-
 	"github.com/stretchr/testify/require"
 
 	"github.com/NVIDIA/nvidia-container-toolkit/internal/lookup/symlinks"
 )
 
 func TestDoesLinkExist(t *testing.T) {
 	tmpDir := t.TempDir()
-
 	require.NoError(
 		t,
 		makeFs(tmpDir,
@@ -42,6 +41,120 @@ func TestDoesLinkExist(t *testing.T) {
 	require.False(t, exists)
 }
 
+func TestCreateLink(t *testing.T) {
+	type link struct {
+		path   string
+		target string
+	}
+	type expectedLink struct {
+		link
+		err error
+	}
+
+	testCases := []struct {
+		description         string
+		containerContents   []dirOrLink
+		link                link
+		expectedCreateError error
+		expectedLinks       []expectedLink
+	}{
+		{
+			description: "link to / resolves to container root",
+			containerContents: []dirOrLink{
+				{path: "/lib/foo", target: "/"},
+			},
+			link: link{
+				path:   "/lib/foo/libfoo.so",
+				target: "libfoo.so.1",
+			},
+			expectedLinks: []expectedLink{
+				{
+					link: link{
+						path:   "{{ .containerRoot }}/libfoo.so",
+						target: "libfoo.so.1",
+					},
+				},
+			},
+		},
+		{
+			description: "link to / resolves to container root; parent relative link",
+			containerContents: []dirOrLink{
+				{path: "/lib/foo", target: "/"},
+			},
+			link: link{
+				path:   "/lib/foo/libfoo.so",
+				target: "../libfoo.so.1",
+			},
+			expectedLinks: []expectedLink{
+				{
+					link: link{
+						path:   "{{ .containerRoot }}/libfoo.so",
+						target: "../libfoo.so.1",
+					},
+				},
+			},
+		},
+		{
+			description: "link to / resolves to container root; absolute link",
+			containerContents: []dirOrLink{
+				{path: "/lib/foo", target: "/"},
+			},
+			link: link{
+				path:   "/lib/foo/libfoo.so",
+				target: "/a-path-in-container/foo/libfoo.so.1",
+			},
+			expectedLinks: []expectedLink{
+				{
+					link: link{
+						path:   "{{ .containerRoot }}/libfoo.so",
+						target: "/a-path-in-container/foo/libfoo.so.1",
+					},
+				},
+				{
+					// We also check that the target is NOT created.
+					link: link{
+						path: "{{ .containerRoot }}/a-path-in-container/foo/libfoo.so.1",
+					},
+					err: os.ErrNotExist,
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			tmpDir := t.TempDir()
+			hostRoot := filepath.Join(tmpDir, "/host-root/")
+			containerRoot := filepath.Join(tmpDir, "/container-root")
+
+			require.NoError(t, makeFs(hostRoot))
+			require.NoError(t, makeFs(containerRoot, tc.containerContents...))
+
+			// nvidia-cdi-hook create-symlinks --link linkSpec
+			err := getTestCommand().createLink(containerRoot, tc.link.target, tc.link.path)
+			// TODO: We may be able to replace this with require.ErrorIs.
+			if tc.expectedCreateError != nil {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+
+			for _, expectedLink := range tc.expectedLinks {
+				path := strings.Replace(expectedLink.path, "{{ .containerRoot }}", containerRoot, -1)
+				path = strings.Replace(path, "{{ .hostRoot }}", hostRoot, -1)
+				if expectedLink.target != "" {
+					target, err := symlinks.Resolve(path)
+					require.ErrorIs(t, err, expectedLink.err)
+					require.Equal(t, expectedLink.target, target)
+				} else {
+					_, err := os.Stat(path)
+					require.ErrorIs(t, err, expectedLink.err)
+				}
+			}
+		})
+	}
+}
+
 func TestCreateLinkRelativePath(t *testing.T) {
 	tmpDir := t.TempDir()
 	hostRoot := filepath.Join(tmpDir, "/host-root/")
@@ -131,6 +244,8 @@ func TestCreateLinkOutOfBounds(t *testing.T) {
 	// require.Error(t, err)
 	_, err = os.Lstat(filepath.Join(hostRoot, "libfoo.so"))
 	require.ErrorIs(t, err, os.ErrNotExist)
+	_, err = os.Lstat(filepath.Join(containerRoot, hostRoot, "libfoo.so"))
+	require.NoError(t, err)
 }
 
 type dirOrLink struct {

diff --git a/go.mod b/go.mod
@@ -6,6 +6,7 @@ require (
 	github.com/NVIDIA/go-nvlib v0.6.1
 	github.com/NVIDIA/go-nvml v0.12.4-0
 	github.com/fsnotify/fsnotify v1.7.0
+	github.com/moby/sys/symlink v0.3.0
 	github.com/opencontainers/runtime-spec v1.2.0
 	github.com/pelletier/go-toml v1.9.5
 	github.com/sirupsen/logrus v1.9.3

diff --git a/go.sum b/go.sum
@@ -28,6 +28,8 @@ github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
 github.com/mndrix/tap-go v0.0.0-20171203230836-629fa407e90b/go.mod h1:pzzDgJWZ34fGzaAZGFW22KVZDfyrYW+QABMrWnJBnSs=
+github.com/moby/sys/symlink v0.3.0 h1:GZX89mEZ9u53f97npBy4Rc3vJKj7JBDj/PN2I22GrNU=
+github.com/moby/sys/symlink v0.3.0/go.mod h1:3eNdhduHmYPcgsJtZXW1W4XUJdZGBIkttZ8xKqPUJq0=
 github.com/mrunalp/fileutils v0.5.0/go.mod h1:M1WthSahJixYnrXQl/DFQuteStB1weuxD2QJNHXfbSQ=
 github.com/opencontainers/runtime-spec v1.0.3-0.20220825212826-86290f6a00fb/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
 github.com/opencontainers/runtime-spec v1.2.0 h1:z97+pHb3uELt/yiAWD691HNHQIF07bE7dzrbT927iTk=