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/http/http_status_code.h"
+#include "net/url_request/url_request_context.h"
+
+const unsigned long kMaxLogResponseSizeInBytes = 600u;

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

This should be static, or in an anonymous namespace.

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

This should be size_t, to match IOBufferWithSize argument size.  Also, "long"
and "unsigned" aren't used much.  If an int isn't enough, prefer uint32_t /
uint64_t, etc.

[Eran Messeri, 2015/07/31 12:55:52]

Done.

[Eran Messeri, 2015/07/31 12:55:52]

Done.

+
+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,
+             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;
+  int read_bytes;

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

This member is now serving no purpose, and can be removed.

[Eran Messeri, 2015/07/31 12:55:52]

Done.

+  std::string assembled_response;
+};
+
+LogProofFetcher::FetchState::FetchState(
+    const std::string& id,
+    const SignedTreeHeadFetchedCallback& fetched_callback,
+    const FetchFailedCallback& failed_callback)
+    : log_id(id),
+      fetched_callback(fetched_callback),
+      failed_callback(failed_callback),
+      response_buffer(new net::IOBufferWithSize(kMaxLogResponseSizeInBytes)) {}
+
+LogProofFetcher::FetchState::~FetchState() {}
+
+LogProofFetcher::LogProofFetcher(net::URLRequestContext* request_context)
+    : request_context_(request_context), weak_factory_(this) {
+  DCHECK(request_context);
+}
+
+LogProofFetcher::~LogProofFetcher() {
+  STLDeleteContainerPairPointers(inflight_requests_.begin(),
+                                 inflight_requests_.end());
+}
+
+void LogProofFetcher::FetchSignedTreeHead(
+    const GURL& base_log_url,
+    const std::string& log_id,
+    const SignedTreeHeadFetchedCallback& fetched_callback,
+    const FetchFailedCallback& failed_callback) {

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

Is there some check upstream that base_log_url is HTTP or HTTPS?  If so, should
DCHECK that's the case here.  If not, should probably just fail the request if
that's not the case.

[Eran Messeri, 2015/07/31 12:55:52]

Done - added DCHECK. The log URLs are hard-coded into Chrome
(https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_known_...)
so the DCHECK should be enough.

+  GURL fetch_url(base_log_url.Resolve("ct/v1/get-sth"));
+  scoped_ptr<net::URLRequest> request = CreateURLRequest(fetch_url);
+
+  FetchState* fetch_state =
+      new FetchState(log_id, fetched_callback, failed_callback);
+  request->Start();
+  inflight_requests_.insert(std::make_pair(request.release(), fetch_state));
+}
+
+void LogProofFetcher::OnResponseStarted(net::URLRequest* request) {

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

I assume we should be following redirects, if any?  (This code will do that,
just verifying it's what we want)

[Eran Messeri, 2015/07/31 12:55:51]

Yes - it is allowed for a CT log to redirect.

+  net::URLRequestStatus status(request->status());
+  DCHECK(inflight_requests_.count(request));
+
+  FetchState* fetch_state = inflight_requests_.find(request)->second;
+
+  if (!status.is_success() || request->GetResponseCode() != 200) {

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

200 -> net::HTTP_OK?  Should be consistent within the file.

[Eran Messeri, 2015/07/31 12:55:52]

Done.

+    int error_value = 0;

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

0 -> net::OK

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

Suggest calling this net_error here, and in the declaration of FailedCallback.

[Eran Messeri, 2015/07/31 12:55:52]

Done.

[Eran Messeri, 2015/07/31 12:55:52]

Done.

+    int http_response_code = request->GetResponseCode();
+    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

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

I suggest removing this comment - it's confusing.  I would have guessed it
applies to the following code, not the preceding code, which is not the case.

[Eran Messeri, 2015/07/31 12:55:51]

Done.

+      DVLOG(1) << "Fetch STH HTTP status: " << http_response_code;
+      error_value = net::OK;

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

Remove the error_value = line.

[Eran Messeri, 2015/07/31 12:55:52]

Done, with it goes the entire else clause.

+    }
+
+    fetch_state->failed_callback.Run(fetch_state->log_id, error_value,
+                                     http_response_code);
+    CleanupRequest(request);
+    return;
+  }
+
+  KickOffARead(request, fetch_state, true);
+}
+
+void LogProofFetcher::OnReadCompleted(net::URLRequest* request,
+                                      int bytes_read) {
+  DCHECK(inflight_requests_.count(request));
+  FetchState* fetch_state = inflight_requests_.find(request)->second;
+
+  bool another_read = HandleReadResult(request, fetch_state, bytes_read);

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

"continue_reading" seems a bit clearer to me, and matches what
LogProofFetcher::KickOffARead calls the value.

[Eran Messeri, 2015/07/31 12:55:51]

Done.

+  KickOffARead(request, fetch_state, another_read);

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

Having a bool that means "Should I really kick off a read" passed to
KickOffARead seems weird.  Suggest getting rid of the last argument to
KickOffARead, and just not calling it if another_read is false.

[Eran Messeri, 2015/07/31 12:55:52]

Good point, apologies for the clunkiness in the first place.

+}
+
+bool LogProofFetcher::HandleReadResult(net::URLRequest* request,
+                                       FetchState* params,
+                                       const int bytes_read) {

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

Remove const on bytes_read.

[Eran Messeri, 2015/07/31 12:55:51]

Done.

+  // Start by checking for an error condition.
+  if (bytes_read == -1 || !request->status().is_success()) {
+    net::URLRequestStatus status(request->status());
+    DVLOG(1) << "Read error: " << status.status() << " " << status.error();
+    int error_value = GetNetErrorFromURLRequestStatus(status);

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

again, suggest net_error

[Eran Messeri, 2015/07/31 12:55:51]

Done.

+    params->failed_callback.Run(params->log_id, error_value, 0);
+    CleanupRequest(request);
+    return false;
+  }
+
+  // Not an error, but no data available, so wait for OnReadComplete

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

OnReadCompleted

[Eran Messeri, 2015/07/31 12:55:52]

Done.

+  // callback.
+  if (request->status().is_io_pending()) {
+    return false;
+  }

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

optional:  Prevailing style in net/ is not to use braces in this case.

[Eran Messeri, 2015/07/31 12:55:52]

Done, removed  braces throughout the file where there was only one statement.

+
+  // Nothing more to read from the stream - finish handling the response.
+  if (bytes_read == 0) {

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

Optional:  May want to handle the complete case last, so that the code is in the
order it occurs.  Of course, this way, total indentation is reduced, and the
true path is at the end...

[Eran Messeri, 2015/07/31 12:55:51]

Unless you strongly object, I'd like to keep it this way - I realize it's a
matter of personal preference, I personally find code easier to read when
there's less indentation. 

+    RequestComplete(request);
+    return false;
+  }
+
+  // We have data, collect it and indicate another read is needed.
+  DVLOG(1) << "Have " << bytes_read << " bytes to assemble.";
+  DCHECK_GE(bytes_read, 0);
+  params->assembled_response.append(params->response_buffer->data(),
+                                    bytes_read);
+  // Indicate this fragment was read.
+  params->read_bytes = 0;
+  return true;
+}
+
+void LogProofFetcher::KickOffARead(net::URLRequest* request,
+                                   FetchState* fetch_state,
+                                   bool should_read) {
+  bool continue_reading = should_read;
+  while (continue_reading) {

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

Per earlier comment, should remove the should_read argument.  Suggest just
making this while(true) as well.

[Eran Messeri, 2015/07/31 12:55:52]

Removed the should_read argument. Don't have a strong feeling about while(true)
- will change if you prefer it this way.

+    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);
+  }
+}
+
+void LogProofFetcher::RequestComplete(net::URLRequest* request) {
+  DCHECK(inflight_requests_.count(request));
+
+  FetchState* params = inflight_requests_.find(request)->second;
+
+  // Extract STH json an parse it.
+  DCHECK_LT(params->assembled_response.size(), kMaxLogResponseSizeInBytes);

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

BUG:  While you're making sure each read is <= (Not less than)
kMaxLogResponseSizeInBytes, you have no code to make sure they're less than
kMaxLogResponseSizeInBytes once you append them all together.

Also, you should add a test for this case:  Bot a response exactly
kMaxLogResponseSizeInBytes and a longer response.

[Eran Messeri, 2015/07/31 12:55:52]

Good catch, changed the code to check each time some bytes are received that the
total does not exceed kMaxLogResponseSizeInBytes (in HandleReadResult).
Added a test as well.

+
+  // 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));

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

You're copying request_params 3 times in this function, which seems a bit much. 
Maybe just keep the request around, and pass along a pointer to it?  Or could
just pass in the callback and the log_id (Or even use base::Bind with the
callback and log id and just pass in the new callback, I suppose).

[Eran Messeri, 2015/07/31 12:55:52]

One of the copies was redundant and removed, so now the parameters will be
copied twice.
Note that the callback for successful JSON parsing (OnSTHJsonParseSuccess) needs
access to the error callback in case the JSON parsed OK but is missing fields to
fill in the SignedTreeHead.
In addition to the response_buffer, I've removed the assembled_response so it's
not copied when the fetch_state is copied to the callbacks.
That should imply that copying the log_id and callbacks around costs almost the
same as copying the FetchState structure, because these are the only things left
there, right? 

+
+  CleanupRequest(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());
+
+  // So that the fetch state will be deleted upon exiting.
+  scoped_ptr<FetchState> params(it->second);
+  // Delete the request, remove from map.
+  scoped_ptr<net::URLRequest> url_request(it->first);

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

Suggest just deleting these - I don't think the scoped_ptrs give us much.  Could
use STLDeleteContainerPairPointers(it, it->next()), but also not sure that gets
us anything.

[Eran Messeri, 2015/07/31 12:55:52]

Done.

+  // Both pointers will be deleted when exiting the method.
+  inflight_requests_.erase(it);
+}
+
+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");

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

Not needed.  "GET" is the default method.

[Eran Messeri, 2015/07/31 12:55:52]

Done.

+  return request.Pass();
+}
+
+void LogProofFetcher::OnSTHJsonParseSuccess(
+    FetchState fetch_state,
+    scoped_ptr<base::Value> parsed_json) {
+  net::ct::SignedTreeHead signed_tree_head;
+  if (!net::ct::FillSignedTreeHead(*parsed_json.get(), &signed_tree_head)) {
+    fetch_state.failed_callback.Run(fetch_state.log_id,
+                                    net::ERR_CT_STH_INCOMPLETE, net::HTTP_OK);
+    return;
+  }
+
+  fetch_state.fetched_callback.Run(fetch_state.log_id, signed_tree_head);
+}
+
+void LogProofFetcher::OnSTHJsonParseError(FetchState params,
+                                          const std::string& error) {
+  params.failed_callback.Run(params.log_id, net::ERR_CT_STH_PARSING_FAILED,
+                             net::HTTP_OK);
+}
+
+}  // namespace certificate_transparency