net: make HSTS hosts use the normal SSL interstitials

net: make HSTS hosts use the normal SSL interstitials

SSL interstitials have better translations for the error messages and this
returns us to the point where we have only a single UI for SSL errors, which
will make some future changes easier.

First, this change changes the SSL error callbacks to take an SSLInfo& rather
than a X509Certificate* (which was already a TODO(wtc) in the code). Most of
this change is the resulting plumbing.

It also adds a |is_hsts_host| flag to the callbacks to denote an HSTS host.

Finally, in ssl_policy.cc the |is_hsts_host| flag causes any error to be
fatal.

BUG=93527
TEST=none

[wtc, 2011-09-23 00:04:51]

Review comments on Patch Set 2:

High-level comments:

1. I suggest removing the "int cert_error" argument
because it can be derived from ssl_info.cert_status.

2. I suggest passing is_hsts_host instead of
must_be_fatal, and let the SSL policy decide whether
the cert error should be fatal based on that info.

Low-level comments follow.

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_cert_e...
File content/browser/ssl/ssl_cert_error_handler.cc (left):

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_cert_e...
content/browser/ssl/ssl_cert_error_handler.cc:24:
ssl_info_.SetCertError(cert_error);
This is interesting -- so in the original code, ssl_info_
only has the |cert| and |cert_status| members properly
initialized.

I found that we only use the |cert|, |cert_status|, and
|security_bits| members of ssl_info_.  The default value
of |security_bits| is -1.  Could you find out in a debugger
if ssl_info.security_bits (note it's the |ssl_info| argument)
is also -1 at this point?  Thanks.

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_cert_e...
File content/browser/ssl/ssl_cert_error_handler.h (right):

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_cert_e...
content/browser/ssl/ssl_cert_error_handler.h:49: const bool must_be_fatal_;  //
true if the error is from an HSTS host.
To be precise, this should also say "and the error is not
"ERR_CERT_UNABLE_TO_CHECK_REVOCATION".

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_policy.cc
File content/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_policy...
content/browser/ssl/ssl_policy.cc:61: OnCertErrorInternal(handler, true);
We can pass !handler->must_be_fatal() instead of true
as the second argument here.  See also the next comment below.

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_policy...
content/browser/ssl/ssl_policy.cc:201: overridable = false;
I think it is better to move this code to the
SSLPolicy::OnCertError() method above.

http://codereview.chromium.org/7976036/diff/21/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7976036/diff/21/net/url_request/url_request.h#...
net/url_request/url_request.h:276: bool must_be_fatal);
The cert_error argument should be removed because it can
be derived from ssl_info.cert_status by calling
net::MapCertStatusToNetError(ssl_info.cert_status).

http://codereview.chromium.org/7976036/diff/21/net/url_request/url_request_ht...
File net/url_request/url_request_http_job.cc (left):

http://codereview.chromium.org/7976036/diff/21/net/url_request/url_request_ht...
net/url_request/url_request_http_job.cc:693: // ssl_info.
The ssl_info structure has become much bigger than it was
when I wrote this comment.  So I was wondering if it's better
to pass only ssl_info.cert_status.

After I reviewed the entire CL, I believe it is better to
pass ssl_info.

http://codereview.chromium.org/7976036/diff/21/net/url_request/url_request_ht...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/7976036/diff/21/net/url_request/url_request_ht...
net/url_request/url_request_http_job.cc:694: const bool r =
context_->transport_security_state()->IsEnabledForHost(
Please pick a better variable name than |r|.

http://codereview.chromium.org/7976036/diff/21/net/url_request/url_request_ht...
net/url_request/url_request_http_job.cc:702: result,
transaction_->GetResponseInfo()->ssl_info, must_be_fatal);
In our current design of SSL error handling, I believe it is
better to simply pass the boolean info "is HSTS host" to
NotifySSLCertificateError, and let the SSL policy decide
whether the certificate error should be fatal.

