Add group-based and owner-based query access rules #9811

cccs-tom · 2021-10-28T20:14:04Z

Fixes #9484.
QueryAccessRule now supports a group regex to enable rules based on group membership and an owner regex to enable rules based on the query's owner.

kokosing · 2021-10-29T12:42:08Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/QueryAccessRule.java

-            @JsonProperty("role") Optional<Pattern> roleRegex)
+            @JsonProperty("role") Optional<Pattern> roleRegex,
+            @JsonProperty("group") Optional<Pattern> groupRegex,
+            @JsonProperty("owner") Optional<Pattern> ownerRegex)


Now can grant alice to access to execute query that belongs to bob which does not make sense.

How about introducing "owner" in separate pull request where we can think of such corner cases? Or we should add check that execute cannot be used together with owner?

I see where you're going with this. I like the idea of validating that an owner rule can't have execute. However, that does mean (in relation to your other comment) that queries that aren't yet executing should be considered "owner-less", imo. My concern is with a configuration like this one:

{ "group": "staff", "owner": "alice", "allow": ["view"] }, { "allow": ["execute"] }

In the above example, assuming that user alice is a member of group staff, if user alice is already considered the owner of a query when they try to execute, the first rule will match. And since execute isn't in the allow list, the call to canExecuteQuery will ultimately return false. We need that first rule to not match in order for alice to be allowed to execute queries.

I will add a commit to validate that any owner based rules do not allow execute.

@kokosing I've added that validation, along with additional tests to cover the situation I outlined in my previous reply. I also updated the docs to include a note about this restriction.

If you still feel that the owner-based rules belong in a separate PR, I can split it. Please let me know.

If you still feel that the owner-based rules belong in a separate PR, I can split it. Please let me know.

I think it is fine. However I would make this PR to have 2 commits. Notice the PR title Add group-based and owner-based query access rules. It is natural to split this using "delimiter" and into:

Add group-based query file based access rules.

Add owner-based query file based access rules.

No problem, I can do that.

kokosing · 2021-10-29T12:43:42Z

...plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java

@@ -327,15 +327,15 @@ public void checkCanSetUser(Optional<Principal> principal, String userName)
    @Override
    public void checkCanExecuteQuery(SystemSecurityContext context)
    {
-        if (!canAccessQuery(context.getIdentity(), QueryAccessRule.AccessMode.EXECUTE)) {
+        if (!canAccessQuery(context.getIdentity(), Optional.empty(), QueryAccessRule.AccessMode.EXECUTE)) {


Isn't the owner of the query is context.getIdentity().getUser()?

The way I was looking at it, a query that is not yet executing doesn't yet have an owner.

kokosing

Looks nice!

kokosing · 2021-10-29T18:56:53Z

lib/trino-plugin-toolkit/src/test/resources/query-invalid.json

+  "queries": [
+    {
+      "group": "contractors",
+      "owner": "alice|dave",


When I look at this json I find owner to be very misleading. It looks like a property of user like group and it takes me a while to recall that it is about the query not the user. How about using queryOwnedBy. IMO the best would be onQueryOwnedBy then it would be more or less like DSL. "group": "contractors", "allow": ["execute"], "onQueryOwnedBy": "alice|dave". However prefix on does not match the existing convention for other rules.

I have no objections. I do want to note however that the owner field was already in the documentation - it was simply ignored by the code. If you're ok with changing it, then so am I! :)

You prefer queryOwnedBy over queryOwner? I don't mind either way - just checking.

I'll be honest, I'm assuming it's just a matter of time for someone to request support for queryOwnerMemberOf (for groups) and queryOwnerHasRole...

Ok, I've split the commits and renamed the owner field to onQueryOwnedBy, as discussed.

kokosing · 2021-10-29T19:02:15Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/QueryAccessRule.java

-            @JsonProperty("role") Optional<Pattern> roleRegex)
+            @JsonProperty("role") Optional<Pattern> roleRegex,
+            @JsonProperty("group") Optional<Pattern> groupRegex,
+            @JsonProperty("owner") Optional<Pattern> ownerRegex)


If you still feel that the owner-based rules belong in a separate PR, I can split it. Please let me know.

I think it is fine. However I would make this PR to have 2 commits. Notice the PR title Add group-based and owner-based query access rules. It is natural to split this using "delimiter" and into:

Add group-based query file based access rules.

Add owner-based query file based access rules.

kokosing · 2021-11-02T12:12:11Z

docs/src/main/sphinx/security/file-system-access-control.rst

-* ``owner`` (optional): regex to match against the query owner name. Defaults to ``.*``.
+* ``role`` (optional): regex to match against role names. Defaults to ``.*``.
+* ``group`` (optional): regex to match against group names. Defaults to ``.*``.
+* ``onQueryOwnedBy`` (optional): regex to match against the query owner name. Defaults to ``.*``.


Let's use queryOwner

kokosing · 2021-11-02T19:12:30Z

Merged, thanks!

sbernauer · 2021-11-02T19:20:26Z

Thanks a lot for working on this @cccs-tom!

cla-bot bot added the cla-signed label Oct 28, 2021

cccs-tom requested a review from kokosing October 28, 2021 20:14

kokosing reviewed Oct 29, 2021

View reviewed changes

cccs-tom requested a review from kokosing October 29, 2021 17:03

kokosing reviewed Oct 29, 2021

View reviewed changes

Add group-based query access rules

b367555

cccs-tom force-pushed the feature/fix-9484 branch from 182bef3 to 568680d Compare October 29, 2021 21:36

cccs-tom requested a review from kokosing October 29, 2021 21:37

kokosing approved these changes Nov 2, 2021

View reviewed changes

cccs-tom force-pushed the feature/fix-9484 branch from 568680d to 2138865 Compare November 2, 2021 14:24

Add owner-based query access rules

2138865

kokosing approved these changes Nov 2, 2021

View reviewed changes

kokosing merged commit dcf8f44 into trinodb:master Nov 2, 2021

github-actions bot added this to the 365 milestone Nov 2, 2021

kokosing mentioned this pull request Nov 2, 2021

Release notes for 365 #9826

Closed

12 tasks

aakashnand mentioned this pull request Jun 23, 2022

Allow group names from FileGroupProvider to be provided in QueryAccessControl #7730

Closed

cccs-tom deleted the feature/fix-9484 branch September 6, 2022 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add group-based and owner-based query access rules #9811

Add group-based and owner-based query access rules #9811

cccs-tom commented Oct 28, 2021

kokosing Oct 29, 2021

cccs-tom Oct 29, 2021

cccs-tom Oct 29, 2021

kokosing Oct 29, 2021

cccs-tom Oct 29, 2021

kokosing Oct 29, 2021

cccs-tom Oct 29, 2021

kokosing left a comment

kokosing Oct 29, 2021

cccs-tom Oct 29, 2021

cccs-tom Oct 29, 2021

kokosing Oct 29, 2021

kokosing Nov 2, 2021

cccs-tom Nov 2, 2021

kokosing commented Nov 2, 2021

sbernauer commented Nov 2, 2021

Add group-based and owner-based query access rules #9811

Add group-based and owner-based query access rules #9811

Conversation

cccs-tom commented Oct 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Nov 2, 2021

sbernauer commented Nov 2, 2021