Certificate Transparency: Fetching consistency proofs.

components/certificate_transparency/log_proof_fetcher.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 <iterator>
 
+#include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "components/safe_json/safe_json_parser.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/request_priority.h"
 #include "net/cert/ct_log_response_parser.h"
 #include "net/cert/signed_tree_head.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_request_context.h"
@@ skipping 11 matching lines @@
     case net::URLRequestStatus::CANCELED:
       return net::ERR_ABORTED;
     case net::URLRequestStatus::FAILED:
       return status.error();
     default:
       NOTREACHED();
       return net::ERR_FAILED;
   }
 }
 
+enum LogRequestType { SIGNED_TREE_HEAD, CONSISTENCY_PROOF };

[Ryan Sleevi, 2015/11/26 00:50:09]

Document

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

Obsolete.

+
 }  // namespace
 
 struct LogProofFetcher::FetchState {
   FetchState(const std::string& log_id,
-             const SignedTreeHeadFetchedCallback& fetched_callback,
+             LogRequestType request_type,
+             const SignedTreeHeadFetchedCallback& sth_fetch_callback,
+             const ConsistencyProofFetchedCallback& proof_fetch_callback,
              const FetchFailedCallback& failed_callback);
   ~FetchState();
 
   std::string log_id;
-  SignedTreeHeadFetchedCallback fetched_callback;
+  LogRequestType request_type;
+  SignedTreeHeadFetchedCallback sth_fetch_callback;
+  ConsistencyProofFetchedCallback proof_fetch_callback;
   FetchFailedCallback failed_callback;
   scoped_refptr<net::IOBufferWithSize> response_buffer;
   std::string assembled_response;
 };
 
 LogProofFetcher::FetchState::FetchState(
     const std::string& log_id,
-    const SignedTreeHeadFetchedCallback& fetched_callback,
+    LogRequestType request_type,
+    const SignedTreeHeadFetchedCallback& sth_fetch_callback,
+    const ConsistencyProofFetchedCallback& proof_fetch_callback,
     const FetchFailedCallback& failed_callback)
     : log_id(log_id),
-      fetched_callback(fetched_callback),
+      request_type(request_type),
+      sth_fetch_callback(sth_fetch_callback),
+      proof_fetch_callback(proof_fetch_callback),
       failed_callback(failed_callback),
-      response_buffer(new net::IOBufferWithSize(kMaxLogResponseSizeInBytes)) {}
+      response_buffer(new net::IOBufferWithSize(kMaxLogResponseSizeInBytes)) {
+  DCHECK(!(sth_fetch_callback.is_null() && proof_fetch_callback.is_null()));

[Ryan Sleevi, 2015/11/26 00:50:09]

Past experience has shown mixing state objects like this is generally a design
smell and a recipe for bugs.

I imagine there will be further, future requests to make of the log - either to
handle V1 responses or for more V2 queries, right?

This makes me wonder whether this should be re-structured somewhat:
a) Does LogProofFetcher need to be a URLRequestDelegate itself? Could it defer
the actual requesting to an inner class (e.g. FetchState) to be the Delegate?
b) It seems like FetchState is really FetchAndParseState. Would it make more
sense for FetchState to have a singular callback that it calls, with the fully
assembled response, and then that callback can take appropriate action? Currying
could help here.

I'm a little concerned about the size and amount of logic growing in the class
here - there's probably a few things we can do to simplify, but perhaps in a
separate CL.

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

Agreed - this is indeed a smell. I've completely changed the FetchState and
added a LogRequest class hierarchy, where subclasses provide the URL to fetch
from and interpret the parsed JSON.
Should be much simpler to read now.

