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.

Sign up for GitHub

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

Jump to bottom

Do not rely on exop_passwd #536

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Do not rely on exop_passwd #536

merged 1 commit into from
Apr 30, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Oct 17, 2022

Instead of relying on exop_passwd for password changing, check rootDSE for support and fallback to mod_replace otherwise.
This should fix AD support.

@susnux susnux added enhancement New feature or request 3. to review labels Oct 17, 2022
@susnux susnux requested review from blizzz and come-nc October 17, 2022 08:58
come-nc
come-nc requested changes Oct 17, 2022
View reviewed changes
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
@come-nc
Copy link
Collaborator

come-nc commented Oct 17, 2022

AD really does not support password EXOP?

@susnux susnux force-pushed the fix/ad-password branch 3 times, most recently from ee5b935 to 8fca3a7 Compare October 17, 2022 10:31
@susnux
Copy link
Contributor Author

susnux commented Oct 17, 2022

How about adding a dropdown setting for the password handling:

  • Default
    • Same as current handling with fallback to userPassword
  • Hash
    • Manually hash the password using ssha
  • Unicode
    • Required for AD without heuristics, setting password to unicodePwd and convert it to UTF16

@come-nc
Copy link
Collaborator

come-nc commented Oct 17, 2022

I am a bit uneasy having a fallback from passwd exop to modify on failure.
If passwd exop is advertised as supported by rootDSE upon default config it should try the exop and error out on failure.

come-nc
come-nc reviewed Oct 17, 2022
View reviewed changes
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
@susnux susnux force-pushed the fix/ad-password branch 2 times, most recently from 7564090 to 4570805 Compare October 19, 2022 13:47
@susnux
Copy link
Contributor Author

susnux commented Oct 19, 2022

@come-nc

I am a bit uneasy having a fallback from passwd exop to modify on failure. If passwd exop is advertised as supported by rootDSE upon default config it should try the exop and error out on failure.

I agree!
Now if rootDSE advertises the password exop, an error will be thrown if it does not succeed.
But if password exop is not supported the password is set by using the userPassword or (if configured) unicodePwd for AD support.

come-nc
come-nc reviewed Oct 20, 2022
View reviewed changes
lib/LDAPConnect.php Outdated Show resolved Hide resolved
@susnux susnux force-pushed the fix/ad-password branch 2 times, most recently from b1f6767 to a94c138 Compare October 20, 2022 13:30
@susnux susnux requested a review from come-nc May 20, 2023 12:18
@susnux susnux force-pushed the fix/ad-password branch 2 times, most recently from 92a1843 to 0ea6521 Compare May 20, 2023 12:37
@susnux
Copy link
Contributor Author

susnux commented May 20, 2023

@come-nc @blizzz I rebased this, is there anything I can help with to get this merged?

(Drone is failing because of #579 )

come-nc
come-nc reviewed May 22, 2023
View reviewed changes
lib/LDAPConnect.php Outdated Show resolved Hide resolved
lib/LDAPConnect.php Outdated Show resolved Hide resolved
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
lib/LDAPUserManager.php Outdated Show resolved Hide resolved
lib/LDAPUserManager.php Outdated
if ($this->configuration->useUnicodePassword()) {
$entry['unicodePwd'] = iconv('UTF-8', 'UTF-16LE', '"' . $password . '"');
} else {
$entry['userPassword'] = $password;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want to hash this? But then it opens a can of worms of which hash to use and so on…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are implementations hashing it automatically if not hashed already, so not sure if we want to go into that rabbit hole of choosing a hash algorithm.

@susnux
Copy link
Contributor Author

susnux commented May 22, 2023

@come-nc fixed your comments

@susnux susnux requested a review from come-nc May 22, 2023 14:49
@Fmstrat
Copy link

Fmstrat commented May 30, 2023

Any idea when this PR will make it's way through? Debating on handling via a custom branch or not.

@blizzz
Copy link
Member

blizzz commented Sep 22, 2023

CI is failing

@susnux susnux force-pushed the fix/ad-password branch 3 times, most recently from 7e415f8 to 0b64829 Compare March 23, 2024 22:20
@susnux susnux mentioned this pull request Mar 23, 2024
Open
@susnux susnux force-pushed the fix/ad-password branch 3 times, most recently from cb453b4 to a680dba Compare April 29, 2024 23:13
@susnux
Copy link
Contributor Author

susnux commented Apr 29, 2024

@blizzz

CI is failing

I was able to finally solve this (was a typo 😅 )

@susnux
If the passwd exop is not supported allow setting unicodePwd for AD
fc56e13 
* Do not rely on exop_passwd but check rootDSE for support and fallback to mod_replace

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux enabled auto-merge April 30, 2024 13:28
@susnux susnux merged commit b454951 into main Apr 30, 2024
26 checks passed
@susnux susnux deleted the fix/ad-password branch April 30, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc approved these changes

@blizzz blizzz Awaiting requested review from blizzz

Assignees
No one assigned
Labels
3. to review enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ldap_modify() instead of ldap_exop_passwd()
4 participants
@susnux @come-nc @Fmstrat @blizzz