Show these three cert errors as warnings, not errors (as requested by Ian):...

Show these two cert errors as warnings, not errors (as requested by Ian):

CERT_STATUS_UNABLE_TO_CHECK_REVOCATION
CERT_STATUS_NO_REVOCATION_MECHANISM

Note: The Omnibox shows no_revocation_mechanism as skull and bones, but that's a separate issue.

BUG=http://crbug.com/52916
TEST=Requires server with cert errors.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=59360

[wtc, 2010-09-13 19:32:20]

LGTM.

Note the comment marked with "UI ISSUE" below, and the
suggestion to not consider "weak signature algorithm" as
just a warning.

http://codereview.chromium.org/3293019/diff/1/3
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3293019/diff/1/3#newcode50
chrome/browser/page_info_model.cc:50: net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM;
Remove net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM from this list.

Nit: cert_warnings can be 'const'.

This needs to be consistent with SSLPolicy::OnCertError().
SSLPolicy::OnCertError() treats "weak signature algorithm"
as an overridable error.  Only "unable to check revocation"
and "no revocation mechanism" are treated as warnings.
(It actually ignores "no revocation mechanism".)

Nit: I'd write lines 51-52 in one line:
  int status_with_warnings_removed = ssl.cert_status() & ~cert_warnings;

http://codereview.chromium.org/3293019/diff/1/3#newcode59
chrome/browser/page_info_model.cc:59: if (cert_status & cert_warnings) {
UI ISSUE: It seems that for a "minor warning", we can still
show the certificate subject and issuer info in the
'description' string.

Note: no other browser reports the "unable to check revocation" warning.  So we
don't want to make our UI
too scary for this warning.

http://codereview.chromium.org/3293019/diff/1/3#newcode66
chrome/browser/page_info_model.cc:66: } else if (cert_status &
net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM) {
Remove this case.  "Weak signature algorithm" should be handled
as an error.

http://codereview.chromium.org/3293019/diff/1/3#newcode72
chrome/browser/page_info_model.cc:72: // OK HTTPS page.
This comment should now say
    // EV OK HTTPS page.
or more simply,
    // EV HTTPS page.

http://codereview.chromium.org/3293019/diff/1/3#newcode111
chrome/browser/page_info_model.cc:111: } else {
This is unrelated to your CL.  But it seems that we should
remove the "else" here.  I think lines 108-114 should be
written like this:
  if (issuer_name.empty()) {
    issuer_name.assign(l10n_util::GetStringUTF16(
        IDS_PAGE_INFO_SECURITY_TAB_UNKNOWN_PARTY));
  }
  description.assign(l10n_util::GetStringFUTF16(
      IDS_PAGE_INFO_SECURITY_TAB_SECURE_IDENTITY, issuer_name));

http://codereview.chromium.org/3293019/diff/1/3#newcode117
chrome/browser/page_info_model.cc:117: // HTTP or bad HTTPS.
This comment needs to be updated.

[ian fette, 2010-09-13 21:12:58]

I'm fine with keeping "weak signature" more severe. What I told Jay was based on
my incorrect recollection of what we were currently doing.

http://codereview.chromium.org/3293019/diff/1/3
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3293019/diff/1/3#newcode59
chrome/browser/page_info_model.cc:59: if (cert_status & cert_warnings) {
On 2010/09/13 19:32:20, wtc wrote:
> UI ISSUE: It seems that for a "minor warning", we can still
> show the certificate subject and issuer info in the
> 'description' string.
> 
> Note: no other browser reports the "unable to check revocation" warning.  So
we
> don't want to make our UI
> too scary for this warning.

I agree with wan-teh. We used to just infobar. That said, I think as long as
it's something not totally scary (gray lock, or some sort of exclamation mark /
informational icon) it's fine. Red skull and crossbones would be overkill for
the revocation status ones.

[Finnur, 2010-09-14 11:19:18]

I've removed the WeakSignature enum from the warnings list and addressed all
issues raised. I'll check in once green.

http://codereview.chromium.org/3293019/diff/1/3
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3293019/diff/1/3#newcode50
chrome/browser/page_info_model.cc:50: net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM;
> This needs to be consistent with SSLPolicy::OnCertError().

I presume since I've removed the WeakSignature enum that this needs no further
work?

http://codereview.chromium.org/3293019/diff/1/3#newcode111
chrome/browser/page_info_model.cc:111: } else {
Agreed.

On 2010/09/13 19:32:20, wtc wrote:
> This is unrelated to your CL.  But it seems that we should
> remove the "else" here.

[wtc, 2010-09-14 22:31:56]

LGTM.

http://codereview.chromium.org/3293019/diff/1/3
File chrome/browser/page_info_model.cc (right):

http://codereview.chromium.org/3293019/diff/1/3#newcode50
chrome/browser/page_info_model.cc:50: net::CERT_STATUS_WEAK_SIGNATURE_ALGORITHM;
On 2010/09/14 11:19:21, Finnur wrote:
>
> I presume since I've removed the WeakSignature enum that this needs no further
> work?

Correct.

http://codereview.chromium.org/3293019/diff/6001/7002#newcode71
chrome/browser/page_info_model.cc:71: } else if ((ssl.cert_status() &
net::CERT_STATUS_IS_EV) != 0) {
Strictly speaking, we should not use "else if" here, because
the two CERT_STATUS_ bits can be set at the same time.

In practice, Chrome never sets the
CERT_STATUS_NO_REVOCATION_MECHANISM bit, so the code here will
work.

Another way to look at it is that if both bits are set,
we will only report the CERT_STATUS_UNABLE_TO_CHECK_REVOCATION bit,
and that's a reasonable thing to do.