Public API for marking keys corporate

KeyPermissionsManager gets an API to explicitly mark keys corporate.
For symmetry, IsKeyCorporate is also moved to the public API.

It will later replace the static function API that is being used by
callers now - this has been left as a follow-up for better mergeability.

Bug: 1127506
Change-Id: I1c1c7552edcd1671b93ba1bd2fcf380ad6b021d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2413249
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Omar Morsi <omorsi@google.com>
Cr-Commit-Position: refs/heads/master@{#808056}

chrome/browser/chromeos/BUILD.gn

@@ -3390,6 +3390,7 @@ source_set("unit_tests") {
     "night_light/night_light_client_unittest.cc",
     "note_taking_helper_unittest.cc",
     "ownership/owner_settings_service_chromeos_unittest.cc",
+    "platform_keys/key_permissions/key_permissions_manager_impl_unittest.cc",
     "plugin_vm/mock_plugin_vm_manager.cc",
     "plugin_vm/mock_plugin_vm_manager.h",
     "plugin_vm/plugin_vm_features_unittest.cc",

chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_manager.h

@@ -118,6 +118,18 @@ class KeyPermissionsManager {
   virtual bool CanUserGrantPermissionFor(
       const std::string& public_key_spki_der,
       const std::vector<platform_keys::TokenId>& key_locations) const = 0;
+
+  // Returns true if the key identified by |public_key_spki_der| that is
+  // located on |key_locations| is marked for corporate usage.
+  virtual bool IsCorporateKey(
+      const std::string& public_key_spki_der,
+      const std::vector<platform_keys::TokenId>& key_locations) const = 0;
+
+  // Marks the key identified by |public_key_spki_der| as corporate usage.
+  // Accepts a single key location because this is intended for usage after key
+  // generation / import, when exactly one location is relevant.
+  virtual void SetCorporateKey(const std::string& public_key_spki_der,
+                               platform_keys::TokenId key_location) const = 0;
 };
 
 }  // namespace platform_keys

chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_manager_impl.cc

@@ -219,10 +219,8 @@ bool KeyPermissionsManagerImpl::PermissionsForExtensionImpl::
 
   // Usage of corporate keys is solely determined by policy. The user must not
   // circumvent this decision.
-  if (key_permissions_->IsCorporateKey(public_key_spki_der_b64,
-                                       key_locations)) {
+  if (key_permissions_->IsCorporateKey(public_key_spki_der, key_locations))
     return PolicyAllowsCorporateKeyUsage();
-  }
 
   // Only permissions for keys that are not designated for corporate usage are
   // determined by user decisions.
@@ -279,14 +277,7 @@ void KeyPermissionsManagerImpl::PermissionsForExtensionImpl::
   if (!IsKeyOnUserSlot(key_locations))
     return;
 
-  DictionaryPrefUpdate update(profile_prefs_, prefs::kPlatformKeys);
-
-  std::unique_ptr<base::DictionaryValue> new_pref_entry(
-      new base::DictionaryValue);
-  new_pref_entry->SetKey(kPrefKeyUsage, base::Value(kPrefKeyUsageCorporate));
-
-  update->SetWithoutPathExpansion(public_key_spki_der_b64,
-                                  std::move(new_pref_entry));
+  key_permissions_->SetCorporateKey(public_key_spki_der, TokenId::kUser);
 }
 
 void KeyPermissionsManagerImpl::PermissionsForExtensionImpl::
@@ -438,12 +429,49 @@ bool KeyPermissionsManagerImpl::CanUserGrantPermissionFor(
   if (profile_is_managed_)
     return false;
 
+  // If this profile is not managed but we find a corporate key, don't allow
+  // the user to grant permissions.
+  return !IsCorporateKey(public_key_spki_der, key_locations);
+}
+
+bool KeyPermissionsManagerImpl::IsCorporateKey(
+    const std::string& public_key_spki_der,
+    const std::vector<TokenId>& key_locations) const {
   std::string public_key_spki_der_b64;
   base::Base64Encode(public_key_spki_der, &public_key_spki_der_b64);
 
-  // If this profile is not managed but we find a corporate key, don't allow
-  // the user to grant permissions.
-  return !IsCorporateKey(public_key_spki_der_b64, key_locations);
+  for (const auto key_location : key_locations) {
+    switch (key_location) {
+      case TokenId::kUser:
+        if (IsCorporateKeyForProfile(public_key_spki_der_b64, profile_prefs_))
+          return true;
+        break;
+      case TokenId::kSystem:
+        return true;
+    }
+  }
+  return false;
+}
+
+void KeyPermissionsManagerImpl::SetCorporateKey(
+    const std::string& public_key_spki_der,
+    TokenId key_location) const {
+  if (key_location == TokenId::kSystem) {
+    // Nothing to do - all system-token keys are currently implicitly corporate.
+    return;
+  }
+
+  std::string public_key_spki_der_b64;
+  base::Base64Encode(public_key_spki_der, &public_key_spki_der_b64);
+
+  DictionaryPrefUpdate update(profile_prefs_, prefs::kPlatformKeys);
+
+  std::unique_ptr<base::DictionaryValue> new_pref_entry(
+      new base::DictionaryValue);
+  new_pref_entry->SetKey(kPrefKeyUsage, base::Value(kPrefKeyUsageCorporate));
+
+  update->SetWithoutPathExpansion(public_key_spki_der_b64,
+                                  std::move(new_pref_entry));
 }
 
 // static
@@ -486,25 +514,6 @@ KeyPermissionsManagerImpl::GetCorporateKeyUsageAllowedAppIds(
   return permissions;
 }
 
