Support host-based deletion for SSLHostStateDelegate

Support host-based deletion for SSLHostStateDelegate

This CL has two parts:
1. Since SSLHostStateDelegate stores the certificate decisions as
   a website setting, we can reuse the pattern-based deletion of website
   settings that is already implemented. However, it was implemented
   in BrowsingDataRemover. To reuse it for SSLHostStateDelegate, we need
   to move the logic to HostContentSettingsMap. Note that this is in line
   with the general pattern applied throughout Chromium in
   crbug.com/589586 where each data storage backend implements its own
   url-/origin-/host-/etc. based deletion.

2. Extend SSLHostStateDelegate::Clear() with a host filter, together with
   its 3 backends, and update ChromeSSLHostStateDelegateTest.

BUG=589586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e7da29897c9d0d10adf94330e0982ee059c0b8fc
Cr-Commit-Position: refs/heads/master@{#416894}

[msramek, 2016-08-29 16:23:35]

msramek@chromium.org changed reviewers:
+ estark@chromium.org, raymes@chromium.org

[msramek, 2016-08-29 16:23:35]

Hi folks,

Please have a look!

Raymes: For part #1, since you pushed against having the method in HCSM when it
was introduced :)

Emily: For part #2, i.e. the changes to SSLHostStateDelegate.

Thanks,
Martin

[estark, 2016-08-30 01:52:15]

estark@chromium.org changed reviewers:
+ jww@chromium.org

[estark, 2016-08-30 01:52:16]

The SSLHostStateDelegate changes lgtm with a couple questions.

jww would be a good person to look at the SSLHostStateDelegate stuff, but he's
on vacation. I'm adding him in hopes that he looks at it when he gets back --
don't think it needs to block on him though.

https://codereview.chromium.org/2292443003/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_ssl_host_state_delegate.cc (right):

https://codereview.chromium.org/2292443003/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_ssl_host_state_delegate.cc:87: for (auto it =
cert_policy_for_host_.begin(); it != cert_policy_map_.end();) {
|cert_policy_map_| doesn't appear to be a thing; guessing this is supposed to be
|cert_policy_for_host_|?

https://codereview.chromium.org/2292443003/diff/1/chrome/browser/ssl/chrome_s...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right):

https://codereview.chromium.org/2292443003/diff/1/chrome/browser/ssl/chrome_s...
chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:150:
DCHECK(url.is_valid());
Just checking that this DCHECK really is a DCHECK -- I can't figure out where
exactly this |primary_pattern| comes from. It's not e.g. a string typed by the
user, is it?

[msramek, 2016-08-30 14:38:10]

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

[commit-bot: I haz the power, 2016-08-30 14:38:22]

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

[msramek, 2016-08-30 14:39:41]

https://codereview.chromium.org/2292443003/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_ssl_host_state_delegate.cc (right):

https://codereview.chromium.org/2292443003/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_ssl_host_state_delegate.cc:87: for (auto it =
cert_policy_for_host_.begin(); it != cert_policy_map_.end();) {
On 2016/08/30 01:52:15, estark wrote:
> |cert_policy_map_| doesn't appear to be a thing; guessing this is supposed to
be
> |cert_policy_for_host_|?

Done. Of course I didn't get a compilation error since this is Android, and I
forgot to run trybots...

https://codereview.chromium.org/2292443003/diff/1/chrome/browser/ssl/chrome_s...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right):

https://codereview.chromium.org/2292443003/diff/1/chrome/browser/ssl/chrome_s...
chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:150:
DCHECK(url.is_valid());
On 2016/08/30 01:52:16, estark wrote:
> Just checking that this DCHECK really is a DCHECK -- I can't figure out where
> exactly this |primary_pattern| comes from. It's not e.g. a string typed by the
> user, is it?

Oh, the shame! The DCHECK is wrong, thanks for catching this :)

The user can't type it, but an administrator can, because it's managed by
policy:
https://www.chromium.org/administrators/policy-list-3#AutoSelectCertificateFo...

If we want to delete data for <host>, we must delete all patterns for
"*://<host>:*" or stricter (i.e. with specified scheme or port). I.e. there are
patterns with invalid URLs that we still care about.

We need to compare two blackboxes - ContentSettingsPattern and |host_filter|,
and I don't think there's a way around that without making one of them
transparent. So I added a GetHost() method to ContentSettingsPattern.

I also added a TODO for myself to investigate whether broadly scoped exceptions
are in practice recognized by ChromeSSLHostStateDelegate. We can talk about that
with Joel when he returns.

[commit-bot: I haz the power, 2016-08-30 15:08:35]

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

[commit-bot: I haz the power, 2016-08-30 15:08:36]

Dry run: 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...)

[msramek, 2016-08-30 15:14:32]

Description was changed from

==========
Support host-based deletion for SSLHostStateDelegate

This CL has two parts:
1. Since SSLHostStateDelegate stores the certificate decisions as
   a website setting, we can reuse the pattern-based deletion of website
   settings that is already implemented. However, it was implemented
   in BrowsingDataRemover. To reuse it for SSLHostStateDelegate, we need
   to move the logic to HostContentSettingsMap. Note that this is in line
   with the general pattern applied throughout Chromium in
   crbug.com/589586 where each data storage backend implements its own
   url-/origin-/host-/etc. based deletion.

2. Extend SSLHostStateDelegate::Clear() with a host filter, together with
   its 3 backends, and update ChromeSSLHostStateDelegateTest.

BUG=589586
==========

to

==========
Support host-based deletion for SSLHostStateDelegate

This CL has two parts:
1. Since SSLHostStateDelegate stores the certificate decisions as
   a website setting, we can reuse the pattern-based deletion of website
   settings that is already implemented. However, it was implemented
   in BrowsingDataRemover. To reuse it for SSLHostStateDelegate, we need
   to move the logic to HostContentSettingsMap. Note that this is in line
   with the general pattern applied throughout Chromium in
   crbug.com/589586 where each data storage backend implements its own
   url-/origin-/host-/etc. based deletion.

2. Extend SSLHostStateDelegate::Clear() with a host filter, together with
   its 3 backends, and update ChromeSSLHostStateDelegateTest.

BUG=589586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[msramek, 2016-08-30 15:14:46]

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

[commit-bot: I haz the power, 2016-08-30 15:15:06]

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, 2016-08-30 16:51:37]

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

[commit-bot: I haz the power, 2016-08-30 16:51:38]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-08-30 23:32:00]

Thanks!

https://codereview.chromium.org/2292443003/diff/1/chrome/browser/ssl/chrome_s...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right):

https://codereview.chromium.org/2292443003/diff/1/chrome/browser/ssl/chrome_s...
chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:150:
DCHECK(url.is_valid());
On 2016/08/30 14:39:41, msramek wrote:
> On 2016/08/30 01:52:16, estark wrote:
> > Just checking that this DCHECK really is a DCHECK -- I can't figure out
where
> > exactly this |primary_pattern| comes from. It's not e.g. a string typed by
the
> > user, is it?
> 
> Oh, the shame! The DCHECK is wrong, thanks for catching this :)
> 
> The user can't type it, but an administrator can, because it's managed by
> policy:
>
https://www.chromium.org/administrators/policy-list-3#AutoSelectCertificateFo...
> 
> If we want to delete data for <host>, we must delete all patterns for
> "*://<host>:*" or stricter (i.e. with specified scheme or port). I.e. there
are
> patterns with invalid URLs that we still care about.
> 
> We need to compare two blackboxes - ContentSettingsPattern and |host_filter|,
> and I don't think there's a way around that without making one of them
> transparent. So I added a GetHost() method to ContentSettingsPattern.
> 
> I also added a TODO for myself to investigate whether broadly scoped
exceptions
> are in practice recognized by ChromeSSLHostStateDelegate. We can talk about
that
> with Joel when he returns.

Hmm, this file is dealing with CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS though,
not CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE. I don't think
SSL_CERT_DECISIONS are managed by policy -- I think they are only set via the
chrome UI. Given that, can we avoid adding the GetHost for now?

The other content_settings changes look good to me though.

[msramek, 2016-08-31 13:42:59]

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

[commit-bot: I haz the power, 2016-08-31 13:43:08]

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

[msramek, 2016-08-31 13:43:11]

https://codereview.chromium.org/2292443003/diff/1/chrome/browser/ssl/chrome_s...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right):

https://codereview.chromium.org/2292443003/diff/1/chrome/browser/ssl/chrome_s...
chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:150:
DCHECK(url.is_valid());
On 2016/08/30 23:32:00, raymes wrote:
> On 2016/08/30 14:39:41, msramek wrote:
> > On 2016/08/30 01:52:16, estark wrote:
> > > Just checking that this DCHECK really is a DCHECK -- I can't figure out
> where
> > > exactly this |primary_pattern| comes from. It's not e.g. a string typed by
> the
> > > user, is it?
> > 
> > Oh, the shame! The DCHECK is wrong, thanks for catching this :)
> > 
> > The user can't type it, but an administrator can, because it's managed by
> > policy:
> >
>
https://www.chromium.org/administrators/policy-list-3#AutoSelectCertificateFo...
> > 
> > If we want to delete data for <host>, we must delete all patterns for
> > "*://<host>:*" or stricter (i.e. with specified scheme or port). I.e. there
> are
> > patterns with invalid URLs that we still care about.
> > 
> > We need to compare two blackboxes - ContentSettingsPattern and
|host_filter|,
> > and I don't think there's a way around that without making one of them
> > transparent. So I added a GetHost() method to ContentSettingsPattern.
> > 
> > I also added a TODO for myself to investigate whether broadly scoped
> exceptions
> > are in practice recognized by ChromeSSLHostStateDelegate. We can talk about
> that
> > with Joel when he returns.
> 
> Hmm, this file is dealing with CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS
though,
> not CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE. I don't think
> SSL_CERT_DECISIONS are managed by policy -- I think they are only set via the
> chrome UI. Given that, can we avoid adding the GetHost for now?
> 
> The other content_settings changes look good to me though.

