Skip to content

Commit

Permalink
Enable NSP by default in chrome
Browse files Browse the repository at this point in the history
This CL enables network state partitioning by default in Chrome, if it's
not already overridden. The current feature-based status remains in
other embedders.

As an optimization, this lumps a number of existing features into one
`IsPartitioningEnabled` status. These separate features were useful for
testing during development, but at this point are enabled or disabled as
a group.

Bug: 1425224
Change-Id: I91962fc328dae41acff99f087456ee3bbb81e5bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4387455
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Brianna Goldstein <brgoldstein@google.com>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Dustin Mitchell <djmitche@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1133254}
  • Loading branch information
Dustin J. Mitchell authored and Chromium LUCI CQ committed Apr 20, 2023
1 parent 79ba02e commit 5624da4
Show file tree
Hide file tree
Showing 23 changed files with 110 additions and 61 deletions.
3 changes: 3 additions & 0 deletions chrome/app/chrome_main_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,9 @@ void ChromeMainDelegate::CommonEarlyInitialization() {
// it if not already overridden by command line, field trial etc.
net::HttpCache::SplitCacheFeatureEnableByDefault();

// Similarly, enable network state partitioning by default.
net::NetworkAnonymizationKey::PartitionByDefault();

#if BUILDFLAG(IS_CHROMEOS)
// Threading features.
base::PlatformThread::InitFeaturesPostFieldTrial();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1433,8 +1433,6 @@ class SignedExchangeReportingBrowserTest
feature_list_.InitWithFeatures(
// enabled_features
{net::features::kPartitionNelAndReportingByNetworkIsolationKey,
// These last two are not strictly necessary, but make this test more
// robust against enabling NetworkIsolationKeys everywhere.
net::features::kPartitionConnectionsByNetworkIsolationKey,
net::features::kPartitionSSLSessionsByNetworkIsolationKey},
// disabled_features
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -760,8 +760,6 @@ class SignedExchangeSubresourcePrefetchBrowserTest
// Needed for reporting test. Doesn't significantly impact other tests.
enable_features.push_back(
net::features::kPartitionNelAndReportingByNetworkIsolationKey);
// These last two are not strictly necessary, but make this test more robust
// against enabling NetworkIsolationKeys everywhere.
enable_features.push_back(
net::features::kPartitionConnectionsByNetworkIsolationKey);
enable_features.push_back(
Expand Down
52 changes: 52 additions & 0 deletions net/base/network_anonymization_key.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,18 @@

namespace net {

namespace {

// True if network state partitioning should be enabled regardless of feature
// settings.
bool g_partition_by_default = false;

// True if NAK::IsPartitioningEnabled has been called, and the value of
// `g_partition_by_default` cannot be changed.
bool g_partition_by_default_locked = false;

} // namespace

NetworkAnonymizationKey::NetworkAnonymizationKey(
const SchemefulSite& top_frame_site,
bool is_cross_site,
Expand Down Expand Up @@ -175,4 +187,44 @@ absl::optional<std::string> NetworkAnonymizationKey::SerializeSiteWithNonce(
return *(const_cast<SchemefulSite&>(site).SerializeWithNonce());
}

// static
bool NetworkAnonymizationKey::IsPartitioningEnabled() {
g_partition_by_default_locked = true;
return g_partition_by_default ||
base::FeatureList::IsEnabled(
features::kSplitHostCacheByNetworkIsolationKey) ||
base::FeatureList::IsEnabled(
features::kPartitionConnectionsByNetworkIsolationKey) ||
base::FeatureList::IsEnabled(
features::kPartitionHttpServerPropertiesByNetworkIsolationKey) ||
base::FeatureList::IsEnabled(
features::kPartitionSSLSessionsByNetworkIsolationKey) ||
base::FeatureList::IsEnabled(
features::kPartitionNelAndReportingByNetworkIsolationKey);
}

// static
void NetworkAnonymizationKey::PartitionByDefault() {
DCHECK(!g_partition_by_default_locked);
// Only set the global if none of the relevant features are overridden.
if (!base::FeatureList::GetInstance()->IsFeatureOverridden(
"SplitHostCacheByNetworkIsolationKey") &&
!base::FeatureList::GetInstance()->IsFeatureOverridden(
"PartitionConnectionsByNetworkIsolationKey") &&
!base::FeatureList::GetInstance()->IsFeatureOverridden(
"PartitionHttpServerPropertiesByNetworkIsolationKey") &&
!base::FeatureList::GetInstance()->IsFeatureOverridden(
"PartitionSSLSessionsByNetworkIsolationKey") &&
!base::FeatureList::GetInstance()->IsFeatureOverridden(
"PartitionNelAndReportingByNetworkIsolationKey")) {
g_partition_by_default = true;
}
}

// static
void NetworkAnonymizationKey::ClearGlobalsForTesting() {
g_partition_by_default = false;
g_partition_by_default_locked = false;
}

} // namespace net
19 changes: 19 additions & 0 deletions net/base/network_anonymization_key.h
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,25 @@ class NET_EXPORT NetworkAnonymizationKey {
const base::Value& value,
NetworkAnonymizationKey* out_network_anonymization_key);

// Determine whether network state partitioning is enabled. This is true if
// any of the features
//
// * `SplitHostCacheByNetworkIsolationKey`
// * `PartitionConnectionsByNetworkIsolationKey`
// * `PartitionHttpServerPropertiesByNetworkIsolationKey`
// * `PartitionSSLSessionsByNetworkIsolationKey`
// * `PartitionNelAndReportingByNetworkIsolationKey`
//
// is enabled, or if `PartitionByDefault()` has been called.
static bool IsPartitioningEnabled();

// Default partitioning to enabled, regardless of feature settings. This must
// be called before any calls to `IsPartitioningEnabled()`.
static void PartitionByDefault();

// Clear partitioning-related globals.
static void ClearGlobalsForTesting();

private:
NetworkAnonymizationKey(
const SchemefulSite& top_frame_site,
Expand Down
3 changes: 1 addition & 2 deletions net/dns/host_resolver_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -632,8 +632,7 @@ class HostResolverManager::RequestImpl
: source_net_log_(std::move(source_net_log)),
request_host_(std::move(request_host)),
network_anonymization_key_(
base::FeatureList::IsEnabled(
features::kSplitHostCacheByNetworkIsolationKey)
NetworkAnonymizationKey::IsPartitioningEnabled()
? std::move(network_anonymization_key)
: NetworkAnonymizationKey()),
parameters_(optional_parameters ? std::move(optional_parameters).value()
Expand Down
6 changes: 3 additions & 3 deletions net/dns/host_resolver_manager_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4274,8 +4274,8 @@ TEST_F(HostResolverManagerTest, NetworkAnonymizationKeyReadFromHostCache) {
};

// Add entries to cache for the empty NIK, NIK1, and NIK2. Only the
// HostResolverManager obeys features::kSplitHostCacheByNetworkIsolationKey,
// so this is fine to do regardless of the feature value.
// HostResolverManager obeys network state partitioning, so this is fine to do
// regardless of the feature value.
for (const auto& cache_entry : kCacheEntries) {
HostCache::Key key("just.testing", DnsQueryType::UNSPECIFIED, 0,
HostResolverSource::ANY,
Expand Down Expand Up @@ -4347,7 +4347,7 @@ TEST_F(HostResolverManagerTest, NetworkAnonymizationKeyReadFromHostCache) {
}

// Test that two requests made with different NetworkAnonymizationKeys are not
// merged if |features::kSplitHostCacheByNetworkIsolationKey| is enabled.
// merged if network state partitioning is enabled.
TEST_F(HostResolverManagerTest, NetworkAnonymizationKeyTwoRequestsAtOnce) {
const SchemefulSite kSite1(GURL("https://origin1.test/"));
const SchemefulSite kSite2(GURL("https://origin2.test/"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,13 @@ base::TaskPriority GetReportingAndNelStoreBackgroundSequencePriority() {
out_network_anonymization_key))
return false;

// If NetworkAnonymizationKeys are disabled for reporting and NEL, but the
// If network state partitionining is disabled, but the
// NetworkAnonymizationKeys is non-empty, ignore the entry. The entry will
// still be in the on-disk database, in case NAKs are re-enabled, it just
// won't be loaded into memory. The entry could still be loaded with an empty
// NetworkAnonymizationKey, but that would require logic to resolve conflicts.
if (!out_network_anonymization_key->IsEmpty() &&
!base::FeatureList::IsEnabled(
features::kPartitionNelAndReportingByNetworkIsolationKey)) {
!NetworkAnonymizationKey::IsPartitioningEnabled()) {
*out_network_anonymization_key = NetworkAnonymizationKey();
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions net/http/http_server_properties.cc
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ HttpServerProperties::HttpServerProperties(
: tick_clock_(tick_clock ? tick_clock
: base::DefaultTickClock::GetInstance()),
clock_(clock ? clock : base::DefaultClock::GetInstance()),
use_network_anonymization_key_(base::FeatureList::IsEnabled(
features::kPartitionHttpServerPropertiesByNetworkIsolationKey)),
use_network_anonymization_key_(
NetworkAnonymizationKey::IsPartitioningEnabled()),
is_initialized_(pref_delegate.get() == nullptr),
properties_manager_(
pref_delegate
Expand Down
4 changes: 2 additions & 2 deletions net/http/http_server_properties.h
Original file line number Diff line number Diff line change
Expand Up @@ -618,8 +618,8 @@ class NET_EXPORT HttpServerProperties
raw_ptr<const base::TickClock> tick_clock_; // Unowned
raw_ptr<base::Clock> clock_; // Unowned

// Cached value of kPartitionHttpServerPropertiesByNetworkIsolationKey
// feature. Cached to improve performance.
// Cached value of whether network state partitioning is enabled. Cached to
// improve performance.
const bool use_network_anonymization_key_;

// Set to true once initial properties have been retrieved from disk by
Expand Down
4 changes: 2 additions & 2 deletions net/http/http_server_properties_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,8 @@ void HttpServerPropertiesManager::ReadPrefs(
std::make_unique<HttpServerProperties::QuicServerInfoMap>(
max_server_configs_stored_in_properties_);

bool use_network_anonymization_key = base::FeatureList::IsEnabled(
features::kPartitionHttpServerPropertiesByNetworkIsolationKey);
bool use_network_anonymization_key =
NetworkAnonymizationKey::IsPartitioningEnabled();

// Iterate `servers_list` (least-recently-used item is in the front) so that
// entries are inserted into `server_info_map` from oldest to newest.
Expand Down
7 changes: 3 additions & 4 deletions net/network_error_logging/network_error_logging_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -333,10 +333,9 @@ class NetworkErrorLoggingServiceImpl : public NetworkErrorLoggingService {
// Backlog of tasks waiting on initialization.
std::vector<base::OnceClosure> task_backlog_;

// Set based on features::kPartitionNelAndReportingByNetworkIsolationKey on
// construction.
bool respect_network_anonymization_key_ = base::FeatureList::IsEnabled(
features::kPartitionNelAndReportingByNetworkIsolationKey);
// Set based on network state partitioning status on construction.
bool respect_network_anonymization_key_ =
NetworkAnonymizationKey::IsPartitioningEnabled();

base::WeakPtrFactory<NetworkErrorLoggingServiceImpl> weak_factory_{this};

Expand Down
3 changes: 1 addition & 2 deletions net/quic/quic_session_key.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ QuicSessionKey::QuicSessionKey(
: server_id_(server_id),
socket_tag_(socket_tag),
network_anonymization_key_(
base::FeatureList::IsEnabled(
features::kPartitionConnectionsByNetworkIsolationKey)
NetworkAnonymizationKey::IsPartitioningEnabled()
? network_anonymization_key
: NetworkAnonymizationKey()),
secure_dns_policy_(secure_dns_policy),
Expand Down
3 changes: 1 addition & 2 deletions net/quic/quic_stream_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1389,8 +1389,7 @@ QuicStreamFactory::QuicStreamFactory(
connectivity_monitor_(default_network_),
ssl_config_service_(ssl_config_service),
use_network_anonymization_key_for_crypto_configs_(
base::FeatureList::IsEnabled(
features::kPartitionHttpServerPropertiesByNetworkIsolationKey)) {
NetworkAnonymizationKey::IsPartitioningEnabled()) {
DCHECK(transport_security_state_);
DCHECK(http_server_properties_);
if (params_.disable_tls_zero_rtt)
Expand Down
7 changes: 2 additions & 5 deletions net/quic/quic_stream_factory_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12397,11 +12397,8 @@ TEST_P(QuicStreamFactoryTest, CryptoConfigCache) {
const char kUserAgentId[] = "spoon";

base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeatures(
// enabled_features
{features::kPartitionConnectionsByNetworkIsolationKey},
// disabled_features
{features::kPartitionHttpServerPropertiesByNetworkIsolationKey});
feature_list.InitAndDisableFeature(
features::kPartitionHttpServerPropertiesByNetworkIsolationKey);

const SchemefulSite kSite1(GURL("https://foo.test/"));
const auto kNetworkAnonymizationKey1 =
Expand Down
2 changes: 1 addition & 1 deletion net/reporting/reporting_header_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class NET_EXPORT ReportingHeaderParser {
// `reporting_source`. `network_anonymization_key` is the NIK which will be
// passed in with reports to be queued. This must match the NIK from
// `isolation_source`, unless it is empty (which will be the case if the
// kPartitionNelAndReportingByNetworkIsolationKey feature is disabled.)
// network state partitioning is disabled).
static void ProcessParsedReportingEndpointsHeader(
ReportingContext* context,
const base::UnguessableToken& reporting_source,
Expand Down
4 changes: 2 additions & 2 deletions net/reporting/reporting_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,8 @@ class ReportingServiceImpl : public ReportingService {
bool initialized_ = false;
std::vector<base::OnceClosure> task_backlog_;

bool respect_network_anonymization_key_ = base::FeatureList::IsEnabled(
features::kPartitionNelAndReportingByNetworkIsolationKey);
bool respect_network_anonymization_key_ =
NetworkAnonymizationKey::IsPartitioningEnabled();

// Allows returning a NetworkAnonymizationKey by reference when
// |respect_network_anonymization_key_| is false.
Expand Down
10 changes: 5 additions & 5 deletions net/reporting/reporting_uploader_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -625,11 +625,11 @@ TEST_F(ReportingUploaderTest, DontCacheResponse) {
// with a different one, and make sure only the requests with the same
// NetworkAnonymizationKey share a socket.
TEST_F(ReportingUploaderTest, RespectsNetworkAnonymizationKey) {
// While features::kPartitionConnectionsByNetworkIsolationKey is not needed
// for reporting code to respect NetworkAnonymizationKey, this test works by
// ensuring that Reporting's NetworkAnonymizationKey makes it to the socket
// pool layer and is respected there, so this test needs to enable
// kPartitionConnectionsByNetworkIsolationKey.
// While network state partitioning is not needed for reporting code to
// respect NetworkAnonymizationKey, this test works by ensuring that
// Reporting's NetworkAnonymizationKey makes it to the socket pool layer and
// is respected there, so this test needs to enable
// network state partitioning.
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(
features::kPartitionConnectionsByNetworkIsolationKey);
Expand Down
6 changes: 2 additions & 4 deletions net/socket/client_socket_pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ ClientSocketPool::GroupId::GroupId(
: destination_(std::move(destination)),
privacy_mode_(privacy_mode),
network_anonymization_key_(
base::FeatureList::IsEnabled(
features::kPartitionConnectionsByNetworkIsolationKey)
NetworkAnonymizationKey::IsPartitioningEnabled()
? std::move(network_anonymization_key)
: NetworkAnonymizationKey()),
secure_dns_policy_(secure_dns_policy) {
Expand Down Expand Up @@ -113,8 +112,7 @@ std::string ClientSocketPool::GroupId::ToString() const {
if (privacy_mode_)
result = "pm/" + result;

if (base::FeatureList::IsEnabled(
features::kPartitionConnectionsByNetworkIsolationKey)) {
if (NetworkAnonymizationKey::IsPartitioningEnabled()) {
result += " <";
result += network_anonymization_key_.ToDebugString();
result += ">";
Expand Down
3 changes: 1 addition & 2 deletions net/socket/ssl_client_socket_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1730,8 +1730,7 @@ SSLClientSessionCache::Key SSLClientSocketImpl::GetSessionCacheKey(
SSLClientSessionCache::Key key;
key.server = host_and_port_;
key.dest_ip_addr = dest_ip_addr;
if (base::FeatureList::IsEnabled(
features::kPartitionSSLSessionsByNetworkIsolationKey)) {
if (NetworkAnonymizationKey::IsPartitioningEnabled()) {
key.network_anonymization_key = ssl_config_.network_anonymization_key;
}
key.privacy_mode = ssl_config_.privacy_mode;
Expand Down
8 changes: 3 additions & 5 deletions net/spdy/spdy_session_key.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,9 @@ SpdySessionKey::SpdySessionKey(
is_proxy_session_(is_proxy_session),
socket_tag_(socket_tag),
network_anonymization_key_(
!base::FeatureList::IsEnabled(
features::kPartitionConnectionsByNetworkIsolationKey)
? NetworkAnonymizationKey()
: network_anonymization_key),

NetworkAnonymizationKey::IsPartitioningEnabled()
? network_anonymization_key
: NetworkAnonymizationKey()),
secure_dns_policy_(secure_dns_policy) {
// IsProxySession::kTrue should only be used with direct connections, since
// using multiple layers of proxies on top of each other isn't supported.
Expand Down
5 changes: 2 additions & 3 deletions net/spdy/spdy_session_key.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,8 @@ class NET_EXPORT_PRIVATE SpdySessionKey {
PrivacyMode privacy_mode_ = PRIVACY_MODE_DISABLED;
IsProxySession is_proxy_session_;
SocketTag socket_tag_;
// Used to separate requests made in different contexts. If
// `kPartitionConnectionsByNetworkIsolationKey` is disabled this will be set
// to an empty key.
// Used to separate requests made in different contexts. If network state
// partitioning is disabled this will be set to an empty key.
NetworkAnonymizationKey network_anonymization_key_;
SecureDnsPolicy secure_dns_policy_;
};
Expand Down
9 changes: 1 addition & 8 deletions services/network/network_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2550,14 +2550,7 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext(
// corresponding value in the URLRequestContext to true at the URLRequest
// layer if all those features are set to respect NIK.
if (require_network_isolation_key_ &&
base::FeatureList::IsEnabled(
net::features::kPartitionConnectionsByNetworkIsolationKey) &&
base::FeatureList::IsEnabled(
net::features::kPartitionHttpServerPropertiesByNetworkIsolationKey) &&
base::FeatureList::IsEnabled(
net::features::kPartitionNelAndReportingByNetworkIsolationKey) &&
base::FeatureList::IsEnabled(
net::features::kPartitionSSLSessionsByNetworkIsolationKey) &&
net::NetworkAnonymizationKey::IsPartitioningEnabled() &&
base::FeatureList::IsEnabled(
domain_reliability::features::
kPartitionDomainReliabilityByNetworkIsolationKey)) {
Expand Down

0 comments on commit 5624da4

Please sign in to comment.