Add SSLStatus flags to feed HTTP_WARNING security level

Add SSLStatus flags to feed HTTP_WARNING security level

This change adds SSLStatus flags to |content_status| for when the page
is detected to contain password or credit card fields over HTTP. Nothing
yet sets these SSLStatus flags.

When the flags are set (and the command-line switch), SecurityStateModel
downgrades the security level to HTTP_WARNING.

This is based on top of https://codereview.chromium.org/2346063002/

BUG=647558
Committed: https://crrev.com/a7040a0af3abeca232b0b4bf493d30f070e37d86
Cr-Commit-Position: refs/heads/master@{#419961}

[estark, 2016-09-19 20:05:02]

Description was changed from

==========
Add SSLStatus flags to feed HTTP_WARNING security level

This change adds SSLStatus flags to |content_status| for when the page
is detected to contain password or credit card fields over HTTP. Nothing
yet sets these SSLStatus flags.

When the flags are set (and the command-line switch), SecurityStateModel
downgrades the security level to HTTP_WARNING.

BUG=647558
==========

to

==========
Add SSLStatus flags to feed HTTP_WARNING security level

This change adds SSLStatus flags to |content_status| for when the page
is detected to contain password or credit card fields over HTTP. Nothing
yet sets these SSLStatus flags.

When the flags are set (and the command-line switch), SecurityStateModel
downgrades the security level to HTTP_WARNING.

This is based on top of https://codereview.chromium.org/2346063002/

BUG=647558
==========

[estark, 2016-09-20 02:24:54]

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

[estark, 2016-09-20 02:24:54]

felt, PTAL. (I know I still need to rebase on top of your HTTP_WARNING rename)

[felt, 2016-09-20 06:09:42]

looks good % rebase and suggestions

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
File components/security_state/security_state_model.cc (right):

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
components/security_state/security_state_model.cc:25: DUBIOUS,
since you're updating the histogram anyway, what do you think of making a new
one (replacement histogram) without DUBIOUS?

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
File components/security_state/security_state_model.h (right):

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
components/security_state/security_state_model.h:179: bool
displayed_nonsecure_password_field;
nit, here and elsewhere:

let's say HTTP instead of non-secure, to be clear. at least until we invent a
less confusing phrase.

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
File components/security_state/security_state_model_unittest.cc (right):

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
components/security_state/security_state_model_unittest.cc:106: }
this seems brittle -- why is the override needed?

[estark, 2016-09-20 17:51:52]

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

[commit-bot: I haz the power, 2016-09-20 17:52:18]

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

[estark, 2016-09-20 17:53:32]

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

[estark, 2016-09-20 17:53:33]

Thanks felt. Addressed your comments, adding some more owners...

jam: for content/public/browser/ssl_status.h
jwd: for histograms.xml

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
File components/security_state/security_state_model.cc (right):

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
components/security_state/security_state_model.cc:25: DUBIOUS,
On 2016/09/20 06:09:41, felt wrote:
> since you're updating the histogram anyway, what do you think of making a new
> one (replacement histogram) without DUBIOUS?

Done.

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
File components/security_state/security_state_model.h (right):

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
components/security_state/security_state_model.h:179: bool
displayed_nonsecure_password_field;
On 2016/09/20 06:09:41, felt wrote:
> nit, here and elsewhere:
> 
> let's say HTTP instead of non-secure, to be clear. at least until we invent a
> less confusing phrase.

Oh yes, I forgot about that. Done. I find "displayed_http_password_field"
confusing because it could be interpreted as a password field in a form that
*submits* to http, so I made it |displayed_password_field_on_http|, and similar
elsewhere.

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
File components/security_state/security_state_model_unittest.cc (right):

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
components/security_state/security_state_model_unittest.cc:106: }
On 2016/09/20 06:09:41, felt wrote:
> this seems brittle -- why is the override needed?

The natural thing would be to use content::IsOriginSecure() here, but we can't
depend on content.

I could just do url.SchemeIsCryptographic() which is kinda cheating but is less
brittle, wdyt?

[jam, 2016-09-20 19:25:50]

On 2016/09/20 17:53:33, estark wrote:
> Thanks felt. Addressed your comments, adding some more owners...
> 
> jam: for content/public/browser/ssl_status.h

How will this be set in the future? I ask because the autofill/credit card code
is outside content, so since content won't set this, or won't use it, is there a
way to keep it outside?

[commit-bot: I haz the power, 2016-09-20 19:28:44]

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

[commit-bot: I haz the power, 2016-09-20 19:28:45]

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...)

[felt, 2016-09-20 19:36:13]

lgtm

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
File components/security_state/security_state_model_unittest.cc (right):

https://codereview.chromium.org/2350273002/diff/40001/components/security_sta...
components/security_state/security_state_model_unittest.cc:106: }
On 2016/09/20 17:53:33, estark wrote:
> On 2016/09/20 06:09:41, felt wrote:
> > this seems brittle -- why is the override needed?
> 
> The natural thing would be to use content::IsOriginSecure() here, but we can't
> depend on content.
> 
> I could just do url.SchemeIsCryptographic() which is kinda cheating but is
less
> brittle, wdyt?

