Skip to content

Commit

Permalink
Treat public slot keys as stored on user token
Browse files Browse the repository at this point in the history 
Keys that are stored on a user's public token are now recognized by
GetKeyLocations.
If no locations were found for a key,
KeyPermissions::CanUserGrantPermissionsFor now returns false as a
safeguard.
A mechanism was introduced in nss_util.cc to simulate a separate private
slot in Chrome OS browsertests.

Bug: 839573
Test: browser_tests --gtest_filter=*PlatformKeysTest*
Change-Id: If8b4cd4ef3a5763dd37a158426ec3cf87e1d10ae
Reviewed-on: https://chromium-review.googlesource.com/1047208
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557667}
  • Loading branch information
Pavol Marko authored and Commit Bot committed May 10, 2018
1 parent 00fdce3 commit 725eaa4
Show file tree
Hide file tree
Showing 7 changed files with 204 additions and 51 deletions.
60 changes: 39 additions & 21 deletions chrome/browser/chromeos/login/webview_login_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,27 +405,6 @@ class WebviewClientCertsLoginTest : public WebviewLoginTest {
public:
WebviewClientCertsLoginTest() {}

void SetUpCommandLine(base::CommandLine* command_line) override {
command_line->AppendSwitch(
switches::kDisableSigninFrameClientCertUserSelection);
WebviewLoginTest::SetUpCommandLine(command_line);
}

void SetUpInProcessBrowserTestFixture() override {
auto fake_session_manager_client =
std::make_unique<FakeSessionManagerClient>();
fake_session_manager_client_ = fake_session_manager_client.get();
DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
std::move(fake_session_manager_client));
device_policy_test_helper_.InstallOwnerKey();
device_policy_test_helper_.MarkAsEnterpriseOwned();

fake_session_manager_client_->set_device_policy(
device_policy_test_helper_.device_policy()->GetBlob());

WebviewLoginTest::SetUpInProcessBrowserTestFixture();
}

