Implement chrome.platformKeys.getKeyPair().

chrome/browser/extensions/api/platform_keys/platform_keys_api.cc

Index: chrome/browser/extensions/api/platform_keys/platform_keys_api.cc
diff --git a/chrome/browser/extensions/api/platform_keys/platform_keys_api.cc b/chrome/browser/extensions/api/platform_keys/platform_keys_api.cc
index 9a4e2d29a5b39b1a1640573e98e742d965f4d367..1d78219a655069a514d9ee4502988e3834031742 100644
--- a/chrome/browser/extensions/api/platform_keys/platform_keys_api.cc
+++ b/chrome/browser/extensions/api/platform_keys/platform_keys_api.cc
@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/values.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service.h"
 #include "chrome/browser/chromeos/platform_keys/platform_keys_service_factory.h"
@@ -20,10 +21,47 @@ namespace extensions {
 namespace api_pk = api::platform_keys;
 namespace api_pki = api::platform_keys_internal;
 
+namespace {
+
+const char kErrorAlgorithmNotSupported[] = "Algorithm not supported.";
+const char kErrorInvalidX509Cert[] =
+    "Certificate is not a valid X.509 certificate.";
+
+struct PublicKeyInfo {
+  // The X.509 Subject Public Key Info of the key in DER encoding.
+  std::string public_key_spki_der;
+
+  // The type of the key.
+  net::X509Certificate::PublicKeyType key_type;

[Ryan Sleevi, 2015/02/10 21:25:28]

Why not default-assign this to unknown, given that this is a primitive type?

It's weird to have the =0 for key_size_bits, since the default constructor for
PublicKeyInfo that the compiler will generate will default-initialize (ergo,
zero-initialize numerics). The = 0 makes this explicit to the reader, but it
seems more useful to make explicit the default value of the key type (which
would be the enum value equivalent to zero, IIRC)

[pneubeck (no reviews), 2015/02/11 14:37:05]

Looking at the specification of initialization/default constructor which is
different for every C++ version, I'd better not rely on any of this.
Added the default key_type and keeping the size_t initialization which is
consistent with the style guide.

"Use in-class member initialization for simple initializations, especially when
a member variable must be initialized the same way in more than one constructor.

If your class defines member variables that aren't initialized in-class, and if
it has no other constructors, you must define a default constructor (one that
takes no arguments). [...]"

Done.

+
+  // The size of the key in bits.
+  size_t key_size_bits = 0;
+};
+
+// Builds a partial WebCrypto Algorithm object from the parameters available in
+// |key_info|, which must the info of an RSA key. This doesn't include sign/hash
+// parameters and thus isn't complete.
+// platform_keys::GetPublicKey() enforced the public exponent 65537.
+void BuildWebCryptoRSAAlgorithmDictionary(const PublicKeyInfo& key_info,
+                                          base::DictionaryValue* algorithm) {
+  CHECK_EQ(net::X509Certificate::kPublicKeyTypeRSA, key_info.key_type);
+  algorithm->SetStringWithoutPathExpansion("name", "RSASSA-PKCS1-v1_5");
+  algorithm->SetIntegerWithoutPathExpansion("modulusLength",
+                                            key_info.key_size_bits);
+
+  // Equals 65537.
+  const char defaultPublicExponent[] = {0x01, 0x00, 0x01};

[Ryan Sleevi, 2015/02/10 21:25:28]

nit: For safety (in the event of future type changes), this should be unsigned
char.

[pneubeck (no reviews), 2015/02/11 14:37:05]

base::BinaryValue::CreateWithCopiedBuffer takes a const char*.
changing to 'const unsigned char defaultPublicExponent[]' does not compile.

[Ryan Sleevi, 2015/02/13 03:24:28]

At this risk of pedantry, I'd recommend a reinterpret cast then.

{0xF1, 0x00, 0x01} can cause... problems. Much like {128, 0, 1}. That is, the
compiler may truncate to the maximum range (or may yell). Forcing it through the
reinterpret_cast<> leaves those bytes pristine in memory and unmolested in the
cast.

[pneubeck (no reviews), 2015/02/14 13:25:14]

I don't quite follow.
It's {0x01, ...} and not {0xF1, ...} which should not cause issues for any size
of char or any other numeric type.
Do you suggest protecting against an accidental change of
defaultPublicExponent's value (e.g. to {0xF1, ....}) or against a change of its
declared type or against a change of the used size of char?
(your first comment mentioned 'future type changes')

[pneubeck (no reviews), 2015/02/14 13:31:18]

added the reinterpert_cast to follow the pattern in net/

+  algorithm->SetWithoutPathExpansion(
+      "publicExponent",
+      base::BinaryValue::CreateWithCopiedBuffer(
+          defaultPublicExponent, arraysize(defaultPublicExponent)));
+}
+
+}  // namespace
+
 namespace platform_keys {
 
 const char kErrorInvalidToken[] = "The token is not valid.";
-const char kErrorAlgorithmNotSupported[] = "Algorithm not supported.";
 const char kTokenIdUser[] = "user";
 const char kTokenIdSystem[] = "system";
 
@@ -54,6 +92,41 @@ std::string PlatformKeysTokenIdToApiId(
 
 }  // namespace platform_keys
 
+PlatformKeysInternalGetPublicKeyFunction::
+    ~PlatformKeysInternalGetPublicKeyFunction() {
+}
+
+ExtensionFunction::ResponseAction
+PlatformKeysInternalGetPublicKeyFunction::Run() {
+  scoped_ptr<api_pki::GetPublicKey::Params> params(
+      api_pki::GetPublicKey::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params);
+
+  const std::vector<char>& cert_der = params->certificate;

[Ryan Sleevi, 2015/02/10 21:25:28]

BUG? What if |cert_der.empty()|

[pneubeck (no reviews), 2015/02/11 14:37:05]

net::X509Certificate::CreateFromBytes should likely handle this.
To be on the safe side, I added explicit handling of this case here.

Done.

[Ryan Sleevi, 2015/02/13 03:24:28]

It does not. It's an API violation to supply that. Either way, validating user
input should happen as early as possible, so thanks for adding this.

+  scoped_refptr<net::X509Certificate> cert_x509 =
+      net::X509Certificate::CreateFromBytes(vector_as_array(&cert_der),
+                                            cert_der.size());
+  if (!cert_x509)
+    return RespondNow(Error(kErrorInvalidX509Cert));
+
+  PublicKeyInfo key_info;
+  if (!chromeos::platform_keys::GetPublicKey(
+          cert_x509, &key_info.public_key_spki_der, &key_info.key_type,
+          &key_info.key_size_bits) ||
+      key_info.key_type != net::X509Certificate::kPublicKeyTypeRSA) {
+    return RespondNow(Error(kErrorAlgorithmNotSupported));
+  }
+
+  api_pki::GetPublicKey::Results::Algorithm algorithm;
+  BuildWebCryptoRSAAlgorithmDictionary(key_info,
+                                       &algorithm.additional_properties);
+
+  return RespondNow(ArgumentList(api_pki::GetPublicKey::Results::Create(
+      std::vector<char>(key_info.public_key_spki_der.begin(),

[Ryan Sleevi, 2015/02/10 21:25:28]

Why |char| and not |unsigned char|?

[pneubeck (no reviews), 2015/02/11 14:37:05]

api_pki::GetPublicKey::Results::Create is generated from the API's .idl .
The generator always creates the same C++ representation for all occurrences of
the JS ArrayBuffer type, namely std::vector<char> .

+                        key_info.public_key_spki_der.end()),
+      algorithm)));
+}
+
 PlatformKeysInternalSelectClientCertificatesFunction::
     ~PlatformKeysInternalSelectClientCertificatesFunction() {
 }
@@ -95,12 +168,27 @@ void PlatformKeysInternalSelectClientCertificatesFunction::
   DCHECK(matches);
   std::vector<linked_ptr<api_pk::Match>> result_matches;
   for (const scoped_refptr<net::X509Certificate>& match : *matches) {
+    PublicKeyInfo key_info;
+    if (!chromeos::platform_keys::GetPublicKey(
+            match, &key_info.public_key_spki_der, &key_info.key_type,
+            &key_info.key_size_bits)) {
+      LOG(ERROR) << "Could not retrieve public key info.";
+      continue;
+    }
+    if (key_info.key_type != net::X509Certificate::kPublicKeyTypeRSA) {
+      LOG(ERROR) << "Skipping unsupported certificate with non-RSA key.";
+      continue;
+    }
+
     linked_ptr<api_pk::Match> result_match(new api_pk::Match);
     std::string der_encoded_cert;
     net::X509Certificate::GetDEREncoded(match->os_cert_handle(),
                                         &der_encoded_cert);
     result_match->certificate.assign(der_encoded_cert.begin(),
                                      der_encoded_cert.end());
+
+    BuildWebCryptoRSAAlgorithmDictionary(
+        key_info, &result_match->key_algorithm.additional_properties);
     result_matches.push_back(result_match);
   }
   Respond(ArgumentList(
@@ -115,32 +203,45 @@ ExtensionFunction::ResponseAction PlatformKeysInternalSignFunction::Run() {
       api_pki::Sign::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params);
   std::string platform_keys_token_id;
-  if (!platform_keys::ValidateToken(params->token_id, &platform_keys_token_id))
+  if (!params->token_id.empty() &&
+      !platform_keys::ValidateToken(params->token_id,
+                                    &platform_keys_token_id)) {
     return RespondNow(Error(platform_keys::kErrorInvalidToken));
-
-  chromeos::platform_keys::HashAlgorithm hash_algorithm;
-  if (params->hash_algorithm_name == "SHA-1")
-    hash_algorithm = chromeos::platform_keys::HASH_ALGORITHM_SHA1;
-  else if (params->hash_algorithm_name == "SHA-256")
-    hash_algorithm = chromeos::platform_keys::HASH_ALGORITHM_SHA256;
-  else if (params->hash_algorithm_name == "SHA-384")
-    hash_algorithm = chromeos::platform_keys::HASH_ALGORITHM_SHA384;
-  else if (params->hash_algorithm_name == "SHA-512")
-    hash_algorithm = chromeos::platform_keys::HASH_ALGORITHM_SHA512;
-  else
-    return RespondNow(Error(platform_keys::kErrorAlgorithmNotSupported));
+  }
 
   chromeos::PlatformKeysService* service =
       chromeos::PlatformKeysServiceFactory::GetForBrowserContext(
           browser_context());
   DCHECK(service);
 
-  service->Sign(
-      platform_keys_token_id,
-      std::string(params->public_key.begin(), params->public_key.end()),
-      hash_algorithm, std::string(params->data.begin(), params->data.end()),
-      extension_id(),
-      base::Bind(&PlatformKeysInternalSignFunction::OnSigned, this));
+  if (params->hash_algorithm_name == "none") {

[Ryan Sleevi, 2015/02/10 21:25:28]

Did you mean to do case-sensitive comparisons for all of these?

[pneubeck (no reviews), 2015/02/11 14:37:05]

Yes.
This API function is internal and only called by specific Javascript code which
ensures to pass the right strings.
Once the json_schema_compiler supports it, these strings can be replaced by an
enum / generated constants.

+    service->SignRSAPKCS1Raw(
+        platform_keys_token_id,
+        std::string(params->data.begin(), params->data.end()),
+        std::string(params->public_key.begin(), params->public_key.end()),
+        extension_id(),
+        base::Bind(&PlatformKeysInternalSignFunction::OnSigned, this));
+  } else {
+    chromeos::platform_keys::HashAlgorithm hash_algorithm;
+    if (params->hash_algorithm_name == "SHA-1") {
+      hash_algorithm = chromeos::platform_keys::HASH_ALGORITHM_SHA1;
+    } else if (params->hash_algorithm_name == "SHA-256") {
+      hash_algorithm = chromeos::platform_keys::HASH_ALGORITHM_SHA256;
+    } else if (params->hash_algorithm_name == "SHA-384") {
+      hash_algorithm = chromeos::platform_keys::HASH_ALGORITHM_SHA384;
+    } else if (params->hash_algorithm_name == "SHA-512") {
+      hash_algorithm = chromeos::platform_keys::HASH_ALGORITHM_SHA512;
+    } else {
+      return RespondNow(Error(kErrorAlgorithmNotSupported));
+    }
+    service->SignRSAPKCS1Digest(
+        platform_keys_token_id,
+        std::string(params->data.begin(), params->data.end()),
+        std::string(params->public_key.begin(), params->public_key.end()),
+        hash_algorithm, extension_id(),
+        base::Bind(&PlatformKeysInternalSignFunction::OnSigned, this));
+  }
+
   return RespondLater();
 }