Add LDAP groups provider plugin #20157

mosiac1 · 2023-12-18T12:37:09Z

Description

Adds an implementation of the Groups Provider plugin type that loads user groups from LDAP.
Supports filtering of fetched groups at the LDAP query level.
Supports fetching of extra users attributes, which are mapped to groups with the pattern {attribute}={value}.

The plugin uses a 2-pass approach:

Find the user object in LDAP, get CN and extra attributes;
Search for groups that the user is member of.

Result are cached with a configurable TTL. The cache has extra logic to save loaded groups only when both steps complete successfully, otherwise returning either empty groups of results from step 1.

Additional context and related issues

Sample configuration:

group-provider.name=ldap
ldap.url=ldaps://my-ldap-server:636
ldap.admin-user=CN=ro-query,OU=users,DC=trino,DC=io
ldap.admin-password=ro-password
ldap.user-base-dn=OU=users,DC=trino,DC=io
ldap.user-search-filter=(&(objectClass=person)(|(name={0})(mail={0}@trino.io)))
ldap.user-search-attributes=uuid
ldap.groups-base-dn=OU=groups,DC=trino,DC=io
ldap.groups-search-filter=(|(cn=dev*)(cn=admin*))
ldap.groups-search-member-attribute=member
ldap.groups-name-attribute=cn
ldap.cache-ttl=10m

The PR also updates the Trino LDAP toolkit to support pass-through search filter arguments to the LDAP client. This lets the LDAP server handle argument substitutions.

Release notes

( ) This is not user-visible or is 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:

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ldap/LdapQuery.java

plugin/trino-ldap-groups-provider/pom.xml

.../trino-ldap-groups-provider/src/main/java/io/trino/plugin/ldapgroups/LdapGroupsProvider.java

...-ldap-groups-provider/src/main/java/io/trino/plugin/ldapgroups/LdapGroupsProviderConfig.java

...ups-provider/src/test/java/io/trino/plugin/ldapgroups/TestLdapGroupsProviderIntegration.java

...trino-ldap-groups-provider/src/main/java/io/trino/plugin/ldapgroups/LdapHighLevelClient.java

mosiac1 · 2024-01-09T15:04:24Z

@Praveen2112 Hello! Pinging you in case you missed the updates. Addressed the review comments, please have a look when you can

github-actions · 2024-01-30T17:05:53Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

Praveen2112 · 2024-01-30T17:32:52Z

@mosiac1 Thank you for addressing the comments !! Will take a look at this PR

mosiac1 · 2024-02-07T11:26:07Z

Hello @Praveen2112! I rebased on top of main, split the changes into 3 commits and added tests for the CachingGroupProvider classes.

Would really appreciate if you can take a look!

Praveen2112

Instead of mock library - can we use the TestingOpenLdapServer which could provide a accurate behavior.

lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/cache/TestCacheConfig.java

...rino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/group/CachingGroupProvider.java

...rino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/group/LoadingGroupProvider.java

...-plugin-toolkit/src/test/java/io/trino/plugin/base/cache/group/TestCachingGroupProvider.java

...n/trino-ldap-group-provider/src/main/java/io/trino/plugin/ldapgroup/LdapHighLevelClient.java

...trino-ldap-group-provider/src/test/java/io/trino/plugin/ldapgroup/TestLdapGroupProvider.java

...group-provider/src/test/java/io/trino/plugin/ldapgroup/TestLdapGroupProviderIntegration.java

ksobolew · 2024-02-07T12:41:49Z

Hi @mosiac1,

Could you take a look at #18174 and see if it is useful for you in "outsourcing" the caching to a generic layer? I can see that it's quite similar to what you have done.

mosiac1 · 2024-02-07T12:53:04Z

Hey @ksobolew,

#18174 looks like a superset of what I added with more configuration, so I would be very happy to move to that.

Please ping me once that gets merged and I'll change this PR to use it. I'll keep mine around until then in case you want to take a look at it.

bazooka720 · 2024-02-07T17:14:14Z

This would be great to have. Any ETA of this?

ksobolew · 2024-02-08T08:09:00Z

@mosiac1 The trouble with my PR was that there were no potential users of this in Trino itself, and the ones that could use it never got close to merging. Now that we have one we can put some pressure on the maintainers :) cc: @kokosing

mosiac1 · 2024-02-08T11:13:27Z

If it helps with reviewing or selling the idea we can cherry-pick your commit into this PR so its all in one place.

kokosing · 2024-02-08T13:22:20Z

Please cherry-pick. This is good idea.

mosiac1 · 2024-02-09T18:10:06Z

Addressed all the review comments and moved some of the components around. Two big changes:

This is now using @ksobolew 's CachingGroupProviderModule;
The plugin now support two methods for fetching groups from LDAP:
- A 1-pass approach, fetching the memberOf attribute which in some AD systems has multiple values representing the groups the user is a member of. The group's name is retrieved from its full DN. Note that this does not support filtering of groups or much customization for what represents the group's name.
- The 2-pass approach initially described in the PR description, get the user CN and attributes and search groups with member filter.

The choice between the two mechanisms is done by setting the ldap.user-member-of-attribute configuration property.

Requested by @Praveen2112 I moved the TestingOpenLdapContainer into trino-testing-containers so it could be used by both the password-authenticator and group-provider tests.

mosiac1 · 2024-02-13T13:44:20Z

@Praveen2112 Please take a look when you have time. The failing test is part of the Hive suite, I assume its being flaky.

github-actions · 2024-03-05T17:10:47Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

Praveen2112 · 2024-03-05T17:21:53Z

I'll taking a look this week. Sorry for the delay

Praveen2112 · 2024-03-15T16:33:13Z

One minor nit : I have is can we add the caching logic as a final commit, once the base LDAP implementation has been added ?

Praveen2112 · 2024-03-28T14:13:12Z

I would like to have a discussion on

Supports fetching of extra users attributes, which are mapped to groups with the pattern {attribute}={value}.

Ideally Group is an LDAP object while these users attributes are a part of the LDAP object. This group can be used for any access controls does it makes sense to rely on a non LDAP object here ? WDYT ?

mosiac1 · 2024-04-02T09:30:50Z

Let me know if you want to catch up offline about this, I'm on the Trino slack.

Keep in mind this is configurable. By default no user attributes are mapped to groups. The plugin needs to be configured with what attributes to map. See the tests for examples.

I don't see what the issue is. To Trino (and to most systems for that matter) grops are just strings; groups gain meaning when access control policies are bound to them. Downstream from the group provider it is irelevant where the groups came from.

We already use this at Bloomberg as part of our access control systems so we can create policies more nuanced than "is user x part of group y"

github-actions · 2024-04-23T17:02:46Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

Praveen2112

I'm a bit confused on different mechanism being used for user attributes for ldap.user-search-attributes we use it key and value while for others it is just value - Can it be symmetric ?

plugin/trino-ldap-group-provider/pom.xml

Praveen2112 · 2024-04-24T16:37:54Z

plugin/trino-ldap-group-provider/src/main/java/io/trino/plugin/ldapgroup/LdapGroupProvider.java

+{
+    private static final Logger log = Logger.get(LdapGroupProvider.class);
+
+    private final String userMemberOfAttribute;


Instead of referring userMemberOfAttribute, can we use LdapGroupProviderConfig#getLdapUserSearchAttributes` ?

plugin/trino-ldap-group-provider/src/main/java/io/trino/plugin/ldapgroup/LdapGroupProvider.java

Praveen2112 · 2024-04-24T16:54:50Z

...ap-group-provider/src/main/java/io/trino/plugin/ldapgroup/LdapGroupProviderSimpleClient.java

+            log.warn("Group with DN [%s] has no Relative Domain Names. Ignoring group.", groupDN);
+            return Optional.empty();
+        }
+        Rdn lastRDN = groupRDNs.getLast();


For my understanding - Why specifically the last entry ?

an LDAP Distinguished Name (DN) is a chain of multiple Relative Distinguished Names (RDNs). for example in cn=users,ou=groups,dc=trino,dc=io - cn=users and ou=groups are RDNs.

these are meant to be read from right to left (you can notice they get more specific). getRdns() will return them in this order, so the last element is the least specific RDN, usually what we would call the "name". in the example above it would be cn=users.

parsing group names from DNs is an heuristic based on the assumption that the last RDN is the Canonical Name we are looking for.

Praveen2112 · 2024-04-24T16:56:06Z

...p-provider/src/test/java/io/trino/plugin/ldapgroup/TestLdapGroupProviderFilteringClient.java

+
+    private LdapGroupProviderConfig config;
+    private LdapGroupProviderFilteringClientConfig filteringConfig;
+    private TestingLdapClient mockClient;


nit: ldapClient

Praveen2112 · 2024-04-24T16:57:30Z

...no-ldap-group-provider/src/test/java/io/trino/plugin/ldapgroup/TestingNamingEnumeration.java

