Fix several cert problems on Linux (take 2)

For http://code.google.com/p/chromium/issues/detail?id=4510
Extract some UI SSL test code into new class SSLTestUtil to avoid duplication.
Point nss at root certs so test_shell can talk to mail.google.com without warnings.
Support ciphers needed to talk to testserver.py.
Load temporary testing cert needed to run unit tests.
Implement part of GetSSLInfo.
Change URL in developer error message to point to chromium.org.
Re-enable url_request_unittest.cc, which seems to have been disabled by mistake.

Later changesets will implement x509 certificates for nss,
finish GetSSLInfo support, and update chrome/browser/ssl_uitest.cc to use SSLTestUtil.

Earlier version was committed as r6063, but was rolled back.


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

[dank, 2008-11-22 00:41:41]

I haven't really settled on the best way to
express LoadTemporaryCert().  It kind of feels
like it belongs someplace else.

[dank, 2008-11-24 22:59:55]

I expect to do a second change later to implement the rest of GetSSLInfo().

The new https test in url_request_unittest.cc is disabled on the mac for now.

Very similar code has passed try server on all platforms;
they haven't responded about this exact patch yet.

[wtc, 2008-11-25 00:57:31]

Dan,

The CL is mostly in good shape.  I have a bunch of comments
below, most of them are nits.

Feel free to address some of my comments in a follow-up CL.

http://codereview.chromium.org/11249/diff/15/255
File base/nss_init.cc (right):

http://codereview.chromium.org/11249/diff/15/255#newcode8
Line 8: #include "base/logging.h"
Nit: list these two Chrome headers after the NSS headers
(which we consider as C system headers).

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

"base/logging.h" is already included below.

http://codereview.chromium.org/11249/diff/15/255#newcode26
Line 26: // Looks fragile and hacky, but supposedly Firefox does something
similar?
Firefox ships its own NSS libraries.  It still needs to
check multiple directories for libnssckbi.so (I think for
historical reasons -- at one point there is a Gecko Runtime
Environment, and NSS is part of the GRE), but its code is
not as fragile as checking "/usr/lib32/nss/libnssckbi.so"
and "/usr/lib/nss/libnssckbi.so", which are specific to
Debian/Ubuntu.

In Fedora, libnssckbi.so is installed in /usr/lib, so you
can just load "libnssckbi.so" rather than its full pathname.