// Installs a testing system slot and imports a client certificate into it.
void SetUpClientCertInSystemSlot() {
{
Expand Down Expand Up @@ -534,6 +513,30 @@ class WebviewClientCertsLoginTest : public WebviewLoginTest {
OobeScreenWaiter(OobeScreen::SCREEN_OOBE_EULA).Wait();
}

protected:
void SetUpCommandLine(base::CommandLine* command_line) override {
command_line->AppendSwitch(
switches::kDisableSigninFrameClientCertUserSelection);
WebviewLoginTest::SetUpCommandLine(command_line);
}

void SetUpInProcessBrowserTestFixture() override {
auto fake_session_manager_client =
std::make_unique<FakeSessionManagerClient>();
fake_session_manager_client_ = fake_session_manager_client.get();
DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
std::move(fake_session_manager_client));
device_policy_test_helper_.InstallOwnerKey();
device_policy_test_helper_.MarkAsEnterpriseOwned();

fake_session_manager_client_->set_device_policy(
device_policy_test_helper_.device_policy()->GetBlob());

WebviewLoginTest::SetUpInProcessBrowserTestFixture();
}

void TearDownOnMainThread() override { TearDownTestSystemSlot(); }

private:
void SetUpTestSystemSlotOnIO(bool* out_system_slot_constructed_successfully) {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
Expand All @@ -542,6 +545,21 @@ class WebviewClientCertsLoginTest : public WebviewLoginTest {
test_system_slot_->ConstructedSuccessfully();
}

void TearDownTestSystemSlot() {
if (!test_system_slot_)
return;

base::RunLoop loop;
content::BrowserThread::PostTaskAndReply(
content::BrowserThread::IO, FROM_HERE,
base::BindOnce(&WebviewClientCertsLoginTest::TearDownTestSystemSlotOnIO,
base::Unretained(this)),
loop.QuitClosure());
loop.Run();
}

void TearDownTestSystemSlotOnIO() { test_system_slot_.reset(); }

// Builds a device ONC dictionary defining a single untrusted authority
// certificate.
base::DictionaryValue BuildDeviceOncDictForUntrustedAuthority(
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/chromeos/platform_keys/key_permissions.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,9 @@ void KeyPermissions::GetPermissionsForExtension(
bool KeyPermissions::CanUserGrantPermissionFor(
const std::string& public_key_spki_der,
const std::vector<KeyLocation>& key_locations) const {
if (key_locations.empty())
return false;

// As keys cannot be tagged for non-corporate usage, the user can currently
// not grant any permissions if the profile is managed.
if (profile_is_managed_)
Expand Down
8 changes: 8 additions & 0 deletions chrome/browser/chromeos/platform_keys/platform_keys_nss.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,14 @@ void GetKeyLocationsWithDB(std::unique_ptr<GetKeyLocationsState> state,
if (rsa_key)
token_ids.push_back(kTokenIdUser);
}
if (token_ids.empty() && cert_db->GetPublicSlot().get()) {
crypto::ScopedSECKEYPrivateKey rsa_key =
crypto::FindNSSKeyFromPublicKeyInfoInSlot(
public_key_vector, cert_db->GetPublicSlot().get());
if (rsa_key)
token_ids.push_back(kTokenIdUser);
}

if (cert_db->GetSystemSlot().get()) {
crypto::ScopedSECKEYPrivateKey rsa_key =
crypto::FindNSSKeyFromPublicKeyInfoInSlot(
Expand Down
21 changes: 21 additions & 0 deletions chrome/browser/chromeos/policy/user_affiliation_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,12 @@ class UserAffiliationBrowserTest
policy::DeviceManagementService::SetRetryDelayForTesting(0);
}

void TearDownOnMainThread() override {
InProcessBrowserTest::TearDownOnMainThread();

TearDownTestSystemSlot();
}

const AccountId account_id_;

void SetUpTestSystemSlot() {
Expand All @@ -178,6 +184,21 @@ class UserAffiliationBrowserTest
test_system_slot_->ConstructedSuccessfully();
}

void TearDownTestSystemSlot() {
if (!test_system_slot_)
return;

base::RunLoop loop;
content::BrowserThread::PostTaskAndReply(
content::BrowserThread::IO, FROM_HERE,
base::BindOnce(&UserAffiliationBrowserTest::TearDownTestSystemSlotOnIO,
base::Unretained(this)),
loop.QuitClosure());
loop.Run();
}

void TearDownTestSystemSlotOnIO() { test_system_slot_.reset(); }

std::unique_ptr<crypto::ScopedTestSystemNSSKeySlot> test_system_slot_;

DISALLOW_COPY_AND_ASSIGN(UserAffiliationBrowserTest);
Expand Down
129 changes: 99 additions & 30 deletions chrome/browser/extensions/api/platform_keys/platform_keys_apitest_nss.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

#include <cryptohi.h>
#include <pk11pub.h>

#include <memory>
#include <utility>
Expand All @@ -20,6 +21,8 @@
#include "chrome/browser/profiles/profile.h"
#include "components/policy/policy_constants.h"
#include "crypto/nss_util_internal.h"
#include "crypto/scoped_nss_types.h"
#include "crypto/scoped_test_nss_db.h"
#include "crypto/scoped_test_system_nss_key_slot.h"
#include "net/cert/nss_cert_database.h"
#include "net/cert/test_root_certs.h"
Expand All @@ -30,15 +33,34 @@ namespace {

class PlatformKeysTest : public PlatformKeysTestBase {
public:
enum class UserClientCertSlot { kPrivateSlot, kPublicSlot };

PlatformKeysTest(EnrollmentStatus enrollment_status,
UserStatus user_status,
bool key_permission_policy)
bool key_permission_policy,
UserClientCertSlot user_client_cert_slot)
: PlatformKeysTestBase(SystemTokenStatus::EXISTS,
enrollment_status,
user_status),
key_permission_policy_(key_permission_policy) {}
key_permission_policy_(key_permission_policy),
user_client_cert_slot_(user_client_cert_slot) {}

void SetUpOnMainThread() override {
if (!IsPreTest()) {
// Set up the private slot before
// |PlatformKeysTestBase::SetUpOnMainThread| triggers the user sign-in.
ASSERT_TRUE(user_private_slot_db_.is_open());
base::RunLoop loop;
content::BrowserThread::PostTaskAndReply(
content::BrowserThread::IO, FROM_HERE,
base::BindOnce(&PlatformKeysTest::SetPrivateSoftwareSlotOnIO,
base::Unretained(this),
crypto::ScopedPK11Slot(
PK11_ReferenceSlot(user_private_slot_db_.slot()))),
loop.QuitClosure());
loop.Run();
}

PlatformKeysTestBase::SetUpOnMainThread();

if (IsPreTest())
Expand Down Expand Up @@ -128,11 +150,18 @@ class PlatformKeysTest : public PlatformKeysTestBase {
}

protected:
// Imported into user's private or public slot, depending on the value of
// |user_client_cert_slot_|.
scoped_refptr<net::X509Certificate> client_cert1_;
// Imported into system slot.
scoped_refptr<net::X509Certificate> client_cert2_;
const extensions::Extension* extension_;

private:
void SetPrivateSoftwareSlotOnIO(crypto::ScopedPK11Slot slot) {
crypto::SetPrivateSoftwareSlotForChromeOSUserForTesting(std::move(slot));
}

void GotPermissionsForExtension(
const base::Closure& done_callback,
std::unique_ptr<chromeos::KeyPermissions::PermissionsForExtension>
Expand All @@ -152,9 +181,16 @@ class PlatformKeysTest : public PlatformKeysTestBase {
}

void SetupTestClientCerts(net::NSSCertDatabase* cert_db) {
// Sanity check to ensure that
// SetPrivateSoftwareSlotForChromeOSUserForTesting took effect.
EXPECT_EQ(user_private_slot_db_.slot(), cert_db->GetPrivateSlot().get());
EXPECT_NE(cert_db->GetPrivateSlot().get(), cert_db->GetPublicSlot().get());

auto* slot = user_client_cert_slot_ == UserClientCertSlot::kPrivateSlot
? cert_db->GetPrivateSlot().get()
: cert_db->GetPublicSlot().get();
client_cert1_ = net::ImportClientCertAndKeyFromFile(
net::GetTestCertsDirectory(), "client_1.pem", "client_1.pk8",
cert_db->GetPrivateSlot().get());
net::GetTestCertsDirectory(), "client_1.pem", "client_1.pk8", slot);
ASSERT_TRUE(client_cert1_.get());

// Import a second client cert signed by another CA than client_1 into the
Expand All @@ -175,6 +211,8 @@ class PlatformKeysTest : public PlatformKeysTestBase {
}

const bool key_permission_policy_;
const UserClientCertSlot user_client_cert_slot_;
crypto::ScopedTestNSSDB user_private_slot_db_;

DISALLOW_COPY_AND_ASSIGN(PlatformKeysTest);
};
Expand Down Expand Up @@ -219,19 +257,32 @@ class TestSelectDelegate
net::CertificateList certs_to_select_;
};

class UnmanagedPlatformKeysTest : public PlatformKeysTest,
public ::testing::WithParamInterface<
PlatformKeysTestBase::EnrollmentStatus> {
struct UnmanagedPlatformKeysTestParams {
UnmanagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus enrollment_status,
PlatformKeysTest::UserClientCertSlot user_client_cert_slot)
: enrollment_status_(enrollment_status),
user_client_cert_slot_(user_client_cert_slot) {}

PlatformKeysTestBase::EnrollmentStatus enrollment_status_;
PlatformKeysTest::UserClientCertSlot user_client_cert_slot_;
};

class UnmanagedPlatformKeysTest
: public PlatformKeysTest,
public ::testing::WithParamInterface<UnmanagedPlatformKeysTestParams> {
public:
UnmanagedPlatformKeysTest()
: PlatformKeysTest(GetParam(),
: PlatformKeysTest(GetParam().enrollment_status_,
UserStatus::UNMANAGED,
false /* unused */) {}
false /* unused */,
GetParam().user_client_cert_slot_) {}
};

struct Params {
Params(PlatformKeysTestBase::EnrollmentStatus enrollment_status,
PlatformKeysTestBase::UserStatus user_status)
struct ManagedPlatformKeysTestParams {
ManagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus enrollment_status,
PlatformKeysTestBase::UserStatus user_status)
: enrollment_status_(enrollment_status), user_status_(user_status) {}

PlatformKeysTestBase::EnrollmentStatus enrollment_status_;
Expand All @@ -240,22 +291,24 @@ struct Params {

class ManagedWithPermissionPlatformKeysTest
: public PlatformKeysTest,
public ::testing::WithParamInterface<Params> {
public ::testing::WithParamInterface<ManagedPlatformKeysTestParams> {
public:
ManagedWithPermissionPlatformKeysTest()
: PlatformKeysTest(GetParam().enrollment_status_,
GetParam().user_status_,
true /* grant the extension key permission */) {}
true /* grant the extension key permission */,
UserClientCertSlot::kPrivateSlot) {}
};

class ManagedWithoutPermissionPlatformKeysTest
: public PlatformKeysTest,
public ::testing::WithParamInterface<Params> {
public ::testing::WithParamInterface<ManagedPlatformKeysTestParams> {
public:
ManagedWithoutPermissionPlatformKeysTest()
: PlatformKeysTest(GetParam().enrollment_status_,
GetParam().user_status_,
false /* do not grant key permission */) {}
false /* do not grant key permission */,
UserClientCertSlot::kPrivateSlot) {}
};

} // namespace
Expand Down Expand Up @@ -299,8 +352,18 @@ IN_PROC_BROWSER_TEST_P(UnmanagedPlatformKeysTest, Permissions) {
INSTANTIATE_TEST_CASE_P(
Unmanaged,
UnmanagedPlatformKeysTest,
::testing::Values(PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTestBase::EnrollmentStatus::NOT_ENROLLED));
::testing::Values(UnmanagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTest::UserClientCertSlot::kPrivateSlot),
UnmanagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::NOT_ENROLLED,
PlatformKeysTest::UserClientCertSlot::kPrivateSlot),
UnmanagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTest::UserClientCertSlot::kPublicSlot),
UnmanagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::NOT_ENROLLED,
PlatformKeysTest::UserClientCertSlot::kPublicSlot)));

IN_PROC_BROWSER_TEST_P(ManagedWithoutPermissionPlatformKeysTest,
PRE_UserPermissionsBlocked) {
Expand Down Expand Up @@ -340,12 +403,15 @@ INSTANTIATE_TEST_CASE_P(
ManagedWithoutPermission,
ManagedWithoutPermissionPlatformKeysTest,
::testing::Values(
Params(PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_AFFILIATED_DOMAIN),
Params(PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_OTHER_DOMAIN),
Params(PlatformKeysTestBase::EnrollmentStatus::NOT_ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_OTHER_DOMAIN)));
ManagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_AFFILIATED_DOMAIN),
ManagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_OTHER_DOMAIN),
ManagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::NOT_ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_OTHER_DOMAIN)));

IN_PROC_BROWSER_TEST_P(ManagedWithPermissionPlatformKeysTest,
PRE_PolicyGrantsAccessToCorporateKey) {
Expand Down Expand Up @@ -396,9 +462,12 @@ INSTANTIATE_TEST_CASE_P(
ManagedWithPermission,
ManagedWithPermissionPlatformKeysTest,
::testing::Values(
Params(PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_AFFILIATED_DOMAIN),
Params(PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_OTHER_DOMAIN),
Params(PlatformKeysTestBase::EnrollmentStatus::NOT_ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_OTHER_DOMAIN)));
ManagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_AFFILIATED_DOMAIN),
ManagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_OTHER_DOMAIN),
ManagedPlatformKeysTestParams(
PlatformKeysTestBase::EnrollmentStatus::NOT_ENROLLED,
PlatformKeysTestBase::UserStatus::MANAGED_OTHER_DOMAIN)));
Loading

0 comments on commit 725eaa4

Please sign in to comment.