Skip to content

Commit

Permalink
Remove support for triple-keyed NAKs.
Browse files Browse the repository at this point in the history 
Any uses of triple-keyed NAKs are now double-keyed.

* Remove frame_site_ from NAK.

* Chase call chains of NAK::GetFrameSite and NAK::IsFrameSiteEnabled, replacing
  with nullopt and false and simplify from there. Remove the now-unused methods.

* Update NAK::IsDoubleKeySchemeEnabled to depend only on
  kEnableCrossSiteFlagNetworkAnonymizationKey.

* Remove all test cases for triple-keyed NAKs, replacing a triple-NIK +
  double-NAK case with a new triple-NIK + 2.5-NAK case.

* Remove the kEnableDoubleKeyNetworkAnonymizationKey feature.

Bug: 1407287
Change-Id: Ida41cad9f9e4337a69b817b4c90628333d0a1952
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4210154
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Brianna Goldstein <brgoldstein@google.com>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Commit-Queue: Dustin Mitchell <djmitche@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1102137}
  • Loading branch information
Dustin J. Mitchell authored and Chromium LUCI CQ committed Feb 7, 2023
1 parent 3b32bec commit c9d1540
Show file tree
Hide file tree
Showing 24 changed files with 145 additions and 622 deletions.
5 changes: 3 additions & 2 deletions chrome/browser/predictors/loading_predictor_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1306,8 +1306,9 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorNetworkIsolationKeyBrowserTest,
EXPECT_EQ(2u, connection_tracker()->GetAcceptedSocketCount());
EXPECT_EQ(2u, connection_tracker()->GetReadSocketCount());
} else {
// Otherwise, the preconnected socket cannot be used.
EXPECT_EQ(3u, connection_tracker()->GetAcceptedSocketCount());
// Otherwise, the preconnected socket is used, so counts remain unchanged
// since the last check.
EXPECT_EQ(2u, connection_tracker()->GetAcceptedSocketCount());
EXPECT_EQ(2u, connection_tracker()->GetReadSocketCount());
}

Expand Down
3 changes: 1 addition & 2 deletions ...e/browser/preloading/prefetch/no_state_prefetch/prerender_nostate_prefetch_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -483,8 +483,7 @@ class NoStatePrefetchBrowserTestHttpCache
if (split_cache_by_network_isolation_key) {
feature_list_.InitWithFeatures(
{net::features::kSplitCacheByNetworkIsolationKey},
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame,
net::features::kEnableDoubleKeyNetworkAnonymizationKey});
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame});
} else {
feature_list_.InitAndDisableFeature(
net::features::kSplitCacheByNetworkIsolationKey);
Expand Down
3 changes: 1 addition & 2 deletions content/browser/loader/navigation_early_hints_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,7 @@ class NavigationEarlyHintsTest : public ContentBrowserTest {
NavigationEarlyHintsTest() {
feature_list_.InitWithFeatures(
{net::features::kSplitCacheByNetworkIsolationKey},
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame,
net::features::kEnableDoubleKeyNetworkAnonymizationKey});
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame});
}
~NavigationEarlyHintsTest() override = default;

Expand Down
7 changes: 2 additions & 5 deletions content/browser/navigation_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,7 @@ class NetworkDoubleKeyIsolationNavigationBrowserTest
public:
NetworkDoubleKeyIsolationNavigationBrowserTest() {
scoped_feature_list_.InitWithFeatures(
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame,
net::features::kEnableDoubleKeyNetworkAnonymizationKey},
{});
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame}, {});
}

protected:
Expand Down Expand Up @@ -4526,8 +4524,7 @@ class NetworkIsolationSplitCacheAppendIframeOrigin
NetworkIsolationSplitCacheAppendIframeOrigin() {
feature_list_.InitWithFeatures(
{net::features::kSplitCacheByNetworkIsolationKey},
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame,
net::features::kEnableDoubleKeyNetworkAnonymizationKey});
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame});
}

