Reapply "webauthn: IsUVPAA changes for the enclave."
This reverts commit 5add81e4db
.
I reverted the original due to test flakes on the builders. The issue is
that the Scoped*UnexportableKeyProviders set the override in their
default constructor, and reset it in their destructor. In order to be
able to clear the default override for a test, I made it optional. But
creating the std::optional involved:
1. Creating the object (sets the override).
2. Moving the object (default method does nothing).
3. Destructing the first object (clears the override).
Thus the override was never set during the tests, thus lots of flakes
because the test depended on the configuration of the bot.
Instead delete the other constructors to defang this trap and use a
unique_ptr instead.
PS1 is the clean revert and PS2 contains the fixes.
Bug: 350054971
Change-Id: Ic54ceee8b617018a6a9f596ffdb6bad2df83f131
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5669719
Auto-Submit: Adam Langley <agl@chromium.org>
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1322201}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
53f84c382b
commit
b8c8526d87
@ -444,7 +444,8 @@ void ChromeWebAuthenticationDelegate::
|
||||
IsUserVerifyingPlatformAuthenticatorAvailableOverride(
|
||||
content::RenderFrameHost* render_frame_host,
|
||||
base::OnceCallback<void(std::optional<bool>)> callback) {
|
||||
// If the testing API is active, its override takes precedence.
|
||||
const bool is_google =
|
||||
render_frame_host->GetLastCommittedOrigin().DomainIs("google.com");
|
||||
content::WebAuthenticationDelegate::
|
||||
IsUserVerifyingPlatformAuthenticatorAvailableOverride(
|
||||
render_frame_host,
|
||||
@ -452,12 +453,14 @@ void ChromeWebAuthenticationDelegate::
|
||||
[](base::WeakPtr<ChromeWebAuthenticationDelegate>
|
||||
web_authentication_delegate,
|
||||
base::WeakPtr<content::BrowserContext> browser_context,
|
||||
const bool is_google,
|
||||
base::OnceCallback<void(std::optional<bool>)> callback,
|
||||
std::optional<bool> testing_api_override) {
|
||||
if (!browser_context || !web_authentication_delegate) {
|
||||
return;
|
||||
}
|
||||
|
||||
// If the testing API is active, its override takes precedence.
|
||||
if (testing_api_override) {
|
||||
std::move(callback).Run(*testing_api_override);
|
||||
return;
|
||||
@ -473,9 +476,29 @@ void ChromeWebAuthenticationDelegate::
|
||||
return;
|
||||
}
|
||||
|
||||
// TODO(enclave): EnclaveAuthenticator availability should not
|
||||
// always imply a UVPA, and should just invoke the callback with
|
||||
// std::nullopt for now.
|
||||
// Without GPM PIN support the enclave authenticator depends on
|
||||
// having an Android LSKF enrolled to the security domain. But
|
||||
// we can't check that without querying the security domain
|
||||
// service, which we don't want to do just because a site
|
||||
// called IsUVPAA. Thus we conservatively ignore the
|
||||
// possibility of using the enclave authenticator here.
|
||||
if (!device::kWebAuthnGpmPin.Get()) {
|
||||
std::move(callback).Run(std::nullopt);
|
||||
return;
|
||||
}
|
||||
|
||||
// The enclave won't allow credentials for an account to be
|
||||
// stored in GPM _in_ that account. So we don't want to tell
|
||||
// accounts.google.com that there is a platform authenticator
|
||||
// if we're later going to reject a create() request for that
|
||||
// reason. Thus we have to be conservative with IsUVPAA
|
||||
// responses on google.com and ignore the possibility of using
|
||||
// the enclave authenticator.
|
||||
if (is_google) {
|
||||
std::move(callback).Run(std::nullopt);
|
||||
return;
|
||||
}
|
||||
|
||||
web_authentication_delegate->BrowserProvidedPasskeysAvailable(
|
||||
browser_context.get(),
|
||||
base::BindOnce(
|
||||
@ -491,7 +514,7 @@ void ChromeWebAuthenticationDelegate::
|
||||
std::move(callback)));
|
||||
},
|
||||
weak_ptr_factory_.GetWeakPtr(),
|
||||
render_frame_host->GetBrowserContext()->GetWeakPtr(),
|
||||
render_frame_host->GetBrowserContext()->GetWeakPtr(), is_google,
|
||||
std::move(callback)));
|
||||
}
|
||||
|
||||
|
@ -150,6 +150,12 @@ constexpr uint8_t kTestProtobuf[] = {
|
||||
0x70, 0x33, 0xF7, 0x80, 0x75, 0x1D, 0x22, 0x13, 0x37, 0xCD, 0x1F, 0x24,
|
||||
0x40, 0xDA, 0x70, 0xA1, 0x03};
|
||||
|
||||
static constexpr char kIsUVPAA[] = R"((() => {
|
||||
window.PublicKeyCredential.isUserVerifyingPlatformAuthenticatorAvailable().
|
||||
then(result => window.domAutomationController.send('IsUVPAA: ' + result),
|
||||
error => window.domAutomationController.send('error ' + error));
|
||||
})())";
|
||||
|
||||
static constexpr char kMakeCredentialUvDiscouraged[] = R"((() => {
|
||||
return navigator.credentials.create({ publicKey: {
|
||||
rp: { name: "www.example.com" },
|
||||
@ -583,7 +589,9 @@ class EnclaveAuthenticatorBrowserTest : public SyncTest {
|
||||
#if BUILDFLAG(IS_WIN)
|
||||
webauthn_dll_override_(&fake_webauthn_dll_),
|
||||
#endif
|
||||
recovery_key_store_(FakeRecoveryKeyStore::New()) {
|
||||
recovery_key_store_(FakeRecoveryKeyStore::New()),
|
||||
mock_hw_provider_(
|
||||
std::make_unique<crypto::ScopedMockUnexportableKeyProvider>()) {
|
||||
#if BUILDFLAG(IS_WIN)
|
||||
// Make webauthn.dll unavailable to ensure a consistent test environment on
|
||||
// Windows. Otherwise the version of webauthn.dll can differ between
|
||||
@ -824,6 +832,22 @@ class EnclaveAuthenticatorBrowserTest : public SyncTest {
|
||||
fake_uv_provider_.emplace<crypto::ScopedFakeUserVerifyingKeyProvider>();
|
||||
}
|
||||
|
||||
bool IsUVPAA() {
|
||||
content::WebContents* web_contents =
|
||||
browser()->tab_strip_model()->GetActiveWebContents();
|
||||
content::DOMMessageQueue message_queue(web_contents);
|
||||
content::ExecuteScriptAsync(web_contents, kIsUVPAA);
|
||||
|
||||
std::string script_result;
|
||||
CHECK(message_queue.WaitForMessage(&script_result));
|
||||
if (script_result == "\"IsUVPAA: true\"") {
|
||||
return true;
|
||||
} else if (script_result == "\"IsUVPAA: false\"") {
|
||||
return false;
|
||||
}
|
||||
NOTREACHED_NORETURN() << "unexpected IsUVPAA result: " << script_result;
|
||||
}
|
||||
|
||||
protected:
|
||||
base::SimpleTestClock clock_;
|
||||
net::EmbeddedTestServer https_server_{net::EmbeddedTestServer::TYPE_HTTPS};
|
||||
@ -843,7 +867,7 @@ class EnclaveAuthenticatorBrowserTest : public SyncTest {
|
||||
biometrics_override_;
|
||||
#endif
|
||||
std::unique_ptr<FakeRecoveryKeyStore> recovery_key_store_;
|
||||
crypto::ScopedMockUnexportableKeyProvider mock_hw_provider_;
|
||||
std::unique_ptr<crypto::ScopedMockUnexportableKeyProvider> mock_hw_provider_;
|
||||
network::TestURLLoaderFactory url_loader_factory_;
|
||||
std::unique_ptr<DelegateObserver> delegate_observer_;
|
||||
std::unique_ptr<ModelObserver> model_observer_;
|
||||
@ -2889,6 +2913,44 @@ IN_PROC_BROWSER_TEST_F(EnclaveAuthenticatorWithoutPinBrowserTest,
|
||||
[](const auto& m) { return IsMechanismEnclaveCredential(m); }));
|
||||
}
|
||||
|
||||
#if BUILDFLAG(IS_LINUX)
|
||||
// These tests are run on Linux because Linux has no platform authenticator
|
||||
// that can effect whether IsUVPAA returns true or not.
|
||||
|
||||
IN_PROC_BROWSER_TEST_F(EnclaveAuthenticatorWithoutPinBrowserTest, IsUVPAA) {
|
||||
// We don't know, at IsUVPAA time, whether there's an Android LSKF on the
|
||||
// account and, without GPM PIN support, that means that we have to assume
|
||||
// that the enclave authenticator isn't available.
|
||||
EXPECT_FALSE(IsUVPAA());
|
||||
}
|
||||
|
||||
IN_PROC_BROWSER_TEST_F(EnclaveAuthenticatorWithPinBrowserTest, IsUVPAA) {
|
||||
// With the enclave authenticator in place, IsUVPAA should return true.
|
||||
EXPECT_TRUE(IsUVPAA());
|
||||
}
|
||||
|
||||
IN_PROC_BROWSER_TEST_F(EnclaveAuthenticatorWithPinBrowserTest,
|
||||
IsUVPAA_GoogleSite) {
|
||||
// With the enclave authenticator in place, IsUVPAA should return false for
|
||||
// google.com sites because we won't create a credential for an account in
|
||||
// that same account. But since we don't know the user.id value at IsUVPAA
|
||||
// time, the result has to be conservative.
|
||||
ASSERT_TRUE(ui_test_utils::NavigateToURL(
|
||||
browser(), https_server_.GetURL("accounts.google.com", "/title1.html")));
|
||||
EXPECT_FALSE(IsUVPAA());
|
||||
}
|
||||
|
||||
IN_PROC_BROWSER_TEST_F(EnclaveAuthenticatorWithPinBrowserTest,
|
||||
IsUVPAA_NoUnexportableKeys) {
|
||||
// Without support for unexportable keys, IsUVPAA should return false because
|
||||
// the enclave cannot be used.
|
||||
mock_hw_provider_.reset();
|
||||
crypto::ScopedNullUnexportableKeyProvider no_hw_key_support;
|
||||
EXPECT_FALSE(IsUVPAA());
|
||||
}
|
||||
|
||||
#endif // IS_LINUX
|
||||
|
||||
} // namespace
|
||||
|
||||
#endif // !defined(MEMORY_SANITIZER)
|
||||
|
@ -2,9 +2,10 @@
|
||||
// Use of this source code is governed by a BSD-style license that can be
|
||||
// found in the LICENSE file.
|
||||
|
||||
#include "crypto/scoped_mock_unexportable_key_provider.h"
|
||||
|
||||
#include <vector>
|
||||
|
||||
#include "crypto/scoped_mock_unexportable_key_provider.h"
|
||||
#include "crypto/unexportable_key.h"
|
||||
|
||||
namespace crypto {
|
||||
|
@ -15,6 +15,10 @@ namespace crypto {
|
||||
class ScopedMockUnexportableKeyProvider {
|
||||
public:
|
||||
ScopedMockUnexportableKeyProvider();
|
||||
ScopedMockUnexportableKeyProvider(const ScopedMockUnexportableKeyProvider&) =
|
||||
delete;
|
||||
ScopedMockUnexportableKeyProvider(ScopedMockUnexportableKeyProvider&&) =
|
||||
delete;
|
||||
~ScopedMockUnexportableKeyProvider();
|
||||
};
|
||||
|
||||
@ -23,6 +27,10 @@ class ScopedMockUnexportableKeyProvider {
|
||||
class ScopedNullUnexportableKeyProvider {
|
||||
public:
|
||||
ScopedNullUnexportableKeyProvider();
|
||||
ScopedNullUnexportableKeyProvider(const ScopedNullUnexportableKeyProvider&) =
|
||||
delete;
|
||||
ScopedNullUnexportableKeyProvider(ScopedNullUnexportableKeyProvider&&) =
|
||||
delete;
|
||||
~ScopedNullUnexportableKeyProvider();
|
||||
};
|
||||
|
||||
|
Reference in New Issue
Block a user