Mark SHA-1 as deprecated

Mark SHA-1 as deprecated

BUG=401365
Committed: https://crrev.com/4f80127285b5be9265ad96ce9b118e414107815a
Cr-Commit-Position: refs/heads/master@{#297331}

[Ryan Sleevi, 2014-08-28 04:20:15]

rsleevi@chromium.org changed reviewers:
+ felt@chromium.org, mkwst@chromium.org, palmer@chromium.org

[Ryan Sleevi, 2014-08-28 04:20:15]

felt, palmer, mkwst: I'm going to give you first crack at this before I pull in
a //net owner.

Thoughts?

(Note there's a set of dependent CLs to land before this)

[Ryan Sleevi, 2014-08-28 04:23:31]

https://codereview.chromium.org/508823009/diff/1/chrome/browser/ssl/ssl_error...
File chrome/browser/ssl/ssl_error_info.cc (right):

https://codereview.chromium.org/508823009/diff/1/chrome/browser/ssl/ssl_error...
chrome/browser/ssl/ssl_error_info.cc:153: break;
This was unrelated, but *cough cough*

https://codereview.chromium.org/508823009/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/508823009/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.cc:503: :
SITE_CONNECTION_STATUS_MIXED_CONTENT;
This is where we can control the icon used, if we want - red-x vs yellow
exclamation.

[palmer, 2014-08-28 21:23:13]

LGTM, pending the UI treatment decision.

https://codereview.chromium.org/508823009/diff/1/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/508823009/diff/1/chrome/app/chromium_strings....
chrome/app/chromium_strings.grd:613: The site is using outdated security
settings that may prevent future versions of Chromium from being able to safely
access it.
This string is OK, but I'd rather try to say something to the effect that Chrome
can't be sure it's the real site, or won't have strong assurance it's the right
site.

https://codereview.chromium.org/508823009/diff/1/chrome/browser/ssl/ssl_error...
File chrome/browser/ssl/ssl_error_info.cc (right):

https://codereview.chromium.org/508823009/diff/1/chrome/browser/ssl/ssl_error...
chrome/browser/ssl/ssl_error_info.cc:153: break;
*cough*

https://codereview.chromium.org/508823009/diff/1/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/508823009/diff/1/net/base/net_error_list.h#ne...
net/base/net_error_list.h:442: // After the deprecation date, this becomes
CERT_WEAK_SIGNATURE_ALGORITHM.
"this becomes" --> certs using deprecated algorithms change to triggering
CERT_WEAK_SIGNATURE_ALGORITHM.

https://codereview.chromium.org/508823009/diff/1/net/cert/cert_status_flags_l...
File net/cert/cert_status_flags_list.h (right):

https://codereview.chromium.org/508823009/diff/1/net/cert/cert_status_flags_l...
net/cert/cert_status_flags_list.h:28: // Deprecated means "Valid beyond the
deprecation period" (e.g. SHA-1 in 2017)
Nit: Punctuation

[Ryan Sleevi, 2014-08-28 21:25:15]

https://codereview.chromium.org/508823009/diff/1/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/508823009/diff/1/chrome/app/chromium_strings....
chrome/app/chromium_strings.grd:613: The site is using outdated security
settings that may prevent future versions of Chromium from being able to safely
access it.
On 2014/08/28 21:23:13, Chromium Palmer wrote:
> This string is OK, but I'd rather try to say something to the effect that
Chrome
> can't be sure it's the real site, or won't have strong assurance it's the
right
> site.

Care to chime in on the bug? I have string exhaustion from wording, so I just
went with felt :)

And it's not about not being sure it's the real site, it's that SHA-1 will be
disabled.

[felt, 2014-08-29 00:28:35]

https://codereview.chromium.org/508823009/diff/1/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/508823009/diff/1/chrome/app/chromium_strings....
chrome/app/chromium_strings.grd:613: The site is using outdated security
settings that may prevent future versions of Chromium from being able to safely
access it.
On 2014/08/28 21:25:14, Ryan Sleevi wrote:
> On 2014/08/28 21:23:13, Chromium Palmer wrote:
> > This string is OK, but I'd rather try to say something to the effect that
> Chrome
> > can't be sure it's the real site, or won't have strong assurance it's the
> right
> > site.
> 
> Care to chime in on the bug? I have string exhaustion from wording, so I just
> went with felt :)
> 
> And it's not about not being sure it's the real site, it's that SHA-1 will be
> disabled.

Where is this discussion happening now? Here, there?

[palmer, 2014-08-29 00:37:46]

> > > This string is OK, but I'd rather try to say something to the effect that
> > Chrome
> > > can't be sure it's the real site, or won't have strong assurance it's the
> > right
> > > site.
> > 
> > Care to chime in on the bug? I have string exhaustion from wording, so I
just
> > went with felt :)
> > 
> > And it's not about not being sure it's the real site, it's that SHA-1 will
be
> > disabled.
> 
> Where is this discussion happening now? Here, there?

Nowhere; I decided that the particulars of the string are not worth
bike-shedding. :) I'm happy enough with how it is here in the CL.

