Don't use X509Certificate in SSLConfig.

Don't use X509Certificate in SSLConfig.

X509Certificate class depends in OS-dependant APIs and hense cannot 
be created inside of sandbox. This change allows specifying 
allow_bed_certs when running inside of sandbox.

BUG=80587
TEST=Unittests

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

[Sergey Ulanov, 2011-07-15 20:29:25]

Wen-Teh, this is an alternative solutin for the problem we have with
X509Certificate when running SSL in sandbox.
It's much simpler than refactoring X509Certificate code.

I would still like to fix certificates code to make it work inside of sandbox,
but this change makes this work less time-critical.

[wtc, 2011-07-19 21:57:01]

LGTM.  CAVEAT: this CL may increase memory usage.  SSLConfig
objects are often copied because we assume it is small,
containing just a few bool fields and pointers.  A DER encoded
certificate is usually 1000 - 1500 bytes.

Since this bug blocks your feature on Mac OS X and I don't
have time to explore alternative solutions (such as
initializing the Mac system certificate library before
starting the sandbox or adding an undecoded X509Certificate
type), I approve this change.  The IsAllowedBadCert code is
only used to handle certificate errors or to trust
self-signed certificates that we've verified out of band.
Increased memory usage should be acceptable in these two
cases.

I have some suggested changes below.

http://codereview.chromium.org/7401003/diff/4011/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/7401003/diff/4011/net/base/ssl_config_service....
net/base/ssl_config_service.h:30: bool IsAllowedBadCert(const std::string&
cert_der, int* cert_status) const;
Please document the new IsAllowedBadCert variant.

It may be better to use const StringPiece& instead of
const std::string&.  This may allow us to avoid copying
in some cases.

I assume it is fine to use function overloading here even
though the Style Guide recommends against it in general:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

Nit: please name the argument "der_cert".  The full
description is "DER encoded certificate", so "der_cert" is
closer to that than "cert_der".  Please do this renaming
throughout the CL, at least in the files under src/net.

If the renaming requires changing too many files, don't do
it.

http://codereview.chromium.org/7401003/diff/4011/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/7401003/diff/4011/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:229: DCHECK(handle);
You can also replace this DCHECK with proper error handling.

http://codereview.chromium.org/7401003/diff/4011/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7401003/diff/4011/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:1018: return error;
Are you sure ssl_info_.cert may be NULL here?  I don't
think this function is executed in the sandbox.

Please add a comment to explain why ssl_info.cert may be
NULL.

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1041: // Returns server_cert_.
Remove "Returns server_cert_."

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1049: certs.AsStringPieceVector());
Please add a comment here that this may fail in the sandbox.

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1531: // code is used inside of sandbox.
Nit: remove "of" on this line and line 1546 below.

Remove the "j" at the end of line 1546.

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1534: server_cert_nss_->derCert.len);
Using StringPiece here would avoid the copying.

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1548: return ERR_CERT_INVALID;
This should be done as follows (compare with lines
1536-1543 above):

  if (!server_cert_) {
    server_cert_verify_result_ = &local_server_cert_verify_result_;
    local_server_cert_verify_result_.Reset();
    local_server_cert_verify_result_.cert_status = CERT_STATUS_INVALID;
    return ERR_CERT_INVALID;
  }

[Sergey Ulanov, 2011-07-19 23:50:21]

>LGTM.  CAVEAT: this CL may increase memory usage.  SSLConfig
>objects are often copied because we assume it is small,
>containing just a few bool fields and pointers.  A DER >encoded
>certificate is usually 1000 - 1500 bytes.

There are only 3 places where we use allowed_bad_certs, so additional memory
consumption caused by this change is not significant.

http://codereview.chromium.org/7401003/diff/4011/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/7401003/diff/4011/net/base/ssl_config_service....
net/base/ssl_config_service.h:30: bool IsAllowedBadCert(const std::string&
cert_der, int* cert_status) const;
On 2011/07/19 21:57:01, wtc wrote:
> Please document the new IsAllowedBadCert variant.
> 
> It may be better to use const StringPiece& instead of
> const std::string&.  This may allow us to avoid copying
> in some cases.
> 
> I assume it is fine to use function overloading here even
> though the Style Guide recommends against it in general:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...
> 
> Nit: please name the argument "der_cert".  The full
> description is "DER encoded certificate", so "der_cert" is
> closer to that than "cert_der".  Please do this renaming
> throughout the CL, at least in the files under src/net.
> 
> If the renaming requires changing too many files, don't do
> it.

Done.

http://codereview.chromium.org/7401003/diff/4011/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/7401003/diff/4011/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:229: DCHECK(handle);
On 2011/07/19 21:57:01, wtc wrote:
> You can also replace this DCHECK with proper error handling.

Done.

http://codereview.chromium.org/7401003/diff/4011/net/http/http_stream_factory...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/7401003/diff/4011/net/http/http_stream_factory...
net/http/http_stream_factory_impl_job.cc:1018: return error;
On 2011/07/19 21:57:01, wtc wrote:
> Are you sure ssl_info_.cert may be NULL here?  I don't
> think this function is executed in the sandbox.
> 
> Please add a comment to explain why ssl_info.cert may be
> NULL.

Normally ssl_info.cert should not be set to NULL, but there might be some cases
when it is NULL. We may fail to create X509Certificate for reasons other than
running in sandbox, for example when OS runs out of disk space, etc. - we don't
want chrome to crash in that case. Added comments.

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1041: // Returns server_cert_.
On 2011/07/19 21:57:01, wtc wrote:
> Remove "Returns server_cert_."

Done.

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1049: certs.AsStringPieceVector());
On 2011/07/19 21:57:01, wtc wrote:
> Please add a comment here that this may fail in the sandbox.

Done.

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1531: // code is used inside of sandbox.
On 2011/07/19 21:57:01, wtc wrote:
> Nit: remove "of" on this line and line 1546 below.
> 
> Remove the "j" at the end of line 1546.

Done.

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1534: server_cert_nss_->derCert.len);
On 2011/07/19 21:57:01, wtc wrote:
> Using StringPiece here would avoid the copying.

Done.

http://codereview.chromium.org/7401003/diff/4011/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1548: return ERR_CERT_INVALID;
On 2011/07/19 21:57:01, wtc wrote:
> This should be done as follows (compare with lines
> 1536-1543 above):
> 
>   if (!server_cert_) {
>     server_cert_verify_result_ = &local_server_cert_verify_result_;
>     local_server_cert_verify_result_.Reset();
>     local_server_cert_verify_result_.cert_status = CERT_STATUS_INVALID;
>     return ERR_CERT_INVALID;
>   }
>     

Done.