fido/mac: move all Keychain query code into TouchIdCredentialStore

Add high-level methods for interacting with Touch ID authenticator
credentials to the existing TouchIdCredentialStore class, and replace
code in TouchIdAuthenticator and {GetAssertion,MakeCredential}Operation
that currently interacts with the macOS keychain API directly.

Also make TouchIdAuthenticator and the Operation classes hold a
TouchIdCredentialStore member, rather than the AuthenticatorConfig that
they previously required to perform operations on the Keychain API
directly.

The FindCredentialsInKeychain() and FindResidentCredentialsInKeychain()
non-member methods in keychain.h are made obsolete by the new
TouchIdCredentialStore member methods and deleted.

This is mostly a refactoring, with a few minor functional changes:
- When looking for credentials from |exclude_list|,
  MakeCredentialOperation now ignores credential descriptors with a
  transports() set that explicitly excludes platform authenticators
  (even if the descriptor's ID matches a known credential). This is
  equivalent to how allow_list is handled in GetAssertionOperation.
- The new CredentialStore methods explicitly signal unexpected macOS
  Keychain API errors in the return value, whereas with
  Find{Resident,}CredentialsInKeychain() was indistinguishable from the
  case where no matching credentials were found. Hence, when
  encountering an error while querying an exclude list e.g. a
  MakeCredential operation would have previously been allowed to
  proceed, but now the authenticator will return a CTAP error to the
  request handler instead.

Change-Id: Ib3a0a881d06fe0e20822281cbb0e3dac66b9399f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984468
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728713}

device/fido/mac/authenticator.h

@@ -16,6 +16,7 @@
 #include "base/strings/string_piece_forward.h"
 #include "device/fido/fido_authenticator.h"
 #include "device/fido/fido_transport_protocol.h"
