Certificate Transparency: Add STH Fetching capability.

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 "base/logging.h"
+#include "base/stl_util.h"
+#include "base/strings/string_piece.h"
+#include "base/strings/stringprintf.h"
+#include "base/values.h"
+#include "components/safe_json/safe_json_parser.h"
+#include "net/base/load_flags.h"
+#include "net/base/request_priority.h"
+#include "net/cert/ct_log_response_parser.h"
+#include "net/cert/ct_log_verifier.h"
+#include "net/cert/signed_tree_head.h"
+#include "net/url_request/url_request_context.h"
+
+const int kMaxLogResponseSizeInBytes = 600;
+
+namespace certificate_transparency {
+
+namespace {
+
+// Shamelessly copied from domain_reliability/util.cc
+int GetNetErrorFromURLRequestStatus(const net::URLRequestStatus& status) {
+  switch (status.status()) {
+    case net::URLRequestStatus::SUCCESS:
+      return net::OK;
+    case net::URLRequestStatus::CANCELED:
+      return net::ERR_ABORTED;
+    case net::URLRequestStatus::FAILED:
+      return status.error();
+    default:
+      NOTREACHED();
+      return net::ERR_FAILED;
+  }
+}
+
+}  // namespace
+
+struct LogProofFetcher::FetchState {
+  FetchState(const std::string& id,
+             FetchSTHCallback fetched_callback,
+             FetchFailedCallback failed_callback);
+  ~FetchState();
+
+  std::string log_id;
+  FetchSTHCallback fetched_callback;
+  FetchFailedCallback failed_callback;
+  scoped_refptr<net::IOBufferWithSize> response_buffer;
+  int read_bytes;
+  std::string assembled_response;
+  int total_response_size;
+};
+
+LogProofFetcher::LogProofFetcher(net::URLRequestContext* request_context)
+    : request_context_(request_context), weak_factory_(this) {
+  DCHECK(request_context);
+}
+
+LogProofFetcher::~LogProofFetcher() {
+  for (auto it = inflight_requests_.begin(); it != inflight_requests_.end();
+       ++it)

[mmenke, 2015/07/17 17:57:47]

Can just delete these - no need to cancel first.  (Should probably have a
comment about that, though)

[Eran Messeri, 2015/07/29 15:16:58]

Done.

+    it->first->Cancel();
+  STLDeleteContainerPairPointers(inflight_requests_.begin(),
+                                 inflight_requests_.end());
+}
+
+void LogProofFetcher::FetchSTH(const GURL& log_url,
+                               const std::string& log_id,
+                               FetchSTHCallback fetched_cb,
+                               FetchFailedCallback failed_cb) {
+  GURL fetch_url(log_url.Resolve("ct/v1/get-sth"));

[mmenke, 2015/07/17 17:57:47]

log_url -> base_log_url, maybe?  I assume we really do want to take in a full
URL here, and not just a hostname.

[Eran Messeri, 2015/07/29 15:16:59]

Correct, we do want to take in a full URL, not just a hostname (multiple logs
can be hosted on the same host).
Changed to base_log_url.

+  scoped_ptr<net::URLRequest> request = CreateURLRequest(fetch_url);
+
+  FetchState* fetch_state = new FetchState(log_id, fetched_cb, failed_cb);
+  auto it = inflight_requests_.insert(std::make_pair(request.release(),
+                                                     fetch_state)).first;
+  it->first->Start();

[mmenke, 2015/07/17 17:57:46]

I think this is sufficiently ugly that you may just want to start the request
before adding it to inflight_requests_ (URLRequest won't invoke any callbacks
synchronously).

[Eran Messeri, 2015/07/29 15:16:58]

Done.

+}
+
+void LogProofFetcher::OnResponseStarted(net::URLRequest* request) {
+  const net::URLRequestStatus& status(request->status());

[mmenke, 2015/07/17 17:57:46]

Given that this is just a pair of ints, suggest just copying it, or using
request->status() everywhere.

This is just because references aren't use a whole lot in chrome, unless they're
really needed (Except for function arguments and accessor return vales).

[Eran Messeri, 2015/07/29 15:16:58]

Done - copied the value.

+  auto it = inflight_requests_.find(request);

[mmenke, 2015/07/17 17:57:47]

optional:  Suggest avoiding the use of auto unless it's needed - at least I find
having to mentally map it to std::pair(blah, blah) adds cognitive overhead.

So I'd suggest something like
DCHECK(inflight_requests_.count(request));

FetchState* fetch_state = inflight_requests_.find(request)->second;

[Eran Messeri, 2015/07/29 15:16:58]

Done - adopted your suggestion.

+  FetchState* fetch_state = it->second;
+
+  DCHECK(it != inflight_requests_.end());

[mmenke, 2015/07/17 17:57:47]

This DCHECK should go before you potentially dereference end()->second.

[Eran Messeri, 2015/07/29 15:16:58]

Done.

+  if (!status.is_success() || request->GetResponseCode() != 200) {
+    int error_value = 0;
+    if (!status.is_success()) {
+      DVLOG(1) << "Fetching STH from " << request->original_url()
+               << " failed. status:" << status.status()
+               << " error:" << status.error();
+      error_value = GetNetErrorFromURLRequestStatus(status);
+    } else {  // request->GetResponseCode != 200
+      DVLOG(1) << "Fetch STH HTTP status: " << request->GetResponseCode();
+      error_value = request->GetResponseCode();

[mmenke, 2015/07/17 17:57:47]

Packing in a net error and an HTTP response code in the same field just seems
like asking for trouble.

What do we expect to use this for?

[Eran Messeri, 2015/07/29 15:16:58]

You're right - I thought I saw this pattern somewhere else but I was,
apparently, mistaken.
Now separated to net error value and http response code.
It will be used to keep track of the reasons communication to a particular CT
log is failing - ultimately reported through UMA (or a similar mechanism) for
each individual CT log.

+    }
+
+    scoped_ptr<FetchState> params = CleanupRequest(request);
+    params->failed_callback.Run(params->log_id, error_value);

[mmenke, 2015/07/17 17:57:47]

Can we just invoke the callback before calling CleanupRequest, instead of having
CleanupRequest return a scoped_ptr?

[Eran Messeri, 2015/07/29 15:16:58]

Yes, done.

+    return;
+  }
+
+  bool continue_reading = true;
+  while (continue_reading) {
+    KickOffARead(request, fetch_state);
+    continue_reading = CheckReadStatus(request, fetch_state);
+  }
+}
+
+void LogProofFetcher::OnReadCompleted(net::URLRequest* request,
+                                      int bytes_read) {
+  auto it = inflight_requests_.find(request);
+  DCHECK(it != inflight_requests_.end());
+  FetchState* params = it->second;
+
+  params->read_bytes = bytes_read;

[mmenke, 2015/07/17 17:57:47]

I don't think you need the read_bytes field.  Can just pass bytes read directly
to CheckReadStatus.

[Eran Messeri, 2015/07/29 15:16:58]

Changed to pass the bytes_read into HandleReadResult.

+
+  bool another_read = CheckReadStatus(request, params);
+
+  while (another_read) {
+    KickOffARead(request, params);
+    another_read = CheckReadStatus(request, params);
+  }

[mmenke, 2015/07/17 17:57:47]

This exactly matches the end of LogProofFetcher::OnResponseStarted.  I suggest
moving it into KickOffARead.

[Eran Messeri, 2015/07/29 15:16:59]

Done.

+}
+
+bool LogProofFetcher::CheckReadStatus(net::URLRequest* request,

[mmenke, 2015/07/17 17:57:46]

HandleReadResult maybe?

[Eran Messeri, 2015/07/29 15:16:59]

Done.

+                                      FetchState* params) {
+  const int bytes_read = params->read_bytes;
+  // Start by checking for an error condition.
+  if (bytes_read == -1 || !request->status().is_success()) {
+    auto status = request->status();

[mmenke, 2015/07/17 17:57:47]

Writing out net::URLRequestStatus isn't a huge task, or just inlining
request->status() everywhere.

[Eran Messeri, 2015/07/29 15:16:58]

Done.

+    DVLOG(1) << "Read error: " << status.status() << " " << status.error();
+    scoped_ptr<FetchState> params(CleanupRequest(request));
+    int error_value = GetNetErrorFromURLRequestStatus(status);
+    params->failed_callback.Run(params->log_id, error_value);
+    return false;
+  }
+
+  // Not an error, but no data available, so wait for OnReadComplete
+  // callback.
+  if (request->status().is_io_pending()) {
+    DVLOG(1) << "IO Pending: Will wait.";
+    return false;
+  }
+
+  // Nothing more to read from the stream - finish handling the response.
+  if (bytes_read == 0) {
+    DVLOG(1) << "No more bytes to read, finishing.";

[mmenke, 2015/07/17 17:57:46]

Do you plan on keeping all these log statements around?

[Eran Messeri, 2015/07/29 15:16:58]

I've removed some, what would you recommend leaving and what should be removed?

[mmenke, 2015/07/29 18:51:50]

I mostly use breakpoint debugging, so not a big fan of having them in general,
particularly when they require you to add more control statements.  Others
actually use them a lot, and have a rather different opinion.  Don't feel like I
can offer much guidance here.

+    RequestComplete(request);
+    return false;
+  }
+
+  // We have data, collect it and indicate another read is needed.
+  DVLOG(1) << "Have " << bytes_read << " bytes to assemble.";
+  DCHECK(bytes_read > 0);

[mmenke, 2015/07/17 17:57:47]

DCHECK_GE

[Eran Messeri, 2015/07/29 15:16:58]

Done.

+  base::StringPiece sth_fragment(params->response_buffer->data(), bytes_read);
+  sth_fragment.AppendToString(&params->assembled_response);

[mmenke, 2015/07/17 17:57:47]

Why not just use:

params->assumbed_response.append(params->response_buffer->data(), bytes_read);

[Eran Messeri, 2015/07/29 15:16:58]

Done.

+  params->total_response_size += bytes_read;

[mmenke, 2015/07/17 17:57:47]

Why is this needed?  Doesn't params->assumbed_response.length() or size() give
you what you want?

[Eran Messeri, 2015/07/29 15:16:58]

Yes, removed.

+  // Indicate this fragment was read.
+  params->read_bytes = 0;
+  return true;
+}
+
+void LogProofFetcher::KickOffARead(net::URLRequest* request,
+                                   FetchState* params) {
+  params->read_bytes = 0;
+  request->Read(params->response_buffer.get(), params->response_buffer->size(),
+                &params->read_bytes);
+}
+
+void LogProofFetcher::RequestComplete(net::URLRequest* request) {
+  auto it(inflight_requests_.find(request));
+  DCHECK(it != inflight_requests_.end());
+  if (it->first != request) {

[mmenke, 2015/07/17 17:57:47]

Get rid of this branch - shouldn't handle cases that shouldn't be possible. 
Better to crash and find the bug.  Also, this is a really weird check to rely
on.  If the DCHECK above passes, and this fails, presumably it's an std::map
bug....

[Eran Messeri, 2015/07/29 15:16:58]

My bad, this is a leftover check from an older version of the code.

+    NOTREACHED();
+    return;
+  }
+
+  FetchState* params = it->second;
+
+  // Extract STH json an parse it.
+  DCHECK(params->total_response_size < kMaxLogResponseSizeInBytes);

[mmenke, 2015/07/17 17:57:46]

DCHECK_LT will produce more useful output on trybots.

[Eran Messeri, 2015/07/29 15:16:58]

Done.

+
+  // Explicitly copy the request params as it'll be cleaned up by
+  // CleanupRequest later on.
+  FetchState request_params = *params;
+  // Get rid of the buffer as it really isn't necessary.
+  request_params.response_buffer = nullptr;
+  safe_json::SafeJsonParser::Parse(
+      params->assembled_response,
+      base::Bind(&LogProofFetcher::OnSTHJsonParseSuccess,
+                 weak_factory_.GetWeakPtr(), request_params),
+      base::Bind(&LogProofFetcher::OnSTHJsonParseError,
+                 weak_factory_.GetWeakPtr(), request_params));
+
+  CleanupRequest(request);
+}
+
+scoped_ptr<LogProofFetcher::FetchState> 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());
+
+  scoped_ptr<FetchState> params(it->second);
+  // Delete the request, remove from map.
+  scoped_ptr<net::URLRequest> url_request(it->first);
+  // Both pointers will be deleted when exiting the method.
+  inflight_requests_.erase(it);
+  return params.Pass();
+}
+
+scoped_ptr<net::URLRequest> LogProofFetcher::CreateURLRequest(
+    const GURL& fetch_sth_url) {
+  DCHECK(request_context_);
+
+  scoped_ptr<net::URLRequest> request = request_context_->CreateRequest(
+      fetch_sth_url, net::DEFAULT_PRIORITY, this);
+  request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                        net::LOAD_DO_NOT_SAVE_COOKIES);
+  request->set_method("GET");
+  return request.Pass();
+}
+
+void LogProofFetcher::OnSTHJsonParseSuccess(
+    FetchState params,
+    scoped_ptr<base::Value> parsed_json) {
+  net::ct::SignedTreeHead sth;

[mmenke, 2015/07/17 17:57:46]

Again, suggest writing this out.

[Eran Messeri, 2015/07/29 15:16:58]

Done.

+  if (!net::ct::FillSignedTreeHead(*parsed_json.get(), &sth)) {
+    params.failed_callback.Run(params.log_id, net::ERR_CT_STH_INCOMPLETE);
+    return;
+  }
+
+  params.fetched_callback.Run(params.log_id, sth);
+}
+
+void LogProofFetcher::OnSTHJsonParseError(FetchState params,
+                                          const std::string& error) {
+  params.failed_callback.Run(params.log_id, net::ERR_CT_STH_PARSING_FAILED);
+}
+
+LogProofFetcher::FetchState::FetchState(const std::string& id,
+                                        FetchSTHCallback fetched_callback,
+                                        FetchFailedCallback failed_callback)
+    : log_id(id),
+      fetched_callback(fetched_callback),
+      failed_callback(failed_callback),
+      response_buffer(new net::IOBufferWithSize(kMaxLogResponseSizeInBytes)),
+      total_response_size(0) {
+}
+
+LogProofFetcher::FetchState::~FetchState() {
+}

[mmenke, 2015/07/17 17:57:47]

These should be up with the declaration of FetchState.

[Eran Messeri, 2015/07/29 15:16:58]

Done.

+
+}  // namespace certificate_transparency