Fixes the remaining net_unittests for OpenSSL, with the exceptions

Fixes the remaining unit tests failures for OpenSSL:
- implements basic client certificate support in ssl socket
- adds special-case IP address support to allow SSL connections to the test server (iff there is a trusted certificate in the store with 127.0.0.1 in its name)
- enables the test server for loading the temporary cert
- implements the DES encryptor (removed TODO about refactoring the file layout as it's already covered by a TODO in the .h file)
- disabled KeygenHandler tests, as this is not implemented for openssl
- disables the (firefox) importer unittests.

BUG=None
TEST=net_unittests now run green

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

[joth, 2010-12-01 16:27:22]

Hi Marcus,

Could you do a pass over this? It's bit of a monster; if there are any
controversial bits I can extract them to another CL.
I'd like to get this in though, to get the openssl experimental builder running
green :-)
http://build.chromium.org/p/chromium.fyi/waterfall?builder=Chromium%20Linux%2...

Joth

[bulach, 2010-12-01 18:01:33]

LGTM

good stuff! few nits below, looking forward to a green pig! :)

http://codereview.chromium.org/5195001/diff/6001/net/base/x509_openssl_util.cc
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/5195001/diff/6001/net/base/x509_openssl_util.c...
net/base/x509_openssl_util.cc:158: DVLOG(1) << "Allowing localhost IP
certificate: " << hostname;
similar to my comments at http://codereview.chromium.org/4646001/, we should
probably check that we're actually in test mode.

TODO(joth): check TestRootCerts::HasInstance() after
http://codereview.chromium.org/4646001/

http://codereview.chromium.org/5195001/diff/6001/net/http/des.cc
File net/http/des.cc (right):

http://codereview.chromium.org/5195001/diff/6001/net/http/des.cc#newcode100
net/http/des.cc:100: reinterpret_cast<DES_cblock*>(hash), &ks, 1);  // 1 =
encrypt.
des.h has a DES_ENCRYPT

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

http://codereview.chromium.org/5195001/diff/6001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:530: NOTIMPLEMENTED();
provided the second part is correct, it may be clearer to split this into:

EVP_PKEY* GetPrivateKey() {
  // TODO(joth): Need a way to acquire the private key for the client cert.
  NOTIMPLEMENTED();
}

EVP_PKEY* privkey = GetPrivateKey();
if (privkey) {
...
}

...or just keep the NOTIMPLEMENTED here and remove the rest..

[joth, 2010-12-01 19:47:11]

Thanks! All done and new snapshot uploaded. (but codereview seems to be
down)


On 1 December 2010 18:01, <bulach@chromium.org> wrote:

> LGTM
>
> good stuff! few nits below, looking forward to a green pig! :)
>
>
>
> http://codereview.chromium.org/5195001/diff/6001/net/base/x509_openssl_util.cc
> File net/base/x509_openssl_util.cc (right):
>
>
>
http://codereview.chromium.org/5195001/diff/6001/net/base/x509_openssl_util.c...
> net/base/x509_openssl_util.cc:158: DVLOG(1) << "Allowing localhost IP
> certificate: " << hostname;
> similar to my comments at http://codereview.chromium.org/4646001/, we
> should probably check that we're actually in test mode.
>
> TODO(joth): check TestRootCerts::HasInstance() after
> http://codereview.chromium.org/4646001/
>
> http://codereview.chromium.org/5195001/diff/6001/net/http/des.cc
> File net/http/des.cc (right):
>
> http://codereview.chromium.org/5195001/diff/6001/net/http/des.cc#newcode100
> net/http/des.cc:100: reinterpret_cast<DES_cblock*>(hash), &ks, 1);  // 1
> = encrypt.
> des.h has a DES_ENCRYPT
>
>
>
http://codereview.chromium.org/5195001/diff/6001/net/socket/ssl_client_socket...
> File net/socket/ssl_client_socket_openssl.cc (right):
>
>
>
http://codereview.chromium.org/5195001/diff/6001/net/socket/ssl_client_socket...
> net/socket/ssl_client_socket_openssl.cc:530: NOTIMPLEMENTED();
> provided the second part is correct, it may be clearer to split this
> into:
>
> EVP_PKEY* GetPrivateKey() {
>  // TODO(joth): Need a way to acquire the private key for the client
> cert.
>  NOTIMPLEMENTED();
> }
>
> EVP_PKEY* privkey = GetPrivateKey();
> if (privkey) {
> ...
> }
>
> ...or just keep the NOTIMPLEMENTED here and remove the rest..
>
>
> http://codereview.chromium.org/5195001/
>