http://codereview.chromium.org/11249/diff/15/255#newcode27
Line 27: SECMODModule *initDefaultRootCert() {
Nit: capitalize "Init" and use plural "Certs".

http://codereview.chromium.org/11249/diff/15/255#newcode29
Line 29: const char *PossibleLocations[kNPossibleLocations] = {
Nit: "PossibleLocations" => "possible_locations"?

http://codereview.chromium.org/11249/diff/15/255#newcode34
Line 34: for (i=0; i<kNPossibleLocations; i++) {
Nit: spaces around '=' and '<'.

http://codereview.chromium.org/11249/diff/15/255#newcode38
Line 38: sprintf(modparams, "name=\"Root Certs\" library=\"%s\"\n",
Although the code is correct, I suggest using snprintf
instead so that static analysis tools won't warn about
this.

http://codereview.chromium.org/11249/diff/15/255#newcode62
Line 62: for (int i=0; i < SSL_NumImplementedCiphers; i++) {
Nit: add spaces around '='.

http://codereview.chromium.org/11249/diff/15/255#newcode67
Line 67: (info.effectiveKeyBits >= 64));
I suggest that you enable info.effectiveKeyBits >= 80
instead.  Anything weaker than 80 bits is considered not
secure enough today.

http://codereview.chromium.org/11249/diff/15/255#newcode72
Line 72: SSL_OptionSetDefault(SSL_SECURITY, PR_TRUE);
I forgot where you enable SSL 2, SSL 3, and TLS.  If you're
planning to do that here, we should disable SSL 2 and enable
SSL 3 and TLS by default.

We should also enable some TLS extensions such as server
name indication (SNI) and session tickets.

http://codereview.chromium.org/11249/diff/15/255#newcode121
Line 121: }
Starting here, all the error returns need to call
CERT_DestroyCertificate first.

http://codereview.chromium.org/11249/diff/15/255#newcode124
Line 124: int rv = CERT_DecodeTrustString(&trust, const_cast<char
*>("TCu,Cu,Tu"));
Add a TODO(port) comment that this const_cast can be
removed after NSS 3.12.3 is released.  Sorry about the
inconvenience!

http://codereview.chromium.org/11249/diff/15/255#newcode137
Line 137: // TODO(port): figure out how to free loaded resources
You can start by having LoadTemporaryCert return the
CERTCertificate pointer to the caller.

http://codereview.chromium.org/11249/diff/15/256
File base/nss_init.h (right):

http://codereview.chromium.org/11249/diff/15/256#newcode19
Line 19: void LoadTemporaryCert(const std::wstring filename);
'filename' should be const std::wstring&.

I'm not sure if this is the right header for this function
because this function has nothing to do with NSS
initialization.

Perhaps this function belongs in one of the files for unit
tests.

http://codereview.chromium.org/11249/diff/15/251
File chrome/browser/ssl_uitest.cc (right):

http://codereview.chromium.org/11249/diff/15/251#newcode28
Line 28: // see also net/url_request/url_request_unittest.cc
Nit: capitalize "See".  Add a period at the end of sentence.

http://codereview.chromium.org/11249/diff/15/251#newcode85
Line 85: // TODO(port): implement LoadTemporaryCert()?
Or ask the user to use Keychain Access to import the root
CA cert into his X509Anchors keychain, and add the Mac
code to check that the root cert is in the X509Anchors
keychain.

http://codereview.chromium.org/11249/diff/15/253
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/11249/diff/15/253#newcode34
Line 34: // Chrome wants it to fail here, and may retry it later.
Nit: "Chromium" instead of "Chrome"?

http://codereview.chromium.org/11249/diff/15/253#newcode85
Line 85: LOG(WARNING) << "Unknown sec error " << err <<
It's better to capitalize "SSL" and "SEC" in these two
error messages.

http://codereview.chromium.org/11249/diff/15/253#newcode88
Line 88: } else {
Nit: In the net module, we usually should use 'else if'
and 'else' when the previous block ends with a return
statement.

http://codereview.chromium.org/11249/diff/15/253#newcode122
Line 122: }  // namespace
Nit: add a blank line below this line.

http://codereview.chromium.org/11249/diff/15/253#newcode245
Line 245: SSLChannelInfo chanInfo;
Nit: "chanInfo" => "channel_info".

http://codereview.chromium.org/11249/diff/15/253#newcode248
Line 248: ssl_info->security_bits = chanInfo.authKeyBits;
authKeyBits is not the right field to use.  It should be
considered in determining ssl_info->security_bits, but for
now we base ssl_info->security_bits entirely on the cipher
suite, as follows.

Call SSL_GetCipherSuiteInfo with channel_info.cipherSuite.
Then use the effectiveKeyBits field in the
SSLCipherSuiteInfo structure.

http://codereview.chromium.org/11249/diff/15/253#newcode250
Line 250: //TODO(port): implement X509Certificate so we can set the cert field!
Nit: add spaces after // in these two lines.

http://codereview.chromium.org/11249/diff/15/252
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/11249/diff/15/252#newcode123
Line 123: // borrowed from src/chrome/browser/ssl_uitest.cc
We should move the shared code to a new file so that we
don't duplicate it in two files.

http://codereview.chromium.org/11249/diff/15/252#newcode190
Line 190: // TODO(port): support temporary root cert on mac
Or ask the user to import the root cert and add code to
verify that the root cert is in the X509Anchors keychain.

[dank, 2008-11-26 00:34:29]

Whew.  I think I addressed all of wtc's comments.
(Sorry this took so long - I did this at home, and my
home system exposed some test failures I had to deal
with first.)

[wtc, 2008-11-26 01:08:44]

LGTM.

After you fix the nits below, please go ahead and check it in.

http://codereview.chromium.org/11249/diff/27/265
File base/nss_init.cc (right):

http://codereview.chromium.org/11249/diff/27/265#newcode25
Line 25: const char *PossibleLocations[] = {
This array should be renamed "possible_locations".  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names

http://codereview.chromium.org/11249/diff/27/266
File base/nss_init.h (right):

http://codereview.chromium.org/11249/diff/27/266#newcode19
Line 19: void LoadTemporaryCert(const std::wstring& filename);
Remove this declaration.  (This has been moved to
the new ssl_test_util.cc file.)

http://codereview.chromium.org/11249/diff/27/262
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/11249/diff/27/262#newcode87
Line 87: } else if (IS_SEC_ERROR(err)) {
Nit: this "else if" should also be replaced by just "if"
because the preceding block ends with a return statement.

http://codereview.chromium.org/11249/diff/27/262#newcode255
Line 255: &cipher_info, sizeof(cipher_info));
Nit: weird indentation.

http://codereview.chromium.org/11249/diff/27/262#newcode259
Line 259: ssl_info->security_bits = 0;
Set ssl_info->security_bits to -1 here.  -1 means "unknown".
0 means "no encryption".

http://codereview.chromium.org/11249/diff/27/263
File net/base/ssl_test_util.cc (right):

http://codereview.chromium.org/11249/diff/27/263#newcode5
Line 5: #include "build/build_config.h"
This can be removed because "net/base/ssl_test_util.h"
includes this.

http://codereview.chromium.org/11249/diff/27/263#newcode7
Line 7: #include "net/base/ssl_test_util.h"
Nit: list "net/base/ssl_test_util.h" first.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/11249/diff/27/263#newcode36
Line 36: const wchar_t SSLTestUtil::kDocRoot[] = L"chrome/test/data";
Add
  // static
above this line.  We do this in our source code before
the definition of static class members.

http://codereview.chromium.org/11249/diff/27/263#newcode88
Line 88: L'/', file_util::kPathSeparator);
Nit: weird indentation.

http://codereview.chromium.org/11249/diff/27/263#newcode92
Line 92: LoadTemporaryCert(GetRootCertPath()));
Nit: weird indentation.

