From 2ccc9f6de35b44a65c1c3c4698e5cb3c840f5359 Mon Sep 17 00:00:00 2001 From: nhamza Date: Mon, 7 Sep 2026 10:38:08 +0300 Subject: [PATCH 1/4] OCPEDGE-2973: Add kubelet image credential provider configuration Read imageCredentialProviderConfigPath and imageCredentialProviderBinDir from the kubelet section, validate them with a trusted-path rule, and set them on KubeletFlags for the embedded kubelet. The keys are filtered out of the KubeletConfiguration passthrough; show-config is unchanged. Enhancement: openshift/enhancements#2089 Co-Authored-By: Claude Opus 4.8 --- .../config/config-openapi-spec.json | 2 +- docs/user/howto_config.md | 48 ++ packaging/microshift/config.yaml | 6 +- pkg/config/config.go | 19 +- pkg/config/kubelet.go | 272 ++++++++++ pkg/config/kubelet_test.go | 485 ++++++++++++++++++ pkg/node/kubelet.go | 28 +- pkg/node/kubelet_test.go | 50 ++ .../kubelet-credential-provider.robot | 144 ++++++ 9 files changed, 1049 insertions(+), 5 deletions(-) create mode 100644 pkg/config/kubelet.go create mode 100644 pkg/config/kubelet_test.go create mode 100644 test/suites/configuration1/kubelet-credential-provider.robot diff --git a/cmd/generate-config/config/config-openapi-spec.json b/cmd/generate-config/config/config-openapi-spec.json index e677bbe94b..31ee73a012 100755 --- a/cmd/generate-config/config/config-openapi-spec.json +++ b/cmd/generate-config/config/config-openapi-spec.json @@ -1009,7 +1009,7 @@ } }, "kubelet": { - "description": "Settings specified in this section are transferred as-is into the Kubelet config." + "description": "Settings specified in this section are transferred as-is into the Kubelet config,\nexcept imageCredentialProviderConfigPath and imageCredentialProviderBinDir, which\nenable the kubelet image credential provider and are applied as kubelet startup\nflags. Both must be set together, be absolute paths, and be owned by root and not\nwritable by group or others, including parent directories and contents." }, "manifests": { "type": "object", diff --git a/docs/user/howto_config.md b/docs/user/howto_config.md index 2ae85bb83e..5c14618d3a 100644 --- a/docs/user/howto_config.md +++ b/docs/user/howto_config.md @@ -551,6 +551,54 @@ those volumes must then be manually deleted by the user. Once the MicroShift con supported values, the user may restart MicroShift. They should see that MicroShift does not redeploy the disabled components after restart. +## Kubelet Image Credential Provider + +The `kubelet` section is normally passed through as-is into the kubelet +configuration. Two keys are the exception: `imageCredentialProviderConfigPath` +and `imageCredentialProviderBinDir` are consumed by MicroShift and applied as +kubelet startup flags. They enable the kubelet +[image credential provider](https://kubernetes.io/docs/tasks/administer-cluster/kubelet-credential-provider/), +which lets kubelet obtain registry credentials from an external provider +binary at image pull time instead of relying on static credentials in CRI-O. +This is intended for token-based registries such as Amazon ECR, whose +credentials expire after a short time. + +```yaml +kubelet: + imageCredentialProviderConfigPath: /etc/microshift/credential-providers.yaml + imageCredentialProviderBinDir: /usr/libexec/microshift/credential-providers +``` + +`imageCredentialProviderConfigPath` is the path to a kubelet +`CredentialProviderConfig` file, or to a directory of such files. +`imageCredentialProviderBinDir` is the directory containing the provider +binaries named by that configuration. MicroShift does not ship any provider +binary; obtain the one for your registry (for example `ecr-credential-provider` +from the upstream `kubernetes/cloud-provider-aws` project) and install it +yourself. On image-based systems the binary must be included in every OS image +build, since `/usr` is replaced on each update. + +Place the bin directory under `/usr/libexec` or `/usr/local/bin`, which carry +the `bin_t` SELinux label that the confined kubelet (`kubelet_t`) is permitted +to execute. A bin directory under `/etc/microshift` (labeled +`kubernetes_file_t`) or `/opt` (labeled `usr_t`) passes MicroShift's path +validation but is denied execution under SELinux enforcing: the provider never +runs, the image pull fails, and the only trace is an AVC denial in the audit +log (`ausearch -m AVC -ts recent`). MicroShift does not validate SELinux +labels, so this is a placement rule you must follow. + +Both keys must be set together and must be absolute paths. Because the +provider binary runs with kubelet's privileges, MicroShift refuses to start +unless both paths, all of their parent directories, and every file inside a +directory are owned by root and not writable by group or others. Symbolic +links are resolved and the resolved path is checked and passed to kubelet. +When the keys are omitted, kubelet starts without a credential provider, as +before. + +Changing either key or the provider configuration file requires a MicroShift +restart. On startup with a valid configuration, the journal contains +`Kubelet image credential provider configured` with the paths in use. + ## Drop-in configuration directory In addition to the existing `/etc/microshift/config.yaml` configuration file there is a `/etc/microshift/config.d` configuration directory where you can place fragments of configuration. diff --git a/packaging/microshift/config.yaml b/packaging/microshift/config.yaml index a6171e6c76..5bfc80a30b 100644 --- a/packaging/microshift/config.yaml +++ b/packaging/microshift/config.yaml @@ -664,7 +664,11 @@ ingress: # If unset, the default timeout is 1h tunnelTimeout: 1h -# Settings specified in this section are transferred as-is into the Kubelet config. +# Settings specified in this section are transferred as-is into the Kubelet config, +# except imageCredentialProviderConfigPath and imageCredentialProviderBinDir, which +# enable the kubelet image credential provider and are applied as kubelet startup +# flags. Both must be set together, be absolute paths, and be owned by root and not +# writable by group or others, including parent directories and contents. kubelet: manifests: # The locations on the filesystem to scan for kustomization diff --git a/pkg/config/config.go b/pkg/config/config.go index 35fafe3a26..f85440e93e 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -56,7 +56,11 @@ type Config struct { Ingress IngressConfig `json:"ingress"` Storage Storage `json:"storage"` Telemetry Telemetry `json:"telemetry"` - // Settings specified in this section are transferred as-is into the Kubelet config. + // Settings specified in this section are transferred as-is into the Kubelet config, + // except imageCredentialProviderConfigPath and imageCredentialProviderBinDir, which + // enable the kubelet image credential provider and are applied as kubelet startup + // flags. Both must be set together, be absolute paths, and be owned by root and not + // writable by group or others, including parent directories and contents. // +kubebuilder:validation:Schemaless Kubelet map[string]any `json:"kubelet"` @@ -67,6 +71,12 @@ type Config struct { // Internal-only fields userSettings *Config `json:"-"` // the values read from the config file + // Read from the Kubelet map during updateComputedValues(). These are kubelet + // flags, not KubeletConfiguration fields. After validation they hold the + // canonical (symlink-resolved) paths. + KubeletImageCredentialProviderConfigPath string `json:"-"` + KubeletImageCredentialProviderBinDir string `json:"-"` + MultiNode MultiNodeConfig `json:"-"` // the value read from commond line Warnings []string `json:"-"` // Warnings that should not prevent the service from starting. @@ -587,6 +597,10 @@ func (c *Config) updateComputedValues() error { c.C2CC.stripEmptyRemoteClusters() c.C2CC.resolveRoutingDefaults() + if err := c.readKubeletCredentialProviderKeys(); err != nil { + return err + } + return nil } @@ -745,6 +759,9 @@ func (c *Config) validate() error { return fmt.Errorf("error validating clusterToCluster: %w", err) } } + if err := c.validateKubeletCredentialProvider(); err != nil { + return err + } return nil } diff --git a/pkg/config/kubelet.go b/pkg/config/kubelet.go new file mode 100644 index 0000000000..51b3641214 --- /dev/null +++ b/pkg/config/kubelet.go @@ -0,0 +1,272 @@ +package config + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "slices" + "syscall" +) + +// isNotExistErr reports whether err means the path cannot exist. It covers both +// a plain "no such file or directory" and ENOTDIR, which EvalSymlinks returns +// when a non-directory appears mid-path (e.g. "/etc/cp.yaml/extra" where +// cp.yaml is a regular file). +func isNotExistErr(err error) bool { + return os.IsNotExist(err) || errors.Is(err, syscall.ENOTDIR) +} + +const ( + // These are configuration key names, not credentials. + kubeletImageCredentialProviderConfigPathKey = "imageCredentialProviderConfigPath" //nolint:gosec // G101: not a credential + kubeletImageCredentialProviderBinDirKey = "imageCredentialProviderBinDir" //nolint:gosec // G101: not a credential +) + +// kubeletReservedKeys lists the keys under the kubelet: section that MicroShift +// consumes itself (as kubelet startup flags) instead of passing through into the +// generated KubeletConfiguration. +var kubeletReservedKeys = []string{ + kubeletImageCredentialProviderConfigPathKey, + kubeletImageCredentialProviderBinDirKey, +} + +// statForTrust returns the owning uid and mode of an already symlink-resolved +// path. It is a package-level variable so tests can exercise the trusted-path +// ownership rules without running as root. +var statForTrust = func(path string) (uid uint32, mode os.FileMode, err error) { + fi, err := os.Lstat(path) + if err != nil { + return 0, 0, err + } + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return 0, 0, fmt.Errorf("unable to determine ownership of %q", path) + } + return st.Uid, fi.Mode(), nil +} + +// readKubeletCredentialProviderKeys copies the two credential-provider keys from +// the schemaless kubelet map into the typed Config fields. c.Kubelet is left +// untouched; canonicalization happens later, during validation. +func (c *Config) readKubeletCredentialProviderKeys() error { + configPath, err := kubeletStringValue(c.Kubelet, kubeletImageCredentialProviderConfigPathKey) + if err != nil { + return err + } + binDir, err := kubeletStringValue(c.Kubelet, kubeletImageCredentialProviderBinDirKey) + if err != nil { + return err + } + c.KubeletImageCredentialProviderConfigPath = configPath + c.KubeletImageCredentialProviderBinDir = binDir + return nil +} + +// kubeletStringValue reads key from the kubelet map. A missing key, an explicit +// null, or an empty string all mean "unset"; a non-string value is an error. +func kubeletStringValue(m map[string]any, key string) (string, error) { + if m == nil { + return "", nil + } + raw, ok := m[key] + if !ok || raw == nil { + return "", nil + } + s, ok := raw.(string) + if !ok { + return "", fmt.Errorf("kubelet.%s must be a string, got %T", key, raw) + } + return s, nil +} + +// KubeletPassthrough returns a copy of the kubelet map with the MicroShift-owned +// keys removed, so only genuine KubeletConfiguration settings are written to the +// generated kubelet config file. A nil map returns nil. +func (c *Config) KubeletPassthrough() map[string]any { + if c.Kubelet == nil { + return nil + } + out := make(map[string]any, len(c.Kubelet)) + for k, v := range c.Kubelet { + if slices.Contains(kubeletReservedKeys, k) { + continue + } + out[k] = v + } + return out +} + +// credentialPathKind selects which object types validateCredentialProviderPath +// accepts for the final object. +type credentialPathKind int + +const ( + // credentialProviderConfigKind accepts a regular file or a directory. + credentialProviderConfigKind credentialPathKind = iota + // credentialProviderBinDirKind accepts only a directory. + credentialProviderBinDirKind +) + +// validateKubeletCredentialProvider validates the two credential-provider keys. +// The rules are applied in order and the first failure wins. On success the +// typed fields are replaced with the canonical (symlink-resolved) paths that are +// handed to the kubelet; c.Kubelet is left untouched. +func (c *Config) validateKubeletCredentialProvider() error { + configPath := c.KubeletImageCredentialProviderConfigPath + binDir := c.KubeletImageCredentialProviderBinDir + + // Neither set: the feature is inactive. + if configPath == "" && binDir == "" { + return nil + } + + // Both keys must be provided together. + if configPath == "" || binDir == "" { + return fmt.Errorf("kubelet.%s and kubelet.%s must be set together", + kubeletImageCredentialProviderConfigPathKey, kubeletImageCredentialProviderBinDirKey) + } + + paths := []struct { + key string + value string + kind credentialPathKind + dst *string + }{ + {kubeletImageCredentialProviderConfigPathKey, configPath, credentialProviderConfigKind, &c.KubeletImageCredentialProviderConfigPath}, + {kubeletImageCredentialProviderBinDirKey, binDir, credentialProviderBinDirKind, &c.KubeletImageCredentialProviderBinDir}, + } + + // Check that both keys are absolute before touching the filesystem. + for _, p := range paths { + if !filepath.IsAbs(p.value) { + return fmt.Errorf("kubelet.%s (%q) must be an absolute path", p.key, p.value) + } + } + + canonical := make([]string, len(paths)) + for i, p := range paths { + resolved, err := validateCredentialProviderPath(p.value, p.kind) + if err != nil { + return fmt.Errorf("error validating kubelet.%s (%q): %w", p.key, p.value, err) + } + canonical[i] = resolved + } + + // The two keys must not resolve to the same path. Kubelet would then read the + // bin dir as the config directory (and vice versa) and fail at registration, + // so reject it here with a clear message instead. + if canonical[0] == canonical[1] { + return fmt.Errorf("kubelet.%s and kubelet.%s must not resolve to the same path (%q)", + kubeletImageCredentialProviderConfigPathKey, kubeletImageCredentialProviderBinDirKey, canonical[0]) + } + + // Store canonical paths only once both keys have passed validation. + for i, p := range paths { + *p.dst = canonical[i] + } + return nil +} + +// validateCredentialProviderPath resolves path, checks that the final object is +// of an acceptable kind, and applies the trusted-path rule. It returns the +// canonical path. The object type is checked against the real filesystem (so a +// FIFO, socket or device is rejected), while ownership is checked through the +// statForTrust hook. +func validateCredentialProviderPath(path string, kind credentialPathKind) (string, error) { + canonical, err := filepath.EvalSymlinks(path) + if err != nil { + if isNotExistErr(err) { + return "", fmt.Errorf("file or directory does not exist") + } + return "", err + } + + fi, err := os.Lstat(canonical) + if err != nil { + return "", err + } + isDir := fi.IsDir() + + switch kind { + case credentialProviderConfigKind: + if !isDir && !fi.Mode().IsRegular() { + return "", fmt.Errorf("%q must be a regular file or a directory", canonical) + } + case credentialProviderBinDirKind: + if !isDir { + return "", fmt.Errorf("%q must be a directory", canonical) + } + } + + if err := validateTrustedChain(canonical); err != nil { + return "", err + } + + // If the final object is a directory, every entry it contains must also + // satisfy the trusted-path rule. + if isDir { + if err := validateDirEntries(canonical); err != nil { + return "", err + } + } + + return canonical, nil +} + +// validateDirEntries applies the trusted-path rule to every entry in dir. +// Symlinked entries are resolved and the full rule, including the target's +// ancestors, is applied to the target. +func validateDirEntries(dir string) error { + entries, err := os.ReadDir(dir) + if err != nil { + return err + } + for _, entry := range entries { + entryPath := filepath.Join(dir, entry.Name()) + resolved, err := filepath.EvalSymlinks(entryPath) + if err != nil { + if isNotExistErr(err) { + return fmt.Errorf("%q does not exist", entryPath) + } + return err + } + if err := validateTrustedChain(resolved); err != nil { + return err + } + } + return nil +} + +// validateTrustedChain walks every component of the canonical (already +// symlink-resolved) path from / to the final object and requires each to be +// owned by root and not writable by group or others. +func validateTrustedChain(canonical string) error { + for _, component := range trustedPathComponents(canonical) { + uid, mode, err := statForTrust(component) + if err != nil { + return err + } + if uid != 0 || mode&0o022 != 0 { + return fmt.Errorf("%q must be owned by root and not writable by group or others", component) + } + } + return nil +} + +// trustedPathComponents returns every path component of abs, ordered from the +// root "/" down to abs itself. +func trustedPathComponents(abs string) []string { + abs = filepath.Clean(abs) + var components []string + for { + components = append(components, abs) + parent := filepath.Dir(abs) + if parent == abs { + break + } + abs = parent + } + slices.Reverse(components) + return components +} diff --git a/pkg/config/kubelet_test.go b/pkg/config/kubelet_test.go new file mode 100644 index 0000000000..fa72532b3a --- /dev/null +++ b/pkg/config/kubelet_test.go @@ -0,0 +1,485 @@ +package config + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeStat overrides the ownership/mode reported for a specific path. +type fakeStat struct { + uid uint32 + mode os.FileMode +} + +// withStatMock installs a statForTrust that reports paths as root-owned and +// non-group/other-writable by default, using the real filesystem only to learn +// whether a path exists and is a directory. Entries in overrides let individual +// components report a different uid/mode so ownership failures can be exercised +// without running as root. +func withStatMock(t *testing.T, overrides map[string]fakeStat) { + t.Helper() + orig := statForTrust + statForTrust = func(path string) (uint32, os.FileMode, error) { + if o, ok := overrides[path]; ok { + return o.uid, o.mode, nil + } + // Default: root-owned, non-group/other-writable, with a mode that + // matches whether the real path is a directory. + fi, err := os.Lstat(path) + if err != nil { + return 0, 0, err + } + mode := os.FileMode(0o644) + if fi.IsDir() { + mode = 0o755 + } + return 0, mode, nil + } + t.Cleanup(func() { statForTrust = orig }) +} + +func TestReadKubeletCredentialProviderKeys(t *testing.T) { + ttests := []struct { + name string + kubelet map[string]any + wantConfig string + wantBinDir string + expectError string + }{ + { + name: "nil map", + kubelet: nil, + }, + { + name: "keys absent", + kubelet: map[string]any{"cpuManagerPolicy": "static"}, + }, + { + name: "both present", + kubelet: map[string]any{ + "imageCredentialProviderConfigPath": "/etc/microshift/cp.yaml", + "imageCredentialProviderBinDir": "/usr/libexec/cp", + }, + wantConfig: "/etc/microshift/cp.yaml", + wantBinDir: "/usr/libexec/cp", + }, + { + name: "only config present", + kubelet: map[string]any{ + "imageCredentialProviderConfigPath": "/etc/microshift/cp.yaml", + }, + wantConfig: "/etc/microshift/cp.yaml", + }, + { + name: "empty string is unset", + kubelet: map[string]any{ + "imageCredentialProviderConfigPath": "", + "imageCredentialProviderBinDir": "", + }, + }, + { + name: "explicit null is unset", + kubelet: map[string]any{ + "imageCredentialProviderConfigPath": nil, + "imageCredentialProviderBinDir": nil, + }, + }, + { + name: "int type is rejected", + kubelet: map[string]any{ + "imageCredentialProviderConfigPath": 42, + }, + expectError: "kubelet.imageCredentialProviderConfigPath must be a string, got int", + }, + { + name: "bool type is rejected", + kubelet: map[string]any{ + "imageCredentialProviderConfigPath": "/etc/microshift/cp.yaml", + "imageCredentialProviderBinDir": true, + }, + expectError: "kubelet.imageCredentialProviderBinDir must be a string, got bool", + }, + { + name: "map type is rejected", + kubelet: map[string]any{ + "imageCredentialProviderConfigPath": map[string]any{"a": "b"}, + }, + expectError: "kubelet.imageCredentialProviderConfigPath must be a string, got map[string]interface {}", + }, + } + + for _, tt := range ttests { + t.Run(tt.name, func(t *testing.T) { + c := &Config{Kubelet: tt.kubelet} + err := c.readKubeletCredentialProviderKeys() + if tt.expectError != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectError) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantConfig, c.KubeletImageCredentialProviderConfigPath) + assert.Equal(t, tt.wantBinDir, c.KubeletImageCredentialProviderBinDir) + // c.Kubelet must never be modified during reading. + assert.Equal(t, tt.kubelet, c.Kubelet) + }) + } +} + +func TestKubeletPassthrough(t *testing.T) { + t.Run("nil map returns nil", func(t *testing.T) { + c := &Config{Kubelet: nil} + assert.Nil(t, c.KubeletPassthrough()) + }) + + t.Run("empty map returns empty, non-nil map", func(t *testing.T) { + c := &Config{Kubelet: map[string]any{}} + got := c.KubeletPassthrough() + assert.NotNil(t, got) + assert.Empty(t, got) + }) + + t.Run("map with only reserved keys returns empty map", func(t *testing.T) { + c := &Config{Kubelet: map[string]any{ + "imageCredentialProviderConfigPath": "/etc/microshift/cp.yaml", + "imageCredentialProviderBinDir": "/usr/libexec/cp", + }} + assert.Empty(t, c.KubeletPassthrough()) + }) + + t.Run("drops exactly the reserved keys and preserves the rest", func(t *testing.T) { + c := &Config{Kubelet: map[string]any{ + "imageCredentialProviderConfigPath": "/etc/microshift/cp.yaml", + "imageCredentialProviderBinDir": "/usr/libexec/cp", + "cpuManagerPolicy": "static", + "kubeReserved": map[string]any{"memory": "500Mi"}, + }} + got := c.KubeletPassthrough() + assert.Equal(t, map[string]any{ + "cpuManagerPolicy": "static", + "kubeReserved": map[string]any{"memory": "500Mi"}, + }, got) + // The original map is untouched. + assert.Contains(t, c.Kubelet, "imageCredentialProviderConfigPath") + assert.Contains(t, c.Kubelet, "imageCredentialProviderBinDir") + }) +} + +// mkRootFile / mkRootDir create real filesystem objects for path/type checks; +// ownership is asserted through the stat mock, not the real files. +func mkFile(t *testing.T, dir, name string, mode os.FileMode) string { + t.Helper() + p := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(p, []byte("x"), mode)) + return p +} + +func mkDir(t *testing.T, dir, name string) string { + t.Helper() + p := filepath.Join(dir, name) + require.NoError(t, os.Mkdir(p, 0o755)) + return p +} + +func TestValidateKubeletCredentialProvider(t *testing.T) { + t.Run("neither set is OK", func(t *testing.T) { + c := &Config{} + assert.NoError(t, c.validateKubeletCredentialProvider()) + }) + + t.Run("only config set", func(t *testing.T) { + c := &Config{KubeletImageCredentialProviderConfigPath: "/etc/cp.yaml"} + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "must be set together") + }) + + t.Run("only bin dir set", func(t *testing.T) { + c := &Config{KubeletImageCredentialProviderBinDir: "/usr/libexec/cp"} + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "must be set together") + }) + + t.Run("relative config path", func(t *testing.T) { + c := &Config{ + KubeletImageCredentialProviderConfigPath: "relative/cp.yaml", + KubeletImageCredentialProviderBinDir: "/usr/libexec/cp", + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "kubelet.imageCredentialProviderConfigPath") + assert.Contains(t, err.Error(), "must be an absolute path") + }) + + t.Run("relative bin dir", func(t *testing.T) { + c := &Config{ + KubeletImageCredentialProviderConfigPath: "/etc/cp.yaml", + KubeletImageCredentialProviderBinDir: "relative/cp", + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "kubelet.imageCredentialProviderBinDir") + assert.Contains(t, err.Error(), "must be an absolute path") + }) + + t.Run("missing config path", func(t *testing.T) { + dir := t.TempDir() + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: filepath.Join(dir, "does-not-exist.yaml"), + KubeletImageCredentialProviderBinDir: dir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "file or directory does not exist") + }) + + t.Run("missing bin dir", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: filepath.Join(dir, "missing"), + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "file or directory does not exist") + }) + + t.Run("config path descends through a non-directory", func(t *testing.T) { + dir := t.TempDir() + file := mkFile(t, dir, "cp.yaml", 0o644) + withStatMock(t, nil) + c := &Config{ + // cp.yaml is a regular file, so treating it as a directory is + // an ENOTDIR mid-path, reported as "does not exist". + KubeletImageCredentialProviderConfigPath: filepath.Join(file, "extra"), + KubeletImageCredentialProviderBinDir: dir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "file or directory does not exist") + }) + + t.Run("config path is a FIFO", func(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, "cp.fifo") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: fifo, + KubeletImageCredentialProviderBinDir: dir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a regular file or a directory") + }) + + t.Run("config path is a regular file", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + assert.NoError(t, c.validateKubeletCredentialProvider()) + }) + + t.Run("config path is a directory", func(t *testing.T) { + dir := t.TempDir() + cfgDir := mkDir(t, dir, "cp.d") + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgDir, + KubeletImageCredentialProviderBinDir: binDir, + } + assert.NoError(t, c.validateKubeletCredentialProvider()) + }) + + t.Run("config path and bin dir resolve to the same directory", func(t *testing.T) { + dir := t.TempDir() + shared := mkDir(t, dir, "shared") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: shared, + KubeletImageCredentialProviderBinDir: shared, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "must not resolve to the same path") + }) + + t.Run("bin dir is a file", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + binFile := mkFile(t, dir, "notadir", 0o644) + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binFile, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "must be a directory") + }) + + t.Run("valid config canonicalizes and stores canonical paths", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + require.NoError(t, c.validateKubeletCredentialProvider()) + wantCfg, _ := filepath.EvalSymlinks(cfgFile) + wantBin, _ := filepath.EvalSymlinks(binDir) + assert.Equal(t, wantCfg, c.KubeletImageCredentialProviderConfigPath) + assert.Equal(t, wantBin, c.KubeletImageCredentialProviderBinDir) + }) +} + +func TestValidateKubeletCredentialProviderTrustedPath(t *testing.T) { + // newValidPair returns a config file and bin dir that both pass validation + // when the default (all-root) stat mock is used. + newValidPair := func(t *testing.T) (string, string) { + dir := t.TempDir() + return mkFile(t, dir, "cp.yaml", 0o644), mkDir(t, dir, "bin") + } + + t.Run("non-root owner on final object", func(t *testing.T) { + cfgFile, binDir := newValidPair(t) + canonical, _ := filepath.EvalSymlinks(binDir) + withStatMock(t, map[string]fakeStat{canonical: {uid: 1000, mode: 0o755}}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), canonical) + assert.Contains(t, err.Error(), "must be owned by root and not writable by group or others") + }) + + t.Run("group-writable ancestor", func(t *testing.T) { + cfgFile, binDir := newValidPair(t) + canonical, _ := filepath.EvalSymlinks(binDir) + ancestor := filepath.Dir(canonical) + withStatMock(t, map[string]fakeStat{ancestor: {uid: 0, mode: 0o775}}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), ancestor) + assert.Contains(t, err.Error(), "must be owned by root") + }) + + t.Run("world-writable final object", func(t *testing.T) { + cfgFile, binDir := newValidPair(t) + canonical, _ := filepath.EvalSymlinks(binDir) + withStatMock(t, map[string]fakeStat{canonical: {uid: 0, mode: 0o757}}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "must be owned by root") + }) + + t.Run("world-writable contained entry", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + binDir := mkDir(t, dir, "bin") + plugin := mkFile(t, binDir, "ecr-credential-provider", 0o755) + canonicalPlugin, _ := filepath.EvalSymlinks(plugin) + withStatMock(t, map[string]fakeStat{canonicalPlugin: {uid: 0, mode: 0o757}}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), canonicalPlugin) + assert.Contains(t, err.Error(), "must be owned by root") + }) + + t.Run("compliant root-owned dir and file is OK", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + binDir := mkDir(t, dir, "bin") + mkFile(t, binDir, "ecr-credential-provider", 0o755) + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + assert.NoError(t, c.validateKubeletCredentialProvider()) + }) + + t.Run("symlink to compliant target resolves to canonical", func(t *testing.T) { + dir := t.TempDir() + realDir := mkDir(t, dir, "real-bin") + cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + link := filepath.Join(dir, "link-bin") + require.NoError(t, os.Symlink(realDir, link)) + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: link, + } + require.NoError(t, c.validateKubeletCredentialProvider()) + wantBin, _ := filepath.EvalSymlinks(realDir) + assert.Equal(t, wantBin, c.KubeletImageCredentialProviderBinDir) + }) + + t.Run("symlinked bin-dir entry is checked at its target including ancestors", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + binDir := mkDir(t, dir, "bin") + // The real plugin lives outside binDir, under an unsafe ancestor. + unsafeParent := mkDir(t, dir, "unsafe") + realPlugin := mkFile(t, unsafeParent, "plugin", 0o755) + require.NoError(t, os.Symlink(realPlugin, filepath.Join(binDir, "plugin"))) + canonicalParent, _ := filepath.EvalSymlinks(unsafeParent) + withStatMock(t, map[string]fakeStat{canonicalParent: {uid: 0, mode: 0o777}}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), canonicalParent) + assert.Contains(t, err.Error(), "must be owned by root") + }) + + t.Run("symlink to unsafe target is rejected", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + realDir := mkDir(t, dir, "real-bin") + link := filepath.Join(dir, "link-bin") + require.NoError(t, os.Symlink(realDir, link)) + canonical, _ := filepath.EvalSymlinks(realDir) + withStatMock(t, map[string]fakeStat{canonical: {uid: 1000, mode: 0o755}}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: link, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "must be owned by root") + }) +} diff --git a/pkg/node/kubelet.go b/pkg/node/kubelet.go index 6a0c8ce6d8..f9632e66cd 100644 --- a/pkg/node/kubelet.go +++ b/pkg/node/kubelet.go @@ -90,6 +90,8 @@ func (s *KubeletServer) configure(cfg *config.Config) { kubeletFlags.NodeLabels["node.openshift.io/os_id"] = osID kubeletFlags.NodeLabels["node.kubernetes.io/instance-type"] = "rhde" + setImageCredentialProviderFlags(kubeletFlags, cfg) + kubeletConfig, err := loadConfigFile(filepath.Join(config.DataDir, "/resources/kubelet/config/config.yaml")) if err != nil { @@ -100,6 +102,25 @@ func (s *KubeletServer) configure(cfg *config.Config) { s.kubeletflags = kubeletFlags } +// setImageCredentialProviderFlags copies the (already validated and +// canonicalized) image credential provider paths onto the kubelet flags. When +// the feature is not configured the flags are left at their defaults. +func setImageCredentialProviderFlags(kubeletFlags *kubeletoptions.KubeletFlags, cfg *config.Config) { + if cfg.KubeletImageCredentialProviderConfigPath == "" { + return + } + + kubeletFlags.ImageCredentialProviderConfigPath = cfg.KubeletImageCredentialProviderConfigPath + kubeletFlags.ImageCredentialProviderBinDir = cfg.KubeletImageCredentialProviderBinDir + + // The paths logged here are the canonical (symlink-resolved) ones handed to + // kubelet. The values the user configured are available from + // `microshift show-config`, so they are not duplicated in the journal. + klog.InfoS("Kubelet image credential provider configured", + "configPath", cfg.KubeletImageCredentialProviderConfigPath, + "binDir", cfg.KubeletImageCredentialProviderBinDir) +} + func (s *KubeletServer) writeConfig(cfg *config.Config) error { data, err := s.generateConfig(cfg) if err != nil { @@ -136,8 +157,11 @@ func (s *KubeletServer) generateConfig(cfg *config.Config) ([]byte, error) { } userProvidedConfig := "" - if cfg.Kubelet != nil { - b, err := yaml.Marshal(cfg.Kubelet) + // The MicroShift-owned keys (image credential provider paths) are applied as + // kubelet startup flags, not KubeletConfiguration fields, so they must be + // filtered out of the generated config here. + if passthrough := cfg.KubeletPassthrough(); len(passthrough) > 0 { + b, err := yaml.Marshal(passthrough) if err != nil { return nil, fmt.Errorf("failed to re-marshal user provided kubelet config: %w", err) } diff --git a/pkg/node/kubelet_test.go b/pkg/node/kubelet_test.go index a98a071c0f..bd8ffd6b64 100644 --- a/pkg/node/kubelet_test.go +++ b/pkg/node/kubelet_test.go @@ -4,6 +4,8 @@ import ( "testing" "github.com/openshift/microshift/pkg/config" + kubeletoptions "k8s.io/kubernetes/cmd/kubelet/app/options" + "github.com/stretchr/testify/assert" ) @@ -11,6 +13,10 @@ func Test_GenerateConfig(t *testing.T) { cfg := config.NewDefault() cfg.Kubelet = map[string]any{ "cpuManagerPolicy": "static", + // Reserved keys are MicroShift-owned kubelet flags and must never + // appear in the generated KubeletConfiguration. + "imageCredentialProviderConfigPath": "/etc/microshift/credential-providers.yaml", + "imageCredentialProviderBinDir": "/usr/libexec/microshift/credential-providers", "reservedMemory": []any{ map[string]any{ "limits": map[string]any{ @@ -47,4 +53,48 @@ reservedMemory: data, err := kubelet.generateConfig(cfg) assert.NoError(t, err) assert.Contains(t, string(data), expectedConfigPart) + // The reserved keys are stripped from the passthrough config. + assert.NotContains(t, string(data), "imageCredentialProviderConfigPath") + assert.NotContains(t, string(data), "imageCredentialProviderBinDir") +} + +func Test_GenerateConfig_EmptyKubelet(t *testing.T) { + // An empty (or reserved-keys-only) kubelet map must not inject anything into + // the generated KubeletConfiguration: the output must match the nil-map case, + // with no stray "{}" appended. + kubelet := &KubeletServer{} + + nilCfg := config.NewDefault() + nilCfg.Kubelet = nil + nilData, err := kubelet.generateConfig(nilCfg) + assert.NoError(t, err) + + emptyCfg := config.NewDefault() + emptyCfg.Kubelet = map[string]any{} + emptyData, err := kubelet.generateConfig(emptyCfg) + assert.NoError(t, err) + + assert.Equal(t, string(nilData), string(emptyData)) + assert.NotContains(t, string(emptyData), "{}") +} + +func Test_setImageCredentialProviderFlags(t *testing.T) { + t.Run("sets both flags to the canonical values when configured", func(t *testing.T) { + cfg := &config.Config{ + KubeletImageCredentialProviderConfigPath: "/etc/microshift/credential-providers.yaml", + KubeletImageCredentialProviderBinDir: "/usr/libexec/microshift/credential-providers", + } + flags := kubeletoptions.NewKubeletFlags() + setImageCredentialProviderFlags(flags, cfg) + assert.Equal(t, "/etc/microshift/credential-providers.yaml", flags.ImageCredentialProviderConfigPath) + assert.Equal(t, "/usr/libexec/microshift/credential-providers", flags.ImageCredentialProviderBinDir) + }) + + t.Run("leaves flags empty when not configured", func(t *testing.T) { + cfg := &config.Config{} + flags := kubeletoptions.NewKubeletFlags() + setImageCredentialProviderFlags(flags, cfg) + assert.Empty(t, flags.ImageCredentialProviderConfigPath) + assert.Empty(t, flags.ImageCredentialProviderBinDir) + }) } diff --git a/test/suites/configuration1/kubelet-credential-provider.robot b/test/suites/configuration1/kubelet-credential-provider.robot new file mode 100644 index 0000000000..948266c115 --- /dev/null +++ b/test/suites/configuration1/kubelet-credential-provider.robot @@ -0,0 +1,144 @@ +*** Settings *** +Documentation Kubelet image credential provider configuration tests + +Resource ../../resources/common.resource +Resource ../../resources/microshift-config.resource +Resource ../../resources/microshift-process.resource +Library ../../resources/journalctl.py + +Suite Setup Setup +Suite Teardown Teardown + +Test Tags slow restart + + +*** Variables *** +${CURSOR} ${EMPTY} +${CP_DROPIN} 10-credential-provider +${CP_CONFIGURED_LOG} Kubelet image credential provider configured +${CP_BIN_DIR} /usr/libexec/microshift/credential-providers +${CP_MOCK_PROVIDER} ${CP_BIN_DIR}/mock-credential-provider +${CP_CONFIG_FILE} /etc/microshift/credential-providers.yaml +${KUBELET_GENERATED_CONFIG} /var/lib/microshift/resources/kubelet/config/config.yaml +${CP_VALID} SEPARATOR=\n +... --- +... kubelet: +... \ \ imageCredentialProviderConfigPath: ${CP_CONFIG_FILE} +... \ \ imageCredentialProviderBinDir: ${CP_BIN_DIR} +${CP_MISSING_BIN_DIR} SEPARATOR=\n +... --- +... kubelet: +... \ \ imageCredentialProviderConfigPath: ${CP_CONFIG_FILE} +... \ \ imageCredentialProviderBinDir: /usr/libexec/microshift/no-such-dir +${CP_ONLY_CONFIG_PATH} SEPARATOR=\n +... --- +... kubelet: +... \ \ imageCredentialProviderConfigPath: ${CP_CONFIG_FILE} +${CP_PROVIDER_CONFIG} SEPARATOR=\n +... apiVersion: kubelet.config.k8s.io/v1 +... kind: CredentialProviderConfig +... providers: +... \ \ - name: mock-credential-provider +... \ \ \ \ matchImages: +... \ \ \ \ \ \ - "registry.example.invalid" +... \ \ \ \ defaultCacheDuration: "1m" +... \ \ \ \ apiVersion: credentialprovider.kubelet.k8s.io/v1 +${CP_MOCK_SCRIPT} SEPARATOR=\n +... \#!/bin/bash +... \# Mock kubelet image credential provider: returns static credentials. +... cat >/dev/null +... cat <<'JSON' +... {"kind":"CredentialProviderResponse", +... "apiVersion":"credentialprovider.kubelet.k8s.io/v1", +... "cacheKeyType":"Registry","cacheDuration":"1m", +... "auth":{"registry.example.invalid": +... {"username":"user","password":"pass"}}} +... JSON + + +*** Test Cases *** +Keys Absent Leaves Kubelet Unchanged + [Documentation] Without the keys, MicroShift starts and no credential provider is configured + [Setup] Run Keywords Remove Credential Provider Config AND Restart MicroShift With Cursor + Pattern Should Not Appear In Log Output ${CURSOR} ${CP_CONFIGURED_LOG} + +Valid Configuration Applies Kubelet Flags + [Documentation] With both keys set, MicroShift starts, logs the configured paths, reports them in + ... show-config, and keeps them out of the generated KubeletConfiguration + [Setup] Apply Credential Provider Config ${CP_VALID} + Pattern Should Appear In Log Output ${CURSOR} ${CP_CONFIGURED_LOG} + ${config}= Show Config effective + Should Be Equal As Strings ${config.kubelet.imageCredentialProviderConfigPath} ${CP_CONFIG_FILE} + Should Be Equal As Strings ${config.kubelet.imageCredentialProviderBinDir} ${CP_BIN_DIR} + Command Should Fail grep -q imageCredentialProvider ${KUBELET_GENERATED_CONFIG} + [Teardown] Remove Credential Provider Config + +Missing Bin Directory Prevents Start + [Documentation] MicroShift fails to start when the bin directory does not exist + [Setup] Apply Invalid Credential Provider Config ${CP_MISSING_BIN_DIR} + Pattern Should Appear In Log Output ${CURSOR} imageCredentialProviderBinDir + Pattern Should Appear In Log Output ${CURSOR} does not exist + [Teardown] Run Keywords Remove Credential Provider Config AND Restart MicroShift + +Only One Key Prevents Start + [Documentation] MicroShift fails to start when only one of the two keys is set + [Setup] Apply Invalid Credential Provider Config ${CP_ONLY_CONFIG_PATH} + Pattern Should Appear In Log Output ${CURSOR} must be set together + [Teardown] Run Keywords Remove Credential Provider Config AND Restart MicroShift + +World Writable Bin Directory Prevents Start + [Documentation] MicroShift refuses to start when the bin directory is writable by others + [Setup] Run Keywords Command Should Work chmod o+w ${CP_BIN_DIR} + ... AND Apply Invalid Credential Provider Config ${CP_VALID} + Pattern Should Appear In Log Output ${CURSOR} must be owned by root and not writable by group or others + [Teardown] Run Keywords Command Should Work chmod o-w ${CP_BIN_DIR} + ... AND Remove Credential Provider Config + ... AND Restart MicroShift + + +*** Keywords *** +Setup + [Documentation] Test suite setup: install a mock provider binary and a provider config + Check Required Env Variables + Login MicroShift Host + Setup Kubeconfig + Command Should Work install -d -o root -g root -m 0755 ${CP_BIN_DIR} + Upload String To File ${CP_MOCK_SCRIPT} ${CP_MOCK_PROVIDER} + Command Should Work chmod 0755 ${CP_MOCK_PROVIDER} + Upload String To File ${CP_PROVIDER_CONFIG} ${CP_CONFIG_FILE} + +Teardown + [Documentation] Remove the drop-in and fixtures, restart MicroShift to restore clean state + Remove Credential Provider Config + Command Should Work rm -rf ${CP_BIN_DIR} ${CP_CONFIG_FILE} + Restart MicroShift + Remove Kubeconfig + Logout MicroShift Host + +Restart MicroShift With Cursor + [Documentation] Record the journal cursor, then restart MicroShift + ${cursor}= Get Journal Cursor + VAR ${CURSOR}= ${cursor} scope=TEST + Restart MicroShift + +Apply Credential Provider Config + [Documentation] Apply a drop-in config and restart MicroShift, recording the journal cursor + [Arguments] ${config} + Remove Drop In MicroShift Config ${CP_DROPIN} + Drop In MicroShift Config ${config} ${CP_DROPIN} + Restart MicroShift With Cursor + +Apply Invalid Credential Provider Config + [Documentation] Apply a drop-in config that should prevent MicroShift from starting + [Arguments] ${config} + Remove Drop In MicroShift Config ${CP_DROPIN} + Restart MicroShift + Drop In MicroShift Config ${config} ${CP_DROPIN} + ${cursor}= Get Journal Cursor + VAR ${CURSOR}= ${cursor} scope=TEST + Run Keyword And Expect Error 0 != 1 Restart MicroShift + +Remove Credential Provider Config + [Documentation] Remove the credential provider drop-in without restarting. + ... The next test's setup restarts MicroShift. + Remove Drop In MicroShift Config ${CP_DROPIN} From eec74ab5c97371a63db64b85ae00837dbbe5fc33 Mon Sep 17 00:00:00 2001 From: nhamza Date: Tue, 8 Sep 2026 13:03:16 +0300 Subject: [PATCH 2/4] OCPEDGE-2973: Pre-validate credential provider config structure Upstream kubelet calls os.Exit(1) when RegisterCredentialProviderPlugins fails (kuberuntime_manager.go:314). In MicroShift, where kubelet runs as a goroutine, that terminates the whole process after etcd, the API server, and the other components have started, and systemd restarts it into the same failure until the start-rate limit trips. The upstream missing-binary error also prints an empty path ("plugin binary executable did not exist"). Validate the three structural conditions that reach that exit, in Config.validate(), after the trusted-path rule and before the canonical paths are stored: - a configuration directory contains at least one .json/.yaml/.yml file; - each file decodes as a CredentialProviderConfig using the vendored k8s.io/kubelet/config/v1 types (apiVersion/kind checked, >=1 provider), so the check cannot drift from the kubelet in the same build; - every providers[].name resolves to an executable in the bin dir via exec.LookPath(filepath.Join(binDir, name)), reporting the joined path (never LookPath's empty-on-error return). Kubelet's semantic validation (matchImages, cache durations) is unexported and deliberately not replicated; those failures still reach the upstream exit path. Adds unit cases (TestValidateKubeletCredentialProviderStructure) and two Robot Framework cases (missing provider binary, empty configuration directory). Co-Authored-By: Claude Opus 4.8 --- pkg/config/kubelet.go | 156 +++++++++++ pkg/config/kubelet_test.go | 243 +++++++++++++++++- .../kubelet-credential-provider.robot | 36 +++ 3 files changed, 430 insertions(+), 5 deletions(-) diff --git a/pkg/config/kubelet.go b/pkg/config/kubelet.go index 51b3641214..2813eba775 100644 --- a/pkg/config/kubelet.go +++ b/pkg/config/kubelet.go @@ -4,11 +4,31 @@ import ( "errors" "fmt" "os" + "os/exec" "path/filepath" "slices" + "strings" "syscall" + + kubeletconfigv1 "k8s.io/kubelet/config/v1" + kubeletconfigv1alpha1 "k8s.io/kubelet/config/v1alpha1" + kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1" + "sigs.k8s.io/yaml" ) +// acceptedCredentialProviderConfigAPIVersions mirrors the CredentialProviderConfig +// apiVersions the vendored kubelet registers and accepts when it decodes the +// provider configuration (see decode() in +// vendor/k8s.io/kubernetes/pkg/credentialprovider/plugin/config.go, which accepts +// any version registered in its scheme for group kubelet.config.k8s.io). Deriving +// the set from the vendored SchemeGroupVersions keeps this structural check from +// drifting from the kubelet built into the same binary. +var acceptedCredentialProviderConfigAPIVersions = map[string]struct{}{ + kubeletconfigv1.SchemeGroupVersion.String(): {}, + kubeletconfigv1beta1.SchemeGroupVersion.String(): {}, + kubeletconfigv1alpha1.SchemeGroupVersion.String(): {}, +} + // isNotExistErr reports whether err means the path cannot exist. It covers both // a plain "no such file or directory" and ENOTDIR, which EvalSymlinks returns // when a non-directory appears mid-path (e.g. "/etc/cp.yaml/extra" where @@ -161,6 +181,18 @@ func (c *Config) validateKubeletCredentialProvider() error { kubeletImageCredentialProviderConfigPathKey, kubeletImageCredentialProviderBinDirKey, canonical[0]) } + // Structural pre-validation of the provider configuration, on the canonical + // paths. Upstream kubelet calls os.Exit(1) when provider registration fails, + // which in MicroShift terminates the whole process after other components are + // up. These checks turn the three structural conditions that reach that exit + // (empty config directory, undecodable config, unresolvable provider name) + // into ordinary fail-fast configuration errors. configPath (the configured + // value) is used only for the error prefix; the filesystem work uses the + // canonical paths. + if err := validateCredentialProviderStructure(configPath, canonical[0], canonical[1]); err != nil { + return err + } + // Store canonical paths only once both keys have passed validation. for i, p := range paths { *p.dst = canonical[i] @@ -254,6 +286,130 @@ func validateTrustedChain(canonical string) error { return nil } +// validateCredentialProviderStructure verifies the structural conditions that +// would otherwise make kubelet call os.Exit(1) at provider registration: a +// configuration directory with no configuration files, a file that does not +// decode as a CredentialProviderConfig, a file that declares no providers, and a +// provider name that does not resolve to an executable in the bin directory. It +// does not replicate kubelet's semantic validation. configKey is the configured +// value, used only in messages; canonicalConfigPath and canonicalBinDir are the +// symlink-resolved paths the checks operate on. +func validateCredentialProviderStructure(configKey, canonicalConfigPath, canonicalBinDir string) error { + prefix := func(err error) error { + return fmt.Errorf("error validating kubelet.%s (%q): %w", + kubeletImageCredentialProviderConfigPathKey, configKey, err) + } + + files, err := collectCredentialProviderConfigFiles(canonicalConfigPath) + if err != nil { + return prefix(err) + } + + for _, file := range files { + names, err := decodeCredentialProviderNames(file) + if err != nil { + return prefix(err) + } + for _, name := range names { + // Kubelet joins the bin dir and the provider name directly; a name + // containing a separator would escape the bin dir, so reject it. + if strings.Contains(name, "/") { + return prefix(fmt.Errorf("provider name %q must not contain \"/\"", name)) + } + // Report the joined path, never exec.LookPath's return value: on + // error LookPath returns an empty string, which is the upstream + // defect that prints "plugin binary executable did not exist". + joined := filepath.Join(canonicalBinDir, name) + if _, err := exec.LookPath(joined); err != nil { + return prefix(fmt.Errorf("provider %q has no executable at %q", name, joined)) + } + } + } + return nil +} + +// collectCredentialProviderConfigFiles returns the configuration files kubelet +// would read for canonicalConfigPath. A regular file yields itself; a directory +// yields its regular entries (symlinks resolved) whose extension is .json, +// .yaml or .yml, sorted lexicographically. An empty directory is an error. +func collectCredentialProviderConfigFiles(canonicalConfigPath string) ([]string, error) { + fi, err := os.Stat(canonicalConfigPath) + if err != nil { + return nil, err + } + if !fi.IsDir() { + return []string{canonicalConfigPath}, nil + } + + entries, err := os.ReadDir(canonicalConfigPath) + if err != nil { + return nil, err + } + + var files []string + for _, entry := range entries { + switch filepath.Ext(entry.Name()) { + case ".json", ".yaml", ".yml": + default: + continue + } + resolved, err := filepath.EvalSymlinks(filepath.Join(canonicalConfigPath, entry.Name())) + if err != nil { + if isNotExistErr(err) { + // A dangling symlink is not a configuration file kubelet reads. + continue + } + return nil, err + } + info, err := os.Lstat(resolved) + if err != nil { + return nil, err + } + if !info.Mode().IsRegular() { + continue + } + files = append(files, resolved) + } + slices.Sort(files) + + if len(files) == 0 { + return nil, fmt.Errorf("directory contains no .json, .yaml, or .yml configuration files") + } + return files, nil +} + +// decodeCredentialProviderNames decodes a single configuration file as a +// CredentialProviderConfig using the vendored k8s.io/kubelet/config/v1 types and +// returns the declared provider names. Decoding with the vendored types keeps the +// check aligned with the kubelet in the same build. It does not validate provider +// contents beyond apiVersion, kind, and the presence of at least one provider. +func decodeCredentialProviderNames(file string) ([]string, error) { + data, err := os.ReadFile(file) + if err != nil { + return nil, fmt.Errorf("unable to read file %q: %w", file, err) + } + + var cfg kubeletconfigv1.CredentialProviderConfig + if err := yaml.Unmarshal(data, &cfg); err != nil { + return nil, fmt.Errorf("file %q is not a valid CredentialProviderConfig: %w", file, err) + } + if cfg.Kind != "CredentialProviderConfig" { + return nil, fmt.Errorf("file %q is not a valid CredentialProviderConfig: unexpected kind %q", file, cfg.Kind) + } + if _, ok := acceptedCredentialProviderConfigAPIVersions[cfg.APIVersion]; !ok { + return nil, fmt.Errorf("file %q is not a valid CredentialProviderConfig: unsupported apiVersion %q", file, cfg.APIVersion) + } + if len(cfg.Providers) == 0 { + return nil, fmt.Errorf("file %q declares no providers", file) + } + + names := make([]string, 0, len(cfg.Providers)) + for _, p := range cfg.Providers { + names = append(names, p.Name) + } + return names, nil +} + // trustedPathComponents returns every path component of abs, ordered from the // root "/" down to abs itself. func trustedPathComponents(abs string) []string { diff --git a/pkg/config/kubelet_test.go b/pkg/config/kubelet_test.go index fa72532b3a..b3de55cee7 100644 --- a/pkg/config/kubelet_test.go +++ b/pkg/config/kubelet_test.go @@ -1,8 +1,10 @@ package config import ( + "fmt" "os" "path/filepath" + "strings" "syscall" "testing" @@ -186,6 +188,38 @@ func mkDir(t *testing.T, dir, name string) string { return p } +// credentialProviderConfigYAML returns a structurally valid +// CredentialProviderConfig declaring one provider per name. +func credentialProviderConfigYAML(names ...string) string { + var b strings.Builder + b.WriteString("apiVersion: kubelet.config.k8s.io/v1\n") + b.WriteString("kind: CredentialProviderConfig\n") + b.WriteString("providers:\n") + for _, n := range names { + fmt.Fprintf(&b, "- name: %s\n", n) + b.WriteString(" matchImages: [\"*.dkr.ecr.*.amazonaws.com\"]\n") + b.WriteString(" defaultCacheDuration: \"12h\"\n") + b.WriteString(" apiVersion: credentialprovider.kubelet.k8s.io/v1\n") + } + return b.String() +} + +// mkConfigFile writes a structurally valid config file naming the given +// providers and returns its path. +func mkConfigFile(t *testing.T, dir, name string, providers ...string) string { + t.Helper() + p := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(p, []byte(credentialProviderConfigYAML(providers...)), 0o644)) + return p +} + +// mkExecProvider writes an executable file (0o755) in binDir named name, so +// exec.LookPath resolves it. +func mkExecProvider(t *testing.T, binDir, name string) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(binDir, name), []byte("#!/bin/sh\n"), 0o755)) +} + func TestValidateKubeletCredentialProvider(t *testing.T) { t.Run("neither set is OK", func(t *testing.T) { c := &Config{} @@ -284,8 +318,9 @@ func TestValidateKubeletCredentialProvider(t *testing.T) { t.Run("config path is a regular file", func(t *testing.T) { dir := t.TempDir() - cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") withStatMock(t, nil) c := &Config{ KubeletImageCredentialProviderConfigPath: cfgFile, @@ -297,7 +332,9 @@ func TestValidateKubeletCredentialProvider(t *testing.T) { t.Run("config path is a directory", func(t *testing.T) { dir := t.TempDir() cfgDir := mkDir(t, dir, "cp.d") + mkConfigFile(t, cfgDir, "cp.yaml", "ecr-credential-provider") binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") withStatMock(t, nil) c := &Config{ KubeletImageCredentialProviderConfigPath: cfgDir, @@ -335,8 +372,9 @@ func TestValidateKubeletCredentialProvider(t *testing.T) { t.Run("valid config canonicalizes and stores canonical paths", func(t *testing.T) { dir := t.TempDir() - cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") withStatMock(t, nil) c := &Config{ KubeletImageCredentialProviderConfigPath: cfgFile, @@ -419,9 +457,9 @@ func TestValidateKubeletCredentialProviderTrustedPath(t *testing.T) { t.Run("compliant root-owned dir and file is OK", func(t *testing.T) { dir := t.TempDir() - cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") binDir := mkDir(t, dir, "bin") - mkFile(t, binDir, "ecr-credential-provider", 0o755) + mkExecProvider(t, binDir, "ecr-credential-provider") withStatMock(t, nil) c := &Config{ KubeletImageCredentialProviderConfigPath: cfgFile, @@ -433,7 +471,8 @@ func TestValidateKubeletCredentialProviderTrustedPath(t *testing.T) { t.Run("symlink to compliant target resolves to canonical", func(t *testing.T) { dir := t.TempDir() realDir := mkDir(t, dir, "real-bin") - cfgFile := mkFile(t, dir, "cp.yaml", 0o644) + mkExecProvider(t, realDir, "ecr-credential-provider") + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") link := filepath.Join(dir, "link-bin") require.NoError(t, os.Symlink(realDir, link)) withStatMock(t, nil) @@ -483,3 +522,197 @@ func TestValidateKubeletCredentialProviderTrustedPath(t *testing.T) { assert.Contains(t, err.Error(), "must be owned by root") }) } + +func TestValidateKubeletCredentialProviderStructure(t *testing.T) { + t.Run("directory with no matching files names the directory", func(t *testing.T) { + dir := t.TempDir() + cfgDir := mkDir(t, dir, "cp.d") + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgDir, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), cfgDir) + assert.Contains(t, err.Error(), "contains no .json, .yaml, or .yml") + }) + + t.Run("directory with only a .txt names the directory", func(t *testing.T) { + dir := t.TempDir() + cfgDir := mkDir(t, dir, "cp.d") + require.NoError(t, os.WriteFile(filepath.Join(cfgDir, "readme.txt"), []byte("x"), 0o644)) + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgDir, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), cfgDir) + assert.Contains(t, err.Error(), "contains no .json, .yaml, or .yml") + }) + + t.Run("wrong kind names the file", func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "cp.yaml") + require.NoError(t, os.WriteFile(cfgFile, []byte( + "apiVersion: kubelet.config.k8s.io/v1\nkind: NotThatKind\nproviders: []\n"), 0o644)) + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), cfgFile) + assert.Contains(t, err.Error(), "is not a valid CredentialProviderConfig") + }) + + t.Run("wrong apiVersion names the file", func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "cp.yaml") + require.NoError(t, os.WriteFile(cfgFile, []byte( + "apiVersion: example.com/v1\nkind: CredentialProviderConfig\nproviders: []\n"), 0o644)) + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), cfgFile) + assert.Contains(t, err.Error(), "is not a valid CredentialProviderConfig") + }) + + t.Run("malformed YAML names the file and includes the decode error", func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "cp.yaml") + require.NoError(t, os.WriteFile(cfgFile, []byte("providers: [ this is : not : yaml\n"), 0o644)) + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), cfgFile) + assert.Contains(t, err.Error(), "is not a valid CredentialProviderConfig") + }) + + t.Run("empty providers reports declares no providers", func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "cp.yaml") + require.NoError(t, os.WriteFile(cfgFile, []byte( + "apiVersion: kubelet.config.k8s.io/v1\nkind: CredentialProviderConfig\nproviders: []\n"), 0o644)) + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), cfgFile) + assert.Contains(t, err.Error(), "declares no providers") + }) + + t.Run("provider with no file in bin dir names provider and joined path", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + canonicalBin, _ := filepath.EvalSymlinks(binDir) + assert.Contains(t, err.Error(), `provider "ecr-credential-provider" has no executable at`) + assert.Contains(t, err.Error(), filepath.Join(canonicalBin, "ecr-credential-provider")) + }) + + t.Run("provider file present but not executable is rejected", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") + binDir := mkDir(t, dir, "bin") + // 0o644: present but not executable, so exec.LookPath rejects it. + require.NoError(t, os.WriteFile(filepath.Join(binDir, "ecr-credential-provider"), []byte("x"), 0o644)) + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), `provider "ecr-credential-provider" has no executable at`) + }) + + t.Run("provider name containing a slash is rejected", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkConfigFile(t, dir, "cp.yaml", "sub/provider") + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), `provider name "sub/provider" must not contain "/"`) + }) + + t.Run("valid single file and executable provider passes", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + assert.NoError(t, c.validateKubeletCredentialProvider()) + }) + + t.Run("valid directory with two files and both providers present passes", func(t *testing.T) { + dir := t.TempDir() + cfgDir := mkDir(t, dir, "cp.d") + mkConfigFile(t, cfgDir, "a.yaml", "provider-a") + mkConfigFile(t, cfgDir, "b.json", "provider-b") + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "provider-a") + mkExecProvider(t, binDir, "provider-b") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgDir, + KubeletImageCredentialProviderBinDir: binDir, + } + assert.NoError(t, c.validateKubeletCredentialProvider()) + }) + + t.Run("world-writable bin dir wins over unresolvable provider", func(t *testing.T) { + dir := t.TempDir() + // Config names a provider that does not exist, but the bin dir is + // world-writable; the trusted-path check runs first, so the permission + // error is reported, not the provider error. + cfgFile := mkConfigFile(t, dir, "cp.yaml", "no-such-provider") + binDir := mkDir(t, dir, "bin") + canonicalBin, _ := filepath.EvalSymlinks(binDir) + withStatMock(t, map[string]fakeStat{canonicalBin: {uid: 0, mode: 0o757}}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "must be owned by root and not writable by group or others") + assert.NotContains(t, err.Error(), "has no executable") + }) +} diff --git a/test/suites/configuration1/kubelet-credential-provider.robot b/test/suites/configuration1/kubelet-credential-provider.robot index 948266c115..979d105573 100644 --- a/test/suites/configuration1/kubelet-credential-provider.robot +++ b/test/suites/configuration1/kubelet-credential-provider.robot @@ -19,6 +19,7 @@ ${CP_CONFIGURED_LOG} Kubelet image credential provider configured ${CP_BIN_DIR} /usr/libexec/microshift/credential-providers ${CP_MOCK_PROVIDER} ${CP_BIN_DIR}/mock-credential-provider ${CP_CONFIG_FILE} /etc/microshift/credential-providers.yaml +${CP_CONFIG_DIR} /etc/microshift/credential-providers.d ${KUBELET_GENERATED_CONFIG} /var/lib/microshift/resources/kubelet/config/config.yaml ${CP_VALID} SEPARATOR=\n ... --- @@ -43,6 +44,20 @@ ${CP_PROVIDER_CONFIG} SEPARATOR=\n ... \ \ \ \ \ \ - "registry.example.invalid" ... \ \ \ \ defaultCacheDuration: "1m" ... \ \ \ \ apiVersion: credentialprovider.kubelet.k8s.io/v1 +${CP_BAD_PROVIDER_CONFIG} SEPARATOR=\n +... apiVersion: kubelet.config.k8s.io/v1 +... kind: CredentialProviderConfig +... providers: +... \ \ - name: no-such-provider +... \ \ \ \ matchImages: +... \ \ \ \ \ \ - "registry.example.invalid" +... \ \ \ \ defaultCacheDuration: "1m" +... \ \ \ \ apiVersion: credentialprovider.kubelet.k8s.io/v1 +${CP_EMPTY_DIR_CONFIG} SEPARATOR=\n +... --- +... kubelet: +... \ \ imageCredentialProviderConfigPath: ${CP_CONFIG_DIR} +... \ \ imageCredentialProviderBinDir: ${CP_BIN_DIR} ${CP_MOCK_SCRIPT} SEPARATOR=\n ... \#!/bin/bash ... \# Mock kubelet image credential provider: returns static credentials. @@ -95,6 +110,27 @@ World Writable Bin Directory Prevents Start ... AND Remove Credential Provider Config ... AND Restart MicroShift +Missing Provider Binary Prevents Start + [Documentation] MicroShift fails to start when a provider names a binary absent from the bin directory. + ... Validation fails before kubelet is configured, so the "configured" line must not appear. + [Setup] Run Keywords Upload String To File ${CP_BAD_PROVIDER_CONFIG} ${CP_CONFIG_FILE} + ... AND Apply Invalid Credential Provider Config ${CP_VALID} + Pattern Should Appear In Log Output ${CURSOR} no executable at + Pattern Should Appear In Log Output ${CURSOR} no-such-provider + Pattern Should Not Appear In Log Output ${CURSOR} ${CP_CONFIGURED_LOG} + [Teardown] Run Keywords Upload String To File ${CP_PROVIDER_CONFIG} ${CP_CONFIG_FILE} + ... AND Remove Credential Provider Config + ... AND Restart MicroShift + +Empty Configuration Directory Prevents Start + [Documentation] MicroShift fails to start when the configuration directory holds no config files + [Setup] Run Keywords Command Should Work install -d -o root -g root -m 0755 ${CP_CONFIG_DIR} + ... AND Apply Invalid Credential Provider Config ${CP_EMPTY_DIR_CONFIG} + Pattern Should Appear In Log Output ${CURSOR} contains no .json, .yaml, or .yml + [Teardown] Run Keywords Command Should Work rm -rf ${CP_CONFIG_DIR} + ... AND Remove Credential Provider Config + ... AND Restart MicroShift + *** Keywords *** Setup From 86190e2414dcad286e6062ec38dc92889874f173 Mon Sep 17 00:00:00 2001 From: nhamza Date: Tue, 8 Sep 2026 13:30:03 +0300 Subject: [PATCH 3/4] OCPEDGE-2973: Decode provider config with kubelet's strict decoder; reject ACLs Decode each credential provider config file with the same strict decoder the vendored kubelet uses (EnableStrict, built from k8s.io/kubernetes/pkg/kubelet/ apis/config and its v1/v1beta1/v1alpha1 sub-packages), so unknown fields are rejected and all three accepted API versions decode to the internal type exactly as the kubelet in the same build does. Report an unreadable file (EACCES) as needing root rather than as invalid. Reject any trusted-path component or directory entry that carries an extended POSIX ACL (system.posix_acl_access), which can grant write access the mode bits do not reveal. Match kubelet's directory read: kubelet does not resolve symlinks and reads every matching entry with os.ReadFile, so a dangling symlink or a non-regular entry (e.g. a FIFO) with a .json/.yaml/.yml extension is now an error rather than a skip, closing a gap that would otherwise reach kubelet's os.Exit(1). Co-Authored-By: Claude Opus 4.8 --- pkg/config/kubelet.go | 136 ++++++++--- pkg/config/kubelet_test.go | 222 +++++++++++++++++- .../kubelet-credential-provider.robot | 10 + 3 files changed, 333 insertions(+), 35 deletions(-) diff --git a/pkg/config/kubelet.go b/pkg/config/kubelet.go index 2813eba775..439769ae67 100644 --- a/pkg/config/kubelet.go +++ b/pkg/config/kubelet.go @@ -10,24 +10,34 @@ import ( "strings" "syscall" - kubeletconfigv1 "k8s.io/kubelet/config/v1" - kubeletconfigv1alpha1 "k8s.io/kubelet/config/v1alpha1" - kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1" - "sigs.k8s.io/yaml" + "golang.org/x/sys/unix" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + kubeletconfig "k8s.io/kubernetes/pkg/kubelet/apis/config" + kubeletconfigv1 "k8s.io/kubernetes/pkg/kubelet/apis/config/v1" + kubeletconfigv1alpha1 "k8s.io/kubernetes/pkg/kubelet/apis/config/v1alpha1" + kubeletconfigv1beta1 "k8s.io/kubernetes/pkg/kubelet/apis/config/v1beta1" ) -// acceptedCredentialProviderConfigAPIVersions mirrors the CredentialProviderConfig -// apiVersions the vendored kubelet registers and accepts when it decodes the -// provider configuration (see decode() in -// vendor/k8s.io/kubernetes/pkg/credentialprovider/plugin/config.go, which accepts -// any version registered in its scheme for group kubelet.config.k8s.io). Deriving -// the set from the vendored SchemeGroupVersions keeps this structural check from -// drifting from the kubelet built into the same binary. -var acceptedCredentialProviderConfigAPIVersions = map[string]struct{}{ - kubeletconfigv1.SchemeGroupVersion.String(): {}, - kubeletconfigv1beta1.SchemeGroupVersion.String(): {}, - kubeletconfigv1alpha1.SchemeGroupVersion.String(): {}, -} +// credentialProviderCodecs is the same strict decoder the vendored kubelet uses +// to read the credential provider configuration: see the scheme setup in +// vendor/k8s.io/kubernetes/pkg/credentialprovider/plugin/plugin.go (lines +// 69-70, 135-138) and decode() in the sibling config.go. Strict decoding +// rejects unknown fields, and all three API versions kubelet accepts +// (v1alpha1, v1beta1, v1 of kubelet.config.k8s.io) are registered together with +// the internal type and its conversions. Building the decoder from the same +// vendored packages keeps this structural check from diverging from the kubelet +// compiled into the same binary; a lenient decoder would let a typo'd field +// through to the os.Exit at registration. +var credentialProviderCodecs = func() serializer.CodecFactory { + s := runtime.NewScheme() + utilruntime.Must(kubeletconfig.AddToScheme(s)) + utilruntime.Must(kubeletconfigv1alpha1.AddToScheme(s)) + utilruntime.Must(kubeletconfigv1beta1.AddToScheme(s)) + utilruntime.Must(kubeletconfigv1.AddToScheme(s)) + return serializer.NewCodecFactory(s, serializer.EnableStrict) +}() // isNotExistErr reports whether err means the path cannot exist. It covers both // a plain "no such file or directory" and ENOTDIR, which EvalSymlinks returns @@ -66,6 +76,23 @@ var statForTrust = func(path string) (uid uint32, mode os.FileMode, err error) { return st.Uid, fi.Mode(), nil } +// aclForTrust reports whether path carries an extended POSIX access ACL. Mode +// bits do not reveal ACL write grants (for example `setfacl -m u:x:rwx dir` +// leaves the mode at 0755), so the trusted-path rule rejects any component that +// carries one. It is a package-level variable so tests can simulate ACLs without +// setfacl or root. A filesystem that stores no ACL for the object (ENODATA) or +// does not support ACLs (ENOTSUP) reports false. +var aclForTrust = func(path string) (bool, error) { + sz, err := unix.Lgetxattr(path, "system.posix_acl_access", nil) + if err != nil { + if errors.Is(err, unix.ENODATA) || errors.Is(err, unix.ENOTSUP) { + return false, nil + } + return false, err + } + return sz > 0, nil +} + // readKubeletCredentialProviderKeys copies the two credential-provider keys from // the schemaless kubelet map into the typed Config fields. c.Kubelet is left // untouched; canonicalization happens later, during validation. @@ -282,6 +309,22 @@ func validateTrustedChain(canonical string) error { if uid != 0 || mode&0o022 != 0 { return fmt.Errorf("%q must be owned by root and not writable by group or others", component) } + if err := checkNoExtendedACL(component); err != nil { + return err + } + } + return nil +} + +// checkNoExtendedACL rejects a path that carries an extended POSIX ACL, which can +// grant write access that the mode bits do not show. +func checkNoExtendedACL(path string) error { + hasACL, err := aclForTrust(path) + if err != nil { + return err + } + if hasACL { + return fmt.Errorf("%q must not have an extended ACL", path) } return nil } @@ -330,8 +373,13 @@ func validateCredentialProviderStructure(configKey, canonicalConfigPath, canonic // collectCredentialProviderConfigFiles returns the configuration files kubelet // would read for canonicalConfigPath. A regular file yields itself; a directory -// yields its regular entries (symlinks resolved) whose extension is .json, -// .yaml or .yml, sorted lexicographically. An empty directory is an error. +// yields its entries whose extension is .json, .yaml or .yml, sorted +// lexicographically. Directory entries are skipped (kubelet checks +// !entry.IsDir()). Kubelet does not resolve symlinks and reads whatever remains +// with os.ReadFile, so a dangling symlink or a non-regular entry with a +// matching extension is an error here rather than a skip: kubelet would fail on +// (or block on, for a FIFO) it and reach os.Exit. An empty directory is an +// error. func collectCredentialProviderConfigFiles(canonicalConfigPath string) ([]string, error) { fi, err := os.Stat(canonicalConfigPath) if err != nil { @@ -353,11 +401,15 @@ func collectCredentialProviderConfigFiles(canonicalConfigPath string) ([]string, default: continue } - resolved, err := filepath.EvalSymlinks(filepath.Join(canonicalConfigPath, entry.Name())) + entryPath := filepath.Join(canonicalConfigPath, entry.Name()) + resolved, err := filepath.EvalSymlinks(entryPath) if err != nil { + // Kubelet does not resolve symlinks: it includes any matching entry + // in configFiles and later os.ReadFile fails, reaching os.Exit. A + // dangling symlink with a matching extension is therefore an error + // here, not a skip. if isNotExistErr(err) { - // A dangling symlink is not a configuration file kubelet reads. - continue + return nil, fmt.Errorf("configuration file %q does not exist (dangling symlink)", entryPath) } return nil, err } @@ -365,9 +417,16 @@ func collectCredentialProviderConfigFiles(canonicalConfigPath string) ([]string, if err != nil { return nil, err } - if !info.Mode().IsRegular() { + if info.IsDir() { + // Kubelet skips directories (it checks !entry.IsDir()); a directory + // named e.g. foo.yaml is ignored by both. continue } + if !info.Mode().IsRegular() { + // Kubelet would os.ReadFile a non-regular entry (e.g. a FIFO named + // x.yaml) and block or fail at registration; reject it here. + return nil, fmt.Errorf("configuration file %q is not a regular file", resolved) + } files = append(files, resolved) } slices.Sort(files) @@ -378,26 +437,37 @@ func collectCredentialProviderConfigFiles(canonicalConfigPath string) ([]string, return files, nil } -// decodeCredentialProviderNames decodes a single configuration file as a -// CredentialProviderConfig using the vendored k8s.io/kubelet/config/v1 types and -// returns the declared provider names. Decoding with the vendored types keeps the -// check aligned with the kubelet in the same build. It does not validate provider -// contents beyond apiVersion, kind, and the presence of at least one provider. +// decodeCredentialProviderNames decodes a single configuration file the same way +// kubelet does (strict, via credentialProviderCodecs) and returns the declared +// provider names. Decoding with the vendored kubelet packages keeps the check +// aligned with the kubelet in the same build: unknown fields are rejected and all +// three accepted API versions convert to the internal type. It does not replicate +// kubelet's semantic validation beyond kind, group, and the presence of at least +// one provider. func decodeCredentialProviderNames(file string) ([]string, error) { data, err := os.ReadFile(file) if err != nil { + // A non-root reader (typically `microshift show-config` against a 0600 + // file) cannot read the file; say so rather than reporting it invalid. + if errors.Is(err, syscall.EACCES) { + return nil, fmt.Errorf("cannot read %q: permission denied (run as root)", file) + } return nil, fmt.Errorf("unable to read file %q: %w", file, err) } - var cfg kubeletconfigv1.CredentialProviderConfig - if err := yaml.Unmarshal(data, &cfg); err != nil { + obj, gvk, err := credentialProviderCodecs.UniversalDecoder().Decode(data, nil, nil) + if err != nil { return nil, fmt.Errorf("file %q is not a valid CredentialProviderConfig: %w", file, err) } - if cfg.Kind != "CredentialProviderConfig" { - return nil, fmt.Errorf("file %q is not a valid CredentialProviderConfig: unexpected kind %q", file, cfg.Kind) + if gvk.Kind != "CredentialProviderConfig" { + return nil, fmt.Errorf("file %q is not a valid CredentialProviderConfig: unexpected kind %q", file, gvk.Kind) } - if _, ok := acceptedCredentialProviderConfigAPIVersions[cfg.APIVersion]; !ok { - return nil, fmt.Errorf("file %q is not a valid CredentialProviderConfig: unsupported apiVersion %q", file, cfg.APIVersion) + if gvk.Group != kubeletconfig.GroupName { + return nil, fmt.Errorf("file %q is not a valid CredentialProviderConfig: unexpected group %q", file, gvk.Group) + } + cfg, ok := obj.(*kubeletconfig.CredentialProviderConfig) + if !ok { + return nil, fmt.Errorf("file %q is not a valid CredentialProviderConfig: unexpected type %T", file, obj) } if len(cfg.Providers) == 0 { return nil, fmt.Errorf("file %q declares no providers", file) diff --git a/pkg/config/kubelet_test.go b/pkg/config/kubelet_test.go index b3de55cee7..281d5c60a3 100644 --- a/pkg/config/kubelet_test.go +++ b/pkg/config/kubelet_test.go @@ -45,6 +45,18 @@ func withStatMock(t *testing.T, overrides map[string]fakeStat) { t.Cleanup(func() { statForTrust = orig }) } +// withACLMock installs an aclForTrust that reports the given canonical paths as +// carrying an extended ACL and every other path as carrying none, so ACL +// rejection can be exercised without setfacl or root. +func withACLMock(t *testing.T, withACL map[string]bool) { + t.Helper() + orig := aclForTrust + aclForTrust = func(path string) (bool, error) { + return withACL[path], nil + } + t.Cleanup(func() { aclForTrust = orig }) +} + func TestReadKubeletCredentialProviderKeys(t *testing.T) { ttests := []struct { name string @@ -189,10 +201,17 @@ func mkDir(t *testing.T, dir, name string) string { } // credentialProviderConfigYAML returns a structurally valid -// CredentialProviderConfig declaring one provider per name. +// CredentialProviderConfig (kubelet.config.k8s.io/v1) declaring one provider per +// name. func credentialProviderConfigYAML(names ...string) string { + return credentialProviderConfigYAMLAt("kubelet.config.k8s.io/v1", names...) +} + +// credentialProviderConfigYAMLAt is credentialProviderConfigYAML with an explicit +// config apiVersion, so the v1beta1 and v1alpha1 code paths can be exercised. +func credentialProviderConfigYAMLAt(apiVersion string, names ...string) string { var b strings.Builder - b.WriteString("apiVersion: kubelet.config.k8s.io/v1\n") + fmt.Fprintf(&b, "apiVersion: %s\n", apiVersion) b.WriteString("kind: CredentialProviderConfig\n") b.WriteString("providers:\n") for _, n := range names { @@ -521,6 +540,76 @@ func TestValidateKubeletCredentialProviderTrustedPath(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "must be owned by root") }) + + t.Run("extended ACL on the bin dir is rejected", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") + canonicalBin, _ := filepath.EvalSymlinks(binDir) + withStatMock(t, nil) + withACLMock(t, map[string]bool{canonicalBin: true}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), canonicalBin) + assert.Contains(t, err.Error(), "must not have an extended ACL") + }) + + t.Run("extended ACL on a bin-dir entry is rejected", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") + entry := filepath.Join(binDir, "ecr-credential-provider") + canonicalEntry, _ := filepath.EvalSymlinks(entry) + withStatMock(t, nil) + withACLMock(t, map[string]bool{canonicalEntry: true}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), canonicalEntry) + assert.Contains(t, err.Error(), "must not have an extended ACL") + }) + + t.Run("extended ACL on an ancestor is rejected", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") + canonicalBin, _ := filepath.EvalSymlinks(binDir) + ancestor := filepath.Dir(canonicalBin) + withStatMock(t, nil) + withACLMock(t, map[string]bool{ancestor: true}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), ancestor) + assert.Contains(t, err.Error(), "must not have an extended ACL") + }) + + t.Run("no extended ACL passes", func(t *testing.T) { + dir := t.TempDir() + cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") + withStatMock(t, nil) + withACLMock(t, map[string]bool{}) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + assert.NoError(t, c.validateKubeletCredentialProvider()) + }) } func TestValidateKubeletCredentialProviderStructure(t *testing.T) { @@ -697,6 +786,80 @@ func TestValidateKubeletCredentialProviderStructure(t *testing.T) { assert.NoError(t, c.validateKubeletCredentialProvider()) }) + t.Run("v1beta1 config decodes and passes", func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "cp.yaml") + require.NoError(t, os.WriteFile(cfgFile, + []byte(credentialProviderConfigYAMLAt("kubelet.config.k8s.io/v1beta1", "ecr-credential-provider")), 0o644)) + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + assert.NoError(t, c.validateKubeletCredentialProvider()) + }) + + t.Run("v1alpha1 config decodes and passes", func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "cp.yaml") + require.NoError(t, os.WriteFile(cfgFile, + []byte(credentialProviderConfigYAMLAt("kubelet.config.k8s.io/v1alpha1", "ecr-credential-provider")), 0o644)) + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + assert.NoError(t, c.validateKubeletCredentialProvider()) + }) + + t.Run("unknown field is rejected by strict decoding", func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "cp.yaml") + // matchImage (singular) is not a field of CredentialProvider; strict + // decoding rejects it, the way kubelet does at registration. + require.NoError(t, os.WriteFile(cfgFile, []byte( + "apiVersion: kubelet.config.k8s.io/v1\n"+ + "kind: CredentialProviderConfig\n"+ + "providers:\n"+ + "- name: ecr-credential-provider\n"+ + " matchImage: [\"*.example.com\"]\n"), 0o644)) + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), cfgFile) + assert.Contains(t, err.Error(), "is not a valid CredentialProviderConfig") + }) + + t.Run("unreadable file reports run-as-root", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root can read a 0000 file; this path cannot be reproduced as root") + } + dir := t.TempDir() + cfgFile := filepath.Join(dir, "cp.yaml") + require.NoError(t, os.WriteFile(cfgFile, + []byte(credentialProviderConfigYAML("ecr-credential-provider")), 0o000)) + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "ecr-credential-provider") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgFile, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), "permission denied (run as root)") + }) + t.Run("world-writable bin dir wins over unresolvable provider", func(t *testing.T) { dir := t.TempDir() // Config names a provider that does not exist, but the bin dir is @@ -715,4 +878,59 @@ func TestValidateKubeletCredentialProviderStructure(t *testing.T) { assert.Contains(t, err.Error(), "must be owned by root and not writable by group or others") assert.NotContains(t, err.Error(), "has no executable") }) + + t.Run("dangling .yaml symlink alongside a valid file is an error naming the link", func(t *testing.T) { + dir := t.TempDir() + cfgDir := mkDir(t, dir, "cp.d") + mkConfigFile(t, cfgDir, "a.yaml", "provider-a") + // A symlink with a matching extension whose target does not exist: + // kubelet would include it and fail at os.ReadFile, reaching os.Exit. + // The per-entry trusted-path walk (validateDirEntries) resolves every + // directory entry and rejects the broken link before the structural + // stage runs, so the observable message is "does not exist"; the + // collectCredentialProviderConfigFiles branch (asserted directly below) + // is the same defense at the structural stage. + dangling := filepath.Join(cfgDir, "b.yaml") + require.NoError(t, os.Symlink(filepath.Join(dir, "nonexistent-target.yaml"), dangling)) + binDir := mkDir(t, dir, "bin") + mkExecProvider(t, binDir, "provider-a") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgDir, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), dangling) + assert.Contains(t, err.Error(), "does not exist") + }) + + t.Run("collectCredentialProviderConfigFiles rejects a dangling symlink", func(t *testing.T) { + dir := t.TempDir() + cfgDir := mkDir(t, dir, "cp.d") + mkConfigFile(t, cfgDir, "a.yaml", "provider-a") + dangling := filepath.Join(cfgDir, "b.yaml") + require.NoError(t, os.Symlink(filepath.Join(dir, "nonexistent-target.yaml"), dangling)) + _, err := collectCredentialProviderConfigFiles(cfgDir) + require.Error(t, err) + assert.Contains(t, err.Error(), dangling) + assert.Contains(t, err.Error(), "dangling symlink") + }) + + t.Run("FIFO named x.yaml is rejected as not a regular file", func(t *testing.T) { + dir := t.TempDir() + cfgDir := mkDir(t, dir, "cp.d") + fifo := filepath.Join(cfgDir, "x.yaml") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + binDir := mkDir(t, dir, "bin") + withStatMock(t, nil) + c := &Config{ + KubeletImageCredentialProviderConfigPath: cfgDir, + KubeletImageCredentialProviderBinDir: binDir, + } + err := c.validateKubeletCredentialProvider() + require.Error(t, err) + assert.Contains(t, err.Error(), fifo) + assert.Contains(t, err.Error(), "is not a regular file") + }) } diff --git a/test/suites/configuration1/kubelet-credential-provider.robot b/test/suites/configuration1/kubelet-credential-provider.robot index 979d105573..4ab66ff693 100644 --- a/test/suites/configuration1/kubelet-credential-provider.robot +++ b/test/suites/configuration1/kubelet-credential-provider.robot @@ -110,6 +110,16 @@ World Writable Bin Directory Prevents Start ... AND Remove Credential Provider Config ... AND Restart MicroShift +Extended ACL On Bin Directory Prevents Start + [Documentation] MicroShift refuses to start when the bin directory carries an extended POSIX + ... ACL, which can grant write access the mode bits do not show. + [Setup] Run Keywords Command Should Work setfacl -m u:nobody:rwx ${CP_BIN_DIR} + ... AND Apply Invalid Credential Provider Config ${CP_VALID} + Pattern Should Appear In Log Output ${CURSOR} must not have an extended ACL + [Teardown] Run Keywords Command Should Work setfacl -b ${CP_BIN_DIR} + ... AND Remove Credential Provider Config + ... AND Restart MicroShift + Missing Provider Binary Prevents Start [Documentation] MicroShift fails to start when a provider names a binary absent from the bin directory. ... Validation fails before kubelet is configured, so the "configured" line must not appear. From c51a2f42cbd70b0547c2dc4ca73cc3b486b1cee0 Mon Sep 17 00:00:00 2001 From: nhamza Date: Tue, 8 Sep 2026 17:09:56 +0300 Subject: [PATCH 4/4] OCPEDGE-2973: Drop redundant ACL and EACCES checks Host validation showed both checks are unnecessary: - Extended-ACL rejection: Linux reports the POSIX ACL mask in the group mode bits, so any ACL entry with effective write access already fails the mode&0o022 test in validateTrustedChain. A dedicated system.posix_acl_access xattr check only ever fired for read-only ACLs, which grant no write and are harmless. Remove aclForTrust, checkNoExtendedACL, the call in validateTrustedChain, the now-unused golang.org/x/sys/unix import, the four ACL unit tests, the withACLMock helper, and the Robot "Extended ACL On Bin Directory Prevents Start" case (suite is now 7 cases). - EACCES "run as root" branch in decodeCredentialProviderNames: every caller of ActiveConfig() runs as root (show-config has a root guard at pkg/cmd/showConfig.go:32), so the branch is unreachable. Remove it and keep the generic "unable to read file %q: %w"; remove the corresponding test. Co-Authored-By: Claude Opus 4.8 --- pkg/config/kubelet.go | 39 ------- pkg/config/kubelet_test.go | 102 ------------------ .../kubelet-credential-provider.robot | 10 -- 3 files changed, 151 deletions(-) diff --git a/pkg/config/kubelet.go b/pkg/config/kubelet.go index 439769ae67..be94ff3d18 100644 --- a/pkg/config/kubelet.go +++ b/pkg/config/kubelet.go @@ -10,7 +10,6 @@ import ( "strings" "syscall" - "golang.org/x/sys/unix" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -76,23 +75,6 @@ var statForTrust = func(path string) (uid uint32, mode os.FileMode, err error) { return st.Uid, fi.Mode(), nil } -// aclForTrust reports whether path carries an extended POSIX access ACL. Mode -// bits do not reveal ACL write grants (for example `setfacl -m u:x:rwx dir` -// leaves the mode at 0755), so the trusted-path rule rejects any component that -// carries one. It is a package-level variable so tests can simulate ACLs without -// setfacl or root. A filesystem that stores no ACL for the object (ENODATA) or -// does not support ACLs (ENOTSUP) reports false. -var aclForTrust = func(path string) (bool, error) { - sz, err := unix.Lgetxattr(path, "system.posix_acl_access", nil) - if err != nil { - if errors.Is(err, unix.ENODATA) || errors.Is(err, unix.ENOTSUP) { - return false, nil - } - return false, err - } - return sz > 0, nil -} - // readKubeletCredentialProviderKeys copies the two credential-provider keys from // the schemaless kubelet map into the typed Config fields. c.Kubelet is left // untouched; canonicalization happens later, during validation. @@ -309,22 +291,6 @@ func validateTrustedChain(canonical string) error { if uid != 0 || mode&0o022 != 0 { return fmt.Errorf("%q must be owned by root and not writable by group or others", component) } - if err := checkNoExtendedACL(component); err != nil { - return err - } - } - return nil -} - -// checkNoExtendedACL rejects a path that carries an extended POSIX ACL, which can -// grant write access that the mode bits do not show. -func checkNoExtendedACL(path string) error { - hasACL, err := aclForTrust(path) - if err != nil { - return err - } - if hasACL { - return fmt.Errorf("%q must not have an extended ACL", path) } return nil } @@ -447,11 +413,6 @@ func collectCredentialProviderConfigFiles(canonicalConfigPath string) ([]string, func decodeCredentialProviderNames(file string) ([]string, error) { data, err := os.ReadFile(file) if err != nil { - // A non-root reader (typically `microshift show-config` against a 0600 - // file) cannot read the file; say so rather than reporting it invalid. - if errors.Is(err, syscall.EACCES) { - return nil, fmt.Errorf("cannot read %q: permission denied (run as root)", file) - } return nil, fmt.Errorf("unable to read file %q: %w", file, err) } diff --git a/pkg/config/kubelet_test.go b/pkg/config/kubelet_test.go index 281d5c60a3..44363163ca 100644 --- a/pkg/config/kubelet_test.go +++ b/pkg/config/kubelet_test.go @@ -45,18 +45,6 @@ func withStatMock(t *testing.T, overrides map[string]fakeStat) { t.Cleanup(func() { statForTrust = orig }) } -// withACLMock installs an aclForTrust that reports the given canonical paths as -// carrying an extended ACL and every other path as carrying none, so ACL -// rejection can be exercised without setfacl or root. -func withACLMock(t *testing.T, withACL map[string]bool) { - t.Helper() - orig := aclForTrust - aclForTrust = func(path string) (bool, error) { - return withACL[path], nil - } - t.Cleanup(func() { aclForTrust = orig }) -} - func TestReadKubeletCredentialProviderKeys(t *testing.T) { ttests := []struct { name string @@ -540,76 +528,6 @@ func TestValidateKubeletCredentialProviderTrustedPath(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "must be owned by root") }) - - t.Run("extended ACL on the bin dir is rejected", func(t *testing.T) { - dir := t.TempDir() - cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") - binDir := mkDir(t, dir, "bin") - mkExecProvider(t, binDir, "ecr-credential-provider") - canonicalBin, _ := filepath.EvalSymlinks(binDir) - withStatMock(t, nil) - withACLMock(t, map[string]bool{canonicalBin: true}) - c := &Config{ - KubeletImageCredentialProviderConfigPath: cfgFile, - KubeletImageCredentialProviderBinDir: binDir, - } - err := c.validateKubeletCredentialProvider() - require.Error(t, err) - assert.Contains(t, err.Error(), canonicalBin) - assert.Contains(t, err.Error(), "must not have an extended ACL") - }) - - t.Run("extended ACL on a bin-dir entry is rejected", func(t *testing.T) { - dir := t.TempDir() - cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") - binDir := mkDir(t, dir, "bin") - mkExecProvider(t, binDir, "ecr-credential-provider") - entry := filepath.Join(binDir, "ecr-credential-provider") - canonicalEntry, _ := filepath.EvalSymlinks(entry) - withStatMock(t, nil) - withACLMock(t, map[string]bool{canonicalEntry: true}) - c := &Config{ - KubeletImageCredentialProviderConfigPath: cfgFile, - KubeletImageCredentialProviderBinDir: binDir, - } - err := c.validateKubeletCredentialProvider() - require.Error(t, err) - assert.Contains(t, err.Error(), canonicalEntry) - assert.Contains(t, err.Error(), "must not have an extended ACL") - }) - - t.Run("extended ACL on an ancestor is rejected", func(t *testing.T) { - dir := t.TempDir() - cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") - binDir := mkDir(t, dir, "bin") - mkExecProvider(t, binDir, "ecr-credential-provider") - canonicalBin, _ := filepath.EvalSymlinks(binDir) - ancestor := filepath.Dir(canonicalBin) - withStatMock(t, nil) - withACLMock(t, map[string]bool{ancestor: true}) - c := &Config{ - KubeletImageCredentialProviderConfigPath: cfgFile, - KubeletImageCredentialProviderBinDir: binDir, - } - err := c.validateKubeletCredentialProvider() - require.Error(t, err) - assert.Contains(t, err.Error(), ancestor) - assert.Contains(t, err.Error(), "must not have an extended ACL") - }) - - t.Run("no extended ACL passes", func(t *testing.T) { - dir := t.TempDir() - cfgFile := mkConfigFile(t, dir, "cp.yaml", "ecr-credential-provider") - binDir := mkDir(t, dir, "bin") - mkExecProvider(t, binDir, "ecr-credential-provider") - withStatMock(t, nil) - withACLMock(t, map[string]bool{}) - c := &Config{ - KubeletImageCredentialProviderConfigPath: cfgFile, - KubeletImageCredentialProviderBinDir: binDir, - } - assert.NoError(t, c.validateKubeletCredentialProvider()) - }) } func TestValidateKubeletCredentialProviderStructure(t *testing.T) { @@ -840,26 +758,6 @@ func TestValidateKubeletCredentialProviderStructure(t *testing.T) { assert.Contains(t, err.Error(), "is not a valid CredentialProviderConfig") }) - t.Run("unreadable file reports run-as-root", func(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("root can read a 0000 file; this path cannot be reproduced as root") - } - dir := t.TempDir() - cfgFile := filepath.Join(dir, "cp.yaml") - require.NoError(t, os.WriteFile(cfgFile, - []byte(credentialProviderConfigYAML("ecr-credential-provider")), 0o000)) - binDir := mkDir(t, dir, "bin") - mkExecProvider(t, binDir, "ecr-credential-provider") - withStatMock(t, nil) - c := &Config{ - KubeletImageCredentialProviderConfigPath: cfgFile, - KubeletImageCredentialProviderBinDir: binDir, - } - err := c.validateKubeletCredentialProvider() - require.Error(t, err) - assert.Contains(t, err.Error(), "permission denied (run as root)") - }) - t.Run("world-writable bin dir wins over unresolvable provider", func(t *testing.T) { dir := t.TempDir() // Config names a provider that does not exist, but the bin dir is diff --git a/test/suites/configuration1/kubelet-credential-provider.robot b/test/suites/configuration1/kubelet-credential-provider.robot index 4ab66ff693..979d105573 100644 --- a/test/suites/configuration1/kubelet-credential-provider.robot +++ b/test/suites/configuration1/kubelet-credential-provider.robot @@ -110,16 +110,6 @@ World Writable Bin Directory Prevents Start ... AND Remove Credential Provider Config ... AND Restart MicroShift -Extended ACL On Bin Directory Prevents Start - [Documentation] MicroShift refuses to start when the bin directory carries an extended POSIX - ... ACL, which can grant write access the mode bits do not show. - [Setup] Run Keywords Command Should Work setfacl -m u:nobody:rwx ${CP_BIN_DIR} - ... AND Apply Invalid Credential Provider Config ${CP_VALID} - Pattern Should Appear In Log Output ${CURSOR} must not have an extended ACL - [Teardown] Run Keywords Command Should Work setfacl -b ${CP_BIN_DIR} - ... AND Remove Credential Provider Config - ... AND Restart MicroShift - Missing Provider Binary Prevents Start [Documentation] MicroShift fails to start when a provider names a binary absent from the bin directory. ... Validation fails before kubelet is configured, so the "configured" line must not appear.