Add a whitelist for QUIC hosts.

Add a whitelist for QUIC hosts.

BUG=576402
Committed: https://crrev.com/1ad82bbc44d87669926c9cffe2ba77564df420a3
Cr-Commit-Position: refs/heads/master@{#368810}

Committed: https://crrev.com/74da0e1aeef5b431bc533c2a0bb24c94fd664e3c
Cr-Commit-Position: refs/heads/master@{#369291}

[Ryan Hamilton, 2016-01-11 22:04:47]

Description was changed from

==========
Add a whitelist for QUIC hosts.

BUG=576402
==========

to

==========
Add a whitelist for QUIC hosts.

BUG=576402
==========

[Ryan Hamilton, 2016-01-11 22:06:05]

rch@chromium.org changed reviewers:
+ agl@chromium.org

[agl, 2016-01-11 22:53:37]

https://codereview.chromium.org/1580583002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1580583002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:383: return base::EndsWith(host,
".snapchat.com",
This doesn't match the bare "snapchat.com" domain, I assume. Is that a problem?

https://codereview.chromium.org/1580583002/diff/40001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1580583002/diff/40001/net/http/transport_secu...
net/http/transport_security_state.h:285: // Returns true iff there is static
(build-in) state for |host| which
s/build/built/
s/which/that/

[Ryan Hamilton, 2016-01-11 22:59:18]

https://codereview.chromium.org/1580583002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1580583002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:383: return base::EndsWith(host,
".snapchat.com",
On 2016/01/11 22:53:37, agl wrote:
> This doesn't match the bare "snapchat.com" domain, I assume. Is that a
problem?

I believe they use app.snapchat.com from the correspondence I've seen, but I'll
followup.

https://codereview.chromium.org/1580583002/diff/40001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1580583002/diff/40001/net/http/transport_secu...
net/http/transport_security_state.h:285: // Returns true iff there is static
(build-in) state for |host| which
On 2016/01/11 22:53:37, agl wrote:
> s/build/built/
> s/which/that/

Done.

[agl, 2016-01-11 23:00:23]

lgtm

[Ryan Sleevi, 2016-01-11 23:54:26]

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

[Ryan Sleevi, 2016-01-11 23:54:27]

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:376: }
nit: This file uses no braces for single-line conditionals

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:378: for (const std::string& white :
session_->params().quic_host_whitelist) {
s/white/whitelisted_host/

Just the color name doesn't seem very descriptive.

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:379: if (host == white)
Is |host| guaranteed to be normalized? What about case-folded?

Should this be a binary search? Is Linear the right thing?

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:383: return base::EndsWith(host,
".snapchat.com",
What about trailing dots? (.snapchat.com.) - do those matter?

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:384: base::CompareCase::INSENSITIVE_ASCII);
If it's normalized (line 379), then you should be able to optimize using
case-sensitive, so that you can just do a memcmp instead of having to
re-normalize host.

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:286: // references the Google pins.
This is something I really have tried hard to keep out of //net, because //net
should not really know about Google or whitelisting.

At the least, could you please add clear comments and documentation indicating
this is temporary, a layering violation, and should be removed? Otherwise, we
should try to have a talk about the design to figure out if there's a better way
to avoid this from leaking into the API contract of //net (for example, the
policy object pushed into QUIC that contains the whitelist, which itself could
then figure out policy checks like this)

[Ryan Hamilton, 2016-01-12 00:21:30]

Patchset #5 (id:80001) has been deleted

[Ryan Hamilton, 2016-01-12 00:22:46]

Switched to using unordered_set, but if you feel strongly that a sorted vector +
short string optimization is better, I'm happy to do that instead.

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:376: }
On 2016/01/11 23:54:26, Ryan Sleevi wrote:
> nit: This file uses no braces for single-line conditionals

Indeed! I had a LOG(INFO) there and when I removed it, I forgot to remove the
{}s. Doh.