Ahh right, content dependency. Umm-- yes, I think SchemeIsCryptographic is
better at least.

[felt, 2016-09-20 19:36:50]

On 2016/09/20 19:25:50, jam wrote:
> On 2016/09/20 17:53:33, estark wrote:
> > Thanks felt. Addressed your comments, adding some more owners...
> > 
> > jam: for content/public/browser/ssl_status.h
> 
> How will this be set in the future? I ask because the autofill/credit card
code
> is outside content, so since content won't set this, or won't use it, is there
a
> way to keep it outside?

driveby note: the autofill/credit card code is in the content layer of a
component

[jam, 2016-09-20 19:42:19]

On 2016/09/20 19:36:50, felt wrote:
> On 2016/09/20 19:25:50, jam wrote:
> > On 2016/09/20 17:53:33, estark wrote:
> > > Thanks felt. Addressed your comments, adding some more owners...
> > > 
> > > jam: for content/public/browser/ssl_status.h
> > 
> > How will this be set in the future? I ask because the autofill/credit card
> code
> > is outside content, so since content won't set this, or won't use it, is
there
> a
> > way to keep it outside?
> 
> driveby note: the autofill/credit card code is in the content layer of a
> component

right, that's an implementation detail of our layered component (i.e. if we
didn't have iOS, that'd still be in components/autofill or maybe just chrome/).
https://www.chromium.org/developers/content-module/content-api has some
guidelines that we use for what goes into src/content/public. the part here that
i'm not sure about is that these new values seem like they won't be read or
written by src/content

[felt, 2016-09-20 19:46:13]

On 2016/09/20 19:42:19, jam wrote:
> On 2016/09/20 19:36:50, felt wrote:
> > On 2016/09/20 19:25:50, jam wrote:
> > > On 2016/09/20 17:53:33, estark wrote:
> > > > Thanks felt. Addressed your comments, adding some more owners...
> > > > 
> > > > jam: for content/public/browser/ssl_status.h
> > > 
> > > How will this be set in the future? I ask because the autofill/credit card
> > code
> > > is outside content, so since content won't set this, or won't use it, is
> there
> > a
> > > way to keep it outside?
> > 
> > driveby note: the autofill/credit card code is in the content layer of a
> > component
> 
> right, that's an implementation detail of our layered component (i.e. if we
> didn't have iOS, that'd still be in components/autofill or maybe just
chrome/).
> https://www.chromium.org/developers/content-module/content-api has some
> guidelines that we use for what goes into src/content/public. the part here
that
> i'm not sure about is that these new values seem like they won't be read or
> written by src/content

WebContentsImpl and SSLManager need to know about the status to set and
propagate it

[jam, 2016-09-20 19:53:02]

On 2016/09/20 19:46:13, felt wrote:
> On 2016/09/20 19:42:19, jam wrote:
> > On 2016/09/20 19:36:50, felt wrote:
> > > On 2016/09/20 19:25:50, jam wrote:
> > > > On 2016/09/20 17:53:33, estark wrote:
> > > > > Thanks felt. Addressed your comments, adding some more owners...
> > > > > 
> > > > > jam: for content/public/browser/ssl_status.h
> > > > 
> > > > How will this be set in the future? I ask because the autofill/credit
card
> > > code
> > > > is outside content, so since content won't set this, or won't use it, is
> > there
> > > a
> > > > way to keep it outside?
> > > 
> > > driveby note: the autofill/credit card code is in the content layer of a
> > > component
> > 
> > right, that's an implementation detail of our layered component (i.e. if we
> > didn't have iOS, that'd still be in components/autofill or maybe just
> chrome/).
> > https://www.chromium.org/developers/content-module/content-api has some
> > guidelines that we use for what goes into src/content/public. the part here
> that
> > i'm not sure about is that these new values seem like they won't be read or
> > written by src/content
> 
> WebContentsImpl and SSLManager need to know about the status to set and
> propagate it

Is the patch that will set/get this available for me to look at? I presumed that
it'll be set outside of src/content since that's where the autofill code is. but
perhaps what i'm missing is that content/ will have to read this to drop the ssl
status icon?

[felt, 2016-09-20 19:56:53]

On 2016/09/20 19:53:02, jam wrote:
> On 2016/09/20 19:46:13, felt wrote:
> > On 2016/09/20 19:42:19, jam wrote:
> > > On 2016/09/20 19:36:50, felt wrote:
> > > > On 2016/09/20 19:25:50, jam wrote:
> > > > > On 2016/09/20 17:53:33, estark wrote:
> > > > > > Thanks felt. Addressed your comments, adding some more owners...
> > > > > > 
> > > > > > jam: for content/public/browser/ssl_status.h
> > > > > 
> > > > > How will this be set in the future? I ask because the autofill/credit
> card
> > > > code
> > > > > is outside content, so since content won't set this, or won't use it,
is
> > > there
> > > > a
> > > > > way to keep it outside?
> > > > 
> > > > driveby note: the autofill/credit card code is in the content layer of a
> > > > component
> > > 
> > > right, that's an implementation detail of our layered component (i.e. if
we
> > > didn't have iOS, that'd still be in components/autofill or maybe just
> > chrome/).
> > > https://www.chromium.org/developers/content-module/content-api has some
> > > guidelines that we use for what goes into src/content/public. the part
here
> > that
> > > i'm not sure about is that these new values seem like they won't be read
or
> > > written by src/content
> > 
> > WebContentsImpl and SSLManager need to know about the status to set and
> > propagate it
> 
> Is the patch that will set/get this available for me to look at? I presumed
that
> it'll be set outside of src/content since that's where the autofill code is.
but
> perhaps what i'm missing is that content/ will have to read this to drop the
ssl
> status icon?

