opa: pass URL query parameters to OPA policy evaluation #3207
Conversation
mefarazath
force-pushed
the
pass-query-params-to-opa
branch
2 times, most recently
from
734e183
to
166fd5b
Compare
mefarazath
changed the title
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 <[email protected]>
mefarazath
force-pushed
the
pass-query-params-to-opa
branch
from
166fd5b
to
6dcb620
Compare
|
👍 |
filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go
Outdated
Show resolved
Hide resolved
…RL path Signed-off-by: Farasath Ahamed <[email protected]>
mefarazath
force-pushed
the
pass-query-params-to-opa
branch
from
91b0291
to
c196e2a
Compare
|
@szuecs I added a few more test cases based on review comments (which removed your previous 👍🏻 rightly so with new changes in) (Update: there a few comments to address.) |
filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go
Outdated
Show resolved
Hide resolved
- 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 <[email protected]>
mefarazath
force-pushed
the
pass-query-params-to-opa
branch
from
db63ae4
to
562a348
Compare
|
2 small things. |
Signed-off-by: Farasath Ahamed <[email protected]>
mefarazath
force-pushed
the
pass-query-params-to-opa
branch
from
1310278
to
22af1e1
Compare
Signed-off-by: Farasath Ahamed <[email protected]>
mefarazath
force-pushed
the
pass-query-params-to-opa
branch
from
569011b
to
66715d3
Compare
Signed-off-by: Farasath Ahamed <[email protected]>
|
👍 |
| if err := validateURLForInvalidUTF8(req.URL); err != nil { | ||
| return nil, fmt.Errorf("invalid url: %w", err) | ||
| } |
There was a problem hiding this comment.
Why do we need this check?
There was a problem hiding this comment.
This was done as a part of reducing errors in decision logs in OPA when the http path contained invalid UTF-8 characters. I have extended this check to query params in this PR and moved the check a bit above.
(Introduced in https://github.com/zalando/skipper/pull/3086/files#diff-dee220a324e25f9867826168abe29e559c7291d044fe87d44165c3ef28384c5cR38)
There was a problem hiding this comment.
Do you mean decision logs in Skipper or in OPA?
There was a problem hiding this comment.
Sorry I should have been precise. I meant the errors in decision logs in OPA.
There was a problem hiding this comment.
So do we want to save a roundtrip here by pre-validating URL?
Can this be done on OPA side, e.g. can it decide to reject request if URL is not valid?
There was a problem hiding this comment.
So do we want to save a roundtrip here by pre-validating URL? Can this be done on OPA side, e.g. can it decide to reject request if URL is not valid?
To be honest, saving the round trip was just a side benefit.
Our main concern was that requests with invalid URLs were failing before even reaching the policy logic (due to parse errors in OPA), but they still generated decision log entries marked as errors. This was creating a lot of noise in some of the application policies, especially when there were frequent malicious requests happening every day. We were worried that this noise could cause the teams to overlook or ignore real policy errors that actually need attention.
|
👍 |
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]>
Improve the envoy request adapting logic to include query parameters sent in the request.
This would allow the policy evaluation
opaAuthorizeRequest*andopaServeResponse*filters to make use of query parameters/values in the policy.