Cert Provisioning: Mark keys as corporate

Mark key pairs generated in certificate provisioning for Chrome OS as
corporate.

Bug: 1127506
Test: unit_tests, browser_tests
Change-Id: I5379f4b186283d076b2b6f02baf1323f174ea543
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2404832
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Michael Ershov <miersh@google.com>
Reviewed-by: Omar Morsi <omorsi@google.com>
Cr-Commit-Position: refs/heads/master@{#808107}

chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker.cc

@@ -16,6 +16,8 @@
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_invalidator.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_metrics.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_serializer.h"
+#include "chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_manager.h"
+#include "chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_manager_user_service.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service_factory.h"
@@ -112,6 +114,26 @@ int GetStateOrderedIndex(CertProvisioningWorkerState state) {
   return res;
 }
 
+// Marks the key |public_key_spki_der| as corporate. |profile| can be nullptr if
+// |scope| is CertScope::kDevice.
+void MarkKeyAsCorporate(CertScope scope,
+                        Profile* profile,
+                        const std::string& public_key_spki_der) {
+  // Device-wide keys are implicitly corporate, and there is currently no
+  // device-wide KeyPermissionsManager.
+  // TODO(https://crbug.com/1127284): Use the device-wide KeyPermissionsManager
+  // when it exists.
+  if (scope == CertScope::kDevice) {
+    return;
+  }
+  DCHECK(profile);
+
+  platform_keys::KeyPermissionsManagerUserServiceFactory::GetForBrowserContext(
+      profile)
+      ->key_permissions_manager()
+      ->SetCorporateKey(public_key_spki_der, GetPlatformKeysTokenId(scope));
+}
+
 }  // namespace
 
 // ============= CertProvisioningWorkerFactory =================================
