Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108833005:
Adds domain names for all qualified CT logs (Closed)
|
Created:
4 years, 5 months ago by Rob Percival Modified:
4 years, 5 months ago CC:
cbentzel+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, chromium-reviews, Eran Messeri Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds domain names for all qualified CT logs
This is required for querying those logs over DNS (more specifically, Google
mirrors of those logs).
BUG=612439
Committed: https://crrev.com/b6f0fc49ec0e4c360d1212bc7832c89a7556c1bd
Cr-Commit-Position: refs/heads/master@{#407519}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addresses Eran's comments #
Total comments: 4
Patch Set 3 : Make dns_domain a required parameter of CtLogVerifier::Create #
Total comments: 4
Patch Set 4 : Use empty string instead of nullptr #Patch Set 5 : Allow a log's DNS domain to be specified on the command-line #Patch Set 6 : Updates command-line documentation #
Total comments: 19
Patch Set 7 : Remove #includes from ct_known_logs_static-inc.h and fix a call to CTLogVerifier::Create #Patch Set 8 : Rebase #Patch Set 9 : Adds domain names for disqualified logs #Patch Set 10 : Updates CTLogInfo comment to say log_dns_domain may be empty, not null #Patch Set 11 : Fixes line length warning #Patch Set 12 : Slightly more documentation about DNS endpoints for logs #
Messages
Total messages: 60 (34 generated)
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robpercival@chromium.org changed reviewers: + eranm@chromium.org
PTAL
On 2016/06/28 23:01:42, Rob Percival wrote: > PTAL Note to selves: we ought to add these domain names to http://www.certificate-transparency.org/known-logs and its log_list.json file at some point.
The CQ bit was unchecked by robpercival@chromium.org
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm once the comments about naming are addressed. (I feel we should be explicit here about the purpose of this domain name). battre - FYI this is an implementation of the CT-over-DNS protocol you've reviewed a (long) while ago. https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_known_logs_... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_known_logs_... net/cert/ct_known_logs_static-inc.h:24: const char* const log_domain; log_dns_domain https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_log_verifier.h File net/cert/ct_log_verifier.h (right): https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:44: // |domain| is the DNS name of the log's DNS API endpoint, if one exists. rename |domain| to |dns_domain| throughout so it's explicit this is the domain for contacting the log via DNS. https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:58: // Returns the log's domain (for CT over DNS queries). Nit: Link to the CT-over-DNS protocol documentation https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:103: std::string domain_; As mentioned above, dns_domain_.
Description was changed from ========== Adds domain names for all qualified CT logs This is required for querying those logs over DNS (more specifically, Google mirrors of those logs). BUG=612439 ========== to ========== Adds domain names for all qualified CT logs This is required for querying those logs over DNS (more specifically, Google mirrors of those logs). BUG=612439 ==========
https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_known_logs_... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_known_logs_... net/cert/ct_known_logs_static-inc.h:24: const char* const log_domain; On 2016/06/30 at 20:02:51, Eran Messeri wrote: > log_dns_domain Done. https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_log_verifier.h File net/cert/ct_log_verifier.h (right): https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:44: // |domain| is the DNS name of the log's DNS API endpoint, if one exists. On 2016/06/30 at 20:02:51, Eran Messeri wrote: > rename |domain| to |dns_domain| throughout so it's explicit this is the domain for contacting the log via DNS. Done. https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:58: // Returns the log's domain (for CT over DNS queries). On 2016/06/30 at 20:02:51, Eran Messeri wrote: > Nit: Link to the CT-over-DNS protocol documentation Done. https://codereview.chromium.org/2108833005/diff/40001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:103: std::string domain_; On 2016/06/30 at 20:02:51, Eran Messeri wrote: > As mentioned above, dns_domain_. Done.
Patchset #2 (id:60001) has been deleted
robpercival@chromium.org changed reviewers: + eroman@chromium.org
PTAL
https://codereview.chromium.org/2108833005/diff/80001/net/cert/ct_known_logs_... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2108833005/diff/80001/net/cert/ct_known_logs_... net/cert/ct_known_logs_static-inc.h:52: "digicert.ct.googleapis.com"}, Is it expected that these are all at googleapis.com? https://codereview.chromium.org/2108833005/diff/80001/net/cert/ct_log_verifier.h File net/cert/ct_log_verifier.h (right): https://codereview.chromium.org/2108833005/diff/80001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:49: const base::StringPiece& dns_domain = nullptr); Can you make this a required parameter?
https://codereview.chromium.org/2108833005/diff/80001/net/cert/ct_known_logs_... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2108833005/diff/80001/net/cert/ct_known_logs_... net/cert/ct_known_logs_static-inc.h:52: "digicert.ct.googleapis.com"}, On 2016/07/16 00:14:28, eroman wrote: > Is it expected that these are all at googleapis.com? Yes, we're the only people running CT DNS servers. We mirror all of the logs so can provide DNS endpoints for all of them. https://codereview.chromium.org/2108833005/diff/80001/net/cert/ct_log_verifier.h File net/cert/ct_log_verifier.h (right): https://codereview.chromium.org/2108833005/diff/80001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:49: const base::StringPiece& dns_domain = nullptr); On 2016/07/16 00:14:28, eroman wrote: > Can you make this a required parameter? Done.
Patchset #3 (id:100001) has been deleted
lgtm https://codereview.chromium.org/2108833005/diff/120001/net/cert/ct_objects_ex... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/2108833005/diff/120001/net/cert/ct_objects_ex... net/cert/ct_objects_extractor_unittest.cc:35: "https://ct.example.com", nullptr); nullptr doesn't make sense to me as a parameter here. Please use empty string instead, or change the type of this parameter. (this parameter is a StringPiece -- nullptr is IMO misleading, since it goes through the implicit const char* constructor and is treated as a zero-length string). https://codereview.chromium.org/2108833005/diff/120001/net/quic/crypto/proof_... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/2108833005/diff/120001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:123: "https://test.example.com", nullptr)); Same comment throughout (nullptr is not distinguishable from empty string, which makes this misleading I think).
https://codereview.chromium.org/2108833005/diff/120001/net/cert/ct_objects_ex... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/2108833005/diff/120001/net/cert/ct_objects_ex... net/cert/ct_objects_extractor_unittest.cc:35: "https://ct.example.com", nullptr); On 2016/07/18 17:09:46, eroman wrote: > nullptr doesn't make sense to me as a parameter here. > Please use empty string instead, or change the type of this parameter. > > (this parameter is a StringPiece -- nullptr is IMO misleading, since it goes > through the implicit const char* constructor and is treated as a zero-length > string). Done. https://codereview.chromium.org/2108833005/diff/120001/net/quic/crypto/proof_... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/2108833005/diff/120001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:123: "https://test.example.com", nullptr)); On 2016/07/18 17:09:46, eroman wrote: > Same comment throughout (nullptr is not distinguishable from empty string, which > makes this misleading I think). Done.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
robpercival@chromium.org changed reviewers: + rsleevi@chromium.org
Hi Ryan, PTAL - this adds domain names for the CT logs, to later be used by LogDnsClient to request inclusion proofs.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Where can I read about the design decision to make the DNS API optional, and what implications that has throughout the rest of the design decisions? For example, naively it would mean that we've got significant infrastructure being wired up to track the STH/SCT state for these logs, except it goes nowhere, because the log is disqualified. https://codereview.chromium.org/2108833005/diff/180001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2108833005/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:542: if (command_line.HasSwitch(switches::kCertificateTransparencyLog)) { 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? https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... net/cert/ct_known_logs_static-inc.h:7: #include "base/time/time.h" This is an -inc file, it's meant to be included inline. If you look, you'll see it's within an unnamed namespace. Don't include these headers. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... net/cert/ct_known_logs_static-inc.h:24: const char* const log_dns_domain; Why this API decision? Why not make it a strong API guarantee that it's always present? https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_objects_ex... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_objects_ex... net/cert/ct_objects_extractor_unittest.cc:35: "https://ct.example.com", ""); std::string(), not "" https://codereview.chromium.org/2108833005/diff/180001/net/cert/multi_log_ct_... File net/cert/multi_log_ct_verifier_unittest.cc (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/multi_log_ct_... net/cert/multi_log_ct_verifier_unittest.cc:55: ct::GetTestPublicKey(), kLogDescription, "https://ct.example.com", "")); std::string() not "" https://codereview.chromium.org/2108833005/diff/180001/net/quic/crypto/proof_... File net/quic/crypto/proof_verifier_chromium_test.cc (right): https://codereview.chromium.org/2108833005/diff/180001/net/quic/crypto/proof_... net/quic/crypto/proof_verifier_chromium_test.cc:123: "https://test.example.com", "")); std::string() not ""
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The decision to make the DNS API optional was a result of the lack of DNS endpoints for the disqualified logs and, when I began this change, we also lacked the ability to run a DNS endpoint for one of the trusted logs. That has since been fixed. There's also the fact that it effectively makes the command-line flag for specifying additional logs useless, but removing that might not be an issue. When it came to wiring up infrastructure, we could have skipped the creation of a SingleTreeTracker for any log that lacked a DNS endpoint, so I don't think that would have been any wasted resources there. I'm more than happy to revisit this decision and see if we can get DNS servers up and running for the disqualified logs. I'll also update our process for adding logs to Chrome to ensure that they have a DNS endpoint set up in time for the end of their compliance monitoring period. https://codereview.chromium.org/2108833005/diff/180001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2108833005/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:542: if (command_line.HasSwitch(switches::kCertificateTransparencyLog)) { On 2016/07/18 19:06:38, Ryan Sleevi (extremely slow) wrote: > 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? 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. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... net/cert/ct_known_logs_static-inc.h:7: #include "base/time/time.h" On 2016/07/18 19:06:38, Ryan Sleevi (extremely slow) wrote: > This is an -inc file, it's meant to be included inline. If you look, you'll see > it's within an unnamed namespace. > > Don't include these headers. Done. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... net/cert/ct_known_logs_static-inc.h:24: const char* const log_dns_domain; On 2016/07/18 19:06:38, Ryan Sleevi (extremely slow) wrote: > Why this API decision? Why not make it a strong API guarantee that it's always > present? Some logs may not have a DNS endpoint to query. The disqualified logs, for instance, do not. However, I can look into the feasibility of getting DNS endpoints set up for them. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_objects_ex... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_objects_ex... net/cert/ct_objects_extractor_unittest.cc:35: "https://ct.example.com", ""); On 2016/07/18 19:06:38, Ryan Sleevi (extremely slow) wrote: > std::string(), not "" Is that safe? Create takes these parameters as StringPieces. A string literal is fine, but I'd have thought a temporary std::string would not be.
Regarding UMA vs chromium-dev vs CT - No, I don't think so. We don't tend to do these things by votes or consensus, and UMA for developer-only flags that are undocumented is... low on the list. The issue it raises is primarily a design issue, which even your reply hints at but doesn't really explore (this is again my suggestion for a design doc), which is whether or not it even makes sense for a command-line specified log to take a DNS option. What does that mean, in practice? When would someone use it? Why would someone use it? These are the sorts of things we want documented in a way that shows we thought about it intentionally. It sounds, from your reply, that the command-line flag shouldn't have the DNS option because it doesn't make sense - there's seemingly no use case for it. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... net/cert/ct_known_logs_static-inc.h:23: // https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft.... Are you using some other form of Markdown? Because this renders pretty roughly in the GitHub UI. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... net/cert/ct_known_logs_static-inc.h:24: const char* const log_dns_domain; On 2016/07/18 22:20:06, Rob Percival wrote: > On 2016/07/18 19:06:38, Ryan Sleevi (extremely slow) wrote: > > Why this API decision? Why not make it a strong API guarantee that it's always > > present? > > Some logs may not have a DNS endpoint to query. The disqualified logs, for > instance, do not. However, I can look into the feasibility of getting DNS > endpoints set up for them. Before doing that, I'm mostly trying to figure out how the design of DNS-based validation is going to work. This is where a design doc is useful. For example, it's unclear whether we're making a choice not to have DNS endpoints for disqualified logs for convenience or for policy reasons. I don't have strong feelings either way, other than it's clear (via design doc or code) the reasoning *why* this decision is made. Concretely, line 22 doesn't help understand why we'd want it to be null or the logic behind that, or the practical implications of what that means. I'm certainly not opposed to not running DNS endpoints for disqualified logs, but it's good to have this sort of discussion spelled out in a design doc, because it's incredibly difficult to know what you intended to do from the code, and these sorts of code review discussions can easily end up lost in the nether or be very hard for someone to read/understand at a later point. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_log_verifi... File net/cert/ct_log_verifier.h (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_log_verifi... net/cert/ct_log_verifier.h:44: // |dns_domain| is the DNS name of the log's DNS API endpoint, if one exists. Why should this be part of the CTLogVerifier API? The description says this is a class for verifying signatures of a single CT log - but this isn't a necessary parameter for that. My guess is because CTLogVerifier has been overloaded to also, as a convenience, hold status information about the log (it's description and URL are examples of similar issues). Are we just smuggling information through the layers via this class? Is that the right design? I'm not sure, and it seems like now that we keep adding fields to it, it's reasonable to ask if this is right - either we should change what CTLogVerifier means, or we should change how we associate this data. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_objects_ex... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_objects_ex... net/cert/ct_objects_extractor_unittest.cc:35: "https://ct.example.com", ""); On 2016/07/18 22:20:06, Rob Percival wrote: > On 2016/07/18 19:06:38, Ryan Sleevi (extremely slow) wrote: > > std::string(), not "" > > Is that safe? Create takes these parameters as StringPieces. A string literal is > fine, but I'd have thought a temporary std::string would not be. You're guaranteed that the temporary std::string() will last for the entire full-statement (which would include the full assignment operation). Because ::Create() goes StringPiece and makes a copy of it (via .as_string()), it's safe. That said, I missed that the API contract was StringPiece here and not string; in this case, use base::StringPiece() rather than the ""
I'll address everything you've brought up in a new section of the design doc (https://docs.google.com/document/d/1FP5J5Sfsg0OR9P4YT0q1dM02iavhi8ix1mZlZe_z-...), and let you know when that's done. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... File net/cert/ct_known_logs_static-inc.h (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_known_logs... net/cert/ct_known_logs_static-inc.h:23: // https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft.... On 2016/07/18 23:14:47, Ryan Sleevi (extremely slow) wrote: > Are you using some other form of Markdown? Because this renders pretty roughly > in the GitHub UI. Judging by the Makefile in the same directory, it's whatever dialect of Markdown kramdown-rfc2629 supports. I wasn't one of the authors. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_log_verifi... File net/cert/ct_log_verifier.h (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_log_verifi... net/cert/ct_log_verifier.h:44: // |dns_domain| is the DNS name of the log's DNS API endpoint, if one exists. On 2016/07/18 23:14:47, Ryan Sleevi (extremely slow) wrote: > Why should this be part of the CTLogVerifier API? > > The description says this is a class for verifying signatures of a single CT log > - but this isn't a necessary parameter for that. > > My guess is because CTLogVerifier has been overloaded to also, as a convenience, > hold status information about the log (it's description and URL are examples of > similar issues). > > Are we just smuggling information through the layers via this class? Is that the > right design? I'm not sure, and it seems like now that we keep adding fields to > it, it's reasonable to ask if this is right - either we should change what > CTLogVerifier means, or we should change how we associate this data. The reason I implemented it this way was simply because this is how all of the other log information is transferred, and it'd have been odd to put it somewhere else. However, I think you're right to question why this class has all of these properties. The only one it actually needs is |public_key|.
https://codereview.chromium.org/2108833005/diff/180001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2108833005/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:542: if (command_line.HasSwitch(switches::kCertificateTransparencyLog)) { On 2016/07/18 22:20:06, Rob Percival wrote: > On 2016/07/18 19:06:38, Ryan Sleevi (extremely slow) wrote: > > 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? > > 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. 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. https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_log_verifi... File net/cert/ct_log_verifier.h (right): https://codereview.chromium.org/2108833005/diff/180001/net/cert/ct_log_verifi... net/cert/ct_log_verifier.h:44: // |dns_domain| is the DNS name of the log's DNS API endpoint, if one exists. On 2016/07/19 00:04:20, Rob Percival wrote: > On 2016/07/18 23:14:47, Ryan Sleevi (extremely slow) wrote: > > Why should this be part of the CTLogVerifier API? > > > > The description says this is a class for verifying signatures of a single CT > log > > - but this isn't a necessary parameter for that. > > > > My guess is because CTLogVerifier has been overloaded to also, as a > convenience, > > hold status information about the log (it's description and URL are examples > of > > similar issues). > > > > Are we just smuggling information through the layers via this class? Is that > the > > right design? I'm not sure, and it seems like now that we keep adding fields > to > > it, it's reasonable to ask if this is right - either we should change what > > CTLogVerifier means, or we should change how we associate this data. > > The reason I implemented it this way was simply because this is how all of the > other log information is transferred, and it'd have been odd to put it somewhere > else. However, I think you're right to question why this class has all of these > properties. The only one it actually needs is |public_key|. Ryan has a good point - the CTLogVerifier has been indeed overloaded to hold additional (static) information about the log. My proposal for this change is to clearly document it. Then, after this change lands, revisit and see if there's usage of the CTLogVerifier that needs the URL / description / dns_domain but does not use any of the verification functions and, if that's the case, think about how to properly separate (one way to do it would be to have a CTLogInfo struct/class that would be passed around and CTLogVerifiers would be created on-demand from the CT log key in CTLogInfo).
LGTM although we should continue the discussion in follow-up. https://codereview.chromium.org/2108833005/diff/180001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2108833005/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:542: if (command_line.HasSwitch(switches::kCertificateTransparencyLog)) { On 2016/07/21 15:03:51, Eran Messeri wrote: > 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). 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.
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Happy for me to submit this then, and possibly remove the command-line flag in a subsequent change?
The CQ bit was checked by robpercival@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/25 16:41:09, Rob Percival wrote: > Happy for me to submit this then, and possibly remove the command-line flag in a > subsequent change? Yes, the LGTM was trying to be clear that it was OK to submit this, but that we should have a follow-up conversation about these aspects.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robpercival@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org, eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2108833005/#ps300001 (title: "Slightly more documentation about DNS endpoints for logs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adds domain names for all qualified CT logs This is required for querying those logs over DNS (more specifically, Google mirrors of those logs). BUG=612439 ========== to ========== Adds domain names for all qualified CT logs This is required for querying those logs over DNS (more specifically, Google mirrors of those logs). BUG=612439 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Adds domain names for all qualified CT logs This is required for querying those logs over DNS (more specifically, Google mirrors of those logs). BUG=612439 ========== to ========== Adds domain names for all qualified CT logs This is required for querying those logs over DNS (more specifically, Google mirrors of those logs). BUG=612439 Committed: https://crrev.com/b6f0fc49ec0e4c360d1212bc7832c89a7556c1bd Cr-Commit-Position: refs/heads/master@{#407519} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/b6f0fc49ec0e4c360d1212bc7832c89a7556c1bd Cr-Commit-Position: refs/heads/master@{#407519} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
