Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Apache Ranger authorizer plugin #22675

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mneethiraj
Copy link

@mneethiraj mneethiraj commented Jul 15, 2024

Description

Added Apache Ranger authorizer plugin to authorize data access in Trino using Apache Ranger policies. Earlier version of this plugin is in Apache Ranger git repo. The plugin has been updated for the changes in SystemAccessControl interface in Trino master branch.

Additional context and related issues

  • RangerSystemAccessControlFactory implements SystemAccessControlFactory
  • RangerSystemAccessControl implements SystemAccessControl

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:

# Apache Ranger authorizer for Trino
* This plugin supports use of Apache Ranger policies to authorize data access in Trino - like operations on catalogs, schemas, tables, columns.
* Column-masking and row-filtering are supported in this plugin.
* Accesses authorized by the plugin are audited for compliance purposes.

## Requirements
* Access to an Apache Ranger instance having authorization policies to be enforced by this plugin
* Access to audit stores (Solr/Elasticsearch/S3/HDFS) to save access audit logs

## Configuration
Add following entries in /etc/trino/access-control.properties to configure Apache Ranger as the authorizer in Trino:

access-control.name=ranger
ranger.service.name=dev_trino
ranger.plugin.config.resource=/etc/trino/ranger-trino-security.xml,/etc/trino/ranger-trino-audit.xml,/etc/trino/ranger-trino-policymgr-ssl.xml

Apache Ranger plugin configurations for policy store and audit store should be updated in following configuration file:
/etc/trino/ranger-trino-security.xml
/etc/trino/ranger-trino-audit.xml
/etc/trino/ranger-trino-policymgr-ssl.xml```

Copy link

cla-bot bot commented Jul 15, 2024

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

4 similar comments
Copy link

cla-bot bot commented Jul 16, 2024

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

Copy link

cla-bot bot commented Jul 16, 2024

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

Copy link

cla-bot bot commented Jul 16, 2024

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

Copy link

cla-bot bot commented Jul 16, 2024

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

@ebyhr ebyhr mentioned this pull request Jul 17, 2024
Copy link

cla-bot bot commented Jul 17, 2024

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

@cla-bot cla-bot bot added the cla-signed label Jul 17, 2024
@mneethiraj
Copy link
Author

This is my first PR in Trino. Looking for help with the failures:

  • ci/check-commits-dispatcher: PR requires a rebase. Found: 4 merge commits. Should I create a new PR?
  • Test failures seem unrelated to this PR

@mosabua
Copy link
Member

mosabua commented Jul 17, 2024

@mneethiraj thanks for sending this PR. I am not sure how it compares to the linked PR from @dprophet but overall we would like to get a Ranger plugin merged.

In terms of other issues:

  • You need to submit a signed CLA.
  • You might need to rebase the PR. Currently there are no conflicts so rebase should be easy.
  • The CI failures seems to be unrelated so you can ignore.

@mneethiraj
Copy link
Author

@mosabua - thank you for the response.

I am not sure how it compares to the linked PR from @dprophet but overall we would like to get a Ranger plugin merged.

PR #13297 was created about 2 years back, built with Trino version 391-SNAPSHOT. Ranger plugin in this PR builds with the current version - 453-SNAPSHOT, updated for changes in authorization interface since then. Also, this PR builds with the most recent Apache Ranger version - 2.4.0.

  • You need to submit a signed CLA.
  • You might need to rebase the PR. Currently there are no conflicts so rebase should be easy.
  • The CI failures seems to be unrelated so you can ignore.
  • I got the confirmation email that my CLA was received today.
  • PR has been rebased. CI is in progress

@lozbrown
Copy link

It would be amazing to get this merged.

Haveing to patch docker images to add the plugin in really slows our ability to keep up with upgrades

@mosabua
Copy link
Member

mosabua commented Jul 18, 2024

Excellent @mneethiraj .. maybe @dprophet and his team can help with the review so we can get this over the finish line easier.

@mneethiraj
Copy link
Author

maybe @dprophet and his team can help with the review so we can get this over the finish line easier.

@dprophet - can you please review this PR and help get this merged?

@lozbrown
Copy link

I guess we need to enhance the helm chart to support the Ranger config.

Additionally that config should be documented in the trino website

@mosabua
Copy link
Member

mosabua commented Jul 23, 2024

Docs should be part of this PR.

Helm chart for running Ranger is a separate topic and potentially out of scope for the Trino chart. Relevant config for this plugin in the Helm chart might be possible with the existing chart or could be added after this is merge.

cc @nineinchnick

@mneethiraj
Copy link
Author

Docs should be part of this PR.

@mosabua - documentation on configuring Trino to use Ranger plugin is included in this PR, at plugin/trino-ranger/README.md. And sample configurations to use Ranger plugin in docker setup are included in directory plugin/trino-ranger/src/main/resources/conf-docker. Please review and let me know if any updates are needed.

@mosabua
Copy link
Member

mosabua commented Jul 23, 2024

The docs should really be user visible .. so you need to add a md file in https://github.com/trinodb/trino/tree/master/docs/src/main/sphinx/security and hook it in https://github.com/trinodb/trino/blob/master/docs/src/main/sphinx/security.md#access-control

Once you moved the content from the readme and updated it I can review

@lozbrown
Copy link

Docs should be part of this PR.

Helm chart for running Ranger is a separate topic and potentially out of scope for the Trino chart. Relevant config for this plugin in the Helm chart might be possible with the existing chart or could be added after this is merge.

cc @nineinchnick

Whilst an official ranger helm chart would be wonderful somewhere it's not really a trino problem.

I was specifically referring to enhancing the trino helm chart to support config for this plugin. Makeing the most used configurations configurable via the values files.

Some of the config for this plugin is xml which is a bit inconsistent / incongruent with all other trino config.

Copy link

@ognjen-it ognjen-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mneethiraj ,

I'm so happy to see that ranger-trino plugin will be up-to-date now.

I just tried to test this plugin with ranger 2.4.0, but I'm always getting the same error, access deny. Audit:

{
    "repoType": 203,
    "repo": "trino",
    "reqUser": "trino",
    "evtTime": "2024-07-29 15:11:48.142",
    "access": "SetUser",
    "resource": "trino",
    "resType": "trinouser",
    "action": "impersonate",
    "result": 0,
    "agent": "trino",
    "policy": 24,
    "enforcer": "ranger-acl",
    "agentHost": "trino-coordinator-662b97674b-9bxst",
    "logType": "RangerAudit",
    "id": "f6914203-d6ba-4103-a601-84ae4ae978fa-7",
    "seq_num": 15,
    "event_count": 1,
    "event_dur_ms": 0,
    "tags": [],
    "cluster_name": ""
}

How did you get it working? Could you please share with us how to test it?

plugin/trino-ranger/README.md Outdated Show resolved Hide resolved
@mneethiraj
Copy link
Author

Merged updates from latest upstream/master.

@mneethiraj mneethiraj reopened this Oct 17, 2024
```


