Resolve issue from feedback for 1417363004

Resolve issue from feedback for 1417363004

Hide feature behind command line switch.
Use default URLRequestContextGetter for HTTP requests.
Use EmbeddedTestServer instead of SpawnedTestServer.

BUG=524788
Committed: https://crrev.com/67cb6fbe33e705ef9b31141e225aba1f708e0bbf
Cr-Commit-Position: refs/heads/master@{#357271}

[Vitaly Buka (NO REVIEWS), 2015-10-30 00:35:01]

Description was changed from

==========
for try bots
==========

to

==========
Addressed feedback for 1417363004

BUG=524788
==========

[Vitaly Buka (NO REVIEWS), 2015-10-30 00:35:01]

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

[Vitaly Buka (NO REVIEWS), 2015-10-30 00:51:06]

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

[Vitaly Buka (NO REVIEWS), 2015-10-30 00:53:52]

Description was changed from

==========
Addressed feedback for 1417363004

BUG=524788
==========

to

==========
Resolves issue from feedback for 1417363004

Hides feature behind command line switch. 
Uses default URLRequestContextGetter for HTTP requests.
Uses EmbeddedTestServer instead of SpawnedTestServer.

BUG=524788
==========

[Vitaly Buka (NO REVIEWS), 2015-10-30 03:44:22]

Description was changed from

==========
Resolves issue from feedback for 1417363004

Hides feature behind command line switch. 
Uses default URLRequestContextGetter for HTTP requests.
Uses EmbeddedTestServer instead of SpawnedTestServer.

BUG=524788
==========

to

==========
Resolve issue from feedback for 1417363004

Hide feature behind command line switch. 
Use default URLRequestContextGetter for HTTP requests.
Use EmbeddedTestServer instead of SpawnedTestServer.

BUG=524788
==========

[Vitaly Buka (NO REVIEWS), 2015-10-30 19:52:29]

vitalybuka@chromium.org changed reviewers:
+ alekseys@chromium.org

[Vitaly Buka (NO REVIEWS), 2015-10-30 19:52:29]

+alekseys

[Aleksey Shlyapnikov, 2015-10-30 21:40:00]

lgtm

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_http.h (right):

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http.h:68: // Privet v3 devices must
supports HTTPS. However the device has self-signed
support

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http.h:69: // certificate so normal
validation is not useful. The only know information
known

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_url_fetcher.cc (right):

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_url_fetcher.cc:157: DVLOG(1) << "Try
number: " << tries_;
Try number -> Attempt

[Vitaly Buka (NO REVIEWS), 2015-10-30 21:49:32]

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_http.h (right):

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http.h:68: // Privet v3 devices must
supports HTTPS. However the device has self-signed
On 2015/10/30 21:40:00, Aleksey Shlyapnikov wrote:
> support

Done.

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http.h:69: // certificate so normal
validation is not useful. The only know information
On 2015/10/30 21:40:00, Aleksey Shlyapnikov wrote:
> known

Done.

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_url_fetcher.cc (right):

https://codereview.chromium.org/1415653004/diff/100001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_url_fetcher.cc:157: DVLOG(1) << "Try
number: " << tries_;
On 2015/10/30 21:40:00, Aleksey Shlyapnikov wrote:
> Try number -> Attempt

Done.

[Ryan Sleevi, 2015-10-30 23:17:28]

Does this code run in the browser process? It seems to be trusting a lot of
network traffic 'as is', which is a security anti-pattern.

I left some comments, mostly trying to look at the CL in context.

I'm going to say LGTM to this CL, although in the future, much like the original
CL, it would have made more sense to break this out to separate CLs

- Switch to ETS in one CL
- Move HTTPS behind a flag in one CL
- Use default URLRequestContext in one CL

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/gcd_private/gcd_private_api.cc (right):

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/gcd_private/gcd_private_api.cc:81: if
(g_gcd_api_flow_factory) {
FWIW this bracing is inconsistent w/ the rest of this function/file

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/gcd_private/gcd_private_api.cc:227: }
more inconsistent bracing

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/gcd_private/gcd_private_api.cc:284:
GCDDeviceMap::iterator found = known_devices_.find(kIDPrefixMdns + name);
BUG: Can't found == .end() under exceptional cases here? It seems it could,
which means 285 would be a security bug.

It's taken me quite a bit of code to figure out whether or not this information
is validated or not, but looking at
//chrome/common/local_discovery/service_discovery_client_impl and the related
thread hops, it 'seems' as if you can double-advertise a device removal and
cause mischief, based on the thread-hops involved.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_http.h (right):

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http.h:74: // secret.
This comment doesn't really read naturally / grammatically correct.

"Privet v3 devices MUST support HTTPS. Before communicating over HTTPS, Privet
v3 pairing generates a shared secret using SPAKE2 over an unauthenticated
channel. This shared secret is used to then authenticate
the fingerprint of a certificate, which is self-signed. The client then
validates that the HTTPS peer is sending a certificate matching that
fingerprint.

For more information on this protocol:
https://...

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_http_impl.cc (right):

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_impl.cc:612: std::string
data_str((const char*)data_->front(), data_->size());
STYLE: This violates the style-guide rules on casting.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_impl.cc:801:
replacements.SetHostStr(host_port_.host());
BUG? Should this be HostForURL()? Do you not have any tests for IPv6?

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_http_unittest.cc (right):

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_unittest.cc:1100:
ASSERT_TRUE(network_thread_.StartWithOptions(network_thread_options));
BUG: Don't create threads like this. Use TestBrowserThread /
TestBrowserThreadBundle if you're trying to create threads for testing in
//content or //chrome that reflect 'real' things.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_unittest.cc:1112:
base::MessageLoop::current()->PostTask(FROM_HERE, quit_);
At some point, preferably in a separate CL, you should scrub these
base::MessageLoop::current() calls in //chrome/browser/local_discovery with
base::ThreadTaskRunnerHandle::Get()

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_unittest.cc:1155: auto fetcher =
client_->CreateURLFetcher(server_->GetURL("/simple.html"),
STYLE: This is not an appropriate use of auto

See the discussion on the dangers of this usage at
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5-Bt3BJzAo0/dis...
(linked from chromium-cpp.appspot.com)

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/ZW8Lm...
for some specific context

[Vitaly Buka (NO REVIEWS), 2015-10-31 03:55:18]

On 2015/10/30 23:17:28, Ryan Sleevi wrote:
> Does this code run in the browser process? 
Correct.

[Vitaly Buka (NO REVIEWS), 2015-10-31 03:55:36]

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/gcd_private/gcd_private_api.cc (right):

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/gcd_private/gcd_private_api.cc:81: if
(g_gcd_api_flow_factory) {
On 2015/10/30 23:17:27, Ryan Sleevi wrote:
> FWIW this bracing is inconsistent w/ the rest of this function/file

Done.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/gcd_private/gcd_private_api.cc:227: }
On 2015/10/30 23:17:27, Ryan Sleevi wrote:
> more inconsistent bracing

Done.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/gcd_private/gcd_private_api.cc:284:
GCDDeviceMap::iterator found = known_devices_.find(kIDPrefixMdns + name);
Thanks. Quite possible.
I suspect half of this file is irrelevant now.
crbug.com/549868

On 2015/10/30 23:17:27, Ryan Sleevi wrote:
> BUG: Can't found == .end() under exceptional cases here? It seems it could,
> which means 285 would be a security bug.
> 
> It's taken me quite a bit of code to figure out whether or not this
information
> is validated or not, but looking at
> //chrome/common/local_discovery/service_discovery_client_impl and the related
> thread hops, it 'seems' as if you can double-advertise a device removal and
> cause mischief, based on the thread-hops involved.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_http.h (right):

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http.h:74: // secret.
On 2015/10/30 23:17:27, Ryan Sleevi wrote:
> This comment doesn't really read naturally / grammatically correct.
> 
> "Privet v3 devices MUST support HTTPS. Before communicating over HTTPS, Privet
> v3 pairing generates a shared secret using SPAKE2 over an unauthenticated
> channel. This shared secret is used to then authenticate
> the fingerprint of a certificate, which is self-signed. The client then
> validates that the HTTPS peer is sending a certificate matching that
> fingerprint.
> 
> For more information on this protocol:
> https://...

Done.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_http_impl.cc (right):

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_impl.cc:612: std::string
data_str((const char*)data_->front(), data_->size());
On 2015/10/30 23:17:27, Ryan Sleevi wrote:
> STYLE: This violates the style-guide rules on casting.

Done.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_impl.cc:801:
replacements.SetHostStr(host_port_.host());
On 2015/10/30 23:17:27, Ryan Sleevi wrote:
> BUG? Should this be HostForURL()? Do you not have any tests for IPv6?
All tests with IPv4. We had IPv6 working, so i need to investigate this.
crbug.com/549870

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
File chrome/browser/local_discovery/privet_http_unittest.cc (right):

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_unittest.cc:1100:
ASSERT_TRUE(network_thread_.StartWithOptions(network_thread_options));
On 2015/10/30 23:17:27, Ryan Sleevi wrote:
> BUG: Don't create threads like this. Use TestBrowserThread /
> TestBrowserThreadBundle if you're trying to create threads for testing in
> //content or //chrome that reflect 'real' things.

Done.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_unittest.cc:1112:
base::MessageLoop::current()->PostTask(FROM_HERE, quit_);
On 2015/10/30 23:17:27, Ryan Sleevi wrote:
> At some point, preferably in a separate CL, you should scrub these
> base::MessageLoop::current() calls in //chrome/browser/local_discovery with
> base::ThreadTaskRunnerHandle::Get()

Acknowledged.

https://codereview.chromium.org/1415653004/diff/120001/chrome/browser/local_d...
chrome/browser/local_discovery/privet_http_unittest.cc:1155: auto fetcher =
client_->CreateURLFetcher(server_->GetURL("/simple.html"),
On 2015/10/30 23:17:27, Ryan Sleevi wrote:
> STYLE: This is not an appropriate use of auto
> 
> See the discussion on the dangers of this usage at
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5-Bt3BJzAo0/dis...
> (linked from http://chromium-cpp.appspot.com)
> 
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/ZW8Lm...
> for some specific context

Done.

[Vitaly Buka (NO REVIEWS), 2015-10-31 03:55:45]

The CQ bit was checked by vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2015-10-31 03:55:45]

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

[commit-bot: I haz the power, 2015-10-31 03:56:15]

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

[commit-bot: I haz the power, 2015-10-31 04:46:53]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-31 04:47:19]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-10-31 04:47:42]

Patchset 9 (id:??) landed as
https://crrev.com/67cb6fbe33e705ef9b31141e225aba1f708e0bbf
Cr-Commit-Position: refs/heads/master@{#357271}