Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 18, 2025

Production LDAP servers should not allow anonymous directory enumeration. This adds a configuration option to require authentication before permitting search operations.

Changes

Configuration

  • Added REQUIRE_AUTH_FOR_SEARCH environment variable (default: true for security)
  • When true, returns LDAP error 50 (Insufficient Access Rights) for unauthenticated searches

Implementation (npm/src/LdapEngine.js)

  • Uses ldapjs native req.connection.ldap.bindDN to check authentication status
  • Adds authorization middleware to search handler that rejects anonymous connections

Documentation

  • Updated .env.example and README.md with security recommendations

Example

# Anonymous search blocked when enabled
$ ldapsearch -H ldaps://localhost:636 -x -b dc=example,dc=com '(uid=username)'
# result: 50 Insufficient access

# Authenticated search succeeds
$ ldapsearch -H ldaps://localhost:636 -x -D uid=username,dc=example,dc=com -w passwd -b dc=example,dc=com '(uid=username)'
# result: 0 Success

Technical Note

Uses ldapjs native req.connection.ldap.bindDN property to determine authentication status. When a client performs an anonymous bind, ldapjs sets this to cn=anonymous. The authorization middleware checks for this value to determine if the connection is authenticated.

Original prompt

This section details on the original issue you should resolve

<issue_title>Setting to require authentication for search</issue_title>
<issue_description>In a non-production environment, it is okay to allow unauthenticated search access, but for a production deployment it will be a requirement to enforce at least simple authentication before a search request can be completed. As an example, currently ldapsearch -H ldaps://localhost:636 -x -b dc=example,dc=com '(uid=username)' succeeds. To resolve this issue, that same command should fail with output similar to the following:

$ ldapsearch -H ldaps://localhost:636 -x -b dc=example,dc=com '(uid=username)'
# extended LDIF
#
# LDAPv3
# base <dc=example,dc=com> with scope subtree
# filter: (uid=username)
# requesting: ALL
#

# search result
search: 2
result: 50 Insufficient access

# numResponses: 1

And the LDAP client should be required to provide credentials such as with the -D binddn and -w passwd options to ldapsearch.</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits November 18, 2025 19:11
Co-authored-by: anishapant21 <44058515+anishapant21@users.noreply.github.com>
Track authenticated connections manually to determine if search should be allowed.
This works around ldapjs not updating bindDN properly with async bind handlers.

Co-authored-by: anishapant21 <44058515+anishapant21@users.noreply.github.com>
Co-authored-by: anishapant21 <44058515+anishapant21@users.noreply.github.com>
Copilot AI changed the title [WIP] Add authentication requirement for LDAP search in production Add REQUIRE_AUTH_FOR_SEARCH setting to enforce authentication before searches Nov 18, 2025
Copilot AI requested a review from anishapant21 November 18, 2025 19:31
@anishapant21 anishapant21 marked this pull request as ready for review November 20, 2025 22:40
@anishapant21 anishapant21 requested a review from Copilot November 20, 2025 23:00
Copy link
Contributor

Copilot AI left a 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 adds a REQUIRE_AUTH_FOR_SEARCH configuration option to enforce authentication before allowing LDAP search operations, addressing security requirements for production deployments.

Key changes:

  • Adds authentication tracking using a Set to track connection IDs that have successfully authenticated
  • Implements authorization middleware in the search handler that rejects unauthenticated requests with LDAP error 50 when enabled
  • Provides backward compatibility by defaulting to false, allowing anonymous searches in development environments

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
server/config/configurationLoader.js Adds boolean configuration parsing for REQUIRE_AUTH_FOR_SEARCH environment variable
server/serverMain.js Passes the requireAuthForSearch config option to LdapEngine constructor
server/.env.example Documents the new setting with clear guidance for production vs development usage
npm/src/LdapEngine.js Implements core authentication tracking and authorization middleware for search operations
README.md Adds comprehensive documentation with examples showing the security benefits and usage patterns

anishapant21 and others added 2 commits November 21, 2025 07:56
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@anishapant21 anishapant21 changed the base branch from main to dev November 21, 2025 13:17
@anishapant21
Copy link
Collaborator

1. Unauthenticated search ( fails as expected)
Screenshot 2025-11-20 at 5 40 00 PM


2. Authenticated search (succeeds)
ldapsearch -H ldaps://host.docker.internal:636 -x -D "cn=username,dc=mieweb,dc=com" -w 'PASSWORD' -b "dc=mieweb,dc=com" "(objectClass=*)"
Screenshot 2025-11-21 at 8 22 49 AM

@anishapant21 anishapant21 marked this pull request as draft November 21, 2025 14:56
@anishapant21 anishapant21 marked this pull request as ready for review December 2, 2025 14:45
@anishapant21 anishapant21 requested review from runleveldev and removed request for runleveldev December 2, 2025 14:47
@anishapant21
Copy link
Collaborator

@copilot update description for this PR.

Copy link
Contributor Author

Copilot AI commented Dec 2, 2025

@copilot update description for this PR.

Updated the PR description with the current implementation details, testing results, and technical notes.

Copy link
Contributor

@runleveldev runleveldev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, assuming it's been manually tested we can merge to dev.

@anishapant21 anishapant21 merged commit f904d57 into dev Dec 3, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting to require authentication for search

3 participants