Remove certificates from Channel ID

net/ssl/channel_id_service.h

Index: net/ssl/channel_id_service.h
diff --git a/net/ssl/channel_id_service.h b/net/ssl/channel_id_service.h
index f1d0ab966f941c29fd25a32c7cbd38ea89ad7917..b3a17ba36473a59ce4b7088aa3082fca202a4bf7 100644
--- a/net/ssl/channel_id_service.h
+++ b/net/ssl/channel_id_service.h
@@ -14,6 +14,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time/time.h"
+#include "crypto/ec_private_key.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_export.h"
 #include "net/ssl/channel_id_store.h"
@@ -106,12 +107,10 @@ class NET_EXPORT ChannelIDService
   // |*out_req| will be initialized with a handle to the async request. This
   // RequestHandle object must be cancelled or destroyed before the
   // ChannelIDService is destroyed.
-  int GetOrCreateChannelID(
-      const std::string& host,
-      std::string* private_key,
-      std::string* cert,
-      const CompletionCallback& callback,
-      RequestHandle* out_req);
+  int GetOrCreateChannelID(const std::string& host,
+                           scoped_ptr<crypto::ECPrivateKey>* key,
+                           const CompletionCallback& callback,
+                           RequestHandle* out_req);
 
   // Fetches the domain bound cert for the specified host if one exists.
   // Returns OK if successful, ERR_FILE_NOT_FOUND if none exists, or an error
@@ -131,12 +130,10 @@ class NET_EXPORT ChannelIDService
   // |*out_req| will be initialized with a handle to the async request. This
   // RequestHandle object must be cancelled or destroyed before the
   // ChannelIDService is destroyed.
-  int GetChannelID(
-      const std::string& host,
-      std::string* private_key,
-      std::string* cert,
-      const CompletionCallback& callback,
-      RequestHandle* out_req);
+  int GetChannelID(const std::string& host,
+                   scoped_ptr<crypto::ECPrivateKey>* key,
+                   const CompletionCallback& callback,
+                   RequestHandle* out_req);
 
   // Returns the backing ChannelIDStore.
   ChannelIDStore* GetChannelIDStore();
@@ -144,7 +141,7 @@ class NET_EXPORT ChannelIDService
   // Public only for unit testing.
   int cert_count();
   uint64 requests() const { return requests_; }
-  uint64 cert_store_hits() const { return cert_store_hits_; }
+  uint64 key_store_hits() const { return key_store_hits_; }
   uint64 inflight_joins() const { return inflight_joins_; }
   uint64 workers_created() const { return workers_created_; }
 
@@ -156,9 +153,8 @@ class NET_EXPORT ChannelIDService
 
   void GotChannelID(int err,
                     const std::string& server_identifier,
-                    base::Time expiration_time,
-                    const std::string& key,
-                    const std::string& cert);
+                    const std::string& private_key,
+                    const std::string& public_key);
   void GeneratedChannelID(
       const std::string& server_identifier,
       int error,
@@ -166,15 +162,14 @@ class NET_EXPORT ChannelIDService
   void HandleResult(int error,
                     const std::string& server_identifier,
                     const std::string& private_key,
-                    const std::string& cert);
+                    const std::string& public_key);
 
   // Searches for an in-flight request for the same domain. If found,
   // attaches to the request and returns true. Returns false if no in-flight
   // request is found.
   bool JoinToInFlightRequest(const base::TimeTicks& request_start,
                              const std::string& domain,
-                             std::string* private_key,
-                             std::string* cert,
+                             scoped_ptr<crypto::ECPrivateKey>* key,
                              bool create_if_missing,
                              const CompletionCallback& callback,
                              RequestHandle* out_req);
@@ -185,8 +180,7 @@ class NET_EXPORT ChannelIDService
   // failure (including failure to find a domain-bound cert of |domain|).
   int LookupChannelID(const base::TimeTicks& request_start,
                       const std::string& domain,
-                      std::string* private_key,
-                      std::string* cert,
+                      scoped_ptr<crypto::ECPrivateKey>* key,
                       bool create_if_missing,
                       const CompletionCallback& callback,
                       RequestHandle* out_req);
@@ -199,7 +193,7 @@ class NET_EXPORT ChannelIDService
   std::map<std::string, ChannelIDServiceJob*> inflight_;
 
   uint64 requests_;
-  uint64 cert_store_hits_;
+  uint64 key_store_hits_;
   uint64 inflight_joins_;
   uint64 workers_created_;
 
@@ -210,6 +204,27 @@ class NET_EXPORT ChannelIDService
   DISALLOW_COPY_AND_ASSIGN(ChannelIDService);
 };
 
