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

net/base/openssl_private_key_store.h

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_BASE_OPENSSL_PRIVATE_KEY_STORE_H_
 #define NET_BASE_OPENSSL_PRIVATE_KEY_STORE_H_
 
+#include <openssl/evp.h>
+
+#include <vector>
+
 #include "base/basictypes.h"
-
-// Avoid including <openssl/evp.h> here.
-typedef struct evp_pkey_st EVP_PKEY;
+#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.

[Ryan Sleevi, 2013/02/15 23:53:26]

nit: delete the "The OpenSSL library ..." comment, because it's unclear why you
mention that purely in the context of this header file in isolation.

// 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.

[digit1, 2013/02/25 14:26:22]

Done.

+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 store a private/public key pair, generated via <keygen> while
+  // visiting |url|, to an appropriate secure system location.

[Ryan Sleevi, 2013/02/15 23:53:26]

Isn't the "appropriate secure system location" misleading - it's an
implementation detail about both whether it's "secure" or "system".

Further, when added to the comment on line 50/51, it's unclear whether or not
FetchPrivateKey will successfully return the private key stored here.

If you do really mean that it persists permanently, then this should be an
asynchronous interface from the start.

[digit1, 2013/02/25 14:26:22]

It must be stored on the system, this is specifically used to implement support
for the <keygen> attribute, and not storing the public/private key pair to the
system store would make this essentially useless.

Apart from that, this is not technically a blocking function on Android, because
all it does it send an intent with the relevant data and return immediately.

Said intent will always display a UI dialog, which will pause the main UI
thread, but this shall not impact the network thread itself. Also, there is
absolutely _no_ way to know if the operation succeeded or not (e.g. the user
could cancel the installation, or the system can reject the data as invalid).
The API is strictly fire-and-forget unfortunately. Also, trying to probe the
store after the fact to check that the key is installed would also force another
UI dialog, so can't be used. Of course this makes unit-testing impossible.

I have just modified this method to better document it and give it a more
meaningful name (StorePrivateKey() is  misleading). If you think this should be
moved elsewhere, I'd prefer you'd file a bug and this be achieved through a
different patch. I don't really think this method belongs here, but I'm not the
one who added it.

I've changed the comments here to better clarify the distinction.

+  // 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;
 
-  // 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;
+  // 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.

[Ryan Sleevi, 2013/02/15 23:53:26]

Are you sure you want this second sentance to be part of the interface? For
example, when combined with the comment I raised above re: StoreKeyPair, you
have to wonder whether or not it was valid for FetchClientCertPrivateKey to
locate private keys that were previously stored to a system location in the past
via StoreKeyPair.

Otherwise, if the intent is that this class *only* stores associations in
memory, then it seems like StoreKeyPair doesn't belong?

+  // |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);
+
+  // 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.

[ppi, 2013/02/15 19:54:46]

This comment seems to be in the course of non atomic edit.

[digit1, 2013/02/25 14:26:22]

Yep, I've fixed that, thanks for pointing this out.

+  // if not found. Assumes that |lock_| is held.
+  int FindKeyPairIndexLocked(EVP_PKEY* public_key);
+
+  base::Lock lock_;
+  std::vector<KeyPair> pairs_;
+
   DISALLOW_COPY_AND_ASSIGN(OpenSSLPrivateKeyStore);
 };
 
 } // namespace net
 
 #endif  // NET_BASE_OPENSSL_PRIVATE_KEY_STORE_H_