[Remoting Host] Select Latest Valid Cert

remoting/host/token_validator_base.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 "remoting/host/token_validator_base.h"
 
 #include <stddef.h>
 
 #include "base/base64.h"
 #include "base/bind.h"
@@ skipping 25 matching lines @@
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_status.h"
 #include "url/gurl.h"
 
 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 {
 
 TokenValidatorBase::TokenValidatorBase(
     const ThirdPartyAuthConfig& third_party_auth_config,
     const std::string& token_scope,
     scoped_refptr<net::URLRequestContextGetter> request_context_getter)
     : third_party_auth_config_(third_party_auth_config),
       token_scope_(token_scope),
@@ skipping 111 matching lines @@
       base::Bind(&TokenValidatorBase::OnCertificatesSelected,
                  weak_factory_.GetWeakPtr(), base::Owned(selected_certs),
                  base::Owned(client_cert_store)));
 }
 
 void TokenValidatorBase::OnCertificatesSelected(
     net::CertificateList* selected_certs,
     net::ClientCertStore* unused) {
   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);
   }
 }
 
 bool TokenValidatorBase::IsValidScope(const std::string& token_scope) {
   // TODO(rmsousa): Deal with reordering/subsets/supersets/aliases/etc.
   return token_scope == token_scope_;
 }
 
 std::string TokenValidatorBase::ProcessResponse(int net_result) {
   // Verify that we got a successful response.
@@ skipping 25 matching lines @@
     return std::string();
   }
 
   std::string shared_secret;
   // Everything is valid, so return the shared secret to the caller.
   dict->GetStringWithoutPathExpansion("access_token", &shared_secret);
   return shared_secret;
 }
 
 }  // namespace remoting