Added CreateOriginBound method to x509_certificate.h.

Added CreateOriginBound method to x509_certificate.h.

This static method branches the CreateSelfSigned code to create a self signed certificate that contains an X509v3 extension that indicates the ASCII weborigin that is bound to the generated certificate.

BUG=88782
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98288

[wtc, 2011-07-21 20:01:29]

Michael, thank you for the patch.  In the future please
click the "Publish+Mail Comments" to send an email (the
message can be empty), otherwise the code reviewer won't
get an email notification.

[rkn, 2011-07-22 21:50:22]

On 2011/07/21 20:01:29, wtc wrote:
> Michael, thank you for the patch.  In the future please
> click the "Publish+Mail Comments" to send an email (the
> message can be empty), otherwise the code reviewer won't
> get an email notification.

Perhaps add BUG=88782?

[wtc, 2011-08-04 00:37:53]

mdietz: thank you for writing the CL, and figuring out how
to add an extension to a self-signed cert with NSS.

Most of my comments below are coding style nits.  But there
is one memory leak fix, and three substantive suggested changes:
- avoid duplicate code between CreateSelfSigned and
  CreateOriginBound
- thread-safe lazy one-time initialization
- create an arena rather borrowing the cert's arena
  (in the unit test only)

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:222: // Use this certificate only after the above
risks are acknowledged.
Remove the security warning (lines 215-222).  Replace it with
a reference to the origin-bound certificate Internet-Draft.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.cc
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:759: X509Certificate*
X509Certificate::CreateOriginBound(
The CreateOriginBound function is identical to CreateSelfSigned
except that it has an additional block of code in the middle
(lines 807-871).  We should refactor to share code.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:807: /* Configure X509 library to handle the
OB_CERT X509 Extension OID */
Use the C++ comment delimiter //.

The Style Guide recommends comments use proper punctuation:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation,_S...

So please make sure your comments have proper capitalization,
periods at the end, etc.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:811: if (!ob_cert_oid_tag_defined) {
Chrome has a Singleton and LazyInstance class to do thread-safe
lazy one-time initialization.  Please use that.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:817: {0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79,
0x02, 0x01, 0x06};
I would format this as follows:
  static const uint8 kObCertOID[] = {
    0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79, 0x02, 0x01, 0x06
  };

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:821: oid_data.oid.len = sizeof(kObCertOID);
Add
  oid_data.offset = SEC_OID_UNKNOWN;

This is a no-op because SEC_OID_UNKNOWN is 0, but most of the
code Ive seen does this.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:822: oid_data.desc = "Origin Bound
Certificate";
Add
  oid_data.mechanism = CKM_INVALID_MECHANISM;

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:825: DCHECK_NE(SEC_OID_UNKNOWN,
ob_cert_oid_tag);
Remove this DCHECK_NE because it has proper error handling and
logging code.

This is the recommendation of the Chromium Coding Style page:
http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:837: if((cert_handle =
CERT_StartCertExtensions(cert)) == NULL){
Add a space after "if".

Add a space before the opening curly brace '{'.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:845: origin.size()+1};
The terminating null should not be included.

I would format this as follows:
  SECItem origin_string_item = {
    siAsciiString,
    (unsigned char*)origin.data(),
    origin.size()
  };

Note the use of data() instead of c_str() (which avoids
the conversion to a C string in theory) and the absence
of +1.

Make the same change to x509_certificate_unittest.cc.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:848: SECItem *asn1_origin_string;
Put * next to the type.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:853: SEC_ASN1_GET(SEC_IA5StringTemplate)))
Indent the wrapped parameters by four spaces.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1025: SECItem ob_cert_oid = {siDEROID,
NULL, 0};
Add a space after '{' and before '}'.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1030: PRBool res;
res => result
    or equal

The Style Guide recommends against using uncommon abbreviations
as variable names:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1033: std::string oid_string =
"1.3.6.1.4.1.11129.2.1.6";
Use a C string because this is passed to NSS:
  static const char oid_string[] = "1.3.6.1.4.1.11129.2.1.6";

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1039: SECItem tmp = {siAsciiString,
Use a more descriptive name than "tmp".

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1058: ASSERT_NE(expected,
static_cast<SECItem*>(NULL));
In these ASSERT_xx and EXPECT_xx macros, the expected
value should go first, and the actual value (usually a
variable) should go second.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1062: oid_string.c_str(), NULL);
You should create an arena:
  PLArenaPool* arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
  ...
  PORT_FreeArena(arena, PR_FALSE);

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1072: &actual);
Align these function arguments.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1077: ASSERT_EQ(res, PR_TRUE);
You can use ASSERT_TRUE(res).

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1079: const uint8 private_key_info[] = {
Add 'static'.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1166:
private_key.reset(crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));
I don't know why we're repeating the test.  The only difference
is how the private key is created.  (I know you copied this
from the CreateSelfSigned test.)

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1173: actual.len = 0;
Call
  SECITEM_FreeItem(&actual, PR_FALSE);