[wtc, 2010-12-01 22:50:05]

Please consider the following review comments as a
drive-by review.  They are all just style nits.

http://codereview.chromium.org/5195001/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/5195001/diff/20001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1896: # OpenSSL build does not support firefix
importer. See http://crbug.com/64926
Nit: this line exceeds the 80-character limit.

http://codereview.chromium.org/5195001/diff/20001/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/5195001/diff/20001/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:437: return
MapCertStatusToNetError(verify_result->cert_status);
Nit: you can remove this return statement.  If you fall
through, the return statement on line 441 will take care
of this case.

http://codereview.chromium.org/5195001/diff/20001/net/base/x509_openssl_util.cc
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/5195001/diff/20001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:158: != cert_names.end()) {
Nit: Chromium's style guide recommend folding long expressions
with operators at the end:
http://www.chromium.org/developers/coding-style#Code_Formatting

So I suggest putting != at the end of the previous line.

http://codereview.chromium.org/5195001/diff/20001/net/http/des.cc
File net/http/des.cc (right):

http://codereview.chromium.org/5195001/diff/20001/net/http/des.cc#newcode97
net/http/des.cc:97:
reinterpret_cast<const_DES_cblock*>(const_cast<uint8*>(key)), &ks);
Question: do you need to cast away the 'const' before
casting to const_DES_cblock?  Just curious...

http://codereview.chromium.org/5195001/diff/20001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_openssl.h (right):

http://codereview.chromium.org/5195001/diff/20001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.h:45: int ClientCertRequestCallback(SSL*
ssl, X509** x509, EVP_PKEY** pkey);
Nit: match the method declaration and definition orders in
.h and .cc files:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

[joth, 2010-12-02 17:12:01]

wtc, thanks for the comments - very sorry but I didn't see them before
committing. (I would blame my email bouncing, but this wasn't actually the
reason this time!)

I've addressed them (except the casting question) in a new patch:
http://codereview.chromium.org/5587001

Cheers!

http://codereview.chromium.org/5195001/diff/6001/net/base/x509_openssl_util.cc
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/5195001/diff/6001/net/base/x509_openssl_util.c...
net/base/x509_openssl_util.cc:158: DVLOG(1) << "Allowing localhost IP
certificate: " << hostname;
On 2010/12/01 18:01:33, bulach wrote:
> similar to my comments at http://codereview.chromium.org/4646001/, we should
> probably check that we're actually in test mode.
> 
> TODO(joth): check TestRootCerts::HasInstance() after
> http://codereview.chromium.org/4646001/

I rolled this comment into the TODO above to support full IP address support.

http://codereview.chromium.org/5195001/diff/6001/net/http/des.cc
File net/http/des.cc (right):

http://codereview.chromium.org/5195001/diff/6001/net/http/des.cc#newcode100
net/http/des.cc:100: reinterpret_cast<DES_cblock*>(hash), &ks, 1);  // 1 =
encrypt.
On 2010/12/01 18:01:33, bulach wrote:
> des.h has a DES_ENCRYPT

Done.

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

