Implement TLS client auth in the OS X OpenSSL port.

Implement TLS client auth in the OS X OpenSSL port.

This introduces a openssl_platform_key.h that looks up and wraps a platform
private key from the platform key store and returns an EVP_PKEY. It is
implemented on Mac and left as a stub on Windows. This will be refactored with
https://crbug.com/394131.

The USE_OPENSSL_CERTS case has been left intact to preserve the existing tests
on Linux but, possibly after the refactor, this will need to change as Linux and
CrOS will likely still use OpenSSL handles for X509Certificate but will not
likely want the OpenSSLClientKeyStore hack.

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

[davidben, 2014-07-15 23:03:50]

Here's an initial version to look at, but don't sign off on this yet: this will
need rebasing after the BoringSSL switch-over and probably shouldn't land until
afterwards to avoid the hack on line 275 of openssl_platform_key_mac.cc. It's
also missing ECDSA support which really wants to wait until BoringSSL. The
equivalent Android code right now needs to include internal headers and
silliness.

[wtc, 2014-07-16 00:09:58]

Review comments on patch set 3:

I didn't review openssl_platform_key_mac.cc.

https://codereview.chromium.org/396803002/diff/40001/net/net.gypi
File net/net.gypi (left):

https://codereview.chromium.org/396803002/diff/40001/net/net.gypi#oldcode135
net/net.gypi:135: 'ssl/openssl_client_key_store.h',

Is it correct to delete these two files?

https://codereview.chromium.org/396803002/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/396803002/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:37: #endif  //
defined(USE_OPENSSL_CERTS)

Nit: I would omit the comments on lines 35 and 37 when the #if #else blocks are
short. The comments make the code less readable.

https://codereview.chromium.org/396803002/diff/40001/net/ssl/openssl_platform...
File net/ssl/openssl_platform_key.h (right):

https://codereview.chromium.org/396803002/diff/40001/net/ssl/openssl_platform...
net/ssl/openssl_platform_key.h:10: #include "net/base/net_export.h"

You don't need to include this header.

https://codereview.chromium.org/396803002/diff/40001/net/ssl/openssl_platform...
File net/ssl/openssl_platform_key_mac.cc (right):

https://codereview.chromium.org/396803002/diff/40001/net/ssl/openssl_platform...
net/ssl/openssl_platform_key_mac.cc:328: // Default missing implementation.

What does "missing implementation" mean?

[davidben, 2014-07-16 16:25:36]

https://codereview.chromium.org/396803002/diff/40001/net/net.gypi
File net/net.gypi (left):

https://codereview.chromium.org/396803002/diff/40001/net/net.gypi#oldcode135
net/net.gypi:135: 'ssl/openssl_client_key_store.h',
On 2014/07/16 00:09:57, wtc wrote:
> 
> Is it correct to delete these two files?

Oops, fixed. Remnant of a previous more aggressive change.

https://codereview.chromium.org/396803002/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/396803002/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:37: #endif  //
defined(USE_OPENSSL_CERTS)
On 2014/07/16 00:09:58, wtc wrote:
> 
> Nit: I would omit the comments on lines 35 and 37 when the #if #else blocks
are
> short. The comments make the code less readable.

Done.

https://codereview.chromium.org/396803002/diff/40001/net/ssl/openssl_platform...
File net/ssl/openssl_platform_key.h (right):

https://codereview.chromium.org/396803002/diff/40001/net/ssl/openssl_platform...
net/ssl/openssl_platform_key.h:10: #include "net/base/net_export.h"
On 2014/07/16 00:09:58, wtc wrote:
> 
> You don't need to include this header.

Done.

https://codereview.chromium.org/396803002/diff/40001/net/ssl/openssl_platform...
File net/ssl/openssl_platform_key_mac.cc (right):

https://codereview.chromium.org/396803002/diff/40001/net/ssl/openssl_platform...
net/ssl/openssl_platform_key_mac.cc:328: // Default missing implementation.
On 2014/07/16 00:09:58, wtc wrote:
> 
> What does "missing implementation" mean?

Oops, removed.

[wtc, 2014-07-16 16:51:53]

