Added CreateOriginBound method to x509_certificate.h.

net/base/x509_certificate_nss.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/x509_certificate.h"
 
 #include <cert.h>
 #include <cryptohi.h>
 #include <keyhi.h>
 #include <nss.h>
 #include <pk11pub.h>
 #include <prerror.h>
 #include <prtime.h>
 #include <secder.h>
 #include <secerr.h>
 #include <sechash.h>
 #include <sslerr.h>
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/singleton.h"
 #include "base/pickle.h"
 #include "base/time.h"
 #include "crypto/nss_util.h"
 #include "crypto/rsa_private_key.h"
 #include "net/base/cert_status_flags.h"
 #include "net/base/cert_verify_result.h"
 #include "net/base/ev_root_ca_metadata.h"
 #include "net/base/net_errors.h"
 
 namespace net {
 
 namespace {
 
+class ObCertOIDWrapper {
+  private:
+    ObCertOIDWrapper();
+
+  public:
+    static SECOidTag ObCertOIDTag;
+
+    static ObCertOIDWrapper* GetInstance() {
+      return Singleton<ObCertOIDWrapper>::get();
+    }
+
+    friend struct DefaultSingletonTraits<ObCertOIDWrapper>;
+
+    DISALLOW_COPY_AND_ASSIGN(ObCertOIDWrapper);
+};

[wtc, 2011/08/19 18:18:08]

The public: section should precede the private: section.

The constructor, friend declaration, and DISALLOW_COPY_AND_ASSIGN
should be in the private: section.

The ObCertOIDTag member does not need to be static because
this class is a singleton.

It should follow data members' naming convention: ob_cert_oid_tag_
and should be declared in the private: section.

Add a public getter:
  SECOidTag ob_cert_oid_tag() const {
    return ob_cert_oid_tag_;
  }

The constructor should initialize ob_cert_oid_tag_(SEC_OID_UNKNOWN).

[mdietz, 2011/08/22 20:09:00]

Done.

+
+SECOidTag ObCertOIDWrapper::ObCertOIDTag;
+
+ObCertOIDWrapper::ObCertOIDWrapper() {
+  // It's harmless if multiple threads enter this block concurrently.

[wtc, 2011/08/19 18:18:08]

Delete this line.

[mdietz, 2011/08/22 20:09:00]

Done.

+  // 1.3.6.1.4.1.11129.2.1.6
+  // (iso.org.dod.internet.private.enterprises.google.googleSecurity.
+  //  certificateExtensions.originBoundCertificate)
+  static const uint8 kObCertOID[] = {
+    0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79, 0x02, 0x01, 0x06
+  };
+  SECOidData oid_data;
+  memset(&oid_data, 0, sizeof(oid_data));
+  oid_data.oid.data = const_cast<uint8*>(kObCertOID);
+  oid_data.oid.len = sizeof(kObCertOID);
+  oid_data.offset = SEC_OID_UNKNOWN;
+  oid_data.desc = "Origin Bound Certificate";
+  oid_data.mechanism = CKM_INVALID_MECHANISM;
+  oid_data.supportedExtension = SUPPORTED_CERT_EXTENSION;
+  ObCertOIDWrapper::ObCertOIDTag = SECOID_AddEntry(&oid_data);
+  if(ObCertOIDWrapper::ObCertOIDTag == SEC_OID_UNKNOWN) {
+    LOG(ERROR) << "OB_CERT OID tag creation failed";
+  }

[wtc, 2011/08/19 18:18:08]

Remove the curly braces for one-liner if bodies.

[mdietz, 2011/08/22 20:09:00]

Done, also added space to fit "if (" coding style.

On 2011/08/19 18:18:08, wtc wrote:

+}
+
 class ScopedCERTCertificatePolicies {
  public:
   explicit ScopedCERTCertificatePolicies(CERTCertificatePolicies* policies)
       : policies_(policies) {}
 
   ~ScopedCERTCertificatePolicies() {
     if (policies_)
       CERT_DestroyCertificatePoliciesExtension(policies_);
   }
 
@@ skipping 573 matching lines @@
 
   serial_number_ = std::string(
       reinterpret_cast<char*>(cert_handle_->serialNumber.data),
       cert_handle_->serialNumber.len);
   // Remove leading zeros.
   while (serial_number_.size() > 1 && serial_number_[0] == 0)
     serial_number_ = serial_number_.substr(1, serial_number_.size() - 1);
 }
 
 // static
-X509Certificate* X509Certificate::CreateSelfSigned(
+CERTCertificate* CreateCertificate(

[wtc, 2011/08/19 18:18:08]

Remove the
  // static
comments for CreateCertificate() and SignCertificate()
because they are not static methods.

Add the 'static' keywords.  Ideally they should be moved
into the unnamed namespace above.  If we want to minimize
the diffs, we can make them file-scope static instead.

Please add short comments to document these two functions.

[mdietz, 2011/08/22 20:09:00]

Done.

     crypto::RSAPrivateKey* key,
     const std::string& subject,
     uint32 serial_number,
     base::TimeDelta valid_duration) {
-  DCHECK(key);
-
   // Create info about public key.
   CERTSubjectPublicKeyInfo* spki =
       SECKEY_CreateSubjectPublicKeyInfo(key->public_key());
   if (!spki)
     return NULL;
 
   // Create the certificate request.
   CERTName* subject_name =
       CERT_AsciiToName(const_cast<char*>(subject.c_str()));
   CERTCertificateRequest* cert_request =
@@ skipping 17 matching lines @@
   if (!cert) {
     PRErrorCode prerr = PR_GetError();
     LOG(ERROR) << "Failed to create certificate: " << prerr;
   }
 
   // Cleanup for resources used to generate the cert.
   CERT_DestroyName(subject_name);
   CERT_DestroyValidity(validity);
   CERT_DestroyCertificateRequest(cert_request);
 
-  if (!cert)
-    return NULL;
+  return cert;
+}
 
-  // Sign the cert here. The logic of this method references SignCert() in NSS
-  // utility certutil: http://mxr.mozilla.org/security/ident?i=SignCert.
-
+// static
+X509Certificate* SignCertificate(

[wtc, 2011/08/19 18:18:08]

The comment for this function should point out the non-obvious
fact that |cert| is always destroyed, even when the function
returns NULL.

Note: it seems better to change this function to return
a success/failure status, and not create an X509Certificate
object, and not destroy |cert| on failure.  Let the caller
create the X509Certificate object and destroy |cert|.
I'll be happy to do this if you don't have time.  You
can just add a TODO comment.

[mdietz, 2011/08/22 20:09:00]

Added the TODO for now.  If I get a chance, I'll do this before I leave for
Houston.

On 2011/08/19 18:18:08, wtc wrote:

+    CERTCertificate* cert,
+    crypto::RSAPrivateKey* key) {
   // |arena| is used to encode the cert.
   PRArenaPool* arena = cert->arena;
   SECOidTag algo_id = SEC_GetSignatureAlgorithmOidTag(key->key()->keyType,
                                                       SEC_OID_SHA1);
   if (algo_id == SEC_OID_UNKNOWN) {
     CERT_DestroyCertificate(cert);
     return NULL;
   }
 
   SECStatus rv = SECOID_SetAlgorithmID(arena, &cert->signature, algo_id, 0);
@@ skipping 28 matching lines @@
   // Sign the ASN1 encoded cert and save it to |result|.
   rv = SEC_DerSignData(arena, result, der.data, der.len, key->key(), algo_id);
   if (rv != SECSuccess) {
     CERT_DestroyCertificate(cert);
     return NULL;
   }
 
   // Save the signed result to the cert.
   cert->derCert = *result;
 
-  X509Certificate* x509_cert = CreateFromHandle(cert, OSCertHandles());
+  X509Certificate* x509_cert =
+    X509Certificate::CreateFromHandle(cert, X509Certificate::OSCertHandles());
   CERT_DestroyCertificate(cert);
   return x509_cert;
 }
 
+// static
+X509Certificate* X509Certificate::CreateSelfSigned(
+    crypto::RSAPrivateKey* key,
+    const std::string& subject,
+    uint32 serial_number,
+    base::TimeDelta valid_duration) {
+  DCHECK(key);
+
+  CERTCertificate* cert = CreateCertificate(key,
+                                            subject,
+                                            serial_number,
+                                            valid_duration);
+
+  if(!cert)
+    return NULL;
+
+  // Sign the cert here. The logic of this method references SignCert() in NSS
+  // utility certutil: http://mxr.mozilla.org/security/ident?i=SignCert.

[wtc, 2011/08/19 18:18:08]

Move this comment before the SignCertificate() function
definition.

Remove the copy of this comment in CreateOriginBound()
on lines 903-904.

[mdietz, 2011/08/22 20:09:00]

Done.

+
+  X509Certificate* x509_cert = SignCertificate(cert, key);
+
+  if(!x509_cert)
+    return NULL;

[wtc, 2011/08/19 18:18:08]

Remove this, because this is handled by the
  return x509_cert;
statement below.

Make the same change to CreateOriginBound() on lines 908-909
below.

[mdietz, 2011/08/22 20:09:00]

Done.

+
+  return x509_cert;
+}
+
 void X509Certificate::GetSubjectAltName(
     std::vector<std::string>* dns_names,
     std::vector<std::string>* ip_addrs) const {
   if (dns_names)
     dns_names->clear();
   if (ip_addrs)
     ip_addrs->clear();
 
   SECItem alt_name;
   SECStatus rv = CERT_FindCertExtension(cert_handle_,
@@ skipping 23 matching lines @@
           reinterpret_cast<char*>(name->name.other.data),
           name->name.other.len));
     }
     name = CERT_GetNextGeneralName(name);
     if (name == alt_name_list)
       break;
   }
   PORT_FreeArena(arena, PR_FALSE);
 }
 
