Skip to content

Commit

Permalink
opa: pass URL query parameters to OPA policy evaluation (#3207)
Browse files Browse the repository at this point in the history 
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.

- Add test cases to cover multi valued query params and trailing ? in URL path

- 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)

- Use req.URL.RequestURI() to set the path with query params

Signed-off-by: Farasath Ahamed <[email protected]>
  • Loading branch information
@mefarazath
mefarazath authored Sep 12, 2024
1 parent bd87e31 commit 7cb3dc2
Show file tree
Hide file tree
Showing 3 changed files with 181 additions and 8 deletions.
25 changes: 21 additions & 4 deletions filters/openpolicyagent/internal/envoy/skipperadapter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package envoy
import (
"fmt"
"net/http"
"net/url"
"strings"
"unicode/utf8"

Expand All @@ -11,6 +12,9 @@ import (
)

func AdaptToExtAuthRequest(req *http.Request, metadata *ext_authz_v3_core.Metadata, contextExtensions map[string]string, rawBody []byte) (*ext_authz_v3.CheckRequest, error) {
if err := validateURLForInvalidUTF8(req.URL); err != nil {
return nil, fmt.Errorf("invalid url: %w", err)
}

headers := make(map[string]string, len(req.Header))
for h, vv := range req.Header {
Expand All @@ -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,
},
Expand All @@ -35,9 +39,22 @@ 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
}

func validateURLForInvalidUTF8(u *url.URL) error {
if !utf8.ValidString(u.Path) {
return fmt.Errorf("invalid utf8 in path: %q", u.Path)
}

return ereq, nil
decodedQuery, err := url.QueryUnescape(u.RawQuery)
if err != nil {
return fmt.Errorf("error unescaping query string %q: %w", u.RawQuery, err)
}

if !utf8.ValidString(decodedQuery) {
return fmt.Errorf("invalid utf8 in query: %q", u.RawQuery)
}

return nil
}
110 changes: 107 additions & 3 deletions filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package opaauthorizerequest

import (
"fmt"
"github.com/zalando/skipper/filters/builtin"
"io"
"log"
"net/http"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -57,6 +59,49 @@ func TestAuthorizeRequestFilter(t *testing.T) {
backendHeaders: make(http.Header),
removeHeaders: make(http.Header),
},
{
msg: "Allow Requests with spaces in path",
filterName: "opaAuthorizeRequest",
bundleName: "somebundle.tar.gz",
regoQuery: "envoy/authz/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,
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&id=1&id=2&msg=help%20me",
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",
Expand All @@ -71,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",
Expand All @@ -96,6 +155,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",
Expand Down Expand Up @@ -242,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",
Expand Down Expand Up @@ -284,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)
Expand All @@ -309,6 +395,23 @@ 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 {
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 {
Expand Down Expand Up @@ -420,8 +523,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...)

Expand Down
54 changes: 53 additions & 1 deletion filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -70,6 +70,26 @@ 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",
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",
Expand Down Expand Up @@ -133,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)
Expand Down Expand Up @@ -164,6 +194,28 @@ func TestAuthorizeRequestFilter(t *testing.T) {
"http_status": 200
}
}
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"]
response := {
"allowed": true,
"headers": {"x-ext-auth-allow": "yes"},
"body": "Welcome from policy!",
"http_status": 200
}
}
allow_object = response {
input.parsed_path = [ "allow", "production" ]
Expand Down

0 comments on commit 7cb3dc2

Please sign in to comment.