-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(1372): LDAP Authentication vs Filter Clarity #1797
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
Conversation
…revention Cheat Sheet
- Update RFC links for LDAP encoding functions in the LDAP Injection Prevention Cheat Sheet
Hey @Jeymz! 👋 |
Why hello there captain! Hope things are well. |
RFC link update looks good. Any Java experts here who can validate the example? |
// Step 2: Bind with the DN and provided password | ||
Hashtable<String, String> env = new Hashtable<>(); | ||
env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory"); | ||
env.put(Context.PROVIDER_URL, "ldap://example.com:389"); |
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.
With 'simple' authN bind type, one really needs to use "ldaps://" which generally is on port 636. You probably can get away with using vanilla LDAP on port 389 for certain variations of SASL (like GSSAPI, SCRAM-SHA-256, etc.) but even then, I wouldn't generally recommend it.
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.
I've pushed an update to address this. Can't believe I missed that in the example. Stupid Jeymz... Good thing we got great members like you to filter out my stupidity 😅
// Step 1: Look up the user DN with a parameterized search | ||
String searchFilter = "(&(uid={0})(objectClass=person))"; | ||
SearchControls controls = new SearchControls(); | ||
controls.setSearchScope(SearchControls.SUBTREE_SCOPE); | ||
|
||
NamingEnumeration<SearchResult> results = ctx.search( | ||
"ou=users,dc=example,dc=com", |
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.
This of course implies where you know to look and assumes the objectClass will be 'person'. That may not be a valid assumption for things like service accounts that are not associated with people.
Another approach that I've seen back when I worked for a telecommunications company was to do an anonymous subtree search first for the UID (or sometimes, a CN) and then matching DNs (usually only one) would be returned, and the BIND operation would be done on that. YMMV.
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.
I've tried to take a stab and using your approach for the example. Thanks for the keen eye on this. I appreciate reviewers that add valuable insights like this to help make implementing these patterns easier for real world scenarios. Really wish I knew more Java.
- LDAPS: Switched from ldap://example.com:389 to ldaps://example.com:636 for secure simple authentication. - Anonymous Search: Opened a context with "none" authentication to look up the DN by uid. - Flexible Filter: The search filter now only requires uid, no assumption about objectClass=person, which makes it work for service accounts or other directory objects. - Resource Safety: Explicitly closing the anonymous context after the search.
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.
I'm not quite ready to approve this. While overall, I think the code looks fine (despite my 2 comments), I still have concerns about whether or not this code example actually addresses the points raised by GitHub issue #1372. I almost think that this code could potentially distract from the 2 main point raised in the issue that "BIND should be used for authentication via LDAP and that passwords should not be escaped as part of that because passwords should not be part of any LDAP filter. Instead of showing additional code and raising additional issues such as future requests to show additional examples using a SASL mechanism or if we would show an example that uses (say) the Mozilla LDAP Java SDK or Spring's LdapTemplate, or a C# .NET example, I'm wondering if we should just put the main points that @einhirn originally brought up in the GitHub issue #1372. And if we really want some examples of LDAP authentication in this CS, rather than using up space, we should just point to a couple of classic examples such as one or mohttps://docs.oracle.com/javase/8/docs/technotes/guides/jndi/jndi-ldap.htmlre of these:
- https://docs.oracle.com/javase/8/docs/technotes/guides/jndi/jndi-ldap.html
- https://www.geeksforgeeks.org/advance-java/spring-security-with-ldap-authentication/
- https://www.baeldung.com/java-ldap-auth
- Etc.
If we do decide to include an LDAP Bind code snippet, I think we should at least drop it into a collapsible section since LDAP authentication is not the main purpose of this cheat sheet.
); | ||
|
||
if (!results.hasMore()) { | ||
throw new AuthenticationException("User not found"); // Obscure to invalid username and/or password client side |
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.
Not critical, but UIDs are generally considered not considered sensitive data within a company, where this is likely to be used, so I would advise liksting the uid that you were searching for and did not find here. If the exceptions are eventually logged to a SIEM, that information can be heplful.
controls.setSearchScope(SearchControls.SUBTREE_SCOPE); | ||
|
||
NamingEnumeration<SearchResult> results = anonCtx.search( | ||
"ou=users,dc=example,dc=com", |
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.
My comment about doing the anonymous search was more aimed at LDAP directory services where UIDs could be in multiple trees (e.g, perhaps "ou=service_accounts" besides "ou=people"), so to illustrate this, you could redo your LDAP search to look in both of those OUs where accounts are stored (preferred) or anchor the search at just "dc=example,dc=com" (a bit less preferred. But if you know all your accounts are directly under "ou=people" it's less necessary.
@kwwall I think I may have been trying to stretch too far to justify the example above. Instead let's just remove the insecure aspects of that example to clarify that we should never use an LDAP search to AuthN a user account and focus only on why allow lists are a must when dealing with unsafe LDAP Query construction. |
@szh Is there any way we can either get some eyes on this to either complete or close this PR? |
@kwwall does it look ok to you now without the example? |
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.
Pull Request Overview
This PR enhances the LDAP Injection Prevention Cheat Sheet by clarifying safe authentication practices and correcting documentation errors. The changes address issue #1372 by removing misleading examples that could lead developers to incorrectly use LDAP search filters for password validation.
- Removed password validation from the Java escaping example to prevent unsafe authentication patterns
- Corrected an RFC4515 link URL that was pointing to an invalid location
- Updated explanatory text to focus on safe search filter construction rather than authentication
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Note:
If your PR is related to an issue, please finish your PR text with the following line:
Please make sure that for your contribution:
[TEXT](URL)
This PR fixes issue #1372 .