Allow multi sign-in even when policy-pushed trusted CA certs are active

Historically, multi sign-in was disallowed when either the primary
Profile or the Profile being added used / had used policy-pushed CA
certificates. The reason for limiting this was that the certificate
verifier used at that time (NSS) had process-wide caches, so certificate
verification results could leak across Profiles.

Now that we use the built-in certificate verifier, this risk does not
exist, so allow multi sign-in regardless of the policy-pushed
certificate state of the primary / secondary Profiles.

Also move clearing of the local_state pref that tracks policy-pushed
certificate usage for a Profile into UserSessionManagerImpl because
MultiProfileUserController does not have a dependency on the
policy-pushed certs infrastructure anymore. In a follow-up the pref can
likely be migrated to be a user Profile pref now instead of a
local_state pref.

Bug: 718002
Test: unit tests, browser tests
Change-Id: I1006e0cb1cb4c9fdd68f5373e18acb27f5c3eaea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2297566
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808879}

chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

@@ -58,6 +58,7 @@
 #include "chrome/browser/chromeos/policy/external_data_handlers/printers_external_data_handler.h"
 #include "chrome/browser/chromeos/policy/external_data_handlers/user_avatar_image_external_data_handler.h"
 #include "chrome/browser/chromeos/policy/external_data_handlers/wallpaper_image_external_data_handler.h"
+#include "chrome/browser/chromeos/policy/policy_cert_service_factory.h"
 #include "chrome/browser/chromeos/policy/user_network_configuration_updater.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/session_length_limiter.h"
@@ -1019,6 +1020,9 @@ void ChromeUserManagerImpl::RemoveNonCryptohomeData(
 
   multi_profile_user_controller_->RemoveCachedValues(account_id.GetUserEmail());
 
+  policy::PolicyCertServiceFactory::ClearUsedPolicyCertificates(
+      account_id.GetUserEmail());
+
   EasyUnlockService::ResetLocalStateForUser(account_id);
 
   ChromeUserManager::RemoveNonCryptohomeData(account_id);

chrome/browser/chromeos/login/users/multi_profile_user_controller.cc

@@ -9,8 +9,6 @@
 #include "ash/public/cpp/login_types.h"
 #include "base/bind.h"
 #include "chrome/browser/chromeos/login/users/multi_profile_user_controller_delegate.h"
-#include "chrome/browser/chromeos/policy/policy_cert_service.h"
-#include "chrome/browser/chromeos/policy/policy_cert_service_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
@@ -92,27 +90,10 @@ MultiProfileUserController::GetPrimaryUserPolicy() {
   if (!user)
     return ALLOWED;
 
-  // Don't allow any secondary profiles if the primary profile is tainted.
-  if (policy::PolicyCertServiceFactory::UsedPolicyCertificates(
-          user->GetAccountId().GetUserEmail())) {
-    // Check directly in local_state before checking if the primary user has
-    // a PolicyCertService. Their profile may have been tainted previously
-    // though they didn't get a PolicyCertService created for this session.
-    return NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED;
-  }
-
   Profile* profile = ProfileHelper::Get()->GetProfileByUser(user);
   if (!profile)
     return ALLOWED;
 
-  // If the primary profile already has policy certificates installed but
-  // hasn't used them yet then it can become tainted at any time during this
-  // session disable secondary profiles in this case too.
-  policy::PolicyCertService* service =
-      policy::PolicyCertServiceFactory::GetForProfile(profile);
-  if (service && service->has_policy_certificates())
-    return NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED;
-
   // No user is allowed if the primary user policy forbids it.
   const std::string behavior =
       profile->GetPrefs()->GetString(prefs::kMultiProfileUserBehavior);
@@ -152,11 +133,6 @@ bool MultiProfileUserController::IsUserAllowedInSession(
   if (primary_user_email.empty() || primary_user_email == user_email)
     return SetUserAllowedReason(reason, ALLOWED);
 
-  // Don't allow profiles potentially tainted by data fetched with policy-pushed
-  // certificates to join a multiprofile session.
-  if (policy::PolicyCertServiceFactory::UsedPolicyCertificates(user_email))
-    return SetUserAllowedReason(reason, NOT_ALLOWED_POLICY_CERT_TAINTED);
-
   UserAllowedInSessionReason primary_user_policy = GetPrimaryUserPolicy();
   if (primary_user_policy != ALLOWED)
     return SetUserAllowedReason(reason, primary_user_policy);
@@ -188,7 +164,6 @@ void MultiProfileUserController::RemoveCachedValues(
   DictionaryPrefUpdate update(local_state_,
                               prefs::kCachedMultiProfileUserBehavior);
   update->RemoveWithoutPathExpansion(user_email, NULL);
-  policy::PolicyCertServiceFactory::ClearUsedPolicyCertificates(user_email);
 }
 
 std::string MultiProfileUserController::GetCachedValue(

chrome/browser/chromeos/login/users/multi_profile_user_controller.h

@@ -42,14 +42,6 @@ class MultiProfileUserController {
     // Owner of the device is not allowed to be added as a secondary user.
     NOT_ALLOWED_OWNER_AS_SECONDARY,
 
-    // Not allowed since it is potentially "tainted" with policy-pushed
-    // certificates.
-    NOT_ALLOWED_POLICY_CERT_TAINTED,
-
-    // Not allowed since primary user is already "tainted" with policy-pushed
-    // certificates.
-    NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
-
     // Not allowed since primary user policy forbids it to be part of
     // multi-profiles session.
     NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,

chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc

@@ -348,9 +348,9 @@ TEST_F(MultiProfileUserControllerTest,
 }
 
 TEST_F(MultiProfileUserControllerTest,
-       UsedPolicyCertificatesDisallowedForSecondary) {
+       UsedPolicyCertificatesAllowedForSecondary) {
   // Verifies that if a regular user is signed-in then other regular users can
-  // be added but tainted users can't.
+  // be added, including users that have used policy-provided trust anchors.
   LoginUser(1);
 
   // TODO(xiyuan): Remove the following SetPrefBehavor when default is
@@ -364,16 +364,15 @@ TEST_F(MultiProfileUserControllerTest,
 
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(
       test_users_[0].GetUserEmail());
-  EXPECT_FALSE(controller()->IsUserAllowedInSession(
+  EXPECT_TRUE(controller()->IsUserAllowedInSession(
       test_users_[0].GetUserEmail(), &reason));
-  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_POLICY_CERT_TAINTED,
-            reason);
+  EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
 }
 
 TEST_F(MultiProfileUserControllerTest,
-       UsedPolicyCertificatesDisallowsSecondaries) {
-  // Verifies that if a tainted user is signed-in then no other users can
-  // be added.
+       SecondaryAllowedWhenPrimaryUsedPolicyCertificates) {
+  // Verifies that if a tainted user is signed-in then other users can still be
+  // added.
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(
       test_users_[0].GetUserEmail());
   LoginUser(0);
@@ -383,19 +382,17 @@ TEST_F(MultiProfileUserControllerTest,
           profile(0), base::BindRepeating(&TestPolicyCertServiceFactory)));
 
   MultiProfileUserController::UserAllowedInSessionReason reason;
-  EXPECT_FALSE(controller()->IsUserAllowedInSession(
+  EXPECT_TRUE(controller()->IsUserAllowedInSession(
       test_users_[1].GetUserEmail(), &reason));
-  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
-            reason);
-  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
+  EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
+  EXPECT_EQ(MultiProfileUserController::ALLOWED,
             MultiProfileUserController::GetPrimaryUserPolicy());
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(
       test_users_[1].GetUserEmail());
-  EXPECT_FALSE(controller()->IsUserAllowedInSession(
+  EXPECT_TRUE(controller()->IsUserAllowedInSession(
       test_users_[1].GetUserEmail(), &reason));
-  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_POLICY_CERT_TAINTED,
-            reason);
-  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
+  EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
+  EXPECT_EQ(MultiProfileUserController::ALLOWED,
             MultiProfileUserController::GetPrimaryUserPolicy());
 
   // Flush tasks posted to IO.
@@ -405,7 +402,7 @@ TEST_F(MultiProfileUserControllerTest,
 TEST_F(MultiProfileUserControllerTest,
        PolicyCertificatesInMemoryDisallowsSecondaries) {
   // Verifies that if a user is signed-in and has policy certificates installed
-  // then no other users can be added.
+  // then other users can still be added.
   LoginUser(0);
 
   // TODO(xiyuan): Remove the following SetPrefBehavor when default is
@@ -432,11 +429,10 @@ TEST_F(MultiProfileUserControllerTest,
       net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"));
   service->SetPolicyTrustAnchorsForTesting(/*trust_anchors=*/certificates);
   EXPECT_TRUE(service->has_policy_certificates());
-  EXPECT_FALSE(controller()->IsUserAllowedInSession(
+  EXPECT_TRUE(controller()->IsUserAllowedInSession(
       test_users_[1].GetUserEmail(), &reason));
-  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
-            reason);
-  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
+  EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
+  EXPECT_EQ(MultiProfileUserController::ALLOWED,
             MultiProfileUserController::GetPrimaryUserPolicy());
 
   // Flush tasks posted to IO.

chrome/browser/ui/ash/session_controller_client_impl_unittest.cc

@@ -281,9 +281,10 @@ TEST_F(SessionControllerClientImplTest, MultiProfileDisallowedByUserPolicy) {
             SessionControllerClientImpl::GetAddUserSessionPolicy());
 }
 
-// Make sure MultiProfile disabled by primary user policy certificates.
+// Make sure MultiProfile is allowed if the primary user has used
+// policy-provided trust anchors.
 TEST_F(SessionControllerClientImplTest,
-       MultiProfileDisallowedByPolicyCertificates) {
+       MultiProfileAllowedWithPolicyCertificates) {
   InitForMultiProfile();
   user_manager()->AddUser(
       AccountId::FromUserEmailGaiaId("bb@b.b", "4444444444"));
@@ -295,14 +296,15 @@ TEST_F(SessionControllerClientImplTest,
             SessionControllerClientImpl::GetAddUserSessionPolicy());
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(
       account_id.GetUserEmail());
-  EXPECT_EQ(ash::AddUserSessionPolicy::ERROR_NOT_ALLOWED_PRIMARY_USER,
+  EXPECT_EQ(ash::AddUserSessionPolicy::ALLOWED,
             SessionControllerClientImpl::GetAddUserSessionPolicy());
 
   // Flush tasks posted to IO.
   base::RunLoop().RunUntilIdle();
 }
 
-// Make sure MultiProfile disabled by primary user certificates in memory.
+// Make sure MultiProfile is allowed if the primary user has policy-provided
+// trust anchors in memory.
 TEST_F(SessionControllerClientImplTest,
        MultiProfileDisallowedByPrimaryUserCertificatesInMemory) {
   TestingProfile* user_profile = InitForMultiProfile();
@@ -327,7 +329,7 @@ TEST_F(SessionControllerClientImplTest,
       net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"));
   service->SetPolicyTrustAnchorsForTesting(/*trust_anchors=*/certificates);
   EXPECT_TRUE(service->has_policy_certificates());
-  EXPECT_EQ(ash::AddUserSessionPolicy::ERROR_NOT_ALLOWED_PRIMARY_USER,
+  EXPECT_EQ(ash::AddUserSessionPolicy::ALLOWED,
             SessionControllerClientImpl::GetAddUserSessionPolicy());
 
   // Flush tasks posted to IO.