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

Bugfix: ldap_connect() ignores argument when given an URI #1278

Closed
wants to merge 1 commit into from

Conversation

2 participants
@Fahrplan
Copy link

commented Feb 14, 2019

ldap_connect() ignores the port argument, if $hostname is in the Form of an ldap-URI (like ldaps://....), therefore the $port must be added to the first argument, if an URI is present, otherwise it allways tries to use Port 636.

This is a quick fix, which probably needs some further adjustment.

Bugfix: ldap_connect() ignores argument when given an URI
ldap_connect() ignores the port argument, if $hostname is in the Form of an ldap-URI (like ldaps://....), therefore the $port must be added to the first argument, if an URI is present, otherwise it allways tries to use Port 636.

This is a quick fix, which probably needs some further adjustment.
@ssddanbrown

This comment has been minimized.

Copy link
Member

commented Feb 16, 2019

Hi @Fahrplan,
Thanks for offering a pull request.

The file you have edited is really only supposed to be a thin wrapper around the core PHP functions so I'd prefer to not add any logic in that file.

There should already be logic to handle this:

$ldapServer = explode(':', $this->config['server']);
$hasProtocol = preg_match('/^ldaps{0,1}\:\/\//', $this->config['server']) === 1;
if (!$hasProtocol) {
array_unshift($ldapServer, '');
}
$hostName = $ldapServer[0] . ($hasProtocol?':':'') . $ldapServer[1];
$defaultPort = $ldapServer[0] === 'ldaps' ? 636 : 389;
/*
* Check if TLS_INSECURE is set. The handle is set to NULL due to the nature of
* the LDAP_OPT_X_TLS_REQUIRE_CERT option. It can only be set globally and not
* per handle.
*/
if($this->config['tls_insecure']) {
$this->ldap->setOption(NULL, LDAP_OPT_X_TLS_REQUIRE_CERT, LDAP_OPT_X_TLS_NEVER);
}
$ldapConnection = $this->ldap->connect($hostName, count($ldapServer) > 2 ? intval($ldapServer[2]) : $defaultPort);

Does the existing code not handle your use-case? If not, please can you confirm the format of your LDAP_SERVER variable?

@Fahrplan

This comment has been minimized.

Copy link
Author

commented Feb 16, 2019

Hi @ssddanbrown,
thank you for your response. After digging into this I got the following:

My LDAP_SERVER variable looks like this:

LDAP_SERVER=ldaps://10.1.1.2:10636

(Our LDAP Server runs on a different port then default AND uses LDAPS-Encryption).

The logic quoted seems incorrect, because ldap_connect($host, $port) ignores the $port argument, when a protocol is given in the $host part, see http://php.net/manual/en/function.ldap-connect.php

When I feed my LDAP_SERVER string through the logic above the $ldapServer variable ends up with:

Array ( [0] => ldaps [1] => //10.1.1.2 [2] => 10636 )

and the $hostname variable is filled with ldaps://10.1.1.2

After that ldap_connect() is called with the first argument, it tries to connect to 10.1.1.2:389, because the port argument gets ignored when a protocol is present.

@ssddanbrown ssddanbrown added this to the v0.26.0 milestone Apr 16, 2019

ssddanbrown added a commit that referenced this pull request Apr 16, 2019

Updated ldap server option parsing to work with protocol and port
- Aligns with PHP behaviour where ports is ignore for full LDAP URI.
- Added tests to check format being passed to LDAP is as expected.
- May be related to #1220
- Related to #1386 and #1278
@ssddanbrown

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Thanks @Fahrplan for explaining the above, Your description made the issue clear. I was not aware of this PHP functionality.

I've just commited c247640 to update the above & simplify the logic, while fixing this issue. Therefore will close this pull request.
Thank you for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.