+#include "device/fido/mac/credential_store.h"
 #include "device/fido/mac/operation.h"
 
 namespace device {
@@ -72,15 +73,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdAuthenticator
   TouchIdAuthenticator(std::string keychain_access_group,
                        std::string metadata_secret);
 
-  // The keychain access group under which credentials are stored in the macOS
-  // keychain for access control. The set of all access groups that the
-  // application belongs to is stored in the entitlements file that gets
-  // embedded into the application during code signing. For more information
-  // see
-  // https://developer.apple.com/documentation/security/ksecattraccessgroup?language=objc.
-  std::string keychain_access_group_;
-
-  std::string metadata_secret_;
+  TouchIdCredentialStore credential_store_;
 
   std::unique_ptr<Operation> operation_;
 

device/fido/mac/authenticator.mm

@@ -16,6 +16,7 @@
 #include "base/stl_util.h"
 #include "base/strings/string_piece.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "components/device_event_log/device_event_log.h"
 #include "device/base/features.h"
 #include "device/fido/authenticator_supported_options.h"
 #include "device/fido/ctap_get_assertion_request.h"
@@ -53,24 +54,23 @@ bool TouchIdAuthenticator::HasCredentialForGetAssertionRequest(
     const CtapGetAssertionRequest& request) {
   if (__builtin_available(macOS 10.12.2, *)) {
     if (request.allow_list.empty()) {
-      return !FindResidentCredentialsInKeychain(keychain_access_group_,
-                                                metadata_secret_, request.rp_id,
-                                                nullptr /* LAContext */)
-                  .empty();
+      base::Optional<std::list<Credential>> resident_credentials =
+          credential_store_.FindResidentCredentials(request.rp_id);
+      if (!resident_credentials) {
+        FIDO_LOG(ERROR) << "FindResidentCredentials() failed";
+        return false;
+      }
+      return !resident_credentials->empty();
     }
 
-    std::set<std::vector<uint8_t>> allowed_credential_ids =
-        FilterInapplicableEntriesFromAllowList(request);
-    if (allowed_credential_ids.empty()) {
-      // The allow list does not have applicable entries, but is not empty.
-      // We must not mistake this for a request for resident credentials and
-      // account choser UI.
+    base::Optional<std::list<Credential>> credentials =
+        credential_store_.FindCredentialsFromCredentialDescriptorList(
+            request.rp_id, request.allow_list);
+    if (!credentials) {
+      FIDO_LOG(ERROR) << "FindCredentialsFromCredentialDescriptorList() failed";
       return false;
     }
-    return !FindCredentialsInKeychain(keychain_access_group_, metadata_secret_,
-                                      request.rp_id, allowed_credential_ids,
-                                      nullptr /* LAContext */)
-                .empty();
+    return !credentials->empty();
   }
   NOTREACHED();
   return false;
@@ -86,8 +86,7 @@ void TouchIdAuthenticator::MakeCredential(CtapMakeCredentialRequest request,
   if (__builtin_available(macOS 10.12.2, *)) {
     DCHECK(!operation_);
     operation_ = std::make_unique<MakeCredentialOperation>(
-        std::move(request), metadata_secret_, keychain_access_group_,
-        std::move(callback));
+        std::move(request), &credential_store_, std::move(callback));
     operation_->Run();
     return;
   }
@@ -99,8 +98,7 @@ void TouchIdAuthenticator::GetAssertion(CtapGetAssertionRequest request,
   if (__builtin_available(macOS 10.12.2, *)) {
     DCHECK(!operation_);
     operation_ = std::make_unique<GetAssertionOperation>(
-        std::move(request), metadata_secret_, keychain_access_group_,
-        std::move(callback));
+        std::move(request), &credential_store_, std::move(callback));
     operation_->Run();
     return;
   }
@@ -186,8 +184,8 @@ base::WeakPtr<FidoAuthenticator> TouchIdAuthenticator::GetWeakPtr() {
 
 TouchIdAuthenticator::TouchIdAuthenticator(std::string keychain_access_group,
                                            std::string metadata_secret)
-    : keychain_access_group_(std::move(keychain_access_group)),
-      metadata_secret_(std::move(metadata_secret)),
+    : credential_store_(
+          {std::move(keychain_access_group), std::move(metadata_secret)}),
       weak_factory_(this) {}
 
 }  // namespace mac

device/fido/mac/credential_store.h

@@ -5,20 +5,110 @@
 #ifndef DEVICE_FIDO_MAC_CREDENTIAL_STORE_H_
 #define DEVICE_FIDO_MAC_CREDENTIAL_STORE_H_
 
+#include <list>
+#include <set>
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "base/component_export.h"
+#include "base/mac/availability.h"
+#include "base/mac/foundation_util.h"
+#include "base/optional.h"
 #include "device/fido/mac/authenticator_config.h"
+#include "device/fido/mac/credential_metadata.h"
 #include "device/fido/platform_credential_store.h"
+#include "device/fido/public_key_credential_descriptor.h"
+#include "device/fido/public_key_credential_user_entity.h"
 
 namespace device {
 namespace fido {
 namespace mac {
 
+class LAContext;
+
+// Credential represents a WebAuthn credential from the keychain.
+struct COMPONENT_EXPORT(FIDO) Credential {
+  Credential(base::ScopedCFTypeRef<SecKeyRef> private_key,
+             std::vector<uint8_t> credential_id);
+  ~Credential();
+  Credential(Credential&& other);
+  Credential& operator=(Credential&& other);
+
+  // An opaque reference to the private key that can be used for signing.
+  base::ScopedCFTypeRef<SecKeyRef> private_key;
+
+  // The credential ID is a handle to the key that gets passed to the RP. This
+  // ID is opaque to the RP, but is obtained by encrypting the credential
+  // metadata with a profile-specific metadata secret. See |CredentialMetadata|
+  // for more information.
+  std::vector<uint8_t> credential_id;
+
+ private:
+  Credential(const Credential&) = delete;
+  Credential& operator=(const Credential&) = delete;
+};
+
+// TouchIdCredentialStore allows operations on Touch ID platform authenticator
+// credentials stored in the macOS keychain.
 class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdCredentialStore
     : public ::device::fido::PlatformCredentialStore {
  public:
-  TouchIdCredentialStore(AuthenticatorConfig config);
+  explicit TouchIdCredentialStore(AuthenticatorConfig config);
   ~TouchIdCredentialStore() override;
 
+  // An LAContext that has been successfully evaluated using |TouchIdContext|
+  // may be passed in |authenticaton_context|, in order to authorize
+  // credentials returned by the other instance methods for signing without
+  // triggering a Touch ID prompt.
+  void set_authentication_context(LAContext* authentication_context) {
+    authentication_context_ = authentication_context;
+  }
+
+  // CreateCredential inserts a new credential into the keychain. It returns
+  // the new credential and its public key, or base::nullopt if an error
+  // occurred.
+  API_AVAILABLE(macosx(10.12.2))
+  base::Optional<std::pair<Credential, base::ScopedCFTypeRef<SecKeyRef>>>
+  CreateCredential(const std::string& rp_id,
+                   const PublicKeyCredentialUserEntity& user,
+                   bool is_resident,
+                   SecAccessControlRef access_control) const;
+
+  // FindCredentialsFromCredentialDescriptorList returns all credentials that
+  // match one of the given |descriptors| and belong to |rp_id|. A descriptor
+  // matches a credential if its transports() set is either empty or contains
+  // FidoTransportProtocol::kInternal, and if its id() is the credential ID.
+  // The returned credentials may be resident or non-resident. If any
+  // unexpected keychain API error occurs, base::nullopt is returned instead.
+  API_AVAILABLE(macosx(10.12.2))
+  base::Optional<std::list<Credential>>
+  FindCredentialsFromCredentialDescriptorList(
+      const std::string& rp_id,
+      const std::vector<PublicKeyCredentialDescriptor>& descriptors) const;
+
+  // FindResidentCredentials returns the resident credentials for the given
+  // |rp_id|, or base::nulltopt if an error occurred.
+  API_AVAILABLE(macosx(10.12.2))
+  base::Optional<std::list<Credential>> FindResidentCredentials(
+      const std::string& rp_id) const;
+
+  // UnsealMetadata returns the CredentialMetadata for the given credential's
+  // ID if it was encoded for the given RP ID, or base::nullopt otherwise.
+  API_AVAILABLE(macosx(10.12.2))
+  base::Optional<CredentialMetadata> UnsealMetadata(
+      const std::string& rp_id,
+      const Credential& credential) const;
+
+  // DeleteCredentialsForUserId deletes all (resident or non-resident)
+  // credentials for the given RP and user ID. Returns true if deleting
+  // succeeded or no matching credential exists, and false otherwise.
+  API_AVAILABLE(macosx(10.12.2))
+  bool DeleteCredentialsForUserId(const std::string& rp_id,
+                                  base::span<const uint8_t> user_id) const;
+
+  // PlatformCredentialStore:
+
   // DeleteCredentials deletes Touch ID authenticator credentials from
   // the macOS keychain that were created within the given time interval and
   // with the given metadata secret (which is tied to a browser profile). The
@@ -41,7 +131,13 @@ class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdCredentialStore
                           base::Time created_not_after) override;
 
  private:
+  API_AVAILABLE(macosx(10.12.2))
+  base::Optional<std::list<Credential>> FindCredentialsImpl(
+      const std::string& rp_id,
+      const std::set<std::vector<uint8_t>>& credential_ids) const;
+
   AuthenticatorConfig config_;
+  LAContext* authentication_context_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(TouchIdCredentialStore);
 };

device/fido/mac/get_assertion_operation.h

@@ -11,7 +11,7 @@
 #include "base/macros.h"
 #include "device/fido/authenticator_get_assertion_response.h"
 #include "device/fido/ctap_get_assertion_request.h"
-#include "device/fido/mac/keychain.h"
+#include "device/fido/mac/credential_store.h"
 #include "device/fido/mac/operation.h"
 #include "device/fido/mac/touch_id_context.h"
 
@@ -36,8 +36,7 @@ class API_AVAILABLE(macosx(10.12.2))
       base::Optional<AuthenticatorGetAssertionResponse>)>;
 
   GetAssertionOperation(CtapGetAssertionRequest request,
-                        std::string metadata_secret,
-                        std::string keychain_access_group,
+                        TouchIdCredentialStore* credential_store,
                         Callback callback);
   ~GetAssertionOperation() override;
 
@@ -53,26 +52,17 @@ class API_AVAILABLE(macosx(10.12.2))
   base::Optional<AuthenticatorGetAssertionResponse> ResponseForCredential(
       const Credential& credential);
 
-  // The secret parameter passed to |CredentialMetadata| operations to encrypt
-  // or encode credential metadata for storage in the macOS keychain.
-  const std::string metadata_secret_;
-  const std::string keychain_access_group_;
-
   const std::unique_ptr<TouchIdContext> touch_id_context_ =
       TouchIdContext::Create();
 
   const CtapGetAssertionRequest request_;
+  TouchIdCredentialStore* const credential_store_;
   Callback callback_;
   std::list<Credential> matching_credentials_;
 
   DISALLOW_COPY_AND_ASSIGN(GetAssertionOperation);
 };
 
-// Returns request.allow_list without entries that have an in inapplicable
-// |transports| field or a |type| other than "public-key".
-std::set<std::vector<uint8_t>> FilterInapplicableEntriesFromAllowList(
-    const CtapGetAssertionRequest& request);
-
 }  // namespace mac
 }  // namespace fido
 }  // namespace device

