Added CreateOriginBound method to x509_certificate.h.

net/base/x509_certificate_nss.cc

Index: net/base/x509_certificate_nss.cc
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index 7224020cb44e528318c204f10f3d6e70ff0dced2..095974afc03a63a3fb89ca9562ffc171e8aee8ca 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -18,6 +18,7 @@
 
 #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"
@@ -31,6 +32,46 @@ 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)
@@ -624,13 +665,11 @@ void X509Certificate::Initialize() {
 }
 
 // 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());
@@ -668,12 +707,13 @@ X509Certificate* X509Certificate::CreateSelfSigned(
   CERT_DestroyValidity(validity);
   CERT_DestroyCertificateRequest(cert_request);
 
-  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.
+  return cert;
+}
 
+// 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,
@@ -722,11 +762,39 @@ X509Certificate* X509Certificate::CreateSelfSigned(
   // 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 {
@@ -770,6 +838,79 @@ void X509Certificate::GetSubjectAltName(
   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 {