From 6dcb620abae41979730571c95aac292110ac8284 Mon Sep 17 00:00:00 2001 From: Farasath Ahamed Date: Mon, 26 Aug 2024 15:02:08 +0200 Subject: [PATCH 1/6] opa: pass URL query parameters to OPA policy evaluation Improve the envoy request adapting logic to include query parameters sent in the request. This would allow the policy evaluationin opaAuthorizeRequest* and opaServeResponse* filters to make use of query parameters/values in the policy. Signed-off-by: Farasath Ahamed --- .../internal/envoy/skipperadapter.go | 10 +++--- .../opaauthorizerequest_test.go | 32 +++++++++++++++++++ .../opaserveresponse/opaserveresponse_test.go | 21 ++++++++++++ 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/filters/openpolicyagent/internal/envoy/skipperadapter.go b/filters/openpolicyagent/internal/envoy/skipperadapter.go index 6d8cf771bd..69c1ec7c44 100644 --- a/filters/openpolicyagent/internal/envoy/skipperadapter.go +++ b/filters/openpolicyagent/internal/envoy/skipperadapter.go @@ -12,6 +12,10 @@ 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) + } + headers := make(map[string]string, len(req.Header)) for h, vv := range req.Header { // This makes headers in the input compatible with what Envoy does, i.e. allows to use policy fragments designed for envoy @@ -25,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.Path, + Path: req.URL.RequestURI(), Headers: headers, RawBody: rawBody, }, @@ -35,9 +39,5 @@ func AdaptToExtAuthRequest(req *http.Request, metadata *ext_authz_v3_core.Metada }, } - if !utf8.ValidString(ereq.Attributes.Request.Http.Path) { - return nil, fmt.Errorf("invalid utf8 in path: %q", ereq.Attributes.Request.Http.Path) - } - return ereq, nil } diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go index ab7616086c..a9ac7d13a0 100644 --- a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go +++ b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go @@ -57,6 +57,20 @@ func TestAuthorizeRequestFilter(t *testing.T) { backendHeaders: make(http.Header), removeHeaders: make(http.Header), }, + { + msg: "Allow Requests with query parameters", + filterName: "opaAuthorizeRequest", + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow", + requestPath: "/allow-with-query?pass=yes&q2=v2", + requestMethod: "GET", + contextExtensions: "", + expectedStatus: http.StatusOK, + expectedBody: "Welcome!", + expectedHeaders: make(http.Header), + backendHeaders: make(http.Header), + removeHeaders: make(http.Header), + }, { msg: "Allow Matching Context Extension", filterName: "opaAuthorizeRequest", @@ -96,6 +110,19 @@ func TestAuthorizeRequestFilter(t *testing.T) { backendHeaders: make(http.Header), removeHeaders: make(http.Header), }, + { + msg: "Simple Forbidden with Query Parameters", + filterName: "opaAuthorizeRequest", + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow", + requestPath: "/allow-with-query?tofail=true", + requestMethod: "GET", + contextExtensions: "", + expectedStatus: http.StatusForbidden, + expectedHeaders: make(http.Header), + backendHeaders: make(http.Header), + removeHeaders: make(http.Header), + }, { msg: "Allow With Structured Rules", filterName: "opaAuthorizeRequest", @@ -311,6 +338,11 @@ func TestAuthorizeRequestFilter(t *testing.T) { input.parsed_path = [ "allow" ] } + allow { + input.parsed_path = [ "allow-with-query" ] + input.parsed_query.pass == ["yes"] + } + allow_context_extensions { input.attributes.contextExtensions["com.mycompany.myprop"] == "myvalue" } diff --git a/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go b/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go index 2746c9e8c7..77f89384e5 100644 --- a/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go +++ b/filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go @@ -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 Query Params", + filterName: "opaServeResponse", + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow_object", + requestPath: "/allow/structured/with-query?pass=yes", + expectedStatus: http.StatusOK, + expectedBody: "Welcome from policy!", + expectedHeaders: map[string][]string{"X-Ext-Auth-Allow": {"yes"}}, + }, { msg: "Allow With opa.runtime execution", filterName: "opaServeResponse", @@ -164,6 +174,17 @@ func TestAuthorizeRequestFilter(t *testing.T) { "http_status": 200 } } + + allow_object = response { + input.parsed_path = [ "allow", "structured", "with-query" ] + input.parsed_query.pass == ["yes"] + response := { + "allowed": true, + "headers": {"x-ext-auth-allow": "yes"}, + "body": "Welcome from policy!", + "http_status": 200 + } + } allow_object = response { input.parsed_path = [ "allow", "production" ] From c196e2a9936ba3d1fed0540b2b428ca826e817b6 Mon Sep 17 00:00:00 2001 From: Farasath Ahamed Date: Tue, 27 Aug 2024 09:44:54 +0200 Subject: [PATCH 2/6] Add test cases to cover multi valued query params and trailing ? in URL path Signed-off-by: Farasath Ahamed --- .../opaauthorizerequest_test.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go index a9ac7d13a0..58df59b8f0 100644 --- a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go +++ b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go @@ -57,12 +57,26 @@ func TestAuthorizeRequestFilter(t *testing.T) { backendHeaders: make(http.Header), removeHeaders: make(http.Header), }, + { + msg: "Allow Requests with trailing ? in URL", + filterName: "opaAuthorizeRequest", + bundleName: "somebundle.tar.gz", + regoQuery: "envoy/authz/allow", + requestPath: "/allow?", + requestMethod: "GET", + contextExtensions: "", + expectedStatus: http.StatusOK, + expectedBody: "Welcome!", + expectedHeaders: make(http.Header), + backendHeaders: make(http.Header), + removeHeaders: make(http.Header), + }, { msg: "Allow Requests with query parameters", filterName: "opaAuthorizeRequest", bundleName: "somebundle.tar.gz", regoQuery: "envoy/authz/allow", - requestPath: "/allow-with-query?pass=yes&q2=v2", + requestPath: "/allow-with-query?pass=yes&id=1&id=2", requestMethod: "GET", contextExtensions: "", expectedStatus: http.StatusOK, @@ -341,6 +355,7 @@ func TestAuthorizeRequestFilter(t *testing.T) { allow { input.parsed_path = [ "allow-with-query" ] input.parsed_query.pass == ["yes"] + input.parsed_query.id == ["1", "2"] } allow_context_extensions { From 562a348a26e81b1affc83526362677fbc33f9616 Mon Sep 17 00:00:00 2001 From: Farasath Ahamed Date: Tue, 3 Sep 2024 16:21:50 +0200 Subject: [PATCH 3/6] Change the logic to build path with query params - Use escaped path + raw query string to build the path set in envoy request - Add test cases to cover few additional special cases (empty query string, space in path) Signed-off-by: Farasath Ahamed --- .../internal/envoy/skipperadapter.go | 32 +++++++-- .../opaauthorizerequest_test.go | 66 +++++++++++++++++-- .../opaserveresponse/opaserveresponse_test.go | 33 +++++++++- 3 files changed, 121 insertions(+), 10 deletions(-) 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"] From 22af1e19e9c196c51fdb167ef3d3c1efd9730e75 Mon Sep 17 00:00:00 2001 From: Farasath Ahamed Date: Mon, 9 Sep 2024 11:03:01 +0200 Subject: [PATCH 4/6] Fix error message when unescaping query string Signed-off-by: Farasath Ahamed --- filters/openpolicyagent/internal/envoy/skipperadapter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filters/openpolicyagent/internal/envoy/skipperadapter.go b/filters/openpolicyagent/internal/envoy/skipperadapter.go index 44d442499f..7f7d5c22e5 100644 --- a/filters/openpolicyagent/internal/envoy/skipperadapter.go +++ b/filters/openpolicyagent/internal/envoy/skipperadapter.go @@ -56,7 +56,7 @@ func validateURLForInvalidUTF8(u *url.URL) error { decodedQuery, err := url.QueryUnescape(u.RawQuery) if err != nil { - return fmt.Errorf("error decoding query string: %w", err) + return fmt.Errorf("error unescaping query string %q: %w", u.RawQuery, err) } if !utf8.ValidString(decodedQuery) { From 66715d3d83b6e2b3822d6acadfd3a27edd62152b Mon Sep 17 00:00:00 2001 From: Farasath Ahamed Date: Tue, 10 Sep 2024 13:22:24 +0200 Subject: [PATCH 5/6] Add unit tests for encoding in query path Signed-off-by: Farasath Ahamed --- .../opaauthorizerequest/opaauthorizerequest_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go index 3ce88fdc14..90b915e5ba 100644 --- a/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go +++ b/filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go @@ -93,7 +93,7 @@ func TestAuthorizeRequestFilter(t *testing.T) { filterName: "opaAuthorizeRequest", bundleName: "somebundle.tar.gz", regoQuery: "envoy/authz/allow", - requestPath: "/allow-with-query?pass=yes&id=1&id=2", + requestPath: "/allow-with-query?pass=yes&id=1&id=2&msg=help%20me", requestMethod: "GET", contextExtensions: "", expectedStatus: http.StatusOK, @@ -411,6 +411,7 @@ func TestAuthorizeRequestFilter(t *testing.T) { input.parsed_path = [ "allow-with-query" ] input.parsed_query.pass == ["yes"] input.parsed_query.id == ["1", "2"] + input.parsed_query.msg == ["help me"] } allow_context_extensions { From d2e3794d4801d7f07ead29da5e98bb65a7d65d5a Mon Sep 17 00:00:00 2001 From: Farasath Ahamed Date: Tue, 10 Sep 2024 21:55:07 +0200 Subject: [PATCH 6/6] Use req.URL.RequestURI() to set the path with query params Signed-off-by: Farasath Ahamed --- filters/openpolicyagent/internal/envoy/skipperadapter.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/filters/openpolicyagent/internal/envoy/skipperadapter.go b/filters/openpolicyagent/internal/envoy/skipperadapter.go index 7f7d5c22e5..7c2284ecb3 100644 --- a/filters/openpolicyagent/internal/envoy/skipperadapter.go +++ b/filters/openpolicyagent/internal/envoy/skipperadapter.go @@ -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: buildPathWithQueryParams(req), + Path: req.URL.RequestURI(), Headers: headers, RawBody: rawBody, }, @@ -42,13 +42,6 @@ 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)