Hook up Chrome Cast sender to Cast CRL.

Hook up Chrome Cast sender to Cast CRL.

* Hook up cast_auth_util to Cast CRL.
* Revocation status is not enforced at the moment.
* Allows custom trust store to be passed to VerifyDeviceCert and ParseAndVerifyCRL. This is so that testing the Chrome Cast sender can use test roots.
* Running the whole proto-test-suite on the Chrome Cast sender.

Sync-up of the following with internal:
* test_suite.proto
* cast_channel.proto

BUG=618463
Committed: https://crrev.com/559a29a14edcfdb03f5c952d0f6534b5bc4b7667
Cr-Commit-Position: refs/heads/master@{#425875}

[ryanchung, 2016-09-01 18:26:36]

Description was changed from

==========
Hook up cast_auth_util to Cast CRL.

Revocation status is not enforced at the moment.

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

to

==========
Hook up cast_auth_util to Cast CRL.

Revocation status is not enforced at the moment.

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

[ryanchung, 2016-09-01 18:26:36]

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

[sheretov, 2016-09-06 21:21:48]

I'm not quite sold on the verification-reverificatino approach here.  See
comments in the code.

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_auth_util.cc:225: // This function does
the following
This comment will need updating with the introduction of revocation checking.

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_auth_util.cc:259: bool
verification_success = cast_crypto::VerifyDeviceCert(
Can we collapse these two invocations of VerifyDeviceCert into:

bool verification_success = cast_crypto::VerifyDeviceCert(
    cert_chain, now, &verification_context, &device_policy, nullptr,
    kEnforceRevocationCheck ?
        cast_crypto::CRLPolicy::CRL_REQUIRED :
        cast_crypto::CRLPolicy::CRL_OPTIONAL);

I understand how something like this is useful in testing code to point out the
source of the verification failure, but in production code it seems like we
should not be trying to verify multiple times with different settings, but
rather figure out the right settings and verify once.

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_auth_util.cc:271:
signature_verification_success =
I don't see how signature_verification_success ever get a non-false value in the
case where:
verification_success == false && reverification_success == true &&
kEnforceRevocationCheck == false.

Presumably, we want the entire process to succeed in that case.

[ryanchung, 2016-09-07 17:23:05]

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_auth_util.cc:225: // This function does
the following
On 2016/09/06 21:21:47, sheretov wrote:
> This comment will need updating with the introduction of revocation checking.

Done.

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_auth_util.cc:259: bool
verification_success = cast_crypto::VerifyDeviceCert(
On 2016/09/06 21:21:47, sheretov wrote:
> Can we collapse these two invocations of VerifyDeviceCert into:
> 
> bool verification_success = cast_crypto::VerifyDeviceCert(
>     cert_chain, now, &verification_context, &device_policy, nullptr,
>     kEnforceRevocationCheck ?
>         cast_crypto::CRLPolicy::CRL_REQUIRED :
>         cast_crypto::CRLPolicy::CRL_OPTIONAL);
> 
> I understand how something like this is useful in testing code to point out
the
> source of the verification failure, but in production code it seems like we
> should not be trying to verify multiple times with different settings, but
> rather figure out the right settings and verify once.

The intention here was that there would be only one invocation in the primary
scenario. The secondary scenario is only run if the primary fails.
The expected scenario, CRL is required and passed in.
The secondary scenario, CRL is optional and omited.
The secondary scenario is meant to be informational only once the feature is
rolled out. (ie. kEnforceRevocationCheck = true).

CRL_OPTIONAL means the CRL can be missing, but if its not missing then it is
used. If the two invocations are collapsed, then there's no way to ignore the
revocation check because the CRL will have to be passed in and processed. It
only helps in the case, the CRL is missing.

Alternatively, we could return more detailed errors from VerifyDeviceCert.

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_auth_util.cc:271:
signature_verification_success =
On 2016/09/06 21:21:47, sheretov wrote:
> I don't see how signature_verification_success ever get a non-false value in
the
> case where:
> verification_success == false && reverification_success == true &&
> kEnforceRevocationCheck == false.
> 
> Presumably, we want the entire process to succeed in that case.

Done.

[ryanchung, 2016-09-08 18:07:38]

Description was changed from

==========
Hook up cast_auth_util to Cast CRL.

Revocation status is not enforced at the moment.

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

to

==========
Hook up Chrome Cast sender to Cast CRL.

* Hook up cast_auth_util to Cast CRL.
* Revocation status is not enforced at the moment.
* Allows custom trust store to be passed to VerifyDeviceCert and
ParseAndVerifyCRL. This is so that testing the Chrome Cast sender can use test
roots.
* Running the whole proto-test-suite on the Chrome Cast sender.

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

[ryanchung, 2016-09-08 18:12:52]

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/1/extensions/browser/api/cast...
extensions/browser/api/cast_channel/cast_auth_util.cc:259: bool
verification_success = cast_crypto::VerifyDeviceCert(
On 2016/09/07 17:23:05, ryanchung wrote:
> On 2016/09/06 21:21:47, sheretov wrote:
> > Can we collapse these two invocations of VerifyDeviceCert into:
> > 
> > bool verification_success = cast_crypto::VerifyDeviceCert(
> >     cert_chain, now, &verification_context, &device_policy, nullptr,
> >     kEnforceRevocationCheck ?
> >         cast_crypto::CRLPolicy::CRL_REQUIRED :
> >         cast_crypto::CRLPolicy::CRL_OPTIONAL);
> > 
> > I understand how something like this is useful in testing code to point out
> the
> > source of the verification failure, but in production code it seems like we
> > should not be trying to verify multiple times with different settings, but
> > rather figure out the right settings and verify once.
> 
> The intention here was that there would be only one invocation in the primary
> scenario. The secondary scenario is only run if the primary fails.
> The expected scenario, CRL is required and passed in.
> The secondary scenario, CRL is optional and omited.
> The secondary scenario is meant to be informational only once the feature is
> rolled out. (ie. kEnforceRevocationCheck = true).
> 
> CRL_OPTIONAL means the CRL can be missing, but if its not missing then it is
> used. If the two invocations are collapsed, then there's no way to ignore the
> revocation check because the CRL will have to be passed in and processed. It
> only helps in the case, the CRL is missing.
> 
> Alternatively, we could return more detailed errors from VerifyDeviceCert.

As we discussed in person, we'll add a TODO here to remove the second
verification once this feature is rolled out and once we implement error
reporting for certificate and revocation errors.

[ryanchung, 2016-09-08 18:17:22]

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

[ryanchung, 2016-09-08 18:17:23]

We are removing the suffix "ForTest" on VerifyDeviceCert() and
ParseAndVerifyCRL() to allow these functions to be used by the Chrome Cast
sender. This way we can use test roots for testing the Chrome Cast sender API.

To accidentally bypass the production trust store, one has to implicitly create
a trust store and pass it in. So, it seems safe enough.

WDYT?

[ryanchung, 2016-09-08 18:18:31]

On 2016/09/08 18:17:23, ryanchung wrote:
> We are removing the suffix "ForTest" on VerifyDeviceCert() and
> ParseAndVerifyCRL() to allow these functions to be used by the Chrome Cast
> sender. This way we can use test roots for testing the Chrome Cast sender API.
> 
> To accidentally bypass the production trust store, one has to implicitly
create
> a trust store and pass it in. So, it seems safe enough.
> 
> WDYT?

I mean, explicitly.

[eroman, 2016-09-08 18:45:47]

Quick comments on the API:

This change has some ambiguity on what the peferred way to call the default case
is:
  
Both:
  VerifyDeviceCert(<args>);
And
  VerifyDeviceCert(<args>, nullptr);

Accomplish the same thing and are pretty similar looking.

This ambiguity could be resolved by either:

 (a) Remove the version with fewer parameters and require all callers of it to
pass nullptr. Explicit, but kind of annoying to type for the normal case.

 (b) Rename the longer version to something more verbose, like
"VerifyDeviceCertUsingCustomTrustStore()", and don't allow nullptr for it.

Although it is a mouthful, my preference would be option (b) since it makes it
very explicit in the code and to reviewers that they are doing something
deserving of attention. (If code mismanages the trust store used then all
security bets are off).

[ryanchung, 2016-09-08 21:55:21]

On 2016/09/08 18:45:47, eroman wrote:
> Quick comments on the API:
> 
> This change has some ambiguity on what the peferred way to call the default
case
> is:
>   
> Both:
>   VerifyDeviceCert(<args>);
> And
>   VerifyDeviceCert(<args>, nullptr);
> 
> Accomplish the same thing and are pretty similar looking.
> 
> This ambiguity could be resolved by either:
> 
>  (a) Remove the version with fewer parameters and require all callers of it to
> pass nullptr. Explicit, but kind of annoying to type for the normal case.
> 
>  (b) Rename the longer version to something more verbose, like
> "VerifyDeviceCertUsingCustomTrustStore()", and don't allow nullptr for it.
> 
> Although it is a mouthful, my preference would be option (b) since it makes it
> very explicit in the code and to reviewers that they are doing something
> deserving of attention. (If code mismanages the trust store used then all
> security bets are off).

I like option b) too but every piece of code that uses this API will both need
to be used in production and be tested using test data. That means these two API
will always be used together (the testing code path and non-testing code path).
I guess that's a good thing that to draw more attention. I've updated the CL to
reflect this.

[sheretov, 2016-09-10 00:26:57]

Couple of nits, but otherwise lgtm.

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
File components/cast_certificate/cast_cert_validator.cc (right):

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
components/cast_certificate/cast_cert_validator.cc:336: return
VerifyDeviceCertImpl(certs, time, context, policy, crl, crl_policy,
Same comment as in cast_crl.cc: seems like we could eliminate the "Impl"
function and use the "UsingCustomTrustStore".

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

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:354: std::unique_ptr<CastCRL>
ParseAndVerifyCRLUsingCustomTrustStore(
Can we eliminate ParseAndVerifyCRLImpl, move all that logic into
ParseAndVerifyCRLUsingCustomTrustStore, and call it from ParseAndVerifyCRL?

[ryanchung, 2016-09-10 00:50:09]

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
File components/cast_certificate/cast_cert_validator.cc (right):

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
components/cast_certificate/cast_cert_validator.cc:336: return
VerifyDeviceCertImpl(certs, time, context, policy, crl, crl_policy,
On 2016/09/10 00:26:57, sheretov wrote:
> Same comment as in cast_crl.cc: seems like we could eliminate the "Impl"
> function and use the "UsingCustomTrustStore".

Done.

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

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
components/cast_certificate/cast_crl.cc:354: std::unique_ptr<CastCRL>
ParseAndVerifyCRLUsingCustomTrustStore(
On 2016/09/10 00:26:57, sheretov wrote:
> Can we eliminate ParseAndVerifyCRLImpl, move all that logic into
> ParseAndVerifyCRLUsingCustomTrustStore, and call it from ParseAndVerifyCRL?

Done.

[eroman, 2016-09-10 01:03:23]

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
File components/cast_certificate/cast_cert_validator_test_helpers.cc (right):

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
components/cast_certificate/cast_cert_validator_test_helpers.cc:102:
base::TimeDelta::FromMilliseconds(time * 1000);
Use FromSeconds().

(It also avoids doing the overflow-prone multiplication on |time|)

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
components/cast_certificate/cast_crl.h:57: // the input of a custom TrustStore.
If |trust_store| is nullptr, the default
This comment doesn't agree with implementation.

https://codereview.chromium.org/2303673004/diff/80001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/80001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:34: const bool
kEnforceRevocationCheck = false;
Who is the intended consumer that would set this to false? (i.e. this is neither
a runtime settable thing, nor a compile-time flag; is it intended for local
debugging sessions or a future switchover?)

https://codereview.chromium.org/2303673004/diff/80001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:149: if
(cache->hash_.compare(crl_bundle_hash) == 0) {
Does this work:
   cache->hash == crl_bundle_hash

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

https://codereview.chromium.org/2303673004/diff/100001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:106: // For production use
pass |trust_store| as nullptr to use the production trust
This comment does not agree with the implementation.

https://codereview.chromium.org/2303673004/diff/100001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/100001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:224: crl =
CastCRLCache::Put(response.crl(), std::move(crl_parsed));
I don't understand the caching behavior here.
 * What happens when it expires?
 * This is not thread-safe, is that expected?
 * The cache is shared by custom and-non custom trust store invocations. Does
this mean a caller with bogus trust store can populate a cache entry that will
then be used when this is called again with default trust store, and possibly
consider that CRL to be trusted?

[ryanchung, 2016-09-14 18:53:40]

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
File components/cast_certificate/cast_cert_validator_test_helpers.cc (right):

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
components/cast_certificate/cast_cert_validator_test_helpers.cc:102:
base::TimeDelta::FromMilliseconds(time * 1000);
On 2016/09/10 01:03:23, eroman wrote:
> Use FromSeconds().
> 
> (It also avoids doing the overflow-prone multiplication on |time|)

Done.

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
File components/cast_certificate/cast_crl.h (right):

https://codereview.chromium.org/2303673004/diff/80001/components/cast_certifi...
components/cast_certificate/cast_crl.h:57: // the input of a custom TrustStore.
If |trust_store| is nullptr, the default
On 2016/09/10 01:03:23, eroman wrote:
> This comment doesn't agree with implementation.

Done.

https://codereview.chromium.org/2303673004/diff/80001/extensions/browser/api/...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/80001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:34: const bool
kEnforceRevocationCheck = false;
On 2016/09/10 01:03:23, eroman wrote:
> Who is the intended consumer that would set this to false? (i.e. this is
neither
> a runtime settable thing, nor a compile-time flag; is it intended for local
> debugging sessions or a future switchover?)

This is for a future switchover. Eventually, we'll flip this flag to enforce
revocation checks. This is just to track the location where we need to make the
change for the switchover.

Not sure we need this flag to be settable during runtime or compile-time. We
don't want the user to be able to flip this.

https://codereview.chromium.org/2303673004/diff/80001/extensions/browser/api/...
extensions/browser/api/cast_channel/cast_auth_util.cc:149: if
(cache->hash_.compare(crl_bundle_hash) == 0) {
On 2016/09/10 01:03:23, eroman wrote:
> Does this work:
>    cache->hash == crl_bundle_hash

Done.

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

https://codereview.chromium.org/2303673004/diff/100001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:106: // For production use
pass |trust_store| as nullptr to use the production trust
On 2016/09/10 01:03:23, eroman wrote:
> This comment does not agree with the implementation.

Done.

https://codereview.chromium.org/2303673004/diff/100001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/100001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:224: crl =
CastCRLCache::Put(response.crl(), std::move(crl_parsed));
On 2016/09/10 01:03:23, eroman wrote:
> I don't understand the caching behavior here.
>  * What happens when it expires?
>  * This is not thread-safe, is that expected?
>  * The cache is shared by custom and-non custom trust store invocations. Does
> this mean a caller with bogus trust store can populate a cache entry that will
> then be used when this is called again with default trust store, and possibly
> consider that CRL to be trusted?

* What happens when it expires?
This is meant to be a dumb cache. It just stores the last successfully parsed
CRL and does not track expiry. The CastCRL itself tracks the expiry of the
information and will cause a failure in VerifyDeviceCert() when appropriate.

The cached CastCRL will only be used if it matches the one passed in |response|.
So I guess it is a little confusing here because if an expired CRL from
|response| is not cached, then it'll fail to ParseAndVerifyCRL(), but if it is
already cached, then we'll fail to VerifyDeviceCert(). 

* This is not thread-safe, is that expected?
Ah you're right, that's a problem. The cache owns the parsed CastCRL. Not sure
how to make this work without using some ref-counting and maybe AutoLock for
Get() and Put().

* The cache is shared by custom and-non custom trust store invocations.
Good point. The cache should not be used in the custom trust store scenario
since the state of the trust store is hard to track.

Thinking about this whole thing, I'm no longer sure if it is worthwhile to cache
the parsed CRL.
The use case is that the device will always send a copy of the CRL with its
certificate during device auth. The idea was to save some compute power by not
processing the CRL if it is the same since all devices share the same CRL. But
I'm not sure its worth the complexity of having a global state. What would you
suggest?

[eroman, 2016-09-14 19:46:42]

https://codereview.chromium.org/2303673004/diff/100001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/100001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:224: crl =
CastCRLCache::Put(response.crl(), std::move(crl_parsed));
On 2016/09/14 18:53:40, ryanchung wrote:
> On 2016/09/10 01:03:23, eroman wrote:
> > I don't understand the caching behavior here.
> >  * What happens when it expires?
> >  * This is not thread-safe, is that expected?
> >  * The cache is shared by custom and-non custom trust store invocations.
Does
> > this mean a caller with bogus trust store can populate a cache entry that
will
> > then be used when this is called again with default trust store, and
possibly
> > consider that CRL to be trusted?
> 
> * What happens when it expires?
> This is meant to be a dumb cache. It just stores the last successfully parsed
> CRL and does not track expiry. The CastCRL itself tracks the expiry of the
> information and will cause a failure in VerifyDeviceCert() when appropriate.
> 
> The cached CastCRL will only be used if it matches the one passed in
|response|.
> So I guess it is a little confusing here because if an expired CRL from
> |response| is not cached, then it'll fail to ParseAndVerifyCRL(), but if it is
> already cached, then we'll fail to VerifyDeviceCert(). 
> 
> * This is not thread-safe, is that expected?
> Ah you're right, that's a problem. The cache owns the parsed CastCRL. Not sure
> how to make this work without using some ref-counting and maybe AutoLock for
> Get() and Put().
> 
> * The cache is shared by custom and-non custom trust store invocations.
> Good point. The cache should not be used in the custom trust store scenario
> since the state of the trust store is hard to track.
> 
> Thinking about this whole thing, I'm no longer sure if it is worthwhile to
cache
> the parsed CRL.
> The use case is that the device will always send a copy of the CRL with its
> certificate during device auth. The idea was to save some compute power by not
> processing the CRL if it is the same since all devices share the same CRL. But
> I'm not sure its worth the complexity of having a global state. What would you
> suggest?

My suggestion would be to not use a cache unless it is needed. "if it is needed"
is best answered by a benchmark.

From a code-review perspective if you do decide to add a cache I think it is
worth proposing as a separate CL for a more thorough review (and then we can
land the uncontentious parts first).

Here is my thinking:

If this isn't on a critical path, then the advantage of not caching is like you
said code simplicity, and also a smaller memory footprint (since there won't be
lingering memory usage for the CRL cache). Since the CRL is being provided
always anyway, the time saving is just the parsing and verification of it, and
not an actual fetch time, so meh.

I haven't run numbers, but my gut feeling is that verifying the CRL is probably
around 50% of the overall cost of doing the device cert verification. It depends
on the length of the respective certificate chains, but the dominant cost will
likely be the RSA signature verification (so count up how many
VerifySignedData() calls each does for an approximate cost model).

If it turns out that performance of VerifyDeviceCert() is too slow, we may want
to consider broader caching not just for CRL part, but for the target
certificate too, so a more general cache may be what we want instead.

Assuming we do decide on a cache, my first thought is to introduce a sort of
"context" object, that encapsulates the trust store + caches. Instead of passing
in a custom trust store as we do today, tests would pass in a custom "context"
object instead (which contains the trust store).

The default context when unspecified could be a global (much how the default
trust store is a global today). If we don't anticipate multi-threaded callers,
then adding some assertions to ensure the entry points (VerifyDeviceCert())
aren't called on other threads should be sufficient.

I dunno, just thinking out loud.

[ryanchung, 2016-09-14 23:35:48]

https://codereview.chromium.org/2303673004/diff/100001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/100001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:224: crl =
CastCRLCache::Put(response.crl(), std::move(crl_parsed));
On 2016/09/14 19:46:42, eroman wrote:
> On 2016/09/14 18:53:40, ryanchung wrote:
> > On 2016/09/10 01:03:23, eroman wrote:
> > > I don't understand the caching behavior here.
> > >  * What happens when it expires?
> > >  * This is not thread-safe, is that expected?
> > >  * The cache is shared by custom and-non custom trust store invocations.
> Does
> > > this mean a caller with bogus trust store can populate a cache entry that
> will
> > > then be used when this is called again with default trust store, and
> possibly
> > > consider that CRL to be trusted?
> > 
> > * What happens when it expires?
> > This is meant to be a dumb cache. It just stores the last successfully
parsed
> > CRL and does not track expiry. The CastCRL itself tracks the expiry of the
> > information and will cause a failure in VerifyDeviceCert() when appropriate.
> > 
> > The cached CastCRL will only be used if it matches the one passed in
> |response|.
> > So I guess it is a little confusing here because if an expired CRL from
> > |response| is not cached, then it'll fail to ParseAndVerifyCRL(), but if it
is
> > already cached, then we'll fail to VerifyDeviceCert(). 
> > 
> > * This is not thread-safe, is that expected?
> > Ah you're right, that's a problem. The cache owns the parsed CastCRL. Not
sure
> > how to make this work without using some ref-counting and maybe AutoLock for
> > Get() and Put().
> > 
> > * The cache is shared by custom and-non custom trust store invocations.
> > Good point. The cache should not be used in the custom trust store scenario
> > since the state of the trust store is hard to track.
> > 
> > Thinking about this whole thing, I'm no longer sure if it is worthwhile to
> cache
> > the parsed CRL.
> > The use case is that the device will always send a copy of the CRL with its
> > certificate during device auth. The idea was to save some compute power by
not
> > processing the CRL if it is the same since all devices share the same CRL.
But
> > I'm not sure its worth the complexity of having a global state. What would
you
> > suggest?
> 
> My suggestion would be to not use a cache unless it is needed. "if it is
needed"
> is best answered by a benchmark.
> 
> From a code-review perspective if you do decide to add a cache I think it is
> worth proposing as a separate CL for a more thorough review (and then we can
> land the uncontentious parts first).
> 
> Here is my thinking:
> 
> If this isn't on a critical path, then the advantage of not caching is like
you
> said code simplicity, and also a smaller memory footprint (since there won't
be
> lingering memory usage for the CRL cache). Since the CRL is being provided
> always anyway, the time saving is just the parsing and verification of it, and
> not an actual fetch time, so meh.
> 
> I haven't run numbers, but my gut feeling is that verifying the CRL is
probably
> around 50% of the overall cost of doing the device cert verification. It
depends
> on the length of the respective certificate chains, but the dominant cost will
> likely be the RSA signature verification (so count up how many
> VerifySignedData() calls each does for an approximate cost model).
> 
> If it turns out that performance of VerifyDeviceCert() is too slow, we may
want
> to consider broader caching not just for CRL part, but for the target
> certificate too, so a more general cache may be what we want instead.
> 
> Assuming we do decide on a cache, my first thought is to introduce a sort of
> "context" object, that encapsulates the trust store + caches. Instead of
passing
> in a custom trust store as we do today, tests would pass in a custom "context"
> object instead (which contains the trust store).
> 
> The default context when unspecified could be a global (much how the default
> trust store is a global today). If we don't anticipate multi-threaded callers,
> then adding some assertions to ensure the entry points (VerifyDeviceCert())
> aren't called on other threads should be sufficient.
> 
> I dunno, just thinking out loud.

I agree. Thanks for the insights. I under-estimated the complexity of adding a
cache. Will remove the cache for now and re-evaluate if it is needed.

This code is on a critical path. The device must be authenticated before casting
can begin. My feeling is that the cost is negligible considering the performance
of any modern computer. Maybe I should do some benchmarks to check. However,
there are such a large range of computers out there I'm not sure what threshold
would be considered too slow.

We also have metrics for time-to-launch Cast session, so I supposed that can be
monitored.

[eroman, 2016-09-14 23:47:46]

Sounds good!

In the future Matt and I will be spending some time profiling and optimizing the
cert verification code, and we will be able to advise better on this matter.

I am kind of just giving you hand-wavy answers for now :)

[ryanchung, 2016-09-16 17:55:53]

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

[ryanchung, 2016-09-16 17:55:54]

+mfoltz@

This CL adds certificate revocation checking to the Chrome sender's cast channel
device authentication.

The existing device authentication process involves verifying the chain of the
Cast certificate.
This CL adds the step of verifying that a Certificate Revocation List (CRL) is
issued by Cast (verify issuer certificate chain and signature of CRL). Then it
checks if any of the certificates in the Cast certificate chain is in the list
of revoked certificates.

The CRL itself is passed by the receiver during device authentication so it does
not need to be fetched by the Cast sender. There is no fetching step, only a
processing step. So, the revocation check is expected to cost around half of the
overall device certificate verification process. Considering the frequency of
browsers verifying certificates for SSL connections, this shouldn't be much of a
problem for any modern computer.

Do you see any concerns (performance or otherwise)? Would it be necessary to
benchmark this formally or can we just monitor the "start session" metrics?

Thanks.

[mark a. foltz, 2016-09-16 23:08:31]

mfoltz@chromium.org changed reviewers:
- mfoltz@chromium.org

[mark a. foltz, 2016-09-16 23:09:41]

mfoltz@chromium.org changed reviewers:
+ mfoltz@chromium.org

[mark a. foltz, 2016-09-16 23:09:42]

vadimgo@ and zivh@ own the code that use this API.  I suggest getting their
feedback.  If this is affecting connection logic I suggest rolling this out
conservatively (i.e. dev channel only) and watching the UMA on connection
success rates.

[ryanchung, 2016-09-16 23:15:07]

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

[eroman, 2016-09-17 00:44:41]

looks good, but will do a final review pass on next patchset before signing off

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

https://codereview.chromium.org/2303673004/diff/160001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:105: // For production use
pass |trust_store| as nullptr to use the production trust
This comment disagrees with above comment (and implementation).

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

https://codereview.chromium.org/2303673004/diff/160001/components/cast_certif...
components/cast_certificate/cast_cert_validator_test_helpers.cc:91:
net::ParsedCertificate::CreateFromCertificateCopy(trusted_root, {}));
You will need to sync -- this has been renamed to ParsedCertificate::Create().
The suggested pattern for use is:

net::CertErrors errors;
scoped_refptr<net::ParsedCertificate> cert =
    net::ParsedCertificate::Create(trusted_root, {}, &errors);
EXPECT_TRUE(cert) << errors.ToDebugString();

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

https://codereview.chromium.org/2303673004/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.h:59: // For production use pass
|trust_store| as nullptr to use the production trust
This doesn't agree with comment above.

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:33: // This flag tracks
the changes necesary to fully enforce revocation.
typo: necessary

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:162: const base::Time&
cert_verification_time,
Are two timestamps necessary?

I could only see this being applicable for test scenarios, but there are other
ways to construct such tests (i.e. modulate the CRL response timestamp rather
than the timestamp here).

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:206:
cast_crypto::VerifyDeviceCertUsingCustomTrustStore(
I may have been wrong when I suggested having a rigid distinction between
default production trust store, and dependency injected trust store for testing.
My concern was ensuring callers can't pass in nullptr by way of a failure, and
magically get an unintended trust store dependency.

But having seen all the if-else cases that have rippled through the code, I
think your original approach of allowing nullptr to mean "use the default" was
better. Apologies for the churn, my mistake!

Feel free to go back to allowing nullptr and cleaning up all these callsites to
just pass through the (optionally null) trust store.

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:251: if
(!kEnforceRevocationCheck) {
[optional] Not sure on your next steps for rollout-deployment, but you may want
to use a base::Feature for ease of configuring this (feel free to worry about
this in a future CL).

[ryanchung, 2016-09-22 21:38:52]

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

https://codereview.chromium.org/2303673004/diff/160001/components/cast_certif...
components/cast_certificate/cast_cert_validator.h:105: // For production use
pass |trust_store| as nullptr to use the production trust
On 2016/09/17 00:44:41, eroman wrote:
> This comment disagrees with above comment (and implementation).

Done. Allowing nullptr to be passed.

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

https://codereview.chromium.org/2303673004/diff/160001/components/cast_certif...
components/cast_certificate/cast_cert_validator_test_helpers.cc:91:
net::ParsedCertificate::CreateFromCertificateCopy(trusted_root, {}));
On 2016/09/17 00:44:41, eroman wrote:
> You will need to sync -- this has been renamed to ParsedCertificate::Create().
> The suggested pattern for use is:
> 
> net::CertErrors errors;
> scoped_refptr<net::ParsedCertificate> cert =
>     net::ParsedCertificate::Create(trusted_root, {}, &errors);
> EXPECT_TRUE(cert) << errors.ToDebugString();

Done. Thanks.

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

https://codereview.chromium.org/2303673004/diff/160001/components/cast_certif...
components/cast_certificate/cast_crl.h:59: // For production use pass
|trust_store| as nullptr to use the production trust
On 2016/09/17 00:44:41, eroman wrote:
> This doesn't agree with comment above.

Done. Allowing nullptr to be passed.

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:33: // This flag tracks
the changes necesary to fully enforce revocation.
On 2016/09/17 00:44:41, eroman wrote:
> typo: necessary

Done.

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:162: const base::Time&
cert_verification_time,
On 2016/09/17 00:44:41, eroman wrote:
> Are two timestamps necessary?
> 
> I could only see this being applicable for test scenarios, but there are other
> ways to construct such tests (i.e. modulate the CRL response timestamp rather
> than the timestamp here).

This mainly for use of a single test case. Every other test case uses the same
time for verifying the certificate and CRL.

The reason is that the CRL can be verified at a different time from when the
certificate is verified. The test case that exercises this condition verifies
the CRL when it was valid but tries to verify the certificate when the CRL has
expired.

However, given we're not using the cache anymore, the two will practically be
verified at the same time. Maybe we should just omit that test here? WDYT?

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:206:
cast_crypto::VerifyDeviceCertUsingCustomTrustStore(
On 2016/09/17 00:44:41, eroman wrote:
> I may have been wrong when I suggested having a rigid distinction between
> default production trust store, and dependency injected trust store for
testing.
> My concern was ensuring callers can't pass in nullptr by way of a failure, and
> magically get an unintended trust store dependency.
> 
> But having seen all the if-else cases that have rippled through the code, I
> think your original approach of allowing nullptr to mean "use the default" was
> better. Apologies for the churn, my mistake!
> 
> Feel free to go back to allowing nullptr and cleaning up all these callsites
to
> just pass through the (optionally null) trust store.

Done.

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:251: if
(!kEnforceRevocationCheck) {
On 2016/09/17 00:44:41, eroman wrote:
> [optional] Not sure on your next steps for rollout-deployment, but you may
want
> to use a base::Feature for ease of configuring this (feel free to worry about
> this in a future CL).

We should roll-out to users in stages and watch the metrics data as Mark
suggested.
Thanks for letting me know about base::Feature. I'll look at how that works and
incorporate that in a future CL.

[eroman, 2016-09-22 22:02:51]

lgtm

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:162: const base::Time&
cert_verification_time,
On 2016/09/22 21:38:51, ryanchung wrote:
> On 2016/09/17 00:44:41, eroman wrote:
> > Are two timestamps necessary?
> > 
> > I could only see this being applicable for test scenarios, but there are
other
> > ways to construct such tests (i.e. modulate the CRL response timestamp
rather
> > than the timestamp here).
> 
> This mainly for use of a single test case. Every other test case uses the same
> time for verifying the certificate and CRL.
> 
> The reason is that the CRL can be verified at a different time from when the
> certificate is verified. The test case that exercises this condition verifies
> the CRL when it was valid but tries to verify the certificate when the CRL has
> expired.
> 
> However, given we're not using the cache anymore, the two will practically be
> verified at the same time. Maybe we should just omit that test here? WDYT?

I would omit this for now.

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

https://codereview.chromium.org/2303673004/diff/220001/components/cast_certif...
components/cast_certificate/cast_cert_validator.cc:259: bool
VerifyDeviceCert(const std::vector<std::string>& certs,
Can you remove this overload of VerifyDeviceCert and instead move this code into
VerifyDeviceCertUsingCustomTrustStore() ?

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

https://codereview.chromium.org/2303673004/diff/220001/components/cast_certif...
components/cast_certificate/cast_crl.cc:321: std::unique_ptr<CastCRL>
ParseAndVerifyCRL(const std::string& crl_proto,
Same thing here -- how about just moving this into
ParseAndVerifyCRLUsingCustomTrustStore

https://codereview.chromium.org/2303673004/diff/220001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util_unittest.cc (right):

https://codereview.chromium.org/2303673004/diff/220001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:189: break;
nit: I suggest omitting the break when there is already a "return.

[ryanchung, 2016-09-22 22:43:35]

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/160001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:162: const base::Time&
cert_verification_time,
On 2016/09/22 22:02:50, eroman wrote:
> On 2016/09/22 21:38:51, ryanchung wrote:
> > On 2016/09/17 00:44:41, eroman wrote:
> > > Are two timestamps necessary?
> > > 
> > > I could only see this being applicable for test scenarios, but there are
> other
> > > ways to construct such tests (i.e. modulate the CRL response timestamp
> rather
> > > than the timestamp here).
> > 
> > This mainly for use of a single test case. Every other test case uses the
same
> > time for verifying the certificate and CRL.
> > 
> > The reason is that the CRL can be verified at a different time from when the
> > certificate is verified. The test case that exercises this condition
verifies
> > the CRL when it was valid but tries to verify the certificate when the CRL
has
> > expired.
> > 
> > However, given we're not using the cache anymore, the two will practically
be
> > verified at the same time. Maybe we should just omit that test here? WDYT?
> 
> I would omit this for now.

Done.

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

https://codereview.chromium.org/2303673004/diff/220001/components/cast_certif...
components/cast_certificate/cast_cert_validator.cc:259: bool
VerifyDeviceCert(const std::vector<std::string>& certs,
On 2016/09/22 22:02:50, eroman wrote:
> Can you remove this overload of VerifyDeviceCert and instead move this code
into
> VerifyDeviceCertUsingCustomTrustStore() ?

Done.

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

https://codereview.chromium.org/2303673004/diff/220001/components/cast_certif...
components/cast_certificate/cast_crl.cc:321: std::unique_ptr<CastCRL>
ParseAndVerifyCRL(const std::string& crl_proto,
On 2016/09/22 22:02:51, eroman wrote:
> Same thing here -- how about just moving this into
> ParseAndVerifyCRLUsingCustomTrustStore

Done.

https://codereview.chromium.org/2303673004/diff/220001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util_unittest.cc (right):

https://codereview.chromium.org/2303673004/diff/220001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util_unittest.cc:189: break;
On 2016/09/22 22:02:51, eroman wrote:
> nit: I suggest omitting the break when there is already a "return.

Done. Removed breaks here and in cast_crl_unittest.cc

[ryanchung, 2016-09-22 22:45:01]

Description was changed from

==========
Hook up Chrome Cast sender to Cast CRL.

* Hook up cast_auth_util to Cast CRL.
* Revocation status is not enforced at the moment.
* Allows custom trust store to be passed to VerifyDeviceCert and
ParseAndVerifyCRL. This is so that testing the Chrome Cast sender can use test
roots.
* Running the whole proto-test-suite on the Chrome Cast sender.

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

to

==========
Hook up Chrome Cast sender to Cast CRL.

* Hook up cast_auth_util to Cast CRL.
* Revocation status is not enforced at the moment.
* Allows custom trust store to be passed to VerifyDeviceCert and
ParseAndVerifyCRL. This is so that testing the Chrome Cast sender can use test
roots.
* Running the whole proto-test-suite on the Chrome Cast sender.

Sync-up of the following with internal:
* test_suite.proto
* cast_channel.proto

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

[ryanchung, 2016-09-22 23:05:26]

+vadimgo@ for cast channel review.

Thanks.

[vadimgo, 2016-10-04 22:30:11]

lgtm

[ryanchung, 2016-10-04 22:40:47]

mfoltz@ for cast_channel owners approval please.

I will make a follow up CL for adding base::Feature to support staged roll-out. 

I will also make another CL for collecting some metrics information. Enforcement
of the revocation feature is currently disabled but the code path can still be
traversed and results recorded.

[mark a. foltz, 2016-10-08 04:44:33]

If I'm reading this correctly, there's no backwards compatibility for devices
that don't provide a CRL in their authentication response.  Is that intended?

https://codereview.chromium.org/2303673004/diff/240001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/240001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:35: // is rolled out.
Instead of a hard coded constant you'll probably want to convert this to a flag
plus a Finch experiment.  That way you can test with this feature enabled and
disabled, and roll it out across the user population at a specific time (instead
of by release milestone).

[ryanchung, 2016-10-11 17:15:16]

On 2016/10/08 04:44:33, mark a. foltz wrote:
> If I'm reading this correctly, there's no backwards compatibility for devices
> that don't provide a CRL in their authentication response.  Is that intended?

Backwards compatibility is in the form of turning off enforcement of the
revocation results. The code will check the revocation status regardless.

It is sufficient to classify together receivers that don't provide a CRL and
those that are explicitly revoked because a malicious receiver will definitely
not pass along an unfavorable CRL.

In patchset 14&15, I've added metrics for revocation status reporting. Even
though enforcement will be off at first, the results can still be used to
evaluate the effectiveness of the check.

https://codereview.chromium.org/2303673004/diff/240001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/240001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:35: // is rolled out.
On 2016/10/08 04:44:33, mark a. foltz wrote:
> Instead of a hard coded constant you'll probably want to convert this to a
flag
> plus a Finch experiment.  That way you can test with this feature enabled and
> disabled, and roll it out across the user population at a specific time
(instead
> of by release milestone).

Sounds good. I've added a feature flag here.

I will write up a plan for a Finch experiment. We can't really run the
experiment until all the Cast devices have been updated to support this.

[mark a. foltz, 2016-10-11 22:12:16]

LGTM

+btolsch as an FYI.  This is another set of conditions that may cause
connections to Cast devices to start failing (once enforced).

It looks like they will be surfaced through the channel logging mechanism, and
thus through the error reporting in the extension API, e.g.
chrome.cast.channel.ErrorInfo.

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

https://codereview.chromium.org/2303673004/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:324: &CastCRLTrustStore::Get());
This can never evaluate to nullptr, right?

[ryanchung, 2016-10-11 22:59:19]

I forgot to update logger.cc
It's been updated in patchset 16 so that the auth errors get propagated
properly.

Thanks.

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

https://codereview.chromium.org/2303673004/diff/280001/components/cast_certif...
components/cast_certificate/cast_crl.cc:324: &CastCRLTrustStore::Get());
On 2016/10/11 22:12:16, mark a. foltz wrote:
> This can never evaluate to nullptr, right?

This can never be nullptr.

[ryanchung, 2016-10-14 21:58:17]

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

[ryanchung, 2016-10-14 21:58:18]

+asargent
Please review /extensions/browser/BUILD.gn

+jwd
Please review histograms.

Thanks you!

[mark a. foltz, 2016-10-17 17:53:39]

Thanks for updating the logging.  Still LGTM.

[asargent_no_longer_on_chrome, 2016-10-17 20:48:24]

lgtm

[jwd, 2016-10-17 20:55:11]

LGTM with nit.

https://codereview.chromium.org/2303673004/diff/300001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/300001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:78: // Must match with
histogram.
Nit: mention it they shouldn't be reordered.

[ryanchung, 2016-10-17 23:58:56]

https://codereview.chromium.org/2303673004/diff/300001/extensions/browser/api...
File extensions/browser/api/cast_channel/cast_auth_util.cc (right):

https://codereview.chromium.org/2303673004/diff/300001/extensions/browser/api...
extensions/browser/api/cast_channel/cast_auth_util.cc:78: // Must match with
histogram.
On 2016/10/17 20:55:11, jwd wrote:
> Nit: mention it they shouldn't be reordered.

Done.

[ryanchung, 2016-10-17 23:59:15]

The CQ bit was checked by ryanchung@chromium.org

[ryanchung, 2016-10-17 23:59:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
sheretov@chromium.org, eroman@chromium.org, vadimgo@chromium.org,
mfoltz@chromium.org, asargent@chromium.org, jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2303673004/#ps320001
(title: "Fixed nit")

[commit-bot: I haz the power, 2016-10-18 00:00:24]

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-10-18 02:58:04]

Description was changed from

==========
Hook up Chrome Cast sender to Cast CRL.

* Hook up cast_auth_util to Cast CRL.
* Revocation status is not enforced at the moment.
* Allows custom trust store to be passed to VerifyDeviceCert and
ParseAndVerifyCRL. This is so that testing the Chrome Cast sender can use test
roots.
* Running the whole proto-test-suite on the Chrome Cast sender.

Sync-up of the following with internal:
* test_suite.proto
* cast_channel.proto

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

to

==========
Hook up Chrome Cast sender to Cast CRL.

* Hook up cast_auth_util to Cast CRL.
* Revocation status is not enforced at the moment.
* Allows custom trust store to be passed to VerifyDeviceCert and
ParseAndVerifyCRL. This is so that testing the Chrome Cast sender can use test
roots.
* Running the whole proto-test-suite on the Chrome Cast sender.

Sync-up of the following with internal:
* test_suite.proto
* cast_channel.proto

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

[commit-bot: I haz the power, 2016-10-18 02:58:05]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-10-18 03:00:18]

Description was changed from

==========
Hook up Chrome Cast sender to Cast CRL.

* Hook up cast_auth_util to Cast CRL.
* Revocation status is not enforced at the moment.
* Allows custom trust store to be passed to VerifyDeviceCert and
ParseAndVerifyCRL. This is so that testing the Chrome Cast sender can use test
roots.
* Running the whole proto-test-suite on the Chrome Cast sender.

Sync-up of the following with internal:
* test_suite.proto
* cast_channel.proto

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

to

==========
Hook up Chrome Cast sender to Cast CRL.

* Hook up cast_auth_util to Cast CRL.
* Revocation status is not enforced at the moment.
* Allows custom trust store to be passed to VerifyDeviceCert and
ParseAndVerifyCRL. This is so that testing the Chrome Cast sender can use test
roots.
* Running the whole proto-test-suite on the Chrome Cast sender.

Sync-up of the following with internal:
* test_suite.proto
* cast_channel.proto

BUG=618463

Committed: https://crrev.com/559a29a14edcfdb03f5c952d0f6534b5bc4b7667
Cr-Commit-Position: refs/heads/master@{#425875}
==========

[commit-bot: I haz the power, 2016-10-18 03:00:20]

Patchset 17 (id:??) landed as
https://crrev.com/559a29a14edcfdb03f5c952d0f6534b5bc4b7667
Cr-Commit-Position: refs/heads/master@{#425875}