...correct. I panicked and searched for the wrong one :)

So! The only SetWebsiteSetting call for SSL_CERT_DECISIONS is in this file, and
it uses the default pattern scoping for the URL generated by
GetSecureGURLForHost(), which is GURL("https://<host>"). The default scoping
will create a "https://<host>:443" pattern, which is also a valid URL.

Even before the introduction of default scopes, SSL_CERT_DECISIONS used
FromURLNoWildcard, so there cannot be any legacy exceptions with wildcards
either.

This is not manageable by policy or extensions.

Which means that yes, the DCHECK is valid after all :-)

[msramek, 2016-08-31 13:54:02]

msramek@chromium.org changed reviewers:
+ nasko@chromium.org, torne@chromium.org

[msramek, 2016-08-31 13:54:03]

...and since Emily already reviewed the main implementation,

+Torne PTAL at android_webview/
+Nasko PTAL at content/

(copy-pasted basic functionality and callsite updates)

Thanks,
Martin

[Torne, 2016-08-31 13:59:47]

android_webview LGTM

[commit-bot: I haz the power, 2016-08-31 15:06:49]

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

[commit-bot: I haz the power, 2016-08-31 15:06:49]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-09-01 01:47:05]

content_settings lgtm

https://codereview.chromium.org/2292443003/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2292443003/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1706: //
|host_settings| contains block.
There is no block for APP_BANNER, I think it's just an empty dict

[msramek, 2016-09-01 10:34:00]

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

[commit-bot: I haz the power, 2016-09-01 10:34:11]

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, 2016-09-01 10:36:29]

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

[commit-bot: I haz the power, 2016-09-01 10:36:30]

Dry run: Try jobs failed on following builders:
  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_...)

[msramek, 2016-09-01 16:31:48]

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

[commit-bot: I haz the power, 2016-09-01 16:32:05]

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

[msramek, 2016-09-01 16:34:56]

Thanks Raymes!

https://codereview.chromium.org/2292443003/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2292443003/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1706: //
|host_settings| contains block.
On 2016/09/01 01:47:04, raymes wrote:
> There is no block for APP_BANNER, I think it's just an empty dict

Rewrote the comment.

There are actually more things that I would fix in this test, but this is not my
code, I'm just copying it over here, so I'd leave it to another CL.

[nasko, 2016-09-01 16:58:15]

content/ LGTM

[commit-bot: I haz the power, 2016-09-01 17:40:42]

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

[commit-bot: I haz the power, 2016-09-01 17:40:43]

Dry run: This issue passed the CQ dry run.

[jww, 2016-09-06 22:56:12]

On 2016/09/01 16:58:15, nasko (slow) wrote:
> content/ LGTM

lgtm

[msramek, 2016-09-07 08:56:06]

Thanks!

[msramek, 2016-09-07 08:56:19]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-09-07 08:56:20]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
raymes@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/2292443003/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-09-07 08:56:48]

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-09-07 09:58:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-07 09:59:51]

Description was changed from

==========
Support host-based deletion for SSLHostStateDelegate

This CL has two parts:
1. Since SSLHostStateDelegate stores the certificate decisions as
   a website setting, we can reuse the pattern-based deletion of website
   settings that is already implemented. However, it was implemented
   in BrowsingDataRemover. To reuse it for SSLHostStateDelegate, we need
   to move the logic to HostContentSettingsMap. Note that this is in line
   with the general pattern applied throughout Chromium in
   crbug.com/589586 where each data storage backend implements its own
   url-/origin-/host-/etc. based deletion.

2. Extend SSLHostStateDelegate::Clear() with a host filter, together with
   its 3 backends, and update ChromeSSLHostStateDelegateTest.

BUG=589586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Support host-based deletion for SSLHostStateDelegate

This CL has two parts:
1. Since SSLHostStateDelegate stores the certificate decisions as
   a website setting, we can reuse the pattern-based deletion of website
   settings that is already implemented. However, it was implemented
   in BrowsingDataRemover. To reuse it for SSLHostStateDelegate, we need
   to move the logic to HostContentSettingsMap. Note that this is in line
   with the general pattern applied throughout Chromium in
   crbug.com/589586 where each data storage backend implements its own
   url-/origin-/host-/etc. based deletion.

2. Extend SSLHostStateDelegate::Clear() with a host filter, together with
   its 3 backends, and update ChromeSSLHostStateDelegateTest.

BUG=589586
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-09-07 09:59:52]

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