authpolicy: Assume single-user
Assume AuthenticateUser, GetUserStatus etc. is only ever called with
one account id and crash if not (for security reasons). Chrome should
make sure this is the case. On user switch, authpolicyd should be
restarted.
The old code kept an account id -> data map, but multiple account ids
have never been properly supported since authpolicyd only keeps track
of one TGT at a time (authenticating with another user wipes krb5cc).
BUG=chromium:555491
TEST=cros_run_unit_tests --board=amd64-generic --packages authpolicy
Change-Id: I230e21f8341d3fed50dd5a77f9081e954c89b81b
Reviewed-on: https://chromium-review.googlesource.com/571225
Commit-Ready: Lutz Justen <ljusten@chromium.org>
Tested-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Roman Sorokin <rsorokin@chromium.org>
diff --git a/authpolicy/authpolicy_unittest.cc b/authpolicy/authpolicy_unittest.cc
index 5063890..61c2a5d 100644
--- a/authpolicy/authpolicy_unittest.cc
+++ b/authpolicy/authpolicy_unittest.cc
@@ -623,6 +623,15 @@
EXPECT_EQ(2, metrics_->GetMetricReportCount(METRIC_KINIT_FAILED_TRY_COUNT));
}
+// Program should die if trying to auth with different account ids.
+TEST_F(AuthPolicyTest, AuthFailsDifferentAccountIds) {
+ EXPECT_EQ(ERROR_NONE, Join(kMachineName, kUserPrincipal, MakePasswordFd()));
+ EXPECT_EQ(ERROR_NONE, Auth(kUserPrincipal, kAccountId, MakePasswordFd()));
+ EXPECT_DEATH(Auth(kUserPrincipal, kAltAccountId, MakePasswordFd()),
+ "Multi-user not supported");
+ EXPECT_EQ(2, metrics_->GetMetricReportCount(METRIC_KINIT_FAILED_TRY_COUNT));
+}
+
// User authentication fails with bad (non-existent) account id.
TEST_F(AuthPolicyTest, AuthFailsWithBadAccountId) {
EXPECT_EQ(ERROR_NONE, Join(kMachineName, kUserPrincipal, MakePasswordFd()));
@@ -705,6 +714,15 @@
EXPECT_EQ(1, metrics_->GetMetricReportCount(METRIC_KINIT_FAILED_TRY_COUNT));
}
+// Program should die if trying to get user status with different account ids
+// than what was used for auth.
+TEST_F(AuthPolicyTest, GetUserStatusFailsDifferentAccountId) {
+ EXPECT_EQ(ERROR_NONE, Join(kMachineName, kUserPrincipal, MakePasswordFd()));
+ EXPECT_EQ(ERROR_NONE, Auth(kUserPrincipal, kAccountId, MakePasswordFd()));
+ EXPECT_DEATH(GetUserStatus(kAltAccountId), "Multi-user not supported");
+ EXPECT_EQ(2, metrics_->GetMetricReportCount(METRIC_KINIT_FAILED_TRY_COUNT));
+}
+
// GetUserStatus succeeds without auth, but tgt is invalid.
TEST_F(AuthPolicyTest, GetUserStatusSucceedsTgtNotFound) {
ActiveDirectoryUserStatus status;
diff --git a/authpolicy/samba_interface.cc b/authpolicy/samba_interface.cc
index 19adcf8..21227c0 100644
--- a/authpolicy/samba_interface.cc
+++ b/authpolicy/samba_interface.cc
@@ -331,21 +331,7 @@
ErrorType error = AuthenticateUserInternal(
user_principal_name, account_id, password_fd, account_info);
- // Memorize the last auth error (if we have an account id as key). Note that
- // account_id can be empty on first auth and account_info->account_id() can be
- // empty on error.
- const std::string& actual_account_id = !account_info->account_id().empty()
- ? account_info->account_id()
- : account_id;
- if (!actual_account_id.empty()) {
- // The account id may not change.
- CHECK(account_id.empty() || account_info->account_id().empty() ||
- account_id == actual_account_id);
-
- const std::string account_id_key(kActiveDirectoryPrefix +
- actual_account_id);
- user_data_[account_id_key].last_auth_error_ = error;
- }
+ last_auth_error_ = error;
return error;
}
@@ -354,6 +340,9 @@
const std::string& account_id,
int password_fd,
ActiveDirectoryAccountInfo* account_info) {
+ if (!account_id.empty())
+ SetUser(kActiveDirectoryPrefix + account_id);
+
// Split user_principal_name into parts and normalize.
std::string user_name, realm, workgroup, normalized_upn;
if (!ParseUserPrincipalName(
@@ -379,6 +368,9 @@
if (error != ERROR_NONE)
return error;
+ if (account_id.empty())
+ SetUser(kActiveDirectoryPrefix + account_info->account_id());
+
// Update normalized_upn. This handles the situation when the user name
// changes on the server and the user logs in with their old user name (e.g.
// from the pods screen in Chrome).
@@ -398,16 +390,16 @@
// wants the sAMAccountName. Also note that pwd_last_set is zero and stale
// at this point if AcquireTgtWithPassword() set a new password, but that's
// fine, the timestamp is updated in the next GetUserStatus() call.
- const std::string account_id_key(kActiveDirectoryPrefix +
- account_info->account_id());
- user_data_[account_id_key] =
- UserData(account_info->sam_account_name(), account_info->pwd_last_set());
+ user_sam_account_name_ = account_info->sam_account_name();
+ user_pwd_last_set_ = account_info->pwd_last_set();
+ user_logged_in_ = true;
return ERROR_NONE;
}
ErrorType SambaInterface::GetUserStatus(
const std::string& account_id, ActiveDirectoryUserStatus* user_status) {
ReloadDebugFlags();
+ SetUser(kActiveDirectoryPrefix + account_id);
// Write Samba configuration file.
ErrorType error = EnsureWorkgroupAndWriteSmbConf();
@@ -437,19 +429,14 @@
if (error != ERROR_NONE)
return error;
- // Note: This might create a new UserData() entry, which is fine.
- const std::string account_id_key(kActiveDirectoryPrefix +
- account_info.account_id());
- UserData& user_data_entry = user_data_[account_id_key];
-
// Determine the status of the password.
ActiveDirectoryUserStatus::PasswordStatus password_status =
- GetUserPasswordStatus(account_info, &user_data_entry.pwd_last_set_);
+ GetUserPasswordStatus(account_info);
*user_status->mutable_account_info() = account_info;
user_status->set_tgt_status(tgt_status);
user_status->set_password_status(password_status);
- user_status->set_last_auth_error(user_data_entry.last_auth_error_);
+ user_status->set_last_auth_error(last_auth_error_);
return ERROR_NONE;
}
@@ -526,15 +513,13 @@
ErrorType SambaInterface::FetchUserGpos(const std::string& account_id_key,
std::string* policy_blob) {
ReloadDebugFlags();
+ SetUser(account_id_key);
- // Get sAMAccountName from account id key (must be logged in to fetch user
- // policy).
- auto iter = user_data_.find(account_id_key);
- if (iter == user_data_.end() || iter->second.sam_account_name_.empty()) {
+ if (!user_logged_in_) {
LOG(ERROR) << "User not logged in. Please call AuthenticateUser first.";
return ERROR_NOT_LOGGED_IN;
}
- const std::string& sam_account_name = iter->second.sam_account_name_;
+ DCHECK(!user_sam_account_name_.empty());
// Write Samba configuration file.
ErrorType error = EnsureWorkgroupAndWriteSmbConf();
@@ -552,7 +537,7 @@
// Get the list of GPOs for the given user name.
protos::GpoList gpo_list;
- error = GetGpoList(sam_account_name, PolicyScope::USER, &gpo_list);
+ error = GetGpoList(user_sam_account_name_, PolicyScope::USER, &gpo_list);
if (error != ERROR_NONE)
return error;
@@ -699,8 +684,7 @@
}
ActiveDirectoryUserStatus::PasswordStatus SambaInterface::GetUserPasswordStatus(
- const ActiveDirectoryAccountInfo& account_info,
- uint64_t* prev_pw_last_set) {
+ const ActiveDirectoryAccountInfo& account_info) {
// See https://msdn.microsoft.com/en-us/library/ms679430(v=vs.85).aspx.
// Password is always valid if it never expires.
@@ -711,17 +695,17 @@
if (account_info.pwd_last_set() == 0)
return ActiveDirectoryUserStatus::PASSWORD_EXPIRED;
- // Memorize pwd_last_set if it wasn't set yet. This happens after
- // the password expired and was reset by AuthenticateUser().
- if (*prev_pw_last_set == 0) {
- *prev_pw_last_set = account_info.pwd_last_set();
+ // Memorize pwd_last_set if it wasn't set yet. This happens after the password
+ // expired and was reset by AuthenticateUser().
+ if (user_pwd_last_set_ == 0) {
+ user_pwd_last_set_ = account_info.pwd_last_set();
return ActiveDirectoryUserStatus::PASSWORD_VALID;
}
// Password changed on the server. Note: Don't update pwd_last_set_ here,
// update it in AuthenticateUser() when we know that Chrome sent the right
// password.
- if (*prev_pw_last_set != account_info.pwd_last_set())
+ if (user_pwd_last_set_ != account_info.pwd_last_set())
return ActiveDirectoryUserStatus::PASSWORD_CHANGED;
// pwd_last_set did not change, password is still valid.
@@ -1304,6 +1288,14 @@
return ERROR_NONE;
}
+void SambaInterface::SetUser(const std::string& account_id_key) {
+ // Don't allow authenticating multiple users. Chrome should prevent that.
+ CHECK(!account_id_key.empty());
+ CHECK(user_account_id_key_.empty() || user_account_id_key_ == account_id_key)
+ << "Multi-user not supported";
+ user_account_id_key_ = account_id_key;
+}
+
void SambaInterface::AnonymizeRealm(const std::string& realm) {
anonymizer_->SetReplacementAllCases(realm, kRealmPlaceholder);
@@ -1314,7 +1306,10 @@
}
void SambaInterface::Reset() {
- user_data_.clear();
+ user_account_id_key_.clear();
+ user_sam_account_name_.clear();
+ user_pwd_last_set_ = 0;
+ user_logged_in_ = false;
config_.reset();
workgroup_.clear();
retry_machine_kinit_ = false;
diff --git a/authpolicy/samba_interface.h b/authpolicy/samba_interface.h
index be94bed..4872e27 100644
--- a/authpolicy/samba_interface.h
+++ b/authpolicy/samba_interface.h
@@ -134,12 +134,10 @@
// Does not perform any server-side checks.
ErrorType GetUserTgtStatus(ActiveDirectoryUserStatus::TgtStatus* tgt_status);
- // Determines the password status by comparing the store last change timestamp
- // to the timestamp we just got from the server. |prev_pw_last_set| is the
- // timestamp of the last password change stored in UserData.
+ // Determines the password status by comparing the old |user_pwd_last_set_|
+ // timestamp to the new timestamp in |account_info|.
ActiveDirectoryUserStatus::PasswordStatus GetUserPasswordStatus(
- const ActiveDirectoryAccountInfo& account_info,
- uint64_t* prev_pw_last_set);
+ const ActiveDirectoryAccountInfo& account_info);
// Retrieves the name of the workgroup. Since the workgroup is expected to
// change very rarely, this function earlies out and returns ERROR_NONE if the
@@ -202,6 +200,11 @@
const char* parser_cmd_string,
std::string* policy_blob) const;
+ // Sets and fixes the current user by account id key. Only one account id is
+ // allowed per user. Calling this multiple times with different account ids
+ // crashes the daemon.
+ void SetUser(const std::string& account_id_key);
+
// Anonymizes |realm| in different capitalizations as well as all parts (e.g.
// if realm is SOME.EXAMPLE.COM, anonymizes SOME, EXAMPLE and COM.
void AnonymizeRealm(const std::string& realm);
@@ -224,18 +227,17 @@
// file does not exist, so this is no performance concern.
void ReloadDebugFlags();
- // Data we memorize for each user.
- struct UserData {
- std::string sam_account_name_; // Logon name.
- uint64_t pwd_last_set_ = 0; // Timestamp of last password change on server.
- ErrorType last_auth_error_ = ERROR_NONE; // Last AuthenticateUser() error.
- UserData() = default;
- UserData(const std::string& sam_account_name, uint64_t pwd_last_set)
- : sam_account_name_(sam_account_name), pwd_last_set_(pwd_last_set) {}
- };
+ // User account_id (aka objectGUID) prefixed by "a-".
+ std::string user_account_id_key_;
+ // User logon name.
+ std::string user_sam_account_name_;
+ // Timestamp of last password change on server.
+ uint64_t user_pwd_last_set_ = 0;
+ // Is the user logged in?
+ bool user_logged_in_ = false;
+ // Last AuthenticateUser() error.
+ ErrorType last_auth_error_ = ERROR_NONE;
- // Maps the account id key ("a-" + user object GUID) to user data.
- std::unordered_map<std::string, UserData> user_data_;
std::unique_ptr<protos::ActiveDirectoryConfig> config_;
std::string workgroup_;
diff --git a/authpolicy/stub_common.cc b/authpolicy/stub_common.cc
index d07ca79..b71a95d 100644
--- a/authpolicy/stub_common.cc
+++ b/authpolicy/stub_common.cc
@@ -37,6 +37,7 @@
// Should still be valid GUIDs, so GuidToOctetString() works.
const char kAccountId[] = "f892eb9d-9e11-4a74-b894-0647e218c4df";
+const char kAltAccountId[] = "21094d26-9e11-4a74-b894-c8cd12a6f83b";
const char kBadAccountId[] = "88adef4f-74ec-420d-b0a5-3726dbe711eb";
const char kExpiredPasswordAccountId[] = "21094d26-2720-4ba4-942c-c8cd12a6f83b";
const char kNeverExpirePasswordAccountId[] =
diff --git a/authpolicy/stub_common.h b/authpolicy/stub_common.h
index d257fc1..0708502 100644
--- a/authpolicy/stub_common.h
+++ b/authpolicy/stub_common.h
@@ -51,6 +51,8 @@
// Default, valid account id (aka objectGUID).
extern const char kAccountId[];
+// Alternative, valid account id.
+extern const char kAltAccountId[];
// Triggers a net ads search error when searching for this objectGUID.
extern const char kBadAccountId[];
// Triggers pwdLastSet=0 in net ads search.
