Handle Origin Bound Certificate expiration.

Handle Origin Bound Certificate expiration.

BUG=107047
TEST=net_unittests, unit_tests

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

[wtc, 2011-12-14 02:03:38]

Review comments on Patch Set 1:

This seems good, but I'd like to take another look tomorrow
morning when my head is fresh.

It's a shame that we didn't include the expiration time in
the database schema.  I guess we assumed that it's easy to
extract the expiraton time from the certificate.  In hindsight
it makes more sense to store the expiration time.  Sorry
about that.

I'm not sure about changing the 'valid_duration' input
parameter to the 'not_valid_after' input parameter in some
places.  I need to think more about that.

Here are some suggested changes.

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

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:328: base::Time now = base::Time::Now();

For unit tests, we may want to provide a way to override the
function for getting the current time.

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

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.h:98: base::Time not_valid_after,

Please document the |type| and |not_valid_after| input
parameters.

For the GenerateCert() method, it may be better to take
a base::TimeDelta validity_period input parameter instead.

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

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:34: base::Time not_valid_after,

Nit: "expiration_time" or "expiry_time" may be easier to
understand than "not_valid_after".

I know you chose the not_valid_after name after the ASN.1
structure in the certificate:

Validity ::= SEQUENCE {
     notBefore      Time,
     notAfter       Time  }

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:43: base::Time not_valid_after() const {
return not_valid_after_; }

Please document this getter method.

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:70: base::Time* not_valid_after,

Please document the |type| and |not_valid_after| output
parameters.

[wtc, 2011-12-15 03:18:51]

Patch Set 1 LGTM.

I will review sqlite_origin_bound_cert_store_unittest.cc tomorrow.

Some comments and suggested changes below.

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

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:256: "UPDATE
origin_bound_certs SET not_valid_after = ? WHERE origin = ?"));

"expires" is another good (and short) name to replace
"not_valid_after".  But "expiration_time" is probably
the most clear name.

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

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:377: now +
base::TimeDelta::FromDays(kValidityPeriodInDays),

It doesn't seem necessary to pass not_valid_after to the
OriginBoundCertServiceWorker constructor.  More on this
below.

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:419: not_valid_after,

It seems that the GenerateCert method doesn't need to take
not_valid_after as input.  It can generate not_valid_after
internally here.

But this requires the GenerateCert method to return
not_valid_after as output, so that it can be passed to the
HandleResult method.

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

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:472:
scoped_ptr<OriginBoundCertService> service(new OriginBoundCertService(store));

Nit: 'service' can be allocated on the stack:
  OriginBoundCertService service(store);

I know you copied this pattern from the existing tests.

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:488: EXPECT_EQ(OK, error);

Nit: please test 'error' (the return value) before testing 'request_handle'.

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

http://codereview.chromium.org/8890073/diff/1/net/base/x509_util.h#newcode24
net/base/x509_util.h:24: // Web origin, serial number and validity period are
given as

"validity period" in this comment may need to be updated.

It may make sense for these two functions to have both
a not_valid_before and not_valid_after parameter, so that
these two functions don't need to call PR_Now().

[wtc, 2011-12-15 23:34:24]

Review comments on sqlite_origin_bound_cert_store_unittest.cc
in Patch Set 1:

