Mark SHA-1 as deprecated

chrome/browser/ui/toolbar/toolbar_model_impl.cc

 // Copyright 2012 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.
 
 #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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/ssl/ssl_error_info.h"
 #include "chrome/browser/ui/toolbar/toolbar_model_delegate.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
@@ skipping 19 matching lines @@
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/policy/policy_cert_service.h"
 #include "chrome/browser/chromeos/policy/policy_cert_service_factory.h"
 #endif
 
 using content::NavigationController;
 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) {
 }
 
 ToolbarModelImpl::~ToolbarModelImpl() {
 }
 
 // static
 ToolbarModel::SecurityLevel ToolbarModelImpl::GetSecurityLevelForWebContents(
       content::WebContents* web_contents) {
@@ skipping 16 matching lines @@
     case content::SECURITY_STYLE_AUTHENTICATED: {
 #if defined(OS_CHROMEOS)
       policy::PolicyCertService* service =
           policy::PolicyCertServiceFactory::GetForProfile(
               Profile::FromBrowserContext(web_contents->GetBrowserContext()));
       if (service && service->UsedPolicyCertificates())
         return SECURITY_POLICY_WARNING;
 #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;
     }
     default:
       NOTREACHED();
       return NONE;
   }
 }
 
 // ToolbarModelImpl Implementation.
@@ skipping 228 matching lines @@
   if (entry &&
       google_util::StartsWithCommandLineGoogleBaseURL(entry->GetVirtualURL()))
     return search_terms;
 
   // Otherwise, extract search terms for HTTPS pages that do not have a security
   // error.
   ToolbarModel::SecurityLevel security_level = GetSecurityLevel(ignore_editing);
   return ((security_level == NONE) || (security_level == SECURITY_ERROR)) ?
       base::string16() : search_terms;
 }