Hook up Chrome Cast sender to Cast CRL.

extensions/browser/api/cast_channel/cast_auth_util.cc

Index: extensions/browser/api/cast_channel/cast_auth_util.cc
diff --git a/extensions/browser/api/cast_channel/cast_auth_util.cc b/extensions/browser/api/cast_channel/cast_auth_util.cc
index 0e5c38844cbe1e84e1e8210a4c4bbd56a9ac34ea..01ce6712f8835cac4be774286c21f8da6fb2e6eb 100644
--- a/extensions/browser/api/cast_channel/cast_auth_util.cc
+++ b/extensions/browser/api/cast_channel/cast_auth_util.cc
@@ -11,6 +11,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "components/cast_certificate/cast_cert_validator.h"
+#include "components/cast_certificate/cast_crl.h"
 #include "extensions/browser/api/cast_channel/cast_message_util.h"
 #include "extensions/common/api/cast_channel/cast_channel.pb.h"
 #include "net/cert/x509_certificate.h"
@@ -26,6 +27,14 @@ const char* const kParseErrorPrefix = "Failed to parse auth message: ";
 // The maximum number of days a cert can live for.
 const int kMaxSelfSignedCertLifetimeInDays = 4;
 
+// Enforce certificate revocation when set to true.
+// If set to false, then any revocation failures are ignored.
+//
+// This flag tracks the changes necesary to fully enforce revocation.

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

typo: necessary

[ryanchung, 2016/09/22 21:38:51]

Done.

+// Flip this flag to true when the Cast certificate revocation feature
+// is rolled out.
+const bool kEnforceRevocationCheck = false;
+
 namespace cast_crypto = ::cast_certificate;
 
 // Extracts an embedded DeviceAuthMessage payload from an auth challenge reply
