From cc397da1bec21d27fbf9ff392958298014630b9c Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Mon, 15 Feb 2016 16:24:21 +0100 Subject: [PATCH 1/3] Remove background job if the server accepted to ask for the shared secret If we don't remove it the server will later ask the remote server to ask for the shared secret which will result in a error log message on the remote server and in some circumstances maybe even to a failure --- apps/federation/api/ocsauthapi.php | 25 ++++++++++++++++---- apps/federation/tests/api/ocsauthapitest.php | 3 +++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/apps/federation/api/ocsauthapi.php b/apps/federation/api/ocsauthapi.php index 058a5966374..1c4e73cc8de 100644 --- a/apps/federation/api/ocsauthapi.php +++ b/apps/federation/api/ocsauthapi.php @@ -33,7 +33,6 @@ use OCP\BackgroundJob\IJobList; use OCP\ILogger; use OCP\IRequest; use OCP\Security\ISecureRandom; -use OCP\Security\StringUtils; /** * Class OCSAuthAPI @@ -99,7 +98,7 @@ class OCSAuthAPI { $token = $this->request->getParam('token'); if ($this->trustedServers->isTrustedServer($url) === false) { - $this->logger->log(\OCP\Util::ERROR, 'remote server not trusted (' . $url . ') while requesting shared secret', ['app' => 'federation']); + $this->logger->error('remote server not trusted (' . $url . ') while requesting shared secret', ['app' => 'federation']); return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN); } @@ -107,10 +106,22 @@ class OCSAuthAPI { // token wins $localToken = $this->dbHandler->getToken($url); if (strcmp($localToken, $token) > 0) { - $this->logger->log(\OCP\Util::ERROR, 'remote server (' . $url . ') presented lower token', ['app' => 'federation']); + $this->logger->info( + 'remote server (' . $url . ') presented lower token. We will initiate the exchange of the shared secret.', + ['app' => 'federation'] + ); return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN); } + // we ask for the shared secret so we no longer have to ask the other server + // to request the shared secret + $this->jobList->remove('OCA\Federation\BackgroundJob\RequestSharedSecret', + [ + 'url' => $url, + 'token' => $localToken + ] + ); + $this->jobList->add( 'OCA\Federation\BackgroundJob\GetSharedSecret', [ @@ -134,12 +145,16 @@ class OCSAuthAPI { $token = $this->request->getParam('token'); if ($this->trustedServers->isTrustedServer($url) === false) { - $this->logger->log(\OCP\Util::ERROR, 'remote server not trusted (' . $url . ') while getting shared secret', ['app' => 'federation']); + $this->logger->error('remote server not trusted (' . $url . ') while getting shared secret', ['app' => 'federation']); return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN); } if ($this->isValidToken($url, $token) === false) { - $this->logger->log(\OCP\Util::ERROR, 'remote server (' . $url . ') didn\'t send a valid token (got ' . $token . ') while getting shared secret', ['app' => 'federation']); + $expectedToken = $this->dbHandler->getToken($url); + $this->logger->error( + 'remote server (' . $url . ') didn\'t send a valid token (got "' . $token . '" but expected "'. $expectedToken . '") while getting shared secret', + ['app' => 'federation'] + ); return new \OC_OCS_Result(null, HTTP::STATUS_FORBIDDEN); } diff --git a/apps/federation/tests/api/ocsauthapitest.php b/apps/federation/tests/api/ocsauthapitest.php index 9c751fff895..d3e61c0641a 100644 --- a/apps/federation/tests/api/ocsauthapitest.php +++ b/apps/federation/tests/api/ocsauthapitest.php @@ -105,8 +105,11 @@ class OCSAuthAPITest extends TestCase { if ($expected === Http::STATUS_OK) { $this->jobList->expects($this->once())->method('add') ->with('OCA\Federation\BackgroundJob\GetSharedSecret', ['url' => $url, 'token' => $token]); + $this->jobList->expects($this->once())->method('remove') + ->with('OCA\Federation\BackgroundJob\RequestSharedSecret', ['url' => $url, 'token' => $localToken]); } else { $this->jobList->expects($this->never())->method('add'); + $this->jobList->expects($this->never())->method('remove'); } $result = $this->ocsAuthApi->requestSharedSecret(); From 835e70dbe2d68e8263affe8aa538f8b5cba25c5f Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Mon, 15 Feb 2016 16:59:49 +0100 Subject: [PATCH 2/3] throw exception if we don't find a token for a given server --- apps/federation/lib/dbhandler.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/apps/federation/lib/dbhandler.php b/apps/federation/lib/dbhandler.php index 9e44c72cc42..3ea84baa3eb 100644 --- a/apps/federation/lib/dbhandler.php +++ b/apps/federation/lib/dbhandler.php @@ -156,6 +156,7 @@ class DbHandler { * * @param string $url * @return string + * @throws \Exception */ public function getToken($url) { $hash = $this->hash($url); @@ -165,6 +166,11 @@ class DbHandler { ->setParameter('url_hash', $hash); $result = $query->execute()->fetch(); + + if (!isset($result['token'])) { + throw new \Exception('No token found for: ' . $url); + } + return $result['token']; } From 9d1d08bf9b0c6f7107e259594d84cb0c7496bebc Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Mon, 15 Feb 2016 17:33:06 +0100 Subject: [PATCH 3/3] forbidden (403) is a valid return status, don't log the whole exception in this case --- apps/federation/backgroundjob/getsharedsecret.php | 8 ++++++-- apps/federation/backgroundjob/requestsharedsecret.php | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/apps/federation/backgroundjob/getsharedsecret.php b/apps/federation/backgroundjob/getsharedsecret.php index a98a17e323b..ebc106ba94e 100644 --- a/apps/federation/backgroundjob/getsharedsecret.php +++ b/apps/federation/backgroundjob/getsharedsecret.php @@ -150,10 +150,14 @@ class GetSharedSecret extends QueuedJob{ } catch (ClientException $e) { $status = $e->getCode(); - $this->logger->logException($e); + if ($status === Http::STATUS_FORBIDDEN) { + $this->logger->info($target . ' refused to exchange a shared secret with you.', ['app' => 'federation']); + } else { + $this->logger->logException($e, ['app' => 'federation']); + } } catch (\Exception $e) { $status = HTTP::STATUS_INTERNAL_SERVER_ERROR; - $this->logger->logException($e); + $this->logger->logException($e, ['app' => 'federation']); } // if we received a unexpected response we try again later diff --git a/apps/federation/backgroundjob/requestsharedsecret.php b/apps/federation/backgroundjob/requestsharedsecret.php index 2db5d09545a..302711af27f 100644 --- a/apps/federation/backgroundjob/requestsharedsecret.php +++ b/apps/federation/backgroundjob/requestsharedsecret.php @@ -148,10 +148,14 @@ class RequestSharedSecret extends QueuedJob { } catch (ClientException $e) { $status = $e->getCode(); - $this->logger->logException($e); + if ($status === Http::STATUS_FORBIDDEN) { + $this->logger->info($target . ' refused to ask for a shared secret.', ['app' => 'federation']); + } else { + $this->logger->logException($e, ['app' => 'federation']); + } } catch (\Exception $e) { $status = HTTP::STATUS_INTERNAL_SERVER_ERROR; - $this->logger->logException($e); + $this->logger->logException($e, ['app' => 'federation']); } // if we received a unexpected response we try again later