Adds domain names for all qualified CT logs

chrome/browser/io_thread.cc

 // Copyright (c) 2012 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 "chrome/browser/io_thread.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/base64.h"
@@ skipping 521 matching lines @@
 #else
   globals_->cert_verifier = net::CertVerifier::CreateDefault();
 #endif
 
   globals_->transport_security_state.reset(new net::TransportSecurityState());
 
   std::vector<scoped_refptr<const net::CTLogVerifier>> ct_logs(
       net::ct::CreateLogVerifiersForKnownLogs());
 
   // Add logs from command line
   if (command_line.HasSwitch(switches::kCertificateTransparencyLog)) {

[Ryan Sleevi, 2016/07/18 19:06:38]

Do we still need this? What's it in use for?

If we do, why is dns_domain optional, when it's not optional in production?

[Rob Percival, 2016/07/18 22:20:06]

Eran and I have used this in the past for testing, but I can't speak for whether
anyone else uses it. I don't suppose there are metrics for command-line flag
usage, e.g. in UMA? If not, we could add that to discover how it's getting used,
but that'd take quite a long time to give us any representative data. I think
there's *some* value in having it, e.g. for people who are starting their own
log and want to test Chrome with it. However, it's far from essential and I very
much doubt it sees any significant usage. We could just propose removing it on
the chromium-dev and certificate-transparency Google Groups and see if anyone
objects. Would you like me to do that?

dns_domain is optional because:
a) If you don't care about getting inclusion proofs for a log's SCTs, you don't
need to talk to it over DNS. We absolutely do want them for the logs Chrome has
trusted, but we have no reason to insist on them for other logs (i.e. those that
the user specifies with this flag).
b) We (Google) don't provide DNS endpoints for non-trusted logs and no one else
supports CT over DNS yet, nor is there an open-source server implementation of
CT over DNS. This would render this flag relatively useless until that changes.

[Eran Messeri, 2016/07/21 15:03:51]

I think the flag should remain, since it does come in handy for testing
occasionally (not just by us, but presumably also by log operators before
submission for inclusion).

I agree that the optionality of inclusion checking should be documented clearly.
We have the benefit of Chrome's CT policy where multiple SCTs are required so we
can prioritize checking inclusion of SCTs from specific logs.

[Ryan Sleevi, 2016/07/22 22:35:45]

We should have this conversation elsewhere on a codereview.

If the testing is limited to the CT team, then I think it's a reasonable bar to
expect you to be able to compile a change in and test, much like other teams do.

I'm not comfortable with keeping a flag that 'might' be useful, especially given
that the number of people who 'might' set this can be measured in the handful.

For example, this is the primary reason why //chrome initializes the set of
known logs, rather than having //net do it - which recently caused an Android
WebView regression.

     std::string switch_value = command_line.GetSwitchValueASCII(
         switches::kCertificateTransparencyLog);
     for (const base::StringPiece& curr_log : base::SplitStringPiece(
              switch_value, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL)) {
       std::vector<std::string> log_metadata = base::SplitString(
           curr_log, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
       CHECK_GE(log_metadata.size(), 3u)
           << "CT log metadata missing: Switch format is "
-          << "'description:base64_key:url_without_schema'.";
+          << "'description:base64_key:url_without_schema[:dns_domain]'.";
       std::string log_description(log_metadata[0]);
       std::string log_url(std::string("https://") + log_metadata[2]);
+      std::string log_dns_domain;
+      if (log_metadata.size() >= 4)
+        log_dns_domain = log_metadata[3];
       std::string ct_public_key_data;
       CHECK(base::Base64Decode(log_metadata[1], &ct_public_key_data))
           << "Unable to decode CT public key.";
       scoped_refptr<const net::CTLogVerifier> external_log_verifier(
           net::CTLogVerifier::Create(ct_public_key_data, log_description,
-                                     log_url));
+                                     log_url, log_dns_domain));
       CHECK(external_log_verifier) << "Unable to parse CT public key.";
       VLOG(1) << "Adding log with description " << log_description;
       ct_logs.push_back(external_log_verifier);
     }
   }
 
   globals_->ct_logs.assign(ct_logs.begin(), ct_logs.end());
 
   net::MultiLogCTVerifier* ct_verifier = new net::MultiLogCTVerifier();
   globals_->cert_transparency_verifier.reset(ct_verifier);
@@ skipping 419 matching lines @@
   // TODO(rtenneti): We should probably use HttpServerPropertiesManager for the
   // system URLRequestContext too. There's no reason this should be tied to a
   // profile.
   return context;
 }
 
 const metrics::UpdateUsagePrefCallbackType&
 IOThread::GetMetricsDataUseForwarder() {
   return metrics_data_use_forwarder_;
 }