Fix client certificate authentication on Mac and Linux introduced in r178732

Fix client certificate authentication on Mac and Linux introduced in r178732

When requesting client authentication, the SSL server may send a list of
acceptable CAs. When discovering matching client certificates, the Mac and
Linux implementations were not fully considering all intermediate certificates
when attempting to discover client certificates.

For example, if the client certficate chain was CC -> Intermediate -> Root, and
the server sent a list of acceptable CAs as Root, then on Mac and Linux, CC
would not be considered, whereas on Windows it would. Further, if the server
listed Intermediate as an acceptable CA, then it would work on all platforms.

BUG=224280, 224897
TEST=See https://docs.google.com/a/chromium.org/document/d/19V5_PBSm7OaFLXzTXdiCdSpt1r1yFYJhuH9X41O2oOs/edit?usp=sharing
R=wtc@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196535

[Ryan Sleevi, 2013-04-16 02:32:49]

wtc: Tests incoming, as well as likely compile fixes for OS X (since I coded on
Linux), but pre-loading the review since it's a bit subtle.

The regression was introduced in http://crrev.com/181104 , which switched the
sockets to using the ClientCertStore, but the original issue was introduced in
http://crrev.com/178732 

The previous implementation for Mac is
http://src.chromium.org/viewvc/chrome/trunk/src/net/base/x509_certificate_mac...
and for Linux is
http://src.chromium.org/viewvc/chrome/trunk/src/net/socket/ssl_client_socket_...

The new code only checks IsIssuedByEncoded(...), which expects that
|intermediate_ca_certs_| contains the actual chain. However, on Mac & Linux, we
don't pick those up when scanning for client certs, hence it always fails when
supplying CA names.

The issue was introduced, ironically, when refactoring the code for unit tests.
Unfortunately, this requires manual testing, which I have not yet completed.
Note that it only fixes the situation when the Intermediate is already installed
in the OS store - if it isn't, then those are http://crbug.com/47658 (Mac) and
http://crbug.com/47656 (Win).

https://codereview.chromium.org/13866049/diff/1/net/cert/x509_certificate.h
File net/cert/x509_certificate.h (left):

https://codereview.chromium.org/13866049/diff/1/net/cert/x509_certificate.h#o...
net/cert/x509_certificate.h:283: CFArrayRef CreateClientCertificateChain()
const;
Because this function is no longer used, I removed it.