To combine Ranger access control with file-based or other access control systems, create the file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mosabua This section could be reused between access control systems documentations. It is quite generic. Or maybe it should not be documented in this place, but maybe on some higher level. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed .. but we can do that in a follow up PR. I think we need a new page similar to authentication types but for multiple access control systems.

I will take this on.

pom.xml Outdated Show resolved Hide resolved
plugin/trino-apache-ranger/pom.xml Outdated Show resolved Hide resolved
@mosabua
Copy link
Member

mosabua commented Oct 22, 2024

Props to everyone working on this PR. What a huge and great effort. I know the community is looking forward to this shipping soon. It looks like its getting really close.

@mneethiraj
Copy link
Author

@kokosing - I updated TestApacheRanger with addition of query execution by alice (config alice@presto), to verify that the queries fail. The tests pass successfully in my local env using testing/bin/ptl test run --environment multinode-apache-ranger -- -g apache-ranger, but the test fails in CI; it seems CI env doesn't have mapping for alice@presto.

CI error suggests alice@trino as the valid name; but with this name, my local test run fails. Can you please suggest a resolution?

tests               | 2024-10-23 14:05:43 SEVERE: Failure cause:
tests               | com.google.inject.ConfigurationException: Guice configuration errors:
tests               | 
tests               | 1) [Guice/MissingImplementation]: No implementation for QueryExecutor annotated with @Named("alice@presto") was bound.
tests               | 
tests               | Did you mean?
tests               |     * QueryExecutor annotated with @Named("hive") bound at QueryExecutorModuleProvider$1$1.configure(QueryExecutorModuleProvider.java:73)
tests               |       \_ installed by: Modules$CombinedModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$CombinedModule -> Modules$CombinedModule -> Modules$CombinedModule -> QueryExecutorModuleProvider$1 -> QueryExecutorModuleProvider$1$1
tests               | 
tests               |     * QueryExecutor annotated with @Element(setName=,uniqueId=750, type=MAPBINDER, keyType=String) bound at QueryExecutorModuleProvider$1.bindDatabaseConnectionBeans(QueryExecutorModuleProvider.java:77)
tests               |       \_ installed by: Modules$CombinedModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$CombinedModule -> Modules$CombinedModule -> Modules$CombinedModule -> QueryExecutorModuleProvider$1
tests               | 
tests               |     * QueryExecutor annotated with @Named("alice@trino") bound at QueryExecutorModuleProvider$1$1.configure(QueryExecutorModuleProvider.java:73)
tests               |       \_ installed by: Modules$CombinedModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$OverrideModule -> Modules$CombinedModule -> Modules$CombinedModule -> Modules$CombinedModule -> QueryExecutorModuleProvider$1 -> QueryExecutorModuleProvider$1$1
tests               | 
tests               |     * 29 more bindings with other annotations.

