Lost connection to LDAP server causes app password to be removed

[RobinMcCorkell]

I've got an LDAP server providing my authentication, and have several Nextcloud clients connected using app passwords added in the web interface. Sometimes my LDAP server goes down or is busy, and Nextcloud complains that it is unavailable, however when the server comes back the app password used by my desktop client is missing, and I have to create a new one.

I'm assuming the client tries a log in with an app password, then the server cannot connect to LDAP so marks that app password as invalid, removing it.

Nothing special in the logs, just a bunch of "bind failed", "lost connection to LDAP" and some bruteforce warnings.

Nextcloud 12.0.3, but I've noticed this for many versions now

[juliusknorr]

cc @blizzz for ldap and @LukasReschke for authentication

[ChristophWurst]

@Xenopathic what version of the Nextcloud server are you using? IIRC we fixed this bug a few months ago.

[blizzz]

@ChristophWurst he states

Nextcloud 12.0.3, but I've noticed this for many versions now

[ChristophWurst]

Thanks, @blizzz, I overlooked that.

@blizzz do you remember the issue/PR title of the related bug? IIRC it's the bug we had on c.nc.com whenever the LDAP server wasn't available. Not sure who submitted/fixed it or how we fixed it. I can't find it right now.

[blizzz]

One issue was #2431 and a fix for this was #3324. IIRC it was not all, there was another code path that we fixed (which affected also non-LDAP users).

One was related to updated scenarios: #4289

I also believe to remember that we also had a fix in the login process, although i cannot find it right now. Perhaps I am mixing it up.

[blizzz]

@Xenopathic can you reliable reproduce it?

[goddib]

I can confirm the behaviour described by @Xenopathic . This also applies, when a user is temporarily deactivated (which is probably desired). I assume either whenever the cronjob runs and checks the users on the LDAP (is that included in the cronjob) or when the user tries to logon (through app and app password) and the user is not found it's removed from the nextcloud database and the password deleted along (even though other settings are retained?)

[blizzz]

@goddib thansk for chiming in and your thought. Users are not removed without manual action. When the LDAP server goes offline and cannot be reached anymore, an exception is thrown, and nothing would continue. @ChristophWurst could there be a code path were leaving that extra-ordinarily would cause such an effect?

[ChristophWurst]

@ChristophWurst could there be a code path were leaving that extra-ordinarily would cause such an effect?

Can't answer that right away but I'll investigate.

[ChristophWurst]

I have the feeling this code here

server/lib/private/User/Session.php

Lines 679 to 684 in 343a695

	if ($this->manager->checkPassword($dbToken->getLoginName(), $pwd) === false
	|| (!is_null($this->activeUser) && !$this->activeUser->isEnabled())) {
	$this->tokenProvider->invalidateToken($token);
	// Password has changed or user was disabled -> log user out
	return false;
	}

could be triggered because either the user is not set (because it cannot be loaded from the LDAP back-end) or the back-end marks it is disabled.
All the other code paths that lead to an app token being deleted seem unlikely to cause this.

@Xenopathic any chance you could try to debug this, e.g. by adding a log statement inside the if-block? Something like \OC::$server->getLogger()->warning(…) should work.

[RobinMcCorkell]

@ChristophWurst I can confirm that block is being hit when the LDAP server is unavailable.

[ChristophWurst]

Thanks for confirmatin @Xenopathic. Did you also see a Login failed: … warning entry in your nextcloud.log? In that case the user manager was asked to check the password and reported it being wrong. Or more likely, the LDAP back-end isn't even registered and thus

server/lib/private/User/Manager.php

Lines 210 to 224 in 343a695

	public function checkPasswordNoLogging($loginName, $password) {
	$loginName = str_replace("\0", '', $loginName);
	$password = str_replace("\0", '', $password);
	foreach ($this->backends as $backend) {
	if ($backend->implementsActions(Backend::CHECK_PASSWORD)) {
	$uid = $backend->checkPassword($loginName, $password);
	if ($uid !== false) {
	return $this->getUserObject($uid, $backend);
	}
	}
	}
	return false;
	}