before resetting actual.data to NULL to avoid a memory leak.

[mdietz, 2011-08-18 00:02:45]

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.cc
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:759: X509Certificate*
X509Certificate::CreateOriginBound(
On 2011/08/04 00:37:53, wtc wrote:
> The CreateOriginBound function is identical to CreateSelfSigned
> except that it has an additional block of code in the middle
> (lines 807-871).  We should refactor to share code.

The create origin bound code block in the middle can't be moved to anywhere
other than where it is now, and it doesn't make sense to overload
CreateSelfSigned with a parameter that allows it to create origin bound certs. 
So I split out the certificate creation and signing code blocks into new private
functions that both CreateOriginBound and CreateSelfSigned make use of.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:807: /* Configure X509 library to handle the
OB_CERT X509 Extension OID */
On 2011/08/04 00:37:53, wtc wrote:
> Use the C++ comment delimiter //.
> 
> The Style Guide recommends comments use proper punctuation:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation%2C...
> 
> So please make sure your comments have proper capitalization,
> periods at the end, etc.

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:811: if (!ob_cert_oid_tag_defined) {
On 2011/08/04 00:37:53, wtc wrote:
> Chrome has a Singleton and LazyInstance class to do thread-safe
> lazy one-time initialization.  Please use that.

Implemented as a Sinlgeton wrapper around a SECOidTag.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:817: {0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79,
0x02, 0x01, 0x06};
On 2011/08/04 00:37:53, wtc wrote:
> I would format this as follows:
>   static const uint8 kObCertOID[] = {
>     0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79, 0x02, 0x01, 0x06
>   };

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:817: {0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79,
0x02, 0x01, 0x06};
On 2011/08/04 00:37:53, wtc wrote:
> I would format this as follows:
>   static const uint8 kObCertOID[] = {
>     0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79, 0x02, 0x01, 0x06
>   };

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:821: oid_data.oid.len = sizeof(kObCertOID);
On 2011/08/04 00:37:53, wtc wrote:
> Add
>   oid_data.offset = SEC_OID_UNKNOWN;
> 
> This is a no-op because SEC_OID_UNKNOWN is 0, but most of the
> code Ive seen does this.

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:822: oid_data.desc = "Origin Bound
Certificate";
On 2011/08/04 00:37:53, wtc wrote:
> Add
>   oid_data.mechanism = CKM_INVALID_MECHANISM;

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:825: DCHECK_NE(SEC_OID_UNKNOWN,
ob_cert_oid_tag);
On 2011/08/04 00:37:53, wtc wrote:
> Remove this DCHECK_NE because it has proper error handling and
> logging code.
> 
> This is the recommendation of the Chromium Coding Style page:
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:837: if((cert_handle =
CERT_StartCertExtensions(cert)) == NULL){
On 2011/08/04 00:37:53, wtc wrote:
> Add a space after "if".
> 
> Add a space before the opening curly brace '{'.

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:845: origin.size()+1};
On 2011/08/04 00:37:53, wtc wrote:
> The terminating null should not be included.
> 
> I would format this as follows:
>   SECItem origin_string_item = {
>     siAsciiString,
>     (unsigned char*)origin.data(),
>     origin.size()
>   };
> 
> Note the use of data() instead of c_str() (which avoids
> the conversion to a C string in theory) and the absence
> of +1.
> 
> Make the same change to x509_certificate_unittest.cc.

Done.

Note to self: Apply same to unittest.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_nss.c...
net/base/x509_certificate_nss.cc:853: SEC_ASN1_GET(SEC_IA5StringTemplate)))
On 2011/08/04 00:37:53, wtc wrote:
> Indent the wrapped parameters by four spaces.

Done.

This might exceed 80 lines on the last line.  Suggestions?

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1025: SECItem ob_cert_oid = {siDEROID,
NULL, 0};
On 2011/08/04 00:37:53, wtc wrote:
> Add a space after '{' and before '}'.

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1030: PRBool res;
On 2011/08/04 00:37:53, wtc wrote:
> res => result
>     or equal
> 
> The Style Guide recommends against using uncommon abbreviations
> as variable names:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming...

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1033: std::string oid_string =
"1.3.6.1.4.1.11129.2.1.6";
On 2011/08/04 00:37:53, wtc wrote:
> Use a C string because this is passed to NSS:
>   static const char oid_string[] = "1.3.6.1.4.1.11129.2.1.6";

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1039: SECItem tmp = {siAsciiString,
On 2011/08/04 00:37:53, wtc wrote:
> Use a more descriptive name than "tmp".

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1058: ASSERT_NE(expected,
static_cast<SECItem*>(NULL));
On 2011/08/04 00:37:53, wtc wrote:
> In these ASSERT_xx and EXPECT_xx macros, the expected
> value should go first, and the actual value (usually a
> variable) should go second.

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1062: oid_string.c_str(), NULL);
On 2011/08/04 00:37:53, wtc wrote:
> You should create an arena:
>   PLArenaPool* arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
>   ...
>   PORT_FreeArena(arena, PR_FALSE);

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1072: &actual);
On 2011/08/04 00:37:53, wtc wrote:
> Align these function arguments.

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1077: ASSERT_EQ(res, PR_TRUE);
On 2011/08/04 00:37:53, wtc wrote:
> You can use ASSERT_TRUE(res).

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1079: const uint8 private_key_info[] = {
On 2011/08/04 00:37:53, wtc wrote:
> Add 'static'.

Done.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1166:
private_key.reset(crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));
On 2011/08/04 00:37:53, wtc wrote:
> I don't know why we're repeating the test.  The only difference
> is how the private key is created.  (I know you copied this
> from the CreateSelfSigned test.)

