sdk: Fix decision log masking of input object #6090
Conversation
epaulson10
force-pushed
the
sdk-decision-log-masking-fix
branch
from
432e0cd
to
936dd1c
Compare
sdk/opa.go
Outdated
| // or []interface{}. If the input was not one of those types, rebuild it from the AST. | ||
| switch (*record.Input).(type) { | ||
| case map[string]interface{}, []interface{}: | ||
| break |
There was a problem hiding this comment.
[nit] This break statement is superfluous. If it just read
switch (*record.Input).(type) {
case map[string]interface{}, []interface{}:
default:
// ...
}it would have the same effect.
There was a problem hiding this comment.
What about other native types, though? Do we need to worry about, say, []string, which won't be covered by []interface{} here? (Or does that need the same treatment because the decision logger also can't deal with []string? I can't remember 😅)
There was a problem hiding this comment.
Thanks, always having a break is something from C that I have trouble giving up.
What about other native types, though?
The masking logic only lets you mask keys in objects. You cannot mask elements in slices (although you can reference an object key that is contained in a slice).
To your point on handling native types, I realized that checking for the type of object with the switch was not good enough, as an inner object of the map[string]interface{} couldbe amap[string]string`, for example. So I dropped the switch and am unconditionally doing the conversion. It would be nice to know if masking was enabled so we could avoid this conversion unless it is necessary, but it seems that requires a rego query to determine.
sdk/opa.go
Outdated
| case map[string]interface{}, []interface{}: | ||
| break | ||
| default: | ||
| asJSON, err := ast.JSON(record.InputAST) |
There was a problem hiding this comment.
💭 I was wondering a bit about some unnecessary work here -- since the execute path needs to do something similar -- but that has already been taken into account: we're using record.InputAST here, not record.Input. 👍
There was a problem hiding this comment.
I realized another way to avoid unnecessary work is to move this logic into the if statement that sees if the logger is registered with the manager. There is no sense in doing this unless we are going to actually log something.
sdk/opa_test.go
Outdated
|
|
||
| defer opa.Stop(ctx) | ||
|
|
||
| // Verify that timestamp matches time.now_ns() value. |
There was a problem hiding this comment.
[nit] Comment doesn't seem acurate anymore -- also, we don't need to deal with time here, do we?
There was a problem hiding this comment.
You have caught me cargo-culting from another test 🙈 Fixed.
The sdk package allows you to pass arbitrary Go objects as the input
document. This is fine for evaluation (as long as the object can be
parsed to an AST), but is problematic for decision log masking, as the
masking logic expects the input on the event to be either a
map[string]interface{} or a []interface{}, and for inner types of the
object to also be similarly generic. Currently, the decision log masking
silently fails if the input object is not of the correct type.
This patch addresses this by rebuilding the input Go type for the
decision log from the parsed AST of the original input object. This
generates a Go type that does not break the masking logic, provided the
input is of a type that can be masked. The conversion from the AST only
happens if the decision logs plugin is actually registered with the
manager to avoid wasting cycles if decision logs are not enabled.
A test is added to cover the masking case for the SDK.
Signed-off-by: Erik Paulson <epaulson10@gmail.com>
epaulson10
force-pushed
the
sdk-decision-log-masking-fix
branch
from
936dd1c
to
939a0f8
Compare
|
I amended my commit based on the above discussion. Main points are:
|
The sdk package allows you to pass arbitrary Go objects as the input document. This is fine for evaluation (as long as the object can be parsed to an AST), but is problematic for decision log masking, as the masking logic expects the input on the event to be either a map[string]interface{} or a []interface{}. Currently, the decision log masking silently fails if the input object is not of the correct type.
This commit addresses this by checking if the input object is of the correct type, and if not, rebuilds it from the input AST before it is logged.
Also, a test is added to cover this case.
Why the changes in this PR are needed?
The SDK package allows you to specify Go structs as the input to a query. However, the decision log masking feature will fail silently when you do so, because the masking only allows the input to be of type
map[string]interface{}or[]interface{}.What are the changes in this PR?
Adds a test that reproduces this issue.
Checks the type of the provided input object, and if it is not compatible with the decision log masking, it rebuilds the object from the parsed AST of the original input.
Notes to assist PR review:
N/A
Further comments:
The idea to rebuild the object from the AST came from how the HTTP server handles the input.