Cast device revocation checking.

Cast device revocation checking.

Cast device certificates may be revoked through two ways:
1. The hash of the public key.
2. A serial number range for an issuer identified by the hash
   of its public key.

A customized proto is used as the medium for this information.

This change contains the implementation for verifying the custom CRL
and verifying a certificate's revocation status based on that CRL.

BUG=618463
Committed: https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
Committed: https://crrev.com/ed1a0da4ea59a8f42d133818eed5e34d45fa163b
Cr-Original-Commit-Position: refs/heads/master@{#407492}
Cr-Commit-Position: refs/heads/master@{#407704}

[ryanchung, 2016-06-09 16:46:40]

ryanchung@chromium.org changed reviewers:
+ sheretov@chromium.org

[sheretov, 2016-06-13 19:18:35]

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:84: // Parse the public key and
calculate its hash.
This is wrong.  We should hash the SPKI directly - not drill into it to decode
the bit string and hash the contents.  Is the Java implementation doing the same
thing?

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:133: // Verifies the CRL is signed by a
trusted CRL authority at the time the CRL was
The CRL, including all the certs in the path should be valid at the time of
revocation check.  Please pass in a parameter with the current time here.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:238: bool
CastCRLImpl::VerifyDeviceCertRevocation(
I would call this checkRevocation

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:253: if (utc_time < validity_start ||
utc_time > validity_end) {
nit: utc_time >= validity_end

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:280:
FindCastTrustAnchorByName(cert_info.issuer_name);
This seems awkward here. Can we pass in the root's SPKI hash as a parameter to
the entire function instead of doing a name lookup?

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:294: if
(revoked_hashes_.find(issuer_cert_info.key_hash) !=
Why are we checking revocation on the trust anchor?

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:325: bool supported_supported_crl_found
= false;
supported_supported?

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:334: if (tbs_crl.version() == 0) {
Please create a constant for the CRL version, so we can track its usage
throughout the code.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:335: supported_crl = crl;
Is this assignment making a copy?  Why have the tbs_crl and supported_crl local
variables, when you could just do the verification and return from within the
loop?

[ryanchung, 2016-06-21 21:45:06]

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:84: // Parse the public key and
calculate its hash.
On 2016/06/13 19:18:35, sheretov wrote:
> This is wrong.  We should hash the SPKI directly - not drill into it to decode
> the bit string and hash the contents.  Is the Java implementation doing the
same
> thing?

OK, Done.
Originally, the hash of the subject public key was used instead of the whole
spki because of following OCSP's way of calculating the issuer's hash.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:133: // Verifies the CRL is signed by a
trusted CRL authority at the time the CRL was
On 2016/06/13 19:18:35, sheretov wrote:
> The CRL, including all the certs in the path should be valid at the time of
> revocation check.  Please pass in a parameter with the current time here.

Added the current time here.
As discussed offline, it is inefficient to re-verify the whole chain of the CRL
issuer every time a cert is checked for revocation. The internal representation
of the CRL will store the earliest of the CRL's expiration and the chain's
expiration as the CRL's expiration.
When a cert is checked for revocation, only the internal representation's
validity period has to be checked.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:238: bool
CastCRLImpl::VerifyDeviceCertRevocation(
On 2016/06/13 19:18:35, sheretov wrote:
> I would call this checkRevocation

Done.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:253: if (utc_time < validity_start ||
utc_time > validity_end) {
On 2016/06/13 19:18:35, sheretov wrote:
> nit: utc_time >= validity_end

Done.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:280:
FindCastTrustAnchorByName(cert_info.issuer_name);
On 2016/06/13 19:18:35, sheretov wrote:
> This seems awkward here. Can we pass in the root's SPKI hash as a parameter to
> the entire function instead of doing a name lookup?

I've updated this function to take in the verified chain. Removed the need to
lookup the anchor.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:294: if
(revoked_hashes_.find(issuer_cert_info.key_hash) !=
On 2016/06/13 19:18:35, sheretov wrote:
> Why are we checking revocation on the trust anchor?

Removed.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:325: bool supported_supported_crl_found
= false;
On 2016/06/13 19:18:35, sheretov wrote:
> supported_supported?

Removed this due to comment below.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:334: if (tbs_crl.version() == 0) {
On 2016/06/13 19:18:35, sheretov wrote:
> Please create a constant for the CRL version, so we can track its usage
> throughout the code.

Done.

https://codereview.chromium.org/2050983002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:335: supported_crl = crl;
On 2016/06/13 19:18:35, sheretov wrote:
> Is this assignment making a copy?  Why have the tbs_crl and supported_crl
local
> variables, when you could just do the verification and return from within the
> loop?

Changed this to return in the loop.

[sheretov, 2016-06-24 20:24:31]

Some comments.  More to come when there's an updated test runner.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:161: return false;
Any reason this failure is not logged like the signature verification failure
above?  Same for the time validity failure below.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:211: const base::Time
overall_validity_end) {
Any reason why this is not a reference?

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:251: // Check the validity of the CRl at
the specified time.
nit: s/CRl/CRL/

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:275: DCHECK(!issuer_key_hash.empty());
Is this DCHECK needed given the condition in the if statement just above?

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:282: return false;
This effectively (once there is at least one revoked SN range) revokes all certs
with a serial number that does not fit into 64 bits.  Seems like fringe
scenario, but one that we should probably agree on a consistent behavior for. 
My inclination is to be lenient here and skip to the next cert in the chain in
this case (that's what the current Java implementation does). The resulting
behavior would be: if the Cast ecosystem ever has certs with serial numbers that
don't fit within 64 bits, such certs cannot be revoked by serial number range.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:311: switch (tbs_crl.version()) {
A switch statement seems less readable than a simple if statement here, at least
for now.  If/when there is another version number to handle, this code can be
updated to use a switch statement.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.h:53: std::unique_ptr<CastCRL>
ParseCRL(const std::string& crl_proto,
How about "ParseAndVerifyCrl"?

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.h:63: bool AddCRLTrustAnchorForTest(const
uint8_t* data,
I have a couple suggestions, both aimed at making it less likely that a test
trust anchor will end up in production code:

1. Instead of adding test trust anchors to production trust anchors for testing,
replace production trust anchors with test trust anchors. So, a function like
this would become SetCRLTrustAnchorForTest().

2. Unless we have scenarios of production certs being used with test CRLs, I
would go even further than (1) and create a single
cast_certificate.SetTrustAnchorsForTest(cert_trust_anchor, crl_trust_anchor)
function that sets both anchors.  Given that the test trust anchors are well
known, may not even need to take parameters, but rather hard-code the test test
trust anchors into that function.

[ryanchung, 2016-06-29 22:09:48]

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:161: return false;
On 2016/06/24 20:24:30, sheretov wrote:
> Any reason this failure is not logged like the signature verification failure
> above?  Same for the time validity failure below.

Done.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:211: const base::Time
overall_validity_end) {
On 2016/06/24 20:24:31, sheretov wrote:
> Any reason why this is not a reference?

Done.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:251: // Check the validity of the CRl at
the specified time.
On 2016/06/24 20:24:31, sheretov wrote:
> nit: s/CRl/CRL/

Done.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:275: DCHECK(!issuer_key_hash.empty());
On 2016/06/24 20:24:30, sheretov wrote:
> Is this DCHECK needed given the condition in the if statement just above?

Done.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:282: return false;
On 2016/06/24 20:24:31, sheretov wrote:
> This effectively (once there is at least one revoked SN range) revokes all
certs
> with a serial number that does not fit into 64 bits.  Seems like fringe
> scenario, but one that we should probably agree on a consistent behavior for. 
> My inclination is to be lenient here and skip to the next cert in the chain in
> this case (that's what the current Java implementation does). The resulting
> behavior would be: if the Cast ecosystem ever has certs with serial numbers
that
> don't fit within 64 bits, such certs cannot be revoked by serial number range.

Done.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.cc:311: switch (tbs_crl.version()) {
On 2016/06/24 20:24:30, sheretov wrote:
> A switch statement seems less readable than a simple if statement here, at
least
> for now.  If/when there is another version number to handle, this code can be
> updated to use a switch statement.

Done.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.h:53: std::unique_ptr<CastCRL>
ParseCRL(const std::string& crl_proto,
On 2016/06/24 20:24:31, sheretov wrote:
> How about "ParseAndVerifyCrl"?

Done.

https://codereview.chromium.org/2050983002/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.h:63: bool AddCRLTrustAnchorForTest(const
uint8_t* data,
On 2016/06/24 20:24:31, sheretov wrote:
> I have a couple suggestions, both aimed at making it less likely that a test
> trust anchor will end up in production code:
> 
> 1. Instead of adding test trust anchors to production trust anchors for
testing,
> replace production trust anchors with test trust anchors. So, a function like
> this would become SetCRLTrustAnchorForTest().
> 
> 2. Unless we have scenarios of production certs being used with test CRLs, I
> would go even further than (1) and create a single
> cast_certificate.SetTrustAnchorsForTest(cert_trust_anchor, crl_trust_anchor)
> function that sets both anchors.  Given that the test trust anchors are well
> known, may not even need to take parameters, but rather hard-code the test
test
> trust anchors into that function.

Done. Keeping them separate for now because we plan on adding production certs
to CRL tests.

[ryanchung, 2016-07-07 16:32:25]

ryanchung@chromium.org changed reviewers:
+ eroman@chromium.org

[ryanchung, 2016-07-07 16:32:26]

PTAL
Thanks.

[sheretov, 2016-07-08 18:07:08]

Next round of comments.  You might want to rebase to take mattm@'s last CL into
account:

https://codereview.chromium.org/1923433002/

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.h (right):

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:96: CRLOptions crl_options)
WARN_UNUSED_RESULT;
Why is structure being passed by value?

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:126: bool VerifyCRL(const Crl& crl,
Documenting the arguments might be useful for readability here.  Especially the
overall_validity_end argument.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:131: net::der::GeneralizedTime
signing_time_generalized =
I would call this "verification_time_generalized".

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:146: signature = crl.signature();
I would combine this with the previous line:
std::string signature = crl.signature();

Same for signature_value_bit_string below.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:151: // Verify the signature
style consistency nit: "// Parse the signature." comment above ends with a
period, and this comment does not.  Same with "//Verify XXX" comments below.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:187: VLOG(2) << "CRL - Expired.";
"CRL not time-valid" or something to that effect, since we are checking for
validity_start as well.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:198: return false;
This may warrant a log statement.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:199: cert_time +=
base::TimeDelta::FromMilliseconds(500);
Consider explaining the rounding logic and why it's needed in a comment.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:229: // Hash of all revoked public key.
s/Hash/Hashes/
s/key/keys/

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:233: std::unordered_map<std::string,
std::unordered_map<uint64_t, uint64_t>>
std::unordered_map<uint64_t, uint64_t> seems like a poor choice for a value type
here.  If there are multiple ranges that start with the same serial number under
the same issuer, they must contend for the same entry in the map.  This could be
alleviated with some kind of incremental range merge, so that {10, 12}, {10,
30}, {10, 20} => {10, 30}, but I don't see it below.  So, right now this
admittedly contrived example will result in an incorrect effective range of {10,
12}, because the first range with the same first_serial_number wins.

Aside: I should add this example to the test suite :)

Using something like std::unordered_set<std::pair<uint64_t, uint64_t>> might be
simpler to understand reason about.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:262: auto issuer_revoked_serials =
revoked_serial_numbers_.find(hash);
readability nit: I find that naming variables like this with an "_iter" suffix
enhances readability.  In this case I would probably go something like
"issuer_iter".

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:265: std::pair<std::string,
std::unordered_map<uint64_t, uint64_t>>
Consider using std::make_pair to improve readability.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:268: issuer_revoked_serials =
revoked_serial_numbers_.find(hash);
No need to call find(). insert() will give you the iterator to the new entry:
issuer_revoked_serials = revoked_serial_numbers.insert(revocation_entry).first;

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:289: VLOG(2) << "CRL expired. Perform
hard fail.";
"CRL not time-valid" or something to that effect, since we are checking for
validity_start as well.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:299: return false;
Log reason?

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:315: if
(!net::der::ParseUint64(parsed_cert->tbs().serial_number,
Derive the value of serial_number once and then use it for every serial number
range in this loop.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:336: CrlBundle cast_crl;
s/cast_crl/crl_bundle/

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.h:47: // * |crl_proto| is the byte
representation of the Cast CRL proto.
How about: "is a serialized cast.CrlBundle proto.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
File components/cast_certificate/cast_crl_unittest.cc (right):

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:34: const
std::vector<std::string> certificate_chain,
const std::vector<std::string>& certificate_chain

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:42: EXPECT_EQ(result,
expected_result == RESULT_SUCCESS);
The logic was a little hard for me to follow: expected_result is being checked
inside of the TestVerifyXXX() functions, but is also returned and checked by the
caller.  Can we stick to doing one or the other?

[ryanchung, 2016-07-08 22:49:30]

and rebased!

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.h (right):

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:96: CRLOptions crl_options)
WARN_UNUSED_RESULT;
On 2016/07/08 18:07:07, sheretov wrote:
> Why is structure being passed by value?

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:126: bool VerifyCRL(const Crl& crl,
On 2016/07/08 18:07:07, sheretov wrote:
> Documenting the arguments might be useful for readability here.  Especially
the
> overall_validity_end argument.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:131: net::der::GeneralizedTime
signing_time_generalized =
On 2016/07/08 18:07:07, sheretov wrote:
> I would call this "verification_time_generalized".

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:146: signature = crl.signature();
On 2016/07/08 18:07:07, sheretov wrote:
> I would combine this with the previous line:
> std::string signature = crl.signature();
> 
> Same for signature_value_bit_string below.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:151: // Verify the signature
On 2016/07/08 18:07:07, sheretov wrote:
> style consistency nit: "// Parse the signature." comment above ends with a
> period, and this comment does not.  Same with "//Verify XXX" comments below.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:187: VLOG(2) << "CRL - Expired.";
On 2016/07/08 18:07:07, sheretov wrote:
> "CRL not time-valid" or something to that effect, since we are checking for
> validity_start as well.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:198: return false;
On 2016/07/08 18:07:07, sheretov wrote:
> This may warrant a log statement.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:199: cert_time +=
base::TimeDelta::FromMilliseconds(500);
On 2016/07/08 18:07:07, sheretov wrote:
> Consider explaining the rounding logic and why it's needed in a comment.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:229: // Hash of all revoked public key.
On 2016/07/08 18:07:07, sheretov wrote:
> s/Hash/Hashes/
> s/key/keys/

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:233: std::unordered_map<std::string,
std::unordered_map<uint64_t, uint64_t>>
On 2016/07/08 18:07:07, sheretov wrote:
> std::unordered_map<uint64_t, uint64_t> seems like a poor choice for a value
type
> here.  If there are multiple ranges that start with the same serial number
under
> the same issuer, they must contend for the same entry in the map.  This could
be
> alleviated with some kind of incremental range merge, so that {10, 12}, {10,
> 30}, {10, 20} => {10, 30}, but I don't see it below.  So, right now this
> admittedly contrived example will result in an incorrect effective range of
{10,
> 12}, because the first range with the same first_serial_number wins.
> 
> Aside: I should add this example to the test suite :)
> 
> Using something like std::unordered_set<std::pair<uint64_t, uint64_t>> might
be
> simpler to understand reason about.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:262: auto issuer_revoked_serials =
revoked_serial_numbers_.find(hash);
On 2016/07/08 18:07:07, sheretov wrote:
> readability nit: I find that naming variables like this with an "_iter" suffix
> enhances readability.  In this case I would probably go something like
> "issuer_iter".

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:265: std::pair<std::string,
std::unordered_map<uint64_t, uint64_t>>
On 2016/07/08 18:07:07, sheretov wrote:
> Consider using std::make_pair to improve readability.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:268: issuer_revoked_serials =
revoked_serial_numbers_.find(hash);
On 2016/07/08 18:07:07, sheretov wrote:
> No need to call find(). insert() will give you the iterator to the new entry:
> issuer_revoked_serials =
revoked_serial_numbers.insert(revocation_entry).first;

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:289: VLOG(2) << "CRL expired. Perform
hard fail.";
On 2016/07/08 18:07:07, sheretov wrote:
> "CRL not time-valid" or something to that effect, since we are checking for
> validity_start as well.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:299: return false;
On 2016/07/08 18:07:07, sheretov wrote:
> Log reason?

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:315: if
(!net::der::ParseUint64(parsed_cert->tbs().serial_number,
On 2016/07/08 18:07:07, sheretov wrote:
> Derive the value of serial_number once and then use it for every serial number
> range in this loop.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.cc:336: CrlBundle cast_crl;
On 2016/07/08 18:07:07, sheretov wrote:
> s/cast_crl/crl_bundle/

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl.h:47: // * |crl_proto| is the byte
representation of the Cast CRL proto.
On 2016/07/08 18:07:07, sheretov wrote:
> How about: "is a serialized cast.CrlBundle proto.

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
File components/cast_certificate/cast_crl_unittest.cc (right):

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:34: const
std::vector<std::string> certificate_chain,
On 2016/07/08 18:07:08, sheretov wrote:
> const std::vector<std::string>& certificate_chain

Done.

https://codereview.chromium.org/2050983002/diff/240001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:42: EXPECT_EQ(result,
expected_result == RESULT_SUCCESS);
On 2016/07/08 18:07:08, sheretov wrote:
> The logic was a little hard for me to follow: expected_result is being checked
> inside of the TestVerifyXXX() functions, but is also returned and checked by
the
> caller.  Can we stick to doing one or the other?

Done.
Changed the return value to be true iff the expected value is seen. This is
still needed to check if the test, as a whole, passed or failed.

[sheretov, 2016-07-08 23:18:40]

LGTM.  On to Eric.

[eroman, 2016-07-12 21:22:01]

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.cc:321: return true;
Can you remove this "return true" and instead have only "return false" in here?

In general "return true" statements in code like this scare me, since it is easy
for others to add code at the end without realizing that it may never be
reached.

I prefer the patter of instead only early-returning for failures (return false)
and having a single "return true" at the very end.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.cc:346:
CastTrustStore::Get().Clear();
optional: Maybe this should be done unconditionally at the start? (So
SetTrustAnchorForTest() with an invalid certificate has the effect of clearing
the trust store as side effect).

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.h (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:34: bool crl_required = true;
Do you plan on adding other properties to this? If not, an enum should be
sufficient in place of CRLOptions.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:75: //
nit: remove extra space.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:108: // Injects trusted root
certificates into the CastTrustStore.
Injects --> Replaces
And can then remove the comment below.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_cert_validator_test_helpers.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator_test_helpers.cc:89: std::string
ReadFromFile(const base::StringPiece& file_name) {
This function seems unnecessary -- How about instead just exposing
ReadTestFileToString() in the header?
(You can add the EXPECT_FALSE() into it if you like too).

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:64: // TODO (ryanchung): Add official
Cast CRL Root here
Why is this a TODO() rather than added here?

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:77: // Converts a base::Time::Exploded
to a net::der::GeneralizedTime.
side-note: This seems like something we should extract to a lower layer -- I
will propose a change for this.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:82: base::Time rounded_time =
base::Time::FromUTCExploded(exploded);
This isn't generally safe pattern, since FromUTCExploded() can fail.

See for instance crbug.com/601900

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:83: rounded_time +=
base::TimeDelta::FromMilliseconds(500);
Why is this rounding desired?
We don't for instance do this in cast_cert_validator.cc

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:112: return base::WrapUnique(new
net::SimpleSignaturePolicy(2048));
Can you add a comment explaining what algorithms are needed?
i.e. RSA SSA PKCS#1 v.1.5 with SHA-256, using RSA keys 2048-bits or longer.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:137: std::string signature =
crl.signature();
can this be a reference instead (to avoid copying the signature data).

Or rather, in the line below you can write:

net::der::BitString(net::der::Input(base::StringPiece(crl.signature())), 0);

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:143: base::WrapUnique(new
net::SimpleSignaturePolicy(2048));
Should this be using CreateCastSignaturePolicy() instead?

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:184: for (const auto cert :
result.paths[result.best_result_index]->path) {
const auto&

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:195: cert_time +=
base::TimeDelta::FromMilliseconds(500);
Same comment as earlier. I don't think there is real value in doing this.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:225: // Hashes of all revoked public
keys.
document what the hash used is.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:239:
base::TimeDelta::FromMilliseconds(tbs_crl.issuance_time_millis());
This is done elsewhere too, suggest extracting to a helper.
Maybe TimeFromMillis() or something?

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:242:
base::TimeDelta::FromMilliseconds(tbs_crl.issuance_time_millis() +
Note that adding the two numbers in this fashion could result in an integer
overflow.
Instead convert them to TimeDelta's separately and then add those (addition of
TimeDelta saturates rather than overflow)

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:284: if ((utc_time < validity_start) ||
(utc_time >= validity_end)) {
validity_end corresponds with notAfter, so shouldn't this be > and not >= ?

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:293:
scoped_refptr<net::ParsedCertificate> parsed_cert = trusted_chain[i];
can you use a const reference instead?

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:295: VLOG(2) << "Certificate chain not
available.";
This shouldn't be reachable.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:311: if
(!net::der::ParseUint64(parsed_cert->tbs().serial_number,
Why is this sufficient?

Serial numbers can be 160 bits long.

And in fact based on
https://bugs.chromium.org/p/chromium/issues/detail?id=621645 it was determined
that they may not even parse as valid numbers

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.h:39: const
std::vector<scoped_refptr<net::ParsedCertificate>>& certs,
You can use the typedef net::ParsedCertificateList for this.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.h:43: // Parse and verify the CRL used to
verify the revocation status of
nit:  Parse and verify --> Parses and verifies

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.h:57: // |data| must remain valid and not
be mutated throughout the lifetime of
side-note: Perhaps we should remove this constraint and just make a copy.
This way it is easier to explain, and since it is test-only code anyway
efficiency doesn't matter as much as clarity.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl_test_untrusted_root_ca_der-inc.h
(right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl_test_untrusted_root_ca_der-inc.h:5: //
Certificate:
Please remove this file from //components/cast_certificate, and instead move it
to  //components/test/data/cast_certificate/

The test code can read it from disk using one of the existing helpers.
The benefit is keeping test data out of this directory, as well as accidental
usage of test data in the production code.
And lastly, it is more convenient to have the file as a .pem anyway (can use it
as input to openssl and such).

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl_unittest.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:50: // Verifies that the
provided Cast CRL signed by a trusted issuer
CRL signed --> CRL is signed

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_test_untrusted_root_ca_der-inc.h (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_test_untrusted_root_ca_der-inc.h:5: //
Certificate:
Same comment -- please move test data out of this directory, and make it a file
loaded from disk instead.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/proto/revocation.proto (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:25: optional bytes
signer_cert = 2;
DESIGN: is this system going to allow for delegating the CRL authority?

Given that there is only one trusted CRL root, and the Crl includes just 1
signer cert (no intermediates), the certificate hiearchy is going to be limited
to only things directly signed by the root.

If that is the system used, then there isn't much advantage to having this
signer_cert over just having the CRL root sign these directly.

Is the intent here that Google would be the only entity capable of signing CRLs?

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:27: // Signature calculated
over the contents of the tbs_crl field.
Add an explanation that the signature algorithm is implied by the version.

Alternately, why not just include a SignatureAlgorithm (DER-encoded) instead?
This would easily extend to future signature algorithms, as well as support for
RSA PSS which has other parameters.
From a coding perspective it is pretty simple, we have a function to parse
SignatureAlgorithms.
And from the server side it is also easy -- if you want to use RSA PKCS#1 with
SHA256 you can just hardcode the signature algorithm to this constant:

https://cs.chromium.org/chromium/src/net/cert/internal/signature_algorithm_un...

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:46: optional uint64
last_serial_number = 3;
Document whether this is inclusive or exclusive (i.e. [first_serial_number,
last_serial_number] vs [first_serial_number, last_serial_number)).

I believe it is the former (inclusive).

[sheretov, 2016-07-12 21:59:04]

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/proto/revocation.proto (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:25: optional bytes
signer_cert = 2;
On 2016/07/12 21:22:01, eroman wrote:
> DESIGN: is this system going to allow for delegating the CRL authority?
> 
> Given that there is only one trusted CRL root, and the Crl includes just 1
> signer cert (no intermediates), the certificate hiearchy is going to be
limited
> to only things directly signed by the root.
> 
> If that is the system used, then there isn't much advantage to having this
> signer_cert over just having the CRL root sign these directly.
> 
> Is the intent here that Google would be the only entity capable of signing
CRLs?

The two-level hierarchy is for operational reasons.  Google is the only entity
that's going to signing these.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:27: // Signature calculated
over the contents of the tbs_crl field.
On 2016/07/12 21:22:01, eroman wrote:
> Add an explanation that the signature algorithm is implied by the version.
> 
> Alternately, why not just include a SignatureAlgorithm (DER-encoded) instead?
> This would easily extend to future signature algorithms, as well as support
for
> RSA PSS which has other parameters.
> From a coding perspective it is pretty simple, we have a function to parse
> SignatureAlgorithms.
> And from the server side it is also easy -- if you want to use RSA PKCS#1 with
> SHA256 you can just hardcode the signature algorithm to this constant:
> 
>
https://cs.chromium.org/chromium/src/net/cert/internal/signature_algorithm_un...

We've gone down that road (AlgorithmIDs sprinkled throughout the structures) and
ended up with a single version number (TbsCrl.version) implying all algorithms. 
This approach significantly simplifies processing logic with one check at the
top, instead of multiple throughout.  Any algorithm upgrade will be a big deal
once there are verifiers deployed on multiple client platforms.  The plan for
that situation is to bless a new algorithm suite as a new TbsCrl.version, and
publish multiple versions of the CRL concurrently in a CrlBundle, so that client
verifiers with support for new algorithms can have a staggered rollout.  Once
the rollout reaches critical mass, and we are comfortable with breaking
non-updated CRL verifiers, we stop including the old version of the CRL in the
published bundle.

[ryanchung, 2016-07-14 16:15:26]

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.cc:321: return true;
On 2016/07/12 21:21:59, eroman wrote:
> Can you remove this "return true" and instead have only "return false" in
here?
> 
> In general "return true" statements in code like this scare me, since it is
easy
> for others to add code at the end without realizing that it may never be
> reached.
> 
> I prefer the patter of instead only early-returning for failures (return
false)
> and having a single "return true" at the very end.

Done. Thanks!

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.cc:346:
CastTrustStore::Get().Clear();
On 2016/07/12 21:22:00, eroman wrote:
> optional: Maybe this should be done unconditionally at the start? (So
> SetTrustAnchorForTest() with an invalid certificate has the effect of clearing
> the trust store as side effect).

Sounds good. Should the return value still be false if an invalid certificate is
provided? False should imply that the operation failed and no side effects are
expected?

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.h (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:34: bool crl_required = true;
On 2016/07/12 21:22:00, eroman wrote:
> Do you plan on adding other properties to this? If not, an enum should be
> sufficient in place of CRLOptions.

Done. Don't have other properties in mind right now. Changed to enum.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:75: //
On 2016/07/12 21:22:00, eroman wrote:
> nit: remove extra space.

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:108: // Injects trusted root
certificates into the CastTrustStore.
On 2016/07/12 21:22:00, eroman wrote:
> Injects --> Replaces
> And can then remove the comment below.

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_cert_validator_test_helpers.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator_test_helpers.cc:89: std::string
ReadFromFile(const base::StringPiece& file_name) {
On 2016/07/12 21:22:00, eroman wrote:
> This function seems unnecessary -- How about instead just exposing
> ReadTestFileToString() in the header?
> (You can add the EXPECT_FALSE() into it if you like too).

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:64: // TODO (ryanchung): Add official
Cast CRL Root here
On 2016/07/12 21:22:01, eroman wrote:
> Why is this a TODO() rather than added here?

The root certificate has not been generated yet.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:82: base::Time rounded_time =
base::Time::FromUTCExploded(exploded);
On 2016/07/12 21:22:00, eroman wrote:
> This isn't generally safe pattern, since FromUTCExploded() can fail.
> 
> See for instance crbug.com/601900

Removed the rounding and removed this. Thanks for the heads up!

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:83: rounded_time +=
base::TimeDelta::FromMilliseconds(500);
On 2016/07/12 21:22:00, eroman wrote:
> Why is this rounding desired?
> We don't for instance do this in cast_cert_validator.cc

Removed rounding.
Originally, this was because the precision of the CRL's expiration was in
milliseconds so the rounding was added to be conservative due to the difference
in precision. We've now changed the precision of the CRL's expiration to seconds
to avoid the confusion here.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:112: return base::WrapUnique(new
net::SimpleSignaturePolicy(2048));
On 2016/07/12 21:22:00, eroman wrote:
> Can you add a comment explaining what algorithms are needed?
> i.e. RSA SSA PKCS#1 v.1.5 with SHA-256, using RSA keys 2048-bits or longer.

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:137: std::string signature =
crl.signature();
On 2016/07/12 21:22:00, eroman wrote:
> can this be a reference instead (to avoid copying the signature data).
> 
> Or rather, in the line below you can write:
> 
> net::der::BitString(net::der::Input(base::StringPiece(crl.signature())), 0);

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:143: base::WrapUnique(new
net::SimpleSignaturePolicy(2048));
On 2016/07/12 21:22:00, eroman wrote:
> Should this be using CreateCastSignaturePolicy() instead?

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:184: for (const auto cert :
result.paths[result.best_result_index]->path) {
On 2016/07/12 21:22:00, eroman wrote:
> const auto&

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:195: cert_time +=
base::TimeDelta::FromMilliseconds(500);
On 2016/07/12 21:22:01, eroman wrote:
> Same comment as earlier. I don't think there is real value in doing this.

Removed the rounding. Updated all instances of time to have a precision of
seconds to avoid this.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:225: // Hashes of all revoked public
keys.
On 2016/07/12 21:22:00, eroman wrote:
> document what the hash used is.

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:239:
base::TimeDelta::FromMilliseconds(tbs_crl.issuance_time_millis());
On 2016/07/12 21:22:00, eroman wrote:
> This is done elsewhere too, suggest extracting to a helper.
> Maybe TimeFromMillis() or something?

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:242:
base::TimeDelta::FromMilliseconds(tbs_crl.issuance_time_millis() +
On 2016/07/12 21:22:00, eroman wrote:
> Note that adding the two numbers in this fashion could result in an integer
> overflow.
> Instead convert them to TimeDelta's separately and then add those (addition of
> TimeDelta saturates rather than overflow)

Done. The CRL proto has been updated to use not_before and not_after to avoid
adding time.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:284: if ((utc_time < validity_start) ||
(utc_time >= validity_end)) {
On 2016/07/12 21:22:00, eroman wrote:
> validity_end corresponds with notAfter, so shouldn't this be > and not >= ?

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:293:
scoped_refptr<net::ParsedCertificate> parsed_cert = trusted_chain[i];
On 2016/07/12 21:22:00, eroman wrote:
> can you use a const reference instead?

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:295: VLOG(2) << "Certificate chain not
available.";
On 2016/07/12 21:22:00, eroman wrote:
> This shouldn't be reachable.

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:311: if
(!net::der::ParseUint64(parsed_cert->tbs().serial_number,
On 2016/07/12 21:22:00, eroman wrote:
> Why is this sufficient?
> 
> Serial numbers can be 160 bits long.
> 
> And in fact based on
> https://bugs.chromium.org/p/chromium/issues/detail?id=621645 it was determined
> that they may not even parse as valid numbers

For serial range revocation, we will only revoke Google-generate certificates.
In this case, serials will be guaranteed to be less than 64 bits in length.

Other certificates are expected to be revoked through revocation of the whole
ICA.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.h:39: const
std::vector<scoped_refptr<net::ParsedCertificate>>& certs,
On 2016/07/12 21:22:01, eroman wrote:
> You can use the typedef net::ParsedCertificateList for this.

To use this without importing the .h file, I'll have to forward declare it like
this:

namespace net {
class ParsedCertificate;
using ParsedCertificateList = std::vector<scoped_refptr<ParsedCertificate>>;
}  // namespace net

Is there a better way?

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.h:43: // Parse and verify the CRL used to
verify the revocation status of
On 2016/07/12 21:22:01, eroman wrote:
> nit:  Parse and verify --> Parses and verifies

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.h:57: // |data| must remain valid and not
be mutated throughout the lifetime of
On 2016/07/12 21:22:01, eroman wrote:
> side-note: Perhaps we should remove this constraint and just make a copy.
> This way it is easier to explain, and since it is test-only code anyway
> efficiency doesn't matter as much as clarity.

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl_test_untrusted_root_ca_der-inc.h
(right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl_test_untrusted_root_ca_der-inc.h:5: //
Certificate:
On 2016/07/12 21:22:01, eroman wrote:
> Please remove this file from //components/cast_certificate, and instead move
it
> to  //components/test/data/cast_certificate/
> 
> The test code can read it from disk using one of the existing helpers.
> The benefit is keeping test data out of this directory, as well as accidental
> usage of test data in the production code.
> And lastly, it is more convenient to have the file as a .pem anyway (can use
it
> as input to openssl and such).

Done. Thanks!

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl_unittest.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:50: // Verifies that the
provided Cast CRL signed by a trusted issuer
On 2016/07/12 21:22:01, eroman wrote:
> CRL signed --> CRL is signed

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_test_untrusted_root_ca_der-inc.h (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_test_untrusted_root_ca_der-inc.h:5: //
Certificate:
On 2016/07/12 21:22:01, eroman wrote:
> Same comment -- please move test data out of this directory, and make it a
file
> loaded from disk instead.

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/proto/revocation.proto (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:25: optional bytes
signer_cert = 2;
On 2016/07/12 21:22:01, eroman wrote:
> DESIGN: is this system going to allow for delegating the CRL authority?
> 
> Given that there is only one trusted CRL root, and the Crl includes just 1
> signer cert (no intermediates), the certificate hiearchy is going to be
limited
> to only things directly signed by the root.
> 
> If that is the system used, then there isn't much advantage to having this
> signer_cert over just having the CRL root sign these directly.
> 
> Is the intent here that Google would be the only entity capable of signing
CRLs?

The CRL will be signed by an ICA issued by the CRL root. This way, it is
possible to rotate out the signer. The CRL ICAs will only be accessible by
Google.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:46: optional uint64
last_serial_number = 3;
On 2016/07/12 21:22:01, eroman wrote:
> Document whether this is inclusive or exclusive (i.e. [first_serial_number,
> last_serial_number] vs [first_serial_number, last_serial_number)).
> 
> I believe it is the former (inclusive).

Done.

[eroman, 2016-07-15 22:52:49]

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.cc:346:
CastTrustStore::Get().Clear();
On 2016/07/14 16:15:24, ryanchung wrote:
> On 2016/07/12 21:22:00, eroman wrote:
> > optional: Maybe this should be done unconditionally at the start? (So
> > SetTrustAnchorForTest() with an invalid certificate has the effect of
clearing
> > the trust store as side effect).
> 
> Sounds good. Should the return value still be false if an invalid certificate
is
> provided? False should imply that the operation failed and no side effects are
> expected?

Good question...

Perhaps your approach is better then since it is easier to explain in comments
what the meaning of the return value is.

Feel free to take either approach as you see fit.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:311: if
(!net::der::ParseUint64(parsed_cert->tbs().serial_number,
On 2016/07/14 16:15:25, ryanchung wrote:
> On 2016/07/12 21:22:00, eroman wrote:
> > Why is this sufficient?
> > 
> > Serial numbers can be 160 bits long.
> > 
> > And in fact based on
> > https://bugs.chromium.org/p/chromium/issues/detail?id=621645 it was
determined
> > that they may not even parse as valid numbers
> 
> For serial range revocation, we will only revoke Google-generate certificates.
> In this case, serials will be guaranteed to be less than 64 bits in length.
> 
> Other certificates are expected to be revoked through revocation of the whole
> ICA.

Thanks for the explanation, that sounds reasonable.

And certainly the proto can always be extended later if this design requirement
changes and we want to support larger serial numbers, so this isn't a forever
commitment.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.h:39: const
std::vector<scoped_refptr<net::ParsedCertificate>>& certs,
On 2016/07/14 16:15:26, ryanchung wrote:
> On 2016/07/12 21:22:01, eroman wrote:
> > You can use the typedef net::ParsedCertificateList for this.
> 
> To use this without importing the .h file, I'll have to forward declare it
like
> this:
> 
> namespace net {
> class ParsedCertificate;
> using ParsedCertificateList = std::vector<scoped_refptr<ParsedCertificate>>;
> }  // namespace net
> 
> Is there a better way?

I would just import the necessary file (that is what we do elsewhere).
If you are concerned it is pulling in too much stuff, we can extract a separate
file for parsed_certificate_list.h that contains just that definition. I
wouldn't worry about this though.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/proto/revocation.proto (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:25: optional bytes
signer_cert = 2;
On 2016/07/14 16:15:26, ryanchung wrote:
> On 2016/07/12 21:22:01, eroman wrote:
> > DESIGN: is this system going to allow for delegating the CRL authority?
> > 
> > Given that there is only one trusted CRL root, and the Crl includes just 1
> > signer cert (no intermediates), the certificate hiearchy is going to be
> limited
> > to only things directly signed by the root.
> > 
> > If that is the system used, then there isn't much advantage to having this
> > signer_cert over just having the CRL root sign these directly.
> > 
> > Is the intent here that Google would be the only entity capable of signing
> CRLs?
> 
> The CRL will be signed by an ICA issued by the CRL root. This way, it is
> possible to rotate out the signer. The CRL ICAs will only be accessible by
> Google.

Thanks sheretov/ryanchung for the explanations!
(And apologies if I glossed over details in our earlier conversations requiring
any repetitions here).

So if a CRL ICA was ever compromised, our recourse would be to change the CRL
root used by Chrome right? (assuming the TTL on those ICAs is long).

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:27: // Signature calculated
over the contents of the tbs_crl field.
On 2016/07/12 21:59:04, sheretov wrote:
> On 2016/07/12 21:22:01, eroman wrote:
> > Add an explanation that the signature algorithm is implied by the version.
> > 
> > Alternately, why not just include a SignatureAlgorithm (DER-encoded)
instead?
> > This would easily extend to future signature algorithms, as well as support
> for
> > RSA PSS which has other parameters.
> > From a coding perspective it is pretty simple, we have a function to parse
> > SignatureAlgorithms.
> > And from the server side it is also easy -- if you want to use RSA PKCS#1
with
> > SHA256 you can just hardcode the signature algorithm to this constant:
> > 
> >
>
https://cs.chromium.org/chromium/src/net/cert/internal/signature_algorithm_un...
> 
> We've gone down that road (AlgorithmIDs sprinkled throughout the structures)
and
> ended up with a single version number (TbsCrl.version) implying all
algorithms. 
> This approach significantly simplifies processing logic with one check at the
> top, instead of multiple throughout.  Any algorithm upgrade will be a big deal
> once there are verifiers deployed on multiple client platforms.  The plan for
> that situation is to bless a new algorithm suite as a new TbsCrl.version, and
> publish multiple versions of the CRL concurrently in a CrlBundle, so that
client
> verifiers with support for new algorithms can have a staggered rollout.  Once
> the rollout reaches critical mass, and we are comfortable with breaking
> non-updated CRL verifiers, we stop including the old version of the CRL in the
> published bundle.

Fair enough, thanks for the explanation.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.cc (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_cert_validator.cc:322: if (result.paths.empty()
||
I'll see about simplifying these checks later, kind of cumbersome.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:64: // TODO (ryanchung): Add official
Cast CRL Root here
nit: TODO(ryanchung)
(not sure if that is normative or not)

Why aren't we adding it yet again? Because it hasn't been generated?

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:78: net::der::GeneralizedTime
convertTimeExploded(
(1) convertTimeExploded --> ConvertTimeExploded
(2) Note that we now have net::der::EncodeTimeAsGeneralizedTime(), which could
perhaps be used (assuming you change the API to take a base::Time instead).

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:90: // Converts a uint64_t utc time to
net::der::GeneralizedTime.
not sure that "utc time" is the right terminology. How about "unix timestamp",
or "seconds since unix epoch" instead?

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:93: base::Time::UnixEpoch() +
base::TimeDelta::FromSeconds(seconds);
This has the possibility of overflow -- FromSeconds takes an int64_t but we are
passing it a uint64_t, so it might wrap around to negative.

This may not cause problems but is technically relying on undefined behavior
which is undesirable (integer overflow for signed types is undefined).

Can you use base::saturated_cast<> instead?

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:127: // Parse the signature.
nti: "Parse the signature" --> "Wrap the signature in a BitString"

(No parsing is actually being done here, just encapsulating the bytes in a
different type.. because X.509 is silly and uses a BitString rather than octet
string).

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:154: }
Please add a comment about why no checks on the leaf certificate are necessary
(i.e. there are no requirements placed here on the leaf certificate having any
particular keyUsages).

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:159:
ConvertTimeSeconds(tbs_crl.not_before_seconds());
What is the role of setting a notBefore on the CRL?

Presumably once something is deemed revoked, it should be revoked immediately
and forever.
I don't think there is harm in having a notBefore, just wondering if it will
actually be useful.

In this case CRL verification fails if the current time is before not_before

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:197: net::der::GeneralizedTime
not_before;
name private variables with trailing underscore.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:198: net::der::GeneralizedTime
not_after;
same

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:207: std::unordered_map<std::string,
std::set<std::pair<uint64_t, uint64_t>>>
For the second part, I suggest using a std::vector<std::pair<,,>> rather than a
std::set<std::pair<,,>>.

It is basically just iterated over O(n) anyway, so a vector will be more
efficient.
The only advantage of a set is de-duplication if the server publishes redundant
ranges.
Not sure that is something we should worry about on the client side.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:229: auto issuer_iter =
revoked_serial_numbers_.find(issuer_hash);
How about using operator[], which combines the "get if it exists, otherwise add
and default construct".
Could look something like this:

auto& serial_number_range = revoked_serial_numbers_[issuer_hash];
serial_number_range.insert(std::make_pair(first_serial_number,
last_serial_number));

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:239: std::pair<uint64_t, uint64_t>
revocation_range(first_serial_number,
Can you define a struct for this instead?

struct SerialNumberRange {
  .. first;
  .. last;
};

I find std::pair<> hard to read.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:264: for (int i = trusted_chain.size() -
1; i >= 0; --i) {
style: I am generally fearful of this sort of loop construct, since changing the
loop counter to "size_t" becomes a bug (and technically the initial assignment
to "i" is an unchecked cast that could underflow).

I prefer either of these constructs instead:

// Iterate the chain in reverse
for (auto it = trusted_chain.rbegin(); it != trusted_chain.rend(); ++it) {
  const auto& parsed_cert = *it;
  ....
}

Or alternately:

  for (size_t i = 0; i < trusted_chain.size(); ++i) {
    // Iterate in reverse order
    const auto& parsed_cert = trusted_chain[trusted_chain.size() - 1 - i];
  }


Or another way is to restructure this so you iterate the chain in the "forward"
direction.
Instead of saving "issuer_key_hash", you could do it as follows:

  for (size_t i = 0; i < trusted_chain.size(); ++i) {
      ....

      // Check if the subordinate certificate was revoked by serial number
      if (i > 0) {
         const auto& subordinate = trusted_chain[i - 1];
         // Lookup in revoked_serial_numbers by (spki_hash,
subordinate->tbs().serial_number)..
      }
   }

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:321: return nullptr;
Failing here seems like a fine choice.
However it does perhaps seem inconsistent with the code above which "continues"
when failing to parse the protobuffer.

No change required, just observing to confirm this was intentional.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:26: virtual ~CastCRL(){};
space?
Suggest running "git cl format"

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:29: // of DER-encoded certificates.
nit on wording: Not sure "DER-encoded" needs to be part of that description.
ParsedCertificate to a large extend aims to abstract away the encoding of the
certificate.
Probably fine to just say "given a chain of X.509 certificates" or something.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:32: // * |certs| is the verified chain of
DER-encoded certificates:
same here -- can leave off the "DER-encoded"

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:35: //   * |certs.back()| must be trusted
anchor.
nit: instead of "must be trusted anchor" how about "assumed to be a trusted
root"

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:38: //   is revoked.
Can you mention the return value?

"Returns true if no certificate in the chain was revoked"

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl_root_ca_cert_der-inc.h (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl_root_ca_cert_der-inc.h:5: const unsigned
char kCastCRLRootCaDer[] = {0x30, /*TODO Cast CRL Root here*/};
TODO(ryanchung)

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl_unittest.cc (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:15: // Converts uint64_t UTC
time to Exploded time.
same comment regarding UTC time (I think the terminology is "unix timestamp"

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:163: // Parses the provided
test suite provided in wire-format proto.
FYI: I didn't review the unit-tests in as much detail, relying on sheretov to
have given a good design review of this protobuf approach.

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
File
components/test/data/cast_certificate/certificates/cast_crl_test_root_ca.pem
(right):

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
components/test/data/cast_certificate/certificates/cast_crl_test_root_ca.pem:1:
-----BEGIN CERTIFICATE-----
Can you include a summary of the certificate in this file?

i.e. the output from

$ openssl x509 -text -noout  -in cast_crl_test_root_ca.pem

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
File components/test/data/cast_certificate/certificates/cast_test_root_ca.pem
(right):

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
components/test/data/cast_certificate/certificates/cast_test_root_ca.pem:1:
-----BEGIN CERTIFICATE-----
same thing here.

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
File components/test/data/cast_certificate/testsuite/testsuite1.pb_text (right):

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
components/test/data/cast_certificate/testsuite/testsuite1.pb_text:1: tests {
What is the relation of this file to the binary .pb by the same name?

[ryanchung, 2016-07-18 23:39:09]

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.cc (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_cert_validator.cc:346:
CastTrustStore::Get().Clear();
On 2016/07/15 22:52:48, eroman wrote:
> On 2016/07/14 16:15:24, ryanchung wrote:
> > On 2016/07/12 21:22:00, eroman wrote:
> > > optional: Maybe this should be done unconditionally at the start? (So
> > > SetTrustAnchorForTest() with an invalid certificate has the effect of
> clearing
> > > the trust store as side effect).
> > 
> > Sounds good. Should the return value still be false if an invalid
certificate
> is
> > provided? False should imply that the operation failed and no side effects
are
> > expected?
> 
> Good question...
> 
> Perhaps your approach is better then since it is easier to explain in comments
> what the meaning of the return value is.
> 
> Feel free to take either approach as you see fit.

I'll stick with original plan. Replace only if anchor is valid.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.h:39: const
std::vector<scoped_refptr<net::ParsedCertificate>>& certs,
On 2016/07/15 22:52:48, eroman wrote:
> On 2016/07/14 16:15:26, ryanchung wrote:
> > On 2016/07/12 21:22:01, eroman wrote:
> > > You can use the typedef net::ParsedCertificateList for this.
> > 
> > To use this without importing the .h file, I'll have to forward declare it
> like
> > this:
> > 
> > namespace net {
> > class ParsedCertificate;
> > using ParsedCertificateList = std::vector<scoped_refptr<ParsedCertificate>>;
> > }  // namespace net
> > 
> > Is there a better way?
> 
> I would just import the necessary file (that is what we do elsewhere).
> If you are concerned it is pulling in too much stuff, we can extract a
separate
> file for parsed_certificate_list.h that contains just that definition. I
> wouldn't worry about this though.

Done.

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
File components/cast_certificate/proto/revocation.proto (right):

https://codereview.chromium.org/2050983002/diff/280001/components/cast_certif...
components/cast_certificate/proto/revocation.proto:25: optional bytes
signer_cert = 2;
On 2016/07/15 22:52:48, eroman wrote:
> On 2016/07/14 16:15:26, ryanchung wrote:
> > On 2016/07/12 21:22:01, eroman wrote:
> > > DESIGN: is this system going to allow for delegating the CRL authority?
> > > 
> > > Given that there is only one trusted CRL root, and the Crl includes just 1
> > > signer cert (no intermediates), the certificate hiearchy is going to be
> > limited
> > > to only things directly signed by the root.
> > > 
> > > If that is the system used, then there isn't much advantage to having this
> > > signer_cert over just having the CRL root sign these directly.
> > > 
> > > Is the intent here that Google would be the only entity capable of signing
> > CRLs?
> > 
> > The CRL will be signed by an ICA issued by the CRL root. This way, it is
> > possible to rotate out the signer. The CRL ICAs will only be accessible by
> > Google.
> 
> Thanks sheretov/ryanchung for the explanations!
> (And apologies if I glossed over details in our earlier conversations
requiring
> any repetitions here).
> 
> So if a CRL ICA was ever compromised, our recourse would be to change the CRL
> root used by Chrome right? (assuming the TTL on those ICAs is long).

I would prefer the ICAs to be short-lived (~ 1 week?). Otherwise it would make
more sense to just sign directly with the root. sheretov@, what do you think?

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:64: // TODO (ryanchung): Add official
Cast CRL Root here
On 2016/07/15 22:52:48, eroman wrote:
> nit: TODO(ryanchung)
> (not sure if that is normative or not)
> 
> Why aren't we adding it yet again? Because it hasn't been generated?

The root hasn't been generated due to administrative reasons...
We could delay submitting this CL until it is ready if that is preferred.
Fixed the nit.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:78: net::der::GeneralizedTime
convertTimeExploded(
On 2016/07/15 22:52:48, eroman wrote:
> (1) convertTimeExploded --> ConvertTimeExploded
> (2) Note that we now have net::der::EncodeTimeAsGeneralizedTime(), which could
> perhaps be used (assuming you change the API to take a base::Time instead).

Went with (2)
I've updated the API in cast_crl.h and cast_cert_validator.h to use base::Time
instead of base::Time::ExplodedTime.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:90: // Converts a uint64_t utc time to
net::der::GeneralizedTime.
On 2016/07/15 22:52:48, eroman wrote:
> not sure that "utc time" is the right terminology. How about "unix timestamp",
> or "seconds since unix epoch" instead?

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:93: base::Time::UnixEpoch() +
base::TimeDelta::FromSeconds(seconds);
On 2016/07/15 22:52:48, eroman wrote:
> This has the possibility of overflow -- FromSeconds takes an int64_t but we
are
> passing it a uint64_t, so it might wrap around to negative.
> 
> This may not cause problems but is technically relying on undefined behavior
> which is undesirable (integer overflow for signed types is undefined).
> 
> Can you use base::saturated_cast<> instead?

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:127: // Parse the signature.
On 2016/07/15 22:52:48, eroman wrote:
> nti: "Parse the signature" --> "Wrap the signature in a BitString"
> 
> (No parsing is actually being done here, just encapsulating the bytes in a
> different type.. because X.509 is silly and uses a BitString rather than octet
> string).

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:154: }
On 2016/07/15 22:52:48, eroman wrote:
> Please add a comment about why no checks on the leaf certificate are necessary
> (i.e. there are no requirements placed here on the leaf certificate having any
> particular keyUsages).

Done.
Ahhh just realized after uploading Patch23... I added the comment on line 131.
I'll move it down here in the next patch.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:159:
ConvertTimeSeconds(tbs_crl.not_before_seconds());
On 2016/07/15 22:52:48, eroman wrote:
> What is the role of setting a notBefore on the CRL?
> 
> Presumably once something is deemed revoked, it should be revoked immediately
> and forever.
> I don't think there is harm in having a notBefore, just wondering if it will
> actually be useful.
> 
> In this case CRL verification fails if the current time is before not_before

That's a very good point.
If a certificate expires, its revocation information will no longer be
distributed in the CRL (to maybe reduce the size...). If a verifier's clock is
slow and is in the past, the revoked certificate is still time-valid (to this
verfier) but the CRL no longer lists the certificate as being revoked.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:197: net::der::GeneralizedTime
not_before;
On 2016/07/15 22:52:48, eroman wrote:
> name private variables with trailing underscore.

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:198: net::der::GeneralizedTime
not_after;
On 2016/07/15 22:52:48, eroman wrote:
> same

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:207: std::unordered_map<std::string,
std::set<std::pair<uint64_t, uint64_t>>>
On 2016/07/15 22:52:48, eroman wrote:
> For the second part, I suggest using a std::vector<std::pair<,,>> rather than
a
> std::set<std::pair<,,>>.
> 
> It is basically just iterated over O(n) anyway, so a vector will be more
> efficient.
> The only advantage of a set is de-duplication if the server publishes
redundant
> ranges.
> Not sure that is something we should worry about on the client side.

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:229: auto issuer_iter =
revoked_serial_numbers_.find(issuer_hash);
On 2016/07/15 22:52:48, eroman wrote:
> How about using operator[], which combines the "get if it exists, otherwise
add
> and default construct".
> Could look something like this:
> 
> auto& serial_number_range = revoked_serial_numbers_[issuer_hash];
> serial_number_range.insert(std::make_pair(first_serial_number,
> last_serial_number));

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:239: std::pair<uint64_t, uint64_t>
revocation_range(first_serial_number,
On 2016/07/15 22:52:48, eroman wrote:
> Can you define a struct for this instead?
> 
> struct SerialNumberRange {
>   .. first;
>   .. last;
> };
> 
> I find std::pair<> hard to read.

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:264: for (int i = trusted_chain.size() -
1; i >= 0; --i) {
On 2016/07/15 22:52:48, eroman wrote:
> style: I am generally fearful of this sort of loop construct, since changing
the
> loop counter to "size_t" becomes a bug (and technically the initial assignment
> to "i" is an unchecked cast that could underflow).
> 
> I prefer either of these constructs instead:
> 
> // Iterate the chain in reverse
> for (auto it = trusted_chain.rbegin(); it != trusted_chain.rend(); ++it) {
>   const auto& parsed_cert = *it;
>   ....
> }
> 
> Or alternately:
> 
>   for (size_t i = 0; i < trusted_chain.size(); ++i) {
>     // Iterate in reverse order
>     const auto& parsed_cert = trusted_chain[trusted_chain.size() - 1 - i];
>   }
> 
> 
> Or another way is to restructure this so you iterate the chain in the
"forward"
> direction.
> Instead of saving "issuer_key_hash", you could do it as follows:
> 
>   for (size_t i = 0; i < trusted_chain.size(); ++i) {
>       ....
> 
>       // Check if the subordinate certificate was revoked by serial number
>       if (i > 0) {
>          const auto& subordinate = trusted_chain[i - 1];
>          // Lookup in revoked_serial_numbers by (spki_hash,
> subordinate->tbs().serial_number)..
>       }
>    }

Done. Forward direction sounds good. Thanks!

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:321: return nullptr;
On 2016/07/15 22:52:48, eroman wrote:
> Failing here seems like a fine choice.
> However it does perhaps seem inconsistent with the code above which
"continues"
> when failing to parse the protobuffer.
> 
> No change required, just observing to confirm this was intentional.

The CRL bundle will contain at max a single copy for each CRL version.
When a CRL fails to parse or is the wrong version, there could be other CRLs in
the bundle that is supported.
However, since only one version of the CRL is supported at this time, a
supported CRL that fails to verify indicates definitively that nothing in the
CRL bundle is usable.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:26: virtual ~CastCRL(){};
On 2016/07/15 22:52:48, eroman wrote:
> space?
> Suggest running "git cl format"

git cl format is removing the space. I don't know why.
Manually added space.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:29: // of DER-encoded certificates.
On 2016/07/15 22:52:49, eroman wrote:
> nit on wording: Not sure "DER-encoded" needs to be part of that description.
> ParsedCertificate to a large extend aims to abstract away the encoding of the
> certificate.
> Probably fine to just say "given a chain of X.509 certificates" or something.

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:32: // * |certs| is the verified chain of
DER-encoded certificates:
On 2016/07/15 22:52:49, eroman wrote:
> same here -- can leave off the "DER-encoded" 

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:35: //   * |certs.back()| must be trusted
anchor.
On 2016/07/15 22:52:49, eroman wrote:
> nit: instead of "must be trusted anchor" how about "assumed to be a trusted
> root"

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:38: //   is revoked.
On 2016/07/15 22:52:49, eroman wrote:
> Can you mention the return value?
> 
> "Returns true if no certificate in the chain was revoked"

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl_root_ca_cert_der-inc.h (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl_root_ca_cert_der-inc.h:5: const unsigned
char kCastCRLRootCaDer[] = {0x30, /*TODO Cast CRL Root here*/};
On 2016/07/15 22:52:49, eroman wrote:
> TODO(ryanchung)

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl_unittest.cc (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:15: // Converts uint64_t UTC
time to Exploded time.
On 2016/07/15 22:52:49, eroman wrote:
> same comment regarding UTC time (I think the terminology is "unix timestamp"

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:163: // Parses the provided
test suite provided in wire-format proto.
On 2016/07/15 22:52:49, eroman wrote:
> FYI: I didn't review the unit-tests in as much detail, relying on sheretov to
> have given a good design review of this protobuf approach.

We decided to use this proto test-suite approach so that the same sets of
inputs/expected-values can be run through the different implementations we have
(eg. iOS, java, chromium).

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
File
components/test/data/cast_certificate/certificates/cast_crl_test_root_ca.pem
(right):

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
components/test/data/cast_certificate/certificates/cast_crl_test_root_ca.pem:1:
-----BEGIN CERTIFICATE-----
On 2016/07/15 22:52:49, eroman wrote:
> Can you include a summary of the certificate in this file?
> 
> i.e. the output from
> 
> $ openssl x509 -text -noout  -in cast_crl_test_root_ca.pem

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
File components/test/data/cast_certificate/certificates/cast_test_root_ca.pem
(right):

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
components/test/data/cast_certificate/certificates/cast_test_root_ca.pem:1:
-----BEGIN CERTIFICATE-----
On 2016/07/15 22:52:49, eroman wrote:
> same thing here.

Done.

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
File components/test/data/cast_certificate/testsuite/testsuite1.pb_text (right):

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
components/test/data/cast_certificate/testsuite/testsuite1.pb_text:1: tests {
On 2016/07/15 22:52:49, eroman wrote:
> What is the relation of this file to the binary .pb by the same name?

They're the same but I thought adding the text-formatted version here will make
the tests more informational if someone doesn't have access to gqui.
Perhaps this file is not needed. Should this be deleted?

[eroman, 2016-07-19 01:54:59]

lgtm

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl.cc (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.cc:64: // TODO (ryanchung): Add official
Cast CRL Root here
On 2016/07/18 23:39:08, ryanchung wrote:
> On 2016/07/15 22:52:48, eroman wrote:
> > nit: TODO(ryanchung)
> > (not sure if that is normative or not)
> > 
> > Why aren't we adding it yet again? Because it hasn't been generated?
> 
> The root hasn't been generated due to administrative reasons...
> We could delay submitting this CL until it is ready if that is preferred.
> Fixed the nit.

That's fine, can submit a follow-up CL, and include a test verifying with that
root too.

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2050983002/diff/380001/components/cast_certif...
components/cast_certificate/cast_crl.h:26: virtual ~CastCRL(){};
On 2016/07/18 23:39:08, ryanchung wrote:
> On 2016/07/15 22:52:48, eroman wrote:
> > space?
> > Suggest running "git cl format"
> 
> git cl format is removing the space. I don't know why.
> Manually added space.

If that is what git cl does, then fine. No need to change
...I don't want to have to think about style formatting issues, am cool with
whatever tool does :)

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
File components/test/data/cast_certificate/testsuite/testsuite1.pb_text (right):

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
components/test/data/cast_certificate/testsuite/testsuite1.pb_text:1: tests {
On 2016/07/18 23:39:09, ryanchung wrote:
> On 2016/07/15 22:52:49, eroman wrote:
> > What is the relation of this file to the binary .pb by the same name?
> 
> They're the same but I thought adding the text-formatted version here will
make
> the tests more informational if someone doesn't have access to gqui.
> Perhaps this file is not needed. Should this be deleted?

Can the test directly load the text formatted proto file (and then the binary
one is no longer needed).

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.h (right):

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:71: // * |time| is the UTC
time to use for determining if the certificate
"UTC time" is no longer meaningful given it is a base::Time

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
File components/cast_certificate/cast_crl_unittest.cc (right):

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:113:
ConvertUnixTimestamp(test_case.cert_verification_time_seconds() * 1000);
I suggest changing ConvertUnixTimestamp to take seconds instead (since the input
data is seconds anyway).

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:115: uint64_t crl_verify_time =
test_case.crl_verification_time_seconds() * 1000;
here for instance.

[ryanchung, 2016-07-19 18:12:08]

The CQ bit was checked by ryanchung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 18:13:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 18:26:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 18:26:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ryanchung, 2016-07-19 18:30:25]

The CQ bit was checked by ryanchung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 18:31:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 18:48:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 18:48:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ryanchung, 2016-07-19 20:53:31]

The CQ bit was checked by ryanchung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 20:54:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ryanchung, 2016-07-19 21:29:54]

Thanks!

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
File components/test/data/cast_certificate/testsuite/testsuite1.pb_text (right):

https://codereview.chromium.org/2050983002/diff/380001/components/test/data/c...
components/test/data/cast_certificate/testsuite/testsuite1.pb_text:1: tests {
On 2016/07/19 01:54:59, eroman wrote:
> On 2016/07/18 23:39:09, ryanchung wrote:
> > On 2016/07/15 22:52:49, eroman wrote:
> > > What is the relation of this file to the binary .pb by the same name?
> > 
> > They're the same but I thought adding the text-formatted version here will
> make
> > the tests more informational if someone doesn't have access to gqui.
> > Perhaps this file is not needed. Should this be deleted?
> 
> Can the test directly load the text formatted proto file (and then the binary
> one is no longer needed).

Reading text formatted protos in Chromium is not supported due to the
LITE_RUNTIME requirement.

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
File components/cast_certificate/cast_cert_validator.h (right):

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:71: // * |time| is the UTC
time to use for determining if the certificate
On 2016/07/19 01:54:59, eroman wrote:
> "UTC time" is no longer meaningful given it is a base::Time

Done.

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
File components/cast_certificate/cast_crl_unittest.cc (right):

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:113:
ConvertUnixTimestamp(test_case.cert_verification_time_seconds() * 1000);
On 2016/07/19 01:54:59, eroman wrote:
> I suggest changing ConvertUnixTimestamp to take seconds instead (since the
input
> data is seconds anyway).

Done.

https://codereview.chromium.org/2050983002/diff/440001/components/cast_certif...
components/cast_certificate/cast_crl_unittest.cc:115: uint64_t crl_verify_time =
test_case.crl_verification_time_seconds() * 1000;
On 2016/07/19 01:54:59, eroman wrote:
> here for instance.

Done.

[ryanchung, 2016-07-19 21:31:32]

ryanchung@chromium.org changed reviewers:
+ asargent@chromium.org

[ryanchung, 2016-07-19 21:31:33]

+asargent Please review extensions/ and chrome/ (updated callsites)

Thanks!

[commit-bot: I haz the power, 2016-07-19 23:33:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 23:33:26]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[asargent_no_longer_on_chrome, 2016-07-20 21:26:41]

extensions stuff lgtm

[ryanchung, 2016-07-20 21:30:52]

ryanchung@chromium.org changed reviewers:
+ davidben@chromium.org

[ryanchung, 2016-07-20 21:30:54]

+davidben Please review DEPS +crypto (cast_crl.cc uses crypto/sha2.h).

Thanks!

[davidben, 2016-07-25 15:09:11]

DEPS rubber stamp lgtm (depending on //crypto when you're using the cert stuff
is reasonable. I'm assuming that all the certificate-related review was handled
by eroman and co)

[ryanchung, 2016-07-25 15:28:19]

The CQ bit was checked by ryanchung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 15:28:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 16:27:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 16:27:37]

Dry run: This issue passed the CQ dry run.

[ryanchung, 2016-07-25 16:39:48]

The CQ bit was checked by ryanchung@chromium.org

[ryanchung, 2016-07-25 16:39:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
sheretov@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2050983002/#ps500001
(title: "Updated networking_private_crypto_unittest.cc")

[commit-bot: I haz the power, 2016-07-25 16:40:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 16:44:20]

Committed patchset #26 (id:500001)

[commit-bot: I haz the power, 2016-07-25 16:45:34]

Description was changed from

==========
Cast device revocation checking.

Cast device certificates may be revoked through two ways:
1. The hash of the public key.
2. A serial number range for an issuer identified by the hash
   of its public key.

A customized proto is used as the medium for this information.

This change contains the implementation for verifying the custom CRL
and verifying a certificate's revocation status based on that CRL.

BUG=618463
==========

to

==========
Cast device revocation checking.

Cast device certificates may be revoked through two ways:
1. The hash of the public key.
2. A serial number range for an issuer identified by the hash
   of its public key.

A customized proto is used as the medium for this information.

This change contains the implementation for verifying the custom CRL
and verifying a certificate's revocation status based on that CRL.

BUG=618463

Committed: https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
Cr-Commit-Position: refs/heads/master@{#407492}
==========

[commit-bot: I haz the power, 2016-07-25 16:45:36]

Patchset 26 (id:??) landed as
https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
Cr-Commit-Position: refs/heads/master@{#407492}

[Mark P, 2016-07-25 19:04:25]

On 2016/07/25 16:45:36, commit-bot: I haz the power wrote:
> Patchset 26 (id:??) landed as
> https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
> Cr-Commit-Position: refs/heads/master@{#407492}

Causes failure on
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...
in
components_unittests

failures
VerifyCastDeviceCertTest.ChromecastGen2InvalidTime

VerifyCastDeviceCertTest.ChromecastGen2InvalidTime (run #1):
[ RUN      ] VerifyCastDeviceCertTest.ChromecastGen2InvalidTime
../../components/cast_certificate/cast_cert_validator_unittest.cc:99: Failure
Value of: base::Time::FromUTCExploded(time, &result)
  Actual: false
Expected: true
[  FAILED  ] VerifyCastDeviceCertTest.ChromecastGen2InvalidTime (13 ms)

[Mark P, 2016-07-25 19:04:54]

A revert of this CL (patchset #26 id:500001) has been created in
https://codereview.chromium.org/2181013002/ by mpearson@chromium.org.

The reason for reverting is: Causes failure on
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...
in
components_unittests

failures
VerifyCastDeviceCertTest.ChromecastGen2InvalidTime

VerifyCastDeviceCertTest.ChromecastGen2InvalidTime (run #1):
[ RUN      ] VerifyCastDeviceCertTest.ChromecastGen2InvalidTime
../../components/cast_certificate/cast_cert_validator_unittest.cc:99: Failure
Value of: base::Time::FromUTCExploded(time, &result)
  Actual: false
Expected: true
[  FAILED  ] VerifyCastDeviceCertTest.ChromecastGen2InvalidTime (13 ms)
.

[ryanchung, 2016-07-25 21:00:50]

Description was changed from

==========
Cast device revocation checking.

Cast device certificates may be revoked through two ways:
1. The hash of the public key.
2. A serial number range for an issuer identified by the hash
   of its public key.

A customized proto is used as the medium for this information.

This change contains the implementation for verifying the custom CRL
and verifying a certificate's revocation status based on that CRL.

BUG=618463

Committed: https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
Cr-Commit-Position: refs/heads/master@{#407492}
==========

to

==========
Cast device revocation checking.

Cast device certificates may be revoked through two ways:
1. The hash of the public key.
2. A serial number range for an issuer identified by the hash
   of its public key.

A customized proto is used as the medium for this information.

This change contains the implementation for verifying the custom CRL
and verifying a certificate's revocation status based on that CRL.

BUG=618463

Committed: https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
Cr-Commit-Position: refs/heads/master@{#407492}
==========

[ryanchung, 2016-07-25 23:48:57]

The test was failing on 32 bit systems because base::Time::FromExploded was
clamping the range to what is supported by mktime and timegm.

Exploded time is not used in the actual logic of this CL.

This only affects cast_cert_validator_unittests.cc where a date is created for
test purposes. Update the date to be less than year 2038 but still after the
cert's expiry.

[ryanchung, 2016-07-25 23:49:18]

The CQ bit was checked by ryanchung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 23:50:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 01:52:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 01:52:48]

Dry run: This issue passed the CQ dry run.

[ryanchung, 2016-07-26 04:10:03]

The CQ bit was checked by ryanchung@chromium.org

[ryanchung, 2016-07-26 04:10:04]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
asargent@chromium.org, sheretov@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/2050983002/#ps520001
(title: "Fixed test failure on 32 bit systems.")

[commit-bot: I haz the power, 2016-07-26 04:10:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 04:14:28]

Description was changed from

==========
Cast device revocation checking.

Cast device certificates may be revoked through two ways:
1. The hash of the public key.
2. A serial number range for an issuer identified by the hash
   of its public key.

A customized proto is used as the medium for this information.

This change contains the implementation for verifying the custom CRL
and verifying a certificate's revocation status based on that CRL.

BUG=618463

Committed: https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
Cr-Commit-Position: refs/heads/master@{#407492}
==========

to

==========
Cast device revocation checking.

Cast device certificates may be revoked through two ways:
1. The hash of the public key.
2. A serial number range for an issuer identified by the hash
   of its public key.

A customized proto is used as the medium for this information.

This change contains the implementation for verifying the custom CRL
and verifying a certificate's revocation status based on that CRL.

BUG=618463

Committed: https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
Cr-Commit-Position: refs/heads/master@{#407492}
==========

[commit-bot: I haz the power, 2016-07-26 04:14:30]

Committed patchset #27 (id:520001)

[commit-bot: I haz the power, 2016-07-26 04:17:08]

Description was changed from

==========
Cast device revocation checking.

Cast device certificates may be revoked through two ways:
1. The hash of the public key.
2. A serial number range for an issuer identified by the hash
   of its public key.

A customized proto is used as the medium for this information.

This change contains the implementation for verifying the custom CRL
and verifying a certificate's revocation status based on that CRL.

BUG=618463

Committed: https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
Cr-Commit-Position: refs/heads/master@{#407492}
==========

to

==========
Cast device revocation checking.

Cast device certificates may be revoked through two ways:
1. The hash of the public key.
2. A serial number range for an issuer identified by the hash
   of its public key.

A customized proto is used as the medium for this information.

This change contains the implementation for verifying the custom CRL
and verifying a certificate's revocation status based on that CRL.

BUG=618463

Committed: https://crrev.com/698608f28ed2df276f920c9691cbfcb8f9069337
Committed: https://crrev.com/ed1a0da4ea59a8f42d133818eed5e34d45fa163b
Cr-Original-Commit-Position: refs/heads/master@{#407492}
Cr-Commit-Position: refs/heads/master@{#407704}
==========

[commit-bot: I haz the power, 2016-07-26 04:17:11]

Patchset 27 (id:??) landed as
https://crrev.com/ed1a0da4ea59a8f42d133818eed5e34d45fa163b
Cr-Commit-Position: refs/heads/master@{#407704}