Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OCPBUGS-27741: Reject Invalid Route Paths with Rewrite-Target Annotation #1791

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

OCPBUGS-27741: Reject Invalid Route Paths with Rewrite-Target Annotation #1791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 90 additions & 3 deletions pkg/route/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ const (
permittedResponseHeaderValueErrorMessage = "Either header value provided is not in correct format or the converter specified is not allowed. The dynamic header value may use HAProxy's %[] syntax and otherwise must be a valid HTTP header value as defined in https://datatracker.ietf.org/doc/html/rfc7230#section-3.2 Sample fetchers allowed are res.hdr, ssl_c_der. Converters allowed are lower, base64."
// routerServiceAccount is used to validate RBAC permissions for externalCertificate
routerServiceAccount = "system:serviceaccount:openshift-ingress:router"
// rewriteTargetAnnotation is route annotation used to specify a rewrite target.
rewriteTargetAnnotation = "haproxy.router.openshift.io/rewrite-target"
)

var (
Expand Down Expand Up @@ -212,6 +214,42 @@ func validateRoute(ctx context.Context, route *routev1.Route, checkHostname bool
result = append(result, errs...)
}

if errs := validatePathWithRewriteTargetAnnotation(route, specPath.Child("path")); len(errs) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears you only have an instant view of the object here, and don't have the prior view of it, is that correct?

Typically in webhooks you can compare state with the old state. If you can do that, you can make this validation ratchet by allowing the persistence of existing bad configuration, and only validate when there is actually a change to the path/annotation.

That would be safer here, as, presently, in theory, someone could upgrade and this would then prevent any writes to the object until the validation issue has been resolved.

If the validation here fails, I understand that HAProxy will be in a crash loop right? So in theory, the cluster would not be upgradeable anyway, and so the argument is that this doesn't need to ratchet?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Let me clarify this: what happens with this current non-ratcheting validation when someone updates with a newly invalid route?

I think the answer is: nothing...until the user changes any field on the invalid route. It's not going to cause a K8S cluster melt-down right? Will ALL updates, including status updates made by the openshift-router be rejected too? (that might be okay, but something for me think about...)

On the other hand, ratcheting validation allows the user to still update other fields route while our spec.path still be invalid (but we won't reject yet), correct?

I think if i understand how non-ratcheting validation compares to ratcheting (e.g. non-ratcheting rejects ALL updates to a route), yes, I think we should be okay since the validation I'm adding results in a HAProxy crash loop, which means the user is already broken.

I'd like to confirm if this is a valid use case for doing the sledgehammer of non-ratcheting validation, since, yes, HAProxy will be crashlooping in any of these situations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the answer is: nothing...until the user changes any field on the invalid route. It's not going to cause a K8S cluster melt-down right? Will ALL updates, including status updates made by the openshift-router be rejected too? (that might be okay, but something for me think about...)

Test this, but, I believe it will prevent ALL writes, including status.

On the other hand, ratcheting validation allows the user to still update other fields route while our spec.path still be invalid (but we won't reject yet), correct?

Correct, they would only be presented with errors when they change something specific to this validation, the spec path or the annotation in this particular case.

I think if i understand how non-ratcheting validation compares to ratcheting (e.g. non-ratcheting rejects ALL updates to a route), yes, I think we should be okay since the validation I'm adding results in a HAProxy crash loop, which means the user is already broken.

How would this manifest in the cluster? Would it cause an upgradeable condition to go false on the cluster operator? If it does, then the user must fix the issue before they upgrade into a version containing this validation, right?

Copy link
Author

@gcs278 gcs278 Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test this, but, I believe it will prevent ALL writes, including status.

Hmm looks like if I try to edit the status via oc edit route... it fails, but the router status update appears to be successful despite the non-ratcheting validation.

How would this manifest in the cluster? Would it cause an upgradeable condition to go false on the cluster operator? If it does, then the user must fix the issue before they upgrade into a version containing this validation, right?

No upgradeable condition currently. The problem with doing upgradeable, is that we want to backport. In theory, I think we could do upgradeable=false on z streams, but previously the upgrade team didn't like the idea of a single route preventing an entire cluster from upgrading (route admin vs. cluster admin personas), so we used Admin-Acks so the cluster admin could override, which only works for minor level upgrades. Upgradeable=false is also going to be more painful to implement.

The real argument against upgradeable=false, is that users that are already in this state (have invalid config), are already broken. Upgradeable doesn't feel useful when routers are already broken.

So, users have a broken HAProxy if they enter this invalid state we are adding validation for. Ratcheting validation would allow them to modify other config fields, but what's the point if they are totally broken? Arguably they should fix the invalid state ASAP. However, the only argument for ratcheting validation is: it is somewhat of a safety mechanism in case we miss an edge case where a newly invalid state WAS actually valid (didn't cause failure). However, it's still ratcheting validation, so eventually if they recreate the route, or try to change spec.path or the rewrite-target annotation, the validation will catch up with them. And this scenario would be a bug we should fix anyways.

So, I don't see any strong reasons to do ratcheting validation. @JoelSpeed Do you agree, or have any thoughts otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm looks like if I try to edit the status via oc edit route... it fails, but the router status update appears to be successful despite the non-ratcheting validation.

Did you use the --subresource status flag when trying oc edit? You'll need to if the object has a status subresource

The real argument against upgradeable=false, is that users that are already in this state (have invalid config), are already broken. Upgradeable doesn't feel useful when routers are already broken.

I wasn't suggesting to implement an upgradeable false, sorry, I meant, if the routers are already broken, how does the cluster operator look? Does an invalid config today (causing restarting HAProxy) already cause an upgradeable false or some other note in the cluster that would prevent the user upgrading into this while already broken?

result = append(result, errs...)
}

return result
}