http://codereview.chromium.org/11249/diff/27/263#newcode124
Line 124: FAIL() << " could not open trusted root CA store";
Wrong indentation.

http://codereview.chromium.org/11249/diff/27/263#newcode135
Line 135: CertFreeCertificateContext(cert);
Wrong indentation.

http://codereview.chromium.org/11249/diff/27/263#newcode139
Line 139: FAIL() << " TEST CONFIGURATION ERROR: you need to import the test ca "
Wrong indentation.

http://codereview.chromium.org/11249/diff/27/261
File net/base/ssl_test_util.h (right):

http://codereview.chromium.org/11249/diff/27/261#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
Thank you for adding this.  Could you change
chrome/browser/ssl_uitest.cc to use this too?

http://codereview.chromium.org/11249/diff/27/261#newcode43
Line 43: PrivateCERTCertificate *cert_;
Try the following instead:
  struct CERTCertificateStr;
  struct CERTCertificateStr *cert;

If this work, you can get rid of the
reinterpret_cast<PrivateCERTCertificate*> in
ssl_test_util.cc.

[wtc, 2008-11-26 05:28:54]

http://codereview.chromium.org/11249/diff/27/261
File net/base/ssl_test_util.h (right):

http://codereview.chromium.org/11249/diff/27/261#newcode43
Line 43: PrivateCERTCertificate *cert_;
On 2008/11/26 01:08:44, wtc wrote:
> Try the following instead:
>   struct CERTCertificateStr;
>   struct CERTCertificateStr *cert_;

Perhaps we need to move the forward declaration of
struct CERTCertificateStr outside class SSLTestUtil,
otherwise it'll be a nested type
struct SSLTestUtil::CERTCertificateStr.

[dank, 2008-11-26 21:30:29]

WTC already OK'd the previous version, but he's on vacation now.
If you don't want to do a full review, just review the delta since
version 9 of the patchset.
I caught some fun problems testing on Ubuntu Intrepid --
including having forgotten two TLS options Wan-Teh had advised
me to enable.

