Handle password placeholder in KerberosAccounts policy

If ${PASSWORD} is present in KerberosAccounts, store the login password
in the kernel keyring. The Kerberos system daemon will then grab the
password to authenticate to a Kerberos account.

Also moves the call to InitializePrimaryProfileServices to an earlier
place. The KerberosCredentialsManager, which is created there, has to be
created when the login password is still stored in the user context, so
that UserSessionManager has a chance to send it to the session manager
(see SaveLoginPassword). However, the call happened when the password
was already cleared, see user_context_.ClearSecrets() in
FinalizePrepareProfile.

BUG=chromium:952240
TEST=Manually tested on device (enabled KerberosEnabled policy, set
      ${PASSWORD} as password for an account in KerberosAccounts, changed
      Gaia password to patch Kerberos password, logged in, verified in
      chrome:settings/kerberosAccounts that the ticket was valid.

Change-Id: I3d82b293c3d236b03928cd5cd75edbea6c8999a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1656570Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: May Lippert <maybelle@chromium.org>
Commit-Queue: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671983}

chrome/browser/browser_process_platform_part_chromeos.cc

@@ -153,19 +153,16 @@ void BrowserProcessPlatformPart::ShutdownCrosComponentManager() {
 void BrowserProcessPlatformPart::InitializePrimaryProfileServices(
     Profile* primary_profile) {
   DCHECK(primary_profile);
-  const user_manager::User* primary_user =
-      chromeos::ProfileHelper::Get()->GetUserByProfile(primary_profile);
-  DCHECK(primary_user);
 
   DCHECK(!kerberos_credentials_manager_);
   kerberos_credentials_manager_ =
       std::make_unique<chromeos::KerberosCredentialsManager>(
-          g_browser_process->local_state(), primary_user);
+          g_browser_process->local_state(), primary_profile);
 
   DCHECK(!in_session_password_change_manager_);
   in_session_password_change_manager_ =
-      chromeos::InSessionPasswordChangeManager::CreateIfEnabled(primary_profile,
-                                                                primary_user);
+      chromeos::InSessionPasswordChangeManager::CreateIfEnabled(
+          primary_profile);
 
   primary_profile_shutdown_subscription_ =
       PrimaryProfileServicesShutdownNotifierFactory::GetInstance()

chrome/browser/chromeos/kerberos/kerberos_credentials_manager.cc

@@ -10,6 +10,10 @@
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/authpolicy/data_pipe_utils.h"
+#include "chrome/browser/chromeos/login/session/user_session_manager.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/policy/profile_policy_connector.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/dbus/kerberos/kerberos_client.h"
 #include "chromeos/dbus/kerberos/kerberos_service.pb.h"
@@ -37,6 +41,9 @@ constexpr char kKrb5Conf[] = "krb5conf";
 constexpr char kLoginId[] = "LOGIN_ID";
 constexpr char kLoginEmail[] = "LOGIN_EMAIL";
 
+// Password placeholder.
+constexpr char kLoginPasswordPlaceholder[] = "${PASSWORD}";
+
 // Default encryption with strong encryption.
 constexpr char kDefaultKerberosConfig[] = R"([libdefaults]
   default_tgs_enctypes = aes256-cts-hmac-sha1-96 aes128-cts-hmac-sha1-96
@@ -95,14 +102,18 @@ class KerberosAddAccountRunner {
   // Kicks off the flow to add (or re-authenticate) a Kerberos account.
   // |manager| is a non-owned pointer to the owning manager.
   // |normalized_principal| is the normalized user principal name, e.g.
-  // user@REALM.COM. |is_managed| is true for accounts set by admins via policy.
-  // |password| is the password of the account. If |remember_password| is true,
-  // the password is remembered by the daemon. |krb5_conf| is set as
-  // configuration. If |allow_existing| is false and an account for
-  // |principal_name| already exists, no action is performed and the method
-  // returns with ERROR_DUPLICATE_PRINCIPAL_NAME. If true, the existing account
-  // is updated. |callback| is called by OnAddAccountRunnerDone() at the end of
-  // the flow, see class description.
+  //   user@REALM.COM.
+  // |is_managed| is true for accounts set by admins via policy.
+  // |password| is the password of the account. If it matches "${PASSWORD}" and
+  //   the account is managed, the login password is used.
+  // If |remember_password| is true, the password is remembered by the daemon.
+  //   The flag has effect when the login password is used.
+  // |krb5_conf| is set as configuration.
+  // If |allow_existing| is false and an account for |principal_name| already
+  //   exists, no action is performed and the method returns with
+  //   ERROR_DUPLICATE_PRINCIPAL_NAME. If true, the existing account is updated.
+  // |callback| is called by OnAddAccountRunnerDone() at the end of the flow,
+  //   see class description.
   KerberosAddAccountRunner(KerberosCredentialsManager* manager,
                            std::string normalized_principal,
                            bool is_managed,
@@ -171,7 +182,8 @@ class KerberosAddAccountRunner {
   }
 
   // Authenticates |normalized_principal_| using |password_| if |password_| is
-  // set. Otherwise, continues with Done().
+  // set. Otherwise, continues with Done(). If |password_| is "${PASSWORD}" and
+  // the account is managed, the login password is used.
   void MaybeAcquireKerberosTgt() {
     if (!password_) {
       Done(kerberos::ERROR_NONE);
@@ -181,6 +193,8 @@ class KerberosAddAccountRunner {
     kerberos::AcquireKerberosTgtRequest request;
     request.set_principal_name(normalized_principal_);
     request.set_remember_password(remember_password_);
+    request.set_use_login_password(is_managed_ &&
+                                   *password_ == kLoginPasswordPlaceholder);
     KerberosClient::Get()->AcquireKerberosTgt(
         request, data_pipe_utils::GetDataReadPipe(*password_).get(),
         base::BindOnce(&KerberosAddAccountRunner::OnAcquireKerberosTgt,
@@ -253,7 +267,7 @@ KerberosCredentialsManager::Observer::~Observer() = default;
 
 KerberosCredentialsManager::KerberosCredentialsManager(
     PrefService* local_state,
-    const user_manager::User* primary_user)
+    const Profile* primary_profile)
     : local_state_(local_state),
       kerberos_files_handler_(
           base::BindRepeating(&KerberosCredentialsManager::GetKerberosFiles,
@@ -261,10 +275,14 @@ KerberosCredentialsManager::KerberosCredentialsManager(
   DCHECK(!g_instance);
   g_instance = this;
 
+  DCHECK(primary_profile);
+  const user_manager::User* primary_user =
+      chromeos::ProfileHelper::Get()->GetUserByProfile(primary_profile);
+  DCHECK(primary_user);
+
   // Set up expansions:
   //   '${LOGIN_ID}'    -> 'user'
   //   '${LOGIN_EMAIL}' -> 'user@EXAMPLE.COM'
-  DCHECK(primary_user);
   std::map<std::string, std::string> substitutions;
   substitutions[kLoginId] =
       primary_user->GetAccountName(false /* use_display_email */);
@@ -300,10 +318,20 @@ KerberosCredentialsManager::KerberosCredentialsManager(
       base::BindRepeating(&KerberosCredentialsManager::UpdateAccountsFromPref,
                           weak_factory_.GetWeakPtr()));
 
-  UpdateAccountsFromPref();
+  // Update accounts if policy is already available or start observing.
+  policy_service_ =
+      primary_profile->GetProfilePolicyConnector()->policy_service();
+  const bool policy_initialized =
+      policy_service_->IsInitializationComplete(policy::POLICY_DOMAIN_CHROME);
+  VLOG(1) << "Policy service initialized at startup: " << policy_initialized;
+  if (policy_initialized)
+    UpdateAccountsFromPref();
+  else
+    policy_service_->AddObserver(policy::POLICY_DOMAIN_CHROME, this);
 }
 
 KerberosCredentialsManager::~KerberosCredentialsManager() {
+  policy_service_->RemoveObserver(policy::POLICY_DOMAIN_CHROME, this);
   DCHECK(g_instance);
   g_instance = nullptr;
 }
@@ -336,6 +364,24 @@ const char* KerberosCredentialsManager::GetDefaultKerberosConfig() {
   return kDefaultKerberosConfig;
 }
 
+void KerberosCredentialsManager::OnPolicyUpdated(
+    const policy::PolicyNamespace& ns,
+    const policy::PolicyMap& previous,
+    const policy::PolicyMap& current) {
+  // Ignore this call. Policy changes are already observed by the registrar.
+}
+
+void KerberosCredentialsManager::OnPolicyServiceInitialized(
+    policy::PolicyDomain domain) {
+  DCHECK(domain == policy::POLICY_DOMAIN_CHROME);
+
+  if (policy_service_->IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) {
+    VLOG(1) << "Policy service initialized";
+    policy_service_->RemoveObserver(policy::POLICY_DOMAIN_CHROME, this);
+    UpdateAccountsFromPref();
+  }
+}
+
 void KerberosCredentialsManager::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
@@ -578,6 +624,7 @@ void KerberosCredentialsManager::UpdateEnabledFromPref() {
   const bool enabled = local_state_->GetBoolean(prefs::kKerberosEnabled);
   if (!enabled) {
     // Note that ClearAccounts logs an error if the operation fails.
+    VLOG(1) << "Kerberos got disabled, clearing accounts";
     ClearAccounts(base::BindOnce([](kerberos::ErrorType) {}));
   }
 }
@@ -591,13 +638,21 @@ void KerberosCredentialsManager::UpdateAddAccountsAllowedFromPref() {
 }
 
 void KerberosCredentialsManager::UpdateAccountsFromPref() {
-  if (!local_state_->GetBoolean(prefs::kKerberosEnabled))
+  if (!local_state_->GetBoolean(prefs::kKerberosEnabled)) {
+    VLOG(1) << "Kerberos disabled";
+    NotifyRequiresLoginPassword(false);
     return;
+  }
 
   const base::Value* accounts = local_state_->GetList(prefs::kKerberosAccounts);
-  if (!accounts)
+  if (!accounts) {
+    VLOG(1) << "No KerberosAccounts policy";
+    NotifyRequiresLoginPassword(false);
     return;
+  }
 
+  VLOG(1) << accounts->GetList().size() << " accounts in KerberosAccounts";
+  bool requires_login_password = false;
   for (const auto& account : accounts->GetList()) {
     // Get the principal. Should always be set.
     const base::Value* principal_value = account.FindPath(kPrincipal);
@@ -622,9 +677,9 @@ void KerberosCredentialsManager::UpdateAccountsFromPref() {
     if (password_str)
       password = std::move(*password_str);
 
-    // Note: Password supports expansion of '${PASSWORD}' into the login
-    // password. This is done in the daemon, however, since Chrome forgets the
-    // password ASAP for security reasons.
+    // Keep track of whether any account has the '${PASSWORD}' placeholder.
+    if (password == kLoginPasswordPlaceholder)
+      requires_login_password = true;
 
     // Get the remember password flag, default to false.
     bool remember_password =
@@ -650,6 +705,17 @@ void KerberosCredentialsManager::UpdateAccountsFromPref() {
         this, principal, true /* is_managed */, password, remember_password,
         krb5_conf, true /* allow_existing */, EmptyResultCallback()));
   }
+
+  // Let UserSessionManager know whether it should keep the login password.
+  NotifyRequiresLoginPassword(requires_login_password);
+}
+
+void KerberosCredentialsManager::NotifyRequiresLoginPassword(
+    bool requires_login_password) {
+  VLOG(1) << "Requires login password: " << requires_login_password;
+  UserSessionManager::GetInstance()->VoteForSavingLoginPassword(
+      UserSessionManager::PasswordConsumingService::kKerberos,
+      requires_login_password);
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/kerberos/kerberos_credentials_manager.h

@@ -15,13 +15,16 @@
 #include "base/optional.h"
 #include "chrome/browser/chromeos/authpolicy/kerberos_files_handler.h"
 #include "chromeos/dbus/kerberos/kerberos_service.pb.h"
+#include "components/policy/core/common/policy_namespace.h"
+#include "components/policy/core/common/policy_service.h"
 
 class PrefRegistrySimple;
 class PrefService;
 class PrefChangeRegistrar;
+class Profile;
 
-namespace user_manager {
-class User;
+namespace policy {
+class PolicyMap;
 }
 
 namespace chromeos {
@@ -29,7 +32,7 @@ namespace chromeos {
 class KerberosAddAccountRunner;
 class VariableExpander;
 
-class KerberosCredentialsManager final {
+class KerberosCredentialsManager : public policy::PolicyService::Observer {
  public:
   using ResultCallback = base::OnceCallback<void(kerberos::ErrorType)>;
   using ListAccountsCallback =
@@ -49,8 +52,8 @@ class KerberosCredentialsManager final {
   };
 
   KerberosCredentialsManager(PrefService* local_state,
-                             const user_manager::User* primary_user);
-  ~KerberosCredentialsManager();
+                             const Profile* primary_profile);
+  ~KerberosCredentialsManager() override;
 
   // Singleton accessor. Available once the primary profile is available.
   // DCHECKs if the instance has not been created yet.
@@ -65,6 +68,14 @@ class KerberosCredentialsManager final {
   // Returns the default Kerberos configuration (krb5.conf).
   static const char* GetDefaultKerberosConfig();
 
+  // PolicyService:
+  void OnPolicyUpdated(const policy::PolicyNamespace& ns,
+                       const policy::PolicyMap& previous,
+                       const policy::PolicyMap& current) override;
+
+  // PolicyService:
+  void OnPolicyServiceInitialized(policy::PolicyDomain domain) override;
+
   // Start observing this object. |observer| is not owned.
   void AddObserver(Observer* observer);
 
@@ -170,9 +181,18 @@ class KerberosCredentialsManager final {
   void UpdateAddAccountsAllowedFromPref();
   void UpdateAccountsFromPref();
 
+  // Informs session manager whether it needs to store the login password in the
+  // kernel keyring. That's the case when '${PASSWORD}' is used as password in
+  // the KerberosAccounts policy. The Kerberos daemon expands that to the login
+  // password.
+  void NotifyRequiresLoginPassword(bool requires_login_password);
+
   // Local state prefs, not owned.
   PrefService* local_state_ = nullptr;
 
+  // Policy service of the primary profile, not owned.
+  policy::PolicyService* policy_service_ = nullptr;
+
   // Called by OnSignalConnected(), puts Kerberos files where GSSAPI finds them.
   KerberosFilesHandler kerberos_files_handler_;
 

chrome/browser/chromeos/login/saml/in_session_password_change_manager.cc

@@ -6,6 +6,7 @@
 
 #include "chrome/browser/chromeos/login/auth/chrome_cryptohome_authenticator.h"
 #include "chrome/browser/chromeos/login/saml/saml_password_expiry_notification.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/login/auth/user_context.h"
@@ -16,23 +17,21 @@ namespace chromeos {
 
 // static
 std::unique_ptr<InSessionPasswordChangeManager>
-InSessionPasswordChangeManager::CreateIfEnabled(
-    Profile* primary_profile,
-    const user_manager::User* primary_user) {
+InSessionPasswordChangeManager::CreateIfEnabled(Profile* primary_profile) {
   if (primary_profile->GetPrefs()->GetBoolean(
           prefs::kSamlInSessionPasswordChangeEnabled)) {
-    return std::make_unique<InSessionPasswordChangeManager>(primary_profile,
-                                                            primary_user);
+    return std::make_unique<InSessionPasswordChangeManager>(primary_profile);
   }
   return nullptr;
 }
 
 InSessionPasswordChangeManager::InSessionPasswordChangeManager(
-    Profile* primary_profile,
-    const user_manager::User* primary_user)
+    Profile* primary_profile)
     : primary_profile_(primary_profile),
-      primary_user_(primary_user),
-      authenticator_(new ChromeCryptohomeAuthenticator(this)) {}
+      primary_user_(ProfileHelper::Get()->GetUserByProfile(primary_profile)),
+      authenticator_(new ChromeCryptohomeAuthenticator(this)) {
+  DCHECK(primary_user_);
+}
 
 InSessionPasswordChangeManager::~InSessionPasswordChangeManager() {}
 

chrome/browser/chromeos/login/saml/in_session_password_change_manager.h

@@ -27,11 +27,9 @@ class InSessionPasswordChangeManager : public AuthStatusConsumer {
  public:
   // Returns null if in-session password change is disabled.
   static std::unique_ptr<InSessionPasswordChangeManager> CreateIfEnabled(
-      Profile* primary_profile,
-      const user_manager::User* primary_user);
+      Profile* primary_profile);
 
-  InSessionPasswordChangeManager(Profile* primary_profile,
-                                 const user_manager::User* primary_user);
+  explicit InSessionPasswordChangeManager(Profile* primary_profile);
   ~InSessionPasswordChangeManager() override;
 
   // Change cryptohome password for primary user.

chrome/browser/chromeos/login/session/chrome_session_manager.cc

@@ -112,6 +112,10 @@ void StartUserSession(Profile* user_profile, const std::string& login_user_id) {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
 
   if (command_line->HasSwitch(chromeos::switches::kLoginUser)) {
+    // TODO(https://crbug.com/977489): There's a lot of code duplication with
+    // UserSessionManager::FinalizePrepareProfile, which is (only!) run for
+    // regular session starts. This needs to be refactored.
+
     // This is done in SessionManager::OnProfileCreated during normal login.
     UserSessionManager* user_session_mgr = UserSessionManager::GetInstance();
     user_manager::UserManager* user_manager = user_manager::UserManager::Get();
@@ -158,6 +162,9 @@ void StartUserSession(Profile* user_profile, const std::string& login_user_id) {
     if (crostini_manager)
       crostini_manager->MaybeUpgradeCrostini();
 
+    g_browser_process->platform_part()->InitializePrimaryProfileServices(
+        user_profile);
+
     if (user->GetType() == user_manager::USER_TYPE_CHILD) {
       ConsumerStatusReportingServiceFactory::GetForBrowserContext(user_profile);
       ScreenTimeControllerFactory::GetForBrowserContext(user_profile);

chrome/browser/chromeos/login/session/user_session_manager.cc

@@ -1111,6 +1111,9 @@ void UserSessionManager::VoteForSavingLoginPassword(
     bool save_password) {
   DCHECK_LT(service, PasswordConsumingService::kCount);
 
+  VLOG(1) << "Password consuming service " << static_cast<size_t>(service)
+          << " votes " << save_password;
+
   // Prevent this code from being called twice from two services or else the
   // second service would trigger the warning below (since the password has been
   // cleared).
@@ -1118,8 +1121,7 @@ void UserSessionManager::VoteForSavingLoginPassword(
     password_was_saved_ = true;
     const std::string& password = user_context_.GetPasswordKey()->GetSecret();
     if (!password.empty()) {
-      VLOG(1) << "Saving login password for service "
-              << static_cast<size_t>(service);
+      VLOG(1) << "Saving login password";
       SessionManagerClient::Get()->SaveLoginPassword(password);
     } else {
       LOG(WARNING) << "Not saving password because password is empty.";
@@ -1546,6 +1548,10 @@ void UserSessionManager::FinalizePrepareProfile(Profile* profile) {
       content::NotificationService::AllSources(),
       content::Details<Profile>(profile));
 
+  // Initialize various services only for primary user.
+  // TODO(https://crbug.com/977489): There's a lot of code duplication with
+  // StartUserSession in chrome_session_manager.cc, which is (only!) run for
+  // session starts after crashes. This needs to be refactored.
   const user_manager::User* user =
       ProfileHelper::Get()->GetUserByProfile(profile);
   session_manager::SessionManager::Get()->NotifyUserProfileLoaded(
@@ -1573,6 +1579,9 @@ void UserSessionManager::FinalizePrepareProfile(Profile* profile) {
     if (crostini_manager)
       crostini_manager->MaybeUpgradeCrostini();
 
+    g_browser_process->platform_part()->InitializePrimaryProfileServices(
+        profile);
+
     TetherService* tether_service = TetherService::Get(profile);
     if (tether_service)
       tether_service->StartTetherIfPossible();
@@ -1592,6 +1601,7 @@ void UserSessionManager::FinalizePrepareProfile(Profile* profile) {
     login::SaveSyncPasswordDataToProfile(user_context_, profile);
   }
 
+  VLOG(1) << "Clearing all secrets";
   user_context_.ClearSecrets();
   if (TokenHandlesEnabled()) {
     CreateTokenUtilIfMissing();

chrome/browser/chromeos/login/session/user_session_manager.h

@@ -149,8 +149,12 @@ class UserSessionManager
     // the OpenNetworkConfiguration policy.
     kNetwork = 0,
 
+    // The Kerberos service needs the login password if ${PASSWORD} is specified
+    // somewhere in the KerberosAccounts policy.
+    kKerberos = 1,
+
     // Should be last. All enum values must be consecutive starting from 0.
-    kCount = 1,
+    kCount = 2,
   };
 
   // Returns UserSessionManager instance.
@@ -338,15 +342,17 @@ class UserSessionManager
   // Shows U2F notification if necessary.
   void MaybeShowU2FNotification();
 
+ protected:
+  // Protected for testability reasons.
+  UserSessionManager();
+  ~UserSessionManager() override;
+
  private:
   friend class test::UserSessionManagerTestApi;
   friend struct base::DefaultSingletonTraits<UserSessionManager>;
 
   typedef std::set<std::string> SigninSessionRestoreStateSet;
 
-  UserSessionManager();
-  ~UserSessionManager() override;
-
   void SetNetworkConnectionTracker(
       network::NetworkConnectionTracker* network_connection_tracker);
 

chrome/browser/chromeos/login/session/user_session_manager_test.cc

@@ -18,17 +18,36 @@
 namespace chromeos {
 
 namespace {
+
 constexpr char kFakePassword[] = "p4zzw0r(|";
-}
+
+// Publicly exposes lifetime methods. Note that the singleton instance
+// UserSessionManager::GetInstance() can't be used since it would be reused
+// between tests.
+class TestUserSessionManager : public UserSessionManager {
+ public:
+  TestUserSessionManager() = default;
+  ~TestUserSessionManager() override = default;
+};
+
+}  // namespace
 
 class UserSessionManagerTest : public testing::Test {
  public:
   UserSessionManagerTest() {
+    static_assert(
+        static_cast<int>(
+            UserSessionManager::PasswordConsumingService::kCount) == 2,
+        "Update PasswordConsumerService_* tests");
+
     SessionManagerClient::InitializeFake();
-    user_session_manager_ = UserSessionManager::GetInstance();
+    user_session_manager_ = std::make_unique<TestUserSessionManager>();
   }
 
-  ~UserSessionManagerTest() override { SessionManagerClient::Shutdown(); }
+  ~UserSessionManagerTest() override {
+    user_session_manager_.reset();
+    SessionManagerClient::Shutdown();
+  }
 
  protected:
   void InitLoginPassword() {
@@ -41,8 +60,13 @@ class UserSessionManagerTest : public testing::Test {
     EXPECT_TRUE(FakeSessionManagerClient::Get()->login_password().empty());
   }
 
-  // Convenience pointer to the singleton, not owned.
-  UserSessionManager* user_session_manager_;
+  // Convenience shortcut to the login password stored in
+  // |user_session_manager_|'s user context.
+  const std::string& GetUserSessionManagerLoginPassword() const {
+    return user_session_manager_->user_context().GetPasswordKey()->GetSecret();
+  }
+
+  std::unique_ptr<TestUserSessionManager> user_session_manager_;
 
   // Allows UserSessionManager to request the NetworkConnectionTracker in its
   // constructor.
@@ -60,26 +84,57 @@ class UserSessionManagerTest : public testing::Test {
 // and clear it from the user context.
 TEST_F(UserSessionManagerTest, PasswordConsumerService_NoSave) {
   InitLoginPassword();
+
+  // First service votes no: Should keep password in user context.
   user_session_manager_->VoteForSavingLoginPassword(
       UserSessionManager::PasswordConsumingService::kNetwork, false);
   EXPECT_TRUE(FakeSessionManagerClient::Get()->login_password().empty());
-  EXPECT_TRUE(user_session_manager_->user_context()
-                  .GetPasswordKey()
-                  ->GetSecret()
-                  .empty());
+  EXPECT_EQ(kFakePassword, GetUserSessionManagerLoginPassword());
+
+  // Second (last) service votes no: Should remove password from user context.
+  user_session_manager_->VoteForSavingLoginPassword(
+      UserSessionManager::PasswordConsumingService::kKerberos, false);
+  EXPECT_TRUE(FakeSessionManagerClient::Get()->login_password().empty());
+  EXPECT_TRUE(GetUserSessionManagerLoginPassword().empty());
 }
 
 // Calling VoteForSavingLoginPassword() with |save_password| set to true should
-// send the password to SessionManager and clear it from the user context.
+// send the password to SessionManager and clear it from the user context once
+// all services have voted.
 TEST_F(UserSessionManagerTest, PasswordConsumerService_Save) {
   InitLoginPassword();
+
+  // First service votes yes: Should send password and remove from user context.
   user_session_manager_->VoteForSavingLoginPassword(
       UserSessionManager::PasswordConsumingService::kNetwork, true);
   EXPECT_EQ(kFakePassword, FakeSessionManagerClient::Get()->login_password());
-  EXPECT_TRUE(user_session_manager_->user_context()
-                  .GetPasswordKey()
-                  ->GetSecret()
-                  .empty());
+  EXPECT_TRUE(GetUserSessionManagerLoginPassword().empty());
+
+  // Second service votes yes: Shouldn't change anything.
+  user_session_manager_->VoteForSavingLoginPassword(
+      UserSessionManager::PasswordConsumingService::kKerberos, true);
+  EXPECT_EQ(kFakePassword, FakeSessionManagerClient::Get()->login_password());
+  EXPECT_TRUE(GetUserSessionManagerLoginPassword().empty());
+}
+
+// Calling OnPasswordConsumingServicePolicyParsed() with |save_password| set to
+// false for one service, followed by true, should send the password to
+// SessionManager on the second service and clear it from the user context.
+TEST_F(UserSessionManagerTest, PasswordConsumerService_NoSave_Save) {
+  InitLoginPassword();
+
+  // First service votes no: Should keep password in user context.
+  user_session_manager_->VoteForSavingLoginPassword(
+      UserSessionManager::PasswordConsumingService::kNetwork, false);
+  EXPECT_TRUE(FakeSessionManagerClient::Get()->login_password().empty());
+  EXPECT_EQ(kFakePassword, GetUserSessionManagerLoginPassword());
+
+  // Second service votes yes: Should save password and remove from user
+  // context.
+  user_session_manager_->VoteForSavingLoginPassword(
+      UserSessionManager::PasswordConsumingService::kKerberos, true);
+  EXPECT_EQ(kFakePassword, FakeSessionManagerClient::Get()->login_password());
+  EXPECT_TRUE(GetUserSessionManagerLoginPassword().empty());
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/profiles/profile_helper.cc

@@ -255,11 +255,6 @@ void ProfileHelper::ProfileStartup(Profile* profile) {
   // GetOffTheRecordProfile() call above.
   profile->InitChromeOSPreferences();
 
-  if (IsPrimaryProfile(profile)) {
-    g_browser_process->platform_part()->InitializePrimaryProfileServices(
-        profile);
-  }
-
   // Add observer so we can see when the first profile's session restore is
   // completed. After that, we won't need the default profile anymore.
   if (!IsSigninProfile(profile) &&

chromeos/dbus/kerberos/fake_kerberos_client.cc

@@ -135,18 +135,27 @@ void FakeKerberosClient::AcquireKerberosTgt(
     return;
   }
 
-  // Remember password.
-  std::string password = ReadPassword(password_fd);
-  if (!password.empty() && request.remember_password())
-    data->password = password;
-
-  // Use remembered password.
-  if (password.empty())
-    password = data->password;
+  std::string password;
+  if (request.use_login_password()) {
+    // "Retrieve" login password.
+    password = "fake_login_password";
 
-  // Erase a previously remembered password.
-  if (!request.remember_password())
+    // Erase a previously remembered password.
     data->password.clear();
+  } else {
+    // Remember password.
+    password = ReadPassword(password_fd);
+    if (!password.empty() && request.remember_password())
+      data->password = password;
+
+    // Use remembered password.
+    if (password.empty())
+      password = data->password;
+
+    // Erase a previously remembered password.
+    if (!request.remember_password())
+      data->password.clear();
+  }
 
   // Reject empty passwords.
   if (password.empty()) {

chromeos/login/auth/user_context.h

@@ -60,8 +60,7 @@ class COMPONENT_EXPORT(CHROMEOS_LOGIN_AUTH) UserContext {
   // its hashed/transformed representation.
   const Key* GetKey() const;
   Key* GetKey();
-  // The plain-text user password. Initialized only on enterprise enrolled
-  // devices. See https://crbug.com/386606.
+  // The plain-text user password. See https://crbug.com/386606.
   const Key* GetPasswordKey() const;
   Key* GetMutablePasswordKey();
   // The challenge-response keys for user authentication. Currently, such keys

components/policy/core/common/policy_service_impl.cc

@@ -129,10 +129,8 @@ void PolicyServiceImpl::RemoveObserver(PolicyDomain domain,
                                        PolicyService::Observer* observer) {
   DCHECK(thread_checker_.CalledOnValidThread());
   auto it = observers_.find(domain);
-  if (it == observers_.end()) {
-    NOTREACHED();
+  if (it == observers_.end())
     return;
-  }
   it->second->RemoveObserver(observer);
   if (!it->second->might_have_observers()) {
     observers_.erase(it);