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 {
+
+struct LogProofFetcher::FetchParams {

[davidben, 2015/07/16 21:53:38]

FetchState perhaps, since you're modifying it and such.

[Eran Messeri, 2015/07/17 15:40:23]

Done.

+  explicit FetchParams(const std::string& id,

[davidben, 2015/07/16 21:53:38]

This doesn't need the explicit.

[Eran Messeri, 2015/07/17 15:40:23]

Done.

+                       FetchSTHCallback fetched_callback,
+                       FetchFailedCallback failed_callback);
+  ~FetchParams();
+
+  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) {

[davidben, 2015/07/16 21:53:38]

I think you can just define these methods all inline.

[Eran Messeri, 2015/07/17 15:40:23]

By "these methods", do you mean c'tor and d'tor? I thought this was strongly
discouraged:
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i....

+  DCHECK(request_context);
+}
+
+LogProofFetcher::~LogProofFetcher() {
+  for (auto it = inflight_requests_.begin(); it != inflight_requests_.end();
+       ++it)
+    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"));
+  scoped_ptr<net::URLRequest> request = CreateURLRequest(fetch_url);
+
+  FetchParams* params = new FetchParams(log_id, fetched_cb, failed_cb);
+  auto it = inflight_requests_.insert(std::make_pair(request.release(), params))
+                .first;
+  it->first->Start();
+}
+
+void LogProofFetcher::OnResponseStarted(net::URLRequest* request) {
+  const net::URLRequestStatus& status(request->status());

[davidben, 2015/07/16 21:53:39]

Nit: I'd probably just use equals rather than parens for these. (You use equals
in line 60.)

[Eran Messeri, 2015/07/17 15:40:23]

Using equals throughout (except when a raw pointer is stored in a scoped_ptr).

+  auto it(inflight_requests_.find(request));
+  auto params(it->second);

[davidben, 2015/07/16 21:53:38]

Mind spelling the type out for this one? I think it's useful to be able to see
it's a FetchParams*. (Whereas |it|'s type is totally useless.)

[Eran Messeri, 2015/07/17 15:40:23]

Done.

+
+  DCHECK(it != inflight_requests_.end());
+  if (!status.is_success() || request->GetResponseCode() != 200) {
+    std::string error_string;
+    if (!status.is_success()) {
+      DVLOG(1) << "Fetching STH from " << request->original_url()
+               << " failed. status:" << status.status()
+               << " error:" << status.error();
+      error_string =
+          base::StringPrintf("%d %d", status.status(), status.error());

[davidben, 2015/07/16 21:53:39]

This seems a slightly unhelpful error string. Where are you envisioning the
error being used?

(This is unfortunately part of //net that's kind of a mess, but we're trying to
get rid of status.status() anyway. We should finish killing that... it *should*
be redundant with status.error() but isn't quiiiite. domain_reliability has this
crazy thing:

https://code.google.com/p/chromium/codesearch#chromium/src/components/domain_...

One thought is to do that and then using net::ErrorToString. Though I dunno what
you're planning on doing with it.

Alternatively, not having an error is stupid, so
https://codereview.chromium.org/1239993004/ should take care of it.)

[Eran Messeri, 2015/07/17 15:40:23]

You're right - the error string is unhelpful, as I expect the callback to
ultimately log something to UMA so an integer / enum here is more appropriate.
In fact, what I need is very similar to what domain_reliability has.

+    } else {  // request->GetResponseCode != 200
+      DVLOG(1) << "Fetch STH HTTP status: " << request->GetResponseCode();
+      error_string = base::StringPrintf("HTTP %d", request->GetResponseCode());
+    }
+
+    scoped_ptr<FetchParams> params(CleanupRequest(request));
+    params->failed_callback.Run(params->log_id, error_string);
+    return;
+  }
+
+  bool continue_reading = true;
+  while (continue_reading) {
+    KickOffARead(request, params);
+    continue_reading = CheckReadStatus(request, params);
+  }

[davidben, 2015/07/16 21:53:39]

This Read loop is somewhat interesting. :-) It doesn't help that
URLRequest::Read is kinda ugly. I asked mmenke, and he recommended just using
URLFetcher. It might save you some of this grief.

[Eran Messeri, 2015/07/17 15:40:23]

Interesting as in "there are some odd edge cases it spectacularly fails to
handle" or as in "give me a few minutes and I'll find some odd edge cases it
spectacularly fails to handle" ? :)

Re URLRequest::Delegate vs URLFetcher, see the mail I've followed-up on with why
I think a URLFetcher would be painful to use here. But if there's a decent way
to wire it in then I could switch to using that.
One option I can think of is having my own URLRequestContextGetter that is *not*
the same one as the IOThread / ProfileIOData have. It should be OK because I
know this object will be freed up before the URLRequestContext it holds is
destroyed. Do you agree with this proposed design approach?

+}
+
+void LogProofFetcher::OnReadCompleted(net::URLRequest* request,
+                                      int bytes_read) {
+  auto it(inflight_requests_.find(request));
+  DCHECK(it != inflight_requests_.end());
+  FetchParams* params(it->second);
+
+  params->read_bytes = bytes_read;
+
+  bool another_read = CheckReadStatus(request, params);
+
+  while (another_read) {
+    KickOffARead(request, params);
+    another_read = CheckReadStatus(request, params);
+  }
+}
+
+bool LogProofFetcher::CheckReadStatus(net::URLRequest* request,
+                                      FetchParams* 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();
+    DVLOG(1) << "Read error: " << status.status() << " " << status.error();
+    scoped_ptr<FetchParams> params(CleanupRequest(request));
+    std::string error_string(
+        base::StringPrintf("%d %d", status.status(), status.error()));

[davidben, 2015/07/16 21:53:38]

Ditto about not being a useful error string.

[Eran Messeri, 2015/07/17 15:40:23]

Done.

+    params->failed_callback.Run(params->log_id, error_string);
+    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.";
+    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);
+  base::StringPiece sth_fragment(params->response_buffer->data(), bytes_read);
+  sth_fragment.AppendToString(&params->assembled_response);
+  params->total_response_size += bytes_read;
+  // Indicate this fragment was read.
+  params->read_bytes = 0;
+  return true;
+}
+
+void LogProofFetcher::KickOffARead(net::URLRequest* request,
+                                   FetchParams* 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) {
+    NOTREACHED();
+    return;
+  }
+
+  FetchParams* params = it->second;
+
+  // Extract STH json an parse it.
+  DCHECK(params->total_response_size < kMaxLogResponseSizeInBytes);
+
+  // Explicitly copy the request params as it'll be cleaned up by
+  // CleanupRequest later on.
+  FetchParams 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::FetchParams> 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<FetchParams> 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(
+    FetchParams params,
+    scoped_ptr<base::Value> parsed_json) {
+  net::ct::SignedTreeHead sth;
+  if (!net::ct::FillSignedTreeHead(*parsed_json.get(), &sth)) {
+    params.failed_callback.Run(params.log_id, "Invalid sth.");
+    return;
+  }
+
+  params.fetched_callback.Run(params.log_id, sth);
+}
+
+void LogProofFetcher::OnSTHJsonParseError(FetchParams params,
+                                          const std::string& error) {
+  params.failed_callback.Run(params.log_id, error);
+}
+
+LogProofFetcher::FetchParams::FetchParams(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::FetchParams::~FetchParams() {
+}
+
+}  // namespace certificate_transparency