@@ -130,13 +139,28 @@ AuthResult AuthenticateChallengeReply(const CastMessage& challenge_reply,
 //
 // * Verifies that the certificate chain |response.client_auth_certificate| +
 //   |response.intermediate_certificate| is valid and chains to a trusted
-//   Cast root.
+//   Cast root. The list of trusted Cast roots can be overrided by providing a
+//   non-nullptr |cast_trust_store|. The certificate is verified at
+//   |cert_verification_time|.
+//
+// * Verifies that none of the certificates in the chain are revoked based on
+//   the CRL provided in the response |response.crl|. The CRL is verified to be
+//   valid and its issuer certificate chains to a trusted Cast CRL root. The
+//   list of trusted Cast CRL roots can be overrided by providing a non-nullptr
+//   |crl_trust_store|. If |crl_policy| is CRL_OPTIONAL then the result of
+//   revocation checking is ignored. The CRL is verified at
+//   |crl_verification_time|.
 //
 // * Verifies that |response.signature| matches the signature
 //   of |signature_input| by |response.client_auth_certificate|'s public
 //   key.
-AuthResult VerifyCredentials(const AuthResponse& response,
-                             const std::string& signature_input) {
+AuthResult VerifyCredentialsImpl(const AuthResponse& response,
+                                 const std::string& signature_input,
+                                 const cast_crypto::CRLPolicy& crl_policy,
+                                 net::TrustStore* cast_trust_store,
+                                 net::TrustStore* crl_trust_store,
+                                 const base::Time& cert_verification_time,

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

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

[ryanchung, 2016/09/22 21:38:51]

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?

[eroman, 2016/09/22 22:02:50]

I would omit this for now.

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

Done.

+                                 const base::Time& crl_verification_time) {
   // Verify the certificate
   std::unique_ptr<cast_crypto::CertVerificationContext> verification_context;
 
@@ -147,19 +171,58 @@ AuthResult VerifyCredentials(const AuthResponse& response,
                     response.intermediate_certificate().begin(),
                     response.intermediate_certificate().end());
 
-  // Use the current time when checking certificate validity.
-  base::Time now = base::Time::Now();
+  // Parse the CRL.
+  std::unique_ptr<cast_crypto::CastCRL> crl;
+  if (crl_trust_store) {
+    crl = cast_crypto::ParseAndVerifyCRLUsingCustomTrustStore(
+        response.crl(), crl_verification_time, crl_trust_store);
+  } else {
+    crl = cast_crypto::ParseAndVerifyCRL(response.crl(), crl_verification_time);
+  }
+  if (!crl && crl_policy == cast_crypto::CRLPolicy::CRL_REQUIRED) {
+    // CRL is invalid.
+    return AuthResult("Failed verifying Cast CRL.",
+                      AuthResult::ERROR_CRL_INVALID);
+  }
 
-  // CRL should not be enforced until it is served.
   cast_crypto::CastDeviceCertPolicy device_policy;
-  if (!cast_crypto::VerifyDeviceCert(
-          cert_chain, now, &verification_context, &device_policy, nullptr,
-          cast_certificate::CRLPolicy::CRL_OPTIONAL)) {
-    // TODO(eroman): The error information was lost; this error is ambiguous.
-    return AuthResult("Failed verifying cast device certificate",
-                      AuthResult::ERROR_CERT_NOT_SIGNED_BY_TRUSTED_CA);
+  bool verification_success;
+  if (cast_trust_store) {
+    verification_success = cast_crypto::VerifyDeviceCertUsingCustomTrustStore(
+        cert_chain, cert_verification_time, &verification_context,
+        &device_policy, crl.get(), crl_policy, cast_trust_store);
+  } else {
+    verification_success = cast_crypto::VerifyDeviceCert(
+        cert_chain, cert_verification_time, &verification_context,
+        &device_policy, crl.get(), crl_policy);
+  }
+  if (!verification_success) {
+    // TODO(ryanchung): Once this feature is completely rolled-out, remove the
+    // reverification step and use error reporting to get verification errors
+    // for metrics.
+    bool verification_no_crl_success;
+    if (cast_trust_store) {
+      verification_no_crl_success =
+          cast_crypto::VerifyDeviceCertUsingCustomTrustStore(

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

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.

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

Done.

+              cert_chain, cert_verification_time, &verification_context,
+              &device_policy, nullptr, cast_crypto::CRLPolicy::CRL_OPTIONAL,
+              cast_trust_store);
+    } else {
+      verification_no_crl_success = cast_crypto::VerifyDeviceCert(
+          cert_chain, cert_verification_time, &verification_context,
+          &device_policy, nullptr, cast_crypto::CRLPolicy::CRL_OPTIONAL);
+    }
+    if (!verification_no_crl_success) {
+      // TODO(eroman): The error information was lost; this error is ambiguous.
+      return AuthResult("Failed verifying cast device certificate",
+                        AuthResult::ERROR_CERT_NOT_SIGNED_BY_TRUSTED_CA);
+    }
+    if (crl_policy == cast_crypto::CRLPolicy::CRL_REQUIRED) {
+      // Device is revoked.
+      return AuthResult("Failed certificate revocation check.",
+                        AuthResult::ERROR_CERT_REVOKED);
+    }
   }
-
   if (!verification_context->VerifySignatureOverData(response.signature(),
                                                      signature_input)) {
     return AuthResult("Failed verifying signature over data",
@@ -181,6 +244,29 @@ AuthResult VerifyCredentials(const AuthResponse& response,
   return success;
 }
 
+AuthResult VerifyCredentials(const AuthResponse& response,
+                             const std::string& signature_input) {
+  base::Time now = base::Time::Now();
+  cast_crypto::CRLPolicy policy = cast_crypto::CRLPolicy::CRL_REQUIRED;
+  if (!kEnforceRevocationCheck) {

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

[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:51]

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.

+    policy = cast_crypto::CRLPolicy::CRL_OPTIONAL;
+  }
+  return VerifyCredentialsImpl(response, signature_input, policy, nullptr,
+                               nullptr, now, now);
+}
+
+AuthResult VerifyCredentialsForTest(const AuthResponse& response,
+                                    const std::string& signature_input,
+                                    const cast_crypto::CRLPolicy& crl_policy,
+                                    net::TrustStore* cast_trust_store,
+                                    net::TrustStore* crl_trust_store,
+                                    const base::Time& cert_verification_time,
+                                    const base::Time& crl_verification_time) {
+  return VerifyCredentialsImpl(response, signature_input, crl_policy,
+                               cast_trust_store, crl_trust_store,
+                               cert_verification_time, crl_verification_time);
+}
+
 }  // namespace cast_channel
 }  // namespace api
 }  // namespace extensions