OPA: Implement row level filtering and column masking #20921
Conversation
plugin/trino-opa/src/main/java/io/trino/plugin/opa/schema/OpaColumnMaskQueryResult.java
Outdated
Show resolved
Hide resolved
plugin/trino-opa/src/main/java/io/trino/plugin/opa/schema/OpaViewExpression.java
Show resolved
Hide resolved
| private Optional<URI> opaRowFiltersUri = Optional.empty(); | ||
| private Optional<URI> opaColumnMaskingUri = Optional.empty(); |
There was a problem hiding this comment.
In the future, it would be nice if the OPA server had a configration endpoint (like Oauth), which told us these URI along with the batch URI. We could then either call this once during setup, or periodically in the background.
There was a problem hiding this comment.
This is a great idea, we were playing with a similar concept. The one thing I'm unsure about is allowing a configuration endpoint to determine whether row filtering is applied at all, for instance - as that feels more like a configuration responsibility. Kind of like if the OAuth config endpoint allowed you to disable auth altogether.
Not opposed to it, just some thoughts.
An alternative:
What's interesting is that we don't technically need these to be different URIs, they could just be different keys from the response and we could do everything OPA-side without including another type of request.
Suppose we had:
opa.policy.uri = https://opa/v1/data/my_policy/allowopa.policy.batched-uri = https://opa/v1/data/my_policy/batchAllowopa.policy.row-filters-uri = https://opa/v1/data/my_policy/rowFiltersopa.policy.column-masking-uri = https://opa/v1/data/my_policy/columnMask
This is equivalent to querying https://opa/v1/data/my_policy for all possible types of request and pulling the relevant field from the response (allow, batchAllow, rowFilters or columnMask).
The OPA policy itself can even directly determine what result to provide so that Trino doesn't need to pop fields from the response manually. This is because the request unequivocally determines what Trino is looking for:
- If
operationisGetRowFilters: the relevant result is the row filters decision - If
operationisGetColumnMask: the relevant result is the column mask decision - If
action.filterResourcesis present: the relevant result is the batch filtering decision - Otherwise, the relevant result is the standard boolean
allowdecision
Some care would need to be exercised for performance (but we can provide a small rego wrapper to deal with this)
The config would then become:
opa.policy.unified-uri(or whatever we call this new "catch-all" URI)opa.policy.enable-row-filteringopa.policy.enable-column-masking
The last two would allow us to disable row filtering and column masking from the config in case there's performance concerns
Maybe we could take this into an issue? CC @sbernauer
There was a problem hiding this comment.
Sounds interesting. Let's discuss on slack
plugin/trino-opa/src/test/java/io/trino/plugin/opa/DistributedQueryRunnerHelper.java
Outdated
Show resolved
Hide resolved
| ConfigurationMetadata<OpaConfig> metadata = ConfigurationMetadata.getValidConfigurationMetadata(OpaConfig.class); | ||
| ImmutableMap.Builder<String, String> opaConfigBuilder = ImmutableMap.builder(); | ||
| try { | ||
| for (ConfigurationMetadata.AttributeMetadata attribute : metadata.getAttributes().values()) { | ||
| convertPropertyToString(attribute.getGetter().invoke(config)).ifPresent( | ||
| propertyValue -> opaConfigBuilder.put(attribute.getInjectionPoint().getProperty(), propertyValue)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This doesn't seem like a good idea. This makes this code dependent on the internal details of the configuration system (and I think this is the only code in Trino that does something like this). This will make maitaining the Airlift configuration system more complex if this pattern catches on as changes to it will require changes to lots of Trino tests.
Instead, I suggest you either manually build the map, or just use the properties in the tests.
There was a problem hiding this comment.
Fair point. I was conflicted about whether I liked this or not, I was only trying to avoid having to keep duplicates of every config key.
I'll change this function
There was a problem hiding this comment.
As a side note, do you think there's any merit to trying to contribute a function that does this to Airlift? Or is it intentionally not provided?
There was a problem hiding this comment.
Airlift supports configurations that can't be reversed like this
plugin/trino-opa/src/test/java/io/trino/plugin/opa/TestOpaAccessControlSystem.java
Outdated
Show resolved
Hide resolved
Sorry about that! This was rebased from our fork and I squashed without too much thought. Will avoid it going forward |
| private Optional<URI> opaRowFiltersUri = Optional.empty(); | ||
| private Optional<URI> opaColumnMaskingUri = Optional.empty(); |
There was a problem hiding this comment.
Sounds interesting. Let's discuss on slack
|
Is there any documentation on the usage of this feature? (Row filtering and Column Masking) |
|
@Zahid-Iqbal-Marth you can get an example here |
Description
Add row level filtering and column masking to the OPA System access control plugin
Additional context and related issues
This was originally discussed as a side-effort of #19532 , and a first cut was added to our fork in bloomberg#16
@mosabua should I create a separate PR for docs for this?
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text: