Certificate Management UI V2: honor client cert enterprise policy part 1

* UI hides "import" and "import and bind" buttons if not allowed.
* Backend checks policy during import and delete operations.
* UI doesn't yet hide the "delete" icon in the certificate list when
  not allowed, will leave that for a later CL.

Bug: 40928765
Change-Id: I5bfbc7f7d39601c033182ac66e6c27bfc4732323
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5882130
Reviewed-by: Hubert Chao <hchao@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1359613}

chrome/browser/ui/webui/certificate_manager/certificate_manager_ui.cc

@@ -18,6 +18,10 @@
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_data_source.h"
 
+#if BUILDFLAG(CHROME_ROOT_STORE_CERT_MANAGEMENT_UI)
+#include "chrome/browser/ui/webui/certificate_manager/client_cert_sources.h"
+#endif
+
 #if BUILDFLAG(IS_CHROMEOS_ASH)
 #include "chrome/browser/ui/webui/certificate_provisioning_ui_handler.h"
 #include "chrome/browser/ui/webui/certificates_handler.h"
@@ -167,6 +171,17 @@ CertificateManagerUI::CertificateManagerUI(content::WebUI* web_ui)
     source->AddResourcePath("", IDR_CERT_MANAGER_DIALOG_V2_HTML);
     AddCertificateManagerV2Strings(source);
     source->AddString("crsLearnMoreUrl", kCRSLearnMoreLink);
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+    ClientCertManagementAccessControls client_cert_policy(profile);
+    source->AddBoolean(
+        "clientCertImportAllowed",
+        client_cert_policy.IsManagementAllowed(
+            ClientCertManagementAccessControls::kSoftwareBacked));
+    source->AddBoolean(
+        "clientCertImportAndBindAllowed",
+        client_cert_policy.IsManagementAllowed(
+            ClientCertManagementAccessControls::kHardwareBacked));
+#endif
 
     auto plural_string_handler = std::make_unique<PluralStringHandler>();
     plural_string_handler->AddLocalizedString(

chrome/browser/ui/webui/certificate_manager/certificate_manager_utils.h

@@ -10,6 +10,33 @@
 #include "base/memory/weak_ptr.h"
 #include "content/public/browser/web_contents.h"
 
+#if BUILDFLAG(IS_CHROMEOS)
+// Enumeration of certificate management permissions which corresponds to
+// values of policy ClientCertificateManagementAllowed.
+// Underlying type is int because values are casting to/from prefs values.
+enum class ClientCertificateManagementPermission : int {
+  // Allow users to manage all certificates
+  kAll = 0,
+  // Allow users to manage user certificates
+  kUserOnly = 1,
+  // Disallow users from managing certificates
+  kNone = 2
+};
+
+// Enumeration of certificate management permissions which corresponds to
+// values of policy CACertificateManagementAllowed.
+// Underlying type is int because values are casting to/from prefs values.
+enum class CACertificateManagementPermission : int {
+  // Allow users to manage all certificates
+  kAll = 0,
+  // Allow users to manage user certificates
+  kUserOnly = 1,
+  // Disallow users from managing certificates
+  kNone = 2
+};
+#endif  // BUILDFLAG(IS_CHROMEOS)
+
 void ShowCertificateDialog(base::WeakPtr<content::WebContents> web_contents,
                            bssl::UniquePtr<CRYPTO_BUFFER> cert);
+
 #endif  // CHROME_BROWSER_UI_WEBUI_CERTIFICATE_MANAGER_CERTIFICATE_MANAGER_UTILS_H_

chrome/browser/ui/webui/certificate_manager/client_cert_sources.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/ui/chrome_select_file_policy.h"
 #include "chrome/browser/ui/webui/certificate_manager/certificate_manager_utils.h"
 #include "chrome/common/net/x509_certificate_model.h"
+#include "chrome/common/pref_names.h"
 #include "content/public/browser/browser_thread.h"
 #include "crypto/crypto_buildflags.h"
 #include "crypto/sha2.h"
@@ -70,6 +71,7 @@
 #include "chrome/browser/net/nss_service_factory.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "components/user_manager/user.h"
+#include "components/user_manager/user_manager.h"
 #include "net/cert/nss_cert_database.h"
 #endif
 
@@ -347,14 +349,8 @@ class ClientCertSource : public CertificateManagerPageHandler::CertSource {
   void ReplyToGetCertificatesCallback(
       CertificateManagerPageHandler::GetCertificatesCallback callback) const {
 #if BUILDFLAG(IS_CHROMEOS_ASH)
-    // TODO(crbug.com/40928765): Double check if there are any conditions where
-    // this would be false. It doesn't seem like there are any - the user NSS
-    // slots should always be opened in readwrite mode, and the enterprise
-    // policy restricting cert management doesn't apply to client certs. There
-    // might be cases like kiosk mode, but in that case the user shouldn't have
-    // been able to import in the first place? (If there are cases, it should
-    // be double checked in the import callback too, not just assuming the data
-    // from the webui is reliable.)
+    // TODO(crbug.com/40928765): This should actually be set by checking
+    // ClientCertManagementAccessControls.IsChangeAllowed on a per-cert basis.
     const bool is_deletable = true;
 #else
     const bool is_deletable = false;
@@ -435,6 +431,17 @@ class CrosClientCertSource : public ClientCertSource,
       return;
     }
 
+    if (!ClientCertManagementAccessControls(profile_).IsManagementAllowed(
+            hardware_backed
+                ? ClientCertManagementAccessControls::kHardwareBacked
+                : ClientCertManagementAccessControls::kSoftwareBacked)) {
+      // TODO(crbug.com/40928765): localize? This is an internal error that
+      // isn't expected to be displayed, so dunno if it needs to be localized.
+      std::move(callback).Run(
+          certificate_manager_v2::mojom::ActionResult::NewError("not allowed"));
+      return;
+    }
+
     import_hardware_backed_ = hardware_backed;
     import_callback_ = std::move(callback);
 
@@ -670,7 +677,7 @@ class CrosClientCertSource : public ClientCertSource,
                 ->CreateNSSCertDatabaseGetterForIOThread(),
             base::BindOnce(
                 &CrosClientCertSource::GotNSSCertDatabaseForDeleteOnIOThread,
-                cert,
+                cert, ClientCertManagementAccessControls(profile_),
                 base::BindOnce(&CrosClientCertSource::FinishedDelete,
                                weak_ptr_factory_.GetWeakPtr(),
                                std::move(callback)))));
