Handle all non-overridable SSL errors in the same place

This causes ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY and ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN errors to be handled by the SSL interstitial, so that we can present a single unified non-overridable warning.

BUG=276540
R=rsleevi@chromium.org
TBR=abarth@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229813

[felt, 2013-09-07 01:55:43]

Hi Ryan,

Can you please review this CL?

Also, are there errors other than ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY and
ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN that need to be handled here?

Thanks,
Adrienne

[Ryan Sleevi, 2013-09-10 20:51:27]

Today these errors are currently handled by the standard "oh crap" general
network error screen, right?

What benefit is to be had by making them show the interstitial screen, given
that they're non-overridable?

How much are these certificate errors versus policy errors versus transport
errors?

https://codereview.chromium.org/23908005/diff/7001/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/23908005/diff/7001/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:897: // the correct cert status, so we
set it here.
comment nit: Can you drop the "we", leaving the comment more concise.

// so set it here.

https://codereview.chromium.org/23908005/diff/7001/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:903: // Ask our delegate to decide what
we should do.
comment nit:
// The error may be overridable. Ask the delegate to decide.

avoids the pronouns, and avoids the "maybe, maybe not" construct.

[felt, 2013-09-10 20:58:14]

On 2013/09/10 20:51:27, Ryan Sleevi wrote:
> Today these errors are currently handled by the standard "oh crap" general
> network error screen, right?
> 
> What benefit is to be had by making them show the interstitial screen, given
> that they're non-overridable?

I made a completely new blocking warning for the "you can't proceed" case (see
https://codereview.chromium.org/23965003/). It's meant for cert pinning
failures, HSTS failures, etc. It is very clear in that version of the error page
that you can't proceed. However, I don't want to have to implement it twice,
hence this CL.

> How much are these certificate errors versus policy errors versus transport
> errors?

The new blocking error page makes it clear that it's a connection error:

https://chromium.googlecode.com/issues/attachment?aid=2765400016000&name=Scre...

(if that link doesn't work, go to the bug and scroll to the bottom to see the
final screenshots)

> 
>
https://codereview.chromium.org/23908005/diff/7001/net/url_request/url_reques...
> File net/url_request/url_request_http_job.cc (right):
> 
>
https://codereview.chromium.org/23908005/diff/7001/net/url_request/url_reques...
> net/url_request/url_request_http_job.cc:897: // the correct cert status, so we
> set it here.
> comment nit: Can you drop the "we", leaving the comment more concise.
> 
> // so set it here.
> 
>
https://codereview.chromium.org/23908005/diff/7001/net/url_request/url_reques...
> net/url_request/url_request_http_job.cc:903: // Ask our delegate to decide
what
> we should do.
> comment nit:
> // The error may be overridable. Ask the delegate to decide.
> 
> avoids the pronouns, and avoids the "maybe, maybe not" construct.

[felt, 2013-09-10 21:03:21]

https://codereview.chromium.org/23908005/diff/7001/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/23908005/diff/7001/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:897: // the correct cert status, so we
set it here.
On 2013/09/10 20:51:27, Ryan Sleevi wrote:
> comment nit: Can you drop the "we", leaving the comment more concise.
> 
> // so set it here.

Done.

https://codereview.chromium.org/23908005/diff/7001/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:903: // Ask our delegate to decide what
we should do.
On 2013/09/10 20:51:27, Ryan Sleevi wrote:
> comment nit:
> // The error may be overridable. Ask the delegate to decide.
> 
> avoids the pronouns, and avoids the "maybe, maybe not" construct.

Done.

[Ryan Sleevi, 2013-09-10 21:20:57]

Sorry I wasn't clear in the question.

I just wanted to make sure that they're currently handled by the general net
error page today.

The comment about not needing some of the localized text is what threw me. I'm a
little nervous, because it couples some of the implementation details - that is,
the providing of the error details to the actual page - but think that's
probably a minimal concern.

[felt, 2013-09-10 22:04:33]

On 2013/09/10 21:20:57, Ryan Sleevi wrote:
> Sorry I wasn't clear in the question.
> 
> I just wanted to make sure that they're currently handled by the general net
> error page today.

Ah, yes, they are.

> The comment about not needing some of the localized text is what threw me. I'm
a
> little nervous, because it couples some of the implementation details - that
is,
> the providing of the error details to the actual page - but think that's
> probably a minimal concern.

I added error details to the SSLErrorInfo. In general, though, I want to
refactor SSLErrorInfo so that it knows whether an error is overridable. That
would let me move some strings out of the SSLBlockingPage and into SSLErrorInfo.
However I think that refactoring is a separate CL.

[felt, 2013-09-12 12:34:15]

On 2013/09/10 22:04:33, felt wrote:
> On 2013/09/10 21:20:57, Ryan Sleevi wrote:
> > Sorry I wasn't clear in the question.
> > 
> > I just wanted to make sure that they're currently handled by the general net
> > error page today.
> 
> Ah, yes, they are.
> 
> > The comment about not needing some of the localized text is what threw me.
I'm
> a
> > little nervous, because it couples some of the implementation details - that
> is,
> > the providing of the error details to the actual page - but think that's
> > probably a minimal concern.
> 
> I added error details to the SSLErrorInfo. In general, though, I want to
> refactor SSLErrorInfo so that it knows whether an error is overridable. That
> would let me move some strings out of the SSLBlockingPage and into
SSLErrorInfo.
> However I think that refactoring is a separate CL.

Are there any other changes you wanted me to make?

[Ryan Sleevi, 2013-09-12 16:43:58]

lgtm

[felt, 2013-10-21 15:13:40]

Committed patchset #7 manually as r229813 (presubmit successful).