@lozbrown lozbrown self-requested a review October 23, 2024 13:10
Copy link

@lozbrown lozbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have added comments to various docs above, mostly stuff i worked out when trying to update helm chart changes

@mneethiraj
Copy link
Author

Merged updates from latest upstream/master.

@mneethiraj mneethiraj reopened this Oct 24, 2024
@mneethiraj
Copy link
Author

CI error suggests alice@trino as the valid name; but with this name, my local test run fails. Can you please suggest a resolution?

@kokosing - this issue was resolved after merging the latest from upstream/master branch.

<property>
<name>xasecure.policymgr.clientssl.truststore.credential.file</name>
<value></value>
<description>Path to credential file for the truststore; the credential should be in alias sslTrustStore</description>

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lozbrown - supporting this Trino way of handling secrets will require enhancements in Apache Ranger library. I suggest tracking this in Apache Ranger community, and update the plugin once a Apache Ranger release includes the enhancements. This shouldn't be blocker for this PR.

Copy link

@lozbrown lozbrown Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mneethiraj I'm very eager to see merged soon too...I watched the contributor call this morning hoping this would have come up but alas no. I agree that at this point I'd rather see it merged as is.

Getting that secret file into the pod without exposing the secret in source control, will be a pain in the proverbial but we'll work it out.

Is there any way not to need that if we're only using 1 way SSL and the ca certs in trust store are not secrets?

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I have not looked at the code, I am good with the docs to be merged as they stand. I will follow up with a separate PR to clean them all up. I believe we should get this in as soon as the code is ready.

```


To combine Ranger access control with file-based or other access control systems, create the file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed .. but we can do that in a follow up PR. I think we need a new page similar to authentication types but for multiple access control systems.

I will take this on.

Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see plenty of comments are not yet addressed. @mneethiraj please ping me when it is ready to another round of review

pom.xml Outdated Show resolved Hide resolved
plugin/trino-apache-ranger/pom.xml Outdated Show resolved Hide resolved
@mneethiraj
Copy link
Author

I see plenty of comments are not yet addressed. @mneethiraj please ping me when it is ready to another round of review

@kokosing - except for removing the implementation of grant/revoke/deny methods, all other comments are addressed.

Co-authored-by: lozbrown <lozbrown@users.noreply.github.com>
Co-authored-by: Grzegorz Kokosiński <grzegorz@starburstdata.com>
@mneethiraj
Copy link
Author

Merged updates from upstream/master.

@mneethiraj mneethiraj reopened this Oct 31, 2024
@mneethiraj
Copy link
Author

except for removing the implementation of grant/revoke/deny methods, all other comments are addressed.

grant/revoke/deny methods updates are done as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bigquery BigQuery connector cla-signed delta-lake Delta Lake connector docs hive Hive connector hudi Hudi connector iceberg Iceberg connector jdbc Relates to Trino JDBC driver mongodb MongoDB connector release-notes stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. ui Web UI
Development

Successfully merging this pull request may close these issues.