[webauthn] Remove enclave account caching

Remove the implementation for enclave account state caching. This was
never needed, and it should be safe to remove now.

Fixed: 375049989
Change-Id: I6b689db50b891bbb2aa1c5dfaf7a39783ed1d508
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5954137
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Auto-Submit: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1373555}

chrome/browser/webauthn/chrome_authenticator_request_delegate.cc

@@ -33,7 +33,6 @@
 #include "base/strings/string_util.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
-#include "base/time/default_clock.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
@@ -1117,7 +1116,6 @@ void ChromeAuthenticatorRequestDelegate::ConfigureDiscoveries(
           enclave_controller_ = std::make_unique<GPMEnclaveController>(
               GetRenderFrameHost(), dialog_model_.get(), rp_id, request_type,
               user_verification_requirement,
-              clock_ ? clock_ : base::DefaultClock::GetInstance(),
               std::move(pending_trusted_vault_connection_));
         }
       }
@@ -1479,12 +1477,6 @@ void ChromeAuthenticatorRequestDelegate::SetTrustedVaultConnectionForTesting(
   pending_trusted_vault_connection_ = std::move(connection);
 }
 
-void ChromeAuthenticatorRequestDelegate::SetClockForTesting(
-    base::Clock* clock) {
-  CHECK(!enclave_controller_);
-  clock_ = clock;
-}
-
 content::RenderFrameHost*
 ChromeAuthenticatorRequestDelegate::GetRenderFrameHost() const {
   content::RenderFrameHost* ret =

chrome/browser/webauthn/chrome_authenticator_request_delegate.h

@@ -33,10 +33,6 @@ class GPMEnclaveController;
 class PrefService;
 class Profile;
 
-namespace base {
-class Clock;
-}
-
 namespace chromeos {
 class PasskeyDialogController;
 }
@@ -299,8 +295,6 @@ class ChromeAuthenticatorRequestDelegate
   void SetTrustedVaultConnectionForTesting(
       std::unique_ptr<trusted_vault::TrustedVaultConnection> connection);
 
-  void SetClockForTesting(base::Clock*);
-
  private:
   FRIEND_TEST_ALL_PREFIXES(ChromeAuthenticatorRequestDelegatePrivateTest,
                            DaysSinceDate);
@@ -434,7 +428,6 @@ class ChromeAuthenticatorRequestDelegate
   // `enclave_controller_` when it is created.
   std::unique_ptr<trusted_vault::TrustedVaultConnection>
       pending_trusted_vault_connection_;
-  raw_ptr<base::Clock> clock_ = nullptr;
 
   base::WeakPtrFactory<ChromeAuthenticatorRequestDelegate> weak_ptr_factory_{
       this};

chrome/browser/webauthn/enclave_authenticator_browsertest.cc

@@ -546,7 +546,6 @@ class EnclaveAuthenticatorBrowserTest : public SyncTest {
         delegate->SetTrustedVaultConnectionForTesting(
             std::move(pending_connection_));
       }
-      delegate->SetClockForTesting(&test_instance_->clock_);
     }
 
     void OnDestroy(ChromeAuthenticatorRequestDelegate* delegate) override {
@@ -715,7 +714,6 @@ class EnclaveAuthenticatorBrowserTest : public SyncTest {
     }
     scoped_icloud_drive_override_ = OverrideICloudDriveEnabled(false);
 #endif
-    clock_.SetNow(base::Time::FromTimeT(1000));
     OSCryptMocker::SetUp();
     // Log call `FIDO_LOG` messages.
     scoped_vmodule_.InitWithSwitches("device_event_log_impl=2");
@@ -973,7 +971,6 @@ class EnclaveAuthenticatorBrowserTest : public SyncTest {
   }
 
  protected:
-  base::SimpleTestClock clock_;
   net::EmbeddedTestServer https_server_{net::EmbeddedTestServer::TYPE_HTTPS};
   const TempDir temp_dir_;
   base::CallbackListSubscription subscription_;
@@ -2094,7 +2091,7 @@ IN_PROC_BROWSER_TEST_F(EnclaveAuthenticatorWithPinBrowserTest,
             AuthenticatorRequestDialogModel::Step::kConditionalMediation);
 
   // Wait for the request to time out.
-  clock_.Advance(GPMEnclaveController::kDownloadAccountStateTimeout);
+  // TODO: advance a clock.
   model_observer()->WaitForLoadingEnclaveTimeout();
 
   // Tap the passkey and expect an error.
@@ -2129,7 +2126,7 @@ IN_PROC_BROWSER_TEST_F(EnclaveAuthenticatorWithPinBrowserTest,
   // Wait for the request to time out.
   model_observer()->SetStepToObserve(
       AuthenticatorRequestDialogModel::Step::kGPMError);
-  clock_.Advance(GPMEnclaveController::kDownloadAccountStateTimeout);
+  // TODO: advance a clock.
   model_observer()->WaitForStep();
 }
 
@@ -3071,8 +3068,6 @@ IN_PROC_BROWSER_TEST_F(EnclaveICloudRecoveryKeyTest, DISABLED_Recovery) {
       ->ClearRegistrationForTesting();
   EnclaveManagerFactory::GetAsEnclaveManagerForProfile(browser()->profile())
       ->ResetForTesting();
-  // Expire any cache.
-  clock_.Advance(base::Hours(10));
 
   // Do a make credential request and recover with the iCloud key.
   {
@@ -3163,76 +3158,6 @@ IN_PROC_BROWSER_TEST_F(EnclaveICloudRecoveryKeyTest, DISABLED_Recovery) {
 
 #endif  // BUILDFLAG(IS_MAC)
 
-class EnclaveAuthenticatorCachingTest
-    : public EnclaveAuthenticatorWithoutPinBrowserTest {
-  base::test::ScopedFeatureList scoped_feature_list{
-      device::kWebAuthnCacheSecurityDomain};
-};
-
-IN_PROC_BROWSER_TEST_F(EnclaveAuthenticatorCachingTest, Caching) {
-  EnableUVKeySupport();
-  trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult
-      registration_state_result;
-
-  registration_state_result.state = trusted_vault::
-      DownloadAuthenticationFactorsRegistrationStateResult::State::kEmpty;
-  SetMockVaultConnectionOnRequestDelegate(std::move(registration_state_result));
-
-  content::WebContents* web_contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  content::DOMMessageQueue message_queue(web_contents);
-  content::ExecuteScriptAsync(web_contents, kMakeCredentialUvDiscouraged);
-  delegate_observer()->WaitForUI();
-
-  // The enclave is not active because the account is empty.
-  EXPECT_TRUE(
-      base::ranges::none_of(dialog_model()->mechanisms, [](const auto& m) {
-        return absl::holds_alternative<
-            AuthenticatorRequestDialogModel::Mechanism::Enclave>(m.type);
-      }));
-
-  dialog_model()->CancelAuthenticatorRequest();
-  std::string script_result;
-  ASSERT_TRUE(message_queue.WaitForMessage(&script_result));
-  delegate_observer()->WaitForDelegateDestruction();
-
-  // The enclave will _still_ not be active for a second request because the
-  // previous result is cached, thus no call to
-  // `SetMockVaultconnectionOnRequestDelegate` is needed.
-  content::ExecuteScriptAsync(web_contents, kMakeCredentialUvDiscouraged);
-  delegate_observer()->WaitForUI();
-
-  EXPECT_TRUE(
-      base::ranges::none_of(dialog_model()->mechanisms, [](const auto& m) {
-        return absl::holds_alternative<
-            AuthenticatorRequestDialogModel::Mechanism::Enclave>(m.type);
-      }));
-
-  dialog_model()->CancelAuthenticatorRequest();
-  ASSERT_TRUE(message_queue.WaitForMessage(&script_result));
-  delegate_observer()->WaitForDelegateDestruction();
-
-  clock_.Advance(base::Hours(10));
-  request_delegate_ = nullptr;
-  registration_state_result.state = trusted_vault::
-      DownloadAuthenticationFactorsRegistrationStateResult::State::kRecoverable;
-  SetMockVaultConnectionOnRequestDelegate(std::move(registration_state_result));
-  content::ExecuteScriptAsync(web_contents, kMakeCredentialUvDiscouraged);
-  delegate_observer()->WaitForUI();
-
-  // Now that the clock has advanced, the cache is stale and the updated account
-  // state will be noticed.
-  EXPECT_FALSE(
-      base::ranges::none_of(dialog_model()->mechanisms, [](const auto& m) {
-        return absl::holds_alternative<
-            AuthenticatorRequestDialogModel::Mechanism::Enclave>(m.type);
-      }));
-
-  dialog_model()->CancelAuthenticatorRequest();
-  ASSERT_TRUE(message_queue.WaitForMessage(&script_result));
-  delegate_observer()->WaitForDelegateDestruction();
-}
-
 #if BUILDFLAG(IS_MAC)
 #define MAYBE_MakeCredentialDeclineGPM DISABLED_MakeCredentialDeclineGPM
 #else

chrome/browser/webauthn/gpm_enclave_controller.cc

@@ -34,7 +34,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
-#include "base/time/clock.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "build/buildflag.h"
@@ -227,56 +226,6 @@ using ChangePinEvent = ChangePinControllerImpl::ChangePinEvent;
 //   +---------------------> |   StartTransaction   | <+
 //                           +----------------------+
 
-// ICloudMember holds a copyable subset of trusted_vault::VaultMember that we
-// need for recovery.
-struct GPMEnclaveController::ICloudMember {
-  explicit ICloudMember(const trusted_vault::VaultMember& member)
-      : public_key(member.public_key->ExportToBytes()) {
-    for (const auto& member_key : member.member_keys) {
-      if (member_key.version > version) {
-        version = member_key.version;
-        wrapped_key = member_key.wrapped_key;
-      }
-    }
-  }
-
-  // The result of exporting the SecureBoxPublicKey.
-  std::vector<uint8_t> public_key;
-
-  // The newest wrapped key for the member.
-  std::vector<uint8_t> wrapped_key;
-
-  // The key epoch.
-  int version = 0;
-};
-
-// DownloadedAccountState holds the subset of information from
-// `trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult` that is
-// required for `GPMEnclaveController` to work. It exists because it's copyable,
-// which the `trusted_vault` structure is not, and thus can be put in a cache.
-struct GPMEnclaveController::DownloadedAccountState {
-  explicit DownloadedAccountState(
-      trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult
-          result,
-      std::string gaia_id)
-      : state(result.state),
-        gpm_pin_metadata(std::move(result.gpm_pin_metadata)),
-        lskf_expiries(std::move(result.lskf_expiries)),
-        gaia_id(std::move(gaia_id)) {
-    std::ranges::transform(result.icloud_keys, std::back_inserter(icloud_keys),
-                           [](const trusted_vault::VaultMember& member) {
-                             return ICloudMember(member);
-                           });
-  }
-
-  trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult::State
-      state;
-  std::optional<trusted_vault::GpmPinMetadata> gpm_pin_metadata;
-  std::vector<ICloudMember> icloud_keys;
-  std::vector<base::Time> lskf_expiries;
-  std::string gaia_id;
-};
-
 // EnclaveUserVerificationMethod enumerates the possible ways that user
 // verification will be performed for an enclave transaction.
 enum class GPMEnclaveController::EnclaveUserVerificationMethod {
@@ -381,42 +330,6 @@ const char* ToString(
   }
 }
 
-// AccountStateCache caches the account state between requests to reduce the
-// load on the security domain service.
-class AccountStateCache {
- public:
-  std::optional<GPMEnclaveController::DownloadedAccountState> Get(
-      base::Clock* clock) {
-    if (!cache_time_) {
-      return std::nullopt;
-    }
-    const base::Time now = clock->Now();
-    if (now < *cache_time_ || (now - *cache_time_) > base::Minutes(30)) {
-      cache_time_.reset();
-      value_.reset();
-      return std::nullopt;
-    }
-    return value_;
-  }
-
-  void Put(base::Clock* clock,
-           const GPMEnclaveController::DownloadedAccountState& state) {
-    if (base::FeatureList::IsEnabled(device::kWebAuthnCacheSecurityDomain)) {
-      cache_time_ = clock->Now();
-      value_ = state;
-    }
-  }
-
- private:
-  std::optional<base::Time> cache_time_;
-  std::optional<GPMEnclaveController::DownloadedAccountState> value_;
-};
-
-AccountStateCache* GetAccountStateCache() {
-  static base::NoDestructor<AccountStateCache> cache;
-  return cache.get();
-}
-
 bool ExpiryTooSoon(base::Time expiry) {
   const base::Time now = base::Time::Now();
   // LSKFs must have at least 18 weeks of validity on them because we don't want
@@ -452,7 +365,6 @@ GPMEnclaveController::GPMEnclaveController(
     const std::string& rp_id,
     device::FidoRequestType request_type,
     device::UserVerificationRequirement user_verification_requirement,
-    base::Clock* clock,
     std::unique_ptr<trusted_vault::TrustedVaultConnection> optional_connection)
     : render_frame_host_id_(render_frame_host->GetGlobalId()),
       rp_id_(rp_id),
@@ -461,8 +373,7 @@ GPMEnclaveController::GPMEnclaveController(
       enclave_manager_(EnclaveManagerFactory::GetAsEnclaveManagerForProfile(
           Profile::FromBrowserContext(render_frame_host->GetBrowserContext()))),
       model_(model),
-      vault_connection_override_(std::move(optional_connection)),
+      vault_connection_override_(std::move(optional_connection)) {
-      clock_(clock) {
   enclave_manager_observer_.Observe(enclave_manager_);
   model_observer_.Observe(model_);
 
@@ -610,21 +521,6 @@ void GPMEnclaveController::OnUVCapabilityKnown(bool can_make_uv_keys) {
 }
 
 void GPMEnclaveController::DownloadAccountState() {
-  std::optional<DownloadedAccountState> maybe_cached;
-  // If the enclave_manager isn't ready then a cached account state can be used
-  // to reduce load on the security domain service. If it is ready then this
-  // must be a create() request, and we want to check that the security domain
-  // epoch hasn't changed and so don't use a cached state.
-  if (!enclave_manager_->is_ready()) {
-    // TODO(enclave): discard cache if gaia id no longer matches.
-    maybe_cached = GetAccountStateCache()->Get(clock_);
-  }
-  if (maybe_cached) {
-    FIDO_LOG(EVENT) << "Using cached account state";
-    OnHaveAccountState(std::move(*maybe_cached));
-    return;
-  }
-
   FIDO_LOG(EVENT) << "Fetching account state";
   account_state_ = AccountState::kChecking;
 
@@ -679,6 +575,8 @@ void GPMEnclaveController::OnAccountStateDownloaded(
     std::unique_ptr<trusted_vault::TrustedVaultConnection> unused,
     trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult
         result) {
+  using Result =
+      trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult;
   if (account_state_ != AccountState::kChecking) {
     // This request timed out.
     return;
@@ -704,15 +602,6 @@ void GPMEnclaveController::OnAccountStateDownloaded(
     return;
   }
 
-  DownloadedAccountState downloaded(std::move(result), std::move(gaia_id));
-  GetAccountStateCache()->Put(clock_, downloaded);
-
-  OnHaveAccountState(DownloadedAccountState(std::move(downloaded)));
-}
-
-void GPMEnclaveController::OnHaveAccountState(DownloadedAccountState result) {
-  using Result =
-      trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult;
   FIDO_LOG(EVENT) << "Account state: " << ToString(result.state)
                   << ", has PIN: " << result.gpm_pin_metadata.has_value()
                   << ", iCloud Keychain keys: " << result.icloud_keys.size();
@@ -753,7 +642,7 @@ void GPMEnclaveController::OnHaveAccountState(DownloadedAccountState result) {
     pin_metadata_ = std::move(result.gpm_pin_metadata);
   }
   security_domain_icloud_recovery_keys_ = std::move(result.icloud_keys);
-  user_gaia_id_ = std::move(result.gaia_id);
+  user_gaia_id_ = std::move(gaia_id);
 
   if (device::kWebAuthnGpmPin.Get()) {
     SetActive(account_state_ != AccountState::kNone);
@@ -856,12 +745,13 @@ void GPMEnclaveController::MaybeAddICloudRecoveryKey() {
 void GPMEnclaveController::OnICloudKeysRetrievedForEnrollment(
     std::vector<std::unique_ptr<device::enclave::ICloudRecoveryKey>>
         local_icloud_keys) {
-  for (const GPMEnclaveController::ICloudMember& recovery_icloud_key :
+  for (const trusted_vault::VaultMember& recovery_icloud_key :
        security_domain_icloud_recovery_keys_) {
+    std::vector<uint8_t> public_key =
+        recovery_icloud_key.public_key->ExportToBytes();
     const auto local_icloud_key_it = std::ranges::find_if(
-        local_icloud_keys, [&recovery_icloud_key](const auto& key) {
+        local_icloud_keys,
-          return key->id() == recovery_icloud_key.public_key;
+        [&public_key](const auto& key) { return key->id() == public_key; });
-        });
     if (local_icloud_key_it != local_icloud_keys.end()) {
       // This device already has an iCloud keychain recovery factor configured
       // for the passkey security domain. Nothing else to do here.
@@ -906,10 +796,11 @@ void GPMEnclaveController::OnICloudKeysRetrievedForRecovery(
       security_domain_icloud_recovery_keys_,
       [&local_icloud_key_it,
        &local_icloud_keys](const auto& recovery_icloud_key) {
+        std::vector<uint8_t> public_key =
+            recovery_icloud_key.public_key->ExportToBytes();
         local_icloud_key_it = std::ranges::find_if(
-            local_icloud_keys, [&recovery_icloud_key](const auto& key) {
+            local_icloud_keys,
-              return key->id() == recovery_icloud_key.public_key;
+            [&public_key](const auto& key) { return key->id() == public_key; });
-            });
         return local_icloud_key_it != local_icloud_keys.end();
       });
   if (local_icloud_key_it == local_icloud_keys.end()) {
@@ -917,10 +808,13 @@ void GPMEnclaveController::OnICloudKeysRetrievedForRecovery(
     model_->SetStep(Step::kRecoverSecurityDomain);
     return;
   }
+  const auto member_key_it = std::ranges::max_element(
+      recovery_icloud_key_it->member_keys,
+      [](const auto& k1, const auto& k2) { return k1.version < k2.version; });
   std::optional<std::vector<uint8_t>> security_domain_secret =
       trusted_vault::DecryptTrustedVaultWrappedKey(
           (*local_icloud_key_it)->key()->private_key(),
-          recovery_icloud_key_it->wrapped_key);
+          member_key_it->wrapped_key);
   if (!security_domain_secret) {
     FIDO_LOG(ERROR)
         << "Could not decrypt security domain secret with iCloud key";
@@ -931,7 +825,7 @@ void GPMEnclaveController::OnICloudKeysRetrievedForRecovery(
   recovered_with_icloud_keychain_ = true;
   enclave_manager_->StoreKeys(user_gaia_id_,
                               {std::move(*security_domain_secret)},
-                              recovery_icloud_key_it->version);
+                              member_key_it->version);
 }
 
 #endif  // BUILDFLAG(IS_MAC)

chrome/browser/webauthn/gpm_enclave_controller.h

@@ -23,10 +23,6 @@
 #include "components/trusted_vault/trusted_vault_connection.h"
 #include "content/public/browser/global_routing_id.h"
 
-namespace base {
-class Clock;
-}
-
 namespace content {
 class RenderFrameHost;
 }  // namespace content
@@ -56,8 +52,6 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
  public:
   static constexpr base::TimeDelta kDownloadAccountStateTimeout =
       base::Seconds(1);
-  struct ICloudMember;
-  struct DownloadedAccountState;
   enum class EnclaveUserVerificationMethod;
 
   enum class AccountState {
@@ -83,7 +77,6 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
       const std::string& rp_id,
       device::FidoRequestType request_type,
       device::UserVerificationRequirement user_verification_requirement,
-      base::Clock* clock,
       // `optional_connection` can be set to override the connection to the
       // security domain service for testing.
       std::unique_ptr<trusted_vault::TrustedVaultConnection>
@@ -135,8 +128,6 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
       trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult
           result);
 
-  void OnHaveAccountState(DownloadedAccountState result);
-
   // Called when enough state has been loaded that the initial UI can be shown.
   // If `active` then the enclave will be a valid mechanism.
   void SetActive(bool active);
@@ -279,7 +270,7 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
 
   // The list of iCloud recovery key members known to the security domain
   // service.
-  std::vector<ICloudMember> security_domain_icloud_recovery_keys_;
+  std::vector<trusted_vault::VaultMember> security_domain_icloud_recovery_keys_;
 
   // |recovered_with_icloud_keychain_| is true if this controller performed a
   // successful recovery from iCloud keychain. This is reset on OnKeysStored().
@@ -329,8 +320,6 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
   // The gaia id of the user at the time the account state was downloaded.
   std::string user_gaia_id_;
 
-  const raw_ptr<base::Clock> clock_;
-
   base::WeakPtrFactory<GPMEnclaveController> weak_ptr_factory_{this};
 };
 

device/fido/features.cc

@@ -113,11 +113,6 @@ BASE_FEATURE(kWebAuthnRecoverFromICloudRecoveryKey,
              "WebAuthenticationRecoverFromICloudRecoveryKey",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
-// Not yet default enabled and not intended to be. Remove after M128 is Stable.
-BASE_FEATURE(kWebAuthnCacheSecurityDomain,
-             "WebAuthenticationCacheSecurityDomain",
-             base::FEATURE_DISABLED_BY_DEFAULT);
-
 // Development flag. Must not be enabled by default.
 BASE_FEATURE(kWebAuthnEnclaveAuthenticatorDelay,
              "WebAuthnEnclaveAuthenticatorDelay",

device/fido/features.h

@@ -85,11 +85,6 @@ BASE_DECLARE_FEATURE(kWebAuthnICloudRecoveryKey);
 COMPONENT_EXPORT(DEVICE_FIDO)
 BASE_DECLARE_FEATURE(kWebAuthnRecoverFromICloudRecoveryKey);
 
-// Cache responses from the security domain. To be used if we're overloading the
-// security domain service.
-COMPONENT_EXPORT(DEVICE_FIDO)
-BASE_DECLARE_FEATURE(kWebAuthnCacheSecurityDomain);
-
 // Send enclave requests with 5 seconds delay. For development purposes only.
 COMPONENT_EXPORT(DEVICE_FIDO)
 BASE_DECLARE_FEATURE(kWebAuthnEnclaveAuthenticatorDelay);