[Ryan Sleevi, 2014-09-24 02:50:35]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org, markusheintz@chromium.org
- mkwst@chromium.org

[Ryan Sleevi, 2014-09-24 02:50:36]

Adding David as a reviewer to get a sense of his hate for this, the same as
trying to get some early feedback from Markus.

Note that there's two different dates at play here - 2016 and 2017.

This causes the _omnibox_ to key off the 2017 date (for now), as planned for
M39. However, the PageInfo bubble (eg: "identity of this website has been
verified", etc) is keyed off the 2016 date. I think this meets the balance
between the two, but I'm happy to re-evaluate this is you wish.

Note that this also updates a few of our places checking IsCertStatusError to
ignore IsCertStatusMinorError, the gist of which I've described below.

I don't like spraying the policy all over the place (and this isn't the
histograms code yet, which will go in ResourceLoader to distinguish frame
types), but at the same time, each of these policy things are 'slightly'
different.

Across the board, //net reports the 2016-01-01 date, and it's up to the higher
layers (namely //chrome) to make an appropriate decision.

This _doesn't_ affect any custom UI bits - so I need to check into ios and
android and they might ALSO have policy sprinkled about them. If there's another
way you'd prefer to smuggle data, please let me know. The only way I can think
is content::SSLStatus, but that 'seems' wrong/layering issues. However, I'm
happy to be told to shut up, to put the evaluation and determination there, and
then make the UI decisions in the higher layers.

Thoughts?

https://codereview.chromium.org/508823009/diff/40001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_policy.cc (right):

https://codereview.chromium.org/508823009/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_policy.cc:113:
!net::IsCertStatusMinorError(info->ssl_cert_status())) {
This allows the following three errors to NOT be treated as mixed content

Unable to check revocation
No revocation mechanism
SHA-1

1&2 are treated as hard failures by enterprise policy, otherwise we'd never set
them. The latter seems correct - for now :)

[Ryan Sleevi, 2014-09-26 19:03:07]

rsleevi@chromium.org changed reviewers:
+ mkwst@chromium.org

[Ryan Sleevi, 2014-09-26 19:03:08]

Markus, David, Chris, Adrienne: Friendly Ping? Each of your inputs is desired
here.

[palmer, 2014-09-26 19:15:04]

Still LGTM.

https://codereview.chromium.org/508823009/diff/40001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/508823009/diff/40001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:73: // related UI treatment -
will increase in subsequent versions.
Don't you mean "decrease"?

https://codereview.chromium.org/508823009/diff/40001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_policy.cc (right):

https://codereview.chromium.org/508823009/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_policy.cc:113:
!net::IsCertStatusMinorError(info->ssl_cert_status())) {
Agree.

[davidben, 2014-09-26 20:09:24]

lgtm. The policy sprinkling is slightly odd, but I don't have significantly
better suggestions. CertStatus exposes a generic-seeming
DEPRECATED_SIGNATURE_ALGORITHM, but it's not especially clear what "deprecated"
means relative to the two dates. (And indeed the comment attached to the flag
was wrong. :-) ) And then other code enforces a SHA-1 specific date for a more
generic flag. (Not that we're deprecating two hashes at once.)

Another possibility would be to plumb two flags out of CertStatus, but I'm
having a hard time even naming them which is not a good sign.

Another option could be to export both DEPRECATED_SIGANTURE_ALGORITHM + has_sha1
on CertVerifyResult and then check has_sha1 + 2017 date in the place that did it
differently. Not sure if that's cleaner or not,

https://codereview.chromium.org/508823009/diff/40001/net/cert/cert_status_fla...
File net/cert/cert_status_flags.cc (right):

https://codereview.chromium.org/508823009/diff/40001/net/cert/cert_status_fla...
net/cert/cert_status_flags.cc:12: bool IsCertStatusMinorError(CertStatus
cert_status) {
Given all the places that are now checking both, perhaps an
IsCertStatusFatalError?

https://codereview.chromium.org/508823009/diff/40001/net/cert/cert_status_fla...
File net/cert/cert_status_flags_list.h (right):

https://codereview.chromium.org/508823009/diff/40001/net/cert/cert_status_fla...
net/cert/cert_status_flags_list.h:27: // Deprecated means "Valid beyond the
deprecation period" (e.g. SHA-1 in 2017)
This comment is off. The flag is actually set for 2016.

[Ryan Sleevi, 2014-09-26 20:24:33]

On 2014/09/26 20:09:24, David Benjamin wrote:
> lgtm. The policy sprinkling is slightly odd, but I don't have significantly
> better suggestions. CertStatus exposes a generic-seeming
> DEPRECATED_SIGNATURE_ALGORITHM, but it's not especially clear what
"deprecated"
> means relative to the two dates. (And indeed the comment attached to the flag
> was wrong. :-) ) And then other code enforces a SHA-1 specific date for a more
> generic flag. (Not that we're deprecating two hashes at once.)
> 
> Another possibility would be to plumb two flags out of CertStatus, but I'm
> having a hard time even naming them which is not a good sign.
> 
> Another option could be to export both DEPRECATED_SIGANTURE_ALGORITHM +
has_sha1
> on CertVerifyResult and then check has_sha1 + 2017 date in the place that did
it
> differently. Not sure if that's cleaner or not,

Yeah, this has been bothering me too.

After thinking about this more, I think it may be better to go from an error
flag (DEPRECATED_SIGNATURE_ALGORITHM) to a status flag (similar to IS_IV, we do
HAS_SHA1).
This paves the way for the eventual deprecation of RSA-1024 using similar
mechanisms, as an extra status flag.

The date bits will still sprinkle around, but I think that's the right decision.
The alternative would be to enshrine policy in
content/public/common/ssl_status.h, and I don't think that's quite right,
because it means date-based churn in a public header (e.g. each of the possible
policy ratchets requires updating ssl_status.h, which seems bad)

[Ryan Sleevi, 2014-09-27 01:00:43]

rsleevi@chromium.org changed reviewers:
+ isherman@chromium.org, pkasting@chromium.org

[Ryan Sleevi, 2014-09-27 01:00:44]

pkasting@chromium.org: Please review changes in chrome/browser/ui/toolbar and
chrome/browser/ui/website_settings

Please note that this is an attempt to keep UI treatment local to //chrome
(Opera wishes to use their own policies and UI treatment for deprecation). Note
that there is separate treatment for the Omnibox vs the Website Settings.

I'm curious if you have any insights/concerns with the checking of the field
trial buckets going on. These are meant to be the escape hatches for M39 onward,
so that iff ABSOLUTELY necessary, we can roll back changes for a release via
Finch. For M39, it's entirely Finch controlled.

isherman@chromium.org: Please review changes in 
histograms.xml

[davidben, 2014-09-29 20:21:32]

https://codereview.chromium.org/508823009/diff/100001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/508823009/diff/100001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:173: sha1_histogram =
SHA1_EXPIRES_AFTER_JANUARY_2016;
SHA1_EXPIRES_AFTER_JANUARY_201*7*

https://codereview.chromium.org/508823009/diff/100001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/508823009/diff/100001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:748:
!IsCertStatusMinorError(ssl_info.cert_status)) ||
Is this bit still necessary now that it's not CertStatus error?

https://codereview.chromium.org/508823009/diff/100001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:772:
!IsCertStatusMinorError(ssl_info.cert_status)) ||
Ditto.

