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"

[mmenke, 2015/08/03 18:18:54]

This should go up a line

[Eran Messeri, 2015/08/04 16:15:43]

Done.

+
+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";
+
+std::string GetLogID() {
+  return std::string("some_id");
+}

[mmenke, 2015/08/03 18:18:54]

Can't this just be a const char[]?

[Eran Messeri, 2015/08/04 16:15:43]

Done.

+
+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),

[mmenke, 2015/08/03 18:18:54]

BUG:  This std::string only includes the first line of kGetSTHHeaders, due to
the NULLs.  I suggest using URLRequestMockHTTPJob instead.  You will need to put
the headers and response bodies in separate files, but the class is much easier
to use.

One downside of this is it doesn't let you test both sync an async responses.

[Eran Messeri, 2015/08/04 16:15:43]

Good point, I've passed the size to the std::string c'tor.
I don't think the added complexity of the file I/O performed by
URLRequestMockHTTPJob is justified in this case - the response returned by this
test job is quite small and I do want to test async responses.

+                          get_sth_data,
+                          true),
+        async_io_(false),
+        response_code_(net::HTTP_OK) {}
+
+  void SetAsyncIO(bool async_io) { async_io_ = async_io; }
+
+  void SetResponseCode(int response_code) { response_code_ = response_code; }
+
+  int GetResponseCode() const override { return response_code_; }
+
+ protected:
+  bool NextReadAsync() override {
+    // 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.
+    if (async_io_) {
+      async_io_ = false;
+      return true;
+    }
+    return false;
+  }
+
+ private:
+  bool async_io_;
+  int response_code_;
+
+  ~FetchSTHTestJob() override {}
+  DISALLOW_COPY_AND_ASSIGN(FetchSTHTestJob);
+};
+
+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) {
+    response_data_ = response_data;
+  }
+
+  void SetAsyncIO(bool async_io) { async_io_ = async_io; }
+
+  void SetResponseCode(int response_code) { response_code_ = response_code; }
+
+ private:
+  bool async_io_;
+  std::string response_data_;
+  int response_code_;
+
+  DISALLOW_COPY_AND_ASSIGN(GetSTHResponseHandler);
+};
+
+class RecordFetchCallbackInvocations {
+ public:
+  RecordFetchCallbackInvocations() : invoked_(false), failed_(false) {}
+
+  virtual void STHFetched(const std::string& log_id,
+                          const net::ct::SignedTreeHead& sth) {
+    invoked_ = true;
+  }
+
+  void FetchingFailed(const std::string& log_id,
+                      int net_error,
+                      int http_response_code) {
+    invoked_ = true;
+    failed_ = true;
+    net_error_ = net_error;
+    http_response_code_ = http_response_code;

[mmenke, 2015/08/03 18:18:54]

Suggest a sanity check here:

if (net_error_ == net::OK) {
  ASSERT_NE(net::WhateverTheConstantFor200Is, http_response_code_);
} else {
  ASSERT_EQ(net::WhateverTheConstantFor200Is, http_response_code_);
}

i.e. One and only one should indicate a failure.

[Eran Messeri, 2015/08/04 16:15:43]

Partly done - I've adopted the first clause, as it makes sense to to assert,
when recording a failure, that the error codes combined (net::OK & net::HTTP_OK)
do not indicate success.
However if the net_error is not net::OK I don't want to make any guarantees on
the http_response_code_ as the value there may not make sense at all.

+  }
+
+  bool invoked() { return invoked_; }
+
+  bool failed() { return failed_; }
+
+  int net_error() { return net_error_; }
+
+  int http_response_code() { return http_response_code_; }
+
+ private:
+  bool invoked_;
+  bool failed_;
+  int net_error_;
+  int http_response_code_;
+};
+
+class ExpectedSuccessCallback : public RecordFetchCallbackInvocations {
+ public:
+  ExpectedSuccessCallback() { net::ct::GetSampleSignedTreeHead(&known_sth_); }
+
+  explicit ExpectedSuccessCallback(const net::ct::SignedTreeHead& sth)
+      : known_sth_(sth) {}
+
+  void STHFetched(const std::string& log_id,
+                  const net::ct::SignedTreeHead& sth) override {
+    ASSERT_EQ(GetLogID(), 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/03 18:18:54]

Can we just move this into RecordFetchCallbackInvocations, and make a method to
set the expected_sth?  If it's NULL, and emit an error it STHFetch is invoked,
and if it's non-NULL can emit an error when FetcherFailed is invoked.

[Eran Messeri, 2015/08/04 16:15:43]

Done - since it's the same expected_sth, I've added a boolean flag to indicate
whether success is expected or not, if it is, the received STH will always be
compared against the known_good one.

+
+    RecordFetchCallbackInvocations::STHFetched(log_id, sth);
+  }
+
+ private:
+  net::ct::SignedTreeHead known_sth_;
+};
+
+class LogProofFetcherTest : public ::testing::Test {
+ public:
+  LogProofFetcherTest()
+      : context_(true),
+        log_url_(std::string(kLogSchema) + "://" + std::string(kLogURL) + "/") {
+    context_.Init();

[mmenke, 2015/08/03 18:18:54]

Not reason to do this - just remove the Init() call, and the context_(true), and
it will be initialized automatically.

[Eran Messeri, 2015/08/04 16:15:43]

Done.

+  }
+
+  void SetUp() override {
+    scoped_ptr<GetSTHResponseHandler> handler(new GetSTHResponseHandler());
+    handler_ = handler.get();
+
+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        kLogSchema, kLogURL, handler.Pass());
+
+    fetcher_.reset(new LogProofFetcher(&context_));

[mmenke, 2015/08/03 18:18:54]

Can just do this in the constructor (And TearDown in the destructor).  No need
for the extra methods.

[Eran Messeri, 2015/08/04 16:15:43]

Done.

+  }
+
+  void TearDown() 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_, GetLogID(),
+        base::Bind(&RecordFetchCallbackInvocations::STHFetched,
+                   base::Unretained(callback)),
+        base::Bind(&RecordFetchCallbackInvocations::FetchingFailed,
+                   base::Unretained(callback)));
+    message_loop_.RunUntilIdle();