+public final class TestingNamingEnumeration<T>
+        implements NamingEnumeration<T>
+{
+    private final Iterator<T> iter;


nit: Can we avoid using abbreviation ?

Praveen2112 · 2024-04-24T16:59:40Z

...group-provider/src/test/java/io/trino/plugin/ldapgroup/TestLdapGroupProviderIntegration.java

+    public TestLdapGroupProviderIntegration()
+            throws Exception
+    {
+        closer = Closer.create();


Can we move them to a setup method and use @BeforeAll instead of constructor

Praveen2112 · 2024-04-24T17:02:10Z

...group-provider/src/test/java/io/trino/plugin/ldapgroup/TestLdapGroupProviderIntegration.java

+    @ParameterizedTest
+    @MethodSource("provideUsersAndExpectedGroups")
+    public void testGetGroups(ConfigBuilder configBuilder, String userName, Set<String> expectedGroups)


It is not generally recommended to use ParameterizedTest - In general, they make it harder to debug tests - We could try to create some sort of an assertYYY - For reference we could try something like TestDecimalCoercers

will look into this a bit, but I personally don't see an overall ban on ParametrizedTests as a good thing. they may be overused but they do have a place

mosiac1 · 2024-04-24T17:33:03Z

@Praveen2112 in reply to your review comment and #20157 (comment)_

No, it cannot be symmetric. We prefix attributes from ldap.user-search-attributes with the key so we can make sense of the values - what meaning do the values in Map have without their keys?
We treat the ldap.user-member-attribute attribute differently because this plugin aims first to load LDAP groups, secondly to load other useful attributes. You can think of the {{ ldap.user-member-attribute }}=... prefix am implicit

We covered this topic at length in our slack chat, please refer to that

Praveen2112 · 2024-04-25T09:04:54Z

@dain / @electrum What is your opinion on having an additional config property ldap.user-search-attributes and adding a implicit prefix for certain groups.

github-actions · 2024-05-16T17:05:04Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

danisimo237 · 2024-05-20T15:25:28Z

Этот запрос на включение некоторое время не выполнялся. Отмечаем команду по связям с разработчиками Trino:@bitsondatadev @colebow @mosabua

Will this pull request be added soon? I think there are a lot of people looking forward to it.

mosiac1 · 2024-05-20T15:29:57Z

I would like to get a consensus on ldap.user-search-attributes (or more generally on how do we want to handle fetching arbitrary user attributes from LDAP) before I go ahead and work on resolving the comments above. Once we agree on that the rest can be sorted out pretty quickly (famous last words).

cc: @Praveen2112 @dain

Praveen2112 · 2024-05-20T16:45:38Z

I would like to get @dain's opinion here - on handling arbitrary user attributes.

dain · 2024-05-21T02:38:56Z

I don't think we should the feature to map ldap attributes to groups. Instead we should work on adding data from authentication to the session, and functions to access this data (so it can be used in ABAC expressions). I'm not sure how we want to do this, but I'd start with a survey of how others have done this. If there is a consensus from others, the design is simple. This would be a separate project from this PR.

mosiac1 · 2024-05-21T12:30:19Z

Thanks for the input Dain.

I agree we should have a better system for fetching and passing around user attributes (attributes = key-value string pairs), but I don't think it should be bound to the login process. It could it be its own separate plugin type (UserAttributeProvider?) that maps an Identity to a list of attributes. It could also be a new function in the GroupProvider interface since groups are really just a type of attribute.

For example, in my use-case, Trino is setup with both oauth and LDAP user/password authentication, but the user attributes I care about are only in LDAP.

I'm not seeing much support for the attribute to group mapping in this PR; that's ok, I will have to remove it. It may take a bit since this isn't a very high priority item for me currently.

danisimo237 · 2024-05-21T12:43:28Z

Thanks for the answer. Of course, we need to choose a more suitable solution. The main thing is that you can manage access rights using ldap groups right away without additional plugins. We tried the ldap group provider implementation, which is freely available. This is a good solution that is convenient to use.

This Guice Module can be used to enable caching in the group provider, by adding it to the list of modules in a Guice context in a group provider factory, or to any other Guice context as needed. Features: * Configurable configuration prefix * Ability to bind the final `GroupProvider` with a custom binding annotation * useful especially when the Guice context is not entirely isolated and there are other `GroupProvider` bindings in it * An `@Inject`-able hook for cache invalidation Author: Krzysztof Sobolewski <[email protected]> Date: Tue Jul 11 16:48:28 2023 +0200

ilsaloving · 2024-09-27T16:27:28Z

Is there any progress on this? I've been looking for a working group ldap plugin, and seeing years of endless arguing over non-core enhancements is somewhat frustrating.

I'm willing to help if I can, just to get this moving along.

mosiac1 · 2024-09-27T16:45:22Z

We had chat about this PR during the last Trino contributor call https://github.com/trinodb/trino/wiki/Contributor-meetings#trino-contributor-call-24-oct-2024.

I don't have a lot of time to work on this unfortunately. We are looking for someone to take the PR to competition, its fairly close.

The discussion on the last few comments between me and Dain was about handling custom user attributes in LDAP. The decision was for that functionality to be removed. In my last push I stripped all of that out. The code for fetching groups from LDAP is in place.

The next steps are:

Rebase
Squash the last commit 154f761 into d6f6539
Go over all unresolved review comments and either address or resolve
- A significant point here is removing the use of @ParameterizedTests
Check and fix any relevant CI failures
Get a review from Dain and/or David
Write docs page

@OmerRaifler (I hope that is the right github user, slack user is https://trinodb.slack.com/team/U06BF92CABC) was interested in working on this, but I haven't heard anything from them

@ilsaloving Please feel free to jump in

mosiac1 · 2024-10-24T16:45:47Z

This PR has now moved to #23900 as @OmerRaifler is taking ownership. We hope to get this shipped soon!

cla-bot bot added the cla-signed label Dec 18, 2023

wendigo requested a review from Praveen2112 December 18, 2023 12:59

Praveen2112 self-assigned this Dec 18, 2023

Praveen2112 reviewed Jan 3, 2024

View reviewed changes

github-actions bot added the stale label Jan 30, 2024

github-actions bot removed the stale label Jan 31, 2024

mosiac1 force-pushed the trinodb/ldap-groups branch from 82cf890 to 4214d89 Compare February 7, 2024 11:24

Praveen2112 reviewed Feb 7, 2024

View reviewed changes

mosiac1 force-pushed the trinodb/ldap-groups branch 2 times, most recently from abd6fcd to 7f0a86e Compare February 9, 2024 18:09

mosiac1 force-pushed the trinodb/ldap-groups branch from 7f0a86e to b518742 Compare February 12, 2024 10:31

This was referenced Mar 1, 2024

Add CachingGroupProviderModule to trino-plugin-toolkit #18174

Closed

Ldap Group Provider #17518

Closed

github-actions bot added the stale label Mar 5, 2024

github-actions bot removed the stale label Mar 6, 2024

github-actions bot added the stale label Apr 23, 2024

Praveen2112 reviewed Apr 24, 2024

View reviewed changes

github-actions bot removed the stale label Apr 24, 2024

github-actions bot added the stale label May 16, 2024

github-actions bot removed the stale label May 20, 2024

mosabua added the stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. label May 29, 2024

mosiac1 and others added 4 commits September 20, 2024 16:29

Move TestingOpenLdapContainer to trino-testing-containers

95f2b4d

Add LDAP filter argument support to JdkLdapClient

c5cb5aa

Add LDAP group provider plugin

d6f6539

mosiac1 force-pushed the trinodb/ldap-groups branch from ea20a24 to dc998eb Compare September 20, 2024 16:27

Strip out user attribute logic

154f761

mosiac1 force-pushed the trinodb/ldap-groups branch from dc998eb to 154f761 Compare September 20, 2024 16:30

OmerRaifler mentioned this pull request Oct 24, 2024

Add LDAP Group Provider Plugin #23900

Open

mosiac1 closed this Oct 24, 2024

Add LDAP groups provider plugin #20157

Add LDAP groups provider plugin #20157

Conversation

mosiac1 commented Dec 18, 2023

Description

Additional context and related issues

Release notes

mosiac1 commented Jan 9, 2024

github-actions bot commented Jan 30, 2024

Praveen2112 commented Jan 30, 2024

mosiac1 commented Feb 7, 2024

Praveen2112 left a comment

Choose a reason for hiding this comment

ksobolew commented Feb 7, 2024

mosiac1 commented Feb 7, 2024

bazooka720 commented Feb 7, 2024

ksobolew commented Feb 8, 2024

mosiac1 commented Feb 8, 2024

kokosing commented Feb 8, 2024

mosiac1 commented Feb 9, 2024

mosiac1 commented Feb 13, 2024

github-actions bot commented Mar 5, 2024

Praveen2112 commented Mar 5, 2024

Praveen2112 commented Mar 15, 2024

Praveen2112 commented Mar 28, 2024

mosiac1 commented Apr 2, 2024

github-actions bot commented Apr 23, 2024

Praveen2112 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosiac1 commented Apr 24, 2024

Praveen2112 commented Apr 25, 2024

github-actions bot commented May 16, 2024

danisimo237 commented May 20, 2024

mosiac1 commented May 20, 2024

Praveen2112 commented May 20, 2024

dain commented May 21, 2024

mosiac1 commented May 21, 2024

danisimo237 commented May 21, 2024

ilsaloving commented Sep 27, 2024

mosiac1 commented Sep 27, 2024 • edited Loading

mosiac1 commented Oct 24, 2024

mosiac1 commented Sep 27, 2024 •

edited

Loading