Skip to content

Commit

Permalink
trusted_vault: retrieve hw_protected keys behind a flag
Browse files Browse the repository at this point in the history
In Ash, introduce a separate TrustedVaultClient for the passkeys
security domain (which is named `hw_protected` for historical reasons).
Ash will use the passkeys domain secret to implement a crosapi service
for creating and challenging passkeys belonging to the primary profile.

Instantiation of the new TrustedVaultClient is guarded by the feature
flag for ChromeOS passkey support (default disabled). In addition,
retrieving the domain secret requires enabling the privileged
chrome.setClientEncryptionKeys() JS API, which currently also is behind
a default disabled flag.

Bug: 1223853
Change-Id: I60c344a13d92321fa8b1d4c96fd79f9f9df7af5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4953915
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1215750}
  • Loading branch information
kreichgauer authored and Chromium LUCI CQ committed Oct 26, 2023
1 parent 5e0687d commit d510e09
Show file tree
Hide file tree
Showing 18 changed files with 349 additions and 98 deletions.
13 changes: 8 additions & 5 deletions chrome/browser/ash/login/session/user_session_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -441,15 +441,18 @@ void SaveSyncTrustedVaultKeysToProfile(
}

if (!trusted_vault_keys.encryption_keys().empty()) {
trusted_vault_service->GetTrustedVaultClient()->StoreKeys(
gaia_id, trusted_vault_keys.encryption_keys(),
trusted_vault_keys.last_encryption_key_version());
trusted_vault_service
->GetTrustedVaultClient(trusted_vault::SecurityDomainId::kChromeSync)
->StoreKeys(gaia_id, trusted_vault_keys.encryption_keys(),
trusted_vault_keys.last_encryption_key_version());
}

