Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2756633002:
Add documentation for Certificate Transparency in //net (Closed)
|
Created:
3 years, 9 months ago by Ryan Sleevi Modified:
3 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd documentation for Certificate Transparency in //net
BUG=700973
Review-Url: https://codereview.chromium.org/2756633002
Cr-Commit-Position: refs/heads/master@{#457766}
Committed: https://chromium.googlesource.com/chromium/src/+/1b256461b832243250d8139c2a0e15e7fd5336ec
Patch Set 1 #
Total comments: 4
Patch Set 2 : Feedback #
Total comments: 17
Patch Set 3 : Feedback #Messages
Total messages: 14 (5 generated)
rsleevi@chromium.org changed reviewers: + eranm@chromium.org, xunjieli@chromium.org
Eran: I'll get your review on this first before trying to TBR it in :) Helen: Would you mind taking a look at this, both because your value on the previous iteration was super-helpful, and from a Cronet perspective and CT outsider, to see how helpful this documentation is at first glance? :)
lgtm - thanks for writing this up! https://codereview.chromium.org/2756633002/diff/1/net/docs/certificate-transp... File net/docs/certificate-transparency.md (right): https://codereview.chromium.org/2756633002/diff/1/net/docs/certificate-transp... net/docs/certificate-transparency.md:19: that certificates that an application trusts will be publicly disclosed in a very small nit: that ... that Can the 2nd 'that' simply be dropped? https://codereview.chromium.org/2756633002/diff/1/net/docs/certificate-transp... net/docs/certificate-transparency.md:101: `TransportSecurityState::RequireCTDelegate` implementation, which allows Do I understand correctly that if said TransportSecurityState::RequireCTDelegate implementation is overridden to not require CT for any certificate, then the CTPolicyEnforcer isn't involved at all in the process?
https://codereview.chromium.org/2756633002/diff/1/net/docs/certificate-transp... File net/docs/certificate-transparency.md (right): https://codereview.chromium.org/2756633002/diff/1/net/docs/certificate-transp... net/docs/certificate-transparency.md:19: that certificates that an application trusts will be publicly disclosed in a On 2017/03/16 12:10:28, Eran Messeri wrote: > very small nit: that ... that > Can the 2nd 'that' simply be dropped? Yup. Thanks https://codereview.chromium.org/2756633002/diff/1/net/docs/certificate-transp... net/docs/certificate-transparency.md:101: `TransportSecurityState::RequireCTDelegate` implementation, which allows On 2017/03/16 12:10:27, Eran Messeri wrote: > Do I understand correctly that if said TransportSecurityState::RequireCTDelegate > implementation is overridden to not require CT for any certificate, then the > CTPolicyEnforcer isn't involved at all in the process? Nope, because the CTPolicyEnforcer is still called beforehand, and if and only if it fails (and not due to expiration) does it go to the RequireCTDelegate
On 2017/03/16 14:10:35, Ryan Sleevi wrote: > https://codereview.chromium.org/2756633002/diff/1/net/docs/certificate-transp... > File net/docs/certificate-transparency.md (right): > > https://codereview.chromium.org/2756633002/diff/1/net/docs/certificate-transp... > net/docs/certificate-transparency.md:19: that certificates that an application > trusts will be publicly disclosed in a > On 2017/03/16 12:10:28, Eran Messeri wrote: > > very small nit: that ... that > > Can the 2nd 'that' simply be dropped? > > Yup. Thanks > > https://codereview.chromium.org/2756633002/diff/1/net/docs/certificate-transp... > net/docs/certificate-transparency.md:101: > `TransportSecurityState::RequireCTDelegate` implementation, which allows > On 2017/03/16 12:10:27, Eran Messeri wrote: > > Do I understand correctly that if said > TransportSecurityState::RequireCTDelegate > > implementation is overridden to not require CT for any certificate, then the > > CTPolicyEnforcer isn't involved at all in the process? > > Nope, because the CTPolicyEnforcer is still called beforehand, and if and only > if it fails (and not due to expiration) does it go to the RequireCTDelegate Acknowledged, thanks for the clarification. Do you think it's worth mentioning in the document? Not sure if it's an implementation detail or an API contract (i.e. the RequireCTDelegate will only be called under these conditions...).
Thanks for writing this up. This is incredibly helpful in understanding the certificates ecosystem and the new CT effort. The doc is very clear, so I only have a few nits below. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... File net/docs/certificate-transparency.md (right): https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:24: Transparency logs as trusted third parties, much like CAs are trusted third nit: consider revising. I am not sure what "its" refers to in "At its most basic level" https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:29: However, with more work, it's possible to minimize the trust afforded to CT extra nitpicky: sometimes you refer to "Certificate Transparency log" and other times the short version, "CT logs". Maybe consider standardize the usage? https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:52: [`URLRequestContext`](../url_request/url_request_context.h). optional nit: maybe lose the articles in front of HttpNetworkSession and URLRequestContext. I think them as proper nouns, but if you think otherwise, please keep these articles. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:60: policies related to certificates issues by particular certificate authorities nit: s/issues/issued https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:64: certificates from these CAs matches the level of security and assurance that nitpicky to make this sentence flow a bit better: s/"certificates from these CAs"/"these CAs" because the comparison is done with "the level of security ... that other CAs provide" and not "the level of security ... that provided by certificates from other CAs" https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:67: `ShouldRequireCT` method nit: add a period at the end of the sentence. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:71: The `CTVerifier` is the core class for parsing and validating the structures optional nit: Could consider removing "the" that's in front of CTVerifier and CTPolicyEnforcer since they are proper nouns. The `CTVerifier` is the core class -> `CTVerifier` is the core clas. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:72: defined in RFC6962 (or future versions), and providing basic information about nit: s/"providing"/"for providing" https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:80: certificates it trusts and the certificate authorities that issue these extra nitpicky: maybe shorten "certificate authorities" to CAs as you've done elsewhere. It will probably make this sentence easier to scan. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:86: returned `CERT_STATUS_IS_EV`) nit: add a period at the end of this sentence. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:96: ensure maximum security, by causing connections that fail to abide by an nit: consider revising the last part of the sentence. by causing connections ... [to fail]? or by [failing] connections that don't abide by ... https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:103: CT should be required, CT should not be required, or whether the default logic nit: consider losing "CT should not be required," because the first whether clause covers this scenario. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:113: both for EV and non-EV. extra nitpicky: maybe add a "certificates" at the end of the sentence. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:116: include: nit: s/include/includes https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:119: [Certificate Transparency Log Policy](https://www.chromium.org/Home/chromium-security/certificate-transparency/log-policy) nit: add a period at the end of this sentence. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:121: parsing SCTs from a variety of logs and validating their signatures, using Excuse my ignorance. It will be helpful to spell out "SCT" here as you did before using CT as a short-hand. https://codereview.chromium.org/2756633002/diff/20001/net/docs/certificate-tr... net/docs/certificate-transparency.md:149: ## Certificate Transparency for `//net` Embedders Could you include a sentence or two to explain the main differences between a "net// Consumer" and a "net// Embedder" ? I can make a guess but it'll be helpful to have the definition spelt out in this section or the previous one.
Thanks Helen! I really appreciate the level of attention to detail, and I hope you don't mind if I send future docs your way as well :) I'm glad you found it helpful - I wanted to try to make it accessible and useful without requiring someone know everything about CT, and wasn't sure if I was able to strike the right balance. Regarding the proper nouns - that's a really interesting and useful point, and I don't think I'd really considered it. I think my general sentiment is that any time I was using the article "the", it was better off dropped - it implies a singular instantiation, and unless it's a singleton, that's not right, and just treating it as a proper noun was good. However, I left several places where I use a/an. Here, I was thinking that the a/an implies "instance of". That is, "The Foo does X." -> "Foo does X." "A Foo needs to Y." -> "An instance of Foo needs to Y." Or, thinking about it differently, what are the guarantees that an object innately provides (that's a proper noun), and what are the things derived classes or distinct instantiations need to do (that's accompanied by an article and implied 'instance of' or 'subclass of'). Do you think I got that right? Apologies for the reflowing on some of this - I was trying to keep the doc within the 80 character limit, although not strictly necessary, as I think it makes both reviewing and editing the doc easier.
Yes, please. I am always happy to learn more about the codebase. Reading documentation is way easier than reading the code :) Removing "the" and leaving "a/an" SGTM. I was thinking the same -- "a/an ClassName" means an instance of that class. Thanks for adding the definition of a consumer and an embedder. This is a very helpful primer with relevant links for readers if they want to find out more. LGTM!
The CQ bit was checked by rsleevi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org Link to the patchset: https://codereview.chromium.org/2756633002/#ps40001 (title: "Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489760123837510,
"parent_rev": "3e87288e88257beea1727e67da8b69a77b7715a1", "commit_rev":
"1b256461b832243250d8139c2a0e15e7fd5336ec"}
Message was sent while issue was closed.
Description was changed from ========== Add documentation for Certificate Transparency in //net BUG=700973 ========== to ========== Add documentation for Certificate Transparency in //net BUG=700973 Review-Url: https://codereview.chromium.org/2756633002 Cr-Commit-Position: refs/heads/master@{#457766} Committed: https://chromium.googlesource.com/chromium/src/+/1b256461b832243250d8139c2a0e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1b256461b832243250d8139c2a0e... |
