Add support for Active Directory LDAP Domain Scope control - #1197
Add support for Active Directory LDAP Domain Scope control#1197OmerRaifler wants to merge 2 commits into
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughAdds an Assessment against linked issues
Assessment against linked issues: Out-of-scope changes No out-of-scope changes identified. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway-ha/src/main/java/io/trino/gateway/ha/security/LbLdapClient.java`:
- Around line 95-104: Escape the raw user value according to LDAP filter rules
before substituting it into the configured search filter. Apply this in both
LbLdapClient.authenticate() and getMemberOf(), preferably through a shared
helper, while preserving the existing ${USER} replacement and
authentication/member lookup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32e42b26-3034-4fa7-80b0-51182bedffc3
📒 Files selected for processing (5)
docs/security.mdgateway-ha/src/main/java/io/trino/gateway/ha/config/LdapConfiguration.javagateway-ha/src/main/java/io/trino/gateway/ha/security/LbLdapClient.javagateway-ha/src/test/java/io/trino/gateway/ha/security/TestLbLdapClient.javagateway-ha/src/test/resources/auth/ldapTestConfig.yml
|
|
||
| public boolean isLdapAdDomainScopeControl() | ||
| { | ||
| return this.ldapAdDomainScopeControl; |
| LbLdapClient(LdapConfiguration ldapConfig, LdapConnectionTemplate ldapConnectionTemplate) | ||
| { | ||
| config = ldapConfig; | ||
| this.ldapConnectionTemplate = ldapConnectionTemplate; |
There was a problem hiding this comment.
Added requireNonNull checks for ldapConfig and ldapConnectionTemplate.
| public UserEntryMapper(String memberOfAttribute) | ||
| { | ||
| memberOf = memberOfAttr; | ||
| this.memberOfAttribute = memberOfAttribute; |
There was a problem hiding this comment.
Added requireNonNull for memberOfAttribute in UserEntryMapper.
| new OpaqueControlFactory(ldapApiService, AD_DOMAIN_SCOPE_CONTROL_OID)); | ||
| return ldapApiService; | ||
| } | ||
| catch (Exception exception) { |
There was a problem hiding this comment.
Narrowed the initialization failure boundary around StandaloneLdapApiService() and wrapped it in IllegalStateException.
8ecc3cd to
86b9260
Compare
| } | ||
|
|
||
| static LdapApiService createLdapApiService() | ||
| throws Exception |
There was a problem hiding this comment.
Exception is too generic, can we narrow it down?
| final class TestLbLdapClient | ||
| { | ||
| private static final Logger log = Logger.get(TestLbLdapClient.class); | ||
| private static final String AD_DOMAIN_SCOPE_CONTROL_OID = "1.2.840.113556.1.4.1339"; |
There was a problem hiding this comment.
Can we use the same definition for AD_DOMAIN_SCOPE_CONTROL_OID from the LbLdapClient?
3 participants
Description
Adds optional support for the Microsoft Active Directory Domain Scope LDAP control.
When
ldapUserBaseDnis configured to the Active Directory domain root (for example,DC=example,DC=com), subtree searches can discover users from all organizational units. However, Active Directory may also return subordinate referrals for naming contexts such asDomainDnsZonesandForestDnsZones, which can cause authentication to fail when using the Apache Directory LDAP client.This change introduces an optional configuration that enables the Microsoft Active Directory Domain Scope control (OID
1.2.840.113556.1.4.1339) for LDAP user searches. When enabled, the search is restricted to the current domain naming context, allowing users across all OUs within the domain to be found without returning subordinate referrals.The feature is specific to Microsoft Active Directory and is disabled by default.
Additional context and related issues
This PR supersedes the previous referral-handling approach #1174 .
During the investigation, it became clear that the underlying requirement was not to follow LDAP referrals, but to prevent Active Directory from generating subordinate referrals when searching from the domain root. The Microsoft Active Directory Domain Scope control provides the desired behavior without requiring generic LDAP referral handling.
Closes #1173
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required, with the following suggested text:
* Add optional support for the Microsoft Active Directory Domain Scope LDAP control, allowing LDAP user searches from the Active Directory domain root without returning subordinate referrals.