Mark SHA-1 as deprecated

chrome/browser/ui/toolbar/toolbar_model_impl.cc

Index: chrome/browser/ui/toolbar/toolbar_model_impl.cc
diff --git a/chrome/browser/ui/toolbar/toolbar_model_impl.cc b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
index 0be1a71c7f96118091d577c2f5ef741845a2cb62..6233713f3774ddf889216194ce829b6f620c70f6 100644
--- a/chrome/browser/ui/toolbar/toolbar_model_impl.cc
+++ b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
@@ -5,8 +5,10 @@
 #include "chrome/browser/ui/toolbar/toolbar_model_impl.h"
 
 #include "base/command_line.h"
+#include "base/metrics/field_trial.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier_factory.h"
 #include "chrome/browser/autocomplete/chrome_autocomplete_scheme_classifier.h"
@@ -46,6 +48,34 @@ using content::NavigationEntry;
 using content::SSLStatus;
 using content::WebContents;
 
+namespace {
+
+// The internal representation of the dates for UI treatment of SHA-1.
+// See http://crbug.com/401365 for details
+static const int64_t kJanuary2017 = INT64_C(13127702400000000);
+static const int64_t kJune2016 = INT64_C(13109213000000000);
+static const int64_t kJanuary2016 = INT64_C(13096080000000000);

[Peter Kasting, 2014/09/29 20:25:52]

Nit: Declare constants in the most local scope possible (i.e. in the function
where you use them).

Seems like these could probably be constructed more safely and readably using
base::Time functions like FromUTCExploded().  It'd probably take more lines of
code and be less efficient, but I don't think either of those matters.

+
+// Converts a SHA-1 field trial group into the appropriate SecurityLevel.
+bool GetSecurityLevelForFieldTrialGroup(const std::string& group,
+                                        ToolbarModel::SecurityLevel* level) {
+  if (group == "Error") {
+    *level = ToolbarModel::SECURITY_ERROR;
+    return true;
+  }
+  if (group == "Warning") {
+    *level = ToolbarModel::SECURITY_WARNING;
+    return true;
+  }
+  if (group == "HTTP") {
+    *level = ToolbarModel::NONE;
+    return true;
+  }
+  return false;

[Peter Kasting, 2014/09/29 20:25:52]

Nit: Shorter:

  if (group == "Error")
    *level = ToolbarModel::SECURITY_ERROR;
  else if (group == "Warning")
    *level = ToolbarModel::SECURITY_WARNING;
  else if (group == "HTTP")
    *level = ToolbarModel::NONE;
  else
    return false;
  return true;

+}
+
+}  // namespace
+
 ToolbarModelImpl::ToolbarModelImpl(ToolbarModelDelegate* delegate)
     : delegate_(delegate) {
 }
@@ -82,12 +112,40 @@ ToolbarModel::SecurityLevel ToolbarModelImpl::GetSecurityLevelForWebContents(
 #endif
       if (!!(ssl.content_status & SSLStatus::DISPLAYED_INSECURE_CONTENT))
         return SECURITY_WARNING;
+      scoped_refptr<net::X509Certificate> cert;
+      if (content::CertStore::GetInstance()->RetrieveCert(ssl.cert_id, &cert) &&
+          (ssl.cert_status & net::CERT_STATUS_SHA1_SIGNATURE_PRESENT)) {
+        ToolbarModel::SecurityLevel security_level = NONE;
+        if (cert->valid_expiry() >=

[Peter Kasting, 2014/09/29 20:25:52]

Nit: I wonder if this could be condensed some by doing e.g.

  const char* field_trial_name = nullptr;
  if (cert->valid_expiry() >= kJanuary2017)
    field_trial_name = "SHA1ToolbarUIJanuary2017";
  else ...  // etc.
  if (field_trial_name && GetSecurityLevelForFieldTrialGroup(...))
    return security_level;

+            base::Time::FromInternalValue(kJanuary2017)) {
+          if (GetSecurityLevelForFieldTrialGroup(

[Peter Kasting, 2014/09/29 20:25:52]

You should probably document why you're using field trials here and when you
expect to remove the field trial calls.

+                  base::FieldTrialList::FindFullName(
+                      "SHA1ToolbarUIJanuary2017"),
+                  &security_level)) {
+            return security_level;
+          }
+        } else if (cert->valid_expiry() >=
+                   base::Time::FromInternalValue(kJune2016)) {
+          if (GetSecurityLevelForFieldTrialGroup(
+                  base::FieldTrialList::FindFullName("SHA1ToolbarUIJune2016"),
+                  &security_level)) {
+            return security_level;
+          }
+        } else if (cert->valid_expiry() >=
+                   base::Time::FromInternalValue(kJanuary2016)) {
+          if (GetSecurityLevelForFieldTrialGroup(
+                  base::FieldTrialList::FindFullName(
+                      "SHA1ToolbarUIJanuary2016"),
+                  &security_level)) {
+            return security_level;
+          }
+        }
+      }
       if (net::IsCertStatusError(ssl.cert_status)) {
         DCHECK(net::IsCertStatusMinorError(ssl.cert_status));
         return SECURITY_WARNING;
       }
-      if ((ssl.cert_status & net::CERT_STATUS_IS_EV) &&
-          content::CertStore::GetInstance()->RetrieveCert(ssl.cert_id, NULL))
+      if ((ssl.cert_status & net::CERT_STATUS_IS_EV) && cert.get())

[Peter Kasting, 2014/09/29 20:25:52]

Nit: Technically, this assumes that RetrieveCert()'s return value matches
whether |cert| is non-null.  Is that guaranteed for all time?  Do we need to
check what the return value of that function was here, rather than checking for
|cert| being non-null?

[Ryan Sleevi, 2014/09/30 00:15:50]

Yup!
Nope, because of the above.

         return EV_SECURE;
       return SECURE;
     }