Skip to content

Commit

Permalink
Cleanup remote attestation policies in TpmChallengeKeySubtle
Browse files Browse the repository at this point in the history 
Remove checks for the policies AttestationEnabledForDevice and AttestationEnabledForUser in TpmChallengeKeySubtle as these can no longer be set by admins (launch/227712). Remove test cases which verify this behavior as well.
Add TODOs to cleanup usage/notion of these policies in other parts.

Bug: b:277706611, b:285556135
Change-Id: I8e0271a362ba5067309d841b3327d42084efac23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4561753
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Commit-Queue: Leon Masopust <lmasopust@google.com>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1153228}
  • Loading branch information
Leon Masopust authored and Chromium LUCI CQ committed Jun 5, 2023
1 parent 088686e commit 6005431
Show file tree
Hide file tree
Showing 13 changed files with 30 additions and 182 deletions.
1 change: 1 addition & 0 deletions chrome/browser/ash/attestation/attestation_ca_client.cc
View file
Expand Up @@ -180,6 +180,7 @@ void AttestationCAClient::OnURLLoadComplete(
void AttestationCAClient::FetchURL(const std::string& url,
const std::string& request,
DataCallback on_response) {
// TODO(b/285556135): Remove mention of DeviceAttestationEnabled
const net::NetworkTrafficAnnotationTag traffic_annotation =
net::DefineNetworkTrafficAnnotation("attestation_ca_client", R"(
semantics {
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/ash/attestation/attestation_policy_observer.h
View file
Expand Up @@ -15,6 +15,8 @@ namespace attestation {

class MachineCertificateUploader;

// TODO(b/285556135): Replace this observer with another trigger which starts
// the certificate upload as soon as device policies are available.
// A class which observes policy changes and uploads a certificate if necessary.
class AttestationPolicyObserver {
public:
Expand Down
95 changes: 15 additions & 80 deletions chrome/browser/ash/attestation/tpm_challenge_key_subtle.cc
View file
Expand Up @@ -101,9 +101,12 @@ bool IsEnterpriseDevice() {
return InstallAttributes::Get()->IsEnterpriseManaged();
}

// For personal devices, we don't need to check if remote attestation is
// enabled in the device, but we need to ask for user consent if the key
// does not exist.
// For unmanaged devices we need to ask for user consent if the key does not
// exist because data will be sent to the PCA.
// Historical note: For managed device there used to be policies to control this
// (AttestationEnabledForUser,AttestationEnabledForDevice) but they were removed
// from the client after having been set to true unconditionally for all clients
// for a long time.
bool IsUserConsentRequired() {
return !IsEnterpriseDevice();
}
Expand Down Expand Up @@ -254,10 +257,13 @@ void TpmChallengeKeySubtleImpl::PrepareMachineKey() {
return;
}

// Check if remote attestation is enabled in the device policy.
GetDeviceAttestationEnabled(base::BindRepeating(
&TpmChallengeKeySubtleImpl::GetDeviceAttestationEnabledCallback,
weak_factory_.GetWeakPtr()));
// Wait for the machine certificate to be uploaded.
if (machine_certificate_uploader_) {
machine_certificate_uploader_->WaitForUploadComplete(base::BindOnce(
&TpmChallengeKeySubtleImpl::PrepareKey, weak_factory_.GetWeakPtr()));
} else {
PrepareKey(true);
}
}

void TpmChallengeKeySubtleImpl::PrepareUserKey() {
Expand All @@ -270,26 +276,15 @@ void TpmChallengeKeySubtleImpl::PrepareUserKey() {
return;
}

if (!IsRemoteAttestationEnabledForUser()) {
std::move(callback_).Run(
Result::MakeError(ResultCode::kUserPolicyDisabledError));
return;
}

if (IsEnterpriseDevice()) {
if (!IsUserAffiliated()) {
std::move(callback_).Run(
Result::MakeError(ResultCode::kUserNotManagedError));
return;
}

// Check if remote attestation is enabled in the device policy.
GetDeviceAttestationEnabled(base::BindRepeating(
&TpmChallengeKeySubtleImpl::GetDeviceAttestationEnabledCallback,
weak_factory_.GetWeakPtr()));
} else {
GetDeviceAttestationEnabledCallback(true);
}

PrepareKey(true);
}

bool TpmChallengeKeySubtleImpl::IsUserAffiliated() const {
Expand All @@ -302,17 +297,6 @@ bool TpmChallengeKeySubtleImpl::IsUserAffiliated() const {
return false;
}

bool TpmChallengeKeySubtleImpl::IsRemoteAttestationEnabledForUser() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(profile_);

PrefService* prefs = profile_->GetPrefs();
if (prefs && prefs->IsManagedPreference(prefs::kAttestationEnabled)) {
return prefs->GetBoolean(prefs::kAttestationEnabled);
}
return false;
}

std::string TpmChallengeKeySubtleImpl::GetEmail() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

Expand Down Expand Up @@ -378,55 +362,6 @@ std::string TpmChallengeKeySubtleImpl::GetUsernameForAttestationClient() const {
return std::string();
}

void TpmChallengeKeySubtleImpl::GetDeviceAttestationEnabled(
const base::RepeatingCallback<void(bool)>& callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

CrosSettings* settings = CrosSettings::Get();
CrosSettingsProvider::TrustedStatus status = settings->PrepareTrustedValues(
base::BindOnce(&TpmChallengeKeySubtleImpl::GetDeviceAttestationEnabled,
weak_factory_.GetWeakPtr(), callback));

bool value = false;
switch (status) {
case CrosSettingsProvider::TRUSTED:
if (!settings->GetBoolean(kDeviceAttestationEnabled, &value)) {
value = false;
}
break;
case CrosSettingsProvider::TEMPORARILY_UNTRUSTED:
// Do nothing. This function will be called again when the values are
// ready.
return;
case CrosSettingsProvider::PERMANENTLY_UNTRUSTED:
// If the value cannot be trusted, we assume that the device attestation
// is false to be on the safe side.
break;
}

callback.Run(value);
}

void TpmChallengeKeySubtleImpl::GetDeviceAttestationEnabledCallback(
bool enabled) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

if (!enabled) {
std::move(callback_).Run(
Result::MakeError(ResultCode::kDevicePolicyDisabledError));
return;
}

// Only the device challenge depends on the certificate to be uploaded.
if ((key_type_ == AttestationKeyType::KEY_DEVICE) &&
machine_certificate_uploader_) {
machine_certificate_uploader_->WaitForUploadComplete(base::BindOnce(
&TpmChallengeKeySubtleImpl::PrepareKey, weak_factory_.GetWeakPtr()));
} else {
PrepareKey(true);
}
}

void TpmChallengeKeySubtleImpl::PrepareKey(bool can_continue) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

Expand Down
8 changes: 0 additions & 8 deletions chrome/browser/ash/attestation/tpm_challenge_key_subtle.h
View file
Expand Up @@ -173,8 +173,6 @@ class TpmChallengeKeySubtleImpl final : public TpmChallengeKeySubtle {
// If this is a device-wide instance without a user-associated |profile_|,
// returns false.
bool IsUserAffiliated() const;
// Returns true if remote attestation is allowed and the setting is managed.
bool IsRemoteAttestationEnabledForUser() const;

// Returns the user email (for user key) or an empty string (for machine key).
std::string GetEmail() const;
Expand Down Expand Up @@ -205,12 +203,6 @@ class TpmChallengeKeySubtleImpl final : public TpmChallengeKeySubtle {
const ::attestation::RegisterKeyWithChapsTokenReply& reply);
void MarkCorporateKeyCallback(chromeos::platform_keys::Status status);

// Returns a trusted value from CrosSettings indicating if the device
// attestation is enabled.
void GetDeviceAttestationEnabled(
const base::RepeatingCallback<void(bool)>& callback);
void GetDeviceAttestationEnabledCallback(bool enabled);

void GetEnrollmentPreparationsCallback(
const ::attestation::GetEnrollmentPreparationsReply& reply);
void PrepareKeyErrorHandlerCallback(
Expand Down
38 changes: 0 additions & 38 deletions chrome/browser/ash/attestation/tpm_challenge_key_subtle_unittest.cc
View file
Expand Up @@ -272,16 +272,11 @@ void TpmChallengeKeySubtleTestBase::SetUp() {
break;
case TestProfileChoice::kAffiliatedProfile:
testing_profile_ = CreateUserProfile(/*is_affiliated=*/true);
testing_profile_->GetTestingPrefService()->SetManagedPref(
prefs::kAttestationEnabled, std::make_unique<base::Value>(true));
break;
}

GetInstallAttributes()->SetCloudManaged("google.com", "device_id");

GetCrosSettingsHelper()->ReplaceDeviceSettingsProviderWithStub();
GetCrosSettingsHelper()->SetBoolean(kDeviceAttestationEnabled, true);

system_token_key_permissions_manager_ =
std::make_unique<platform_keys::MockKeyPermissionsManager>();
platform_keys::KeyPermissionsManagerImpl::
Expand Down Expand Up @@ -457,16 +452,6 @@ TEST_P(DeviceKeysAccessTpmChallengeKeySubtleTest,
TpmChallengeKeyResultCode::kNonEnterpriseDeviceError));
}

TEST_P(DeviceKeysAccessTpmChallengeKeySubtleTest,
DeviceKeyDeviceAttestationDisabled) {
GetCrosSettingsHelper()->SetBoolean(kDeviceAttestationEnabled, false);

RunOneStepAndExpect(
KEY_DEVICE, /*will_register_key=*/false, kEmptyKeyName,
TpmChallengeKeyResult::MakeError(
TpmChallengeKeyResultCode::kDevicePolicyDisabledError));
}

TEST_F(UnaffiliatedUserTpmChallengeKeySubtleTest, DeviceKeyUserNotManaged) {
RunOneStepAndExpect(KEY_DEVICE,
/*will_register_key=*/false, kEmptyKeyName,
Expand All @@ -481,37 +466,14 @@ TEST_F(SigninProfileTpmChallengeKeySubtleTest, UserKeyUserKeyNotAvailable) {
TpmChallengeKeyResultCode::kUserKeyNotAvailableError));
}

TEST_F(AffiliatedUserTpmChallengeKeySubtleTest, UserKeyUserPolicyDisabled) {
GetProfile()->GetTestingPrefService()->SetManagedPref(
prefs::kAttestationEnabled, std::make_unique<base::Value>(false));

RunOneStepAndExpect(KEY_USER,
/*will_register_key=*/false, kEmptyKeyName,
TpmChallengeKeyResult::MakeError(
TpmChallengeKeyResultCode::kUserPolicyDisabledError));
}

// Checks that a user should be affiliated with a device
TEST_F(UnaffiliatedUserTpmChallengeKeySubtleTest, UserKeyUserNotAffiliated) {
GetProfile()->GetTestingPrefService()->SetManagedPref(
prefs::kAttestationEnabled, std::make_unique<base::Value>(true));

RunOneStepAndExpect(KEY_USER,
/*will_register_key=*/false, kEmptyKeyName,
TpmChallengeKeyResult::MakeError(
TpmChallengeKeyResultCode::kUserNotManagedError));
}

TEST_F(AffiliatedUserTpmChallengeKeySubtleTest,
UserKeyDeviceAttestationDisabled) {
GetCrosSettingsHelper()->SetBoolean(kDeviceAttestationEnabled, false);

RunOneStepAndExpect(
KEY_USER, /*will_register_key=*/false, kEmptyKeyName,
TpmChallengeKeyResult::MakeError(
TpmChallengeKeyResultCode::kDevicePolicyDisabledError));
}

TEST_P(DeviceKeysAccessTpmChallengeKeySubtleTest, DoesKeyExistDbusFailed) {
AttestationClient::Get()
->GetTestInterface()
Expand Down
46 changes: 0 additions & 46 deletions chrome/browser/ash/login/saml/saml_browsertest.cc
View file
Expand Up @@ -2013,26 +2013,9 @@ IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationTest,
ASSERT_FALSE(fake_saml_idp()->IsLastChallengeResponseExists());
}

// Verify that device attestation is not available when device attestation is
// not enabled.
IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest,
DeviceAttestationNotEnabledError) {
SetAllowedUrlsPolicy({fake_saml_idp()->GetIdpHost()});

StartSamlAndWaitForIdpPageLoad(
saml_test_users::kFourthUserCorpExampleTestEmail);

if (Test::HasFailure()) {
return;
}

ASSERT_FALSE(fake_saml_idp()->IsLastChallengeResponseExists());
}

// Verify that device attestation works when all policies configured correctly.
IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest, Success) {
SetAllowedUrlsPolicy({fake_saml_idp()->GetIdpHost()});
settings_provider_->SetBoolean(kDeviceAttestationEnabled, true);

StartSamlAndWaitForIdpPageLoad(
saml_test_users::kFourthUserCorpExampleTestEmail);
Expand All @@ -2052,7 +2035,6 @@ IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest, Success) {
// allowed URLs list.
IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest, PolicyNoMatchError) {
SetAllowedUrlsPolicy({fake_saml_idp()->GetIdpDomain()});
settings_provider_->SetBoolean(kDeviceAttestationEnabled, true);

StartSamlAndWaitForIdpPageLoad(
saml_test_users::kFourthUserCorpExampleTestEmail);
Expand All @@ -2070,7 +2052,6 @@ IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest, PolicyNoMatchError) {
// from allowed URLs list.
IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest, PolicyRegexSuccess) {
SetAllowedUrlsPolicy({"[*.]" + fake_saml_idp()->GetIdpDomain()});
settings_provider_->SetBoolean(kDeviceAttestationEnabled, true);

StartSamlAndWaitForIdpPageLoad(
saml_test_users::kFourthUserCorpExampleTestEmail);
Expand All @@ -2091,7 +2072,6 @@ IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest, PolicyRegexSuccess) {
IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest,
PolicyTwoEntriesSuccess) {
SetAllowedUrlsPolicy({"example2.com", fake_saml_idp()->GetIdpHost()});
settings_provider_->SetBoolean(kDeviceAttestationEnabled, true);

StartSamlAndWaitForIdpPageLoad(
saml_test_users::kFourthUserCorpExampleTestEmail);
Expand All @@ -2115,7 +2095,6 @@ IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest,
SetAllowedUrlsPolicy({fake_saml_idp()->GetIdpHost()});
SetDeviceContextAwareAccessSignalsAllowlistPolicy(
{fake_saml_idp()->GetIdpHost()});
settings_provider_->SetBoolean(kDeviceAttestationEnabled, true);

StartSamlAndWaitForIdpPageLoad(
saml_test_users::kFourthUserCorpExampleTestEmail);
Expand All @@ -2139,7 +2118,6 @@ IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest,

IN_PROC_BROWSER_TEST_F(SAMLDeviceAttestationEnrolledTest, TimeoutError) {
SetAllowedUrlsPolicy({"example2.com", fake_saml_idp()->GetIdpHost()});
settings_provider_->SetBoolean(kDeviceAttestationEnabled, true);

AttestationClient::Get()
->GetTestInterface()
Expand Down Expand Up @@ -2176,12 +2154,6 @@ class SAMLDeviceTrustTest : public SAMLDeviceAttestationTest,
DeviceStateMixin::State::OOBE_COMPLETED_CLOUD_ENROLLED);
}

void SetUpInProcessBrowserTestFixture() override {
SAMLDeviceAttestationTest::SetUpInProcessBrowserTestFixture();
// Enable device trust feature.
settings_provider_->SetBoolean(kDeviceAttestationEnabled, true);
}

void ExpectDeviceTrustSuccessful(bool expected) {
ASSERT_EQ(fake_saml_idp()->DeviceTrustHeaderRecieved(), expected);
ASSERT_EQ(fake_saml_idp()->IsLastChallengeResponseExists(), expected);
Expand Down Expand Up @@ -2249,24 +2221,6 @@ IN_PROC_BROWSER_TEST_P(SAMLDeviceTrustEnrolledTest, EmptyPolicy) {
ASSERT_FALSE(fake_saml_idp()->DeviceTrustHeaderRecieved());
}

// Verify that device trust is not available when device trust is
// not enabled.
IN_PROC_BROWSER_TEST_P(SAMLDeviceTrustEnrolledTest,
DeviceTrustNotEnabledError) {
SetDeviceContextAwareAccessSignalsAllowlistPolicy(
{fake_saml_idp()->GetIdpHost()});
settings_provider_->SetBoolean(kDeviceAttestationEnabled, false);

StartSamlAndWaitForIdpPageLoad(
saml_test_users::kSixthUserCorpExampleTestEmail);

if (Test::HasFailure()) {
return;
}

ASSERT_FALSE(fake_saml_idp()->IsLastChallengeResponseExists());
}

// Verify that device trust is available for URLs that match a pattern
// from allowed URLs list.
IN_PROC_BROWSER_TEST_P(SAMLDeviceTrustEnrolledTest, PolicyRegexSuccess) {
Expand Down
7 changes: 1 addition & 6 deletions .../browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api_unittest.cc
View file
Expand Up @@ -277,12 +277,7 @@ class EPKChallengeUserKeyTest : public EPKChallengeKeyTestBase {
func_->set_extension(extension_.get());
}

void SetUp() override {
EPKChallengeKeyTestBase::SetUp();

// Set the user preferences.
prefs_->SetBoolean(prefs::kAttestationEnabled, true);
}
void SetUp() override { EPKChallengeKeyTestBase::SetUp(); }

base::Value::List CreateArgs() { return CreateArgsInternal(true); }

Expand Down
8 changes: 4 additions & 4 deletions chrome/browser/lacros/keystore_service_lacros_browsertest.cc
View file
Expand Up @@ -170,11 +170,11 @@ IN_PROC_BROWSER_TEST_F(KeystoreServiceLacrosBrowserTest, WrongFormattingUser) {

ASSERT_TRUE(result->is_error_message());

// TODO(https://crbug.com/1134349): Currently this errors out because remote
// attestation is disabled. We want this to error out because of a poorly
// formatted attestation message.
// TODO(https://crbug.com/1134349): Currently this errors out because the
// device is not enterprise enrolled. We want this to error out because of a
// poorly formatted attestation message.
const char expected_error_message[] =
"Remote attestation is not enabled for your account.";
"Failed to get Enterprise certificate. Error code = 2";
EXPECT_EQ(expected_error_message, result->get_error_message());
}

Expand Down
1 change: 1 addition & 0 deletions chrome/common/pref_names.cc
View file
Expand Up @@ -572,6 +572,7 @@ const char kLastSessionLength[] = "session.last_session_length";
// honored for public accounts.
const char kTermsOfServiceURL[] = "terms_of_service.url";

// TODO(b/285556135): Remove this pref together with AttestationEnabledForUser
// Indicates whether the remote attestation is enabled for the user.
const char kAttestationEnabled[] = "attestation.enabled";

Expand Down
1 change: 1 addition & 0 deletions chromeos/ash/components/settings/cros_settings_names.cc
View file
Expand Up @@ -303,6 +303,7 @@ const char kFeatureFlags[] = "cros.feature_flags";
const char kVariationsRestrictParameter[] =
"cros.variations_restrict_parameter";

// TODO(b/285556135): Remove this pref together with AttestationEnabledForDevice
// A boolean pref that indicates whether enterprise attestation is enabled for
// the device.
const char kDeviceAttestationEnabled[] = "cros.device.attestation_enabled";
Expand Down

0 comments on commit 6005431

Please sign in to comment.