Support for using OS-native certificates for SSL client auth....

Support for using OS-native certificates for SSL client
auth.

Known Limitations:
- Only SSL3/TLS1.0 handshakes are supported. It's unlikely
  SSLv2 will/should ever be implemented. NSS does not yet
  support TLS1.1/1.2.
- On Windows, only CryptoAPI keys are supported. Keys that
  can only be accessed via CNG will fail.

Technical Notes:
Windows: 
- Only the AT_KEYEXCHANGE key is used, per
http://msdn.microsoft.com/en-us/library/aa387461(VS.85).aspx 
- CryptSetHashParam is used to directly set the hash value.
  This *should* be supported by all CSPs that are compatible
  with RSA/SChannel, AFAICT, but testing is needed.

NSS:
- The define NSS_PLATFORM_CLIENT_AUTH is used to guard all
  of the new/patched code. The primary implementation
  details are in sslplatf.c.

Patch author: Ryan Sleevi <rsleevi@chromium.org>;
Original review URL: http://codereview.chromium.org/2828002

BUG=148, 37560, 45369
TEST=Attempt to authenticate with a site that requires SSL
client authentication (e.g., https://foaf.me/simpleLogin.php
with a FOAF+SSL client certificate).

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

[wtc, 2010-09-23 23:43:40]

rsleevi: this is essentially your Patch Set 5, updated to the
current trunk.  I made only the following minor changes.

1. In Chromium proper, changed
  #ifdef NSS_ENABLE_PLATFORM_AUTH
to
  #if defined(NSS_ENABLE_PLATFORM_AUTH)
to follow the Chromium coding style.  I didn't make this change
to the NSS files because NSS doesn't mind the #ifdef form.

2. PlatformAuthHandler was renamed PlatformClientAuthHandler.

3. In ssl_client_socket_nss_factory.cc, removed
"build/build_config.h" and "net/socket/ssl_client_socket_win.h".

4. Removed ssl_client_auth_handler_win.cc from the CL.
The change you made to that file has already been checked
in.  (That file has also been renamed and moved to a
different directory.)

[wtc, 2010-10-15 00:40:05]

Status update: I have reviewed everything except sslplatf.c.
I should be able to finish the review tomorrow and upload a
new Patch Set, which you can then merge into your real CL.

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

http://codereview.chromium.org/3455019/diff/1/2#newcode1512
net/socket/ssl_client_socket_nss.cc:1512: cert =
that->ssl_config_.client_cert->os_cert_handle();
Why the special case for i == 0?  You should be able to use
CFArrayGetValueAtIndex(chain, 0) here.  Is it because
CFArrayGetValueAtIndex(chain, 0) is a SecIdentityRef?
If so, would be nice to add a comment to explain this.

