Use NSS to generate Origin-Bound Certs on Win and Mac.

Use NSS to generate Origin-Bound Certs on Win and Mac.

The platform RSAPrivateKey is used to generate the private key, which is then imported into NSS to generate the certificate.
X509Certificate::CreateOriginBound is moved to x509_util::CreateOriginBoundCert so it can be shared by those platforms, and removes the unnecessary X509Certificate generation step.

BUG=88782
TEST=X509UtilNSSTest.CreateOriginBoundCert & manual testing: try on win or mac, check if generated cert has the OBC extension.

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

[wtc, 2011-10-17 19:09:27]

Patch Set 2 LGTM.

Please mention the new X509UtilNSSTest.CreateOriginBoundCert
unit test in the TEST= field of the CL's commit message.

I believe all of my comments below are style nits.

http://codereview.chromium.org/8296014/diff/2001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_service.cc (left):

http://codereview.chromium.org/8296014/diff/2001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.cc:357: return ERR_GET_CERT_BYTES_FAILED;

I believe this is the only place where ERR_GET_CERT_BYTES_FAILED
is used.  We should either remove this error code, or
improve the comment for it in net_error_list.h, e.g., mention
X509Certificate::GetDEREncoded explicitly.

Please make the net_error_list.h change last, or in a
separate CL because it will cause many files to be
recompiled.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util.h
File net/base/x509_util.h (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util.h#newcode22
net/base/x509_util.h:22: // Create an origin bound certificate containing the
public key in |key|.

Nit: Create => Creates

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util.h#newcode28
net/base/x509_util.h:28: //
http://balfanz.github.com/tls-obc-spec/draft-balfanz-tls-obc-00.html
This URL is now broken.  Although there is a newer version
of the draft on http://balfanz.github.com (I don't have the
URL off hand), I suggest that we use the URL for the official
IETF draft (it has some typos though):
http://tools.ietf.org/html/draft-balfanz-tls-obc-00

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc
File net/base/x509_util_nss.cc (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:6: #include "net/base/x509_util_nss.h"

If we follow the Style Guide strictly, only "net/base/x509_util_nss.h"
should be listed here.  But I understand why you also listed
"net/base/x509_util.h" here.

I'll allow it.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:8: #include "base/debug/leak_annotations.h" //XXX

Can you remove the "//XXX" comment?

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:22: #include <secport.h>

These NSS headers are considered system headers (they are
system headers on Linux) and should be listed on line 7
above.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:76: SECKEYPublicKeyStr* public_key,

Search for "SECKEYPublicKeyStr" and "SECKEYPrivateKeyStr" in
this file and change them to "SECKEYPublicKey" and
"SECKEYPrivateKey".

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:193: return NULL;

Nit: let's rewrite lines 189-193 as follows:

  if (!SignCertificate(cert, key)) {
    CERT_DestroyCertificate(cert);
    return NULL;
  }

  return cert;

Make a similar change to lines 301-312 below.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:211: crypto::ScopedSECKEYPrivateKey
private_key_holder;

Nit: our convention is to name this kind of ScopedFoo variable
scoped_foo.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:227: der_private_key_info.data = const_cast<unsigned
char*>(&key_data.front());

Nit: &key_data.front() => &key_data[0]

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:232: KU_DIGITAL_SIGNATURE;

Nit: align this with KU_KEY_ENCIPHERMENT on the previous line?

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.h
File net/base/x509_util_nss.h (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.h#new...
net/base/x509_util_nss.h:15: struct SECKEYPublicKeyStr;

Please replace these two lines with
typedef struct SECKEYPrivateKeyStr SECKEYPrivateKey;
typedef struct SECKEYPublicKeyStr SECKEYPublicKey;

so that you can use SECKEYPublicKey and SECKEYPrivateKey
on lines 22-23 below.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.h#new...
net/base/x509_util_nss.h:23: SECKEYPrivateKeyStr* key,

Document this function.

Nit: this parameter should be renamed private_key.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss_unitt...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss_unitt...
net/base/x509_util_nss_unittest.cc:14: #include <secoid.h>

List the headers in this order:

#include "net/base/x509_util.h"
#include "net/base/x509_util_nss.h"

#include <cert.h>
#include <secoid.h>

#include "base/memory/scoped_ptr.h"
	
#include "base/memory/ref_counted.h"
#include "crypto/rsa_private_key.h"
#include "net/base/x509_certificate.h"
#include "testing/gtest/include/gtest/gtest.h"

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss_unitt...
net/base/x509_util_nss_unittest.cc:19: const char* data, int length) {

Nit: format this as follows:

CERTCertificate* CreateNSSCertHandleFromBytes(const char* data,
                                              int length) {

|length| should ideally be size_t or unsigned int.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_openssl.cc
File net/base/x509_util_openssl.cc (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_openssl.c...
net/base/x509_util_openssl.cc:73: bool CreateOriginBoundCert(

Nit: list this function first, if we consider x509_util.h
as being before x509_util_openssl.h.

http://codereview.chromium.org/8296014/diff/2001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/8296014/diff/2001/net/net.gyp#newcode1170
net/net.gyp:1170: {  # else !use_openssl: remove the unneeded files

Put this on the previous line.  See line 1152 above for an
example.

[mattm, 2011-10-17 22:54:19]

http://codereview.chromium.org/8296014/diff/2001/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_service.cc (left):

http://codereview.chromium.org/8296014/diff/2001/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.cc:357: return ERR_GET_CERT_BYTES_FAILED;
On 2011/10/17 19:09:27, wtc wrote:
> 
> I believe this is the only place where ERR_GET_CERT_BYTES_FAILED
> is used.  We should either remove this error code, or
> improve the comment for it in net_error_list.h, e.g., mention
> X509Certificate::GetDEREncoded explicitly.
> 
> Please make the net_error_list.h change last, or in a
> separate CL because it will cause many files to be
> recompiled.

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util.h
File net/base/x509_util.h (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util.h#newcode22
net/base/x509_util.h:22: // Create an origin bound certificate containing the
public key in |key|.
On 2011/10/17 19:09:27, wtc wrote:
> 
> Nit: Create => Creates

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util.h#newcode28
net/base/x509_util.h:28: //
http://balfanz.github.com/tls-obc-spec/draft-balfanz-tls-obc-00.html
On 2011/10/17 19:09:27, wtc wrote:
> This URL is now broken.  Although there is a newer version
> of the draft on http://balfanz.github.com (I don't have the
> URL off hand), I suggest that we use the URL for the official
> IETF draft (it has some typos though):
> http://tools.ietf.org/html/draft-balfanz-tls-obc-00

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc
File net/base/x509_util_nss.cc (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:8: #include "base/debug/leak_annotations.h" //XXX
On 2011/10/17 19:09:27, wtc wrote:
> 
> Can you remove the "//XXX" comment?

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:22: #include <secport.h>
On 2011/10/17 19:09:27, wtc wrote:
> 
> These NSS headers are considered system headers (they are
> system headers on Linux) and should be listed on line 7
> above.

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:76: SECKEYPublicKeyStr* public_key,
On 2011/10/17 19:09:27, wtc wrote:
> 
> Search for "SECKEYPublicKeyStr" and "SECKEYPrivateKeyStr" in
> this file and change them to "SECKEYPublicKey" and
> "SECKEYPrivateKey".

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:193: return NULL;
On 2011/10/17 19:09:27, wtc wrote:
> 
> Nit: let's rewrite lines 189-193 as follows:
> 
>   if (!SignCertificate(cert, key)) {
>     CERT_DestroyCertificate(cert);
>     return NULL;
>   }
> 
>   return cert;
> 
> Make a similar change to lines 301-312 below.

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:211: crypto::ScopedSECKEYPrivateKey
private_key_holder;
On 2011/10/17 19:09:27, wtc wrote:
> 
> Nit: our convention is to name this kind of ScopedFoo variable
> scoped_foo.

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:227: der_private_key_info.data = const_cast<unsigned
char*>(&key_data.front());
On 2011/10/17 19:09:27, wtc wrote:
> 
> Nit: &key_data.front() => &key_data[0]

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:232: KU_DIGITAL_SIGNATURE;
On 2011/10/17 19:09:27, wtc wrote:
> 
> Nit: align this with KU_KEY_ENCIPHERMENT on the previous line?

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.h
File net/base/x509_util_nss.h (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.h#new...
net/base/x509_util_nss.h:15: struct SECKEYPublicKeyStr;
On 2011/10/17 19:09:27, wtc wrote:
> 
> Please replace these two lines with
> typedef struct SECKEYPrivateKeyStr SECKEYPrivateKey;
> typedef struct SECKEYPublicKeyStr SECKEYPublicKey;
> 
> so that you can use SECKEYPublicKey and SECKEYPrivateKey
> on lines 22-23 below.

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss.h#new...
net/base/x509_util_nss.h:23: SECKEYPrivateKeyStr* key,
On 2011/10/17 19:09:27, wtc wrote:
> 
> Document this function.
> 
> Nit: this parameter should be renamed private_key.

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss_unitt...
File net/base/x509_util_nss_unittest.cc (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss_unitt...
net/base/x509_util_nss_unittest.cc:14: #include <secoid.h>
On 2011/10/17 19:09:27, wtc wrote:
> 
> List the headers in this order:
> 
> #include "net/base/x509_util.h"
> #include "net/base/x509_util_nss.h"
> 
> #include <cert.h>
> #include <secoid.h>
> 
> #include "base/memory/scoped_ptr.h"
> 	
> #include "base/memory/ref_counted.h"
> #include "crypto/rsa_private_key.h"
> #include "net/base/x509_certificate.h"
> #include "testing/gtest/include/gtest/gtest.h"

Done.

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_nss_unitt...
net/base/x509_util_nss_unittest.cc:19: const char* data, int length) {
On 2011/10/17 19:09:27, wtc wrote:
> 
> Nit: format this as follows:
> 
> CERTCertificate* CreateNSSCertHandleFromBytes(const char* data,
>                                               int length) {
> 
> |length| should ideally be size_t or unsigned int.

Done (actually it all fit on one line).

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_openssl.cc
File net/base/x509_util_openssl.cc (right):

http://codereview.chromium.org/8296014/diff/2001/net/base/x509_util_openssl.c...
net/base/x509_util_openssl.cc:73: bool CreateOriginBoundCert(
On 2011/10/17 19:09:27, wtc wrote:
> 
> Nit: list this function first, if we consider x509_util.h
> as being before x509_util_openssl.h.

Done.

http://codereview.chromium.org/8296014/diff/2001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/8296014/diff/2001/net/net.gyp#newcode1170
net/net.gyp:1170: {  # else !use_openssl: remove the unneeded files
On 2011/10/17 19:09:27, wtc wrote:
> 
> Put this on the previous line.  See line 1152 above for an
> example.

Done.

[wtc, 2011-10-17 23:14:31]

Patch Set 3 LGTM!

http://codereview.chromium.org/8296014/diff/9001/net/base/net_error_list.h
File net/base/net_error_list.h (left):

http://codereview.chromium.org/8296014/diff/9001/net/base/net_error_list.h#ol...
net/base/net_error_list.h:617: NET_ERROR(GET_CERT_BYTES_FAILED, -713)

In general we can't remove an error code without reserving
the code value (to avoid confusion in bug reports, for example).

Since this was only used by code disabled by default, this
is OK.