Change NSS's native auth patch to use PCERT_KEY_CONTEXT instead of HCRYPTPROV on Win

When performing SSL client authentication on Windows via NSS, change the returned key type to use a PCERT_KEY_CONTEXT allocated via NSS's PORT_Alloc(), rather than an HCRYPTPROV, for native client certificate authentication.

There are two reasons for doing this; first, a PCERT_KEY_CONTEXT lets us transmit a dwKeySpec, indicating whether to use the AT_KEYEXCHANGE or AT_SIGNATURE key for CryptoAPI keys. Second, a small piece of syntactic fluff, a PCERT_KEY_CONTEXT easily supports CNG keys for Vista+, which though not presently supported, is a TODO.

R=wtc
BUG=37560, 71748
TEST=Perform SSL client auth on Windows.

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

[Ryan Sleevi, 2010-11-11 03:59:30]

wtc: Please take a look.

One area of (possible) concern is the requirement for the caller to PORT_Alloc()
the structure. Because it's run in a callback, ownership of the PlatformKey
needs to be transferred into NSS before the callback returns - it cannot remain
with the caller because they'll be out of scope.

A few approaches I considered and ruled out:
  1) NSS allocates the PCERT_KEY_CONTEXT before invoking the callback, the
caller just writes into it.
  2) New public API routines for creating & duplicating platform keys.

I ruled out #2 for unnecessary complexity - keeping the OS types out of the NSS
public headers seemed very desirable. #1 had the benefit that it keeps the
allocation semantics hidden, but the problem is that the structure may not be
forwards compatible.

Using PORT_Alloc() seemed to give the caller (SSLClientSocketNSS) the most
flexibility. You can use CryptAcquireCertificatePrivateKey, as we do, which will
search all available private keys if the certificate properties are
missing/mismatched, or you can use CertGetCertificateContextProperty with a
CERT_KEY_CONTEXT_PROP_ID to get the private key if you trust the properties are
up to date.

[Ryan Sleevi, 2011-01-13 00:38:36]

wtc: ping?

[wtc, 2011-02-04 01:32:16]

LGTM!  I'm very sorry about the long delay in reviewing this
CL.

Please make the suggested changes below before checking this
in.

http://codereview.chromium.org/4670004/diff/9001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4670004/diff/9001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1542: LOG(ERROR) << "Internal NSS allocation
error";
Remove this error message, partly because PORT_Alloc() is
just a wrapper around malloc().  It's not an NSS internal
memory allocator.

http://codereview.chromium.org/4670004/diff/9001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1545: memset(key_context, 0,
sizeof(*key_context));
You can use PORT_ZAlloc() instead, which returns zeroed
memory.

http://codereview.chromium.org/4670004/diff/9001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1548: HCRYPTPROV provider = NULL;
Delete 'provider'.

http://codereview.chromium.org/4670004/diff/9001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:1588: *result_private_key =
reinterpret_cast<void*>(key_context);
I believe this reinterpret_cast<void*> can be removed.
Any data pointer can be implicitly converted to void*,
even in C++.  What C++ disallows is the implicit conversion
in the opposite direction.

Please make the same change to the Mac code at line 1708
below.

http://codereview.chromium.org/4670004/diff/9001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/4670004/diff/9001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:354: *          - Windows: A PCERT_KEY_CONTEXT,
allocated via PORT_Alloc().
This should say "A pointer to PCERT_KEY_CONTEXT", right?

Also, you should clarify that it is CERT_KEY_CONTEXT that
is allocated via PORT_Alloc(), in this comment and in the
Description of the CL.

http://codereview.chromium.org/4670004/diff/9001/net/third_party/nss/ssl/ssli...
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/4670004/diff/9001/net/third_party/nss/ssl/ssli...
net/third_party/nss/ssl/sslimpl.h:857: typedef struct _CERT_KEY_CONTEXT
*PlatformKey;
Since we already include <wincrypt.h> on line 71 above,
we should say
  typedef PCERT_KEY_CONTEXT PlatformKey;

http://codereview.chromium.org/4670004/diff/9001/net/third_party/nss/ssl/sslp...
File net/third_party/nss/ssl/sslplatf.c (right):

http://codereview.chromium.org/4670004/diff/9001/net/third_party/nss/ssl/sslp...
net/third_party/nss/ssl/sslplatf.c:109: PORT_Free(key);
Rewrite this in this style:
    if (key) {
        if (key->dwKeySpec != CERT_NCRYPT_KEY_SPEC)
            CryptReleaseContext(key->hCryptProv, 0);
        /* FIXME(rsleevi): Close CNG keys. */
        PORT_Free(key);
    }

[Ryan Sleevi, 2011-02-05 04:43:20]

http://codereview.chromium.org/4670004/diff/9001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/4670004/diff/9001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:354: *          - Windows: A PCERT_KEY_CONTEXT,
allocated via PORT_Alloc().
On 2011/02/04 01:32:16, wtc wrote:
> This should say "A pointer to PCERT_KEY_CONTEXT", right?
> 
> Also, you should clarify that it is CERT_KEY_CONTEXT that
> is allocated via PORT_Alloc(), in this comment and in the
> Description of the CL.

Thanks. Here, it is supposed to be a pointer-to-a-PCERT_KEY_CONTEXT, since it
needs to be a pointer-to-a-pointer. The CL description is correct though, in
that it changes from an HCRYPTPROV to a PCERT_KEY_CONTEXT (that is, a pointer to
a CERT_KEY_CONTEXT), so I left that as is.

[wtc, 2011-02-05 05:29:40]

The CL's description says:

  change the returned key type to use a PCERT_KEY_CONTEXT
  allocated via NSS's PORT_Alloc(), ...

My complaint is that it says the PCERT_KEY_CONTEXT (note the P)
is allocated via PORT_Alloc().  But it is CERT_KEY_CONTEXT that
is allocated via PORT_Alloc().