Sign self-signed certs with SHA256.

Avoid creating keys and self-signed certs separately.

Security best-practices dictate that the same public key should not
be signed by multiple hash algorithms.  This CL prevents that
problem by replacing x509_util::CreateSelfSignedCertificate with
CreateKeyAndSelfSignedCertificate.

This should allow us to change hash functions in x509_utils without
worrying that users may re-sign old keys with the new hash function.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232292

[bemasc, 2013-10-18 00:50:13]

Some things I'd particularly appreciate review on:
1.  Is this an idiomatic design for distinguishing between public and internal
functions in x509_utils?
2.  Is it OK to make attestation_policy_observer_unittest.cc to generate a new
keypair from scratch in every test?

[bemasc, 2013-10-18 15:23:02]

On 2013/10/18 00:50:13, bemasc wrote:
> Some things I'd particularly appreciate review on:
> 1.  Is this an idiomatic design for distinguishing between public and internal
> functions in x509_utils?
> 2.  Is it OK to make attestation_policy_observer_unittest.cc to generate a new
> keypair from scratch in every test?

Oh, and
3.  Is "&raw_key" an idiomatic design for returning an uncopyable object that
may fail?  Unfortunately scoped_ptr in Chromium doesn't seem to have an
.accept() method, which would be convenient for this.

[Darren Krahn, 2013-10-18 21:01:32]

Hey, I'm the unofficial owner of chromeos/attestation.  The hard-coded key is
only for performance -- I'll let you judge whether performance while generating
keys every time is reasonable.  I would prefer keeping the hard-coded key but if
this is the only affected test then it may not be worth the effort.

https://codereview.chromium.org/27832002/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/27832002/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:197:
scoped_ptr<crypto::RSAPrivateKey> key(raw_key);  // Just deletes the key.
optional nit: I would say just call 'delete' at this point.

[bemasc, 2013-10-18 22:03:34]

A changed landed yesterday to support self-signed certs with OpenSSL.  That has
now been merged in.

https://codereview.chromium.org/27832002/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/27832002/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:197:
scoped_ptr<crypto::RSAPrivateKey> key(raw_key);  // Just deletes the key.
On 2013/10/18 21:01:32, Darren Krahn wrote:
> optional nit: I would say just call 'delete' at this point.

Done.

[bemasc, 2013-10-18 22:25:34]

On 2013/10/18 21:01:32, Darren Krahn wrote:
> Hey, I'm the unofficial owner of chromeos/attestation.  The hard-coded key is
> only for performance -- I'll let you judge whether performance while
generating
> keys every time is reasonable.  I would prefer keeping the hard-coded key but
if
> this is the only affected test then it may not be worth the effort.

There's very little effort involved for me.  It's more a question of design.

Part of the point of this CL is to prevent users from signing the same key
twice, so that we can freely upgrade the key signing algorithms without worrying
about signing old keys.  To that end, the key-signing functions are now either
deprecated or "internal-only".

So the question is, is it worth using one of those functions in unit tests in
another module, in order to save 100ms on tests?  I don't know the answer to
that.

[Ryan Sleevi, 2013-10-18 22:44:44]

On 2013/10/18 15:23:02, bemasc wrote:
> On 2013/10/18 00:50:13, bemasc wrote:
> > Some things I'd particularly appreciate review on:
> > 1.  Is this an idiomatic design for distinguishing between public and
internal
> > functions in x509_utils?
> > 2.  Is it OK to make attestation_policy_observer_unittest.cc to generate a
new
> > keypair from scratch in every test?
> 
> Oh, and
> 3.  Is "&raw_key" an idiomatic design for returning an uncopyable object that
> may fail?  Unfortunately scoped_ptr in Chromium doesn't seem to have an
> .accept() method, which would be convenient for this.

Pass a scoped_ptr<Foo>* foo, and use foo->reset(raw_key)

[Ryan Sleevi, 2013-10-18 22:45:13]

On 2013/10/18 00:50:13, bemasc wrote:
> Some things I'd particularly appreciate review on:
> 1.  Is this an idiomatic design for distinguishing between public and internal
> functions in x509_utils?

Is it necessary to actually expose in the header? Seems like you can do this
within an unnamed namespace in the .cc

> 2.  Is it OK to make attestation_policy_observer_unittest.cc to generate a new
> keypair from scratch in every test?

