Generalize crypto::SignatureCreator to allow choice of hash function, so as to support SHA256 (not …

crypto/signature_creator.h

Index: crypto/signature_creator.h
diff --git a/crypto/signature_creator.h b/crypto/signature_creator.h
index 0f3e05b5d9c2dacd52499e4042bc20ab1adf0ba6..3683ec5ee3a3bbc4720425c0766613a3fb86a126 100644
--- a/crypto/signature_creator.h
+++ b/crypto/signature_creator.h
@@ -24,15 +24,25 @@ namespace crypto {
 class RSAPrivateKey;
 
 // Signs data using a bare private key (as opposed to a full certificate).
-// Currently can only sign data using SHA-1 with RSA encryption.
+// Currently can only sign data using SHA-1 or SHA-256 with RSA encryption.

[Ryan Sleevi, 2014/09/10 22:09:29]

Would you mind updating this comment to be more precise.

// Currently, can only sign data using SHA-1 or SHA-256, with RSA PKCS#1v1.5
encryption

[dougsteed, 2014/09/12 00:14:37]

Done in upcoming new version.

 class CRYPTO_EXPORT SignatureCreator {
  public:
+  // The set of supported hash functions. Extend as required.
+  enum HashAlgorithm {
+    SHA1,
+    SHA256,
+  };
+
   ~SignatureCreator();
 
   // Create an instance. The caller must ensure that the provided PrivateKey
-  // instance outlives the created SignatureCreator.
+  // instance outlives the created SignatureCreator. Uses SHA-1.
   static SignatureCreator* Create(RSAPrivateKey* key);

[Ryan Sleevi, 2014/09/10 22:09:29]

FWIW, Would prefer to update all of these callsites to be explicit about their
algorithm choice. Is that a lot?

[dougsteed, 2014/09/12 00:14:37]

No, it's not a lot, but our callsite itself has not yet been upstreamed so there
may be other callers not in evidence. What is the general position on
backwards-compatibility? I was trying to keep the existing function for any
existing callers. Could we deprecate the old function rather than removing it?
Or maybe also change the callsites we can see?

[Ryan Sleevi, 2014/09/12 00:19:02]

If it's upstream, we fix it in the entire codebase at once if reasonable.
If it's downstream/closed source, it's the responsibility of downstream to fix
their code.
I'm not sure I understand the last question.

I'm suggesting we update all callers in Chromium. If there are downstream
consumers, they update their calls when they sync.

This is true for any refactorings that go in in Chrome, from big or small.

 
+  // Create an instance. As above, but with the HashAlgorithm specified.
+  static SignatureCreator* CreateUsingSpecifiedHash(RSAPrivateKey* key,

[davidben, 2014/09/10 22:03:54]

Nit: Maybe s/UsingSpecified/With/ or s/UsingSpecified/Using/? Slightly shorter
and says the same thing. (Same below.)

[dougsteed, 2014/09/12 00:14:37]

Since you describe it as a nit, can I continue to use the name I proposed? Since
we are already using this in our code, it will be a lot more straightforward. I
actually considered both your alternatives when doing this originally, and I
preferred my proposal although it is longer. Reason is that even the existing
functions are still "using" (or "with") a hash, it's just that before the change
it was implicit (not "specified"). I will certainly change if you object
strongly.

[Ryan Sleevi, 2014/09/12 00:19:02]

I'm not sure why longer is better. CreateWithHash is a lot more concise.

But I think it's also influenced by the above remark regarding how we handle
upstream/downstream splits.

+                                                    HashAlgorithm hash_alg);
+
   // Signs the precomputed SHA-1 digest |data| using private |key| as
   // specified in PKCS #1 v1.5.

[Ryan Sleevi, 2014/09/10 22:09:29]

If adding hash-algorithm support, why not make the hash explicit here as well?

[dougsteed, 2014/09/12 00:14:37]

Done below. As with the above, I kept the existing function for
backwards-compatibility. Let me know if that is being too conservative.

   static bool Sign(RSAPrivateKey* key,
@@ -40,6 +50,14 @@ class CRYPTO_EXPORT SignatureCreator {
                    int data_len,
                    std::vector<uint8>* signature);
 
+  // Signs the precomputed |hash_alg| digest |data| using private |key| as
+  // specified in PKCS #1 v1.5.
+  static bool SignUsingSpecifiedHash(RSAPrivateKey* key,
+                                     HashAlgorithm hash_alg,
+                                     const uint8* data,
+                                     int data_len,
+                                     std::vector<uint8>* signature);
+
   // Update the signature with more data.
   bool Update(const uint8* data_part, int data_part_len);