simply returns false as the user is not found in other back-ends.

@blizzz can you confirm that the LDAP back-end is not registered in the OCP\User\Manager if the LDAP server is unavailable?

[RobinMcCorkell]

I see "Bind failed: 49: Invalid credentials" twice, followed by "Login failed", then the debug statement that I inserted is hit.

[blizzz]

@Xenopathic this is an error returned from the LDAP server, so at this point of time it was indeed online, and contacted, and returned that error. If the server went away, the error would be different, and especially we throw a ServerNotAvailable exception.

@ChristophWurst the LDAP backend is always registered, when at least one enabled configuration is active. The LDAP server state is detected on demand.

[ChristophWurst]

@ChristophWurst the LDAP backend is always registered, when at least one enabled configuration is active. The LDAP server state is detected on demand.

Alright. So if I understand this correctly, the back-end will probably return false for checkPassword since the user object cannot be loaded in that state.

Maybe we should fail harder in that case, e.g. by throwing a ServerNotAvailable exception. Otherwise a consumer of the back-end cannot differentiate between a user not being found and a temporary back-end outage.

server/apps/user_ldap/lib/User_LDAP.php

Lines 151 to 184 in 343a695

	public function checkPassword($uid, $password) {
	try {
	$ldapRecord = $this->getLDAPUserByLoginName($uid);
	} catch(NotOnLDAP $e) {
	if($this->ocConfig->getSystemValue('loglevel', Util::WARN) === Util::DEBUG) {
	\OC::$server->getLogger()->logException($e, ['app' => 'user_ldap']);
	}
	return false;
	}
	$dn = $ldapRecord['dn'][0];
	$user = $this->access->userManager->get($dn);
	if(!$user instanceof User) {
	Util::writeLog('user_ldap',
	'LDAP Login: Could not get user object for DN ' . $dn .
	'. Maybe the LDAP entry has no set display name attribute?',
	Util::WARN);
	return false;
	}
	if($user->getUsername() !== false) {
	//are the credentials OK?
	if(!$this->access->areCredentialsValid($dn, $password)) {
	return false;
	}
	$this->access->cacheUserExists($user->getUsername());
	$user->processAttributes($ldapRecord);
	$user->markLogin();
	return $user->getUsername();
	}
	return false;
	}

[blizzz]

as stated above, a ServerNotAvailable would be thrown if it is not available. However the LDAP server in fact replied that the bind operation was not successful.

[blizzz]

@Xenopathic or are you behind a proxy, load balancer or similar that might return confusing responses?

[RobinMcCorkell]

I think that might be it: I've captured a packet trace and am seeing "Invalid credentials" being returned by the LDAP server. For example, when I block the LDAP port in iptables, Nextcloud still connects via IPv6, but then LDAP is configured to pass bind requests to saslauthd (to hit Kerberos), which bubbles down to the system's PAM stack, specifically SSSD. SSSD tries to connect to LDAP via IPv4, and hits the firewall block, which logs "Authentication service cannot retrieve authentication info". Sadly this causes LDAP to claim an invalid bind, hence Nextcloud (correctly) thinks the password has changed.

In the more general case, I guess there's some kind of race condition between the parts of my authentication stack that can cause LDAP to claim invalid credentials.

I'm surprised though that this causes the token to be invalidated. I thought the purpose of an application token was to make it independent from the password, so invalidating the token if the password changes/if the user cannot authenticate seems silly to me.

[ChristophWurst]

I'm surprised though that this causes the token to be invalidated. I thought the purpose of an application token was to make it independent from the password, so invalidating the token if the password changes/if the user cannot authenticate seems silly to me.

Yes and no. The problem is that some features like external storage require the login password in some situations, thus we needed a ways to access that password even if an app password is used. For that reason the password is encrypted and stored with app password and decrypted as needed.

A possible solution was proposed here #2581 (comment).

[RobinMcCorkell]

Yep, I think this is a duplicate of #2581 at this point. Sorry for the noise!