On 2014/07/15 23:03:50, David Benjamin wrote:
> Here's an initial version to look at, but don't sign off on this yet: this
will
> need rebasing after the BoringSSL switch-over and probably shouldn't land
until
> afterwards to avoid the hack on line 275 of openssl_platform_key_mac.cc.

David, should we review this CL carefully right now, or should we wait until it
is rebased after the BoringSSL switch-over?

Does most of the code in openssl_platform_key_mac.cc come from the Mac code in
net/third_party/nss/ssl/sslplatf.c and ssl_client_socket_nss.cc? I am wondering
if I can review openssl_platform_key_mac.cc by comparing it with those two
files.

[davidben, 2014-07-16 19:22:06]

On 2014/07/16 16:51:53, wtc wrote:
> On 2014/07/15 23:03:50, David Benjamin wrote:
> > Here's an initial version to look at, but don't sign off on this yet: this
> will
> > need rebasing after the BoringSSL switch-over and probably shouldn't land
> until
> > afterwards to avoid the hack on line 275 of openssl_platform_key_mac.cc.
> 
> David, should we review this CL carefully right now, or should we wait until
it
> is rebased after the BoringSSL switch-over?

BoringSSL switch-over is looking good, so probably just wait. It won't be a
significant change though; some function pointers in RSA_METHOD get moved around
or renamed, and the size hack goes away.

> Does most of the code in openssl_platform_key_mac.cc come from the Mac code in
> net/third_party/nss/ssl/sslplatf.c and ssl_client_socket_nss.cc? I am
wondering
> if I can review openssl_platform_key_mac.cc by comparing it with those two
> files.

Yeah, that's where it's all from.

[davidben, 2014-07-18 21:42:17]

Alright, this is ready for more careful review now. Also +agl because I forgot
earlier.

I've rebased on top of BoringSSL, gotten that working, removed the RSA_size
hack, and gotten ECDSA keys also working. The files to look at for reference are
sslplatf.c, keystore_openssl.cc, and bits of ssl_client_socket_nss.cc.

Note that this is not quite functional yet because of a BoringSSL bug that I'll
be dealing with shortly. That and because the BoringSSL build on Mac is actually
broken right now pending https://codereview.chromium.org/404613002/. But if you
put it all together, it works. I tested with an MIT certificate and with the
ECDSA instructions from https://crbug.com/349775.

[davidben, 2014-07-23 20:05:08]

Updated to mark keys as opaque which should fix the client auth bug.

[Ryan Sleevi, 2014-07-23 22:38:48]

Going to need to spend more time reviewing, particularly the OS X bits.

A more fundamental question on the design though.

https://codereview.chromium.org/396803002/diff/120001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/396803002/diff/120001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:33: #if defined(USE_OPENSSL_CERTS)
USE_OPENSSL_CERTS is only meant to apply to the representation of an
X509Certificate object, not keys.

