diff --git a/filters/openpolicyagent/internal/envoy/skipperadapter.go b/filters/openpolicyagent/internal/envoy/skipperadapter.go index 69c1ec7c44..44d442499f 100644 --- a/filters/openpolicyagent/internal/envoy/skipperadapter.go +++ b/filters/openpolicyagent/internal/envoy/skipperadapter.go @@ -3,6 +3,7 @@ package envoy import ( "fmt" "net/http" + "net/url" "strings" "unicode/utf8" @@ -11,9 +12,8 @@ import ( ) func AdaptToExtAuthRequest(req *http.Request, metadata *ext_authz_v3_core.Metadata, contextExtensions map[string]string, rawBody []byte) (*ext_authz_v3.CheckRequest, error) { - - if !utf8.ValidString(req.URL.Path) { - return nil, fmt.Errorf("invalid utf8 in path: %q", req.URL.Path) + if err := validateURLForInvalidUTF8(req.URL); err != nil { + return nil, fmt.Errorf("invalid url: %w", err) } headers := make(map[string]string, len(req.Header)) @@ -29,7 +29,7 @@ func AdaptToExtAuthRequest(req *http.Request, metadata *ext_authz_v3_core.Metada Http: &ext_authz_v3.AttributeContext_HttpRequest{ Host: req.Host, Method: req.Method, - Path: req.URL.RequestURI(), + Path: buildPathWithQueryParams(req), Headers: headers, RawBody: rawBody, }, @@ -41,3 +41,27 @@ func AdaptToExtAuthRequest(req *http.Request, metadata *ext_authz_v3_core.Metada return ereq, nil } + +func buildPathWithQueryParams(req *http.Request) string { + if req.URL.RawQuery == "" { + return url.PathEscape(req.URL.Path) + } + return url.PathEscape(req.URL.Path) + "?" + req.URL.RawQuery +} + +func validateURLForInvalidUTF8(u *url.URL) error { + if !utf8.ValidString(u.Path) { + return fmt.Errorf("invalid utf8 in path: %q", u.Path) + } + + decodedQuery, err := url.QueryUnescape(u.RawQuery) + if err != nil { + return fmt.Errorf("error decoding query string: %w", err) + } + + if !utf8.ValidString(decodedQuery) { + return fmt.Errorf("invalid utf8 in query: %q", u.RawQuery) + } + + return nil +} diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go index 58df59b8f0..3ce88fdc14 100644 --- a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go +++ b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go @@ -2,6 +2,7 @@ package opaauthorizerequest import ( "fmt" + "github.com/zalando/skipper/filters/builtin" "io" "log" "net/http" @@ -29,7 +30,8 @@ func TestAuthorizeRequestFilter(t *testing.T) { for _, ti := range []struct { msg string filterName string - extraeskip string + extraeskipBefore string + extraeskipAfter string bundleName string regoQuery string requestPath string @@ -58,11 +60,26 @@ func TestAuthorizeRequestFilter(t *testing.T) { removeHeaders: make(http.Header), }, { - msg: "Allow Requests with trailing ? in URL", + msg: "Allow Requests with spaces in path", filterName: "opaAuthorizeRequest", bundleName: "somebundle.tar.gz", regoQuery: "envoy/authz/allow", - requestPath: "/allow?", + requestPath: "/my%20path", + requestMethod: "GET", + contextExtensions: "", + expectedStatus: http.StatusOK, + expectedBody: "Welcome!", + expectedHeaders: make(http.Header), + backendHeaders: make(http.Header), + removeHeaders: make(http.Header), + }, + { + msg: "Allow Requests with request path overridden by the setPath filter", + filterName: "opaAuthorizeRequest", + extraeskipBefore: `setPath("/allow") ->`, + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow", + requestPath: "/some-random-path-that-would-fail", requestMethod: "GET", contextExtensions: "", expectedStatus: http.StatusOK, @@ -99,6 +116,20 @@ func TestAuthorizeRequestFilter(t *testing.T) { backendHeaders: make(http.Header), removeHeaders: make(http.Header), }, + { + msg: "Allow Requests with an empty query string", + filterName: "opaAuthorizeRequest", + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow_context_extensions", + requestPath: "/path-with-empty-query?", + requestMethod: "GET", + contextExtensions: "com.mycompany.myprop: myvalue", + expectedStatus: http.StatusOK, + expectedBody: "Welcome!", + expectedHeaders: make(http.Header), + backendHeaders: make(http.Header), + removeHeaders: make(http.Header), + }, { msg: "Allow Matching Environment", filterName: "opaAuthorizeRequest", @@ -283,7 +314,7 @@ func TestAuthorizeRequestFilter(t *testing.T) { { msg: "Chained OPA filter with body", filterName: "opaAuthorizeRequestWithBody", - extraeskip: `-> opaAuthorizeRequestWithBody("somebundle.tar.gz")`, + extraeskipAfter: `-> opaAuthorizeRequestWithBody("somebundle.tar.gz")`, bundleName: "somebundle.tar.gz", regoQuery: "envoy/authz/allow_body", requestMethod: "POST", @@ -325,6 +356,20 @@ func TestAuthorizeRequestFilter(t *testing.T) { backendHeaders: make(http.Header), removeHeaders: make(http.Header), }, + { + msg: "Invalid UTF-8 in Query", + filterName: "opaAuthorizeRequest", + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow", + requestPath: "/allow?%c0%ae=%c0%ae%c0%ae", + requestMethod: "GET", + contextExtensions: "", + expectedStatus: http.StatusBadRequest, + expectedBody: "", + expectedHeaders: make(http.Header), + backendHeaders: make(http.Header), + removeHeaders: make(http.Header), + }, } { t.Run(ti.msg, func(t *testing.T) { t.Logf("Running test for %v", ti) @@ -350,6 +395,16 @@ func TestAuthorizeRequestFilter(t *testing.T) { allow { input.parsed_path = [ "allow" ] + input.parsed_query = {} + } + + allow { + input.parsed_path = [ "my path" ] + } + + allow { + input.parsed_path = [ "/path-with-empty-query" ] + input.parsed_query = {} } allow { @@ -467,8 +522,9 @@ func TestAuthorizeRequestFilter(t *testing.T) { fr.Register(ftSpec) ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, opts...) fr.Register(ftSpec) + fr.Register(builtin.NewSetPath()) - r := eskip.MustParse(fmt.Sprintf(`* -> %s("%s", "%s") %s -> "%s"`, ti.filterName, ti.bundleName, ti.contextExtensions, ti.extraeskip, clientServer.URL)) + r := eskip.MustParse(fmt.Sprintf(`* -> %s %s("%s", "%s") %s -> "%s"`, ti.extraeskipBefore, ti.filterName, ti.bundleName, ti.contextExtensions, ti.extraeskipAfter, clientServer.URL)) proxy := proxytest.New(fr, r...) diff --git a/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go b/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go index 77f89384e5..89c4e611d8 100644 --- a/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go +++ b/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go @@ -17,7 +17,7 @@ import ( "github.com/zalando/skipper/filters/openpolicyagent" ) -func TestAuthorizeRequestFilter(t *testing.T) { +func TestServerResponseFilter(t *testing.T) { for _, ti := range []struct { msg string filterName string @@ -70,6 +70,16 @@ func TestAuthorizeRequestFilter(t *testing.T) { expectedBody: "Welcome from policy!", expectedHeaders: map[string][]string{"X-Ext-Auth-Allow": {"yes"}}, }, + { + msg: "Allow With Structured Rules and empty query string in path", + filterName: "opaServeResponse", + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow_object", + requestPath: "/allow/structured/with-empty-query-string?", + expectedStatus: http.StatusOK, + expectedBody: "Welcome from policy!", + expectedHeaders: map[string][]string{"X-Ext-Auth-Allow": {"yes"}}, + }, { msg: "Allow With Structured Rules and Query Params", filterName: "opaServeResponse", @@ -143,6 +153,16 @@ func TestAuthorizeRequestFilter(t *testing.T) { expectedBody: "", expectedHeaders: make(http.Header), }, + { + msg: "Invalid UTF-8 in Query String", + filterName: "opaServeResponse", + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow", + requestPath: "/allow?%c0%ae=%c0%ae%c0%ae", + expectedStatus: http.StatusBadRequest, + expectedBody: "", + expectedHeaders: make(http.Header), + }, } { t.Run(ti.msg, func(t *testing.T) { t.Logf("Running test for %v", ti) @@ -175,6 +195,17 @@ func TestAuthorizeRequestFilter(t *testing.T) { } } + allow_object = response { + input.parsed_path = [ "allow", "structured", "with-empty-query-string" ] + input.parsed_query == {} + response := { + "allowed": true, + "headers": {"x-ext-auth-allow": "yes"}, + "body": "Welcome from policy!", + "http_status": 200 + } + } + allow_object = response { input.parsed_path = [ "allow", "structured", "with-query" ] input.parsed_query.pass == ["yes"]