@@ -678,6 +685,7 @@ class CrosClientCertSource : public ClientCertSource,
 
   static void GotNSSCertDatabaseForDeleteOnIOThread(
       scoped_refptr<net::X509Certificate> cert,
+      ClientCertManagementAccessControls client_cert_policy,
       base::OnceCallback<void(bool nss_delete_result)> finished_delete_callback,
       net::NSSCertDatabase* cert_db) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
@@ -692,6 +700,22 @@ class CrosClientCertSource : public ClientCertSource,
       return;
     }
 
+    const auto hardware_backed =
+        cert_db->IsHardwareBacked(nss_cert.get())
+            ? ClientCertManagementAccessControls::kHardwareBacked
+            : ClientCertManagementAccessControls::kSoftwareBacked;
+    const auto device_wide =
+        cert_db->IsCertificateOnSlot(nss_cert.get(),
+                                     cert_db->GetSystemSlot().get())
+            ? ClientCertManagementAccessControls::kDeviceWide
+            : ClientCertManagementAccessControls::kUser;
+    if (!client_cert_policy.IsChangeAllowed(hardware_backed, device_wide)) {
+      content::GetUIThreadTaskRunner({})->PostTask(
+          FROM_HERE,
+          base::BindOnce(std::move(finished_delete_callback), false));
+      return;
+    }
+
     cert_db->DeleteCertAndKeyAsync(
         std::move(nss_cert),
         base::BindPostTask(content::GetUIThreadTaskRunner({}),
@@ -817,4 +841,39 @@ CreateExtensionsClientCertSource(Profile* profile) {
   return std::make_unique<ExtensionsClientCertSource>(
       certificate_provider_service->CreateCertificateProvider());
 }
+
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+ClientCertManagementAccessControls::ClientCertManagementAccessControls(
+    Profile* profile)
+    : is_guest_(
+          user_manager::UserManager::Get()->IsLoggedInAsGuest() ||
+          user_manager::UserManager::Get()->IsLoggedInAsManagedGuestSession()),
+      is_kiosk_(user_manager::UserManager::Get()->IsLoggedInAsAnyKioskApp()),
+      client_cert_policy_(static_cast<ClientCertificateManagementPermission>(
+          profile->GetPrefs()->GetInteger(
+              prefs::kClientCertificateManagementAllowed))) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+}
+
+bool ClientCertManagementAccessControls::IsManagementAllowed(
+    KeyStorage key_storage) const {
+  return !(key_storage == kHardwareBacked && is_guest_) && !is_kiosk_ &&
+         client_cert_policy_ != ClientCertificateManagementPermission::kNone;
+}
+
+bool ClientCertManagementAccessControls::IsChangeAllowed(
+    KeyStorage key_storage,
+    CertLocation cert_location) const {
+  if (!IsManagementAllowed(key_storage)) {
+    return false;
+  }
+
+  if (cert_location == kUser) {
+    return client_cert_policy_ != ClientCertificateManagementPermission::kNone;
+  }
+
+  return client_cert_policy_ == ClientCertificateManagementPermission::kAll;
+}
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+
 #endif

chrome/browser/ui/webui/certificate_manager/client_cert_sources.h

@@ -9,6 +9,7 @@
 
 #include "build/build_config.h"
 #include "chrome/browser/ui/webui/certificate_manager/certificate_manager_handler.h"
+#include "chrome/browser/ui/webui/certificate_manager/certificate_manager_utils.h"
 
 class Profile;
 
@@ -28,4 +29,40 @@ std::unique_ptr<CertificateManagerPageHandler::CertSource>
 CreateExtensionsClientCertSource(Profile* profile);
 #endif
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+class ClientCertManagementAccessControls {
+ public:
+  enum KeyStorage {
+    kSoftwareBacked,
+    kHardwareBacked,
+  };
+  enum CertLocation {
+    kUser,
+    kDeviceWide,
+  };
+
+  // Creates an object that can be used to check whether management functions
+  // should be allowed. Once created the object is immutable and can be
+  // accessed on any thread. The object should not be cached, as the policies
+  // can change during runtime, so a new object should be created before every
+  // operation to confirm that the operation is allowed with the current
+  // policies.
+  explicit ClientCertManagementAccessControls(Profile* profile);
+
+  // Calculates whether management, such as importing client certs, is allowed
+  // for the given key storage location.
+  bool IsManagementAllowed(KeyStorage key_storage) const;
+
+  // Calculates whether changing (such as deleting) a specific client cert with
+  // the given key and cert storage locations is allowed.
+  bool IsChangeAllowed(KeyStorage key_storage,
+                       CertLocation cert_location) const;
+
+ private:
+  const bool is_guest_;
+  const bool is_kiosk_;
+  const ClientCertificateManagementPermission client_cert_policy_;
+};
+#endif
+
 #endif  // CHROME_BROWSER_UI_WEBUI_CERTIFICATE_MANAGER_CLIENT_CERT_SOURCES_H_

chrome/browser/ui/webui/certificate_manager/client_cert_sources_ash_unittest.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/ash/kcer/kcer_factory_ash.h"
 #include "chrome/browser/ash/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/webui/certificate_manager/certificate_manager_utils.h"
 #include "chrome/browser/ui/webui/certificate_manager/client_cert_sources.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
@@ -159,6 +160,21 @@ class ClientCertSourceAshUnitTest
     }
   }
 
