Add support for Open Policy Agent

[vagaerg]

Description

This PR supersedes #17940 which was getting hard to follow due to the large number of comments

Additional context and related issues

Docs PR by @mosabua is #20246

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:

## Security
* Support access control with Open Policy Agent (OPA)

And make it link to the docs

[vagaerg]

From the old PR I believe the only items that remain unresolved:

Grants:

#17940 (comment)

I am happy to remove this and implement a default deny if @dain or others think there is no value to supporting this in OPA.

IMHO OPA could provide a simple central point to enforce global policies as to what users can grant permissions on what catalogs, while deferring more granular permissioning to the connector access control logic for each catalog if needed.

Given that the meaning of a GRANT statement varies greatly across different connectors, users would need to write logic that depends on what catalog is being operated upon, but there is no reason they cannot do this with some logic in OPA.

Allowing OPA to answer an authorization question for a grant query would still let users implement their more specific permissioning logic at the connector access control level if needed, whereas denying here would immediately deny any query. It also lets users decide whether they want to globally allow or deny this style of queries.

I don't have lots of experience with this specific part of Trino, but the rest of the plugin is written with the goal of deferring all authorization requests to OPA such that users can plug in any policy they want without changing any Trino logic; and I think this same goal could be beneficial here.

Fields in the request context

#17940 (comment)

I removed enabledRoles and catalogRoles. I have for now kept extraCredentials pending discussion on whether there is a cleaner way to pass in additional credentials. My original response

We have played around (just for demos, nothing we have actually deployed) with the idea of using extra credentials to pass in JWTs that grant users extra permissions that are validated via OPA. Do you know what would be a better alternative to do this without using extraCredentials?

Including the Trino version number in the request & documenting fields in the request

From discussion on slack with @dain
I would like to add the Trino version to the OpaQueryContext object.

What way would I be able to obtain the Trino version programatically? The only way I've seen is from the NodeManager but that is not passed into the Access Control factory.

This item is pending

Cleaning up OpaQueryInputResource builders

#17940 (comment)

I would prefer to defer this for now if @dain is OK with that

Follow-up PR

We have 3 additional contributions we would like to make, happy to open PRs after this is merged:

• Addressing the OpaQueryInputResource building mentioned by @dain
• Implementing row level filtering and column masking
• Adding more options for authenticating against the OPA endpoint: OPA can be configured to require clients authenticate against it with basic credentials or mTLS (docs)

[dain]

For extraCredentials, my main concern passing through all extra credentials. My suggestion is that we leave them out for now, but if we want to add them in the future, we add a configuration property for which one to include.

For version, I put up a PR to add it to SystemAccessControlContext #19585

[vagaerg]

For extraCredentials, my main concern passing through all extra credentials. My suggestion is that we leave them out for now, but if we want to add them in the future, we add a configuration property for which one to include.

Sounds good, will remove them for now

For version, I put up a PR to add it to SystemAccessControlContext #19585

That would be great! The PR looks good, thank you!

[dain]

Some comments

[dain]

Some comments

[dain]

final for utilitiy classes

[dain]

Looking good

[dain]

We have been removing Parameterized tests throughtout the code base because they are difficult to understand and debug because the test cases are separated from the test code, and they are often over abstracted into generic functions like this, so a reader can not know what is actually being tested. Please just inline the cases.

[dain]

The Collection type here is a raw type which disables type checking in Java. Add something <?>, or even better set the actual type.

[dain]

as with the other tests, remove use of @ParameterizedTest

[dain]

Raw Map type

[vagaerg]

All comments should be addressed, I've removed parameterization on some tests, particularly the following ones in TestOpaAccessControl:

• testNoResourceAction
• testFunctionResourceActions
• testCanExecuteTableProcedure

The new assertAccessControlMethodBehaviour method will assert the authorizer method under test allows/denies correctly depending on the response returned by OPA and that it throws if an illegal response is returned. This method checks the request being sent to OPA,

If you are happy with the changes made, I will change all the other tests accordingly

[Cerebus]

Late to the party, but how is this performant at scale, particularly with the follow-on PR for row-based access control? It seems to me that a REST call per row in the input is going to really stink.

Why not use the Compile API to pre-fill user and context into the policies, then execute the query with the data locally? E.g., https://blog.openpolicyagent.org/write-policy-in-opa-enforce-policy-in-sql-d9d24db93bf4

[vagaerg]

That is a good question. From our experimentation it seems to be working well enough as we’re deploying it alongside each worker. The OPA binary is very simple to deploy and run so we just co locate it. That said, I’d love to hear your suggestion on the compile API, I’m not familiar with it. Would this allow us to compile the rego and execute it externally to trino? The row level filtering I wouldn’t expect to be much of an issue, as the actual rows are never sent to OPA. OPA just produces filter expressions that Trino applies locally - kind of like adding an extra WHERE clause Thanks!

…

