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..e35b2363b59cf4e7d4a97b86905078bf7a6e8a94 100644
--- a/net/base/openssl_private_key_store.h
+++ b/net/base/openssl_private_key_store.h
@@ -5,44 +5,113 @@
 #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;
+
+// OpenSSLPrivateKeyStore provides an interface for storing
+// or locating the associated private key for a given public
+// key. Because OpenSSL does not provide any notion of key
+// or certificate storage, unlike other platforms, this class
+// provides a basic interface to implement these services.
+// IMPORTANT: This is NET_EXPORT to allow client code to call
+// GetInstance() and RecordClientCertPrivateKey(). All other methods
+// are internal to net/.

[Ryan Sleevi, 2013/02/25 19:51:07]

style: You can just NET_EXPORT those methods then

[digit1, 2013/02/26 11:03:13]

Good to know, thanks, I've done just that.

+// This class shall only be used from the network thread.
+class NET_EXPORT OpenSSLPrivateKeyStore {
  public:
   // Platforms must define this factory function as appropriate.
   static OpenSSLPrivateKeyStore* GetInstance();
 
-  virtual ~OpenSSLPrivateKeyStore() {}
+  struct EVP_PKEY_Deleter {
+    inline void operator()(EVP_PKEY* ptr) const {
+      EVP_PKEY_free(ptr);
+    }
+  };
 
-  // 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 scoped_ptr<EVP_PKEY, EVP_PKEY_Deleter> ScopedEVP_PKEY;
+
+  // Called to permanently store a private/public key pair, generated
+  // via <keygen> while visiting |url|, to an appropriate system
+  // location. Increments |pkey|'s reference count, so the caller is still
+  // responsible for calling EVP_PKEY_free on it. Note that this has
+  // nothing to do with the methods RecordClientCertPrivateKey() and
+  // FetchClientCertPrivateKey() below.
+  // |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;

[Ryan Sleevi, 2013/02/25 19:51:07]

DESIGN: I'm concerned about the class here, if only because what's highlighted
in the (paraphrased) comment "NOTE: This method has nothing to do with (the rest
of the class)"

It's also a bit inverted to have a public pure virtual method, since it's hard
to guarantee the correctness of implementations. Normally when we're defining
classes that are intended to be subclassed, we tend to make the pure virtual
some protected method, and then have a public non-virtual that implements the
behaviour.

[digit1, 2013/02/26 11:03:13]

To address this, I've separated both usage in my latest patch. There is now an
OpenSSLClientKeyStore class which is used exclusively to implement the in-memory
store of client certificate private keys, while OpenSSLPrivateKeyStore only
deals with the installation of (public,private) key pairs to the system.

This ensures that any potential issue with OpenSSLPrivateKeyStore can be
addressed completely separately.

+
+  // Record the association between a certificate and its
+  // private key. This method should be called _before_
+  // FetchClientCertPrivateKey to ensure that the private key is returned
+  // when it is called later. The association is recorded in memory
+  // exclusively.
+  // |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);

[Ryan Sleevi, 2013/02/25 19:51:07]

These (RecordClientCertPrivateKey/FetchClientCertPrivateKey) don't still need to
be |virtual|, do they?

[digit1, 2013/02/26 11:03:13]

Indeed, I've removed the virtual.

+
+  // 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. This increments the reference
+  // count of the private key on success.
+  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:
-  OpenSSLPrivateKeyStore() {}
+  OpenSSLPrivateKeyStore();
+
+  virtual ~OpenSSLPrivateKeyStore();
+
+  // 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);
 
  private:
+  // 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.
+  };
+
+  // Returns the index of the keypair for |public_key|. or -1 if not found.
+  int FindKeyPairIndex(EVP_PKEY* public_key);
+
+  std::vector<KeyPair> pairs_;
+
   DISALLOW_COPY_AND_ASSIGN(OpenSSLPrivateKeyStore);
 };