Certificate Transparency: Fetching consistency proofs.

components/certificate_transparency/log_proof_fetcher_unittest.cc

Index: components/certificate_transparency/log_proof_fetcher_unittest.cc
diff --git a/components/certificate_transparency/log_proof_fetcher_unittest.cc b/components/certificate_transparency/log_proof_fetcher_unittest.cc
index f6caa3beb98bfd091dff8a772d4e987fe5a60abf..de0f48c4cd46659ae4b5154da545c08d266f8a98 100644
--- a/components/certificate_transparency/log_proof_fetcher_unittest.cc
+++ b/components/certificate_transparency/log_proof_fetcher_unittest.cc
@@ -25,11 +25,11 @@ namespace certificate_transparency {
 
 namespace {
 
-const char kGetSTHHeaders[] =
+const char kGetResponseHeaders[] =
     "HTTP/1.1 200 OK\n"
     "Content-Type: application/json; charset=ISO-8859-1\n";
 
-const char kGetSTHNotFoundHeaders[] =
+const char kGetResponseNotFoundHeaders[] =
     "HTTP/1.1 404 Not Found\n"
     "Content-Type: text/html; charset=iso-8859-1\n";
 
@@ -38,23 +38,30 @@ const char kLogHost[] = "ct.log.example.com";
 const char kLogPathPrefix[] = "somelog";
 const char kLogID[] = "some_id";
 
-class FetchSTHTestJob : public net::URLRequestTestJob {
+// Node returned will be chr(node_id) * 32.
+std::string GetDummyConsistencyProofNode(size_t node_id) {
+  return std::string(32, static_cast<char>(node_id));
+}
+
+const size_t kDummyConsistencyProofLength = 4;
+
+class LogFetchTestJob : public net::URLRequestTestJob {
  public:
-  FetchSTHTestJob(const std::string& get_sth_data,
-                  const std::string& get_sth_headers,
+  LogFetchTestJob(const std::string& get_log_data,
+                  const std::string& get_log_headers,
                   net::URLRequest* request,
                   net::NetworkDelegate* network_delegate)
       : URLRequestTestJob(request,
                           network_delegate,
-                          get_sth_headers,
-                          get_sth_data,
+                          get_log_headers,
+                          get_log_data,
                           true),
         async_io_(false) {}
 
   void set_async_io(bool async_io) { async_io_ = async_io; }
 
  private:
-  ~FetchSTHTestJob() override {}
+  ~LogFetchTestJob() override {}
 
   bool NextReadAsync() override {
     // Response with indication of async IO only once, otherwise the final
@@ -73,26 +80,41 @@ class FetchSTHTestJob : public net::URLRequestTestJob {
 
   bool async_io_;
 
-  DISALLOW_COPY_AND_ASSIGN(FetchSTHTestJob);
+  DISALLOW_COPY_AND_ASSIGN(LogFetchTestJob);
 };
 
-class GetSTHResponseHandler : public net::URLRequestInterceptor {
+class LogGetResponseHandler : public net::URLRequestInterceptor {
  public:
-  GetSTHResponseHandler()
+  LogGetResponseHandler()
       : async_io_(false),
         response_body_(""),

[mmenke, 2015/11/18 19:25:16]

nit:  Not needed (Know it was like that before)

[Eran Messeri, 2015/11/24 22:53:38]

Done.

         response_headers_(
-            std::string(kGetSTHHeaders, arraysize(kGetSTHHeaders))) {}
-  ~GetSTHResponseHandler() override {}
+            std::string(kGetResponseHeaders, arraysize(kGetResponseHeaders))),
+        expected_old_tree_size_(0),
+        expected_new_tree_size_(0) {}
+  ~LogGetResponseHandler() override {}
 
   // URLRequestInterceptor implementation:
   net::URLRequestJob* MaybeInterceptRequest(
       net::URLRequest* request,
       net::NetworkDelegate* network_delegate) const override {
-    std::string expected_url = base::StringPrintf(
-        "%s://%s/%s/ct/v1/get-sth", kLogSchema, kLogHost, kLogPathPrefix);
+    std::string base_expected_url = base::StringPrintf(
+        "%s://%s/%s/ct/v1/", kLogSchema, kLogHost, kLogPathPrefix);

[mmenke, 2015/11/18 19:25:16]

I don't think we want logic in here that matches the logic in production code to
deduce the expected URL.  Suggest adding a setter, and explicitly calling it in
tests.

Alternatively, could make an accessor for the URL we end up fetching, and put
the EXPECT in the test body (Or in RunGetConsistencyFetcherWithCallback /
RunFetcherWithCallback) - think that makes for clearer tests.

[Eran Messeri, 2015/11/24 22:53:38]

Done - by adding a setter, as the MaybeInterceptRequest method is const, I can't
change any fields.

[mmenke, 2015/11/25 17:40:28]

You can always use mutable.  Anyhow, fine as-is.

[Eran Messeri, 2015/11/26 22:07:12]

Acknowledged.

+
+    std::string expected_url;
+    if (expected_old_tree_size_ == 0 && expected_new_tree_size_ == 0) {
+      // Expecting get-sth
+      expected_url = base_expected_url + std::string("get-sth");
+    } else {
+      // Expecting get-consistency-proof
+      expected_url = base_expected_url +
+                     base::StringPrintf(
+                         "get-sth-consistency?first=%lu&second=%lu",
+                         static_cast<unsigned long>(expected_old_tree_size_),
+                         static_cast<unsigned long>(expected_new_tree_size_));
+    }
     EXPECT_EQ(GURL(expected_url), request->url());
-    FetchSTHTestJob* job = new FetchSTHTestJob(
+    LogFetchTestJob* job = new LogFetchTestJob(
         response_body_, response_headers_, request, network_delegate);
     job->set_async_io(async_io_);
     return job;
@@ -106,6 +128,12 @@ class GetSTHResponseHandler : public net::URLRequestInterceptor {
     response_headers_ = response_headers;
   }
 
+  void set_expect_get_consistency_proof(size_t expected_old_tree_size,
+                                        size_t expected_new_tree_size) {
+    expected_old_tree_size_ = expected_old_tree_size;
+    expected_new_tree_size_ = expected_new_tree_size;
+  }
+
   void set_async_io(bool async_io) { async_io_ = async_io; }
 
  private:
@@ -113,7 +141,10 @@ class GetSTHResponseHandler : public net::URLRequestInterceptor {
   std::string response_body_;
   std::string response_headers_;
 
-  DISALLOW_COPY_AND_ASSIGN(GetSTHResponseHandler);
+  size_t expected_old_tree_size_;
+  size_t expected_new_tree_size_;
+
+  DISALLOW_COPY_AND_ASSIGN(LogGetResponseHandler);
 };
 
 class RecordFetchCallbackInvocations {
@@ -146,6 +177,19 @@ class RecordFetchCallbackInvocations {
               sth.signature.signature_data);
   }
 
+  void ConsistencyProofFetched(
+      const std::string& log_id,
+      const std::vector<std::string>& consistency_proof) {
+    ASSERT_TRUE(expect_success_);
+    ASSERT_FALSE(invoked_);
+    invoked_ = true;

[mmenke, 2015/11/18 19:25:15]

Record which method was invoked?

[Eran Messeri, 2015/11/24 22:53:38]

Done - that made the invoked_ field obsolete so I've removed it.

+    EXPECT_EQ(kDummyConsistencyProofLength, consistency_proof.size());
+    for (size_t i = 0; i < kDummyConsistencyProofLength; ++i) {
+      EXPECT_EQ(GetDummyConsistencyProofNode(i), consistency_proof[i])
+          << " node: " << i;

[mmenke, 2015/11/18 19:25:16]

Again, think it's cleaner to store these in the test fixture, and then make the
test body check them.

[Eran Messeri, 2015/11/24 22:53:38]

Done - both for the consistency proof and the STH fetching.

+    }
+  }
+
   void FetchingFailed(const std::string& log_id,
                       int net_error,
                       int http_response_code) {
@@ -179,7 +223,7 @@ class LogProofFetcherTest : public ::testing::Test {
                                     kLogSchema,
                                     kLogHost,
                                     kLogPathPrefix)) {
-    scoped_ptr<GetSTHResponseHandler> handler(new GetSTHResponseHandler());
+    scoped_ptr<LogGetResponseHandler> handler(new LogGetResponseHandler());
     handler_ = handler.get();
 
     net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
@@ -209,12 +253,26 @@ class LogProofFetcherTest : public ::testing::Test {
     message_loop_.RunUntilIdle();

[mmenke, 2015/11/18 19:25:16]

optional:  I did sign off on this before...But think it would be cleaner to use
a RunLoop invoked by RecordFetchCallbackInvocations, rather than rely on
everything happening without any intervening idle time.  The disadvantage is on
breakage, the test hangs.  The advantage is it's more flexible, and doesn't
break if someone changes how the test works (Switches to a real test server, or
whatever).

[Eran Messeri, 2015/11/24 22:53:38]

IIRC there was a problem with the async_io case - I'll dig up the bug number
shortly.

   }
 
+  void RunGetConsistencyFetcherWithCallback(
+      RecordFetchCallbackInvocations* callback) {
+    const size_t kOldTree = 5;
+    const size_t kNewTree = 8;
+    handler_->set_expect_get_consistency_proof(kOldTree, kNewTree);
+    fetcher_->FetchConsistencyProof(
+        log_url_, kLogID, kOldTree, kNewTree,
+        base::Bind(&RecordFetchCallbackInvocations::ConsistencyProofFetched,
+                   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_;
+  LogGetResponseHandler* handler_;
 };
 
 TEST_F(LogProofFetcherTest, TestValidGetReply) {
@@ -291,8 +349,8 @@ TEST_F(LogProofFetcherTest, TestLogReplyIsExactlyMaxSize) {
 }
 
 TEST_F(LogProofFetcherTest, TestLogRepliesWithHttpError) {
-  handler_->set_response_headers(
-      std::string(kGetSTHNotFoundHeaders, arraysize(kGetSTHNotFoundHeaders)));
+  handler_->set_response_headers(std::string(
+      kGetResponseNotFoundHeaders, arraysize(kGetResponseNotFoundHeaders)));
 
   RecordFetchCallbackInvocations callback(false);
   RunFetcherWithCallback(&callback);
@@ -302,6 +360,33 @@ TEST_F(LogProofFetcherTest, TestLogRepliesWithHttpError) {
   EXPECT_EQ(net::HTTP_NOT_FOUND, callback.http_response_code());
 }
 
+TEST_F(LogProofFetcherTest, TestValidGetConsistencyValidReply) {
+  std::vector<std::string> proof;
+  for (size_t i = 0; i < kDummyConsistencyProofLength; ++i)
+    proof.push_back(GetDummyConsistencyProofNode(i));
+
+  std::string consistency_proof_reply_data =
+      net::ct::CreateConsistencyProofJsonString(proof);
+  handler_->set_response_body(consistency_proof_reply_data);
+
+  RecordFetchCallbackInvocations callback(true);
+  RunGetConsistencyFetcherWithCallback(&callback);
+
+  ASSERT_TRUE(callback.invoked());
+}
+
+TEST_F(LogProofFetcherTest, TestInvalidGetConsistencyReplyInvalidJSON) {
+  std::string consistency_proof_reply_data = "{\"consistency\": [1,2]}";
+  handler_->set_response_body(consistency_proof_reply_data);
+
+  RecordFetchCallbackInvocations callback(false);
+  RunGetConsistencyFetcherWithCallback(&callback);
+
+  ASSERT_TRUE(callback.invoked());
+  EXPECT_EQ(net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED, callback.net_error());
+  EXPECT_EQ(net::HTTP_OK, callback.http_response_code());
+}
+
 }  // namespace
 
 }  // namespace certificate_transparency