___________________ Pablo Arteaga This message was sent from a mobile device, apologies for any typos or inadvertent auto-corrects. On 14 Dec 2023, at 21:55, T. Miller ***@***.***> wrote:  Late to the party, but how is this performant at scale, particularly with the follow-on PR for row-based access control? It seems to me that a REST call per row in the input is going to really stink. Why not use the Compile API to pre-fill user and context into the policies, then execute the query with the data locally? — Reply to this email directly, view it on GitHub<#19532 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ALEL2M36BW75V5O54TMH4MTYJNYTLAVCNFSM6AAAAAA6PWIKPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJWG4YTQMJZGU>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[vagaerg]

Late to the party, but how is this performant at scale, particularly with the follow-on PR for row-based access control? It seems to me that a REST call per row in the input is going to really stink.

Why not use the Compile API to pre-fill user and context into the policies, then execute the query with the data locally? E.g., https://blog.openpolicyagent.org/write-policy-in-opa-enforce-policy-in-sql-d9d24db93bf4

Just saw your updated comment with the link, thanks!
That certainly looks interesting, and perhaps something to consider in the future.

IMHO, at the moment we should keep a simple solution where OPA makes a full, final, decision.
This is mostly because Trino has no more context than what is sent to OPA: We are serializing the entirety of the available information that Trino passes down to the SPI, which isn't that large.

In fact, Trino doesn't really know all that much about the data either: in the example you linked, the application contacting OPA is aware of what pets are owned by whom and what clinicians are allowed to interact with them. This is not the case in Trino - many of these are coming from business logic that may be entirely external to Trino, and as such may as well be pushed into OPA.

E.g.: the information as to who owns a table is only partially available in Trino - tables may well be created by scripts and thus be owned by fake service accounts.

That said, I think it would be nice to in the future use this compile logic to allow OPA to return expressions that Trino would execute on the data and make the decision internally. The reason I think we should defer that is two-fold:

• This is a first cut of an OPA authorizer: fully externalizing the decision making is what is most in line with the OPA design principles, and also what we believe would cover a majority of users
• The complexity involved with using the compile API: turning a JSON blob into something that we would execute on top of the data internally is rather tricky, and raises all sorts of security & design questions

As for the row level filtering, note that row level filtering is done within Trino. There is never a case where each row is sent to OPA, because the OPA plugin never even sees the rows.

The Trino SPI expects authorizers to return a list of ViewExpression objects (code) that contain expressions that are applied internally to further refine the results - kind of like adding an extra WHERE clause.

And, in fact, using row level filtering would bring us one step closer to the system you suggested 😄

[vagaerg]

I rebased the PR and bumped the version.
There's a draft PR for row level filtering & column masking here: bloomberg#16 , I will bring it over once we get this one merged. That PR also adds some extra system tests.

I will also update the documentation branch (I'm around halfway through with that) and we can merge that after this PR too

[findepi]

cc @trinodb/maintainers for new plugin

[mosabua]

Is this term "permissioning" really what we want to use. Is that some sort of weird terminology used in OPA .. if not I would prefer we use something like
allow-security-operations or allow-sql-security-operations or allow-sql-security-statements .. wdyt ?

[dain]

I agree, the work "permissioning" is strange, and we should rename the java methods, and the config property. I suggest allow-permission-management-operations or allow-security-management-operations. @mosabua for the docs, we should mention that OPA does not have a permissions management backend for Trino, so these operations will fail regardless... meaning there is no code that takes a SQL GRANT statement and created OPA rules.

[shohamyamin]

@vagaerg @mosabua any progress with the pr?

[soenkeliebau]

I just had to look up the link for something else as well and was wondering about the status :)

Is it maybe worth getting together for two hours one evening and hashing out the remaining open points? I'd volunteer to write minutes and post them here so everything is documented and public..
@vagaerg @mosabua @dain ?

[dain]

This is good. I have one comment about changing the work "permissioning", and there is a trivial merge conflict for the main pom.

Can you update these two items, squash and push, and I'll merge it?

[dain]

I agree, the work "permissioning" is strange, and we should rename the java methods, and the config property. I suggest allow-permission-management-operations or allow-security-management-operations. @mosabua for the docs, we should mention that OPA does not have a permissions management backend for Trino, so these operations will fail regardless... meaning there is no code that takes a SQL GRANT statement and created OPA rules.

[mosabua]

Sounds good @dain .. once this PR is merged and the details are therefore settled I will update the docs PR to the current status and ask for reviews again from @vagaerg and others.

[vagaerg]

I just had to look up the link for something else as well and was wondering about the status :)

Is it maybe worth getting together for two hours one evening and hashing out the remaining open points? I'd volunteer to write minutes and post them here so everything is documented and public.. @vagaerg @mosabua @dain ?

Absolutely, sorry I missed this comment - I am happy to meet up whether for this PR (which will hopefully be merged soon!) or for the follow ups for row level filtering / column masking :)

Thanks a lot for your offer @soenkeliebau !

[vagaerg]

Rebase and renaming of the field is now done @dain
I will open a small PR shortly after this is merged to remove all parameterization. I would rather get this one merged soon and follow up on a separate PR if that's fine by you.

Thanks!

[sbernauer]

Many many thanks @vagaerg for your perseverance and @dain for merging!