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..a74a51db20a196f45ffedb8ae74abf013dcb5e6e 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"
@@ -42,201 +44,376 @@ int GetNetErrorFromURLRequestStatus(const net::URLRequestStatus& status) {
 
 }  // namespace
 
-struct LogProofFetcher::FetchState {
-  FetchState(const std::string& log_id,
-             const SignedTreeHeadFetchedCallback& fetched_callback,
-             const FetchFailedCallback& failed_callback);
-  ~FetchState();
-
-  std::string log_id;
-  SignedTreeHeadFetchedCallback fetched_callback;
-  FetchFailedCallback failed_callback;
-  scoped_refptr<net::IOBufferWithSize> response_buffer;
-  std::string assembled_response;
+// Interface for handling the response from a CT log for particular
+// requests.
+// All log responses are JSON and should be parsed; however the response
+// to each request should be parsed and validated diffenetly.
+class LogProofFetcher::LogResponseHandler {
+ public:
+  // |log_id| will be passed to the callback to indicate which log
+  // this failure pretains to.
+  // All requests could fail so the |failure_callback| is shared to all
+  // request types.
+  LogResponseHandler(
+      const std::string& log_id,
+      const LogProofFetcher::FetchFailedCallback& failure_callback);
+  virtual ~LogResponseHandler();
+
+  // Handle respones JSON that parsed successfully, usually by
+  // invoking the success callback.
+  virtual void HandleParsedJson(const base::Value& parsed_json) = 0;
+
+  // Handle failure to parse response JSON, usually by invoking the failure
+  // callback with a request-specific net error code.
+  virtual void HandleJsonParseFailure(const std::string& json_error) = 0;
+
+  // Handle network failure to complete the request to the log.
+  virtual void HandleNetFailure(int net_error, int http_response_code);

[Ryan Sleevi, 2015/12/08 01:13:52]

nit: s/Net/Network/

[mmenke, 2015/12/08 17:10:12]

net may actually be more accurate - net::kErrorDomain is "net".

[Eran Messeri, 2015/12/14 13:01:42]

Leaving as mmenke suggested.

+
+ protected:
+  const std::string log_id_;
+  const LogProofFetcher::FetchFailedCallback failure_callback_;
 };
 
-LogProofFetcher::FetchState::FetchState(
+LogProofFetcher::LogResponseHandler::LogResponseHandler(
     const std::string& log_id,
-    const SignedTreeHeadFetchedCallback& fetched_callback,
-    const FetchFailedCallback& failed_callback)
-    : log_id(log_id),
-      fetched_callback(fetched_callback),
-      failed_callback(failed_callback),
-      response_buffer(new net::IOBufferWithSize(kMaxLogResponseSizeInBytes)) {}
+    const LogProofFetcher::FetchFailedCallback& failure_callback)
+    : log_id_(log_id), failure_callback_(failure_callback) {
+  DCHECK(!failure_callback_.is_null());
+}
 
-LogProofFetcher::FetchState::~FetchState() {}
+LogProofFetcher::LogResponseHandler::~LogResponseHandler() {}
 
-LogProofFetcher::LogProofFetcher(net::URLRequestContext* request_context)
-    : request_context_(request_context), weak_factory_(this) {
-  DCHECK(request_context);
+void LogProofFetcher::LogResponseHandler::HandleNetFailure(
+    int net_error,
+    int http_response_code) {
+  failure_callback_.Run(log_id_, net_error, http_response_code);
 }
 
-LogProofFetcher::~LogProofFetcher() {
-  STLDeleteContainerPairPointers(inflight_requests_.begin(),
-                                 inflight_requests_.end());
-}
+class LogProofFetcher::GetSTHLogResponseHandler
+    : public LogProofFetcher::LogResponseHandler {
+ public:
+  GetSTHLogResponseHandler(
+      const std::string& log_id,
+      const LogProofFetcher::SignedTreeHeadFetchedCallback& sth_fetch_callback,
+      const LogProofFetcher::FetchFailedCallback& failure_callback)
+      : LogResponseHandler(log_id, failure_callback),
+        sth_fetched_(sth_fetch_callback) {}
+
+  // Fill a net::ct::SignedTreeHead instance from the parsed JSON and, if
+  // successful, invoke the success callback with this STH.
+  // Otherwise, invoke the failure callback.
+  void HandleParsedJson(const base::Value& parsed_json) override {
+    net::ct::SignedTreeHead signed_tree_head;
+    if (net::ct::FillSignedTreeHead(parsed_json, &signed_tree_head)) {

[Ryan Sleevi, 2015/12/08 01:13:52]

nit: restructure this so errors first?

if (!net::ct::....) {
  failure_callback_.Run(...);
  return;
}

sth_fetched_.Run(...)


Note: In general, invoking callback_.Run(), where |callback_| is a member, is a
highly error-prone pattern, particularly for cases of potential re-entrency. For
this reason, I generally encourage people to use something like either PostTask
or, alternatively, base::ResetAndReturn(&callback_).Run(...)

[Eran Messeri, 2015/12/14 13:01:42]

Done - switched to using base::ResetAndReturn and switched error handling order.

+      sth_fetched_.Run(log_id_, signed_tree_head);
+    } else {
+      failure_callback_.Run(log_id_, net::ERR_CT_STH_INCOMPLETE, net::HTTP_OK);
+    }
+  }
 
-void LogProofFetcher::FetchSignedTreeHead(
-    const GURL& base_log_url,
-    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"));
-  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);
+  // Invoke the error callback indicating that STH parsing failed.
+  void HandleJsonParseFailure(const std::string& json_error) override {
+    failure_callback_.Run(log_id_, net::ERR_CT_STH_PARSING_FAILED,
+                          net::HTTP_OK);
+  }
+
+ private:
+  const LogProofFetcher::SignedTreeHeadFetchedCallback sth_fetched_;
+};
+
+class LogProofFetcher::GetConsistencyProofLogResponseHandler
+    : public LogProofFetcher::LogResponseHandler {
+ public:
+  GetConsistencyProofLogResponseHandler(
+      const std::string& log_id,
+      const LogProofFetcher::ConsistencyProofFetchedCallback&
+          proof_fetch_callback,
+      const LogProofFetcher::FetchFailedCallback& failure_callback)
+      : LogResponseHandler(log_id, failure_callback),
+        proof_fetched_(proof_fetch_callback) {}
+
+  // Fill a vector of strings with nodes from the received consistency proof
+  // and, if successful, invoke the success callback with this vector.
+  // Otherwise, invoke the failure callback indicating proof parsing has failed.
+  void HandleParsedJson(const base::Value& parsed_json) override {
+    std::vector<std::string> consistency_proof;
+    if (net::ct::FillConsistencyProof(parsed_json, &consistency_proof)) {

[Ryan Sleevi, 2015/12/08 01:13:52]

same comments re: error handling

[Eran Messeri, 2015/12/14 13:01:42]

Done.

+      proof_fetched_.Run(log_id_, consistency_proof);
+    } else {
+      failure_callback_.Run(
+          log_id_, net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED, net::HTTP_OK);
+    }
+  }
+
+  // Invoke the error callback indicating proof fetching failed.
+  void HandleJsonParseFailure(const std::string& json_error) override {
+    failure_callback_.Run(log_id_, net::ERR_CT_CONSISTENCY_PROOF_PARSING_FAILED,
+                          net::HTTP_OK);
+  }
+
+ private:
+  const LogProofFetcher::ConsistencyProofFetchedCallback proof_fetched_;
+};
 
-  FetchState* fetch_state =
-      new FetchState(log_id, fetched_callback, failed_callback);
-  request->Start();
-  inflight_requests_.insert(std::make_pair(request.release(), fetch_state));
+// Class for issuing a particular request from a CT log and assembling the
+// response.
+// Creates the URLRequest instance for fetching the URL from the log
+// (supplied as |request_url| in the c'tor) and implements the
+// URLRequest::Delegate interface for assembling the response.
+class LogProofFetcher::LogFetcher : public net::URLRequest::Delegate {
+ public:
+  using SuccessCallback = base::Callback<void(LogFetcher*)>;
+  using FailureCallback = base::Callback<void(LogFetcher*, int, int)>;
+  LogFetcher(net::URLRequestContext* request_context,
+             const GURL& request_url,
+             const SuccessCallback& success_callback,
+             const FailureCallback& failure_callback);
+  // Nothing explicit; The URLRequest instance held will be deleted
+  // automatically.

[Ryan Sleevi, 2015/12/08 01:13:52]

I'm having trouble trying to figure out what you're saying here.

[Eran Messeri, 2015/12/14 13:01:42]

Removed the comment ; I wanted to say there's no need to manually/explicitly
delete anything in the d'tor, but it's obvious from the empty body.

+  ~LogFetcher() override {}
+
+  // net::URLRequest::Delegate
+  void OnResponseStarted(net::URLRequest* request) override;
+  void OnReadCompleted(net::URLRequest* request, int bytes_read) override;
+
+  const std::string& assembled_response() { return assembled_response_; }
+
+ private:
+  // Handles the final result of a URLRequest::Read call on the request.
+  // Returns true if another read should be started, false if the read
+  // failed completely or we have to wait for OnResponseStarted to
+  // be called.
+  bool HandleReadResult(int bytes_read);
+
+  // Calls URLRequest::Read on |request| repeatedly, until HandleReadResult
+  // indicates it should no longer be called. Usually this would be when there
+  // is pending IO that requires waiting for OnResponseStarted to be called.
+  void StartNextRead();
+
+  // Invokes the success callback.
+  void RequestComplete();
+  // Invokes the failure callback with the supplied error information.

[Ryan Sleevi, 2015/12/08 01:13:52]

newline between 195 and 196

[Eran Messeri, 2015/12/14 13:01:42]

N/A anymore.

+  // After this method the LogFetcher instance may be deleted and |this|
+  // no longer valid.
+  void InvokeFailureCallback(int net_error, int http_response_code);
+
+  scoped_ptr<net::URLRequest> url_request_;
+  const GURL request_url_;
+  SuccessCallback success_callback_;
+  FailureCallback failure_callback_;
+  scoped_refptr<net::IOBufferWithSize> response_buffer_;
+  std::string assembled_response_;
+
+  DISALLOW_COPY_AND_ASSIGN(LogFetcher);
+};
+
+LogProofFetcher::LogFetcher::LogFetcher(net::URLRequestContext* request_context,
+                                        const GURL& request_url,
+                                        const SuccessCallback& success_callback,
+                                        const FailureCallback& failure_callback)
+    : request_url_(request_url),
+      success_callback_(success_callback),
+      failure_callback_(failure_callback),
+      response_buffer_(new net::IOBufferWithSize(
+          LogProofFetcher::kMaxLogResponseSizeInBytes)) {

[Ryan Sleevi, 2015/12/08 01:13:52]

Is this really sufficient? I didn't see any comments explaining how this value
was decided or when to grow it.

[Eran Messeri, 2015/12/14 13:01:42]

Good point - added documentation to the constant where it's defined. Turns out
more bytes are needed to accommodate consistency proofs between large trees.

+  DCHECK(request_url_.SchemeIsHTTPOrHTTPS());
+  url_request_ =
+      request_context->CreateRequest(request_url_, net::DEFAULT_PRIORITY, this);
+  // This request should not send any cookies or otherwise identifying data
+  // as CT logs are expected to be publicly-accessible and connections to them
+  // stateless.
+  url_request_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                             net::LOAD_DO_NOT_SAVE_COOKIES |
+                             net::LOAD_DO_NOT_SEND_AUTH_DATA);
+
+  url_request_->Start();
 }
 
-void LogProofFetcher::OnResponseStarted(net::URLRequest* request) {
+void LogProofFetcher::LogFetcher::OnResponseStarted(net::URLRequest* request) {
+  DCHECK_EQ(url_request_.get(), 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) {

[Ryan Sleevi, 2015/12/08 01:13:52]

mmenke: Is this right? What about situations of cached responses or any other
2XX series.

[mmenke, 2015/12/08 17:10:12]

Redirects don't result in calling this method - they're automatically followed
(Should they be, for these requests).  304s are replaced with the 200 or 3xx
redirect when we're reading from cache.  So this should be fine, unless there's
some reason we don't want a cached response, in which case we need a load flag
to prevent that.

[Eran Messeri, 2015/12/14 13:01:42]

Cached responses are fine for all replies from the log:
* For get-sth, cached responses are fine as long as cache expiration dates are
honoured. The log may use HTTP caching headers to tell clients there's no point
in asking for a new STH until the cache expires. 
* For get-consistency-proof the reply for a particular request (where the old
and new tree sizes are specified) is never going to change.

     int net_error = net::OK;
     int http_response_code = request->GetResponseCode();
 
-    DVLOG(1) << "Fetching STH from " << request->original_url()
+    DVLOG(1) << "Fetching STH from " << request_url_
              << " failed. status:" << status.status()
              << " error:" << status.error()
              << " http response code: " << http_response_code;

[Ryan Sleevi, 2015/12/08 01:13:52]

Seems unnecessary; the NetLog will already have this

[Eran Messeri, 2015/12/14 13:01:42]

Done.

     if (!status.is_success())
       net_error = GetNetErrorFromURLRequestStatus(status);
 
-    InvokeFailureCallback(request, net_error, http_response_code);
+    InvokeFailureCallback(net_error, http_response_code);
     return;
   }
 
-  StartNextRead(request, fetch_state);
+  StartNextRead();
 }
 
-void LogProofFetcher::OnReadCompleted(net::URLRequest* request,
-                                      int bytes_read) {
-  DCHECK(inflight_requests_.count(request));
-  FetchState* fetch_state = inflight_requests_.find(request)->second;
+void LogProofFetcher::LogFetcher::OnReadCompleted(net::URLRequest* request,
+                                                  int bytes_read) {
+  DCHECK_EQ(url_request_.get(), request);
 
-  if (HandleReadResult(request, fetch_state, bytes_read))
-    StartNextRead(request, fetch_state);
+  if (HandleReadResult(bytes_read))
+    StartNextRead();
 }
 
-bool LogProofFetcher::HandleReadResult(net::URLRequest* request,
-                                       FetchState* fetch_state,
-                                       int bytes_read) {
+bool LogProofFetcher::LogFetcher::HandleReadResult(int bytes_read) {
   // Start by checking for an error condition.
   // If there are errors, invoke the failure callback and clean up the
   // request.
-  if (bytes_read == -1 || !request->status().is_success()) {
-    net::URLRequestStatus status(request->status());
+  if (bytes_read == -1 || !url_request_->status().is_success()) {

[Ryan Sleevi, 2015/12/08 01:13:52]

Shouldn't this be bytes_read < 0 ?

[Eran Messeri, 2015/12/14 13:01:42]

Done.

+    net::URLRequestStatus status(url_request_->status());
     DVLOG(1) << "Read error: " << status.status() << " " << status.error();
-    InvokeFailureCallback(request, GetNetErrorFromURLRequestStatus(status),
-                          net::OK);
+    InvokeFailureCallback(GetNetErrorFromURLRequestStatus(status), net::OK);
 
     return false;
   }
 
   // Not an error, but no data available, so wait for OnReadCompleted
   // callback.
-  if (request->status().is_io_pending())
+  if (url_request_->status().is_io_pending())
     return false;
 
   // Nothing more to read from the stream - finish handling the response.
   if (bytes_read == 0) {
-    RequestComplete(request);
+    RequestComplete();
     return false;
   }
 
   // We have data, collect it and indicate another read is needed.
   DVLOG(1) << "Have " << bytes_read << " bytes to assemble.";

[Ryan Sleevi, 2015/12/08 01:13:52]

Unnecessary?

[Eran Messeri, 2015/12/14 13:01:42]

Removed, although I originally added it to identify the code has reached a
particular flow to handle partial response. 
Arguably this is not necessary anymore because the code works :)

   DCHECK_GE(bytes_read, 0);
-  fetch_state->assembled_response.append(fetch_state->response_buffer->data(),
-                                         bytes_read);
-  if (fetch_state->assembled_response.size() > kMaxLogResponseSizeInBytes) {
+  assembled_response_.append(response_buffer_->data(), bytes_read);

[Ryan Sleevi, 2015/12/08 01:13:52]

Suggestion: Alternatively write this as 

if (bytes_read > LogProofFetcher::kMaxLogResponseSizeInBytes ||
   (LogProofFetcher::kMaxLogResponseSizeInBytes - bytes_read) <
assembled_response_.size()) {
  ...
}

The reason for doing this is that it prevents you from doing .append() with the
'attacker' controlled size, potentially growing beyond what you're desiring
before you do the check.

[Eran Messeri, 2015/12/14 13:01:42]

Good point, done.

+  if (assembled_response_.size() >
+      LogProofFetcher::kMaxLogResponseSizeInBytes) {
     // Log response is too big, invoke the failure callback.
-    InvokeFailureCallback(request, net::ERR_FILE_TOO_BIG, net::HTTP_OK);
+    InvokeFailureCallback(net::ERR_FILE_TOO_BIG, net::HTTP_OK);
     return false;
   }
 
   return true;
 }
 
-void LogProofFetcher::StartNextRead(net::URLRequest* request,
-                                    FetchState* fetch_state) {
+void LogProofFetcher::LogFetcher::StartNextRead() {
   bool continue_reading = true;
   while (continue_reading) {
     int read_bytes = 0;
-    request->Read(fetch_state->response_buffer.get(),
-                  fetch_state->response_buffer->size(), &read_bytes);
-    continue_reading = HandleReadResult(request, fetch_state, read_bytes);
+    url_request_->Read(response_buffer_.get(), response_buffer_->size(),

[Ryan Sleevi, 2015/12/08 01:13:52]

1) Why aren't you checking this return code?
2) Why aren't you decreasing what you present as the max_bytes to read?

[mmenke, 2015/12/08 17:10:12]

url_request_->Read response codes are really weird and confusing. 
ResourceLoader, the primary consumer of the class, does the same thing.  Patches
welcome!

I'd really like to have it return an error code, like net does internally
everywhere (And maybe take a separate pointer to populate with the number of
bytes read, if any, rather than using positive numbers to indicate success), and
get rid of URLRequest::status().
We should have a test for that, if we don't already.

[Eran Messeri, 2015/12/14 13:01:42]

There is a test case covering a response that's too big:
TEST_F(LogProofFetcherTest, TestLogReplyIsTooLong

I did not decrease the value presented as max_bytes for code simplicity:
- Right now handling of a response that's bigger than kMaxLogResponseSizeInBytes
is done in HandleReadResult.
- If I were to do decrease max_bytes presented here I'd have to check if the
assembled_response_ size is exactly kMaxLogResponseSizeInBytes, otherwise I'd be
passing 0 as max_bytes to read.
- That will happen when the response is too big: The first
kMaxLogResponseSizeInBytes bytes will be read and put into the
assembled_response_. Another Read() must be performed anyway to determine if the
response was *exactly* kMaxLogResponseSizeInBytes or more.

Two options:
- Leaving the code as-is: The extra Read is performed the same way all other
reads do, with the same buffer size. Advantages: Same code path, easier to
follow. Disadvantages: We may consume at most 2 * kMaxLogResponseSizeInBytes
when the response is too big. 
- Special-case handling here to resize the response_buffer_ to 1 bytes just to
detect if the response is beyond kMaxLogResponesSizeInBytes. 

I argue that for code simplicity it's OK not to do special-case handling here
for the unlikely event that we'll use 2*kMaxLogResponseSizeInBytes bytes before
disposing of everything and invoking the error callback.

[mmenke, 2015/12/16 15:45:28]

Ah, right...we append to a string after each read, instead of continuing to read
into the buffer where the last read ended.  I'm not too concerned about the
extra memory + CPU cost of those appends.

+                       &read_bytes);
+    continue_reading = HandleReadResult(read_bytes);
   }
 }
 
-void LogProofFetcher::RequestComplete(net::URLRequest* request) {
-  DCHECK(inflight_requests_.count(request));
+void LogProofFetcher::LogFetcher::RequestComplete() {
+  // Get rid of the buffer as it really isn't necessary.
+  response_buffer_ = nullptr;
+  success_callback_.Run(this);
+}
 
-  FetchState* fetch_state = inflight_requests_.find(request)->second;
+void LogProofFetcher::LogFetcher::InvokeFailureCallback(
+    int net_error,
+    int http_response_code) {
+  failure_callback_.Run(this, net_error, http_response_code);
+  // NOTE: |this| not valid after this callback as the LogFetcher instance
+  // invoking the callback will be deleted by the callback.
+}
 
-  // Get rid of the buffer as it really isn't necessary.
-  fetch_state->response_buffer = nullptr;
+LogProofFetcher::LogProofFetcher(net::URLRequestContext* request_context)
+    : request_context_(request_context), weak_factory_(this) {
+  DCHECK(request_context);
+}
+
+LogProofFetcher::~LogProofFetcher() {
+  STLDeleteContainerPairPointers(inflight_fetches_.begin(),
+                                 inflight_fetches_.end());
+}
+
+void LogProofFetcher::FetchSignedTreeHead(
+    const GURL& base_log_url,
+    const std::string& log_id,
+    const SignedTreeHeadFetchedCallback& fetched_callback,
+    const FetchFailedCallback& failed_callback) {
+  const GURL request_url = base_log_url.Resolve("ct/v1/get-sth");
+  StartFetch(request_url, new GetSTHLogResponseHandler(log_id, fetched_callback,
+                                                       failed_callback));
+}
+
+void LogProofFetcher::FetchConsistencyProof(
+    const GURL& base_log_url,
+    const std::string& log_id,
+    uint64_t old_tree_size,
+    uint64_t new_tree_size,
+    const ConsistencyProofFetchedCallback& fetched_callback,
+    const FetchFailedCallback& failed_callback) {
+  const GURL request_url = base_log_url.Resolve(base::StringPrintf(
+      "ct/v1/get-sth-consistency?first=%" PRIu64 "&second=%" PRIu64,
+      old_tree_size, new_tree_size));
+  StartFetch(request_url, new GetConsistencyProofLogResponseHandler(
+                              log_id, fetched_callback, failed_callback));
+}
+
+void LogProofFetcher::StartFetch(const GURL& request_url,
+                                 LogResponseHandler* log_request) {
+  LogFetcher::SuccessCallback success_callback = base::Bind(
+      &LogProofFetcher::OnFetchCompleted, weak_factory_.GetWeakPtr());
+  LogFetcher::FailureCallback failure_callback =
+      base::Bind(&LogProofFetcher::OnFetchFailed, weak_factory_.GetWeakPtr());
+
+  LogFetcher* fetch = new LogFetcher(request_context_, request_url,
+                                     success_callback, failure_callback);
+  inflight_fetches_.insert(std::make_pair(fetch, log_request));
+}
+
+void LogProofFetcher::OnFetchCompleted(LogFetcher* fetcher) {
+  DCHECK(inflight_fetches_.find(fetcher) != inflight_fetches_.end());
+  // The |fetcher| will be deleted once JSON parsing will end.
+  // It must exist until then as it holds the assembled response buffer,
+  // whose reference is taken here.
   safe_json::SafeJsonParser::Parse(
-      fetch_state->assembled_response,
-      base::Bind(&LogProofFetcher::OnSTHJsonParseSuccess,
-                 weak_factory_.GetWeakPtr(), request),
-      base::Bind(&LogProofFetcher::OnSTHJsonParseError,
-                 weak_factory_.GetWeakPtr(), request));
-}
-
-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_.
-  STLDeleteContainerPairPointers(it, next_it);
-  inflight_requests_.erase(it);
-}
-
-void LogProofFetcher::InvokeFailureCallback(net::URLRequest* request,
-                                            int net_error,
-                                            int http_response_code) {
-  DCHECK(inflight_requests_.count(request));
-  auto it = inflight_requests_.find(request);
-  FetchState* fetch_state = it->second;
-
-  fetch_state->failed_callback.Run(fetch_state->log_id, net_error,
-                                   http_response_code);
-  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);
-  } else {
-    fetch_state->failed_callback.Run(fetch_state->log_id,
-                                     net::ERR_CT_STH_INCOMPLETE, net::HTTP_OK);
-  }
+      fetcher->assembled_response(),
+      base::Bind(&LogProofFetcher::OnJsonParseSuccess,
+                 weak_factory_.GetWeakPtr(), fetcher),
+      base::Bind(&LogProofFetcher::OnJsonParseError, weak_factory_.GetWeakPtr(),
+                 fetcher));
+}
+
+void LogProofFetcher::OnFetchFailed(LogFetcher* fetcher,
+                                    int net_error,
+                                    int http_response_code) {
+  DCHECK(inflight_fetches_.find(fetcher) != inflight_fetches_.end());
+  LogResponseHandler* handler = inflight_fetches_.find(fetcher)->second;
+  handler->HandleNetFailure(net_error, http_response_code);
+  CleanupRequest(fetcher);
+}
 
-  CleanupRequest(request);
+void LogProofFetcher::OnJsonParseSuccess(LogFetcher* fetcher,
+                                         scoped_ptr<base::Value> parsed_json) {
+  DCHECK(inflight_fetches_.find(fetcher) != inflight_fetches_.end());
+  LogResponseHandler* handler = inflight_fetches_.find(fetcher)->second;
+  handler->HandleParsedJson(*parsed_json);
+  CleanupRequest(fetcher);
 }
 
-void LogProofFetcher::OnSTHJsonParseError(net::URLRequest* request,
-                                          const std::string& error) {
-  InvokeFailureCallback(request, net::ERR_CT_STH_PARSING_FAILED, net::HTTP_OK);
+void LogProofFetcher::OnJsonParseError(LogFetcher* fetcher,
+                                       const std::string& error) {
+  DCHECK(inflight_fetches_.find(fetcher) != inflight_fetches_.end());
+  LogResponseHandler* handler = inflight_fetches_.find(fetcher)->second;
+  handler->HandleJsonParseFailure(error);
+  CleanupRequest(fetcher);
+}
+
+void LogProofFetcher::CleanupRequest(LogFetcher* fetcher) {
+  auto it = inflight_fetches_.find(fetcher);
+  DCHECK(it != inflight_fetches_.end());
+
+  auto next = it;
+  std::advance(next, 1);
+
+  STLDeleteContainerPairPointers(it, next);
+  inflight_fetches_.erase(it);
 }
 
 }  // namespace certificate_transparency