device/fido/mac/get_assertion_operation.mm

@@ -19,7 +19,6 @@
 #include "components/device_event_log/device_event_log.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/mac/credential_metadata.h"
-#include "device/fido/mac/keychain.h"
 #include "device/fido/mac/util.h"
 #include "device/fido/public_key_credential_descriptor.h"
 #include "device/fido/public_key_credential_user_entity.h"
@@ -32,14 +31,14 @@ namespace mac {
 
 using base::ScopedCFTypeRef;
 
-GetAssertionOperation::GetAssertionOperation(CtapGetAssertionRequest request,
-                                             std::string metadata_secret,
-                                             std::string keychain_access_group,
-                                             Callback callback)
-    : metadata_secret_(std::move(metadata_secret)),
-      keychain_access_group_(std::move(keychain_access_group)),
-      request_(std::move(request)),
+GetAssertionOperation::GetAssertionOperation(
+    CtapGetAssertionRequest request,
+    TouchIdCredentialStore* credential_store,
+    Callback callback)
+    : request_(std::move(request)),
+      credential_store_(credential_store),
       callback_(std::move(callback)) {}
+
 GetAssertionOperation::~GetAssertionOperation() = default;
 
 void GetAssertionOperation::Run() {
@@ -57,30 +56,28 @@ void GetAssertionOperation::PromptTouchIdDone(bool success) {
                              base::nullopt);
     return;
   }
-  std::set<std::vector<uint8_t>> allowed_credential_ids =
-      FilterInapplicableEntriesFromAllowList(request_);
-  if (allowed_credential_ids.empty() && !request_.allow_list.empty()) {
-    // The caller checking
-    // TouchIdAuthenticator::HasCredentialForGetAssertionRequest() should have
-    // caught this.
-    NOTREACHED();
-    std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrNoCredentials,
+
+  // Setting an authentication context authorizes credentials returned from the
+  // credential store for signing without triggering yet another Touch ID
+  // prompt.
+  credential_store_->set_authentication_context(
+      touch_id_context_->authentication_context());
+
+  const bool empty_allow_list = request_.allow_list.empty();
+  base::Optional<std::list<Credential>> credentials =
+      empty_allow_list
+          ? credential_store_->FindResidentCredentials(request_.rp_id)
+          : credential_store_->FindCredentialsFromCredentialDescriptorList(
+                request_.rp_id, request_.allow_list);
+
+  if (!credentials) {
+    FIDO_LOG(ERROR) << "FindCredentialsFromCredentialDescriptorList() failed";
+    std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrOther,
                              base::nullopt);
     return;
   }
-  const bool empty_allow_list = request_.allow_list.empty();
 
-  std::list<Credential> credentials =
-      empty_allow_list
-          ? FindResidentCredentialsInKeychain(
-                keychain_access_group_, metadata_secret_, request_.rp_id,
-                touch_id_context_->authentication_context())
-          : FindCredentialsInKeychain(
-                keychain_access_group_, metadata_secret_, request_.rp_id,
-                allowed_credential_ids,
-                touch_id_context_->authentication_context());
-
-  if (credentials.empty()) {
+  if (credentials->empty()) {
     // TouchIdAuthenticator::HasCredentialForGetAssertionRequest() is
     // invoked first to ensure this doesn't occur.
     NOTREACHED();
@@ -90,7 +87,7 @@ void GetAssertionOperation::PromptTouchIdDone(bool success) {
   }
 
   base::Optional<AuthenticatorGetAssertionResponse> response =
-      ResponseForCredential(credentials.front());
+      ResponseForCredential(credentials->front());
   if (!response) {
     std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrNoCredentials,
                              base::nullopt);
@@ -98,9 +95,9 @@ void GetAssertionOperation::PromptTouchIdDone(bool success) {
   }
 
   if (empty_allow_list) {
-    response->SetNumCredentials(credentials.size());
-    credentials.pop_front();
-    matching_credentials_ = std::move(credentials);
+    response->SetNumCredentials(credentials->size());
+    credentials->pop_front();
+    matching_credentials_ = std::move(*credentials);
   }
 
   std::move(callback_).Run(CtapDeviceResponseCode::kSuccess,
@@ -124,13 +121,13 @@ void GetAssertionOperation::GetNextAssertion(Callback callback) {
 
 base::Optional<AuthenticatorGetAssertionResponse>
 GetAssertionOperation::ResponseForCredential(const Credential& credential) {
-  base::Optional<CredentialMetadata> metadata = UnsealCredentialId(
-      metadata_secret_, request_.rp_id, credential.credential_id);
+  base::Optional<CredentialMetadata> metadata =
+      credential_store_->UnsealMetadata(request_.rp_id, credential);
   if (!metadata) {
     // The keychain query already filtered for the RP ID encoded under this
     // operation's metadata secret, so the credential id really should have
     // been decryptable.
-    FIDO_LOG(ERROR) << "UnsealCredentialId failed";
+    FIDO_LOG(ERROR) << "UnsealMetadata failed";
     return base::nullopt;
   }
 
@@ -150,20 +147,6 @@ GetAssertionOperation::ResponseForCredential(const Credential& credential) {
   return response;
 }
 
-std::set<std::vector<uint8_t>> FilterInapplicableEntriesFromAllowList(
-    const CtapGetAssertionRequest& request) {
-  std::set<std::vector<uint8_t>> allowed_credential_ids;
-  for (const auto& credential_descriptor : request.allow_list) {
-    if (credential_descriptor.credential_type() == CredentialType::kPublicKey &&
-        (credential_descriptor.transports().empty() ||
-         base::Contains(credential_descriptor.transports(),
-                        FidoTransportProtocol::kInternal))) {
-      allowed_credential_ids.insert(credential_descriptor.id());
-    }
-  }
-  return allowed_credential_ids;
-}
-
 }  // namespace mac
 }  // namespace fido
 }  // namespace device

device/fido/mac/get_assertion_operation_unittest_mac.mm

@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "device/fido/mac/get_assertion_operation.h"
-
 #include <array>
 
 #include <Foundation/Foundation.h>
@@ -14,6 +12,9 @@
 #include "base/test/task_environment.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/fido_test_data.h"
+#include "device/fido/mac/authenticator_config.h"
+#include "device/fido/mac/credential_store.h"
+#include "device/fido/mac/get_assertion_operation.h"
 #include "device/fido/mac/make_credential_operation.h"
 #include "device/fido/test_callback_receiver.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -45,7 +46,9 @@ bool MakeCredential() API_AVAILABLE(macos(10.12.2)) {
       PublicKeyCredentialParams(
           {{PublicKeyCredentialParams::
                 CredentialInfo() /* defaults to ES-256 */}}));
-  MakeCredentialOperation op(request, "test-profile", kKeychainAccessGroup,
+  TouchIdCredentialStore credential_store(
+      AuthenticatorConfig{"test-profile", kKeychainAccessGroup});
+  MakeCredentialOperation op(request, &credential_store,
                              callback_receiver.callback());
 
   op.Run();
@@ -68,7 +71,9 @@ API_AVAILABLE(macos(10.12.2)) {
                        base::Optional<AuthenticatorGetAssertionResponse>>
       callback_receiver;
   auto request = MakeTestRequest();
-  GetAssertionOperation op(request, "test-profile", kKeychainAccessGroup,
+  TouchIdCredentialStore credential_store(
+      AuthenticatorConfig{"test-profile", kKeychainAccessGroup});
+  GetAssertionOperation op(request, &credential_store,
                            callback_receiver.callback());
 
   op.Run();

device/fido/mac/keychain.h

@@ -5,12 +5,6 @@
 #ifndef DEVICE_FIDO_MAC_KEYCHAIN_H_
 #define DEVICE_FIDO_MAC_KEYCHAIN_H_
 
-#include <stdint.h>
-#include <list>
-#include <set>
-#include <string>
-#include <vector>
-
 #import <Foundation/Foundation.h>
 #import <LocalAuthentication/LocalAuthentication.h>
 #import <Security/Security.h>
@@ -19,7 +13,6 @@
 #include "base/mac/scoped_cftyperef.h"
 #include "base/macros.h"
 #include "base/no_destructor.h"
-#include "base/optional.h"
 
 namespace device {
 namespace fido {
@@ -71,57 +64,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) API_AVAILABLE(macos(10.12.2)) Keychain {
 
   DISALLOW_COPY_AND_ASSIGN(Keychain);
 };
-
-// Credential represents a WebAuthn credential from the keychain.
-struct COMPONENT_EXPORT(FIDO) Credential {
-  Credential(base::ScopedCFTypeRef<SecKeyRef> private_key,
-             std::vector<uint8_t> credential_id);
-  ~Credential();
-  Credential(Credential&& other);
-  Credential& operator=(Credential&& other);
-
-  // An opaque reference to the private key that can be used for signing.
-  base::ScopedCFTypeRef<SecKeyRef> private_key;
-
-  // The credential ID is a handle to the key that gets passed to the RP. This
-  // ID is opaque to the RP, but is obtained by encrypting the credential
-  // metadata with a profile-specific metadata secret. See |CredentialMetadata|
-  // for more information.
-  std::vector<uint8_t> credential_id;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(Credential);
-};
-
-// FindCredentialsInKeychain returns a list of credentials for the given
-// |rp_id| with credential IDs matching those from |allowed_credential_ids|,
-// which may not be empty. The returned credentials may be resident or
-// non-resident.
-//
-// An LAContext that has been successfully evaluated using |TouchIdContext| may
-// be passed in |authenticaton_context|, in order to authorize the credential's
-// private key for signing. The authentication may also be null if the caller
-// only wants to check for existence of a key, but does not intend to create a
-// signature from it. (I.e., the credential's SecKeyRef should not be passed to
-// |KeyCreateSignature| if no authentication context was provided, since that
-// would trigger a Touch ID prompt dialog).
-COMPONENT_EXPORT(FIDO)
-std::list<Credential> FindCredentialsInKeychain(
-    const std::string& keychain_access_group,
-    const std::string& metadata_secret,
-    const std::string& rp_id,
-    const std::set<std::vector<uint8_t>>& allowed_credential_ids,
-    LAContext* authentication_context) API_AVAILABLE(macosx(10.12.2));
-
-// FindResidentCredentialsInKeychain returns a list of resident credentials for
-// the given |rp_id|.
-COMPONENT_EXPORT(FIDO)
-std::list<Credential> FindResidentCredentialsInKeychain(
-    const std::string& keychain_access_group,
-    const std::string& metadata_secret,
-    const std::string& rp_id,
-    LAContext* authentication_context) API_AVAILABLE(macosx(10.12.2));
-
 }  // namespace mac
 }  // namespace fido
 }  // namespace device

device/fido/mac/keychain.mm

@@ -4,15 +4,6 @@
 
 #include "device/fido/mac/keychain.h"
 
-#import <Foundation/Foundation.h>
-
-#include "base/mac/foundation_util.h"
-#include "base/mac/mac_logging.h"
-#include "base/stl_util.h"
-#include "base/strings/sys_string_conversions.h"
-#include "components/device_event_log/device_event_log.h"
-#include "device/fido/mac/credential_metadata.h"
-
 namespace device {
 namespace fido {
 namespace mac {
@@ -71,115 +62,6 @@ OSStatus Keychain::ItemDelete(CFDictionaryRef query) {
   return SecItemDelete(query);
 }
 
-Credential::Credential(base::ScopedCFTypeRef<SecKeyRef> private_key_,
-                       std::vector<uint8_t> credential_id_)
-    : private_key(std::move(private_key_)),
-      credential_id(std::move(credential_id_)) {}
-Credential::~Credential() = default;
-Credential::Credential(Credential&& other) = default;
-Credential& Credential::operator=(Credential&& other) = default;
-
-// Like FindCredentialsInKeychain(), but empty |allowed_credential_ids| allows
-// any credential to match.
-static std::list<Credential> FindCredentialsImpl(
-    const std::string& keychain_access_group,
-    const std::string& metadata_secret,
-    const std::string& rp_id,
-    const std::set<std::vector<uint8_t>>& allowed_credential_ids,
-    LAContext* authentication_context) API_AVAILABLE(macosx(10.12.2)) {
-  base::ScopedCFTypeRef<CFMutableDictionaryRef> query(
-      CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
-  CFDictionarySetValue(query, kSecClass, kSecClassKey);
-  CFDictionarySetValue(query, kSecAttrAccessGroup,
-                       base::SysUTF8ToNSString(keychain_access_group));
-  CFDictionarySetValue(
-      query, kSecAttrLabel,
-      base::SysUTF8ToNSString(EncodeRpId(metadata_secret, rp_id)));
-  if (authentication_context) {
-    CFDictionarySetValue(query, kSecUseAuthenticationContext,
-                         authentication_context);
-  }
-  CFDictionarySetValue(query, kSecReturnRef, @YES);
-  CFDictionarySetValue(query, kSecReturnAttributes, @YES);
-  CFDictionarySetValue(query, kSecMatchLimit, kSecMatchLimitAll);
-
-  base::ScopedCFTypeRef<CFArrayRef> keychain_items;
-  OSStatus status = Keychain::GetInstance().ItemCopyMatching(
-      query, reinterpret_cast<CFTypeRef*>(keychain_items.InitializeInto()));
-  if (status == errSecItemNotFound) {
-    // No credentials for the RP.
-    return {};
-  }
-  if (status != errSecSuccess) {
-    OSSTATUS_DLOG(ERROR, status) << "SecItemCopyMatching failed";
-    return {};
-  }
-
-  // Filter credentials for the RP down to |allowed_credential_ids|, unless it's
-  // empty in which case all credentials should be returned.
-  std::list<Credential> result;
-  for (CFIndex i = 0; i < CFArrayGetCount(keychain_items); ++i) {
-    CFDictionaryRef attributes = base::mac::CFCast<CFDictionaryRef>(
-        CFArrayGetValueAtIndex(keychain_items, i));
-    CFDataRef application_label = base::mac::GetValueFromDictionary<CFDataRef>(
-        attributes, kSecAttrApplicationLabel);
-    SecKeyRef key =
-        base::mac::GetValueFromDictionary<SecKeyRef>(attributes, kSecValueRef);
-    if (!application_label || !key) {
-      // Corrupted keychain?
-      DLOG(ERROR) << "could not find application label or key ref: "
-                  << attributes;
-      continue;
-    }
-    std::vector<uint8_t> credential_id(CFDataGetBytePtr(application_label),
-                                       CFDataGetBytePtr(application_label) +
-                                           CFDataGetLength(application_label));
-    if (!allowed_credential_ids.empty() &&
-        !base::Contains(allowed_credential_ids, credential_id)) {
-      continue;
-    }
-    base::ScopedCFTypeRef<SecKeyRef> private_key(key,
-                                                 base::scoped_policy::RETAIN);
-    result.emplace_back(
-        Credential(std::move(private_key), std::move(credential_id)));
-  }
-  return result;
-}
-
-std::list<Credential> FindCredentialsInKeychain(
-    const std::string& keychain_access_group,
-    const std::string& metadata_secret,
-    const std::string& rp_id,
-    const std::set<std::vector<uint8_t>>& allowed_credential_ids,
-    LAContext* authentication_context) {
-  if (allowed_credential_ids.empty()) {
-    NOTREACHED();
-    return {};
-  }
-  return FindCredentialsImpl(keychain_access_group, metadata_secret, rp_id,
-                             allowed_credential_ids, authentication_context);
-}
-
-std::list<Credential> FindResidentCredentialsInKeychain(
-    const std::string& keychain_access_group,
-    const std::string& metadata_secret,
-    const std::string& rp_id,
-    LAContext* authentication_context) {
-  std::list<Credential> result = FindCredentialsImpl(
-      keychain_access_group, metadata_secret, rp_id,
-      /*allowed_credential_ids=*/{}, authentication_context);
-  result.remove_if([&metadata_secret, &rp_id](const Credential& credential) {
-    auto opt_metadata =
-        UnsealCredentialId(metadata_secret, rp_id, credential.credential_id);
-    if (!opt_metadata) {
-      FIDO_LOG(ERROR) << "UnsealCredentialId() failed";
-      return true;
-    }
-    return !opt_metadata->is_resident;
-  });
-  return result;
-}
-
 }  // namespace mac
 }  // namespace fido
 }  // namespace device

device/fido/mac/make_credential_operation.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "device/fido/authenticator_make_credential_response.h"
 #include "device/fido/ctap_make_credential_request.h"
+#include "device/fido/mac/credential_store.h"
 #include "device/fido/mac/operation.h"
 #include "device/fido/mac/touch_id_context.h"
 
@@ -52,8 +53,7 @@ class API_AVAILABLE(macosx(10.12.2))
       base::Optional<AuthenticatorMakeCredentialResponse>)>;
 
   MakeCredentialOperation(CtapMakeCredentialRequest request,
-                          std::string profile_id,
-                          std::string keychain_access_group,
+                          TouchIdCredentialStore* credential_store,
                           Callback callback);
   ~MakeCredentialOperation() override;
 
@@ -63,21 +63,11 @@ class API_AVAILABLE(macosx(10.12.2))
  private:
   void PromptTouchIdDone(bool success);
 
-  // DefaultKeychainQuery returns a default keychain query dictionary that has
-  // the keychain item class, keychain access group and RP ID filled out (but
-  // not the credential ID). More fields can be set on the return value to
-  // refine the query.
-  base::ScopedCFTypeRef<CFMutableDictionaryRef> DefaultKeychainQuery() const;
-
-  // The secret parameter passed to |CredentialMetadata| operations to encrypt
-  // or encode credential metadata for storage in the macOS keychain.
-  const std::string metadata_secret_;
-  const std::string keychain_access_group_;
-
   const std::unique_ptr<TouchIdContext> touch_id_context_ =
       TouchIdContext::Create();
 
   const CtapMakeCredentialRequest request_;
+  TouchIdCredentialStore* const credential_store_;
   Callback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(MakeCredentialOperation);

device/fido/mac/make_credential_operation.mm

@@ -22,7 +22,7 @@
 #include "device/fido/fido_parsing_utils.h"
 #include "device/fido/fido_transport_protocol.h"
 #include "device/fido/mac/credential_metadata.h"
-#include "device/fido/mac/keychain.h"
+#include "device/fido/mac/credential_store.h"
 #include "device/fido/mac/util.h"
 #include "device/fido/strings/grit/fido_strings.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -31,17 +31,14 @@ namespace device {
 namespace fido {
 namespace mac {
 
-using base::ScopedCFTypeRef;
-
 MakeCredentialOperation::MakeCredentialOperation(
     CtapMakeCredentialRequest request,
-    std::string metadata_secret,
-    std::string keychain_access_group,
+    TouchIdCredentialStore* credential_store,
     Callback callback)
-    : metadata_secret_(std::move(metadata_secret)),
-      keychain_access_group_(std::move(keychain_access_group)),
-      request_(std::move(request)),
+    : request_(std::move(request)),
+      credential_store_(credential_store),
       callback_(std::move(callback)) {}
+
 MakeCredentialOperation::~MakeCredentialOperation() = default;
 
 void MakeCredentialOperation::Run() {
@@ -76,91 +73,47 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
     return;
   }
 
-  // Evaluate that excludeList does not contain any credentials stored by this
-  // authenticator.
-  for (auto& credential : request_.exclude_list) {
-    ScopedCFTypeRef<CFMutableDictionaryRef> query = DefaultKeychainQuery();
-    CFDictionarySetValue(query, kSecAttrApplicationLabel,
-                         [NSData dataWithBytes:credential.id().data()
-                                        length:credential.id().size()]);
-    OSStatus status = SecItemCopyMatching(query, nullptr);
-    if (status == errSecSuccess) {
-      // Excluded item found.
-      DVLOG(1) << "credential from excludeList found";
-      std::move(callback_).Run(
-          CtapDeviceResponseCode::kCtap2ErrCredentialExcluded, base::nullopt);
-      return;
-    }
-    if (status != errSecItemNotFound) {
-      // Unexpected keychain error.
-      OSSTATUS_DLOG(ERROR, status) << "failed to check for excluded credential";
+  // Setting an authentication context authorizes credentials returned from the
+  // credential store for signing without triggering yet another Touch ID
+  // prompt.
+  credential_store_->set_authentication_context(
+      touch_id_context_->authentication_context());
+
+  if (!request_.exclude_list.empty()) {
+    base::Optional<std::list<Credential>> credentials =
+        credential_store_->FindCredentialsFromCredentialDescriptorList(
+            request_.rp.id, request_.exclude_list);
+    if (!credentials) {
+      FIDO_LOG(ERROR) << "Failed to check for excluded credentials";
       std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrOther,
                                base::nullopt);
       return;
     }
-  }
-
-  // Delete the key pair for this RP + user handle if one already exists.
-  //
-  // Note that because the rk bit is not encoded here, a resident credential
-  // may overwrite a non-resident credential and vice versa.
-  const std::string encoded_rp_id_user_id =
-      EncodeRpIdAndUserId(metadata_secret_, request_.rp.id, request_.user.id);
-  {
-    ScopedCFTypeRef<CFMutableDictionaryRef> query = DefaultKeychainQuery();
-    CFDictionarySetValue(query, kSecAttrApplicationTag,
-                         base::SysUTF8ToNSString(encoded_rp_id_user_id));
-    OSStatus status = Keychain::GetInstance().ItemDelete(query);
-    if (status != errSecSuccess && status != errSecItemNotFound) {
-      OSSTATUS_DLOG(ERROR, status) << "SecItemDelete failed";
-      std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrOther,
-                               base::nullopt);
+    if (!credentials->empty()) {
+      std::move(callback_).Run(
+          CtapDeviceResponseCode::kCtap2ErrCredentialExcluded, base::nullopt);
       return;
     }
   }
 
-  // Generate the new key pair.
-  const std::vector<uint8_t> credential_id =
-      SealCredentialId(metadata_secret_, request_.rp.id,
-                       CredentialMetadata::FromPublicKeyCredentialUserEntity(
-                           request_.user, request_.resident_key_required));
-
-  ScopedCFTypeRef<CFMutableDictionaryRef> params(
-      CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
-  CFDictionarySetValue(params, kSecAttrKeyType,
-                       kSecAttrKeyTypeECSECPrimeRandom);
-  CFDictionarySetValue(params, kSecAttrKeySizeInBits, @256);
-  CFDictionarySetValue(params, kSecAttrSynchronizable, @NO);
-  CFDictionarySetValue(params, kSecAttrTokenID, kSecAttrTokenIDSecureEnclave);
-
-  ScopedCFTypeRef<CFMutableDictionaryRef> private_key_params =
-      DefaultKeychainQuery();
-  CFDictionarySetValue(params, kSecPrivateKeyAttrs, private_key_params);
-  CFDictionarySetValue(private_key_params, kSecAttrIsPermanent, @YES);
-  CFDictionarySetValue(private_key_params, kSecAttrAccessControl,
-                       touch_id_context_->access_control());
-  CFDictionarySetValue(private_key_params, kSecUseAuthenticationContext,
-                       touch_id_context_->authentication_context());
-  CFDictionarySetValue(private_key_params, kSecAttrApplicationTag,
-                       base::SysUTF8ToNSString(encoded_rp_id_user_id));
-  CFDictionarySetValue(private_key_params, kSecAttrApplicationLabel,
-                       [NSData dataWithBytes:credential_id.data()
-                                      length:credential_id.size()]);
-
-  ScopedCFTypeRef<CFErrorRef> cferr;
-  ScopedCFTypeRef<SecKeyRef> private_key(
-      Keychain::GetInstance().KeyCreateRandomKey(params,
-                                                 cferr.InitializeInto()));
-  if (!private_key) {
-    FIDO_LOG(ERROR) << "SecKeyCreateRandomKey failed: " << cferr;
+  // Delete the key pair for this RP + user handle if one already exists.
+  //
+  // TODO(crbug/1025065): Decide whether we should evict non-resident
+  // credentials at all.
+  if (!credential_store_->DeleteCredentialsForUserId(request_.rp.id,
+                                                     request_.user.id)) {
     std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrOther,
                              base::nullopt);
     return;
   }
-  ScopedCFTypeRef<SecKeyRef> public_key(
-      Keychain::GetInstance().KeyCopyPublicKey(private_key));
-  if (!public_key) {
-    FIDO_LOG(ERROR) << "SecKeyCopyPublicKey failed";
+
+  // Generate the new key pair.
+  base::Optional<std::pair<Credential, base::ScopedCFTypeRef<SecKeyRef>>>
+      credential = credential_store_->CreateCredential(
+          request_.rp.id, request_.user, request_.resident_key_required,
+          touch_id_context_->access_control());
+  if (!credential) {
+    FIDO_LOG(ERROR) << "CreateCredential() failed";
     std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrOther,
                              base::nullopt);
     return;
@@ -169,8 +122,8 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
   // Create attestation object. There is no separate attestation key pair, so
   // we perform self-attestation.
   base::Optional<AttestedCredentialData> attested_credential_data =
-      MakeAttestedCredentialData(credential_id,
-                                 SecKeyRefToECPublicKey(public_key));
+      MakeAttestedCredentialData(credential->first.credential_id,
+                                 SecKeyRefToECPublicKey(credential->second));
   if (!attested_credential_data) {
     FIDO_LOG(ERROR) << "MakeAttestedCredentialData failed";
     std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrOther,
@@ -179,8 +132,9 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
   }
   AuthenticatorData authenticator_data = MakeAuthenticatorData(
       request_.rp.id, std::move(*attested_credential_data));
-  base::Optional<std::vector<uint8_t>> signature = GenerateSignature(
-      authenticator_data, request_.client_data_hash, private_key);
+  base::Optional<std::vector<uint8_t>> signature =
+      GenerateSignature(authenticator_data, request_.client_data_hash,
+                        credential->first.private_key);
   if (!signature) {
     FIDO_LOG(ERROR) << "MakeSignature failed";
     std::move(callback_).Run(CtapDeviceResponseCode::kCtap2ErrOther,
@@ -198,18 +152,6 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
                            std::move(response));
 }
 
-base::ScopedCFTypeRef<CFMutableDictionaryRef>
-MakeCredentialOperation::DefaultKeychainQuery() const {
-  base::ScopedCFTypeRef<CFMutableDictionaryRef> query(
-      CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
-  CFDictionarySetValue(query, kSecClass, kSecClassKey);
-  CFDictionarySetValue(query, kSecAttrAccessGroup,
-                       base::SysUTF8ToNSString(keychain_access_group_));
-  CFDictionarySetValue(
-      query, kSecAttrLabel,
-      base::SysUTF8ToNSString(EncodeRpId(metadata_secret_, request_.rp.id)));
-  return query;
-}
 }  // namespace mac
 }  // namespace fido
 }  // namespace device

device/fido/mac/make_credential_operation_unittest_mac.mm

@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "device/fido/mac/make_credential_operation.h"
-
 #include <array>
 
 #include <Foundation/Foundation.h>
@@ -14,6 +12,9 @@
 #include "base/test/task_environment.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/fido_test_data.h"
+#include "device/fido/mac/authenticator_config.h"
+#include "device/fido/mac/credential_store.h"
+#include "device/fido/mac/make_credential_operation.h"
 #include "device/fido/test_callback_receiver.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -49,7 +50,9 @@ API_AVAILABLE(macosx(10.12.2)) {
                        base::Optional<AuthenticatorMakeCredentialResponse>>
       callback_receiver;
   auto request = MakeTestRequest();
-  MakeCredentialOperation op(request, "test-profile", kKeychainAccessGroup,
+  TouchIdCredentialStore credential_store(
+      AuthenticatorConfig{"test-profile", kKeychainAccessGroup});
+  MakeCredentialOperation op(request, &credential_store,
                              callback_receiver.callback());
 
   op.Run();