-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fuzzy OPA #216
Fuzzy OPA #216
Conversation
cc8f74e
to
62ca8c8
Compare
I'm afraid this might affect performance with multiple Rego rules having to be registered as queries, both while pre-compiling the policy and later when the built-in OPA module evaluates the policy and handles control back to Authorino carrying a bigger, more complex response. Maybe we should put this under a field option added to the API: spec:
authorization:
- opa:
inlineRego: |
r1 { ... }
r2 { ... }
allow { true }
fuzzy: true # defaults to false |
pkg/config/authorization/opa.go
Outdated
policyFileName := opa.policyUID + ".rego" | ||
|
||
var module *opaParser.Module | ||
rules := make(map[string]interface{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "allow" query should always be injected or runtime errors may happen:
panic: interface conversion: interface {} is nil, not bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this error would not happen due to
authorino/pkg/config/authorization/opa.go
Line 21 in b07bb9c
default allow = false |
On the other hand, if #216 (comment) is done, then yes. In case of looping through the list of rules to register all the queries is behind a conditional (i.e. only when fuzzy: true
), then we'd need to ensure the "allow" query is always injected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
986edf2
to
8f807cd
Compare
8f807cd
to
4714a2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good!
22a862e
to
ee29199
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok to me, just small comment around the type casting
Just tested the changes and it works
|
Adds the option to return all values in the OPA virtual document (all rules), instead of just the "allow" rule that is used by Authorino to decide about the policy overall.
This allows to extract individual values from the Rego virtual documents, e.g. to be used in subsequent authorization policies (i.e.
> priority
) and custom responses and implement use cases such as of fuzzy authorization.Partial OPA policies can be defined to evaluate complex authorization rules and "export" values via Authorization JSON to other evaluators. The overall authorization decision can be postponed by setting
allow = true
in the partial policies.E.g.:
Verification steps
Breaking changes
The resolved object returned by the OPA authorization evaluators is no longer a simple boolean value, but now an actual object
{ "allow": boolean, ...other rules }
, where...other rules
is only available whenallValues: true
.Bug fixes
Fixed non-boolean values set for the ‘allow’ rule crashes Authorino (Thanks, @maleck13!)
Related to #109