Skip to content

Commit

Permalink
Only allow host-relative LDConfig paths
Browse files Browse the repository at this point in the history 
This change only allows host-relative LDConfig paths.

An allow-ldconfig-from-container feature flag is added to allow for this
the default behaviour to be changed.

Signed-off-by: Evan Lezar <[email protected]>
  • Loading branch information
@elezar
elezar committed Nov 22, 2024
1 parent c076436 commit c20d246
Show file tree
Hide file tree
Showing 5 changed files with 194 additions and 35 deletions.
17 changes: 16 additions & 1 deletion internal/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package config

import (
"bufio"
"errors"
"fmt"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -51,6 +53,8 @@ var (
NVIDIAContainerToolkitExecutable = "nvidia-container-toolkit"
)

var errInvalidConfig = errors.New("invalid config value")

// Config represents the contents of the config.toml file for the NVIDIA Container Toolkit
// Note: This is currently duplicated by the HookConfig in cmd/nvidia-container-toolkit/hook_config.go
type Config struct {
Expand Down Expand Up @@ -127,7 +131,18 @@ func GetDefault() (*Config, error) {
return &d, nil
}

func getLdConfigPath() string {
// assertValid checks for a valid config.
func (c *Config) assertValid() error {
if !c.Features.AllowLDConfigFromContainer.IsEnabled() && !strings.HasPrefix(c.NVIDIAContainerCLIConfig.Ldconfig, "@") {
return fmt.Errorf("%w: nvidia-container-cli.ldconfig value %q is not host-relative (does not start with a '@')", errInvalidConfig, c.NVIDIAContainerCLIConfig.Ldconfig)
}
return nil
}

// getLdConfigPath allows us to override this function for testing.
var getLdConfigPath = getLdConfigPathStub

func getLdConfigPathStub() string {
return NormalizeLDConfigPath("@/sbin/ldconfig")
}

Expand Down
161 changes: 131 additions & 30 deletions internal/config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,23 +44,21 @@ func TestGetConfigWithCustomConfig(t *testing.T) {

func TestGetConfig(t *testing.T) {
testCases := []struct {
description string
contents []string
expectedError error
inspectLdconfig bool
distIdsLike []string
expectedConfig *Config
description string
contents []string
expectedError error
distIdsLike []string
expectedConfig *Config
}{
{
description: "empty config is default",
inspectLdconfig: true,
description: "empty config is default",
expectedConfig: &Config{
AcceptEnvvarUnprivileged: true,
SupportedDriverCapabilities: "compat32,compute,display,graphics,ngx,utility,video",
NVIDIAContainerCLIConfig: ContainerCLIConfig{
Root: "",
LoadKmods: true,
Ldconfig: "WAS_CHECKED",
Ldconfig: "@/test/ld/config/path",
},
NVIDIAContainerRuntimeConfig: RuntimeConfig{
DebugFilePath: "/dev/null",
Expand Down Expand Up @@ -93,7 +91,7 @@ func TestGetConfig(t *testing.T) {
"supported-driver-capabilities = \"compute,utility\"",
"nvidia-container-cli.root = \"/bar/baz\"",
"nvidia-container-cli.load-kmods = false",
"nvidia-container-cli.ldconfig = \"/foo/bar/ldconfig\"",
"nvidia-container-cli.ldconfig = \"@/foo/bar/ldconfig\"",
"nvidia-container-cli.user = \"foo:bar\"",
"nvidia-container-runtime.debug = \"/foo/bar\"",
"nvidia-container-runtime.discover-mode = \"not-legacy\"",
Expand All @@ -113,7 +111,7 @@ func TestGetConfig(t *testing.T) {
NVIDIAContainerCLIConfig: ContainerCLIConfig{
Root: "/bar/baz",
LoadKmods: false,
Ldconfig: "/foo/bar/ldconfig",
Ldconfig: "@/foo/bar/ldconfig",
User: "foo:bar",
},
NVIDIAContainerRuntimeConfig: RuntimeConfig{
Expand Down Expand Up @@ -146,6 +144,53 @@ func TestGetConfig(t *testing.T) {
},
},
},
{
description: "feature allows ldconfig to be overridden",
contents: []string{
"[nvidia-container-cli]",
"ldconfig = \"/foo/bar/ldconfig\"",
"[features]",
"allow-ldconfig-from-container = true",
},
expectedConfig: &Config{
AcceptEnvvarUnprivileged: true,
SupportedDriverCapabilities: "compat32,compute,display,graphics,ngx,utility,video",
NVIDIAContainerCLIConfig: ContainerCLIConfig{
Ldconfig: "/foo/bar/ldconfig",
LoadKmods: true,
},
NVIDIAContainerRuntimeConfig: RuntimeConfig{
DebugFilePath: "/dev/null",
LogLevel: "info",
Runtimes: []string{"docker-runc", "runc", "crun"},
Mode: "auto",
Modes: modesConfig{
CSV: csvModeConfig{
MountSpecPath: "/etc/nvidia-container-runtime/host-files-for-container.d",
},
CDI: cdiModeConfig{
DefaultKind: "nvidia.com/gpu",
AnnotationPrefixes: []string{
"cdi.k8s.io/",
},
SpecDirs: []string{
"/etc/cdi",
"/var/run/cdi",
},
},
},
},
NVIDIAContainerRuntimeHookConfig: RuntimeHookConfig{
Path: "nvidia-container-runtime-hook",
},
NVIDIACTKConfig: CTKConfig{
Path: "nvidia-ctk",
},
Features: features{
AllowLDConfigFromContainer: ptr(feature(true)),
},
},
},
{
description: "config options set in section",
contents: []string{
Expand All @@ -154,7 +199,7 @@ func TestGetConfig(t *testing.T) {
"[nvidia-container-cli]",
"root = \"/bar/baz\"",
"load-kmods = false",
"ldconfig = \"/foo/bar/ldconfig\"",
"ldconfig = \"@/foo/bar/ldconfig\"",
"user = \"foo:bar\"",
"[nvidia-container-runtime]",
"debug = \"/foo/bar\"",
Expand All @@ -179,7 +224,7 @@ func TestGetConfig(t *testing.T) {
NVIDIAContainerCLIConfig: ContainerCLIConfig{
Root: "/bar/baz",
LoadKmods: false,
Ldconfig: "/foo/bar/ldconfig",
Ldconfig: "@/foo/bar/ldconfig",
User: "foo:bar",
},
NVIDIAContainerRuntimeConfig: RuntimeConfig{
Expand Down Expand Up @@ -213,16 +258,15 @@ func TestGetConfig(t *testing.T) {
},
},
{
description: "suse config",
distIdsLike: []string{"suse", "opensuse"},
inspectLdconfig: true,
description: "suse config",
distIdsLike: []string{"suse", "opensuse"},
expectedConfig: &Config{
AcceptEnvvarUnprivileged: true,
SupportedDriverCapabilities: "compat32,compute,display,graphics,ngx,utility,video",
NVIDIAContainerCLIConfig: ContainerCLIConfig{
Root: "",
LoadKmods: true,
Ldconfig: "WAS_CHECKED",
Ldconfig: "@/test/ld/config/path",
User: "root:video",
},
NVIDIAContainerRuntimeConfig: RuntimeConfig{
Expand Down Expand Up @@ -250,9 +294,8 @@ func TestGetConfig(t *testing.T) {
},
},
{
description: "suse config overrides user",
distIdsLike: []string{"suse", "opensuse"},
inspectLdconfig: true,
description: "suse config overrides user",
distIdsLike: []string{"suse", "opensuse"},
contents: []string{
"nvidia-container-cli.user = \"foo:bar\"",
},
Expand All @@ -262,7 +305,7 @@ func TestGetConfig(t *testing.T) {
NVIDIAContainerCLIConfig: ContainerCLIConfig{
Root: "",
LoadKmods: true,
Ldconfig: "WAS_CHECKED",
Ldconfig: "@/test/ld/config/path",
User: "foo:bar",
},
NVIDIAContainerRuntimeConfig: RuntimeConfig{
Expand Down Expand Up @@ -293,6 +336,7 @@ func TestGetConfig(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
defer setGetLdConfigPathForTest()()
defer setGetDistIDLikeForTest(tc.distIdsLike)()
reader := strings.NewReader(strings.Join(tc.contents, "\n"))

Expand All @@ -305,17 +349,59 @@ func TestGetConfig(t *testing.T) {
cfg, err := tomlCfg.Config()
require.NoError(t, err)

// We first handle the ldconfig path since this is currently system-dependent.
if tc.inspectLdconfig {
ldconfig := cfg.NVIDIAContainerCLIConfig.Ldconfig
require.True(t, strings.HasPrefix(ldconfig, "@/sbin/ldconfig"))
remaining := strings.TrimPrefix(ldconfig, "@/sbin/ldconfig")
require.True(t, remaining == ".real" || remaining == "")
require.EqualValues(t, tc.expectedConfig, cfg)
})
}
}

cfg.NVIDIAContainerCLIConfig.Ldconfig = "WAS_CHECKED"
}
func TestAssertValid(t *testing.T) {
defer setGetLdConfigPathForTest()()

require.EqualValues(t, tc.expectedConfig, cfg)
testCases := []struct {
description string
config *Config
expectedError error
}{
{
description: "default is valid",
config: func() *Config {
config, _ := GetDefault()
return config
}(),
},
{
description: "alternative host ldconfig path is valid",
config: &Config{
NVIDIAContainerCLIConfig: ContainerCLIConfig{
Ldconfig: "@/some/host/path",
},
},
},
{
description: "non-host path is invalid",
config: &Config{
NVIDIAContainerCLIConfig: ContainerCLIConfig{
Ldconfig: "/non/host/path",
},
},
expectedError: errInvalidConfig,
},
{
description: "feature flag allows non-host path",
config: &Config{
NVIDIAContainerCLIConfig: ContainerCLIConfig{
Ldconfig: "/non/host/path",
},
Features: features{
AllowLDConfigFromContainer: ptr(feature(true)),
},
},
},
}

for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
require.ErrorIs(t, tc.config.assertValid(), tc.expectedError)
})
}
}
Expand All @@ -335,3 +421,18 @@ func setGetDistIDLikeForTest(ids []string) func() {
getDistIDLike = original
}
}

// prt returns a reference to whatever type is passed into it
func ptr[T any](x T) *T {
return &x
}

func setGetLdConfigPathForTest() func() {
previous := getLdConfigPath
getLdConfigPath = func() string {
return "@/test/ld/config/path"
}
return func() {
getLdConfigPath = previous
}
}
7 changes: 4 additions & 3 deletions internal/config/features.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,15 @@ type features struct {
// DisableImexChannelCreation ensures that the implicit creation of
// requested IMEX channels is skipped when invoking the nvidia-container-cli.
DisableImexChannelCreation *feature `toml:"disable-imex-channel-creation,omitempty"`
// AllowLDConfigFromContainer allows non-host ldconfig paths to be used.
// If this feature flag is not set to 'true' only host-rooted config paths
// (i.e. paths starting with an '@' are considered valid)
AllowLDConfigFromContainer *feature `toml:"allow-ldconfig-from-container,omitempty"`
}

//nolint:unused
type feature bool

// IsEnabled checks whether a feature is explicitly enabled.
//
//nolint:unused
func (f *feature) IsEnabled() bool {
if f != nil {
return bool(*f)
Expand Down
13 changes: 13 additions & 0 deletions internal/config/toml.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,19 @@ func loadConfigTomlFrom(reader io.Reader) (*Toml, error) {

// Config returns the typed config associated with the toml tree.
func (t *Toml) Config() (*Config, error) {
cfg, err := t.configNoOverrides()
if err != nil {
return nil, err
}
if err := cfg.assertValid(); err != nil {
return nil, err
}
return cfg, nil
}

// configNoOverrides returns the typed config associated with the toml tree.
// This config does not include feature-specific overrides.
func (t *Toml) configNoOverrides() (*Config, error) {
cfg, err := GetDefault()
if err != nil {
return nil, err
Expand Down
31 changes: 30 additions & 1 deletion internal/config/toml_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,12 @@ func TestTomlContents(t *testing.T) {
}

func TestConfigFromToml(t *testing.T) {
defer setGetLdConfigPathForTest()()

testCases := []struct {
description string
contents map[string]interface{}
expectedError error
expectedConfig *Config
}{
{
Expand All @@ -226,13 +229,39 @@ func TestConfigFromToml(t *testing.T) {
return c
}(),
},
{
description: "invalid ldconfig value raises error",
contents: map[string]interface{}{
"nvidia-container-cli": map[string]interface{}{
"ldconfig": "/some/ldconfig/path",
},
},
expectedError: errInvalidConfig,
},
{
description: "feature allows ldconfig override",
contents: map[string]interface{}{
"nvidia-container-cli": map[string]interface{}{
"ldconfig": "/some/ldconfig/path",
},
"features": map[string]interface{}{
"allow-ldconfig-from-container": true,
},
},
expectedConfig: func() *Config {
c, _ := GetDefault()
c.NVIDIAContainerCLIConfig.Ldconfig = "/some/ldconfig/path"
c.Features.AllowLDConfigFromContainer = ptr(feature(true))
return c
}(),
},
}

for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
tomlCfg := fromMap(tc.contents)
config, err := tomlCfg.Config()
require.NoError(t, err)
require.ErrorIs(t, err, tc.expectedError)
require.EqualValues(t, tc.expectedConfig, config)
})
}
Expand Down

0 comments on commit c20d246

Please sign in to comment.