Agreed, removed.

http://codereview.chromium.org/7384002/diff/1/net/base/x509_certificate_unitt...
net/base/x509_certificate_unittest.cc:1173: actual.len = 0;
On 2011/08/04 00:37:53, wtc wrote:
> Call
>   SECITEM_FreeItem(&actual, PR_FALSE);
> 
> before resetting actual.data to NULL to avoid a memory leak.

No longer an issue since I've removed this code.

[mdietz, 2011-08-18 21:59:48]

New patch set committed that addresses wtc's comments.

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

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:888: if((ok = CERT_AddExtension(cert_handle,
Can you replace all instances of "if(" with "if (" and then adjust indentation
of subsequent lines accordingly?

[wtc, 2011-08-19 17:38:01]

mdietz: thank you for Patch Set 3.  It seems that you missed
a patch I sent to you by email on Aug. 3.  Please look for
an email from me with the subject
"My changes to Michael's Chromium patch".

The code review tool won't let me paste the patch here
because it's too big, so I reproduced just the description
of the patch in my email here.

I will write my review comments on Patch Set 3 next.

================================================

Attached is an modified version of Patch Set 1.

I only made the following changes to fix build issues.

1. Update to the tip of the Chromium trunk.

2. Add stub implementations of X509Certificate::CreateOriginBound() to
x509_certificate_win.cc, x509_certificate_openssl.cc, and
x509_certificate_mac.cc.  In x509_certificate_mac.cc, add comment to
summarize a failed attempt to implement the function using Apple's
certificate library functions.

3. In x509_certificate_unittest.cc, put the NSS headers inside #if
defined(USE_NSS).

4. In x509_certificate_unittest.cc, move the
X509CertificateTest.CreateOriginBound unit test outside the existing
ifdef block, because this unit test needs a different ifdef (#if
defined(USE_NSS)).  Add a CreateOriginBoundNotImplemented variant of
the unit test to test the stub implementations on non-Linux platforms.

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

Review comments on Patch Set 3:

High-level comments:

1. I like your solution of CreateCertificate() and
SignCertificate() very much!

2. My comments below are all coding style issues, except
for two bugs in the unit test: PORT_FreeArena is called too
early, while data in the arena ('ob_cert_oid') is still
being used, and a missing SECITEM_FreeItem call to free
'actual'.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate.h...
net/base/x509_certificate.h:191: // Subject, web origin, serial number and
validity period are given as
Remove "Subject, " from this line.

Remove lines 195-199.

Remove the |subject| input argument from CreateOriginBound().

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:49: };
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).

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:54: // It's harmless if multiple threads enter
this block concurrently.
Delete this line.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:72: }
Remove the curly braces for one-liner if bodies.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:668: CERTCertificate* CreateCertificate(
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.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:714: X509Certificate* SignCertificate(
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.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:788: // utility certutil:
http://mxr.mozilla.org/security/ident?i=SignCert.
Move this comment before the SignCertificate() function
definition.

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

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:793: return NULL;
Remove this, because this is handled by the
  return x509_cert;
statement below.

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

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:842: X509Certificate*
X509Certificate::CreateOriginBound(
Move this function up to follow CreateSelfSigned().

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:851: subject,
Change
    subject
to
    "CN=anonymous.invalid"

This is recommended by Dirk's Internet-Draft.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:867: // Create stack allocated SECItem for
IA5String encoding
Nit: remove "stack allocated".  Add a period.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:882: == NULL){
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) {

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:888: if((ok = CERT_AddExtension(cert_handle,
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.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1130: SECItem actual = {siBuffer, NULL,
0};
Add spaces after '{' and before '}'.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1133: PRBool result;
This is C++ code, so please declare variables when they're
first used.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1144: origin.size()};
Please format this as follows:
  SECItem extension_object = {
    siAsciiString,
    (unsigned char*)origin.data(),
    origin.size()
  };

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1154: EXPECT_EQ("subject",
cert->subject().GetDisplayName());
Change "subject" to "anonymous.invalid".

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1158: expected =
SEC_ASN1EncodeItem(cert->os_cert_handle()->arena, NULL,
We should call PORT_NewArena() early and use 'arena' instead
of cert->os_cert_handle()->arena here.

Move the PORT_FreeArena() call to the end of this unit test.
We cannot free 'arena' while 'expected' and 'ob_cert_oid'
are still in use.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1184: ASSERT_TRUE(result);
Add
  SECITEM_FreeItem(&actual, PR_FALSE);
otherwise 'actual' is leaked.

[mdietz, 2011-08-19 18:43:22]

Yes, I completely missed that patch when applying the changes to patch set 1. 
I'll get address your new comments and apply those changes and get a new patch
set out today.

--Mike

On 2011/08/19 17:38:01, wtc wrote:
> mdietz: thank you for Patch Set 3.  It seems that you missed
> a patch I sent to you by email on Aug. 3.  Please look for
> an email from me with the subject
> "My changes to Michael's Chromium patch".
> 
> The code review tool won't let me paste the patch here
> because it's too big, so I reproduced just the description
> of the patch in my email here.
> 
> I will write my review comments on Patch Set 3 next.
> 
> ================================================
> 
> Attached is an modified version of Patch Set 1.
> 
> I only made the following changes to fix build issues.
> 
> 1. Update to the tip of the Chromium trunk.
> 
> 2. Add stub implementations of X509Certificate::CreateOriginBound() to
> x509_certificate_win.cc, x509_certificate_openssl.cc, and
> x509_certificate_mac.cc.  In x509_certificate_mac.cc, add comment to
> summarize a failed attempt to implement the function using Apple's
> certificate library functions.
> 
> 3. In x509_certificate_unittest.cc, put the NSS headers inside #if
> defined(USE_NSS).
> 
> 4. In x509_certificate_unittest.cc, move the
> X509CertificateTest.CreateOriginBound unit test outside the existing
> ifdef block, because this unit test needs a different ifdef (#if
> defined(USE_NSS)).  Add a CreateOriginBoundNotImplemented variant of
> the unit test to test the stub implementations on non-Linux platforms.

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

Applied wtc's August 3 patch, addressed patch set 3 comments.  New patch set 4
uploaded.

--Mike

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate.h...
net/base/x509_certificate.h:191: // Subject, web origin, serial number and
validity period are given as
On 2011/08/19 18:18:08, wtc wrote:
> Remove "Subject, " from this line.
> 
> Remove lines 195-199.
> 
> Remove the |subject| input argument from CreateOriginBound().

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:49: };
On 2011/08/19 18:18:08, wtc wrote:
> 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).

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:54: // It's harmless if multiple threads enter
this block concurrently.
On 2011/08/19 18:18:08, wtc wrote:
> Delete this line.

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:72: }
Done, also added space to fit "if (" coding style.

On 2011/08/19 18:18:08, wtc wrote:
> Remove the curly braces for one-liner if bodies.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:668: CERTCertificate* CreateCertificate(
On 2011/08/19 18:18:08, wtc wrote:
> 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.

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:714: X509Certificate* SignCertificate(
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:
> 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.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:788: // utility certutil:
http://mxr.mozilla.org/security/ident?i=SignCert.
On 2011/08/19 18:18:08, wtc wrote:
> Move this comment before the SignCertificate() function
> definition.
> 
> Remove the copy of this comment in CreateOriginBound()
> on lines 903-904.

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:793: return NULL;
On 2011/08/19 18:18:08, wtc wrote:
> Remove this, because this is handled by the
>   return x509_cert;
> statement below.
> 
> Make the same change to CreateOriginBound() on lines 908-909
> below.

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:842: X509Certificate*
X509Certificate::CreateOriginBound(
On 2011/08/19 18:18:08, wtc wrote:
> Move this function up to follow CreateSelfSigned().

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:867: // Create stack allocated SECItem for
IA5String encoding
On 2011/08/19 18:18:08, wtc wrote:
> Nit: remove "stack allocated".  Add a period.

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:882: == NULL){
Much cleaner, thanks.

On 2011/08/19 18:18:08, wtc wrote:
> 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) {

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:888: if((ok = CERT_AddExtension(cert_handle,
On 2011/08/18 22:59:41, rkn wrote:
> Can you replace all instances of "if(" with "if (" and then adjust indentation
> of subsequent lines accordingly?

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1130: SECItem actual = {siBuffer, NULL,
0};
On 2011/08/19 18:18:08, wtc wrote:
> Add spaces after '{' and before '}'.

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1133: PRBool result;
On 2011/08/19 18:18:08, wtc wrote:
> This is C++ code, so please declare variables when they're
> first used.

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1144: origin.size()};
On 2011/08/19 18:18:08, wtc wrote:
> Please format this as follows:
>   SECItem extension_object = {
>     siAsciiString,
>     (unsigned char*)origin.data(),
>     origin.size()
>   };

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1154: EXPECT_EQ("subject",
cert->subject().GetDisplayName());
Just removed subject since it's no longer passed as a parameter to
CreateOriginBound().

On 2011/08/19 18:18:08, wtc wrote:
> Change "subject" to "anonymous.invalid".

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1158: expected =
SEC_ASN1EncodeItem(cert->os_cert_handle()->arena, NULL,
On 2011/08/19 18:18:08, wtc wrote:
> We should call PORT_NewArena() early and use 'arena' instead
> of cert->os_cert_handle()->arena here.
> 
> Move the PORT_FreeArena() call to the end of this unit test.
> We cannot free 'arena' while 'expected' and 'ob_cert_oid'
> are still in use.

Done.

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1184: ASSERT_TRUE(result);
On 2011/08/19 18:18:08, wtc wrote:
> Add
>   SECITEM_FreeItem(&actual, PR_FALSE);
> otherwise 'actual' is leaked.

Done.

[mdietz, 2011-08-22 23:25:26]

Unit tests all pass for this patch set, however once I fixed a build regression
in which the origin bound certificate logic was never being called in
ssl_client_socket_nss.cc I ran into an interesting error.

When trying to generate an origin bound certificate outside of the
CreateOriginBound unit test it throws this error:

[2469:2491:3561021322418:FATAL:thread_restrictions.cc(54)]
LazyInstance/Singleton is not allowed to be used on this thread.  Most likely
it's because this thread is not joinable, so AtExitManager may have deleted the
object on shutdown, leading to a potential shutdown crash.
Backtrace:
	base::debug::StackTrace::StackTrace() [0x7ffff2783a5a]
	logging::LogMessage::~LogMessage() [0x7ffff27a80da]
	base::ThreadRestrictions::AssertSingletonAllowed() [0x7ffff27f7710]
	Singleton<>::get() [0x7ffff2c6853e]
	net::(anonymous namespace)::ObCertOIDWrapper::GetInstance() [0x7ffff2c6506f]
	net::X509Certificate::CreateOriginBound() [0x7ffff2c67210]
	net::OriginBoundCertService::GenerateCert() [0x7ffff2c4ce2a]
	net::OriginBoundCertServiceWorker::Run() [0x7ffff2c4d7ec]
	DispatchToMethod<>() [0x7ffff2c501a7]
	RunnableMethod<>::Run() [0x7ffff2c500ea]
	base::subtle::TaskClosureAdapter::Run() [0x7ffff27f3fcb]
	base::internal::Invoker1<>::DoInvoke() [0x7ffff27b0358]
	base::Callback<>::Run() [0x7ffff224d369]
	base::(anonymous namespace)::WorkerThread::ThreadMain() [0x7ffff4df8b2f]
	base::(anonymous namespace)::ThreadFunc() [0x7ffff27f58af]
	start_thread [0x7fffec6999ca]
	0x7fffe9c6570d

Any suggestions for how to address this?

On 2011/08/22 20:09:00, mdietz wrote:
> Applied wtc's August 3 patch, addressed patch set 3 comments.  New patch set 4
> uploaded.
> 
> --Mike
> 
> http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate.h
> File net/base/x509_certificate.h (right):
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate.h...
> net/base/x509_certificate.h:191: // Subject, web origin, serial number and
> validity period are given as
> On 2011/08/19 18:18:08, wtc wrote:
> > Remove "Subject, " from this line.
> > 
> > Remove lines 195-199.
> > 
> > Remove the |subject| input argument from CreateOriginBound().
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> File net/base/x509_certificate_nss.cc (right):
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:49: };
> On 2011/08/19 18:18:08, wtc wrote:
> > 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).
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:54: // It's harmless if multiple threads
enter
> this block concurrently.
> On 2011/08/19 18:18:08, wtc wrote:
> > Delete this line.
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:72: }
> Done, also added space to fit "if (" coding style.
> 
> On 2011/08/19 18:18:08, wtc wrote:
> > Remove the curly braces for one-liner if bodies.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:668: CERTCertificate* CreateCertificate(
> On 2011/08/19 18:18:08, wtc wrote:
> > 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.
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:714: X509Certificate* SignCertificate(
> 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:
> > 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.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:788: // utility certutil:
> http://mxr.mozilla.org/security/ident?i=SignCert.
> On 2011/08/19 18:18:08, wtc wrote:
> > Move this comment before the SignCertificate() function
> > definition.
> > 
> > Remove the copy of this comment in CreateOriginBound()
> > on lines 903-904.
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:793: return NULL;
> On 2011/08/19 18:18:08, wtc wrote:
> > Remove this, because this is handled by the
> >   return x509_cert;
> > statement below.
> > 
> > Make the same change to CreateOriginBound() on lines 908-909
> > below.
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:842: X509Certificate*
> X509Certificate::CreateOriginBound(
> On 2011/08/19 18:18:08, wtc wrote:
> > Move this function up to follow CreateSelfSigned().
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:867: // Create stack allocated SECItem for
> IA5String encoding
> On 2011/08/19 18:18:08, wtc wrote:
> > Nit: remove "stack allocated".  Add a period.
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:882: == NULL){
> Much cleaner, thanks.
> 
> On 2011/08/19 18:18:08, wtc wrote:
> > 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) {
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:888: if((ok = CERT_AddExtension(cert_handle,
> On 2011/08/18 22:59:41, rkn wrote:
> > Can you replace all instances of "if(" with "if (" and then adjust
indentation
> > of subsequent lines accordingly?
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
> File net/base/x509_certificate_unittest.cc (right):
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:1130: SECItem actual = {siBuffer, NULL,
> 0};
> On 2011/08/19 18:18:08, wtc wrote:
> > Add spaces after '{' and before '}'.
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:1133: PRBool result;
> On 2011/08/19 18:18:08, wtc wrote:
> > This is C++ code, so please declare variables when they're
> > first used.
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:1144: origin.size()};
> On 2011/08/19 18:18:08, wtc wrote:
> > Please format this as follows:
> >   SECItem extension_object = {
> >     siAsciiString,
> >     (unsigned char*)origin.data(),
> >     origin.size()
> >   };
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:1154: EXPECT_EQ("subject",
> cert->subject().GetDisplayName());
> Just removed subject since it's no longer passed as a parameter to
> CreateOriginBound().
> 
> On 2011/08/19 18:18:08, wtc wrote:
> > Change "subject" to "anonymous.invalid".
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:1158: expected =
> SEC_ASN1EncodeItem(cert->os_cert_handle()->arena, NULL,
> On 2011/08/19 18:18:08, wtc wrote:
> > We should call PORT_NewArena() early and use 'arena' instead
> > of cert->os_cert_handle()->arena here.
> > 
> > Move the PORT_FreeArena() call to the end of this unit test.
> > We cannot free 'arena' while 'expected' and 'ob_cert_oid'
> > are still in use.
> 
> Done.
> 
>
http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:1184: ASSERT_TRUE(result);
> On 2011/08/19 18:18:08, wtc wrote:
> > Add
> >   SECITEM_FreeItem(&actual, PR_FALSE);
> > otherwise 'actual' is leaked.
> 
> Done.

[wtc, 2011-08-23 01:32:20]

Review comments on Patch Set 5:

This looks good.  You should be able to commit it after one
more iteration.  Thanks!

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/7384002/diff/11001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1154: EXPECT_EQ("subject",
cert->subject().GetDisplayName());
On 2011/08/22 20:09:00, mdietz wrote:
> Just removed subject since it's no longer passed as a parameter to
> CreateOriginBound().

Right, but it is useful to verify that the subject in
the generated cert matches the subject name recommended
in Dirk's Internet-Draft.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:40: }
I would format this method as follows:

  static ObCertOIDWrapper* GetInstance() {
    return Singleton<ObCertOIDWrapper,
                     LeakySingletonTraits<ObCertOIDWrapper> >::get();
  }

Making this a leaky singleton is fine.  Please add a comment
to explain why (to allow the singleton to be constructed on
a worker thread, which is not joined on process shutdown).

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:54: };
Please fix the indentation in this class.

"public:" and "private:" should be indented by only one space.

The other lines should be indented by two spaces as usual.

The private section should be declared in the order
recommended by the Style Guide:

 private:
  friend struct DefaultSingletonTraits<ObCertOIDWrapper>;

  ObCertOIDWrapper();

  SECOidTag ob_cert_oid_tag_;

  DISALLOW_COPY_AND_ASSIGN(ObCertOIDWrapper);

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:57: : ob_cert_oid_tag_(SEC_OID_UNKNOWN) {
The Style Guide recommends indenting the colon of the
initializer list by four spaces.  Alternatively, you can
put this on the previous line if it fits.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:783: X509Certificate::CreateFromHandle(cert,
X509Certificate::OSCertHandles());
Nit: indent this line by four spaces.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:788: X509Certificate*
X509Certificate::CreateSelfSigned(
Resurrect the "// static" comment for this method because
this is a static method.

Add a "// static" comment to the CreateOriginBound method
on line 808 below.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:800: if(!cert)
Add a space after "if" here and on line 820 below.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:851: != SECSuccess){
Nit: move this to the previous line.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:6: #include <secoid.h>
Remove these two lines.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1129: 
Remove this blank line.

[mdietz, 2011-08-23 20:52:56]

All comments acted upon.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:40: }
On 2011/08/23 01:32:21, wtc wrote:
> I would format this method as follows:
> 
>   static ObCertOIDWrapper* GetInstance() {
>     return Singleton<ObCertOIDWrapper,
>                      LeakySingletonTraits<ObCertOIDWrapper> >::get();
>   }
> 
> Making this a leaky singleton is fine.  Please add a comment
> to explain why (to allow the singleton to be constructed on
> a worker thread, which is not joined on process shutdown).

Done.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:54: };
On 2011/08/23 01:32:21, wtc wrote:
> Please fix the indentation in this class.
> 
> "public:" and "private:" should be indented by only one space.
> 
> The other lines should be indented by two spaces as usual.
> 
> The private section should be declared in the order
> recommended by the Style Guide:
> 
>  private:
>   friend struct DefaultSingletonTraits<ObCertOIDWrapper>;
> 
>   ObCertOIDWrapper();
> 
>   SECOidTag ob_cert_oid_tag_;
> 
>   DISALLOW_COPY_AND_ASSIGN(ObCertOIDWrapper);
> 
> See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:57: : ob_cert_oid_tag_(SEC_OID_UNKNOWN) {
On 2011/08/23 01:32:21, wtc wrote:
> The Style Guide recommends indenting the colon of the
> initializer list by four spaces.  Alternatively, you can
> put this on the previous line if it fits.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

Done.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:783: X509Certificate::CreateFromHandle(cert,
X509Certificate::OSCertHandles());
On 2011/08/23 01:32:21, wtc wrote:
> Nit: indent this line by four spaces.

Done.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:788: X509Certificate*
X509Certificate::CreateSelfSigned(
On 2011/08/23 01:32:21, wtc wrote:
> Resurrect the "// static" comment for this method because
> this is a static method.
> 
> Add a "// static" comment to the CreateOriginBound method
> on line 808 below.

Done.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:800: if(!cert)
On 2011/08/23 01:32:21, wtc wrote:
> Add a space after "if" here and on line 820 below.

Done.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:851: != SECSuccess){
On 2011/08/23 01:32:21, wtc wrote:
> Nit: move this to the previous line.

Done.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:6: #include <secoid.h>
On 2011/08/23 01:32:21, wtc wrote:
> Remove these two lines.

Done.

http://codereview.chromium.org/7384002/diff/22001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1129: 
On 2011/08/23 01:32:21, wtc wrote:
> Remove this blank line.

Done.

[wtc, 2011-08-23 21:51:07]

LGTM.

Please fix the nits below, and upload a new patch set.

I assume you have tested the CL on try servers.  For
trivial fixes it is not necessary to redo the try servers/

Then, check the "Commit" checkbox on the CL's code review
page (at the bottom of the file list) to submit the CL to
the commit queue.

http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:42: LeakySingletonTraits<ObCertOIDWrapper>
>::get();
Please format these two lines as follows:

      return Singleton<ObCertOIDWrapper,
                       LeakySingletonTraits<ObCertOIDWrapper> >::get();

http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:57: };
The methods and members of this class should be indented by two spaces relative
to "class", or one space relative to "public:" and "private:".

http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1144: // Origin Bound Cert OID
Nit: the Style Guide recommends proper punctuation in comments.
Please make sure all the sentences in the comments end with
a period (.).

[mdietz, 2011-08-23 22:12:04]

I don't think I was ever granted committer privs on the SVN repo.  So I can't
authenticate to the trybot in order to test against it.

--Mike

On 2011/08/23 21:51:07, wtc wrote:
> LGTM.
> 
> Please fix the nits below, and upload a new patch set.
> 
> I assume you have tested the CL on try servers.  For
> trivial fixes it is not necessary to redo the try servers/
> 
> Then, check the "Commit" checkbox on the CL's code review
> page (at the bottom of the file list) to submit the CL to
> the commit queue.
> 
>
http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_n...
> File net/base/x509_certificate_nss.cc (right):
> 
>
http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:42: LeakySingletonTraits<ObCertOIDWrapper>
> >::get();
> Please format these two lines as follows:
> 
>       return Singleton<ObCertOIDWrapper,
>                        LeakySingletonTraits<ObCertOIDWrapper> >::get();
> 
>
http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_n...
> net/base/x509_certificate_nss.cc:57: };
> The methods and members of this class should be indented by two spaces
relative
> to "class", or one space relative to "public:" and "private:".
> 
>
http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_u...
> File net/base/x509_certificate_unittest.cc (right):
> 
>
http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:1144: // Origin Bound Cert OID
> Nit: the Style Guide recommends proper punctuation in comments.
> Please make sure all the sentences in the comments end with
> a period (.).

[wtc, 2011-08-23 22:47:26]

I created a CL based on your Patch Set 6 and am testing it on the
try servers.

[wtc, 2011-08-24 01:39:42]

mdietz: Try server testing shows two changes are required.

http://codereview.chromium.org/7384002/diff/24001/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/7384002/diff/24001/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:330:
base::TimeDelta::FromDays(kValidityPeriodInDays));
This fails on Windows, etc. because X509Certificate::CreateOriginBound()
is not implemented.

For now, we need to use ifdef:

#if defined(USE_NSS)
  scoped_refptr<X509Certificate> x509_cert = X509Certificate::CreateOriginBound(
      key.get(),
      origin,
      serial_number,
      base::TimeDelta::FromDays(kValidityPeriodInDays));
#else
  scoped_refptr<X509Certificate> x509_cert = X509Certificate::CreateSelfSigned(
      key.get(),
      "CN=anonymous.invalid",
      serial_number,
      base::TimeDelta::FromDays(kValidityPeriodInDays));
#endif

Alternatively, the stub implementations of CreateOriginBound
can call CreateSelfSigned("CN=anonymous.invalid") instead of
returning NULL.

http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_u...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_u...
net/base/x509_certificate_unittest.cc:1164: EXPECT_EQ("subject",
cert->subject().GetDisplayName());
IMPORTANT: change "subject" to "anonymous.invalid".

[mdietz, 2011-08-24 20:10:29]

I fixed these issues.  Can you rerun this on the try bot for me?

On 2011/08/24 01:39:42, wtc wrote:
> mdietz: Try server testing shows two changes are required.
> 
>
http://codereview.chromium.org/7384002/diff/24001/net/base/origin_bound_cert_...
> File net/base/origin_bound_cert_service.cc (right):
> 
>
http://codereview.chromium.org/7384002/diff/24001/net/base/origin_bound_cert_...
> net/base/origin_bound_cert_service.cc:330:
> base::TimeDelta::FromDays(kValidityPeriodInDays));
> This fails on Windows, etc. because X509Certificate::CreateOriginBound()
> is not implemented.
> 
> For now, we need to use ifdef:
> 
> #if defined(USE_NSS)
>   scoped_refptr<X509Certificate> x509_cert =
X509Certificate::CreateOriginBound(
>       key.get(),
>       origin,
>       serial_number,
>       base::TimeDelta::FromDays(kValidityPeriodInDays));
> #else
>   scoped_refptr<X509Certificate> x509_cert =
X509Certificate::CreateSelfSigned(
>       key.get(),
>       "CN=anonymous.invalid",
>       serial_number,
>       base::TimeDelta::FromDays(kValidityPeriodInDays));
> #endif
> 
> Alternatively, the stub implementations of CreateOriginBound
> can call CreateSelfSigned("CN=anonymous.invalid") instead of
> returning NULL.
> 
>
http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_u...
> File net/base/x509_certificate_unittest.cc (right):
> 
>
http://codereview.chromium.org/7384002/diff/24001/net/base/x509_certificate_u...
> net/base/x509_certificate_unittest.cc:1164: EXPECT_EQ("subject",
> cert->subject().GetDisplayName());
> IMPORTANT: change "subject" to "anonymous.invalid".

[wtc, 2011-08-24 21:35:14]

LGTM.  I am testing Patch Set 7 on the try servers for you.

http://codereview.chromium.org/7384002/diff/31001/net/base/x509_certificate_n...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/7384002/diff/31001/net/base/x509_certificate_n...
net/base/x509_certificate_nss.cc:42: LeakySingletonTraits<ObCertOIDWrapper>
>::get();
Please format these two lines as follows:
    return Singleton<ObCertOIDWrapper,
                     LeakySingletonTraits<ObCertOIDWrapper> >::get();

[wtc, 2011-08-25 01:44:16]

Most of the try server results are back.  They look good.
You can click the "Commit" checkbox.  I would wait until
tomorrow morning unless you plan to work late tonight.

[mdietz, 2011-08-25 17:58:58]

Pushed to the commit queue.

On 2011/08/25 01:44:16, wtc wrote:
> Most of the try server results are back.  They look good.
> You can click the "Commit" checkbox.  I would wait until
> tomorrow morning unless you plan to work late tonight.

[commit-bot: I haz the power, 2011-08-25 20:29:25]

Change committed as 98288

[mdietz, 2011-08-26 01:07:28]

Looks like this patch set was reverted.  I took a look at the trybot error logs
and it looks like the errors shown there had nothing to do with this patch set. 
Is there a process for re-committing?

On 2011/08/25 20:29:25, I haz the power (commit-bot) wrote:
> Change committed as 98288

[wtc, 2011-08-26 02:03:13]

On 2011/08/26 01:07:28, mdietz wrote:
> Looks like this patch set was reverted.  I took a look at the trybot error
logs
> and it looks like the errors shown there had nothing to do with this patch
set.

I agree.  This CL should only cause compilation errors or
failure of either rkn's origin-bound cert tests or the new
unit test you added.  If the errors occurred elsewhere, they
are not caused by this CL.

> Is there a process for re-committing?

I am not sure if you can simply click the "Commit" checkbox
in this CL again since this CL has been marked "Closed".

Just create a new CL with the same contents and commit it.
Since you don't have an "owner" status, you'll need to ask
me to review the new CL again.