Remember the intermediate CA certs if the server sends them to us....

Remember the intermediate CA certs if the server sends them to us.

BUG=10911
TEST=net_unittests passes


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

[wtc, 2009-05-28 01:59:08]

http://codereview.chromium.org/115700/diff/1/2
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/115700/diff/1/2#newcode53
Line 53: // Client must take ownership of the returned value.
Nit: we usually use "caller" instead of "client" because
in the network stack, "client" has another meaning (the
opposite of "server").

http://codereview.chromium.org/115700/diff/1/2#newcode54
Line 54: char* GetDefaultCertServerNickname(
If this function is based significantly on Mozilla code,
you need to reference the original Mozilla code and include
its MPL/GPL/LGPL license.

http://codereview.chromium.org/115700/diff/1/2#newcode54
Line 54: char* GetDefaultCertServerNickname(
Should we remove "Server" from the name of this function?

"get default cert server nickname" sounds strange.

Or did you mean "GetDefaultServerCertNickname"?

Since we only call this function on CA certs, strictly
speaking 'cert' is not a server cert.  This also applies
to the 'server_name' local variable name.

http://codereview.chromium.org/115700/diff/1/2#newcode85
Line 85: strcpy(name.get(), nickname.c_str());
Do we really need to copy 'nickname' to 'name'?  Why can't
we pass nickname.c_str() directly to SEC_CertNicknameConflict?

If it's because SEC_CertNicknameConflict takes a 'char*'
argument, I just verified that it is safe to use const_cast
here.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/s...

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/s...

http://codereview.chromium.org/115700/diff/1/2#newcode614
Line 614: // Remember the intermediate CA certs if the server sends them to us.
A critical issue here is that we should only remember the
*valid* intermediate CA certs:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/...

If that's the case, in the future this method should just save the
cert_list in the SSLClientSocketNSS object, and we
actually call PK11_Import Cert when the certificate
verification succeeds.

For now, you can reorder this method so that it calls
SSL_AuthCertificate first, and add a if (rv == SECSuccess)
check as the Mozilla code does.

http://codereview.chromium.org/115700/diff/1/2#newcode621
Line 621: // This cert was found on a token, no need to remember it in the temp
Please remove "in the temp db".  That is a stale comment.

http://codereview.chromium.org/115700/diff/1/2#newcode635
Line 635: // We have found a signer cert that we want to remember.
Nit: let's change "signer cert" to "CA cert" or "issuer cert".

http://codereview.chromium.org/115700/diff/1/2#newcode637
Line 637: if (!nickname.get()) {
This should be
    if (nickname.get()) {
without '!', right?

or !nickname.empty()?  (Not sure if scoped_array has the
'empty' method.)

[wtc, 2009-05-28 02:12:51]

http://codereview.chromium.org/115700/diff/1/2
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/115700/diff/1/2#newcode640
Line 640: PK11_ImportCert(slot, node->cert, CK_INVALID_HANDLE,
This call remembers the cert permanently (in the NSS
cert database).

On second thought, I think we may need to remember the
certs differently because we handle SSL certificate errors
differently than Mozilla.

1. We should remember *valid* intermediate CA certs
permanently in the NSS cert DB as Mozilla does.

2. We probably should remember *invalid* intermediate CA
certs temporarily, in memory.  Mozilla used to store
the certs in a hashtable.  See the (now unused) RememberCert function:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/...

I think we can just store the cert_list in the
SSLClientSocketNSS object for now.

Since I'm not sure if we need to do #2, we can wait until
later to determine if we really need to do this.

[ukai, 2009-05-28 08:00:19]

Thanks for review.

Anyway, I found the issue with net_unittests.
It seems HTTPSRequestTest::HTTPSGetTest got test root CA and remembered it, and
failed to change trust of the root CA in HTTPSRequestTest::HTTPSMismatchedTest.
How can we forget a CA cert from db?

http://codereview.chromium.org/115700/diff/1/2
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/115700/diff/1/2#newcode53
Line 53: // Client must take ownership of the returned value.
On 2009/05/28 01:59:09, wtc wrote:
> Nit: we usually use "caller" instead of "client" because
> in the network stack, "client" has another meaning (the
> opposite of "server").

Done.

http://codereview.chromium.org/115700/diff/1/2#newcode54
Line 54: char* GetDefaultCertServerNickname(
On 2009/05/28 01:59:09, wtc wrote:
> Should we remove "Server" from the name of this function?
> 
> "get default cert server nickname" sounds strange.
> 
> Or did you mean "GetDefaultServerCertNickname"?
> 
> Since we only call this function on CA certs, strictly
> speaking 'cert' is not a server cert.  This also applies
> to the 'server_name' local variable name.

Removed "Server" from the name of this function.

http://codereview.chromium.org/115700/diff/1/2#newcode54
Line 54: char* GetDefaultCertServerNickname(
On 2009/05/28 01:59:09, wtc wrote:
> If this function is based significantly on Mozilla code,
> you need to reference the original Mozilla code and include
> its MPL/GPL/LGPL license.

Done.

http://codereview.chromium.org/115700/diff/1/2#newcode85
Line 85: strcpy(name.get(), nickname.c_str());
On 2009/05/28 01:59:09, wtc wrote:
> Do we really need to copy 'nickname' to 'name'?  Why can't
> we pass nickname.c_str() directly to SEC_CertNicknameConflict?
> 
> If it's because SEC_CertNicknameConflict takes a 'char*'
> argument, I just verified that it is safe to use const_cast
> here.
> 
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/s...
> 
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/s...

Done.

http://codereview.chromium.org/115700/diff/1/2#newcode614
Line 614: // Remember the intermediate CA certs if the server sends them to us.
On 2009/05/28 01:59:09, wtc wrote:
> A critical issue here is that we should only remember the
> *valid* intermediate CA certs:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/...
> 
> If that's the case, in the future this method should just save the
> cert_list in the SSLClientSocketNSS object, and we
> actually call PK11_Import Cert when the certificate
> verification succeeds.
> 
> For now, you can reorder this method so that it calls
> SSL_AuthCertificate first, and add a if (rv == SECSuccess)
> check as the Mozilla code does.

Done.

http://codereview.chromium.org/115700/diff/1/2#newcode621
Line 621: // This cert was found on a token, no need to remember it in the temp
On 2009/05/28 01:59:09, wtc wrote:
> Please remove "in the temp db".  That is a stale comment.

Done.

http://codereview.chromium.org/115700/diff/1/2#newcode635
Line 635: // We have found a signer cert that we want to remember.
On 2009/05/28 01:59:09, wtc wrote:
> Nit: let's change "signer cert" to "CA cert" or "issuer cert".

Done.

http://codereview.chromium.org/115700/diff/1/2#newcode637
Line 637: if (!nickname.get()) {
On 2009/05/28 01:59:09, wtc wrote:
> This should be
>     if (nickname.get()) {
> without '!', right?
> 
> or !nickname.empty()?  (Not sure if scoped_array has the
> 'empty' method.)

Done.

http://codereview.chromium.org/115700/diff/1/2#newcode640
Line 640: PK11_ImportCert(slot, node->cert, CK_INVALID_HANDLE,
On 2009/05/28 02:12:51, wtc wrote:
> This call remembers the cert permanently (in the NSS
> cert database).
> 
> On second thought, I think we may need to remember the
> certs differently because we handle SSL certificate errors
> differently than Mozilla.
> 
> 1. We should remember *valid* intermediate CA certs
> permanently in the NSS cert DB as Mozilla does.
> 
> 2. We probably should remember *invalid* intermediate CA
> certs temporarily, in memory.  Mozilla used to store
> the certs in a hashtable.  See the (now unused) RememberCert function:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/...
> 
> I think we can just store the cert_list in the
> SSLClientSocketNSS object for now.
> 
> Since I'm not sure if we need to do #2, we can wait until
> later to determine if we really need to do this.

Add the code to remember cert_list_ in SSLClientSocketNSS.

[wtc, 2009-05-28 17:47:22]

On 2009/05/28 08:00:19, ukai wrote:
> Thanks for review.
> 
> Anyway, I found the issue with net_unittests.
> It seems HTTPSRequestTest::HTTPSGetTest got test root CA and remembered it,
and
> failed to change trust of the root CA in
HTTPSRequestTest::HTTPSMismatchedTest.
> How can we forget a CA cert from db?

Hmm... we only want to remember the *intermediate* CAs,
not the root CA.  Could you tell me which NSS call failed
when we try to change the trust of the root CA?

You can use the 'certutil' command-line tool to remove
a CA cert from db.  The certutil man page is at
http://www.mozilla.org/projects/security/pki/nss/tools/certutil.html.

You need to specify -d sql:/home/ukai/.pki/nssdb.

You can actually just remove your ~/.pki/nssdb directory
and start from scratch.

If we need to delete the CA cert from other Chromium
users/developers' NSS db, then we'll need to add a
SEC_DeletePermCertificate call to net_unittests to do that.
Use certutil as sample code:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/certutil...

Is that what you want to do?

[wtc, 2009-05-28 18:14:28]

Let's not check this in until we have fixed the problem
with remembering our test root CA cert, which prevents
us from trusting our test root CA cert in subsequent unit
tests.

Other than that issue, this looks good.  Some suggested
changes below.

http://codereview.chromium.org/115700/diff/1002/7
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/115700/diff/1002/7#newcode5
Line 5: // It include code GetDefaultCertNickname(), derived from
Nit: include => includes

http://codereview.chromium.org/115700/diff/1002/7#newcode50
Line 50: #include <certt.h>
Nit: you can remove this #include <certt.h> now because
ssl_client_socket_nss.h includes it.

http://codereview.chromium.org/115700/diff/1002/7#newcode370
Line 370: cert_list_           = NULL;
Nit: this should be put inside the if (cert_list_) statement.

http://codereview.chromium.org/115700/diff/1002/7#newcode453
Line 453: cert_list_ = NULL;
Nit: same as above -- please put this inside if (cert_list_).

http://codereview.chromium.org/115700/diff/1002/7#newcode652
Line 652: // NSS calls this if an incoming certificate needs to be verified.
This also contains code derived from Mozilla code.

http://codereview.chromium.org/115700/diff/1002/7#newcode684
Line 684: // We have found a CA cert that we want to remember.
I guess we should add another test:

    if (node->cert->isRoot) {
      // We don't want to remember the root cert.
      continue;
    }

Speaking of which, we probably should combine all those
    if (XXX) {
      continue;
    }
statements into one:
    if (XXX || YYY || XXX) {
      continue;
    }

http://codereview.chromium.org/115700/diff/1002/8
File net/base/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/115700/diff/1002/8#newcode98
Line 98: // Intermediate CA certs.
This comment is slightly inaccurate because cert_list_ also
contains the server cert itself.  Let's just say
  // Certificate chain.

[wtc, 2009-05-28 18:27:03]

On 2009/05/28 08:00:19, ukai wrote:
>
> It seems HTTPSRequestTest::HTTPSGetTest got test root CA and remembered it,
and
> failed to change trust of the root CA in
HTTPSRequestTest::HTTPSMismatchedTest.

I looked at the code.  I guess it is the
CERT_ChangeCertTrust call that fails.  Anyway, could
you print the value of PORT_GetError() after the NSS
function fails?

Also, could you print cert->isperm after the
CERT_DecodeCertFromPackage call?

Thanks.

[wtc, 2009-05-28 23:01:12]

On 2009/05/28 18:27:03, wtc wrote:
>
> I looked at the code.  I guess it is the
> CERT_ChangeCertTrust call that fails.  Anyway, could
> you print the value of PORT_GetError() after the NSS
> function fails?
> 
> Also, could you print cert->isperm after the
> CERT_DecodeCertFromPackage call?

I decided to build and debugged net_unittests on Linux
today, so please ignore this request.

I found that CERT_ChangeCertTrust failed with
SEC_ERROR_TOKEN_NOT_LOGGED_IN (-8037), and
cert->isperm is PR_TRUE.  Apparently NSS requires
you to log in if you want to change the trust of
a permanent cert.

We can avoid this issue by not remembering root
CA certs.

[wtc, 2009-05-28 23:19:04]

I got it working without errors!  Please see the suggested
changes below.

http://codereview.chromium.org/115700/diff/1002/7
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/115700/diff/1002/7#newcode374
Line 374: // TODO(wtc): Send SSL close_notify alert.
I found that NSS_Shutdown fails at the end of net_unittests.
After a lot of debugging, I found that it's because
InvalidateSessionIfBadCertificate calls UpdateServerCert,
which creates cert_list_ again, and we already destroyed
cert_list_ earlier at line 368.

My fix is to move lines 374-381 to the begining of the Disconnect method.

http://codereview.chromium.org/115700/diff/1002/7#newcode451
Line 451: if (cert_list_)
I believe lines 451-453 can be deleted.

http://codereview.chromium.org/115700/diff/1002/7#newcode463
Line 463: cert_list_ = CERT_GetCertChainFromCert(
We can add
  DCHECK(!cert_list_);
before this line, to verify that lines 451-453 can be deleted.

http://codereview.chromium.org/115700/diff/1002/7#newcode661
Line 661: X509Certificate* cert = that->UpdateServerCert();
Since X509Certificate is a reference-counted object, we
should always store a pointer to it in a scoped_refptr.
So this line should read
  scoped_refptr<X509Certificate> cert = that->UpdateServerCert();

http://codereview.chromium.org/115700/diff/1002/7#newcode674
Line 674: if (node->cert->isperm) {
As I suggested before, we should add if (node->cert->isRoot)
here.

[ukai, 2009-05-29 02:48:44]

Thanks for review and fixes!

http://codereview.chromium.org/115700/diff/1002/7
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/115700/diff/1002/7#newcode5
Line 5: // It include code GetDefaultCertNickname(), derived from
On 2009/05/28 18:14:28, wtc wrote:
> Nit: include => includes

Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode50
Line 50: #include <certt.h>
On 2009/05/28 18:14:28, wtc wrote:
> Nit: you can remove this #include <certt.h> now because
> ssl_client_socket_nss.h includes it.

Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode370
Line 370: cert_list_           = NULL;
On 2009/05/28 18:14:28, wtc wrote:
> Nit: this should be put inside the if (cert_list_) statement.

Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode374
Line 374: // TODO(wtc): Send SSL close_notify alert.
On 2009/05/28 23:19:05, wtc wrote:
> I found that NSS_Shutdown fails at the end of net_unittests.
> After a lot of debugging, I found that it's because
> InvalidateSessionIfBadCertificate calls UpdateServerCert,
> which creates cert_list_ again, and we already destroyed
> cert_list_ earlier at line 368.
> 
> My fix is to move lines 374-381 to the begining of the Disconnect method.

Thanks for debugging!
Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode451
Line 451: if (cert_list_)
On 2009/05/28 23:19:05, wtc wrote:
> I believe lines 451-453 can be deleted.

Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode453
Line 453: cert_list_ = NULL;
On 2009/05/28 18:14:28, wtc wrote:
> Nit: same as above -- please put this inside if (cert_list_).

Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode463
Line 463: cert_list_ = CERT_GetCertChainFromCert(
On 2009/05/28 23:19:05, wtc wrote:
> We can add
>   DCHECK(!cert_list_);
> before this line, to verify that lines 451-453 can be deleted.

Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode652
Line 652: // NSS calls this if an incoming certificate needs to be verified.
On 2009/05/28 18:14:28, wtc wrote:
> This also contains code derived from Mozilla code.

Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode661
Line 661: X509Certificate* cert = that->UpdateServerCert();
On 2009/05/28 23:19:05, wtc wrote:
> Since X509Certificate is a reference-counted object, we
> should always store a pointer to it in a scoped_refptr.
> So this line should read
>   scoped_refptr<X509Certificate> cert = that->UpdateServerCert();

Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode674
Line 674: if (node->cert->isperm) {
On 2009/05/28 23:19:05, wtc wrote:
> As I suggested before, we should add if (node->cert->isRoot)
> here.

Done.

http://codereview.chromium.org/115700/diff/1002/7#newcode684
Line 684: // We have found a CA cert that we want to remember.
On 2009/05/28 18:14:28, wtc wrote:
> I guess we should add another test:
> 
>     if (node->cert->isRoot) {
>       // We don't want to remember the root cert.
>       continue;
>     }
> 
> Speaking of which, we probably should combine all those
>     if (XXX) {
>       continue;
>     }
> statements into one:
>     if (XXX || YYY || XXX) {
>       continue;
>     }

Done.

http://codereview.chromium.org/115700/diff/1002/8
File net/base/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/115700/diff/1002/8#newcode98
Line 98: // Intermediate CA certs.
On 2009/05/28 18:14:28, wtc wrote:
> This comment is slightly inaccurate because cert_list_ also
> contains the server cert itself.  Let's just say
>   // Certificate chain.

Done.

[wtc, 2009-05-29 03:42:47]

LGTM!

Please fix the nits below and check this in.  I don't need
to review it again.

http://codereview.chromium.org/115700/diff/1006/14
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/115700/diff/1006/14#newcode5
Line 5: // It includes code GetDefaultCertNickname(), derived from
Nit: change "It" to "This file".

http://codereview.chromium.org/115700/diff/1006/14#newcode10
Line 10: // mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp
Nit: add a period (.) at the end of this sentence.

http://codereview.chromium.org/115700/diff/1006/14#newcode370
Line 370: transport_->Disconnect();
Nit: add a blank line below this line.

[ukai, 2009-05-29 05:54:37]

Thanks!
Fixed all nits.

http://codereview.chromium.org/115700/diff/1006/14
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/115700/diff/1006/14#newcode5
Line 5: // It includes code GetDefaultCertNickname(), derived from
On 2009/05/29 03:42:47, wtc wrote:
> Nit: change "It" to "This file".

Done.

http://codereview.chromium.org/115700/diff/1006/14#newcode10
Line 10: // mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp
On 2009/05/29 03:42:47, wtc wrote:
> Nit: add a period (.) at the end of this sentence.

Done.

http://codereview.chromium.org/115700/diff/1006/14#newcode370
Line 370: transport_->Disconnect();
On 2009/05/29 03:42:47, wtc wrote:
> Nit: add a blank line below this line.

Done.