Certificate Transparency: Logging SCTs to the NetLog.

net/cert/ct_signed_certificate_timestamp_log_param.cc

+// Copyright 2013 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 "net/cert/ct_signed_certificate_timestamp_log_param.h"
+
+#include <algorithm>
+#include <string>
+
+#include "base/base64.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "base/values.h"
+#include "net/cert/ct_verify_result.h"
+#include "net/cert/signed_certificate_timestamp.h"
+
+namespace net {
+
+namespace ct {
+
+namespace {
+
+base::DictionaryValue* GetSCTFieldsAsStrings(

[wtc, 2013/11/27 16:00:58]

Nit: it is rare to see a file with no comments :-)

I think these three internal functions could benefit from a concise,
one-sentence comment that describes what they do.

[eroman, 2013/11/27 20:39:55]

Please rename this function; the "AsStrings" part of the name is no longer as
relevant. Also the "Fields" part of the name is a bit weak.

Some other ideas:
  CreateDictionaryFromSCT()
  SCTToDictionary()
  ToDictionary()

[Eran M. (Google), 2013/11/27 22:08:50]

Done.

[Eran M. (Google), 2013/11/27 22:08:50]

Done.

+    const SignedCertificateTimestamp& sct) {
+  base::DictionaryValue* out = new base::DictionaryValue();
+
+  std::string origin_string;

[eroman, 2013/11/27 20:39:55]

[Optional] Stylistically I recommend splitting this off into a helper
OriginToString(). Similar to wtc's comment, I recommend additionally adding a
HashAlgorithmToString() and SignatureAlgorithmToString().

[Eran M. (Google), 2013/11/27 22:08:50]

Done, done and done.

+  switch (sct.origin) {
+    case SignedCertificateTimestamp::SCT_EMBEDDED:
+      origin_string = "embedded_in_certificate";
+      break;
+    case SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION:
+      origin_string = "tls_extension";
+      break;
+    case SignedCertificateTimestamp::SCT_FROM_OCSP_RESPONSE:
+      origin_string = "ocsp";
+      break;
+  }
+
+  out->SetString("origin", origin_string);
+  out->SetInteger("version", sct.version);

[wtc, 2013/11/27 16:00:58]

Nit: the function name says "get SCT fields as strings", but the sct.version
field is set as an integer, contradicting the function name.

Perhaps the function name should say "PrintableValues" rather than "Strings".

[Eran M. (Google), 2013/11/27 22:08:50]

Renamed function as suggested.

+  std::string log_id_b64;

[wtc, 2013/11/27 16:00:58]

Nit: consider reusing the same std::string local variable to receive the output
of all base::Base64Encode() calls in this function.

Alternatively, this function can call AddBase64EncodedStringToDictionary().

[Eran M. (Google), 2013/11/27 22:08:50]

Done - calling AddBase64EncodedStringToDictionary (silly of me not to notice it
earlier).

+  base::Base64Encode(sct.log_id, &log_id_b64);

[wtc, 2013/11/27 16:00:58]

Nit: it may be a good idea to check the return value of base::Base64Encode.

[eroman, 2013/11/27 20:39:55]

You already have a helper which abstracts the base64 encoding, use it instead
(i.e. there should only be one line in this file which base64 encodes).

  SetBinaryData("log_id", sct.log_id, out);

[Eran M. (Google), 2013/11/27 22:08:50]

Base64Encode promises not to change the output parameter's value if it fails, so
it means we'll add an empty string if it does. I think it's a reasonable thing
to do - I'm open to another approach if there's something better to do with
base64 encoding failure.

[Eran M. (Google), 2013/11/27 22:08:50]

Done.

+
+  out->SetString("log_id", log_id_b64);
+  base::TimeDelta time_since_epoch = sct.timestamp - base::Time::UnixEpoch();
+  out->SetString("timestamp",
+      base::Int64ToString(time_since_epoch.InMilliseconds()));
+
+  std::string extensions_b64;
+  base::Base64Encode(sct.extensions, &extensions_b64);

[eroman, 2013/11/27 20:39:55]

SetBinaryData("extensions", sct.extensions, out);

[Eran M. (Google), 2013/11/27 22:08:50]

Done.

+  out->SetString("extensions", extensions_b64);
+
+  out->SetInteger("hash_algorithm", sct.signature.hash_algorithm);
+  out->SetInteger("signature_algorithm", sct.signature.signature_algorithm);
+  std::string signature_data_b64;
+  base::Base64Encode(sct.signature.signature_data, &signature_data_b64);

[eroman, 2013/11/27 20:39:55]

SetBinaryData("extensions", sct.extensions, out);

[Eran M. (Google), 2013/11/27 22:08:50]

Done.

+  out->SetString("signature_data", signature_data_b64);
+
+  return out;
+}
+
+base::ListValue* SCTListToPrintableValues(
+    const ct::SCTList& sct_list) {
+

[wtc, 2013/11/27 16:00:58]

Nit: delete this blank line and line 79.

[Eran M. (Google), 2013/11/27 22:08:50]

Done.

+  base::ListValue* output_scts = new base::ListValue();
+  for (ct::SCTList::const_iterator it = sct_list.begin();
+      it != sct_list.end();
+      ++it)

[wtc, 2013/11/27 16:00:58]

Nit: I think people usually align these with the first character after "for (",
even though it implies five chars indentation.

In this case, it seems that everything may fit on the same line.

[Eran M. (Google), 2013/11/27 22:08:50]

It almost fits... I've indented as you suggested.

+    output_scts->Append(GetSCTFieldsAsStrings(*(it->get())));
+
+  return output_scts;
+}
+
+void AddBase64EncodedStringToDictionary(

[eroman, 2013/11/27 20:39:55]

I recommend calling this:

  SetBinaryData(const char* key, const std::string& value,
base::DictionaryValue* dict);

[Eran M. (Google), 2013/11/27 22:08:50]

Done.

+    base::DictionaryValue* dict,
+    const char* description,
+    const std::string& data) {
+
+  std::string b64_data;
+  base::Base64Encode(data, &b64_data);
+
+  dict->SetString(description, b64_data);
+}
+
+}  // namespace
+
+}  // namespace ct
+
+base::Value* NetLogSignedCertificateTimestampCallback(
+    const ct::CTVerifyResult* ct_result, NetLog::LogLevel log_level) {

[wtc, 2013/11/27 16:00:58]

IMPORTANT: the |log_level| parameter is not used in these two Callback
functions. If the |log_level| parameter is not required by NetLog, we probably
should remove the parameter.

[eroman, 2013/11/27 20:39:55]

The parameter is required, hence this is needed to compile.

+  base::DictionaryValue* dict = new base::DictionaryValue();
+
+  dict->Set("verified_scts",
+      ct::SCTListToPrintableValues(ct_result->verified_scts));

[wtc, 2013/11/27 16:00:58]

Nit: it is strange to see these internal functions with the ct:: prefix. Make
them look like "important" functions declared in some header.

I would suggest not wrapping "namespace ct" around these three internal
functions.

[Eran M. (Google), 2013/11/27 22:08:50]

Done.

+
+  dict->Set("failed_to_verify_scts",

[wtc, 2013/11/27 16:00:58]

Nit: just wondering why the name string doesn't match the name of the struct
member |unverified_scts|.

[Eran M. (Google), 2013/11/27 22:08:50]

For no good reason, really. Changed.

+      ct::SCTListToPrintableValues(ct_result->unverified_scts));
+
+  dict->Set("scts_from_unknown_logs",
+      ct::SCTListToPrintableValues(ct_result->unknown_logs_scts));

[wtc, 2013/11/27 16:00:58]

Nit: in these three dict->Set() calls, the second argument should be aligned
with the first argument.

[Eran M. (Google), 2013/11/27 22:08:50]

Done.

+
+  return dict;
+}
+
+base::Value* NetLogRawSignedCertificateTimestampCallback(
+    const std::string* embedded_scts,
+    const std::string* sct_list_from_ocsp,
+    const std::string* sct_list_from_tls_extension,
+    NetLog::LogLevel log_level) {
+  base::DictionaryValue* dict = new base::DictionaryValue();
+
+  ct::AddBase64EncodedStringToDictionary(dict, "embedded_scts", *embedded_scts);
+  ct::AddBase64EncodedStringToDictionary(
+      dict, "scts_from_ocsp_response", *sct_list_from_ocsp);
+  ct::AddBase64EncodedStringToDictionary(
+      dict, "scts_from_tls_extension", *sct_list_from_tls_extension);
+
+  return dict;
+}
+
+}  // namespace net