gosec: reorganize filepath validation

We recently added filepath sanitization before
to feed it in the `os.ReadFile`.
Turns out the linter was still unhappy because
it seems it can't track the variable sanitization
across function calls bonundaries.

With this change we rework the validation
to have them in the same function block of
the `ReadFile` call. This is arguably (much)
more wasteful (in relative terms) because we
redo multiple times redundant validations,
for example when we handle the config file directory.

Still, this makes the linter demonstrably
happier and it's (hopefully) safer because
we can't call ReadFile by mistake with unsanitized
input.

In addition, configfile reading is done once at runtime,
so the extra cost should be amortized.

X-Gosec-Scan: 2.21.4
Signed-off-by: Francesco Romani <[email protected]>

pkg/config/cfgfile.go

@@ -44,19 +44,17 @@ func FixExtraConfigPath(configRoot string) string {
 }
 
 func FromFiles(pArgs *ProgArgs, configRoot, extraConfigPath string) error {
-	cfgRoot, err := validateConfigRootPath(configRoot)
-	if err != nil {
-		return err
+	if configRoot == "" {
+		return errors.New("configRoot is not allowed to be an empty string")
 	}
-	extraCfgPath, err := validateConfigRootPath(extraConfigPath)
-	if err != nil {
-		return err
+	if extraConfigPath == "" {
+		return errors.New("extraConfigPath is not allowed to be an empty string")
 	}
-	err = fromDaemonFiles(pArgs, cfgRoot)
+	err := fromDaemonFiles(pArgs, configRoot)
 	if err != nil && !errors.Is(err, os.ErrNotExist) {
 		return err
 	}
-	return fromExtraFile(pArgs, extraCfgPath)
+	return fromExtraFile(pArgs, extraConfigPath)
 }
 
 func fromExtraFile(pArgs *ProgArgs, extraConfigPath string) error {
@@ -108,11 +106,7 @@ func fromDaemonFiles(pArgs *ProgArgs, configPathRoot string) error {
 				klog.Infof("configlet %q not regular file: ignored", configlet.Name())
 				continue
 			}
-			configletPath, err := validateConfigPath(filepath.Join(configletDir, configlet.Name()))
-			if err != nil {
-				klog.Infof("could not load %q: %v", configlet.Name(), err)
-				continue
-			}
+			configletPath := filepath.Join(configletDir, configlet.Name())
 			if pArgs.Global.Debug {
 				klog.Infof("loading configlet: %q", configletPath)
 			}
@@ -130,11 +124,11 @@ func fromDaemonFiles(pArgs *ProgArgs, configPathRoot string) error {
 }
 
 func loadConfiglet(confObj map[string]interface{}, configPath string) error {
-	obj := make(map[string]interface{})
-	data, err := os.ReadFile(configPath)
+	data, err := ReadConfiglet(configPath)
 	if err != nil {
 		return err
 	}
+	obj := make(map[string]interface{})
 	err = yaml.Unmarshal(data, &obj)
 	if err != nil {
 		return err
@@ -191,7 +185,7 @@ type config struct {
 
 func readExtraConfig(configPath string) (config, error) {
 	conf := config{}
-	data, err := os.ReadFile(configPath)
+	data, err := ReadConfiglet(configPath)
 	if err != nil {
 		// config is optional
 		if errors.Is(err, os.ErrNotExist) {

pkg/config/flags.go

@@ -108,20 +108,12 @@ Special targets:
 	}
 
 	params := flags.Args()
-	extraConfigPath, err := validateConfigRootPath(configPath)
-	if err != nil {
-		return DefaultConfigRoot, LegacyExtraConfigPath, err
-	}
-
 	if len(params) > 1 {
-		return DefaultConfigRoot, extraConfigPath, fmt.Errorf("too many config roots given (%d), currently supported up to 1", len(params))
+		return DefaultConfigRoot, configPath, fmt.Errorf("too many config roots given (%d), currently supported up to 1", len(params))
 	}
 	if len(params) == 0 {
-		return DefaultConfigRoot, extraConfigPath, nil
-	}
-	configRoot, err := validateConfigRootPath(params[0])
-	if err != nil {
-		return DefaultConfigRoot, LegacyExtraConfigPath, err
+		return DefaultConfigRoot, configPath, nil
 	}
+	configRoot := params[0]
 	return configRoot, FixExtraConfigPath(configRoot), nil
 }

pkg/config/validation.go

@@ -20,6 +20,7 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
+	"os"
 	"path/filepath"
 	"strings"
 
@@ -43,35 +44,30 @@ func Validate(pArgs *ProgArgs) error {
 	return nil
 }
 
-func validateConfigPath(configletPath string) (string, error) {
-	configRoot := filepath.Clean(configletPath)
+func ReadConfiglet(configPath string) ([]byte, error) {
+	// to make gosec happy, the validation logic must be in the same function on which we call `os.ReadFile`.
+	// IOW, it seems the linter cannot track variable sanitization across functions.
+	configRoot := filepath.Clean(configPath)
 	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)
+			return nil, fmt.Errorf("failed to validate config 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
+		return nil, err
 	}
 	if pattern == "" {
-		return "", fmt.Errorf("failed to validate configRoot path %q: does not match any allowed pattern", cfgRoot)
+		return nil, fmt.Errorf("failed to validate configRoot path %q: does not match any allowed pattern", cfgRoot)
 	}
-	return cfgRoot, nil
 
-}
-
-func validateConfigRootPath(configRoot string) (string, error) {
-	if configRoot == "" {
-		return "", fmt.Errorf("configRoot is not allowed to be an empty string")
-	}
-	return validateConfigPath(configRoot)
+	return os.ReadFile(cfgRoot)
 }
 
 // IsConfigRootAllowed checks if an *already cleaned and canonicalized* path is among the allowed list.