@@ -513,6 +535,8 @@ void CertProvisioningWorkerImpl::OnRegisterKeyDone(
 void CertProvisioningWorkerImpl::MarkKey() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  MarkKeyAsCorporate(cert_scope_, profile_, public_key_);
+
   platform_keys_service_->SetAttributeForKey(
       GetPlatformKeysTokenId(cert_scope_), public_key_,
       platform_keys::KeyAttributeType::kCertificateProvisioningId,

chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker_unittest.cc

@@ -4,6 +4,9 @@
 
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker.h"
 
+#include <memory>
+#include <string>
+
 #include "base/base64.h"
 #include "base/callback.h"
 #include "base/json/json_string_value_serializer.h"
@@ -19,6 +22,9 @@
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_metrics.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_test_helpers.h"
 #include "chrome/browser/chromeos/cert_provisioning/mock_cert_provisioning_invalidator.h"
+#include "chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_manager.h"
+#include "chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_manager_user_service.h"
+#include "chrome/browser/chromeos/platform_keys/key_permissions/mock_key_permissions_manager.h"
 #include "chrome/browser/chromeos/platform_keys/mock_platform_keys_service.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
@@ -308,6 +314,31 @@ const std::string& GetPublicKey() {
         .WillOnce(RunOnceCallback<2>(platform_keys::Status::kSuccess)); \
   }
 
+// A fake KeyPermissionsManagerUserService which returns a KeyPermissionsManager
+// pointer passed to its constructor.
+class FakeKeyPermissionsManagerUserService
+    : public platform_keys::KeyPermissionsManagerUserService {
+ public:
+  FakeKeyPermissionsManagerUserService(
+      platform_keys::KeyPermissionsManager* key_permissions_manager)
+      : key_permissions_manager_(key_permissions_manager) {}
+  ~FakeKeyPermissionsManagerUserService() override = default;
+
+  platform_keys::KeyPermissionsManager* key_permissions_manager() override {
+    return key_permissions_manager_;
+  }
+
+  static std::unique_ptr<KeyedService> Build(
+      platform_keys::KeyPermissionsManager* key_permissions_manager,
+      content::BrowserContext* browser_context) {
+    return std::make_unique<FakeKeyPermissionsManagerUserService>(
+        key_permissions_manager);
+  }
+
+ private:
+  platform_keys::KeyPermissionsManager* key_permissions_manager_;
+};
+
 // A mock for observing the result callback of the worker.
 class CallbackObserver {
  public:
@@ -357,6 +388,12 @@ class CertProvisioningWorkerTest : public ::testing::Test {
     platform_keys::PlatformKeysServiceFactory::GetInstance()
         ->SetDeviceWideServiceForTesting(platform_keys_service_);
 
+    platform_keys::KeyPermissionsManagerUserServiceFactory::GetInstance()
+        ->SetTestingFactory(
+            GetProfile(),
+            base::BindRepeating(&FakeKeyPermissionsManagerUserService::Build,
+                                &key_permissions_manager_));
+
     // Only explicitly expected removals are allowed.
     EXPECT_CALL(*platform_keys_service_, RemoveCertificate).Times(0);
     EXPECT_CALL(*platform_keys_service_, RemoveKey).Times(0);
@@ -419,6 +456,7 @@ class CertProvisioningWorkerTest : public ::testing::Test {
 
   policy::MockCloudPolicyClient cloud_policy_client_;
   platform_keys::MockPlatformKeysService* platform_keys_service_ = nullptr;
+  StrictMock<platform_keys::MockKeyPermissionsManager> key_permissions_manager_;
 };
 
 // Checks that the worker makes all necessary requests to other modules during
@@ -463,6 +501,9 @@ TEST_F(CertProvisioningWorkerTest, Success) {
     EXPECT_REGISTER_KEY_OK(*mock_tpm_challenge_key, StartRegisterKeyStep);
     EXPECT_CALL(state_change_callback_observer_, StateChangeCallback());
 
+    EXPECT_CALL(key_permissions_manager_,
+                SetCorporateKey(GetPublicKey(), platform_keys::TokenId::kUser));
+
     EXPECT_SET_ATTRIBUTE_FOR_KEY_OK(SetAttributeForKey(
         platform_keys::TokenId::kUser, GetPublicKey(),
         platform_keys::KeyAttributeType::kCertificateProvisioningId,
@@ -536,6 +577,9 @@ TEST_F(CertProvisioningWorkerTest, NoVaSuccess) {
         kCertScopeStrUser, kCertProfileId, kCertProfileVersion, GetPublicKey(),
         /*callback=*/_));
 
+    EXPECT_CALL(key_permissions_manager_,
+                SetCorporateKey(GetPublicKey(), platform_keys::TokenId::kUser));
+
     EXPECT_SET_ATTRIBUTE_FOR_KEY_OK(SetAttributeForKey(
         platform_keys::TokenId::kUser, GetPublicKey(),
         platform_keys::KeyAttributeType::kCertificateProvisioningId,
@@ -615,6 +659,10 @@ TEST_F(CertProvisioningWorkerTest, TryLaterManualRetry) {
 
     EXPECT_REGISTER_KEY_OK(*mock_tpm_challenge_key, StartRegisterKeyStep);
 
+    // Note: No call to KeyPermissionsManager::SetCorporateKey, because it is
+    // only performed for platform_keys::TokenId::kUser, but this test works
+    // with the system token.
+
     EXPECT_SET_ATTRIBUTE_FOR_KEY_OK(SetAttributeForKey(
         platform_keys::TokenId::kSystem, GetPublicKey(),
         platform_keys::KeyAttributeType::kCertificateProvisioningId,
@@ -725,6 +773,9 @@ TEST_F(CertProvisioningWorkerTest, TryLaterWait) {
 
     EXPECT_REGISTER_KEY_OK(*mock_tpm_challenge_key, StartRegisterKeyStep);
 
+    EXPECT_CALL(key_permissions_manager_,
+                SetCorporateKey(GetPublicKey(), platform_keys::TokenId::kUser));
+
     EXPECT_SET_ATTRIBUTE_FOR_KEY_OK(SetAttributeForKey(
         platform_keys::TokenId::kUser, GetPublicKey(),
         platform_keys::KeyAttributeType::kCertificateProvisioningId,
@@ -1024,6 +1075,9 @@ TEST_F(CertProvisioningWorkerTest, RemoveRegisteredKey) {
 
     EXPECT_REGISTER_KEY_OK(*mock_tpm_challenge_key, StartRegisterKeyStep);
 
+    EXPECT_CALL(key_permissions_manager_,
+                SetCorporateKey(GetPublicKey(), platform_keys::TokenId::kUser));
+
     EXPECT_SET_ATTRIBUTE_FOR_KEY_FAIL(SetAttributeForKey(
         platform_keys::TokenId::kUser, GetPublicKey(),
         platform_keys::KeyAttributeType::kCertificateProvisioningId,
@@ -1179,6 +1233,9 @@ TEST_F(CertProvisioningWorkerTest, SerializationSuccess) {
 
     EXPECT_REGISTER_KEY_OK(*mock_tpm_challenge_key, StartRegisterKeyStep);
 
+    EXPECT_CALL(key_permissions_manager_,
+                SetCorporateKey(GetPublicKey(), platform_keys::TokenId::kUser));
+
     EXPECT_SET_ATTRIBUTE_FOR_KEY_OK(SetAttributeForKey(
         platform_keys::TokenId::kUser, GetPublicKey(),
         platform_keys::KeyAttributeType::kCertificateProvisioningId,