diff --git a/internal/dag/builder_test.go b/internal/dag/builder_test.go index cb0485c5284..781069e8c98 100644 --- a/internal/dag/builder_test.go +++ b/internal/dag/builder_test.go @@ -780,6 +780,28 @@ func TestDAGInsert(t *testing.T) { }, } + i15InvalidRegex := &v1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "regex", + Namespace: "default", + }, + Spec: v1beta1.IngressSpec{ + Rules: []v1beta1.IngressRule{{ + IngressRuleValue: v1beta1.IngressRuleValue{ + HTTP: &v1beta1.HTTPIngressRuleValue{ + Paths: []v1beta1.HTTPIngressPath{{ + Path: "^\\/(?!\\/)(.*?)", + Backend: v1beta1.IngressBackend{ + ServiceName: "kuard", + ServicePort: intstr.FromString("http"), + }, + }}, + }, + }, + }}, + }, + } + i16 := &v1beta1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "wildcards", @@ -4023,6 +4045,13 @@ func TestDAGInsert(t *testing.T) { }, ), }, + "insert ingress with invalid regex route": { + objs: []interface{}{ + i15InvalidRegex, + s1, + }, + want: listeners(), + }, // issue 1234 "insert ingress with wildcard hostnames": { objs: []interface{}{ diff --git a/internal/dag/conditions.go b/internal/dag/conditions.go index e0876251c16..afcec89abfd 100644 --- a/internal/dag/conditions.go +++ b/internal/dag/conditions.go @@ -179,3 +179,10 @@ func headerMatchConditionsValid(conditions []contour_api_v1.MatchCondition) erro return nil } + +// ValidateRegex returns an error if the supplied +// RE2 regex syntax is invalid. +func ValidateRegex(regex string) error { + _, err := regexp.Compile(regex) + return err +} diff --git a/internal/dag/ingress_processor.go b/internal/dag/ingress_processor.go index 3a0b154c071..d5a514691c1 100644 --- a/internal/dag/ingress_processor.go +++ b/internal/dag/ingress_processor.go @@ -140,7 +140,15 @@ func (p *IngressProcessor) computeIngressRule(ing *v1beta1.Ingress, rule v1beta1 continue } - r := route(ing, path, s, clientCertSecret, p.FieldLogger) + r, err := route(ing, path, s, clientCertSecret, p.FieldLogger) + if err != nil { + p.WithError(err). + WithField("name", ing.GetName()). + WithField("namespace", ing.GetNamespace()). + WithField("regex", path). + Errorf("path regex is not valid") + return + } // should we create port 80 routes for this ingress if annotation.TLSRequired(ing) || annotation.HTTPAllowed(ing) { @@ -158,7 +166,7 @@ func (p *IngressProcessor) computeIngressRule(ing *v1beta1.Ingress, rule v1beta1 } // route builds a dag.Route for the supplied Ingress. -func route(ingress *v1beta1.Ingress, path string, service *Service, clientCertSecret *Secret, log logrus.FieldLogger) *Route { +func route(ingress *v1beta1.Ingress, path string, service *Service, clientCertSecret *Secret, log logrus.FieldLogger) (*Route, error) { log = log.WithFields(logrus.Fields{ "name": ingress.Name, "namespace": ingress.Namespace, @@ -177,13 +185,17 @@ func route(ingress *v1beta1.Ingress, path string, service *Service, clientCertSe } if strings.ContainsAny(path, "^+*[]%") { - // path smells like a regex + // validate the regex + if err := ValidateRegex(path); err != nil { + return nil, err + } + r.PathMatchCondition = &RegexMatchCondition{Regex: path} - return r + return r, nil } r.PathMatchCondition = &PrefixMatchCondition{Prefix: path} - return r + return r, nil } // rulesFromSpec merges the IngressSpec's Rules with a synthetic diff --git a/internal/envoy/v2/regex.go b/internal/envoy/v2/regex.go index 71304da8ca3..707feb2cec4 100644 --- a/internal/envoy/v2/regex.go +++ b/internal/envoy/v2/regex.go @@ -15,28 +15,14 @@ package v2 import ( matcher "github.com/envoyproxy/go-control-plane/envoy/type/matcher" - "github.com/projectcontour/contour/internal/protobuf" ) -// maxRegexProgramSize is the default value for the Envoy regex max -// program size tunable. There's no way to really know what a good -// value for this is, except that the RE2 maintainer thinks that 100 -// is low. As a rule of thumb, each '.*' expression costs about 15 -// units of program size. AFAIK, there's no obvious correlation -// between regex size and execution time. -// -// See also https://github.com/envoyproxy/envoy/pull/9171#discussion_r351974033 -// and https://github.com/projectcontour/contour/issues/2240 -const maxRegexProgramSize = 1 << 20 - -// SafeRegexMatch retruns a matcher.RegexMatcher for the supplied regex. +// SafeRegexMatch returns a matcher.RegexMatcher for the supplied regex. // SafeRegexMatch does not escape regex meta characters. func SafeRegexMatch(regex string) *matcher.RegexMatcher { return &matcher.RegexMatcher{ EngineType: &matcher.RegexMatcher_GoogleRe2{ - GoogleRe2: &matcher.RegexMatcher_GoogleRE2{ - MaxProgramSize: protobuf.UInt32(maxRegexProgramSize), - }, + GoogleRe2: &matcher.RegexMatcher_GoogleRE2{}, }, Regex: regex, } diff --git a/internal/envoy/v2/regex_test.go b/internal/envoy/v2/regex_test.go index ab7df1e7464..6c50372b34a 100644 --- a/internal/envoy/v2/regex_test.go +++ b/internal/envoy/v2/regex_test.go @@ -29,9 +29,7 @@ func TestSafeRegexMatch(t *testing.T) { regex: "", want: &matcher.RegexMatcher{ EngineType: &matcher.RegexMatcher_GoogleRe2{ - GoogleRe2: &matcher.RegexMatcher_GoogleRE2{ - MaxProgramSize: protobuf.UInt32(maxRegexProgramSize), - }, + GoogleRe2: &matcher.RegexMatcher_GoogleRE2{}, }, }, }, @@ -39,9 +37,7 @@ func TestSafeRegexMatch(t *testing.T) { regex: "chrome", want: &matcher.RegexMatcher{ EngineType: &matcher.RegexMatcher_GoogleRe2{ - GoogleRe2: &matcher.RegexMatcher_GoogleRE2{ - MaxProgramSize: protobuf.UInt32(maxRegexProgramSize), - }, + GoogleRe2: &matcher.RegexMatcher_GoogleRE2{}, }, Regex: "chrome", }, @@ -50,9 +46,7 @@ func TestSafeRegexMatch(t *testing.T) { regex: "[a-z]+$", want: &matcher.RegexMatcher{ EngineType: &matcher.RegexMatcher_GoogleRe2{ - GoogleRe2: &matcher.RegexMatcher_GoogleRE2{ - MaxProgramSize: protobuf.UInt32(maxRegexProgramSize), - }, + GoogleRe2: &matcher.RegexMatcher_GoogleRE2{}, }, Regex: "[a-z]+$", // meta characters are not escaped. }, diff --git a/internal/featuretests/v2/route_test.go b/internal/featuretests/v2/route_test.go index b750af21e30..13adea82480 100644 --- a/internal/featuretests/v2/route_test.go +++ b/internal/featuretests/v2/route_test.go @@ -1509,6 +1509,47 @@ func TestRoutePrefixRouteRegex(t *testing.T) { TypeUrl: routeType, Nonce: "1", }) + + invalid := &v1beta1.Ingress{ + ObjectMeta: meta, + Spec: v1beta1.IngressSpec{ + Backend: &v1beta1.IngressBackend{ + ServiceName: "kuard", + ServicePort: intstr.FromInt(80), + }, + Rules: []v1beta1.IngressRule{{ + IngressRuleValue: v1beta1.IngressRuleValue{ + HTTP: &v1beta1.HTTPIngressRuleValue{ + Paths: []v1beta1.HTTPIngressPath{{ + Path: "^\\/(?!\\/)(.*?)", + Backend: v1beta1.IngressBackend{ + ServiceName: "kuard", + ServicePort: intstr.FromInt(80), + }, + }}, + }, + }, + }}, + }, + } + rh.OnAdd(invalid) + + // check that it's been translated correctly. + c.Request(routeType).Equals(&envoy_api_v2.DiscoveryResponse{ + VersionInfo: "1", + Resources: routeResources(t, + envoy_v2.RouteConfiguration("ingress_http", + envoy_v2.VirtualHost("*", + &envoy_api_v2_route.Route{ + Match: routePrefix("/"), + Action: routecluster("default/kuard/80/da39a3ee5e"), + }, + ), + ), + ), + TypeUrl: routeType, + Nonce: "1", + }) } func assertRDS(t *testing.T, c *Contour, versioninfo string, ingressHTTP, ingressHTTPS []*envoy_api_v2_route.VirtualHost) {