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.

Review URL: https://codereview.chromium.org/27832002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@232292 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 166 insertions, 33 deletions

diff --git a/net/cert/x509_util.cc b/net/cert/x509_util.cc
index 8beb557..cc0a2c5 100644
--- a/net/cert/x509_util.cc
+++ b/net/cert/x509_util.cc
@@ -4,13 +4,24 @@
 
 #include "net/cert/x509_util.h"
 
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/time/time.h"
+#include "crypto/ec_private_key.h"
+#include "crypto/rsa_private_key.h"
 #include "net/cert/x509_certificate.h"
 
 namespace net {
 
 namespace x509_util {
 
+// RSA keys created by CreateKeyAndSelfSignedCert will be of this length.
+static const uint16 kRSAKeyLength = 1024;
+
+// Certificates made by CreateKeyAndSelfSignedCert and
+//  CreateKeyAndDomainBoundCertEC will be signed using this digest algorithm.
+static const DigestAlgorithm kSignatureDigestAlgorithm = DIGEST_SHA256;
+
 ClientCertSorter::ClientCertSorter() : now_(base::Time::Now()) {}
 
 bool ClientCertSorter::operator()(
@@ -44,6 +55,53 @@ bool ClientCertSorter::operator()(
   return a_intermediates.size() < b_intermediates.size();
 }
 
+bool CreateKeyAndDomainBoundCertEC(const std::string& domain,
+                                   uint32 serial_number,
+                                   base::Time not_valid_before,
+                                   base::Time not_valid_after,
+                                   scoped_ptr<crypto::ECPrivateKey>* key,
+                                   std::string* der_cert) {
+  scoped_ptr<crypto::ECPrivateKey> new_key(crypto::ECPrivateKey::Create());
+  if (!new_key.get())
+    return false;
+
+  bool success = CreateDomainBoundCertEC(new_key.get(),
+                                         kSignatureDigestAlgorithm,
+                                         domain,
+                                         serial_number,
+                                         not_valid_before,
+                                         not_valid_after,
+                                         der_cert);
+  if (success)
+    key->reset(new_key.release());
+
+  return success;
+}
+
+bool CreateKeyAndSelfSignedCert(const std::string& subject,
+                                uint32 serial_number,
+                                base::Time not_valid_before,
+                                base::Time not_valid_after,
+                                scoped_ptr<crypto::RSAPrivateKey>* key,
+                                std::string* der_cert) {
+  scoped_ptr<crypto::RSAPrivateKey> new_key(
+      crypto::RSAPrivateKey::Create(kRSAKeyLength));
+  if (!new_key.get())
+    return false;
+
+  bool success = CreateSelfSignedCert(new_key.get(),
+                                      kSignatureDigestAlgorithm,
+                                      subject,
+                                      serial_number,
+                                      not_valid_before,
+                                      not_valid_after,
+                                      der_cert);
+  if (success)
+    key->reset(new_key.release());
+
+  return success;
+}
+
 }  // namespace x509_util
 
 }  // namespace net
diff --git a/net/cert/x509_util.h b/net/cert/x509_util.h
index 513878d..e6b0e44 100644
--- a/net/cert/x509_util.h
+++ b/net/cert/x509_util.h
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/time/time.h"
 #include "net/base/net_export.h"
 
@@ -22,6 +23,12 @@ class X509Certificate;
 
 namespace x509_util {
 
+// Supported digest algorithms for signing certificates.
+enum DigestAlgorithm {
+  DIGEST_SHA1,
+  DIGEST_SHA256
+};
+
 // Returns true if the times can be used to create an X.509 certificate.
 // Certificates can accept dates from Jan 1st, 1 to Dec 31, 9999.  A bug in NSS
 // limited the range to 1950-9999
@@ -30,25 +37,34 @@ namespace x509_util {
 NET_EXPORT_PRIVATE bool IsSupportedValidityRange(base::Time not_valid_before,
                                                  base::Time not_valid_after);
 
-// Creates a server bound certificate containing the public key in |key|.
+// Creates a private keypair and server bound certificate.
 // Domain, serial number and validity period are given as
 // parameters. The certificate is signed by the private key in |key|.
-// The hashing algorithm for the signature is SHA-1.
+// The signature algorithm may be updated periodically to match best practices.
 //
 // See Internet Draft draft-balfanz-tls-obc-00 for more details:
 // http://tools.ietf.org/html/draft-balfanz-tls-obc-00
-NET_EXPORT_PRIVATE bool CreateDomainBoundCertEC(
-    crypto::ECPrivateKey* key,
+NET_EXPORT_PRIVATE bool CreateKeyAndDomainBoundCertEC(
     const std::string& domain,
     uint32 serial_number,
     base::Time not_valid_before,
     base::Time not_valid_after,
+    scoped_ptr<crypto::ECPrivateKey>* key,
     std::string* der_cert);
 
-// Create a self-signed certificate containing the public key in |key|.
+// Helper function for CreateKeyAndDomainBoundCertEC.
+NET_EXPORT_PRIVATE 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);
+
+// Creates a public-private keypair and a self-signed certificate.
 // Subject, serial number and validity period are given as parameters.
-// The certificate is signed by the private key in |key|. The hashing
-// algorithm for the signature is SHA-1.
+// The certificate is signed by the private key in |key|. The key length and
+// signature algorithm may be updated periodically to match best practices.
 //
 // |subject| is a distinguished name defined in RFC4514 with _only_ a CN
 // component, as in:
@@ -62,7 +78,19 @@ NET_EXPORT_PRIVATE bool CreateDomainBoundCertEC(
 // 2. Self-signed certificates cannot be revoked.
 //
 // Use this certificate only after the above risks are acknowledged.
+NET_EXPORT bool CreateKeyAndSelfSignedCert(
+    const std::string& subject,
+    uint32 serial_number,
+    base::Time not_valid_before,
+    base::Time not_valid_after,
+    scoped_ptr<crypto::RSAPrivateKey>* key,
+    std::string* der_cert);
+
+// Creates a self-signed certificate from a provided key, using the specified
+// hash algorithm.  You should not re-use a key for signing data with multiple
+// signature algorithms or parameters.
 NET_EXPORT bool CreateSelfSignedCert(crypto::RSAPrivateKey* key,
+                                     DigestAlgorithm alg,
                                      const std::string& subject,
                                      uint32 serial_number,
                                      base::Time not_valid_before,
diff --git a/net/cert/x509_util_nss.cc b/net/cert/x509_util_nss.cc
index de09b95..67ad467 100644
--- a/net/cert/x509_util_nss.cc
+++ b/net/cert/x509_util_nss.cc
@@ -134,6 +134,16 @@ CERTCertificate* CreateCertificate(
   return cert;
 }
 
+SECOidTag ToSECOid(x509_util::DigestAlgorithm alg) {
+  switch (alg) {
+    case x509_util::DIGEST_SHA1:
+      return SEC_OID_SHA1;
+    case x509_util::DIGEST_SHA256:
+      return SEC_OID_SHA256;
+  }
+  return SEC_OID_UNKNOWN;
+}
+
 // Signs a certificate object, with |key| generating a new X509Certificate
 // and destroying the passed certificate object (even when NULL is returned).
 // The logic of this method references SignCert() in NSS utility certutil:
@@ -142,11 +152,12 @@ CERTCertificate* CreateCertificate(
 // certificate signing process.
 bool SignCertificate(
     CERTCertificate* cert,
-    SECKEYPrivateKey* key) {
+    SECKEYPrivateKey* key,
+    SECOidTag hash_algorithm) {
   // |arena| is used to encode the cert.
   PLArenaPool* arena = cert->arena;
   SECOidTag algo_id = SEC_GetSignatureAlgorithmOidTag(key->keyType,
-                                                      SEC_OID_SHA1);
+                                                      hash_algorithm);
   if (algo_id == SEC_OID_UNKNOWN)
     return false;
 
@@ -240,6 +251,7 @@ CERTName* CreateCertNameFromEncoded(PLArenaPool* arena,
 namespace x509_util {
 
 bool CreateSelfSignedCert(crypto::RSAPrivateKey* key,
+                          DigestAlgorithm alg,
                           const std::string& subject,
                           uint32 serial_number,
                           base::Time not_valid_before,
@@ -255,7 +267,7 @@ bool CreateSelfSignedCert(crypto::RSAPrivateKey* key,
   if (!cert)
     return false;
 
-  if (!SignCertificate(cert, key->key())) {
+  if (!SignCertificate(cert, key->key(), ToSECOid(alg))) {
     CERT_DestroyCertificate(cert);
     return false;
   }
@@ -280,6 +292,7 @@ bool IsSupportedValidityRange(base::Time not_valid_before,
 }
 
 bool CreateDomainBoundCertEC(crypto::ECPrivateKey* key,
+                             DigestAlgorithm alg,
                              const std::string& domain,
                              uint32 serial_number,
                              base::Time not_valid_before,
@@ -341,7 +354,7 @@ bool CreateDomainBoundCertEC(crypto::ECPrivateKey* key,
     return false;
   }
 
-  if (!SignCertificate(cert, key->key())) {
+  if (!SignCertificate(cert, key->key(), ToSECOid(alg))) {
     CERT_DestroyCertificate(cert);
     return false;
   }
diff --git a/net/cert/x509_util_nss_unittest.cc b/net/cert/x509_util_nss_unittest.cc
index 968cc14..0ad5ecd 100644
--- a/net/cert/x509_util_nss_unittest.cc
+++ b/net/cert/x509_util_nss_unittest.cc
@@ -141,21 +141,20 @@ void VerifyDomainBoundCert(const std::string& domain,
 
 }  // namespace
 
-// This test creates a domain-bound cert from an EC private key and
+// This test creates a domain-bound cert and an EC private key and
 // then verifies the content of the certificate.
-TEST(X509UtilNSSTest, CreateDomainBoundCertEC) {
+TEST(X509UtilNSSTest, CreateKeyAndDomainBoundCertEC) {
   // Create a sample ASCII weborigin.
   std::string domain = "weborigin.com";
   base::Time now = base::Time::Now();
 
-  scoped_ptr<crypto::ECPrivateKey> private_key(
-      crypto::ECPrivateKey::Create());
+  scoped_ptr<crypto::ECPrivateKey> private_key;
   std::string der_cert;
-  ASSERT_TRUE(x509_util::CreateDomainBoundCertEC(
-      private_key.get(),
+  ASSERT_TRUE(x509_util::CreateKeyAndDomainBoundCertEC(
       domain, 1,
       now,
       now + base::TimeDelta::FromDays(1),
+      &private_key,
       &der_cert));
 
   VerifyDomainBoundCert(domain, der_cert);
diff --git a/net/cert/x509_util_openssl.cc b/net/cert/x509_util_openssl.cc
index 571d46e..2e52221 100644
--- a/net/cert/x509_util_openssl.cc
+++ b/net/cert/x509_util_openssl.cc
@@ -18,11 +18,26 @@
 
 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 {
 
 namespace {
 
 X509* CreateCertificate(EVP_PKEY* key,
+                        DigestAlgorithm alg,
                         const std::string& common_name,
                         uint32_t serial_number,
                         base::Time not_valid_before,
@@ -100,9 +115,19 @@ X509* CreateCertificate(EVP_PKEY* key,
   return cert.release();
 }
 
-bool SignAndDerEncodeCert(X509* cert, EVP_PKEY* key, std::string* der_encoded) {
+bool SignAndDerEncodeCert(X509* cert,
+                          EVP_PKEY* key,
+                          DigestAlgorithm alg,
+                          std::string* der_encoded) {
+  // 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, key, EVP_sha1())) {
+  if (!X509_sign(cert, key, md)) {
     LOG(ERROR) << "Could not sign certificate with key.";
     return false;
   }
@@ -188,6 +213,7 @@ bool IsSupportedValidityRange(base::Time not_valid_before,
 
 bool CreateDomainBoundCertEC(
     crypto::ECPrivateKey* key,
+    DigestAlgorithm alg,
     const std::string& domain,
     uint32 serial_number,
     base::Time not_valid_before,
@@ -197,6 +223,7 @@ bool CreateDomainBoundCertEC(
   // Create certificate.
   crypto::ScopedOpenSSL<X509, X509_free> cert(
       CreateCertificate(key->key(),
+                        alg,
                         "CN=anonymous.invalid",
                         serial_number,
                         not_valid_before,
@@ -237,10 +264,11 @@ bool CreateDomainBoundCertEC(
   }
 
   // Sign and encode it.
-  return SignAndDerEncodeCert(cert.get(), key->key(), der_cert);
+  return SignAndDerEncodeCert(cert.get(), key->key(), alg, der_cert);
 }
 
 bool CreateSelfSignedCert(crypto::RSAPrivateKey* key,
+                          DigestAlgorithm alg,
                           const std::string& common_name,
                           uint32 serial_number,
                           base::Time not_valid_before,
@@ -249,6 +277,7 @@ bool CreateSelfSignedCert(crypto::RSAPrivateKey* key,
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
   crypto::ScopedOpenSSL<X509, X509_free> cert(
       CreateCertificate(key->key(),
+                        alg,
                         common_name,
                         serial_number,
                         not_valid_before,
@@ -256,7 +285,7 @@ bool CreateSelfSignedCert(crypto::RSAPrivateKey* key,
   if (!cert.get())
     return false;
 
-  return SignAndDerEncodeCert(cert.get(), key->key(), der_encoded);
+  return SignAndDerEncodeCert(cert.get(), key->key(), alg, der_encoded);
 }
 
 bool ParsePrincipalKeyAndValueByIndex(X509_NAME* name,
diff --git a/net/cert/x509_util_openssl_unittest.cc b/net/cert/x509_util_openssl_unittest.cc
index 83677f8..c99f811 100644
--- a/net/cert/x509_util_openssl_unittest.cc
+++ b/net/cert/x509_util_openssl_unittest.cc
@@ -122,6 +122,7 @@ TEST(X509UtilOpenSSLTest, CreateDomainBoundCertEC) {
   std::string der_cert;
   ASSERT_TRUE(
       x509_util::CreateDomainBoundCertEC(private_key.get(),
+                                         x509_util::DIGEST_SHA1,
                                          domain,
                                          1,
                                          now,
diff --git a/net/cert/x509_util_unittest.cc b/net/cert/x509_util_unittest.cc
index 8fa24d2..716f61e 100644
--- a/net/cert/x509_util_unittest.cc
+++ b/net/cert/x509_util_unittest.cc
@@ -52,32 +52,33 @@ TEST(X509UtilTest, SortClientCertificates) {
   ASSERT_FALSE(certs[5].get());
 }
 
-// This test creates a self-signed cert from a private key and then verify the
+// This test creates a self-signed cert and a private key and then verifies the
 // content of the certificate.
-TEST(X509UtilTest, CreateSelfSigned) {
-  scoped_ptr<crypto::RSAPrivateKey> private_key(
-      crypto::RSAPrivateKey::Create(1024));
-
-  ASSERT_TRUE(private_key.get());
+TEST(X509UtilTest, CreateKeyAndSelfSigned) {
+  scoped_ptr<crypto::RSAPrivateKey> private_key;
 
   std::string der_cert;
-  ASSERT_TRUE(x509_util::CreateSelfSignedCert(
-      private_key.get(),
+  ASSERT_TRUE(x509_util::CreateKeyAndSelfSignedCert(
       "CN=subject",
       1,
       base::Time::Now(),
       base::Time::Now() + base::TimeDelta::FromDays(1),
+      &private_key,
       &der_cert));
 
+  ASSERT_TRUE(private_key.get());
+
   scoped_refptr<X509Certificate> cert(X509Certificate::CreateFromBytes(
       der_cert.data(), der_cert.size()));
   ASSERT_TRUE(cert.get());
 
   EXPECT_EQ("subject", cert->subject().GetDisplayName());
   EXPECT_FALSE(cert->HasExpired());
+}
 
-  cert = NULL;
-
+// This test creates a self-signed cert from a private key and then verifies the
+// content of the certificate.
+TEST(X509UtilTest, CreateSelfSigned) {
   const uint8 private_key_info[] = {
     0x30, 0x82, 0x02, 0x78, 0x02, 0x01, 0x00, 0x30,
     0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7,
@@ -165,18 +166,22 @@ TEST(X509UtilTest, CreateSelfSigned) {
   input.resize(sizeof(private_key_info));
   memcpy(&input.front(), private_key_info, sizeof(private_key_info));
 
-  private_key.reset(crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));
+  scoped_ptr<crypto::RSAPrivateKey> private_key(
+      crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));
   ASSERT_TRUE(private_key.get());
 
+  std::string der_cert;
   ASSERT_TRUE(x509_util::CreateSelfSignedCert(
       private_key.get(),
+      x509_util::DIGEST_SHA1,
       "CN=subject",
       1,
       base::Time::Now(),
       base::Time::Now() + base::TimeDelta::FromDays(1),
       &der_cert));
 
-  cert = X509Certificate::CreateFromBytes(der_cert.data(), der_cert.size());
+  scoped_refptr<X509Certificate> cert =
+      X509Certificate::CreateFromBytes(der_cert.data(), der_cert.size());
   ASSERT_TRUE(cert.get());
 
   EXPECT_EQ("subject", cert->subject().GetDisplayName());