Support EC certs in OriginBoundCertService and OriginBoundCertStore.

Support EC certs in OriginBoundCertService and OriginBoundCertStore.

OriginBoundCertService::GetOriginBoundCert takes a vector of requested cert
types in order of preferrence and will return or generate one of an acceptable
type.

BUG=88782
TEST=net_unittests, unit_tests

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

[mattm, 2011-11-24 02:57:39]

Doing a proper DB upgrade wasn't that hard, so I just went ahead and did that.

[Ryan Sleevi, 2011-11-25 00:07:02]

drive by with nits

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:214: OriginBoundCertType type)
nit: Doesn't need to be explicit anymore

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service_unittest.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:10: #if !defined(USE_OPENSSL)
nit: Move this to the bottom of the includes (
http://www.chromium.org/developers/coding-style#TOC-Platform-defines )

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:342: #error "not implemented"
Is this necessary, given the #if !defined(USE_OPENSSL) on line 33?

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

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:29: OriginBoundCertType type;
This may put you close to the clang style warning about ctors needing to be
out-of-lined.

It looks for scores >= 10 - class members count for 3, int members for 1.
Probably want to run it against the _clang trybots.

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

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_type...
net/base/origin_bound_cert_type.h:11: // These values may be persisted to disk. 
Do not change or reorder them.
Why not just explicitly specify their values then?

ORIGIN_BOUND_INVALID_CERT_TYPE = 0, etc

Makes it easier if support for a type ever needs to be deprecated.

[wtc, 2011-11-30 23:23:40]

Review comments on Patch Set 1:

I didn't review the unit tests.

The important comments are marked with "IMPORTANT".

http://codereview.chromium.org/8662036/diff/1/chrome/browser/net/sqlite_origi...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

http://codereview.chromium.org/8662036/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:131: "cert_type INTEGER
DEFAULT 1)"))

Nit: it seems better to put cert_type before private_key
and cert.

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:345: job = new
OriginBoundCertServiceJob(worker, requested_types[0]);

IMPORTANT: We should not blindly pick requested_types[0].  We should
pick the first element of requested_types that we support.

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:373: case ORIGIN_BOUND_RSA_CERT:

See the Style Guide for an example of how to format a case
with curly braces:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.h:37: // NSS issue.)

