LDAP Group Provider Support #8335

coryjmaklin · 2021-06-21T15:13:12Z

This PR adds the code necessary to reference LDAP groups in Trino ACLs.

Context

We're using file based ACLs to authorize users connected to Trino.
https://trino.io/docs/current/security/file-system-access-control.html

In the documentation, it's mentioned "For group-based rules to match, users need to be assigned to groups by a Group provider."

In other words, we have to create a group provider to specify the following:

{
"group": "trino_admins",
"privileges": ["SELECT", "INSERT", "DELETE", "OWNERSHIP"]
}

There exists a trino-password-authenticators plugin that takes care of authenticating users using their LDAP credentials.
https://github.com/trinodb/trino/tree/master/plugin/trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap

In the same plugin, there's already have a file based Group Provider.
https://github.com/trinodb/trino/blob/master/plugin/trino-password-authenticators/src/main/java/io/trino/plugin/password/PasswordAuthenticatorPlugin.java

There are open issues asking to add this as a feature.
#6824
#2919

Implementation

We move the common functions into a utility class.
We rename the LdapAuthenticatorClient to LdapClient because Authenticator is a type of Trino plugin, and we're re-purposing the class for the Group Provider plugin.
We know that a LDAP group will have a common name (i.e. cn) because it's required by the definition
When retrieving the "memberof" attribute, the 'O' must be lowercase. When specifying that we want it in the search results, we the 'O' must be uppercase.

Testing

Configure the memberOf overlay in your LDAP instance
Create a group-provider.properties with the following properties:

group-provider.name=ldap
ldap.allow-insecure=
ldap.url=
ldap.user-base-dn=
ldap.user-bind-pattern=
ldap.bind-dn=
ldap.bind-password=

Connect to Trino and verify the ACLs are applied accordingly.

cla-bot · 2021-06-21T15:13:14Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: U-SLT\cjmakli.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cccs-tom

I flagged a couple of minor things, but overall, lgtm!

cccs-tom · 2021-06-21T19:07:46Z

...o-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapGroupProvider.java

+    {
+        this.client = requireNonNull(client, "client is null");
+
+        this.userBindSearchPatterns = ldapConfig.getUserBindSearchPatterns();


Should we perform a requireNonNull() check on ldapConfig before reaching this point? Or are we just letting the NPE happen in that case?

I believe it's unnecessary because unlike the client, we're not keeping a reference, and we check the values afterwards.

cccs-tom · 2021-06-21T19:13:42Z

...trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/JdkLdapClient.java

+                    }
+                }
+            }
+            search.close();


Nitpick: This probably doesn't need to be closed explicitly, since it's managed by the try-with-resources.

cccs-mjs · 2021-06-21T20:29:36Z

Typo in PR description: necessarily -> necessary

cccs-mjs

Seems good. In the PR description it'd be useful to provide a sentence or two about the proposed code changes in general terms (e.g. new classes, name changes, moving shared methods out LdapUtil etc)

cccs-mjs · 2021-06-21T20:49:15Z

...trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/JdkLdapClient.java

+            ImmutableSet.Builder<String> groupNames = ImmutableSet.builder();
+            if (search.hasMore()) {
+                Attributes attributes = search.next().getAttributes();
+                Attribute memberOfAttribute = attributes.get("memberof");


Is memberof case sensitive? Elsewhere, on lines 124 and 151 you use memberOf.

Also, could make this string a constant.

For whatever reason, memberof has to have a lowercase 'O' on line 122, and uppercase on line 151. If you don't it returns null.

cccs-mjs · 2021-06-21T21:04:15Z

...trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/JdkLdapClient.java

@@ -49,16 +54,16 @@
 import static javax.naming.Context.SECURITY_CREDENTIALS;
 import static javax.naming.Context.SECURITY_PRINCIPAL;

-public class JdkLdapAuthenticatorClient
-        implements LdapAuthenticatorClient
+public class JdkLdapClient


What's the reason for renaming the class? Could add to PR description.

cccs-mjs · 2021-06-21T21:06:09Z

...in/trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapClient.java

@@ -17,7 +17,7 @@

 import java.util.Set;

-public interface LdapAuthenticatorClient
+public interface LdapClient


What's the reason for renaming the interface? Could add to PR description.

I added an explanation in the PR description. Essentially, Authenticator is a type of Trino plugin, but we're re-purposing the client for Group Provider plugin as well.

cla-bot · 2021-06-22T17:12:39Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: U-SLT\cjmakli.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2021-06-22T18:16:42Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: U-SLT\cjmakli.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot · 2021-06-22T18:23:31Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: U-SLT\cjmakli.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

coryjmaklin · 2021-06-22T18:33:57Z

Typo in PR description: necessarily -> necessary

Updated

rimolive · 2021-07-14T15:24:07Z

Any chance to help in this PR to have it merged asap?

electrum · 2021-08-18T00:56:07Z

...o-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapGroupProvider.java

+                throw new RuntimeException("Authentication error");
+            }
+        }
+        return null;