+// Exports the public and private keys from the provided crypto::ECPrivateKey
+// |key| and writes them to |public_key| and |private_key|. If there's an error
+// calling ExportEncryptedPrivateKey or ExportPublicKey on the ECPrivateKey,
+// then this function returns ERR_PRIVATE_KEY_EXPORT_FAILED. Otherwise it
+// returns OK.
+//
+// This function is only exposed here so that it can be used in unittests.
+NET_EXPORT_PRIVATE int ExportKeypair(scoped_ptr<crypto::ECPrivateKey>& key,

[Ryan Sleevi, 2015/04/10 00:42:14]

style: const-ref or pass as raw pointer

[Ryan Sleevi, 2015/04/10 00:42:14]

naming: KeyPair

[nharper, 2015/04/25 02:59:18]

Done.

[nharper, 2015/04/25 02:59:18]

Done.

+                                     std::string* public_key,
+                                     std::string* private_key);

[Ryan Sleevi, 2015/04/09 22:40:10]

This is a little weird. Why isn't this part of the interface of
crypto::ECPrivateKey?

[nharper, 2015/04/10 00:32:09]

crypto::ECPrivateKey deals in std::vector<uint8> instead of std::string. It also
separates this out into 2 methods (ExportEncryptedPrivateKey and
ExportPublicKey). I tried to minimize the number of times we translate between
std::string keypairs and crypto::ECPrivateKey, but it ended up being done in
multiple places (depending on whether the key is created or provided, and
whether we get the result synchronously or asynchronously), so I factored it out
into its own function (and it's also used in unittests). The code for either
this function has a TODO about possibly changing crypto::ECPrivateKey to output
a std::string* instead of a vector.

[Ryan Sleevi, 2015/04/10 00:42:14]

We only need the two calls because of NSS, right? That is, in a 'sane' API, we'd
just be exporting the private key, and so the only distinction between this and
the function on crypto::ECPrivateKey is... std vs vector?

[nharper, 2015/04/25 02:59:18]

Yes, in a sane API (like openssl's) we'd only need the private key. If we assume
a sane API, then the difference between this and ExportPrivateKey (the Encrypted
part is a restriction imposed by NSS - in an openssl-only world that could also
be removed) is std::string vs std::vector<uint8> - i.e. the type of container
and whether the underlying type is signed or unsigned.

+
+// This function is the opposite of ExportKeypair. It takes DER encoded public
+// and private keys |public_key| and |private_key| and creates a
+// crypto::ECPrivateKey. If there is an issue creating the crypto::ECPrivateKey,
+// this function returns ERR_UNEXPECTED. Otherwise it returns OK.
+//
+// This function is only exposed here so that it can be used in unittests.
+NET_EXPORT_PRIVATE int CreateECPrivateKeyFromSerializedKey(
+    const std::string& public_key,
+    const std::string& private_key,
+    scoped_ptr<crypto::ECPrivateKey>* key_out);

[Ryan Sleevi, 2015/04/09 22:40:10]

Why does the API require the public key to create the private key?

[nharper, 2015/04/10 00:32:09]

My opinion is that it shouldn't be needed, but the NSS code requires it
(boringssl ignores the public key).

When the NSS code creates the SECKEYPrivateKey, it uses PK11_UnwrapPrivKey,
which takes a SECItem* publicValue (which is generated from the public key
passed in to crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo). I haven't
read through very carefully the implemenation of PK11_UnwrapPrivKey to see what
exactly it's doing with the SECItem, but it does use it. It might be possible to
generate some other SECItem to pass in (I don't know if it's actually possible),
but that seems like a lot of work and setup for a situation where we already
have access to the public key.

[Ryan Sleevi, 2015/04/10 00:42:14]

On the other hand, it encourages readers/API designers to think the public key
is required, when it's "not" (for all values of not not equal to iOS,
post-Chimera)

I'd be inclined to recover the public key from the private key. However, your
further explanation makes me think that the format of |private_key| supplied to
this call varies depending on whether you're using NSS vs BoringSSL - is that
correct?

If so, that's sort of a dangerous API to leave lying around...

[nharper, 2015/04/25 02:59:18]

The format of the encrypted private_key is the same for both NSS and BoringSSL -
it's a pkcs8 EncryptedPrivateKeyInfo with an encryptionAlgorithm of
pbeWithSHAAnd3-KeyTripleDES-CBC (1.2.840.113549.1.12.1.3).

I'm not sure it's worth it for me to figure out the NSS code to recover the
public key from the private key, especially given the direction we're headed in.
Also, if we push the transition from crypto::ECPrivateKey to std::string down
from here (ChannelIDService) to ChannelIDStore, it should be less visible to
readers.

 }  // namespace net
 
 #endif  // NET_SSL_CHANNEL_ID_SERVICE_H_