0

Revert "[webauthn] Recover from iCloud keychain"

This reverts commit 08fd217182.

Reason for revert: Failing EnclaveAuthenticatorWithPinBrowserTest.BiometricsDisabledDuringRequest
https://ci.chromium.org/ui/p/chromium/builders/ci/Mac12%20Tests/22676/overview
https://ci.chromium.org/ui/p/chromium/builders/ci/Mac13%20Tests/19006/overview

Original change's description:
> [webauthn] Recover from iCloud keychain
>
> Implement recovering passkeys from the iCloud keychain secret.
>
> This feature is guarded by the
> WebAuthenticationRecoverFromICloudRecoveryKey flag.
>
> Bug: 356399838
> Change-Id: Iaf96aeadb1f58df505cf2711038f56119e661ff9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5775800
> Commit-Queue: Ken Buchanan <kenrb@chromium.org>
> Reviewed-by: Adam Langley <agl@chromium.org>
> Auto-Submit: Nina Satragno <nsatragno@chromium.org>
> Commit-Queue: Nina Satragno <nsatragno@chromium.org>
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1345116}

Bug: 356399838
Change-Id: I752de478736094e4e02173f03ef08fb21a638a8b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5803379
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Owners-Override: Keishi Hattori <keishi@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1345267}
This commit is contained in:
Keishi Hattori
2024-08-22 06:06:12 +00:00
committed by Chromium LUCI CQ
parent 8c15fa5666
commit 1e8b7afdc2
7 changed files with 42 additions and 178 deletions
chrome/browser/webauthn
device/fido
tools/metrics/histograms/metadata/webauthn

