Repository navigation
Warn about creation rules with identical path_regex - #2312
Open
rishabhvenu wants to merge 1 commit into
Open
rishabhvenu wants to merge 1 commit into
rishabhvenu wants to merge 1 commit into
Conversation
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 <rishiryan4@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2238.
Only the first matching creation rule is used. If two creation rules have the same
path_regex, the second one can never be selected, and its settings (in the issue:encrypted_regex) are silently not applied. sops currently says nothing about this.This follows the scope suggested in #2238 (comment) and #2238 (comment): only rules with an identical
path_regexstring are reported. Rules with different regular expressions that match the same files are not reported, and rules withoutpath_regexare ignored. Which rule is selected does not change.With the config from the issue,
sops encrypt secrets.yamlnow prints this on stderr (captured with stderr redirected to a file):Rule numbers count from 1. If a
path_regexis used three times, rules 2 and 3 each get one warning that points at rule 1.Implementation notes:
path_regexis the only field of a creation rule that is used for matching, so comparing that string is enough to know the later rule is unreachable.LoadCreationRuleForFile, after the config file is parsed and before a rule is matched. It does not run inloadConfigFile, because that is also called byLoadStoresConfig(twice per command for the input and output store), which would repeat the warning.sops updatekeys a.yaml b.yamlloads the creation rules once per file, so the config path is remembered in a package-levelsync.Mapand the warnings are shown once per config file per process. This is the same idea asshowedConfigFileWarningincmd/sops/main.go.configpackage had no logger. I added one withlogging.NewLogger("CONFIG"), like the other packages do. The alternative would be to return the warnings to the caller the wayLookupConfigFiledoes, butLoadCreationRuleForFileis called from bothcmd/sops/main.goand theupdatekeyssubcommand and its signature would have to change. I can switch to that if you prefer it.destination_rulesuse the same first-match logic. I left them alone because the issue is about creation rules. The same check could be added there if wanted.Tests:
TestFindDuplicatePathRegexes: table test for the detection helper (no rules, distinct regexes, different regexes matching the same files, rules withoutpath_regex, one duplicate, non-adjacent duplicate, triple, two duplicated regexes).TestLoadCreationRuleForFileWarnsAboutDuplicatePathRegex: loads a config file with apath_regexused three times, checks that exactly two warnings are logged, that the first rule is still selected, and that loading the same config again for another file logs nothing. This test fails if the call inLoadCreationRuleForFileis removed.TestLoadCreationRuleForFileDoesNotWarnWithoutDuplicatePathRegex: no warning for overlapping but different regexes and for several rules withoutpath_regex.github.com/sirupsen/logrus/hooks/test, which is part of the logrus module already ingo.mod.sops encryptandsops -eprint the warning once,sops updatekeys -yon two files prints it once,sops decryptprints nothing, and a config withsecrets.yaml$followed by.*\.yaml$prints nothing. The encrypted output is the same as before (first rule applied).Ran
go build ./...,go vet ./...,gofmt -l config/,go test ./config/... ./cmd/...andgo test -race ./config/.staticcheck ./config/reports the same single existing finding as onmain.Not tested: the
hcvaultandkmsunit tests need Docker and were not run, and the Rust functional tests were not run. Neither package importsconfig. No changelog entry and no documentation change are included; the first-match behaviour itself is unchanged.