FOGProject / fogproject

An open source computer cloning & management system
https://fogproject.org
GNU General Public License v3.0
1.11k stars 221 forks source link

LDAP plugin group search uses "name=" to find the name of the group with no option to specify a different attribute #461

Closed ggiesen closed 1 month ago

ggiesen commented 2 years ago

I'm attempting to set up LDAP authentication with FOG using the LDAP plugin against our FreeIPA servers, but it looks like the LDAP plugin can only match groups using the "name" attribute which FreeIPA groups do not carry. AD has a name attribute (which as far as I can tell always matches the value in "cn"), not sure about OpenLDAP or other LDAP implementations.

/var/log/php-fpm/www-error.log

[22-Feb-2022 21:54:15 UTC] Plugin LDAP::_result(). Search Method: search; Filter: (&(|(name=fog_mobile))(member=uid=someuser,cn=users,cn=accounts,dc=ipa,dc=example,dc=com)); Result: 0
[22-Feb-2022 21:54:15 UTC] Plugin LDAP::_result(). Search Method: search; Filter: (&(|(name=fog_admin))(member=uid=someuser,cn=users,cn=accounts,dc=ipa,dc=example,dc=com)); Result: 0

Would it make sense to change the attribute it matches on to be "cn=" rather than "name="? Or alternatively, the ability to specify an attribute to match on?

https://github.com/FOGProject/fogproject/blob/171d63724131c396029992730660497d48410842/packages/web/lib/plugins/ldap/class/ldap.class.php#L622-L623

https://github.com/FOGProject/fogproject/blob/171d63724131c396029992730660497d48410842/packages/web/lib/plugins/ldap/class/ldap.class.php#L646-L647

I was able to patch the file to make it work, but not sure which approach you'd prefer permanently:

diff -urN ldap.class.php.orig ldap.class.php
--- ldap.class.php.orig 2022-02-22 06:46:01.401606271 +0000
+++ ldap.class.php      2022-02-23 00:21:40.452462149 +0000
@@ -619,8 +619,8 @@
         $adminGroups = explode(',', $adminGroup);
         $adminGroups = array_map('trim', $adminGroups);
         $filter = sprintf(
-            '(&(|(name=%s))(%s=%s))',
-            implode(')(name=', (array)$adminGroups),
+            '(&(|(cn=%s))(%s=%s))',
+            implode(')(cn=', (array)$adminGroups),
             $grpMemAttr,
             $this->escape($userDN, null, LDAP_ESCAPE_FILTER)
         );
@@ -643,8 +643,8 @@
         $userGroups = explode(',', $userGroup);
         $userGroups = array_map('trim', $userGroups);
         $filter = sprintf(
-            '(&(|(name=%s))(%s=%s))',
-            implode(')(name=', (array)$userGroups),
+            '(&(|(cn=%s))(%s=%s))',
+            implode(')(cn=', (array)$userGroups),
             $grpMemAttr,
             $this->escape($userDN, null, LDAP_ESCAPE_FILTER)
         );
Sebastian-Roth commented 2 years ago

@fernandoGietz Hi, would you find some time to look into this?

ggiesen commented 2 years ago

I did start working on a PR for this, but haven't had time to complete or test it:

https://github.com/ggiesen/fogproject/compare/dev-branch...ldap_plugin_enhancements

Sebastian-Roth commented 2 years ago

@ggiesen What you have there looks very promising. I am wondering if we can get this revived and merged.

Sebastian-Roth commented 1 year ago

@ggiesen Any news on this. I somehow lost track of this and I wonder if your PR is ready to be merged into the official dev-branch?

ggiesen commented 1 year ago

Sorry, I've been pulled in a bunch of different directions at work since I did this and haven't used FOG much since then until recently (although now dealing with a bunch of cert/URL issues). I expect it'll probably be at least a couple months before I get back to this.

mastacontrola commented 1 month ago

While more testing will be necessary, I think that can be handled after the testing is performed. As for the feature add, this is already completed.

Closing issue.

Thank you!