Please include the NSS bug number or provide enough detail
(such as "NSS cannot import unencrypted PrivateKeyInfo for EC
keys").

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.h:54: // PrivateKeyInfo struct, and |cert|
stores a DER-encoded certificate.

Also document the new |type| output parameter.

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

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:43: const std::string& origin,

IMPORTANT: I think we should also pass requested_types as an input argument.  Or
are you storing only one type of cert
for each origin?

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

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_type...
net/base/origin_bound_cert_type.h:12: enum OriginBoundCertType {
I suggest renaming this type SSLClientCertType.  We can
even name these enumerators after the official SSL client
cert types and their values: rsa_sign (1), ecdsa_sign (64).

http://www.iana.org/assignments/tls-parameters/tls-parameters.xml#tls-paramet...

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:1550: case ORIGIN_BOUND_RSA_CERT:

Format this as follows:

  case ORIGIN_BOUND_RSA_CERT: {
    statements;
    ...
    break;
  }

See an example in the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:1573: // SECKEYPrivateKey directly?

IMPORTANT: Yes, we should do something along that line.

The RSA cert case above is actually an abridged version of
crypto::RSAPrivateKey::CreateFromPrivateKeyInfo() in
crypto/rsa_private_key_nss.cc.  We should do something
similar for EC certs.

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:1597: NOTREACHED();

Nit: add a break statement to the default case.

BUT, I think we should return some ERR_xxx error code, rather than falling
through to return OK.

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:2523:
requested_types.push_back(ORIGIN_BOUND_RSA_CERT);

I think we should add both the EC and RSA cert types (in
that order) to requested_types.

Or are we not ready to use EC certs yet?

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_nss.h
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.h:170: const std::vector<OriginBoundCertType>&
requested_types,

Nit: name this parameter cert_types or requested_cert_types.

[wtc, 2011-11-30 23:29:04]

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service_unittest.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:325: // work to extract so we use
some NSS functions here to do it.

I seem to remember agl wrote some code to extract the SPKI.
Please look for it.  It seems to be related to CRL sets or
public key pinning.  You can ask agl if you don't find it.

[Ryan Sleevi, 2011-11-30 23:35:48]

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service_unittest.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:325: // work to extract so we use
some NSS functions here to do it.
On 2011/11/30 23:29:04, wtc wrote:
> 
> I seem to remember agl wrote some code to extract the SPKI.
> Please look for it.  It seems to be related to CRL sets or
> public key pinning.  You can ask agl if you don't find it.

asn1::ExtractSPKIFromDERCert()

Lives in net/base/asn1_util.h

The discussion at the time of review is that it should only be run over 'valid'
certificates (eg: NSS/CryptoAPI/CAPI have otherwise validated the ASN.1
correctness). That should be fine here - as all OBCs should be generated in-app.

[mattm, 2011-12-01 00:06:27]

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:345: job = new
OriginBoundCertServiceJob(worker, requested_types[0]);
On 2011/11/30 23:23:40, wtc wrote:
> 
> IMPORTANT: We should not blindly pick requested_types[0].  We should
> pick the first element of requested_types that we support.

My assumption was that the OriginBoundCertType enum will only contain types we
support, so we don't need to check here unless it's just as a bug catcher.. 
(picking types we support would happen when we translate from the server
supplied certTypes into our vector of enum values)

But if I do the other change you suggested to just pass the official cert types
values then we would need to check in here.

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

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:43: const std::string& origin,
On 2011/11/30 23:23:40, wtc wrote:
> 
> IMPORTANT: I think we should also pass requested_types as an input argument. 
Or
> are you storing only one type of cert
> for each origin?

This is only storing one type of cert per origin.  I figured the case of a
server switching back and forth between only accepted one type of cert, then
another type, would be pretty unlikely, so the extra work of storing multiple
types per origin didn't seem worth it.

[mattm, 2011-12-02 01:55:59]

updated

http://codereview.chromium.org/8662036/diff/1/chrome/browser/net/sqlite_origi...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

http://codereview.chromium.org/8662036/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:131: "cert_type INTEGER
DEFAULT 1)"))
On 2011/11/30 23:23:40, wtc wrote:
> 
> Nit: it seems better to put cert_type before private_key
> and cert.

Could probably do that since the selects and such specify the columns
explicitly, but I think it may be better to have in this order so that a fresh
db and an upgraded db have the same structure.

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:214: OriginBoundCertType type)
On 2011/11/25 00:07:02, Ryan Sleevi wrote:
> nit: Doesn't need to be explicit anymore

Done.

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:373: case ORIGIN_BOUND_RSA_CERT:
On 2011/11/30 23:23:40, wtc wrote:
> 
> See the Style Guide for an example of how to format a case
> with curly braces:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