for (const SyncTrustedVaultKeys::TrustedRecoveryMethod& method :
trusted_vault_keys.trusted_recovery_methods()) {
trusted_vault_service->GetTrustedVaultClient()->AddTrustedRecoveryMethod(
gaia_id, method.public_key, method.type_hint, base::DoNothing());
trusted_vault_service
->GetTrustedVaultClient(trusted_vault::SecurityDomainId::kChromeSync)
->AddTrustedRecoveryMethod(gaia_id, method.public_key, method.type_hint,
base::DoNothing());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "chrome/browser/profiles/profile_keyed_service_factory.h"
#include "chrome/browser/signin/identity_manager_factory.h"
#include "chrome/browser/trusted_vault/trusted_vault_service_factory.h"
#include "components/trusted_vault/trusted_vault_server_constants.h"
#include "components/trusted_vault/trusted_vault_service.h"

namespace ash {
Expand Down Expand Up @@ -47,8 +48,8 @@ TrustedVaultBackendServiceFactoryAsh::BuildServiceInstanceForBrowserContext(
Profile* profile = Profile::FromBrowserContext(context);
return std::make_unique<TrustedVaultBackendServiceAsh>(
IdentityManagerFactory::GetForProfile(profile),
TrustedVaultServiceFactory::GetForProfile(profile)
->GetTrustedVaultClient());
TrustedVaultServiceFactory::GetForProfile(profile)->GetTrustedVaultClient(
trusted_vault::SecurityDomainId::kChromeSync));
}

} // namespace ash
2 changes: 1 addition & 1 deletion chrome/browser/sync/chrome_sync_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ ChromeSyncClient::CreateDataTypeControllers(syncer::SyncService* sync_service) {

trusted_vault::TrustedVaultClient* ChromeSyncClient::GetTrustedVaultClient() {
return TrustedVaultServiceFactory::GetForProfile(profile_)
->GetTrustedVaultClient();
->GetTrustedVaultClient(trusted_vault::SecurityDomainId::kChromeSync);
}

syncer::SyncInvalidationsService*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -988,9 +988,9 @@ class SingleClientNigoriWithWebApiTest : public SyncTest {
return security_domains_server_.get();
}

trusted_vault::TrustedVaultClient* GetTrustedVaultClient() {
trusted_vault::TrustedVaultClient* GetSyncTrustedVaultClient() {
return TrustedVaultServiceFactory::GetForProfile(GetProfile(0))
->GetTrustedVaultClient();
->GetTrustedVaultClient(trusted_vault::SecurityDomainId::kChromeSync);
}

protected:
Expand Down Expand Up @@ -1255,7 +1255,7 @@ IN_PROC_BROWSER_TEST_P(
/*trusted_vault_keys=*/{trusted_vault_key}),
GetFakeServer());
ASSERT_TRUE(SetupClients());
GetTrustedVaultClient()->StoreKeys(
GetSyncTrustedVaultClient()->StoreKeys(
kGaiaId, GetSecurityDomainsServer()->GetAllTrustedVaultKeys(),
/*last_key_version=*/GetSecurityDomainsServer()->GetCurrentEpoch());

Expand Down Expand Up @@ -1369,7 +1369,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,
.Wait());
base::RunLoop run_loop;
static_cast<trusted_vault::StandaloneTrustedVaultClient*>(
GetSyncService(0)->GetSyncClientForTest()->GetTrustedVaultClient())
GetSyncTrustedVaultClient())
->WaitForFlushForTesting(run_loop.QuitClosure());
run_loop.Run();
}
Expand Down Expand Up @@ -1680,7 +1680,7 @@ IN_PROC_BROWSER_TEST_F(
.Wait());
base::RunLoop run_loop;
static_cast<trusted_vault::StandaloneTrustedVaultClient*>(
GetSyncService(0)->GetSyncClientForTest()->GetTrustedVaultClient())
GetSyncTrustedVaultClient())
->WaitForFlushForTesting(run_loop.QuitClosure());
run_loop.Run();
}
Expand Down Expand Up @@ -1726,7 +1726,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,
/*trusted_vault_keys=*/{trusted_vault_key}, migration_time),
GetFakeServer());
ASSERT_TRUE(SetupClients());
GetTrustedVaultClient()->StoreKeys(
GetSyncTrustedVaultClient()->StoreKeys(
kGaiaId, GetSecurityDomainsServer()->GetAllTrustedVaultKeys(),
/*last_key_version=*/GetSecurityDomainsServer()->GetCurrentEpoch());
ASSERT_TRUE(SetupSync());
Expand Down Expand Up @@ -1822,14 +1822,14 @@ IN_PROC_BROWSER_TEST_F(
// Mimic the key being available upon startup but recoverability degraded.
GetSecurityDomainsServer()->RequirePublicKeyToAvoidRecoverabilityDegraded(
kTestRecoveryMethodPublicKey);
GetTrustedVaultClient()->StoreKeys(
GetSyncTrustedVaultClient()->StoreKeys(
kGaiaId, GetSecurityDomainsServer()->GetAllTrustedVaultKeys(),
/*last_key_version=*/GetSecurityDomainsServer()->GetCurrentEpoch());

// Mimic a recovery method being added before or during sign-in, which should
// be deferred until sign-in completes.
base::RunLoop run_loop;
GetTrustedVaultClient()->AddTrustedRecoveryMethod(
GetSyncTrustedVaultClient()->AddTrustedRecoveryMethod(
kGaiaId, kTestRecoveryMethodPublicKey, kTestMethodTypeHint,
run_loop.QuitClosure());

Expand Down Expand Up @@ -1865,7 +1865,7 @@ IN_PROC_BROWSER_TEST_F(
// Mimic the key being available upon startup but recoverability degraded.
GetSecurityDomainsServer()->RequirePublicKeyToAvoidRecoverabilityDegraded(
kTestRecoveryMethodPublicKey);
GetTrustedVaultClient()->StoreKeys(
GetSyncTrustedVaultClient()->StoreKeys(
kGaiaId, GetSecurityDomainsServer()->GetAllTrustedVaultKeys(),
/*last_key_version=*/GetSecurityDomainsServer()->GetCurrentEpoch());
ASSERT_TRUE(GetSecurityDomainsServer()->IsRecoverabilityDegraded());
Expand All @@ -1880,7 +1880,7 @@ IN_PROC_BROWSER_TEST_F(
// Mimic a recovery method being added during a persistent auth error, which
// should be deferred until the auth error is resolved.
base::RunLoop run_loop;
GetTrustedVaultClient()->AddTrustedRecoveryMethod(
GetSyncTrustedVaultClient()->AddTrustedRecoveryMethod(
kGaiaId, kTestRecoveryMethodPublicKey, kTestMethodTypeHint,
run_loop.QuitClosure());

Expand Down Expand Up @@ -1910,7 +1910,7 @@ IN_PROC_BROWSER_TEST_F(
/*trusted_vault_keys=*/{trusted_vault_key}),
GetFakeServer());
ASSERT_TRUE(SetupClients());
GetTrustedVaultClient()->StoreKeys(
GetSyncTrustedVaultClient()->StoreKeys(
kGaiaId, GetSecurityDomainsServer()->GetAllTrustedVaultKeys(),
/*last_key_version=*/GetSecurityDomainsServer()->GetCurrentEpoch());
ASSERT_TRUE(SetupSync());
Expand Down Expand Up @@ -2040,7 +2040,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientNigoriWithWebApiTest,

// Mimic a recovery method being added.
base::RunLoop run_loop;
GetTrustedVaultClient()->AddTrustedRecoveryMethod(
GetSyncTrustedVaultClient()->AddTrustedRecoveryMethod(
kGaiaId, kTestRecoveryMethodPublicKey, kTestMethodTypeHint,
run_loop.QuitClosure());
run_loop.Run();
Expand Down Expand Up @@ -2220,7 +2220,7 @@ IN_PROC_BROWSER_TEST_F(
/*trusted_vault_keys=*/{trusted_vault_key}),
GetFakeServer());
ASSERT_TRUE(SetupClients());
GetTrustedVaultClient()->StoreKeys(
GetSyncTrustedVaultClient()->StoreKeys(
kGaiaId, GetSecurityDomainsServer()->GetAllTrustedVaultKeys(),
/*last_key_version=*/GetSecurityDomainsServer()->GetCurrentEpoch());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,10 @@ class EncryptionKeyApi
return;
}

trusted_vault_service_->GetTrustedVaultClient()->AddTrustedRecoveryMethod(
gaia_id, public_key, method_type_hint, std::move(callback));
trusted_vault_service_
->GetTrustedVaultClient(trusted_vault::SecurityDomainId::kChromeSync)
->AddTrustedRecoveryMethod(gaia_id, public_key, method_type_hint,
std::move(callback));
}

private:
Expand All @@ -130,7 +132,8 @@ class EncryptionKeyApi
const std::vector<chrome::mojom::TrustedVaultKeyPtr>& keys) {
CHECK(!keys.empty());
switch (security_domain) {
case trusted_vault::SecurityDomainId::kChromeSync: {
case trusted_vault::SecurityDomainId::kChromeSync:
case trusted_vault::SecurityDomainId::kPasskeys: {
base::UmaHistogramBoolean(
"Sync.TrustedVaultJavascriptSetEncryptionKeysIsIncognito",
trusted_vault_service_ == nullptr);
Expand All @@ -144,8 +147,14 @@ class EncryptionKeyApi
base::ranges::transform(keys, std::back_inserter(keys_as_bytes),
&chrome::mojom::TrustedVaultKey::bytes);
const int32_t version = keys.back()->version;
trusted_vault_service_->GetTrustedVaultClient()->StoreKeys(
gaia_id, keys_as_bytes, version);
trusted_vault::TrustedVaultClient* trusted_vault_client =
trusted_vault_service_->GetTrustedVaultClient(security_domain);
if (!trusted_vault_client) {
DLOG(ERROR) << "No TrustedVaultClient for security domain "
<< static_cast<int>(security_domain);
return;
}
trusted_vault_client->StoreKeys(gaia_id, keys_as_bytes, version);
}
}
}
Expand Down

0 comments on commit d510e09

Please sign in to comment.