[Private Network Access] Introduce preflight policies. [chromium/src : main]
Titouan Rigoudy (Gerrit)
Attention is currently required from: Takashi Toyoshima.
Titouan Rigoudy would like Takashi Toyoshima to review this change.
[Private Network Access] Introduce preflight policies.
This change:
* Adds the `kPreflightWarn` and `kPreflighBlock` private network request
policies.
* Introduces two new feature flags controlling the rollout of the new
policies.
* Modifies `network::PrivateNetworkAccessCheck()` to handle the new
policies.
* Adds unit tests for `network::URLLoader` to check this handling.
* Adds browser tests to
content/browser/renderer_host/private_network_access_browsertest.cc
Fixed: chromium:1251963
Change-Id: Ice0990bc8caa47977846e70a4e63c5dfe01add28
---
M services/network/public/mojom/client_security_state.mojom
M third_party/blink/renderer/core/inspector/inspector_network_agent.cc
M content/public/common/content_features.h
M services/network/private_network_access_check.h
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M services/network/public/cpp/private_network_access_check_result.cc
M services/network/url_loader_unittest.cc
M content/public/common/content_features.cc
M tools/metrics/histograms/enums.xml
M services/network/public/cpp/private_network_access_check_result.h
M services/network/private_network_access_check.cc
M content/browser/renderer_host/private_network_access_util.cc
M services/network/public/mojom/cors.mojom
M content/browser/renderer_host/private_network_access_browsertest.cc
M third_party/blink/renderer/platform/loader/cors/cors_error_string.cc
M content/browser/devtools/protocol/network_handler.cc
16 files changed, 710 insertions(+), 81 deletions(-)
To view, visit change 3179212. To unsubscribe, or for help writing mail filters, visit settings.
Titouan Rigoudy (Gerrit)
Attention is currently required from: Takashi Toyoshima.
Matt Menke (Gerrit)
Attention is currently required from: Titouan Rigoudy, Takashi Toyoshima.
1 comment:
Patchset:
To view, visit change 3179212. To unsubscribe, or for help writing mail filters, visit settings.
Takashi Toyoshima (Gerrit)
Attention is currently required from: Titouan Rigoudy.
9 comments:
File content/browser/renderer_host/private_network_access_util.cc:
Patch Set #4, Line 26:
kAllowjust in case, is this intentional to drop kWarn case for !IsEnabled(kWarnAboutSecurePrivateNetworkRequests) && is_web_secure_context?
Patch Set #4, Line 66:
!policies.is_web_secure_contextIs this really an invert of is_web_secure_context? Polarity seems inverted from the original check at line 19.
File content/public/common/content_features.h:
CONTENT_EXPORT extern const base::Feature kPrivateNetworkAccessSendPreflights;
CONTENT_EXPORT extern const base::Feature
kPrivateNetworkAccessEnforcePreflights;dict order?
File content/public/common/content_features.cc:
Patch Set #4, Line 591:
// Enables sending CORS preflight requests ahead of private network requests.Is sending preflight was the original behavior? If so, .._ENABLED_BY_DEFAULT seems preferable.
Patch Set #4, Line 601:
EnforcePreflightsMy first impression of EnforcePreflights is to enforce to send preflight.
How about FollowPreflightResponses, RespectPreflightResults, or so?
File services/network/private_network_access_check.cc:
Patch Set #4, Line 71:
UMA_HISTOGRAM_ENUMERATIONIs this really performance critical? Otherwise, base::UmaHistogramEnumeration was fine enough.
File services/network/public/cpp/private_network_access_check_result.cc:
case Result::kBlockedByPolicyPreflightWarn:
case Result::kBlockedByPolicyPreflightBlock:
return CorsError::kUnexpectedPrivateNetworkAccess;may place at the last case after kBlockedByTargetIpAddressSpace to follow enum order for better readability.
File services/network/public/mojom/cors.mojom:
// - `kUnexpectedPrivateNetworkAccess`
// - `kInvalidPrivateNetworkAccess`
// - `kInsecurePrivateNetwork`Maybe list them in the same order with enum is better if there is no reason?
// - `kInsecurePrivateNetwork`
// - `kInvalidPrivateNetworkAccess`
// - `kUnexpectedPrivateNetworkAccess`
File third_party/blink/renderer/core/inspector/inspector_network_agent.cc:
case network::mojom::CorsError::kUnexpectedPrivateNetworkAccess:
return protocol::Network::CorsErrorEnum::UnexpectedPrivateNetworkAccess;
better to be after InvalidPrivateNetworkAccess?
To view, visit change 3179212. To unsubscribe, or for help writing mail filters, visit settings.
Titouan Rigoudy (Gerrit)
Attention is currently required from: Takashi Toyoshima.
9 comments:
File content/browser/renderer_host/private_network_access_util.cc:
Patch Set #4, Line 26:
kAllow
just in case, is this intentional to drop kWarn case for !IsEnabled(kWarnAboutSecurePrivateNetworkRe […]
I think you mean IsEnabled(kWarn...) && is_web_secure_context, in which case see my comment below.
Patch Set #4, Line 66:
!policies.is_web_secure_context
Is this really an invert of is_web_secure_context? Polarity seems inverted from the original check a […]
It does seem that way, but it's because of the early return style used here. Note also the || instead of &&.
We used to return kWarn if:
is_web_secure_context &&
IsEnabled(kWarnAboutSecure...) &&
on L19. Now we would fall through all the previous ifs until L66 (provided the new preflight features are not enabled), at which point we would return kWarn on L69.If the new preflight features are enabled, however, then they would apply and we would return kPreflightBlock or kPreflightWarn.
There are tests for all this in content/browser/renderer_host/private_network_access_browsertest.cc, dating back to the time when this check lived right inside RenderFrameHostImpl. It might be time for some unit tests here instead...
File content/public/common/content_features.h:
CONTENT_EXPORT extern const base::Feature kPrivateNetworkAccessSendPreflights;
CONTENT_EXPORT extern const base::Feature
kPrivateNetworkAccessEnforcePreflights;dict order?
Done
File content/public/common/content_features.cc:
Patch Set #4, Line 591:
// Enables sending CORS preflight requests ahead of private network requests.Is sending preflight was the original behavior? If so, .._ENABLED_BY_DEFAULT seems preferable.
It was not. We do not have the code in place to send preflights for private network requests yet, though it should be the next CL I send you 😊
Patch Set #4, Line 601:
EnforcePreflights
My first impression of EnforcePreflights is to enforce to send preflight. […]
Right, I was trying to be too terse with my single-verb form... I like your RespectPreflightResults suggestion!
File services/network/private_network_access_check.cc:
Patch Set #4, Line 71:
UMA_HISTOGRAM_ENUMERATION
This is called on every connection obtained during a load, so I thought that the suggestion here [1] applied:
when recording metrics on the critical path (e.g. called in a loop or logged multiple times per second), use the macros
This can be logged multiple times a second, so I thought maybe I should use a macro, but I don't feel strongly about it. Happy to defer to your expertise here!
File services/network/public/cpp/private_network_access_check_result.cc:
case Result::kBlockedByPolicyPreflightWarn:
case Result::kBlockedByPolicyPreflightBlock:
return CorsError::kUnexpectedPrivateNetworkAccess;
may place at the last case after kBlockedByTargetIpAddressSpace to follow enum order for better read […]
Done
File services/network/public/mojom/cors.mojom:
// - `kUnexpectedPrivateNetworkAccess`
// - `kInvalidPrivateNetworkAccess`
// - `kInsecurePrivateNetwork`
Maybe list them in the same order with enum is better if there is no reason? […]
Done
File third_party/blink/renderer/core/inspector/inspector_network_agent.cc:
case network::mojom::CorsError::kUnexpectedPrivateNetworkAccess:
return protocol::Network::CorsErrorEnum::UnexpectedPrivateNetworkAccess;
better to be after InvalidPrivateNetworkAccess?
Done
To view, visit change 3179212. To unsubscribe, or for help writing mail filters, visit settings.
Takashi Toyoshima (Gerrit)
Attention is currently required from: Titouan Rigoudy.
Patch set 5:Code-Review +1
3 comments:
File content/browser/renderer_host/private_network_access_util.cc:
Patch Set #4, Line 26:
kAllow
I think you mean IsEnabled(kWarn...) && is_web_secure_context, in which case see my comment below.
Ack
Patch Set #4, Line 66:
!policies.is_web_secure_context
It does seem that way, but it's because of the early return style used here. […]
thank you for your all explanation.
yep, this kind of condition checks would necessary be too complex, and having tests sound a right approach.
File services/network/private_network_access_check.cc:
Patch Set #4, Line 71:
UMA_HISTOGRAM_ENUMERATION
This is called on every connection obtained during a load, so I thought that the suggestion here [1] […]
Hum. This could be hit multiple times per second in some busy scenarios, but compared to other operations per one request (e.g. connection establishment), the time to take the histogram seems ignorably short. So, my impression is function is still fine. But it isn't a strong opinion, and feel free to decide the direction. Both would be possible.
To view, visit change 3179212. To unsubscribe, or for help writing mail filters, visit settings.
Titouan Rigoudy (Gerrit)
Attention is currently required from: Arthur Sonzogni, Andrey Kosyakov, Robert Sesek.
Titouan Rigoudy would like Arthur Sonzogni, Andrey Kosyakov and Robert Sesek to review this change.
16 files changed, 710 insertions(+), 79 deletions(-)
Titouan Rigoudy (Gerrit)
Attention is currently required from: Arthur Sonzogni, Andrey Kosyakov, Robert Sesek.
2 comments:
Patchset:
Thanks for the review! Sending onwards to devtools, content and security OWNERS.
File services/network/private_network_access_check.cc:
Patch Set #4, Line 71:
UMA_HISTOGRAM_ENUMERATION
Hum. […]
Ok! Reverted to a function then, optimizing for code size seems preferable.
To view, visit change 3179212. To unsubscribe, or for help writing mail filters, visit settings.
Robert Sesek (Gerrit)
Attention is currently required from: Arthur Sonzogni, Andrey Kosyakov, Titouan Rigoudy.
Patch set 6:Code-Review +1
Andrey Kosyakov (Gerrit)
Attention is currently required from: Arthur Sonzogni, Titouan Rigoudy.
Patch set 6:Code-Review +1
2 comments:
Patchset:
devtools lgtm with comment
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #6, Line 4921:
UnexpectedPrivateNetworkAccessLet's start documenting these -- could you please add comments to the above three that explain when each occurs and highlight the differences. Something along the lines of cors_error_strings messages would be great.
To view, visit change 3179212. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Titouan Rigoudy.
Patch set 6:Code-Review +1
3 comments:
Patchset:
LGTM % one nit below:
File content/browser/renderer_host/private_network_access_browsertest.cc:
Could you put a bug ID here.
Patch Set #6, Line 2623:
running afoul ofNote: I learned new English words.
To view, visit change 3179212. To unsubscribe, or for help writing mail filters, visit settings.
Titouan Rigoudy (Gerrit)
Patch set 7:Commit-Queue +2
4 comments:
Patchset:
Thanks everyone for the reviews!
File content/browser/renderer_host/private_network_access_browsertest.cc:
Could you put a bug ID here.
Ha! Good catch, thanks. Done.
Patch Set #6, Line 2623:
running afoul ofNote: I learned new English words.
😊
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #6, Line 4921:
UnexpectedPrivateNetworkAccess
Let's start documenting these -- could you please add comments to the above three that explain when […]
Done
To view, visit change 3179212. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/browser/renderer_host/private_network_access_browsertest.cc
Insertions: 8, Deletions: 6.
@@ -2616,9 +2616,10 @@
// Check that the page can load a local resource.
//
- // TODO(https://crbug.com/XXX): The page should be able to load this resource,
- // since in this feature flag configuration we should send CORS preflight
- // requests but not gate the actual request based on the preflight's outcome.
+ // TODO(https://crbug.com/1256775): The page should be able to load this
+ // resource, since in this feature flag configuration we should send CORS
+ // preflight requests but not gate the actual request based on the preflight's
+ // outcome.
//
// We load the resource from a secure origin to avoid running afoul of mixed
// content restrictions.
@@ -2666,9 +2667,10 @@
// Check that the page can load a local resource.
//
- // TODO(https://crbug.com/XXX): The page should be able to load this resource,
- // since in this feature flag configuration we should send CORS preflight
- // requests but not gate the actual request based on the preflight's outcome.
+ // TODO(https://crbug.com/1256775): The page should be able to load this
+ // resource, since in this feature flag configuration we should send CORS
+ // preflight requests but not gate the actual request based on the preflight's
+ // outcome.
//
// We load it from a secure origin to avoid running afoul of mixed content
// restrictions.
```
```
The name of the file: third_party/blink/public/devtools_protocol/browser_protocol.pdl
Insertions: 5, Deletions: 0.
@@ -4916,8 +4916,13 @@
MethodDisallowedByPreflightResponse
HeaderDisallowedByPreflightResponse
RedirectContainsCredentials
+ # Request was a private network request initiated by a non-secure context.
InsecurePrivateNetwork
+ # Request carried a target IP address space property that did not match
+ # the target resource's address space.
InvalidPrivateNetworkAccess
+ # Request was a private network request yet did not carry a target IP
+ # address space.
UnexpectedPrivateNetworkAccess
NoCorsRedirectModeNotFollow
```
[Private Network Access] Introduce preflight policies.
This change:
* Adds the `kPreflightWarn` and `kPreflighBlock` private network request
policies.
* Introduces two new feature flags controlling the rollout of the new
policies.
* Modifies `network::PrivateNetworkAccessCheck()` to handle the new
policies.
* Adds unit tests for `network::URLLoader` to check this handling.
* Adds browser tests to
content/browser/renderer_host/private_network_access_browsertest.cc
Fixed: chromium:1251963
Change-Id: Ice0990bc8caa47977846e70a4e63c5dfe01add28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3179212
Commit-Queue: Titouan Rigoudy <tit...@chromium.org>
Reviewed-by: Robert Sesek <rse...@chromium.org>
Reviewed-by: Andrey Kosyakov <ca...@chromium.org>
Reviewed-by: Arthur Sonzogni <arthurs...@chromium.org>
Reviewed-by: Takashi Toyoshima <toyo...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#928163}
---
M services/network/public/mojom/client_security_state.mojom
M third_party/blink/renderer/core/inspector/inspector_network_agent.cc
M content/public/common/content_features.h
M services/network/private_network_access_check.h
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M services/network/public/cpp/private_network_access_check_result.cc
M services/network/url_loader_unittest.cc
M content/public/common/content_features.cc
M tools/metrics/histograms/enums.xml
M services/network/public/cpp/private_network_access_check_result.h
M services/network/private_network_access_check.cc
M content/browser/renderer_host/private_network_access_util.cc
M services/network/public/mojom/cors.mojom
M content/browser/renderer_host/private_network_access_browsertest.cc
M third_party/blink/renderer/platform/loader/cors/cors_error_string.cc
M content/browser/devtools/protocol/network_handler.cc
16 files changed, 724 insertions(+), 79 deletions(-)