[mmenke, 2015/08/03 18:18:54]

Not a big fan of RunUntilIdle - it does generally work when everything's
happening on one thread, but why not use a RunLoop instead?  Call its run method
here, and have the callbacks invoke its quit method.  Tends to be much more
robust against guture refactorings, though it's not always doable.

[Eran Messeri, 2015/08/04 16:15:43]

Not entirely sure why, but the interceptor (added to the URLRequestFilter in the
c'tor) wouldn't get invoked unless I create a MessageLoopForIO? Does the
URLRequestFilter have any assumptions on the MessageLoop used?

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

Not that I'm aware of.  You need a MessageLoopForIO for some of the sockets
callbacks, but none of that should matter for this test.  Feel free to
investigate, if you're curious (I am, actually, I just don't have time), but not
something to block the CL on.

+  }
+
+  base::MessageLoopForIO message_loop_;
+  net::TestURLRequestContext context_;
+  safe_json::TestingJsonParser::ScopedFactoryOverride factory_override_;
+  scoped_ptr<LogProofFetcher> fetcher_;
+  GURL log_url_;

[mmenke, 2015/08/03 18:18:54]

const

[Eran Messeri, 2015/08/04 16:15:43]

Done.

+  GetSTHResponseHandler* handler_;
+};
+
+TEST_F(LogProofFetcherTest, TestValidGetSTHReply) {

[mmenke, 2015/08/03 18:18:54]

Suggest getting rid of STH in test names, to make them more readable.

i.e.

TestValidGetSTHReply -> TestValidReply
LogProofFetcherTest -> TestValidAsynReply
TestInvalidGetSTHReplyIncompleteSTH -> TestInvalidReplyIncomplete
etc.

If the fact that these are about SignedTreeHeads is important enough to be in
the names of the tests, then I'd recommend renaming LogProofFetcher and the test
fixture to include it (And I'd suggest writing it out as well - does make for a
rather long class name, but think it's best to be clear, since people who have
no idea what an STH is may be seeing test failures).

[Eran Messeri, 2015/08/04 16:15:43]

Done - removed STH from test names.

+  SetValidSTHJSONResponse();
+
+  ExpectedSuccessCallback cb;

[mmenke, 2015/08/03 18:18:54]

optional:  Suggest just writing out callback.  Style guide discourages uncommon
abbreviations.  While cb isn't all that uncommon, think just writing it out is
much more readable.  Also matches the class name.

[Eran Messeri, 2015/08/04 16:15:43]

Done.

+  RunFetcherWithCallback(&cb);
+
+  ASSERT_TRUE(cb.invoked());
+  ASSERT_FALSE(cb.failed());
+}
+
+TEST_F(LogProofFetcherTest, TestValidGetSTHReplyAsyncIO) {
+  SetValidSTHJSONResponse();
+  handler_->SetAsyncIO(true);
+
+  ExpectedSuccessCallback cb;
+  RunFetcherWithCallback(&cb);
+
+  ASSERT_TRUE(cb.invoked());
+  ASSERT_FALSE(cb.failed());
+}
+
+TEST_F(LogProofFetcherTest, TestInvalidGetSTHReplyIncompleteSTH) {
+  std::string sth_json_reply_data = net::ct::CreateSignedTreeHeadJsonString(
+      21 /* tree_size */, 123456u /* timestamp */, std::string(""),
+      std::string(""));
+  handler_->SetResponseData(sth_json_reply_data);
+
+  RecordFetchCallbackInvocations cb;
+  RunFetcherWithCallback(&cb);
+
+  ASSERT_TRUE(cb.invoked());
+  ASSERT_TRUE(cb.failed());
+  ASSERT_EQ(net::ERR_CT_STH_INCOMPLETE, cb.net_error());
+}
+
+TEST_F(LogProofFetcherTest, TestInvalidGetSTHReplyInvalidJSON) {
+  std::string sth_json_reply_data = "{\"tree_size\":21,\"timestamp\":}";
+  handler_->SetResponseData(sth_json_reply_data);
+
+  RecordFetchCallbackInvocations cb;
+  RunFetcherWithCallback(&cb);
+
+  ASSERT_TRUE(cb.invoked());
+  ASSERT_TRUE(cb.failed());
+  ASSERT_EQ(net::ERR_CT_STH_PARSING_FAILED, cb.net_error());
+}
+
+TEST_F(LogProofFetcherTest, TestLogReplyIsTooLong) {
+  std::string sth_json_reply_data = net::ct::GetSampleSTHAsJson();
+  // kMaxLogResponseSizeInBytes is 600 - add that much to make sure the response
+  // is too big.
+  sth_json_reply_data.append(std::string(600, ' '));

[mmenke, 2015/08/03 18:18:54]

It's much better to expose MaxLogResponseSizeInBytes as a public static constant
in LogProofFetcher, and just access it here.  Nothing else needs it, but hard
coding these sorts of constants just makes changing them a pain.

[Eran Messeri, 2015/08/04 16:15:43]

Done.

+  handler_->SetResponseData(sth_json_reply_data);
+
+  RecordFetchCallbackInvocations cb;
+  RunFetcherWithCallback(&cb);
+
+  ASSERT_TRUE(cb.invoked());
+  ASSERT_TRUE(cb.failed());
+  ASSERT_EQ(net::ERR_FILE_TOO_BIG, cb.net_error());
+  ASSERT_EQ(net::HTTP_OK, cb.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(600 - sth_json_reply_data.size(), ' '));
+  handler_->SetResponseData(sth_json_reply_data);
+
+  ExpectedSuccessCallback cb;
+  RunFetcherWithCallback(&cb);
+
+  ASSERT_TRUE(cb.invoked());
+  ASSERT_FALSE(cb.failed());
+}
+
+TEST_F(LogProofFetcherTest, TestLogRepliesWithHttpError) {
+  handler_->SetResponseCode(net::HTTP_NOT_FOUND);
+
+  RecordFetchCallbackInvocations cb;
+  RunFetcherWithCallback(&cb);
+
+  ASSERT_TRUE(cb.invoked());
+  ASSERT_TRUE(cb.failed());
+  ASSERT_EQ(net::OK, cb.net_error());
+  ASSERT_EQ(net::HTTP_NOT_FOUND, cb.http_response_code());
+}
+
+}  // namespace
+
+}  // namespace certificate_transparency