LDAP wizard 'Could not connect to LDAP' #20020

Closed
Xenopathic opened this Issue Oct 25, 2015 · 41 comments

Comments

Projects
None yet
@Xenopathic
Member

Xenopathic commented Oct 25, 2015

I upgraded my ownCloud instance from 8.1.3 to 8.2.0, with a configured LDAP server (which worked). Today I wanted to change one of the LDAP settings, but the wizard breaks with a 'Could not connect to LDAP' error. All users and groups continue to display correctly though. The following is seen in the logs:

Oct 25 11:12:57 atlas.lan.mccorkell.me.uk ownCloud[32387]: {PHP} ldap_connect(): Could not create session handle: Bad parameter to an ldap routine at /usr/share/webapps/owncloud/apps/user_ldap/lib/ldap.php#257
Oct 25 11:12:57 atlas.lan.mccorkell.me.uk ownCloud[32387]: {PHP} ldap_set_option(): supplied argument is not a valid ldap link resource at /usr/share/webapps/owncloud/apps/user_ldap/lib/ldap.php#257
Oct 25 11:12:57 atlas.lan.mccorkell.me.uk ownCloud[32387]: {PHP} ldap_set_option(): supplied argument is not a valid ldap link resource at /usr/share/webapps/owncloud/apps/user_ldap/lib/ldap.php#257
Oct 25 11:12:57 atlas.lan.mccorkell.me.uk ownCloud[32387]: {PHP} ldap_set_option(): supplied argument is not a valid ldap link resource at /usr/share/webapps/owncloud/apps/user_ldap/lib/ldap.php#257

LDAP configuration:

+-------------------------------+--------------------------------------------+
| Configuration                 |                                            |
+-------------------------------+--------------------------------------------+
| hasMemberOfFilterSupport      |                                            |
| hasPagedResultSupport         |                                            |
| homeFolderNamingRule          |                                            |
| lastJpegPhotoLookup           | 0                                          |
| ldapAgentName                 |                                            |
| ldapAgentPassword             | ***                                        |
| ldapAttributesForGroupSearch  |                                            |
| ldapAttributesForUserSearch   |                                            |
| ldapBackupHost                |                                            |
| ldapBackupPort                |                                            |
| ldapBase                      | dc=mccorkell,dc=me,dc=uk                   |
| ldapBaseGroups                | ou=groups,dc=mccorkell,dc=me,dc=uk         |
| ldapBaseUsers                 | ou=users,dc=mccorkell,dc=me,dc=uk          |
| ldapCacheTTL                  | 600                                        |
| ldapConfigurationActive       | 1                                          |
| ldapEmailAttribute            | mail                                       |
| ldapExperiencedAdmin          | 0                                          |
| ldapExpertUUIDGroupAttr       |                                            |
| ldapExpertUUIDUserAttr        |                                            |
| ldapExpertUsernameAttr        | uid                                        |
| ldapGroupDisplayName          | cn                                         |
| ldapGroupFilter               | (&(|(objectclass=posixGroup)))             |
| ldapGroupFilterGroups         |                                            |
| ldapGroupFilterMode           | 0                                          |
| ldapGroupFilterObjectclass    | posixGroup                                 |
| ldapGroupMemberAssocAttr      | memberUid                                  |
| ldapHost                      | zeus.lan.mccorkell.me.uk                   |
| ldapIgnoreNamingRules         |                                            |
| ldapLoginFilter               | (&(|(objectclass=posixAccount))(uid=%uid)) |
| ldapLoginFilterAttributes     |                                            |
| ldapLoginFilterEmail          | 0                                          |
| ldapLoginFilterMode           | 0                                          |
| ldapLoginFilterUsername       | 1                                          |
| ldapNestedGroups              | 0                                          |
| ldapOverrideMainServer        | 0                                          |
| ldapPagingSize                | 500                                        |
| ldapPort                      | 389                                        |
| ldapQuotaAttribute            |                                            |
| ldapQuotaDefault              |                                            |
| ldapTLS                       | 0                                          |
| ldapUserDisplayName           | cn                                         |
| ldapUserFilter                | (|(objectclass=posixAccount))              |
| ldapUserFilterGroups          |                                            |
| ldapUserFilterMode            | 0                                          |
| ldapUserFilterObjectclass     | posixAccount                               |
| ldapUuidGroupAttribute        | auto                                       |
| ldapUuidUserAttribute         | auto                                       |
| turnOffCertCheck              | 0                                          |
| useMemberOfToDetectMembership | 0                                          |
+-------------------------------+--------------------------------------------+

