Add SSL error assistant component to dynamically update captive portal list

Add SSL Error Assistant component to dynamically update captive portal list

This CL adds the SSL Error Assistant component that will dynamically update the
captive portal certificate list to be used by SSLErrorHandler. The component's
implementation is mostly taken from FileTypePolicies component.

For the time being, the component is only enabled on platforms where captive
portal detection is enabled (i.e. desktop).

BUG=640835
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2581903002
Cr-Commit-Position: refs/heads/master@{#449525}
Committed: https://chromium.googlesource.com/chromium/src/+/4404d45a6532616d24d676113d5dfb0d519085b9

[meacer, 2017-02-07 02:36:08]

Description was changed from

==========
Add initial version of TLS error assistant component.

BUG=
==========

to

==========
Add initial version of TLS error assistant component.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[meacer, 2017-02-07 23:16:33]

meacer@chromium.org changed reviewers:
+ estark@chromium.org, nparker@chromium.org

[meacer, 2017-02-07 23:16:33]

estark, nparker: PTAL?

estark: chrome/browser/ssl/*
nparker: chrome/browser/component_updater/* (this is very similar to file type
policies component)

[meacer, 2017-02-07 23:19:08]

Description was changed from

==========
Add initial version of TLS error assistant component.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add SSL Error Assistant component to dynamically update captive portal list

This CL adds the SSL Error Assistant component that will dynamically update the
captive portal certificate list to be used by SSLErrorHandler. The component's
implementation is mostly taken from FileTypePolicies component.

BUG=640835
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[estark, 2017-02-08 20:31:17]

c/b/ssl lgtm with a couple nits/questions

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:4023:
"sha256/fjZPHewEHTrMDX3I1ecEIeoy3WFxHyGplOLv28kIbtI=";
Magic value; I suggest adding a comment explaining from whence it came.

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:4066: // Set the SPKI hash to
captive-portal.badssl.com leaf certificate. This
Ah, I think this comment or something like it belongs on line 4022 now.

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:4137: // Set interstitial delay to zero.
nit: explain why this is needed

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:4216: // Try dynamically updating the
error assistant config with the an empty
nit: extra "the"

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:286: CHECK(proto);
Why is this a CHECK instead of a DCHECK? (probably worth a comment)

[meacer, 2017-02-08 22:34:32]

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:4023:
"sha256/fjZPHewEHTrMDX3I1ecEIeoy3WFxHyGplOLv28kIbtI=";
On 2017/02/08 20:31:17, estark wrote:
> Magic value; I suggest adding a comment explaining from whence it came.

Done.

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:4066: // Set the SPKI hash to
captive-portal.badssl.com leaf certificate. This
On 2017/02/08 20:31:17, estark wrote:
> Ah, I think this comment or something like it belongs on line 4022 now.

Done.

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:4137: // Set interstitial delay to zero.
On 2017/02/08 20:31:17, estark wrote:
> nit: explain why this is needed

Not needed, removed.

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:4216: // Try dynamically updating the
error assistant config with the an empty
On 2017/02/08 20:31:17, estark wrote:
> nit: extra "the"

Done.

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/2581903002/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:286: CHECK(proto);
On 2017/02/08 20:31:17, estark wrote:
> Why is this a CHECK instead of a DCHECK? (probably worth a comment)

If it's null, it's going to crash a couple of lines below anyways, so better to
do it early.

[meacer, 2017-02-09 02:13:05]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 02:13:34]

Dry run: 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, 2017-02-09 04:23:00]

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

[commit-bot: I haz the power, 2017-02-09 04:23:01]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[meacer, 2017-02-09 22:15:07]

meacer@chromium.org changed reviewers:
+ waffles@chromium.org

[meacer, 2017-02-09 22:15:07]

waffles: Can you also PTAL at the component updater bits? Once this lands, we'll
be ready to start testing the component. Thanks.

[meacer, 2017-02-09 22:16:13]

Description was changed from

==========
Add SSL Error Assistant component to dynamically update captive portal list

This CL adds the SSL Error Assistant component that will dynamically update the
captive portal certificate list to be used by SSLErrorHandler. The component's
implementation is mostly taken from FileTypePolicies component.

BUG=640835
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add SSL Error Assistant component to dynamically update captive portal list

This CL adds the SSL Error Assistant component that will dynamically update the
captive portal certificate list to be used by SSLErrorHandler. The component's
implementation is mostly taken from FileTypePolicies component.

For the time being, the component is only enabled on platforms where captive
portal detection is enabled (i.e. desktop).

BUG=640835
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[waffles, 2017-02-09 22:33:56]

component_updater lgtm, except for logging

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
File chrome/browser/component_updater/ssl_error_assistant_component_installer.cc
(right):

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:34:
VLOG(1) << "Reading SSL error assistant config from file: "
Can we use DVLOG? This will happen in the performance-critical startup period of
Chrome.
Same for both VLOGs below.

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:77:
LOG(ERROR) << ">> GetInstalledPath";
Remove this line.

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:101:
LOG(ERROR) << ">> VerifyInstallation";
Remove this line.

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:132:
VLOG(1) << "Registering SSLS Error Assistant component.";
Can we use DVLOG?

[meacer, 2017-02-09 23:06:39]

meacer@chromium.org changed reviewers:
+ sky@chromium.org

[meacer, 2017-02-09 23:06:40]

sky, can you PTAL at chrome/browser/chrome_browser_main.cc?

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
File chrome/browser/component_updater/ssl_error_assistant_component_installer.cc
(right):

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:34:
VLOG(1) << "Reading SSL error assistant config from file: "
On 2017/02/09 22:33:56, waffles wrote:
> Can we use DVLOG? This will happen in the performance-critical startup period
of
> Chrome.
> Same for both VLOGs below.

Done, here and elsewhere.

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:77:
LOG(ERROR) << ">> GetInstalledPath";
On 2017/02/09 22:33:56, waffles wrote:
> Remove this line.

Done.

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:101:
LOG(ERROR) << ">> VerifyInstallation";
On 2017/02/09 22:33:56, waffles wrote:
> Remove this line.

Done.

https://codereview.chromium.org/2581903002/diff/80001/chrome/browser/componen...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:132:
VLOG(1) << "Registering SSLS Error Assistant component.";
On 2017/02/09 22:33:56, waffles wrote:
> Can we use DVLOG?

Done.

[sky, 2017-02-10 01:06:33]

LGTM

[Nathan Parker, 2017-02-10 01:21:28]

lgtm

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/compone...
File chrome/browser/component_updater/ssl_error_assistant_component_installer.cc
(right):

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:34:
DVLOG(1) << "Reading SSL error assistant config from file: "
I'd argue much of this logging isn't useful to anyone except... you, while under
development.  If that's the case, remove it now or soon. I may be guilty of
leaving it in file_type_policies... yup.

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:130:
DVLOG(1) << "Registering SSLS Error Assistant component.";
nit: "SSLS"

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.cc:516:
g_config.Pointer()->SetErrorAssistantProto(std::move(config_proto));
When can this get called? Any threading comments or DCHECKs you want to add
here?

[meacer, 2017-02-10 01:29:55]

Thanks all!

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/compone...
File chrome/browser/component_updater/ssl_error_assistant_component_installer.cc
(right):

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:34:
DVLOG(1) << "Reading SSL error assistant config from file: "
On 2017/02/10 01:21:28, Nathan Parker wrote:
> I'd argue much of this logging isn't useful to anyone except... you, while
under
> development.  If that's the case, remove it now or soon. I may be guilty of
> leaving it in file_type_policies... yup.

Done :)

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/compone...
chrome/browser/component_updater/ssl_error_assistant_component_installer.cc:130:
DVLOG(1) << "Registering SSLS Error Assistant component.";
On 2017/02/10 01:21:28, Nathan Parker wrote:
> nit: "SSLS"

Done.

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/2581903002/diff/100001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.cc:516:
g_config.Pointer()->SetErrorAssistantProto(std::move(config_proto));
On 2017/02/10 01:21:28, Nathan Parker wrote:
> When can this get called? Any threading comments or DCHECKs you want to add
> here?

ConfigSingleton::SetErrorAssistantProto has a
DCHECK_CURRENTLY_ON(content::BrowserThread::UI), and SSLErrorHandler also has a
comment in its header saying it should only be used from UI thread.

[meacer, 2017-02-10 01:30:00]

The CQ bit was checked by meacer@chromium.org

[meacer, 2017-02-10 01:30:00]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
waffles@chromium.org, nparker@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2581903002/#ps120001
(title: "nparker comments")

[commit-bot: I haz the power, 2017-02-10 01:30:18]

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, 2017-02-10 02:31:01]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486690200073450,
"parent_rev": "74b7a0510c3aca53680a8d65fdc65d384c1d04b6", "commit_rev":
"4404d45a6532616d24d676113d5dfb0d519085b9"}

[commit-bot: I haz the power, 2017-02-10 02:31:32]

Description was changed from

==========
Add SSL Error Assistant component to dynamically update captive portal list

This CL adds the SSL Error Assistant component that will dynamically update the
captive portal certificate list to be used by SSLErrorHandler. The component's
implementation is mostly taken from FileTypePolicies component.

For the time being, the component is only enabled on platforms where captive
portal detection is enabled (i.e. desktop).

BUG=640835
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add SSL Error Assistant component to dynamically update captive portal list

This CL adds the SSL Error Assistant component that will dynamically update the
captive portal certificate list to be used by SSLErrorHandler. The component's
implementation is mostly taken from FileTypePolicies component.

For the time being, the component is only enabled on platforms where captive
portal detection is enabled (i.e. desktop).

BUG=640835
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2581903002
Cr-Commit-Position: refs/heads/master@{#449525}
Committed:
https://chromium.googlesource.com/chromium/src/+/4404d45a6532616d24d676113d5d...
==========

[commit-bot: I haz the power, 2017-02-10 02:31:33]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/4404d45a6532616d24d676113d5d...