Just some nits.  Looks good!

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

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:164:
TEST_F(SQLiteOriginBoundCertStoreTest, TestUpgrade) {

Should we rename this test TestUpgradeV1?

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:190:
add_smt.BindString(0, "https://www.google.com:443");

Nit: to be more realistic, should we omit the default port 443? in the origin?

NOTE: the changes I suggested for this test also apply to
the next test, TestUpgradeV2.

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:197:
"'https://foo.com',X'CC',X'DD');"

Nit: this cert can use X'AA' and X'BB' now.  This would
require also updating the ASSERT_STREQ statements on lines
225-226 below.

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:229:
certs.clear();

I believe these two lines are equivalent to:
    STLDeleteElements(&certs);

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:293: 

Nit: delete one blank line.

[mattm, 2011-12-20 00:28:38]

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

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:256: "UPDATE
origin_bound_certs SET not_valid_after = ? WHERE origin = ?"));
On 2011/12/15 03:18:51, wtc wrote:
> 
> "expires" is another good (and short) name to replace
> "not_valid_after".  But "expiration_time" is probably
> the most clear name.

Done.

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

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:164:
TEST_F(SQLiteOriginBoundCertStoreTest, TestUpgrade) {
On 2011/12/15 23:34:28, wtc wrote:
> 
> Should we rename this test TestUpgradeV1?

Done.

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:190:
add_smt.BindString(0, "https://www.google.com:443");
On 2011/12/15 23:34:28, wtc wrote:
> 
> Nit: to be more realistic, should we omit the default port 443? in the origin?

That's actually how the origin is stored by the code. 
(SSLClientSocketNSS::OriginBoundClientAuthHandler just does origin = "https://"
+ host_and_port_.ToString();)

I see this doesn't actually match the serialization rules of the weborigin spec,
but since we're already doing it this way we'd need to add more DB upgrade code
if we wanted to fix it.  As long as it's consistent I guess we can just leave
it.

> NOTE: the changes I suggested for this test also apply to
> the next test, TestUpgradeV2.

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:197:
"'https://foo.com',X'CC',X'DD');"
On 2011/12/15 23:34:28, wtc wrote:
> 
> Nit: this cert can use X'AA' and X'BB' now.  This would
> require also updating the ASSERT_STREQ statements on lines
> 225-226 below.

Done.

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:229:
certs.clear();
On 2011/12/15 23:34:28, wtc wrote:
> 
> I believe these two lines are equivalent to:
>     STLDeleteElements(&certs);

Done.

http://codereview.chromium.org/8890073/diff/1/chrome/browser/net/sqlite_origi...
chrome/browser/net/sqlite_origin_bound_cert_store_unittest.cc:293: 
On 2011/12/15 23:34:28, wtc wrote:
> 
> Nit: delete one blank line.

Done.

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

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:377: now +
base::TimeDelta::FromDays(kValidityPeriodInDays),
On 2011/12/15 03:18:51, wtc wrote:
> 
> It doesn't seem necessary to pass not_valid_after to the
> OriginBoundCertServiceWorker constructor.  More on this
> below.

Done.

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.cc:419: not_valid_after,
On 2011/12/15 03:18:51, wtc wrote:
> 
> It seems that the GenerateCert method doesn't need to take
> not_valid_after as input.  It can generate not_valid_after
> internally here.
> 
> But this requires the GenerateCert method to return
> not_valid_after as output, so that it can be passed to the
> HandleResult method.

Done.

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

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service.h:98: base::Time not_valid_after,
On 2011/12/14 02:03:39, wtc wrote:
> 
> Please document the |type| and |not_valid_after| input
> parameters.
> 
> For the GenerateCert() method, it may be better to take
> a base::TimeDelta validity_period input parameter instead.

I documented the expiration_time return value.  I think that like origin, the
type input is pretty self-explanitory.

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

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:472:
scoped_ptr<OriginBoundCertService> service(new OriginBoundCertService(store));
On 2011/12/15 03:18:51, wtc wrote:
> 
> Nit: 'service' can be allocated on the stack:
>   OriginBoundCertService service(store);
> 
> I know you copied this pattern from the existing tests.

Done.

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_serv...
net/base/origin_bound_cert_service_unittest.cc:488: EXPECT_EQ(OK, error);
On 2011/12/15 03:18:51, wtc wrote:
> 
> Nit: please test 'error' (the return value) before testing 'request_handle'.

Done.

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

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:34: base::Time not_valid_after,
On 2011/12/14 02:03:39, wtc wrote:
> 
> Nit: "expiration_time" or "expiry_time" may be easier to
> understand than "not_valid_after".
> 
> I know you chose the not_valid_after name after the ASN.1
> structure in the certificate:
> 
> Validity ::= SEQUENCE {
>      notBefore      Time,
>      notAfter       Time  }

Done.

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:43: base::Time not_valid_after() const {
return not_valid_after_; }
On 2011/12/14 02:03:39, wtc wrote:
> 
> Please document this getter method.

Done.

http://codereview.chromium.org/8890073/diff/1/net/base/origin_bound_cert_stor...
net/base/origin_bound_cert_store.h:70: base::Time* not_valid_after,
On 2011/12/14 02:03:39, wtc wrote:
> 
> Please document the |type| and |not_valid_after| output
> parameters.

Done.

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

http://codereview.chromium.org/8890073/diff/1/net/base/x509_util.h#newcode24
net/base/x509_util.h:24: // Web origin, serial number and validity period are
given as
On 2011/12/15 03:18:51, wtc wrote:
> 
> "validity period" in this comment may need to be updated.

I think with the following change, "validity period" still describes the given
params.

> It may make sense for these two functions to have both
> a not_valid_before and not_valid_after parameter, so that
> these two functions don't need to call PR_Now().

done

[wtc, 2011-12-20 19:46:54]

Patch Set 5 LGTM.

http://codereview.chromium.org/8890073/diff/8007/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/8890073/diff/8007/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.cc:465: cert->swap(der_cert);

Nit: it would be better to set *expiration_time only when
the function returns successfully here.  This requires adding
a local variable to hold the expiration time.

http://codereview.chromium.org/8890073/diff/8007/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_store.h (right):

http://codereview.chromium.org/8890073/diff/8007/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_store.h:66: // |type| is the ClientCertificateType of
the returned key, and

Nit: returned key => returned certificate

I think you can change "Validity notAfter" to simply
"expiration".

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

http://codereview.chromium.org/8890073/diff/8007/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:258: base::TimeDelta valid_duration) {

Is it more convenient for the CreateSelfSignedCert method
to take a valid_duration parameter?

For API consistency, it looks bad for one function to take
valid_duration and two functions to take not_valid_before and
not_valid_after.  Ideally we should fix this.  You can handle
this as a TODO.

http://codereview.chromium.org/8890073/diff/15006/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

http://codereview.chromium.org/8890073/diff/15006/chrome/chrome_tests.gypi#ol...
chrome/chrome_tests.gypi:3931:
'browser/net/sqlite_origin_bound_cert_store_unittest.cc',

This was added by guohui in http://codereview.chromium.org/8318006/.

I guess she meant to add
chrome/browser/net/sqlite_persistent_cookie_store_perftest.cc.
I suggest that we deal with this in a separate CL, if
possible.

[mattm, 2011-12-20 20:38:54]

http://codereview.chromium.org/8890073/diff/8007/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_service.cc (right):

http://codereview.chromium.org/8890073/diff/8007/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_service.cc:465: cert->swap(der_cert);
On 2011/12/20 19:46:55, wtc wrote:
> 
> Nit: it would be better to set *expiration_time only when
> the function returns successfully here.  This requires adding
> a local variable to hold the expiration time.

Done.

http://codereview.chromium.org/8890073/diff/8007/net/base/origin_bound_cert_s...
File net/base/origin_bound_cert_store.h (right):

http://codereview.chromium.org/8890073/diff/8007/net/base/origin_bound_cert_s...
net/base/origin_bound_cert_store.h:66: // |type| is the ClientCertificateType of
the returned key, and
On 2011/12/20 19:46:55, wtc wrote:
> 
> Nit: returned key => returned certificate
> 
> I think you can change "Validity notAfter" to simply
> "expiration".

Done.

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

http://codereview.chromium.org/8890073/diff/8007/net/base/x509_util_nss.cc#ne...
net/base/x509_util_nss.cc:258: base::TimeDelta valid_duration) {
On 2011/12/20 19:46:55, wtc wrote:
> 
> Is it more convenient for the CreateSelfSignedCert method
> to take a valid_duration parameter?
> 
> For API consistency, it looks bad for one function to take
> valid_duration and two functions to take not_valid_before and
> not_valid_after.  Ideally we should fix this.  You can handle
> this as a TODO.

Yeah, I was initially looking at updating this all the way up to the
X509Certificate API, but the mac API that's used for generated the self signed
certs actually takes the param as a delta.  (Could just calculate back from the
absolute times, but it would be weird if you ask for one time and the returned
cert has a slightly different time than you asked for.)  So then I gave up, but
really I could just change the x509_util api and keep the X509Certificate API. 

done.

http://codereview.chromium.org/8890073/diff/15006/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (left):

http://codereview.chromium.org/8890073/diff/15006/chrome/chrome_tests.gypi#ol...
chrome/chrome_tests.gypi:3931:
'browser/net/sqlite_origin_bound_cert_store_unittest.cc',
On 2011/12/20 19:46:55, wtc wrote:
> 
> This was added by guohui in http://codereview.chromium.org/8318006/.
> 
> I guess she meant to add
> chrome/browser/net/sqlite_persistent_cookie_store_perftest.cc.
> I suggest that we deal with this in a separate CL, if
> possible.

Yeah, I was going to file a bug for that.  Haven't gotten around to it yet.

[commit-bot: I haz the power, 2011-12-20 20:39:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/8890073/23003

[commit-bot: I haz the power, 2011-12-20 22:17:35]

Try job failure for 8890073-23003 (retry) on linux_rel for steps "browser_tests,
ui_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...