[Peter Kasting, 2014-09-29 20:25:53]

LGTM

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:57: static const int64_t
kJanuary2016 = INT64_C(13096080000000000);
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.

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:74: return false;
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;

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:119: if (cert->valid_expiry() >=
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;

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:121: if
(GetSecurityLevelForFieldTrialGroup(
You should probably document why you're using field trials here and when you
expect to remove the field trial calls.

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:148: if ((ssl.cert_status &
net::CERT_STATUS_IS_EV) && cert.get())
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?

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:405: if (ssl.cert_status
& net::CERT_STATUS_IS_EV) {
This function is sure long with lots of nested conditionals.  I wonder if
there's any way to break it into sub-functions for greater readability?

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:453: static const int64_t
kSHA1LastIssuanceDate = INT64_C(13096080000000000);
See comments in toolbar_model_impl.cc.

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:461:
site_identity_details_ += UTF8ToUTF16("\n\n");
Nit: Stick a "+" on the end here and add in the next line.

[Ilya Sherman, 2014-09-29 21:09:18]

histograms lgtm, thanks

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

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/508823009/diff/100001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:148: if ((ssl.cert_status &
net::CERT_STATUS_IS_EV) && cert.get())
On 2014/09/29 20:25:52, Peter Kasting wrote:
> Nit: Technically, this assumes that RetrieveCert()'s return value matches
> whether |cert| is non-null.  Is that guaranteed for all time?

Yup!

> Do we need to
> check what the return value of that function was here, rather than checking
for
> |cert| being non-null?

Nope, because of the above.

[Ryan Sleevi, 2014-09-30 00:17:18]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2014-09-30 00:18:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/508823009/140001

[commit-bot: I haz the power, 2014-09-30 01:28:17]

Committed patchset #8 (id:140001) as 3ef5ae260e6d2833c0897ad6a74c243fa9abf5c5

[commit-bot: I haz the power, 2014-09-30 01:29:06]

Patchset 8 (id:??) landed as
https://crrev.com/4f80127285b5be9265ad96ce9b118e414107815a
Cr-Commit-Position: refs/heads/master@{#297331}

[felt, 2014-10-09 00:46:13]

sorry this CL got lost in my inbox. luckily i saw it so i can wince over that
break statement

https://codereview.chromium.org/508823009/diff/1/chrome/browser/ssl/ssl_error...
File chrome/browser/ssl/ssl_error_info.cc (right):

https://codereview.chromium.org/508823009/diff/1/chrome/browser/ssl/ssl_error...
chrome/browser/ssl/ssl_error_info.cc:153: break;
On 2014/08/28 21:23:13, Chromium Palmer wrote:
> *cough*

facepalm