Implement chrome.platformKeys.getKeyPair().

chrome/browser/chromeos/platform_keys/platform_keys.h

Index: chrome/browser/chromeos/platform_keys/platform_keys.h
diff --git a/chrome/browser/chromeos/platform_keys/platform_keys.h b/chrome/browser/chromeos/platform_keys/platform_keys.h
index 05d0deff4c727389132ad45e17c9884cda2e8e55..81cef3f2c70239e1f7b5127255b168fce8ff8ba6 100644
--- a/chrome/browser/chromeos/platform_keys/platform_keys.h
+++ b/chrome/browser/chromeos/platform_keys/platform_keys.h
@@ -12,17 +12,13 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "net/cert/x509_certificate.h"
 #include "net/ssl/ssl_client_cert_type.h"
 
 namespace content {
 class BrowserContext;
 }
 
-namespace net {
-class X509Certificate;
-typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
-}
-
 namespace chromeos {
 
 namespace platform_keys {
@@ -38,12 +34,31 @@ extern const char kTokenIdSystem[];
 
 // Supported hash algorithms.
 enum HashAlgorithm {
+  HASH_ALGORITHM_NONE,  // The value if no hash function is selected.
   HASH_ALGORITHM_SHA1,
   HASH_ALGORITHM_SHA256,
   HASH_ALGORITHM_SHA384,
   HASH_ALGORITHM_SHA512
 };
 
+// Parameters to the SignRSA function.
+struct SignRSAParams {
+  // The data that will be signed.
+  std::string data;
+
+  // Must be the DER encoding of a SubjectPublicKeyInfo.
+  std::string public_key;
+
+  // If true, |data| will not be hashed before signing. Only PKCS#1 v1.5 padding
+  // will be applied before signing.
+  // If false, |hash_algorithm| must be set to a value != NONE.
+  bool sign_direct_pkcs_padded = false;

[Ryan Sleevi, 2015/02/07 02:09:40]

It feels like by moving this into a struct, you're using in class initialization
(
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization
) 

to avoid
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

At the least, it seems like SignRSA() should take params "as expected", and if
it needs to bundle into a struct, that should be an internal implementation
detail for marshalling, not a public API contract.

[pneubeck (no reviews), 2015/02/08 10:52:00]

The possibility of using this as default arguments wasn't intentional.
Changed to explicit constructors to prevent that.
(With the side effect of 3-times as much code...)

I'm not convinced about the 'should take params "as expected"' as the number of
arguments is already significant and any passing of this number of arguments is
a huge amount of boilerplate that makes the code less readable.
This class is meant to encapsulate these arguments, so that code that just
passes them around is oblivious to its content.
(And I have already code that just passes it around.)

+
+  // Determines the hash algorithm that is used to digest |data| before signing.
+  // Ignored if |sign_direct_pkcs_padded| is true.
+  HashAlgorithm hash_algorithm = HASH_ALGORITHM_NONE;
+};
+
 struct ClientCertificateRequest {
   ClientCertificateRequest();
   ~ClientCertificateRequest();
@@ -57,6 +72,23 @@ struct ClientCertificateRequest {
   std::vector<std::string> certificate_authorities;
 };
 
+// Holds details about a certificate's (subject) key, i.e. the X.509 Subject
+// Public Key Info (SPKI) of the certificate.
+// As this class supports only specific types of keys (currently only RSA), this
+// does not necessarily mirror the internal structure of a X.509 SPKI.
+struct SubjectPublicKeyInfo {
+  SubjectPublicKeyInfo();
+  ~SubjectPublicKeyInfo();
+
+  std::string public_key_spki_der;
+  net::X509Certificate::PublicKeyType key_type;
+
+  // For RSA a public exponent of 65537 is assumed, so there is no member for
+  // that.
+
+  size_t key_size_bits = 0;
+};

[Ryan Sleevi, 2015/02/07 02:09:40]

Ditto this. I don't really understand this structure much :(

[pneubeck (no reviews), 2015/02/08 10:52:00]

I can only guess what you find problematic here.
As this struct seems not to form a reasonable general concept to you, I changed
it's description/intention to only hold the results of the GetPublicKey
function.
That function happens to return several seemingly unrelated properties because I
didn't want unnecessary many calls to CERT_ExtractPublicKey (because of
potential runtime cost).
If a larger number of calls to CERT_ExtractPublicKey is unproblematic, I can
also split the function into several with more conceptually reasonable results.

[Ryan Sleevi, 2015/02/10 00:59:32]

Sorry I wasn't clearer:

I don't understand why you don't do

bool GetPublicKey(scoped_refptr<X509Certificate> certificate,
                  std::string* spki_der,
                  net::X509Certificate::PublicKeyType* key_type,
                  size_t* key_size_in_bits);

+
 namespace subtle {
 // Functions of this namespace shouldn't be called directly from the context of
 // an extension. Instead use PlatformKeysService which enforces restrictions
@@ -77,19 +109,14 @@ void GenerateRSAKey(const std::string& token_id,
 typedef base::Callback<void(const std::string& signature,
                             const std::string& error_message)> SignCallback;
 
-// Digests |data| with |hash_algorithm| and afterwards signs the digest with the
-// private key matching |public_key|, if that key is stored in the given token.
-// |token_id| is currently ignored, instead the user token associated with
-// |browser_context| is always used. |public_key| must be the DER encoding of a
-// SubjectPublicKeyInfo. |callback| will be invoked with the signature or an
-// error message.
-// Currently supports RSA keys only.
-void Sign(const std::string& token_id,
-          const std::string& public_key,
-          HashAlgorithm hash_algorithm,
-          const std::string& data,
-          const SignCallback& callback,
-          content::BrowserContext* browser_context);
+// Optionally digests |data|, applies PKCS1 padding and afterwards signs the
+// data with the private key matching |params.public_key|. If a non empty token
+// id is provided and the key is not found in that token, the operation aborts.
+// |callback| will be invoked with the signature or an error message.
+void SignRSA(const std::string& token_id,
+             scoped_ptr<SignRSAParams> params,
+             const SignCallback& callback,
+             content::BrowserContext* browser_context);
 
 // If the certificate request could be processed successfully, |matches| will
 // contain the list of matching certificates (which may be empty) and
@@ -107,6 +134,10 @@ void SelectClientCertificates(const ClientCertificateRequest& request,
 
 }  // namespace subtle
 
+// Fills |info| with information about the key certified by |certificate|.
+bool GetPublicKey(scoped_refptr<net::X509Certificate> certificate,
+                  SubjectPublicKeyInfo* info);
+
 // If the list of certificates could be successfully retrieved, |certs| will
 // contain the list of available certificates (maybe empty) and |error_message|
 // will be empty. If an error occurred, |certs| will be empty and