TrustStoreWin: refactor how cert stores are passed in tests
Pass the cert stores in a CertStores struct rather than as individual parameters, to avoid the risk of getting the parameter order confused. Also reduces some redundancy from repeating the CertOpenStore calls in multiple tests. Change-Id: I94f40dfe745bc8b46ddb28cfe064d92dfb5aea55 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4166726 Reviewed-by: Hubert Chao <hchao@chromium.org> Commit-Queue: Matt Mueller <mattm@chromium.org> Cr-Commit-Position: refs/heads/main@{#1094625}
This commit is contained in:
Matt Mueller
committed by
Chromium LUCI CQ
Chromium LUCI CQ
parent
a8bef63bc7
commit
50492356ca
@ -252,24 +252,19 @@ bool AreCertsEq(const std::shared_ptr<const ParsedCertificate> cert_1,
|
||||
// Test to ensure that path building stops when an intermediate cert is
|
||||
// encountered that is not usable for TLS because it is explicitly distrusted.
|
||||
TEST_F(PathBuilderMultiRootWindowsTest, TrustStoreWinOnlyFindTrustedTLSPath) {
|
||||
crypto::ScopedHCERTSTORE root_store(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
crypto::ScopedHCERTSTORE intermediate_store(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
crypto::ScopedHCERTSTORE disallowed_store(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
TrustStoreWin::CertStores stores =
|
||||
TrustStoreWin::CertStores::CreateInMemoryStoresForTesting();
|
||||
|
||||
AddToStoreWithEKURestriction(root_store.get(), d_by_d_,
|
||||
AddToStoreWithEKURestriction(stores.roots.get(), d_by_d_,
|
||||
szOID_PKIX_KP_SERVER_AUTH);
|
||||
AddToStoreWithEKURestriction(root_store.get(), e_by_e_,
|
||||
AddToStoreWithEKURestriction(stores.roots.get(), e_by_e_,
|
||||
szOID_PKIX_KP_SERVER_AUTH);
|
||||
AddToStoreWithEKURestriction(intermediate_store.get(), c_by_e_,
|
||||
AddToStoreWithEKURestriction(stores.intermediates.get(), c_by_e_,
|
||||
szOID_PKIX_KP_SERVER_AUTH);
|
||||
AddToStoreWithEKURestriction(disallowed_store.get(), c_by_d_, nullptr);
|
||||
AddToStoreWithEKURestriction(stores.disallowed.get(), c_by_d_, nullptr);
|
||||
|
||||
std::unique_ptr<TrustStoreWin> trust_store = TrustStoreWin::CreateForTesting(
|
||||
std::move(root_store), std::move(intermediate_store),
|
||||
std::move(disallowed_store));
|
||||
std::unique_ptr<TrustStoreWin> trust_store =
|
||||
TrustStoreWin::CreateForTesting(std::move(stores));
|
||||
|
||||
CertPathBuilder path_builder(
|
||||
b_by_c_, trust_store.get(), &delegate_, time_, KeyPurpose::ANY_EKU,
|
||||
@ -299,19 +294,14 @@ TEST_F(PathBuilderMultiRootWindowsTest, TrustStoreWinOnlyFindTrustedTLSPath) {
|
||||
// path, then path building should fail, even if the root is enabled for
|
||||
// TLS.
|
||||
TEST_F(PathBuilderMultiRootWindowsTest, TrustStoreWinNoPathEKURestrictions) {
|
||||
crypto::ScopedHCERTSTORE root_store(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
crypto::ScopedHCERTSTORE intermediate_store(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
crypto::ScopedHCERTSTORE disallowed_store(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
TrustStoreWin::CertStores stores =
|
||||
TrustStoreWin::CertStores::CreateInMemoryStoresForTesting();
|
||||
|
||||
AddToStoreWithEKURestriction(root_store.get(), d_by_d_,
|
||||
AddToStoreWithEKURestriction(stores.roots.get(), d_by_d_,
|
||||
szOID_PKIX_KP_SERVER_AUTH);
|
||||
AddToStoreWithEKURestriction(disallowed_store.get(), c_by_d_, nullptr);
|
||||
std::unique_ptr<TrustStoreWin> trust_store = TrustStoreWin::CreateForTesting(
|
||||
std::move(root_store), std::move(intermediate_store),
|
||||
std::move(disallowed_store));
|
||||
AddToStoreWithEKURestriction(stores.disallowed.get(), c_by_d_, nullptr);
|
||||
std::unique_ptr<TrustStoreWin> trust_store =
|
||||
TrustStoreWin::CreateForTesting(std::move(stores));
|
||||
|
||||
CertPathBuilder path_builder(
|
||||
b_by_c_, trust_store.get(), &delegate_, time_, KeyPurpose::ANY_EKU,
|
||||
|
||||
@ -81,6 +81,30 @@ bool IsCertTrustedForServerAuth(PCCERT_CONTEXT cert) {
|
||||
|
||||
} // namespace
|
||||
|
||||
TrustStoreWin::CertStores::CertStores() = default;
|
||||
TrustStoreWin::CertStores::~CertStores() = default;
|
||||
TrustStoreWin::CertStores::CertStores(CertStores&& other) = default;
|
||||
TrustStoreWin::CertStores& TrustStoreWin::CertStores::operator=(
|
||||
CertStores&& other) = default;
|
||||
|
||||
// static
|
||||
TrustStoreWin::CertStores
|
||||
TrustStoreWin::CertStores::CreateInMemoryStoresForTesting() {
|
||||
TrustStoreWin::CertStores stores;
|
||||
stores.roots = crypto::ScopedHCERTSTORE(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
stores.intermediates = crypto::ScopedHCERTSTORE(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
stores.disallowed = crypto::ScopedHCERTSTORE(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
return stores;
|
||||
}
|
||||
|
||||
TrustStoreWin::CertStores
|
||||
TrustStoreWin::CertStores::CreateNullStoresForTesting() {
|
||||
return TrustStoreWin::CertStores();
|
||||
}
|
||||
|
||||
class TrustStoreWin::Impl {
|
||||
public:
|
||||
// Creates a TrustStoreWin.
|
||||
@ -183,14 +207,11 @@ class TrustStoreWin::Impl {
|
||||
// all_certs_store should be a combination of root_cert_store and
|
||||
// intermediate_cert_store, but we ask callers to explicitly pass this in so
|
||||
// that all the error checking can happen outside of the constructor.
|
||||
Impl(crypto::ScopedHCERTSTORE root_cert_store,
|
||||
crypto::ScopedHCERTSTORE intermediate_cert_store,
|
||||
crypto::ScopedHCERTSTORE disallowed_cert_store,
|
||||
crypto::ScopedHCERTSTORE all_certs_store)
|
||||
: root_cert_store_(std::move(root_cert_store)),
|
||||
intermediate_cert_store_(std::move(intermediate_cert_store)),
|
||||
Impl(CertStores stores, crypto::ScopedHCERTSTORE all_certs_store)
|
||||
: root_cert_store_(std::move(stores.roots)),
|
||||
intermediate_cert_store_(std::move(stores.intermediates)),
|
||||
all_certs_store_(std::move(all_certs_store)),
|
||||
disallowed_cert_store_(std::move(disallowed_cert_store)) {}
|
||||
disallowed_cert_store_(std::move(stores.disallowed)) {}
|
||||
|
||||
~Impl() = default;
|
||||
Impl(const Impl& other) = delete;
|
||||
@ -313,26 +334,22 @@ TrustStoreWin::Impl* TrustStoreWin::MaybeInitializeAndGetImpl() {
|
||||
}
|
||||
|
||||
std::unique_ptr<TrustStoreWin> TrustStoreWin::CreateForTesting(
|
||||
crypto::ScopedHCERTSTORE root_cert_store,
|
||||
crypto::ScopedHCERTSTORE intermediate_cert_store,
|
||||
crypto::ScopedHCERTSTORE disallowed_cert_store) {
|
||||
CertStores stores) {
|
||||
crypto::ScopedHCERTSTORE all_certs_store(
|
||||
CertOpenStore(CERT_STORE_PROV_COLLECTION, 0, NULL, 0, nullptr));
|
||||
|
||||
if (all_certs_store.get() && intermediate_cert_store.get()) {
|
||||
CertAddStoreToCollection(all_certs_store.get(),
|
||||
intermediate_cert_store.get(),
|
||||
if (all_certs_store.get() && stores.intermediates.get()) {
|
||||
CertAddStoreToCollection(all_certs_store.get(), stores.intermediates.get(),
|
||||
/*dwUpdateFlags=*/0, /*dwPriority=*/0);
|
||||
}
|
||||
if (all_certs_store.get() && root_cert_store.get()) {
|
||||
CertAddStoreToCollection(all_certs_store.get(), root_cert_store.get(),
|
||||
if (all_certs_store.get() && stores.roots.get()) {
|
||||
CertAddStoreToCollection(all_certs_store.get(), stores.roots.get(),
|
||||
/*dwUpdateFlags=*/0, /*dwPriority=*/0);
|
||||
}
|
||||
|
||||
return base::WrapUnique(
|
||||
new TrustStoreWin(std::make_unique<TrustStoreWin::Impl>(
|
||||
std::move(root_cert_store), std::move(intermediate_cert_store),
|
||||
std::move(disallowed_cert_store), std::move(all_certs_store))));
|
||||
std::move(stores), std::move(all_certs_store))));
|
||||
}
|
||||
|
||||
TrustStoreWin::TrustStoreWin(std::unique_ptr<Impl> impl)
|
||||
|
||||
@ -20,6 +20,27 @@ namespace net {
|
||||
// TODO(https://crbug.com/1239270): confirm this is thread safe.
|
||||
class NET_EXPORT TrustStoreWin : public TrustStore {
|
||||
public:
|
||||
struct NET_EXPORT_PRIVATE CertStores {
|
||||
~CertStores();
|
||||
CertStores(CertStores&& other);
|
||||
CertStores& operator=(CertStores&& other);
|
||||
|
||||
// Create a CertStores object with the stores pre-initialized with
|
||||
// in-memory cert stores for testing purposes.
|
||||
static CertStores CreateInMemoryStoresForTesting();
|
||||
|
||||
// Create a CertStores object with null cert store pointers for testing
|
||||
// purposes.
|
||||
static CertStores CreateNullStoresForTesting();
|
||||
|
||||
crypto::ScopedHCERTSTORE roots;
|
||||
crypto::ScopedHCERTSTORE intermediates;
|
||||
crypto::ScopedHCERTSTORE disallowed;
|
||||
|
||||
private:
|
||||
CertStores();
|
||||
};
|
||||
|
||||
// Creates a TrustStoreWin.
|
||||
TrustStoreWin();
|
||||
|
||||
@ -31,10 +52,7 @@ class NET_EXPORT TrustStoreWin : public TrustStore {
|
||||
// as if it's the source of truth for roots for `GetTrust,
|
||||
// and `intermediate_cert_store` as an extra store (in addition to
|
||||
// root_cert_store) for locating certificates during `SyncGetIssuersOf`.
|
||||
static std::unique_ptr<TrustStoreWin> CreateForTesting(
|
||||
crypto::ScopedHCERTSTORE root_cert_store,
|
||||
crypto::ScopedHCERTSTORE intermediate_cert_store,
|
||||
crypto::ScopedHCERTSTORE disallowed_cert_store);
|
||||
static std::unique_ptr<TrustStoreWin> CreateForTesting(CertStores stores);
|
||||
|
||||
// Loads user settings from Windows CertStores. If there are errors,
|
||||
// the underlyingTrustStoreWin object may not read all Windows
|
||||
|
||||
@ -57,13 +57,6 @@ constexpr CertificateTrust ExpectedTrustForAnchor() {
|
||||
class TrustStoreWinTest : public testing::Test {
|
||||
public:
|
||||
void SetUp() override {
|
||||
root_store_.reset(CertOpenStore(CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING,
|
||||
NULL, 0, nullptr));
|
||||
intermediate_store_.reset(CertOpenStore(
|
||||
CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
disallowed_store_.reset(CertOpenStore(CERT_STORE_PROV_MEMORY,
|
||||
X509_ASN_ENCODING, NULL, 0, nullptr));
|
||||
|
||||
ASSERT_TRUE(ParseCertFromFile("multi-root-A-by-B.pem", &a_by_b_));
|
||||
ASSERT_TRUE(ParseCertFromFile("multi-root-B-by-C.pem", &b_by_c_));
|
||||
ASSERT_TRUE(ParseCertFromFile("multi-root-B-by-F.pem", &b_by_f_));
|
||||
@ -109,16 +102,13 @@ class TrustStoreWinTest : public testing::Test {
|
||||
}
|
||||
|
||||
std::unique_ptr<TrustStoreWin> CreateTrustStoreWin() {
|
||||
return TrustStoreWin::CreateForTesting(std::move(root_store_),
|
||||
std::move(intermediate_store_),
|
||||
std::move(disallowed_store_));
|
||||
return TrustStoreWin::CreateForTesting(std::move(stores_));
|
||||
}
|
||||
|
||||
// The cert stores that will be used to create the trust store. These handles
|
||||
// will be null after CreateTrustStoreWin() is called.
|
||||
crypto::ScopedHCERTSTORE root_store_;
|
||||
crypto::ScopedHCERTSTORE intermediate_store_;
|
||||
crypto::ScopedHCERTSTORE disallowed_store_;
|
||||
TrustStoreWin::CertStores stores_ =
|
||||
TrustStoreWin::CertStores::CreateInMemoryStoresForTesting();
|
||||
|
||||
std::shared_ptr<const ParsedCertificate> a_by_b_, b_by_c_, b_by_f_, c_by_d_,
|
||||
c_by_e_, d_by_d_, e_by_e_, f_by_e_;
|
||||
@ -127,9 +117,8 @@ class TrustStoreWinTest : public testing::Test {
|
||||
TEST_F(TrustStoreWinTest, GetTrustInitializationError) {
|
||||
// Simulate an initialization error by using null stores.
|
||||
std::unique_ptr<TrustStoreWin> trust_store_win =
|
||||
TrustStoreWin::CreateForTesting(crypto::ScopedHCERTSTORE(),
|
||||
crypto::ScopedHCERTSTORE(),
|
||||
crypto::ScopedHCERTSTORE());
|
||||
TrustStoreWin::CreateForTesting(
|
||||
TrustStoreWin::CertStores::CreateNullStoresForTesting());
|
||||
ASSERT_TRUE(trust_store_win);
|
||||
CertificateTrust trust = trust_store_win->GetTrust(d_by_d_.get(), nullptr);
|
||||
EXPECT_EQ(CertificateTrust::ForUnspecified().ToDebugString(),
|
||||
@ -137,8 +126,8 @@ TEST_F(TrustStoreWinTest, GetTrustInitializationError) {
|
||||
}
|
||||
|
||||
TEST_F(TrustStoreWinTest, GetTrust) {
|
||||
ASSERT_TRUE(AddToStore(root_store_.get(), d_by_d_));
|
||||
ASSERT_TRUE(AddToStore(intermediate_store_.get(), c_by_d_));
|
||||
ASSERT_TRUE(AddToStore(stores_.roots.get(), d_by_d_));
|
||||
ASSERT_TRUE(AddToStore(stores_.intermediates.get(), c_by_d_));
|
||||
|
||||
std::unique_ptr<TrustStoreWin> trust_store_win = CreateTrustStoreWin();
|
||||
ASSERT_TRUE(trust_store_win);
|
||||
@ -166,14 +155,14 @@ TEST_F(TrustStoreWinTest, GetTrust) {
|
||||
// - kMultiRootCByE: only has szOID_ANY_ENHANCED_KEY_USAGE set
|
||||
// - kMultiRootCByD: no EKU usages set
|
||||
TEST_F(TrustStoreWinTest, GetTrustRestrictedEKU) {
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(root_store_.get(), d_by_d_,
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(stores_.roots.get(), d_by_d_,
|
||||
szOID_PKIX_KP_SERVER_AUTH));
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(root_store_.get(), e_by_e_,
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(stores_.roots.get(), e_by_e_,
|
||||
szOID_PKIX_KP_CLIENT_AUTH));
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(root_store_.get(), c_by_e_,
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(stores_.roots.get(), c_by_e_,
|
||||
szOID_ANY_ENHANCED_KEY_USAGE));
|
||||
ASSERT_TRUE(
|
||||
AddToStoreWithEKURestriction(root_store_.get(), c_by_d_, nullptr));
|
||||
AddToStoreWithEKURestriction(stores_.roots.get(), c_by_d_, nullptr));
|
||||
|
||||
std::unique_ptr<TrustStoreWin> trust_store_win = CreateTrustStoreWin();
|
||||
ASSERT_TRUE(trust_store_win);
|
||||
@ -210,12 +199,12 @@ TEST_F(TrustStoreWinTest, GetTrustRestrictedEKU) {
|
||||
// - kMultiRootDByD (dupe): only has szOID_PKIX_KP_SERVER_AUTH set
|
||||
// - kMultiRootDByD (dupe 2): no EKU usages set
|
||||
TEST_F(TrustStoreWinTest, GetTrustRestrictedEKUDuplicateCerts) {
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(root_store_.get(), d_by_d_,
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(stores_.roots.get(), d_by_d_,
|
||||
szOID_PKIX_KP_CLIENT_AUTH));
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(root_store_.get(), d_by_d_,
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(stores_.roots.get(), d_by_d_,
|
||||
szOID_PKIX_KP_SERVER_AUTH));
|
||||
ASSERT_TRUE(
|
||||
AddToStoreWithEKURestriction(root_store_.get(), d_by_d_, nullptr));
|
||||
AddToStoreWithEKURestriction(stores_.roots.get(), d_by_d_, nullptr));
|
||||
|
||||
std::unique_ptr<TrustStoreWin> trust_store_win = CreateTrustStoreWin();
|
||||
ASSERT_TRUE(trust_store_win);
|
||||
@ -227,12 +216,12 @@ TEST_F(TrustStoreWinTest, GetTrustRestrictedEKUDuplicateCerts) {
|
||||
|
||||
// Test that disallowed certs will be distrusted regardless of EKU settings.
|
||||
TEST_F(TrustStoreWinTest, GetTrustDisallowedCerts) {
|
||||
ASSERT_TRUE(AddToStore(root_store_.get(), d_by_d_));
|
||||
ASSERT_TRUE(AddToStore(root_store_.get(), e_by_e_));
|
||||
ASSERT_TRUE(AddToStore(stores_.roots.get(), d_by_d_));
|
||||
ASSERT_TRUE(AddToStore(stores_.roots.get(), e_by_e_));
|
||||
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(disallowed_store_.get(), d_by_d_,
|
||||
ASSERT_TRUE(AddToStoreWithEKURestriction(stores_.disallowed.get(), d_by_d_,
|
||||
szOID_PKIX_KP_CLIENT_AUTH));
|
||||
ASSERT_TRUE(AddToStore(disallowed_store_.get(), e_by_e_));
|
||||
ASSERT_TRUE(AddToStore(stores_.disallowed.get(), e_by_e_));
|
||||
|
||||
std::unique_ptr<TrustStoreWin> trust_store_win = CreateTrustStoreWin();
|
||||
ASSERT_TRUE(trust_store_win);
|
||||
@ -257,9 +246,8 @@ MATCHER_P(ParsedCertEq, expected_cert, "") {
|
||||
TEST_F(TrustStoreWinTest, GetIssuersInitializationError) {
|
||||
// Simulate an initialization error by using null stores.
|
||||
std::unique_ptr<TrustStoreWin> trust_store_win =
|
||||
TrustStoreWin::CreateForTesting(crypto::ScopedHCERTSTORE(),
|
||||
crypto::ScopedHCERTSTORE(),
|
||||
crypto::ScopedHCERTSTORE());
|
||||
TrustStoreWin::CreateForTesting(
|
||||
TrustStoreWin::CertStores::CreateNullStoresForTesting());
|
||||
ASSERT_TRUE(trust_store_win);
|
||||
ParsedCertificateList issuers;
|
||||
trust_store_win->SyncGetIssuersOf(b_by_f_.get(), &issuers);
|
||||
@ -267,10 +255,10 @@ TEST_F(TrustStoreWinTest, GetIssuersInitializationError) {
|
||||
}
|
||||
|
||||
TEST_F(TrustStoreWinTest, GetIssuers) {
|
||||
ASSERT_TRUE(AddToStore(root_store_.get(), d_by_d_));
|
||||
ASSERT_TRUE(AddToStore(intermediate_store_.get(), c_by_d_));
|
||||
ASSERT_TRUE(AddToStore(intermediate_store_.get(), c_by_e_));
|
||||
ASSERT_TRUE(AddToStore(intermediate_store_.get(), f_by_e_));
|
||||
ASSERT_TRUE(AddToStore(stores_.roots.get(), d_by_d_));
|
||||
ASSERT_TRUE(AddToStore(stores_.intermediates.get(), c_by_d_));
|
||||
ASSERT_TRUE(AddToStore(stores_.intermediates.get(), c_by_e_));
|
||||
ASSERT_TRUE(AddToStore(stores_.intermediates.get(), f_by_e_));
|
||||
|
||||
std::unique_ptr<TrustStoreWin> trust_store_win = CreateTrustStoreWin();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user