Certificate Transparency: Fetching of Signed Tree Heads (DRAFT)

components/certificate_transparency/log_proofs_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.

[davidben, 2015/05/07 21:59:36]

Since this already has some significant logic now, unit test?

[Eran Messeri, 2015/07/10 13:15:47]

Acknowledged - will add to a later patchset. Any examples you can point to of
tests using a mock/test URLRequestContext?
(I see there's a TestURLRequestContext so will look up myself if you don't have
anything off the top of your head)

+
+#include "components/certificate_transparency/log_proofs_fetcher.h"
+
+#include "base/base64.h"
+#include "base/json/json_reader.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/strings/string_piece.h"
+#include "base/time/time.h"
+#include "base/values.h"
+#include "net/base/load_flags.h"
+#include "net/base/request_priority.h"
+#include "net/cert/ct_log_verifier.h"
+#include "net/cert/ct_serialization.h"
+#include "net/cert/signed_tree_head.h"
+#include "net/url_request/url_request_context.h"
+
+const int kMaxSTHSize = 600;

[davidben, 2015/05/07 21:59:37]

Probably worth a comment. Notably mention that it's in bytes.

[Eran Messeri, 2015/07/10 13:15:47]

Done.

+
+namespace certificate_transparency {
+
+LogProofsFetcher::LogProofsFetcher(net::URLRequestContext* request_context)
+    : request_context_(request_context) {
+}
+
+LogProofsFetcher::~LogProofsFetcher() {
+  STLDeleteContainerPairPointers(inflight_requests_.begin(),
+                                 inflight_requests_.end());
+}
+
+scoped_ptr<net::URLRequest> LogProofsFetcher::CreateURLRequest(

[davidben, 2015/05/07 21:59:37]

Nit: This doesn't match header file order.

[Eran Messeri, 2015/07/10 13:15:47]

Done.

+    GURL fetch_sth_url) {
+  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);
+  return request.Pass();
+}
+
+void LogProofsFetcher::FetchSTH(net::CTLogVerifier* verifier) {
+  GURL fetch_url(verifier->url().Resolve("/get-sth"));
+  scoped_ptr<net::URLRequest> request = CreateURLRequest(fetch_url);
+  request->set_method("GET");

[davidben, 2015/05/07 21:59:36]

Nit: Somewhat strange separation between CreateRequest and FetchSTH; the former
sets the URL while the latter sets the method.

[Eran Messeri, 2015/07/10 13:15:48]

Fixed, CreateURLRequest now sets the method.

+
+  net::URLRequest* raw_request = request.get();
+  FetchParams* params = new FetchParams(verifier);
+  inflight_requests_.insert(std::make_pair(request.release(), params));
+  raw_request->Start();

[davidben, 2015/05/07 21:59:36]

Could also write this perhaps as

  auto it = inflight_requests.insert(...)->first;
  it->first->Start();

[Eran Messeri, 2015/07/10 13:15:47]

Done.

+}
+
+void LogProofsFetcher::OnResponseStarted(net::URLRequest* request) {
+  const net::URLRequestStatus& status(request->status());
+  VLOG(0) << "Response started for fetch-sth from " << request->original_url();
+  if (!status.is_success()) {
+    LOG(WARNING) << "Fetching STH from " << request->original_url()
+                 << " failed. status:" << status.status()
+                 << " error:" << status.error();

[davidben, 2015/05/07 21:59:37]

I imagine later we'll surface this up through to something interesting?

[Eran Messeri, 2015/07/10 13:15:47]

Correct. Particularly, I'd like DomainReliability to collect these metrics and
report to us, or collect UMA for that.

+    RequestCleanup(request);
+  } else if (request->GetResponseCode() != 200) {
+    LOG(WARNING) << "Fetch STH HTTP status: " << request->GetResponseCode();
+    RequestCleanup(request);
+    return;
+  }
+
+  RequestsMap::iterator it(inflight_requests_.find(request));
+  DCHECK(it != inflight_requests_.end());
+  scoped_refptr<net::IOBufferWithSize> buffer(it->second->sth_buffer);
+  VLOG(0) << "Kicking off read.";
+  request->Read(buffer.get(), buffer->size(), &(it->second->read_bytes));

[davidben, 2015/05/07 21:59:36]

Nit: I don't think those parens are necessary.

[davidben, 2015/05/07 21:59:36]

[Confirmed with mmenke that ignoring URLRequest::Read and checking
request->status().is_io_pending() is preferred for now, since that's what
ResourceLoader does. Making this API sane in the cleanup queue.]

[Eran Messeri, 2015/07/10 13:15:47]

Done.

[Eran Messeri, 2015/07/10 13:15:47]

Acknowledged.

+  if (request->status().is_io_pending()) {
+    VLOG(0) << "IO Pending: Will wait.";
+    return;
+  } else {
+    VLOG(0) << "Data available: Invoking OnReadComplete.";
+    OnReadCompleted(request, it->second->read_bytes);
+  }
+}
+
+void LogProofsFetcher::OnReadCompleted(net::URLRequest* request,
+                                       int bytes_read) {

[davidben, 2015/05/07 21:59:37]

bytes_read may be -1 on error. You pull the error code out of request->status().
(Yeah, this API is kinda messy.)

[Eran Messeri, 2015/07/10 13:15:47]

Done, is there anything interesting to do with the error code other than logging
though? In case of bytes_read being -1 I'm just cleaning up the request now.

+  VLOG(0) << "Read complete, got " << bytes_read << " bytes.";
+  RequestComplete(request, bytes_read);

[davidben, 2015/05/07 21:59:37]

There's no guarantee that the entire request will come in in one Read. You need
to keep pumping the read loop until it returns 0.

[Eran Messeri, 2015/07/10 13:15:47]

Fixed by creating a CheckReadStatus method to handle the result. Let me know
what you think of it.

+}
+
+void LogProofsFetcher::RequestComplete(net::URLRequest* request,
+                                       int bytes_read) {
+  RequestsMap::iterator it(inflight_requests_.find(request));
+  DCHECK(it != inflight_requests_.end());
+  if (it->first != request) {
+    LOG(WARNING) << "Different requests?!";

[davidben, 2015/05/07 21:59:37]

This can just be a DCHECK, right? If this fails, std::map is kinda broken...

[Eran Messeri, 2015/07/10 13:15:47]

Done.

+  }
+
+  FetchParams* params(it->second);

[davidben, 2015/05/07 21:59:36]

Nit: This can just use =

[Eran Messeri, 2015/07/10 13:15:48]

Done.

+  VLOG(0) << "Got " << bytes_read << " bytes.";
+
+  // Extract STH json an parse it

[davidben, 2015/05/07 21:59:37]

Nit: period at the end of comment.

[davidben, 2015/05/07 21:59:37]

I'd DCHECK that bytes_read is within the buffer's size.

[Eran Messeri, 2015/07/10 13:15:46]

Done.

[Eran Messeri, 2015/07/10 13:15:47]

Done.

+  base::StringPiece json_sth(params->sth_buffer->data(), bytes_read);
+  VLOG(0) << "STH data: " << json_sth;
+  scoped_ptr<net::ct::SignedTreeHead> sth;
+  if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) {

[davidben, 2015/05/07 21:59:36]

This looks like it'll just crash on a null pointer (a test would probably have
noticed ;-) ). sth.get() is null. I think you want to stack-allocate it and pass
in &sth. Or initialize it to new SignedTreeHead if you need to pass it around.