+  bool GetCertificateInfosContainsCertWithHash(
+      std::string_view hash_hex) const {
+    base::test::TestFuture<
+        std::vector<certificate_manager_v2::mojom::SummaryCertInfoPtr>>
+        get_certs_waiter;
+    cert_source_->GetCertificateInfos(get_certs_waiter.GetCallback());
+    const auto& certs = get_certs_waiter.Get();
+    for (const auto& cert : certs) {
+      if (cert->sha256hash_hex == hash_hex) {
+        return true;
+      }
+    }
+    return false;
+  }
+
  protected:
   base::test::ScopedFeatureList feature_list_;
   AccountId account_{AccountId::FromUserEmail(kUsername)};
@@ -195,17 +211,7 @@ TEST_P(ClientCertSourceAshUnitTest,
   EXPECT_FALSE(SlotContainsCertWithHash(
       crypto::GetPublicSlotForChromeOSUser(username_hash()).get(),
       kTestClientCertHashHex));
-
-  {
-    base::test::TestFuture<
-        std::vector<certificate_manager_v2::mojom::SummaryCertInfoPtr>>
-        get_certs_waiter;
-    cert_source_->GetCertificateInfos(get_certs_waiter.GetCallback());
-    const auto& certs = get_certs_waiter.Get();
-    for (const auto& cert : certs) {
-      EXPECT_NE(cert->sha256hash_hex, kTestClientCertHashHex);
-    }
-  }
+  EXPECT_FALSE(GetCertificateInfosContainsCertWithHash(kTestClientCertHashHex));
 
   {
     // The correct password for the client.p12 file.
@@ -240,26 +246,38 @@ TEST_P(ClientCertSourceAshUnitTest,
   EXPECT_TRUE(SlotContainsCertWithHash(
       crypto::GetPublicSlotForChromeOSUser(username_hash()).get(),
       kTestClientCertHashHex));
+  EXPECT_TRUE(GetCertificateInfosContainsCertWithHash(kTestClientCertHashHex));
 
+  // Try deleting the imported certificate while it is not allowed by
+  // enterprise policy and verify that the deletion failed and the certificate
+  // is still present.
+  profile()->GetPrefs()->SetInteger(
+      prefs::kClientCertificateManagementAllowed,
+      static_cast<int>(ClientCertificateManagementPermission::kNone));
   {
-    base::test::TestFuture<
-        std::vector<certificate_manager_v2::mojom::SummaryCertInfoPtr>>
-        get_certs_waiter;
-    cert_source_->GetCertificateInfos(get_certs_waiter.GetCallback());
-    const auto& certs = get_certs_waiter.Get();
-    bool found = false;
-    for (const auto& cert : certs) {
-      if (cert->sha256hash_hex == kTestClientCertHashHex) {
-        found = true;
-        break;
-      }
-    }
-    EXPECT_TRUE(found);
+    fake_page_->set_mocked_confirmation_result(true);
+    base::test::TestFuture<certificate_manager_v2::mojom::ActionResultPtr>
+        delete_waiter;
+    cert_source_->DeleteCertificate(kTestClientCertHashHex,
+                                    delete_waiter.GetCallback());
+
+    certificate_manager_v2::mojom::ActionResultPtr delete_result =
+        delete_waiter.Take();
+    ASSERT_TRUE(delete_result);
+    ASSERT_TRUE(delete_result->is_error());
+    EXPECT_EQ(delete_result->get_error(), "delete failed");
   }
 
-  // Now try deleting the imported certificate and verify that it is no longer
-  // present.
+  EXPECT_TRUE(SlotContainsCertWithHash(
+      crypto::GetPublicSlotForChromeOSUser(username_hash()).get(),
+      kTestClientCertHashHex));
+  EXPECT_TRUE(GetCertificateInfosContainsCertWithHash(kTestClientCertHashHex));
 
+  // Now try changing the enterprise policy to allow it, delete the imported
+  // certificate, and verify that it is no longer present.
+  profile()->GetPrefs()->SetInteger(
+      prefs::kClientCertificateManagementAllowed,
+      static_cast<int>(ClientCertificateManagementPermission::kUserOnly));
   {
     fake_page_->set_mocked_confirmation_result(true);
     base::test::TestFuture<certificate_manager_v2::mojom::ActionResultPtr>
@@ -276,17 +294,21 @@ TEST_P(ClientCertSourceAshUnitTest,
   EXPECT_FALSE(SlotContainsCertWithHash(
       crypto::GetPublicSlotForChromeOSUser(username_hash()).get(),
       kTestClientCertHashHex));
+  EXPECT_FALSE(GetCertificateInfosContainsCertWithHash(kTestClientCertHashHex));
+}
 
-  {
-    base::test::TestFuture<
-        std::vector<certificate_manager_v2::mojom::SummaryCertInfoPtr>>
-        get_certs_waiter;
-    cert_source_->GetCertificateInfos(get_certs_waiter.GetCallback());
-    const auto& certs = get_certs_waiter.Get();
-    for (const auto& cert : certs) {
-      EXPECT_NE(cert->sha256hash_hex, kTestClientCertHashHex);
-    }
-  }
+TEST_P(ClientCertSourceAshUnitTest, ImportPkcs12NotAllowedByPolicy) {
+  profile()->GetPrefs()->SetInteger(
+      prefs::kClientCertificateManagementAllowed,
+      static_cast<int>(ClientCertificateManagementPermission::kNone));
+  base::test::TestFuture<certificate_manager_v2::mojom::ActionResultPtr>
+      import_waiter;
+  DoImport(import_waiter.GetCallback());
+  certificate_manager_v2::mojom::ActionResultPtr import_result =
+      import_waiter.Take();
+  ASSERT_TRUE(import_result);
+  ASSERT_TRUE(import_result->is_error());
+  EXPECT_EQ(import_result->get_error(), "not allowed");
 }
 
 TEST_P(ClientCertSourceAshUnitTest, ImportPkcs12PasswordWrong) {

chrome/browser/ui/webui/certificates_handler.h

@@ -14,6 +14,7 @@
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/certificate_manager_model.h"
+#include "chrome/browser/ui/webui/certificate_manager/certificate_manager_utils.h"
 #include "components/file_access/scoped_file_access.h"
 #include "content/public/browser/web_ui_message_handler.h"
 #include "net/cert/nss_cert_database.h"
@@ -27,30 +28,6 @@ class PrefRegistrySyncable;
 enum class Slot { kUser, kSystem };
 enum class CertificateSource { kBuiltIn, kImported };
 
-// Enumeration of certificate management permissions which corresponds to
-// values of policy ClientCertificateManagementAllowed.
-// Underlying type is int because values are casting to/from prefs values.
-enum class ClientCertificateManagementPermission : int {
-  // Allow users to manage all certificates
-  kAll = 0,
-  // Allow users to manage user certificates
-  kUserOnly = 1,
-  // Disallow users from managing certificates
-  kNone = 2
-};
-
-// Enumeration of certificate management permissions which corresponds to
-// values of policy CACertificateManagementAllowed.
-// Underlying type is int because values are casting to/from prefs values.
-enum class CACertificateManagementPermission : int {
-  // Allow users to manage all certificates
-  kAll = 0,
-  // Allow users to manage user certificates
-  kUserOnly = 1,
-  // Disallow users from managing certificates
-  kNone = 2
-};
-
 namespace certificate_manager {
 
 class FileAccessProvider;

ui/webui/resources/cr_components/certificate_manager/certificate_manager_v2.ts

@@ -158,7 +158,13 @@ export class CertificateManagerV2Element extends
 
       clientPlatformSubpageLists_: {
         type: Array<SubpageCertificateList>,
-        computed: 'computeClientPlatformSubpageLists_(showClientCertImport_)',
+        // <if expr="chromeos_ash">
+        computed: 'computeClientPlatformSubpageLists_(showClientCertImport_,' +
+            'showClientCertImportAndBind_)',
+        // </if>
+        // <if expr="not chromeos_ash">
+        computed: 'computeClientPlatformSubpageLists_()',
+        // </if>
       },
 
       toastMessage_: String,
@@ -176,7 +182,21 @@ export class CertificateManagerV2Element extends
         value: false,
       },
 
-      showClientCertImport_: Boolean,
+      // <if expr="chromeos_ash">
+      showClientCertImport_: {
+        type: Boolean,
+        value() {
+          return loadTimeData.getBoolean('clientCertImportAllowed');
+        },
+      },
+
+      showClientCertImportAndBind_: {
+        type: Boolean,
+        value() {
+          return loadTimeData.getBoolean('clientCertImportAndBindAllowed');
+        },
+      },
+      // </if>
 
       certificateSourceEnum_: {
         type: Object,
@@ -205,19 +225,9 @@ export class CertificateManagerV2Element extends
   private enterpriseSubpageLists_: SubpageCertificateList[];
   private platformSubpageLists_: SubpageCertificateList[];
   private clientPlatformSubpageLists_: SubpageCertificateList[];
-  // <if expr="not chromeos_ash">
-  private showClientCertImport_: boolean = false;
-  // </if>
   // <if expr="chromeos_ash">
-  // TODO(crbug.com/40928765): Import should also be disabled in kiosk mode or
-  // when disabled by policy (if there is any policy that applies to client
-  // certs). (And these conditions should be re-checked by the import handler
-  // in C++ code rather than trusting the webui.)
-  // TODO(crbug.com/40928765): This controls both "import" and "import and
-  // bind". If we implement client cert import on Linux too we should make a
-  // separate bool for each so that "import and bind" is only enabled on
-  // chromeos.
-  private showClientCertImport_: boolean = true;
+  private showClientCertImport_: boolean;
+  private showClientCertImportAndBind_: boolean;
   // </if>
 
   override ready() {
@@ -405,12 +415,18 @@ export class CertificateManagerV2Element extends
             'certificateManagerV2ClientCertsFromPlatform'),
         certSource: CertificateSource.kPlatformClientCert,
         hideExport: true,
+        // <if expr="chromeos_ash">
         showImport: this.showClientCertImport_,
-        showImportAndBind: this.showClientCertImport_,
+        showImportAndBind: this.showClientCertImportAndBind_,
         // TODO(crbug.com/40928765): Figure out how we want to display the
         // import buttons/etc on this subpage. For now just show the header
         // when we need the import buttons to be visible.
-        hideHeader: !this.showClientCertImport_,
+        hideHeader:
+            !this.showClientCertImport_ && !this.showClientCertImportAndBind_,
+        // </if>
+        // <if expr="not chromeos_ash">
+        hideHeader: true,
+        // </if>
       },
     ];
   }