Add new SecurityLevel for Http Bad state

components/security_state/security_state_model.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_SECURITY_STATE_SECURITY_STATE_MODEL_H_
 #define COMPONENTS_SECURITY_STATE_SECURITY_STATE_MODEL_H_
 
 #include <stdint.h>
 
 #include "base/macros.h"
@@ skipping 21 matching lines @@
   // If you reorder, add, or delete values from this enum, you must also
   // update the UI icons in ToolbarModelImpl::GetIconForSecurityLevel.
   //
   // A Java counterpart will be generated for this enum.
   // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.components.security_state
   // GENERATED_JAVA_CLASS_NAME_OVERRIDE: ConnectionSecurityLevel
   enum SecurityLevel {
     // HTTP/no URL/HTTPS but with insecure passive content on the page.
     NONE,
 
+    // CAUTION: Do ***not*** use this SecurityLevel state, except behind a
+    // switches::kMarkHttpAs flag! (https://crbug.com/647754)

[Peter Kasting, 2016/09/19 18:38:21]

Nit: Does this caution really belong here?  I'm worried about it getting left in
forever, and I'm wondering if its inclusion now really buys us much.  The
consequences for misuse are low (stuff doesn't work like you wanted) and the
people likely to misuse this are we folks implementing this stuff who know what
we're doing (this isn't broadly consumed).

[felt, 2016/09/19 18:54:09]

I'm afraid of someone randomly seeing it and trying to (mis)use it. What do you
think of that concern?

[Peter Kasting, 2016/09/19 18:58:30]

Why would someone randomly see it?  In what sort of way would they misuse it?

I'm not opposed to making misuse more difficult, but I'd like to understand the
nature of the theoretical misuse so we can figure out the best way to prevent
it.  I mean, maybe we need to tell people more about what this enum value means,
or how it should be displayed, or something, rather than just slapping a vague
warning on it.  Maybe the current warning just makes people guard their misuse
with a mis-check of this switch?

I would remove it if you don't have specific concerns about particular ways this
might be misused, though.

[felt, 2016/09/20 00:20:09]

Reworded, PTAL.

+    // HTTP, but something about the page (e.g., inclusion of private info)
+    // suggests we should show a warning for it.
+    HTTP_WARNING,

[Peter Kasting, 2016/09/19 18:38:21]

Hmm.  These names seem worrisome, especially since "SECURITY_WARNING"
technically applies to all three "warnings about security".  Trying to think of
better ones.

HTTP_WARNING -> INSECURE_HTTP (to match the warning text more closely?)

SECURITY_WARNING -> OUTDATED_PROTOCOL (to match the comment there?  Or is this
used in other cases)?

SECURITY_POLICY_WARNING -> LOCAL_CERTIFICATE (same rationale)

[felt, 2016/09/19 18:54:09]

SECURITY_WARNING is being deprecated in another CL by lgarron. Given that, are
you still concerned?

[Peter Kasting, 2016/09/19 18:58:30]

If that were gone (does deprecated mean "removed"?), this would be significantly
better.  That said, I'd still like to improve the names as much as we can, if we
can.  I don't know whether you think my suggestions represent an improvement.

[felt, 2016/09/20 00:20:09]

Lucas is removing SECURITY_WARNING completely, see
https://codereview.chromium.org/2329153002/diff/90001/components/security_sta....

I also agree that SECURITY_POLICY_WARNING is a terrible name. I'll refactor that
in a separate CL.

For this new state:
I don't like INSECURE_HTTP (HTTP is always insecure). And despite trying, I
can't think of anything better than HTTP_WARNING (an HTTP site where we show an
extra WARNING). My preference is to remove SECURITY_POLICY (as Lucas is doing),
rename SECURITY_POLICY_WARNING (as you suggest), and name this as HTTP_WARNING.

[Peter Kasting, 2016/09/20 00:50:05]

Yay.
Cool.
I knew that would be the counterargument :)
HTTP_SHOWING_WARNING?
VISIBLY_INSECURE_HTTP?
HTTP_NOTIFY_INSECURE?

Not sure any of these are better.

[felt, 2016/09/20 01:09:56]

Sold on HTTP_SHOW_WARNING.

+
     // HTTPS with valid EV cert.
     EV_SECURE,
 
     // HTTPS (non-EV) with valid cert.
     SECURE,
 
     // HTTPS, but with an outdated protocol version.
     SECURITY_WARNING,
 
     // HTTPS, but the certificate verification chain is anchored on a
@@ skipping 143 matching lines @@
   mutable VisibleSecurityState visible_security_state_;
 
   SecurityStateModelClient* client_;
 
   DISALLOW_COPY_AND_ASSIGN(SecurityStateModel);
 };
 
 }  // namespace security_state
 
 #endif  // COMPONENTS_SECURITY_STATE_SECURITY_STATE_MODEL_H_