// validatePathWithRewriteTargetAnnotation tests the spec.path field for invalid syntax when the
// rewrite-target annotation is set. This addresses OCPBUGS-27741 by rejecting invalid spec.path
// values, while preserving compatibility for users who may have depended on the unintended
// behavior caused by the bug.
func validatePathWithRewriteTargetAnnotation(route *routev1.Route, fldPath *field.Path) field.ErrorList {
result := field.ErrorList{}
if _, ok := route.Annotations[rewriteTargetAnnotation]; ok {
// Since regex compilation is expensive, first determine if there are any regex meta characters in spec.path.
if containsRegexMetaChars(route.Spec.Path) {
// Replicate the regex used in the haproxy-config.template.
regex := fmt.Sprintf("^%s/?(.*)$", route.Spec.Path)
if _, err := regexp.Compile(regex); err != nil {
result = append(result, field.Invalid(fldPath, route.Spec.Path, fmt.Sprintf("cannot contain invalid regex characters while %s is specified", rewriteTargetAnnotation)))
}
}
// These validations are to protect against HAProxy config file level errors.
if hasUnmatchedSingleQuotes(route.Spec.Path) {
result = append(result, field.Invalid(fldPath, route.Spec.Path, fmt.Sprintf("cannot contain unmatched ' while %s is specified", rewriteTargetAnnotation)))
}
if hasUnmatchedDoubleQuotes(route.Spec.Path) {
result = append(result, field.Invalid(fldPath, route.Spec.Path, fmt.Sprintf("cannot contain unmatched \" while %s is specified", rewriteTargetAnnotation)))
}
if strings.Contains(route.Spec.Path, "#") {
result = append(result, field.Invalid(fldPath, route.Spec.Path, fmt.Sprintf("cannot contain # while %s is specified", rewriteTargetAnnotation)))
}
if strings.Contains(route.Spec.Path, "\n") || strings.Contains(route.Spec.Path, "\r") {
result = append(result, field.Invalid(fldPath, route.Spec.Path, fmt.Sprintf("cannot contain newlines while %s is specified", rewriteTargetAnnotation)))
}
}
return result
}

Expand Down Expand Up @@ -526,10 +564,59 @@ func validateKubeFinalizerName(stringValue string, fldPath *field.Path) field.Er
}

func Warnings(route *routev1.Route) []string {
var warnings []string
if len(route.Spec.Host) != 0 && len(route.Spec.Subdomain) != 0 {
var warnings []string
warnings = append(warnings, "spec.host is set; spec.subdomain may be ignored")
return warnings
}
return nil
warnings = append(warnings, warnPathWithRewriteTargetAnnotation(route)...)

return warnings
}