People with nss older than 3.12 won't be able to connect to https://gmail.com,
but they will be able to connect to https://mail.google.com.

[M-A Ruel, 2008-11-26 21:42:03]

patchset 10 lgtm (assuming wtc reviewed patchset 9)

http://codereview.chromium.org/11249/diff/38/286
File base/nss_init.cc (right):

http://codereview.chromium.org/11249/diff/38/286#newcode25
Line 25: const char *possible_locations[] = {
Me. Hate. Local. Arrays. Especially. When. Constant.

Ok, I'll let you go, but only _this_ time.

[dank, 2008-11-26 22:00:50]

On 2008/11/26 21:42:03, M-A wrote:
> Me. Hate. Local. Arrays. Especially. When. Constant.

OK, fixed.  And bonus!  I also fixed a bug that made
the first array element useless!

[dank, 2008-12-01 21:18:15]

Had to roll this back on Wednesday, I had forgotten
to add the new source file net/base/ssl_test_helper.cc
to the vcproj.

Wan-teh, this has changed a bit since you last reviewed it, one last check would
be nice.

Avi, can you provide the magic xcode diff to add that file on the Mac?

[wtc, 2008-12-02 01:13:23]

LGTM.  NOTE: please do not check it in without
reading my question on chrome/app/chrome_exe.vcproj.

http://codereview.chromium.org/11249/diff/1202/1404
File base/nss_init.cc (right):

http://codereview.chromium.org/11249/diff/1202/1404#newcode31
Line 31: };
Nit: add a blank line after this line.

http://codereview.chromium.org/11249/diff/1202/1404#newcode39
Line 39: "name=\"Root Certs\" library=\"%s\"\n", possible_locations[i]);
Remove \n at the end of the string.

http://codereview.chromium.org/11249/diff/1202/1405
File base/nss_init.h (right):

http://codereview.chromium.org/11249/diff/1202/1405#newcode8
Line 8: #include <string>
Remove this #include, and then remove this file from the CL.

http://codereview.chromium.org/11249/diff/1202/1410
File chrome/app/chrome_exe.vcproj (right):

http://codereview.chromium.org/11249/diff/1202/1410#newcode47
Line 47: BufferSecurityCheck="false"
Did you add these two lines by accident?  They seem to
turn off two useful checks.

http://codereview.chromium.org/11249/diff/1202/1402
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/11249/diff/1202/1402#newcode470
Line 470: // V2 compatible hello means no SNI, which would cause errors like
Does this mean the gmail server supports SNI?!

http://codereview.chromium.org/11249/diff/1202/1402#newcode482
Line 482: rv = SSL_OptionSet(nss_fd_, SSL_ENABLE_TLS, ssl_config_.tls1_enabled);
Nice catch!

You may want to add a comment that says SNI is enabled
automatically if TLS is enabled.

http://codereview.chromium.org/11249/diff/1202/1402#newcode492
Line 492: // TODO(port): drop build-time support for old NSS once we're all on
NSS 3.12
Does anyone ask that we support NSS 3.11.x?  We will need
NSS 3.12 for EV certificate support.

http://codereview.chromium.org/11249/diff/1202/1409
File net/base/ssl_test_util.cc (right):

http://codereview.chromium.org/11249/diff/1202/1409#newcode55
Line 55: // TODO(port): remove these const_casts after NSS 3.12.3 is released
We will only be able to remove one of the const_casts:
the one in CERT_DecodeTrustString.

http://codereview.chromium.org/11249/diff/1202/1401
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/11249/diff/1202/1401#newcode157
Line 157: DCHECK_EQ(url_request_metrics.object_count,0);
Should we use EXPECT_EQ instead of DCHECK_EQ here?

[wtc, 2008-12-02 02:10:39]

LGTM.

http://codereview.chromium.org/11249/diff/1209/1417
File base/nss_init.cc (right):

http://codereview.chromium.org/11249/diff/1209/1417#newcode24
Line 24: const char* kModulePath = "libnssckbi.so";
Why can you get rid of the possible_locations array?