Certificate Transparency: Add STH Fetching capability.

components/certificate_transparency/log_proof_fetcher_unittest.cc

+// Copyright 2015 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 "components/certificate_transparency/log_proof_fetcher.h"
+
+#include <string>
+
+#include "components/safe_json/testing_json_parser.h"
+#include "net/base/net_errors.h"
+#include "net/base/network_delegate.h"
+#include "net/cert/signed_tree_head.h"
+#include "net/http/http_status_code.h"
+#include "net/test/ct_test_util.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_interceptor.h"
+#include "net/url_request/url_request_job.h"
+#include "net/url_request/url_request_test_job.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace certificate_transparency {
+
+namespace {
+
+const char kGetSTHHeaders[] =
+    "HTTP/1.1 200 OK\0"
+    "Content-Type: application/json; charset=ISO-8859-1\0"
+    "\0";
+
+const char kLogSchema[] = "https";
+const char kLogURL[] = "ct.log.example.com";
+const char kLogID[] = "some_id";
+
+class FetchSTHTestJob : public net::URLRequestTestJob {
+ public:
+  FetchSTHTestJob(const std::string& get_sth_data,
+                  net::URLRequest* request,
+                  net::NetworkDelegate* network_delegate)
+      : URLRequestTestJob(
+            request,
+            network_delegate,
+            std::string(kGetSTHHeaders, arraysize(kGetSTHHeaders)),
+            get_sth_data,
+            true),
+        async_io_(false),
+        response_code_(net::HTTP_OK) {}
+
+  void SetAsyncIO(bool async_io) { async_io_ = async_io; }

[mmenke, 2015/08/04 19:54:30]

set_asnyc_io

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+
+  void SetResponseCode(int response_code) { response_code_ = response_code; }
+
+  int GetResponseCode() const override { return response_code_; }

[mmenke, 2015/08/04 19:54:29]

By default, this will just be grabbed from the headers we set instead.  Having
this not match when the headers returns seems like a bad idea, so I suggest
getting rid of it and changing the header string instead.

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+
+ protected:
+  bool NextReadAsync() override {

[mmenke, 2015/08/04 19:54:29]

This can be private (parent classes can access private override methods)

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+    // Response with indication of async IO only once, otherwise the final
+    // Read would (incorrectly) be classified as async, causing the
+    // URLRequestJob to try reading another time and failing on a CHECK
+    // that the raw_read_buffer_ is not null.

[mmenke, 2015/08/04 19:54:30]

Why is raw read buffer NULL?  The final read of a request can normally be an
asynchronous 0-byte read.

Just want to know if there's a bug in the test fixture, in the new code, in
TestURLRequestJob, or elsewhere in net/

[Eran Messeri, 2015/08/05 13:10:52]

I'll follow up on that over email - still trying to track the call sequence that
leads to that.
As far as I can tell NotifyReadComplete is invoked a third time (after the
initial read() invokes the delegate's callback with 0 bytes indicating async IO
and the 2nd read invokes the delegate's callback with the actual data) if
NextReadAsync() returns true every time. 
If NextReadAsync() only returns true once then NotifyReadComplete is only
invoked twice.

+    if (async_io_) {
+      async_io_ = false;
+      return true;
+    }
+    return false;
+  }
+
+ private:
+  bool async_io_;
+  int response_code_;

[mmenke, 2015/08/04 19:54:30]

member variables go at the end, after all methods, including the destructor.

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+
+  ~FetchSTHTestJob() override {}
+  DISALLOW_COPY_AND_ASSIGN(FetchSTHTestJob);

[mmenke, 2015/08/04 19:54:30]

nit:  Blank line before DISALLOW_COPY_AND_ASSIGN

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+};
+
+class GetSTHResponseHandler : public net::URLRequestInterceptor {
+ public:
+  GetSTHResponseHandler()
+      : async_io_(false), response_data_(""), response_code_(net::HTTP_OK) {}
+  ~GetSTHResponseHandler() override {}
+
+  // URLRequestInterceptor implementation:
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    FetchSTHTestJob* job =
+        new FetchSTHTestJob(response_data_, request, network_delegate);
+    job->SetAsyncIO(async_io_);
+    job->SetResponseCode(response_code_);
+    return job;
+  }
+
+  void SetResponseData(std::string response_data) {

[mmenke, 2015/08/04 19:54:30]

set_response_data

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+    response_data_ = response_data;

[mmenke, 2015/08/04 19:54:30]

Suggest response_data -> response_body everywhere.  response_data is indeed what
URLRequestTestJob calls it, but it's much less clear than response_body, and
should probably be renamed in the test class as well, at some point.

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+  }
+
+  void SetAsyncIO(bool async_io) { async_io_ = async_io; }

[mmenke, 2015/08/04 19:54:30]

set_asnyc_io

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+
+  void SetResponseCode(int response_code) { response_code_ = response_code; }

[mmenke, 2015/08/04 19:54:30]

set_response_code (Though, per earlier comment, I think we should get rid of
this method)

[Eran Messeri, 2015/08/05 13:10:53]

Done.

+
+ private:
+  bool async_io_;
+  std::string response_data_;
+  int response_code_;
+
+  DISALLOW_COPY_AND_ASSIGN(GetSTHResponseHandler);
+};
+
+class RecordFetchCallbackInvocations {
+ public:
+  RecordFetchCallbackInvocations() : expect_success_(false), invoked_(false) {}
+
+  virtual void STHFetched(const std::string& log_id,

[mmenke, 2015/08/04 19:54:30]

virtual no longer needed

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+                          const net::ct::SignedTreeHead& sth) {

[mmenke, 2015/08/04 19:54:30]

ASSERT_FALSE(invoked_);

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+    ASSERT_TRUE(expect_success_);
+    invoked_ = true;
+    // If expected to succeed, expecting the known_good STH.
+    net::ct::SignedTreeHead known_sth;
+    net::ct::GetSampleSignedTreeHead(&known_sth);

[mmenke, 2015/08/04 19:54:30]

suggest known -> expected (More common for tests, and makes it completely clear
which is expected, and which is the you're testing)

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+
+    ASSERT_EQ(kLogID, log_id);
+    ASSERT_EQ(sth.version, known_sth.version);
+    ASSERT_EQ(sth.timestamp, known_sth.timestamp);
+    ASSERT_EQ(sth.tree_size, known_sth.tree_size);
+    ASSERT_STREQ(sth.sha256_root_hash, known_sth.sha256_root_hash);
+    ASSERT_EQ(sth.signature.hash_algorithm, known_sth.signature.hash_algorithm);
+    ASSERT_EQ(sth.signature.signature_algorithm,
+              known_sth.signature.signature_algorithm);
+    ASSERT_EQ(sth.signature.signature_data, known_sth.signature.signature_data);

[mmenke, 2015/08/04 19:54:30]

For all of these, expected should be first.

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+  }
+
+  void FetchingFailed(const std::string& log_id,
+                      int net_error,
+                      int http_response_code) {

[mmenke, 2015/08/04 19:54:30]

ASSERT_FALSE(invoked_);

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+    ASSERT_FALSE(expect_success_);
+    invoked_ = true;
+    net_error_ = net_error;
+    http_response_code_ = http_response_code;
+    if (net_error_ == net::OK) {
+      ASSERT_NE(net::HTTP_OK, http_response_code_);
+    }
+  }
+
+  bool invoked() { return invoked_; }

[mmenke, 2015/08/04 19:54:30]

bool invoked() const ...

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+
+  int net_error() { return net_error_; }

[mmenke, 2015/08/04 19:54:30]

bool net_error() const ...

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+
+  int http_response_code() { return http_response_code_; }

[mmenke, 2015/08/04 19:54:30]

const

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+
+  void set_expect_success(bool expect_success) {
+    expect_success_ = expect_success;
+  }
+
+ private:
+  bool expect_success_;

[mmenke, 2015/08/04 19:54:30]

optional:  Suggest making this a constructor parameter, and making it const.

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+
+  bool invoked_;
+  int net_error_;
+  int http_response_code_;

[mmenke, 2015/08/04 19:54:30]

All of these should be initialized in the constructor.

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+};
+
+class LogProofFetcherTest : public ::testing::Test {
+ public:
+  LogProofFetcherTest()
+      : log_url_(std::string(kLogSchema) + "://" + std::string(kLogURL) + "/") {
+    scoped_ptr<GetSTHResponseHandler> handler(new GetSTHResponseHandler());
+    handler_ = handler.get();
+
+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        kLogSchema, kLogURL, handler.Pass());
+
+    fetcher_.reset(new LogProofFetcher(&context_));
+  }
+
+  ~LogProofFetcherTest() override {
+    net::URLRequestFilter::GetInstance()->RemoveHostnameHandler(kLogSchema,
+                                                                kLogURL);
+  }
+
+ protected:
+  void SetValidSTHJSONResponse() {
+    std::string sth_json_reply_data = net::ct::GetSampleSTHAsJson();
+    handler_->SetResponseData(sth_json_reply_data);
+  }
+
+  void RunFetcherWithCallback(RecordFetchCallbackInvocations* callback) {
+    fetcher_->FetchSignedTreeHead(
+        log_url_, kLogID,
+        base::Bind(&RecordFetchCallbackInvocations::STHFetched,
+                   base::Unretained(callback)),
+        base::Bind(&RecordFetchCallbackInvocations::FetchingFailed,
+                   base::Unretained(callback)));
+    message_loop_.RunUntilIdle();
+  }
+
+  base::MessageLoopForIO message_loop_;
+  net::TestURLRequestContext context_;
+  safe_json::TestingJsonParser::ScopedFactoryOverride factory_override_;
+  scoped_ptr<LogProofFetcher> fetcher_;
+  const GURL log_url_;
+  GetSTHResponseHandler* handler_;
+};
+
+TEST_F(LogProofFetcherTest, TestValidGetReply) {
+  SetValidSTHJSONResponse();
+
+  RecordFetchCallbackInvocations callback;
+  callback.set_expect_success(true);
+
+  RunFetcherWithCallback(&callback);
+
+  ASSERT_TRUE(callback.invoked());
+}
+
+TEST_F(LogProofFetcherTest, TestValidGetReplyAsyncIO) {
+  SetValidSTHJSONResponse();
+  handler_->SetAsyncIO(true);
+
+  RecordFetchCallbackInvocations callback;
+  callback.set_expect_success(true);
+  RunFetcherWithCallback(&callback);
+
+  ASSERT_TRUE(callback.invoked());
+}
+
+TEST_F(LogProofFetcherTest, TestInvalidGetReplyIncompleteJSON) {
+  std::string sth_json_reply_data = net::ct::CreateSignedTreeHeadJsonString(
+      21 /* tree_size */, 123456u /* timestamp */, std::string(""),
+      std::string(""));

[mmenke, 2015/08/04 19:54:30]

nit:  std::string() (x2)

[Eran Messeri, 2015/08/05 13:10:52]

Done.

+  handler_->SetResponseData(sth_json_reply_data);
+
+  RecordFetchCallbackInvocations callback;
+  callback.set_expect_success(false);
+  RunFetcherWithCallback(&callback);
+
+  ASSERT_TRUE(callback.invoked());
+  ASSERT_EQ(net::ERR_CT_STH_INCOMPLETE, callback.net_error());
+}
+
+TEST_F(LogProofFetcherTest, TestInvalidGetReplyInvalidJSON) {
+  std::string sth_json_reply_data = "{\"tree_size\":21,\"timestamp\":}";
+  handler_->SetResponseData(sth_json_reply_data);
+
+  RecordFetchCallbackInvocations callback;
+  callback.set_expect_success(false);
+  RunFetcherWithCallback(&callback);
+
+  ASSERT_TRUE(callback.invoked());
+  ASSERT_EQ(net::ERR_CT_STH_PARSING_FAILED, callback.net_error());
+}
+
+TEST_F(LogProofFetcherTest, TestLogReplyIsTooLong) {
+  std::string sth_json_reply_data = net::ct::GetSampleSTHAsJson();
+  // Add kMaxLogResponseSizeInBytes to make sure the response is too big.
+  sth_json_reply_data.append(std::string(kMaxLogResponseSizeInBytes, ' '));
+  handler_->SetResponseData(sth_json_reply_data);
+
+  RecordFetchCallbackInvocations callback;
+  callback.set_expect_success(false);
+  RunFetcherWithCallback(&callback);
+
+  ASSERT_TRUE(callback.invoked());
+  ASSERT_EQ(net::ERR_FILE_TOO_BIG, callback.net_error());
+  ASSERT_EQ(net::HTTP_OK, callback.http_response_code());
+}
+
+TEST_F(LogProofFetcherTest, TestLogReplyIsExactlyMaxSize) {
+  std::string sth_json_reply_data = net::ct::GetSampleSTHAsJson();
+  // Extend the reply to be exactly kMaxLogResponseSizeInBytes.
+  sth_json_reply_data.append(std::string(
+      kMaxLogResponseSizeInBytes - sth_json_reply_data.size(), ' '));
+  handler_->SetResponseData(sth_json_reply_data);
+
+  RecordFetchCallbackInvocations callback;
+  callback.set_expect_success(true);
+  RunFetcherWithCallback(&callback);
+
+  ASSERT_TRUE(callback.invoked());
+}
+
+TEST_F(LogProofFetcherTest, TestLogRepliesWithHttpError) {
+  handler_->SetResponseCode(net::HTTP_NOT_FOUND);
+
+  RecordFetchCallbackInvocations callback;
+  callback.set_expect_success(false);
+  RunFetcherWithCallback(&callback);
+
+  ASSERT_TRUE(callback.invoked());
+  ASSERT_EQ(net::OK, callback.net_error());
+  ASSERT_EQ(net::HTTP_NOT_FOUND, callback.http_response_code());
+}
+
+}  // namespace
+
+}  // namespace certificate_transparency