Blimp: add support for SSL connections.

blimp/net/exact_match_cert_verifier.cc

+// Copyright 2016 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 "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "blimp/net/exact_match_cert_verifier.h"

[Ryan Sleevi, 2016/02/19 22:56:08]

STYLE: This should be at the top of the file, before any other includes

Please see
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

[Kevin M, 2016/02/22 22:53:32]

Done.

+#include "net/base/net_errors.h"
+#include "net/cert/cert_verifier.h"
+#include "net/cert/cert_verify_result.h"
+#include "net/cert/x509_certificate.h"
+
+namespace blimp {
+
+ExactMatchCertVerifier::ExactMatchCertVerifier(
+    scoped_refptr<net::X509Certificate> engine_cert)
+    : engine_cert_(engine_cert) {}

[Ryan Sleevi, 2016/02/19 22:56:08]

STYLE: You're passing a scoped_refptr<> as non-const, so you need to std::move()
this to avoid unnecessary refcount churn.

[Kevin M, 2016/02/22 22:53:31]

Done. I converted this parameter into a const reference.

[Ryan Sleevi, 2016/02/23 02:26:26]

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/hDMBe...

the std::move path is the recommended path :)

[Kevin M, 2016/02/23 20:26:46]

Done.

+
+ExactMatchCertVerifier::~ExactMatchCertVerifier() {}
+
+int ExactMatchCertVerifier::Verify(net::X509Certificate* cert,
+                                   const std::string&,
+                                   const std::string&,
+                                   int,
+                                   net::CRLSet*,
+                                   net::CertVerifyResult* verify_result,
+                                   const net::CompletionCallback&,
+                                   scoped_ptr<net::CertVerifier::Request>*,
+                                   const net::BoundNetLog&) {
+  verify_result->Reset();
+
+  if (!cert->Equals(engine_cert_.get())) {

[Ryan Sleevi, 2016/02/19 22:56:08]

BUG: You return an error code but fail to set the CertStatus of |verify_result|

[Kevin M, 2016/02/22 22:53:32]

Done.

+    return net::ERR_CERT_INVALID;
+  }
+
+  verify_result->verified_cert = cert;

[Ryan Sleevi, 2016/02/19 22:56:08]

POTENTIAL BUG: You fail to fill in public_key_hashes to match verified_cert.
Downstream consumers of CertVerifier generally expect this to be filled (the
exception is unit tests)

[Kevin M, 2016/02/22 22:53:31]

Done.

+  return net::OK;
+}
+
+}  // namespace blimp