From bf48a27239603af01575fd83d5110089430d6c41 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Tue, 26 Nov 2024 18:04:11 +0100 Subject: [PATCH] config: add path validation WIP TBD Signed-off-by: Francesco Romani --- pkg/config/cfgfile.go | 18 ++++++---- pkg/config/cfgfile_test.go | 3 ++ pkg/config/config.go | 23 +++++++++++-- pkg/config/config_test.go | 6 +++- pkg/config/flags_test.go | 19 +++++++++-- pkg/config/validation.go | 70 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 127 insertions(+), 12 deletions(-) diff --git a/pkg/config/cfgfile.go b/pkg/config/cfgfile.go index 3e0cfb92..8a3c8eaf 100644 --- a/pkg/config/cfgfile.go +++ b/pkg/config/cfgfile.go @@ -94,17 +94,21 @@ func fromDaemonFiles(pArgs *ProgArgs, configPathRoot string) error { // this directory may be missing, that's expected and fine configletDir := filepath.Join(configPathRoot, "daemon", "config.yaml.d") - if configLets, err := os.ReadDir(configletDir); err == nil { - for _, configLet := range configLets { - if !configLet.Type().IsRegular() { - klog.Infof("configlet %q not regular file: ignored", configLet.Name()) + if configlets, err := os.ReadDir(configletDir); err == nil { + for _, configlet := range configlets { + if !configlet.Type().IsRegular() { + klog.Infof("configlet %q not regular file: ignored", configlet.Name()) + continue + } + configletPath, err := validateConfigLetPath(configletDir, configlet.Name()) + if err != nil { + klog.Infof("could not load: %v", err) continue } - configLetPath := filepath.Join(configletDir, configLet.Name()) if pArgs.Global.Debug { - klog.Infof("loading configlet: %q", configLetPath) + klog.Infof("loading configlet: %q", configletPath) } - err = loadConfiglet(confObj, configLetPath) + err = loadConfiglet(confObj, configletPath) if err != nil { return err } diff --git a/pkg/config/cfgfile_test.go b/pkg/config/cfgfile_test.go index acd1bbf4..303d8e1c 100644 --- a/pkg/config/cfgfile_test.go +++ b/pkg/config/cfgfile_test.go @@ -69,6 +69,9 @@ func TestReadResourceExclude(t *testing.T) { } func TestFromFiles(t *testing.T) { + closer := setupTest(t) + t.Cleanup(closer) + type testCase struct { name string } diff --git a/pkg/config/config.go b/pkg/config/config.go index 75ab7d9b..dd848f5c 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -20,6 +20,7 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "k8s.io/klog/v2" "sigs.k8s.io/yaml" @@ -107,13 +108,19 @@ func LoadArgs(args ...string) (ProgArgs, error) { return pArgs, err } + // this needs to be done as early as possible + cfgRoot, err := validateConfigRootPath(configRoot) + if err != nil { + return pArgs, err + } + // now the real processing begins. From now on we waste nothing if pArgs.Global.Debug { - klog.Infof("configRoot=%q extraConfigPath=%q", configRoot, extraConfigPath) + klog.Infof("configRoot=%q extraConfigPath=%q", cfgRoot, extraConfigPath) klog.Infof("config defaults:{{\n%s}}", pArgs.ToYAMLString()) } - err = FromFiles(&pArgs, configRoot, extraConfigPath) + err = FromFiles(&pArgs, cfgRoot, extraConfigPath) if err != nil { return pArgs, err } @@ -150,3 +157,15 @@ func Finalize(pArgs *ProgArgs) error { } return err } + +func UserRunDir() (string, error) { + return filepath.Join("/run", "user", fmt.Sprintf("%d", os.Getuid()), "rte"), nil +} + +func UserHomeDir() (string, error) { + homeDir, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(homeDir, ".rte"), nil +} diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index cbaa957d..6127d1e6 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -26,6 +26,9 @@ import ( ) func TestLoadArgs(t *testing.T) { + closer := setupTest(t) + t.Cleanup(closer) + type testCase struct { name string } @@ -39,7 +42,8 @@ func TestLoadArgs(t *testing.T) { }, } { t.Run(tcase.name, func(t *testing.T) { - confRoot := filepath.Join(testDataDir, "conftree", tcase.name) + userDir, _ := UserRunDir() + confRoot := filepath.Join(userDir, "conftree", tcase.name) environ := filepath.Join(confRoot, "_env", "vars.yaml") setupEnviron(t, environ) diff --git a/pkg/config/flags_test.go b/pkg/config/flags_test.go index 791996f1..49350323 100644 --- a/pkg/config/flags_test.go +++ b/pkg/config/flags_test.go @@ -19,6 +19,7 @@ package config import ( "flag" "fmt" + "log" "os" "path/filepath" "runtime" @@ -137,10 +138,24 @@ func setupTestWithEnv(t *testing.T, envs map[string]string) func() { baseDir = filepath.Dir(file) testDataDir = filepath.Clean(filepath.Join(baseDir, "..", "..", "test", "data")) - return envSetter(envs) + userDir, err := UserRunDir() + if err == nil { + err2 := os.CopyFS(userDir, os.DirFS(testDataDir)) + log.Printf("copyfs %q -> %q err=%v", testDataDir, userDir, err2) + } + + closer := envSetter(envs) + return func() { + closer() + if err != nil { + return + } + err2 := os.Remove(userDir) + log.Printf("unlinked %q -> %q err=%v", testDataDir, userDir, err2) + } } -func envSetter(envs map[string]string) (closer func()) { +func envSetter(envs map[string]string) func() { originalEnvs := map[string]string{} for name, value := range envs { diff --git a/pkg/config/validation.go b/pkg/config/validation.go index bc90d23d..e4bf35b3 100644 --- a/pkg/config/validation.go +++ b/pkg/config/validation.go @@ -17,6 +17,12 @@ limitations under the License. package config import ( + "errors" + "fmt" + "io/fs" + "path/filepath" + "strings" + metricssrv "github.com/k8stopologyawareschedwg/resource-topology-exporter/pkg/metrics/server" "github.com/k8stopologyawareschedwg/resource-topology-exporter/pkg/resourcemonitor" ) @@ -36,3 +42,67 @@ func Validate(pArgs *ProgArgs) error { return nil } + +func validateConfigLetPath(configletDir, configletName string) (string, error) { + configletPath, err := filepath.EvalSymlinks(filepath.Clean(filepath.Join(configletDir, configletName))) + if err != nil { + return "", err + } + if filepath.Dir(configletPath) != configletDir { + return "", fmt.Errorf("configlet %q is not within %q", configletName, configletDir) + } + return configletPath, nil +} + +func validateConfigRootPath(configRoot string) (string, error) { + if configRoot == "" { + return "", fmt.Errorf("configRoot is not allowed to be an empty string") + } + + configRoot = filepath.Clean(configRoot) + cfgRoot, err := filepath.EvalSymlinks(configRoot) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + // reset to original value, it somehow passed the symlink check + cfgRoot = configRoot + } else { + return "", fmt.Errorf("failed to validate configRoot path: %w", err) + } + } + // else either success or checking a non-existing path. Which can be still OK. + + pattern, err := IsConfigRootAllowed(cfgRoot, UserRunDir, UserHomeDir) + if err != nil { + return "", err + } + if pattern == "" { + return "", errors.New("failed to validate configRoot path: not matches any allowed pattern") + } + + // clear pattern to make the tool happy + relPath := strings.TrimPrefix(cfgRoot, pattern) + return filepath.Abs(filepath.Clean(filepath.Join(pattern, relPath))) +} + +func IsConfigRootAllowed(cfgPath string, addDirFns ...func() (string, error)) (string, error) { + allowedPatterns := []string{ + "/etc/rte", + "/run/rte", + "/var/rte", + "/usr/local/etc/rte", + } + for _, addDirFn := range addDirFns { + userDir, err := addDirFn() + if err != nil { + return "", err + } + allowedPatterns = append(allowedPatterns, userDir) + } + + for _, pattern := range allowedPatterns { + if strings.HasPrefix(cfgPath, pattern) { + return pattern, nil + } + } + return "", nil +}