I considered leaving this function in, and having ClientCertStoreImpl call it,
but I particularly wanted to avoid the situation of having to have
ClientCertStoreImpl grab the SecKeyRef (needed for a SecIdentityRef) when it
doesn't need one.

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
File net/ssl/client_cert_store_impl_mac.cc (right):

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_mac.cc:111: if
(!new_cert->IsIssuedByEncoded(valid_issuers))
The downside to this approach is that |new_cert->os_cert_handle()| is checked
twice (first on line 147, since it's the same OSCertHandle, then again here). On
OS X, this forces the cert to be parsed twice into its NSS
representation/CachedCert, but I think this is a minor overhead for the code
simplicity - and for keeping this merge friendly (eg: not changing
IsIssuedByEncoded)

[wtc, 2013-04-16 18:50:32]

Patch set 1 LGTM.

https://codereview.chromium.org/13866049/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/13866049/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:1410: os_error =
SecIdentityCopyPrivateKey(identity, &private_key);

Just wanted to make sure that we don't need to get the Mac
Security Services lock around  this SecIdentityCopyPrivateKey
call and the SecCertificateGetData call on line 1428.

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
File net/ssl/client_cert_store_impl_mac.cc (right):

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_mac.cc:121: bool query_keychain,

query_keychain probably should be documented.

It's not clear to me when query_keychain should be true.
Based on the three callers of this function, I can see the
rule is whether the caller is trying to *get* or *select*
client certs. But I don't know the rationale of this rule.
It would be nice to document this for future maintainers of
this code.

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_nss.cc:37: ca_names_items[i].type =
siDERNameBuffer;

This can be the default siBuffer, which seems to be the |type|
value NSS itself uses:
http://mxr.mozilla.org/nss/source/lib/certhigh/certhigh.c#541

I believe NSS_CmpCertChainWCANames doesn't care about the
|type| value, so it is best to not use siDERNameBuffer,
which is only used in one file:
http://mxr.mozilla.org/nss/ident?i=siDERNameBuffer

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_nss.cc:62: NSS_CmpCertChainWCANames(node->cert,
&ca_names) == SECSuccess)) {

It seems that we only need to call one of cert->IsIssuedByEncoded()
and NSS_CmpCertChainWCANames(), depending on the value of
query_nssdb.

It also seems that cert->IsIssuedByEncoded() should call
NSS_CmpCertChainWCANames(). We used to call NSS_CmpCertChainWCANames()
but that call is gone. Perhaps that's the source of this bug?

[Ryan Sleevi, 2013-04-16 19:00:26]

https://codereview.chromium.org/13866049/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/13866049/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:1410: os_error =
SecIdentityCopyPrivateKey(identity, &private_key);
On 2013/04/16 18:50:32, wtc wrote:
> 
> Just wanted to make sure that we don't need to get the Mac
> Security Services lock around  this SecIdentityCopyPrivateKey
> call and the SecCertificateGetData call on line 1428.

Right. Neither of these call into the CSSM/CDSA bits.

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
File net/ssl/client_cert_store_impl_mac.cc (right):

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_mac.cc:121: bool query_keychain,
On 2013/04/16 18:50:32, wtc wrote:
> 
> query_keychain probably should be documented.
> 
> It's not clear to me when query_keychain should be true.
> Based on the three callers of this function, I can see the
> rule is whether the caller is trying to *get* or *select*
> client certs. But I don't know the rationale of this rule.
> It would be nice to document this for future maintainers of
> this code.

Good point. Will add comments to these functions. Also, see reply to NSS code
for more explanation.

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/13866049/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_nss.cc:62: NSS_CmpCertChainWCANames(node->cert,
&ca_names) == SECSuccess)) {
On 2013/04/16 18:50:32, wtc wrote:
> 
> It seems that we only need to call one of cert->IsIssuedByEncoded()
> and NSS_CmpCertChainWCANames(), depending on the value of
> query_nssdb.
> 
> It also seems that cert->IsIssuedByEncoded() should call
> NSS_CmpCertChainWCANames(). We used to call NSS_CmpCertChainWCANames()
> but that call is gone. Perhaps that's the source of this bug?

Yes, that is the (real) root of the bug, but the issue here is as it relates to
testing.

IsIssuedByEncoded() was introduced so that we could have consistent
testing/filtering, even if we couldn't do 'actual' client cert discovery and
testing. See the ClientCertStoreImpl unittests.

IsIssuedByEncoded() only checks that |cert| or its
|intermediate_ca_certificates_| contain the issuer name - it does not attempt to
perform any chain/path discovery.

ClientCertStoreImpl is where we do the actual *discovery* portion of enumerating
chains. As discussed with ppi@ and digit@, the plan was to (eventually) move
this into an asynchronous operation, so that we could further optimize the time
spent here (eg: useful for proxies that require SSL client auth, or sites with
policy-configured certs) by doing things like caching.

GetClientCerts() is the "public" API, while "SelectClientCerts" is the testing
API (and in a follow-up, I'll rename it to be SelectClientCertsForTesting so we
get the nice presubmit warning).

For testing, we DON'T want to query any DBs - we want something hermetically
reproducible. That's why NSS_CmpCertWCANames() is only called on query_nssdb.

The bug came because the Windows code for querying the cert store was properly
ported, but the Linux & Mac implementations  weren't.

That said, I will switch it to only use IsIssuedByEncoded() when !query_nssdb,
since that's an easy optimization to make.

[Ryan Sleevi, 2013-04-19 23:21:43]

Updated comments, also wrote a doc for the manual testing scenarios for this
bug. Cleaned up the client certs script to make it easier to distinguish which
certs are which.

[commit-bot: I haz the power, 2013-04-19 23:22:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/13866049/16001

[commit-bot: I haz the power, 2013-04-20 00:01:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-20 00:53:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/13866049/35001

[commit-bot: I haz the power, 2013-04-20 01:33:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-20 01:56:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/13866049/43002

[commit-bot: I haz the power, 2013-04-20 02:18:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-22 22:04:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/13866049/53001

[commit-bot: I haz the power, 2013-04-22 22:05:50]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-04-22 22:08:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/13866049/53001

[commit-bot: I haz the power, 2013-04-22 23:50:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-22 23:53:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/13866049/53001

[commit-bot: I haz the power, 2013-04-23 00:52:06]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-25 21:21:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/13866049/53001

[commit-bot: I haz the power, 2013-04-25 23:25:49]

Change committed as 196535