diff --git a/cmd/common-main.go b/cmd/common-main.go index 18aad7346..7f6ead797 100644 --- a/cmd/common-main.go +++ b/cmd/common-main.go @@ -36,6 +36,7 @@ import ( "strings" "syscall" "time" + "unicode" "github.com/dustin/go-humanize" fcolor "github.com/fatih/color" @@ -540,6 +541,34 @@ func (e envKV) String() string { return fmt.Sprintf("%s=%s", e.Key, e.Value) } +func isValidEnvName(name string) bool { + if name == "" || !isEnvNameStart(name[0]) { + return false + } + for i := 1; i < len(name); i++ { + if !isEnvNameStart(name[i]) && (name[i] < '0' || name[i] > '9') { + return false + } + } + return true +} + +func isEnvNameStart(ch byte) bool { + return ch == '_' || ch >= 'A' && ch <= 'Z' || ch >= 'a' && ch <= 'z' +} + +func trimExportPrefix(envEntry string) string { + rest, ok := strings.CutPrefix(envEntry, "export") + if !ok || rest == "" { + return envEntry + } + trimmed := strings.TrimLeftFunc(rest, unicode.IsSpace) + if len(trimmed) == len(rest) { + return envEntry + } + return trimmed +} + func parsEnvEntry(envEntry string) (envKV, error) { envEntry = strings.TrimSpace(envEntry) if envEntry == "" { @@ -554,13 +583,19 @@ func parsEnvEntry(envEntry string) (envKV, error) { Skip: true, }, nil } - envTokens := strings.SplitN(strings.TrimSpace(strings.TrimPrefix(envEntry, "export")), config.EnvSeparator, 2) + envTokens := strings.SplitN(trimExportPrefix(envEntry), config.EnvSeparator, 2) if len(envTokens) != 2 { - return envKV{}, fmt.Errorf("envEntry malformed; %s, expected to be of form 'KEY=value'", envEntry) + return envKV{}, errors.New("missing '='") } - key := envTokens[0] - val := envTokens[1] + key := strings.TrimSpace(envTokens[0]) + val := strings.TrimSpace(envTokens[1]) + if !isValidEnvName(key) { + return envKV{}, fmt.Errorf("invalid environment variable name %q", key) + } + if strings.IndexByte(val, 0) >= 0 { + return envKV{}, errors.New("environment variable value contains NUL") + } // Remove quotes from the value if found if len(val) >= 2 { @@ -587,10 +622,12 @@ func minioEnvironFromFile(envConfigFile string) ([]envKV, error) { defer f.Close() var ekvs []envKV scanner := bufio.NewScanner(f) + lineNo := 0 for scanner.Scan() { + lineNo++ ekv, err := parsEnvEntry(scanner.Text()) if err != nil { - return nil, err + return nil, fmt.Errorf("%s:%d: %w", envConfigFile, lineNo, err) } if ekv.Skip { // Skips empty lines @@ -599,7 +636,7 @@ func minioEnvironFromFile(envConfigFile string) ([]envKV, error) { ekvs = append(ekvs, ekv) } if err = scanner.Err(); err != nil { - return nil, err + return nil, fmt.Errorf("%s: %w", envConfigFile, err) } return ekvs, nil } @@ -666,12 +703,15 @@ func loadEnvVarsFromFiles() { } if env.IsSet(config.EnvConfigEnvFile) { - ekvs, err := minioEnvironFromFile(env.Get(config.EnvConfigEnvFile, "")) + envConfigFile := env.Get(config.EnvConfigEnvFile, "") + ekvs, err := minioEnvironFromFile(envConfigFile) if err != nil && !os.IsNotExist(err) { logger.Fatal(err, "Unable to read the config environment file") } for _, ekv := range ekvs { - os.Setenv(ekv.Key, ekv.Value) + if err := os.Setenv(ekv.Key, ekv.Value); err != nil { + logger.Fatal(err, "Unable to set %s from config environment file %s", ekv.Key, envConfigFile) + } } } } diff --git a/cmd/common-main_test.go b/cmd/common-main_test.go index 9757267d2..c13fa7a8e 100644 --- a/cmd/common-main_test.go +++ b/cmd/common-main_test.go @@ -19,8 +19,10 @@ package cmd import ( "errors" + "fmt" "os" "reflect" + "strings" "testing" ) @@ -181,3 +183,162 @@ MINIO_ROOT_PASSWORD=minio123`, }) } } + +func Test_minioEnvironFromFileWhitespaceAndValidation(t *testing.T) { + testCases := []struct { + name string + content string + want []envKV + errLine int + errContains string + errExcludes string + }{ + { + name: "spaces and tabs around separator", + content: "MINIO_ROOT_USER = minio\nMINIO_ROOT_PASSWORD\t=\tminio123", + want: []envKV{ + {Key: "MINIO_ROOT_USER", Value: "minio"}, + {Key: "MINIO_ROOT_PASSWORD", Value: "minio123"}, + }, + }, + { + name: "export tab and quoted spaces", + content: "export\tMINIO_ROOT_USER = \" minio user \"\nexport MINIO_ROOT_PASSWORD = ' minio secret '", + want: []envKV{ + {Key: "MINIO_ROOT_USER", Value: " minio user "}, + {Key: "MINIO_ROOT_PASSWORD", Value: " minio secret "}, + }, + }, + { + name: "export Unicode whitespace", + content: "export\u00a0MINIO_ROOT_USER=value", + want: []envKV{ + {Key: "MINIO_ROOT_USER", Value: "value"}, + }, + }, + { + name: "export is only a standalone prefix", + content: "export=value\nexportFOO=bar", + want: []envKV{ + {Key: "export", Value: "value"}, + {Key: "exportFOO", Value: "bar"}, + }, + }, + { + name: "unquoted whitespace empty value and additional separators", + content: "UNQUOTED = value \nEMPTY =\nTOKEN = scheme://user:password@example.com?a=b", + want: []envKV{ + {Key: "UNQUOTED", Value: "value"}, + {Key: "EMPTY", Value: ""}, + {Key: "TOKEN", Value: "scheme://user:password@example.com?a=b"}, + }, + }, + { + name: "valid underscore and digits", + content: "_MINIO_2=value", + want: []envKV{ + {Key: "_MINIO_2", Value: "value"}, + }, + }, + { + name: "missing separator redacts the line", + content: "MINIO_ROOT_PASSWORD=valid\nsuper-secret-without-equals", + errLine: 2, + errContains: "missing '='", + errExcludes: "super-secret-without-equals", + }, + { + name: "empty name", + content: "=empty-name-secret", + errLine: 1, + errContains: `invalid environment variable name ""`, + errExcludes: "empty-name-secret", + }, + { + name: "digit leading name", + content: "1MINIO_ROOT_USER=digit-leading-secret", + errLine: 1, + errContains: `invalid environment variable name "1MINIO_ROOT_USER"`, + errExcludes: "digit-leading-secret", + }, + { + name: "hyphenated name", + content: "MINIO-ROOT-USER=hyphen-secret", + errLine: 1, + errContains: `invalid environment variable name "MINIO-ROOT-USER"`, + errExcludes: "hyphen-secret", + }, + { + name: "whitespace in name", + content: "MINIO ROOT USER=whitespace-secret", + errLine: 1, + errContains: `invalid environment variable name "MINIO ROOT USER"`, + errExcludes: "whitespace-secret", + }, + { + name: "NUL in name", + content: "MINIO\x00ROOT=nul-name-secret", + errLine: 1, + errContains: "invalid environment variable name", + errExcludes: "nul-name-secret", + }, + { + name: "NUL in value", + content: "MINIO_ROOT_USER=before\x00nul-value-secret", + errLine: 1, + errContains: "environment variable value contains NUL", + errExcludes: "nul-value-secret", + }, + { + name: "diagnostic has file and line but no value", + content: "MINIO_ROOT_USER=valid\nBAD KEY=super-secret-value", + errLine: 2, + errContains: `invalid environment variable name "BAD KEY"`, + errExcludes: "super-secret-value", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + tmpfile, err := os.CreateTemp(t.TempDir(), "testfile") + if err != nil { + t.Fatal(err) + } + if _, err = tmpfile.WriteString(testCase.content); err != nil { + t.Fatal(err) + } + if err = tmpfile.Close(); err != nil { + t.Fatal(err) + } + + got, err := minioEnvironFromFile(tmpfile.Name()) + if testCase.errContains == "" { + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(got, testCase.want) { + t.Errorf("expected %v, got %v", testCase.want, got) + } + return + } + + if err == nil { + t.Fatal("expected an error") + } + errText := err.Error() + location := fmt.Sprintf("%s:%d:", tmpfile.Name(), testCase.errLine) + if !strings.Contains(errText, location) { + t.Errorf("expected error to contain %q, got %q", location, errText) + } + if !strings.Contains(errText, testCase.errContains) { + t.Errorf("expected error to contain %q, got %q", testCase.errContains, errText) + } + if testCase.errExcludes != "" && strings.Contains(errText, testCase.errExcludes) { + t.Errorf("expected error to redact %q, got %q", testCase.errExcludes, errText) + } + if got != nil { + t.Errorf("expected no entries on parse error, got %v", got) + } + }) + } +}