+}
 
 LogProofFetcher::FetchState::~FetchState() {}
 
 LogProofFetcher::LogProofFetcher(net::URLRequestContext* request_context)
     : request_context_(request_context), weak_factory_(this) {
   DCHECK(request_context);
 }
 
 LogProofFetcher::~LogProofFetcher() {
   STLDeleteContainerPairPointers(inflight_requests_.begin(),
                                  inflight_requests_.end());
 }
 
 void LogProofFetcher::FetchSignedTreeHead(
     const GURL& base_log_url,
     const std::string& log_id,
     const SignedTreeHeadFetchedCallback& fetched_callback,
     const FetchFailedCallback& failed_callback) {
+  GURL fetch_url(base_log_url.Resolve("ct/v1/get-sth"));
+
+  FetchFromLog(fetch_url, log_id, fetched_callback,
+               ConsistencyProofFetchedCallback(), failed_callback);
+}
+
+void LogProofFetcher::FetchConsistencyProof(
+    const GURL& base_log_url,
+    const std::string& log_id,
+    size_t old_tree_size,

[Ryan Sleevi, 2015/11/26 00:50:09]

BUG: I'm fairly sure these should be uint64_t types, and not size_t. 32-bit
clients shouldn't be limited to 32-bit entries from logs, right?

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

Correct, fixed.

+    size_t new_tree_size,
+    const ConsistencyProofFetchedCallback& fetched_callback,
+    const FetchFailedCallback& failed_callback) {
   DCHECK(base_log_url.SchemeIsHTTPOrHTTPS());
-  GURL fetch_url(base_log_url.Resolve("ct/v1/get-sth"));
+
+  std::string relative = base::StringPrintf(
+      "ct/v1/get-sth-consistency?first=%" PRIuS "&second=%" PRIuS,
+      old_tree_size, new_tree_size);
+  GURL fetch_url = base_log_url.Resolve(relative);
+
+  FetchFromLog(fetch_url, log_id, SignedTreeHeadFetchedCallback(),
+               fetched_callback, failed_callback);
+}
+
+void LogProofFetcher::FetchFromLog(
+    const GURL& request_url,
+    const std::string& log_id,
+    const SignedTreeHeadFetchedCallback& sth_fetched_callback,
+    const ConsistencyProofFetchedCallback& proof_fetched_callback,
+    const FetchFailedCallback& failed_callback) {
+  DCHECK(request_url.SchemeIsHTTPOrHTTPS());
+  DCHECK(!failed_callback.is_null());
+
+  LogRequestType request_type;
+  if (!sth_fetched_callback.is_null()) {
+    request_type = LogRequestType::SIGNED_TREE_HEAD;
+  } else {
+    DCHECK(!proof_fetched_callback.is_null());
+    request_type = LogRequestType::CONSISTENCY_PROOF;
+  }

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

optional:  May be cleaner as an argument...Or maybe not.

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

Obsolete - see restructuring of the code.

+
   scoped_ptr<net::URLRequest> request =
-      request_context_->CreateRequest(fetch_url, net::DEFAULT_PRIORITY, this);
+      request_context_->CreateRequest(request_url, net::DEFAULT_PRIORITY, this);
   request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                         net::LOAD_DO_NOT_SAVE_COOKIES |
                         net::LOAD_DO_NOT_SEND_AUTH_DATA);
 
   FetchState* fetch_state =
