fido/mac: extract a PlatformCredentialStore interface

This wraps the existing CountWebAuthnCredentials and
DeleteWebAuthnCredentials functions in a new interface called
PlatformCredentialStore. The Touch ID implementation of the interface
encapsulates the |metadata_secret| and |keychain_access_group|
parameters that currently have to be fetched from the Profile and passed
explicitly. This will allow us to not leak this implementation detail
into the browsing data deletion code.

This also moves the existing TouchIdAuthenticatorConfig struct from
//content into //device/fido/mac, so that it can be passed as the ctor
argument to the credential store.

This is only a structural change. No functional changes intended.

Change-Id: I4eda36d82c80fa13a91f5ce5e508f4a23803574f
Reviewed-on: https://chromium-review.googlesource.com/1242400Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#595931}

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc

@@ -139,7 +139,7 @@
 
 #if defined(OS_MACOSX)
 #include "components/os_crypt/os_crypt_pref_names_mac.h"
-#include "device/fido/mac/browsing_data_deletion.h"
+#include "device/fido/mac/credential_store.h"
 #endif  // defined(OS_MACOSX)
 
 #if BUILDFLAG(ENABLE_PLUGINS)
@@ -792,11 +792,10 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
                              CreatePendingTaskCompletionClosureForMojo());
 
 #if defined(OS_MACOSX)
-    auto authenticator_config = ChromeAuthenticatorRequestDelegate::
-        TouchIdAuthenticatorConfigForProfile(profile_);
-    device::fido::mac::DeleteWebAuthnCredentials(
-        authenticator_config.keychain_access_group,
-        authenticator_config.metadata_secret, delete_begin_, delete_end_);
+    device::fido::mac::TouchIdCredentialStore(
+        ChromeAuthenticatorRequestDelegate::
+            TouchIdAuthenticatorConfigForProfile(profile_))
+        .DeleteCredentials(delete_begin_, delete_end_);
 
     // When clearing passwords for all time, reset preferences that are used to
     // prevent overwriting the encryption key in the Keychain.

content/public/browser/authenticator_request_client_delegate.h

@@ -15,6 +15,10 @@
 #include "device/fido/fido_request_handler_base.h"
 #include "device/fido/fido_transport_protocol.h"
 
+#if defined(OS_MACOSX)
+#include "device/fido/mac/authenticator_config.h"
+#endif
+
 namespace device {
 class FidoAuthenticator;
 }
@@ -80,18 +84,7 @@ class CONTENT_EXPORT AuthenticatorRequestClientDelegate
   virtual bool IsFocused();
 
 #if defined(OS_MACOSX)
-  struct TouchIdAuthenticatorConfig {
-    // The keychain-access-group value used for WebAuthn credentials
-    // stored in the macOS keychain by the built-in Touch ID
-    // authenticator. For more information on this, refer to
-    // |device::fido::TouchIdAuthenticator|.
-    std::string keychain_access_group;
-    // The secret used to derive key material when encrypting WebAuthn
-    // credential metadata for storage in the macOS keychain. Chrome returns
-    // different secrets for each user profile in order to logically separate
-    // credentials per profile.
-    std::string metadata_secret;
-  };
+  using TouchIdAuthenticatorConfig = device::fido::mac::AuthenticatorConfig;
 
   // Returns configuration data for the built-in Touch ID platform
   // authenticator. May return nullopt if the authenticator is not used or not

device/fido/BUILD.gn