// warnPathWithRewriteTargetAnnotation returns warnings if a provided route has
// the rewrite-target annotation set, and the spec.path contains invalid
// values that, while valid, may lead to unintended behavior.
func warnPathWithRewriteTargetAnnotation(route *routev1.Route) []string {
var warnings []string
if _, ok := route.Annotations[rewriteTargetAnnotation]; ok {
if containsRegexMetaChars(route.Spec.Path) {
warnings = append(warnings, fmt.Sprintf("spec.path contains regex meta characters which may lead to unexpected behavior using %s", rewriteTargetAnnotation))
}
if strings.Contains(route.Spec.Path, `'`) {
warnings = append(warnings, fmt.Sprintf("spec.path contains a ' which may lead to unexpected behavior using %s", rewriteTargetAnnotation))
}
if strings.Contains(route.Spec.Path, `"`) {
warnings = append(warnings, fmt.Sprintf("spec.path contains a \" which may lead to unexpected behavior using %s", rewriteTargetAnnotation))
}
}
return warnings
}

// hasUnmatchedSingleQuotes returns true if input string has unmatched single quotes.
func hasUnmatchedSingleQuotes(input string) bool {
singleQuoteCount := strings.Count(input, `'`)
if singleQuoteCount%2 != 0 {
return true
}
return false
}

// hasUnmatchedDoubleQuotes returns true if input string has unmatched double quotes.
func hasUnmatchedDoubleQuotes(input string) bool {
doubleQuoteCount := strings.Count(input, `"`)
if doubleQuoteCount%2 != 0 {
return true
}
return false
}

// containsRegexMetaChars returns true if the input string contains a regex meta character.
func containsRegexMetaChars(input string) bool {
regexMetaChars := `.+*?^$()[]{}|\`
for _, char := range regexMetaChars {
if strings.ContainsRune(input, char) {
return true
}
}
return false
}
136 changes: 126 additions & 10 deletions pkg/route/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,21 @@ func TestValidateRoute(t *testing.T) {
var headerNameXSS string = "X-XSS-Protection"
invalidNumRequests := make([]routev1.RouteHTTPHeader, maxRequestHeaderList+1)
invalidNumResponses := make([]routev1.RouteHTTPHeader, maxResponseHeaderList+1)
routeWithPathAndRewriteAnnotation := func(path, rewriteTargetAnnotation string) *routev1.Route {
return &routev1.Route{
ObjectMeta: metav1.ObjectMeta{
Name: "wildcardpolicy",
Namespace: "foo",
Annotations: map[string]string{
"haproxy.router.openshift.io/rewrite-target": rewriteTargetAnnotation,
},
},
Spec: routev1.RouteSpec{
To: createRouteSpecTo("serviceName", "Service"),
Path: path,
},
}
}
tests := []struct {
name string
route *routev1.Route
Expand Down Expand Up @@ -1007,13 +1022,69 @@ func TestValidateRoute(t *testing.T) {
},
expectedErrors: 1,
},
{
name: "spec.path should not contain unmatched single quotes when rewrite-target annotation is set",
route: routeWithPathAndRewriteAnnotation("/foo'", "foo"),
expectedErrors: 1,
},
{
name: "spec.path should not contain unmatched double quotes when rewrite-target annotation is set",
route: routeWithPathAndRewriteAnnotation(`/foo"`, "foo"),
expectedErrors: 1,
},
{
name: "spec.path should not contain hash symbol when rewrite-target annotation is set",
route: routeWithPathAndRewriteAnnotation("/foo#", "foo"),
expectedErrors: 1,
},
{
name: "spec.path should not contain new lines when rewrite-target annotation is set",
route: routeWithPathAndRewriteAnnotation("/foo\n\r", "foo"),
expectedErrors: 1,
},
{
name: "spec.path should not contain an invalid regex with [a-Z] when rewrite-target annotation is set",
route: routeWithPathAndRewriteAnnotation("/foo[a-Z]", "foo"),
expectedErrors: 1,
},
{
name: "spec.path should not contain an invalid regex with invalid escape when rewrite-target annotation is set",
route: routeWithPathAndRewriteAnnotation(`/foo\o`, "foo"),
expectedErrors: 1,
},
{
name: "spec.path should can contain an valid regex with + when rewrite-target annotation is set",
route: routeWithPathAndRewriteAnnotation("/foo+", "foo"),
expectedErrors: 0,
},
{
name: "spec.path can contain an valid regex with [A-z] when rewrite-target annotation is set",
route: routeWithPathAndRewriteAnnotation("/foo[A-z]", "foo"),
expectedErrors: 0,
},
{
name: "spec.path can contain an invalid regex when rewrite-target annotation is NOT set",
route: &routev1.Route{
ObjectMeta: metav1.ObjectMeta{
Name: "wildcardpolicy",
Namespace: "foo",
},
Spec: routev1.RouteSpec{
To: createRouteSpecTo("serviceName", "Service"),
Path: "/foo[a-Z]",
},
},
expectedErrors: 0,
},
}

