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..c201989dadd79e4d78acf393dfb3c7295ac1ab14 100644
--- a/components/certificate_transparency/log_proof_fetcher.cc
+++ b/components/certificate_transparency/log_proof_fetcher.cc
@@ -9,6 +9,7 @@
 #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 +41,22 @@ int GetNetErrorFromURLRequestStatus(const net::URLRequestStatus& status) {
   }
 }
 
+enum LogRequestType { SIGNED_TREE_HEAD, CONSISTENCY_PROOF };
+
 }  // 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 +64,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()));
+}
 
 LogProofFetcher::FetchState::~FetchState() {}
 
@@ -83,16 +96,37 @@ void LogProofFetcher::FetchSignedTreeHead(
     const FetchFailedCallback& failed_callback) {

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

Suggest putting everything in this method except the line to get the fetch_url
into a subroutine, shared with FetchConsistencyProof.  Not a fan of duplicated
code.  You could just inline CreateRequest in that method, too, unless you think
you'll need to add another method where it will be useful.

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

Done. I note that the use of two callbacks is a smell of poor man's inheritance
pattern - I can fix it to have a delegate class that will do the json parsing
and invoke the callback for each case.

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

Could also make a single callback that takes a value indicating success or
failure.  I don't have strong feelings about it, though the prototype of
FetchFromLog is looking pretty ugly now.  :(

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

Should be fixed now that I've switched to using a proper class hierarchy for
handling the various log requests.

   DCHECK(base_log_url.SchemeIsHTTPOrHTTPS());
   GURL fetch_url(base_log_url.Resolve("ct/v1/get-sth"));
-  scoped_ptr<net::URLRequest> request =
-      request_context_->CreateRequest(fetch_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);
+  net::URLRequest* request = CreateRequest(fetch_url);
 
   FetchState* fetch_state =
-      new FetchState(log_id, fetched_callback, failed_callback);
+      new FetchState(log_id, LogRequestType::SIGNED_TREE_HEAD, fetched_callback,
+                     ConsistencyProofFetchedCallback(), failed_callback);
   request->Start();
-  inflight_requests_.insert(std::make_pair(request.release(), fetch_state));
+  inflight_requests_.insert(std::make_pair(request, fetch_state));
+}
+
+void LogProofFetcher::FetchConsistencyProof(
+    const GURL& base_log_url,
+    const std::string& log_id,
+    size_t old_tree_size,
+    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=%lu&second=%lu",
+                         static_cast<unsigned long>(old_tree_size),
+                         static_cast<unsigned long>(new_tree_size));

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

include format_macros.h and use PRIuS to use printf methods with size_t's.  Or
if these should really be unsigned ints, make the arguments to this method
unsigned ints.

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

Done. These are size_t - they indicate size of a Merkle tree (which is always
non-negative).

+  GURL fetch_url = base_log_url.Resolve(relative);
+
+  net::URLRequest* request = CreateRequest(fetch_url);
+
+  FetchState* fetch_state = new FetchState(
+      log_id, LogRequestType::CONSISTENCY_PROOF,
+      SignedTreeHeadFetchedCallback(), fetched_callback, failed_callback);
+  request->Start();
+  inflight_requests_.insert(std::make_pair(request, fetch_state));
 }
 
 void LogProofFetcher::OnResponseStarted(net::URLRequest* request) {
@@ -185,12 +219,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) {
@@ -225,7 +270,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)) {

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

nit:  While you're here, .get() isn't needed.

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

Done.

-    fetch_state->fetched_callback.Run(fetch_state->log_id, signed_tree_head);
+    DCHECK(!(fetch_state->sth_fetch_callback.is_null()));

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

This DCHECK makes much more sense in FetchSignedTreeHead.  You should also check
the failed_callback there, too (Or NULL-check it before calling it).

You can keep a DCHECK here for documentation purposes, if you want.

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

Good point - DCHECK made much earlier on, removed from here.

+    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 +285,39 @@ 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.get(), &consistency_proof)) {

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

See comment in OnSTHJsonParseSuccess.

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

Done.

+    DCHECK(!(fetch_state->proof_fetch_callback.is_null()));

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

See comment in OnSTHJsonParseSuccess.

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

Done (removed unnecessary DCHECKs here as they are performed before the
FetchState instance is created).

+    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);
+}
+
+net::URLRequest* LogProofFetcher::CreateRequest(const GURL& url) {
+  scoped_ptr<net::URLRequest> request =
+      request_context_->CreateRequest(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);
+  return request.release();
+}
+
 }  // namespace certificate_transparency