@@ -98,6 +98,7 @@ component("fido") {
     "opaque_attestation_statement.h",
     "opaque_public_key.cc",
     "opaque_public_key.h",
+    "platform_credential_store.h",
     "public_key.cc",
     "public_key.h",
     "public_key_credential_descriptor.cc",
@@ -165,10 +166,11 @@ component("fido") {
     sources += [
       "mac/authenticator.h",
       "mac/authenticator.mm",
-      "mac/browsing_data_deletion.h",
-      "mac/browsing_data_deletion.mm",
+      "mac/authenticator_config.h",
       "mac/credential_metadata.cc",
       "mac/credential_metadata.h",
+      "mac/credential_store.h",
+      "mac/credential_store.mm",
       "mac/get_assertion_operation.h",
       "mac/get_assertion_operation.mm",
       "mac/keychain.h",

device/fido/mac/authenticator_config.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DEVICE_FIDO_MAC_AUTHENTICATOR_CONFIG_H_
+#define DEVICE_FIDO_MAC_AUTHENTICATOR_CONFIG_H_
+
+#include <string>
+
+#include "base/component_export.h"
+
+namespace device {
+namespace fido {
+namespace mac {
+
+struct COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorConfig {
+  // The keychain-access-group value used for WebAuthn credentials
+  // stored in the macOS keychain by the built-in Touch ID
+  // authenticator.
+  std::string keychain_access_group;
+  // The secret used to derive key material when encrypting WebAuthn
+  // credential metadata for storage in the macOS keychain. Chrome returns
+  // different secrets for each user profile in order to logically separate
+  // credentials per profile.
+  std::string metadata_secret;
+};
+
+}  // namespace mac
+}  // namespace fido
+}  // namespace device
+
+#endif  // DEVICE_FIDO_MAC_AUTHENTICATOR_CONFIG_H_

device/fido/mac/browsing_data_deletion.h

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef DEVICE_FIDO_MAC_BROWSING_DATA_DELETION_H_
-#define DEVICE_FIDO_MAC_BROWSING_DATA_DELETION_H_
-
-#include <string>
-
-#include "base/component_export.h"
-#include "base/time/time.h"
-
-namespace device {
-namespace fido {
-namespace mac {
-
-// DeleteWebAuthnCredentiuals 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
-// |keychain_access_group| parameter is an identifier tied to Chrome's code
-// signing identity that identifies the set of all keychain items associated
-// with the Touch ID WebAuthentication authenticator.
-//
-// Returns false if any attempt to delete a credential failed (but others may
-// still have succeeded), and true otherwise.
-//
-// On platforms where Touch ID is not supported, or when the Touch ID WebAuthn
-// authenticator feature flag is disabled, this method does nothing and returns
-// true.
-bool COMPONENT_EXPORT(DEVICE_FIDO)
-    DeleteWebAuthnCredentials(const std::string& keychain_access_group,
-                              const std::string& profile_metadata_secret,
-                              base::Time created_not_before,
-                              base::Time created_not_after);
-
-// CountWebAuthnCredentials returns the number of credentials that would get
-// deleted by a call to |DeleteWebAuthnCredentials| with identical arguments.
-size_t COMPONENT_EXPORT(DEVICE_FIDO)
-    CountWebAuthnCredentials(const std::string& keychain_access_group,
-                             const std::string& profile_metadata_secret,
-                             base::Time created_not_before,
-                             base::Time created_not_after);
-
-}  // namespace mac
-}  // namespace fido
-}  // namespace device
-
-#endif  // DEVICE_FIDO_MAC_BROWSING_DATA_DELETION_H_

device/fido/mac/browsing_data_deletion_unittest.mm

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "device/fido/mac/browsing_data_deletion.h"
+#include "device/fido/mac/credential_store.h"
 
 #include <Foundation/Foundation.h>
 #include <Security/Security.h>
@@ -17,6 +17,7 @@
 #include "device/fido/ctap_make_credential_request.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/mac/authenticator.h"
+#include "device/fido/mac/authenticator_config.h"
 #include "device/fido/mac/keychain.h"
 #include "device/fido/test_callback_receiver.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -149,14 +150,16 @@ class BrowsingDataDeletionTest : public testing::Test {
 
   bool DeleteCredentials() { return DeleteCredentials(kMetadataSecret); }
   bool DeleteCredentials(const std::string& metadata_secret) {
-    return DeleteWebAuthnCredentials(kKeychainAccessGroup, metadata_secret,
-                                     base::Time(), base::Time::Max());
+    return TouchIdCredentialStore(
+               AuthenticatorConfig{kKeychainAccessGroup, metadata_secret})
+        .DeleteCredentials(base::Time(), base::Time::Max());
   }
 
   size_t CountCredentials() { return CountCredentials(kMetadataSecret); }
   size_t CountCredentials(const std::string& metadata_secret) {
-    return CountWebAuthnCredentials(kKeychainAccessGroup, metadata_secret,
-                                    base::Time(), base::Time::Max());
+    return TouchIdCredentialStore(
+               AuthenticatorConfig{kKeychainAccessGroup, metadata_secret})
+        .CountCredentials(base::Time(), base::Time::Max());
   }
 
   base::test::ScopedFeatureList scoped_feature_list_;

device/fido/mac/credential_store.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DEVICE_FIDO_MAC_CREDENTIAL_STORE_H_
+#define DEVICE_FIDO_MAC_CREDENTIAL_STORE_H_
+
+#include "base/component_export.h"
+#include "device/fido/mac/authenticator_config.h"
+#include "device/fido/platform_credential_store.h"
+
+namespace device {
+namespace fido {
+namespace mac {
+
+class COMPONENT_EXPORT(DEVICE_FIDO) TouchIdCredentialStore
+    : public ::device::fido::PlatformCredentialStore {
+ public:
+  TouchIdCredentialStore(AuthenticatorConfig config);
+  ~TouchIdCredentialStore() override;
+
+  // 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
+  // |keychain_access_group| parameter is an identifier tied to Chrome's code
+  // signing identity that identifies the set of all keychain items associated
+  // with the Touch ID WebAuthentication authenticator.
+  //
+  // Returns false if any attempt to delete a credential failed (but others may
+  // still have succeeded), and true otherwise.
+  //
+  // On platforms where Touch ID is not supported, or when the Touch ID WebAuthn
+  // authenticator feature flag is disabled, this method does nothing and
+  // returns true.
+  bool DeleteCredentials(base::Time created_not_before,
+                         base::Time created_not_after) override;
+
+  // CountCredentials returns the number of credentials that would get
+  // deleted by a call to |DeleteWebAuthnCredentials| with identical arguments.
+  size_t CountCredentials(base::Time created_not_before,
+                          base::Time created_not_after) override;
+
+ private:
+  AuthenticatorConfig config_;
+
+  DISALLOW_COPY_AND_ASSIGN(TouchIdCredentialStore);
+};
+
+}  // namespace mac
+}  // namespace fido
+}  // namespace device
+
+#endif  // DEVICE_FIDO_MAC_CREDENTIAL_STORE_H_

device/fido/mac/browsing_data_deletion.mm → device/fido/mac/credential_store.mm

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "device/fido/mac/browsing_data_deletion.h"
+#include "device/fido/mac/credential_store.h"
 
 #include <string>
 
@@ -17,8 +17,6 @@
 #include "base/optional.h"
 #include "base/stl_util.h"
 #include "base/strings/sys_string_conversions.h"
-#include "base/time/time.h"
-#include "build/build_config.h"
 #include "device/base/features.h"
 #include "device/fido/mac/credential_metadata.h"
 #include "device/fido/mac/keychain.h"
@@ -53,7 +51,7 @@ void FilterKeychainItemsByCreationDate(
 }
 base::Optional<std::vector<base::ScopedCFTypeRef<CFDictionaryRef>>>
 QueryKeychainItemsForProfile(const std::string& keychain_access_group,
-                             const std::string& profile_metadata_secret,
+                             const std::string& metadata_secret,
                              base::Time created_not_before,
                              base::Time created_not_after)
     API_AVAILABLE(macosx(10.12.2)) {
@@ -117,7 +115,7 @@ QueryKeychainItemsForProfile(const std::string& keychain_access_group,
       continue;
     }
     base::Optional<std::string> opt_rp_id = CredentialMetadata::DecodeRpId(
-        profile_metadata_secret, base::SysCFStringRefToUTF8(sec_attr_label));
+        metadata_secret, base::SysCFStringRefToUTF8(sec_attr_label));
     if (!opt_rp_id) {
       DVLOG(1) << "key doesn't belong to this profile";
       continue;
@@ -133,15 +131,15 @@ QueryKeychainItemsForProfile(const std::string& keychain_access_group,
 }
 
 bool DoDeleteWebAuthnCredentials(const std::string& keychain_access_group,
-                                 const std::string& profile_metadata_secret,
+                                 const std::string& metadata_secret,
                                  base::Time created_not_before,
                                  base::Time created_not_after)
     API_AVAILABLE(macosx(10.12.2)) {
   bool result = true;
   base::Optional<std::vector<base::ScopedCFTypeRef<CFDictionaryRef>>>
-      keychain_items = QueryKeychainItemsForProfile(
-          keychain_access_group, profile_metadata_secret, created_not_before,
-          created_not_after);
+      keychain_items =
+          QueryKeychainItemsForProfile(keychain_access_group, metadata_secret,
+                                       created_not_before, created_not_after);
   if (!keychain_items) {
     return false;
   }
@@ -177,14 +175,14 @@ bool DoDeleteWebAuthnCredentials(const std::string& keychain_access_group,
 }
 
 size_t DoCountWebAuthnCredentials(const std::string& keychain_access_group,
-                                  const std::string& profile_metadata_secret,
+                                  const std::string& metadata_secret,
                                   base::Time created_not_before,
                                   base::Time created_not_after)
     API_AVAILABLE(macosx(10.12.2)) {
   base::Optional<std::vector<base::ScopedCFTypeRef<CFDictionaryRef>>>
-      keychain_items = QueryKeychainItemsForProfile(
-          keychain_access_group, profile_metadata_secret, created_not_before,
-          created_not_after);
+      keychain_items =
+          QueryKeychainItemsForProfile(keychain_access_group, metadata_secret,
+                                       created_not_before, created_not_after);
   if (!keychain_items) {
     DLOG(ERROR) << "Failed to query credentials in keychain";
     return 0;
@@ -194,37 +192,33 @@ size_t DoCountWebAuthnCredentials(const std::string& keychain_access_group,
 }
 }  // namespace
 
-bool DeleteWebAuthnCredentials(const std::string& keychain_access_group,
-                               const std::string& profile_metadata_secret,
-                               base::Time created_not_before,
-                               base::Time created_not_after) {
-#if defined(OS_MACOSX)
+TouchIdCredentialStore::TouchIdCredentialStore(AuthenticatorConfig config)
+    : config_(std::move(config)) {}
+TouchIdCredentialStore::~TouchIdCredentialStore() = default;
+
+bool TouchIdCredentialStore::DeleteCredentials(base::Time created_not_before,
+                                               base::Time created_not_after) {
   if (base::FeatureList::IsEnabled(device::kWebAuthTouchId)) {
     // Touch ID uses macOS APIs available in 10.12.2 or newer. No need to check
     // for credentials in lower OS versions.
     if (__builtin_available(macos 10.12.2, *)) {
-      return DoDeleteWebAuthnCredentials(keychain_access_group,
-                                         profile_metadata_secret,
+      return DoDeleteWebAuthnCredentials(config_.keychain_access_group,
+                                         config_.metadata_secret,
                                          created_not_before, created_not_after);
     }
   }
-#endif  // defined(OS_MACOSX)
   return true;
 }
 
-size_t CountWebAuthnCredentials(const std::string& keychain_access_group,
-                                const std::string& profile_metadata_secret,
-                                base::Time created_not_before,
-                                base::Time created_not_after) {
-#if defined(OS_MACOSX)
+size_t TouchIdCredentialStore::CountCredentials(base::Time created_not_before,
+                                                base::Time created_not_after) {
   if (base::FeatureList::IsEnabled(device::kWebAuthTouchId)) {
     if (__builtin_available(macos 10.12.2, *)) {
-      return DoCountWebAuthnCredentials(keychain_access_group,
-                                        profile_metadata_secret,
+      return DoCountWebAuthnCredentials(config_.keychain_access_group,
+                                        config_.metadata_secret,
                                         created_not_before, created_not_after);
     }
   }
-#endif  // defined(OS_MACOSX)
   return 0;
 }
 

device/fido/platform_credential_store.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DEVICE_FIDO_PLATFORM_CREDENTIAL_STORE_H_
+#define DEVICE_FIDO_PLATFORM_CREDENTIAL_STORE_H_
+
+#include "base/component_export.h"
+#include "base/time/time.h"
+
+namespace device {
+namespace fido {
+
+// The PlatformCredentialStore interface wraps methods for deleting WebAuthn
+// credentials that belong to authenticators integrated into Chrome (currently
+// only the TouchIdAuthenticator in //device/fido/mac).
+class COMPONENT_EXPORT(DEVICE_FIDO) PlatformCredentialStore {
+ public:
+  virtual ~PlatformCredentialStore() = default;
+
+  // DeleteCredentials deletes WebAuthn credentials that were created within the
+  // given time interval from local storage.
+  //
+  // Returns false if any attempt to delete a credential failed (but others may
+  // still have succeeded), and true otherwise.
+  virtual bool DeleteCredentials(base::Time created_not_before,
+                                 base::Time created_not_after) = 0;
+
+  // CountCredentials returns the number of credentials that would get deleted
+  // by a call to |DeleteCredentials| with identical arguments.
+  virtual size_t CountCredentials(base::Time created_not_before,
+                                  base::Time created_not_after) = 0;
+};
+
+}  // namespace fido
+}  // namespace device
+
+#endif  // DEVICE_FIDO_PLATFORM_CREDENTIAL_STORE_H_