From 50acdb3ffeb0ec8625468c0b5b0b7878c876648a Mon Sep 17 00:00:00 2001 From: Rishabh Venu Date: Wed, 7 Oct 2026 20:41:05 -0400 Subject: [PATCH] Warn about creation rules with identical path_regex Only the first matching creation rule is used. A creation rule that has the same path_regex as an earlier rule can therefore never be selected, and its settings (for example encrypted_regex) are silently not applied. Log a warning for every such rule when the creation rules are loaded. The warnings are shown once per config file, since the config file is loaded again for every file that is processed. Rules with different regular expressions that match the same files are not reported, and the selected rule does not change. Signed-off-by: Rishabh Venu --- config/config.go | 60 ++++++++++++++++++ config/config_test.go | 143 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+) diff --git a/config/config.go b/config/config.go index 511df1bc15..1c4c13a4bd 100644 --- a/config/config.go +++ b/config/config.go @@ -10,6 +10,7 @@ import ( "path/filepath" "regexp" "strings" + "sync" "github.com/getsops/sops/v3" "github.com/getsops/sops/v3/age" @@ -18,11 +19,25 @@ import ( "github.com/getsops/sops/v3/hckms" "github.com/getsops/sops/v3/hcvault" "github.com/getsops/sops/v3/kms" + "github.com/getsops/sops/v3/logging" "github.com/getsops/sops/v3/pgp" "github.com/getsops/sops/v3/publish" + "github.com/sirupsen/logrus" "go.yaml.in/yaml/v3" ) +var ( + log *logrus.Logger + + // Config files for which the duplicate path_regex warnings were already shown to the user. + // Used and set by warnDuplicatePathRegexes(). + warnedDuplicatePathRegexes sync.Map +) + +func init() { + log = logging.NewLogger("CONFIG") +} + type fileSystem interface { Stat(name string) (os.FileInfo, error) } @@ -610,6 +625,50 @@ func parseCreationRuleForFile(conf *configFile, confPath, filePath string, kmsEn return config, nil } +// duplicatePathRegex describes a creation rule that has the same path_regex as an earlier creation rule. +// First and Duplicate are the indices of the two rules in the list of creation rules. +type duplicatePathRegex struct { + PathRegex string + First int + Duplicate int +} + +// findDuplicatePathRegexes returns the creation rules whose path_regex is identical to the path_regex of an +// earlier creation rule. Since the first matching creation rule is used, such a rule is never used. +// Rules without path_regex are ignored, and so are rules whose path_regex differs but matches the same files. +func findDuplicatePathRegexes(rules []creationRule) []duplicatePathRegex { + var duplicates []duplicatePathRegex + first := make(map[string]int) + for i, r := range rules { + if r.PathRegex == "" { + continue + } + if j, ok := first[r.PathRegex]; ok { + duplicates = append(duplicates, duplicatePathRegex{PathRegex: r.PathRegex, First: j, Duplicate: i}) + continue + } + first[r.PathRegex] = i + } + return duplicates +} + +// warnDuplicatePathRegexes logs a warning for every creation rule that has the same path_regex as an earlier +// creation rule. The warnings are only shown once for every config file. +func warnDuplicatePathRegexes(conf *configFile, confPath string) { + duplicates := findDuplicatePathRegexes(conf.CreationRules) + if len(duplicates) == 0 { + return + } + if _, shown := warnedDuplicatePathRegexes.LoadOrStore(confPath, true); shown { + return + } + for _, d := range duplicates { + log.Warnf( + "creation rules %d and %d in %q have the same path_regex %q; only the first matching creation rule is used, so rule %d has no effect", + d.First+1, d.Duplicate+1, confPath, d.PathRegex, d.Duplicate+1) + } +} + // LoadCreationRuleForFile load the configuration for a given SOPS file from the config file at confPath. A kmsEncryptionContext // should be provided for configurations that do not contain key groups, as there's no way to specify context inside // a SOPS config file outside of key groups. @@ -618,6 +677,7 @@ func LoadCreationRuleForFile(confPath string, filePath string, kmsEncryptionCont if err != nil { return nil, err } + warnDuplicatePathRegexes(conf, confPath) return parseCreationRuleForFile(conf, confPath, filePath, kmsEncryptionContext) } diff --git a/config/config_test.go b/config/config_test.go index 04bed7f564..3f17e25566 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -7,6 +7,8 @@ import ( "testing" "github.com/getsops/sops/v3/keys" + "github.com/sirupsen/logrus" + logrustest "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" ) @@ -919,3 +921,144 @@ creation_rules: // Format: ARN|context where context is "AppName:myapp" assert.Equal(t, "arn:aws:kms:us-west-2:123456789012:key/12345678-1234-1234-1234-123456789012|AppName:myapp", conf.KeyGroups[0][0].ToString()) } + +func TestFindDuplicatePathRegexes(t *testing.T) { + for _, tc := range []struct { + name string + pathRegex []string + expected []duplicatePathRegex + }{ + { + name: "no rules", + }, + { + name: "distinct path_regex", + pathRegex: []string{"foo/.*", "bar/.*"}, + }, + { + name: "different path_regex matching the same files", + pathRegex: []string{`secrets\.yaml$`, `.*\.yaml$`, `.*`}, + }, + { + name: "rules without path_regex", + pathRegex: []string{"", "foo/.*", ""}, + }, + { + name: "identical path_regex", + pathRegex: []string{"secrets.yaml$", "secrets.yaml$"}, + expected: []duplicatePathRegex{ + {PathRegex: "secrets.yaml$", First: 0, Duplicate: 1}, + }, + }, + { + name: "identical path_regex with other rules in between", + pathRegex: []string{"foo/.*", "", "bar/.*", "foo/.*"}, + expected: []duplicatePathRegex{ + {PathRegex: "foo/.*", First: 0, Duplicate: 3}, + }, + }, + { + name: "path_regex used three times", + pathRegex: []string{"foo/.*", "foo/.*", "foo/.*"}, + expected: []duplicatePathRegex{ + {PathRegex: "foo/.*", First: 0, Duplicate: 1}, + {PathRegex: "foo/.*", First: 0, Duplicate: 2}, + }, + }, + { + name: "two duplicated path_regex", + pathRegex: []string{"foo/.*", "bar/.*", "bar/.*", "foo/.*"}, + expected: []duplicatePathRegex{ + {PathRegex: "bar/.*", First: 1, Duplicate: 2}, + {PathRegex: "foo/.*", First: 0, Duplicate: 3}, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + var rules []creationRule + for _, pathRegex := range tc.pathRegex { + rules = append(rules, creationRule{PathRegex: pathRegex}) + } + assert.Equal(t, tc.expected, findDuplicatePathRegexes(rules)) + }) + } +} + +// captureConfigLog records the log entries written by this package until the end of the test. +func captureConfigLog(t *testing.T) *logrustest.Hook { + hook := logrustest.NewLocal(log) + t.Cleanup(func() { + log.ReplaceHooks(make(logrus.LevelHooks)) + }) + return hook +} + +func writeConfigFile(confBytes []byte, t *testing.T) string { + confPath := path.Join(t.TempDir(), configFileName) + assert.Nil(t, os.WriteFile(confPath, confBytes, 0o600)) + return confPath +} + +// This is a regression test for https://github.com/getsops/sops/issues/2238 +func TestLoadCreationRuleForFileWarnsAboutDuplicatePathRegex(t *testing.T) { + var sampleConfigWithDuplicatePathRegex = []byte(` +creation_rules: + - path_regex: secrets.yaml$ + pgp: first + encrypted_regex: ^(a|b|c)$ + - path_regex: other.yaml$ + pgp: other + - path_regex: secrets.yaml$ + pgp: second + encrypted_regex: ^(d|e|f)$ + - path_regex: secrets.yaml$ + pgp: third + - pgp: default +`) + confPath := writeConfigFile(sampleConfigWithDuplicatePathRegex, t) + hook := captureConfigLog(t) + + conf, err := LoadCreationRuleForFile(confPath, path.Join(path.Dir(confPath), "secrets.yaml"), nil) + assert.Nil(t, err) + // The first matching rule is still the one that is used + assert.Equal(t, "first", conf.KeyGroups[0][0].ToString()) + assert.Equal(t, "^(a|b|c)$", conf.EncryptedRegex) + + // One warning for every rule that repeats the path_regex of an earlier rule + entries := hook.AllEntries() + if len(entries) != 2 { + t.Fatalf("Expected 2 warnings but got %d", len(entries)) + } + for i, duplicate := range []int{3, 4} { + assert.Equal(t, logrus.WarnLevel, entries[i].Level) + assert.Equal(t, fmt.Sprintf( + "creation rules 1 and %d in %q have the same path_regex \"secrets.yaml$\"; only the first matching creation rule is used, so rule %d has no effect", + duplicate, confPath, duplicate), entries[i].Message) + } + + // The config file is loaded for every file, the warnings should only be shown once + hook.Reset() + conf, err = LoadCreationRuleForFile(confPath, path.Join(path.Dir(confPath), "other.yaml"), nil) + assert.Nil(t, err) + assert.Equal(t, "other", conf.KeyGroups[0][0].ToString()) + assert.Equal(t, 0, len(hook.AllEntries())) +} + +func TestLoadCreationRuleForFileDoesNotWarnWithoutDuplicatePathRegex(t *testing.T) { + var sampleConfigWithoutDuplicatePathRegex = []byte(` +creation_rules: + - path_regex: secrets\.yaml$ + pgp: first + - path_regex: .*\.yaml$ + pgp: second + - pgp: third + - pgp: fourth +`) + confPath := writeConfigFile(sampleConfigWithoutDuplicatePathRegex, t) + hook := captureConfigLog(t) + + conf, err := LoadCreationRuleForFile(confPath, path.Join(path.Dir(confPath), "secrets.yaml"), nil) + assert.Nil(t, err) + assert.Equal(t, "first", conf.KeyGroups[0][0].ToString()) + assert.Equal(t, 0, len(hook.AllEntries())) +}