[Eran Messeri, 2015/07/10 13:15:47]

Fixed.

+    VLOG(0) << "Parsed STH successfully.";
+    if (params->verifier->VerifySignedTreeHead(sth.get())) {
+      VLOG(0) << "Received valid STH.";
+    } else {
+      VLOG(0) << "Received invalid STH.";
+    }
+  } else {
+    LOG(ERROR) << "Invalid STH.";

[davidben, 2015/05/07 21:59:37]

(I'm assuming this is all to be routed up to something real later.)

[Eran Messeri, 2015/07/10 13:15:47]

It's now passed to a callback.

+  }
+  RequestCleanup(request);
+}
+
+void LogProofsFetcher::RequestCleanup(net::URLRequest* request) {
+  VLOG(0) << "Cleaning up request to " << request->original_url();
+  RequestsMap::iterator 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.
+  VLOG(0) << "Erasing iterator.";
+  inflight_requests_.erase(it);
+}
+
+LogProofsFetcher::FetchParams::FetchParams(net::CTLogVerifier* verifier)
+    : verifier(verifier), sth_buffer(new net::IOBufferWithSize(kMaxSTHSize)) {
+}
+
+LogProofsFetcher::FetchParams::~FetchParams() {
+}
+
+bool CTLogResponseParser::FillSignedTreeHead(const base::StringPiece& json_sth,
+                                             net::ct::SignedTreeHead* sth) {

[davidben, 2015/05/07 21:59:37]

How much do we trust our JSON parser on untrusted sources? (I don't actually
know.)

[Eran Messeri, 2015/07/10 13:15:47]

Not a lot, which is why I've switched to the SafeJsonParser.

+  base::JSONReader json_reader;
+  base::Value* json = json_reader.Read(json_sth);

[davidben, 2015/05/07 21:59:37]

This leaks memory. scoped_ptr<base::Value> json(json_reader.Read(json_sth));

(That function really should return a scoped_ptr...)

[Eran Messeri, 2015/07/10 13:15:47]

Done - irrelevant now.

+  if (json == NULL) {

[davidben, 2015/05/07 21:59:37]

nullptr. Or just !json.

[Eran Messeri, 2015/07/10 13:15:47]

Done - irrelevant now.

+    LOG(WARNING) << "Empty JSON.";
+    return false;
+  }
+

[davidben, 2015/05/07 21:59:37]

Comment linking to documentation on the format? [I'm assuming RFC 6962]

[Eran Messeri, 2015/07/10 13:15:46]

Should be in the ct_log_response_parser.

+  const base::DictionaryValue* json_dict;
+  if (!json->GetAsDictionary(&json_dict)) {
+    LOG(WARNING) << "Json value not a dictionary.";

[davidben, 2015/05/07 21:59:37]

Json -> JSON, ditto below

[Eran Messeri, 2015/07/10 13:15:47]

Done - irrelevant. 

+    return false;
+  }
+
+  int tree_size;
+  if (!json_dict->GetInteger("tree_size", &tree_size)) {
+    LOG(WARNING) << "Json dictionary does not contain tree_size";
+    return false;
+  }
+
+  double timestamp;
+  if (!json_dict->GetDouble("timestamp", &timestamp)) {
+    LOG(WARNING) << "Json dictionary does not contain timestamp";
+    return false;
+  }
+
+  std::string sha256_root_hash;
+  if (!json_dict->GetString("sha256_root_hash", &sha256_root_hash)) {
+    LOG(WARNING) << "Json dictionary does not contain sha256_root_hash";
+    return false;
+  }
+
+  std::string tree_head_signature;
+  if (!json_dict->GetString("tree_head_signature", &tree_head_signature)) {
+    LOG(WARNING) << "Json dictionary does not contain tree_head_signature";
+    return false;
+  }
+
+  std::string decoded_root_hash;
+  if (!base::Base64Decode(sha256_root_hash, &decoded_root_hash)) {
+    LOG(WARNING) << "Failed decoding sha256_root_hash";
+    return false;
+  }
+
+  if (decoded_root_hash.length() != net::ct::kSthRootHashLength) {
+    LOG(WARNING) << "sha256_root_hash must be exactly 32 bit.";

[davidben, 2015/05/07 21:59:36]

bit -> bits

[Eran Messeri, 2015/07/10 13:15:47]

N/A.

+    return false;
+  }
+
+  std::string decoded_signature;
+  if (!base::Base64Decode(tree_head_signature, &decoded_signature)) {

[davidben, 2015/05/07 21:59:37]

[Incidentally, RFC 6962 fails to mention that this field is base64-encoded.]

[Eran Messeri, 2015/07/10 13:15:48]

Filed http://trac.tools.ietf.org/wg/trans/trac/ticket/91. Will also file an
errata for RFC6962.

+    LOG(WARNING) << "Failed decoding tree_head_signature";
+    return false;
+  }
+
+  base::StringPiece sp(decoded_signature);
+  if (!DecodeDigitallySigned(&sp, &(sth->signature))) {

[davidben, 2015/05/07 21:59:36]

parens after & unnecessary.

[davidben, 2015/05/07 21:59:37]

This should check sp.empty() afterwards, or you'll ignore trailing junk in the
signature.

[Eran Messeri, 2015/07/10 13:15:47]

Good point, will do in a separate CL (since the code does not live here
anymore).

[Eran Messeri, 2015/07/10 13:15:47]

Done.

+    LOG(WARNING) << "Failed decoding signature to DigitallySigned";
+    return false;
+  }
+
+  sth->version = net::ct::SignedTreeHead::V1;
+  sth->tree_size = tree_size;
+  sth->timestamp =
+      base::Time::UnixEpoch() + base::TimeDelta::FromMilliseconds(timestamp);
+  memcpy(sth->sha256_root_hash, decoded_root_hash.c_str(),

[davidben, 2015/05/07 21:59:36]

Nit: I would c_str -> data. It's totally meaningless in C++11 which requires
they be the same, but data makes it clear you don't care about the trailing NUL.

[Eran Messeri, 2015/07/10 13:15:47]

Done - N/A anymore.

+         net::ct::kSthRootHashLength);
+  return true;
+}
+
+}  // namespace certificate_transparency