Use different secret for auth and access tokens
Temporarily secret guaranties invalidation of access tokens on device
reboot. Without that when device updates, we can have tokens signed
with valid key, but with invalid format, or user_id collision.
Change-Id: I0a6dbd782165715d781501456a4fd29bb060ffdd
Reviewed-on: https://weave-review.googlesource.com/2062
Reviewed-by: Alex Vakulenko <avakulenko@google.com>
diff --git a/src/device_registration_info.cc b/src/device_registration_info.cc
index 445c138..a91ae6f 100644
--- a/src/device_registration_info.cc
+++ b/src/device_registration_info.cc
@@ -1321,7 +1321,7 @@
LOG(INFO) << "Device is unregistered from the cloud. Deleting credentials";
if (auth_manager_)
- auth_manager_->SetSecret({}, RootClientTokenOwner::kNone);
+ auth_manager_->SetAuthSecret({}, RootClientTokenOwner::kNone);
Config::Transaction change{config_};
// Keep cloud_id to switch to detect kInvalidCredentials after restart.
diff --git a/src/device_registration_info_unittest.cc b/src/device_registration_info_unittest.cc
index 808f545..e21a147 100644
--- a/src/device_registration_info_unittest.cc
+++ b/src/device_registration_info_unittest.cc
@@ -203,6 +203,7 @@
16, 33, 82, 71, 10, 72, 62, 45, 1, 77, 97, 70, 24},
{21, 6, 58, 4, 66, 13, 14, 60, 55, 22, 11, 38, 96, 40, 81, 90, 3, 51, 50,
23, 56, 76, 47, 46, 27, 69, 20, 80, 88, 93, 15, 61},
+ {},
&clock_};
std::unique_ptr<DeviceRegistrationInfo> dev_reg_;
ComponentManagerImpl component_manager_;
diff --git a/src/privet/auth_manager.cc b/src/privet/auth_manager.cc
index 27a5e7e..86a564d 100644
--- a/src/privet/auth_manager.cc
+++ b/src/privet/auth_manager.cc
@@ -95,34 +95,39 @@
AuthManager::AuthManager(Config* config,
const std::vector<uint8_t>& certificate_fingerprint)
- : config_{config}, certificate_fingerprint_{certificate_fingerprint} {
+ : config_{config},
+ certificate_fingerprint_{certificate_fingerprint},
+ access_secret_{CreateSecret()} {
if (config_) {
- SetSecret(config_->GetSettings().secret,
- config_->GetSettings().root_client_token_owner);
+ SetAuthSecret(config_->GetSettings().secret,
+ config_->GetSettings().root_client_token_owner);
} else {
- SetSecret({}, RootClientTokenOwner::kNone);
+ SetAuthSecret({}, RootClientTokenOwner::kNone);
}
}
-AuthManager::AuthManager(const std::vector<uint8_t>& secret,
+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) {
- SetSecret(secret, RootClientTokenOwner::kNone);
+ access_secret_ = access_secret.size() == kSha256OutputSize ? access_secret
+ : CreateSecret();
+ SetAuthSecret(auth_secret, RootClientTokenOwner::kNone);
if (clock)
clock_ = clock;
}
-void AuthManager::SetSecret(const std::vector<uint8_t>& secret,
- RootClientTokenOwner owner) {
- secret_ = secret;
+void AuthManager::SetAuthSecret(const std::vector<uint8_t>& secret,
+ RootClientTokenOwner owner) {
+ auth_secret_ = secret;
- if (secret.size() != kSha256OutputSize) {
- secret_ = CreateSecret();
+ if (auth_secret_.size() != kSha256OutputSize) {
+ auth_secret_ = CreateSecret();
owner = RootClientTokenOwner::kNone;
}
- if (!config_ || (config_->GetSettings().secret == secret_ &&
+ if (!config_ || (config_->GetSettings().secret == auth_secret_ &&
config_->GetSettings().root_client_token_owner == owner)) {
return;
}
@@ -140,7 +145,7 @@
base::TimeDelta ttl) const {
std::string data_str{CreateTokenData(user_info, Now() + ttl)};
std::vector<uint8_t> data{data_str.begin(), data_str.end()};
- std::vector<uint8_t> hash{HmacSha256(secret_, data)};
+ std::vector<uint8_t> hash{HmacSha256(access_secret_, data)};
hash.insert(hash.end(), data.begin(), data.end());
return hash;
}
@@ -158,7 +163,7 @@
}
std::vector<uint8_t> hash(token.begin(), token.begin() + kSha256OutputSize);
std::vector<uint8_t> data(token.begin() + kSha256OutputSize, token.end());
- if (hash != HmacSha256(secret_, data)) {
+ if (hash != HmacSha256(access_secret_, data)) {
Error::AddTo(error, FROM_HERE, errors::kDomain,
errors::kInvalidAuthorization, "Invalid signature");
return false;
@@ -222,7 +227,7 @@
return false;
}
- SetSecret(claim->first->GetSecret(), claim->second);
+ SetAuthSecret(claim->first->GetAuthSecret(), claim->second);
pending_claims_.clear();
return true;
}
@@ -236,10 +241,11 @@
scope.GetCaveat(), issued.GetCaveat(),
};
- CHECK_EQ(kSha256OutputSize, secret_.size());
+ CHECK_EQ(kSha256OutputSize, auth_secret_.size());
UwMacaroon macaroon{};
- CHECK(uw_macaroon_new_from_root_key_(
- &macaroon, secret_.data(), secret_.size(), caveats, arraysize(caveats)));
+ CHECK(uw_macaroon_new_from_root_key_(&macaroon, auth_secret_.data(),
+ auth_secret_.size(), caveats,
+ arraysize(caveats)));
std::vector<uint8_t> token(kMaxMacaroonSize);
size_t len = 0;
@@ -260,8 +266,9 @@
return false;
}
- CHECK_EQ(kSha256OutputSize, secret_.size());
- return uw_macaroon_verify_(&macaroon, secret_.data(), secret_.size());
+ CHECK_EQ(kSha256OutputSize, auth_secret_.size());
+ return uw_macaroon_verify_(&macaroon, auth_secret_.data(),
+ auth_secret_.size());
}
std::vector<uint8_t> AuthManager::CreateSessionId() {
diff --git a/src/privet/auth_manager.h b/src/privet/auth_manager.h
index 8b99254..1e6ad01 100644
--- a/src/privet/auth_manager.h
+++ b/src/privet/auth_manager.h
@@ -28,8 +28,9 @@
const std::vector<uint8_t>& certificate_fingerprint);
// Constructor for tests.
- AuthManager(const std::vector<uint8_t>& secret,
+ 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);
~AuthManager();
@@ -40,7 +41,8 @@
UserInfo* user_info,
ErrorPtr* error) const;
- const std::vector<uint8_t>& GetSecret() const { return secret_; }
+ const std::vector<uint8_t>& GetAuthSecret() const { return auth_secret_; }
+ const std::vector<uint8_t>& GetAccessSecret() const { return access_secret_; }
const std::vector<uint8_t>& GetCertificateFingerprint() const {
return certificate_fingerprint_;
}
@@ -55,8 +57,8 @@
std::vector<uint8_t> GetRootClientAuthToken() const;
bool IsValidAuthToken(const std::vector<uint8_t>& token) const;
- void SetSecret(const std::vector<uint8_t>& secret,
- RootClientTokenOwner owner);
+ void SetAuthSecret(const std::vector<uint8_t>& secret,
+ RootClientTokenOwner owner);
std::vector<uint8_t> CreateSessionId();
@@ -66,8 +68,9 @@
base::Clock* clock_{&default_clock_};
uint32_t session_counter_{0};
- std::vector<uint8_t> secret_;
+ std::vector<uint8_t> auth_secret_; // Persistent.
std::vector<uint8_t> certificate_fingerprint_;
+ std::vector<uint8_t> access_secret_; // New on every reboot.
std::deque<std::pair<std::unique_ptr<AuthManager>, RootClientTokenOwner>>
pending_claims_;
diff --git a/src/privet/auth_manager_unittest.cc b/src/privet/auth_manager_unittest.cc
index 2685be5..d656fed 100644
--- a/src/privet/auth_manager_unittest.cc
+++ b/src/privet/auth_manager_unittest.cc
@@ -20,7 +20,8 @@
class AuthManagerTest : public testing::Test {
public:
void SetUp() override {
- EXPECT_GE(auth_.GetSecret().size(), 32u);
+ EXPECT_GE(auth_.GetAuthSecret().size(), 32u);
+ EXPECT_GE(auth_.GetAccessSecret().size(), 32u);
EXPECT_GE(auth_.GetCertificateFingerprint().size(), 32u);
EXPECT_CALL(clock_, Now())
@@ -28,32 +29,37 @@
}
protected:
- const std::vector<uint8_t> kSecret{69, 53, 17, 37, 80, 73, 2, 5, 79, 64, 41,
- 57, 12, 54, 65, 63, 72, 74, 93, 81, 20, 95,
- 89, 3, 94, 92, 27, 21, 49, 90, 36, 6};
- const std::vector<uint8_t> kSecret2{
+ const std::vector<uint8_t> kSecret1{
78, 40, 39, 68, 29, 19, 70, 86, 38, 61, 13, 55, 33, 32, 51, 52,
34, 43, 97, 48, 8, 56, 11, 99, 50, 59, 24, 26, 31, 71, 76, 28};
+ const std::vector<uint8_t> kSecret2{
+ 69, 53, 17, 37, 80, 73, 2, 5, 79, 64, 41, 57, 12, 54, 65, 63,
+ 72, 74, 93, 81, 20, 95, 89, 3, 94, 92, 27, 21, 49, 90, 36, 6};
const std::vector<uint8_t> kFingerprint{
22, 47, 23, 77, 42, 98, 96, 25, 83, 16, 9, 14, 91, 44, 15, 75,
60, 62, 10, 18, 82, 35, 88, 100, 30, 45, 7, 46, 67, 84, 58, 85};
test::MockClock clock_;
- AuthManager auth_{kSecret, kFingerprint, &clock_};
+ AuthManager auth_{kSecret1, kFingerprint, kSecret2, &clock_};
};
TEST_F(AuthManagerTest, RandomSecret) {
- EXPECT_GE(auth_.GetSecret().size(), 32u);
+ AuthManager auth{{}, {}, {}, &clock_};
+ EXPECT_EQ(auth.GetAuthSecret().size(), 32u);
+ EXPECT_EQ(auth.GetAccessSecret().size(), 32u);
}
TEST_F(AuthManagerTest, DifferentSecret) {
- AuthManager auth{kSecret2, {}};
- EXPECT_GE(auth.GetSecret().size(), 32u);
- EXPECT_NE(auth_.GetSecret(), auth.GetSecret());
+ AuthManager auth{kSecret2, {}, kSecret1};
+ EXPECT_EQ(auth.GetAuthSecret().size(), 32u);
+ EXPECT_EQ(auth.GetAccessSecret().size(), 32u);
+ EXPECT_NE(auth_.GetAccessSecret(), auth.GetAccessSecret());
+ EXPECT_NE(auth_.GetAuthSecret(), auth.GetAuthSecret());
}
TEST_F(AuthManagerTest, Constructor) {
- EXPECT_EQ(kSecret, auth_.GetSecret());
+ EXPECT_EQ(kSecret1, auth_.GetAuthSecret());
+ EXPECT_EQ(kSecret2, auth_.GetAccessSecret());
EXPECT_EQ(kFingerprint, auth_.GetCertificateFingerprint());
}
@@ -112,7 +118,7 @@
for (size_t i = 0; i < 1000; ++i) {
EXPECT_CALL(clock_, Now()).WillRepeatedly(Return(kStartTime));
- AuthManager auth{{}, {}, &clock_};
+ AuthManager auth{{}, {}, {}, &clock_};
auto token = auth.CreateAccessToken(UserInfo{AuthScope::kUser, 5},
base::TimeDelta::FromSeconds(i));
@@ -135,20 +141,20 @@
}
TEST_F(AuthManagerTest, GetRootClientAuthToken) {
- EXPECT_EQ("UFTBUcgd9d0HnPRnLeroN2mCQgECRgMaVArkgA==",
+ EXPECT_EQ("UK1ACOc3cWGjGBoTIX2bd3qCQgECRgMaVArkgA==",
Base64Encode(auth_.GetRootClientAuthToken()));
}
TEST_F(AuthManagerTest, GetRootClientAuthTokenDifferentTime) {
auto new_time = clock_.Now() + base::TimeDelta::FromDays(15);
EXPECT_CALL(clock_, Now()).WillRepeatedly(Return(new_time));
- EXPECT_EQ("UGKqwMYGQNOd8jeYFDOsM02CQgECRgMaVB6rAA==",
+ EXPECT_EQ("UBpNF8g/GbNUmAyHg1qqJr+CQgECRgMaVB6rAA==",
Base64Encode(auth_.GetRootClientAuthToken()));
}
TEST_F(AuthManagerTest, GetRootClientAuthTokenDifferentSecret) {
- AuthManager auth{kSecret2, {}, &clock_};
- EXPECT_EQ("UK1ACOc3cWGjGBoTIX2bd3qCQgECRgMaVArkgA==",
+ AuthManager auth{kSecret2, {}, kSecret1, &clock_};
+ EXPECT_EQ("UFTBUcgd9d0HnPRnLeroN2mCQgECRgMaVArkgA==",
Base64Encode(auth.GetRootClientAuthToken()));
}
@@ -156,7 +162,7 @@
EXPECT_TRUE(auth_.IsValidAuthToken(auth_.GetRootClientAuthToken()));
// Multiple attempts with random secrets.
for (size_t i = 0; i < 1000; ++i) {
- AuthManager auth{{}, {}, &clock_};
+ AuthManager auth{{}, {}, {}, &clock_};
auto token = auth.GetRootClientAuthToken();
EXPECT_FALSE(auth_.IsValidAuthToken(token));
@@ -166,7 +172,7 @@
class AuthManagerClaimTest : public testing::Test {
public:
- void SetUp() override { EXPECT_GE(auth_.GetSecret().size(), 32u); }
+ void SetUp() override { EXPECT_EQ(auth_.GetAuthSecret().size(), 32u); }
bool TestClaim(RootClientTokenOwner owner, RootClientTokenOwner claimer) {
Config::Transaction change{&config_};
diff --git a/src/privet/security_manager_unittest.cc b/src/privet/security_manager_unittest.cc
index f4a7b17..7085e83 100644
--- a/src/privet/security_manager_unittest.cc
+++ b/src/privet/security_manager_unittest.cc
@@ -121,6 +121,8 @@
138, 176, 141, 37, 63, 223, 40, 153, 121, 134, 23, 120, 106, 24, 205,
7, 135,
},
+ {22, 47, 23, 77, 42, 98, 96, 25, 83, 16, 9, 14, 91, 44, 15, 75, 60, 62,
+ 10, 18, 82, 35, 88, 100, 30, 45, 7, 46, 67, 84, 58, 85},
&clock_};
SecurityManager security_{&auth_manager_,
{PairingType::kEmbeddedCode},
