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..4199bccc4b6711ae59e92b263fe1fd1bb025b909 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,72 @@ 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
 };
 
+// This class contains the parameters required for signing data with a RSA key.
+class SignRSAParams {

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

I still have an issue with the SignRSAParams, which is more or less the same as
the issue with GetPublicKeyResult.

I don't think either of these APIs needs to be explicitly using a struct to
bundle arguments. I have no objection if your internal implementation uses such
a struct for thread marshalling, but I can't help but feel these should be two
explicit methods

SignRSA[PKCS1?]Digest(token_id, data, public_key, hash_algorithm, callback,
browser_context)

SignRSA[PKCS1?]Raw(token_id, data, public_key, callback, browser_context)


The struct created the case where the object could be in an inconsitent state,
which you've now has to resolve by making this object be a class with heap-based
helper functions.

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

If I follow your reasoning correctly, than _no_ API in Chrome should ever expose
arguments bundled in a struct/class.
I find that very surprising for the reasons explained:

- Code using the API will have to pass around individual arguments even if it
should/could be oblivious to the individual arguments. That creates unnecessary
depedencies and thus makes future maintenance and refactorings harder.

- Similarly for the reader, this tends to expose more details than he needs to
see. In some cases it is totally sufficient to know about a 'sign operation with
arguments X' vs. a 'sign operation with public key XY and hash algorithm YZ and
data D, ....'. If details have to be known, he can look back at the source of
the argument.

- Unnamed arguments: On the caller site multiple arguments of the same type are
indistinguishable. A struct/class can clarify that.

- Boilerplate: in every case it increases the amount of code necessary to call
the function.
It complicates the layers on top as well.
As I explained already, the SignRSA function _is_ already in the namespace
'subtle'. The public interface (except the extension API) for signing is in the
PlatformKeysService.
Inconsistency can be CHECKed without much boilerplate overhead.
It doesn't have to be heap-based but comes in handy here as it prevents repeated
copies of the data and public_key strings.

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

Generally speaking, yes. That's correct. Don't make C++ try to look like
Javascript.
I admit I don't entirely follow this argument, but I consider the line of
reasoning somewhat specious, to put it bluntly.
I _strongly_ disagree with this. I think your proposed change harms, not helps,
readability.
 
This is C++. This is true for every single method call ever. This is perhaps the
weakest argument.
Subtle is not an escape clause to make code less readable or to diverge from
common practice. Yours is the first code I've seen in five years on Chrome to
make this argument, and that alone should be somewhat telling. I'm not trying to
pick on you - I think this code, from a JS idiomatic sense, probably makes
sense. But you essentially have two methods, with two common arguments. That
does not make a suitable justification for a struct, with creators, and added
DCHECKs, and further validation being required on methods that accept this.
That you have to at all is a design flaw.


It's clear from your reply that you feel this is important that it's an
acceptable pattern. I feel the otherwise - I think it hinders readability, adds
more code than necessary (look at how much just to describe the struct, let
alone add the error checks), and it does not feel idiomatic C++ to me, in
Chromium or at large. I don't feel comfortable approving the change with the
struct, because I don't see any reason for its technical necessity. We can take
it to the mailing list if you'd like additional opinions, because I don't want
to be "Do it my way or else", but I also feel like I need to hold the line on
"Don't do it this way".

[pneubeck (no reviews), 2015/02/11 15:57:51]

Aaalright... the line has to bend to not cross it.

On 2015/02/10 21:25:28, Ryan Sleevi wrote:
I referenced 'namespace subtle' only to explain that this could qualify as
'internal implementation' of the PlatformKeysService because it didn't happen to
be in platform_keys_service_nss.cc .
I mentioned that in reply to your "... I have no objection if your internal
implementation uses such a struct for thread marshalling ...". But apparently I
didn't read your emphasis on 'thread marshalling'.

+ public:
+  // Creates parameters for signing a digest, meaning that |data| will at first
+  // be hashed using the algorithm identified by |hash_algorithm|, then PKCS#1
+  // v1.5 padded and afterwards signed.
+  static scoped_ptr<SignRSAParams> CreateSignParamsWithHashing(
+      const std::string data,
+      const std::string public_key,
+      HashAlgorithm hash_algorithm);
+
+  // Creates parameters for direct signing, meaning that |data| will be PKCS#1
+  // v1.5 padded and afterwards signed. No hashing/digesting will be done.
+  // The size of |data| (number of octets) must be smaller than k - 11, where k
+  // is the key size in octets.
+  static scoped_ptr<SignRSAParams> CreateDirectSignParams(
+      const std::string data,
+      const std::string public_key);
+
+  ~SignRSAParams();
+
+  // The data that will be signed.
+  const std::string& data() const { return data_; }
+
+  // The public key identifying the private key that will be used for signing.
+  // Must be the DER encoding of a GetPublicKeyResult.
+  const std::string& public_key() const { return 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| is set to a value != NONE.
+  bool sign_direct_pkcs_padded() const { return sign_direct_pkcs_padded_; }
+
+  // Determines the hash algorithm that is used to digest |data| before signing.
+  // Returns HASH_ALGORITHM_NONE if |sign_direct_pkcs_padded| is true.
+  HashAlgorithm hash_algorithm() const { return hash_algorithm_; }
+
+ private:
+  SignRSAParams(const std::string& data_,
+                const std::string& public_key_,
+                bool sign_direct_pkcs_padded_,
+                HashAlgorithm hash_algorithm_);
+
+  // The data that will be signed.
+  const std::string data_;
+
+  // Must be the DER encoding of a GetPublicKeyResult.
+  const 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.
+  const bool sign_direct_pkcs_padded_;
+
+  // Determines the hash algorithm that is used to digest |data| before signing.
+  // Ignored if |sign_direct_pkcs_padded| is true.
+  const HashAlgorithm hash_algorithm_;

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

DISALLOW_COPY_AND_ASSIGN

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

Acknowledged.

+};
+
 struct ClientCertificateRequest {
   ClientCertificateRequest();
   ~ClientCertificateRequest();
@@ -57,6 +113,23 @@ struct ClientCertificateRequest {
   std::vector<std::string> certificate_authorities;
 };
 
+// This struct is used to hold the results of the GetPublicKey() function.
+// Holds details about a certificate's (subject) key, i.e. the X.509 Subject
+// Public Key Info (SPKI) of the certificate.
+struct GetPublicKeyResult {
+  GetPublicKeyResult();
+  ~GetPublicKeyResult();
+
+  // 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;
+
+  // The size of the key in bits.
+  size_t key_size_bits = 0;
+};
+
 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 +150,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 +175,14 @@ void SelectClientCertificates(const ClientCertificateRequest& request,
 
 }  // namespace subtle
 
+// If possible, fills |info| with information about the key certified by
+// |certificate| and returns true.
+// If an error occurs, does not modify |info| and returns false.
+// It is handled as an error, if the key is an RSA key with public exponent
+// not equal to 65537.
+bool GetPublicKey(scoped_refptr<net::X509Certificate> certificate,

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

This should be "const scoped_refptr<...>&"

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

Done.

+                  GetPublicKeyResult* 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