Reset access token secret on new black list entry
To avoid checking all access tokens against black list, we just invalidate all
of them to make clients to go through auth API.
BUG:27300728
Change-Id: Iddf08e076037469db9dc859288645a8fe2991914
Reviewed-on: https://weave-review.googlesource.com/2708
Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/src/device_manager.cc b/src/device_manager.cc
index 4c0d3ee..aebda67 100644
--- a/src/device_manager.cc
+++ b/src/device_manager.cc
@@ -33,8 +33,12 @@
: config_{new Config{config_store}},
component_manager_{new ComponentManagerImpl{task_runner}} {
if (http_server) {
- auth_manager_.reset(new privet::AuthManager(
- config_.get(), http_server->GetHttpsCertificateFingerprint()));
+ black_list_manager_.reset(new AccessBlackListManagerImpl{config_store});
+ auth_manager_.reset(
+ new privet::AuthManager(config_.get(), black_list_manager_.get(),
+ http_server->GetHttpsCertificateFingerprint()));
+ access_api_handler_.reset(
+ new AccessApiHandler{this, black_list_manager_.get()});
}
device_info_.reset(new DeviceRegistrationInfo(
@@ -42,10 +46,6 @@
network, auth_manager_.get()));
base_api_handler_.reset(new BaseApiHandler{device_info_.get(), this});
- black_list_manager_.reset(new AccessBlackListManagerImpl{config_store});
- access_api_handler_.reset(
- new AccessApiHandler{this, black_list_manager_.get()});
-
device_info_->Start();
if (http_server) {
diff --git a/src/privet/auth_manager.cc b/src/privet/auth_manager.cc
index 027ad5a..795dafa 100644
--- a/src/privet/auth_manager.cc
+++ b/src/privet/auth_manager.cc
@@ -6,10 +6,12 @@
#include <algorithm>
+#include <base/bind.h>
#include <base/guid.h>
#include <base/rand_util.h>
#include <base/strings/string_number_conversions.h>
+#include "src/access_black_list_manager.h"
#include "src/config.h"
#include "src/data_encoding.h"
#include "src/privet/constants.h"
@@ -275,10 +277,16 @@
} // namespace
AuthManager::AuthManager(Config* config,
+ AccessBlackListManager* black_list,
const std::vector<uint8_t>& certificate_fingerprint)
: config_{config},
+ black_list_{black_list},
certificate_fingerprint_{certificate_fingerprint},
access_secret_{CreateSecret()} {
+ if (black_list_) {
+ black_list_->AddEntryAddedCallback(base::Bind(
+ &AuthManager::ResetAccessSecret, weak_ptr_factory_.GetWeakPtr()));
+ }
if (config_) {
SetAuthSecret(config_->GetSettings().secret,
config_->GetSettings().root_client_token_owner);
@@ -290,8 +298,9 @@
AuthManager::AuthManager(const std::vector<uint8_t>& auth_secret,
const std::vector<uint8_t>& certificate_fingerprint,
const std::vector<uint8_t>& access_secret,
- base::Clock* clock)
- : AuthManager(nullptr, certificate_fingerprint) {
+ base::Clock* clock,
+ AccessBlackListManager* black_list)
+ : AuthManager(nullptr, black_list, certificate_fingerprint) {
access_secret_ = access_secret.size() == kSha256OutputSize ? access_secret
: CreateSecret();
SetAuthSecret(auth_secret, RootClientTokenOwner::kNone);
@@ -401,7 +410,8 @@
};
pending_claims_.push_back(std::make_pair(
- std::unique_ptr<AuthManager>{new AuthManager{nullptr, {}}}, owner));
+ std::unique_ptr<AuthManager>{new AuthManager{nullptr, nullptr, {}}},
+ owner));
if (pending_claims_.size() > kMaxPendingClaims)
pending_claims_.pop_front();
return pending_claims_.back().first->GetRootClientAuthToken(owner);
@@ -547,6 +557,12 @@
ssid_time <= Now();
}
+void AuthManager::ResetAccessSecret() {
+ auto new_secret = CreateSecret();
+ CHECK(new_secret != access_secret_);
+ access_secret_.swap(new_secret);
+}
+
std::vector<uint8_t> AuthManager::DelegateToUser(
const std::vector<uint8_t>& token,
base::TimeDelta ttl,
diff --git a/src/privet/auth_manager.h b/src/privet/auth_manager.h
index f0a5761..83899da 100644
--- a/src/privet/auth_manager.h
+++ b/src/privet/auth_manager.h
@@ -10,6 +10,7 @@
#include <vector>
#include <base/gtest_prod_util.h>
+#include <base/memory/weak_ptr.h>
#include <base/time/default_clock.h>
#include <base/time/time.h>
#include <weave/error.h>
@@ -18,6 +19,7 @@
namespace weave {
+class AccessBlackListManager;
class Config;
enum class RootClientTokenOwner;
@@ -26,13 +28,15 @@
class AuthManager {
public:
AuthManager(Config* config,
+ AccessBlackListManager* black_list,
const std::vector<uint8_t>& certificate_fingerprint);
// Constructor for tests.
AuthManager(const std::vector<uint8_t>& auth_secret,
const std::vector<uint8_t>& certificate_fingerprint,
const std::vector<uint8_t>& access_secret,
- base::Clock* clock = nullptr);
+ base::Clock* clock = nullptr,
+ AccessBlackListManager* black_list = nullptr);
~AuthManager();
std::vector<uint8_t> CreateAccessToken(const UserInfo& user_info,
@@ -74,12 +78,15 @@
private:
friend class AuthManagerTest;
+ void ResetAccessSecret();
+
// Test helpers. Device does not need to implement delegation.
std::vector<uint8_t> DelegateToUser(const std::vector<uint8_t>& token,
base::TimeDelta ttl,
const UserInfo& user_info) const;
Config* config_{nullptr}; // Can be nullptr for tests.
+ AccessBlackListManager* black_list_{nullptr};
base::DefaultClock default_clock_;
base::Clock* clock_{&default_clock_};
mutable uint32_t session_counter_{0};
@@ -91,6 +98,7 @@
std::deque<std::pair<std::unique_ptr<AuthManager>, RootClientTokenOwner>>
pending_claims_;
+ base::WeakPtrFactory<AuthManager> weak_ptr_factory_{this};
DISALLOW_COPY_AND_ASSIGN(AuthManager);
};
diff --git a/src/privet/auth_manager_unittest.cc b/src/privet/auth_manager_unittest.cc
index 3e5aeab..8a4938e 100644
--- a/src/privet/auth_manager_unittest.cc
+++ b/src/privet/auth_manager_unittest.cc
@@ -11,13 +11,26 @@
#include "src/config.h"
#include "src/data_encoding.h"
#include "src/privet/mock_delegates.h"
+#include "src/test/mock_black_list_manager.h"
#include "src/test/mock_clock.h"
using testing::Return;
+using testing::SaveArg;
+using testing::_;
namespace weave {
namespace privet {
+class MockBlackListManager : public test::MockAccessBlackListManager {
+ public:
+ MockBlackListManager() {
+ EXPECT_CALL(*this, AddEntryAddedCallback(_))
+ .WillOnce(SaveArg<0>(&changed_callback_));
+ }
+
+ base::Closure changed_callback_;
+};
+
class AuthManagerTest : public testing::Test {
public:
void SetUp() override {
@@ -46,7 +59,8 @@
60, 62, 10, 18, 82, 35, 88, 100, 30, 45, 7, 46, 67, 84, 58, 85};
test::MockClock clock_;
- AuthManager auth_{kSecret1, kFingerprint, kSecret2, &clock_};
+ MockBlackListManager black_list_;
+ AuthManager auth_{kSecret1, kFingerprint, kSecret2, &clock_, &black_list_};
};
TEST_F(AuthManagerTest, RandomSecret) {
@@ -152,8 +166,9 @@
TEST_F(AuthManagerTest, CreateTokenDifferentInstance) {
EXPECT_NE(auth_.CreateAccessToken(
UserInfo{AuthScope::kUser, TestUserId{"123"}}, {}),
- AuthManager({}, {}).CreateAccessToken(
- UserInfo{AuthScope::kUser, TestUserId{"123"}}, {}));
+ AuthManager({}, nullptr, {})
+ .CreateAccessToken(
+ UserInfo{AuthScope::kUser, TestUserId{"123"}}, {}));
}
TEST_F(AuthManagerTest, ParseAccessToken) {
@@ -189,6 +204,22 @@
}
}
+TEST_F(AuthManagerTest, AccessTokenAfterReset) {
+ UserInfo user_info;
+ auto token1 = auth_.CreateAccessToken(
+ UserInfo{AuthScope::kViewer, TestUserId{"555"}}, {});
+ EXPECT_TRUE(auth_.ParseAccessToken(token1, &user_info, nullptr));
+
+ black_list_.changed_callback_.Run();
+
+ auto token2 = auth_.CreateAccessToken(
+ UserInfo{AuthScope::kViewer, TestUserId{"555"}}, {});
+
+ EXPECT_NE(token1, token2);
+ EXPECT_FALSE(auth_.ParseAccessToken(token1, &user_info, nullptr));
+ EXPECT_TRUE(auth_.ParseAccessToken(token2, &user_info, nullptr));
+}
+
TEST_F(AuthManagerTest, GetRootClientAuthToken) {
EXPECT_EQ("WCCDQxkgAUYIGhudoQBCDABQX3fPR5zsPnrs9aOSvS7/eQ==",
Base64Encode(
@@ -332,7 +363,7 @@
protected:
Config config_{nullptr};
- AuthManager auth_{&config_, {}};
+ AuthManager auth_{&config_, nullptr, {}};
};
TEST_F(AuthManagerClaimTest, WithPreviosOwner) {
