LDAP Group Provider Support #8335
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,9 +23,13 @@ | |
| import javax.naming.AuthenticationException; | ||
| import javax.naming.NamingEnumeration; | ||
| import javax.naming.NamingException; | ||
| import javax.naming.directory.Attribute; | ||
| import javax.naming.directory.Attributes; | ||
| import javax.naming.directory.DirContext; | ||
| import javax.naming.directory.SearchControls; | ||
| import javax.naming.directory.SearchResult; | ||
| import javax.naming.ldap.LdapName; | ||
| import javax.naming.ldap.Rdn; | ||
| import javax.net.ssl.SSLContext; | ||
| import javax.net.ssl.TrustManager; | ||
| import javax.net.ssl.TrustManagerFactory; | ||
|
|
@@ -36,6 +40,7 @@ | |
| import java.security.GeneralSecurityException; | ||
| import java.security.KeyStore; | ||
| import java.util.Arrays; | ||
| import java.util.Enumeration; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
|
|
@@ -49,16 +54,16 @@ | |
| import static javax.naming.Context.SECURITY_CREDENTIALS; | ||
| import static javax.naming.Context.SECURITY_PRINCIPAL; | ||
|
|
||
| public class JdkLdapClient | ||
|
There was a problem hiding this comment. What's the reason for renaming the class? Could add to PR description. There was a problem hiding this comment. Please follow: https://github.com/trinodb/trino/blob/master/DEVELOPMENT.md#git-merge-strategy
Please extract class rename into a separate commit. |
||
| implements LdapClient | ||
| { | ||
| private static final Logger log = Logger.get(JdkLdapClient.class); | ||
|
|
||
| private final Map<String, String> basicEnvironment; | ||
| private final Optional<SSLContext> sslContext; | ||
|
|
||
| @Inject | ||
| public JdkLdapClient(LdapConfig ldapConfig) | ||
| { | ||
| String ldapUrl = requireNonNull(ldapConfig.getLdapUrl(), "ldapUrl is null"); | ||
| if (ldapUrl.startsWith("ldap://")) { | ||
|
|
@@ -72,7 +77,7 @@ public JdkLdapAuthenticatorClient(LdapConfig ldapConfig) | |
| .build(); | ||
|
|
||
| this.sslContext = Optional.ofNullable(ldapConfig.getTrustCertificate()) | ||
| .map(JdkLdapClient::createSslContext); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -106,11 +111,44 @@ public Set<String> lookupUserDistinguishedNames(String searchBase, String search | |
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Set<String> lookupUserGroups(String searchBase, String searchFilter, String contextUserDistinguishedName, String contextPassword) | ||
|
There was a problem hiding this comment. 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. |
||
| throws NamingException | ||
| { | ||
| try (CloseableContext context = createUserDirContext(contextUserDistinguishedName, contextPassword); CloseableSearchResults search = searchContext(searchBase, searchFilter, context)) { | ||
| ImmutableSet.Builder<String> groupNames = ImmutableSet.builder(); | ||
| if (search.hasMore()) { | ||
| Attributes attributes = search.next().getAttributes(); | ||
| Attribute memberOfAttribute = attributes.get("memberof"); | ||
|
There was a problem hiding this comment. Is Also, could make this string a constant. There was a problem hiding this comment. For whatever reason, |
||
| if (memberOfAttribute == null) { | ||
| log.error("No memberOf attribute found... The ldap group provider requires the memberOf overlay to be enabled."); | ||
|
There was a problem hiding this comment. 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. |
||
| } | ||
| else { | ||
| for (Enumeration groupDns = memberOfAttribute.getAll(); groupDns.hasMoreElements(); ) { | ||
| String dn = groupDns.nextElement().toString(); | ||
| LdapName ln = new LdapName(dn); | ||
| for (Rdn rdn : ln.getRdns()) { | ||
| if (rdn.getType().equalsIgnoreCase("cn")) { | ||
| groupNames.add(rdn.getValue().toString()); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return groupNames.build(); | ||
| } | ||
| } | ||
|
|
||
| private static CloseableSearchResults searchContext(String searchBase, String searchFilter, CloseableContext context) | ||
| throws NamingException | ||
| { | ||
| SearchControls searchControls = new SearchControls(); | ||
| searchControls.setSearchScope(SearchControls.SUBTREE_SCOPE); | ||
| searchControls.setReturningAttributes(new String[] | ||
| { | ||
| "memberOf" | ||
| }); | ||
| return new CloseableSearchResults(context.search(searchBase, searchFilter, searchControls)); | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,82 @@ | ||||||
| /* | ||||||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
| * you may not use this file except in compliance with the License. | ||||||
| * You may obtain a copy of the License at | ||||||
| * | ||||||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||||||
| * | ||||||
| * Unless required by applicable law or agreed to in writing, software | ||||||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| * See the License for the specific language governing permissions and | ||||||
| * limitations under the License. | ||||||
| */ | ||||||
| package io.trino.plugin.password.ldap; | ||||||
|
|
||||||
| import io.airlift.log.Logger; | ||||||
| import io.trino.spi.security.AccessDeniedException; | ||||||
| import io.trino.spi.security.GroupProvider; | ||||||
|
|
||||||
| import javax.inject.Inject; | ||||||
| import javax.naming.NamingException; | ||||||
|
|
||||||
| import java.util.List; | ||||||
| import java.util.Optional; | ||||||
| import java.util.Set; | ||||||
|
|
||||||
| import static com.google.common.base.Preconditions.checkArgument; | ||||||
| import static java.util.Objects.requireNonNull; | ||||||
|
|
||||||
| public class LdapGroupProvider | ||||||
| implements GroupProvider | ||||||
| { | ||||||
| private static final Logger log = Logger.get(LdapGroupProvider.class); | ||||||
|
|
||||||
| private final LdapClient client; | ||||||
|
|
||||||
| private final List<String> userBindSearchPatterns; | ||||||
| private final Optional<String> userBaseDistinguishedName; | ||||||
| private final Optional<String> bindDistinguishedName; | ||||||
| private final Optional<String> bindPassword; | ||||||
|
|
||||||
| @Inject | ||||||
| public LdapGroupProvider(LdapClient client, LdapConfig ldapConfig) | ||||||
| { | ||||||
| this.client = requireNonNull(client, "client is null"); | ||||||
|
|
||||||
| this.userBindSearchPatterns = ldapConfig.getUserBindSearchPatterns(); | ||||||
|
There was a problem hiding this comment. Should we perform a requireNonNull() check on ldapConfig before reaching this point? Or are we just letting the NPE happen in that case? There was a problem hiding this comment. I believe it's unnecessary because unlike the client, we're not keeping a reference, and we check the values afterwards. |
||||||
| this.userBaseDistinguishedName = Optional.ofNullable(ldapConfig.getUserBaseDistinguishedName()); | ||||||
| this.bindDistinguishedName = Optional.ofNullable(ldapConfig.getBindDistingushedName()); | ||||||
| this.bindPassword = Optional.ofNullable(ldapConfig.getBindPassword()); | ||||||
|
|
||||||
| checkArgument( | ||||||
| userBaseDistinguishedName.isPresent(), | ||||||
| "Base distinguished name (DN) for user must be provided"); | ||||||
| checkArgument( | ||||||
| bindDistinguishedName.isPresent() == bindPassword.isPresent(), | ||||||
| "Both bind distinguished name and bind password must be provided"); | ||||||
| checkArgument( | ||||||
| !userBindSearchPatterns.isEmpty(), | ||||||
| "User bind search pattern must be provided"); | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public Set<String> getGroups(String user) | ||||||
| { | ||||||
| if (LdapUtil.containsSpecialCharacters(user)) { | ||||||
|
|
||||||
| throw new AccessDeniedException("Username contains a special LDAP character"); | ||||||
| } | ||||||
| for (String userBindSearchPattern : userBindSearchPatterns) { | ||||||
| String userDistinguishedName = LdapUtil.replaceUser(userBindSearchPattern, user); | ||||||
| String searchBase = userBaseDistinguishedName.orElseThrow(); | ||||||
| try { | ||||||
| return client.lookupUserGroups(searchBase, userDistinguishedName, bindDistinguishedName.orElseThrow(), bindPassword.orElseThrow()); | ||||||
|
There was a problem hiding this comment. We are breaking a loop in first iteration and so we are ignoring rest of There was a problem hiding this comment. Reminder to keep #8134 in mind. All patterns must be tried and only the last AccessDeniedException must be bubbled up. |
||||||
| } | ||||||
| catch (NamingException e) { | ||||||
| log.debug(e, "Authentication failed for user [%s], %s", user, e.getMessage()); | ||||||
| throw new RuntimeException("Authentication error"); | ||||||
| } | ||||||
| } | ||||||
| return null; | ||||||
|
There was a problem hiding this comment. This should return an empty set
Suggested change
|
||||||
| } | ||||||
| } | ||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /* | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.trino.plugin.password.ldap; | ||
|
|
||
| import com.google.inject.Injector; | ||
| import com.google.inject.Scopes; | ||
| import io.airlift.bootstrap.Bootstrap; | ||
| import io.trino.spi.security.GroupProvider; | ||
| import io.trino.spi.security.GroupProviderFactory; | ||
|
|
||
| import java.util.Map; | ||
|
|
||
| import static io.airlift.configuration.ConfigBinder.configBinder; | ||
|
|
||
| public class LdapGroupProviderFactory | ||
| implements GroupProviderFactory | ||
| { | ||
| @Override | ||
| public String getName() | ||
| { | ||
| return "ldap"; | ||
|
There was a problem hiding this comment. 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"? |
||
| } | ||
|
|
||
| @Override | ||
| public GroupProvider create(Map<String, String> config) | ||
| { | ||
| Bootstrap app = new Bootstrap( | ||
| binder -> { | ||
| configBinder(binder).bindConfig(LdapConfig.class); | ||
| binder.bind(LdapGroupProvider.class).in(Scopes.SINGLETON); | ||
| binder.bind(LdapClient.class).to(JdkLdapClient.class).in(Scopes.SINGLETON); | ||
|
There was a problem hiding this comment. Won't this fail at runtime if There was a problem hiding this comment. It is in separate guice context, so I think it is not an issue. |
||
| }); | ||
|
|
||
| Injector injector = app | ||
| .strictConfig() | ||
| .doNotInitializeLogging() | ||
| .setRequiredConfigurationProperties(config) | ||
| .initialize(); | ||
|
|
||
| return injector.getInstance(LdapGroupProvider.class); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please improve commit message: