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

Added support for not canonicalizing the SASL realm on sasl_binds for LDAP #382

Closed
wants to merge 1 commit into from

Conversation

pwillred
Copy link

@pwillred pwillred commented Jul 9, 2013

This patch adds an additional parameter to ldap_sasl_bind in the ldap extension to allow developers to avoid canonicalizing the realm during SASL binds.

Currently, php always canonicalizes the hostname for SASL binds. If reverse DNS is not properly set up or cannot be modified by the site administrator, this can result in SASL attempting to make requests using an incorrect realm. In particular, when using GSSAPI, the server will ignore the SASL realm option and delegate that to GSSAPI. This can result in GSSAPI attempting to acquire a cross realm ticket to a nonexistant realm, resulting in the bind failing with a "Local Error" from GSSAPI.

Similar functionality can be found in the "-N" option in ldapsearch from OpenLDAP.

… LDAP,

similar to the -N flag for ldapsearch.
@pwillred
Copy link
Author

The Travis CI build failures appear to be due to failing tests in modules that are not ldap; I believe the issues it is reporting are unrelated to the above commit. If I am mistaken, please let me know and I will attempt to repair the bug.

@smalyshev
Copy link
Contributor

This function already has 8 params. At some point one has to admit something is going wrong here and convert it to $options...

@pwillred
Copy link
Author

Can't really argue with that sentiment. The initial patch was designed to change as little as possible in the function.

Two additional branches have been created in pwillred/php-src: ldap_sasl_options_back and ldap_sasl_options. The ldap_sasl_options branch accepts two arguments, the required link and an options array where keys equal the parameter names from ldap_sasl_bind as it currently exists.

ldap_sasl_options_back accepts the same arguments as above, but is backwards compatible with the current version. The second argument is either an array as described above, or a string for the bind dn. Options in the array trump options passed as old style arguments.

How do the revised patches look?

@weltling
Copy link
Contributor

Independent from that options thing, where is the test? :)

@php-pulls
Copy link

Comment on behalf of krakjoe at php.net:

Since this PR has unsatisfactory test coverage, and since the author seems to have abandoned working on it, and since the proper solution would require a different patch and probably an RFC, I'm closing this PR.

If the author is listening, please take this action as encouragement to create an RFC and start a discussion about these changes.

Please ensure any future PR has satisfactory test coverage, and those tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants