Remove SSLStatus::security_style member and content::SecurityStyle

Remove SSLStatus::security_style member and content::SecurityStyle

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around.

Since content::SecurityStyle no longer serves any purpose besides
ferrying information to devtools, I removed content::SecurityStyle and
just use blink::WebSecurityStyle everywhere (which has been pulled out
into its own file, from blink::WebURLResponse::SecurityStyle).

This should hopefully make SecurityLevel/SecurityStyle less confusing,
and remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

Committed: https://crrev.com/47ba9c7d92885d137c51b462d8af87e3f3a6ecdd
Cr-Commit-Position: refs/heads/master@{#424436}

[estark, 2016-10-06 21:44:35]

Description was changed from

==========
Remove SSLStatus::security_style member

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around. This
should hopefully remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

BUG=648326
==========

to

==========
Remove SSLStatus::security_style member

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around. This
should hopefully remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

[estark, 2016-10-06 21:45:26]

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

[commit-bot: I haz the power, 2016-10-06 21:46:09]

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

[estark, 2016-10-06 21:46:36]

Description was changed from

==========
Remove SSLStatus::security_style member

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around. This
should hopefully remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

to

==========
Remove SSLStatus::security_style member

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around.

This should hopefully make SecurityLevel/SecurityStyle less confusing,
and remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

[commit-bot: I haz the power, 2016-10-06 21:49:13]

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

[commit-bot: I haz the power, 2016-10-06 21:49:14]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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_...)

[estark, 2016-10-06 21:58:16]

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

[commit-bot: I haz the power, 2016-10-06 21:58:43]

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

[estark, 2016-10-06 22:00:16]

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

[estark, 2016-10-06 22:00:18]

felt, could you do a first pass over this before I send to owners? (I recommend
reviewing Patch Set 1 because I left some comments on that to try to explain
some things; Patch Set 2 is just a rebase.)

https://codereview.chromium.org/2400673003/diff/1/android_webview/native/aw_a...
File android_webview/native/aw_autofill_client.cc (right):

https://codereview.chromium.org/2400673003/diff/1/android_webview/native/aw_a...
android_webview/native/aw_autofill_client.cc:199: return
navigation_entry->GetURL().SchemeIsCryptographic() &&
This is the part that is most sketchy, IMO. AwAutofillClient and
ChromeAutofillClient have to do their own calculation to decide whether the page
is secure.

ChromeAutofillClient will soon be moving to use a SecurityLevel to make this
decision instead, but we'll still be doing this calculation here. We could just
stick a helper method on SSLStatus, like
SSLStatus::IsCryptographicWithNoMajorCertErrors() or something and use that
here.

https://codereview.chromium.org/2400673003/diff/1/chrome/browser/ssl/chrome_s...
File chrome/browser/ssl/chrome_security_state_model_client.h (left):

https://codereview.chromium.org/2400673003/diff/1/chrome/browser/ssl/chrome_s...
chrome/browser/ssl/chrome_security_state_model_client.h:43: bool
RetrieveCert(scoped_refptr<net::X509Certificate>* cert) override;
This was a mostly unrelated cleanup that I did while I was here; no need for
this method anymore.

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
File components/security_state/security_state_model.h (left):

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
components/security_state/security_state_model.h:151: SecurityLevel
initial_security_level;
|initial_security_level| used to be the way that ChromeSSMClient provided the
SSLStatus::security_style to SSM, and SSM used that as a baseline starting point
for computing the SecurityLevel. This isn't needed anymore; SSM just calculates
the baseline starting point itself instead of taking it from
SSLStatus::security_style.

https://codereview.chromium.org/2400673003/diff/1/content/child/web_url_loade...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2400673003/diff/1/content/child/web_url_loade...
content/child/web_url_loader_impl.cc:283: GetSecurityStyleForResource(url,
info.cert_status);
note that this is only for devtools (thereby sticking to the theme that
securitystyle-is-for-devtools); we early-return at the top of this function if
|report_security_info| is false, and it's only true when we need security info
for devtools.

[commit-bot: I haz the power, 2016-10-06 22:27:52]

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

[commit-bot: I haz the power, 2016-10-06 22:27:52]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[felt, 2016-10-07 03:30:48]

https://codereview.chromium.org/2400673003/diff/1/android_webview/native/aw_a...
File android_webview/native/aw_autofill_client.cc (right):

https://codereview.chromium.org/2400673003/diff/1/android_webview/native/aw_a...
android_webview/native/aw_autofill_client.cc:199: return
navigation_entry->GetURL().SchemeIsCryptographic() &&
On 2016/10/06 22:00:17, estark wrote:
> This is the part that is most sketchy, IMO. AwAutofillClient and
> ChromeAutofillClient have to do their own calculation to decide whether the
page
> is secure.
> 
> ChromeAutofillClient will soon be moving to use a SecurityLevel to make this
> decision instead, but we'll still be doing this calculation here. We could
just
> stick a helper method on SSLStatus, like
> SSLStatus::IsCryptographicWithNoMajorCertErrors() or something and use that
> here.

Were these the only callers that you found? If so I don't think it's worth
adding the helper method, especially if we switch the other one over to a
SecurityLevel.

https://codereview.chromium.org/2400673003/diff/1/chrome/browser/ssl/bad_cloc...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/2400673003/diff/1/chrome/browser/ssl/bad_cloc...
chrome/browser/ssl/bad_clock_blocking_page.cc:109: entry->GetSSL() =
content::SSLStatus(ssl_info_.cert, ssl_info_);
Does the omnibox still get correctly downgraded for all the blocking pages? It
*should*, but just checking you tested

https://codereview.chromium.org/2400673003/diff/1/chrome/browser/ssl/chrome_s...
File chrome/browser/ssl/chrome_security_state_model_client.h (left):

https://codereview.chromium.org/2400673003/diff/1/chrome/browser/ssl/chrome_s...
chrome/browser/ssl/chrome_security_state_model_client.h:43: bool
RetrieveCert(scoped_refptr<net::X509Certificate>* cert) override;
On 2016/10/06 22:00:17, estark wrote:
> This was a mostly unrelated cleanup that I did while I was here; no need for
> this method anymore.

🙌

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
File components/security_state/security_state_model.cc (right):

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
components/security_state/security_state_model.cc:120:
SecurityStateModel::SecurityLevel GetSecurityLevelForRequest(
The downside, I guess, is that this method gets slightly harder to follow.

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
components/security_state/security_state_model.cc:164: return
SecurityStateModel::kRanInsecureContentLevel;
Tangent: why do we have this constant instead of just directly marking the state
here?

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
File components/security_state/security_state_model.h (left):

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
components/security_state/security_state_model.h:151: SecurityLevel
initial_security_level;
On 2016/10/06 22:00:17, estark wrote:
> |initial_security_level| used to be the way that ChromeSSMClient provided the
> SSLStatus::security_style to SSM, and SSM used that as a baseline starting
point
> for computing the SecurityLevel. This isn't needed anymore; SSM just
calculates
> the baseline starting point itself instead of taking it from
> SSLStatus::security_style.

I prefer the new way, I find it more self explanatory

[felt, 2016-10-07 03:33:29]

https://codereview.chromium.org/2400673003/diff/20001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2400673003/diff/20001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:107: // differently if they use SHA1
signatures.) https://crbug.com/648326
Leaving this as a TODO feels a little squicky to me, but I don't have a better
suggestion at the moment.

[estark, 2016-10-07 06:30:52]

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

[commit-bot: I haz the power, 2016-10-07 06:31:19]

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

[estark, 2016-10-07 06:34:31]

Thanks felt.

https://codereview.chromium.org/2400673003/diff/1/android_webview/native/aw_a...
File android_webview/native/aw_autofill_client.cc (right):

https://codereview.chromium.org/2400673003/diff/1/android_webview/native/aw_a...
android_webview/native/aw_autofill_client.cc:199: return
navigation_entry->GetURL().SchemeIsCryptographic() &&
On 2016/10/07 03:30:47, felt wrote:
> On 2016/10/06 22:00:17, estark wrote:
> > This is the part that is most sketchy, IMO. AwAutofillClient and
> > ChromeAutofillClient have to do their own calculation to decide whether the
> page
> > is secure.
> > 
> > ChromeAutofillClient will soon be moving to use a SecurityLevel to make this
> > decision instead, but we'll still be doing this calculation here. We could
> just
> > stick a helper method on SSLStatus, like
> > SSLStatus::IsCryptographicWithNoMajorCertErrors() or something and use that
> > here.
> 
> Were these the only callers that you found? If so I don't think it's worth
> adding the helper method, especially if we switch the other one over to a
> SecurityLevel.

The bots found another one, which is this weird
chrome/browser/android/policy/policy_auditor.cc thing that I've never seen
before. It's a little different though, and also should maybe be using SSM
instead? Do you know any more about this policy auditor thing? If not I'll ask
an android person.

https://codereview.chromium.org/2400673003/diff/1/chrome/browser/ssl/bad_cloc...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/2400673003/diff/1/chrome/browser/ssl/bad_cloc...
chrome/browser/ssl/bad_clock_blocking_page.cc:109: entry->GetSSL() =
content::SSLStatus(ssl_info_.cert, ssl_info_);
On 2016/10/07 03:30:48, felt wrote:
> Does the omnibox still get correctly downgraded for all the blocking pages? It
> *should*, but just checking you tested

Tested manually, they look right except for crbug.com/653779 which is
independent of this change. (Also added assertions to bad clock browser tests to
check this.)

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
File components/security_state/security_state_model.cc (right):

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
components/security_state/security_state_model.cc:120:
SecurityStateModel::SecurityLevel GetSecurityLevelForRequest(
On 2016/10/07 03:30:48, felt wrote:
> The downside, I guess, is that this method gets slightly harder to follow.

You think so? Oddly I find it easier to follow this way... but maybe that's only
because I just wrote it.

(In particular, one of the things I find confusing about the old way is that we
were implicitly assuming certain relationships between the
initial_security_level and other parts of the VisibleSecurityState, like we
weren't checking for active mixed content if the initial security level was
SECURITY_WARNING and just left it at SECURITY_WARNING. In practice this was fine
because we were never actually setting an initial security level of
SECURITY_WARNING anywhere, but it seems like a weird thing still.)

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
components/security_state/security_state_model.cc:164: return
SecurityStateModel::kRanInsecureContentLevel;
On 2016/10/07 03:30:48, felt wrote:
> Tangent: why do we have this constant instead of just directly marking the
state
> here?

We send this constant to devtools so that devtools knows what icon to use when
it says that there's mixed content on the page. (See
https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_security_state...)

e.g. if we were to change to start showing red triangle for passive mixed
content, we would change this constant and then devtools would start magically
showing red when it warns about passive mixed content.

https://codereview.chromium.org/2400673003/diff/20001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2400673003/diff/20001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:107: // differently if they use SHA1
signatures.) https://crbug.com/648326
On 2016/10/07 03:33:29, felt wrote:
> Leaving this as a TODO feels a little squicky to me, but I don't have a better
> suggestion at the moment.

Yeah. :( What I really want to do is compute this value in
ResourceDispatcherHostImpl in the browser process (from which it would be easy
to reach into the embedder via ResourceDispatcherHostDelegate) and send it down
with the response, instead of computing it here in the renderer. But that would
interfere with jam's network servicification effort for which he painstakingly
removed SecurityStyles from the resource loader code in the browser process.
Anyway, after I polish this up a bit more I'll add him as an owners reviewer and
see if he has any suggestions for how to do this.

[commit-bot: I haz the power, 2016-10-07 07:58:56]

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

[commit-bot: I haz the power, 2016-10-07 07:58:57]

Dry run: This issue passed the CQ dry run.

[felt, 2016-10-07 15:08:10]

approach looks good, on balance i think this is an improvement

https://codereview.chromium.org/2400673003/diff/1/android_webview/native/aw_a...
File android_webview/native/aw_autofill_client.cc (right):

https://codereview.chromium.org/2400673003/diff/1/android_webview/native/aw_a...
android_webview/native/aw_autofill_client.cc:199: return
navigation_entry->GetURL().SchemeIsCryptographic() &&
On 2016/10/07 06:34:31, estark wrote:
> On 2016/10/07 03:30:47, felt wrote:
> > On 2016/10/06 22:00:17, estark wrote:
> > > This is the part that is most sketchy, IMO. AwAutofillClient and
> > > ChromeAutofillClient have to do their own calculation to decide whether
the
> > page
> > > is secure.
> > > 
> > > ChromeAutofillClient will soon be moving to use a SecurityLevel to make
this
> > > decision instead, but we'll still be doing this calculation here. We could
> > just
> > > stick a helper method on SSLStatus, like
> > > SSLStatus::IsCryptographicWithNoMajorCertErrors() or something and use
that
> > > here.
> > 
> > Were these the only callers that you found? If so I don't think it's worth
> > adding the helper method, especially if we switch the other one over to a
> > SecurityLevel.
> 
> The bots found another one, which is this weird
> chrome/browser/android/policy/policy_auditor.cc thing that I've never seen
> before. It's a little different though, and also should maybe be using SSM
> instead? Do you know any more about this policy auditor thing? If not I'll ask
> an android person.

I've never seen that policy auditor thing before. The matching Java class didn't
clear things up either, except perhaps it is somehow related to enterprise
policies? Please let me know what you find, I'd very much like to know what that
class is doing & why.

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
File components/security_state/security_state_model.cc (right):

https://codereview.chromium.org/2400673003/diff/1/components/security_state/s...
components/security_state/security_state_model.cc:164: return
SecurityStateModel::kRanInsecureContentLevel;
On 2016/10/07 06:34:31, estark wrote:
> On 2016/10/07 03:30:48, felt wrote:
> > Tangent: why do we have this constant instead of just directly marking the
> state
> > here?
> 
> We send this constant to devtools so that devtools knows what icon to use when
> it says that there's mixed content on the page. (See
>
https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_security_state...)
> 
> e.g. if we were to change to start showing red triangle for passive mixed
> content, we would change this constant and then devtools would start magically
> showing red when it warns about passive mixed content.

Ah, right on.

[estark, 2016-10-07 19:14:56]

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

[estark, 2016-10-07 19:14:57]

jam: another clean-up for you to take a look at, please! I tried to explain in
the CL description what I'm doing and why but lmk if it isn't clear.

https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:100: // TODO(estark): Figure out a way for
the embedder to provide the
jam: curious if you have any ideas about how to accomplish this TODO. Ideally
I'd like this logic to live in //chrome somewhere or in
//components/security_state. (This logic chooses the icon that shows up next to
each request in the devtools security panel, so I think it should be under
chrome's control, just like the lock icon is.)

[jam, 2016-10-10 15:59:01]

https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:100: // TODO(estark): Figure out a way for
the embedder to provide the
On 2016/10/07 19:14:57, estark wrote:
> jam: curious if you have any ideas about how to accomplish this TODO. Ideally
> I'd like this logic to live in //chrome somewhere or in
> //components/security_state. (This logic chooses the icon that shows up next
to
> each request in the devtools security panel, so I think it should be under
> chrome's control, just like the lock icon is.)

