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

feature: LDAP auth update: alerts, api, remember me #7335

Merged
merged 4 commits into from Oct 30, 2017

Conversation

Projects
None yet
7 participants
@murrant
Member

murrant commented Sep 13, 2017

Add ability to use a bind account if the server does not allow anonymous bind.
If the server does allow anonymous bind, no config change is needed.
Defer ldap connection until it is needed (saves connections from pollers)
Use Config class

FYI, I have no way to test this.

TODO: update/validate docs

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926


This change is Reviewable

Fixes: #3069

#7291, #7237

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Sep 13, 2017

Thank you for submitting a PR @murrant! We have found the following @laf, @supertylerc and @Ultra2D based on the history of these files to review this PR.

mention-bot commented Sep 13, 2017

Thank you for submitting a PR @murrant! We have found the following @laf, @supertylerc and @Ultra2D based on the history of these files to review this PR.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf
Member

laf commented Sep 14, 2017

@zlesnr

This comment has been minimized.

Show comment
Hide comment
@zlesnr

zlesnr Sep 26, 2017

Any update on this? We'd like to see this get in, as currently we are unable to issue API keys.

zlesnr commented Sep 26, 2017

Any update on this? We'd like to see this get in, as currently we are unable to issue API keys.

@michelbragaguimaraes

This comment has been minimized.

Show comment
Hide comment
@michelbragaguimaraes

michelbragaguimaraes Sep 26, 2017

@zlesnr I am facing the same issue. I'd like to configure oxydized and I use active directory to auth.

michelbragaguimaraes commented Sep 26, 2017

@zlesnr I am facing the same issue. I'd like to configure oxydized and I use active directory to auth.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 26, 2017

Member

@zlesnr have you tested this?

@michelbragaguimaraes You should be using active directory auth then which with a bind account supports the API already.

Member

laf commented Sep 26, 2017

@zlesnr have you tested this?

@michelbragaguimaraes You should be using active directory auth then which with a bind account supports the API already.

@michelbragaguimaraes

This comment has been minimized.

Show comment
Hide comment
@michelbragaguimaraes

michelbragaguimaraes Sep 26, 2017

@laf That was it. I just set the binduser and it worked.
Awesome... Thank Ya!

michelbragaguimaraes commented Sep 26, 2017

@laf That was it. I just set the binduser and it worked.
Awesome... Thank Ya!

@zlesnr

This comment has been minimized.

Show comment
Hide comment
@zlesnr

zlesnr Sep 27, 2017

@laf We're not in a position to test at this time.

zlesnr commented Sep 27, 2017

@laf We're not in a position to test at this time.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 27, 2017

Member

Ok. Without people testing then I'm not sure when this will get merged in.

Member

laf commented Sep 27, 2017

Ok. Without people testing then I'm not sure when this will get merged in.

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 3, 2017

I tested this. Our current configuration stopped working. We are not allowing anonymous access to our ldap, perhaps the previous implementation used the user-bound ldap-connection. After setting a bind user (needed to do a small fix to set binduser with full dn), authentication worked. I can also confirm that group levels seems to be working.

I am not sure how things are supposed to work. But:

  • The user list gets populated, but there are duplicate users. We have several $config['auth_ldap_groups']['...'] in our configuration, where users can be part of different groups. Users are listed once pr every group they are a member of.
  • scripts/auth_test.php shows empty username, realname, user_id and email. This is working in the master branch (I dont know if this is a false alarm if the script also needs to be updated).
  • Pressing "Edit Permissions"/"Edit User" does not lead anywhere. Something happens, and the page is reloaded. I wouldn't expect it to be possible to do something here, but thought I could check if the user information was present.
  • Trying to create an API token gives the error message: "ERROR: error with data, please ensure a valid user and token have been specified.". The user list is populated with duplicates as described earlier.

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 92 at r1 (raw file):

        $connection = get_ldap_connection();

        $filter = '(' . Config::get('auth_ldap_prefix') . $username . ')';