private:
Expand Down
5 changes: 1 addition & 4 deletions content/browser/network/split_cache_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -393,8 +393,7 @@ class SplitCacheRegistrableDomainContentBrowserTest
SplitCacheRegistrableDomainContentBrowserTest() {
feature_list_.InitWithFeatures(
{net::features::kSplitCacheByNetworkIsolationKey},
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame,
net::features::kEnableDoubleKeyNetworkAnonymizationKey});
{net::features::kForceIsolationInfoFrameOriginToTopLevelFrame});
}

private:
Expand All @@ -412,8 +411,6 @@ class SplitCacheContentBrowserTestEnabled
std::vector<base::test::FeatureRef> disabled_features;
disabled_features.push_back(
net::features::kForceIsolationInfoFrameOriginToTopLevelFrame);
disabled_features.push_back(
net::features::kEnableDoubleKeyNetworkAnonymizationKey);
// When the test parameter is true, we test the split cache with
// PlzDedicatedWorker enabled.
if (GetParam())
Expand Down
5 changes: 2 additions & 3 deletions content/browser/web_package/web_bundle_element_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,8 @@ class WebBundleElementBrowserTest : public ContentBrowserTest {
public:
protected:
WebBundleElementBrowserTest() {
feature_list_.InitWithFeatures(
{}, {net::features::kForceIsolationInfoFrameOriginToTopLevelFrame,
net::features::kEnableDoubleKeyNetworkAnonymizationKey});
feature_list_.InitAndDisableFeature(
net::features::kForceIsolationInfoFrameOriginToTopLevelFrame);
}
~WebBundleElementBrowserTest() override = default;

Expand Down
4 changes: 0 additions & 4 deletions net/base/features.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,6 @@ BASE_FEATURE(kPartitionNelAndReportingByNetworkIsolationKey,
"PartitionNelAndReportingByNetworkIsolationKey",
base::FEATURE_DISABLED_BY_DEFAULT);

BASE_FEATURE(kEnableDoubleKeyNetworkAnonymizationKey,
"EnableDoubleKeyNetworkAnonymizationKey",
base::FEATURE_DISABLED_BY_DEFAULT);

BASE_FEATURE(kEnableCrossSiteFlagNetworkAnonymizationKey,
"EnableCrossSiteFlagNetworkAnonymizationKey",
base::FEATURE_DISABLED_BY_DEFAULT);
Expand Down
10 changes: 1 addition & 9 deletions net/base/features.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,17 +148,9 @@ NET_EXPORT BASE_DECLARE_FEATURE(kPartitionNelAndReportingByNetworkIsolationKey);
// `frame_site ` -> nullopt
// `is_cross_site` -> true if the `top_frame_site` is cross site when compared
// to the frame site. The frame site will not be stored in this key so the value
// of is_cross_site will be computed at key construction. This feature overrides
// `kEnableDoubleKeyNetworkAnonymizationKey` if both are enabled.
// of is_cross_site will be computed at key construction.
NET_EXPORT BASE_DECLARE_FEATURE(kEnableCrossSiteFlagNetworkAnonymizationKey);

// Creates a double keyed NetworkAnonymizationKey which is used to partition the
// network state. This double key will have the following properties:
// `top_frame_site` -> the schemeful site of the top level page.
// `frame_site ` -> nullopt
// `is_cross_site` -> nullopt
NET_EXPORT BASE_DECLARE_FEATURE(kEnableDoubleKeyNetworkAnonymizationKey);

// Enables sending TLS 1.3 Key Update messages on TLS 1.3 connections in order
// to ensure that this corner of the spec is exercised. This is currently
// disabled by default because we discovered incompatibilities with some
Expand Down
35 changes: 8 additions & 27 deletions net/base/isolation_info.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,22 +215,15 @@ IsolationInfo IsolationInfo::DoNotUseCreatePartialFromNak(
network_anonymization_key.GetTopFrameSite()->site_as_origin_;

absl::optional<url::Origin> frame_origin;
if (NetworkAnonymizationKey::IsFrameSiteEnabled() &&
network_anonymization_key.GetFrameSite().has_value()) {
// If frame site is set on the network anonymization key, use it to set the
// frame origin on the isolation info.
frame_origin = network_anonymization_key.GetFrameSite()->site_as_origin_;
} else if (NetworkAnonymizationKey::IsCrossSiteFlagSchemeEnabled() &&
network_anonymization_key.GetIsCrossSite().value()) {
// If frame site is not set on the network anonymization key but we know
// that it is cross site to the top level site, create an empty origin to
// use as the frame origin for the isolation info. This should be cross site
// with the top level origin.
if (NetworkAnonymizationKey::IsCrossSiteFlagSchemeEnabled() &&
network_anonymization_key.GetIsCrossSite().value()) {
// If we know that the origin is cross site to the top level site, create an
// empty origin to use as the frame origin for the isolation info. This
// should be cross site with the top level origin.
frame_origin = url::Origin();
} else {
// If frame sit is not set on the network anonymization key and we don't
// know that it's cross site to the top level site, use the top frame site
// to set the frame origin.
// If we don't know that it's cross site to the top level site, use the top
// frame site to set the frame origin.
frame_origin = top_frame_origin;
}

Expand Down Expand Up @@ -352,18 +345,6 @@ IsolationInfo::CreateNetworkAnonymizationKeyForIsolationInfo(
return NetworkAnonymizationKey();
}

// When IsolationInfo::IsFrameSiteEnabled and
// NetworkAnonymizationKey::IsFrameSiteEnabled set the `nak_frame_site` to the
// passed value. When NetworkAnonymizationKey::IsFrameSiteEnabled is false set
// the `nak_frame_site` to nullopt. When IsolationInfo::IsFrameSiteEnabled is
// false but NetworkAnonymizationKey::IsFrameSiteEnabled is true we might have
// the frame_site passed correctly to the constructor OR we might have created
// a double key in which case we cannot determine the `nak_frame_site`.
absl::optional<SchemefulSite> nak_frame_site =
NetworkAnonymizationKey::IsFrameSiteEnabled() && frame_origin.has_value()
? absl::make_optional((SchemefulSite(*frame_origin)))
: absl::nullopt;

bool nak_is_cross_site;
if (frame_origin) {
SiteForCookies site_for_cookies =
Expand All @@ -376,7 +357,7 @@ IsolationInfo::CreateNetworkAnonymizationKeyForIsolationInfo(
}

return NetworkAnonymizationKey(
SchemefulSite(*top_frame_origin), nak_frame_site,
SchemefulSite(*top_frame_origin), absl::nullopt,
absl::make_optional(nak_is_cross_site),
nonce ? absl::make_optional(*nonce) : absl::nullopt);
}
Expand Down
73 changes: 9 additions & 64 deletions net/base/isolation_info_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,35 +30,26 @@ namespace net {
// flag that enables this scheme will be enabled for testing. When the bool
// parameter is `false` the flag that enables the scheme will be disabled.
struct IsolationInfoEnabledFeatureFlagsTestingParam {
const bool enableDoubleKeyNetworkAnonymizationKey;
const bool enableDoubleKeyIsolationInfo;
const bool enableDoubleKeyAndCrossSiteBitNetworkAnonymizationKey;
};

// The three cases we need to account for:
// 0. Triple-keying is enabled for both IsolationInfo and
// 0. Double-keying is enabled for both IsolationInfo and
// NetworkAnonymizationKey.
// 1. Double-keying is enabled for both IsolationInfo and
// NetworkAnonymizationKey.
// 2. Triple-keying is enabled for IsolationInfo and double-keying is enabled
// 1. Triple-keying is enabled for IsolationInfo and double-keying is enabled
// for NetworkAnonymizationKey.
// 3. Triple-keying is enabled for IsolationInfo and double-keying +
// 2. Triple-keying is enabled for IsolationInfo and double-keying +
// cross-site-bit is enabled for NetworkAnonymizationKey.
// Note: At the current time double-keyed IsolationInfo is only supported when
// double-keying or double-keying + is cross site bit are enabled for
// NetworkAnonymizationKey.
const IsolationInfoEnabledFeatureFlagsTestingParam kFlagsParam[] = {
{/*enableDoubleKeyNetworkAnonymizationKey=*/false,
/*enableDoubleKeyIsolationInfo=*/false,
/*enableDoubleKeyAndCrossSiteBitNetworkAnonymizationKey=*/false},
{/*enableDoubleKeyNetworkAnonymizationKey=*/true,
/*enableDoubleKeyIsolationInfo=*/true,
{/*enableDoubleKeyIsolationInfo=*/true,
/*enableDoubleKeyAndCrossSiteBitNetworkAnonymizationKey=*/false},
{/*enableDoubleKeyNetworkAnonymizationKey=*/true,
/*enableDoubleKeyIsolationInfo=*/false,
{/*enableDoubleKeyIsolationInfo=*/false,
/*enableDoubleKeyAndCrossSiteBitNetworkAnonymizationKey=*/false},
{/*enableDoubleKeyNetworkAnonymizationKey=*/false,
/*enableDoubleKeyIsolationInfo=*/false,
{/*enableDoubleKeyIsolationInfo=*/false,
/*enableDoubleKeyAndCrossSiteBitNetworkAnonymizationKey=*/true}};

namespace {
Expand All @@ -79,14 +70,6 @@ class IsolationInfoTest : public testing::Test,
net::features::kForceIsolationInfoFrameOriginToTopLevelFrame);
}

if (IsDoubleKeyNetworkAnonymizationKeyEnabled()) {
enabled_features.push_back(
net::features::kEnableDoubleKeyNetworkAnonymizationKey);
} else {
disabled_features.push_back(
net::features::kEnableDoubleKeyNetworkAnonymizationKey);
}

if (IsDoubleKeyAndCrossSiteBitNetworkAnonymizationKeyEnabled()) {
enabled_features.push_back(
net::features::kEnableCrossSiteFlagNetworkAnonymizationKey);
Expand All @@ -102,10 +85,6 @@ class IsolationInfoTest : public testing::Test,
return GetParam().enableDoubleKeyIsolationInfo;
}

static bool IsDoubleKeyNetworkAnonymizationKeyEnabled() {
return GetParam().enableDoubleKeyNetworkAnonymizationKey;
}

static bool IsDoubleKeyAndCrossSiteBitNetworkAnonymizationKeyEnabled() {
return GetParam().enableDoubleKeyAndCrossSiteBitNetworkAnonymizationKey;
}
Expand Down Expand Up @@ -190,52 +169,20 @@ TEST_P(IsolationInfoTest, CreateNetworkAnonymizationKeyForIsolationInfo) {
kNonce1);
EXPECT_EQ(isolation_info.nonce().value(), kNonce1);

if (IsDoubleKeyNetworkAnonymizationKeyEnabled() &&
!IsDoubleKeyAndCrossSiteBitNetworkAnonymizationKeyEnabled() &&
if (!IsDoubleKeyAndCrossSiteBitNetworkAnonymizationKeyEnabled() &&
IsDoubleKeyIsolationInfoEnabled()) {
// Double-keyed IsolationInfo + double-keyed NetworkAnonymizationKey case.
EXPECT_DEATH_IF_SUPPORTED(nak.GetFrameSite(), "");
EXPECT_EQ(absl::nullopt, nak.GetFrameSiteForTesting());
EXPECT_EQ(kOrigin1, isolation_info.top_frame_origin());
EXPECT_DEATH_IF_SUPPORTED(isolation_info.frame_origin(), "");
EXPECT_EQ(absl::nullopt, isolation_info.frame_origin_for_testing());
EXPECT_DEATH_IF_SUPPORTED(
isolation_info.network_anonymization_key().GetFrameSite(), "");
EXPECT_EQ(
absl::nullopt,
isolation_info.network_anonymization_key().GetFrameSiteForTesting());
} else if (!IsDoubleKeyIsolationInfoEnabled() &&
!IsDoubleKeyAndCrossSiteBitNetworkAnonymizationKeyEnabled() &&
IsDoubleKeyNetworkAnonymizationKeyEnabled()) {
!IsDoubleKeyAndCrossSiteBitNetworkAnonymizationKeyEnabled()) {
// Triple-keyed IsolationInfo + double-keyed NetworkAnonymizationKey case.
EXPECT_DEATH_IF_SUPPORTED(nak.GetFrameSite(), "");
EXPECT_EQ(absl::nullopt, nak.GetFrameSiteForTesting());
EXPECT_DEATH_IF_SUPPORTED(
isolation_info.network_anonymization_key().GetFrameSite(), "");
EXPECT_EQ(
absl::nullopt,
isolation_info.network_anonymization_key().GetFrameSiteForTesting());
EXPECT_EQ(isolation_info.frame_origin(), kOrigin2);
} else if (!IsDoubleKeyIsolationInfoEnabled() &&
!IsDoubleKeyAndCrossSiteBitNetworkAnonymizationKeyEnabled() &&
!IsDoubleKeyNetworkAnonymizationKeyEnabled()) {
// Triple-keyed IsolationInfo + triple-keyed NetworkAnonymizationKey case.
EXPECT_EQ(nak.GetFrameSite(), net::SchemefulSite(kOrigin2));
EXPECT_EQ(isolation_info.network_anonymization_key().GetFrameSite(),
net::SchemefulSite(kOrigin2));
EXPECT_EQ(isolation_info.frame_origin(), kOrigin2);
} else if (!IsDoubleKeyIsolationInfoEnabled() &&
IsDoubleKeyAndCrossSiteBitNetworkAnonymizationKeyEnabled() &&
!IsDoubleKeyNetworkAnonymizationKeyEnabled()) {
IsDoubleKeyAndCrossSiteBitNetworkAnonymizationKeyEnabled()) {
// Triple-keyed IsolationInfo + double-keyed + cross site bit
// NetworkAnonymizationKey case.
EXPECT_DEATH_IF_SUPPORTED(nak.GetFrameSite(), "");
EXPECT_EQ(absl::nullopt, nak.GetFrameSiteForTesting());
EXPECT_DEATH_IF_SUPPORTED(
isolation_info.network_anonymization_key().GetFrameSite(), "");
EXPECT_EQ(
absl::nullopt,
isolation_info.network_anonymization_key().GetFrameSiteForTesting());
EXPECT_EQ(isolation_info.frame_origin(), kOrigin2);
EXPECT_EQ(isolation_info.network_anonymization_key().GetIsCrossSite(),
true);
Expand Down Expand Up @@ -707,8 +654,6 @@ TEST_P(IsolationInfoTest, CreatePartialTransient) {
isolation_info.network_anonymization_key().GetTopFrameSite());

if (IsDoubleKeyIsolationInfoEnabled()) {
EXPECT_DEATH_IF_SUPPORTED(kNIK.GetFrameSite(), "");
EXPECT_EQ(kNIK.GetFrameSiteForTesting(), absl::nullopt);
EXPECT_DEATH_IF_SUPPORTED(isolation_info.frame_origin(), "");
EXPECT_EQ(isolation_info.frame_origin_for_testing(), absl::nullopt);
} else {
Expand Down
Loading

0 comments on commit c9d1540

Please sign in to comment.