From e298bb0a874141a323f0cac8209be49d55685f34 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 3 Nov 2023 12:43:04 -0400 Subject: [PATCH] OCPBUGS-22739: Properly handle rewrite-target annotation Process the `haproxy.router.openshift.io/rewrite-target` annotation value to prevent HaProxy configuration issues while maintaining API compatibility. Properly handle values with spaces, backslashes, and other special characters. Add a new package, rewritetarget, to organize the functions that process the rewrite target annotation. Add a separate unit test package, rewritetarget_test, to enforce black-box testing principles. --- .../haproxy/conf/haproxy-config.template | 4 +- pkg/router/template/template_helper.go | 3 + .../util/rewritetarget/rewritetarget.go | 159 ++++++++++++++++++ .../util/rewritetarget/rewritetarget_test.go | 94 +++++++++++ 4 files changed, 258 insertions(+), 2 deletions(-) create mode 100644 pkg/router/template/util/rewritetarget/rewritetarget.go create mode 100644 pkg/router/template/util/rewritetarget/rewritetarget_test.go diff --git a/images/router/haproxy/conf/haproxy-config.template b/images/router/haproxy/conf/haproxy-config.template index 797db3226..c55107501 100644 --- a/images/router/haproxy/conf/haproxy-config.template +++ b/images/router/haproxy/conf/haproxy-config.template @@ -671,9 +671,9 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }} {{- with $pathRewriteTarget := firstMatch $pathRewriteTargetPattern (index $cfg.Annotations "haproxy.router.openshift.io/rewrite-target") }} # Path rewrite target {{- if eq $pathRewriteTarget "/" }} - http-request replace-path ^{{ $cfg.Path }}/?(.*)$ {{ $pathRewriteTarget }}\1 + http-request replace-path ^{{ $cfg.Path }}/?(.*)$ '{{ processRewriteTarget $pathRewriteTarget }}' {{- else }} - http-request replace-path ^{{ $cfg.Path }}(.*)$ {{ $pathRewriteTarget }}\1 + http-request replace-path ^{{ $cfg.Path }}(.*)$ '{{ processRewriteTarget $pathRewriteTarget }}' {{- end }} {{- end }}{{/* rewrite target */}} diff --git a/pkg/router/template/template_helper.go b/pkg/router/template/template_helper.go index 7ce99c22c..81e57eb1e 100644 --- a/pkg/router/template/template_helper.go +++ b/pkg/router/template/template_helper.go @@ -19,6 +19,7 @@ import ( templateutil "github.com/openshift/router/pkg/router/template/util" haproxyutil "github.com/openshift/router/pkg/router/template/util/haproxy" "github.com/openshift/router/pkg/router/template/util/haproxytime" + "github.com/openshift/router/pkg/router/template/util/rewritetarget" ) const ( @@ -411,4 +412,6 @@ var helperFunctions = template.FuncMap{ "clipHAProxyTimeoutValue": clipHAProxyTimeoutValue, //clips extrodinarily high timeout values to be below the maximum allowed timeout value "parseIPList": parseIPList, //parses the list of IPs/CIDRs (IPv4/IPv6) + + "processRewriteTarget": rewritetarget.SanitizeInput, //sanitizes `haproxy.router.openshift.io/rewrite-target` annotation } diff --git a/pkg/router/template/util/rewritetarget/rewritetarget.go b/pkg/router/template/util/rewritetarget/rewritetarget.go new file mode 100644 index 000000000..822824aca --- /dev/null +++ b/pkg/router/template/util/rewritetarget/rewritetarget.go @@ -0,0 +1,159 @@ +package rewritetarget + +import ( + "regexp" + "strings" +) + +var ( + // oneOrMorePercentSigns matches one or more literal percent + // signs. + oneOrMorePercentSigns = regexp.MustCompile(`%+`) + + // oneOrMoreBackslashes matches one or more literal + // backslashes. + oneOrMoreBackslashes = regexp.MustCompile(`\\+`) +) + +type runeResult struct { + value string // The rune to append to the result. + escaped bool // Whether the next rune is escaped. + skip bool // Whether to skip appending this rune. + stop bool // Whether to stop processing further runes. +} + +type processFunc func(char rune, escaped bool) runeResult + +// processRunes iterates through the input string and calls processFn +// for each rune. It uses the result of processFn to determine whether +// to continue, skip the rune, or stop processing the input. It returns +// a processed string. +func processRunes(input string, processFn processFunc) string { + var sb strings.Builder + + // Increase buffer capacity to improve performance by + // preallocating space for the string being built. The buffer + // is set to 125% of the input length to accommodate + // additional characters without the need for further + // allocation. + sb.Grow(len(input) + (len(input) / 4)) + + // escapeFlag tracks if the next rune is escaped. + escapeFlag := false + + for _, char := range input { + result := processFn(char, escapeFlag) + escapeFlag = result.escaped + if result.stop { + break + } + if !result.skip { + sb.WriteString(result.value) + } + } + + return sb.String() +} + +// replacePercentSigns replaces each % to with %% if the current sequence +// contains an odd number %. Previously, a single % produced a syntax error, +// and users had to use %% to represent a literal %. To ensure compatibility +// while resolving this problem, we must continue to replace single % with +// double %%, but leave double %% as is, so that existing users who manually +// addressed the issue are not broken. Furthermore, even sequences, such as +// %%%% or %%%%%% were also valid and should be left untouched, while odd +// sequences such as %%% or %%%%% were invalid, and each % should be doubled +// to ensure every % is escaped. +func replacePercentSigns(val string) string { + return oneOrMorePercentSigns.ReplaceAllStringFunc(val, func(match string) string { + if len(match)%2 == 1 { + // For an odd count of %, replace each % with %%. + return strings.ReplaceAll(match, "%", "%%") + } + return match + }) +} + +// removeSingleBackslashes removes single backslashes \. +func removeSingleBackslashes(val string) string { + return oneOrMoreBackslashes.ReplaceAllStringFunc(val, func(match string) string { + if len(match) == 1 { + return oneOrMoreBackslashes.ReplaceAllString(match, "") + } + return match + }) +} + +// convertDoubleBackslashes converts double backslashes \\ to single +// backslashes \. +func convertDoubleBackslashes(val string) string { + return strings.ReplaceAll(val, `\\`, `\`) +} + +// newProcessHashCreator creates a function to process '#' runes in a +// string. It returns a processFunc which will set +// encounteredCommentMarker to true and stop processing if it +// encounters a '#' that is not escaped. +func newProcessHashCreator(encounteredCommentMarker *bool) processFunc { + return func(char rune, escaped bool) runeResult { + if char == '#' && !escaped { + *encounteredCommentMarker = true + return runeResult{stop: true} + } + return runeResult{value: string(char), escaped: char == '\\' && !escaped} + } +} + +// processDoubleQuotes is a processFunc that processes double quote +// characters. It skips the double quote if it's not escaped. +func processDoubleQuotes(char rune, escaped bool) runeResult { + if char == '"' && !escaped { + return runeResult{skip: true} + } + return runeResult{value: string(char), escaped: char == '\\' && !escaped} +} + +// processSingleQuotes is a processFunc that processes single quote +// characters. It skips the single quote if it's not escaped. If it's +// escaped, it adds an escaped single quote for later conversion. +func processSingleQuotes(char rune, escaped bool) runeResult { + if char == '\'' && !escaped { + return runeResult{skip: true} + } else if char == '\'' && escaped { + // TRICKY: Because our later operation removes single \, + // we should double escape \\ it, so it will be converted + // to \ later. + return runeResult{value: `'\\''`} + } + return runeResult{value: string(char), escaped: char == '\\' && !escaped} +} + +// SanitizeInput processes the `haproxy.router.openshift.io/rewrite-target` +// annotation value for API compatibility while properly handling values +// with spaces, backslashes, and other special characters. Because the +// annotation value was initially introduced without being enclosed in +// single quotes, certain characters were interpreted rather than being +// treated literally. We later added the single quotes wrapping to resolve +// OCPBUGS-22739. However, we must still maintain API compatibility after +// this change: the annotation values MUST be interpreted to the same values +// after updating to enclose the value in single quotes. +func SanitizeInput(val string) string { + var encounteredCommentMarker bool + + val = processRunes(val, newProcessHashCreator(&encounteredCommentMarker)) + val = processRunes(val, processDoubleQuotes) + val = processRunes(val, processSingleQuotes) + val = replacePercentSigns(val) + val = removeSingleBackslashes(val) + val = convertDoubleBackslashes(val) + + if !encounteredCommentMarker { + // The literal `\1` is appended to annotations without + // comments to comply with HAProxy rewrite rule + // expectations, which necessitate a capture group + // reference (i.e., `\1`) for dynamic substitutions. + val += `\1` + } + + return val +} diff --git a/pkg/router/template/util/rewritetarget/rewritetarget_test.go b/pkg/router/template/util/rewritetarget/rewritetarget_test.go new file mode 100644 index 000000000..605588cb4 --- /dev/null +++ b/pkg/router/template/util/rewritetarget/rewritetarget_test.go @@ -0,0 +1,94 @@ +package rewritetarget_test + +import ( + "github.com/openshift/router/pkg/router/template/util/rewritetarget" + "testing" +) + +func Test_SanitizeInput(t *testing.T) { + testCases := []struct { + name string + input string + output string + }{ + { + name: "single percent should be doubled", + input: `%foo`, + output: `%%foo\1`, + }, + { + name: "double percent should be not changed", + input: `%%foo`, + output: `%%foo\1`, + }, + { + name: "triple percent should be doubled entirely", + input: `%%%foo`, + output: `%%%%%%foo\1`, + }, + { + name: "quad percent should be not changed", + input: `%%%%foo`, + output: `%%%%foo\1`, + }, + { + name: "single quotes should be removed, unless escaped", + input: `'foo'foo\'`, + output: `foofoo'\''\1`, + }, + { + name: "single backslash should be dropped", + input: `\foo\`, + output: `foo\1`, + }, + { + name: "double backslash should be single", + input: `\\foo\\`, + output: `\foo\\1`, + }, + { + name: "triple backslash should be double", + input: `\\\foo\\\`, + output: `\\foo\\\1`, + }, + { + name: "quad backslash should be double", + input: `\\\\foo\\\\`, + output: `\\foo\\\1`, + }, + { + name: "comment in beginning should be remove everything", + input: `# foo # foo`, + output: ``, + }, + { + name: "don't remove escaped comments", + input: `\# foo # foo`, + output: `# foo `, + }, + { + name: "comment in middle should remove everything following", + input: `foo # foo`, + output: `foo `, + }, + { + name: "double quotes should get removed, unless escaped", + input: `"foo"foo\"`, + output: `foofoo"\1`, + }, + { + name: "combination", + input: `\\foo\"foo"\#foo\foo'foo\'#foo`, + output: `\foo"foo#foofoofoo'\''`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := rewritetarget.SanitizeInput(tc.input) + if got != tc.output { + t.Errorf("Failure: expected %s, got %s", tc.output, got) + } + }) + } +}