Refactor EnsureOpenSSLInit and openssl_util into base

Refactor EnsureOpenSSLInit and openssl_util into base
This allows the base/crypto methods to call EnsureOpenSSLInit.
Also factors out the SSL_CTX and X509_STORE to be more closely associated with their consumers (ssl socket and X509Certificate resp.) rather than process wide globals.


BUG=None
TEST=None

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

[bulach, 2010-11-16 14:23:50]

LGTM

nice cleanup! minor suggestions below:

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate_op...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl.cc:335: base::EnsureOpenSSLInit();
you may want to remove it from here and put inside
CreateOSCertHandlesFromPKCS7Bytes, as CreateOSCertHandleFromBytes already checks
it.

http://codereview.chromium.org/4963002/diff/2001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/4963002/diff/2001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:75: :
ssl_ctx_(SSL_CTX_new(SSLv23_client_method())) {
see below, perhaps move out of the initializer list, and call 
 base::EnsureOpenSSLInit(); before setting it in the ctor body..

http://codereview.chromium.org/4963002/diff/2001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:119: base::EnsureOpenSSLInit();
it'd be better to move this to OpenSSLContext's ctor itself

[wtc, 2010-11-16 15:35:07]

Please consider my comments as drive-by review comments
because I only reviewed the API (.h files) changes.

http://codereview.chromium.org/4963002/diff/2001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/4963002/diff/2001/base/openssl_util.h#newcode17
base/openssl_util.h:17: class ScopedSSL {
This template class should be named ScopedOpenSSL.  Would
that be too wordy?

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:239: // Returns a handle to a global, in-memory
trusted root certificate store. We
Nit: is this certificate store for trusted roots only, or
does it also hold test server's certificate?

[joth, 2010-11-16 15:57:41]

Thanks. I also move the EnsureOpenSSLInit into the X509 init singleton, using a
compound statement :)

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate_op...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate_op...
net/base/x509_certificate_openssl.cc:335: base::EnsureOpenSSLInit();
On 2010/11/16 14:23:50, bulach wrote:
> you may want to remove it from here and put inside
> CreateOSCertHandlesFromPKCS7Bytes, as CreateOSCertHandleFromBytes already
checks
> it.

Done.

http://codereview.chromium.org/4963002/diff/2001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/4963002/diff/2001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:75: :
ssl_ctx_(SSL_CTX_new(SSLv23_client_method())) {
On 2010/11/16 14:23:50, bulach wrote:
> see below, perhaps move out of the initializer list, and call 
>  base::EnsureOpenSSLInit(); before setting it in the ctor body..

Done.
ScopedSSL makes this a little more complex.
However I realized this class is extraneousness; I can make SSL_CTX a singleton
directly by defining the appropriate traits, so I did it that way.

http://codereview.chromium.org/4963002/diff/2001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:119: base::EnsureOpenSSLInit();
On 2010/11/16 14:23:50, bulach wrote:
> it'd be better to move this to OpenSSLContext's ctor itself

Done.

[joth, 2010-11-16 16:07:19]

Cheers! Let me know if you can suggest a better wording for the comment.

http://codereview.chromium.org/4963002/diff/2001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/4963002/diff/2001/base/openssl_util.h#newcode17
base/openssl_util.h:17: class ScopedSSL {
On 2010/11/16 15:35:07, wtc wrote:
> This template class should be named ScopedOpenSSL.  Would
> that be too wordy?

Done.

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:239: // Returns a handle to a global, in-memory
trusted root certificate store. We
On 2010/11/16 15:35:07, wtc wrote:
> Nit: is this certificate store for trusted roots only, or
> does it also hold test server's certificate?

The latter, but only after the test code has used this method to access the
store and inject its certs (as per the "e.g.")
Is it incorrect to term this a "root" cert?

[wtc, 2010-11-16 20:20:19]

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:239: // Returns a handle to a global, in-memory
trusted root certificate store. We
On 2010/11/16 16:07:19, joth wrote:
>
> The latter, but only after the test code has used this method to access the
> store and inject its certs (as per the "e.g.")
> Is it incorrect to term this a "root" cert?

A test server's cert is not a "root" cert.  "Root" is short
for "root CA", which is the highest CA in the certificate
chain.  A test server's cert is the lowest cert in the
certificate chain.

You should change
    trusted root certificate store
to simply
    certificate store

[joth, 2010-11-17 09:57:13]

Thanks, landing this now.

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/4963002/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:239: // Returns a handle to a global, in-memory
trusted root certificate store. We
On 2010/11/16 20:20:19, wtc wrote:
> On 2010/11/16 16:07:19, joth wrote:
> >
> > The latter, but only after the test code has used this method to access the
> > store and inject its certs (as per the "e.g.")
> > Is it incorrect to term this a "root" cert?
> 
> A test server's cert is not a "root" cert.  "Root" is short
> for "root CA", which is the highest CA in the certificate
> chain.  A test server's cert is the lowest cert in the
> certificate chain.
> 
> You should change
>     trusted root certificate store
> to simply
>     certificate store

Done.