Skip to content

Commit

Permalink
Revert "[DTC] Create 'Happy Path' browser test with non-mock key mana…
Browse files Browse the repository at this point in the history 
…gement"

This reverts commit 6ecf8b3.

Reason for revert: suspect causing browser_tests failure on Win10 Tests x64 bot.

The failing test:
All/DeviceTrustBrowserTest.AttestationKeyCreation/3

First build failure:
https://ci.chromium.org/ui/p/chromium/builders/ci/Win10%20Tests%20x64/73998/test-results

Sample log:
---
Received fatal exception EXCEPTION_ACCESS_VIOLATION
Backtrace:
	base::`anonymous namespace'::PostTaskAndReplyTaskRunner::PostTask [0x00007FF609CA1BC4+52] (o:\base\task\task_runner.cc:41)
	base::internal::PostTaskAndReplyImpl::PostTaskAndReply [0x00007FF60B68E34E+350] (o:\base\threading\post_task_and_reply_impl.cc:140)
	base::TaskRunner::PostTaskAndReply [0x00007FF609CA1AC7+135] (o:\base\task\task_runner.cc:53)
	base::TaskRunner::PostTaskAndReplyWithResult<printing::mojom::ResultCode,printing::mojom::ResultCode,base::OnceCallback,base::OnceCallback,void,void> [0x00007FF6065C0BA7+221] (o:\base\task\task_runner.h:157)
	enterprise_connectors::WinKeyRotationCommand::Trigger [0x00007FF6096C9EAE+190] (o:\chrome\browser\enterprise\connectors\device_trust\key_management\browser\commands\win_key_rotation_command.cc:223)
---

Original change's description:
> [DTC] Create 'Happy Path' browser test with non-mock key management
>
> Create a new browser test for device trust full attestation flow which
> uses actual implementation for key management, instead of mocking
> them out.
>
> This first test case is for the success case where there is not key
> on the machine.
>
> Bug: b:256843502
> Change-Id: I40a5e235a57bbfdbf8a90fdeb5b4c8b6cb7f40ca
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4004448
> Commit-Queue: Zonghan Xu <xzonghan@chromium.org>
> Reviewed-by: Sebastien Lalancette <seblalancette@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1075662}

Bug: b:256843502
Change-Id: I680a9a1478cee60b9ab7beb654b70185380f2f40
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4056560
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Takashi Sakamoto <tasak@google.com>
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Cr-Commit-Position: refs/heads/main@{#1075707}
  • Loading branch information
@tasak
tasak authored and Chromium LUCI CQ committed Nov 25, 2022
1 parent a1f1d42 commit 3b00d21
Show file tree
Hide file tree
Showing 10 changed files with 116 additions and 408 deletions.
165 changes: 63 additions & 102 deletions chrome/browser/enterprise/connectors/device_trust/device_trust_browsertest.cc
View file
Expand Up @@ -49,10 +49,6 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

#if BUILDFLAG(IS_WIN)
#include "chrome/browser/enterprise/connectors/device_trust/test/device_trust_test_environment_win.h"
#endif // #if BUILDFLAG(IS_WIN)

#if BUILDFLAG(IS_CHROMEOS_ASH)
#include "chrome/browser/ash/attestation/mock_tpm_challenge_key.h"
#include "chrome/browser/ash/attestation/tpm_challenge_key.h"
Expand Down Expand Up @@ -265,71 +261,6 @@ class DeviceTrustBrowserTestBase
return active_browser->profile()->GetPrefs();
}

void VerifyAttestationFlowOutcome() {
if (!is_enabled()) {
// If the feature flag is disabled, the attestation flow should not have
// been triggered (and that is the end of the test);
EXPECT_FALSE(initial_attestation_request_);
EXPECT_FALSE(challenge_response_request_);

histogram_tester_.ExpectTotalCount(kFunnelHistogramName, 0);
histogram_tester_.ExpectTotalCount(kResultHistogramName, 0);
histogram_tester_.ExpectTotalCount(kLatencySuccessHistogramName, 0);
histogram_tester_.ExpectTotalCount(kLatencyFailureHistogramName, 0);
return;
}

// Attestation flow should be fully done.
EXPECT_TRUE(initial_attestation_request_);

// Validate that the two requests contain expected information. URLs' paths
// have to be used for comparison due to how the HostResolver is replacing
// domains with '127.0.0.1' in tests.
EXPECT_EQ(initial_attestation_request_->GetURL().path(),
GetRedirectUrl().path());
EXPECT_EQ(
initial_attestation_request_->headers.find(kDeviceTrustHeader)->second,
kDeviceTrustHeaderValue);

// Response header should always be set, even in error cases (i.e.
// use_v2_header is false).
EXPECT_TRUE(challenge_response_request_.has_value());

ExpectFunnelStep(DTAttestationFunnelStep::kAttestationFlowStarted);
ExpectFunnelStep(DTAttestationFunnelStep::kChallengeReceived);

EXPECT_EQ(challenge_response_request_->GetURL().path(),
GetRedirectLocationUrl().path());
const std::string& challenge_response =
challenge_response_request_->headers
.find(kVerifiedAccessResponseHeader)
->second;

if (use_v2_header()) {
// TODO(crbug.com/1241857): Add challenge-response validation.
EXPECT_TRUE(!challenge_response.empty());

ExpectFunnelStep(DTAttestationFunnelStep::kSignalsCollected);
ExpectFunnelStep(DTAttestationFunnelStep::kChallengeResponseSent);
histogram_tester_.ExpectUniqueSample(kResultHistogramName,
DTAttestationResult::kSuccess, 1);
histogram_tester_.ExpectTotalCount(kLatencySuccessHistogramName, 1);
histogram_tester_.ExpectTotalCount(kLatencyFailureHistogramName, 0);
} else {
static constexpr char kFailedToParseChallengeJsonResponse[] =
"{\"error\":\"failed_to_parse_challenge\"}";
EXPECT_EQ(challenge_response, kFailedToParseChallengeJsonResponse);
histogram_tester_.ExpectBucketCount(
kFunnelHistogramName, DTAttestationFunnelStep::kSignalsCollected, 0);
histogram_tester_.ExpectBucketCount(
kFunnelHistogramName, DTAttestationFunnelStep::kChallengeResponseSent,
0);
histogram_tester_.ExpectTotalCount(kResultHistogramName, 0);
histogram_tester_.ExpectTotalCount(kLatencySuccessHistogramName, 0);
histogram_tester_.ExpectTotalCount(kLatencyFailureHistogramName, 1);
}
}

net::test_server::EmbeddedTestServerHandle test_server_handle_;
base::test::ScopedFeatureList scoped_feature_list_;
base::HistogramTester histogram_tester_;
Expand Down Expand Up @@ -421,12 +352,8 @@ class DeviceTrustDesktopBrowserTest : public DeviceTrustBrowserTestBase {
void SetUpOnMainThread() override {
DeviceTrustBrowserTestBase::SetUpOnMainThread();

#if BUILDFLAG(IS_WIN)
device_trust_test_environment_win_.emplace();
#else // BUILDFLAG(IS_WIN)
scoped_persistence_delegate_factory_.emplace();
scoped_rotation_command_factory_.emplace();
#endif

safe_browsing::SetProfileDMToken(browser()->profile(), "dm_token");

Expand Down Expand Up @@ -455,38 +382,88 @@ class DeviceTrustDesktopBrowserTest : public DeviceTrustBrowserTestBase {
}
#endif

#if BUILDFLAG(IS_WIN)
absl::optional<DeviceTrustTestEnvironmentWin>
device_trust_test_environment_win_;
#else // BUILDFLAG(IS_WIN)
absl::optional<test::ScopedKeyPersistenceDelegateFactory>
scoped_persistence_delegate_factory_;
absl::optional<ScopedKeyRotationCommandFactory>
scoped_rotation_command_factory_;
#endif
};

using DeviceTrustBrowserTest = DeviceTrustDesktopBrowserTest;
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

// Tests that the whole attestation flow occurs when navigating to an
// allowed domain.
IN_PROC_BROWSER_TEST_P(DeviceTrustBrowserTest, AttestationFullFlowKeyExists) {
IN_PROC_BROWSER_TEST_P(DeviceTrustBrowserTest, AttestationFullFlow) {
GURL redirect_url = GetRedirectUrl();
TestNavigationManager first_navigation(web_contents(), redirect_url);

#if BUILDFLAG(IS_WIN)
// Windows DT test environment mocks the register and we need to manually
// create the DT key first
device_trust_test_environment_win_->SetUpExistingKey();
#endif

// Add allowed domain to Prefs and trigger a navigation to it.
SetPolicy();
NavigateToUrl(redirect_url);

first_navigation.WaitForNavigationFinished();
VerifyAttestationFlowOutcome();

if (!is_enabled()) {
// If the feature flag is disabled, the attestation flow should not have
// been triggered (and that is the end of the test);
EXPECT_FALSE(initial_attestation_request_);
EXPECT_FALSE(challenge_response_request_);

histogram_tester_.ExpectTotalCount(kFunnelHistogramName, 0);
histogram_tester_.ExpectTotalCount(kResultHistogramName, 0);
histogram_tester_.ExpectTotalCount(kLatencySuccessHistogramName, 0);
histogram_tester_.ExpectTotalCount(kLatencyFailureHistogramName, 0);
return;
}

// Attestation flow should be fully done.
EXPECT_TRUE(initial_attestation_request_);

// Validate that the two requests contain expected information. URLs' paths
// have to be used for comparison due to how the HostResolver is replacing
// domains with '127.0.0.1' in tests.
EXPECT_EQ(initial_attestation_request_->GetURL().path(),
GetRedirectUrl().path());
EXPECT_EQ(
initial_attestation_request_->headers.find(kDeviceTrustHeader)->second,
kDeviceTrustHeaderValue);

// Response header should always be set, even in error cases (i.e.
// use_v2_header is false).
EXPECT_TRUE(challenge_response_request_.has_value());

ExpectFunnelStep(DTAttestationFunnelStep::kAttestationFlowStarted);
ExpectFunnelStep(DTAttestationFunnelStep::kChallengeReceived);

EXPECT_EQ(challenge_response_request_->GetURL().path(),
GetRedirectLocationUrl().path());
const std::string& challenge_response =
challenge_response_request_->headers.find(kVerifiedAccessResponseHeader)
->second;

if (use_v2_header()) {
// TODO(crbug.com/1241857): Add challenge-response validation.
EXPECT_TRUE(!challenge_response.empty());

ExpectFunnelStep(DTAttestationFunnelStep::kSignalsCollected);
ExpectFunnelStep(DTAttestationFunnelStep::kChallengeResponseSent);
histogram_tester_.ExpectUniqueSample(kResultHistogramName,
DTAttestationResult::kSuccess, 1);
histogram_tester_.ExpectTotalCount(kLatencySuccessHistogramName, 1);
histogram_tester_.ExpectTotalCount(kLatencyFailureHistogramName, 0);
} else {
static constexpr char kFailedToParseChallengeJsonResponse[] =
"{\"error\":\"failed_to_parse_challenge\"}";
EXPECT_EQ(challenge_response, kFailedToParseChallengeJsonResponse);
histogram_tester_.ExpectBucketCount(
kFunnelHistogramName, DTAttestationFunnelStep::kSignalsCollected, 0);
histogram_tester_.ExpectBucketCount(
kFunnelHistogramName, DTAttestationFunnelStep::kChallengeResponseSent,
0);
histogram_tester_.ExpectTotalCount(kResultHistogramName, 0);
histogram_tester_.ExpectTotalCount(kLatencySuccessHistogramName, 0);
histogram_tester_.ExpectTotalCount(kLatencyFailureHistogramName, 1);
}
}

// Tests that the attestation flow does not get triggered when navigating to a
Expand Down Expand Up @@ -633,22 +610,6 @@ IN_PROC_BROWSER_TEST_P(DeviceTrustBrowserTest, SignalsContract) {
}
}

#if BUILDFLAG(IS_WIN)
// Windows DT test environment mocks the registry and DT key does not exist by
// default In this test case a key will be created by DeviceTrustKeyManager
IN_PROC_BROWSER_TEST_P(DeviceTrustBrowserTest, AttestationKeyCreation) {
GURL redirect_url = GetRedirectUrl();
TestNavigationManager first_navigation(web_contents(), redirect_url);

// Add allowed domain to Prefs and trigger a navigation to it.
SetPolicy();
NavigateToUrl(redirect_url);

first_navigation.WaitForNavigationFinished();
VerifyAttestationFlowOutcome();
}
#endif

INSTANTIATE_TEST_SUITE_P(All,
DeviceTrustBrowserTest,
testing::Combine(testing::Bool(), testing::Bool()));
Expand Down
7 changes: 1 addition & 6 deletions chrome/browser/enterprise/connectors/device_trust/key_management/browser/commands/BUILD.gn
View file
Expand Up @@ -3,11 +3,7 @@
# found in the LICENSE file.

source_set("commands") {
friend = [
"//chrome/browser/enterprise/connectors/device_trust/test:test_support",
":test_support",
":unit_tests",
]
friend = [ ":unit_tests" ]

public = [
"key_rotation_command.h",
Expand Down Expand Up @@ -100,7 +96,6 @@ source_set("unit_tests") {
"//base",
"//base/test:test_support",
"//chrome/browser/enterprise/connectors/device_trust/common",
"//chrome/browser/enterprise/connectors/device_trust/test:test_support",
"//testing/gtest",
]

Expand Down
31 changes: 13 additions & 18 deletions ...prise/connectors/device_trust/key_management/browser/commands/win_key_rotation_command.cc
View file
Expand Up @@ -146,32 +146,27 @@ HRESULT RunGoogleUpdateElevatedCommand(const wchar_t* command,

} // namespace

WinKeyRotationCommand::WinKeyRotationCommand()
: WinKeyRotationCommand(
base::BindRepeating(&RunGoogleUpdateElevatedCommand)) {}
WinKeyRotationCommand::WinKeyRotationCommand() = default;

WinKeyRotationCommand::WinKeyRotationCommand(
RunGoogleUpdateElevatedCommandFn run_elevated_command)
: WinKeyRotationCommand(
run_elevated_command,
base::ThreadPool::CreateCOMSTATaskRunner(
{base::TaskPriority::USER_BLOCKING, base::MayBlock()})) {}

WinKeyRotationCommand::WinKeyRotationCommand(
RunGoogleUpdateElevatedCommandFn run_elevated_command,
scoped_refptr<base::SingleThreadTaskRunner> com_thread_runner)
: com_thread_runner_(com_thread_runner),
run_elevated_command_(run_elevated_command) {
DCHECK(run_elevated_command_);
DCHECK(com_thread_runner_);
}
: run_elevated_command_(run_elevated_command) {}

WinKeyRotationCommand::~WinKeyRotationCommand() = default;

void WinKeyRotationCommand::Trigger(const KeyRotationCommand::Params& params,
Callback callback) {
DCHECK(!callback.is_null());

if (!com_thread_runner_) {
com_thread_runner_ = base::ThreadPool::CreateCOMSTATaskRunner(
{base::TaskPriority::USER_BLOCKING, base::MayBlock()});
}

RunGoogleUpdateElevatedCommandFn run_elevated_command =
run_elevated_command_ ? run_elevated_command_
: &RunGoogleUpdateElevatedCommand;

com_thread_runner_->PostTaskAndReplyWithResult(
FROM_HERE,
base::BindOnce(
Expand All @@ -190,7 +185,7 @@ void WinKeyRotationCommand::Trigger(const KeyRotationCommand::Params& params,
// and sleep time are pretty arbitrary choices.
HRESULT hr = S_OK;
for (int i = 0; i < 10; ++i) {
hr = run_elevated_command.Run(
hr = run_elevated_command(
installer::kCmdRotateDeviceTrustKey,
{token_base64, params.dm_server_url, nonce_base64},
&return_code);
Expand Down Expand Up @@ -218,7 +213,7 @@ void WinKeyRotationCommand::Trigger(const KeyRotationCommand::Params& params,
}
return status;
},
params, run_elevated_command_, waiting_enabled_),
params, run_elevated_command, waiting_enabled_),
std::move(callback));
}

Expand Down
11 changes: 4 additions & 7 deletions ...rprise/connectors/device_trust/key_management/browser/commands/win_key_rotation_command.h
View file
Expand Up @@ -23,18 +23,15 @@ class WinKeyRotationCommand : public KeyRotationCommand {
static const HRESULT GOOPDATE_E_APP_USING_EXTERNAL_UPDATER = 0xA043081D;

using RunGoogleUpdateElevatedCommandFn =
base::RepeatingCallback<HRESULT(const wchar_t* command,
const std::vector<std::string>& args,
DWORD* return_code)>;
HRESULT (*)(const wchar_t* command,
const std::vector<std::string>& args,
DWORD* return_code);

// The second constructor is used in tests to override the behaviour of
// Google Update.
WinKeyRotationCommand();
explicit WinKeyRotationCommand(
RunGoogleUpdateElevatedCommandFn run_elevated_command);
WinKeyRotationCommand(
RunGoogleUpdateElevatedCommandFn run_elevated_command,
scoped_refptr<base::SingleThreadTaskRunner> com_thread_runner);
~WinKeyRotationCommand() override;

// KeyRotationCommand:
Expand All @@ -46,7 +43,7 @@ class WinKeyRotationCommand : public KeyRotationCommand {
private:
scoped_refptr<base::SingleThreadTaskRunner> com_thread_runner_;
bool waiting_enabled_ = true;
RunGoogleUpdateElevatedCommandFn run_elevated_command_;
RunGoogleUpdateElevatedCommandFn run_elevated_command_ = nullptr;
};

} // namespace enterprise_connectors
Expand Down

0 comments on commit 3b00d21

Please sign in to comment.