diff --git a/changes/27477-do-not-interpolate-gitops-text-sections b/changes/27477-do-not-interpolate-gitops-text-sections new file mode 100644 index 0000000000..752e682154 --- /dev/null +++ b/changes/27477-do-not-interpolate-gitops-text-sections @@ -0,0 +1 @@ +- Fixed an issue with the gitops command caused when trying to interpolate variables inside the 'description'/'remediation' sections. \ No newline at end of file diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index ce7eb4b817..55e2f38b35 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -178,8 +178,11 @@ func expandEnv(s string, failOnSecret bool) (string, error) { } s = escapeString(s, preventEscapingPrefix) + exclusionZones := getExclusionZones(s) + var err *multierror.Error - s = fleet.MaybeExpand(s, func(env string) (string, bool) { + s = fleet.MaybeExpand(s, func(env string, startPos, endPos int) (string, bool) { + switch { case strings.HasPrefix(env, preventEscapingPrefix): return "$" + strings.TrimPrefix(env, preventEscapingPrefix), true @@ -193,6 +196,15 @@ func expandEnv(s string, failOnSecret bool) (string, error) { } return "", false } + + // Don't expand fleet vars if they are inside an 'exclusion' zone, + // i.e. 'description' or 'resolution'.... + for _, z := range exclusionZones { + if startPos >= z[0] && endPos <= z[1] { + return "", false + } + } + v, ok := os.LookupEnv(env) if !ok { err = multierror.Append(err, fmt.Errorf("environment variable %q not set", env)) @@ -230,7 +242,7 @@ func LookupEnvSecrets(s string, secretsMap map[string]string) error { return errors.New("secretsMap cannot be nil") } var err *multierror.Error - _ = fleet.MaybeExpand(s, func(env string) (string, bool) { + _ = fleet.MaybeExpand(s, func(env string, startPos, endPos int) (string, bool) { if strings.HasPrefix(env, fleet.ServerSecretPrefix) { // lookup the secret and save it, but don't replace v, ok := os.LookupEnv(env) @@ -258,3 +270,31 @@ func escapeString(s string, preventEscapingPrefix string) string { return strings.Repeat("\\", (len(match)/2)-1) + "$" + preventEscapingPrefix }) } + +// getExclusionZones returns which positions inside 's' should be +// excluded from variable interpolation. +func getExclusionZones(s string) [][2]int { + // We need a different pattern per section because + // the delimiting end pattern ((?:^\s+\w+:|\z)) includes the next + // section token, meaning the matching logic won't work in case + // we have a 'resolution:' followed by a 'description:' or + // vice versa, and we try using something like (?:resolution:|description:) + toExclude := []string{ + "resolution", + "description", + } + patterns := make([]*regexp.Regexp, 0, len(toExclude)) + for _, e := range toExclude { + pattern := fmt.Sprintf(`(?m)^\s*(?:%s:)(.|[\r\n])*?(?:^\s+\w+:|\z)`, e) + patterns = append(patterns, regexp.MustCompile(pattern)) + } + + var zones [][2]int + for _, pattern := range patterns { + result := pattern.FindAllStringIndex(s, -1) + for _, r := range result { + zones = append(zones, [2]int{r[0], r[1]}) + } + } + return zones +} diff --git a/pkg/spec/spec_test.go b/pkg/spec/spec_test.go index 3e8ab56471..eecf92e71d 100644 --- a/pkg/spec/spec_test.go +++ b/pkg/spec/spec_test.go @@ -223,3 +223,54 @@ func TestLookupEnvSecrets(t *testing.T) { require.Equal(t, tc.expResult, secretsMap) } } + +func TestGetExclusionZones(t *testing.T) { + testCases := []struct { + fixturePath []string + expected map[[2]int]string + }{ + { + []string{"testdata", "policies", "policies.yml"}, + map[[2]int]string{ + [2]int{46, 106}: " description: This policy should always fail.\n resolution:", + [2]int{93, 155}: " resolution: There is no resolution for this policy.\n query:", + [2]int{268, 328}: " description: This policy should always pass.\n resolution:", + [2]int{315, 678}: " resolution: |\n Automated method:\n Ask your system administrator to deploy the following script which will ensure proper Security Auditing Retention:\n cp /etc/security/audit_control ./tmp.txt; origExpire=$(cat ./tmp.txt | grep expire-after); sed \"s/${origExpire}/expire-after:60d OR 5G/\" ./tmp.txt > /etc/security/audit_control; rm ./tmp.txt;\n query:", + }, + }, + { + []string{"testdata", "global_config_no_paths.yml"}, + map[[2]int]string{ + [2]int{866, 949}: " description: Collect osquery performance stats directly from osquery\n query:", // + [2]int{1754, 1818}: " description: This policy should always fail.\n resolution:", // + [2]int{1803, 1869}: " resolution: There is no resolution for this policy.\n query:", // + [2]int{1986, 2050}: " description: This policy should always pass.\n resolution:", // + [2]int{2035, 2101}: " resolution: There is no resolution for this policy.\n query:", // + [2]int{2394, 2458}: " description: This policy should always fail.\n resolution:", // + [2]int{2443, 2509}: " resolution: There is no resolution for this policy.\n query:", // + [2]int{2613, 2677}: " description: This policy should always fail.\n resolution:", // + [2]int{2662, 3035}: " resolution: |\n Automated method:\n Ask your system administrator to deploy the following script which will ensure proper Security Auditing Retention:\n cp /etc/security/audit_control ./tmp.txt; origExpire=$(cat ./tmp.txt | grep expire-after); sed \"s/${origExpire}/expire-after:60d OR 5G/\" ./tmp.txt > /etc/security/audit_control; rm ./tmp.txt;\n query:", + [2]int{6102, 6149}: " description: A cool global label\n query:", // + [2]int{6246, 6292}: " description: A fly global label\n hosts:", // + }, + }, + } + + for _, tC := range testCases { + fPath := filepath.Join(tC.fixturePath...) + + t.Run(fPath, func(t *testing.T) { + fContents, err := os.ReadFile(fPath) + require.NoError(t, err) + + contents := string(fContents) + actual := getExclusionZones(contents) + require.Equal(t, len(tC.expected), len(actual)) + + for pos, text := range tC.expected { + require.Contains(t, actual, pos) + require.Equal(t, contents[pos[0]:pos[1]], text, pos) + } + }) + } +} diff --git a/pkg/spec/testdata/global_config_no_paths.yml b/pkg/spec/testdata/global_config_no_paths.yml index bdb158e7ba..4aad54bb9c 100644 --- a/pkg/spec/testdata/global_config_no_paths.yml +++ b/pkg/spec/testdata/global_config_no_paths.yml @@ -84,8 +84,11 @@ policies: - name: 😊😊 Failing policy platform: linux description: This policy should always fail. - resolution: There is no resolution for this policy. - query: SELECT 1 FROM osquery_info WHERE start_time < 0; + resolution: | + Automated method: + Ask your system administrator to deploy the following script which will ensure proper Security Auditing Retention: + cp /etc/security/audit_control ./tmp.txt; origExpire=$(cat ./tmp.txt | grep expire-after); sed "s/${origExpire}/expire-after:60d OR 5G/" ./tmp.txt > /etc/security/audit_control; rm ./tmp.txt; + query: SELECT 1; agent_options: command_line_flags: distributed_denylist_duration: 0 diff --git a/pkg/spec/testdata/policies/policies.yml b/pkg/spec/testdata/policies/policies.yml index 46f6d55469..3a773e7372 100644 --- a/pkg/spec/testdata/policies/policies.yml +++ b/pkg/spec/testdata/policies/policies.yml @@ -6,11 +6,14 @@ - name: Passing policy platform: linux,windows,darwin,chrome description: This policy should always pass. - resolution: There is no resolution for this policy. + resolution: | + Automated method: + Ask your system administrator to deploy the following script which will ensure proper Security Auditing Retention: + cp /etc/security/audit_control ./tmp.txt; origExpire=$(cat ./tmp.txt | grep expire-after); sed "s/${origExpire}/expire-after:60d OR 5G/" ./tmp.txt > /etc/security/audit_control; rm ./tmp.txt; query: SELECT 1; - name: No root logins (macOS, Linux) platform: linux,darwin query: SELECT 1 WHERE NOT EXISTS (SELECT * FROM last WHERE username = "root" AND time > (( SELECT unix_time FROM time ) - 3600 )) - critical: true + critical: true \ No newline at end of file diff --git a/server/datastore/mysql/secret_variables.go b/server/datastore/mysql/secret_variables.go index 14aa17cab7..1b327ce680 100644 --- a/server/datastore/mysql/secret_variables.go +++ b/server/datastore/mysql/secret_variables.go @@ -149,7 +149,7 @@ func (ds *Datastore) expandEmbeddedSecrets(ctx context.Context, document string) return "", nil, fleet.MissingSecretsError{MissingSecrets: missingSecrets} } - expanded := fleet.MaybeExpand(document, func(s string) (string, bool) { + expanded := fleet.MaybeExpand(document, func(s string, startPos, endPos int) (string, bool) { if !strings.HasPrefix(s, fleet.ServerSecretPrefix) { return "", false } diff --git a/server/fleet/fleet_vars.go b/server/fleet/fleet_vars.go index 92024d8529..214d18b8cf 100644 --- a/server/fleet/fleet_vars.go +++ b/server/fleet/fleet_vars.go @@ -26,10 +26,10 @@ func ContainsPrefixVars(text, prefix string) []string { } // MaybeExpand conditionally replaces ${var} or $var in the string based on the mapping function. -// Only repalces the variable with the mapper string if it returns true. +// Only replaces the variable with the mapper string if it returns true. // The mapper returning false will leave the original variable unchanged. // Based on os.Expand -func MaybeExpand(s string, mapping func(string) (string, bool)) string { +func MaybeExpand(s string, mapping func(string, int, int) (string, bool)) string { var buf []byte // ${} is all ASCII, so bytes are fine for this operation. i := 0 @@ -47,7 +47,7 @@ func MaybeExpand(s string, mapping func(string) (string, bool)) string { w = 0 buf = append(buf, s[j]) } else { - replacement, shouldReplace := mapping(name) + replacement, shouldReplace := mapping(name, j, j+w+1) if shouldReplace { buf = append(buf, replacement...) } else { diff --git a/server/fleet/fleet_vars_test.go b/server/fleet/fleet_vars_test.go index 992ba7eb4e..eec75b7353 100644 --- a/server/fleet/fleet_vars_test.go +++ b/server/fleet/fleet_vars_test.go @@ -33,15 +33,27 @@ This is $OTHER_VAR, $ $$ $* ${} in a sentence with${ALSO_OTHER_VAR}in the middle We want to remember BREAD and alsoSHORTCAKEare important. ` - mapping := map[string]string{ + envVars := map[string]string{ "BANANA": "BREAD", "STRAWBERRY": "SHORTCAKE", } - mapper := func(s string) (string, bool) { + expectedPositions := [][]int{ + {9, 19}, + {23, 25}, + {26, 28}, + {51, 68}, + {103, 123}, + {132, 158}, + } + + mapper := func(s string, startPos, endPos int) (string, bool) { + require.Contains(t, expectedPositions, []int{startPos, endPos}, script[startPos:endPos]) + if strings.HasPrefix(s, ServerSecretPrefix) { - return mapping[strings.TrimPrefix(s, ServerSecretPrefix)], true + return envVars[strings.TrimPrefix(s, ServerSecretPrefix)], true } + return "", false }