Allow replacement string to substitute against original user in impersonation by chongfeng-hu · Pull Request #14962 · trinodb/trino

chongfeng-hu · 2022-11-09T02:09:45Z

Description

Allow replacement string to substitute against original user in impersonation

Non-technical explanation

Support replacement string in new_user to substitute against original_user in impersonation rules. E.g., the following rule allows a user in the form team_backend to impersonate the team_backend_sandbox user, but not arbitrary users:

{
    "impersonation": [
        {
            "original_user": "team_(.*)",
            "new_user": "team_$1_sandbox",
            "allow": true
        }
    ]
}

Relevant Trino doc on impersonation: https://trino.io/docs/current/security/file-system-access-control.html#impersonation-rules

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

cla-bot · 2022-11-09T02:09:47Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java

cla-bot · 2022-11-09T16:59:52Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

chongfeng-hu · 2022-11-10T01:03:33Z

All tests passed. Reviews appreciated.

kokosing

Looks good, few comments.

kokosing · 2022-11-10T12:52:06Z

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

StringBuilder is mutable object. Please replace it with something immutable here like Pattern

Also I think it should be a local variable in match method.

Java 17 Matcher lacks a method String substituteReplacement(String replacement) which, unlike
replaceAll() that replaces all matched groups in original string with substituted replacement, just simply returns the substituted replacement string, without replacing the original string.

Therefore, the only other viable option is to use appendReplacement(). However, this method either requires StringBuffer or StringBuilder, both mutable. In my earlier version, I had declared the StringBuilder object in match method, the only downside is that each method invocation will require a new StringBuilder to be constructed. It was lifted here for perf reasons. I will move it back to match.

kokosing · 2022-11-10T12:55:03Z

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

Provide first generic explanation of substitution and then use example with backend

Done.

I have added more detailed generic explanations on lines 547-549 above, as that section is more suited for generic explanations. I have left this line untouched because this section specifically focuses on explaining the examples included below (user-impersonation.json).

kokosing · 2022-11-13T20:10:13Z

core/trino-main/src/test/resources/catalog_impersonation.json

maybe let's replace $2 with $1 to make this example more interesting. WDYT?

"new_user": "internal-$2-$1-sandbox",

kokosing · 2022-11-13T20:17:30Z

core/trino-main/src/test/resources/catalog_impersonation.json

Please provide tests where number of groups is incorrect. For example:

{ "original_user": "svc_(.*)", "new_user": "$2_$1_prod", "allow": true },

and

{ "original_user": "svc_(.*)", "allow": true },

1st example: done.

2nd example: done. please note that new_user is required, even though it can be explicitly set to .*. This likely is to force the user to explicitly spell out match-all, if that's their intention, whereas allowing it optional and defaulting to match-all might result in unintentional global match, causing security risks. Therefore, I have slightly modified your example, by setting new_user to .* explicitly.

Also in order to avoid interfering with other tests, I've used new prefixes abc_ and def_ respectively.

kokosing · 2022-11-14T11:22:51Z

core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java

Can you please also assert the error message? I don't IndexOutOfBoundsException should be an error that we can throw to user. I think it should be TrinoException with an actionable error message for user.

done -- the error message is No group 2, which clearly points out the issue, and is also actionable.

kokosing · 2022-11-14T11:23:32Z

core/trino-main/src/test/resources/catalog_impersonation.json

replace abc with missing_replacement

kokosing · 2022-11-14T11:23:51Z

core/trino-main/src/test/resources/catalog_impersonation.json

replace def with incorrect_number_of_replacements_groups

kokosing · 2022-11-15T12:08:24Z

core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java

As an user it would take me a lot of time to understand where my configuration is incorrect. Please improve error message and use TrinoException

@kokosing will appreciate your review when you get a chance.

…sonation Support replacement string in new_user to substitute against original_user in impersonation rules. E.g., the following rule allows a user in the form team_backend to impersonate the team_backend_sandbox user, but not arbitrary users: { "impersonation": [ { "original_user": "team_(.*)", "new_user": "team_$1_sandbox", "allow": true } ] }

kokosing · 2022-11-18T13:48:12Z

Merged, thanks!

github-actions bot added the docs label Nov 9, 2022

ebyhr reviewed Nov 9, 2022

View reviewed changes

core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java Outdated Show resolved Hide resolved

chongfeng-hu force-pushed the impersonation_substitution branch from 152098d to bf3501b Compare November 9, 2022 16:59

chongfeng-hu force-pushed the impersonation_substitution branch from bf3501b to fe2c514 Compare November 9, 2022 18:56

cla-bot bot added the cla-signed label Nov 9, 2022

chongfeng-hu force-pushed the impersonation_substitution branch from fe2c514 to ec9fae4 Compare November 9, 2022 23:21

ebyhr requested review from dain and kokosing November 10, 2022 01:05

kokosing reviewed Nov 10, 2022

View reviewed changes

chongfeng-hu force-pushed the impersonation_substitution branch 2 times, most recently from 09e7180 to 987e013 Compare November 10, 2022 19:31

kokosing reviewed Nov 13, 2022

View reviewed changes

chongfeng-hu force-pushed the impersonation_substitution branch from 987e013 to ed4ae9c Compare November 13, 2022 21:02

kokosing reviewed Nov 14, 2022

View reviewed changes

chongfeng-hu force-pushed the impersonation_substitution branch from ed4ae9c to 133b260 Compare November 14, 2022 17:04

kokosing reviewed Nov 15, 2022

View reviewed changes

chongfeng-hu added 5 commits November 15, 2022 08:14

address comments

d8cca99

address more review comments

dbc6cd0

address even more comments

12e38f4

address yet more comments

870f2ff

chongfeng-hu force-pushed the impersonation_substitution branch from 133b260 to 870f2ff Compare November 15, 2022 17:41

kokosing approved these changes Nov 18, 2022

View reviewed changes

kokosing merged commit 9f090ba into trinodb:master Nov 18, 2022

chongfeng-hu deleted the impersonation_substitution branch November 18, 2022 16:45

hashhar added this to the 404 milestone Nov 18, 2022

colebow mentioned this pull request Nov 21, 2022

Add Trino 405 release notes #15139

Merged

Conversation

chongfeng-hu commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Non-technical explanation

Release notes

Uh oh!

cla-bot bot commented Nov 9, 2022

Uh oh!

Uh oh!

cla-bot bot commented Nov 9, 2022

Uh oh!

chongfeng-hu commented Nov 10, 2022

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chongfeng-hu Nov 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing commented Nov 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

chongfeng-hu commented Nov 9, 2022 •

edited

Loading

chongfeng-hu Nov 13, 2022 •

edited

Loading