Implement net/ support for Android's NetworkSecurityPolicy

Implement net/ support for Android's NetworkSecurityPolicy

NetworkSecurityPolicy allows apps to disallow cleartext requests,
globally (starting from M) or by hostname (starting from N). This CL
adds support for this feature in URLRequestHttpJob, and a flag on the
URLRequestContext to enable it. It's off by default and only prevents
requests using the "http" and "ws" schemes. Uses will be added in future
CLs.

BUG=474197, 473848
Committed: https://crrev.com/d21d6d14ce943e47ecaaa221870bb89ae82c120b
Cr-Commit-Position: refs/heads/master@{#438661}

[mgersh, 2016-12-05 22:07:20]

Description was changed from

==========
Implement net/ support for Android's NetworkSecurityPolicy

BUG=474197, 473848
==========

to

==========
Implement net/ support for Android's NetworkSecurityPolicy

NetworkSecurityPolicy allows apps to disallow cleartext requests, globally
(starting from M) or by hostname (starting from N). This CL adds support for
this feature in URLRequestHttpJob, and a flag on the URLRequestContext to enable
it. It's off by default and only prevents requests using the "http" and "ws"
schemes. Uses will be added in future CLs.

BUG=474197, 473848
==========

[mgersh, 2016-12-05 22:08:09]

Description was changed from

==========
Implement net/ support for Android's NetworkSecurityPolicy

NetworkSecurityPolicy allows apps to disallow cleartext requests, globally
(starting from M) or by hostname (starting from N). This CL adds support for
this feature in URLRequestHttpJob, and a flag on the URLRequestContext to enable
it. It's off by default and only prevents requests using the "http" and "ws"
schemes. Uses will be added in future CLs.

BUG=474197, 473848
==========

to

==========
Implement net/ support for Android's NetworkSecurityPolicy

NetworkSecurityPolicy allows apps to disallow cleartext requests, 
globally (starting from M) or by hostname (starting from N). This CL 
adds support for this feature in URLRequestHttpJob, and a flag on the 
URLRequestContext to enable it. It's off by default and only prevents 
requests using the "http" and "ws" schemes. Uses will be added in future 
CLs.

BUG=474197, 473848
==========

[mgersh, 2016-12-05 22:08:53]

Patchset #1 (id:1) has been deleted

[mgersh, 2016-12-05 22:16:13]

mgersh@chromium.org changed reviewers:
+ pauljensen@chromium.org

[mgersh, 2016-12-05 22:16:13]

PTAL. I've discussed the net/url_request/ part of the design a bunch with mmenke
and sleevi, but sending the actual CL to you because of android stuff.

This CL just implements the underlying ability to check the
NetworkSecurityPolicy without turning it on anywhere. I've done some manual
testing with the Cronet sample app on both M and N, but no unit tests in this CL
because I haven't figured out how to do that. My plan is to modify the Cronet
sample app and its tests to test this feature once I turn it on for Cronet in a
separate CL. I can also write tests here if you have a suggestion for how to do
it.

[pauljensen, 2016-12-07 13:20:28]

This needs some tests, I think it blocks secure traffic.
Can you use setCleartextTrafficPermitted() via reflection to test?
http://androidxref.com/7.0.0_r1/xref/frameworks/base/core/java/android/securi...

What is the performance impact of this?  i.e. how much latency does it add?  You
can probably just write a benchmark that calls IsClearTextPermitted() a million
times and time it.  Do these calls result in IPC? We don't want to be blocking
the network thread for any substantial periods of time.

Please add mmenke@ or sleevi@ to comment on the net/ approach, I haven't been
involved in previous discussions.

https://codereview.chromium.org/2546213003/diff/20001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/2546213003/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:248:
@CalledByNative
@TargetApi(Build.VERSION_CODES.N)

https://codereview.chromium.org/2546213003/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:249: public
static boolean isCleartextPermitted(String host) {
needs comment

https://codereview.chromium.org/2546213003/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:249: public
static boolean isCleartextPermitted(String host) {
private

https://codereview.chromium.org/2546213003/diff/20001/net/android/network_lib...
File net/android/network_library.h (right):

https://codereview.chromium.org/2546213003/diff/20001/net/android/network_lib...
net/android/network_library.h:48: bool IsCleartextPermitted(const std::string&
host);
add comment

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_context.h:233: void
set_check_cleartext_permitted(bool check_cleartext_permitted) {
these functions need comments

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_context.h:278: // Only used on Android.
policy->system policy

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:175: net::URLRequestJob*
MaybeInternallyRedirectOrFail(
needs a comment

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:196:
!net::android::IsCleartextPermitted(url.host()))
I don't see a check for https...this really needs some tests

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:198: net::ERR_BLOCKED_BY_CLIENT);
multi-line if clauses should have curly braces

[mgersh, 2016-12-13 17:02:57]

mgersh@chromium.org changed reviewers:
+ mmenke@chromium.org

[mgersh, 2016-12-13 17:02:58]

We discussed this in person, but so it's written down:
- Making 100 calls to
NetworkSecurityPolicy.getInstance().isCleartextTrafficPermitted() generally took
0-5 ms, occasionally up to 10 ms. That's around 50 microseconds per call.
- The test I'm adding here tests the M code path, not the N one because the
Android internals make it way more complicated to do in a similar way. I'll add
a test of the per-hostname version to the Cronet sample app once I turn the
feature on for Cronet.

https://codereview.chromium.org/2546213003/diff/20001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/2546213003/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:248:
@CalledByNative
On 2016/12/07 13:20:28, pauljensen wrote:
> @TargetApi(Build.VERSION_CODES.N)

Done.

https://codereview.chromium.org/2546213003/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:249: public
static boolean isCleartextPermitted(String host) {
On 2016/12/07 13:20:28, pauljensen wrote:
> private

Done.

https://codereview.chromium.org/2546213003/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:249: public
static boolean isCleartextPermitted(String host) {
On 2016/12/07 13:20:28, pauljensen wrote:
> needs comment

Done.

https://codereview.chromium.org/2546213003/diff/20001/net/android/network_lib...
File net/android/network_library.h (right):

https://codereview.chromium.org/2546213003/diff/20001/net/android/network_lib...
net/android/network_library.h:48: bool IsCleartextPermitted(const std::string&
host);
On 2016/12/07 13:20:28, pauljensen wrote:
> add comment

Done.

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_context.h:233: void
set_check_cleartext_permitted(bool check_cleartext_permitted) {
On 2016/12/07 13:20:28, pauljensen wrote:
> these functions need comments

Done.

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_context.h:278: // Only used on Android.
On 2016/12/07 13:20:28, pauljensen wrote:
> policy->system policy

Done.

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:175: net::URLRequestJob*
MaybeInternallyRedirectOrFail(
On 2016/12/07 13:20:28, pauljensen wrote:
> needs a comment

Done.

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:196:
!net::android::IsCleartextPermitted(url.host()))
On 2016/12/07 13:20:28, pauljensen wrote:
> I don't see a check for https...this really needs some tests

It's there, just not visible in the diff. I added a comment on it.

https://codereview.chromium.org/2546213003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:198: net::ERR_BLOCKED_BY_CLIENT);
On 2016/12/07 13:20:28, pauljensen wrote:
> multi-line if clauses should have curly braces

Done.

[mmenke, 2016-12-13 18:39:15]

This seems reasonable to me

https://codereview.chromium.org/2546213003/diff/40001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2546213003/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:177: net::URLRequestJob*
MaybeInternallyRedirectOrFail(
I think this does enough weird stuff that we should just inline it in
URLRequestHttpJob::Factory (Switch url.SchemeIsCryptographic() to
!url.SchemeIsCryptographic(), and do all the extra stuff in the if below)

https://codereview.chromium.org/2546213003/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:182: // Neither check applies to https
and wss requests
nit: End these comments with periods.

[mgersh, 2016-12-13 19:01:58]

https://codereview.chromium.org/2546213003/diff/40001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2546213003/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:177: net::URLRequestJob*
MaybeInternallyRedirectOrFail(
On 2016/12/13 18:39:14, mmenke wrote:
> I think this does enough weird stuff that we should just inline it in
> URLRequestHttpJob::Factory (Switch url.SchemeIsCryptographic() to
> !url.SchemeIsCryptographic(), and do all the extra stuff in the if below)

Done.

https://codereview.chromium.org/2546213003/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:182: // Neither check applies to https
and wss requests
On 2016/12/13 18:39:14, mmenke wrote:
> nit: End these comments with periods.

Done.

[mmenke, 2016-12-13 19:04:56]

net/url_request LGTM ( did not look at the test).

https://codereview.chromium.org/2546213003/diff/60001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2546213003/diff/60001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:217: net::ERR_BLOCKED_BY_CLIENT);
May want to add another error code for this.  ERR_CLEARTEXT_TRAFFIC_NOT_ALLOWED
or somesuch.

[mgersh, 2016-12-13 19:34:42]

https://codereview.chromium.org/2546213003/diff/60001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2546213003/diff/60001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:217: net::ERR_BLOCKED_BY_CLIENT);
On 2016/12/13 19:04:56, mmenke wrote:
> May want to add another error code for this. 
ERR_CLEARTEXT_TRAFFIC_NOT_ALLOWED
> or somesuch.

Done.

[pauljensen, 2016-12-14 15:31:22]

https://codereview.chromium.org/2546213003/diff/80001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/2546213003/diff/80001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:271: private
static void setUpSecurityPolicyForTesting() throws Exception {
Can we move this to a test-only file, e.g.
src/net/test/android/javatests/src/org/chromium/net/test/util/AndroidNetworkLibraryTestUtil.java

android::SetUpSecurityPolicyForTesting() can move into the test file itself or a
test util file with an _android suffix.

https://codereview.chromium.org/2546213003/diff/80001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:275:
setCleartextTrafficPermitted.invoke(NetworkSecurityPolicy.getInstance(), false);
Can we make the "false" a boolean parameter we pass up from native code, and
then we can test when it's set and when it's not set?

[mgersh, 2016-12-14 16:57:37]

https://codereview.chromium.org/2546213003/diff/80001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/2546213003/diff/80001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:271: private
static void setUpSecurityPolicyForTesting() throws Exception {
On 2016/12/14 15:31:22, pauljensen wrote:
> Can we move this to a test-only file, e.g.
>
src/net/test/android/javatests/src/org/chromium/net/test/util/AndroidNetworkLibraryTestUtil.java
> 
> android::SetUpSecurityPolicyForTesting() can move into the test file itself or
a
> test util file with an _android suffix.

Done. The native one is simple enough that if I'm not going to keep it with
other android things, I might as well just inline it in the test, so I did that.

https://codereview.chromium.org/2546213003/diff/80001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:275:
setCleartextTrafficPermitted.invoke(NetworkSecurityPolicy.getInstance(), false);
On 2016/12/14 15:31:22, pauljensen wrote:
> Can we make the "false" a boolean parameter we pass up from native code, and
> then we can test when it's set and when it's not set?

Done.

[pauljensen, 2016-12-14 18:08:05]

lgtm module comments

https://codereview.chromium.org/2546213003/diff/100001/net/android/javatests/...
File
net/android/javatests/src/org/chromium/net/AndroidNetworkLibraryTestUtil.java
(right):

https://codereview.chromium.org/2546213003/diff/100001/net/android/javatests/...
net/android/javatests/src/org/chromium/net/AndroidNetworkLibraryTestUtil.java:20:
* Helper for tests that simulates an app disallowing cleartext traffic entirely.
nit: might want to mention only works for M+

https://codereview.chromium.org/2546213003/diff/100001/net/android/network_li...
File net/android/network_library.h (right):

https://codereview.chromium.org/2546213003/diff/100001/net/android/network_li...
net/android/network_library.h:54: NET_EXPORT_PRIVATE void
SetUpSecurityPolicyForTesting();
remove

[mgersh, 2016-12-14 19:17:24]

Thanks for your help getting this working!

https://codereview.chromium.org/2546213003/diff/100001/net/android/javatests/...
File
net/android/javatests/src/org/chromium/net/AndroidNetworkLibraryTestUtil.java
(right):

https://codereview.chromium.org/2546213003/diff/100001/net/android/javatests/...
net/android/javatests/src/org/chromium/net/AndroidNetworkLibraryTestUtil.java:20:
* Helper for tests that simulates an app disallowing cleartext traffic entirely.
On 2016/12/14 18:08:05, pauljensen wrote:
> nit: might want to mention only works for M+

Done.

https://codereview.chromium.org/2546213003/diff/100001/net/android/network_li...
File net/android/network_library.h (right):

https://codereview.chromium.org/2546213003/diff/100001/net/android/network_li...
net/android/network_library.h:54: NET_EXPORT_PRIVATE void
SetUpSecurityPolicyForTesting();
On 2016/12/14 18:08:05, pauljensen wrote:
> remove

Done.

[mgersh, 2016-12-14 19:17:29]

The CQ bit was checked by mgersh@chromium.org

[mgersh, 2016-12-14 19:17:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/2546213003/#ps120001
(title: "fixes")

[commit-bot: I haz the power, 2016-12-14 19:18:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 19:22:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 19:22:59]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mgersh, 2016-12-14 19:36:21]

The CQ bit was checked by mgersh@chromium.org

[mgersh, 2016-12-14 19:36:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2546213003/#ps140001
(title: "fixes")

[commit-bot: I haz the power, 2016-12-14 19:37:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 22:08:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 22:08:18]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mgersh, 2016-12-14 22:14:05]

The CQ bit was checked by mgersh@chromium.org

[commit-bot: I haz the power, 2016-12-14 22:14:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 23:06:40]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481753645239210,
"parent_rev": "cb1a0b1d8fda13e5bccfc3bb5aa2af9183d5be0c", "commit_rev":
"5dfa85116d6be9875da0ce2d5224f8ad81f95075"}

[commit-bot: I haz the power, 2016-12-14 23:07:07]

Description was changed from

==========
Implement net/ support for Android's NetworkSecurityPolicy

NetworkSecurityPolicy allows apps to disallow cleartext requests, 
globally (starting from M) or by hostname (starting from N). This CL 
adds support for this feature in URLRequestHttpJob, and a flag on the 
URLRequestContext to enable it. It's off by default and only prevents 
requests using the "http" and "ws" schemes. Uses will be added in future 
CLs.

BUG=474197, 473848
==========

to

==========
Implement net/ support for Android's NetworkSecurityPolicy

NetworkSecurityPolicy allows apps to disallow cleartext requests, 
globally (starting from M) or by hostname (starting from N). This CL 
adds support for this feature in URLRequestHttpJob, and a flag on the 
URLRequestContext to enable it. It's off by default and only prevents 
requests using the "http" and "ws" schemes. Uses will be added in future 
CLs.

BUG=474197, 473848

Review-Url: https://codereview.chromium.org/2546213003
==========

[commit-bot: I haz the power, 2016-12-14 23:07:09]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-12-14 23:09:54]

Description was changed from

==========
Implement net/ support for Android's NetworkSecurityPolicy

NetworkSecurityPolicy allows apps to disallow cleartext requests, 
globally (starting from M) or by hostname (starting from N). This CL 
adds support for this feature in URLRequestHttpJob, and a flag on the 
URLRequestContext to enable it. It's off by default and only prevents 
requests using the "http" and "ws" schemes. Uses will be added in future 
CLs.

BUG=474197, 473848

Review-Url: https://codereview.chromium.org/2546213003
==========

to

==========
Implement net/ support for Android's NetworkSecurityPolicy

NetworkSecurityPolicy allows apps to disallow cleartext requests,
globally (starting from M) or by hostname (starting from N). This CL
adds support for this feature in URLRequestHttpJob, and a flag on the
URLRequestContext to enable it. It's off by default and only prevents
requests using the "http" and "ws" schemes. Uses will be added in future
CLs.

BUG=474197, 473848

Committed: https://crrev.com/d21d6d14ce943e47ecaaa221870bb89ae82c120b
Cr-Commit-Position: refs/heads/master@{#438661}
==========

[commit-bot: I haz the power, 2016-12-14 23:09:55]

Patchset 7 (id:??) landed as
https://crrev.com/d21d6d14ce943e47ecaaa221870bb89ae82c120b
Cr-Commit-Position: refs/heads/master@{#438661}

[mef, 2016-12-14 23:26:44]

mef@chromium.org changed reviewers:
+ mef@chromium.org

[mef, 2016-12-14 23:26:44]

Nice!

https://codereview.chromium.org/2546213003/diff/140001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2546213003/diff/140001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:214: // ERR_BLOCKED_BY_CLIENT if not.
nit: ERR_CLEARTEXT_NOT_PERMITTED, not ERR_BLOCKED_BY_CLIENT.

[mgersh, 2016-12-15 15:12:37]

https://codereview.chromium.org/2546213003/diff/140001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2546213003/diff/140001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:214: // ERR_BLOCKED_BY_CLIENT if not.
On 2016/12/14 23:26:44, mef wrote:
> nit: ERR_CLEARTEXT_NOT_PERMITTED, not ERR_BLOCKED_BY_CLIENT.

Oh, oops. Making a new CL to fix that.