Closed RobinMcCorkell closed 9 years ago
@Xenopathic are you able to reproduce this with an instance installed from scratch ? (just to confirm / evaluate severity, I haven't tried it myself)
Yes, just reproduced with the latest master (although it technically wasn't a clean instance, but the LDAP configuration was completely clean)
Now I'm slightly concerned, I'm reproducing this on 8.1.0 as well...
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()
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 :speak_no_evil:
We should at least make a note somewhere. Or can we fix/work-around this? The Netways link seems to indicate this?
cc @cmonteroluque
We need to understand how many distributions ship the affected PHP version. Let's do release note for now @carlaschroder
@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...
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"}
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."
Sorry, that was a question. moar coffee.
@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
ok thanks @Xenopathic
thanks @carlaschroder
@cmonteroluque @carlaschroder Note, this seems to affect 8.1 as well, perhaps even earlier versions.
Gottit @Xenopathic, all the way back to 7.0.
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?)
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
@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.
@blizzz Yes, that patch fixes things, the wizard works as expected.
@Xenopathic good. Meanwhile I confirmed, it fails with PHP 5.6.4 i.e. PHP < 5.6.11. Bollocks.
So which options do we have?
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?
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.
@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?
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.
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.
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)
okay, my bad
And the port doesn't get ignored either, I tried changing the port and the connection failed, so I think that's our solution
Ah, you also say it fails only with the wizard, i.e. normal use does work as it should?
@blizzz Well, I think it does, but that might be caching playing a part
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
@Xenopathic yes, we're not checking whether there is a protocol specified, so my fault. I'll have a fix ready in soon.
@Xenopathic this should do the trick: https://github.com/owncloud/core/pull/20155
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?
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 Thanks for stopping by and confirming the behaviour was undocumented, and the fault lies with ownCloud :smile: 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 formldap://hostname:port
orldaps://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?
I'll see to that @Xenopathic. Thanks for the suggestion.
@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
Guys, there is the fixing PR: https://github.com/owncloud/core/issues/20155
Setting to 8.2.1 (backport was approved)
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
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:
LDAP configuration:
cc @blizzz