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 0e836f2df652fc705c32de4ae13ecf26e642ea3f..d0db0eff9d5025e1bcbfab16653a6da2a9bc50d4 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,6 +34,7 @@ extern const char kTokenIdSystem[];
 
 // Supported hash algorithms.
 enum HashAlgorithm {
+  HASH_ALGORITHM_NONE,

[Ryan Sleevi, 2015/02/03 01:44:49]

Needs documentation about expected behaviours (you've got it on 98/99, but
that's a *use* of this enum rather than an explanation about what this could
mean)

[pneubeck (no reviews), 2015/02/05 10:41:57]

Done.

   HASH_ALGORITHM_SHA1,
   HASH_ALGORITHM_SHA256,
   HASH_ALGORITHM_SHA384,
@@ -57,6 +54,20 @@ struct ClientCertificateRequest {
   std::vector<std::string> certificate_authorities;
 };
 
+struct PublicKeyInfo {

[Ryan Sleevi, 2015/02/03 01:44:50]

NEeds documentation

[pneubeck (no reviews), 2015/02/03 20:15:00]

changed the name to SPKI and documented it.

+  PublicKeyInfo();
+  ~PublicKeyInfo();
+
+  std::vector<char> public_key_spki_der;

[Ryan Sleevi, 2015/02/03 01:44:50]

Why |char| and not |uint8_t| ?

[pneubeck (no reviews), 2015/02/03 10:48:18]

Generated API code uses vector<char>, the code that I used from net/ mostly
std::string.
I'll change this back to std::string which is more consistent with the rest of
this file.

+  net::X509Certificate::PublicKeyType key_type;
+
+  // For RSA a public exponent of 65537 is assumed, so there is no member for
+  // that.

[Ryan Sleevi, 2015/02/03 01:44:50]

This doesn't always hold true

[pneubeck (no reviews), 2015/02/03 10:48:18]

Is it acceptable to restrict it to 65537 if I implement the verification of
that?
Or should we allow usage (but not generation) of such certs/keys?

+
+  // Set if |key_type| equals kPublicKeyTypeRSA.
+  unsigned int modulus_length_bits;

[Ryan Sleevi, 2015/02/03 01:44:50]

This is a weird interface then, as ec keys have a strength/public key length.

[pneubeck (no reviews), 2015/02/03 20:15:00]

changed the name to key_size_bits

+};
+
 namespace subtle {
 // Functions of this namespace shouldn't be called directly from the context of
 // an extension. Instead use PlatformKeysService which enforces restrictions
@@ -84,6 +95,8 @@ typedef base::Callback<void(const std::string& signature,
 // SubjectPublicKeyInfo. |callback| will be invoked with the signature or an
 // error message.
 // Currently supports RSA keys only.
+// If |hash_algorithm| is HASH_ALGORITHM_NONE, |data| will not be hashed before
+// signing. PKCS#1 v1.5 padding will still be applied.

[Ryan Sleevi, 2015/02/03 01:44:50]

OK, so from a documentation standpoint, it's a little weird.

Consider
- If this API is meant to take EC keys, what would HASH_ALGORITHM_NONE mean?
- The maximum size of |data| is limited in the case of PKCS#1 v1.5 signing with
an RSA key (to modulus_len - 11, IIRC).

Maybe this should be split into two APIs - Sign() and SignDirectPKCS1Padded()
[since lines 91-92 no longer apply, and it's not exactly Direct signing], with a
shared under the hood impl?

[pneubeck (no reviews), 2015/02/03 10:48:18]

This function is in namespace subtle and an implementation details of the
PlatformKeysService::Sign (more a SignRSA currently).
PlatformKeysService::Sign itself has some amount of implementation which I'd
hate to duplicate.

The question is how to pass around the Sign parameters "Sign(HashAlgorithm) or
DirectSign".
I got annoyed already by the huge number of arguments of this function that I'd
consider putting them into a struct including a new 'bool direct_sign':

struct SignParams { // or SignRSAParams
  std::string token_id;
  std::string public_key;
  HashAlgorithm hash_algorithm;  // Ignored if direct_sign==true.
  bool direct_sign;
  std::string data;
};

[pneubeck (no reviews), 2015/02/05 10:41:57]

done.
I addressed your comments by clearer documentation ('optionally digest...',
'hash algorithm ignored if direct_sign_pkcs_padded is true...') and by renaming
the Sign function to SignRSA.

 void Sign(const std::string& token_id,
           const std::string& public_key,
           HashAlgorithm hash_algorithm,
@@ -107,6 +120,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,
+                  PublicKeyInfo* 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