Support retrieving all key pairs using platform keys service

This CL extends platform keys service by introducing one new function
(GetSubjectPublicKeyInfoList), which returns a list of
SubjectPublicKeyInfo strings for the public keys available in a certain
slot.

Bug: 1045895
Change-Id: I68d0f59d96df506784acd137b0df68ce41ed3c6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2153045Reviewed-by: Michael Ershov <miersh@google.com>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Commit-Queue: Omar Morsi <omorsi@google.com>
Cr-Commit-Position: refs/heads/master@{#760965}

chrome/browser/chromeos/platform_keys/mock_platform_keys_service.h

@@ -73,6 +73,11 @@ class MockPlatformKeysService : public PlatformKeysService {
                const GetCertificatesCallback& callback),
               (override));
 
+  MOCK_METHOD(void,
+              GetAllKeys,
+              (const std::string& token_id, GetAllKeysCallback callback),
+              (override));
+
   MOCK_METHOD(void,
               ImportCertificate,
               (const std::string& token_id,

chrome/browser/chromeos/platform_keys/platform_keys_service.h

@@ -106,6 +106,16 @@ using GetCertificatesCallback =
     base::Callback<void(std::unique_ptr<net::CertificateList> certs,
                         const std::string& error_message)>;
 
+// If the list of key pairs could be successfully retrieved, |error_message|
+// will be empty and |public_key_spki_der_list| will contain the list of
+// available key pairs (may be empty if no key pairs exist). Each available key
+// pair is represented as a DER-encoded SubjectPublicKeyInfo. If an error
+// occurred, |public_key_spki_der_list| will be empty and |error_message|
+// will be set to an error message.
+using GetAllKeysCallback =
+    base::OnceCallback<void(std::vector<std::string> public_key_spki_der_list,
+                            const std::string& error_message)>;
+
 // If an error occurred during import, |error_message| will be set to an error
 // message.
 using ImportCertificateCallback =
@@ -212,6 +222,13 @@ class PlatformKeysService : public KeyedService {
   virtual void GetCertificates(const std::string& token_id,
                                const GetCertificatesCallback& callback) = 0;
 
+  // Returns the list of all keys available from the given |token_id| as a list
+  // of der-encoded SubjectPublicKeyInfo strings. |callback| will be invoked on
+  // the UI thread with the list of available public keys, possibly with an
+  // error message. Must be called on the UI thread.
+  virtual void GetAllKeys(const std::string& token_id,
+                          GetAllKeysCallback callback) = 0;
+
   // Imports |certificate| to the given token if the certified key is already
   // stored in this token. Any intermediate of |certificate| will be ignored.
   // |token_id| specifies the token to store the certificate on and can
@@ -277,6 +294,8 @@ class PlatformKeysServiceImpl final : public PlatformKeysService {
       const SelectCertificatesCallback& callback) override;
   void GetCertificates(const std::string& token_id,
                        const GetCertificatesCallback& callback) override;
+  void GetAllKeys(const std::string& token_id,
+                  GetAllKeysCallback callback) override;
   void ImportCertificate(const std::string& token_id,
                          const scoped_refptr<net::X509Certificate>& certificate,
                          const ImportCertificateCallback& callback) override;

chrome/browser/chromeos/platform_keys/platform_keys_service_browsertest.cc

@@ -164,6 +164,17 @@ class SignExecutionWaiter : public ExecutionWaiter<const std::string&> {
     return std::get<0>(result_callback_args());
   }
 };
+
+class GetAllKeysExecutionWaiter
+    : public ExecutionWaiter<std::vector<std::string>> {
+ public:
+  GetAllKeysExecutionWaiter() = default;
+  ~GetAllKeysExecutionWaiter() = default;
+
+  const std::vector<std::string> public_keys() const {
+    return std::get<0>(result_callback_args());
+  }
+};
 }  // namespace
 
 class PlatformKeysServiceBrowserTest
@@ -214,6 +225,19 @@ class PlatformKeysServiceBrowserTest
     return platform_keys_service_;
   }
 
