Skip to content

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

Closed
Fahrplan wants to merge 1 commit into
BookStackApp:masterfrom
Fahrplan:patch-1
Closed

Bugfix: ldap_connect() ignores argument when given an URI#1278
Fahrplan wants to merge 1 commit into
BookStackApp:masterfrom
Fahrplan:patch-1

Conversation

@Fahrplan

@Fahrplan Fahrplan commented Feb 14, 2019

Copy link
Copy Markdown

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.

@Fahrplan
Bugfix: ldap_connect() ignores argument when given an URI
0fc51b5 
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

ssddanbrown commented Feb 16, 2019

Copy link
Copy Markdown
Member

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:

BookStack/app/Auth/Access/LdapService.php

Lines 166 to 183 in 25bc28a

$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

Fahrplan commented Feb 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

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
@ssddanbrown
Updated ldap server option parsing to work with protocol and port
c247640 
- 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

ssddanbrown commented Apr 16, 2019

Copy link
Copy Markdown
Member

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!

@ssddanbrown ssddanbrown mentioned this pull request Apr 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

v0.26.0

Development

Successfully merging this pull request may close these issues.

2 participants

@Fahrplan @ssddanbrown