Might be a bit late to change this now, but: More normal naming convention also in line with the group-configuration would be "auth_ldap_userbase" for auth_ldap_suffix, and "auth_ldap_userattr" for auth_ldap_prefix.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):


        if ($entries['count']) {
            return $entries[0][Config::get('auth_ldap_uid_attribute' . 'uidnumber')][0];

Looking at our directory this is "uidNumber".


html/includes/authentication/ldap.inc.php, line 180 at r1 (raw file):

                $username = $entry['uid'][0];
                $realname = $entry['cn'][0];
                $user_id = $entry[Config::get('auth_ldap_uid_attribute' . 'uidnumber')][0];

Looking at our directory this is uidNumber.


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

        if (ldap_bind(
            $ldap_connection,
            get_full_dn(Config::get('auth_ldap_binduser')),

This forces the binduser to be mixed with the users. Suggesting to only use "Config::get('auth_ldap_binduser')", the full DN of the bind user must then be specified in the config file.


Comments from Reviewable

janegilv commented Oct 3, 2017

I tested this. Our current configuration stopped working. We are not allowing anonymous access to our ldap, perhaps the previous implementation used the user-bound ldap-connection. After setting a bind user (needed to do a small fix to set binduser with full dn), authentication worked. I can also confirm that group levels seems to be working.

I am not sure how things are supposed to work. But:

  • The user list gets populated, but there are duplicate users. We have several $config['auth_ldap_groups']['...'] in our configuration, where users can be part of different groups. Users are listed once pr every group they are a member of.
  • scripts/auth_test.php shows empty username, realname, user_id and email. This is working in the master branch (I dont know if this is a false alarm if the script also needs to be updated).
  • Pressing "Edit Permissions"/"Edit User" does not lead anywhere. Something happens, and the page is reloaded. I wouldn't expect it to be possible to do something here, but thought I could check if the user information was present.
  • Trying to create an API token gives the error message: "ERROR: error with data, please ensure a valid user and token have been specified.". The user list is populated with duplicates as described earlier.

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 92 at r1 (raw file):

        $connection = get_ldap_connection();

        $filter = '(' . Config::get('auth_ldap_prefix') . $username . ')';

Might be a bit late to change this now, but: More normal naming convention also in line with the group-configuration would be "auth_ldap_userbase" for auth_ldap_suffix, and "auth_ldap_userattr" for auth_ldap_prefix.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):


        if ($entries['count']) {
            return $entries[0][Config::get('auth_ldap_uid_attribute' . 'uidnumber')][0];

Looking at our directory this is "uidNumber".


html/includes/authentication/ldap.inc.php, line 180 at r1 (raw file):

                $username = $entry['uid'][0];
                $realname = $entry['cn'][0];
                $user_id = $entry[Config::get('auth_ldap_uid_attribute' . 'uidnumber')][0];

Looking at our directory this is uidNumber.


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

        if (ldap_bind(
            $ldap_connection,
            get_full_dn(Config::get('auth_ldap_binduser')),

This forces the binduser to be mixed with the users. Suggesting to only use "Config::get('auth_ldap_binduser')", the full DN of the bind user must then be specified in the config file.


Comments from Reviewable

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 4, 2017

Member

Fixed duplicates in userlist. Can you update and pastebin the outptu of auth_test.php Perhaps soemthing isn't working.

I suspect we have some sort of issue with the user id.


Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 92 at r1 (raw file):

Previously, janegilv wrote…

Might be a bit late to change this now, but: More normal naming convention also in line with the group-configuration would be auth_ldap_suffix "auth_ldap_userbase" for auth_ldap_suffix, and "auth_ldap_userattr" for auth_ldap_prefix.

This is inherited and would break everyone's ldap auth to change it now. I agree the names are less than optimal, but not worth the pain to change it at this time.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, janegilv wrote…

Looking at our directory this is "uidNumber".

That is what is being used unless you set auth_ldap_uid_attribute


html/includes/authentication/ldap.inc.php, line 180 at r1 (raw file):

That is what is being used unless you set auth_ldap_uid_attribute


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, janegilv wrote…

This forces the binduser to be mixed with the users. Suggesting to only use "Config::get('auth_ldap_binduser')", the full DN of the bind user must then be specified in the config file.

the default auth_ldap_prefix and auth_ldap_suffix is '', so if you don't set that, you can specify auth_ldap_binduser as full dn. I'm not really sure if that is adequate or not. Again, the only experience I have with ldap is AD.


Comments from Reviewable

Member

murrant commented Oct 4, 2017

Fixed duplicates in userlist. Can you update and pastebin the outptu of auth_test.php Perhaps soemthing isn't working.

I suspect we have some sort of issue with the user id.


Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 92 at r1 (raw file):

Previously, janegilv wrote…

Might be a bit late to change this now, but: More normal naming convention also in line with the group-configuration would be auth_ldap_suffix "auth_ldap_userbase" for auth_ldap_suffix, and "auth_ldap_userattr" for auth_ldap_prefix.

This is inherited and would break everyone's ldap auth to change it now. I agree the names are less than optimal, but not worth the pain to change it at this time.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, janegilv wrote…

Looking at our directory this is "uidNumber".

That is what is being used unless you set auth_ldap_uid_attribute


html/includes/authentication/ldap.inc.php, line 180 at r1 (raw file):

That is what is being used unless you set auth_ldap_uid_attribute


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, janegilv wrote…

This forces the binduser to be mixed with the users. Suggesting to only use "Config::get('auth_ldap_binduser')", the full DN of the bind user must then be specified in the config file.

the default auth_ldap_prefix and auth_ldap_suffix is '', so if you don't set that, you can specify auth_ldap_binduser as full dn. I'm not really sure if that is adequate or not. Again, the only experience I have with ldap is AD.


Comments from Reviewable

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 4, 2017

Duplicates in userlist is no longer shown.

Output from auth_test.php (with some information sensored):

sudo -iu librenms scripts/auth_test.php  -u user1 -d -v
Authentication Method: ldap
Password: 
Authenticate user user1: 
AUTH SUCCESS

User ():
  username => test
  realname => testfirst testlast
  user_id => 
  email => test@test.net
Groups: ...

As you are saying, the user_id is missing. Also, this is the information for another user ("test") which the script has been run for earlier. Could the ldap-connection be cached/not closed with the other user logged in? I tried going as far as restarting the server, but the issue still persists.


Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

That is what is being used unless you set auth_ldap_uid_attribute

Notice the upper-case "N"


html/includes/authentication/ldap.inc.php, line 180 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

That is what is being used unless you set auth_ldap_uid_attribute

Notice the upper-case "N"


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

the default auth_ldap_prefix and auth_ldap_suffix is '', so if you don't set that, you can specify auth_ldap_binduser as full dn. I'm not really sure if that is adequate or not. Again, the only experience I have with ldap is AD.

So we use auth_ldap_prefix and auth_ldap_suffix to tell librenms where to search for users. E.g. auth_ldap_prefix="uid=", and auth_ldap_suffix="ou=people,dc=domain,dc=net". Now our bind-users "serviceaccounts" are separated. e.g. "cn=librenms,ou=serviceaccount,dc=domain,dc=net". This makes us unable to filter where in the tree, and on which type the user search should happen.


Comments from Reviewable

janegilv commented Oct 4, 2017

Duplicates in userlist is no longer shown.

Output from auth_test.php (with some information sensored):

sudo -iu librenms scripts/auth_test.php  -u user1 -d -v
Authentication Method: ldap
Password: 
Authenticate user user1: 
AUTH SUCCESS

User ():
  username => test
  realname => testfirst testlast
  user_id => 
  email => test@test.net
Groups: ...

As you are saying, the user_id is missing. Also, this is the information for another user ("test") which the script has been run for earlier. Could the ldap-connection be cached/not closed with the other user logged in? I tried going as far as restarting the server, but the issue still persists.


Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

That is what is being used unless you set auth_ldap_uid_attribute

Notice the upper-case "N"


html/includes/authentication/ldap.inc.php, line 180 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

That is what is being used unless you set auth_ldap_uid_attribute

Notice the upper-case "N"


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

the default auth_ldap_prefix and auth_ldap_suffix is '', so if you don't set that, you can specify auth_ldap_binduser as full dn. I'm not really sure if that is adequate or not. Again, the only experience I have with ldap is AD.

So we use auth_ldap_prefix and auth_ldap_suffix to tell librenms where to search for users. E.g. auth_ldap_prefix="uid=", and auth_ldap_suffix="ou=people,dc=domain,dc=net". Now our bind-users "serviceaccounts" are separated. e.g. "cn=librenms,ou=serviceaccount,dc=domain,dc=net". This makes us unable to filter where in the tree, and on which type the user search should happen.


Comments from Reviewable

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 4, 2017

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, janegilv wrote…

So we use auth_ldap_prefix and auth_ldap_suffix to tell librenms where to search for users. E.g. auth_ldap_prefix="uid=", and auth_ldap_suffix="ou=people,dc=domain,dc=net". Now our bind-users "serviceaccounts" are separated. e.g. "cn=librenms,ou=serviceaccount,dc=domain,dc=net". This makes us unable to filter where in the tree, and on which type the user search should happen.

So if this is unclear, what I am proposing is to not use get_full_dn(Config::get...) in this line, only Config::get...


Comments from Reviewable

janegilv commented Oct 4, 2017

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, janegilv wrote…

So we use auth_ldap_prefix and auth_ldap_suffix to tell librenms where to search for users. E.g. auth_ldap_prefix="uid=", and auth_ldap_suffix="ou=people,dc=domain,dc=net". Now our bind-users "serviceaccounts" are separated. e.g. "cn=librenms,ou=serviceaccount,dc=domain,dc=net". This makes us unable to filter where in the tree, and on which type the user search should happen.

So if this is unclear, what I am proposing is to not use get_full_dn(Config::get...) in this line, only Config::get...


Comments from Reviewable

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 5, 2017

Member

Did you try setting auth_ldap_uid_attribute?

There is no cache in LibreNMS, perhaps OpenLDAP? Also, are you sure the info is incorrect?


Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, janegilv wrote…

Notice the upper-case "N"

Yeah, I noticed the N. Your ldap implementation is case sensitive for this? I think it would be ok to change the default to uidNumber, but I just kept what had been there in the past (after looking it has been this way since the fork).

If you set it to uidNumber does that work?


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, janegilv wrote…

So if this is unclear, what I am proposing is to not use get_full_dn(Config::get...) in this line, only Config::get...

That is clear, but doesn't seem like a good solution. Most users just want to put in the username, not the full dn. Any better ideas on how to allow both?


Comments from Reviewable

Member

murrant commented Oct 5, 2017

Did you try setting auth_ldap_uid_attribute?

There is no cache in LibreNMS, perhaps OpenLDAP? Also, are you sure the info is incorrect?


Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, janegilv wrote…

Notice the upper-case "N"

Yeah, I noticed the N. Your ldap implementation is case sensitive for this? I think it would be ok to change the default to uidNumber, but I just kept what had been there in the past (after looking it has been this way since the fork).

If you set it to uidNumber does that work?


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, janegilv wrote…

So if this is unclear, what I am proposing is to not use get_full_dn(Config::get...) in this line, only Config::get...

That is clear, but doesn't seem like a good solution. Most users just want to put in the username, not the full dn. Any better ideas on how to allow both?


Comments from Reviewable

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 5, 2017

Member

@janegilv I just found a bad typo regarding auth_ldap_uid_attribute. Please test with the new code, thanks.

Member

murrant commented Oct 5, 2017

@janegilv I just found a bad typo regarding auth_ldap_uid_attribute. Please test with the new code, thanks.

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 5, 2017

So I tested the last version, but still the same output (with or without the config option). If I mess around with the username of our test-user, the returned information sometimes changes to the information for a different user. Perhaps there is a search with multiple hits, and the wrong one is selected somewhere?

Is there an easy way of getting debug output for logins?


Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

That is clear, but doesn't seem like a good solution. Most users just want to put in the username, not the full dn. Any better ideas on how to allow both?

In all other software I have used the full DN is used, I guess this is what users would expect. In AD the tree-structure is probably more defined and homogenous across environments than ldap.


Comments from Reviewable

janegilv commented Oct 5, 2017

So I tested the last version, but still the same output (with or without the config option). If I mess around with the username of our test-user, the returned information sometimes changes to the information for a different user. Perhaps there is a search with multiple hits, and the wrong one is selected somewhere?

Is there an easy way of getting debug output for logins?


Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 329 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

That is clear, but doesn't seem like a good solution. Most users just want to put in the username, not the full dn. Any better ideas on how to allow both?

In all other software I have used the full DN is used, I guess this is what users would expect. In AD the tree-structure is probably more defined and homogenous across environments than ldap.


Comments from Reviewable

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 5, 2017

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

Yeah, I noticed the N. Your ldap implementation is case sensitive for this? I think it would be ok to change the default to uidNumber, but I just kept what had been there in the past (after looking it has been this way since the fork).

If you set it to uidNumber does that work?

I dont know if the implementation is case-sensitive or not. Just noticed it differed.


Comments from Reviewable

janegilv commented Oct 5, 2017

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, murrant (Tony Murray) wrote…

Yeah, I noticed the N. Your ldap implementation is case sensitive for this? I think it would be ok to change the default to uidNumber, but I just kept what had been there in the past (after looking it has been this way since the fork).

If you set it to uidNumber does that work?

I dont know if the implementation is case-sensitive or not. Just noticed it differed.


Comments from Reviewable

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 5, 2017

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, janegilv wrote…

I dont know if the implementation is case-sensitive or not. Just noticed it differed.

The problem with the userid seems to be here. If I replace Config::get('auth_ldap_uid_attribute' . 'uidnumber') with only uidnumber, this returns the uid. The output then becomes:

sudo -iu librenms scripts/auth_test.php  -u test -v
Authentication Method: ldap
Password: 
Authenticate user test: 
AUTH SUCCESS

User (1xxx):
Groups: ...

uidnumber is the correct one for the right users. Rest of the information about the user disappears.


Comments from Reviewable

janegilv commented Oct 5, 2017

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, janegilv wrote…

I dont know if the implementation is case-sensitive or not. Just noticed it differed.

The problem with the userid seems to be here. If I replace Config::get('auth_ldap_uid_attribute' . 'uidnumber') with only uidnumber, this returns the uid. The output then becomes:

sudo -iu librenms scripts/auth_test.php  -u test -v
Authentication Method: ldap
Password: 
Authenticate user test: 
AUTH SUCCESS

User (1xxx):
Groups: ...

uidnumber is the correct one for the right users. Rest of the information about the user disappears.


Comments from Reviewable

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 5, 2017

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, janegilv wrote…

The problem with the userid seems to be here. If I replace Config::get('auth_ldap_uid_attribute' . 'uidnumber') with only uidnumber, this returns the uid. The output then becomes:

sudo -iu librenms scripts/auth_test.php  -u test -v
Authentication Method: ldap
Password: 
Authenticate user test: 
AUTH SUCCESS

User (1xxx):
Groups: ...

uidnumber is the correct one for the right users. Rest of the information about the user disappears.

Doing the same in all spots where Config::get... is used to reference an array makes auth_test behave as expected. I dont know PHP well enough to know the proper way of referencing here.


Comments from Reviewable

janegilv commented Oct 5, 2017

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):

Previously, janegilv wrote…

The problem with the userid seems to be here. If I replace Config::get('auth_ldap_uid_attribute' . 'uidnumber') with only uidnumber, this returns the uid. The output then becomes:

sudo -iu librenms scripts/auth_test.php  -u test -v
Authentication Method: ldap
Password: 
Authenticate user test: 
AUTH SUCCESS

User (1xxx):
Groups: ...

uidnumber is the correct one for the right users. Rest of the information about the user disappears.

Doing the same in all spots where Config::get... is used to reference an array makes auth_test behave as expected. I dont know PHP well enough to know the proper way of referencing here.


Comments from Reviewable

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 5, 2017

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):
Also uidnumber has to be lowercase. php_get_entries API:

The structure of the array is as follows. The attribute index is converted to lowercase. (Attributes are case-insensitive for directory servers, but not when used as array indices.)


Comments from Reviewable

janegilv commented Oct 5, 2017

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


html/includes/authentication/ldap.inc.php, line 148 at r1 (raw file):
Also uidnumber has to be lowercase. php_get_entries API:

The structure of the array is as follows. The attribute index is converted to lowercase. (Attributes are case-insensitive for directory servers, but not when used as array indices.)


Comments from Reviewable

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 11, 2017

Member

@janegilv I'm not sure if you've tested the most recent code or not. Also, lets not use reviewable, it was getting a little confusing.

Member

murrant commented Oct 11, 2017

@janegilv I'm not sure if you've tested the most recent code or not. Also, lets not use reviewable, it was getting a little confusing.

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 12, 2017

Sorry about the wall of text up there.

uidNumber has to be all lowercase. All the ldap-attribute are converted to lowercase in the array-keys. Maybe it would make sense to convert the attributes from Configuration::get to lowercase in order to remove confusion when configuring. If users configure with a mixed-case attribute as seen in their ldap, this will not work.

In every other application I have used, the bind-user is specified with a full dn in the configuration. I've had to make manual changes for our current setup to work now. One option would be to rename the configuration option: auth_ldap_binduser -> auth_ldap_binddn.

The last commit seems to be working fine with these changes. I can add API-tokens and connect them to users, edit user rights and so on.

janegilv commented Oct 12, 2017

Sorry about the wall of text up there.

uidNumber has to be all lowercase. All the ldap-attribute are converted to lowercase in the array-keys. Maybe it would make sense to convert the attributes from Configuration::get to lowercase in order to remove confusion when configuring. If users configure with a mixed-case attribute as seen in their ldap, this will not work.

In every other application I have used, the bind-user is specified with a full dn in the configuration. I've had to make manual changes for our current setup to work now. One option would be to rename the configuration option: auth_ldap_binduser -> auth_ldap_binddn.

The last commit seems to be working fine with these changes. I can add API-tokens and connect them to users, edit user rights and so on.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 13, 2017

Member

Ok, thank you that makes sense as to why the code was originally lowercase.

Member

murrant commented Oct 13, 2017

Ok, thank you that makes sense as to why the code was originally lowercase.

murrant added some commits Sep 13, 2017

feature: LDAP auth update: alerts, api, remember me
Defer ldap connection until it is needed (saves connections from pollers)
Add ability to use a bind account if the server does not allow anonymous bind.
If the server does allow anonymous bind, no config change is needed.
Use Config class

FYI, I have no way to test this.

TODO: update/validate docs
fix bug in Config get for auth_ldap_uid_attribute, `.` should have be…
…en `,`

Change case of uidNumber to match common configs (should be case insensitive anyway)
revert uidnumber case changes and fix up user supplied ones as it is …
…unintuitive that they need to be lowercase.

Add auth_ldap_binddn setting to allow more a more specific way to enter the bind user.
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 13, 2017

Member

@janegilv Ok, one final test? I think this is looking good.

Other testers would be great too.

Member

murrant commented Oct 13, 2017

@janegilv Ok, one final test? I think this is looking good.

Other testers would be great too.

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Oct 13, 2017

The inspection completed: 20 new issues, 2 updated code elements

scrutinizer-notifier commented Oct 13, 2017

The inspection completed: 20 new issues, 2 updated code elements

@janegilv

This comment has been minimized.

Show comment
Hide comment
@janegilv

janegilv Oct 13, 2017

Seems to be working well.

janegilv commented Oct 13, 2017

Seems to be working well.

@laf laf merged commit 9d73cd4 into librenms:master Oct 30, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@RalfHerzog RalfHerzog referenced this pull request Oct 31, 2017

Merged

Support ldap bind #16

mcq8 added a commit to mcq8/librenms that referenced this pull request Nov 12, 2017

laf added a commit that referenced this pull request Nov 18, 2017

refactor: Refactored authorizers to classes (#7497)
* Refactored authorizers to classes

* Merge changes for #7335

* ! fix php 5.3 incompatibility

* Update ADAuthorizationAuthorizer.php

* Fix get_user -> getUser

* Rename AuthorizerFactory to Auth, fix interface missing functions

* Add phpdocs to all interface methods and normalize the names a bit.

* Re-work auth_test.php AD bind tests to work properly with the new class.
Reflection is not the nicest tool, but I think it is appropriate here.
Handle exceptions more nicely in auth_test.php

* Restore AD getUseList fix

Not sure how it got removed

* fix auth_test.php style

@murrant murrant deleted the murrant:ldap-update branch Nov 23, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.