feat: add generic LDAP authentication strategy using ldapts#1496
feat: add generic LDAP authentication strategy using ldapts#14961saac-k wants to merge 7 commits intofinos:mainfrom
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Add ldapts (v8.1.7) for modern LDAP client support and passport-custom (v1.1.1) for custom Passport strategy creation. Signed-off-by: Kwangjin Ko <kyet@me.com>
Add LDAP auth type definition to config.schema.json and generated TypeScript types with LdapConfig interface. Signed-off-by: Kwangjin Ko <kyet@me.com>
Add disabled ldap authentication entry with sensible defaults for attribute mappings, and group settings. Signed-off-by: Kwangjin Ko <kyet@me.com>
Add new LDAP authentication strategy that uses ldapts for LDAP operations and passport-custom for Passport integration. The authentication flow: 1. Bind with service account 2. Search for user entry 3. Check group memberships (user/admin) 4. Verify user password via user bind 5. Sync user profile to database Signed-off-by: Kwangjin Ko <kyet@me.com>
Add ldap module to passport strategy registry and include it in the list of username/password login strategies. Signed-off-by: Kwangjin Ko <kyet@me.com>
Test cases cover: successful auth with admin/non-admin roles, user not found, user group rejection, invalid password, connection errors, multiple entries in search result, missing credentials, and escapeFilterValue with normal strings, LDAP injection attempts, and RFC 4515 special characters. Signed-off-by: Kwangjin Ko <kyet@me.com>
|
Since this work was done outside of my company, I changed the commit email to my personal email and added GPG signatures to sign the CLA as an individual contributor. |
There was a problem hiding this comment.
Requesting changes for the two required fixes in the inline comments:
- LDAP group lookup errors are currently swallowed as “not a member”.
- Missing LDAP email falls back to an empty string, which can break user upserts with the file DB unique email index.
The remaining inline comments are nits.
| "required": ["type", "enabled", "oidcConfig"] | ||
| }, | ||
| { | ||
| "title": "LDAP Auth Config", |
There was a problem hiding this comment.
nit, non-blocking Since the config schema changed, please consider regenerating website/docs/configuration/reference.mdx as described in CONTRIBUTING.md. The generated website schema reference otherwise will not include the new LDAP auth config.
|
|
||
| // login strategies that will work with /login e.g. take username and password | ||
| const appropriateLoginStrategies = [passportLocal.type, passportAD.type]; | ||
| const appropriateLoginStrategies = [passportLocal.type, passportAD.type, passportLdap.type]; |
There was a problem hiding this comment.
nit, non-blocking I do not think the CLI should implement LDAP directly: it already posts username/password to /api/auth/login, so it should work with LDAP when LDAP is the selected server-side username/password strategy. It may be useful to mention in docs or tests that git-proxy-cli login works with configured username/password auth methods, while git-proxy-cli create-user is still local DB user provisioning and does not create LDAP users.
| const userDN = entry.dn as string; | ||
|
|
||
| // Step 4: Check user group membership | ||
| const isMember = await isUserInGroup(client, ldapConfig, userDN, ldapConfig.userGroupDN); |
There was a problem hiding this comment.
nit, non-blocking The implementation checks user/admin group membership before verifying the submitted password. I would normally verify the user bind immediately after resolving the user DN, then do group authorization only for authenticated credentials. That avoids extra LDAP work for bad-password attempts and reduces timing/log side effects.
| }; | ||
|
|
||
| const createClient = (ldapConfig: LdapConfig): Client => { | ||
| return new Client({ |
There was a problem hiding this comment.
nit, non-blocking It may be worth setting or exposing LDAP operation bounds. ldapts supports client timeouts and search options such as timeLimit/sizeLimit; without them, a slow or misconfigured LDAP server can leave login requests waiting longer than expected or return unbounded matches.
|
|
||
| const searchBase = ldapConfig.groupSearchBase || groupDN; | ||
|
|
||
| try { |
There was a problem hiding this comment.
isUserInGroup() catches every LDAP search error and returns false, so LDAP outages, invalid filters, bad search bases, or insufficient bind permissions all look the same as "user is not in the required group." For the required userGroupDN check, that should fail authentication as an LDAP/config error rather than silently denying the user. This would also make the admin-group try/catch below meaningful: let the helper throw, treat user-group lookup errors as authentication errors, and only downgrade admin-group lookup errors to admin=false if that is the intended best-effort behavior.
| // Step 7: Extract profile attributes and sync to database | ||
| const userObj = { | ||
| username: String(entry[usernameAttr] || username).toLowerCase(), | ||
| email: String(entry[emailAttr] || '').toLowerCase(), |
There was a problem hiding this comment.
email falls back to an empty string when the configured LDAP email attribute is missing. The file DB has a unique email index, so two LDAP users without mail can both authenticate successfully against LDAP but the second one can fail during db.updateUser() because it attempts to upsert another user with email: "". Could we either require the configured email attribute before updating the user, omit email when it is absent, or derive a stable fallback that remains unique per user?
| userDN: string, | ||
| groupDN: string, | ||
| ): Promise<boolean> => { | ||
| const groupFilter = (ldapConfig.groupSearchFilter || '(member={{dn}})').replaceAll( |
There was a problem hiding this comment.
nit, non-blocking For broader "generic LDAP" support, groupSearchFilter could also support a {{username}} placeholder in addition to {{dn}}. That would cover schemas such as posixGroup / memberUid without forcing group membership to reference the user DN.
Closes #1488
Add a new
ldapauthentication type that integrates with any standards-compliant LDAP server. The existingactivedirectorytype relies on theactivedirectory2library which is AD-specific and incompatible with lightweightLDAP servers (e.g. lldap). This PR introduces a generic LDAP strategy built on
ldaptsand
passport-custom, lowering the barrier to entry for teams that want simple, manageable user authentication withoutsetting up a full Active Directory.
This implementation is not
lldap-specific — it supports any standards-compliant LDAP server, which means it should also work with Active Directory. As a result, it may be possible in the future to replace the deprecatedactivedirectory2andldapjsdependencies withldapts.Testing