Done.

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:378: for (const std::string& white :
session_->params().quic_host_whitelist) {
On 2016/01/11 23:54:27, Ryan Sleevi wrote:
> s/white/whitelisted_host/
> 
> Just the color name doesn't seem very descriptive.

Agreed. (though this code is now gone.)

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:379: if (host == white)
On 2016/01/11 23:54:27, Ryan Sleevi wrote:
> Is |host| guaranteed to be normalized? What about case-folded?
> 
> Should this be a binary search? Is Linear the right thing?

Hopefully, this will always be empty, but if it's not we should be performant :>
Switched to unordered_set which makes it constant.

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:383: return base::EndsWith(host,
".snapchat.com",
On 2016/01/11 23:54:27, Ryan Sleevi wrote:
> What about trailing dots? (.snapchat.com.) - do those matter?

they don't matter in this case.

https://codereview.chromium.org/1580583002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:384: base::CompareCase::INSENSITIVE_ASCII);
On 2016/01/11 23:54:27, Ryan Sleevi wrote:
> If it's normalized (line 379), then you should be able to optimize using
> case-sensitive, so that you can just do a memcmp instead of having to
> re-normalize host.

Done.

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:286: // references the Google pins.
On 2016/01/11 23:54:27, Ryan Sleevi wrote:
> This is something I really have tried hard to keep out of //net, because //net
> should not really know about Google or whitelisting.

Should transport_security_state_static and friends move elsewhere?

> At the least, could you please add clear comments and documentation indicating
> this is temporary, a layering violation, and should be removed? Otherwise, we
> should try to have a talk about the design to figure out if there's a better
way
> to avoid this from leaking into the API contract of //net (for example, the
> policy object pushed into QUIC that contains the whitelist, which itself could
> then figure out policy checks like this)

Hopefully clear comment added.

[Ryan Sleevi, 2016-01-12 00:29:39]

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.cc:1113: if (!result.has_pins)
BUG? You should be checking enable_static_pins_, right? You seem to be
duplicating the subset of GetStaticDomainState relevant for this function.

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.cc:1116: return
kPinsets[result.pinset_id].accepted_pins == kGoogleAcceptableCerts;
BUG? The original code defends against corruption for pinset_id (see lines
1077/1078). Why is that not necessary here? Is that above check unnecessary?

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:286: // references the Google pins.
On 2016/01/12 00:22:46, Ryan Hamilton wrote:
> Should transport_security_state_static and friends move elsewhere?

TSS is fine - it's a vendor-neutral whitelist. Inferring which vendor is more of
the bit that's gross - especially when it's the vendor shipping the browser ;)

Comment works for me - perhaps even more clear than necessary :)

[Ryan Hamilton, 2016-01-12 01:21:25]

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.cc:1113: if (!result.has_pins)
On 2016/01/12 00:29:39, Ryan Sleevi wrote:
> BUG? You should be checking enable_static_pins_, right? You seem to be
> duplicating the subset of GetStaticDomainState relevant for this function.

I think this is a feature. In the initial version of this CL, I called into
GetStaticDomainState() and observed to agl that this meant that local builds
would not have these hosts on the whitelist. He suggested that since the data
still exists, we could make use of it, so not checking enable_static_pins_ is
intentional.

https://codereview.chromium.org/1580583002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.cc:1116: return
kPinsets[result.pinset_id].accepted_pins == kGoogleAcceptableCerts;
On 2016/01/12 00:29:39, Ryan Sleevi wrote:
> BUG? The original code defends against corruption for pinset_id (see lines
> 1077/1078). Why is that not necessary here? Is that above check unnecessary?

It's defending against corruption? Interesting. I didn't catch that.
Done.

[Ryan Sleevi, 2016-01-12 01:28:06]

LGTM % nit

https://codereview.chromium.org/1580583002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl.h (right):

https://codereview.chromium.org/1580583002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl.h:121: // Returns true if QUIC is whitelisted
for |host|.
comment nit: Note about format of host (I presume it's already canonicalized in
some form? Should mention that)

[Ryan Hamilton, 2016-01-12 05:42:37]

Thanks!

https://codereview.chromium.org/1580583002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl.h (right):

https://codereview.chromium.org/1580583002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl.h:121: // Returns true if QUIC is whitelisted
for |host|.
On 2016/01/12 01:28:06, Ryan Sleevi wrote:
> comment nit: Note about format of host (I presume it's already canonicalized
in
> some form? Should mention that)

Done.

[Ryan Hamilton, 2016-01-12 05:43:10]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-01-12 05:43:11]

The patchset sent to the CQ was uploaded after l-g-t-m from agl@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1580583002/#ps140001
(title: "comment")

[commit-bot: I haz the power, 2016-01-12 05:43:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1580583002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1580583002/140001

[commit-bot: I haz the power, 2016-01-12 06:53:08]

Description was changed from

==========
Add a whitelist for QUIC hosts.

BUG=576402
==========

to

==========
Add a whitelist for QUIC hosts.

BUG=576402
==========

[commit-bot: I haz the power, 2016-01-12 06:53:10]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-01-12 06:54:16]

Description was changed from

==========
Add a whitelist for QUIC hosts.

BUG=576402
==========

to

==========
Add a whitelist for QUIC hosts.

BUG=576402

