platformKeys: Add per-extension sign permissions.

PlatformKeysService now supports persisting whether an extension is allowed to sign data with a key an unlimited number of times.

Currently, these permissions are only granted in the accompanying browser test and not in production, because UI is still missing.

BUG=450167

Review URL: https://codereview.chromium.org/905523002

Cr-Commit-Position: refs/heads/master@{#317053}

chrome/browser/chromeos/platform_keys/platform_keys.h

@@ -113,16 +113,19 @@ void SelectClientCertificates(const ClientCertificateRequest& request,
 
 }  // namespace subtle
 
+// Returns the DER encoding of the X.509 Subject Public Key Info of the public
+// key in |certificate|.
+std::string GetSubjectPublicKeyInfo(
+    const scoped_refptr<net::X509Certificate>& certificate);
+
 // Obtains information about the public key in |certificate|.
 // If |certificate| contains an RSA key, sets |key_size_bits| to the modulus
-// length, |public_key_spki_der| to the DER encoding of the X.509 Subject Public
-// Key Info, and |key_type| to type RSA and returns true.
+// length, and |key_type| to type RSA and returns true.
 // If |certificate| contains any other key type, or if the public exponent of
 // the RSA key in |certificate| is not F4, returns false and does not update any
 // of the output parameters.
 // All pointer arguments must not be null.
 bool GetPublicKey(const scoped_refptr<net::X509Certificate>& certificate,
-                  std::string* public_key_spki_der,
                   net::X509Certificate::PublicKeyType* key_type,
                   size_t* key_size_bits);
 

chrome/browser/chromeos/platform_keys/platform_keys_nss.cc

@@ -777,12 +777,15 @@ void SelectClientCertificates(const ClientCertificateRequest& request,
 
 }  // namespace subtle
 
+std::string GetSubjectPublicKeyInfo(
+    const scoped_refptr<net::X509Certificate>& certificate) {
+  const SECItem& spki_der = certificate->os_cert_handle()->derPublicKey;
+  return std::string(spki_der.data, spki_der.data + spki_der.len);
+}
+
 bool GetPublicKey(const scoped_refptr<net::X509Certificate>& certificate,
-                  std::string* public_key_spki_der,
                   net::X509Certificate::PublicKeyType* key_type,
                   size_t* key_size_bits) {
-  const SECItem& spki_der = certificate->os_cert_handle()->derPublicKey;
-
   net::X509Certificate::PublicKeyType key_type_tmp =
       net::X509Certificate::kPublicKeyTypeUnknown;
   size_t key_size_bits_tmp = 0;
@@ -810,10 +813,8 @@ bool GetPublicKey(const scoped_refptr<net::X509Certificate>& certificate,
     return false;
   }
 
-  public_key_spki_der->assign(spki_der.data, spki_der.data + spki_der.len);
   *key_type = key_type_tmp;
   *key_size_bits = key_size_bits_tmp;
-
   return true;
 }
 

chrome/browser/chromeos/platform_keys/platform_keys_service.cc

@@ -17,18 +17,125 @@ using content::BrowserThread;
 
 namespace chromeos {
 
+struct PlatformKeysService::KeyEntry {
+  // The base64-encoded DER of a X.509 Subject Public Key Info.
+  std::string spki_b64;
+
+  // True if the key can be used once for singing.
+  // This permission is granted if an extension generated a key using the
+  // enterprise.platformKeys API, so that it can build a certification request..
+  // After the first signing operation this permission will be revoked.
+  bool sign_once = false;
+
+  // True if the key can be used for signing an unlimited number of times.
+  // This permission is granted by the user or by policy to allow the extension
+  // to use the key for signing through the enterprise.platformKeys or
+  // platformKeys API.
+  // This permission is granted until revoked by the user or the policy.
+  bool sign_unlimited = false;
+};
+
 namespace {
 
 const char kErrorKeyNotAllowedForSigning[] =
     "This key is not allowed for signing. Either it was used for signing "
     "before or it was not correctly generated.";
+
+// The key at which platform key specific data is stored in each extension's
+// state store.
+// From older versions of ChromeOS, this key can hold a list of base64 and
+// DER-encoded SPKIs. A key can be used for signing at most once if it is part
+// of that list
+// and removed from that list afterwards.
+//
+// The current format of data that is written to the PlatformKeys field is a
+// list of serialized KeyEntry objects:
+//   { 'SPKI': string,
+//     'signOnce': bool,  // if not present, defaults to false
+//     'signUnlimited': bool  // if not present, defaults to false
+//   }
+//
+// Do not change this constant as clients will lose their existing state.
 const char kStateStorePlatformKeys[] = "PlatformKeys";
+const char kStateStoreSPKI[] = "SPKI";
+const char kStateStoreSignOnce[] = "signOnce";
+const char kStateStoreSignUnlimited[] = "signUnlimited";
+
+scoped_ptr<PlatformKeysService::KeyEntries> KeyEntriesFromState(
+    const base::Value& state) {
+  scoped_ptr<PlatformKeysService::KeyEntries> new_entries(
+      new PlatformKeysService::KeyEntries);
+
+  const base::ListValue* entries = nullptr;
+  if (!state.GetAsList(&entries)) {
+    LOG(ERROR) << "Found a state store of wrong type.";
+    return new_entries.Pass();
+  }
+  for (const base::Value* entry : *entries) {
+    if (!entry) {
+      LOG(ERROR) << "Found invalid NULL entry in PlatformKeys state store.";
+      continue;
+    }
 
-scoped_ptr<base::StringValue> GetPublicKeyValue(
-    const std::string& public_key_spki_der) {
-  std::string public_key_spki_der_b64;
-  base::Base64Encode(public_key_spki_der, &public_key_spki_der_b64);
-  return make_scoped_ptr(new base::StringValue(public_key_spki_der_b64));
+    PlatformKeysService::KeyEntry new_entry;
+    const base::DictionaryValue* dict_entry = nullptr;
+    if (entry->GetAsString(&new_entry.spki_b64)) {
+      // This handles the case that the store contained a plain list of base64
+      // and DER-encoded SPKIs from an older version of ChromeOS.
+      new_entry.sign_once = true;
+    } else if (entry->GetAsDictionary(&dict_entry)) {
+      dict_entry->GetStringWithoutPathExpansion(kStateStoreSPKI,
+                                                &new_entry.spki_b64);
+      dict_entry->GetBooleanWithoutPathExpansion(kStateStoreSignOnce,
+                                                 &new_entry.sign_once);
+      dict_entry->GetBooleanWithoutPathExpansion(kStateStoreSignUnlimited,
+                                                 &new_entry.sign_unlimited);
+    } else {
+      LOG(ERROR) << "Found invalid entry of type " << entry->GetType()
+                 << " in PlatformKeys state store.";
+      continue;
+    }
+    new_entries->push_back(new_entry);
+  }
+  return new_entries.Pass();
+}
+
+scoped_ptr<base::ListValue> KeyEntriesToState(
+    const PlatformKeysService::KeyEntries& entries) {
+  scoped_ptr<base::ListValue> new_state(new base::ListValue);
+  for (const PlatformKeysService::KeyEntry& entry : entries) {
+    // Drop entries that the extension doesn't have any permissions for anymore.
+    if (!entry.sign_once && !entry.sign_unlimited)
+      continue;
+
+    scoped_ptr<base::DictionaryValue> new_entry(new base::DictionaryValue);
+    new_entry->SetStringWithoutPathExpansion(kStateStoreSPKI, entry.spki_b64);
+    // Omit writing default values, namely |false|.
+    if (entry.sign_once) {
+      new_entry->SetBooleanWithoutPathExpansion(kStateStoreSignOnce,
+                                                entry.sign_once);
+    }
+    if (entry.sign_unlimited) {
+      new_entry->SetBooleanWithoutPathExpansion(kStateStoreSignUnlimited,
+                                                entry.sign_unlimited);
+    }
+    new_state->Append(new_entry.release());
+  }
+  return new_state.Pass();
+}
+
+// Searches |platform_keys| for an entry for |public_key_spki_der_b64|. If found
+// returns a pointer to it, otherwise returns null.
+PlatformKeysService::KeyEntry* GetMatchingEntry(
+    const std::string& public_key_spki_der_b64,
+    PlatformKeysService::KeyEntries* platform_keys) {
+  for (PlatformKeysService::KeyEntry& entry : *platform_keys) {
+    // For every ASN.1 value there is exactly one DER encoding, so it is fine to
+    // compare the DER (or its base64 encoding).
+    if (entry.spki_b64 == public_key_spki_der_b64)
+      return &entry;
+  }
+  return nullptr;
 }
 
 }  // namespace
@@ -55,14 +162,16 @@ class PlatformKeysService::PermissionUpdateTask : public Task {
   // Creates a task that reads the current permission for an extension to access
   // a certain key. Afterwards it updates and persists the permission to the new
   // value |new_permission_value|. |callback| will be run after the permission
-  // was persisted. The old permission value is then accessible through
-  // old_permission_value().
-  PermissionUpdateTask(const bool new_permission_value,
+  // was persisted. The old permission values are then available through
+  // old_key_entry().
+  PermissionUpdateTask(const SignPermission permission,
+                       const bool new_permission_value,
                        const std::string& public_key_spki_der,
                        const std::string& extension_id,
                        base::Callback<void(Task*)> callback,
                        PlatformKeysService* service)
-      : new_permission_value_(new_permission_value),
+      : permission_(permission),
+        new_permission_value_(new_permission_value),
         public_key_spki_der_(public_key_spki_der),
         extension_id_(extension_id),
         callback_(callback),
@@ -78,9 +187,9 @@ class PlatformKeysService::PermissionUpdateTask : public Task {
 
   bool IsDone() override { return next_step_ == Step::DONE; }
 
-  // The original permission value before setting the new value
-  // |new_permission_value|.
-  bool old_permission_value() { return old_permission_value_; }
+  // The old key entry with the old permissions before setting |permission| to
+  // the new value |new_permission_value|.
+  const KeyEntry& old_key_entry() { return old_key_entry_; }
 
  private:
   void DoStep() {
@@ -113,38 +222,59 @@ class PlatformKeysService::PermissionUpdateTask : public Task {
                                   weak_factory_.GetWeakPtr()));
   }
 
-  void GotPlatformKeys(scoped_ptr<base::ListValue> platform_keys) {
+  void GotPlatformKeys(scoped_ptr<KeyEntries> platform_keys) {
     platform_keys_ = platform_keys.Pass();
     DoStep();
   }
 
-  // Returns whether the extension has permission to use the key for signing
-  // according to the PlatformKeys value read from the extensions state store.
-  // Invalidates the key if it was found to be valid.
+  // Persists the existing KeyEntry in |old_key_entry_|, updates the entry with
+  // the new permission and persists it to the extension's state store if it was
+  // changed.
   void WriteUpdate() {
-    scoped_ptr<base::StringValue> key_value(
-        GetPublicKeyValue(public_key_spki_der_));
+    DCHECK(platform_keys_);
 
-    base::ListValue::const_iterator it = platform_keys_->Find(*key_value);
-    old_permission_value_ = it != platform_keys_->end();
-    if (old_permission_value_ == new_permission_value_)
-      return;
-
-    if (new_permission_value_)
-      platform_keys_->Append(key_value.release());
-    else
-      platform_keys_->Remove(*key_value, nullptr);
+    std::string public_key_spki_der_b64;
+    base::Base64Encode(public_key_spki_der_, &public_key_spki_der_b64);
+
+    KeyEntry* matching_entry =
+        GetMatchingEntry(public_key_spki_der_b64, platform_keys_.get());
+
+    if (!matching_entry) {
+      platform_keys_->push_back(KeyEntry());
+      matching_entry = &platform_keys_->back();
+      matching_entry->spki_b64 = public_key_spki_der_b64;
+    } else if (permission_ == SignPermission::ONCE && new_permission_value_) {
+      // The one-time sign permission is supposed to be granted once per key
+      // during generation. Generated keys should be unique and thus this case
+      // should never occur.
+      NOTREACHED() << "Requested one-time sign permission on existing key.";
+    }
+    old_key_entry_ = *matching_entry;
+
+    bool* permission_value = nullptr;
+    switch (permission_) {
+      case SignPermission::ONCE:
+        permission_value = &matching_entry->sign_once;
+        break;
+      case SignPermission::UNLIMITED:
+        permission_value = &matching_entry->sign_unlimited;
+        break;
+    }
 
-    service_->SetPlatformKeysOfExtension(extension_id_, platform_keys_.Pass());
+    if (*permission_value != new_permission_value_) {
+      *permission_value = new_permission_value_;
+      service_->SetPlatformKeysOfExtension(extension_id_, *platform_keys_);
+    }
   }
 
   Step next_step_ = Step::READ_PLATFORM_KEYS;
-  scoped_ptr<base::ListValue> platform_keys_;
-  bool old_permission_value_ = false;
+  KeyEntry old_key_entry_;
 
+  const SignPermission permission_;
   const bool new_permission_value_;
   const std::string public_key_spki_der_;
   const std::string extension_id_;
+  scoped_ptr<KeyEntries> platform_keys_;
   base::Callback<void(Task*)> callback_;
   PlatformKeysService* const service_;
   base::WeakPtrFactory<PermissionUpdateTask> weak_factory_;
@@ -198,17 +328,21 @@ class PlatformKeysService::SignTask : public Task {
         next_step_ = Step::SIGN_OR_ABORT;
         UpdatePermission();
         return;
-      case Step::SIGN_OR_ABORT:
+      case Step::SIGN_OR_ABORT: {
         next_step_ = Step::DONE;
-        if (!service_->permission_check_enabled_ ||
-            permission_update_->old_permission_value()) {
+        bool sign_granted = permission_update_->old_key_entry().sign_once ||
+                            permission_update_->old_key_entry().sign_unlimited;
+        if (sign_granted) {
           Sign();
         } else {
+          if (!callback_.is_null()) {
             callback_.Run(std::string() /* no signature */,
                           kErrorKeyNotAllowedForSigning);
+          }
           DoStep();
         }
         return;
+      }
       case Step::DONE:
         service_->TaskFinished(this);
         // |this| might be invalid now.
@@ -221,7 +355,8 @@ class PlatformKeysService::SignTask : public Task {
   // signing operations with that key.
   void UpdatePermission() {
     permission_update_.reset(new PermissionUpdateTask(
-        false /* new permission value */, public_key_, extension_id_,
+        SignPermission::ONCE, false /* new permission value */, public_key_,
+        extension_id_,
         base::Bind(&SignTask::DidUpdatePermission, weak_factory_.GetWeakPtr()),
         service_));
     permission_update_->Start();
@@ -251,7 +386,7 @@ class PlatformKeysService::SignTask : public Task {
   }
 
   Step next_step_ = Step::UPDATE_PERMISSION;
-  scoped_ptr<base::ListValue> platform_keys_;
+  scoped_ptr<KeyEntries> platform_keys_;
   scoped_ptr<PermissionUpdateTask> permission_update_;
 
   const std::string token_id_;
@@ -271,6 +406,213 @@ class PlatformKeysService::SignTask : public Task {
   DISALLOW_COPY_AND_ASSIGN(SignTask);
 };
 
+class PlatformKeysService::SelectTask : public Task {
+ public:
+  enum class Step {
+    GET_MATCHING_CERTS,
+    SELECT_CERTS,
+    READ_PLATFORM_KEYS,
+    UPDATE_PERMISSION,
+    FILTER_BY_PERMISSIONS,
+    DONE,
+  };
+
+  // This task determines all known client certs matching |request|. If
+  // |interactive| is true, calls |service->select_delegate_->Select()| to
+  // select a cert from all matches. The extension with |extension_id| will be
+  // granted unlimited sign permission for the selected cert.
+  // Finally, either the selection or, if |interactive| is false, matching certs
+  // that the extension has permission for are passed to |callback|.
+  SelectTask(const platform_keys::ClientCertificateRequest& request,
+             bool interactive,
+             const std::string& extension_id,
+             const SelectCertificatesCallback& callback,
+             PlatformKeysService* service)
+      : request_(request),
+        interactive_(interactive),
+        extension_id_(extension_id),
+        callback_(callback),
+        service_(service),
+        weak_factory_(this) {}
+  ~SelectTask() override {}
+
+  void Start() override {
+    CHECK(next_step_ == Step::GET_MATCHING_CERTS);
+    DoStep();
+  }
+  bool IsDone() override { return next_step_ == Step::DONE; }
+
+ private:
+  void DoStep() {
+    switch (next_step_) {
+      case Step::GET_MATCHING_CERTS:
+        if (interactive_)
+          next_step_ = Step::SELECT_CERTS;
+        else  // Skip SelectCerts and UpdatePermission if not interactive.
+          next_step_ = Step::READ_PLATFORM_KEYS;
+        GetMatchingCerts();
+        return;
+      case Step::SELECT_CERTS:
+        next_step_ = Step::UPDATE_PERMISSION;
+        SelectCerts();
+        return;
+      case Step::UPDATE_PERMISSION:
+        next_step_ = Step::READ_PLATFORM_KEYS;
+        UpdatePermission();
+        return;
+      case Step::READ_PLATFORM_KEYS:
+        next_step_ = Step::FILTER_BY_PERMISSIONS;
+        ReadPlatformKeys();
+        return;
+      case Step::FILTER_BY_PERMISSIONS:
+        next_step_ = Step::DONE;
+        FilterSelectionByPermission();
+        return;
+      case Step::DONE:
+        service_->TaskFinished(this);
+        // |this| might be invalid now.
+        return;
+    }
+  }
+
+  // Retrieves all certificates matching |request_|. Will call back to
+  // |GotMatchingCerts()|.
+  void GetMatchingCerts() {
+    platform_keys::subtle::SelectClientCertificates(
+        request_,
+        base::Bind(&SelectTask::GotMatchingCerts, weak_factory_.GetWeakPtr()),
+        service_->browser_context_);
+  }
+
+  // If the certificate request could be processed successfully, |matches| will
+  // contain the list of matching certificates (maybe empty) and |error_message|
+  // will be empty. If an error occurred, |matches| will be null and
+  // |error_message| contain an error message.
+  void GotMatchingCerts(scoped_ptr<net::CertificateList> matches,
+                        const std::string& error_message) {
+    if (!error_message.empty()) {
+      next_step_ = Step::DONE;
+      callback_.Run(nullptr /* no certificates */, error_message);
+      DoStep();
+      return;
+    }
+    matches_.swap(*matches);
+    DoStep();
+  }
+
+  // Calls |service_->select_delegate_->Select()| to select a cert from
+  // |matches_|, which will be stored in |selected_cert_|.
+  // Will call back to |GotSelection()|.
+  void SelectCerts() {
+    CHECK(interactive_);
+    if (matches_.empty()) {
+      // Don't show a select dialog if no certificate is matching.
+      DoStep();
+      return;
+    }
+    service_->select_delegate_->Select(
+        extension_id_, matches_,
+        base::Bind(&SelectTask::GotSelection, base::Unretained(this)));
+  }
+
+  // Will be called by |SelectCerts()| with the selected cert or null if no cert
+  // was selected.
+  void GotSelection(scoped_refptr<net::X509Certificate> selected_cert) {
+    selected_cert_ = selected_cert;
+    DoStep();
+  }
+
+  // Updates the extension's state store about unlimited sign permission for the
+  // selected cert. Does nothing if no cert was selected.
+  // Will call back to |DidUpdatePermission()|.
+  void UpdatePermission() {
+    CHECK(interactive_);
+    if (!selected_cert_) {
+      DoStep();
+      return;
+    }
+    const std::string public_key_spki_der(
+        platform_keys::GetSubjectPublicKeyInfo(selected_cert_));
+    permission_update_.reset(new PermissionUpdateTask(
+        SignPermission::UNLIMITED, true /* new permission value */,
+        public_key_spki_der, extension_id_,
+        base::Bind(&SelectTask::DidUpdatePermission, base::Unretained(this)),
+        service_));
+    permission_update_->Start();
+  }
+
+  void DidUpdatePermission(Task* /* task */) { DoStep(); }
+
+  // Reads the PlatformKeys value from the extension's state store and calls
+  // back to GotPlatformKeys().
+  void ReadPlatformKeys() {
+    service_->GetPlatformKeysOfExtension(
+        extension_id_,
+        base::Bind(&SelectTask::GotPlatformKeys, weak_factory_.GetWeakPtr()));
+  }
+
+  void GotPlatformKeys(scoped_ptr<KeyEntries> platform_keys) {
+    platform_keys_ = platform_keys.Pass();
+    DoStep();
+  }
+
+  // Filters from all matches (if not interactive) or from the selection (if
+  // interactive), the certificates that the extension has unlimited sign
+  // permission for. Passes the filtered certs to |callback_|.
+  void FilterSelectionByPermission() {
+    scoped_ptr<net::CertificateList> selection(new net::CertificateList);
+    if (interactive_) {
+      if (selected_cert_)
+        selection->push_back(selected_cert_);
+    } else {
+      selection->assign(matches_.begin(), matches_.end());
+    }
+
+    scoped_ptr<net::CertificateList> filtered_certs(new net::CertificateList);
+    for (scoped_refptr<net::X509Certificate> selected_cert : *selection) {
+      const std::string public_key_spki_der(
+          platform_keys::GetSubjectPublicKeyInfo(selected_cert));
+      std::string public_key_spki_der_b64;
+      base::Base64Encode(public_key_spki_der, &public_key_spki_der_b64);
+
+      KeyEntry* matching_entry =
+          GetMatchingEntry(public_key_spki_der_b64, platform_keys_.get());
+      if (!matching_entry || !matching_entry->sign_unlimited)
+        continue;
+
+      filtered_certs->push_back(selected_cert);
+    }
+    // Note: In the interactive case this should have filtered exactly the
+    // one selected cert. Checking the permissions again is not striclty
+    // necessary but this ensures that the permissions were updated correctly.
+    CHECK(!selected_cert_ || (filtered_certs->size() == 1 &&
+                              filtered_certs->front() == selected_cert_));
+    callback_.Run(filtered_certs.Pass(), std::string() /* no error */);
+    DoStep();
+  }
+
+  Step next_step_ = Step::GET_MATCHING_CERTS;
+  scoped_ptr<KeyEntries> platform_keys_;
+  scoped_ptr<PermissionUpdateTask> permission_update_;
+
+  net::CertificateList matches_;
+  scoped_refptr<net::X509Certificate> selected_cert_;
+  platform_keys::ClientCertificateRequest request_;
+  const bool interactive_;
+  const std::string extension_id_;
+  const SelectCertificatesCallback callback_;
+  PlatformKeysService* const service_;
+  base::WeakPtrFactory<SelectTask> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SelectTask);
+};
+
+PlatformKeysService::SelectDelegate::SelectDelegate() {
+}
+
+PlatformKeysService::SelectDelegate::~SelectDelegate() {
+}
+
 PlatformKeysService::PlatformKeysService(
     content::BrowserContext* browser_context,
     extensions::StateStore* state_store)
@@ -283,8 +625,22 @@ PlatformKeysService::PlatformKeysService(
 PlatformKeysService::~PlatformKeysService() {
 }
 
-void PlatformKeysService::DisablePermissionCheckForTesting() {
-  permission_check_enabled_ = false;
+void PlatformKeysService::SetSelectDelegate(
+    scoped_ptr<SelectDelegate> delegate) {
+  select_delegate_ = delegate.Pass();
+}
+
+void PlatformKeysService::GrantUnlimitedSignPermission(
+    const std::string& extension_id,
+    scoped_refptr<net::X509Certificate> cert) {
+  const std::string public_key_spki_der(
+      platform_keys::GetSubjectPublicKeyInfo(cert));
+
+  StartOrQueueTask(make_scoped_ptr(new PermissionUpdateTask(
+      SignPermission::UNLIMITED, true /* new permission value */,
+      public_key_spki_der, extension_id,
+      base::Bind(&PlatformKeysService::TaskFinished, base::Unretained(this)),
+      this)));
 }
 
 void PlatformKeysService::GenerateRSAKey(const std::string& token_id,
@@ -326,15 +682,12 @@ void PlatformKeysService::SignRSAPKCS1Raw(const std::string& token_id,
 
 void PlatformKeysService::SelectClientCertificates(
     const platform_keys::ClientCertificateRequest& request,
+    bool interactive,
     const std::string& extension_id,
     const SelectCertificatesCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  platform_keys::subtle::SelectClientCertificates(
-      request,
-      base::Bind(&PlatformKeysService::SelectClientCertificatesCallback,
-                 weak_factory_.GetWeakPtr(), extension_id, callback),
-      browser_context_);
+  StartOrQueueTask(make_scoped_ptr(
+      new SelectTask(request, interactive, extension_id, callback, this)));
 }
 
 void PlatformKeysService::StartOrQueueTask(scoped_ptr<Task> task) {
@@ -367,9 +720,9 @@ void PlatformKeysService::GetPlatformKeysOfExtension(
 
 void PlatformKeysService::SetPlatformKeysOfExtension(
     const std::string& extension_id,
-    scoped_ptr<base::ListValue> platform_keys) {
+    const KeyEntries& platform_keys) {
   state_store_->SetExtensionValue(extension_id, kStateStorePlatformKeys,
-                                  platform_keys.Pass());
+                                  KeyEntriesToState(platform_keys));
 }
 
 void PlatformKeysService::GeneratedKey(const std::string& extension_id,
@@ -382,7 +735,8 @@ void PlatformKeysService::GeneratedKey(const std::string& extension_id,
   }
 
   StartOrQueueTask(make_scoped_ptr(new PermissionUpdateTask(
-      true /* new permission value */, public_key_spki_der, extension_id,
+      SignPermission::ONCE, true /* new permission value */,
+      public_key_spki_der, extension_id,
       base::Bind(&PlatformKeysService::RegisteredGeneratedKey,
                  base::Unretained(this), callback, public_key_spki_der),
       this)));
@@ -396,35 +750,16 @@ void PlatformKeysService::RegisteredGeneratedKey(
   TaskFinished(task);
 }
 
-void PlatformKeysService::SelectClientCertificatesCallback(
-    const std::string& extension_id,
-    const SelectCertificatesCallback& callback,
-    scoped_ptr<net::CertificateList> matches,
-    const std::string& error_message) {
-  if (permission_check_enabled_)
-    matches->clear();
-
-  // TODO(pneubeck): Remove all certs that the extension doesn't have access to.
-  callback.Run(matches.Pass(), error_message);
-}
 
 void PlatformKeysService::GotPlatformKeysOfExtension(
     const std::string& extension_id,
     const GetPlatformKeysCallback& callback,
     scoped_ptr<base::Value> value) {
-  if (!value)
-    value.reset(new base::ListValue);
-
-  base::ListValue* keys = NULL;
-  if (!value->GetAsList(&keys)) {
-    LOG(ERROR) << "Found a value of wrong type.";
-
-    keys = new base::ListValue;
-    value.reset(keys);
-  }
+  scoped_ptr<KeyEntries> key_entries(new KeyEntries);
+  if (value)
+    key_entries = KeyEntriesFromState(*value);
 
-  ignore_result(value.release());
-  callback.Run(make_scoped_ptr(keys));
+  callback.Run(key_entries.Pass());
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/platform_keys/platform_keys_service.h

@@ -39,6 +39,36 @@ namespace chromeos {
 
 class PlatformKeysService : public KeyedService {
  public:
+  struct KeyEntry;
+  using KeyEntries = std::vector<KeyEntry>;
+
+  // The SelectDelegate is used to select a single certificate from all
+  // certificates matching a request (see SelectClientCertificates). E.g. this
+  // can happen by exposing UI to let the user select.
+  class SelectDelegate {
+   public:
+    // TODO(pneubeck): Handle if the selection was aborted, e.g. by the user.
+    using CertificateSelectedCallback =
+        base::Callback<void(scoped_refptr<net::X509Certificate> selection)>;
+
+    SelectDelegate();
+    virtual ~SelectDelegate();
+
+    // Called on an interactive SelectClientCertificates call with the list of
+    // matching certificates, |certs|.
+    // The certificate passed to |callback| will be forwarded to the
+    // calling extension and the extension will get unlimited sign permission
+    // for this cert. By passing null to |callback|, no cert will be selected.
+    // Must eventually call |callback| or be destructed. |callback| must not be
+    // called after this delegate is destructed.
+    virtual void Select(const std::string& extension_id,
+                        const net::CertificateList& certs,
+                        const CertificateSelectedCallback& callback) = 0;
+
+   private:
+    DISALLOW_ASSIGN(SelectDelegate);
+  };
+
   // Stores registration information in |state_store|, i.e. for each extension
   // the list of public keys that are valid to be used for signing. Each key can
   // be used for signing at most once.
@@ -50,18 +80,22 @@ class PlatformKeysService : public KeyedService {
                                extensions::StateStore* state_store);
   ~PlatformKeysService() override;
 
-  // Disables the checks whether an extension is allowed to read client
-  // certificates or allowed to use the signing function of a key.
-  // TODO(pneubeck): Remove this once a permissions are implemented.
-  void DisablePermissionCheckForTesting();
+  // Sets the delegate which will be used for interactive
+  // SelectClientCertificates calls.
+  void SetSelectDelegate(scoped_ptr<SelectDelegate> delegate);
+
+  // Grants unlimited sign permission for |cert| to the extension with the ID
+  // |extension_id|.
+  void GrantUnlimitedSignPermission(const std::string& extension_id,
+                                    scoped_refptr<net::X509Certificate> cert);
 
   // If the generation was successful, |public_key_spki_der| will contain the
   // DER encoding of the SubjectPublicKeyInfo of the generated key and
   // |error_message| will be empty. If it failed, |public_key_spki_der| will be
   // empty and |error_message| contain an error message.
-  typedef base::Callback<void(const std::string& public_key_spki_der,
-                              const std::string& error_message)>
-      GenerateKeyCallback;
+  using GenerateKeyCallback =
+      base::Callback<void(const std::string& public_key_spki_der,
+                          const std::string& error_message)>;
 
   // Generates an RSA key pair with |modulus_length_bits| and registers the key
   // to allow a single sign operation by the given extension. |token_id| is
@@ -77,8 +111,8 @@ class PlatformKeysService : public KeyedService {
   // If signing was successful, |signature| will be contain the signature and
   // |error_message| will be empty. If it failed, |signature| will be empty and
   // |error_message| contain an error message.
-  typedef base::Callback<void(const std::string& signature,
-                              const std::string& error_message)> SignCallback;
+  using SignCallback = base::Callback<void(const std::string& signature,
+                                           const std::string& error_message)>;
 
   // Digests |data|, applies PKCS1 padding and afterwards signs the data with
   // the private key matching |params.public_key|. If a non empty token id is
@@ -118,27 +152,35 @@ class PlatformKeysService : public KeyedService {
   // contain the list of matching certificates (maybe empty) and |error_message|
   // will be empty. If an error occurred, |matches| will be null and
   // |error_message| contain an error message.
-  typedef base::Callback<void(scoped_ptr<net::CertificateList> matches,
-                              const std::string& error_message)>
-      SelectCertificatesCallback;
-
-  // Returns the list of all certificates that match |request|. |callback| will
-  // be invoked with these matches or an error message.
+  using SelectCertificatesCallback =
+      base::Callback<void(scoped_ptr<net::CertificateList> matches,
+                          const std::string& error_message)>;
+
+  // Returns a list of certificates matching |request|.
+  // 1) all certificates that match the request (like being rooted in one of the
+  // give CAs) are determined. 2) if |interactive| is true, the currently set
+  // SelectDelegate is used to select a single certificate from these matches
+  // which will the extension will also be granted access to. 3) only
+  // certificates, that the extension has unlimited sign permission for, will be
+  // returned.
+  // |callback| will be invoked with these certificates or an error message.
   // Will only call back during the lifetime of this object.
-  // TODO(pneubeck): Add the interactive option and integrate the select
-  // certificate dialog.
   void SelectClientCertificates(
       const platform_keys::ClientCertificateRequest& request,
+      bool interactive,
       const std::string& extension_id,
       const SelectCertificatesCallback& callback);
 
  private:
   using GetPlatformKeysCallback =
-      base::Callback<void(scoped_ptr<base::ListValue> platform_keys)>;
+      base::Callback<void(scoped_ptr<KeyEntries> platform_keys)>;
+
+  enum SignPermission { ONCE, UNLIMITED };
 
-  class Task;
-  class SignTask;
   class PermissionUpdateTask;
+  class SelectTask;
+  class SignTask;
+  class Task;
 
   // Starts |task| eventually. To ensure that at most one |Task| is running at a
   // time, it queues |task| for later execution if necessary.
@@ -159,7 +201,7 @@ class PlatformKeysService : public KeyedService {
   // Writes |platform_keys| to the state store of the extension with id
   // |extension_id|.
   void SetPlatformKeysOfExtension(const std::string& extension_id,
-                                  scoped_ptr<base::ListValue> platform_keys);
+                                  const KeyEntries& platform_keys);
 
   // Callback used by |GenerateRSAKey|.
   // If the key generation was successful, registers the generated public key
@@ -179,17 +221,6 @@ class PlatformKeysService : public KeyedService {
                               const std::string& public_key_spki_der,
                               Task* task);
 
-  // Calback used by |SelectClientCertificates|.
-  // If the certificate request could be processed successfully, |matches| will
-  // contain the list of matching certificates (maybe empty) and |error_message|
-  // will be empty. If an error occurred, |matches| will be null and
-  // |error_message| contain an error message.
-  void SelectClientCertificatesCallback(
-      const std::string& extension_id,
-      const SelectCertificatesCallback& callback,
-      scoped_ptr<net::CertificateList> matches,
-      const std::string& error_message);
-
   // Callback used by |GetPlatformKeysOfExtension|.
   // Is called with |value| set to the PlatformKeys value read from the
   // StateStore, which it forwards to |callback|. On error, calls |callback|
@@ -200,7 +231,7 @@ class PlatformKeysService : public KeyedService {
 
   content::BrowserContext* browser_context_;
   extensions::StateStore* state_store_;
-  bool permission_check_enabled_ = true;
+  scoped_ptr<SelectDelegate> select_delegate_;
   std::queue<linked_ptr<Task>> tasks_;
   base::WeakPtrFactory<PlatformKeysService> weak_factory_;
 

chrome/browser/extensions/api/platform_keys/platform_keys_api.cc

@@ -114,8 +114,9 @@ PlatformKeysInternalGetPublicKeyFunction::Run() {
     return RespondNow(Error(kErrorInvalidX509Cert));
 
   PublicKeyInfo key_info;
-  if (!chromeos::platform_keys::GetPublicKey(
-          cert_x509, &key_info.public_key_spki_der, &key_info.key_type,
+  key_info.public_key_spki_der =
+      chromeos::platform_keys::GetSubjectPublicKeyInfo(cert_x509);
+  if (!chromeos::platform_keys::GetPublicKey(cert_x509, &key_info.key_type,
                                              &key_info.key_size_bits) ||
       key_info.key_type != net::X509Certificate::kPublicKeyTypeRSA) {
     return RespondNow(Error(kErrorAlgorithmNotSupported));
@@ -154,7 +155,7 @@ PlatformKeysInternalSelectClientCertificatesFunction::Run() {
   }
 
   service->SelectClientCertificates(
-      request, extension_id(),
+      request, params->details.interactive, extension_id(),
       base::Bind(&PlatformKeysInternalSelectClientCertificatesFunction::
                      OnSelectedCertificates,
                  this));
@@ -173,8 +174,9 @@ void PlatformKeysInternalSelectClientCertificatesFunction::
   std::vector<linked_ptr<api_pk::Match>> result_matches;
   for (const scoped_refptr<net::X509Certificate>& match : *matches) {
     PublicKeyInfo key_info;
-    if (!chromeos::platform_keys::GetPublicKey(
-            match, &key_info.public_key_spki_der, &key_info.key_type,
+    key_info.public_key_spki_der =
+        chromeos::platform_keys::GetSubjectPublicKeyInfo(match);
+    if (!chromeos::platform_keys::GetPublicKey(match, &key_info.key_type,
                                                &key_info.key_size_bits)) {
       LOG(ERROR) << "Could not retrieve public key info.";
       continue;

chrome/browser/extensions/api/platform_keys/platform_keys_apitest_nss.cc

@@ -100,8 +100,8 @@ class PlatformKeysTest : public ExtensionApiTest,
       loop.Run();
     }
 
-    chromeos::PlatformKeysServiceFactory::GetForBrowserContext(
-        browser()->profile())->DisablePermissionCheckForTesting();
+    base::FilePath extension_path = test_data_dir_.AppendASCII("platform_keys");
+    extension_ = LoadExtension(extension_path);
   }
 
   void TearDownOnMainThread() override {
@@ -115,22 +115,47 @@ class PlatformKeysTest : public ExtensionApiTest,
     loop.Run();
   }
 
+  chromeos::PlatformKeysService* GetPlatformKeysService() {
+    return chromeos::PlatformKeysServiceFactory::GetForBrowserContext(
+        browser()->profile());
+  }
+
+  bool RunExtensionTest(const std::string& test_suite_name) {
+    // By default, the system token is not available.
+    std::string system_token_availability;
+
+    // Only if the current user is of the same domain as the device is enrolled
+    // to, the system token is available to the extension.
+    if (GetParam().device_status_ == DEVICE_STATUS_ENROLLED &&
+        GetParam().user_affiliation_ == USER_AFFILIATION_ENROLLED_DOMAIN) {
+      system_token_availability = "systemTokenEnabled";
+    }
+
+    GURL url = extension_->GetResourceURL(base::StringPrintf(
+        "basic.html?%s#%s", system_token_availability.c_str(),
+        test_suite_name.c_str()));
+    return RunExtensionSubtest("platform_keys", url.spec());
+  }
+
+ protected:
+  scoped_refptr<net::X509Certificate> client_cert1_;
+  scoped_refptr<net::X509Certificate> client_cert2_;
+  const extensions::Extension* extension_;
+
  private:
   void SetupTestCerts(const base::Closure& done_callback,
                       net::NSSCertDatabase* cert_db) {
-    scoped_refptr<net::X509Certificate> client_cert1 =
-        net::ImportClientCertAndKeyFromFile(net::GetTestCertsDirectory(),
-                                            "client_1.pem", "client_1.pk8",
+    client_cert1_ = net::ImportClientCertAndKeyFromFile(
+        net::GetTestCertsDirectory(), "client_1.pem", "client_1.pk8",
         cert_db->GetPrivateSlot().get());
-    ASSERT_TRUE(client_cert1.get());
+    ASSERT_TRUE(client_cert1_.get());
 
     // Import a second client cert signed by another CA than client_1 into the
     // system wide key slot.
-    scoped_refptr<net::X509Certificate> client_cert2 =
-        net::ImportClientCertAndKeyFromFile(net::GetTestCertsDirectory(),
-                                            "client_2.pem", "client_2.pk8",
+    client_cert2_ = net::ImportClientCertAndKeyFromFile(
+        net::GetTestCertsDirectory(), "client_2.pem", "client_2.pk8",
         test_system_slot_->slot());
-    ASSERT_TRUE(client_cert2.get());
+    ASSERT_TRUE(client_cert2_.get());
 
     done_callback.Run();
   }
@@ -157,22 +182,58 @@ class PlatformKeysTest : public ExtensionApiTest,
   scoped_ptr<crypto::ScopedTestSystemNSSKeySlot> test_system_slot_;
 };
 
+class TestSelectDelegate
+    : public chromeos::PlatformKeysService::SelectDelegate {
+ public:
+  explicit TestSelectDelegate(
+      scoped_refptr<net::X509Certificate> cert_to_select)
+      : cert_to_select_(cert_to_select) {}
+  ~TestSelectDelegate() override {}
+
+  void Select(const std::string& extension_id,
+              const net::CertificateList& certs,
+              const CertificateSelectedCallback& callback) override {
+    if (!cert_to_select_) {
+      callback.Run(nullptr /* no cert */);
+      return;
+    }
+    scoped_refptr<net::X509Certificate> selection;
+    for (scoped_refptr<net::X509Certificate> cert : certs) {
+      if (cert->Equals(cert_to_select_.get())) {
+        selection = cert;
+        break;
+      }
+    }
+    callback.Run(selection);
+  }
+
+ private:
+  scoped_refptr<net::X509Certificate> cert_to_select_;
+};
+
 }  // namespace
 
+// Basic tests that start with already granted permissions for both client_cert1
+// and client_cert2.
+// On interactive calls, the simulated user does not select any cert.
 IN_PROC_BROWSER_TEST_P(PlatformKeysTest, Basic) {
-  // By default, the system token is not available.
-  std::string system_token_availability;
+  GetPlatformKeysService()->SetSelectDelegate(
+      make_scoped_ptr(new TestSelectDelegate(nullptr /* select no cert */)));
+  GetPlatformKeysService()->GrantUnlimitedSignPermission(extension_->id(),
+                                                         client_cert1_);
+  GetPlatformKeysService()->GrantUnlimitedSignPermission(extension_->id(),
+                                                         client_cert2_);
+
+  ASSERT_TRUE(RunExtensionTest("basicTests")) << message_;
+}
 
-  // Only if the current user is of the same domain as the device is enrolled
-  // to, the system token is available to the extension.
-  if (GetParam().device_status_ == DEVICE_STATUS_ENROLLED &&
-      GetParam().user_affiliation_ == USER_AFFILIATION_ENROLLED_DOMAIN) {
-    system_token_availability = "systemTokenEnabled";
-  }
+// This permission test starts without any granted permissions.
+// On interactive calls, the simulated user selects client_1, if matching.
+IN_PROC_BROWSER_TEST_P(PlatformKeysTest, Permissions) {
+  GetPlatformKeysService()->SetSelectDelegate(
+      make_scoped_ptr(new TestSelectDelegate(client_cert1_)));
 
-  ASSERT_TRUE(RunExtensionSubtest("platform_keys",
-                                  "basic.html?" + system_token_availability))
-      << message_;
+  ASSERT_TRUE(RunExtensionTest("permissionTests")) << message_;
 }
 
 INSTANTIATE_TEST_CASE_P(

chrome/test/data/extensions/api_test/platform_keys/basic.js

@@ -4,11 +4,13 @@
 
 'use strict';
 
-var systemTokenEnabled = (location.href.indexOf("systemTokenEnabled") != -1);
+var systemTokenEnabled = (location.search.indexOf("systemTokenEnabled") != -1);
+var selectedTestSuite = location.hash.slice(1);
+console.log('[SELECTED TEST SUITE] ' + selectedTestSuite +
+            ', systemTokenEnable ' + systemTokenEnabled);
 
 var assertEq = chrome.test.assertEq;
 var assertTrue = chrome.test.assertTrue;
-var assertThrows = chrome.test.assertThrows;
 var fail = chrome.test.fail;
 var succeed = chrome.test.succeed;
 var callbackPass = chrome.test.callbackPass;
@@ -112,10 +114,9 @@ function sortCerts(certs) {
   return certs.sort(compareArrays);
 }
 
-function assertCertsSelected(request, expectedCerts, callback) {
+function assertCertsSelected(details, expectedCerts, callback) {
   chrome.platformKeys.selectClientCertificates(
-      {interactive: false, request: request},
-      callbackPass(function(actualMatches) {
+      details, callbackPass(function(actualMatches) {
         assertEq(expectedCerts.length, actualMatches.length,
                  'Number of stored certs not as expected');
         if (expectedCerts.length == actualMatches.length) {
@@ -190,32 +191,54 @@ function testHasSubtleCryptoMethods(token) {
   succeed();
 }
 
-function testSelectAllCerts() {
-  var requestAll = {
+var requestAll = {
   certificateTypes: [],
   certificateAuthorities: []
+};
+
+// Depends on |data|, thus it cannot be created immediately.
+function requestCA1() {
+  return {
+    certificateTypes: [],
+    certificateAuthorities: [data.client_1_issuer_dn.buffer]
   };
+}
+
+function testSelectAllCerts() {
   var expectedCerts = [data.client_1];
   if (systemTokenEnabled)
     expectedCerts.push(data.client_2);
-  assertCertsSelected(requestAll, expectedCerts);
+  assertCertsSelected({interactive: false, request: requestAll}, expectedCerts);
 }
 
 function testSelectCA1Certs() {
-  var requestCA1 = {
-    certificateTypes: [],
-    certificateAuthorities: [data.client_1_issuer_dn.buffer]
-  };
-  assertCertsSelected(requestCA1, [data.client_1]);
+  assertCertsSelected({interactive: false, request: requestCA1()},
+                      [data.client_1]);
+}
+
+function testSelectAllReturnsNoCerts() {
+  assertCertsSelected({interactive: false, request: requestAll},
+                      [] /* no certs selected */);
+}
+
+function testSelectAllReturnsClient1() {
+  assertCertsSelected({interactive: false, request: requestAll},
+                      [data.client_1]);
+}
+
+function testInteractiveSelectNoCerts() {
+  assertCertsSelected({interactive: true, request: requestAll},
+                      [] /* no certs selected */);
+}
+
+function testInteractiveSelectClient1() {
+  assertCertsSelected({interactive: true, request: requestAll},
+                      [data.client_1]);
 }
 
 function testMatchResult() {
-  var requestCA1 = {
-    certificateTypes: [],
-    certificateAuthorities: [data.client_1_issuer_dn.buffer]
-  };
   chrome.platformKeys.selectClientCertificates(
-      {interactive: false, request: requestCA1},
+      {interactive: false, request: requestCA1()},
       callbackPass(function(matches) {
         var expectedAlgorithm = {
           modulusLength: 2048,
@@ -282,7 +305,7 @@ function testSignNoHash() {
       }));
 }
 
-function testSignSha1() {
+function testSignSha1Client1() {
   var keyParams = {
     // Algorithm names are case-insensitive.
     hash: {name: 'Sha-1'}
@@ -305,18 +328,86 @@ function testSignSha1() {
       }));
 }
 
-function runTests() {
+// TODO(pneubeck): Test this by verifying that no private key is returned, once
+// that's implemented.
+function testSignFails(cert) {
+  var keyParams = {
+    hash: {name: 'SHA-1'}
+  };
+  var signParams = {
+    name: 'RSASSA-PKCS1-v1_5'
+  };
+  chrome.platformKeys.getKeyPair(
+      cert.buffer, keyParams, callbackPass(function(publicKey, privateKey) {
+        chrome.platformKeys.subtleCrypto()
+            .sign(signParams, privateKey, data.raw_data)
+            .then(function(signature) { fail('sign was expected to fail.'); },
+                  callbackPass(function(error) {
+                    assertTrue(error instanceof Error);
+                    assertEq(
+                        'The operation failed for an operation-specific reason',
+                        error.message);
+                  }));
+      }));
+}
+
+function testSignClient1Fails() {
+  testSignFails(data.client_1);
+}
+
+function testSignClient2Fails() {
+  testSignFails(data.client_2);
+}
+
+var testSuites = {
+  // These tests assume already granted permissions for client_1 and client_2.
+  // On interactive selectClientCertificates calls, the simulated user does not
+  // select any cert.
+  basicTests: function() {
     var tests = [
       testStaticMethods,
       testSelectAllCerts,
       testSelectCA1Certs,
+      testInteractiveSelectNoCerts,
       testMatchResult,
       testGetKeyPair,
       testSignNoHash,
-    testSignSha1
+      testSignSha1Client1,
     ];
 
     chrome.test.runTests(tests);
-}
+  },
+
+  // This test suite starts without any granted permissions.
+  // On interactive selectClientCertificates calls, the simulated user selects
+  // client_1, if matching.
+  permissionTests: function() {
+    var tests = [
+      // Without permissions both sign attempts fail.
+      testSignClient1Fails,
+      testSignClient2Fails,
+
+      // Without permissions, non-interactive select calls return no certs.
+      testSelectAllReturnsNoCerts,
+
+      testInteractiveSelectClient1,
+      // Now that the permission for client_1 is granted.
+
+      // Verify that signing with client_1 is possible and with client_2 still
+      // fails.
+      testSignSha1Client1,
+      testSignClient2Fails,
+
+      // Verify that client_1 can still be selected interactively.
+      testInteractiveSelectClient1,
+
+      // Verify that client_1 but not client_2 is selected in non-interactive
+      // calls.
+      testSelectAllReturnsClient1,
+    ];
+
+    chrome.test.runTests(tests);
+  }
+};
 
-setUp(runTests);
+setUp(testSuites[selectedTestSuite]);