@ -73,7 +73,6 @@
#include "components/sync/service/sync_service.h"
#include "components/sync/service/sync_service_impl.h"
#include "components/sync/test/fake_server_network_resources.h"
#include "components/trusted_vault/securebox.h"
#include "components/trusted_vault/test/mock_trusted_vault_connection.h"
#include "components/trusted_vault/trusted_vault_connection.h"
#include "components/webauthn/core/browser/passkey_model.h"
@ -981,11 +980,8 @@ class EnclaveAuthenticatorWithPinBrowserTest
public:
EnclaveAuthenticatorWithPinBrowserTest() {
scoped_feature_list_.InitWithFeaturesAndParameters(
{{
device::kWebAuthnEnclaveAuthenticator,
{{device::kWebAuthnGpmPin.name, "true"}},
},
{device::kWebAuthnRecoverFromICloudRecoveryKey, {}},
{{device::kWebAuthnEnclaveAuthenticator,
{{device::kWebAuthnGpmPin.name, "true"}}},
{device::kWebAuthnICloudRecoveryKey, {}}},
/*disabled_features=*/{
device::kWebAuthnUseInsecureSoftwareUnexportableKeys});
@ -2814,7 +2810,9 @@ IN_PROC_BROWSER_TEST_F(EnclaveICloudRecoveryKeyTest,
EXPECT_EQ(recovery_keys.size(), 1u);
}
// Tests enrolling an iCloud recovery key, then recovering from it.
// Tests enrolling an iCloud recovery key, then recovering from it. Recovery is
// not implemented yet, so this test verifies that Chrome does not try enrolling
// a new iCloud recovery key if one is already enrolled.
IN_PROC_BROWSER_TEST_F(EnclaveICloudRecoveryKeyTest, Recovery) {
{
// Do a make credential request and enroll a PIN.
@ -2866,47 +2864,22 @@ IN_PROC_BROWSER_TEST_F(EnclaveICloudRecoveryKeyTest, Recovery) {
// Expire any cache.
clock_.Advance(base::Hours(10));
// Do a make credential request and recover with the iCloud key.
// Do a make credential request and recover with a PIN.
{
// Set up the mock trusted vault connection to download the iCloud recovery
// factor that should have been added.
trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult
registration_state_result;
registration_state_result.state =
trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult::
State::kRecoverable;
registration_state_result.key_version = kSecretVersion;
registration_state_result.gpm_pin_metadata = trusted_vault::GpmPinMetadata(
"public key",
EnclaveManager::MakeWrappedPINForTesting(kSecurityDomainSecret,
"123456"),
/*expiry=*/base::Time::Now() + base::Seconds(10000));
registration_state_result.state =
trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult::
State::kRecoverable;
registration_state_result.key_version = kSecretVersion;
const auto icloud_member = std::ranges::find_if(
security_domain_service_->members(),
[](const trusted_vault_pb::SecurityDomainMember& member) {
return member.member_type() ==
trusted_vault_pb::SecurityDomainMember::
MEMBER_TYPE_ICLOUD_KEYCHAIN;
});
ASSERT_NE(icloud_member, security_domain_service_->members().end());
std::vector<trusted_vault::MemberKeys> member_keys;
auto member_key = icloud_member->memberships().at(0).keys().at(0);
member_keys.emplace_back(
member_key.epoch(),
std::vector<uint8_t>(member_key.wrapped_key().begin(),
member_key.wrapped_key().end()),
std::vector<uint8_t>(member_key.member_proof().begin(),
member_key.member_proof().end()));
registration_state_result.icloud_keys.emplace_back(
trusted_vault::SecureBoxPublicKey::CreateByImport(
std::vector<uint8_t>(icloud_member->public_key().begin(),
icloud_member->public_key().end())),
std::move(member_keys));
SetMockVaultConnectionOnRequestDelegate(
std::move(registration_state_result));
// Running the request should result in recovering automatically after the
// "Trust this computer" screen.
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
content::DOMMessageQueue message_queue(web_contents);
@ -2920,15 +2893,22 @@ IN_PROC_BROWSER_TEST_F(EnclaveICloudRecoveryKeyTest, Recovery) {
->enclave_controller_for_testing()
->account_state_for_testing(),
GPMEnclaveController::AccountState::kRecoverable);
model_observer()->SetStepToObserve(
AuthenticatorRequestDialogController::Step::kRecoverSecurityDomain);
dialog_model()->OnTrustThisComputer();
model_observer()->WaitForStep();
EnclaveManagerFactory::GetAsEnclaveManagerForProfile(browser()->profile())
->StoreKeys(kGaiaId,
{std::vector<uint8_t>(std::begin(kSecurityDomainSecret),
std::end(kSecurityDomainSecret))},
kSecretVersion);
std::string script_result;
ASSERT_TRUE(message_queue.WaitForMessage(&script_result));
EXPECT_EQ(script_result, "\"webauthn: OK\"");
delegate_observer()->WaitForDelegateDestruction();
// Make sure no new recovery key was enrolled.
// Make sure a no new recovery key was enrolled.
base::test::TestFuture<
std::vector<std::unique_ptr<device::enclave::ICloudRecoveryKey>>>
future;

@ -51,13 +51,11 @@
#include "components/device_event_log/device_event_log.h"
#include "components/prefs/pref_service.h"
#include "components/signin/public/base/consent_level.h"
#include "components/signin/public/identity_manager/account_info.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "components/signin/public/identity_manager/primary_account_access_token_fetcher.h"
#include "components/trusted_vault/frontend_trusted_vault_connection.h"
#include "components/trusted_vault/securebox.h"
#include "components/trusted_vault/trusted_vault_connection.h"
#include "components/trusted_vault/trusted_vault_crypto.h"
#include "components/trusted_vault/trusted_vault_server_constants.h"
#include "components/webauthn/core/browser/passkey_model.h"
#include "content/public/browser/render_frame_host.h"
@ -222,29 +220,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,
@ -252,24 +227,20 @@ struct GPMEnclaveController::ICloudMember {
struct GPMEnclaveController::DownloadedAccountState {
explicit DownloadedAccountState(
trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult
result,
std::string gaia_id)
result)
: 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);
});
lskf_expiries(std::move(result.lskf_expiries)) {
std::ranges::transform(
result.icloud_keys, std::back_inserter(icloud_keys),
[](const auto& key) { return key.public_key->ExportToBytes(); });
}
trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult::State
state;
std::optional<trusted_vault::GpmPinMetadata> gpm_pin_metadata;
std::vector<ICloudMember> icloud_keys;
std::vector<std::vector<uint8_t>> icloud_keys;
std::vector<base::Time> lskf_expiries;
std::string gaia_id;
};
// EnclaveUserVerificationMethod enumerates the possible ways that user
@ -586,7 +557,6 @@ void GPMEnclaveController::DownloadAccountState() {
// 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) {
@ -619,13 +589,12 @@ void GPMEnclaveController::DownloadAccountState() {
trusted_vault::SecurityDomainId::kPasskeys, identity_manager,
url_loader_factory);
auto* conn = trusted_vault_conn.get();
CoreAccountInfo account =
identity_manager->GetPrimaryAccountInfo(signin::ConsentLevel::kSignin);
download_account_state_request_ =
conn->DownloadAuthenticationFactorsRegistrationState(
account,
identity_manager->GetPrimaryAccountInfo(
signin::ConsentLevel::kSignin),
base::BindOnce(&GPMEnclaveController::OnAccountStateDownloaded,
weak_ptr_factory_.GetWeakPtr(), account.gaia,
weak_ptr_factory_.GetWeakPtr(),
std::move(trusted_vault_conn)));
}
@ -645,7 +614,6 @@ void GPMEnclaveController::OnAccountStateTimeOut() {
}
void GPMEnclaveController::OnAccountStateDownloaded(
std::string gaia_id,
std::unique_ptr<trusted_vault::TrustedVaultConnection> unused,
trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult
result) {
@ -674,7 +642,7 @@ void GPMEnclaveController::OnAccountStateDownloaded(
return;
}
DownloadedAccountState downloaded(std::move(result), std::move(gaia_id));
DownloadedAccountState downloaded(std::move(result));
GetAccountStateCache()->Put(clock_, downloaded);
OnHaveAccountState(DownloadedAccountState(std::move(downloaded)));
@ -723,7 +691,6 @@ 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);
if (device::kWebAuthnGpmPin.Get()) {
SetActive(account_state_ != AccountState::kNone);
@ -748,16 +715,13 @@ void GPMEnclaveController::SetActive(bool active) {
}
void GPMEnclaveController::OnKeysStored() {
if (model_->step() == Step::kRecoverSecurityDomain) {
device::enclave::RecordEvent(device::enclave::Event::kRecoverySuccessful);
} else {
device::enclave::RecordEvent(
device::enclave::Event::kICloudRecoverySuccessful);
if (model_->step() != Step::kRecoverSecurityDomain) {
return;
}
CHECK(enclave_manager_->has_pending_keys());
CHECK(!enclave_manager_->is_ready());
device::enclave::RecordEvent(device::enclave::Event::kRecoverySuccessful);
if (pin_metadata_.has_value() || *can_make_uv_keys_) {
if (!enclave_manager_->AddDeviceToAccount(
std::move(pin_metadata_),
@ -790,39 +754,23 @@ void GPMEnclaveController::OnDeviceAdded(bool success) {
OnEnclaveAccountSetUpComplete();
}
void GPMEnclaveController::RecoverSecurityDomain() {
#if BUILDFLAG(IS_MAC)
if (base::FeatureList::IsEnabled(
device::kWebAuthnRecoverFromICloudRecoveryKey)) {
device::enclave::ICloudRecoveryKey::Retrieve(
base::BindOnce(&GPMEnclaveController::OnICloudKeysRetrievedForRecovery,
weak_ptr_factory_.GetWeakPtr()),
kICloudKeychainRecoveryKeyAccessGroup);
} else {
model_->SetStep(Step::kRecoverSecurityDomain);
}
#else
model_->SetStep(Step::kRecoverSecurityDomain);
#endif // BUILDFLAG(IS_MAC)
}
#if BUILDFLAG(IS_MAC)
void GPMEnclaveController::MaybeAddICloudRecoveryKey() {
device::enclave::ICloudRecoveryKey::Retrieve(
base::BindOnce(&GPMEnclaveController::OnICloudKeysRetrievedForEnrollment,
base::BindOnce(&GPMEnclaveController::OnLocalICloudRecoveryKeysRetrieved,
weak_ptr_factory_.GetWeakPtr()),
kICloudKeychainRecoveryKeyAccessGroup);
}
void GPMEnclaveController::OnICloudKeysRetrievedForEnrollment(
void GPMEnclaveController::OnLocalICloudRecoveryKeysRetrieved(
std::vector<std::unique_ptr<device::enclave::ICloudRecoveryKey>>
local_icloud_keys) {
for (const GPMEnclaveController::ICloudMember& recovery_icloud_key :
for (const std::vector<uint8_t>& recovery_icloud_key :
security_domain_icloud_recovery_keys_) {
const auto local_icloud_key_it = std::ranges::find_if(
local_icloud_keys, [&recovery_icloud_key](const auto& key) {
return key->id() == recovery_icloud_key.public_key;
return key->id() == recovery_icloud_key;
});
if (local_icloud_key_it != local_icloud_keys.end()) {
// This device already has an iCloud keychain recovery factor configured
@ -864,43 +812,6 @@ void GPMEnclaveController::EnrollICloudRecoveryKey(
weak_ptr_factory_.GetWeakPtr())));
}
void GPMEnclaveController::OnICloudKeysRetrievedForRecovery(
std::vector<std::unique_ptr<device::enclave::ICloudRecoveryKey>>
local_icloud_keys) {
// Find the matching pair of local iCloud private key and the SDS recovery
// member.
auto local_icloud_key_it = local_icloud_keys.end();
auto recovery_icloud_key_it = std::ranges::find_if(
security_domain_icloud_recovery_keys_,
[&local_icloud_key_it,
&local_icloud_keys](const auto& recovery_icloud_key) {
local_icloud_key_it = std::ranges::find_if(
local_icloud_keys, [&recovery_icloud_key](const auto& key) {
return key->id() == recovery_icloud_key.public_key;
});
return local_icloud_key_it != local_icloud_keys.end();
});
if (local_icloud_key_it == local_icloud_keys.end()) {
FIDO_LOG(DEBUG) << "Could not find matching iCloud recovery key";
model_->SetStep(Step::kRecoverSecurityDomain);
return;
}
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);
if (!security_domain_secret) {
FIDO_LOG(ERROR)
<< "Could not decrypt security domain secret with iCloud key";
model_->SetStep(Step::kRecoverSecurityDomain);
return;
}
FIDO_LOG(EVENT) << "Successful recovery from iCloud recovery key";
enclave_manager_->StoreKeys(user_gaia_id_,
{std::move(*security_domain_secret)},
recovery_icloud_key_it->version);
}
#endif // BUILDFLAG(IS_MAC)
void GPMEnclaveController::OnEnclaveAccountSetUpComplete() {
@ -1030,7 +941,7 @@ void GPMEnclaveController::OnGPMPasskeySelected(
device::enclave::RecordEvent(device::enclave::Event::kOnboarding);
model_->SetStep(Step::kTrustThisComputerAssertion);
} else {
RecoverSecurityDomain();
model_->SetStep(Step::kRecoverSecurityDomain);
}
break;
@ -1103,7 +1014,7 @@ void GPMEnclaveController::OnTrustThisComputer() {
// doesn't end up being successful.
ResetDeclinedBootstrappingCount(
content::RenderFrameHost::FromID(render_frame_host_id_));
RecoverSecurityDomain();
model_->SetStep(Step::kRecoverSecurityDomain);
}
void GPMEnclaveController::OnGPMPinOptionChanged(bool is_arbitrary) {

@ -56,7 +56,6 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
public:
static constexpr base::TimeDelta kDownloadAccountStateTimeout =
base::Seconds(1);
struct ICloudMember;
struct DownloadedAccountState;
enum class EnclaveUserVerificationMethod;
@ -130,7 +129,6 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
// Called when the account state has finished downloading.
void OnAccountStateDownloaded(
std::string gaia_id,
std::unique_ptr<trusted_vault::TrustedVaultConnection> unused,
trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult
result);
@ -147,17 +145,13 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
// Called when the local device has been added to the security domain.
void OnDeviceAdded(bool success);
// Initiates recovery from an iCloud keychain recovery key or MagicArch
// depending on availability.
void RecoverSecurityDomain();
#if BUILDFLAG(IS_MAC)
// Enrolls an iCloud keychain recovery factor if available and needed.
void MaybeAddICloudRecoveryKey();
// Called when Chrome has retrieved the iCloud recovery keys present in the
// current device.
void OnICloudKeysRetrievedForEnrollment(
void OnLocalICloudRecoveryKeysRetrieved(
std::vector<std::unique_ptr<device::enclave::ICloudRecoveryKey>>
local_icloud_keys);
@ -165,12 +159,6 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
// which case we skip to the next step.
void EnrollICloudRecoveryKey(
std::unique_ptr<device::enclave::ICloudRecoveryKey> key);
// Called when Chrome has retrieved the iCloud recovery keys present in the
// current device.
void OnICloudKeysRetrievedForRecovery(
std::vector<std::unique_ptr<device::enclave::ICloudRecoveryKey>>
local_icloud_keys);
#endif // BUILDFLAG(IS_MAC)
// Called when the enclave enrollment is complete.
@ -273,9 +261,9 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
// domain service.
std::optional<trusted_vault::GpmPinMetadata> pin_metadata_;
// The list of iCloud recovery key members known to the security domain
// The list of iCloud recovery key public keys known to the security domain
// service.
std::vector<ICloudMember> security_domain_icloud_recovery_keys_;
std::vector<std::vector<uint8_t>> security_domain_icloud_recovery_keys_;
// The pending request to fetch the state of the trusted vault.
std::unique_ptr<trusted_vault::TrustedVaultConnection::Request>
@ -318,9 +306,6 @@ class GPMEnclaveController : AuthenticatorRequestDialogModel::Observer,
// Whether the user confirmed GPM PIN creation in the flow.
bool gpm_pin_creation_confirmed_ = false;
// 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};

@ -21,9 +21,8 @@ enum class Event {
kMakeCredential = 6,
kMakeCredentialPriorityShown = 7,
kMakeCredentialPriorityDeclined = 8,
kICloudRecoverySuccessful = 9,
kMaxValue = 9,
kMaxValue = 8,
};
COMPONENT_EXPORT(DEVICE_FIDO) void RecordEvent(Event event);

@ -133,11 +133,6 @@ BASE_FEATURE(kWebAuthnICloudRecoveryKey,
"WebAuthenticationICloudRecoveryKey",
base::FEATURE_DISABLED_BY_DEFAULT);
// Not yet enabled by default.
BASE_FEATURE(kWebAuthnRecoverFromICloudRecoveryKey,
"WebAuthenticationRecoverFromICloudRecoveryKey",
base::FEATURE_DISABLED_BY_DEFAULT);
// Not yet default enabled and not intended to be. Remove after M128 is Stable.
BASE_FEATURE(kWebAuthnCacheSecurityDomain,
"WebAuthenticationCacheSecurityDomain",

@ -117,11 +117,6 @@ BASE_DECLARE_FEATURE(kWebAuthnCredProtectWin10BugWorkaround);
COMPONENT_EXPORT(DEVICE_FIDO)
BASE_DECLARE_FEATURE(kWebAuthnICloudRecoveryKey);
// Retrieve and recover from recovery keys on iCloud keychain for the enclave
// authenticator.
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)

@ -229,7 +229,6 @@ chromium-metrics-reviews@google.com.
mechanism"/>
<int value="8"
label="User backed out of create passkey priority mechanism screen"/>
<int value="9" label="User recovered from an iCloud Keychain recovery key"/>
</enum>
<enum name="WebAuthenticationFidoTransport">