-bool KeyPermissionsManagerImpl::IsCorporateKey(
-    const std::string& public_key_spki_der_b64,
-    const std::vector<TokenId>& key_locations) const {
-  for (const auto key_location : key_locations) {
-    switch (key_location) {
-      case TokenId::kUser:
-        LOG(ERROR) << " user";
-        if (IsCorporateKeyForProfile(public_key_spki_der_b64, profile_prefs_))
-          return true;
-        break;
-      case TokenId::kSystem:
-        return true;
-      default:
-        NOTREACHED();
-    }
-  }
-  return false;
-}
-
 void KeyPermissionsManagerImpl::CreatePermissionObjectAndPassToCallback(
     const std::string& extension_id,
     const PermissionsCallback& callback,

chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_manager_impl.h

@@ -125,7 +125,16 @@ class KeyPermissionsManagerImpl : public KeyPermissionsManager {
       const std::string& public_key_spki_der,
       const std::vector<platform_keys::TokenId>& key_locations) const override;
 
+  bool IsCorporateKey(
+      const std::string& public_key_spki_der,
+      const std::vector<platform_keys::TokenId>& key_locations) const override;
+
+  void SetCorporateKey(const std::string& public_key_spki_der,
+                       platform_keys::TokenId key_location) const override;
+
   // Returns true if |public_key_spki_der_b64| is a corporate usage key.
+  // TOOD(http://crbug.com/1127284): Remove this and migrate callers to
+  // IsCorporateKey().
   static bool IsCorporateKeyForProfile(
       const std::string& public_key_spki_der_b64,
       const PrefService* const profile_prefs);
@@ -136,10 +145,6 @@ class KeyPermissionsManagerImpl : public KeyPermissionsManager {
       policy::PolicyService* const profile_policies);
 
  private:
-  bool IsCorporateKey(
-      const std::string& public_key_spki_der_b64,
-      const std::vector<platform_keys::TokenId>& key_locations) const;
-
   // Creates a PermissionsForExtension object from |extension_id| and |value|
   // and passes the object to |callback|.
   void CreatePermissionObjectAndPassToCallback(

chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_manager_impl_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/platform_keys/key_permissions/key_permissions_manager_impl.h"
+
+#include <memory>
+
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "chrome/browser/extensions/test_extension_system.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/policy/core/common/mock_policy_service.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/test/browser_task_environment.h"
+#include "extensions/browser/state_store.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace platform_keys {
+
+class KeyPermissionsManagerImplTest : public ::testing::Test {
+ public:
+  KeyPermissionsManagerImplTest() = default;
+  KeyPermissionsManagerImplTest(const KeyPermissionsManagerImplTest&) = delete;
+  KeyPermissionsManagerImplTest& operator=(
+      const KeyPermissionsManagerImplTest&) = delete;
+  ~KeyPermissionsManagerImplTest() override = default;
+
+  void SetUp() override {
+    auto mock_policy_service = std::make_unique<policy::MockPolicyService>();
+    policy_service_ = mock_policy_service.get();
+    TestingProfile::Builder builder;
+    builder.SetPolicyService(std::move(mock_policy_service));
+    profile_ = builder.Build();
+
+    extensions::TestExtensionSystem* extension_system =
+        static_cast<extensions::TestExtensionSystem*>(
+            extensions::ExtensionSystem::Get(profile_.get()));
+    extension_system->CreateExtensionService(
+        base::CommandLine::ForCurrentProcess(),
+        /*install_directory=*/base::FilePath(),
+        /*autoupdate_enabled=*/false);
+    extensions_state_store_ = extension_system->state_store();
+
+    key_permissions_manager_ = std::make_unique<KeyPermissionsManagerImpl>(
+        /*profile_is_managed=*/true, profile_->GetPrefs(), policy_service_,
+        extensions_state_store_);
+  }
+
+ protected:
+  content::BrowserTaskEnvironment task_environment_;
+  std::unique_ptr<TestingProfile> profile_;
+  // Owned by |profile_|.
+  policy::MockPolicyService* policy_service_ = nullptr;
+  // Owned by |profile_|.
+  extensions::StateStore* extensions_state_store_ = nullptr;
+
+  std::unique_ptr<KeyPermissionsManagerImpl> key_permissions_manager_;
+};
+
+TEST_F(KeyPermissionsManagerImplTest, SystemTokenKeyIsImplicitlyCorporate) {
+  EXPECT_TRUE(key_permissions_manager_->IsCorporateKey("some_public_key",
+                                                       {TokenId::kSystem}));
+  EXPECT_TRUE(key_permissions_manager_->IsCorporateKey(
+      "some_public_key", {TokenId::kUser, TokenId::kSystem}));
+}
+
+TEST_F(KeyPermissionsManagerImplTest, CorporateRoundTrip) {
+  // By default, user-token keys are not corporate.
+  EXPECT_FALSE(key_permissions_manager_->IsCorporateKey("some_public_key",
+                                                        {TokenId::kUser}));
+
+  key_permissions_manager_->SetCorporateKey("some_public_key", TokenId::kUser);
+
+  EXPECT_TRUE(key_permissions_manager_->IsCorporateKey("some_public_key",
+                                                       {TokenId::kUser}));
+
+  // Check that a repeated call doesn't corrupt the stored state.
+  key_permissions_manager_->SetCorporateKey("some_public_key", TokenId::kUser);
+
+  EXPECT_TRUE(key_permissions_manager_->IsCorporateKey("some_public_key",
+                                                       {TokenId::kUser}));
+}
+
+}  // namespace platform_keys
+}  // namespace chromeos