-      new FetchState(log_id, fetched_callback, failed_callback);
+      new FetchState(log_id, request_type, sth_fetched_callback,
+                     proof_fetched_callback, failed_callback);
   request->Start();
   inflight_requests_.insert(std::make_pair(request.release(), fetch_state));
 }
 
 void LogProofFetcher::OnResponseStarted(net::URLRequest* request) {
   net::URLRequestStatus status(request->status());
   DCHECK(inflight_requests_.count(request));
   FetchState* fetch_state = inflight_requests_.find(request)->second;
 
   if (!status.is_success() || request->GetResponseCode() != net::HTTP_OK) {
@@ skipping 74 matching lines @@
   }
 }
 
 void LogProofFetcher::RequestComplete(net::URLRequest* request) {
   DCHECK(inflight_requests_.count(request));
 
   FetchState* fetch_state = inflight_requests_.find(request)->second;
 
   // Get rid of the buffer as it really isn't necessary.
   fetch_state->response_buffer = nullptr;
-  safe_json::SafeJsonParser::Parse(
-      fetch_state->assembled_response,
-      base::Bind(&LogProofFetcher::OnSTHJsonParseSuccess,
-                 weak_factory_.GetWeakPtr(), request),
-      base::Bind(&LogProofFetcher::OnSTHJsonParseError,
-                 weak_factory_.GetWeakPtr(), request));
+  if (fetch_state->request_type == LogRequestType::SIGNED_TREE_HEAD) {
+    safe_json::SafeJsonParser::Parse(
+        fetch_state->assembled_response,
+        base::Bind(&LogProofFetcher::OnSTHJsonParseSuccess,
+                   weak_factory_.GetWeakPtr(), request),
+        base::Bind(&LogProofFetcher::OnSTHJsonParseError,
+                   weak_factory_.GetWeakPtr(), request));
+  } else if (fetch_state->request_type == LogRequestType::CONSISTENCY_PROOF) {
+    safe_json::SafeJsonParser::Parse(
+        fetch_state->assembled_response,
+        base::Bind(&LogProofFetcher::OnConsistencyProofJsonParseSuccess,
+                   weak_factory_.GetWeakPtr(), request),
+        base::Bind(&LogProofFetcher::OnConsistencyProofJsonParseError,
+                   weak_factory_.GetWeakPtr(), request));
+  } else {
+    NOTREACHED();
+  }
 }
 
 void LogProofFetcher::CleanupRequest(net::URLRequest* request) {
   DVLOG(1) << "Cleaning up request to " << request->original_url();
   auto it = inflight_requests_.find(request);
   DCHECK(it != inflight_requests_.end());
   auto next_it = it;
   std::advance(next_it, 1);
 
   // Delete FetchState and URLRequest, then the entry from inflight_requests_.
@@ skipping 13 matching lines @@
   CleanupRequest(request);
 }
 
 void LogProofFetcher::OnSTHJsonParseSuccess(
     net::URLRequest* request,
     scoped_ptr<base::Value> parsed_json) {
   DCHECK(inflight_requests_.count(request));
 
   FetchState* fetch_state = inflight_requests_.find(request)->second;
   net::ct::SignedTreeHead signed_tree_head;
-  if (net::ct::FillSignedTreeHead(*parsed_json.get(), &signed_tree_head)) {
-    fetch_state->fetched_callback.Run(fetch_state->log_id, signed_tree_head);
+  if (net::ct::FillSignedTreeHead(*parsed_json, &signed_tree_head)) {
+    fetch_state->sth_fetch_callback.Run(fetch_state->log_id, signed_tree_head);
   } else {
     fetch_state->failed_callback.Run(fetch_state->log_id,
                                      net::ERR_CT_STH_INCOMPLETE, net::HTTP_OK);
   }
 
   CleanupRequest(request);
 }
 
 void LogProofFetcher::OnSTHJsonParseError(net::URLRequest* request,
                                           const std::string& error) {
   InvokeFailureCallback(request, net::ERR_CT_STH_PARSING_FAILED, net::HTTP_OK);
 }
 
+void LogProofFetcher::OnConsistencyProofJsonParseSuccess(
+    net::URLRequest* request,
+    scoped_ptr<base::Value> parsed_json) {
+  DCHECK(inflight_requests_.count(request));
+  FetchState* fetch_state = inflight_requests_.find(request)->second;
+  std::vector<std::string> consistency_proof;
+  if (net::ct::FillConsistencyProof(*parsed_json, &consistency_proof)) {
+    fetch_state->proof_fetch_callback.Run(fetch_state->log_id,
+                                          consistency_proof);
+  } else {
+    fetch_state->failed_callback.Run(
+        fetch_state->log_id, net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED,
+        net::HTTP_OK);
+  }
+
+  CleanupRequest(request);
+}
+
+void LogProofFetcher::OnConsistencyProofJsonParseError(
+    net::URLRequest* request,
+    const std::string& error) {
+  InvokeFailureCallback(request, net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED,
+                        net::HTTP_OK);
+}
+
 }  // namespace certificate_transparency