+  // Generates a key pair in the given |token_id| using platform keys service
+  // and returns the SubjectPublicKeyInfo string encoded in DER format.
+  std::string GenerateKeyPair(const std::string& token_id) {
+    const unsigned int kKeySize = 2048;
+
+    GenerateKeyExecutionWaiter generate_key_waiter;
+    platform_keys_service()->GenerateRSAKey(token_id, kKeySize,
+                                            generate_key_waiter.GetCallback());
+    generate_key_waiter.Wait();
+
+    return generate_key_waiter.public_key_spki_der();
+  }
+
  private:
   const AccountId test_user_account_id_ = AccountId::FromUserEmailGaiaId(
       kTestUserEmail,
@@ -284,8 +308,45 @@ IN_PROC_BROWSER_TEST_P(PlatformKeysServiceBrowserTest, GenerateRsaAndSign) {
   }
 }
 
-// TODO(https://crbug.com/1067591): Enable for sign-in screen by adding
-// ProfileToUse::kSigninProfile.
+// Generates a key pair in tokens accessible from the profile under test and
+// retrieves them.
+IN_PROC_BROWSER_TEST_P(PlatformKeysServiceBrowserTest, GetAllKeys) {
+  // Generate key pair in every token.
+  std::map<std::string, std::string> token_key_map;
+  for (const std::string& token_id : GetParam().token_ids) {
+    const std::string public_key_spki_der = GenerateKeyPair(token_id);
+    ASSERT_FALSE(public_key_spki_der.empty());
+    token_key_map[token_id] = public_key_spki_der;
+  }
+
+  // Only keys in the requested token should be retrieved.
+  for (const std::string& token_id : GetParam().token_ids) {
+    GetAllKeysExecutionWaiter get_all_keys_waiter;
+    platform_keys_service()->GetAllKeys(token_id,
+                                        get_all_keys_waiter.GetCallback());
+    get_all_keys_waiter.Wait();
+
+    EXPECT_TRUE(get_all_keys_waiter.error_message().empty());
+    std::vector<std::string> public_keys = get_all_keys_waiter.public_keys();
+    EXPECT_EQ(public_keys.size(), 1U);
+    EXPECT_EQ(public_keys[0], token_key_map[token_id]);
+  }
+}
+
+IN_PROC_BROWSER_TEST_P(PlatformKeysServiceBrowserTest,
+                       GetAllKeysWhenNoKeysGenerated) {
+  for (const std::string& token_id : GetParam().token_ids) {
+    GetAllKeysExecutionWaiter get_all_keys_waiter;
+    platform_keys_service()->GetAllKeys(token_id,
+                                        get_all_keys_waiter.GetCallback());
+    get_all_keys_waiter.Wait();
+
+    EXPECT_TRUE(get_all_keys_waiter.error_message().empty());
+    std::vector<std::string> public_keys = get_all_keys_waiter.public_keys();
+    EXPECT_EQ(public_keys.size(), 0U);
+  }
+}
+
 INSTANTIATE_TEST_SUITE_P(
     AllSupportedProfileTypes,
     PlatformKeysServiceBrowserTest,

chrome/browser/chromeos/platform_keys/platform_keys_service_nss.cc

@@ -7,10 +7,12 @@
 #include <cert.h>
 #include <cryptohi.h>
 #include <keyhi.h>
+#include <pk11pub.h>
 #include <secder.h>
 #include <stddef.h>
 #include <stdint.h>
 
+#include <memory>
 #include <utility>
 
 #include "base/bind.h"
@@ -377,6 +379,34 @@ class GetCertificatesState : public NSSOperationState {
   GetCertificatesCallback callback_;
 };
 
+class GetAllKeysState : public NSSOperationState {
+ public:
+  explicit GetAllKeysState(ServiceWeakPtr weak_ptr,
+                           GetAllKeysCallback callback);
+  ~GetAllKeysState() override = default;
+
+  void OnError(const base::Location& from,
+               const std::string& error_message) override {
+    CallBack(from, std::vector<std::string>() /* no public keys */,
+             error_message);
+  }
+
+  void CallBack(const base::Location& from,
+                std::vector<std::string> public_key_spki_der_list,
+                const std::string& error_message) {
+    auto bound_callback =
+        base::BindOnce(std::move(callback_),
+                       std::move(public_key_spki_der_list), error_message);
+    origin_task_runner_->PostTask(
+        from, base::BindOnce(&NSSOperationState::RunCallback,
+                             std::move(bound_callback), service_weak_ptr_));
+  }
+
+ private:
+  // Must be called on origin thread, therefore use CallBack().
+  GetAllKeysCallback callback_;
+};
+
 class ImportCertificateState : public NSSOperationState {
  public:
   ImportCertificateState(ServiceWeakPtr weak_ptr,
@@ -532,6 +562,10 @@ GetCertificatesState::GetCertificatesState(
     const GetCertificatesCallback& callback)
     : NSSOperationState(weak_ptr), callback_(callback) {}
 
+GetAllKeysState::GetAllKeysState(ServiceWeakPtr weak_ptr,
+                                 GetAllKeysCallback callback)
+    : NSSOperationState(weak_ptr), callback_(std::move(callback)) {}
+
 ImportCertificateState::ImportCertificateState(
     ServiceWeakPtr weak_ptr,
     const scoped_refptr<net::X509Certificate>& certificate,
@@ -891,6 +925,57 @@ void GetCertificatesWithDB(std::unique_ptr<GetCertificatesState> state,
       base::BindOnce(&DidGetCertificates, std::move(state)), slot);
 }
 
+// Does the actual retrieval of the SubjectPublicKeyInfo string on a worker
+// thread. Used by GetAllKeysWithDb().
+void GetAllKeysOnWorkerThread(std::unique_ptr<GetAllKeysState> state) {
+  DCHECK(state->slot_.get());
+
+  std::vector<std::string> public_key_spki_der_list;
+
+  // This assumes that all public keys on the slots are actually key pairs with
+  // private + public keys, so it's sufficient to get the public keys (and also
+  // not necessary to check that a private key for that public key really
+  // exists).
+  SECKEYPublicKeyList* public_keys =
+      PK11_ListPublicKeysInSlot(state->slot_.get(), /*nickname=*/nullptr);
+
+  if (!public_keys) {
+    state->CallBack(FROM_HERE, std::move(public_key_spki_der_list),
+                    std::string() /* no error */);
+    return;
+  }
+
+  for (SECKEYPublicKeyListNode* node = PUBKEY_LIST_HEAD(public_keys);
+       !PUBKEY_LIST_END(node, public_keys); node = PUBKEY_LIST_NEXT(node)) {
+    crypto::ScopedSECItem subject_public_key_info(
+        SECKEY_EncodeDERSubjectPublicKeyInfo(node->key));
+    if (!subject_public_key_info) {
+      LOG(WARNING) << "Could not encode subject public key info.";
+      continue;
+    }
+    public_key_spki_der_list.push_back(std::string(
+        subject_public_key_info->data,
+        subject_public_key_info->data + subject_public_key_info->len));
+  }
+
+  SECKEY_DestroyPublicKeyList(public_keys);
+  state->CallBack(FROM_HERE, std::move(public_key_spki_der_list),
+                  std::string() /* no error */);
+}
+
+// Continues the retrieval of the SubjectPublicKeyInfo list with |cert_db|.
+// Used by GetAllKeys().
+void GetAllKeysWithDb(std::unique_ptr<GetAllKeysState> state,
+                      net::NSSCertDatabase* cert_db) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  base::ThreadPool::PostTask(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+      base::BindOnce(&GetAllKeysOnWorkerThread, std::move(state)));
+}
+
 // Does the actual certificate importing on the IO thread. Used by
 // ImportCertificate().
 void ImportCertificateWithDB(std::unique_ptr<ImportCertificateState> state,
@@ -1238,6 +1323,19 @@ void PlatformKeysServiceImpl::GetCertificates(
                   browser_context_, state_ptr);
 }
 
+void PlatformKeysServiceImpl::GetAllKeys(const std::string& token_id,
+                                         GetAllKeysCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  auto state = std::make_unique<GetAllKeysState>(weak_factory_.GetWeakPtr(),
+                                                 std::move(callback));
+
+  NSSOperationState* state_ptr = state.get();
+  GetCertDatabase(token_id,
+                  base::BindRepeating(&GetAllKeysWithDb, base::Passed(&state)),
+                  browser_context_, state_ptr);
+}
+
 void PlatformKeysServiceImpl::ImportCertificate(
     const std::string& token_id,
     const scoped_refptr<net::X509Certificate>& certificate,