for _, tc := range tests {
errs := ValidateRoute(context.Background(), tc.route, &testSARCreator{allow: false}, &testSecretGetter{}, routecommon.RouteValidationOptions{AllowExternalCertificates: false})
if len(errs) != tc.expectedErrors {
t.Errorf("Test case %s expected %d error(s), got %d. %v", tc.name, tc.expectedErrors, len(errs), errs)
}
t.Run(tc.name, func(t *testing.T) {
errs := ValidateRoute(context.Background(), tc.route, &testSARCreator{allow: false}, &testSecretGetter{}, routecommon.RouteValidationOptions{AllowExternalCertificates: false})
if len(errs) != tc.expectedErrors {
t.Errorf("Test case %s expected %d error(s), got %d. %v", tc.name, tc.expectedErrors, len(errs), errs)
}
})
}
}

Expand Down Expand Up @@ -2701,10 +2772,12 @@ func TestValidateEdgeReencryptInsecureEdgeTerminationPolicy(t *testing.T) {

func TestWarnings(t *testing.T) {
for _, tc := range []struct {
name string
host string
subdomain string
expected []string
name string
host string
subdomain string
path string
rewriteTargetExists bool
expected []string
}{
{
name: "both host and subdomain set",
Expand All @@ -2723,14 +2796,57 @@ func TestWarnings(t *testing.T) {
{
name: "both host and subdomain unset",
},
{
name: "no-warning path with rewrite-target annotation",
path: "/foo",
rewriteTargetExists: true,
expected: []string{},
},
{
name: "warning path with single quotes with rewrite-target annotation",
path: "/'foo'",
rewriteTargetExists: true,
expected: []string{"spec.path contains a ' haproxy.router.openshift.io/rewrite-target may produce an unexpected result"},
},
{
name: "warning path with double quotes with rewrite-target annotation",
path: `/"foo"`,
rewriteTargetExists: true,
expected: []string{"spec.path contains a \" haproxy.router.openshift.io/rewrite-target may produce an unexpected result"},
},
{
name: "warning path with regex meta character with rewrite-target annotation",
path: "/foo$",
rewriteTargetExists: true,
expected: []string{"spec.path contains regex meta characters, haproxy.router.openshift.io/rewrite-target may provide an unexpected result"},
},
{
name: "no-warning path with hash character with rewrite-target annotation (will be rejected)",
path: "/foo#",
rewriteTargetExists: true,
expected: []string{},
},
{
name: "no-warning path with regex meta character without rewrite-target annotation",
path: "/foo^",
rewriteTargetExists: false,
expected: []string{},
},
} {
t.Run(tc.name, func(t *testing.T) {
actual := Warnings(&routev1.Route{
route := &routev1.Route{
Spec: routev1.RouteSpec{
Path: tc.path,
Host: tc.host,
Subdomain: tc.subdomain,
},
})
}
if tc.rewriteTargetExists {
route.Annotations = map[string]string{
"haproxy.router.openshift.io/rewrite-target": "foo",
}
}
actual := Warnings(route)
if len(actual) != len(tc.expected) {
t.Fatalf("expected %#v, got %#v", tc.expected, actual)
}
Expand Down