This should return an empty set

Suggested change

return null;

return ImmutableSet.of();

MiguelWeezardo · 2021-08-27T09:16:01Z

...o-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapAuthenticator.java

@@ -110,17 +107,17 @@ public Principal createAuthenticatedPrincipal(String user, String password)
    private Principal authenticateWithUserBind(Credential credential)
    {
        String user = credential.getUser();
-        if (containsSpecialCharacters(user)) {
+        if (LdapUtil.containsSpecialCharacters(user)) {


Please use static imports from LdapUtil.

MiguelWeezardo · 2021-08-27T09:35:19Z

...trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/JdkLdapClient.java

@@ -106,11 +111,44 @@ public boolean isGroupMember(String searchBase, String groupSearch, String conte
        }
    }

+    @Override
+    public Set<String> lookupUserGroups(String searchBase, String searchFilter, String contextUserDistinguishedName, String contextPassword)


nit: Would it be possible to avoid two LDAP queries while logging in and getting user groups in a separate call? Logging in is simply checking if a context can be created, and a similar context is later used for getting groups.

MiguelWeezardo · 2021-08-27T10:00:44Z

...ord-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapGroupProviderFactory.java

+                binder -> {
+                    configBinder(binder).bindConfig(LdapConfig.class);
+                    binder.bind(LdapGroupProvider.class).in(Scopes.SINGLETON);
+                    binder.bind(LdapClient.class).to(JdkLdapClient.class).in(Scopes.SINGLETON);


Won't this fail at runtime if LdapClient interface was already bound in LdapAuthenticatorFactory? Maybe we need some @ForAuthentication and @ForGroupProvider annotations?

It is in separate guice context, so I think it is not an issue.

MiguelWeezardo · 2021-08-27T10:34:22Z

...rino-password-authenticators/src/test/java/io/trino/plugin/password/ldap/TestLdapClient.java

+import java.util.Optional;
+import java.util.Set;
+
+public class TestLdapClient


This mock LDAP implementation is great for unit testing, but it would be nice to have tests with an actual LDAP server. I don't think you would have detected the "memberof"/"memberOf" issue using only these tests.

Notice that we have SinglenodeLdap or SinglenodeLdapBindDn environments already created.

Basically to create product tests, you need to add a test io.trino.tests.product.jdbc.TestLdapTrinoJdbc and add to the product test environment configuration for LDAP group provider.

MiguelWeezardo · 2021-08-27T11:32:07Z

...ord-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapGroupProviderFactory.java

+    @Override
+    public String getName()
+    {
+        return "ldap";


Starburst already has a group provider called "ldap", and this will cause conflicts while parsing configuration, since their config properties are different. Maybe use "ldap-group-provider", "ldap-query" or "ldap-trino"?

kokosing · 2021-08-31T11:56:40Z

...sword-authenticators/src/main/java/io/trino/plugin/password/PasswordAuthenticatorPlugin.java

@@ -17,6 +17,7 @@
 import io.trino.plugin.password.file.FileAuthenticatorFactory;


Please improve commit message:

Introduce LDAP group provider

kokosing · 2021-08-31T11:57:39Z

...trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/JdkLdapClient.java

@@ -49,16 +54,16 @@
 import static javax.naming.Context.SECURITY_CREDENTIALS;
 import static javax.naming.Context.SECURITY_PRINCIPAL;

-public class JdkLdapAuthenticatorClient
-        implements LdapAuthenticatorClient
+public class JdkLdapClient


Please follow: https://github.com/trinodb/trino/blob/master/DEVELOPMENT.md#git-merge-strategy

Mechanical changes (like refactoring and renaming) should be separated from logical and functional changes.

Please extract class rename into a separate commit.

kokosing · 2021-08-31T11:59:54Z

...trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/JdkLdapClient.java

+                Attributes attributes = search.next().getAttributes();
+                Attribute memberOfAttribute = attributes.get("memberof");
+                if (memberOfAttribute == null) {
+                    log.error("No memberOf attribute found... The ldap group provider requires the memberOf overlay to be enabled.");


I think we should fail instead of silenlty ignore this problem. It will some time to users to understand why ldap group provider does not return groups. Throwing an exception here could make the realize the problem sooner.

kokosing · 2021-08-31T12:00:48Z

...o-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapAuthenticator.java

@@ -144,7 +141,7 @@ private Principal authenticateWithUserBind(Credential credential)
    private Principal authenticateWithBindDistinguishedName(Credential credential)
    {
        String user = credential.getUser();
-        if (containsSpecialCharacters(user)) {
+        if (LdapUtil.containsSpecialCharacters(user)) {


extraction of LdapUtil can go as separate commit too.

kokosing · 2021-08-31T12:01:15Z

...o-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapGroupProvider.java

+    @Override
+    public Set<String> getGroups(String user)
+    {
+        if (LdapUtil.containsSpecialCharacters(user)) {


static import

kokosing · 2021-08-31T12:03:50Z

...o-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapGroupProvider.java

+            String userDistinguishedName = LdapUtil.replaceUser(userBindSearchPattern, user);
+            String searchBase = userBaseDistinguishedName.orElseThrow();
+            try {
+                return client.lookupUserGroups(searchBase, userDistinguishedName, bindDistinguishedName.orElseThrow(), bindPassword.orElseThrow());


We are breaking a loop in first iteration and so we are ignoring rest of userBindSearchPatterns. Is this intentional? If so, why? Can you please add a comemnt?

Reminder to keep #8134 in mind. All patterns must be tried and only the last AccessDeniedException must be bubbled up.

kokosing · 2021-08-31T12:04:44Z

...ord-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapGroupProviderFactory.java

+                binder -> {
+                    configBinder(binder).bindConfig(LdapConfig.class);
+                    binder.bind(LdapGroupProvider.class).in(Scopes.SINGLETON);
+                    binder.bind(LdapClient.class).to(JdkLdapClient.class).in(Scopes.SINGLETON);


It is in separate guice context, so I think it is not an issue.

kokosing · 2021-08-31T12:04:51Z

plugin/trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapUtil.java

+
+import com.google.common.base.CharMatcher;
+
+public class LdapUtil


kokosing · 2021-08-31T12:05:11Z

plugin/trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapUtil.java

+    private static final CharMatcher SPECIAL_CHARACTERS = CharMatcher.anyOf(",=+<>#;*()\"\\\u0000");
+    private static final CharMatcher WHITESPACE = CharMatcher.anyOf(" \r");
+
+    private LdapUtil()


private LdapUtil() {}

kokosing · 2021-08-31T12:07:48Z

...rino-password-authenticators/src/test/java/io/trino/plugin/password/ldap/TestLdapClient.java

+import java.util.Optional;
+import java.util.Set;
+
+public class TestLdapClient


Notice that we have SinglenodeLdap or SinglenodeLdapBindDn environments already created.

Basically to create product tests, you need to add a test io.trino.tests.product.jdbc.TestLdapTrinoJdbc and add to the product test environment configuration for LDAP group provider.

MiguelWeezardo · 2021-10-08T23:18:14Z

@coryjmaklin how can we help to get this merged?

kokosing · 2021-10-12T08:21:18Z

@coryjmaklin Would you mind if we would take over this pull request?

coryjmaklin · 2021-10-12T12:51:05Z

@coryjmaklin Would you mind if we would take over this pull request?

Sorry. I don't have time to work on it at present. If you want to take it over, by all means.

rimolive · 2021-10-25T14:03:01Z

@MiguelWeezardo @kokosing Let me know how can I help in this. I'm very interested to see this merged.

kokosing · 2021-10-26T19:38:24Z

@rimolive Thank you ❤️ Please open a new pull request based on this one with all the comments addressed (applied or responded). Please make sure to mark @coryjmaklin as a co-author of the commit that you are goin to create. Together with @MiguelWeezardo we will be happy to do a review and merge the code eventually.

rimolive · 2021-10-29T20:23:47Z

Anyone knows why am I getting this error in trino-main tests?

[ERROR] Tests run: 7872, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 969.778 s <<< FAILURE! - in TestSuite [ERROR] io.trino.util.TestTimeZoneUtils.testNamedZones Time elapsed: 0.037 s <<< FAILURE! java.lang.IllegalArgumentException: The datetime zone id 'Pacific/Kanton' is not recognised at org.joda.time.DateTimeZone.forID(DateTimeZone.java:247) at io.trino.util.TestTimeZoneUtils.testNamedZones(TestTimeZoneUtils.java:51) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104) at org.testng.internal.Invoker.invokeMethod(Invoker.java:645) at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851) at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177) at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129) at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:829)

I'm using OpenJDK 11.0.13 to run maven build

MiguelWeezardo · 2021-11-08T09:31:38Z

Anyone knows why am I getting this error in trino-main tests?

[ERROR] Tests run: 7872, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 969.778 s <<< FAILURE! - in TestSuite [ERROR] io.trino.util.TestTimeZoneUtils.testNamedZones Time elapsed: 0.037 s <<< FAILURE! java.lang.IllegalArgumentException: The datetime zone id 'Pacific/Kanton' is not recognised at org.joda.time.DateTimeZone.forID(DateTimeZone.java:247) at io.trino.util.TestTimeZoneUtils.testNamedZones(TestTimeZoneUtils.java:51) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104) at org.testng.internal.Invoker.invokeMethod(Invoker.java:645) at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:851) at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1177) at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:129) at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:112) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:829)

I'm using OpenJDK 11.0.13 to run maven build

Looks like your JDK knows of a zone which isn't yet supported by joda-time version Trino uses.
I see a couple of possible solutions:

upgrade joda-time
downgrade to OpenJDK 11.0.11
add an exception to TestTimeZoneUtils.testNamedZones

rimolive · 2021-11-30T12:13:52Z

@kokosing @MiguelWeezardo Please check updated PR: #10116

kokosing · 2021-11-30T13:59:23Z

Closing in favor #10116. Thank you, @rimolive !

coryjmaklin requested review from cccs-mjs and cccs-tom June 21, 2021 15:13

cccs-tom reviewed Jun 21, 2021

View reviewed changes

cccs-mjs reviewed Jun 21, 2021

View reviewed changes

coryjmaklin force-pushed the ldap-group-provider branch from 31115c8 to 3c43117 Compare June 22, 2021 18:16

coryjmaklin force-pushed the ldap-group-provider branch from 3c43117 to 8a77991 Compare June 22, 2021 18:23

ldap group provider plugin

01bce45

coryjmaklin force-pushed the ldap-group-provider branch from 8a77991 to 01bce45 Compare June 22, 2021 18:31

cla-bot bot added the cla-signed label Jun 22, 2021

coryjmaklin marked this pull request as ready for review June 22, 2021 18:41

coryjmaklin requested review from dain, MiguelWeezardo and electrum June 22, 2021 18:45

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

electrum reviewed Aug 18, 2021

View reviewed changes

MiguelWeezardo reviewed Aug 27, 2021

View reviewed changes

kokosing reviewed Aug 31, 2021

View reviewed changes

rimolive mentioned this pull request Nov 30, 2021

Introduce LDAP Group Provider #10116

Closed

kokosing closed this Nov 30, 2021

eformat mentioned this pull request May 16, 2023

Ldap Group Provider #17518

Closed

		@@ -17,6 +17,7 @@
		import io.trino.plugin.password.file.FileAuthenticatorFactory;


		import com.google.common.base.CharMatcher;

		public class LdapUtil

LDAP Group Provider Support #8335

LDAP Group Provider Support #8335

Conversation

coryjmaklin commented Jun 21, 2021 • edited Loading

Context

Implementation

Testing

cla-bot bot commented Jun 21, 2021

cccs-tom 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

cccs-mjs commented Jun 21, 2021

cccs-mjs 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

cla-bot bot commented Jun 22, 2021

cla-bot bot commented Jun 22, 2021

cla-bot bot commented Jun 22, 2021

coryjmaklin commented Jun 22, 2021

rimolive commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelWeezardo Aug 27, 2021 • edited Loading

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

MiguelWeezardo Aug 27, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelWeezardo commented Oct 8, 2021

kokosing commented Oct 12, 2021

coryjmaklin commented Oct 12, 2021

rimolive commented Oct 25, 2021

kokosing commented Oct 26, 2021

rimolive commented Oct 29, 2021

MiguelWeezardo commented Nov 8, 2021

rimolive commented Nov 30, 2021

kokosing commented Nov 30, 2021 • edited Loading

coryjmaklin commented Jun 21, 2021 •

edited

Loading

MiguelWeezardo Aug 27, 2021 •

edited

Loading

MiguelWeezardo Aug 27, 2021 •

edited

Loading

kokosing commented Nov 30, 2021 •

edited

Loading