Skip to content
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

ldap_get_attributes does not lowercase keys and breaks with AD #6375

Closed
auroraeosrose opened this issue Oct 30, 2018 · 4 comments
Closed

ldap_get_attributes does not lowercase keys and breaks with AD #6375

auroraeosrose opened this issue Oct 30, 2018 · 4 comments
Labels
ldap :octocat: help-wanted 👩‍💻 ready for dev These issues are ready for someone to work on them - take your pick!

Comments

@auroraeosrose
Copy link

Please confirm you have done the following before posting your bug report:

Describe the bug

This is actually an issue with the PHP ldap extension
Although ldap_get_entries intelligently lower cases indexes from attributes returned, ldap_get_attributes does not. I'd argue this is actually a bug in PHP but - backwards compatibility (ugh) so we'll never get it "fixed"

This means that in order to get ldap logins working with autocreating users on first login, you need to used cased settings

untitled

However, if you used case settings... synchronize doesn't work because that code uses ldap_get_entities which requires the LOWER CASE version of all those keys

Now, I need a beer after finding this today and yesterday

Fix is actually stupidly simple, I might have a chance to do a PR later this week unless someone else is faster

in findAndBindUserLdap in app/Models/Ldap.php change the return line to be

return array_change_key_case($user);

This will verify our attributes are always cased lower
Also I think this will fix at least 10 related ldap issues I'm seeing in bugs here which all seem to have the same "invalid username and password" as the symptom (I had to dig REALLY deep to find the real bug)

Server (please complete the following information):

  • Snipe-IT Version v4.6.6 - build 3926 (master)
  • OS: Windows Server 2016 Standard
  • Web Server: IIS 10
  • PHP Version 7.2.11

Yes, I like shiny

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser firefox
  • Version 63.0
@snipe
Copy link
Owner

snipe commented Oct 30, 2018

Hi @auroraeosrose - long time no see :)

Thanks for the heads up - I'll patch this to master, but we're in the middle of a PR for v5 that will effectively rewrite a lot of the guts of how LDAP works so I need to dig deeper into that PR to see if we need to patch it forward as well.

@snipe snipe closed this as completed in f744696 Oct 30, 2018
@snipe
Copy link
Owner

snipe commented Oct 30, 2018

I believe we use both cases, since authenticating and syncing don't use the same methods, and it's in the docs where to use which case. We switched to that way to make it so that you didn't need to first authenticate with a user who has search capabilities (which is stored in your LDAP settings) and then attempt the login when a user tries to login with LDAP/AD (because people complained, and also because it's a weird step to have to go through for just logging in), but it introduced the issue you're describing. Damned if you do, damned if you don't.

I could also be totally misremembering the situation though, as it was a few years and a few thousand commits ago.

@auroraeosrose
Copy link
Author

Using both is fine, this is just a gotcha in the ldap extension api ... thanks for the quick fix!

@seunis
Copy link

seunis commented Dec 27, 2018

Please confirm you have done the following before posting your bug report:

Describe the bug

This is actually an issue with the PHP ldap extension
Although ldap_get_entries intelligently lower cases indexes from attributes returned, ldap_get_attributes does not. I'd argue this is actually a bug in PHP but - backwards compatibility (ugh) so we'll never get it "fixed"

This means that in order to get ldap logins working with autocreating users on first login, you need to used cased settings

untitled

However, if you used case settings... synchronize doesn't work because that code uses ldap_get_entities which requires the LOWER CASE version of all those keys

Now, I need a beer after finding this today and yesterday

Fix is actually stupidly simple, I might have a chance to do a PR later this week unless someone else is faster

in findAndBindUserLdap in app/Models/Ldap.php change the return line to be

return array_change_key_case($user);

This will verify our attributes are always cased lower
Also I think this will fix at least 10 related ldap issues I'm seeing in bugs here which all seem to have the same "invalid username and password" as the symptom (I had to dig REALLY deep to find the real bug)

Server (please complete the following information):

  • Snipe-IT Version v4.6.6 - build 3926 (master)
  • OS: Windows Server 2016 Standard
  • Web Server: IIS 10
  • PHP Version 7.2.11

Yes, I like shiny

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser firefox
  • Version 63.0

Hello I am very new to this forum and also Linux

I was able to get my LDAP to communicate with setup but currently my users are not able to login.
I fee a fix on this page but I have no idea how to get to that path "in findAndBindUserLdap in app/Models/Ldap.php change the return line to be"

@snipe snipe added ldap 👩‍💻 ready for dev These issues are ready for someone to work on them - take your pick! :octocat: help-wanted and removed ldap labels Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ldap :octocat: help-wanted 👩‍💻 ready for dev These issues are ready for someone to work on them - take your pick!
Projects
None yet
Development

No branches or pull requests

3 participants