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())) +}