Wire up SSL client authentication for OpenSSL/Android through the net/ stack

net/base/openssl_private_key_store.h

Index: net/base/openssl_private_key_store.h
diff --git a/net/base/openssl_private_key_store.h b/net/base/openssl_private_key_store.h
index edd54f38aa150159336a088bbc74475106a0c211..b1739092356be17a558b319924fbd6ca339e7c6c 100644
--- a/net/base/openssl_private_key_store.h
+++ b/net/base/openssl_private_key_store.h
@@ -5,42 +5,123 @@
 #ifndef NET_BASE_OPENSSL_PRIVATE_KEY_STORE_H_
 #define NET_BASE_OPENSSL_PRIVATE_KEY_STORE_H_
 
-#include "base/basictypes.h"
+#include <openssl/evp.h>
+
+#include <vector>
 
-// Avoid including <openssl/evp.h> here.
-typedef struct evp_pkey_st EVP_PKEY;
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/lock.h"
+#include "crypto/openssl_util.h"
+#include "net/base/net_export.h"
 
 class GURL;
 
 namespace net {
 
-// Defines an abstract store for private keys; the OpenSSL library does not
-// provide this service so it is left to individual platforms to provide it.
-//
-// The contract is that the private key will be stored in an appropriate secure
-// system location, and be available to the SSLClientSocketOpenSSL when using a
-// client certificate created against the associated public key for client
-// authentication.
-class OpenSSLPrivateKeyStore {
+class X509Certificate;
+
+// Defines an interface to the store for private keys. The OpenSSL library
+// does not provide this service so it is left to individual platforms to
+// provide it.
+class NET_EXPORT OpenSSLPrivateKeyStore {
  public:
   // Platforms must define this factory function as appropriate.
   static OpenSSLPrivateKeyStore* GetInstance();
 
-  virtual ~OpenSSLPrivateKeyStore() {}
+  // Helper class to simplify the definition of custom scoped_ptr<>
+  // types which is a specific function for destruction, instead of
+  // delete, delete[] or free(). Usage is:
+  //
+  //   typedef ScopedPtrFunc<T, T_free>::type ScopedT;
+  //
+  // Which defines ScopedT as a scoped_ptr<T> which will call T_free()
+  // at destruction time, instead of.
+  //
+  // This is considerably simpler than the default which requires defining
+  // a custom structure for each <T, T_free> pair, as in:
+  //
+  //   struct T_Deleter {
+  //     inline void operator()(T* ptr) {
+  //       T_free(ptr);
+  //     }
+  //   };
+  //   typedef scoped_ptr<T, T_Deleter> ScopedT;
+  //
+  template <typename T, void (deleter)(T*)>
+  struct ScopedPtrFunc {
+    inline void operator()(T* ptr) const {
+      deleter(ptr);
+    }
+    typedef scoped_ptr<T, ScopedPtrFunc<T, deleter> > type;
+  };

[Ryan Sleevi, 2013/02/13 23:25:55]

I would have trouble stamping this change. It feels like a hack. If scoped_ptr<>
should be fixed to be consistent with unique_ptr (which allows function
pointers), then it should be fixed. Otherwise, we shouldn't be declaring these
sorts of generic helpers in something as specific as this file.

[digit1, 2013/02/14 06:23:50]

ok, I don't think it's reasonable to depend on changes to scoped_ptr at this
point, so which alternative would you prefer here then:

1/ typedef crypto::ScopedOpenSSL<EVP_PKEY, EVP_PKEY_free> ScopedEVP_PKEY;

2
  struct EVP_PKEY_Deleter {
    inline void operator()(EVP_PKEY* ptr) const {
      EVP_PKEY_free(ptr);
    }
  };
  typedef scoped_ptr<EVP_PKEY, EVP_PKEY_deleter> ScopedEVP_PKEY;

Both are equivalent in the sense that they don't require callers to change their
use.

[Ryan Sleevi, 2013/02/14 07:15:00]

I'm not sure I understand your point about "not depend on changes to
scoped_ptr". We've been collectively trying to reduce the custom-scoped-X
classes, following a series of scoped_ptr<> cleanups to bring it into alignment
with unique_ptr<> in the spec. It's already on my radar to fix
crypto/scoped_capi_types and crypto/scoped_nss_types. I'm particularly sensitive
to new introductions of generics and new scoped types, trying to make sure we're
not creating more work for us to fix later.

I think 1/ is the lesser of the two evils when only given those two options, if
only because we've been doing 1/ (unfortunately), but I think we should be
working to entirely rid the codebase of ScopedOpenSSL in favour of using
scoped_ptr<> consistently, which is why you're getting push back. This is the
first CL touching the scoped_x types now that we have them unified, which is why
I'm saying "Hrm, we should clean this up while we're here."

Part of the general "leave the code better then when you got there" philosophy
in Chrome, even if it results in 'more' requests.

[digit1, 2013/02/14 08:24:39]

My feeling is that changes to core classes and definitions in base/, that
potentially affect the whole codebase in subtle ways, tend to take a very large
time to land. What I meant is to avoid having to wait for scoped_ptr<> to really
be able to support function deleters natively, since it's not a core feature of
this patch.

 It's already on my radar to fix
I'm not sure that fixing scoped_ptr<> itself is going to be easy, so I won't
attempt to do it here. I've recently uploaded a new patch that uses the
EVP_PKEY_Deleter approach, but I'll revert this to crypto::ScopedOpenSSL for the
reasons you outlined. It's better to only do that kind of cleanup once, but this
will happen in the future.

 
-  // Called to store a private key generated via <keygen> while visiting |url|.
-  // Does not takes ownership of |pkey|, the caller reamins responsible to
-  // EVP_PKEY_free it. (Internally, a copy maybe made or the reference count
-  // incremented).
+  typedef ScopedPtrFunc<EVP_PKEY, EVP_PKEY_free>::type ScopedEVP_PKEY;
+
+  // Called to store a private/public key pair, generated via <keygen> while
+  // visiting |url|, to an appropriate secure system location.
+  // Increments |pkey|'s reference count, so the caller is still responsible
+  // for calling EVP_PKEY_free on it.
+  // |url| is the corresponding server URL.
+  // |pkey| is the key pair handle.
   // Returns false if an error occurred whilst attempting to store the key.
-  virtual bool StorePrivateKey(const GURL& url, EVP_PKEY* pkey) = 0;
+  virtual bool StoreKeyPair(const GURL& url, EVP_PKEY* pkey) = 0;
+
+  // Record the association between a certificate and its private key.
+  // This method should be called _before_ FetchPrivateKey to ensure that
+  // the private key is returned when it is called later.
+  // |cert| is a handle to a certificate object.
+  // |private_key| is an OpenSSL EVP_PKEY that corresponds to the
+  // certificate's private key.
+  // Returns false if an error occured.
+  // This function does not take ownership of the private_key, but may
+  // increment its internal reference count.
+  virtual bool RecordClientCertPrivateKey(const X509Certificate* cert,
+                                          EVP_PKEY* private_key);
+
+  // Given a certificate's |public_key|, return the corresponding private
+  // key that has been recorded previously by RecordClientCertPrivateKey().
+  // |cert| is a client certificate.
+  // |*private_key| will be reset to its matching private key on success.
+  // Returns true on success, false otherwise.
+  virtual bool FetchClientCertPrivateKey(const X509Certificate* cert,
+                                         ScopedEVP_PKEY* private_key);
 
-  // Given a |public_key| part returns the corresponding private key, or NULL
-  // if no key found. Does NOT return ownership.
-  virtual EVP_PKEY* FetchPrivateKey(EVP_PKEY* public_key) = 0;
+  // Flush all recorded keys. Used only during testing.
+  virtual void Flush();
 
  protected:

[Ryan Sleevi, 2013/02/13 23:25:55]

Please examine the use of protected here and ensure it's consistent with the
Style Guide (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...
)

As written, I think this should be private (which would mean the class is
non-inheritable), plus a friend to a test fixture - rather than protected.

[digit1, 2013/02/14 06:23:50]

The class must be derived by OpenSSLMemoryPrivateKeyStore and
OpenSSLPrivateKeyStoreAndroid in order to override the StoreKeyPair() method.

I think it's possible to move most stuff to private though, except for the
constructor, destructor and AddKeyPair.

I'm not sure about what you want here, can I ask you to clarify?

[Ryan Sleevi, 2013/02/14 07:15:00]

I'm asking that you carefully evaluate what needs to be protected, and what
can/should be private. As the style guide says, especially with datamembers,
they should be private.

Ctor, Dtor, AddKeyPair is fine - if that's the minimal interface to expose.
Exposing KeyPair, lock_, and pairs_ is problematic though.

[digit1, 2013/02/14 08:24:39]

That's exactly what I've already done, so I think we should be good here.
Thanks.

-  OpenSSLPrivateKeyStore() {}
+  OpenSSLPrivateKeyStore();
+
+  virtual ~OpenSSLPrivateKeyStore();
+
+  // KeyPair is an internal class used to hold a pair of private / public
+  // EVP_PKEY objects, with appropriate ownership.
+  class KeyPair {
+   public:
+    explicit KeyPair(EVP_PKEY* pub_key, EVP_PKEY* priv_key);
+    KeyPair(const KeyPair& other);
+    ~KeyPair();
+
+    EVP_PKEY* public_key_;
+    EVP_PKEY* private_key_;
+
+   private:
+    KeyPair();  // intentionally not implemented.
+  };
+
+  // Adds a given public/private key pair.
+  // |pub_key| and |private_key| can point to the same object.
+  // This increments the reference count on both objects, caller
+  // must still call EVP_PKEY_free on them.
+  void AddKeyPair(EVP_PKEY* pub_key, EVP_PKEY* private_key);
+
+  // Returns the index of the keypair for |public_key|. or -1 if not found.
+  // if not found. Assumes that |lock_| is held.
+  int FindKeyPairIndexLocked(EVP_PKEY* public_key);
+
+  base::Lock lock_;

[Ryan Sleevi, 2013/02/14 07:15:00]

Please make sure to fully comment/explain the threading lifetime - perhaps on
24-26 - so that it's clear why this is locked.

You mentioned worker threads before - but that surprises me, as from looking at
the CL, it seems like it should only be the UI thread (on the activity
completion) and on the IO thread (when performing handshakes). In the activity
completion model (in chrome/), it seems much more preferable to simply PostTask
to the BrowserThread::IO to do the modification, so that this object is *only*
accessed on the IO thread, rather than to require this object to implement
thread-safe locking.

The reasoning behind that is that it's easier to reason about the code when it's
single-threaded and has a defined lifetime.

The PostTask model is further simplified when using scoped_ptr<> with custom
deleters, since then you can use base::Passed to pass the EVP_PKEY to the IO
thread in a scoped_ptr<EVP_PKEY, EVP_PKEY_free>. Using a custom type, you can't
use base::Passed(), and you can't use base::Owned() with an custom-deleter.

[digit1, 2013/02/14 08:24:39]

The worker threads are mentioned in the Android GetInstance() implementation. I
didn't write this code and don't know exactly what it refers to. Apart from
that, your understanding is correct that only the UI thread shall call
RecordClientCertPrivateKey(), all other functions should exclusively be called
from the I/O thread. That's why the initial proposal was to expose this method
through a single NET_EXPORT function, and keep the OpenSSLPrivateKeyStore
internal to net/
I agree that this sounds a better alternative, but now I'm all confused about
the best implementation for ScopedEVP_PKEY. I'll have to perform some testing to
see if it works in practice though. base::Bind() compilation errors are
essentially undecipherable :)

+  std::vector<KeyPair> pairs_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(OpenSSLPrivateKeyStore);