No, not yet. You can check out the design doc:
https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6...

This will also need to go through DevTools, and WebContentsImpl is already
responsible for notifying all of the toolbar observers about SSL UI changes.

[jam, 2016-09-20 20:18:03]

On 2016/09/20 19:56:53, felt wrote:
> On 2016/09/20 19:53:02, jam wrote:
> > On 2016/09/20 19:46:13, felt wrote:
> > > On 2016/09/20 19:42:19, jam wrote:
> > > > On 2016/09/20 19:36:50, felt wrote:
> > > > > On 2016/09/20 19:25:50, jam wrote:
> > > > > > On 2016/09/20 17:53:33, estark wrote:
> > > > > > > Thanks felt. Addressed your comments, adding some more owners...
> > > > > > > 
> > > > > > > jam: for content/public/browser/ssl_status.h
> > > > > > 
> > > > > > How will this be set in the future? I ask because the
autofill/credit
> > card
> > > > > code
> > > > > > is outside content, so since content won't set this, or won't use
it,
> is
> > > > there
> > > > > a
> > > > > > way to keep it outside?
> > > > > 
> > > > > driveby note: the autofill/credit card code is in the content layer of
a
> > > > > component
> > > > 
> > > > right, that's an implementation detail of our layered component (i.e. if
> we
> > > > didn't have iOS, that'd still be in components/autofill or maybe just
> > > chrome/).
> > > > https://www.chromium.org/developers/content-module/content-api has some
> > > > guidelines that we use for what goes into src/content/public. the part
> here
> > > that
> > > > i'm not sure about is that these new values seem like they won't be read
> or
> > > > written by src/content
> > > 
> > > WebContentsImpl and SSLManager need to know about the status to set and
> > > propagate it
> > 
> > Is the patch that will set/get this available for me to look at? I presumed
> that
> > it'll be set outside of src/content since that's where the autofill code is.
> but
> > perhaps what i'm missing is that content/ will have to read this to drop the
> ssl
> > status icon?
> 
> No, not yet. You can check out the design doc:
>
https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6...
> 
> This will also need to go through DevTools, and WebContentsImpl is already
> responsible for notifying all of the toolbar observers about SSL UI changes.

lgtm, thanks for the background

[jwd, 2016-09-20 22:25:12]

histograms LGTM

[estark, 2016-09-20 23:19:38]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-09-20 23:20:12]

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-21 02:19:40]

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

[commit-bot: I haz the power, 2016-09-21 02:19:41]

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...)

[estark, 2016-09-21 02:23:54]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-09-21 02:24:33]

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-21 03:08:18]

Description was changed from

==========
Add SSLStatus flags to feed HTTP_WARNING security level

This change adds SSLStatus flags to |content_status| for when the page
is detected to contain password or credit card fields over HTTP. Nothing
yet sets these SSLStatus flags.

When the flags are set (and the command-line switch), SecurityStateModel
downgrades the security level to HTTP_WARNING.

This is based on top of https://codereview.chromium.org/2346063002/

BUG=647558
==========

to

==========
Add SSLStatus flags to feed HTTP_WARNING security level

This change adds SSLStatus flags to |content_status| for when the page
is detected to contain password or credit card fields over HTTP. Nothing
yet sets these SSLStatus flags.

When the flags are set (and the command-line switch), SecurityStateModel
downgrades the security level to HTTP_WARNING.

This is based on top of https://codereview.chromium.org/2346063002/

BUG=647558
==========

[commit-bot: I haz the power, 2016-09-21 03:08:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-21 03:09:37]

Description was changed from

==========
Add SSLStatus flags to feed HTTP_WARNING security level

This change adds SSLStatus flags to |content_status| for when the page
is detected to contain password or credit card fields over HTTP. Nothing
yet sets these SSLStatus flags.

When the flags are set (and the command-line switch), SecurityStateModel
downgrades the security level to HTTP_WARNING.

This is based on top of https://codereview.chromium.org/2346063002/

BUG=647558
==========

to

==========
Add SSLStatus flags to feed HTTP_WARNING security level

This change adds SSLStatus flags to |content_status| for when the page
is detected to contain password or credit card fields over HTTP. Nothing
yet sets these SSLStatus flags.

When the flags are set (and the command-line switch), SecurityStateModel
downgrades the security level to HTTP_WARNING.

This is based on top of https://codereview.chromium.org/2346063002/

BUG=647558

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

[commit-bot: I haz the power, 2016-09-21 03:09:38]

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