[Ryan Sleevi, 2013-10-18 22:45:22]

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.cc#ne...
net/cert/x509_util.cc:19: static const uint16 kRSAKeyLength = 1024;
Switching to SHA-256 but using RSA-1024 provides no security benefits. RSA-2048
or greater.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.cc#ne...
net/cert/x509_util.cc:22: // digest algorithm.
It's a bit odd, this comment, since you use it in CreateKeyAndDomainBoundCertEC

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h
File net/cert/x509_util.h (right):

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:46: crypto::ECPrivateKey** key,
Pass a scoped_ptr<crypto::ECPrivateKey>*, rather than a pointer-pointer.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:52: crypto::HMAC::HashAlgorithm alg,
The dependency on crypto::HMAC is wrong here.

Likewise, you don't need to expose this in a public header.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:80: crypto::RSAPrivateKey** key,
ditto scoped_ptr

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:95: // signed again later with a different algorithm. 
Eventually, all users of this
You're not signing keys, you're signing data with a key.

because you should not re-use a key for signing data with multiple signature
algorithms or parameters.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util_nss.cc
File net/cert/x509_util_nss.cc (right):

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util_nss.c...
net/cert/x509_util_nss.cc:137: SECOidTag
HashAlgorithmToIdTag(crypto::HMAC::HashAlgorithm alg) {
ToSECOid

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util_nss.c...
net/cert/x509_util_nss.cc:159: SECOidTag alg_id_tag) {
s/alg_id_tag/hash_algorithm/

[bemasc, 2013-10-18 22:52:22]

On 2013/10/18 22:45:13, Ryan Sleevi wrote:
> On 2013/10/18 00:50:13, bemasc wrote:
> > Some things I'd particularly appreciate review on:
> > 1.  Is this an idiomatic design for distinguishing between public and
internal
> > functions in x509_utils?
> 
> Is it necessary to actually expose in the header? Seems like you can do this
> within an unnamed namespace in the .cc

I couldn't figure out how to do that while sharing the same implementation of
CreateKeyAndSelfSignedCert for OpenSSL and NSS.  Is there a good way to do that?
 Or would you prefer to duplicate that function?

[Ryan Sleevi, 2013-10-18 22:57:36]

Maybe I missed something while reviewing, but why is it necessary to both
specify the hash AND have a new key generated?

You should only need to do one or the other - either supply the hash/sig alg,
and let the legacy code keep using SHA-1 and new code using SHA-256, or generate
a key on the fly, and have it always use "best hash at time of generation"

My inclination is caller-specifies-hash is fine, along with
caller-specifying-key.

Yes, this creates a potential issue if callers misuse this API. But it avoids
the need for re-generating a new key every time, and, if I'm reading right,
avoids the need for the Internal helper functions.

[bemasc, 2013-10-19 00:37:53]

On 2013/10/18 22:44:44, Ryan Sleevi wrote:
> On 2013/10/18 15:23:02, bemasc wrote:
> > On 2013/10/18 00:50:13, bemasc wrote:
> > > Some things I'd particularly appreciate review on:
> > > 1.  Is this an idiomatic design for distinguishing between public and
> internal
> > > functions in x509_utils?
> > > 2.  Is it OK to make attestation_policy_observer_unittest.cc to generate a
> new
> > > keypair from scratch in every test?
> > 
> > Oh, and
> > 3.  Is "&raw_key" an idiomatic design for returning an uncopyable object
that
> > may fail?  Unfortunately scoped_ptr in Chromium doesn't seem to have an
> > .accept() method, which would be convenient for this.
> 
> Pass a scoped_ptr<Foo>* foo, and use foo->reset(raw_key)
Done.  Actually I did "if (success) foo->reset(temp_key.release())", for
automatic cleanup on failure.

[bemasc, 2013-10-19 00:47:44]

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.cc#ne...
net/cert/x509_util.cc:19: static const uint16 kRSAKeyLength = 1024;
On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> Switching to SHA-256 but using RSA-1024 provides no security benefits.
RSA-2048
> or greater.

Done.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.cc#ne...
net/cert/x509_util.cc:22: // digest algorithm.
On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> It's a bit odd, this comment, since you use it in
CreateKeyAndDomainBoundCertEC

Fixed.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h
File net/cert/x509_util.h (right):

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:46: crypto::ECPrivateKey** key,
On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> Pass a scoped_ptr<crypto::ECPrivateKey>*, rather than a pointer-pointer.

Done.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:52: crypto::HMAC::HashAlgorithm alg,
On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> The dependency on crypto::HMAC is wrong here.

I'm not sure what you mean.

> Likewise, you don't need to expose this in a public header.

In light of the new design, I've removed "Internal" from the name.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:80: crypto::RSAPrivateKey** key,
On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> ditto scoped_ptr

Done.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:95: // signed again later with a different algorithm. 
Eventually, all users of this
On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> You're not signing keys, you're signing data with a key.
> 
> because you should not re-use a key for signing data with multiple signature
> algorithms or parameters.

Done.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:95: // signed again later with a different algorithm. 
Eventually, all users of this
Removed this function.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util_nss.cc
File net/cert/x509_util_nss.cc (right):

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util_nss.c...
net/cert/x509_util_nss.cc:137: SECOidTag
HashAlgorithmToIdTag(crypto::HMAC::HashAlgorithm alg) {
On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> ToSECOid

Done.

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util_nss.c...
net/cert/x509_util_nss.cc:159: SECOidTag alg_id_tag) {
On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> s/alg_id_tag/hash_algorithm/

Done.

[bemasc, 2013-10-19 00:53:30]

On 2013/10/18 22:57:36, Ryan Sleevi wrote:
> Maybe I missed something while reviewing, but why is it necessary to both
> specify the hash AND have a new key generated?

Not sure I understand this sentence, but I think I get the idea.

> My inclination is caller-specifies-hash is fine, along with
> caller-specifying-key.
> 
> Yes, this creates a potential issue if callers misuse this API. But it avoids
> the need for re-generating a new key every time, and, if I'm reading right,
> avoids the need for the Internal helper functions.

OK. Now there is CreateSelfSignedCert, which takes a hash and a key, and
CreateKeyAndSelfSignedCert, which uses SHA-256 and RSA-2048.  Both are "public".
 Same for the BoundCertEC versions.

[bemasc, 2013-10-21 17:46:05]

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.cc#ne...
net/cert/x509_util.cc:19: static const uint16 kRSAKeyLength = 1024;
On 2013/10/19 00:47:45, bemasc wrote:
> On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> > Switching to SHA-256 but using RSA-1024 provides no security benefits.
> RSA-2048
> > or greater.
> 
> Done.

I've switched this back to RSA-1024.  juberti@ recommends that we land this
change with RSA-1024, and benchmark on the slowest supported ARM devices before
upgrading to 2048.

[Ryan Sleevi, 2013-10-21 17:56:13]

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h
File net/cert/x509_util.h (right):

https://codereview.chromium.org/27832002/diff/457001/net/cert/x509_util.h#new...
net/cert/x509_util.h:52: crypto::HMAC::HashAlgorithm alg,
On 2013/10/19 00:47:45, bemasc wrote:
> On 2013/10/18 22:45:23, Ryan Sleevi wrote:
> > The dependency on crypto::HMAC is wrong here.
> 
> I'm not sure what you mean.

I meant that X.509 has nothing to do with HMAC. You're just overloading it
because it has hash names.

I see us either moving crypto::HMAC::HashAlgorithm into crypto/, or making a
specific hash algorithm for the cert. Having crypto::HashTo[EVP/OidTag] in
crypto_util_openssl/crypto_util_nss is one approach, so that these functions can
use it, or simply defining a new enum for the supported *cert* hash algs.

Considering that there are other types of cert types that don't align with HMAC
(eg: RSA-PSS), I don't want us to overload the enum.

> 
> > Likewise, you don't need to expose this in a public header.
> 
> In light of the new design, I've removed "Internal" from the name.

[Ryan Sleevi, 2013-10-21 17:58:00]

So, looks good, mod the crypto::HMAC concern. I'll give the final stamp if you
can address that.

[bemasc, 2013-10-23 00:40:33]

On 2013/10/21 17:56:13, Ryan Sleevi wrote:
> I see us either moving crypto::HMAC::HashAlgorithm into crypto/, or making a
> specific hash algorithm for the cert.

I did the latter.  PTAL.

[Ryan Sleevi, 2013-10-29 19:49:39]

lgtm

https://codereview.chromium.org/27832002/diff/1277001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/27832002/diff/1277001/net/cert/x509_util.cc#n...
net/cert/x509_util.cc:75: if (success) {
style nit: Don't need braces here. Mostly because it's inconsistent with net/
and with lines 65/89 to include the braces for the one-liner (outside of the
shared internal SPDY/QUIC code)

[bemasc, 2013-10-29 20:25:25]

https://codereview.chromium.org/27832002/diff/1277001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/27832002/diff/1277001/net/cert/x509_util.cc#n...
net/cert/x509_util.cc:75: if (success) {
On 2013/10/29 19:49:40, Ryan Sleevi wrote:
> style nit: Don't need braces here. Mostly because it's inconsistent with net/
> and with lines 65/89 to include the braces for the one-liner (outside of the
> shared internal SPDY/QUIC code)

Done.

[commit-bot: I haz the power, 2013-10-29 20:45:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bemasc@chromium.org/27832002/1787001

[commit-bot: I haz the power, 2013-10-29 20:45:48]

Failed to apply patch for net/cert/x509_util_openssl.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file net/cert/x509_util_openssl.cc
  Hunk #1 succeeded at 18 with fuzz 1 (offset 3 lines).
  Hunk #2 FAILED at 64.
  Hunk #3 FAILED at 153.
  2 out of 3 hunks FAILED -- saving rejects to file
net/cert/x509_util_openssl.cc.rej

Patch:       net/cert/x509_util_openssl.cc
Index: net/cert/x509_util_openssl.cc
===================================================================
--- net/cert/x509_util_openssl.cc	(revision 229411)
+++ net/cert/x509_util_openssl.cc	(working copy)
@@ -15,6 +15,20 @@
 
 namespace net {
 
+namespace {
+
+const EVP_MD* ToEVP(x509_util::DigestAlgorithm alg) {
+  switch (alg) {
+    case x509_util::DIGEST_SHA1:
+      return EVP_sha1();
+    case x509_util::DIGEST_SHA256:
+      return EVP_sha256();
+  }
+  return NULL;
+}
+
+}  // namespace
+
 namespace x509_util {
 
 bool IsSupportedValidityRange(base::Time not_valid_before,
@@ -50,18 +64,19 @@
   return true;
 }
 
-bool CreateDomainBoundCertEC(
-    crypto::ECPrivateKey* key,
-    const std::string& domain,
-    uint32 serial_number,
-    base::Time not_valid_before,
-    base::Time not_valid_after,
-    std::string* der_cert) {
+bool CreateDomainBoundCertEC(crypto::ECPrivateKey* key,
+                             DigestAlgorithm alg,
+                             const std::string& domain,
+                             uint32 serial_number,
+                             base::Time not_valid_before,
+                             base::Time not_valid_after,
+                             std::string* der_cert) {
   NOTIMPLEMENTED();
   return false;
 }
 
 bool CreateSelfSignedCert(crypto::RSAPrivateKey* key,
+                          DigestAlgorithm alg,
                           const std::string& common_name,
                           uint32 serial_number,
                           base::Time not_valid_before,
@@ -139,8 +154,15 @@
     return false;
   }
 
+  // Get the message digest algorithm
+  const EVP_MD* md = ToEVP(alg);
+  if (!md) {
+    LOG(ERROR) << "Unrecognized hash algorithm.";
+    return false;
+  }
+
   // Sign it with the private key.
-  if (!X509_sign(cert.get(), key->key(), EVP_sha1())) {
+  if (!X509_sign(cert.get(), key->key(), md)) {
     LOG(ERROR) << "Could not sign certificate with key.";
     return false;
   }

[commit-bot: I haz the power, 2013-10-30 00:22:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bemasc@chromium.org/27832002/2337001

[commit-bot: I haz the power, 2013-10-30 01:19:09]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[bemasc, 2013-10-30 19:03:13]

This change is "fully reviewed" but needs some more approvals.

wez: Please review minor change to remoting/base/rsa_key_pair.cc.

fischman: Please review change to content/browser/media/webrtc_identity_store.cc

pastarmovj: Please review change to
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc

[Ami GONE FROM CHROMIUM, 2013-10-30 19:21:56]

Rubberstamp OWNERS LGTM for c/b/media/ but see comment below before acting on
this LGTM.

https://codereview.chromium.org/27832002/diff/2337001/content/browser/media/w...
File content/browser/media/webrtc_identity_store.cc (right):

https://codereview.chromium.org/27832002/diff/2337001/content/browser/media/w...
content/browser/media/webrtc_identity_store.cc:44: bool success =
net::x509_util::CreateKeyAndSelfSignedCert(
Does this CL change what used to be signed with SHA1 to being signed with
SHA256?  If so, is that a problem for cached identities?  Please get jiayl@'s
signoff on this part of this CL.

[bemasc, 2013-10-30 19:30:48]

https://codereview.chromium.org/27832002/diff/2337001/content/browser/media/w...
File content/browser/media/webrtc_identity_store.cc (right):

https://codereview.chromium.org/27832002/diff/2337001/content/browser/media/w...
content/browser/media/webrtc_identity_store.cc:44: bool success =
net::x509_util::CreateKeyAndSelfSignedCert(
On 2013/10/30 19:21:57, Ami Fischman wrote:
> Does this CL change what used to be signed with SHA1 to being signed with
> SHA256?  If so, is that a problem for cached identities?  Please get jiayl@'s
> signoff on this part of this CL.

This CL is intended not to re-sign any old keys using SHA256.  In the case of
webrtc, the code being replaced here both generates a new keypair and signs the
certificate.  Pre-existing keys should not be affected.

[Wez, 2013-10-30 19:45:07]

lgtm

https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
File remoting/base/rsa_key_pair.cc (right):

https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
remoting/base/rsa_key_pair.cc:98: // multiple signature algorithms.
So should we be making arrangements to switch Chromoting to SHA256?

[bemasc, 2013-10-30 20:18:44]

https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
File remoting/base/rsa_key_pair.cc (right):

https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
remoting/base/rsa_key_pair.cc:98: // multiple signature algorithms.
On 2013/10/30 19:45:08, Wez wrote:
> So should we be making arrangements to switch Chromoting to SHA256?

The important thing here is not really SHA1 vs. SHA256, but "hash agility" in
general.  I think you should probably be making arrangements to be able to
upgrade hash functions periodically without creating a big headache.

(My favorite discussion of hash agility is: http://valerieaurora.org/hash.html)

[Sergey Ulanov, 2013-10-31 06:25:27]

https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
File remoting/base/rsa_key_pair.cc (right):

https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
remoting/base/rsa_key_pair.cc:98: // multiple signature algorithms.
Why signing the same key with a different hash function is a problem?
Beside that, it it's not |key_| that's being signed, but the whole certificate,
so we are not signing the same data because serial number is generated randomly
and different expiration date is used every time. 

Also, we never rely on signature for self-signed certs, so the choice of the
hash function here is immaterial for security of the chromoting protocol.

[pastarmovj, 2013-10-31 09:24:16]

attestation owner LGTM.

[Ryan Sleevi, 2013-10-31 17:28:07]

On 2013/10/31 06:25:27, Sergey Ulanov wrote:
>
https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
> File remoting/base/rsa_key_pair.cc (right):
> 
>
https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
> remoting/base/rsa_key_pair.cc:98: // multiple signature algorithms.
> Why signing the same key with a different hash function is a problem?
> Beside that, it it's not |key_| that's being signed, but the whole
certificate,
> so we are not signing the same data because serial number is generated
randomly
> and different expiration date is used every time. 
> 
> Also, we never rely on signature for self-signed certs, so the choice of the
> hash function here is immaterial for security of the chromoting protocol.

For overall security, you should not re-use the key to sign messages with
multiple (different) hash algorithms. Even though RSA-PKCS#1v1.5 (which is what
is used when signing the cert) contains an embedded identifier (as part of the
digestAlgorithm structure), the overall security recommends not re-using the
key.

Hence this change.

[Sergey Ulanov, 2013-10-31 21:10:31]

ok, remoting - LGTM

On 2013/10/31 17:28:07, Ryan Sleevi wrote:
> On 2013/10/31 06:25:27, Sergey Ulanov wrote:
> >
>
https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
> > File remoting/base/rsa_key_pair.cc (right):
> > 
> >
>
https://codereview.chromium.org/27832002/diff/2337001/remoting/base/rsa_key_p...
> > remoting/base/rsa_key_pair.cc:98: // multiple signature algorithms.
> > Why signing the same key with a different hash function is a problem?
> > Beside that, it it's not |key_| that's being signed, but the whole
> certificate,
> > so we are not signing the same data because serial number is generated
> randomly
> > and different expiration date is used every time. 
> > 
> > Also, we never rely on signature for self-signed certs, so the choice of the
> > hash function here is immaterial for security of the chromoting protocol.
> 
> For overall security, you should not re-use the key to sign messages with
> multiple (different) hash algorithms. Even though RSA-PKCS#1v1.5 (which is
what
> is used when signing the cert) contains an embedded identifier (as part of the
> digestAlgorithm structure), the overall security recommends not re-using the
> key.
> 
> Hence this change.

[commit-bot: I haz the power, 2013-10-31 21:20:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bemasc@chromium.org/27832002/2337001

[commit-bot: I haz the power, 2013-10-31 23:11:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bemasc@chromium.org/27832002/2337001

[commit-bot: I haz the power, 2013-11-01 05:14:31]

Change committed as 232292