Verify certificate of Privet v3 device

chrome/browser/local_discovery/privet_http_impl.cc

Index: chrome/browser/local_discovery/privet_http_impl.cc
diff --git a/chrome/browser/local_discovery/privet_http_impl.cc b/chrome/browser/local_discovery/privet_http_impl.cc
index dc3b4eff03d7e14d2d256f3af2bbb9d5dc5bd913..5833e12b0d921496b389eebb8de14b74d7e5a17f 100644
--- a/chrome/browser/local_discovery/privet_http_impl.cc
+++ b/chrome/browser/local_discovery/privet_http_impl.cc
@@ -15,8 +15,13 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/thread_task_runner_handle.h"
 #include "chrome/browser/local_discovery/privet_constants.h"
+#include "chrome/common/chrome_content_client.h"
 #include "chrome/common/cloud_print/cloud_print_constants.h"
 #include "net/base/url_util.h"
+#include "net/cert/cert_verifier.h"
+#include "net/cert/cert_verify_result.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_builder.h"
 #include "url/gurl.h"
 
 #if defined(ENABLE_PRINT_PREVIEW)
@@ -30,6 +35,7 @@
 namespace local_discovery {
 
 namespace {
+
 const char kUrlPlaceHolder[] = "http://host/";
 const char kPrivetRegisterActionArgName[] = "action";
 const char kPrivetRegisterUserArgName[] = "user";
@@ -81,6 +87,105 @@ GURL CreatePrivetParamURL(const std::string& path,
   return url.ReplaceComponents(replacements);
 }
 
+// Used before we have any certificate fingerprint.
+class FailingCertVerifier : public net::CertVerifier {
+ public:
+  int Verify(net::X509Certificate* cert,
+             const std::string& hostname,
+             const std::string& ocsp_response,
+             int flags,
+             net::CRLSet* crl_set,
+             net::CertVerifyResult* verify_result,
+             const net::CompletionCallback& callback,
+             scoped_ptr<Request>* out_req,
+             const net::BoundNetLog& net_log) override {
+    verify_result->Reset();
+    verify_result->verified_cert = cert;
+    verify_result->cert_status = net::CERT_STATUS_INVALID;
+    return net::ERR_CERT_INVALID;
+  }
+};
+
+// Class verifies certificate by its fingerprint received using different
+// channel. It's the only know information about device with self-signed
+// certificate.
+class FingerprintVerifier : public net::CertVerifier {
+ public:
+  explicit FingerprintVerifier(
+      const net::SHA256HashValue& certificate_fingerprint)
+      : certificate_fingerprint_(certificate_fingerprint) {}
+
+  int Verify(net::X509Certificate* cert,
+             const std::string& hostname,
+             const std::string& ocsp_response,
+             int flags,
+             net::CRLSet* crl_set,
+             net::CertVerifyResult* verify_result,
+             const net::CompletionCallback& callback,
+             scoped_ptr<Request>* out_req,
+             const net::BoundNetLog& net_log) override {
+    // Mark certificate as invalid as we didn't check it.
+    verify_result->Reset();
+    verify_result->verified_cert = cert;
+    verify_result->cert_status = net::CERT_STATUS_INVALID;
+
+    auto fingerprint =
+        net::X509Certificate::CalculateFingerprint256(cert->os_cert_handle());
+
+    return certificate_fingerprint_.Equals(fingerprint) ? net::OK
+                                                        : net::ERR_CERT_INVALID;
+  }
+
+ private:
+  net::SHA256HashValue certificate_fingerprint_;
+
+  DISALLOW_COPY_AND_ASSIGN(FingerprintVerifier);
+};
+
+class PrivetContextGetter : public net::URLRequestContextGetter {
+ public:
+  PrivetContextGetter(
+      const scoped_refptr<base::SingleThreadTaskRunner>& net_task_runner,
+      const net::SHA256HashValue& certificate_fingerprint)
+      : verifier_(new FingerprintVerifier(certificate_fingerprint)),
+        net_task_runner_(net_task_runner) {}
+
+  // Don't allow any https without fingerprint. Device with valid certificate
+  // may be different from the one which user is trying to pair.
+  explicit PrivetContextGetter(
+      const scoped_refptr<base::SingleThreadTaskRunner>& net_task_runner)
+      : verifier_(new FailingCertVerifier()),
+        net_task_runner_(net_task_runner) {}
+
+  net::URLRequestContext* GetURLRequestContext() override {
+    if (!context_) {
+      net::URLRequestContextBuilder builder;
+      builder.set_proxy_service(net::ProxyService::CreateDirect());
+      builder.SetSpdyAndQuicEnabled(false, false);
+      builder.DisableHttpCache();
+      builder.SetCertVerifier(verifier_.Pass());
+      builder.set_user_agent(::GetUserAgent());
+      context_ = builder.Build();
+    }
+    return context_.get();
+  }
+
+  scoped_refptr<base::SingleThreadTaskRunner> GetNetworkTaskRunner()
+      const override {
+    return net_task_runner_;
+  }
+
+ protected:
+  ~PrivetContextGetter() override = default;

[mmenke, 2015/10/29 16:12:26]

BUG:  The URLRequestContext must be deleted on the network thread.  I guess your
tests are just using one thread?  The URLRequestContext should have caught this,
otherwise.  Suggest adding a network thread to your tests, for just such issues.

[Vitaly Buka (NO REVIEWS), 2015/10/29 18:37:12]

That's not what I see from code:
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

context_ is deleted with PrivetContextGetter.
PrivetContextGetter is derived from net::URLRequestContextGetter which always
deletes itself on network thread.

Test uses real IO thread and main thread, adding
DCHECK(net_task_runner_->BelongsToCurrentThread()); does not trigger any issues

[mmenke, 2015/10/29 18:40:18]

You're right - I completely forgot about that magic (Now-a-days, none of the
primary contents are owned by their getters).

+
+ private:
+  scoped_ptr<net::CertVerifier> verifier_;
+  scoped_ptr<net::URLRequestContext> context_;
+  scoped_refptr<base::SingleThreadTaskRunner> net_task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrivetContextGetter);
+};
+
 }  // namespace
 
 PrivetInfoOperationImpl::PrivetInfoOperationImpl(
@@ -179,8 +284,8 @@ void PrivetRegisterOperationImpl::OnError(PrivetURLFetcher* fetcher,
     reason = FAILURE_MALFORMED_RESPONSE;
   } else if (error == PrivetURLFetcher::TOKEN_ERROR) {
     reason = FAILURE_TOKEN;
-  } else if (error == PrivetURLFetcher::RETRY_ERROR) {
-    reason = FAILURE_RETRY;
+  } else if (error == PrivetURLFetcher::UNKNOWN_ERROR) {
+    reason = FAILURE_UNKNOWN;
   }
 
   delegate_->OnPrivetRegisterError(this,
@@ -691,7 +796,17 @@ PrivetHTTPClientImpl::PrivetHTTPClientImpl(
     const std::string& name,
     const net::HostPortPair& host_port,
     net::URLRequestContextGetter* request_context)
-    : name_(name), request_context_(request_context), host_port_(host_port) {}
+    : PrivetHTTPClientImpl(name,
+                           host_port,
+                           request_context->GetNetworkTaskRunner()) {}
+
+PrivetHTTPClientImpl::PrivetHTTPClientImpl(
+    const std::string& name,
+    const net::HostPortPair& host_port,
+    const scoped_refptr<base::SingleThreadTaskRunner>& net_task_runner)
+    : name_(name),
+      context_getter_(new PrivetContextGetter(net_task_runner)),

[mmenke, 2015/10/29 16:12:26]

URLRequestContexts are very heavy weight objects.  What's the lifetime of this
object?  How often are they created?  It's not clear from the code.

[Vitaly Buka (NO REVIEWS), 2015/10/29 18:37:12]

For PrivetHTTPClient printing functionality this can be created many times when
we enumerate devices. But I don't need any custom context, browser context is
OK. I undo this part.

We need own context only to do HTTPS, and it's only happening during device
setup. This happens once per SPAKE2 pinCode typed by user. So quite rare.

+      host_port_(host_port) {}
 
 PrivetHTTPClientImpl::~PrivetHTTPClientImpl() {
 }
@@ -712,14 +827,13 @@ scoped_ptr<PrivetURLFetcher> PrivetHTTPClientImpl::CreateURLFetcher(
     PrivetURLFetcher::Delegate* delegate) {
   GURL::Replacements replacements;
   replacements.SetHostStr(host_port_.host());
-  std::string port(
-      base::UintToString(host_port_.port()));  // Keep string alive.
+  std::string port = base::UintToString(host_port_.port());
   replacements.SetPortStr(port);
+  std::string scheme = IsInHttpsMode() ? "https" : "http";
+  replacements.SetSchemeStr(scheme);
   return scoped_ptr<PrivetURLFetcher>(
-      new PrivetURLFetcher(url.ReplaceComponents(replacements),
-                           request_type,
-                           request_context_.get(),
-                           delegate));
+      new PrivetURLFetcher(url.ReplaceComponents(replacements), request_type,
+                           context_getter_, delegate));
 }
 
 void PrivetHTTPClientImpl::RefreshPrivetToken(
@@ -734,6 +848,19 @@ void PrivetHTTPClientImpl::RefreshPrivetToken(
   }
 }
 
+void PrivetHTTPClientImpl::SwitchToHttps(
+    uint16_t port,
+    const net::SHA256HashValue& certificate_fingerprint) {
+  use_https_ = true;
+  host_port_.set_port(port);
+  context_getter_ = new PrivetContextGetter(
+      context_getter_->GetNetworkTaskRunner(), certificate_fingerprint);
+}
+
+bool PrivetHTTPClientImpl::IsInHttpsMode() const {
+  return use_https_;
+}
+
 void PrivetHTTPClientImpl::OnPrivetInfoDone(
     const base::DictionaryValue* value) {
   info_operation_.reset();