Certificate Transparency: Fetching consistency proofs.

components/certificate_transparency/log_proof_fetcher.cc

Index: components/certificate_transparency/log_proof_fetcher.cc
diff --git a/components/certificate_transparency/log_proof_fetcher.cc b/components/certificate_transparency/log_proof_fetcher.cc
index 7e51858708b0acb5d60dbd550cba646ff6ac5ab6..65dde941f09da5248e6c5ff3b04c8181aa0b0056 100644
--- a/components/certificate_transparency/log_proof_fetcher.cc
+++ b/components/certificate_transparency/log_proof_fetcher.cc
@@ -6,9 +6,11 @@
 
 #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"
@@ -40,16 +42,22 @@ int GetNetErrorFromURLRequestStatus(const net::URLRequestStatus& status) {
   }
 }
 
+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;
@@ -57,12 +65,18 @@ struct LogProofFetcher::FetchState {
 
 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() {}
 
@@ -81,16 +95,56 @@ void LogProofFetcher::FetchSignedTreeHead(
     const std::string& log_id,
     const SignedTreeHeadFetchedCallback& fetched_callback,
     const FetchFailedCallback& failed_callback) {
-  DCHECK(base_log_url.SchemeIsHTTPOrHTTPS());
   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());
+
+  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));
 }
@@ -185,12 +239,23 @@ void LogProofFetcher::RequestComplete(net::URLRequest* request) {
 
   // 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) {
@@ -224,8 +289,8 @@ void LogProofFetcher::OnSTHJsonParseSuccess(
 
   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);
@@ -239,4 +304,29 @@ void LogProofFetcher::OnSTHJsonParseError(net::URLRequest* request,
   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