Skip to content

Commit

Permalink
Expect-CT: Pass NetworkIsolationKey through SpdySession::CanPool().
Browse files Browse the repository at this point in the history 
SpdySession::CanPool() checks CertificateTransparency state, so in
order to double-key the Expect-CT database, we need to pass the
NetworkIsolationKey through CanPool() down to
TransportSecurityState::CheckCTRequirements().

BUG: 969893
Change-Id: I612be66f8a530d01edd14b6286617e20a6b28f0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2212702
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#776542}
  • Loading branch information
Matt Menke authored and Commit Bot committed Jun 9, 2020
1 parent e088438 commit c74a926
Show file tree
Hide file tree
Showing 6 changed files with 407 additions and 53 deletions.
2 changes: 1 addition & 1 deletion net/quic/quic_chromium_client_session.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1362,7 +1362,7 @@ bool QuicChromiumClientSession::CanPool(

return SpdySession::CanPool(transport_security_state_, ssl_info,
*ssl_config_service_, session_key_.host(),
hostname);
hostname, network_isolation_key);
}

bool QuicChromiumClientSession::ShouldCreateIncomingStream(
Expand Down
164 changes: 160 additions & 4 deletions net/quic/quic_chromium_client_session_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ class QuicChromiumClientSessionTest
base::span<MockWrite>())),
random_(0),
helper_(&clock_, &random_),
transport_security_state_(std::make_unique<TransportSecurityState>()),
session_key_(kServerHostname,
kServerPort,
PRIVACY_MODE_DISABLED,
Expand Down Expand Up @@ -180,7 +181,7 @@ class QuicChromiumClientSessionTest
session_.reset(new TestingQuicChromiumClientSession(
connection, std::move(socket),
/*stream_factory=*/nullptr, &crypto_client_stream_factory_, &clock_,
&transport_security_state_, /*ssl_config_service=*/nullptr,
transport_security_state_.get(), /*ssl_config_service=*/nullptr,
base::WrapUnique(static_cast<QuicServerInfo*>(nullptr)), session_key_,
/*require_confirmation=*/false,
/*max_allowed_push_id=*/0, migrate_session_early_v2_,
Expand Down Expand Up @@ -277,7 +278,7 @@ class QuicChromiumClientSessionTest
quic::test::MockRandom random_;
QuicChromiumConnectionHelper helper_;
quic::test::MockAlarmFactory alarm_factory_;
TransportSecurityState transport_security_state_;
std::unique_ptr<TransportSecurityState> transport_security_state_;
MockCryptoClientStreamFactory crypto_client_stream_factory_;
quic::QuicClientPushPromiseIndex push_promise_index_;
QuicSessionKey session_key_;
Expand All @@ -302,6 +303,86 @@ INSTANTIATE_TEST_SUITE_P(VersionIncludeStreamDependencySequence,
// TODO(950069): Add testing for frame_origin in NetworkIsolationKey using
// kAppendInitiatingFrameOriginToNetworkIsolationKey.

// Basic test of ProofVerifyDetailsChromium is converted to SSLInfo retrieved
// through QuicChromiumClientSession::GetSSLInfo(). Doesn't test some of the
// more complicated fields.
TEST_P(QuicChromiumClientSessionTest, GetSSLInfo1) {
MockQuicData quic_data(version_);
if (VersionUsesHttp3(version_.transport_version))
quic_data.AddWrite(SYNCHRONOUS, client_maker_.MakeInitialSettingsPacket(1));
quic_data.AddRead(ASYNC, ERR_IO_PENDING);
quic_data.AddRead(ASYNC, OK); // EOF
quic_data.AddSocketDataToFactory(&socket_factory_);

Initialize();

ProofVerifyDetailsChromium details;
details.is_fatal_cert_error = false;
details.cert_verify_result.verified_cert =
ImportCertFromFile(GetTestCertsDirectory(), "spdy_pooling.pem");
details.cert_verify_result.is_issued_by_known_root = true;
details.ct_verify_result.policy_compliance =
ct::CTPolicyCompliance::CT_POLICY_COMPLIES_VIA_SCTS;
details.ct_verify_result.policy_compliance_required = true;

CompleteCryptoHandshake();
session_->OnProofVerifyDetailsAvailable(details);

SSLInfo ssl_info;
ASSERT_TRUE(session_->GetSSLInfo(&ssl_info));
EXPECT_TRUE(ssl_info.is_valid());

EXPECT_EQ(details.is_fatal_cert_error, ssl_info.is_fatal_cert_error);
EXPECT_TRUE(ssl_info.cert->EqualsIncludingChain(
details.cert_verify_result.verified_cert.get()));
EXPECT_EQ(details.cert_verify_result.cert_status, ssl_info.cert_status);
EXPECT_EQ(details.cert_verify_result.is_issued_by_known_root,
ssl_info.is_issued_by_known_root);
EXPECT_EQ(details.ct_verify_result.policy_compliance,
ssl_info.ct_policy_compliance);
EXPECT_EQ(details.ct_verify_result.policy_compliance_required,
ssl_info.ct_policy_compliance_required);
}

// Just like GetSSLInfo1, but uses different values.
TEST_P(QuicChromiumClientSessionTest, GetSSLInfo2) {
MockQuicData quic_data(version_);
if (VersionUsesHttp3(version_.transport_version))
quic_data.AddWrite(SYNCHRONOUS, client_maker_.MakeInitialSettingsPacket(1));
quic_data.AddRead(ASYNC, ERR_IO_PENDING);
quic_data.AddRead(ASYNC, OK); // EOF
quic_data.AddSocketDataToFactory(&socket_factory_);

Initialize();

ProofVerifyDetailsChromium details;
details.is_fatal_cert_error = false;
details.cert_verify_result.verified_cert =
ImportCertFromFile(GetTestCertsDirectory(), "spdy_pooling.pem");
details.cert_verify_result.is_issued_by_known_root = false;
details.ct_verify_result.policy_compliance =
ct::CTPolicyCompliance::CT_POLICY_NOT_ENOUGH_SCTS;
details.ct_verify_result.policy_compliance_required = false;

CompleteCryptoHandshake();
session_->OnProofVerifyDetailsAvailable(details);

SSLInfo ssl_info;
ASSERT_TRUE(session_->GetSSLInfo(&ssl_info));
EXPECT_TRUE(ssl_info.is_valid());

EXPECT_EQ(details.is_fatal_cert_error, ssl_info.is_fatal_cert_error);
EXPECT_TRUE(ssl_info.cert->EqualsIncludingChain(
details.cert_verify_result.verified_cert.get()));
EXPECT_EQ(details.cert_verify_result.cert_status, ssl_info.cert_status);
EXPECT_EQ(details.cert_verify_result.is_issued_by_known_root,
ssl_info.is_issued_by_known_root);
EXPECT_EQ(details.ct_verify_result.policy_compliance,
ssl_info.ct_policy_compliance);
EXPECT_EQ(details.ct_verify_result.policy_compliance_required,
ssl_info.ct_policy_compliance_required);
}

TEST_P(QuicChromiumClientSessionTest, IsFatalErrorNotSetForNonFatalError) {
MockQuicData quic_data(version_);
if (VersionUsesHttp3(version_.transport_version))
Expand Down Expand Up @@ -1539,6 +1620,81 @@ TEST_P(QuicChromiumClientSessionTest, CanPool) {
}
}

TEST_P(QuicChromiumClientSessionTest, CanPoolExpectCT) {
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeatures(
/* enabled_features */
{TransportSecurityState::kDynamicExpectCTFeature,
features::kPartitionExpectCTStateByNetworkIsolationKey},
/* disabled_features */
{});

NetworkIsolationKey network_isolation_key =
NetworkIsolationKey::CreateTransient();
// Need to create a session key after setting
// kPartitionExpectCTStateByNetworkIsolationKey, otherwise, it will ignore the
// NetworkIsolationKey value.
session_key_ = QuicSessionKey(
kServerHostname, kServerPort, PRIVACY_MODE_DISABLED, SocketTag(),
network_isolation_key, false /* disable_secure_dns */);

// Need to create this after enabling
// kPartitionExpectCTStateByNetworkIsolationKey.
transport_security_state_ = std::make_unique<TransportSecurityState>();

MockQuicData quic_data(version_);
if (VersionUsesHttp3(version_.transport_version))
quic_data.AddWrite(SYNCHRONOUS, client_maker_.MakeInitialSettingsPacket(1));
quic_data.AddRead(ASYNC, ERR_IO_PENDING);
quic_data.AddRead(ASYNC, OK); // EOF
quic_data.AddSocketDataToFactory(&socket_factory_);
Initialize();

// Load a cert that is valid for:
// www.example.org
// mail.example.org
// www.example.com

// Details with a CT error.
ProofVerifyDetailsChromium details;
details.cert_verify_result.verified_cert =
ImportCertFromFile(GetTestCertsDirectory(), "spdy_pooling.pem");
ASSERT_TRUE(details.cert_verify_result.verified_cert.get());
details.cert_verify_result.is_issued_by_known_root = true;
details.ct_verify_result.policy_compliance =
ct::CTPolicyCompliance::CT_POLICY_NOT_ENOUGH_SCTS;

CompleteCryptoHandshake();
session_->OnProofVerifyDetailsAvailable(details);

// Pooling succeeds if CT isn't required.
EXPECT_TRUE(session_->CanPool("www.example.org", PRIVACY_MODE_DISABLED,
SocketTag(), network_isolation_key,
false /* disable_secure_dns */));

// Adding Expect-CT data for different NetworkIsolationKeys should have no
// effect.
base::Time expiry = base::Time::Now() + base::TimeDelta::FromDays(1);
transport_security_state_->AddExpectCT(
"www.example.org", expiry, true /* enforce */, GURL() /* report_url */,
NetworkIsolationKey::CreateTransient());
transport_security_state_->AddExpectCT(
"www.example.org", expiry, true /* enforce */, GURL() /* report_url */,
NetworkIsolationKey());
EXPECT_TRUE(session_->CanPool("www.example.org", PRIVACY_MODE_DISABLED,
SocketTag(), network_isolation_key,
false /* disable_secure_dns */));

// Adding Expect-CT data for the same NetworkIsolationKey should prevent
// pooling.
transport_security_state_->AddExpectCT(
"www.example.org", expiry, true /* enforce */, GURL() /* report_url */,
network_isolation_key);
EXPECT_FALSE(session_->CanPool("www.example.org", PRIVACY_MODE_DISABLED,
SocketTag(), network_isolation_key,
false /* disable_secure_dns */));
}

// Much as above, but uses a non-empty NetworkIsolationKey.
TEST_P(QuicChromiumClientSessionTest, CanPoolWithNetworkIsolationKey) {
base::test::ScopedFeatureList feature_list;
Expand Down Expand Up @@ -1629,7 +1785,7 @@ TEST_P(QuicChromiumClientSessionTest, ConnectionNotPooledWithDifferentPin) {
quic_data.AddSocketDataToFactory(&socket_factory_);
Initialize();

transport_security_state_.EnableStaticPinsForTesting();
transport_security_state_->EnableStaticPinsForTesting();

ProofVerifyDetailsChromium details;
details.cert_verify_result.verified_cert =
Expand Down Expand Up @@ -1661,7 +1817,7 @@ TEST_P(QuicChromiumClientSessionTest, ConnectionPooledWithMatchingPin) {
quic_data.AddSocketDataToFactory(&socket_factory_);
Initialize();

transport_security_state_.EnableStaticPinsForTesting();
transport_security_state_->EnableStaticPinsForTesting();

ProofVerifyDetailsChromium details;
details.cert_verify_result.verified_cert =
Expand Down
70 changes: 60 additions & 10 deletions net/spdy/spdy_network_transaction_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "net/http/http_response_info.h"
#include "net/http/http_server_properties.h"
#include "net/http/http_transaction_test_util.h"
#include "net/http/transport_security_state.h"
#include "net/log/net_log_event_type.h"
#include "net/log/net_log_with_source.h"
#include "net/log/test_net_log.h"
Expand Down Expand Up @@ -411,7 +412,8 @@ class SpdyNetworkTransactionTest : public TestWithTaskEnvironment {
SpdySessionKey key(HostPortPair::FromURL(request_.url),
ProxyServer::Direct(), PRIVACY_MODE_DISABLED,
SpdySessionKey::IsProxySession::kFalse, SocketTag(),
NetworkIsolationKey(), false /* disable_secure_dns */);
request_.network_isolation_key,
false /* disable_secure_dns */);
HttpNetworkSession* session = helper.session();
base::WeakPtr<SpdySession> spdy_session =
session->spdy_session_pool()->FindAvailableSession(
Expand Down Expand Up @@ -6414,28 +6416,53 @@ struct PushUrlTestParams {
const char* url_to_fetch;
const char* url_to_push;
bool client_cert_sent;
bool expect_ct_error;
SpdyPushedStreamFate expected_fate;
} push_url_test_cases[] = {
// http scheme cannot be pushed (except by trusted proxy).
{"https://www.example.org/foo.html", "http://www.example.org/foo.js", false,
{"https://www.example.org/foo.html", "http://www.example.org/foo.js",
false /* client_cert_sent */, false /* expect_ct_error */,
SpdyPushedStreamFate::kNonHttpsPushedScheme},
// ftp scheme cannot be pushed.
{"https://www.example.org/foo.html", "ftp://www.example.org/foo.js", false,
{"https://www.example.org/foo.html", "ftp://www.example.org/foo.js",
false /* client_cert_sent */, false /* expect_ct_error */,
SpdyPushedStreamFate::kInvalidUrl},
// Cross subdomain, certificate not valid.
{"https://www.example.org/foo.html", "https://blat.www.example.org/foo.js",
false, SpdyPushedStreamFate::kCertificateMismatch},
false /* client_cert_sent */, false /* expect_ct_error */,
SpdyPushedStreamFate::kCertificateMismatch},
// Cross domain, certificate not valid.
{"https://www.example.org/foo.html", "https://www.foo.com/foo.js", false,
{"https://www.example.org/foo.html", "https://www.foo.com/foo.js",
false /* client_cert_sent */, false /* expect_ct_error */,
SpdyPushedStreamFate::kCertificateMismatch},
// Cross domain, certificate valid, but cross-origin push is rejected on a
// connection with client certificate.
{"https://www.example.org/foo.html", "https://mail.example.org/foo.js",
true, SpdyPushedStreamFate::kCertificateMismatch}};
true /* client_cert_sent */, false /* expect_ct_error */,
SpdyPushedStreamFate::kCertificateMismatch},
// Cross domain, certificate valid, but cross-origin push is rejected on a
// connection with an Expect-CT error.
{"https://www.example.org/foo.html", "https://mail.example.org/foo.js",
false /* client_cert_sent */, true /* expect_ct_error */,
SpdyPushedStreamFate::kCertificateMismatch}};

class SpdyNetworkTransactionPushUrlTest
: public SpdyNetworkTransactionTest,
public ::testing::WithParamInterface<PushUrlTestParams> {
public:
SpdyNetworkTransactionPushUrlTest() {
// Set features needed for the |expect_ct_error| case, where it's important
// to check that NetworkIsolationKeys are respected.
feature_list_.InitWithFeatures(
/* enabled_features */
{TransportSecurityState::kDynamicExpectCTFeature,
features::kPartitionExpectCTStateByNetworkIsolationKey,
features::kPartitionConnectionsByNetworkIsolationKey,
features::kPartitionSSLSessionsByNetworkIsolationKey},
/* disabled_features */
{});
}

protected:
// In this test we want to verify that we can't accidentally push content
// which can't be pushed by this content server.
Expand Down Expand Up @@ -6478,6 +6505,9 @@ class SpdyNetworkTransactionPushUrlTest
SequencedSocketData data(reads, writes);

request_.url = GURL(GetParam().url_to_fetch);
// Set a NetworkIsolationKey for the |expect_ct_error| case, to make sure
// NetworkIsolationKeys are respected.
request_.network_isolation_key = NetworkIsolationKey::CreateTransient();

// Enable cross-origin push. Since we are not using a proxy, this should
// not actually enable cross-origin SPDY push.
Expand All @@ -6487,15 +6517,33 @@ class SpdyNetworkTransactionPushUrlTest
"https://123.45.67.89:443", net::ProxyServer::SCHEME_HTTP));
session_deps->proxy_resolution_service->SetProxyDelegate(
proxy_delegate.get());
NormalSpdyTransactionHelper helper(request_, DEFAULT_PRIORITY, log_,
std::move(session_deps));

helper.RunPreTestSetup();

auto ssl_provider = std::make_unique<SSLSocketDataProvider>(ASYNC, OK);
ssl_provider->ssl_info.client_cert_sent = GetParam().client_cert_sent;
ssl_provider->ssl_info.cert =
ImportCertFromFile(GetTestCertsDirectory(), "spdy_pooling.pem");
if (GetParam().expect_ct_error) {
ssl_provider->ssl_info.ct_policy_compliance =
ct::CTPolicyCompliance::CT_POLICY_NOT_ENOUGH_SCTS;
ssl_provider->ssl_info.is_issued_by_known_root = true;

session_deps->transport_security_state->AddExpectCT(
"mail.example.org",
base::Time::Now() + base::TimeDelta::FromDays(1) /* expiry */, true,
GURL(), request_.network_isolation_key);
}

NormalSpdyTransactionHelper helper(request_, DEFAULT_PRIORITY, log_,
std::move(session_deps));

helper.RunPreTestSetup();

if (GetParam().expect_ct_error) {
ssl_provider->ssl_info.ct_policy_compliance =
ct::CTPolicyCompliance::CT_POLICY_NOT_ENOUGH_SCTS;
ssl_provider->ssl_info.is_issued_by_known_root = true;
}

helper.AddDataWithSSLSocketDataProvider(&data, std::move(ssl_provider));

HttpNetworkTransaction* trans = helper.trans();
Expand Down Expand Up @@ -6533,6 +6581,8 @@ class SpdyNetworkTransactionPushUrlTest
1);
histogram_tester.ExpectTotalCount("Net.SpdyPushedStreamFate", 1);
}

base::test::ScopedFeatureList feature_list_;
};

INSTANTIATE_TEST_SUITE_P(All,
Expand Down
Loading

0 comments on commit c74a926

Please sign in to comment.