Support generation of EC keys using P256 curve and support ECDSA certs.

webrtc/base/nssidentity.cc

Index: webrtc/base/nssidentity.cc
diff --git a/webrtc/base/nssidentity.cc b/webrtc/base/nssidentity.cc
index bbcc73e675ae769f993c0fe777c51022d7734361..54b00979b6ceaa1d660b21bd82c580828bc0cd13 100644
--- a/webrtc/base/nssidentity.cc
+++ b/webrtc/base/nssidentity.cc
@@ -47,43 +47,69 @@ NSSKeyPair::~NSSKeyPair() {
     SECKEY_DestroyPublicKey(pubkey_);
 }
 
-NSSKeyPair *NSSKeyPair::Generate() {
-  SECKEYPrivateKey *privkey = NULL;
-  SECKEYPublicKey *pubkey = NULL;
-  PK11RSAGenParams rsaparams;
-  rsaparams.keySizeInBits = 1024;
-  rsaparams.pe = 0x010001;  // 65537 -- a common RSA public exponent.
-
-  privkey = PK11_GenerateKeyPair(NSSContext::GetSlot(),
-                                 CKM_RSA_PKCS_KEY_PAIR_GEN,
-                                 &rsaparams, &pubkey, PR_FALSE /*permanent*/,
-                                 PR_FALSE /*sensitive*/, NULL);
+NSSKeyPair* NSSKeyPair::Generate(KeyType key_type) {
+  SECKEYPrivateKey* privkey = nullptr;
+  SECKEYPublicKey* pubkey = nullptr;
+  SSLKEAType ssl_kea_type;
+  if (key_type == KT_RSA) {
+    PK11RSAGenParams rsa_params;
+    rsa_params.keySizeInBits = 1024;
+    rsa_params.pe = 0x010001;  // 65537 -- a common RSA public exponent.
+
+    privkey = PK11_GenerateKeyPair(
+        NSSContext::GetSlot(), CKM_RSA_PKCS_KEY_PAIR_GEN, &rsa_params, &pubkey,
+        PR_FALSE /*permanent*/, PR_FALSE /*sensitive*/, nullptr);
+
+    ssl_kea_type = ssl_kea_rsa;
+  } else if (key_type == KT_ECDSA) {
+    unsigned char param_buf[12];  // OIDs are small
+    SECItem ecdsa_params = {siBuffer, param_buf, sizeof(param_buf)};
+    SECOidData* oid_data = SECOID_FindOIDByTag(SEC_OID_SECG_EC_SECP256R1);
+    if (!oid_data || oid_data->oid.len > sizeof(param_buf) - 2) {
+      LOG(LS_ERROR) << "oid_data incorrect: " << oid_data->oid.len;
+      return nullptr;
+    }
+    ecdsa_params.data[0] = SEC_ASN1_OBJECT_ID;
+    ecdsa_params.data[1] = oid_data->oid.len;
+    memcpy(ecdsa_params.data + 2, oid_data->oid.data, oid_data->oid.len);
+    ecdsa_params.len = oid_data->oid.len + 2;
+
+    privkey = PK11_GenerateKeyPair(
+        NSSContext::GetSlot(), CKM_EC_KEY_PAIR_GEN, &ecdsa_params, &pubkey,
+        PR_FALSE /*permanent*/, PR_FALSE /*sensitive*/, nullptr);
+
+    ssl_kea_type = ssl_kea_ecdh;
+  } else {
+    LOG(LS_ERROR) << "Key type requested not understood";

[tommi, 2015/06/15 21:02:42]

What about having something like this at the top of the function?:

CHECK(key_type == KT_RSA || key_type == KT_ECDSA);

And then skip this else clause.

[juberti1, 2015/06/16 02:45:04]

Seems kind of drastic. If we added another key type, and then didn't update
nssidentity, would we want to crash in that case?

I agree with CHECK when there is no sensible way to recover, but the caller
needs to handle a NULL return here regardless.

[tommi, 2015/06/16 14:38:01]

If we add a new key type and don't need to handle it here, then it sounds like
it would be by design and a CHECK() isn't the right thing to do.  I was
wondering whether this implementation would always be able to handle all types
and therefore shouldn't expect anything else to come along.

+    return nullptr;
+  }
+
   if (!privkey) {
-    LOG(LS_ERROR) << "Couldn't generate key pair";
-    return NULL;
+    LOG(LS_ERROR) << "Couldn't generate key pair: " << PORT_GetError();
+    return nullptr;
   }
 
-  return new NSSKeyPair(privkey, pubkey);
+  return new NSSKeyPair(privkey, pubkey, ssl_kea_type);
 }
 
 // Just make a copy.
-NSSKeyPair *NSSKeyPair::GetReference() {
-  SECKEYPrivateKey *privkey = SECKEY_CopyPrivateKey(privkey_);
+NSSKeyPair* NSSKeyPair::GetReference() {
+  SECKEYPrivateKey* privkey = SECKEY_CopyPrivateKey(privkey_);
   if (!privkey)
-    return NULL;
+    return nullptr;
 
-  SECKEYPublicKey *pubkey = SECKEY_CopyPublicKey(pubkey_);
+  SECKEYPublicKey* pubkey = SECKEY_CopyPublicKey(pubkey_);
   if (!pubkey) {
     SECKEY_DestroyPrivateKey(privkey);
-    return NULL;
+    return nullptr;
   }
 
-  return new NSSKeyPair(privkey, pubkey);
+  return new NSSKeyPair(privkey, pubkey, ssl_kea_type_);
 }
 
 NSSCertificate::NSSCertificate(CERTCertificate* cert)
     : certificate_(CERT_DupCertificate(cert)) {
-  ASSERT(certificate_ != NULL);
+  ASSERT(certificate_ != nullptr);
 }
 
 static void DeleteCert(SSLCertificate* cert) {
@@ -112,7 +138,7 @@ NSSCertificate::NSSCertificate(CERTCertList* cert_list) {
 
 NSSCertificate::NSSCertificate(CERTCertificate* cert, SSLCertChain* chain)
     : certificate_(CERT_DupCertificate(cert)) {
-  ASSERT(certificate_ != NULL);
+  ASSERT(certificate_ != nullptr);

[tommi, 2015/06/15 21:02:42]

Can we switch over to using DCHECK instead of ASSERT?  (I'm fine with doing that
in another cl)

[juberti1, 2015/06/16 02:45:04]

I think that should be done globally (pthatcher/pbos talked about this IIRC)

   if (chain)
     chain_.reset(chain->Copy());
 }
@@ -122,27 +148,27 @@ NSSCertificate::~NSSCertificate() {
     CERT_DestroyCertificate(certificate_);
 }
 
-NSSCertificate *NSSCertificate::FromPEMString(const std::string &pem_string) {
+NSSCertificate* NSSCertificate::FromPEMString(const std::string& pem_string) {
   std::string der;
   if (!SSLIdentity::PemToDer(kPemTypeCertificate, pem_string, &der))
-    return NULL;
+    return nullptr;
 
   SECItem der_cert;
   der_cert.data = reinterpret_cast<unsigned char *>(const_cast<char *>(
       der.data()));
   der_cert.len = checked_cast<unsigned int>(der.size());
-  CERTCertificate *cert = CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
-      &der_cert, NULL, PR_FALSE, PR_TRUE);
+  CERTCertificate* cert = CERT_NewTempCertificate(
+      CERT_GetDefaultCertDB(), &der_cert, nullptr, PR_FALSE, PR_TRUE);
 
   if (!cert)
-    return NULL;
+    return nullptr;
 
   NSSCertificate* ret = new NSSCertificate(cert);
   CERT_DestroyCertificate(cert);
   return ret;
 }
 
-NSSCertificate *NSSCertificate::GetReference() const {
+NSSCertificate* NSSCertificate::GetReference() const {
   return new NSSCertificate(certificate_, chain_.get());
 }
 
@@ -180,8 +206,8 @@ static bool Certifies(CERTCertificate* parent, CERTCertificate* child) {
 
   // Check that the parent's privkey was actually used to generate the child's
   // signature.
-  SECStatus verified = CERT_VerifySignedDataWithPublicKey(
-      &child->signatureWrap, parent_key, NULL);
+  SECStatus verified = CERT_VerifySignedDataWithPublicKey(&child->signatureWrap,
+                                                          parent_key, nullptr);
   SECKEY_DestroyPublicKey(parent_key);
   return verified == SECSuccess;
 }
@@ -199,7 +225,7 @@ bool NSSCertificate::IsValidChain(const CERTCertList* cert_list) {
 
 bool NSSCertificate::GetDigestLength(const std::string& algorithm,
                                      size_t* length) {
-  const SECHashObject *ho;
+  const SECHashObject* ho;

[tommi, 2015/06/15 21:02:42]

nit: initialize to nullptr

[torbjorng (webrtc), 2015/06/16 14:11:51]

A cleanup in old code, but OK...

 
   if (!GetDigestObject(algorithm, &ho))
     return false;
@@ -261,7 +287,7 @@ bool NSSCertificate::ComputeDigest(const std::string& algorithm,
                                    unsigned char* digest,
                                    size_t size,
                                    size_t* length) const {
-  const SECHashObject *ho;
+  const SECHashObject* ho;
 
   if (!GetDigestObject(algorithm, &ho))
     return false;
@@ -288,7 +314,7 @@ bool NSSCertificate::GetChain(SSLCertChain** chain) const {
   return true;
 }
 
-bool NSSCertificate::Equals(const NSSCertificate *tocompare) const {
+bool NSSCertificate::Equals(const NSSCertificate* tocompare) const {

[tommi, 2015/06/15 21:02:42]

nit: to_compare

[torbjorng (webrtc), 2015/06/16 14:11:51]

I will defer this cleanup in old to separate CL.

[torbjorng (webrtc), 2015/06/16 14:11:51]

I don't understand this comment.

[tommi, 2015/06/16 14:38:01]

the naming rules would be to use an underscore in between the two words as is
actually done elsewhere in this file (e.g. "cert_list").  The variable name
here, doesn't follow that.

   if (!certificate_->derCert.len)
     return false;
   if (!tocompare->certificate_->derCert.len)
@@ -302,10 +328,9 @@ bool NSSCertificate::Equals(const NSSCertificate *tocompare) const {
                 certificate_->derCert.len) == 0;
 }
 
-
-bool NSSCertificate::GetDigestObject(const std::string &algorithm,
-                                     const SECHashObject **hop) {
-  const SECHashObject *ho;
+bool NSSCertificate::GetDigestObject(const std::string& algorithm,
+                                     const SECHashObject** hop) {
+  const SECHashObject* ho;
   HASH_HashType hash_type;
 
   if (algorithm == DIGEST_SHA_1) {
@@ -337,16 +362,17 @@ NSSIdentity::NSSIdentity(NSSKeyPair* keypair, NSSCertificate* cert)
     : keypair_(keypair), certificate_(cert) {
 }
 
-NSSIdentity* NSSIdentity::GenerateInternal(const SSLIdentityParams& params) {
+NSSIdentity* NSSIdentity::GenerateInternal(const SSLIdentityParams& params,
+                                           KeyType key_type) {
   std::string subject_name_string = "CN=" + params.common_name;
-  CERTName *subject_name = CERT_AsciiToName(
-      const_cast<char *>(subject_name_string.c_str()));
-  NSSIdentity *identity = NULL;
-  CERTSubjectPublicKeyInfo *spki = NULL;
-  CERTCertificateRequest *certreq = NULL;
-  CERTValidity *validity = NULL;
-  CERTCertificate *certificate = NULL;
-  NSSKeyPair *keypair = NSSKeyPair::Generate();
+  CERTName* subject_name =
+      CERT_AsciiToName(const_cast<char*>(subject_name_string.c_str()));
+  NSSIdentity* identity = nullptr;
+  CERTSubjectPublicKeyInfo* spki = nullptr;
+  CERTCertificateRequest* certreq = nullptr;
+  CERTValidity* validity = nullptr;
+  CERTCertificate* certificate = nullptr;
+  NSSKeyPair* keypair = NSSKeyPair::Generate(key_type);
   SECItem inner_der;
   SECStatus rv;
   PLArenaPool* arena;
@@ -358,7 +384,7 @@ NSSIdentity* NSSIdentity::GenerateInternal(const SSLIdentityParams& params) {
       now + static_cast<PRTime>(params.not_after) * PR_USEC_PER_SEC;
 
   inner_der.len = 0;
-  inner_der.data = NULL;
+  inner_der.data = nullptr;
 
   if (!keypair) {
     LOG(LS_ERROR) << "Couldn't generate key pair";
@@ -376,7 +402,7 @@ NSSIdentity* NSSIdentity::GenerateInternal(const SSLIdentityParams& params) {
     goto fail;
   }
 
-  certreq = CERT_CreateCertificateRequest(subject_name, spki, NULL);
+  certreq = CERT_CreateCertificateRequest(subject_name, spki, nullptr);
   if (!certreq) {
     LOG(LS_ERROR) << "Couldn't create certificate signing request";
     goto fail;
@@ -405,22 +431,42 @@ NSSIdentity* NSSIdentity::GenerateInternal(const SSLIdentityParams& params) {
 
   arena = certificate->arena;
 
-  rv = SECOID_SetAlgorithmID(arena, &certificate->signature,
-                             SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION, NULL);
-  if (rv != SECSuccess)
+  rv = SECFailure;  // Trigger failure for undefined key_type.
+  if (key_type == KT_RSA) {
+    rv = SECOID_SetAlgorithmID(arena, &certificate->signature,
+                               SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION,

[juberti1, 2015/06/16 02:45:04]

This seems like it could be factored to simply set the OID in a local var, and
then use common code for SetAlgorithmID and DerSignData below.

[torbjorng (webrtc), 2015/06/16 14:11:51]

Done.

+                               nullptr);
+  } else if (key_type == KT_ECDSA) {
+    rv =
+        SECOID_SetAlgorithmID(arena, &certificate->signature,
+                              SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE, nullptr);
+  }
+  if (rv != SECSuccess) {
+    LOG(LS_ERROR) << "Couldn't set hashing algorithm";

[tommi, 2015/06/15 21:02:42]

Are these errors something we should expect to happen or are they "the world
must be ending" type of errors?
If so, I'd rather have [D]CHECKs in place of error handlers+gotos and errors
that the caller can't be expected to handle.

[juberti1, 2015/06/16 02:45:05]

Caller needs to handle failure because keygen can fail (insufficient entropy
pool, bad params)

[torbjorng (webrtc), 2015/06/16 14:11:51]

Some of these "goto fail" could probably not happen, but some of them are for
real (for reasons mentioned by juberti) and need to be handled by caller.  If we
determine a cleanup is needed, let's put that in a separate CL and let's not
forget that we will BoringSSL everything.  :-)

     goto fail;
+  }
 
   // Set version to X509v3.
   *(certificate->version.data) = 2;
   certificate->version.len = 1;
 
   if (!SEC_ASN1EncodeItem(arena, &inner_der, certificate,
-                          SEC_ASN1_GET(CERT_CertificateTemplate)))
+                          SEC_ASN1_GET(CERT_CertificateTemplate))) {
+    LOG(LS_ERROR) << "Couldn't encode certificate";
     goto fail;
+  }
+
+  rv = SECFailure;  // Trigger failure for undefined key_type.
+  if (key_type == KT_RSA) {
+    rv = SEC_DerSignData(arena, &signed_cert, inner_der.data, inner_der.len,
+                         keypair->privkey(),
+                         SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION);
+  } else if (key_type == KT_ECDSA) {
+    rv = SEC_DerSignData(arena, &signed_cert, inner_der.data, inner_der.len,
+                         keypair->privkey(),
+                         SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE);
+  }
 
-  rv = SEC_DerSignData(arena, &signed_cert, inner_der.data, inner_der.len,
-                       keypair->privkey(),
-                       SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION);
   if (rv != SECSuccess) {
     LOG(LS_ERROR) << "Couldn't sign certificate";
     goto fail;
@@ -443,16 +489,18 @@ NSSIdentity* NSSIdentity::GenerateInternal(const SSLIdentityParams& params) {
   return identity;
 }
 
-NSSIdentity* NSSIdentity::Generate(const std::string &common_name) {
+NSSIdentity* NSSIdentity::Generate(const std::string& common_name,
+                                   KeyType key_type) {
   SSLIdentityParams params;
   params.common_name = common_name;
   params.not_before = CERTIFICATE_WINDOW;
   params.not_after = CERTIFICATE_LIFETIME;
-  return GenerateInternal(params);
+  return GenerateInternal(params, key_type);
 }
 
-NSSIdentity* NSSIdentity::GenerateForTest(const SSLIdentityParams& params) {
-  return GenerateInternal(params);
+NSSIdentity* NSSIdentity::GenerateForTest(const SSLIdentityParams& params,
+                                          KeyType key_type) {
+  return GenerateInternal(params, key_type);
 }
 
 SSLIdentity* NSSIdentity::FromPEMStrings(const std::string& private_key,
@@ -460,7 +508,7 @@ SSLIdentity* NSSIdentity::FromPEMStrings(const std::string& private_key,
   std::string private_key_der;
   if (!SSLIdentity::PemToDer(
       kPemTypeRsaPrivateKey, private_key, &private_key_der))
-    return NULL;
+    return nullptr;
 
   SECItem private_key_item;
   private_key_item.data = reinterpret_cast<unsigned char *>(
@@ -470,35 +518,42 @@ SSLIdentity* NSSIdentity::FromPEMStrings(const std::string& private_key,
   const unsigned int key_usage = KU_KEY_ENCIPHERMENT | KU_DATA_ENCIPHERMENT |
       KU_DIGITAL_SIGNATURE;
 
-  SECKEYPrivateKey* privkey = NULL;
-  SECStatus rv =
-      PK11_ImportDERPrivateKeyInfoAndReturnKey(NSSContext::GetSlot(),
-                                               &private_key_item,
-                                               NULL, NULL, PR_FALSE, PR_FALSE,
-                                               key_usage, &privkey, NULL);
+  SECKEYPrivateKey* privkey = nullptr;
+  SECStatus rv = PK11_ImportDERPrivateKeyInfoAndReturnKey(
+      NSSContext::GetSlot(), &private_key_item, nullptr, nullptr, PR_FALSE,
+      PR_FALSE, key_usage, &privkey, nullptr);
   if (rv != SECSuccess) {
     LOG(LS_ERROR) << "Couldn't import private key";
-    return NULL;
+    return nullptr;
   }
 
-  SECKEYPublicKey *pubkey = SECKEY_ConvertToPublicKey(privkey);
+  SECKEYPublicKey* pubkey = SECKEY_ConvertToPublicKey(privkey);
   if (rv != SECSuccess) {
     SECKEY_DestroyPrivateKey(privkey);
     LOG(LS_ERROR) << "Couldn't convert private key to public key";
-    return NULL;
+    return nullptr;
+  }
+
+  SSLKEAType ssl_kea_type;
+  if (private_key.find("-----BEGIN RSA PRIVATE KEY-----") == 0) {

[tommi, 2015/06/15 21:02:42]

nit: could use startswith as suggested in the other review

[torbjorng (webrtc), 2015/06/16 14:11:51]

Done.

+    ssl_kea_type = ssl_kea_rsa;
+  } else {
+    // We might want to check more key types here.  But since we're moving to
+    // Open/BoringSSL, don't bother.  Besides, this will likely be correct for
+    // any future key type, causing a test to do more harm than good.
+    ssl_kea_type = ssl_kea_ecdh;
   }
 
   // Assign to a scoped_ptr so we don't leak on error.
-  scoped_ptr<NSSKeyPair> keypair(new NSSKeyPair(privkey, pubkey));
+  scoped_ptr<NSSKeyPair> keypair(new NSSKeyPair(privkey, pubkey, ssl_kea_type));
 
   scoped_ptr<NSSCertificate> cert(NSSCertificate::FromPEMString(certificate));
   if (!cert) {
     LOG(LS_ERROR) << "Couldn't parse certificate";
-    return NULL;
+    return nullptr;
   }
 
   // TODO(ekr@rtfm.com): Check the public key against the certificate.
-
   return new NSSIdentity(keypair.release(), cert.release());
 }
 
@@ -506,15 +561,15 @@ NSSIdentity::~NSSIdentity() {
   LOG(LS_INFO) << "Destroying NSS identity";
 }
 
-NSSIdentity *NSSIdentity::GetReference() const {
-  NSSKeyPair *keypair = keypair_->GetReference();
+NSSIdentity* NSSIdentity::GetReference() const {
+  NSSKeyPair* keypair = keypair_->GetReference();
   if (!keypair)
-    return NULL;
+    return nullptr;
 
-  NSSCertificate *certificate = certificate_->GetReference();
+  NSSCertificate* certificate = certificate_->GetReference();
   if (!certificate) {
     delete keypair;
-    return NULL;
+    return nullptr;
   }
 
   return new NSSIdentity(keypair, certificate);
@@ -529,4 +584,3 @@ NSSCertificate &NSSIdentity::certificate() const {
 }  // rtc namespace
 
 #endif  // HAVE_NSS_SSL_H
-