Skip to content

[Ldap] Add support for sasl_bind and whoami LDAP operations #58042

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
Merged
merged 1 commit into from
Aug 30, 2024
Merged

[Ldap] Add support for sasl_bind and whoami LDAP operations #58042

merged 1 commit into from
Aug 30, 2024

Conversation

manu0401
Copy link
Contributor

Q A
Branch? 7.2
Bug fix? yes
New feature? yes
Deprecations? no
Issues
License MIT 
SASL bind let the caller supply various options, including proxy
authentication, where one user uses its own credentials to login
as another one (subjected to LDAP directory access contro usingl
authzFrom/authzTo attributes). In this case, ldapwhoami is used
to retreive the resulting authenticated and authorized DN after
bind success.

Tested with SimpleSAMLphp 2.2.2 with minor patches.

@carsonbot carsonbot added this to the 7.2 milestone Aug 20, 2024
@carsonbot

This comment was marked as resolved.

Sign in to view
stof
stof reviewed Aug 20, 2024
View reviewed changes
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

this cannot be accepted without tests

@carsonbot carsonbot changed the title Sasl bind [Ldap] Sasl bind Aug 20, 2024
@nicolas-grekas nicolas-grekas changed the title [Ldap] Sasl bind [Ldap] Add support for sasl_bind and whoami LDAP operations Aug 21, 2024
nicolas-grekas
nicolas-grekas reviewed Aug 21, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks for submitting.

nicolas-grekas
nicolas-grekas approved these changes Aug 22, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I pushed a commit with the CS fixes I had in mind.
Happy to see you here BTW @manu0401 ;)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 22, 2024 •
edited
Loading

I was wondering: would it make sense to remove the whoami method and make saslBind return it instead? Would that cover the related use cases or would that be restrictive?

@manu0401
Copy link
Contributor Author

Thank you for committing. I would leave saslBind and whoami as distinct methods. Code transitioning from native PHP function calls to Symfony will be easier to port if you stick to the original functions behavior.

fabpot
fabpot requested changes Aug 22, 2024
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Just some CS change suggestions.

@stof
Copy link
Member

stof commented Aug 22, 2024

If whoami only works after sasl_bind and is the logical next step (which I'm not sure about as I never used those), I would favor the idea of @nicolas-grekas. Adding an API for that in symfony/ldap gives us a chance to expose an API with an easier DX instead of copying the ext-ldap API.

Code won't magically be compatible with symfony/ldap and ext-ldap APIs anyway even if the method names are similar to the ext-ldap API.

@manu0401
Copy link
Contributor Author

If whoami only works after sasl_bind and is the logical next step

I am not sure it is carved into stone that authz_id cannot change without a new sasl_bind(). One could imagine that the server grants you a proxy identity for only one operation, for instance. I do not know if the use case exists, but at least it would be possible to do it.

nicolas-grekas
nicolas-grekas approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks for the discussion. Works for me as is.
I addressed the last CS comments.

@nicolas-grekas
Copy link
Member

Thank you @manu0401.

@nicolas-grekas nicolas-grekas merged commit 4c6c323 into symfony:7.2 Aug 30, 2024
6 of 10 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Aug 30, 2024
Closed
@OskarStark OskarStark changed the title [Ldap] Add support for sasl_bind and whoami LDAP operations [Ldap] Add support for sasl_bind and whoami LDAP operations Sep 2, 2024
@OskarStark OskarStark mentioned this pull request Sep 3, 2024
Merged
@fabpot fabpot mentioned this pull request Oct 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot requested changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Feature Ldap Status: Reviewed
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
6 participants
@manu0401 @carsonbot @nicolas-grekas @stof @fabpot @OskarStark