Committed: https://crrev.com/1ad82bbc44d87669926c9cffe2ba77564df420a3
Cr-Commit-Position: refs/heads/master@{#368810}
==========

[commit-bot: I haz the power, 2016-01-12 06:54:17]

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

[trchen, 2016-01-13 00:23:33]

A revert of this CL (patchset #7 id:140001) has been created in
https://codereview.chromium.org/1574423003/ by trchen@chromium.org.

The reason for reverting is: Speculative revert for Android Cronet bots failure
(CronetTestInstrumentation).

[Ryan Hamilton, 2016-01-13 16:49:57]

Description was changed from

==========
Add a whitelist for QUIC hosts.

BUG=576402

Committed: https://crrev.com/1ad82bbc44d87669926c9cffe2ba77564df420a3
Cr-Commit-Position: refs/heads/master@{#368810}
==========

to

==========
Add a whitelist for QUIC hosts.

BUG=576402

Committed: https://crrev.com/1ad82bbc44d87669926c9cffe2ba77564df420a3
Cr-Commit-Position: refs/heads/master@{#368810}
==========

[Ryan Hamilton, 2016-01-13 20:01:10]

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

[Ryan Hamilton, 2016-01-13 20:01:11]

mef: I've verified that the cronet tests pass for me locally. Can you apply the
patch and make sure they do for you, then review?

[Ryan Hamilton, 2016-01-13 21:57:38]

mef: ok, really working now based on the approach we discussed offline

[mef, 2016-01-13 22:18:15]

https://codereview.chromium.org/1580583002/diff/240001/components/cronet/andr...
File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
(right):

https://codereview.chromium.org/1580583002/diff/240001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:144:
.put("store_server_configs_in_properties", true)
nit: do we need all these options, or did you add them as an example?

https://codereview.chromium.org/1580583002/diff/240001/net/url_request/url_re...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/1580583002/diff/240001/net/url_request/url_re...
net/url_request/url_request_context_builder.h:229:
std::unordered_set<std::string> quic_host_whitelist) {
nit: should this be 'const std::unordered_set<std::string>&'?

[mef, 2016-01-13 22:30:10]

lgtm, all cronet tests pass.

[Ryan Hamilton, 2016-01-13 22:34:59]

Thanks so much!

https://codereview.chromium.org/1580583002/diff/240001/components/cronet/andr...
File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
(right):

https://codereview.chromium.org/1580583002/diff/240001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:144:
.put("store_server_configs_in_properties", true)
On 2016/01/13 22:18:15, mef wrote:
> nit: do we need all these options, or did you add them as an example?

Good point. I copied them from the previous setup, but we only need the
whitelist. Done.

https://codereview.chromium.org/1580583002/diff/240001/net/url_request/url_re...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/1580583002/diff/240001/net/url_request/url_re...
net/url_request/url_request_context_builder.h:229:
std::unordered_set<std::string> quic_host_whitelist) {
On 2016/01/13 22:18:15, mef wrote:
> nit: should this be 'const std::unordered_set<std::string>&'?

Done.

[Ryan Hamilton, 2016-01-13 22:35:04]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-01-13 22:35:04]

The patchset sent to the CQ was uploaded after l-g-t-m from agl@chromium.org,
rsleevi@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1580583002/#ps260001
(title: "fix comments")

[commit-bot: I haz the power, 2016-01-13 22:39:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1580583002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1580583002/260001

[commit-bot: I haz the power, 2016-01-14 00:47:00]

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

[commit-bot: I haz the power, 2016-01-14 00:47:01]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[Ryan Hamilton, 2016-01-14 00:52:10]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2016-01-14 00:56:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1580583002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1580583002/260001

[commit-bot: I haz the power, 2016-01-14 02:49:41]

Description was changed from

==========
Add a whitelist for QUIC hosts.

BUG=576402

Committed: https://crrev.com/1ad82bbc44d87669926c9cffe2ba77564df420a3
Cr-Commit-Position: refs/heads/master@{#368810}
==========

to

==========
Add a whitelist for QUIC hosts.

BUG=576402

Committed: https://crrev.com/1ad82bbc44d87669926c9cffe2ba77564df420a3
Cr-Commit-Position: refs/heads/master@{#368810}
==========

[commit-bot: I haz the power, 2016-01-14 02:49:42]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-01-14 02:50:33]

Description was changed from

==========
Add a whitelist for QUIC hosts.

BUG=576402

Committed: https://crrev.com/1ad82bbc44d87669926c9cffe2ba77564df420a3
Cr-Commit-Position: refs/heads/master@{#368810}
==========

to

==========
Add a whitelist for QUIC hosts.

BUG=576402

Committed: https://crrev.com/1ad82bbc44d87669926c9cffe2ba77564df420a3
Cr-Commit-Position: refs/heads/master@{#368810}

Committed: https://crrev.com/74da0e1aeef5b431bc533c2a0bb24c94fd664e3c
Cr-Commit-Position: refs/heads/master@{#369291}
==========

[commit-bot: I haz the power, 2016-01-14 02:50:34]

Patchset 13 (id:??) landed as
https://crrev.com/74da0e1aeef5b431bc533c2a0bb24c94fd664e3c
Cr-Commit-Position: refs/heads/master@{#369291}