I'm not sure I understand why this #ifdef

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
File net/ssl/openssl_platform_key.h (right):

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key.h:21: crypto::ScopedEVP_PKEY
FetchClientCertPrivateKey(
I've mentioned in the past, I don't think we want to continue this pattern of
globals like this.

In the least, this won't really work for Linux/ChromeOS, which need some set of
context objects (//content/browser/net/nss_context ), which is basically a
handle to "the user's key store".

You've talked repeatedly of pushing the key in, rather than having the socket
grab the key globally. Why not do that now, since that's where we need to go
anyways for CrOS?

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
File net/ssl/openssl_platform_key_mac.cc (right):

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:58: CSSM_CC_HANDLE handle_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:108: CHECK(false);
NOTREACHED/NOTIMPLEMENTED, one of which should be the same thing.

[davidben, 2014-07-23 22:59:21]

Sure, I can do the refactoring first instead. I figured it'd be worthwhile to
get Mac client auth working first, but the other order works too. That's also
basically the answer to your #ifdef question:

I had, in a previous version, hidden everything behind FetchClientCertPrivateKey
and then the Android impl of FetchClientCertPrivateKey happened to talk to
OpenSSLClientKeyStore. I completely agree that tying anything about the key to
USE_OPENSSL_CERTS doesn't actually make sense and that ifdef is silly.

I opted instead to leave the Linux and Android path alone because of
ssl_client_socket_openssl_unittest.cc. The SSLClientSocketOpenSSLClientAuthTest
assumes OpenSSLClientKeyStore. Right now those tests run on Linux. On neither
Linux nor Android do they exercise any of the platform bits[*], but they at
least exercise the SSLClientSocketOpenSSL state machine around client auth, so
that's something. They, however, depend on OpenSSLClientKeyStore which does
require USE_OPENSSL_CERTS as an implementation detail.

Post-refactor, if we move the OSKeyHandle -> EVP_PKEY wrapper code outside of
SSLClientSocket, we could actually have that style of test on all platforms,
independent of whether the platform allows us to create a temporary OSKeyHandle.
Then again, that does mean what SSL impl we use leaks outside
SSLClientSocketOpenSSL which we otherwise avoid doing. So that's slightly
irksome. (I suppose the way to support this kind of test and also avoid leaking
would be to make SSLClientSocket take OSKeyHandle | std::string and, if it sees
the latter, would deserialize the key from some format or another.)

In the meantime, leaving the USE_OPENSSL_CERTS path as-is doesn't break Android
and doesn't regress our test coverage on Linux + OpenSSL. I can also make those
tests OS_ANDROID rather than USE_OPENSSL_CERTS and make
openssl_platform_key_android.cc talk to OpenSSLClientKeyStore. That would avoid
the silly #ifdef but lose us those tests on Linux. Or I can block all this on
the refactor and start that tomorrow.

[*] Aside: we actually could change that on Android today without the refactor.
Make LoadPrivateKeyOpenSSL call out to the Java function to create a PrivateKey
object (it won't be backed by libkeystore.so, but it will use the same Java
interface that we talk to) and then call our EVP_PKEY wrapper code.

[Ryan Sleevi, 2014-07-23 23:05:24]

On Jul 23, 2014 3:59 PM, <davidben@chromium.org> wrote:
>
> Sure, I can do the refactoring first instead. I figured it'd be
worthwhile to
> get Mac client auth working first, but the other order works too. That's
also
> basically the answer to your #ifdef question:
>
> I had, in a previous version, hidden everything behind
FetchClientCertPrivateKey
> and then the Android impl of FetchClientCertPrivateKey happened to talk to
> OpenSSLClientKeyStore. I completely agree that tying anything about the
key to
> USE_OPENSSL_CERTS doesn't actually make sense and that ifdef is silly.
>
> I opted instead to leave the Linux and Android path alone because of
> ssl_client_socket_openssl_unittest.cc. The
SSLClientSocketOpenSSLClientAuthTest
> assumes OpenSSLClientKeyStore. Right now those tests run on Linux. On
neither
> Linux nor Android do they exercise any of the platform bits[*], but they
at
> least exercise the SSLClientSocketOpenSSL state machine around client
auth, so
> that's something. They, however, depend on OpenSSLClientKeyStore which
does
> require USE_OPENSSL_CERTS as an implementation detail.
>
> Post-refactor, if we move the OSKeyHandle -> EVP_PKEY wrapper code
outside of
> SSLClientSocket, we could actually have that style of test on all
platforms,
> independent of whether the platform allows us to create a temporary
OSKeyHandle.
> Then again, that does mean what SSL impl we use leaks outside
> SSLClientSocketOpenSSL which we otherwise avoid doing.

It doesn't really leak the SSL impl though, does it? Just the crypto impl.

ChromeOS is going to want/need a standard API for wrapping Chaps keys,
which I had assumed we would use as EVP_PKEY (note: they bundle //crypto
for utilities on CrOS, so we probably want to have the abstraction there)

So that's slightly
> irksome. (I suppose the way to support this kind of test and also avoid
leaking
> would be to make SSLClientSocket take OSKeyHandle | std::string and, if
it sees
> the latter, would deserialize the key from some format or another.)
>
> In the meantime, leaving the USE_OPENSSL_CERTS path as-is doesn't break
Android
> and doesn't regress our test coverage on Linux + OpenSSL. I can also make
those
> tests OS_ANDROID rather than USE_OPENSSL_CERTS and make
> openssl_platform_key_android.cc talk to OpenSSLClientKeyStore. That would
avoid
> the silly #ifdef but lose us those tests on Linux. Or I can block all
this on
> the refactor and start that tomorrow.
>

K, I was confused because I thought you were shelving the refactor. In that
case, let's proceed on this while the refactor goes on,

> [*] Aside: we actually could change that on Android today without the
refactor.
> Make LoadPrivateKeyOpenSSL call out to the Java function to create a
PrivateKey
> object (it won't be backed by libkeystore.so, but it will use the same
Java
> interface that we talk to) and then call our EVP_PKEY wrapper code.
>
> https://codereview.chromium.org/396803002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[davidben, 2014-07-23 23:13:10]

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
File net/ssl/openssl_platform_key_mac.cc (right):

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:58: CSSM_CC_HANDLE handle_;
On 2014/07/23 22:38:47, Ryan Sleevi wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:108: CHECK(false);
On 2014/07/23 22:38:47, Ryan Sleevi wrote:
> NOTREACHED/NOTIMPLEMENTED, one of which should be the same thing.

(I just copied this from keystore_openssl.cc.)

I dunno we if have a release mode NOTREACHED. If this function actually gets
called, we'll get a use-after-free. Could also just implement it I suppose.
SecKeyRef is ref-counted. But this should just never get called. There is
exactly one place in BoringSSL that ever dups a CRYPTO_EX_DATA. Unfortunately,
that's for an EC_KEY, just not for a codepath we care about. Fortunately, I
don't think it properly dup'd things in OpenSSL, so I want to see if we can drop
that part of the API and replace that codepath with an up_ref or something.
(https://crbug.com/391192)

*shrug*

[davidben, 2014-07-23 23:18:49]

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
File net/ssl/openssl_platform_key_mac.cc (right):

https://codereview.chromium.org/396803002/diff/120001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:108: CHECK(false);
On 2014/07/23 23:13:09, David Benjamin wrote:
> On 2014/07/23 22:38:47, Ryan Sleevi wrote:
> > NOTREACHED/NOTIMPLEMENTED, one of which should be the same thing.
> 
> (I just copied this from keystore_openssl.cc.)
> 
> I dunno we if have a release mode NOTREACHED. If this function actually gets
> called, we'll get a use-after-free. Could also just implement it I suppose.
> SecKeyRef is ref-counted. But this should just never get called. There is
> exactly one place in BoringSSL that ever dups a CRYPTO_EX_DATA. Unfortunately,
> that's for an EC_KEY, just not for a codepath we care about. Fortunately, I
> don't think it properly dup'd things in OpenSSL, so I want to see if we can
drop
> that part of the API and replace that codepath with an up_ref or something.
> (https://crbug.com/391192)
> 
> *shrug*

(Hrmf, okay, no. EC_KEY_dup is probably not immediately trivially removed
because some internal code does use it, and on an ECDSA key. Anyway point is,
within BoringSSL, it's only used with an ECDH key, but that changing would be
bad.)

[agl, 2014-07-24 18:39:41]

BoringSSL bits LGTM.

The similarity with the Android code here suggests that we might want to cycle
back to this at some point and pull the BoringSSL bits into some common code
that takes a pointer to an abstract class with a few functions, but let's not
worry about that now.

[Ryan Sleevi, 2014-07-24 19:13:03]

Yeah, +1 to what AGL said

We're going to end up with
- Android (JNI)
- ChromeOS (Chaps? PKCS#11?)
- Linux (PKCS#11?)
- Mac (Security.framework)
- Win (CAPI)
- Win (CNG)

Can probably abstract the BoringSSL glue, and keep the OS-specific stuff in
Chrome-land.

But OS X bits LGTM % a comment nit.

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_client_...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_client_...
net/ssl/openssl_client_key_store.cc:113: return crypto::ScopedEVP_PKEY();
Random comment: This code prevents NRVO, because it's not using a named
variable.

See http://msdn.microsoft.com/en-us/library/ms364057(v=vs.80).aspx for a bit of
the documentation and limits (in an MSVC context, but also applies largely to
Clang and GCC)

However, because of our move emulation, using a named ScopedEVP_PKEY would
require us to use .Pass() at each return point, which would also violate NRVO.

Turns out, there's no "guaranteed" way to ensure NRVO with a scoped-ptr return
value. Didn't realize it until this code though.

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_platfor...
File net/ssl/openssl_platform_key_mac.cc (right):

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:89: extern const RSA_METHOD mac_rsa_method;
Does this really need to be extern linkage? It doesn't seem to be, because it's
just an internal detail for the EVP/ENGINE interface.

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:184: // opened with CRYPT_SILENT, but is
there an equivalent?
Yeah, we can nuke this TODO.

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:224: CSSM_ALGID_NONE, &signature_data) !=
CSSM_OK) {
Can you explain more the CSSM_ALGID_NONE, and contrast with what NSS does?

Is |in| a DigestInfo structure that's already been created? Do we know this
works with CSPs?

(I *think* this should be ok, as it's what Apple does for SSL and S/MIME, but a
better context would help, either here or in the function comment)

[davidben, 2014-07-24 21:02:25]

Abstracting away BoringSSL glue: agreed. It's all just implementing one function
per key type.

Also updated BUILD.gn to hopefully appease the Linux GN bot. We'll see.

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_client_...
File net/ssl/openssl_client_key_store.cc (right):

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_client_...
net/ssl/openssl_client_key_store.cc:113: return crypto::ScopedEVP_PKEY();
On 2014/07/24 19:13:02, Ryan Sleevi wrote:
> Random comment: This code prevents NRVO, because it's not using a named
> variable.
> 
> See http://msdn.microsoft.com/en-us/library/ms364057(v=vs.80).aspx for a bit
of
> the documentation and limits (in an MSVC context, but also applies largely to
> Clang and GCC)
> 
> However, because of our move emulation, using a named ScopedEVP_PKEY would
> require us to use .Pass() at each return point, which would also violate NRVO.
> 
> Turns out, there's no "guaranteed" way to ensure NRVO with a scoped-ptr return
> value. Didn't realize it until this code though.

(Tangentially, I wish we could spell those lines return NULL or return nullptr.
I suppose C++11 will fix that one.)

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_platfor...
File net/ssl/openssl_platform_key_mac.cc (right):

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:89: extern const RSA_METHOD mac_rsa_method;
On 2014/07/24 19:13:02, Ryan Sleevi wrote:
> Does this really need to be extern linkage? It doesn't seem to be, because
it's
> just an internal detail for the EVP/ENGINE interface.

It's just to forward-declare it so that BoringSSLEngine can refer to it before
all the functions in it are defined.

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:184: // opened with CRYPT_SILENT, but is
there an equivalent?
On 2014/07/24 19:13:02, Ryan Sleevi wrote:
> Yeah, we can nuke this TODO.

Done.

https://codereview.chromium.org/396803002/diff/140001/net/ssl/openssl_platfor...
net/ssl/openssl_platform_key_mac.cc:224: CSSM_ALGID_NONE, &signature_data) !=
CSSM_OK) {
On 2014/07/24 19:13:02, Ryan Sleevi wrote:
> Can you explain more the CSSM_ALGID_NONE, and contrast with what NSS does?
> 
> Is |in| a DigestInfo structure that's already been created? Do we know this
> works with CSPs?
> 
> (I *think* this should be ok, as it's what Apple does for SSL and S/MIME, but
a
> better context would help, either here or in the function comment)

Yes, |in| at this point is a DigestInfo or SHA-1 + MD5 concatenation, so all we
need to do is sign it. The code in sslplatf.c actually does the same thing. It
passes in digestAlg, but that's set to CSSM_ALGID_NONE and never changed.

[davidben, 2014-07-28 22:46:56]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-07-28 22:48:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/396803002/200001

[commit-bot: I haz the power, 2014-07-29 07:51:43]

Change committed as 286112