Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2184053002:
Remove --certificate-transparency-log command-line flag (Closed)
|
Created:
4 years, 4 months ago by Eran Messeri Modified:
4 years, 4 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, rsleevi+watch_chromium.org, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, Eran Messeri, certificate-transparency-chrome_googlegroups.com, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove --certificate-transparency-log command-line flag
This removes the --certificate-transparency-log command-line flag
from Chrome initialization. It was used to be able to add additional
logs at runtime, but this was only ever intended to be used when
standing up a new log, and if you're standing up a new log, then it's
not unreasonable to expect locally compiling Chromium to test.
If there is significant concern from log operators, we can revisit how to
add logs on demand, but so far, there hasn't been a need.
BUG=632351
Committed: https://crrev.com/bfb1dee36cd495b0d7c2227fe87678bb4a2fad15
Cr-Commit-Position: refs/heads/master@{#408408}
Patch Set 1 #Patch Set 2 : Removing more direct users of MultiLogCTVerifier #Patch Set 3 : Compilation fixes #Patch Set 4 : Android compilation fix #Patch Set 5 : Revert changes unrelated to flag removal. #
Messages
Total messages: 34 (25 generated)
Description was changed from ========== Certificate Transparency: Remove the command-line flag. The CT command-line flag allowed specifying additional CT logs on the command line. It does not seem necessary anymore because testing new logs can be done by recompiling Chromium after adding extra logs to the header file that includes the list of known logs. Removing this flag would allow: - Initialization of the CTVerifier at the net/ layer, rather than the IOThread, and having to pass it around. - Stronger guarantees on the CTLogVerifier instances: CTLogVerifier instances created from the list of known logs have all the fields initialized, including the DNS domain for auditing. The MultiLogCTVerifier class is now better structured to be initialized during creation. BUG= ========== to ========== Certificate Transparency: Remove the command-line flag. The CT command-line flag allowed specifying additional CT logs on the command line. It does not seem necessary anymore because testing new logs can be done by recompiling Chromium after adding extra logs to the header file that includes the list of known logs. Removing this flag would allow: - Initialization of the CTVerifier at the net/ layer, rather than the IOThread, and having to pass it around. - Stronger guarantees on the CTLogVerifier instances: CTLogVerifier instances created from the list of known logs have all the fields initialized, including the DNS domain for auditing. The MultiLogCTVerifier class is now better structured to be initialized during creation. BUG= ==========
eranm@chromium.org changed reviewers: + rsleevi@chromium.org
Ryan, PTAL.
The CQ bit was checked by eranm@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by eranm@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...)
Description was changed from ========== Certificate Transparency: Remove the command-line flag. The CT command-line flag allowed specifying additional CT logs on the command line. It does not seem necessary anymore because testing new logs can be done by recompiling Chromium after adding extra logs to the header file that includes the list of known logs. Removing this flag would allow: - Initialization of the CTVerifier at the net/ layer, rather than the IOThread, and having to pass it around. - Stronger guarantees on the CTLogVerifier instances: CTLogVerifier instances created from the list of known logs have all the fields initialized, including the DNS domain for auditing. The MultiLogCTVerifier class is now better structured to be initialized during creation. BUG= ========== to ========== Certificate Transparency: Remove the command-line flag. The CT command-line flag allowed specifying additional CT logs on the command line. It does not seem necessary anymore because testing new logs can be done by recompiling Chromium after adding extra logs to the header file that includes the list of known logs. Removing this flag would allow: - Initialization of the CTVerifier at the net/ layer, rather than the IOThread, and having to pass it around. - Stronger guarantees on the CTLogVerifier instances: CTLogVerifier instances created from the list of known logs have all the fields initialized, including the DNS domain for auditing. The MultiLogCTVerifier class is now better structured to be initialized during creation. Long term, it may be possible to get rid of the "global" CT verifier instances in favour of creating them locally in the URLRequestContexts where needed. BUG= ==========
The CQ bit was checked by eranm@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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eranm@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.
Eran: While I'm not trying to be overly negative, this CL repeats a number of patterns and problems we've discussed in the past, and I'm concerned that things aren't improving. I'm not sure how I can better help you achieve what you're wanting to do, but this current approach is not good. I'm torn between listing the problems exhaustively, as opposed to highlighting some illustratively; I feel both can cause bad results and be received wrong, and my intent certainly is not to cause you stress or upset your feelings. I hope you consider that when reading this feedback. The CL description is an example of what we've discussed as poor documentation. I appreciate that you tried to describe the subtlety and nuance, and it helped contextualize the motivation. However, the very first file I opened - ssl_client_transport - shows a change that was not at all covered in your description. That is, your changing of MultiLogCTVerifier to CTVerifier Had it been included in the description, it would have revealed the second problem: This CL is doing too much. You start off with "remove the command-line flag", but then go considerably further with "Replace initialization with a factory function". That's not what we discussed, and I intentionally avoided this in previous CLs, and would have been happy to tell you not to do this yet. Either way, you shouldn't do it in the same CL - what we discussed on Hangouts was a very small change, to remove the flag. Even if we had intended to switch to factory functions, you aren't returning properly scoped objects (like we've discussed in the past and which I've linked https://www.chromium.org/developers/smart-pointer-guidelines to in the past) - CTVerifier vs scoped_ptr<>. The approach taken has further issues that we've talked about - by placing them in multi_log_ct_verifier.h, you don't gain any dependency isolation (everyone still has to know about the MLCV), and by keeping the constructors public, you've largely served to introduce additional methods, without clear guidance on how to use. Concrete next steps are to create a new CL that just does what you say you do in the description: Remove the command-line flag. Please do not do anything more than that.
Description was changed from ========== Certificate Transparency: Remove the command-line flag. The CT command-line flag allowed specifying additional CT logs on the command line. It does not seem necessary anymore because testing new logs can be done by recompiling Chromium after adding extra logs to the header file that includes the list of known logs. Removing this flag would allow: - Initialization of the CTVerifier at the net/ layer, rather than the IOThread, and having to pass it around. - Stronger guarantees on the CTLogVerifier instances: CTLogVerifier instances created from the list of known logs have all the fields initialized, including the DNS domain for auditing. The MultiLogCTVerifier class is now better structured to be initialized during creation. Long term, it may be possible to get rid of the "global" CT verifier instances in favour of creating them locally in the URLRequestContexts where needed. BUG= ========== to ========== Certificate Transparency: Remove the command-line flag. The CT command-line flag allowed specifying additional CT logs on the command line. It does not seem necessary anymore because testing new logs can be done by recompiling Chromium after adding extra logs to the header file that includes the list of known logs. Removing this flag would allow: - Initialization of the CTVerifier at the net/ layer, rather than the IOThread, and having to pass it around. - Stronger guarantees on the CTLogVerifier instances: CTLogVerifier instances created from the list of known logs have all the fields initialized, including the DNS domain for auditing. Long term, it may be possible to get rid of the "global" CT verifier instances in favour of creating them locally in the URLRequestContexts where needed. BUG= ==========
Removed all changes unrelated to flag removal. The bigger context is that I'm trying to address the pain point you've mentioned in https://codereview.chromium.org/2108833005/#msg43, that the set of known logs cannot be initialized in //net. Not allowing logs from sources other than the logs provided in CreateLogVerifiersForKnownLogs seems like an essential first step. However, I do not know how to get to the point where the //net layer produces a CTVerifier instance initialized with these logs, as you say replacing initialization with factory methods is something you avoided (why?). It seems to me there's an opportunity to get rid of one "global" (https://cs.chromium.org/chromium/src/chrome/browser/io_thread.h?dr=C&q=io_thr...) since this list of logs can now be created locally. Furthermore it seems to me that potentially the CTVerifier instances in IOThread::Globals / ProfileIOData can be created just for the URLRequestContext they're used in, rather than managing them globally (we'd still have to carefully think about lifetime management as the CTVerifiers have observers set on them for observing new SCTs and said observers either have to be destroyed after the CTVerifier instances or removed from the CTVerifier). After this change lands I'll follow up on both items (discussing it on net-dev@ before sending any changes).
On 2016/07/28 08:32:58, Eran Messeri wrote: > After this change lands I'll follow up on both items (discussing it on net-dev@ > before sending any changes). Why don't you schedule some GVC time, since I don't think our discussions on design-docs and the mailing lists are helping improve things.
Description was changed from ========== Certificate Transparency: Remove the command-line flag. The CT command-line flag allowed specifying additional CT logs on the command line. It does not seem necessary anymore because testing new logs can be done by recompiling Chromium after adding extra logs to the header file that includes the list of known logs. Removing this flag would allow: - Initialization of the CTVerifier at the net/ layer, rather than the IOThread, and having to pass it around. - Stronger guarantees on the CTLogVerifier instances: CTLogVerifier instances created from the list of known logs have all the fields initialized, including the DNS domain for auditing. Long term, it may be possible to get rid of the "global" CT verifier instances in favour of creating them locally in the URLRequestContexts where needed. BUG= ========== to ========== Remove --certificate-transparency-log command-line flag This removes the --certificate-transparency-log command-line flag from Chrome initialization. It was used to be able to add additional logs at runtime, but this was only ever intended to be used when standing up a new log, and if you're standing up a new log, then it's not unreasonable to expect locally compiling Chromium to test. If there is significant concern from log operators, we can revisit how to add logs on demand, but so far, there hasn't been a need. BUG=632351 ==========
Description was changed from ========== Remove --certificate-transparency-log command-line flag This removes the --certificate-transparency-log command-line flag from Chrome initialization. It was used to be able to add additional logs at runtime, but this was only ever intended to be used when standing up a new log, and if you're standing up a new log, then it's not unreasonable to expect locally compiling Chromium to test. If there is significant concern from log operators, we can revisit how to add logs on demand, but so far, there hasn't been a need. BUG=632351 ========== to ========== Remove --certificate-transparency-log command-line flag This removes the --certificate-transparency-log command-line flag from Chrome initialization. It was used to be able to add additional logs at runtime, but this was only ever intended to be used when standing up a new log, and if you're standing up a new log, then it's not unreasonable to expect locally compiling Chromium to test. If there is significant concern from log operators, we can revisit how to add logs on demand, but so far, there hasn't been a need. BUG=632351 ==========
I filed a bug and reworded the description, so that way we can track what release this went away with, some of the design rationale, etc. We can then use the bug for further follow-up / reference as appropriate.
lgtm
The CQ bit was checked by rsleevi@chromium.org
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 ========== Remove --certificate-transparency-log command-line flag This removes the --certificate-transparency-log command-line flag from Chrome initialization. It was used to be able to add additional logs at runtime, but this was only ever intended to be used when standing up a new log, and if you're standing up a new log, then it's not unreasonable to expect locally compiling Chromium to test. If there is significant concern from log operators, we can revisit how to add logs on demand, but so far, there hasn't been a need. BUG=632351 ========== to ========== Remove --certificate-transparency-log command-line flag This removes the --certificate-transparency-log command-line flag from Chrome initialization. It was used to be able to add additional logs at runtime, but this was only ever intended to be used when standing up a new log, and if you're standing up a new log, then it's not unreasonable to expect locally compiling Chromium to test. If there is significant concern from log operators, we can revisit how to add logs on demand, but so far, there hasn't been a need. BUG=632351 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Remove --certificate-transparency-log command-line flag This removes the --certificate-transparency-log command-line flag from Chrome initialization. It was used to be able to add additional logs at runtime, but this was only ever intended to be used when standing up a new log, and if you're standing up a new log, then it's not unreasonable to expect locally compiling Chromium to test. If there is significant concern from log operators, we can revisit how to add logs on demand, but so far, there hasn't been a need. BUG=632351 ========== to ========== Remove --certificate-transparency-log command-line flag This removes the --certificate-transparency-log command-line flag from Chrome initialization. It was used to be able to add additional logs at runtime, but this was only ever intended to be used when standing up a new log, and if you're standing up a new log, then it's not unreasonable to expect locally compiling Chromium to test. If there is significant concern from log operators, we can revisit how to add logs on demand, but so far, there hasn't been a need. BUG=632351 Committed: https://crrev.com/bfb1dee36cd495b0d7c2227fe87678bb4a2fad15 Cr-Commit-Position: refs/heads/master@{#408408} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bfb1dee36cd495b0d7c2227fe87678bb4a2fad15 Cr-Commit-Position: refs/heads/master@{#408408} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