+// static
+X509Certificate* X509Certificate::CreateOriginBound(

[wtc, 2011/08/19 18:18:08]

Move this function up to follow CreateSelfSigned().

[mdietz, 2011/08/22 20:09:00]

Done.

+    crypto::RSAPrivateKey* key,
+    const std::string& subject,
+    const std::string& origin,
+    uint32 serial_number,
+    base::TimeDelta valid_duration) {
+  DCHECK(key);
+
+  CERTCertificate* cert = CreateCertificate(key,
+                                            subject,

[wtc, 2011/08/19 18:18:08]

Change
    subject
to
    "CN=anonymous.invalid"

This is recommended by Dirk's Internet-Draft.

+                                            serial_number,
+                                            valid_duration);
+
+  if(!cert)
+    return NULL;
+
+  SECStatus ok;
+
+  // Create opaque handle used to add extensions later.
+  void* cert_handle;
+  if ((cert_handle = CERT_StartCertExtensions(cert)) == NULL) {
+    LOG(ERROR) << "Unable to get opaque handle for adding extensions";
+    return NULL;
+  }
+
+  // Create stack allocated SECItem for IA5String encoding

[wtc, 2011/08/19 18:18:08]

Nit: remove "stack allocated".  Add a period.

[mdietz, 2011/08/22 20:09:00]

Done.

+  SECItem origin_string_item = {
+    siAsciiString,
+    (unsigned char*)origin.data(),
+    origin.size()
+  };
+
+  // IA5Encode and arena allocate SECItem
+  SECItem* asn1_origin_string;
+  if ((asn1_origin_string = SEC_ASN1EncodeItem(cert->arena,
+                                               NULL,
+                                               &origin_string_item,
+                                               SEC_ASN1_GET(
+                                                 SEC_IA5StringTemplate
+                                               )))
+     == NULL){

[wtc, 2011/08/19 18:18:08]

I would format this as follows:
  SECItem* asn1_origin_string = SEC_ASN1EncodeItem(
      cert->arena, NULL, &origin_string_item,
      SEC_ASN1_GET(SEC_IA5StringTemplate));
  if (asn1_origin_string == NULL) {

[mdietz, 2011/08/22 20:09:00]

Much cleaner, thanks.

On 2011/08/19 18:18:08, wtc wrote:

+    LOG(ERROR) << "Unable to get ASN1 encoding for origin in ob_cert extension";
+    return NULL;
+  }
+
+  // Add the extension to the opaque handle
+  if((ok = CERT_AddExtension(cert_handle,

[rkn, 2011/08/18 22:59:41]

Can you replace all instances of "if(" with "if (" and then adjust indentation
of subsequent lines accordingly?

[wtc, 2011/08/19 18:18:08]

No need to save the function return value in the 'ok'
variable here and on line 898 below.

Remove the declaration of 'ok' on line 858 above.

[mdietz, 2011/08/22 20:09:00]

Done.

+                             ObCertOIDWrapper::GetInstance()->ObCertOIDTag,
+                             asn1_origin_string,
+                             PR_TRUE, PR_TRUE))
+      != SECSuccess){
+    LOG(ERROR) << "Unable to add origin bound cert extension to opaque handle";
+    return NULL;
+  }
+
+  // Copy extension into x509 cert
+  if((ok = CERT_FinishExtensions(cert_handle)) != SECSuccess){
+    LOG(ERROR) << "Unable to copy extension to X509 cert";
+    return NULL;
+  }
+
+  // Sign the cert here. The logic of this method references SignCert() in NSS
+  // utility certutil: http://mxr.mozilla.org/security/ident?i=SignCert.
+
+  X509Certificate* x509_cert = SignCertificate(cert, key);
+
+  if(!x509_cert)
+    return NULL;
+
+  return x509_cert;
+}
+
 int X509Certificate::VerifyInternal(const std::string& hostname,
                                     int flags,
                                     CertVerifyResult* verify_result) const {
   // Make sure that the hostname matches with the common name of the cert.
   SECStatus status = CERT_VerifyCertName(cert_handle_, hostname.c_str());
   if (status != SECSuccess)
     verify_result->cert_status |= CERT_STATUS_COMMON_NAME_INVALID;
 
   // Make sure that the cert is valid now.
   SECCertTimeValidity validity = CERT_CheckCertValidTimes(
@@ skipping 226 matching lines @@
 
 // static
 bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
                                                 Pickle* pickle) {
   return pickle->WriteData(
       reinterpret_cast<const char*>(cert_handle->derCert.data),
       cert_handle->derCert.len);
 }
 
 }  // namespace net