[Remoting Host] Select Latest Valid Cert

remoting/host/token_validator_base.cc

Index: remoting/host/token_validator_base.cc
diff --git a/remoting/host/token_validator_base.cc b/remoting/host/token_validator_base.cc
index 0160dd4e6892f8148e6df156e4ef020b9950db05..d43791752faec2489d8428cd6973246c011a5887 100644
--- a/remoting/host/token_validator_base.cc
+++ b/remoting/host/token_validator_base.cc
@@ -43,6 +43,22 @@ namespace {
 const int kBufferSize = 4096;
 const char kCertIssuerWildCard[] = "*";
 
+// Returns the best certificate to use. The returned value will only be null if

[Sergey Ulanov, 2016/09/27 00:40:06]

maybe explain what "best certificate" means.

[Yuwei, 2016/09/27 21:37:42]

Done.

+// both |c1| and |c2| are both null.
+net::X509Certificate* GetBestCertificate(net::X509Certificate* c1,

[Lambros, 2016/09/27 00:23:33]

I wonder if this could be written as a Predicate:
bool betterThan(cert* c1, cert* c2) {..}
?
And apply it with std::max_element over the list?

[Yuwei, 2016/09/27 21:37:41]

Done.

+                                         net::X509Certificate* c2) {
+  if (!c1) {
+    return c2;
+  }
+  if (!c2) {

[Sergey Ulanov, 2016/09/27 00:40:06]

I think this code would be simpler if it didn't have to deal with null
parameters. c2 is never null here. Also the case when c1 is null can be handled
in GetBestCertificate().

[Yuwei, 2016/09/27 21:37:41]

Obsolete. Now c1 and c2 will never be null.

+    return c1;
+  }
+  if (c1->valid_start() != c2->valid_start()) {

[Sergey Ulanov, 2016/09/27 00:40:06]

remove  {} for consistency with other single-line if statements in this file

[Yuwei, 2016/09/27 21:37:42]

Done.

+    return c1->valid_start() > c2->valid_start() ? c1 : c2;

[Sergey Ulanov, 2016/09/27 00:40:06]

Should we also verify that valid_start() is not in future?

[Yuwei, 2016/09/27 21:37:41]

Yep. Forgot that... Added the check.

+  }
+  return c1->valid_expiry() > c2->valid_expiry() ? c1 : c2;
+}
+
 }  // namespace
 
 namespace remoting {
@@ -175,16 +191,18 @@ void TokenValidatorBase::OnCertificatesSelected(
   const std::string& issuer =
       third_party_auth_config_.token_validation_cert_issuer;
   if (request_) {

[Sergey Ulanov, 2016/09/27 00:40:06]

if (!request)
  return;

[Yuwei, 2016/09/27 21:37:42]

Done.

-    for (size_t i = 0; i < selected_certs->size(); ++i) {
-      net::X509Certificate* cert = (*selected_certs)[i].get();
-      if (issuer == kCertIssuerWildCard ||
-          issuer == cert->issuer().common_name) {
-        request_->ContinueWithCertificate(
-            cert, net::FetchClientCertPrivateKey(cert).get());
-        return;
+    net::X509Certificate* best_match_cert = nullptr;
+    for (auto& cert : *selected_certs) {
+      if ((issuer == kCertIssuerWildCard ||
+           issuer == cert->issuer().common_name) &&
+          (!cert->HasExpired() || cert->valid_expiry().is_null())) {
+        best_match_cert =
+            GetBestCertificate(best_match_cert, cert.get());
       }
     }
-    request_->ContinueWithCertificate(nullptr, nullptr);
+    request_->ContinueWithCertificate(
+        best_match_cert, best_match_cert ?
+            net::FetchClientCertPrivateKey(best_match_cert).get() : nullptr);
   }
 }