The most straightforward solution would be to add a new embedder api (i.e. on
ContentClient, since we don't have a ContentChildClient). it should just return
a blink::WebURLResponse::SecurityStyle, no need for an intermediate type?

The one question I have is: it seems blink needs to know all the different
states for the UI indicator since that's where the devtools enums are that
control the UI. If so, why shouldn't content/ be the place where this is done?
i.e. if the logic is going to be the same for all embedders, and content/ has
the necessary info to calculate this, why burden every embedder with calculating
it?

https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:108: SecurityStyle
GetSecurityStyleForResource(const GURL& url,
return blink::WebURLResponse::SecurityStyle and then no need to convert below in
the one calling site?

[estark, 2016-10-10 18:17:22]

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

[commit-bot: I haz the power, 2016-10-10 18:17:36]

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

[estark, 2016-10-10 18:20:40]

https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:100: // TODO(estark): Figure out a way for
the embedder to provide the
On 2016/10/10 15:59:01, jam wrote:
> On 2016/10/07 19:14:57, estark wrote:
> > jam: curious if you have any ideas about how to accomplish this TODO.
Ideally
> > I'd like this logic to live in //chrome somewhere or in
> > //components/security_state. (This logic chooses the icon that shows up next
> to
> > each request in the devtools security panel, so I think it should be under
> > chrome's control, just like the lock icon is.)
> 
> The most straightforward solution would be to add a new embedder api (i.e. on
> ContentClient, since we don't have a ContentChildClient). it should just
return
> a blink::WebURLResponse::SecurityStyle, no need for an intermediate type?
> 
> The one question I have is: it seems blink needs to know all the different
> states for the UI indicator since that's where the devtools enums are that
> control the UI. If so, why shouldn't content/ be the place where this is done?
> i.e. if the logic is going to be the same for all embedders, and content/ has
> the necessary info to calculate this, why burden every embedder with
calculating
> it?

While the set of states is the same for all embedders, in general the logic
won't be the same for all of them. For example, Chrome would ideally use the red
dangerous state here for resources that are main resources loaded with SHA1
certificates (https://crbug.com/527234), but other embedders take different
approaches to SHA1 deprecations. For example, Opera shows a neutral icon on
https://sha12017.badssl.com but Chrome shows the red dangerous state.

Does that make sense? (If so, if it's okay with you I'll do the ContentClient
thing on a follow-up CL since this one's already big.)

https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:108: SecurityStyle
GetSecurityStyleForResource(const GURL& url,
On 2016/10/10 15:59:01, jam wrote:
> return blink::WebURLResponse::SecurityStyle and then no need to convert below
in
> the one calling site?

Done.

This made me wonder, if I pull out blink::WebURLResponse::SecurityStyle into its
own separate header file in Blink, then I think I'm allowed to depend on it in
//content and //chrome? If so, then I could probably just get rid of
content::SecurityStyle entirely, since at this point it only serves as an
intermediate value to translate into blink::WebURLResponse::SecurityStyles.

[commit-bot: I haz the power, 2016-10-10 19:45:49]

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

[commit-bot: I haz the power, 2016-10-10 19:45:50]

Dry run: This issue passed the CQ dry run.

[jam, 2016-10-10 20:13:21]

lgtm

I found these multiple enums confusing when I was working in this area, so this
simplification is great.

On 2016/10/10 18:20:40, estark wrote:
>
https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
> File content/child/web_url_loader_impl.cc (right):
> 
>
https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
> content/child/web_url_loader_impl.cc:100: // TODO(estark): Figure out a way
for
> the embedder to provide the
> On 2016/10/10 15:59:01, jam wrote:
> > On 2016/10/07 19:14:57, estark wrote:
> > > jam: curious if you have any ideas about how to accomplish this TODO.
> Ideally
> > > I'd like this logic to live in //chrome somewhere or in
> > > //components/security_state. (This logic chooses the icon that shows up
next
> > to
> > > each request in the devtools security panel, so I think it should be under
> > > chrome's control, just like the lock icon is.)
> > 
> > The most straightforward solution would be to add a new embedder api (i.e.
on
> > ContentClient, since we don't have a ContentChildClient). it should just
> return
> > a blink::WebURLResponse::SecurityStyle, no need for an intermediate type?
> > 
> > The one question I have is: it seems blink needs to know all the different
> > states for the UI indicator since that's where the devtools enums are that
> > control the UI. If so, why shouldn't content/ be the place where this is
done?
> > i.e. if the logic is going to be the same for all embedders, and content/
has
> > the necessary info to calculate this, why burden every embedder with
> calculating
> > it?
> 
> While the set of states is the same for all embedders, in general the logic
> won't be the same for all of them. For example, Chrome would ideally use the
red
> dangerous state here for resources that are main resources loaded with SHA1
> certificates (https://crbug.com/527234), but other embedders take different
> approaches to SHA1 deprecations. For example, Opera shows a neutral icon on
> https://sha12017.badssl.com but Chrome shows the red dangerous state.
> 
> Does that make sense? (If so, if it's okay with you I'll do the ContentClient
> thing on a follow-up CL since this one's already big.)

Yes that makes sense, thanks for the explanation.

> 
>
https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
> content/child/web_url_loader_impl.cc:108: SecurityStyle
> GetSecurityStyleForResource(const GURL& url,
> On 2016/10/10 15:59:01, jam wrote:
> > return blink::WebURLResponse::SecurityStyle and then no need to convert
below
> in
> > the one calling site?
> 
> Done.
> 
> This made me wonder, if I pull out blink::WebURLResponse::SecurityStyle into
its
> own separate header file in Blink, then I think I'm allowed to depend on it in
> //content and //chrome? If so, then I could probably just get rid of
> content::SecurityStyle entirely, since at this point it only serves as an
> intermediate value to translate into blink::WebURLResponse::SecurityStyles.

Sounds great. and yes, chrome and content can depend on blink enums, see
https://cs.chromium.org/chromium/src/chrome/browser/DEPS?rcl=0&l=87

[estark, 2016-10-10 22:46:27]

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

[commit-bot: I haz the power, 2016-10-10 22:47:04]

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-10-10 22:50:05]

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

[commit-bot: I haz the power, 2016-10-10 22:50:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2016-10-10 22:55:57]

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

[commit-bot: I haz the power, 2016-10-10 22:56:16]

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

[estark, 2016-10-10 22:58:27]

On 2016/10/10 20:13:21, jam wrote:
> lgtm
> 
> I found these multiple enums confusing when I was working in this area, so
this
> simplification is great.
> 
> On 2016/10/10 18:20:40, estark wrote:
> >
>
https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
> > File content/child/web_url_loader_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
> > content/child/web_url_loader_impl.cc:100: // TODO(estark): Figure out a way
> for
> > the embedder to provide the
> > On 2016/10/10 15:59:01, jam wrote:
> > > On 2016/10/07 19:14:57, estark wrote:
> > > > jam: curious if you have any ideas about how to accomplish this TODO.
> > Ideally
> > > > I'd like this logic to live in //chrome somewhere or in
> > > > //components/security_state. (This logic chooses the icon that shows up
> next
> > > to
> > > > each request in the devtools security panel, so I think it should be
under
> > > > chrome's control, just like the lock icon is.)
> > > 
> > > The most straightforward solution would be to add a new embedder api (i.e.
> on
> > > ContentClient, since we don't have a ContentChildClient). it should just
> > return
> > > a blink::WebURLResponse::SecurityStyle, no need for an intermediate type?
> > > 
> > > The one question I have is: it seems blink needs to know all the different
> > > states for the UI indicator since that's where the devtools enums are that
> > > control the UI. If so, why shouldn't content/ be the place where this is
> done?
> > > i.e. if the logic is going to be the same for all embedders, and content/
> has
> > > the necessary info to calculate this, why burden every embedder with
> > calculating
> > > it?
> > 
> > While the set of states is the same for all embedders, in general the logic
> > won't be the same for all of them. For example, Chrome would ideally use the
> red
> > dangerous state here for resources that are main resources loaded with SHA1
> > certificates (https://crbug.com/527234), but other embedders take different
> > approaches to SHA1 deprecations. For example, Opera shows a neutral icon on
> > https://sha12017.badssl.com but Chrome shows the red dangerous state.
> > 
> > Does that make sense? (If so, if it's okay with you I'll do the
ContentClient
> > thing on a follow-up CL since this one's already big.)
> 
> Yes that makes sense, thanks for the explanation.
> 
> > 
> >
>
https://codereview.chromium.org/2400673003/diff/40001/content/child/web_url_l...
> > content/child/web_url_loader_impl.cc:108: SecurityStyle
> > GetSecurityStyleForResource(const GURL& url,
> > On 2016/10/10 15:59:01, jam wrote:
> > > return blink::WebURLResponse::SecurityStyle and then no need to convert
> below
> > in
> > > the one calling site?
> > 
> > Done.
> > 
> > This made me wonder, if I pull out blink::WebURLResponse::SecurityStyle into
> its
> > own separate header file in Blink, then I think I'm allowed to depend on it
in
> > //content and //chrome? If so, then I could probably just get rid of
> > content::SecurityStyle entirely, since at this point it only serves as an
> > intermediate value to translate into blink::WebURLResponse::SecurityStyles.
> 
> Sounds great. and yes, chrome and content can depend on blink enums, see
> https://cs.chromium.org/chromium/src/chrome/browser/DEPS?rcl=0&l=87

Ok, cool. I thought this would be a small mechanical change, but it's actually a
somewhat large mechanical change that blew up the size of the CL a bit. Oops.
Your l-g-t-m still covers almost all of the newly touched files, but I'll wait a
day or two before landing in case you or felt wants to take another look.

[estark, 2016-10-10 23:00:04]

Description was changed from

==========
Remove SSLStatus::security_style member

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around.

This should hopefully make SecurityLevel/SecurityStyle less confusing,
and remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

to

==========
Remove SSLStatus::security_style member and content::SecurityStyle

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around.

Since content::SecurityStyle no longer serves any purpose besides
ferrying information to devtools, I removed content::SecurityStyle and
just use blink::WebSecurityStyle everywhere (which has been pulled out
into its own file, from blink::WebURLResponse::SecurityStyle).

This should hopefully make SecurityLevel/SecurityStyle less confusing,
and remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

[estark, 2016-10-10 23:01:46]

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

[estark, 2016-10-10 23:01:47]

+meacer: can you please review
content/public/common/common_param_traits_macros.h? (Lucky you, only one file
out of this disgustingly large CL :P)

[commit-bot: I haz the power, 2016-10-10 23:12:57]

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

[commit-bot: I haz the power, 2016-10-10 23:12:58]

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

[estark, 2016-10-10 23:24:10]

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

[commit-bot: I haz the power, 2016-10-10 23:24:28]

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

[meacer, 2016-10-10 23:43:17]

common_param_traits_macros.h lgtm.

https://codereview.chromium.org/2400673003/diff/120001/chrome/browser/android...
File chrome/browser/android/policy/policy_auditor.cc (right):

https://codereview.chromium.org/2400673003/diff/120001/chrome/browser/android...
chrome/browser/android/policy/policy_auditor.cc:53: return
CERTIFICATE_FAIL_UNSPECIFIED;
nit: maybe it's me, but this code looks prone to goto fail. Perhaps it could use
more curly braces.

[commit-bot: I haz the power, 2016-10-11 00:33:39]

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

[commit-bot: I haz the power, 2016-10-11 00:33:40]

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

[estark, 2016-10-11 00:49:12]

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

[commit-bot: I haz the power, 2016-10-11 00:49:35]

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

[estark, 2016-10-11 01:16:20]

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

[estark, 2016-10-11 01:16:28]

https://codereview.chromium.org/2400673003/diff/120001/chrome/browser/android...
File chrome/browser/android/policy/policy_auditor.cc (right):

https://codereview.chromium.org/2400673003/diff/120001/chrome/browser/android...
chrome/browser/android/policy/policy_auditor.cc:53: return
CERTIFICATE_FAIL_UNSPECIFIED;
On 2016/10/10 23:43:17, Mustafa Emre Acer wrote:
> nit: maybe it's me, but this code looks prone to goto fail. Perhaps it could
use
> more curly braces.

Yeah, I like curly braces too. Done.

[commit-bot: I haz the power, 2016-10-11 01:16:44]

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-10-11 02:48:32]

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

[commit-bot: I haz the power, 2016-10-11 02:48:34]

Dry run: This issue passed the CQ dry run.

[felt, 2016-10-11 13:45:26]

still lgtm

[jam, 2016-10-11 15:31:27]

lgtm

[estark, 2016-10-11 15:33:31]

The CQ bit was checked by estark@chromium.org

[estark, 2016-10-11 15:33:31]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2400673003/#ps160001
(title: "more curly braces")

[commit-bot: I haz the power, 2016-10-11 15:33:55]

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

[estark, 2016-10-11 15:34:18]

cc lgarron since your WARNING CL will conflict with this, sorry :(

[commit-bot: I haz the power, 2016-10-11 15:40:35]

Description was changed from

==========
Remove SSLStatus::security_style member and content::SecurityStyle

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around.

Since content::SecurityStyle no longer serves any purpose besides
ferrying information to devtools, I removed content::SecurityStyle and
just use blink::WebSecurityStyle everywhere (which has been pulled out
into its own file, from blink::WebURLResponse::SecurityStyle).

This should hopefully make SecurityLevel/SecurityStyle less confusing,
and remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

to

==========
Remove SSLStatus::security_style member and content::SecurityStyle

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around.

Since content::SecurityStyle no longer serves any purpose besides
ferrying information to devtools, I removed content::SecurityStyle and
just use blink::WebSecurityStyle everywhere (which has been pulled out
into its own file, from blink::WebURLResponse::SecurityStyle).

This should hopefully make SecurityLevel/SecurityStyle less confusing,
and remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

[commit-bot: I haz the power, 2016-10-11 15:40:38]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-11 15:43:31]

Description was changed from

==========
Remove SSLStatus::security_style member and content::SecurityStyle

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around.

Since content::SecurityStyle no longer serves any purpose besides
ferrying information to devtools, I removed content::SecurityStyle and
just use blink::WebSecurityStyle everywhere (which has been pulled out
into its own file, from blink::WebURLResponse::SecurityStyle).

This should hopefully make SecurityLevel/SecurityStyle less confusing,
and remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

to

==========
Remove SSLStatus::security_style member and content::SecurityStyle

Previously, //content used SSLStatus::security_style to express an
opinion about the overall security state of the
page. SecurityStateModel/ChromeSecurityStateModelClient used this
SecurityStyle as a starting point, applying Chrome-specific policies
such as SHA1 deprecation to produce a SecurityLevel, which corresponds
to the lock icon. Then, for conveying the security state of a page back
to DevTools, the SecurityLevel would get converted back into a
SecurityStyle (often a different SecurityStyle than what //content
assigned in SSLStatus::security_style). Additionally, SecurityStyles are
used to convey per-request security info to DevTools.

This CL removes SSLStatus::security_style, so that //content no longer
assigns an overall security state to a page. This means that
SecurityStyles are now only used to convey security
information (per-page or per-request) to DevTools. We only convert from
SecurityLevels to SecurityStyles, and never the other way around.

Since content::SecurityStyle no longer serves any purpose besides
ferrying information to devtools, I removed content::SecurityStyle and
just use blink::WebSecurityStyle everywhere (which has been pulled out
into its own file, from blink::WebURLResponse::SecurityStyle).

This should hopefully make SecurityLevel/SecurityStyle less confusing,
and remove the temptation to use SSLStatus::security_style
as an indicator of overall page security state instead of the
embedder-specific SecurityLevel.

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

Committed: https://crrev.com/47ba9c7d92885d137c51b462d8af87e3f3a6ecdd
Cr-Commit-Position: refs/heads/master@{#424436}
==========

[commit-bot: I haz the power, 2016-10-11 15:43:33]

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