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

webrtc/base/nssidentity.cc

 /*
  *  Copyright 2012 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 29 matching lines @@
 // This is to compensate for slightly incorrect system clocks.
 static const int CERTIFICATE_WINDOW = -60*60*24;
 
 NSSKeyPair::~NSSKeyPair() {
   if (privkey_)
     SECKEY_DestroyPrivateKey(privkey_);
   if (pubkey_)
     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.
+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,
-                                 &rsaparams, &pubkey, PR_FALSE /*permanent*/,
-                                 PR_FALSE /*sensitive*/, NULL);
-  if (!privkey) {
-    LOG(LS_ERROR) << "Couldn't generate key pair";
-    return NULL;
+    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;
   }
 
-  return new NSSKeyPair(privkey, pubkey);
+  if (!privkey) {
+    LOG(LS_ERROR) << "Couldn't generate key pair: " << PORT_GetError();
+    return nullptr;
+  }
+
+  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) {
   delete cert;
 }
 
 NSSCertificate::NSSCertificate(CERTCertList* cert_list) {
   // Copy the first cert into certificate_.
   CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
   certificate_ = CERT_DupCertificate(node->cert);
 
   // Put any remaining certificates into the chain.
   node = CERT_LIST_NEXT(node);
   std::vector<SSLCertificate*> certs;
   for (; !CERT_LIST_END(node, cert_list); node = CERT_LIST_NEXT(node)) {
     certs.push_back(new NSSCertificate(node->cert));
   }
 
   if (!certs.empty())
     chain_.reset(new SSLCertChain(certs));
 
   // The SSLCertChain constructor copies its input, so now we have to delete
   // the originals.
   std::for_each(certs.begin(), certs.end(), DeleteCert);
 }
 
 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());
 }
 
 NSSCertificate::~NSSCertificate() {
   if (certificate_)
     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());
 }
 
 std::string NSSCertificate::ToPEMString() const {
   return SSLIdentity::DerToPem(kPemTypeCertificate,
                                certificate_->derCert.data,
                                certificate_->derCert.len);
 }
 
 void NSSCertificate::ToDER(Buffer* der_buffer) const {
@@ skipping 17 matching lines @@
     return false;
 
   // Extract the parent's public key, or fail if the key could not be read
   // (e.g. certificate is corrupted).
   SECKEYPublicKey* parent_key = CERT_ExtractPublicKey(parent);
   if (!parent_key)
     return false;
 
   // 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;
 }
 
 bool NSSCertificate::IsValidChain(const CERTCertList* cert_list) {
   CERTCertListNode* child = CERT_LIST_HEAD(cert_list);
   for (CERTCertListNode* parent = CERT_LIST_NEXT(child);
        !CERT_LIST_END(parent, cert_list);
        child = parent, parent = CERT_LIST_NEXT(parent)) {
     if (!Certifies(parent->cert, child->cert))
       return false;
   }
   return true;
 }
 
 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;
 
   *length = ho->length;
 
   return true;
 }
 
 bool NSSCertificate::GetSignatureDigestAlgorithm(std::string* algorithm) const {
@@ skipping 41 matching lines @@
       algorithm->clear();
       return false;
   }
   return true;
 }
 
 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;
 
   if (size < ho->length)  // Sanity check for fit
     return false;
 
   SECStatus rv = HASH_HashBuf(ho->type, digest,
                               certificate_->derCert.data,
                               certificate_->derCert.len);
   if (rv != SECSuccess)
     return false;
 
   *length = ho->length;
 
   return true;
 }
 
 bool NSSCertificate::GetChain(SSLCertChain** chain) const {
   if (!chain_)
     return false;
 
   *chain = chain_->Copy();
   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)
     return false;
 
   if (certificate_->derCert.len != tocompare->certificate_->derCert.len)
     return false;
 
   return memcmp(certificate_->derCert.data,
                 tocompare->certificate_->derCert.data,
                 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) {
     hash_type = HASH_AlgSHA1;
   // HASH_AlgSHA224 is not supported in the chromium linux build system.
 #if 0
   } else if (algorithm == DIGEST_SHA_224) {
     hash_type = HASH_AlgSHA224;
 #endif
   } else if (algorithm == DIGEST_SHA_256) {
@@ skipping 11 matching lines @@
   ASSERT(ho->length >= 20);  // Can't happen
   *hop = ho;
 
   return true;
 }
 
 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;
   SECItem signed_cert;
   PRTime now = PR_Now();
   PRTime not_before =
       now + static_cast<PRTime>(params.not_before) * PR_USEC_PER_SEC;
   PRTime not_after =
       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";
     goto fail;
   }
 
   if (!subject_name) {
     LOG(LS_ERROR) << "Couldn't convert subject name " << subject_name;
     goto fail;
   }
 
   spki = SECKEY_CreateSubjectPublicKeyInfo(keypair->pubkey());
   if (!spki) {
     LOG(LS_ERROR) << "Couldn't create SPKI";
     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;
   }
 
   validity = CERT_CreateValidity(not_before, not_after);
   if (!validity) {
     LOG(LS_ERROR) << "Couldn't create validity";
     goto fail;
   }
 
   unsigned long serial;
   // Note: This serial in principle could collide, but it's unlikely
   rv = PK11_GenerateRandom(reinterpret_cast<unsigned char *>(&serial),
                            sizeof(serial));
   if (rv != SECSuccess) {
     LOG(LS_ERROR) << "Couldn't generate random serial";
     goto fail;
   }
 
   certificate = CERT_CreateCertificate(serial, subject_name, validity, certreq);
   if (!certificate) {
     LOG(LS_ERROR) << "Couldn't create certificate";
     goto fail;
   }
 
   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 = SEC_DerSignData(arena, &signed_cert, inner_der.data, inner_der.len,
-                       keypair->privkey(),
-                       SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION);
+  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);
+  }
+
   if (rv != SECSuccess) {
     LOG(LS_ERROR) << "Couldn't sign certificate";
     goto fail;
   }
   certificate->derCert = signed_cert;
 
   identity = new NSSIdentity(keypair, new NSSCertificate(certificate));
 
   goto done;
 
  fail:
   delete keypair;
 
  done:
   if (certificate) CERT_DestroyCertificate(certificate);
   if (subject_name) CERT_DestroyName(subject_name);
   if (spki) SECKEY_DestroySubjectPublicKeyInfo(spki);
   if (certreq) CERT_DestroyCertificateRequest(certreq);
   if (validity) CERT_DestroyValidity(validity);
   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,
                                          const std::string& certificate) {
   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 *>(
       const_cast<char *>(private_key_der.c_str()));
   private_key_item.len = checked_cast<unsigned int>(private_key_der.size());
 
   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());
 }
 
 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);
 }
 
 
 NSSCertificate &NSSIdentity::certificate() const {
   return *certificate_;
 }
 
 
 }  // rtc namespace
 
 #endif  // HAVE_NSS_SSL_H
-