Cast device revocation checking.

components/cast_certificate/cast_cert_validator.cc

Index: components/cast_certificate/cast_cert_validator.cc
diff --git a/components/cast_certificate/cast_cert_validator.cc b/components/cast_certificate/cast_cert_validator.cc
index ec1fae2ee850d8a9b230ef0bb2cfc0b4d7debc6b..b39d7b49aec478ca71b75f156a6e70d820aedfdf 100644
--- a/components/cast_certificate/cast_cert_validator.cc
+++ b/components/cast_certificate/cast_cert_validator.cc
@@ -14,6 +14,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/singleton.h"
 #include "net/cert/internal/cert_issuer_source_static.h"
+#include "components/cast_certificate/cast_crl.h"
 #include "net/cert/internal/certificate_policies.h"
 #include "net/cert/internal/extended_key_usage.h"
 #include "net/cert/internal/parse_certificate.h"
@@ -267,7 +268,9 @@ net::ParseCertificateOptions GetCertParsingOptions() {
 bool VerifyDeviceCert(const std::vector<std::string>& certs,
                       const base::Time::Exploded& time,
                       std::unique_ptr<CertVerificationContext>* context,
-                      CastDeviceCertPolicy* policy) {
+                      CastDeviceCertPolicy* policy,
+                      const CastCRL* crl,
+                      CRLOptions& crl_options) {
   if (certs.empty())
     return false;
 
@@ -307,7 +310,22 @@ bool VerifyDeviceCert(const std::vector<std::string>& certs,
 
   // Check properties of the leaf certificate (key usage, policy), and construct
   // a CertVerificationContext that uses its public key.
-  return CheckTargetCertificate(target_cert.get(), context, policy);
+  if (!CheckTargetCertificate(target_cert.get(), context, policy))
+    return false;
+
+  // Check if a CRL is available.
+  if (!crl) {
+    if (crl_options.crl_required) {
+      return false;
+    }
+    return true;

[eroman, 2016/07/12 21:21:59]

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.

[ryanchung, 2016/07/14 16:15:25]

Done. Thanks!

+  }
+  if (result.paths.empty() ||
+      !result.paths[result.best_result_index]->is_success())
+    return false;
+
+  return crl->CheckRevocation(result.paths[result.best_result_index]->path,
+                              time);
 }
 
 std::unique_ptr<CertVerificationContext> CertVerificationContextImplForTest(
@@ -318,13 +336,14 @@ std::unique_ptr<CertVerificationContext> CertVerificationContextImplForTest(
       new CertVerificationContextImpl(net::der::Input(spki), "CommonName"));
 }
 
-bool AddTrustAnchorForTest(const uint8_t* data, size_t length) {
+bool SetTrustAnchorForTest(const uint8_t* data, size_t length) {
   scoped_refptr<net::ParsedCertificate> anchor(
       net::ParsedCertificate::CreateFromCertificateData(
           data, length, net::ParsedCertificate::DataSource::EXTERNAL_REFERENCE,
           GetCertParsingOptions()));
   if (!anchor)
     return false;
+  CastTrustStore::Get().Clear();

[eroman, 2016/07/12 21:22:00]

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).

[ryanchung, 2016/07/14 16:15:24]

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?

[eroman, 2016/07/15 22:52:48]

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.

[ryanchung, 2016/07/18 23:39:07]

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

   CastTrustStore::Get().AddTrustedCertificate(std::move(anchor));
   return true;
 }