http://codereview.chromium.org/7976036/diff/21/net/url_request/url_request_ht...
net/url_request/url_request_http_job.cc:729: bool
URLRequestHttpJob::ShouldTreatAsCertificateError(int result) {
We should remove the ShouldTreatAsCertificateError method.

http://codereview.chromium.org/7976036/diff/21/webkit/fileapi/file_writer_del...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/7976036/diff/21/webkit/fileapi/file_writer_del...
webkit/fileapi/file_writer_delegate.cc:160: bool must_be_fatal) {
Probably should use the formatting style recommended in
http://www.chromium.org/developers/coding-style#TOC-Code-formatting

(Search for "OK3".)

[agl, 2011-09-23 18:45:38]

Have removed the |cert_result| parameter and made |must_be_fatal| into
|is_hsts_host|. The logic around whether they should be fatal is in
ssl_policy.cc now.

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_cert_e...
File content/browser/ssl/ssl_cert_error_handler.cc (left):

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_cert_e...
content/browser/ssl/ssl_cert_error_handler.cc:24:
ssl_info_.SetCertError(cert_error);
On 2011/09/23 00:04:51, wtc wrote:
> Could you find out in a debugger
> if ssl_info.security_bits (note it's the |ssl_info| argument)
> is also -1 at this point?  Thanks.

ssl_info.security_bits has the value that you would expect (i.e. 128 for most
sites).

[wtc, 2011-09-26 19:15:09]

LGTM on Patch Set 4.  Thanks!

Please update the CL's commit message before you check this
in.  It still mentions the old must_be_fatal parameter name.

Please fix the following nits before checkin.

http://codereview.chromium.org/7976036/diff/9002/content/browser/ssl/ssl_cert...
File content/browser/ssl/ssl_cert_error_handler.cc (right):

http://codereview.chromium.org/7976036/diff/9002/content/browser/ssl/ssl_cert...
content/browser/ssl/ssl_cert_error_handler.cc:7: #include
"net/base/cert_status_flags.h"
List this header before "net/base/x509_certificate.h".

http://codereview.chromium.org/7976036/diff/9002/content/browser/ssl/ssl_mana...
File content/browser/ssl/ssl_manager.cc (right):

http://codereview.chromium.org/7976036/diff/9002/content/browser/ssl/ssl_mana...
content/browser/ssl/ssl_manager.cc:31: << " url: " << request->url().spec();
Optional: I suggest we log cert_status instead of cert_error.
You can add a TODO if you don't want to make the change now.

cert_status is a bit field, so we'd want to log it in hex,
perhaps with a std::hex iostream manipulator.

http://codereview.chromium.org/7976036/diff/9002/net/proxy/proxy_script_fetch...
File net/proxy/proxy_script_fetcher_impl.cc (right):

http://codereview.chromium.org/7976036/diff/9002/net/proxy/proxy_script_fetch...
net/proxy/proxy_script_fetcher_impl.cc:199: result_code_ =
net::MapCertStatusToNetError(ssl_info.cert_status);
Remove net:: because this file is in the net namespace.

http://codereview.chromium.org/7976036/diff/9002/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/7976036/diff/9002/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:700: is_hsts_host);
We can consider making is_hsts_host a bit flag in
ssl_info.cert_status.  But that requires making
context_->transport_security_state() available to the
lower layers (at least the HttpCache layer, which saves
ssl_info in the HTTP cache).  So I am not sure if it is
worth doing.

http://codereview.chromium.org/7976036/diff/9002/webkit/fileapi/file_writer_d...
File webkit/fileapi/file_writer_delegate.h (right):

http://codereview.chromium.org/7976036/diff/9002/webkit/fileapi/file_writer_d...
webkit/fileapi/file_writer_delegate.h:51: bool must_be_fatal) OVERRIDE;
must_be_fatal => is_hsts_host

[wtc, 2011-09-26 19:18:48]

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_cert_e...
File content/browser/ssl/ssl_cert_error_handler.cc (left):

http://codereview.chromium.org/7976036/diff/21/content/browser/ssl/ssl_cert_e...
content/browser/ssl/ssl_cert_error_handler.cc:24:
ssl_info_.SetCertError(cert_error);
On 2011/09/23 18:45:38, agl wrote:
>
> ssl_info.security_bits has the value that you would expect (i.e. 128 for most
> sites).

That should be fine.  Could be considered an improvement :-)