Remove certificates from Channel ID

net/ssl/channel_id_service.h

 // Copyright 2014 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_SSL_CHANNEL_ID_SERVICE_H_
 #define NET_SSL_CHANNEL_ID_SERVICE_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #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"
 
 namespace base {
 class TaskRunner;
 }
 
 namespace net {
 
@@ skipping 72 matching lines @@
   // PrivateKeyInfo struct, and |cert| stores a DER-encoded certificate.
   // The PrivateKeyInfo is always an ECDSA private key.
   //
   // |callback| must not be null. ERR_IO_PENDING is returned if the operation
   // could not be completed immediately, in which case the result code will
   // be passed to the callback when available.
   //
   // |*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
   // code upon failure.
   //
   // On successful completion, |private_key| stores a DER-encoded
   // PrivateKeyInfo struct, and |cert| stores a DER-encoded certificate.
   // The PrivateKeyInfo is always an ECDSA private key.
   //
   // |callback| must not be null. ERR_IO_PENDING is returned if the operation
   // could not be completed immediately, in which case the result code will
   // be passed to the callback when available. If an in-flight
   // GetChannelID is pending, and a new GetOrCreateDomainBoundCert
   // request arrives for the same domain, the GetChannelID request will
   // not complete until a new cert is created.
   //
   // |*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();
 
   // 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_; }
 
  private:
   // Cancels the specified request. |req| is the handle stored by
   // GetChannelID(). After a request is canceled, its completion
   // callback will not be called.
   void CancelRequest(ChannelIDServiceRequest* req);
 
   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,
       scoped_ptr<ChannelIDStore::ChannelID> channel_id);
   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);
 
   // Looks for the domain bound cert for |domain| in this service's store.
   // Returns OK if it can be found synchronously, ERR_IO_PENDING if the
   // result cannot be obtained synchronously, or a network error code on
   // 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);
 
   scoped_ptr<ChannelIDStore> channel_id_store_;
   scoped_refptr<base::TaskRunner> task_runner_;
 
   // inflight_ maps from a server to an active generation which is taking
   // place.
   std::map<std::string, ChannelIDServiceJob*> inflight_;
 
   uint64 requests_;
-  uint64 cert_store_hits_;
+  uint64 key_store_hits_;
   uint64 inflight_joins_;
   uint64 workers_created_;
 
   bool is_system_time_valid_;
 
   base::WeakPtrFactory<ChannelIDService> weak_ptr_factory_;
 
   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_