Done.

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service.h (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.h:37: // NSS issue.)
On 2011/11/30 23:23:40, wtc wrote:
> 
> Please include the NSS bug number or provide enough detail
> (such as "NSS cannot import unencrypted PrivateKeyInfo for EC
> keys").

Done.

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.h:54: // PrivateKeyInfo struct, and |cert|
stores a DER-encoded certificate.
On 2011/11/30 23:23:40, wtc wrote:
> 
> Also document the new |type| output parameter.

Done.

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
File net/base/origin_bound_cert_service_unittest.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:325: // work to extract so we use
some NSS functions here to do it.
On 2011/11/30 23:29:04, wtc wrote:
> 
> I seem to remember agl wrote some code to extract the SPKI.
> Please look for it.  It seems to be related to CRL sets or
> public key pinning.  You can ask agl if you don't find it.

Done.

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:325: // work to extract so we use
some NSS functions here to do it.
On 2011/11/30 23:35:48, Ryan Sleevi wrote:
> On 2011/11/30 23:29:04, wtc wrote:
> > 
> > I seem to remember agl wrote some code to extract the SPKI.
> > Please look for it.  It seems to be related to CRL sets or
> > public key pinning.  You can ask agl if you don't find it.
> 
> asn1::ExtractSPKIFromDERCert()
> 
> Lives in net/base/asn1_util.h
> 
> The discussion at the time of review is that it should only be run over
'valid'
> certificates (eg: NSS/CryptoAPI/CAPI have otherwise validated the ASN.1
> correctness). That should be fine here - as all OBCs should be generated
in-app.

Thanks!

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:342: #error "not implemented"
On 2011/11/25 00:07:02, Ryan Sleevi wrote:
> Is this necessary, given the #if !defined(USE_OPENSSL) on line 33?

Nope, it was more of a documentation thing (like, if you remove the outer ifdef,
it'd still be obvious what you need to do here).  But it's gone now anyway.

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

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:29: OriginBoundCertType type;
On 2011/11/25 00:07:02, Ryan Sleevi wrote:
> This may put you close to the clang style warning about ctors needing to be
> out-of-lined.
> 
> It looks for scores >= 10 - class members count for 3, int members for 1.
> Probably want to run it against the _clang trybots.
 
will do.

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

http://codereview.chromium.org/8662036/diff/1/net/base/origin_bound_cert_type...
net/base/origin_bound_cert_type.h:12: enum OriginBoundCertType {
On 2011/11/30 23:23:40, wtc wrote:
> I suggest renaming this type SSLClientCertType.  We can
> even name these enumerators after the official SSL client
> cert types and their values: rsa_sign (1), ecdsa_sign (64).
> 
>
http://www.iana.org/assignments/tls-parameters/tls-parameters.xml#tls-paramet...

Done.

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:1550: case ORIGIN_BOUND_RSA_CERT:
On 2011/11/30 23:23:40, wtc wrote:
> 
> Format this as follows:
> 
>   case ORIGIN_BOUND_RSA_CERT: {
>     statements;
>     ...
>     break;
>   }
> 
> See an example in the Style Guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

Done.

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:1573: // SECKEYPrivateKey directly?
On 2011/11/30 23:23:40, wtc wrote:
> 
> IMPORTANT: Yes, we should do something along that line.
> 
> The RSA cert case above is actually an abridged version of
> crypto::RSAPrivateKey::CreateFromPrivateKeyInfo() in
> crypto/rsa_private_key_nss.cc.  We should do something
> similar for EC certs.

Done.  (It's still a fair bit of common code, so I put it in a static method
instead of copying it here like the RSA case.)

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:1597: NOTREACHED();
On 2011/11/30 23:23:40, wtc wrote:
> 
> Nit: add a break statement to the default case.
> 
> BUT, I think we should return some ERR_xxx error code, rather than falling
> through to return OK.

Done.

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.cc:2523:
requested_types.push_back(ORIGIN_BOUND_RSA_CERT);
On 2011/11/30 23:23:40, wtc wrote:
> 
> I think we should add both the EC and RSA cert types (in
> that order) to requested_types.
> 
> Or are we not ready to use EC certs yet?

Done.

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_nss.h
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/8662036/diff/1/net/socket/ssl_client_socket_ns...
net/socket/ssl_client_socket_nss.h:170: const std::vector<OriginBoundCertType>&
requested_types,
On 2011/11/30 23:23:40, wtc wrote:
> 
> Nit: name this parameter cert_types or requested_cert_types.

Done.

[wtc, 2011-12-02 22:06:59]

Patch Set 4 LGTM.

I reviewed Patch Set 3 (hence the comments are on Patch Set 3)
and the diffs between 3 and 4.

My main concern is with the way we handle an inflight job
that has an unacceptable cert type for the new request in
origin_bound_cert_service.cc.  Please read my comments in
origin_bound_cert_service.cc carefully.

http://codereview.chromium.org/8662036/diff/1/chrome/browser/net/sqlite_origi...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

http://codereview.chromium.org/8662036/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:131: "cert_type INTEGER
DEFAULT 1)"))
On 2011/12/02 01:55:59, mattm wrote:
> but I think it may be better to have in this order so that a fresh
> db and an upgraded db have the same structure.

I didn't realize an upgraded db has to add a new column
at the end.  This order is fine.

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:132: "cert_type INTEGER
DEFAULT 1)"))

Is it necessary to specify a default value for cert_type?
You should document that the value 1 means rsa_sign.

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:215: "INTEGER DEFAULT 1"))
{

"DEFAULT 1" makes sense here because all the certs in version 1
databases are rsa_sign.  Can we explicitly set the value
of the cert_type column to 1, rather than relying on the
default value?

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
File chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc (right):

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:119:
store_->DeleteOriginBoundCert(*certs[1]);

Nit: if the order of deleting certs doesn't matter, it
looks nicer to delete *certs[0] and then *certs[1].

http://codereview.chromium.org/8662036/diff/15005/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8662036/diff/15005/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:70: // Import the key pair and return in |public_key|
and |key|.

Nit: Import => Imports
     return => returns
     ref => reference

http://codereview.chromium.org/8662036/diff/15005/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:73: static bool ImportFromEncryptedPrivateKeyInfo(

DESIGN: this function probably should be moved to some
NSS util file.  Since it doesn't use the ECPrivateKey type,
it doesn't really belong in this class.

You can move this function later.

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

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:50: const char
OriginBoundCertService::kEPKIPassword[] = "";

Nit: it seems that this should be moved to line 285, to be
with the other OriginBoundCertService stuff.

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:321: return ERR_INVALID_ARGUMENT;

Ideally we should add a new error code for this condition.
You can make this a TODO.

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:351: inflight_joins_++;

BUG: inflight_joins_++; should not be incremented
until we determine the new request can join.

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:357: << " for " << origin;

We should add a comment about this case.  I believe that if
we get here, it means the server is misconfigured, and we
won't be able to do the right thing for that server, unless
we store more than one type of certificate for each origin.

Given that, it is also reasonable to let the inflight job
finish, and abort the current request instead.  This way
we don't need test the new code that aborts an inflight
job.

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:421: scoped_ptr<crypto::ECPrivateKey>
key(crypto::ECPrivateKey::Create());

Please make sure ECPrivateKey has a TODO comment about
adding a "curve" parameter to the Create() method.

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service_unittest.cc (right):

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service_unittest.cc:114:
TEST(OriginBoundCertServiceTest, UnhandledTypes) {

Nit: UnhandledTypes => UnsupportedTypes ?

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service_unittest.cc:344: EXPECT_EQ(1u,
service->inflight_joins());

IMPORTANT: it seems that service->inflight_joins() should be
0 because request 2 cannot join with inflight request 1.

http://codereview.chromium.org/8662036/diff/15005/net/base/ssl_client_cert_ty...
File net/base/ssl_client_cert_type.h (right):

http://codereview.chromium.org/8662036/diff/15005/net/base/ssl_client_cert_ty...
net/base/ssl_client_cert_type.h:16: // 224-255 are Reserved for Private Use, we
pick one to use as "invalid".

Since 0 is also unassigned, I think we can just use it as
"invalid".  It seems unlikely that 0 will be assigned.

But 255 is a fine choice, too.

http://codereview.chromium.org/8662036/diff/15005/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/8662036/diff/15005/net/net.gyp#newcode194
net/net.gyp:194: 'base/origin_bound_cert_type.h',

BUG: the new header file has been renamed.

http://codereview.chromium.org/8662036/diff/15005/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8662036/diff/15005/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1571: ob_private_key_.end());

Nit: this copies the PrivateKeyInfo.  If
crypto::ECPrivateKey::ImportFromEncryptedPrivateKeyInfo()
takes a SECItem or a data/len pair instead of const std::vector<uint8>&,
we can avoid this copying.

[mattm, 2011-12-05 22:19:20]

thanks, updated.

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:132: "cert_type INTEGER
DEFAULT 1)"))
On 2011/12/02 22:06:59, wtc wrote:
> 
> Is it necessary to specify a default value for cert_type?

Ah, changed the DB update code to avoid needing the default.

> You should document that the value 1 means rsa_sign. 

done (at the update location)

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:215: "INTEGER DEFAULT 1"))
{
On 2011/12/02 22:06:59, wtc wrote:
> 
> "DEFAULT 1" makes sense here because all the certs in version 1
> databases are rsa_sign.  Can we explicitly set the value
> of the cert_type column to 1, rather than relying on the
> default value?

Done.

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
File chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc (right):

http://codereview.chromium.org/8662036/diff/15005/chrome/browser/net/sqlite_o...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:119:
store_->DeleteOriginBoundCert(*certs[1]);
On 2011/12/02 22:06:59, wtc wrote:
> 
> Nit: if the order of deleting certs doesn't matter, it
> looks nicer to delete *certs[0] and then *certs[1].

Done.

http://codereview.chromium.org/8662036/diff/15005/crypto/ec_private_key.h
File crypto/ec_private_key.h (right):

http://codereview.chromium.org/8662036/diff/15005/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:70: // Import the key pair and return in |public_key|
and |key|.
On 2011/12/02 22:06:59, wtc wrote:
> 
> Nit: Import => Imports
>      return => returns
>      ref => reference

Done.

http://codereview.chromium.org/8662036/diff/15005/crypto/ec_private_key.h#new...
crypto/ec_private_key.h:73: static bool ImportFromEncryptedPrivateKeyInfo(
On 2011/12/02 22:06:59, wtc wrote:
> 
> DESIGN: this function probably should be moved to some
> NSS util file.  Since it doesn't use the ECPrivateKey type,
> it doesn't really belong in this class.
> 
> You can move this function later.

Added TODO.

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

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:50: const char
OriginBoundCertService::kEPKIPassword[] = "";
On 2011/12/02 22:06:59, wtc wrote:
> 
> Nit: it seems that this should be moved to line 285, to be
> with the other OriginBoundCertService stuff.

Done.

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:321: return ERR_INVALID_ARGUMENT;
On 2011/12/02 22:06:59, wtc wrote:
> 
> Ideally we should add a new error code for this condition.
> You can make this a TODO.

done.  (I made an empty requested_types still return INVALID_ARGUMENT)

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:351: inflight_joins_++;
On 2011/12/02 22:06:59, wtc wrote:
> 
> BUG: inflight_joins_++; should not be incremented
> until we determine the new request can join.

Done.

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:357: << " for " << origin;
On 2011/12/02 22:06:59, wtc wrote:
> 
> We should add a comment about this case.  I believe that if
> we get here, it means the server is misconfigured, and we
> won't be able to do the right thing for that server, unless
> we store more than one type of certificate for each origin.
> 
> Given that, it is also reasonable to let the inflight job
> finish, and abort the current request instead.  This way
> we don't need test the new code that aborts an inflight
> job.

Done.

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:421: scoped_ptr<crypto::ECPrivateKey>
key(crypto::ECPrivateKey::Create());
On 2011/12/02 22:06:59, wtc wrote:
> 
> Please make sure ECPrivateKey has a TODO comment about
> adding a "curve" parameter to the Create() method.

confirmed it does

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_service_unittest.cc (right):

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service_unittest.cc:114:
TEST(OriginBoundCertServiceTest, UnhandledTypes) {
On 2011/12/02 22:06:59, wtc wrote:
> 
> Nit: UnhandledTypes => UnsupportedTypes ?

Done.

http://codereview.chromium.org/8662036/diff/15005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service_unittest.cc:344: EXPECT_EQ(1u,
service->inflight_joins());
On 2011/12/02 22:06:59, wtc wrote:
> 
> IMPORTANT: it seems that service->inflight_joins() should be
> 0 because request 2 cannot join with inflight request 1.

Done.

http://codereview.chromium.org/8662036/diff/15005/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/8662036/diff/15005/net/net.gyp#newcode194
net/net.gyp:194: 'base/origin_bound_cert_type.h',
On 2011/12/02 22:06:59, wtc wrote:
> 
> BUG: the new header file has been renamed.

Done.

http://codereview.chromium.org/8662036/diff/15005/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8662036/diff/15005/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:1571: ob_private_key_.end());
On 2011/12/02 22:06:59, wtc wrote:
> 
> Nit: this copies the PrivateKeyInfo.  If
> crypto::ECPrivateKey::ImportFromEncryptedPrivateKeyInfo()
> takes a SECItem or a data/len pair instead of const std::vector<uint8>&,
> we can avoid this copying.

Done.

[mattm, 2011-12-05 23:34:56]

updated again (fixed win/mac compile & refactored some to fix the clang warning
(removes the struct OriginBoundCertInfo which was basically identical to class
OriginBoundCert))

[wtc, 2011-12-06 00:18:04]

Patch Set 7 LGTM.

The most important comments are the comments in
net/base/net_error_list.h about the two new error codes.

All the other comments are just nits.

http://codereview.chromium.org/8662036/diff/26005/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/8662036/diff/26005/net/base/net_error_list.h#n...
net/base/net_error_list.h:518: NET_ERROR(ORIGIN_BOUND_CERT_TYPE_UNSUPPORTED,
-354)

This error code does not need to be specific to origin-bound
certs because it also applies to regular client certificates.

So, I suggest the following:
  // Server request for client certificate did not contain any types we support.
  NET_ERROR(CLIENT_AUTH_CERT_TYPE_UNSUPPORTED, -354)

Note: we've used "CLIENT_AUTH_CERT" instead of just "CLIENT_CERT"
in other error codes, so I'm trying to be consistent here.

http://codereview.chromium.org/8662036/diff/26005/net/base/net_error_list.h#n...
net/base/net_error_list.h:522:
NET_ERROR(ORIGIN_BOUND_CERT_GENERATION_TYPE_MISMATCH, -355)

The 300-399 range is the HTTP errors.  Since these are not
HTTP errors, we should add them to a different range.  I
suggest adding them to either the 700-799 range (which has
two origin-bound cert errors) or the 100-199 range (which
has several SSL client certificate errors).

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

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:342: std::map<std::string,
OriginBoundCertServiceJob*>::iterator j;

We can keep the initialization of 'job' to NULL. But
I believe j can be changed back to a const_iterator now.

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_store.h (right):

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_store.h:27: // and the cert.

Nit: should we also mention the "type" field?

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_store.h:33: const std::string& privatekey,

Nit: privatekey => private_key

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_store.h:46: std::string cert_;

Should we document these fields?  See the original definition
of the OriginBoundCertInfo struct.

[mattm, 2011-12-06 00:54:01]

thanks, updated again.

http://codereview.chromium.org/8662036/diff/26005/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/8662036/diff/26005/net/base/net_error_list.h#n...
net/base/net_error_list.h:518: NET_ERROR(ORIGIN_BOUND_CERT_TYPE_UNSUPPORTED,
-354)
On 2011/12/06 00:18:05, wtc wrote:
> 
> This error code does not need to be specific to origin-bound
> certs because it also applies to regular client certificates.
> 
> So, I suggest the following:
>   // Server request for client certificate did not contain any types we
support.
>   NET_ERROR(CLIENT_AUTH_CERT_TYPE_UNSUPPORTED, -354)
> 
> Note: we've used "CLIENT_AUTH_CERT" instead of just "CLIENT_CERT"
> in other error codes, so I'm trying to be consistent here.

Done.

http://codereview.chromium.org/8662036/diff/26005/net/base/net_error_list.h#n...
net/base/net_error_list.h:522:
NET_ERROR(ORIGIN_BOUND_CERT_GENERATION_TYPE_MISMATCH, -355)
On 2011/12/06 00:18:05, wtc wrote:
> 
> The 300-399 range is the HTTP errors.  Since these are not
> HTTP errors, we should add them to a different range.  I
> suggest adding them to either the 700-799 range (which has
> two origin-bound cert errors) or the 100-199 range (which
> has several SSL client certificate errors).

oops.  Moved to the 1xx range since that seemed appropriate for errors caused by
the server rather than locally.

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

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_service.cc:342: std::map<std::string,
OriginBoundCertServiceJob*>::iterator j;
On 2011/12/06 00:18:05, wtc wrote:
> 
> We can keep the initialization of 'job' to NULL. But
> I believe j can be changed back to a const_iterator now.

Done.

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
File net/base/origin_bound_cert_store.h (right):

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_store.h:27: // and the cert.
On 2011/12/06 00:18:05, wtc wrote:
> 
> Nit: should we also mention the "type" field?

Done.

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_store.h:33: const std::string& privatekey,
On 2011/12/06 00:18:05, wtc wrote:
> 
> Nit: privatekey => private_key

Done.

http://codereview.chromium.org/8662036/diff/26005/net/base/origin_bound_cert_...
net/base/origin_bound_cert_store.h:46: std::string cert_;
On 2011/12/06 00:18:05, wtc wrote:
> 
> Should we document these fields?  See the original definition
> of the OriginBoundCertInfo struct.

Done.

[wtc, 2011-12-06 00:58:52]

Patch Set 8 LGTM.