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

Generalize crypto::SignatureCreator to allow choice of hash function, so as to support SHA256 (not just SHA1).

BUG=412531
R=rsleevi@chromium.org,davidben@chromium.org

TBR=pfeldman@chromium.org

Committed: https://crrev.com/0cf460ec9f9b56c22c5101ff599e7e842c541089
Cr-Commit-Position: refs/heads/master@{#295747}

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

lgtm with two minor nits. Leaving Ryan to OWNERS stamp and in case he has strong
opinions on API naming.

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h
File crypto/signature_creator.h (right):

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:43: static SignatureCreator*
CreateUsingSpecifiedHash(RSAPrivateKey* key,
Nit: Maybe s/UsingSpecified/With/ or s/UsingSpecified/Using/? Slightly shorter
and says the same thing. (Same below.)

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator_uni...
File crypto/signature_creator_unittest.cc (right):

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator_uni...
crypto/signature_creator_unittest.cc:23: // This is the algorithm ID for SHA-1
with RSA encryption.
Nit: s/SHA-1/SHA-256/

[Ryan Sleevi, 2014-09-10 22:09:30]

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h
File crypto/signature_creator.h (right):

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:27: // Currently can only sign data using SHA-1 or
SHA-256 with RSA encryption.
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

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:40: static SignatureCreator* Create(RSAPrivateKey*
key);
FWIW, Would prefer to update all of these callsites to be explicit about their
algorithm choice. Is that a lot?

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:47: // specified in PKCS #1 v1.5.
If adding hash-algorithm support, why not make the hash explicit here as well?

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

Please take a look at my responses to the comments.
About to upload a new version with one updated comment.
Depending on reaction to my comments, will upload further revisions.

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h
File crypto/signature_creator.h (right):

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:27: // Currently can only sign data using SHA-1 or
SHA-256 with RSA encryption.
On 2014/09/10 22:09:29, Ryan Sleevi wrote:
> 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

Done in upcoming new version.

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:40: static SignatureCreator* Create(RSAPrivateKey*
key);
On 2014/09/10 22:09:29, Ryan Sleevi wrote:
> FWIW, Would prefer to update all of these callsites to be explicit about their
> algorithm choice. Is that a lot?

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?

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:43: static SignatureCreator*
CreateUsingSpecifiedHash(RSAPrivateKey* key,
On 2014/09/10 22:03:54, David Benjamin wrote:
> Nit: Maybe s/UsingSpecified/With/ or s/UsingSpecified/Using/? Slightly shorter
> and says the same thing. (Same below.)
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.

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:47: // specified in PKCS #1 v1.5.
On 2014/09/10 22:09:29, Ryan Sleevi wrote:
> If adding hash-algorithm support, why not make the hash explicit here as well?

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

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

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h
File crypto/signature_creator.h (right):

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:40: static SignatureCreator* Create(RSAPrivateKey*
key);
On 2014/09/12 00:14:37, dougsteed wrote:
> On 2014/09/10 22:09:29, Ryan Sleevi wrote:
> > FWIW, Would prefer to update all of these callsites to be explicit about
their
> > algorithm choice. Is that a lot?
> 
> 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? 

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 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?

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.

https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
crypto/signature_creator.h:43: static SignatureCreator*
CreateUsingSpecifiedHash(RSAPrivateKey* key,
On 2014/09/12 00:14:37, dougsteed wrote:
> On 2014/09/10 22:03:54, David Benjamin wrote:
> > Nit: Maybe s/UsingSpecified/With/ or s/UsingSpecified/Using/? Slightly
shorter
> > and says the same thing. (Same below.)
> 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.

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.

[dougsteed, 2014-09-12 21:13:07]

On 2014/09/12 00:19:02, Ryan Sleevi wrote:
> https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h
> File crypto/signature_creator.h (right):
> 
>
https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
> crypto/signature_creator.h:40: static SignatureCreator* Create(RSAPrivateKey*
> key);
> On 2014/09/12 00:14:37, dougsteed wrote:
> > On 2014/09/10 22:09:29, Ryan Sleevi wrote:
> > > FWIW, Would prefer to update all of these callsites to be explicit about
> their
> > > algorithm choice. Is that a lot?
> > 
> > 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? 
> 
> 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 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?
> 
> 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.
> 
>
https://codereview.chromium.org/560583002/diff/1/crypto/signature_creator.h#n...
> crypto/signature_creator.h:43: static SignatureCreator*
> CreateUsingSpecifiedHash(RSAPrivateKey* key,
> On 2014/09/12 00:14:37, dougsteed wrote:
> > On 2014/09/10 22:03:54, David Benjamin wrote:
> > > Nit: Maybe s/UsingSpecified/With/ or s/UsingSpecified/Using/? Slightly
> shorter
> > > and says the same thing. (Same below.)
> > 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.
> 
> 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.

PTAL.

Removed my unnecessary attempt to maintain backwards compatibility, just
changed the function signatures to require specification of the hash algorithm,
rather than adding a new function.

Didn't change the names to add "WithHash" since as I am not retaining
the old functions it is not necessary to distinguish them, and there's no
inherent
need to include "WithHash" in the name any more than "WithKey".

Also updated the existing chromium call-sites to explicitly specify use of SHA1.

[Ryan Sleevi, 2014-09-12 21:17:10]

Thanks Doug!

LGTM

[dougsteed, 2014-09-12 21:28:22]

The CQ bit was checked by dougsteed@chromium.org

[commit-bot: I haz the power, 2014-09-12 21:31:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/560583002/40001

[commit-bot: I haz the power, 2014-09-12 21:49:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 21:49:21]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dougsteed, 2014-09-12 22:21:06]

dougsteed@chromium.org changed reviewers:
+ atwilson@chromium.org, kaznacheev@chromium.org, wez@chromium.org,
ygorshenin@chromium.org, yoz@chromium.org

[dougsteed, 2014-09-12 22:24:12]

On 2014/09/12 21:49:21, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Could the following owners review the corresponding files:
ygorshenin - c/b/chromeos/ownership/*
kazncheev - c/b/devtools/*
yoz - c/b/extensions/*
atwilson - components/policy/*
wez - remoting/*

[Yoyo Zhou, 2014-09-12 23:17:51]

extensions LGTM

[Wez, 2014-09-13 21:37:25]

remoting/ LGTM

[dougsteed, 2014-09-17 16:21:55]

dougsteed@chromium.org changed reviewers:
+ pfeldman@chromium.org
- kaznacheev@chromium.org

[dougsteed, 2014-09-17 16:24:16]

On 2014/09/12 22:24:12, dougsteed wrote:
> On 2014/09/12 21:49:21, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Could the following owners review the corresponding files:
> ygorshenin - c/b/chromeos/ownership/*
> kazncheev - c/b/devtools/*
> yoz - c/b/extensions/*
> atwilson - components/policy/*
> wez - remoting/*

pfeldman - c/b/devtools/*

(substituting pfeldman for kazncheev, as the latter is on vacation).

[ygorshenin1, 2014-09-17 18:26:05]

ygorshenin - c/b/chromeos/ownership/* lgtm, but you need to rebase your CL since
owner_settings_service.{cc|h} were moved to components/ownership/*.

[alokp, 2014-09-17 23:21:25]

alokp@chromium.org changed reviewers:
+ alokp@chromium.org

[alokp, 2014-09-17 23:21:26]

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
File crypto/signature_creator_openssl.cc (right):

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
crypto/signature_creator_openssl.cc:65: ScopedRSA rsa_key
(EVP_PKEY_get1_RSA(key->key()));
lint error: extra space

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
File crypto/signature_creator_unittest.cc (right):

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
crypto/signature_creator_unittest.cc:28: 
lint error: extra space

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
crypto/signature_creator_unittest.cc:143: }
no new line.

[dougsteed, 2014-09-18 18:04:40]

https://codereview.chromium.org/560583002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service.cc (right):

https://codereview.chromium.org/560583002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service.cc:98:
crypto::SignatureCreator::SHA1);
Noticed the missing ")" myself. Will be fixed in next upload.

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
File crypto/signature_creator_openssl.cc (right):

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
crypto/signature_creator_openssl.cc:65: ScopedRSA rsa_key
(EVP_PKEY_get1_RSA(key->key()));
On 2014/09/17 23:21:26, Alok Priyadarshi wrote:
> lint error: extra space

Done.

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
File crypto/signature_creator_unittest.cc (right):

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
crypto/signature_creator_unittest.cc:28: 
On 2014/09/17 23:21:26, Alok Priyadarshi wrote:
> lint error: extra space

Can't see the extra space, sorry.
Or are you referring to the blank line 28 ?

https://codereview.chromium.org/560583002/diff/40001/crypto/signature_creator...
crypto/signature_creator_unittest.cc:143: }
On 2014/09/17 23:21:26, Alok Priyadarshi wrote:
> no new line.

Done.

[alokp, 2014-09-18 18:21:29]

lgtm.

You may already know this - 'git cl format' is an amazing tool to format your
patch. I no longer worry about formatting when writing code.

[Andrew T Wilson (Slow), 2014-09-19 15:51:25]

policy lgtm

[dougsteed, 2014-09-19 16:15:45]

The CQ bit was checked by dougsteed@chromium.org

[commit-bot: I haz the power, 2014-09-19 16:17:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/560583002/60001

[pfeldman, 2014-09-19 16:32:14]

devtools lgtm

[commit-bot: I haz the power, 2014-09-19 18:46:17]

Committed patchset #4 (id:60001) as fb6fe2937547f2f3e210784c319c5f8004745fae

[commit-bot: I haz the power, 2014-09-19 18:46:59]

Patchset 4 (id:??) landed as
https://crrev.com/0cf460ec9f9b56c22c5101ff599e7e842c541089
Cr-Commit-Position: refs/heads/master@{#295747}