diff --git a/chrome/browser/ui/webui/certificate_manager/certificate_manager_ui.cc b/chrome/browser/ui/webui/certificate_manager/certificate_manager_ui.cc index 40ea93196b832..8b304d7eee0aa 100644 --- a/chrome/browser/ui/webui/certificate_manager/certificate_manager_ui.cc +++ b/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( diff --git a/chrome/browser/ui/webui/certificate_manager/certificate_manager_utils.h b/chrome/browser/ui/webui/certificate_manager/certificate_manager_utils.h index 60165672154eb..f8728ab7b7296 100644 --- a/chrome/browser/ui/webui/certificate_manager/certificate_manager_utils.h +++ b/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_ diff --git a/chrome/browser/ui/webui/certificate_manager/client_cert_sources.cc b/chrome/browser/ui/webui/certificate_manager/client_cert_sources.cc index d30418a6148f0..3e3bf6d0b217c 100644 --- a/chrome/browser/ui/webui/certificate_manager/client_cert_sources.cc +++ b/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 diff --git a/chrome/browser/ui/webui/certificate_manager/client_cert_sources.h b/chrome/browser/ui/webui/certificate_manager/client_cert_sources.h index b68d51f4606e4..63c7c0c1cbed7 100644 --- a/chrome/browser/ui/webui/certificate_manager/client_cert_sources.h +++ b/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_ diff --git a/chrome/browser/ui/webui/certificate_manager/client_cert_sources_ash_unittest.cc b/chrome/browser/ui/webui/certificate_manager/client_cert_sources_ash_unittest.cc index 62abb2220e83f..bdca5e8c8dbda 100644 --- a/chrome/browser/ui/webui/certificate_manager/client_cert_sources_ash_unittest.cc +++ b/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) { diff --git a/chrome/browser/ui/webui/certificates_handler.h b/chrome/browser/ui/webui/certificates_handler.h index 304522c53ac29..85b5f01866005 100644 --- a/chrome/browser/ui/webui/certificates_handler.h +++ b/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; diff --git a/ui/webui/resources/cr_components/certificate_manager/certificate_manager_v2.ts b/ui/webui/resources/cr_components/certificate_manager/certificate_manager_v2.ts index b918effd5c8ed..7aaade7b48b0f 100644 --- a/ui/webui/resources/cr_components/certificate_manager/certificate_manager_v2.ts +++ b/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> }, ]; }