cc @blizzz

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 26, 2015

Member

@Xenopathic are you able to reproduce this with an instance installed from scratch ? (just to confirm / evaluate severity, I haven't tried it myself)

Member

PVince81 commented Oct 26, 2015

@Xenopathic are you able to reproduce this with an instance installed from scratch ? (just to confirm / evaluate severity, I haven't tried it myself)

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 26, 2015

Member

Yes, just reproduced with the latest master (although it technically wasn't a clean instance, but the LDAP configuration was completely clean)

Member

Xenopathic commented Oct 26, 2015

Yes, just reproduced with the latest master (although it technically wasn't a clean instance, but the LDAP configuration was completely clean)

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 26, 2015

Member

Now I'm slightly concerned, I'm reproducing this on 8.1.0 as well...

Member

Xenopathic commented Oct 26, 2015

Now I'm slightly concerned, I'm reproducing this on 8.1.0 as well...

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 26, 2015

Member

Oh ffs PHP, it's an upstream regression: https://dev.icinga.org/issues/9298 https://www.netways.org/issues/2931

Basically, they removed support for host:port in ldap_connect()

Member

Xenopathic commented Oct 26, 2015

Oh ffs PHP, it's an upstream regression: https://dev.icinga.org/issues/9298 https://www.netways.org/issues/2931

Basically, they removed support for host:port in ldap_connect()

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@LukasReschke

LukasReschke Oct 26, 2015

Member

So we need to block PHP 5.6.11 for the LDAP app? Though then again that would probably mess with custom backports of distributions. Bloody stuff 🙊

We should at least make a note somewhere. Or can we fix/work-around this? The Netways link seems to indicate this?

cc @cmonteroluque

Member

LukasReschke commented Oct 26, 2015

So we need to block PHP 5.6.11 for the LDAP app? Though then again that would probably mess with custom backports of distributions. Bloody stuff 🙊

We should at least make a note somewhere. Or can we fix/work-around this? The Netways link seems to indicate this?

cc @cmonteroluque

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Oct 26, 2015

Member

We need to understand how many distributions ship the affected PHP version. Let's do release note for now @carlaschroder

Member

karlitschek commented Oct 26, 2015

We need to understand how many distributions ship the affected PHP version. Let's do release note for now @carlaschroder

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 26, 2015

Member

@LukasReschke @karlitschek It's not just 5.6.11, I'm running 5.6.14 (yay Arch!) and getting this issue. It's a regression in that it's worked before and no longer works, but I'm not familiar enough with php-ldap to say if it was ever designed to work like this...

Member

Xenopathic commented Oct 26, 2015

@LukasReschke @karlitschek It's not just 5.6.11, I'm running 5.6.14 (yay Arch!) and getting this issue. It's a regression in that it's worked before and no longer works, but I'm not familiar enough with php-ldap to say if it was ever designed to work like this...

@kalletabur

This comment has been minimized.

Show comment
Hide comment
@kalletabur

kalletabur Oct 27, 2015

Having same issues.
Web gui under LDAP Users "Only these object classes" and "Only from these groups" are grayed out and only manual LDAP query is available. Server settings are as usual and on commandline is verified that LDAP server can be connected and port opened.

OwnCloud version: 8.1.3.0
PHP 5.6.13+dfsg-0+deb8u1
Debian 8.2

"reqId":"reqId","remoteAddr":"10.10.10.10","app":"PHP","message":"ldap_connect(): Could not create session handle: Bad parameter to an ldap routine at \/var\/www\/owncloud\/apps\/user_ldap\/lib\/ldap.php#257","level":3,"time":"2015-10-27T09:51:23+00:00"}
{"reqId":"reqId","remoteAddr":"10.10.10.10","app":"PHP","message":"ldap_set_option(): supplied argument is not a valid ldap link resource at \/var\/www\/owncloud\/apps\/user_ldap\/lib\/ldap.php#257","level":3,"time":"2015-10-27T09:51:23+00:00"}
{"reqId":"reqId","remoteAddr":"10.10.10.10","app":"PHP","message":"ldap_set_option(): supplied argument is not a valid ldap link resource at \/var\/www\/owncloud\/apps\/user_ldap\/lib\/ldap.php#257","level":3,"time":"2015-10-27T09:51:23+00:00"}
{"reqId":"reqId","remoteAddr":"10.10.10.10","app":"PHP","message":"ldap_set_option(): supplied argument is not a valid ldap link resource at \/var\/www\/owncloud\/apps\/user_ldap\/lib\/ldap.php#257","level":3,"time":"2015-10-27T09:51:23+00:00"}

Having same issues.
Web gui under LDAP Users "Only these object classes" and "Only from these groups" are grayed out and only manual LDAP query is available. Server settings are as usual and on commandline is verified that LDAP server can be connected and port opened.

OwnCloud version: 8.1.3.0
PHP 5.6.13+dfsg-0+deb8u1
Debian 8.2

"reqId":"reqId","remoteAddr":"10.10.10.10","app":"PHP","message":"ldap_connect(): Could not create session handle: Bad parameter to an ldap routine at \/var\/www\/owncloud\/apps\/user_ldap\/lib\/ldap.php#257","level":3,"time":"2015-10-27T09:51:23+00:00"}
{"reqId":"reqId","remoteAddr":"10.10.10.10","app":"PHP","message":"ldap_set_option(): supplied argument is not a valid ldap link resource at \/var\/www\/owncloud\/apps\/user_ldap\/lib\/ldap.php#257","level":3,"time":"2015-10-27T09:51:23+00:00"}
{"reqId":"reqId","remoteAddr":"10.10.10.10","app":"PHP","message":"ldap_set_option(): supplied argument is not a valid ldap link resource at \/var\/www\/owncloud\/apps\/user_ldap\/lib\/ldap.php#257","level":3,"time":"2015-10-27T09:51:23+00:00"}
{"reqId":"reqId","remoteAddr":"10.10.10.10","app":"PHP","message":"ldap_set_option(): supplied argument is not a valid ldap link resource at \/var\/www\/owncloud\/apps\/user_ldap\/lib\/ldap.php#257","level":3,"time":"2015-10-27T09:51:23+00:00"}
@carlaschroder

This comment has been minimized.

Show comment
Hide comment
@carlaschroder

carlaschroder Oct 27, 2015

Sigh PHP. One expects this sort of behavior from an infant, not from a mature, essential piece of infrastructure. So the release note says "PHP 5.6.11 breaks our nice LDAP wizard, but you can still do stuff via CLI."

Sigh PHP. One expects this sort of behavior from an infant, not from a mature, essential piece of infrastructure. So the release note says "PHP 5.6.11 breaks our nice LDAP wizard, but you can still do stuff via CLI."

@carlaschroder

This comment has been minimized.

Show comment
Hide comment
@carlaschroder

carlaschroder Oct 27, 2015

Sorry, that was a question. moar coffee.

Sorry, that was a question. moar coffee.

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 27, 2015

Member

@carlaschroder That sounds about right, but make sure you say PHP 5.6.11+, since all newer versions in the 5.6 branch are affected. And as far as I can tell no fix is in sight upstream

Member

Xenopathic commented Oct 27, 2015

@carlaschroder That sounds about right, but make sure you say PHP 5.6.11+, since all newer versions in the 5.6 branch are affected. And as far as I can tell no fix is in sight upstream

@carlaschroder

This comment has been minimized.

Show comment
Hide comment

ok thanks @Xenopathic

@cmonteroluque

This comment has been minimized.

Show comment
Hide comment
Contributor

cmonteroluque commented Oct 28, 2015

@carlaschroder

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 28, 2015

Member

@cmonteroluque @carlaschroder Note, this seems to affect 8.1 as well, perhaps even earlier versions.

Member

Xenopathic commented Oct 28, 2015

@cmonteroluque @carlaschroder Note, this seems to affect 8.1 as well, perhaps even earlier versions.

@carlaschroder

This comment has been minimized.

Show comment
Hide comment
@carlaschroder

carlaschroder Oct 28, 2015

Gottit @Xenopathic, all the way back to 7.0.

Gottit @Xenopathic, all the way back to 7.0.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Oct 29, 2015

Member

Setting to critical, I hope we can find a workaround until upstream is fixed (if ever, unless it was a mistake instead of a design choice?)

Member

PVince81 commented Oct 29, 2015

Setting to critical, I hope we can find a workaround until upstream is fixed (if ever, unless it was a mistake instead of a design choice?)

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

In PHPs release notes this change was not mentioned. So, no idea whether it was intentional or by chance. I posted a questions in the PHP bug tracker: https://bugs.php.net/bug.php?id=69471

Contributor

blizzz commented Oct 29, 2015

In PHPs release notes this change was not mentioned. So, no idea whether it was intentional or by chance. I posted a questions in the PHP bug tracker: https://bugs.php.net/bug.php?id=69471

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

@Xenopathic i don't have such a PHP version available, yet, could you try to see whether

diff --git a/apps/user_ldap/lib/connection.php b/apps/user_ldap/lib/connection.php
index f6b123b..7dced6d 100644
--- a/apps/user_ldap/lib/connection.php
+++ b/apps/user_ldap/lib/connection.php
@@ -564,10 +564,6 @@ class Connection extends LDAPUtility {
        if(empty($host)) {
            return false;
        }
-       if(strpos($host, '://') !== false) {
-           //ldap_connect ignores port parameter when URLs are passed
-           $host .= ':' . $port;
-       }
        $this->ldapConnectionRes = $this->ldap->connect($host, $port);
        if($this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_PROTOCOL_VERSION, 3)) {
            if($this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_REFERRALS, 0)) {
diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php
index a819b2e..198194d 100644
--- a/apps/user_ldap/lib/wizard.php
+++ b/apps/user_ldap/lib/wizard.php
@@ -1038,8 +1038,6 @@ class Wizard extends LDAPUtility {
        if(isset($hostInfo['scheme'])) {
            if(isset($hostInfo['port'])) {
                //problem
-           } else {
-               $host .= ':' . $port;
            }
        }
        \OCP\Util::writeLog('user_ldap', 'Wiz: Attempting to connect ', \OCP\Util::DEBUG);
@@ -1291,7 +1289,7 @@ class Wizard extends LDAPUtility {
            return $this->cr;
        }
        $cr = $this->ldap->connect(
-           $this->configuration->ldapHost.':'.$this->configuration->ldapPort,
+           $this->configuration->ldapHost,
            $this->configuration->ldapPort);

        $this->ldap->setOption($cr, LDAP_OPT_PROTOCOL_VERSION, 3);

works?

I am pretty sure this will break on older PHP versions when using ldap(s):// protocols and not 389 port.

Contributor

blizzz commented Oct 29, 2015

@Xenopathic i don't have such a PHP version available, yet, could you try to see whether

diff --git a/apps/user_ldap/lib/connection.php b/apps/user_ldap/lib/connection.php
index f6b123b..7dced6d 100644
--- a/apps/user_ldap/lib/connection.php
+++ b/apps/user_ldap/lib/connection.php
@@ -564,10 +564,6 @@ class Connection extends LDAPUtility {
        if(empty($host)) {
            return false;
        }
-       if(strpos($host, '://') !== false) {
-           //ldap_connect ignores port parameter when URLs are passed
-           $host .= ':' . $port;
-       }
        $this->ldapConnectionRes = $this->ldap->connect($host, $port);
        if($this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_PROTOCOL_VERSION, 3)) {
            if($this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_REFERRALS, 0)) {
diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php
index a819b2e..198194d 100644
--- a/apps/user_ldap/lib/wizard.php
+++ b/apps/user_ldap/lib/wizard.php
@@ -1038,8 +1038,6 @@ class Wizard extends LDAPUtility {
        if(isset($hostInfo['scheme'])) {
            if(isset($hostInfo['port'])) {
                //problem
-           } else {
-               $host .= ':' . $port;
            }
        }
        \OCP\Util::writeLog('user_ldap', 'Wiz: Attempting to connect ', \OCP\Util::DEBUG);
@@ -1291,7 +1289,7 @@ class Wizard extends LDAPUtility {
            return $this->cr;
        }
        $cr = $this->ldap->connect(
-           $this->configuration->ldapHost.':'.$this->configuration->ldapPort,
+           $this->configuration->ldapHost,
            $this->configuration->ldapPort);

        $this->ldap->setOption($cr, LDAP_OPT_PROTOCOL_VERSION, 3);

works?

I am pretty sure this will break on older PHP versions when using ldap(s):// protocols and not 389 port.

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 29, 2015

Member

@blizzz Yes, that patch fixes things, the wizard works as expected.

Member

Xenopathic commented Oct 29, 2015

@blizzz Yes, that patch fixes things, the wizard works as expected.

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

@Xenopathic good. Meanwhile I confirmed, it fails with PHP 5.6.4 i.e. PHP < 5.6.11. Bollocks.

Contributor

blizzz commented Oct 29, 2015

@Xenopathic good. Meanwhile I confirmed, it fails with PHP 5.6.4 i.e. PHP < 5.6.11. Bollocks.

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

So which options do we have?

  • sniffing PHP version: error prone, distributions might have for some reason backported the patch, or excluded it.
  • trying to bind host URL first without, then with port. If later works, add the port to the host and save like this. This will break in the future when PHP version is upgraded. Might look odd to admin as well in the wizard.
  • trying to bind host URL first with port, if it fails then without port. If it works with port, set a marker to always to it. If, it some point it fails in production, try without the port. If it works, an PHP update was done, reset the marker. Could be attempted in both directions to be backward compatible (i.e. move to a host with an older PHP version).

We can simplify it again once PHP 5.6 meets the grim reaper.

So far only the third option does really make sense, however this requires heavier changes as well. Opinions?

Contributor

blizzz commented Oct 29, 2015

So which options do we have?

  • sniffing PHP version: error prone, distributions might have for some reason backported the patch, or excluded it.
  • trying to bind host URL first without, then with port. If later works, add the port to the host and save like this. This will break in the future when PHP version is upgraded. Might look odd to admin as well in the wizard.
  • trying to bind host URL first with port, if it fails then without port. If it works with port, set a marker to always to it. If, it some point it fails in production, try without the port. If it works, an PHP update was done, reset the marker. Could be attempted in both directions to be backward compatible (i.e. move to a host with an older PHP version).

We can simplify it again once PHP 5.6 meets the grim reaper.

So far only the third option does really make sense, however this requires heavier changes as well. Opinions?

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 29, 2015

Member

How expensive is an ldap_connect() that breaks like this? I suspect it's much less expensive than a DB query, so I'd suggest just trying the 5.6.5+ method (aka the patch posted above), and if that fails, then try the old way.

Member

Xenopathic commented Oct 29, 2015

How expensive is an ldap_connect() that breaks like this? I suspect it's much less expensive than a DB query, so I'd suggest just trying the 5.6.5+ method (aka the patch posted above), and if that fails, then try the old way.

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

@Xenopathic ldap_connect() does not tell you anything until you attempt an ldap_bind(). All in all not too expensive, since the error should directly come from PHP.

Could you try running this (well, adjusted to your LDAP) in a PHP console:

$cr = ldap_connect('ldap://ldap.owncloud.bzoc:7770', 389);
var_dump($cr);
var_dump(ldap_errno($cr));
$status = ldap_bind($cr, 'uid=owncloudagent,ou=Users,dc=owncloud,dc=bzoc', '123456');
var_dump($cr);
var_dump(ldap_errno($cr));

and paste the output?

Contributor

blizzz commented Oct 29, 2015

@Xenopathic ldap_connect() does not tell you anything until you attempt an ldap_bind(). All in all not too expensive, since the error should directly come from PHP.

Could you try running this (well, adjusted to your LDAP) in a PHP console:

$cr = ldap_connect('ldap://ldap.owncloud.bzoc:7770', 389);
var_dump($cr);
var_dump(ldap_errno($cr));
$status = ldap_bind($cr, 'uid=owncloudagent,ou=Users,dc=owncloud,dc=bzoc', '123456');
var_dump($cr);
var_dump(ldap_errno($cr));

and paste the output?

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 29, 2015

Member
php > $cr = ldap_connect('ldap://zeus.lan.mccorkell.me.uk:389', 389);                          
php > var_dump($cr);
resource(2) of type (ldap link)
php > var_dump(ldap_errno($cr));
int(0)
php > $status = ldap_bind($cr);                                          
PHP Warning:  ldap_bind(): Unable to bind to server: Protocol error in php shell code on line 1
PHP Stack trace:
PHP   1. {main}() php shell code:0
PHP   2. ldap_bind() php shell code:1
php > var_dump($cr);
resource(2) of type (ldap link)
php > var_dump(ldap_errno($cr));
int(2)

That test didn't seem to reproduce the error, but I tried the following too:

php > $cr = ldap_connect('zeus.lan.mccorkell.me.uk:389', 389);
PHP Warning:  ldap_connect(): Could not create session handle: Bad parameter to an ldap routine in php shell code on line 1
PHP Stack trace:
PHP   1. {main}() php shell code:0
PHP   2. ldap_connect() php shell code:1
php > var_dump($cr);
bool(false)

So it seems that LDAP URIs may work with a port, but without a leading ldap:// (secure protocols are available) the error is triggered.

Member

Xenopathic commented Oct 29, 2015

php > $cr = ldap_connect('ldap://zeus.lan.mccorkell.me.uk:389', 389);                          
php > var_dump($cr);
resource(2) of type (ldap link)
php > var_dump(ldap_errno($cr));
int(0)
php > $status = ldap_bind($cr);                                          
PHP Warning:  ldap_bind(): Unable to bind to server: Protocol error in php shell code on line 1
PHP Stack trace:
PHP   1. {main}() php shell code:0
PHP   2. ldap_bind() php shell code:1
php > var_dump($cr);
resource(2) of type (ldap link)
php > var_dump(ldap_errno($cr));
int(2)

That test didn't seem to reproduce the error, but I tried the following too:

php > $cr = ldap_connect('zeus.lan.mccorkell.me.uk:389', 389);
PHP Warning:  ldap_connect(): Could not create session handle: Bad parameter to an ldap routine in php shell code on line 1
PHP Stack trace:
PHP   1. {main}() php shell code:0
PHP   2. ldap_connect() php shell code:1
php > var_dump($cr);
bool(false)

So it seems that LDAP URIs may work with a port, but without a leading ldap:// (secure protocols are available) the error is triggered.

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

Oh, it should be appended only with LDAP Urls. That's then an issue in the wizard code.

But you see that in the first attempt also an protocol error pops up.

Contributor

blizzz commented Oct 29, 2015

Oh, it should be appended only with LDAP Urls. That's then an issue in the wizard code.

But you see that in the first attempt also an protocol error pops up.

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 29, 2015

Member

Yep, fixing the protocol errors by using LDAPv3 and URIs work properly:

php > $cr = ldap_connect('ldap://zeus.lan.mccorkell.me.uk:389');
php > ldap_set_option($cr, LDAP_OPT_PROTOCOL_VERSION, 3);
php > ldap_bind($cr);
php > var_dump(ldap_errno($cr));
int(0)
Member

Xenopathic commented Oct 29, 2015

Yep, fixing the protocol errors by using LDAPv3 and URIs work properly:

php > $cr = ldap_connect('ldap://zeus.lan.mccorkell.me.uk:389');
php > ldap_set_option($cr, LDAP_OPT_PROTOCOL_VERSION, 3);
php > ldap_bind($cr);
php > var_dump(ldap_errno($cr));
int(0)
@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

okay, my bad

Contributor

blizzz commented Oct 29, 2015

okay, my bad

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 29, 2015

Member

And the port doesn't get ignored either, I tried changing the port and the connection failed, so I think that's our solution

Member

Xenopathic commented Oct 29, 2015

And the port doesn't get ignored either, I tried changing the port and the connection failed, so I think that's our solution

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

Ah, you also say it fails only with the wizard, i.e. normal use does work as it should?

Contributor

blizzz commented Oct 29, 2015

Ah, you also say it fails only with the wizard, i.e. normal use does work as it should?

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 29, 2015

Member

@blizzz Well, I think it does, but that might be caching playing a part

Member

Xenopathic commented Oct 29, 2015

@blizzz Well, I think it does, but that might be caching playing a part

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Oct 29, 2015

Member

Changing the code back to the (broken) master version after setting up the server, then logging in as an LDAP user works, so it looks like it's just the wizard

Member

Xenopathic commented Oct 29, 2015

Changing the code back to the (broken) master version after setting up the server, then logging in as an LDAP user works, so it looks like it's just the wizard

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

@Xenopathic yes, we're not checking whether there is a protocol specified, so my fault. I'll have a fix ready in soon.

Contributor

blizzz commented Oct 29, 2015

@Xenopathic yes, we're not checking whether there is a protocol specified, so my fault. I'll have a fix ready in soon.

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Oct 29, 2015

Contributor

@Xenopathic this should do the trick: #20155

Contributor

blizzz commented Oct 29, 2015

@Xenopathic this should do the trick: #20155

@MCMic

This comment has been minimized.

Show comment
Hide comment
@MCMic

MCMic Nov 3, 2015

Hello, I’m the mainainer of php-ldap, I’m just hearing about this now (Someone kindly mentioned it in https://bugs.php.net/bug.php?id=69471 )
It was of course not intended, and I’m quite surprised as there is a test for this mode of connection: https://github.com/php/php-src/blob/PHP-5.6/ext/ldap/tests/ldap_connect_variation.phpt
I just checked on my computer it’s still passing without errors on PHP-5.6 branch.

Can you check if the test does fail in your configuration?
Which version of openldap lib is your PHP version built against?

MCMic commented Nov 3, 2015

Hello, I’m the mainainer of php-ldap, I’m just hearing about this now (Someone kindly mentioned it in https://bugs.php.net/bug.php?id=69471 )
It was of course not intended, and I’m quite surprised as there is a test for this mode of connection: https://github.com/php/php-src/blob/PHP-5.6/ext/ldap/tests/ldap_connect_variation.phpt
I just checked on my computer it’s still passing without errors on PHP-5.6 branch.

Can you check if the test does fail in your configuration?
Which version of openldap lib is your PHP version built against?

@MCMic

This comment has been minimized.

Show comment
Hide comment
@MCMic

MCMic Nov 3, 2015

Hum, can you confirm what fails exactly? If using "host:port" without "ldap://" fails, it’s not surprising, as this is not tested and not listed as authorized in the documentation.
But what bugs me is that https://www.netways.org/issues/2931 says it fails even with the ldap:// and that I can’t reproduce

MCMic commented Nov 3, 2015

Hum, can you confirm what fails exactly? If using "host:port" without "ldap://" fails, it’s not surprising, as this is not tested and not listed as authorized in the documentation.
But what bugs me is that https://www.netways.org/issues/2931 says it fails even with the ldap:// and that I can’t reproduce

@Xenopathic

This comment has been minimized.

Show comment
Hide comment
@Xenopathic

Xenopathic Nov 3, 2015

Member

@MCMic Thanks for stopping by and confirming the behaviour was undocumented, and the fault lies with ownCloud 😄 Perhaps it'd be a good idea to formally document the supported syntax? Currently the doc page says this:

hostname: If you are using OpenLDAP 2.x.x you can specify a URL instead of the hostname. To use LDAP with SSL, compile OpenLDAP 2.x.x with SSL support, configure PHP with SSL, and set this parameter as ldaps://hostname/.

What about something more along the lines of this:

hostname: This field supports using a hostname (without a :port specification, use the second parameter to specify this) or with OpenLDAP 2.x.x and later a full LDAP URI, of the form ldap://hostname:port or ldaps://hostname:port for SSL encryption.

Alternatively, since the last release of OpenLDAP 1.x was in January 2000, perhaps the documentation should only officially mention using LDAP URIs, with a backwards compat note for using hostname + port separately?

Member

Xenopathic commented Nov 3, 2015

@MCMic Thanks for stopping by and confirming the behaviour was undocumented, and the fault lies with ownCloud 😄 Perhaps it'd be a good idea to formally document the supported syntax? Currently the doc page says this:

hostname: If you are using OpenLDAP 2.x.x you can specify a URL instead of the hostname. To use LDAP with SSL, compile OpenLDAP 2.x.x with SSL support, configure PHP with SSL, and set this parameter as ldaps://hostname/.

What about something more along the lines of this:

hostname: This field supports using a hostname (without a :port specification, use the second parameter to specify this) or with OpenLDAP 2.x.x and later a full LDAP URI, of the form ldap://hostname:port or ldaps://hostname:port for SSL encryption.

Alternatively, since the last release of OpenLDAP 1.x was in January 2000, perhaps the documentation should only officially mention using LDAP URIs, with a backwards compat note for using hostname + port separately?

@heiglandreas

This comment has been minimized.

Show comment
Hide comment
@heiglandreas

heiglandreas Nov 3, 2015

I'll see to that @Xenopathic. Thanks for the suggestion.

I'll see to that @Xenopathic. Thanks for the suggestion.

@directict

This comment has been minimized.

Show comment
Hide comment
@directict

directict Nov 4, 2015

@Xenopathic

diff --git a/apps/user_ldap/lib/connection.php b/apps/user_ldap/lib/connection.php
index f6b123b..7dced6d 100644
--- a/apps/user_ldap/lib/connection.php
+++ b/apps/user_ldap/lib/connection.php
@@ -564,10 +564,6 @@ class Connection extends LDAPUtility {
        if(empty($host)) {
            return false;
        }
-       if(strpos($host, '://') !== false) {
-           //ldap_connect ignores port parameter when URLs are passed
-           $host .= ':' . $port;
-       }
        $this->ldapConnectionRes = $this->ldap->connect($host, $port);
        if($this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_PROTOCOL_VERSION, 3)) {
            if($this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_REFERRALS, 0)) {
diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php
index a819b2e..198194d 100644
--- a/apps/user_ldap/lib/wizard.php
+++ b/apps/user_ldap/lib/wizard.php
@@ -1038,8 +1038,6 @@ class Wizard extends LDAPUtility {
        if(isset($hostInfo['scheme'])) {
            if(isset($hostInfo['port'])) {
                //problem
-           } else {
-               $host .= ':' . $port;
            }
        }
        \OCP\Util::writeLog('user_ldap', 'Wiz: Attempting to connect ', \OCP\Util::DEBUG);
@@ -1291,7 +1289,7 @@ class Wizard extends LDAPUtility {
            return $this->cr;
        }
        $cr = $this->ldap->connect(
-           $this->configuration->ldapHost.':'.$this->configuration->ldapPort,
+           $this->configuration->ldapHost,
            $this->configuration->ldapPort);

        $this->ldap->setOption($cr, LDAP_OPT_PROTOCOL_VERSION, 3);

After implementing this, it works... how will this be implemented?
i use PHP 5.6.14

directict commented Nov 4, 2015

@Xenopathic

diff --git a/apps/user_ldap/lib/connection.php b/apps/user_ldap/lib/connection.php
index f6b123b..7dced6d 100644
--- a/apps/user_ldap/lib/connection.php
+++ b/apps/user_ldap/lib/connection.php
@@ -564,10 +564,6 @@ class Connection extends LDAPUtility {
        if(empty($host)) {
            return false;
        }
-       if(strpos($host, '://') !== false) {
-           //ldap_connect ignores port parameter when URLs are passed
-           $host .= ':' . $port;
-       }
        $this->ldapConnectionRes = $this->ldap->connect($host, $port);
        if($this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_PROTOCOL_VERSION, 3)) {
            if($this->ldap->setOption($this->ldapConnectionRes, LDAP_OPT_REFERRALS, 0)) {
diff --git a/apps/user_ldap/lib/wizard.php b/apps/user_ldap/lib/wizard.php
index a819b2e..198194d 100644
--- a/apps/user_ldap/lib/wizard.php
+++ b/apps/user_ldap/lib/wizard.php
@@ -1038,8 +1038,6 @@ class Wizard extends LDAPUtility {
        if(isset($hostInfo['scheme'])) {
            if(isset($hostInfo['port'])) {
                //problem
-           } else {
-               $host .= ':' . $port;
            }
        }
        \OCP\Util::writeLog('user_ldap', 'Wiz: Attempting to connect ', \OCP\Util::DEBUG);
@@ -1291,7 +1289,7 @@ class Wizard extends LDAPUtility {
            return $this->cr;
        }
        $cr = $this->ldap->connect(
-           $this->configuration->ldapHost.':'.$this->configuration->ldapPort,
+           $this->configuration->ldapHost,
            $this->configuration->ldapPort);

        $this->ldap->setOption($cr, LDAP_OPT_PROTOCOL_VERSION, 3);

After implementing this, it works... how will this be implemented?
i use PHP 5.6.14

@blizzz

This comment has been minimized.

Show comment
Hide comment
@blizzz

blizzz Nov 4, 2015

Contributor

Guys, there is the fixing PR: #20155

Contributor

blizzz commented Nov 4, 2015

Guys, there is the fixing PR: #20155

@PVince81 PVince81 added this to the 9.0-current milestone Nov 4, 2015

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 4, 2015

Member

Setting to 8.2.1 (backport was approved)

Member

PVince81 commented Nov 4, 2015

Setting to 8.2.1 (backport was approved)

@PVince81 PVince81 modified the milestones: 8.2.1-current-maintenance, 9.0-current Nov 4, 2015

@MorrisJobke MorrisJobke closed this in #20155 Nov 4, 2015

@MorrisJobke MorrisJobke modified the milestones: 9.0-current, 8.2.1-current-maintenance Nov 4, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment