BGmot / zabbix

This repository is a fork of official Zabbix repository https://github.com/zabbix/zabbix.git. See README why it was forked.
GNU General Public License v2.0
31 stars 8 forks source link

Issue with 'Case sensitive login' option #19

Closed Shaman0S closed 2 years ago

Shaman0S commented 2 years ago

I have patched Zabbix 5.4 successfully with https://github.com/BGmot/zabbix/blob/release/5.4-bg/bg-scripts/bg-features-install.sh

Once done have amended existing users so they correspond with AD sAMAccountName attribute. In LDAP Setting the checkbox 'Case sensitive login' was set and all existing users were able to login. However if a user tried to log in with different capitalization, e.g. john.doe instead of John.Doe this created a new user john.doe in Zabbix.

To avoid user duplication I have unset 'Case sensitive login' checkbox. After that UI denied to login at all with error in frontend like

SQL statement execution has failed "INSERT INTO users (username,name,surname,url,passwd,roleid,userid) VALUES ('John.Doe','John Doe','','','23412b4ea46a7432c21eefb90f1354fb','2','258')".

and logline in error.log

[Thu Apr 28 22:44:44.441631 2022] [php7:notice] [pid 16654] [client 192.168.0.2:63530] PHP Notice:  Undefined variable: fields in /var/www/html/zabbix/include/classes/api/services/CUser.php on line 1454, referer: https://my.zabbix.system/zabbix/index.php
[Thu Apr 28 22:44:44.441741 2022] [php7:warn] [pid 16654] [client 192.168.0.2:63530] PHP Warning:  implode(): Invalid arguments passed in /var/www/html/zabbix/include/classes/api/services/CUser.php on line 1454, referer: https://my.zabbix.system/zabbix/index.php
BGmot commented 2 years ago

I see the issue. 5.4 is no longer supported, usually I don't release new versions for not supported branches. Are you going to upgrade to 6.x soon or want me to fix 5.4?

BGmot commented 2 years ago

Should be fixed by https://github.com/BGmot/zabbix/commit/64787eac0f64fad23bf2b11f7c71aa4c5cfe387a Please try again updated code https://github.com/BGmot/zabbix/tree/5.4.12-bg

Shaman0S commented 2 years ago

Thank you! Will test next Monday first thing.

BGmot commented 2 years ago

Just to clarify: 1) all duplicates need to be removed from Users 2) now duplicates will not be created and if "Case sensitive" is checked then authentication will just fail if the case of letters does not match to what's in LDAP server.

Shaman0S commented 2 years ago

I have applied the patch. It does not allow to login at all. Looking to the code line 236. $result[0]['cn'][0] != $user This is OK for OpenLDAP, but for other implementations where cn does not equal to uid I believe that fails.

I think it needs to get an additional field defined in cnf['search_attribute'] (sAMAccountName in my case) from ldap server in line 231 and then compare that field with $user

BGmot commented 2 years ago

oh right... I am not able to test it with ActiveDirectory LDAP. Will try to fix it tonight and let you test it. Thanks for you for the feedback.

BGmot commented 2 years ago

Should be fixed by https://github.com/BGmot/zabbix/commit/b08c967352f72e8326cdc5f73feb0986c435d236 Please test.

Shaman0S commented 2 years ago

Hi, have tested it and it still does not allow to login.

Have created a pull request for the fix that works

BGmot commented 2 years ago

I tested my code with Windows Active Directory and it worked well. I can't merge your code for the reason mentioned in PR. If you are willing to troubleshoot further please add this after line 232 in ui/include/classes/ldap/CLdap.php

sdff($result);

then try to login when it fails and search for file zabbix.log under /tmp folder. Post what you have in the file here (removing all sensitive info of course).

Shaman0S commented 2 years ago
array('count'=>1,0=>array (
  'cn' =>
  array (
    'count' => 1,
    0 => 'John Doe',
  ),
  0 => 'cn',
  'memberof' =>
  array (
    'count' => 2,
    0 => 'CN=Group1,OU=Groups,DC=example,DC=com',
    1 => 'CN=Group2,OU=Groups,DC=example,DC=com',
  ),
  1 => 'memberof',
  'mail' =>
  array (
    'count' => 1,
    0 => 'John.Doe@example.com',
  ),
  2 => 'mail',
  'count' => 3,
  'dn' => 'CN=John Doe,OU=Users,DC=example,DC=com',
))
BGmot commented 2 years ago

It's not from AD it looks like it's from OpenLDAP.

Shaman0S commented 2 years ago

Well, in b08c967 line 231 it returns attributes as defined which is ['cn','memberof', 'mail'] and nothing that would be $this->cnf['search_attribute'].

Additionally in error.log

[Mon May 30 15:38:45.047803 2022] [php7:notice] [pid 26029] [client 192.168.0.2:53009] PHP Notice:  Undefined index: samaccountname in /var/www/html/zabbix/include/classes/ldap/CLdap.php on line 243, referer: https://my.zabbix.system/zabbix/index.php
BGmot commented 2 years ago

Indeed, wonder how it worked with my AD. Let me test more tonight.

BGmot commented 2 years ago

My apologies, my code I tested with is a bit different. Please try with this commit in place https://github.com/BGmot/zabbix/commit/4e3574524d544ed494c6193cbf59fd4131de90e6

Shaman0S commented 2 years ago

Tested latest with different options and cases. Works as expected. Thanks for the fix.