http://codereview.chromium.org/5195001/diff/6001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:530: NOTIMPLEMENTED();
On 2010/12/01 18:01:33, bulach wrote:
> provided the second part is correct, it may be clearer to split this into:
> 
> EVP_PKEY* GetPrivateKey() {
>   // TODO(joth): Need a way to acquire the private key for the client cert.
>   NOTIMPLEMENTED();
> }
> 
> EVP_PKEY* privkey = GetPrivateKey();
> if (privkey) {
> ...
> }
> 
> ...or just keep the NOTIMPLEMENTED here and remove the rest..

Done.

http://codereview.chromium.org/5195001/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/5195001/diff/20001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1896: # OpenSSL build does not support firefix
importer. See http://crbug.com/64926
On 2010/12/01 22:50:05, wtc wrote:
> Nit: this line exceeds the 80-character limit.

Done
- I didn't think 80 cols applied in .gyp files, but Marcus had requested it too.

http://codereview.chromium.org/5195001/diff/20001/net/base/x509_certificate_o...
File net/base/x509_certificate_openssl.cc (right):

http://codereview.chromium.org/5195001/diff/20001/net/base/x509_certificate_o...
net/base/x509_certificate_openssl.cc:437: return
MapCertStatusToNetError(verify_result->cert_status);
On 2010/12/01 22:50:05, wtc wrote:
> Nit: you can remove this return statement.  If you fall
> through, the return statement on line 441 will take care
> of this case.

Done.

http://codereview.chromium.org/5195001/diff/20001/net/base/x509_openssl_util.cc
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/5195001/diff/20001/net/base/x509_openssl_util....
net/base/x509_openssl_util.cc:158: != cert_names.end()) {
On 2010/12/01 22:50:05, wtc wrote:
> Nit: Chromium's style guide recommend folding long expressions
> with operators at the end:
> http://www.chromium.org/developers/coding-style#Code_Formatting
> 
> So I suggest putting != at the end of the previous line.

Done.

http://codereview.chromium.org/5195001/diff/20001/net/http/des.cc
File net/http/des.cc (right):

http://codereview.chromium.org/5195001/diff/20001/net/http/des.cc#newcode97
net/http/des.cc:97:
reinterpret_cast<const_DES_cblock*>(const_cast<uint8*>(key)), &ks);
On 2010/12/01 22:50:05, wtc wrote:
> Question: do you need to cast away the 'const' before
> casting to const_DES_cblock?  Just curious...

unfortunately it seems so (sigh). The openssl api takes a non-const
const_DES_cblock* as input.

http://codereview.chromium.org/5195001/diff/20001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_openssl.h (right):

http://codereview.chromium.org/5195001/diff/20001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.h:45: int ClientCertRequestCallback(SSL*
ssl, X509** x509, EVP_PKEY** pkey);
On 2010/12/01 22:50:05, wtc wrote:
> Nit: match the method declaration and definition orders in
> .h and .cc files:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done as near as I can.
The .cc has slipped quite a way out of order (in fact, never was in order) and
I'm reluctant to do a major re-ordering right now as I know several other people
have got sizable patches pending on this file and I don't want to disrupt them
more than necessary.

[wtc, 2010-12-03 18:56:05]

joth: it was me who commented on this CL too late (due to the
American Thanksgiving holidays).

http://codereview.chromium.org/5195001/diff/20001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_openssl.h (right):

http://codereview.chromium.org/5195001/diff/20001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.h:45: int ClientCertRequestCallback(SSL*
ssl, X509** x509, EVP_PKEY** pkey);
On 2010/12/02 17:12:01, joth wrote:
>
> The .cc has slipped quite a way out of order (in fact, never was in order) and
> I'm reluctant to do a major re-ordering right now as I know several other
people
> have got sizable patches pending on this file and I don't want to disrupt them
> more than necessary.

YES!  This is the right call.  Do not make gratuitous reorderings
merely to comply with this Style Guide recommendation.

I suggested matching declaration and definition orders because
you were adding a new method.  I didn't mean to ask you to fix
the ordering of existing code.