Cast device revocation checking.

components/cast_certificate/cast_cert_validator.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/cast_certificate/cast_cert_validator.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 
 #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"
 #include "net/cert/internal/parse_name.h"
 #include "net/cert/internal/parsed_certificate.h"
 #include "net/cert/internal/path_builder.h"
 #include "net/cert/internal/signature_algorithm.h"
 #include "net/cert/internal/signature_policy.h"
 #include "net/cert/internal/trust_store.h"
 #include "net/cert/internal/verify_signed_data.h"
@@ skipping 233 matching lines @@
   // necessary. Should revisit this for removal in 2017 if not earlier.
   options.allow_invalid_serial_numbers = true;
   return options;
 }
 
 }  // namespace
 
 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;
 
   // No reference to these ParsedCertificates is kept past the end of this
   // function, so using EXTERNAL_REFERENCE here is safe.
   scoped_refptr<net::ParsedCertificate> target_cert;
   net::CertIssuerSourceStatic intermediate_cert_issuer_source;
   for (size_t i = 0; i < certs.size(); ++i) {
     scoped_refptr<net::ParsedCertificate> cert(
         net::ParsedCertificate::CreateFromCertificateData(
@@ skipping 19 matching lines @@
                                     signature_policy.get(),
                                     ConvertExplodedTime(time), &result);
   path_builder.AddCertIssuerSource(&intermediate_cert_issuer_source);
   net::CompletionStatus rv = path_builder.Run(base::Closure());
   DCHECK_EQ(rv, net::CompletionStatus::SYNC);
   if (!result.is_success())
     return false;
 
   // 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(
     const base::StringPiece& spki) {
   // Use a bogus CommonName, since this is just exposed for testing signature
   // verification by unittests.
   return base::WrapUnique(
       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;
 }
 
 }  // namespace cast_certificate