http://codereview.chromium.org/3455019/diff/1/9
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/3455019/diff/1/9#newcode1807
net/third_party/nss/ssl/sslimpl.h:1807: extern CERTCertificateList*
hack_NewCertificateListFromCertList(
hack_NewCertificateListFromCertList calls only public API
functions, so clients can create the platform-supplied chain
with purely public API functions.  Now they have to create
NSS CERTCertificate objects, incurring the cost of certificate
decoding.

[Ryan Sleevi, 2010-10-15 01:00:41]

Thanks for the status update. I've addressed your comments below and look
forward to seeing your changes. I'll also begin more aggressively testing on
Mac. I need to update the CL description, since I have tested somewhat on Mac so
far, with the exception of surprise ejections mid-connection.

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

http://codereview.chromium.org/3455019/diff/1/2#newcode1512
net/socket/ssl_client_socket_nss.cc:1512: cert =
that->ssl_config_.client_cert->os_cert_handle();
On 2010/10/15 00:40:06, wtc wrote:
> Why the special case for i == 0?  You should be able to use
> CFArrayGetValueAtIndex(chain, 0) here.  Is it because
> CFArrayGetValueAtIndex(chain, 0) is a SecIdentityRef?
> If so, would be nice to add a comment to explain this.

Correct, it was because of the SecIdentityRef was stored in |chain|, which would
have meant needing to call SecIdentityCopyCertificate() to get |cert|. I've made
a note to fix this in the next CL, which I'll wait for your feedback on.

http://codereview.chromium.org/3455019/diff/1/9
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/3455019/diff/1/9#newcode1807
net/third_party/nss/ssl/sslimpl.h:1807: extern CERTCertificateList*
hack_NewCertificateListFromCertList(
Could you explain which public API functions?

The only ones documented in cert.h that I saw are CERT_CertChainFromCert,
CERT_CertListFromCert, and CERT_DupCertList, and CERT_DestroyCertificateList.

The primary reason I reused CERTCertList was because the APIs for
creation/destroying/modification were already well exposed (CERT_NewCertList,
_DestroyCertList, RemoveCertListNode, etc) and the comments in the code
suggested an intent to move to it - namely the comment in certdb.c before
CERT_NewCertList (line 2606 - 2609 in my 3.12.6 sources)

/*
 * a real list of certificates - need to convert CERTCertificateList
 * stuff and ASN 1 encoder/decoder over to using this...
 */

While the structure itself is exposed in certt.h, what isn't clear from the API
is whether it's acceptable to manually create the arena and allocate the objects
into the arena - hence the hack_ name as the means of keeping that detail within
NSS.

If you're comfortable surfacing the implementation details - that the
CERTCertificateList should be created via PORT_ArenaAlloc, and then the
individual SECItems within it should also be allocated via the arena, and then
the arena stored within the structure, that certainly seems an alternative
(since the type is public, assuming I am understanding these headers). 

I suspect a more preferable approach would be exposing a method to create a
CERTCertificateList from a given array of raw data/SECItems that will then do
the previously mentioned steps. This had been my original implementation until I
ran across that comment in the certdb.c, which I took to mean I should be using
a CERTCertificateList.

[wtc, 2010-10-16 00:29:04]

rsleevi: OK, I made one pass through the CL.  Please
review the differences between Patch Set 1 and Patch Set 2.

Most of my changes are simple renaming and cleanup.

Note that I changed the code to do either platform client auth
or PKCS #11 client auth, but not both (platform with PKCS #11
fallback). On Mac and Windows we will only do platform client
auth.  On Linux we will only do PKCS #11 client auth.  I
didn't make all the changes for this, but this is the direction
we should be going.

I didn't check the Mac Keychain/CSSM calls carefully
because I can't find their documentation on the Web.

Please merge my changes to your real CL, and we will do
future reviews there.  Thanks again for writing this CL!
Let's get this baby reviewed and checked in soon!

http://codereview.chromium.org/3455019/diff/1/11
File net/third_party/nss/ssl/sslplatf.c (right):

http://codereview.chromium.org/3455019/diff/1/11#newcode132
net/third_party/nss/ssl/sslplatf.c:132:
ssl_PlatformAuthTokenPresent(PlatformAuthInfo* info)
Is this implementation correct?  Does the CryptAcquireContextA
call fail (with the NTE_BAD_KEYSET error) if the smart card
is removed?

Also, I suspect ssl_PlatformAuthTokenPresent may be
unnecessary for CryptoAPI-based SSL client auth.  We should
study how NSS calls this function.  Perhaps it's only
necessary for PKCS #11.

We may need to pass CRYPT_VERIFYCONTEXT or CRYPT_SILENT to
this CryptAcquireContextA call to suppress UI.

http://codereview.chromium.org/3455019/diff/1/11#newcode203
net/third_party/nss/ssl/sslplatf.c:203: /* TODO(rsleevi): Should AT_SIGNATURE
also be checked if doing client
I think the dwKeySpec value should be part of the PlatformKey.
That is, PlatformKey for Windows should be the
(HCRYPTPROV hProv, DWORD dwKeySpec) pair.

Note: the sample code at
http://msdn.microsoft.com/en-us/library/aa382371(v=VS.85).aspx
uses AT_SIGNATURE.

http://codereview.chromium.org/3455019/diff/1/11#newcode235
net/third_party/nss/ssl/sslplatf.c:235: /* TODO: Support CALG_ECDSA once tested
*/
Should this comment be removed, because you have
"case CALG_ECDSA" at line 225 above.

http://codereview.chromium.org/3455019/diff/1/11#newcode453
net/third_party/nss/ssl/sslplatf.c:453: // TODO(rsleevi): Should it be
kSecCredentialTypeNoUI? In Win32, at least,
We want UI here.  Did I miss something?

http://codereview.chromium.org/3455019/diff/1/11#newcode509
net/third_party/nss/ssl/sslplatf.c:509: if (cssmSignature)
BUG: should we also delete cspHandle, cssmKey, and cssmCreds here?

[Ryan Sleevi, 2010-10-16 01:37:43]

I'll keep future comments over there, but I'll address the last notes here for
thread-readability.

Primary reference for the CSSM/CDSA functions:
http://www.opensource.apple.com/source/Security/Security-37594/doc/

Secondary/cross-reference:
http://www.opengroup.org/onlinepubs/009609799/index.htm

You can also find a PDF on the spec buried there somewhere in the site. I have
it saved locally, but it was a free registration to download.

The Apple stuff I'm constantly having to pull out of archives, as Apple revamped
developer.apple.com to focus on IOS, and much of the older documentation is now
404'ing. MSDN has the same problem now and then, but with Apple it's been really
bad with the Security functions. Good luck trying to find a copy of the 10.5 +
10.6 API deprecations and additions to Security.framework (archive.org doesn't
help there, so some of it's from memory)

Also, as with much of Apple APIs, a good portion of additional documentation is
stuck within the public headers themselves. I still haven't decided where I
appreciate it or hate it (as it never appears anywhere else) :)

For the actual comments on the issue, I'm working on merging Patch Set 7 (which
were just a few one-line changes to the UI bits), but I think you'll find a
better explanation about the UI concerns / API concern addressed in the message
that went out with that patchset.

http://codereview.chromium.org/3455019/diff/1/11
File net/third_party/nss/ssl/sslplatf.c (right):

http://codereview.chromium.org/3455019/diff/1/11#newcode453
net/third_party/nss/ssl/sslplatf.c:453: // TODO(rsleevi): Should it be
kSecCredentialTypeNoUI? In Win32, at least,
On 2010/10/16 00:29:04, wtc wrote:
> We want UI here.  Did I miss something?

The main concern here was just GUI blocking (all of) the IO thread. The main
concern is that something like http://crbug.com/16979 is going to resurface -
unless a series of asynchronous functions are also added to NSS to indicate
"Hey, get the key/privileges/prompt". I thought I remembered davidben doing some
work on the PKCS#11-prompting side of this problem.

http://codereview.chromium.org/3455019/diff/1/11#newcode509
net/third_party/nss/ssl/sslplatf.c:509: if (cssmSignature)
On 2010/10/16 00:29:04, wtc wrote:
> BUG: should we also delete cspHandle, cssmKey, and cssmCreds here?

The documentation for SecKeyGetCSPHandle, SecKeyGetCSSMHandle, and
SecKeyGetCredentials all state that the output remains valid until the key
reference is released and shouldn't be modified.

So not a bug :)