Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1100003006:
Certificate Transparency: Fetching of Signed Tree Heads (DRAFT) (Closed)
|
Created:
5 years, 8 months ago by Eran Messeri Modified:
4 years, 7 months ago Reviewers:
CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency: Fetching of Signed Tree Heads (DRAFT)
This change lays out the groundwork for fetching and validating signed
tree heads from recognized CT logs.
BUG=480867
Patch Set 1 #
Total comments: 110
Patch Set 2 : Merging with master #
Total comments: 82
Patch Set 3 : Review comments, adding shutdown of the proof fetcher. #Patch Set 4 : Revised design, addressed some comments #
Total comments: 22
Patch Set 5 : Some bits have been committed to master #Patch Set 6 : Pre merge of LogProofFetcher #Patch Set 7 : BUILD.gn file fixes per review comments #Messages
Total messages: 13 (4 generated)
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1100003006/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1100003006/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:743: net::CTLogVerifier::Create(ct_public_key_data, log_description, "")); s/""/std::string()/ https://codereview.chromium.org/1100003006/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:901: certificate_transparency::NewSCTsObserver* scts_observer = LEAK/DESIGN: Where does this ever get deleted? If AddObserver takes ownership, than RemoveObserver becomes a dangerous API (since the pointer may have been deleted by AddObserver's implementation whenever it wants, so it's not safe for the caller to hold on to the pointer). I think (having only read header files thus far) that AddObserver *doesn't* take ownership, in which case, this is a leak. https://codereview.chromium.org/1100003006/diff/1/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/1100003006/diff/1/components/BUILD.gn#newcode168 components/BUILD.gn:168: "//components/certificate_transparency", # Blocked on content. Is this just copy-pasta? nothing in //components/certificate_trasparency depends on content. I think what you want is to do is move line 29 to line 221 https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency.gypi (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency.gypi:16: '../content/content.gyp:content_common', You don't seem to depend on this (as reflected in your gn) https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/BUILD.gn:5: static_library("ct_proofs_fetcher") { This name isn't right; it doesn't match what you said in the GYP file (that this target is called 'certificate_transparency') nor does it have all your files. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/DEPS (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/DEPS:3: "+base/memory", Just +base is sufficient (it includes subdirectories) https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/DEPS:6: "+net/url_request", just +net is sufficient here for a component. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/log_proofs_fetcher.cc (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:21: const int kMaxSTHSize = 600; 1) Document 2) SizeIn.... Bytes? Kb? kMaxSTHSizeInBytes ? https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:32: } SPECIAL NOTE: Because you have a URLRequestContext , and you're making requests, you have to follow the rule that all of this requests are cancelled before the URLRequest is destroyed. [I haven't checked that, not sure how best to ensure it, but just an early design note we can go over] https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:45: GURL fetch_url(fetch_sth_url); SOMETHING: I suspect we have something for appending paths to GURLs here w/o having to do the to-ascii-and-back conversion in line 44. I'll have to check. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:57: VLOG(0) << "Response started for fetch-sth from " << request->original_url(); VLOG(0) is wrong; VLOG(1) at best. That said, this seems unnecessary - the net-internals log will have this state transition. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:61: << " error:" << status.error(); ditto; net-internals will have this (since it'll have logged the request) https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:64: LOG(WARNING) << "Fetch STH HTTP status: " << request->GetResponseCode(); ditto; net-internals will have this. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:69: RequestsMap::iterator it(inflight_requests_.find(request)); auto it = inflight_requests_.find(request); https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:72: VLOG(0) << "Kicking off read."; all these vlogs are in net-internals; I'll stop whinging :) https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:91: RequestsMap::iterator it(inflight_requests_.find(request)); auto it = inflight_requests_.find(request) https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:93: if (it->first != request) { seems like a fatal error here, right? if (it->first != request) { NOTREACHED(); return; } https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:104: if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) { DESIGN: Crap, I forgot the important thing to note - we don't process JSON in the browser. I know, I know, you're probably thinking, "WHAT!??", but if it's remotely supplied JSON, from non-Google servers, then we don't process it in the browser. We'd also prefer not to process Google-supplied JSON in the browser, but we tend to be lazy in this respect, since we generally consider ourselves trustworthy (since we are contributing the code...) Let's chat about the design implications of this. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:104: if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) { SECURITY BUG: You forgot to allocate |sth|, but FillSignedTreeHead happily starts scribbling into it. I believe you meant to write net::ct::SignedTreeHead sth; if (CTLog....(json_sth, &sth)) { } https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:104: if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) { STYLE: We tend to put error control *before* the rest of processing, so that we can clearly reason about the current state of execution at any given time. if (!CTLogResponseParser::....) { LOG(ERROR) << "Invalid STH"; // Probably too chatty RequestCleanup(request); return; } .... https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:106: if (params->verifier->VerifySignedTreeHead(sth.get())) { ditto here, whenever this "becomes something important". (It's unclear whether/if/how we want to surface information about STH verifications; it may be that we introduce some delegate about OnSTHVerified() / OnSTHFailed() or something more nuanced, and then let the higher layers glue up appropriate policy) https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:119: RequestsMap::iterator it(inflight_requests_.find(request)); auto https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:126: VLOG(0) << "Erasing iterator."; too chatty (as with all the VLOGs), so hushing on that. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:141: if (json == NULL) { s/NULL/nullptr https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/log_proofs_fetcher.h (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:12: #include "net/base/io_buffer.h" You don't need to include this; you can forward declare IOBufferWithSize https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:14: IWYU: Missing ref_counted https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:17: } // namespace base https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:31: class LogProofsFetcher : public net::URLRequest::Delegate { naming nit: It's weird to call this plural, and I think reads weird. From the point of view of consumers, they fetch a single STH (re: line 37), so this probably makes more sense as LogProofFetcher [even if it's used to fetch multiple proofs through multiple calls to FetchSTH] https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:31: class LogProofsFetcher : public net::URLRequest::Delegate { style: documentation https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:35: ~LogProofsFetcher() override; delete newline on 34? https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:37: void FetchSTH(net::CTLogVerifier* verifier); documentation https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:44: scoped_ptr<net::URLRequest> CreateURLRequest(GURL fetch_sth_url); pass as const-ref https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:48: void RequestCleanup(net::URLRequest* request); naming nit: RequestCleanup -> CleanupRequest (RequestCleanup can either be Verb-Noun or Noun-Verb, and has different meanings; CleanupRequest makes it clear it's Verb-Noun) https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:50: struct NET_EXPORT_PRIVATE FetchParams { 1) NET_EXPORT_PRIVATE isn't needed 2) You can forward declare this, since you use a pointer in line 59 3) Ordering - http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:59: typedef std::map<net::URLRequest*, FetchParams*> RequestsMap; You don't actually need this typedef, do you, since you can use auto & friends https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:69: class CTLogResponseParser { style: move to a separate file. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:71: // Extracts STH from the json struct s/json/JSON/ https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/new_scts_observer.h (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:26: class LogProofsFetcher; newline between 25 & 26 https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:28: class NewSCTsObserver : public net::CTVerifier::Observer { Document https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:30: NewSCTsObserver(LogProofsFetcher* fetcher); explicit https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:30: NewSCTsObserver(LogProofsFetcher* fetcher); since you take ownership of this (line 40), you should have this as explicit NewSCTsObserver(scoped_ptr<LogProofsFetcher> fetcher); (and use the .Pass() semantics which are equivalent to C++11 move semantics) https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:37: typedef std::map<std::string, linked_ptr<net::ct::SignedTreeHead>> IDToSTHMap; This typedef isn't needed, is it? especially with the ability to use auto in the C++ code https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:39: IDToSTHMap sths_; document https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:45: } // namespace // namespace certificate_transparency https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:47: #endif #endif // COMPONENTS_CERTIFICATE_TRANSPARENCY_NEW_SCTS_OBSERVER_H_ https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_known_logs_stat... File net/cert/ct_known_logs_static.h (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_known_logs_stat... net/cert/ct_known_logs_static.h:23: "ct.googleapis.com/pilot"}, BUG: You forgot the scheme for all these URLs. Yes, 6962 says they MUST be HTTPS, so it might be suitable to simply comment on line 12 to that effect, leave the scheme off these URLs, but add the schemes in the .cc; However, since this information is exported/exposed, I think it may be cleaner to always put the https:// in here, so that we don't have multiple callsites needing to remember to do so before presenting/acting on the URL. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier.h File net/cert/ct_log_verifier.h (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier.h#... net/cert/ct_log_verifier.h:58: bool VerifySignedTreeHead(const ct::SignedTreeHead* signed_tree_head); No need to pass as pointer - just pass as const-ref https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier_un... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier_un... net/cert/ct_log_verifier_unittest.cc:22: log_ = CTLogVerifier::Create(ct::GetTestPublicKey(), "testlog", "").Pass(); s/""/std::string() https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier_un... net/cert/ct_log_verifier_unittest.cc:82: ASSERT_TRUE(log_->VerifySignedTreeHead(sth.get())); no need for .get() for scoped_ptr's; it's cleaner that way. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier_un... net/cert/ct_log_verifier_unittest.cc:97: scoped_ptr<CTLogVerifier> log = CTLogVerifier::Create(key, "testlog", ""); s/""/std::string() https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_objects_extract... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_objects_extract... net/cert/ct_objects_extractor_unittest.cc:34: log_ = CTLogVerifier::Create(ct::GetTestPublicKey(), "testlog", "").Pass(); s/""/std::string()/ https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_verifier.h#newc... net/cert/ct_verifier.h:26: CTVerifier(); DESIGN: This mixes interface with implementation That is, this used to be a pure virtual class, now it's a pure virtual with public virtual. I think we need to rethinking this; lets chat. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_verifier.h#newc... net/cert/ct_verifier.h:45: class NET_EXPORT Observer { http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_verifier.h#newc... net/cert/ct_verifier.h:62: void AddObserver(Observer* observer); Will there ever be more than one observer at a time with your API contract? If not, let's look at alternatives. https://codereview.chromium.org/1100003006/diff/1/net/cert/multi_log_ct_verif... File net/cert/multi_log_ct_verifier_unittest.cc (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/multi_log_ct_verif... net/cert/multi_log_ct_verifier_unittest.cc:42: CTLogVerifier::Create(ct::GetTestPublicKey(), kLogDescription, "")); s/""/std::string()
rsleevi@chromium.org changed reviewers: + davidben@chromium.org, eroman@chromium.org
Adding Eric and David to this CL
Did an initial pass. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency.gypi (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency.gypi:10: 'type': 'static_library', IMPORTANT: Because this is a static_library rather than <(component), while you don't need a certificate_transparency_export.h, you cannot depend on this from more than one component (in the shared library vs static library sense of the word). Otherwise you get two copies of it. That's probably fine since //content isn't going to depend on this anyway. But, for instance, content -> certificate_transparency + chrome -> certificate_transparency isn't going to work. I don't have a good sense of which //components normally goes with. It seems to mostly be static_library, but there are a few <(component)s. I guess that makes sense since //content doesn't typically depend on these. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency.gypi:16: '../content/content.gyp:content_common', This doesn't actually depend on //content (and the dependency isn't in GN or DEPS). https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/BUILD.gn:8: "log_proofs_fetcher.h", new_scts_observer.{cc,h}? https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/log_proofs_fetcher.cc (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:3: // found in the LICENSE file. Since this already has some significant logic now, unit test? https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:21: const int kMaxSTHSize = 600; Probably worth a comment. Notably mention that it's in bytes. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:34: scoped_ptr<net::URLRequest> LogProofsFetcher::CreateURLRequest( Nit: This doesn't match header file order. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:46: request->set_method("GET"); Nit: Somewhat strange separation between CreateRequest and FetchSTH; the former sets the URL while the latter sets the method. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:51: raw_request->Start(); Could also write this perhaps as auto it = inflight_requests.insert(...)->first; it->first->Start(); https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:60: << " error:" << status.error(); I imagine later we'll surface this up through to something interesting? https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:72: request->Read(buffer.get(), buffer->size(), &(it->second->read_bytes)); Nit: I don't think those parens are necessary. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:72: request->Read(buffer.get(), buffer->size(), &(it->second->read_bytes)); [Confirmed with mmenke that ignoring URLRequest::Read and checking request->status().is_io_pending() is preferred for now, since that's what ResourceLoader does. Making this API sane in the cleanup queue.] https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:83: int bytes_read) { bytes_read may be -1 on error. You pull the error code out of request->status(). (Yeah, this API is kinda messy.) https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:85: RequestComplete(request, bytes_read); There's no guarantee that the entire request will come in in one Read. You need to keep pumping the read loop until it returns 0. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:93: LOG(WARNING) << "Different requests?!"; This can just be a DCHECK, right? If this fails, std::map is kinda broken... https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:96: FetchParams* params(it->second); Nit: This can just use = https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:99: // Extract STH json an parse it I'd DCHECK that bytes_read is within the buffer's size. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:99: // Extract STH json an parse it Nit: period at the end of comment. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:103: if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) { This looks like it'll just crash on a null pointer (a test would probably have noticed ;-) ). sth.get() is null. I think you want to stack-allocate it and pass in &sth. Or initialize it to new SignedTreeHead if you need to pass it around. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:111: LOG(ERROR) << "Invalid STH."; (I'm assuming this is all to be routed up to something real later.) https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:137: net::ct::SignedTreeHead* sth) { How much do we trust our JSON parser on untrusted sources? (I don't actually know.) https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:139: base::Value* json = json_reader.Read(json_sth); This leaks memory. scoped_ptr<base::Value> json(json_reader.Read(json_sth)); (That function really should return a scoped_ptr...) https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:140: if (json == NULL) { nullptr. Or just !json. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:144: Comment linking to documentation on the format? [I'm assuming RFC 6962] https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:147: LOG(WARNING) << "Json value not a dictionary."; Json -> JSON, ditto below https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:182: LOG(WARNING) << "sha256_root_hash must be exactly 32 bit."; bit -> bits https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:187: if (!base::Base64Decode(tree_head_signature, &decoded_signature)) { [Incidentally, RFC 6962 fails to mention that this field is base64-encoded.] https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:193: if (!DecodeDigitallySigned(&sp, &(sth->signature))) { This should check sp.empty() afterwards, or you'll ignore trailing junk in the signature. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:193: if (!DecodeDigitallySigned(&sp, &(sth->signature))) { parens after & unnecessary. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:202: memcpy(sth->sha256_root_hash, decoded_root_hash.c_str(), Nit: I would c_str -> data. It's totally meaningless in C++11 which requires they be the same, but data makes it clear you don't care about the trailing NUL. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/log_proofs_fetcher.h (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.h:30: Comment describing what this class does. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.h:32: public: Document that the LogProofsFetcher cannot outlive request_context. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.h:36: Comment for what this function does. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.h:37: void FetchSTH(net::CTLogVerifier* verifier); Nit: STH -> SignedTreeHead? Most APIs seem to use the full name. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.h:50: struct NET_EXPORT_PRIVATE FetchParams { Why is this NET_EXPORT_PRIVATE? It's not in net and doesn't need to be exported at all. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.h:69: class CTLogResponseParser { We don't usually create empty classes for naming. Either use a namespace or have the function sit on its own. (There's only one, so maybe just have it sit on its own.) Actually... does this function not already exist as net/cert/ct_log_response_parser.h? https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.h:71: // Extracts STH from the json struct Period at the end. Mention that it returns false on parse error. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.h:74: }; Probably wants to be in a separate header / source file as it's not really part of the LogProofsFetcher. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/new_scts_observer.cc (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/new_scts_observer.cc:27: // fresher STH. (I'm assuming this is to be filled in later.) https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/new_scts_observer.h (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/new_scts_observer.h:27: Comment describing what this class does. https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_log_verifier.h File net/cert/ct_log_verifier.h (left): https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:57: bool SetSignedTreeHead(scoped_ptr<ct::SignedTreeHead> signed_tree_head); I'm assume this'll go away in a rebase? I remember reviewing this already. https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:48: Add a comment for when this is called and what |verifier| means here (since you have both CTVerifier and CTLogVerifier). Maybe verifier -> log_verifier or log? https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:50: CTLogVerifier* verifier) {} I wonder if this is better in MultiLogCTVerifier instead. Then you keep the interface pure. CTVerifier doesn't know that a single verifier secretly multiplexes between multiple CTLogVerifiers. It seems //net itself only cares about the Verify hook. You could argue that anything keying off of OnSCTVerified is a detail of MultiLogCTVerifier's Verify implementation: "hey net stack, here is how you verify SCTs. Oh, don't worry, I'll deal with checking up with the log asynchronously". I don't feel that strongly either way. Mostly the CTLogVerifier part was a little confusing. https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:61: // called back with notifications. Is this true? We're not using an ObserverListThreadSafe, don't need this cross-threading behavior (only called on IO thread) and the rest of this interface doesn't have much need for thread-safety. I think it's better to use the default //net "everything is single-threaded" contract.
Oh hrm, I missed that this still said DRAFT and Ryan left comments on the previous patch set. There's a good chance we overlapped.
Ryan, David, PTAL, focusing on the wiring in the following files: chrome/browser/io_thread.cc chrome/browser/io_thread.h chrome/browser/profiles/profile_impl_io_data.cc chrome/browser/profiles/profile_io_data.cc chrome/browser/profiles/profile_io_data.h I'd like you to comment on the viability of the chosen design which follows quite closely what we discussed over email (the major difference being that the URLRequestContext does not take ownership of the CTVerifier as the URLRequestContext can be cloned and I didn't want to make the CTVerifier ref-counted, opting for explicit life-cycle management instead). Please note that this is still a DRAFT cl, some known issues: * Chatty! (LOG/VLOG statements scattered around). * No tests. * Have not reworked the OffTheRecordProfileIOData to use the new design. * Have not addressed David's comments about URL fetching. * Documentation, style lacking. https://codereview.chromium.org/1100003006/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1100003006/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:743: net::CTLogVerifier::Create(ct_public_key_data, log_description, "")); On 2015/04/24 10:42:05, Ryan Sleevi wrote: > s/""/std::string()/ Obsolete. https://codereview.chromium.org/1100003006/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:901: certificate_transparency::NewSCTsObserver* scts_observer = On 2015/04/24 10:42:05, Ryan Sleevi wrote: > LEAK/DESIGN: Where does this ever get deleted? > > If AddObserver takes ownership, than RemoveObserver becomes a dangerous API > (since the pointer may have been deleted by AddObserver's implementation > whenever it wants, so it's not safe for the caller to hold on to the pointer). > > I think (having only read header files thus far) that AddObserver *doesn't* take > ownership, in which case, this is a leak. Per design discussion over email, the observer is now owned by the IOThread globals and gets deleted before the URLRequestContext. To prevent the CTVerifier from invoking it after it was deleted (but before the CTVerifier itself was deleted) I've added a StopNotifying method to the CTVerifier which causes it to nullify the pointer. https://codereview.chromium.org/1100003006/diff/1/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/1100003006/diff/1/components/BUILD.gn#newcode168 components/BUILD.gn:168: "//components/certificate_transparency", # Blocked on content. On 2015/04/24 10:42:05, Ryan Sleevi wrote: > Is this just copy-pasta? nothing in //components/certificate_trasparency depends > on content. > > I think what you want is to do is move line 29 to line 221 Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency.gypi (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency.gypi:16: '../content/content.gyp:content_common', On 2015/04/24 10:42:06, Ryan Sleevi wrote: > You don't seem to depend on this (as reflected in your gn) I do now (see TreeStateTracker). https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/BUILD.gn:5: static_library("ct_proofs_fetcher") { On 2015/04/24 10:42:06, Ryan Sleevi wrote: > This name isn't right; it doesn't match what you said in the GYP file (that this > target is called 'certificate_transparency') nor does it have all your files. Fixed. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/DEPS (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/DEPS:3: "+base/memory", On 2015/04/24 10:42:06, Ryan Sleevi wrote: > Just +base is sufficient (it includes subdirectories) Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/DEPS:6: "+net/url_request", On 2015/04/24 10:42:06, Ryan Sleevi wrote: > just +net is sufficient here for a component. Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/log_proofs_fetcher.cc (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:21: const int kMaxSTHSize = 600; On 2015/04/24 10:42:07, Ryan Sleevi wrote: > 1) Document > 2) SizeIn.... Bytes? Kb? > > kMaxSTHSizeInBytes ? Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:32: } On 2015/04/24 10:42:06, Ryan Sleevi wrote: > SPECIAL NOTE: Because you have a URLRequestContext , and you're making requests, > you have to follow the rule that all of this requests are cancelled before the > URLRequest is destroyed. > > [I haven't checked that, not sure how best to ensure it, but just an early > design note we can go over] Done - cancelled all requests. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:45: GURL fetch_url(fetch_sth_url); On 2015/04/24 10:42:06, Ryan Sleevi wrote: > SOMETHING: I suspect we have something for appending paths to GURLs here w/o > having to do the to-ascii-and-back conversion in line 44. I'll have to check. Found it - GURL has a Resolve method that does that. Note changes to ct_known_logs_static to preperd '/' as it needed for Resolve() to correctly append to the path rather than replace. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:57: VLOG(0) << "Response started for fetch-sth from " << request->original_url(); On 2015/04/24 10:42:06, Ryan Sleevi wrote: > VLOG(0) is wrong; VLOG(1) at best. > > That said, this seems unnecessary - the net-internals log will have this state > transition. I've changed all VLOGs in this file to VLOG(1), however I intend to remove them all; they were added to aid debugging. Please ignore all LOG/VLOG statements - I'll tidy them up in a follow-up patchset. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:61: << " error:" << status.error(); On 2015/04/24 10:42:06, Ryan Sleevi wrote: > ditto; net-internals will have this (since it'll have logged the request) Acknowledged. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:64: LOG(WARNING) << "Fetch STH HTTP status: " << request->GetResponseCode(); On 2015/04/24 10:42:07, Ryan Sleevi wrote: > ditto; net-internals will have this. Acknowledged. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:69: RequestsMap::iterator it(inflight_requests_.find(request)); On 2015/04/24 10:42:06, Ryan Sleevi wrote: > auto it = inflight_requests_.find(request); Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:72: VLOG(0) << "Kicking off read."; On 2015/04/24 10:42:07, Ryan Sleevi wrote: > all these vlogs are in net-internals; I'll stop whinging :) Acknowledged. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:91: RequestsMap::iterator it(inflight_requests_.find(request)); On 2015/04/24 10:42:06, Ryan Sleevi wrote: > auto it = inflight_requests_.find(request) Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:93: if (it->first != request) { On 2015/04/24 10:42:06, Ryan Sleevi wrote: > seems like a fatal error here, right? > > if (it->first != request) { > NOTREACHED(); > return; > } Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:104: if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) { On 2015/04/24 10:42:06, Ryan Sleevi wrote: > SECURITY BUG: You forgot to allocate |sth|, but FillSignedTreeHead happily > starts scribbling into it. > > I believe you meant to write > > net::ct::SignedTreeHead sth; > if (CTLog....(json_sth, &sth)) { > > } Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:104: if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) { On 2015/04/24 10:42:06, Ryan Sleevi wrote: > STYLE: We tend to put error control *before* the rest of processing, so that we > can clearly reason about the current state of execution at any given time. > > if (!CTLogResponseParser::....) { > LOG(ERROR) << "Invalid STH"; // Probably too chatty > RequestCleanup(request); > return; > } > > .... Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:104: if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) { On 2015/04/24 10:42:06, Ryan Sleevi wrote: > SECURITY BUG: You forgot to allocate |sth|, but FillSignedTreeHead happily > starts scribbling into it. > > I believe you meant to write > > net::ct::SignedTreeHead sth; > if (CTLog....(json_sth, &sth)) { > > } Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:106: if (params->verifier->VerifySignedTreeHead(sth.get())) { On 2015/04/24 10:42:06, Ryan Sleevi wrote: > ditto here, whenever this "becomes something important". > > (It's unclear whether/if/how we want to surface information about STH > verifications; it may be that we introduce some delegate about OnSTHVerified() / > OnSTHFailed() or something more nuanced, and then let the higher layers glue up > appropriate policy) Ack,Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:119: RequestsMap::iterator it(inflight_requests_.find(request)); On 2015/04/24 10:42:07, Ryan Sleevi wrote: > auto Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:126: VLOG(0) << "Erasing iterator."; On 2015/04/24 10:42:06, Ryan Sleevi wrote: > too chatty (as with all the VLOGs), so hushing on that. Acknowledged. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.cc:141: if (json == NULL) { On 2015/04/24 10:42:06, Ryan Sleevi wrote: > s/NULL/nullptr Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/log_proofs_fetcher.h (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:12: #include "net/base/io_buffer.h" On 2015/04/24 10:42:07, Ryan Sleevi wrote: > You don't need to include this; you can forward declare IOBufferWithSize Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:14: On 2015/04/24 10:42:07, Ryan Sleevi wrote: > IWYU: Missing ref_counted Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:17: } On 2015/04/24 10:42:07, Ryan Sleevi wrote: > // namespace base Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:31: class LogProofsFetcher : public net::URLRequest::Delegate { On 2015/04/24 10:42:07, Ryan Sleevi wrote: > naming nit: It's weird to call this plural, and I think reads weird. > > From the point of view of consumers, they fetch a single STH (re: line 37), so > this probably makes more sense as LogProofFetcher [even if it's used to fetch > multiple proofs through multiple calls to FetchSTH] Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:31: class LogProofsFetcher : public net::URLRequest::Delegate { On 2015/04/24 10:42:07, Ryan Sleevi wrote: > style: documentation Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:35: ~LogProofsFetcher() override; On 2015/04/24 10:42:07, Ryan Sleevi wrote: > delete newline on 34? Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:37: void FetchSTH(net::CTLogVerifier* verifier); On 2015/04/24 10:42:07, Ryan Sleevi wrote: > documentation Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:44: scoped_ptr<net::URLRequest> CreateURLRequest(GURL fetch_sth_url); On 2015/04/24 10:42:07, Ryan Sleevi wrote: > pass as const-ref Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:48: void RequestCleanup(net::URLRequest* request); On 2015/04/24 10:42:07, Ryan Sleevi wrote: > naming nit: RequestCleanup -> CleanupRequest (RequestCleanup can either be > Verb-Noun or Noun-Verb, and has different meanings; CleanupRequest makes it > clear it's Verb-Noun) Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:50: struct NET_EXPORT_PRIVATE FetchParams { On 2015/04/24 10:42:07, Ryan Sleevi wrote: > 1) NET_EXPORT_PRIVATE isn't needed > 2) You can forward declare this, since you use a pointer in line 59 > 3) Ordering - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:59: typedef std::map<net::URLRequest*, FetchParams*> RequestsMap; On 2015/04/24 10:42:07, Ryan Sleevi wrote: > You don't actually need this typedef, do you, since you can use auto & friends Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:69: class CTLogResponseParser { On 2015/04/24 10:42:07, Ryan Sleevi wrote: > style: move to a separate file. Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/log_proofs_fetcher.h:71: // Extracts STH from the json struct On 2015/04/24 10:42:07, Ryan Sleevi wrote: > s/json/JSON/ Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... File components/certificate_transparency/new_scts_observer.h (right): https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:26: class LogProofsFetcher; On 2015/04/24 10:42:08, Ryan Sleevi wrote: > newline between 25 & 26 Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:28: class NewSCTsObserver : public net::CTVerifier::Observer { On 2015/04/24 10:42:07, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:30: NewSCTsObserver(LogProofsFetcher* fetcher); On 2015/04/24 10:42:07, Ryan Sleevi wrote: > explicit Ack, n/a anymore (c'tor has two parameters). https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:30: NewSCTsObserver(LogProofsFetcher* fetcher); On 2015/04/24 10:42:07, Ryan Sleevi wrote: > since you take ownership of this (line 40), you should have this as > > explicit NewSCTsObserver(scoped_ptr<LogProofsFetcher> fetcher); > > > (and use the .Pass() semantics which are equivalent to C++11 move semantics) Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:37: typedef std::map<std::string, linked_ptr<net::ct::SignedTreeHead>> IDToSTHMap; On 2015/04/24 10:42:07, Ryan Sleevi wrote: > This typedef isn't needed, is it? especially with the ability to use auto in the > C++ code Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:39: IDToSTHMap sths_; On 2015/04/24 10:42:07, Ryan Sleevi wrote: > document Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:45: } // namespace On 2015/04/24 10:42:07, Ryan Sleevi wrote: > // namespace certificate_transparency Done. https://codereview.chromium.org/1100003006/diff/1/components/certificate_tran... components/certificate_transparency/new_scts_observer.h:47: #endif On 2015/04/24 10:42:07, Ryan Sleevi wrote: > #endif // COMPONENTS_CERTIFICATE_TRANSPARENCY_NEW_SCTS_OBSERVER_H_ Done. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_known_logs_stat... File net/cert/ct_known_logs_static.h (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_known_logs_stat... net/cert/ct_known_logs_static.h:23: "ct.googleapis.com/pilot"}, On 2015/04/24 10:42:08, Ryan Sleevi wrote: > BUG: You forgot the scheme for all these URLs. > > Yes, 6962 says they MUST be HTTPS, so it might be suitable to simply comment on > line 12 to that effect, leave the scheme off these URLs, but add the schemes in > the .cc; > > However, since this information is exported/exposed, I think it may be cleaner > to always put the https:// in here, so that we don't have multiple callsites > needing to remember to do so before presenting/acting on the URL. Done. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier.h File net/cert/ct_log_verifier.h (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier.h#... net/cert/ct_log_verifier.h:58: bool VerifySignedTreeHead(const ct::SignedTreeHead* signed_tree_head); On 2015/04/24 10:42:08, Ryan Sleevi wrote: > No need to pass as pointer - just pass as const-ref Done. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier_un... File net/cert/ct_log_verifier_unittest.cc (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier_un... net/cert/ct_log_verifier_unittest.cc:22: log_ = CTLogVerifier::Create(ct::GetTestPublicKey(), "testlog", "").Pass(); On 2015/04/24 10:42:08, Ryan Sleevi wrote: > s/""/std::string() Done. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier_un... net/cert/ct_log_verifier_unittest.cc:82: ASSERT_TRUE(log_->VerifySignedTreeHead(sth.get())); On 2015/04/24 10:42:08, Ryan Sleevi wrote: > no need for .get() for scoped_ptr's; it's cleaner that way. Done. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_log_verifier_un... net/cert/ct_log_verifier_unittest.cc:97: scoped_ptr<CTLogVerifier> log = CTLogVerifier::Create(key, "testlog", ""); On 2015/04/24 10:42:08, Ryan Sleevi wrote: > s/""/std::string() Done. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_objects_extract... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_objects_extract... net/cert/ct_objects_extractor_unittest.cc:34: log_ = CTLogVerifier::Create(ct::GetTestPublicKey(), "testlog", "").Pass(); On 2015/04/24 10:42:08, Ryan Sleevi wrote: > s/""/std::string()/ Done. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_verifier.h#newc... net/cert/ct_verifier.h:26: CTVerifier(); On 2015/04/24 10:42:08, Ryan Sleevi wrote: > DESIGN: This mixes interface with implementation > > That is, this used to be a pure virtual class, now it's a pure virtual with > public virtual. I think we need to rethinking this; lets chat. Done - this class remains pure virtual (without any data members); I have left the SetObserver here as I expect every implementation will notify the observer of new SCTs. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_verifier.h#newc... net/cert/ct_verifier.h:45: class NET_EXPORT Observer { On 2015/04/24 10:42:08, Ryan Sleevi wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/1100003006/diff/1/net/cert/ct_verifier.h#newc... net/cert/ct_verifier.h:62: void AddObserver(Observer* observer); On 2015/04/24 10:42:08, Ryan Sleevi wrote: > Will there ever be more than one observer at a time with your API contract? > > If not, let's look at alternatives. No - changed to only having one. https://codereview.chromium.org/1100003006/diff/1/net/cert/multi_log_ct_verif... File net/cert/multi_log_ct_verifier_unittest.cc (right): https://codereview.chromium.org/1100003006/diff/1/net/cert/multi_log_ct_verif... net/cert/multi_log_ct_verifier_unittest.cc:42: CTLogVerifier::Create(ct::GetTestPublicKey(), kLogDescription, "")); On 2015/04/24 10:42:08, Ryan Sleevi wrote: > s/""/std::string() Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency.gypi (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency.gypi:10: 'type': 'static_library', On 2015/05/07 21:59:36, David Benjamin wrote: > IMPORTANT: Because this is a static_library rather than <(component), while you > don't need a certificate_transparency_export.h, you cannot depend on this from > more than one component (in the shared library vs static library sense of the > word). Otherwise you get two copies of it. That's probably fine since //content > isn't going to depend on this anyway. But, for instance, content -> > certificate_transparency + chrome -> certificate_transparency isn't going to > work. > > I don't have a good sense of which //components normally goes with. It seems to > mostly be static_library, but there are a few <(component)s. I guess that makes > sense since //content doesn't typically depend on these. AFAICT the dependency now is chrome -> certificate_transparency -> content. Is that sensible? I see no reason content will ever depend on that directly. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency.gypi:16: '../content/content.gyp:content_common', On 2015/05/07 21:59:36, David Benjamin wrote: > This doesn't actually depend on //content (and the dependency isn't in GN or > DEPS). It now does. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/BUILD.gn (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/BUILD.gn:8: "log_proofs_fetcher.h", On 2015/05/07 21:59:36, David Benjamin wrote: > new_scts_observer.{cc,h}? Done (sort of; it's now called tree_state_tracker.) https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/log_proofs_fetcher.h (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.h:50: struct NET_EXPORT_PRIVATE FetchParams { On 2015/05/07 21:59:38, David Benjamin wrote: > Why is this NET_EXPORT_PRIVATE? It's not in net and doesn't need to be exported > at all. Done.
https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:697: for (auto it = known_logs.begin(); it != known_logs.end(); ++it) STYLE: "const auto&" alternatively: for (known_log : known_logs) { globals_->ct_logs.push_back(known_log); } https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.h:131: std::vector<linked_ptr<net::CTLogVerifier>> ct_logs; XXX - Replacing linked_ptr<> w/ making net::CTLogVerifier explicitly RefCounted https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.h:165: scoped_ptr<net::CTVerifier::Observer> cert_transparency_observer; XXX - sanity check this with eroman to make sure having this for |proxy_script_fetcher| makes sense https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1118: new certificate_transparency::TreeStateTracker( XXX https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1278: cert_transparency_verifier_->StopNotifications(); XXX https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.h:593: mutable scoped_ptr<net::CTVerifier::Observer> cert_transparency_observer_; XXX https://codereview.chromium.org/1100003006/diff/60001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.cc (right): https://codereview.chromium.org/1100003006/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:39: for (auto it = inflight_requests_.begin(); it != inflight_requests_.end(); const auto& || for (inflight_request : inflight_requests_) { inflight_request.first->cancel(); } https://codereview.chromium.org/1100003006/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:52: request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES | TBD: Whether or not we should send AUTH_DATA or any of the other load flags https://codereview.chromium.org/1100003006/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.cc:125: scoped_refptr<safe_json_parser::SafeJsonParser> parser = nit: s/parser = /parser(/ https://codereview.chromium.org/1100003006/diff/60001/components/certificate_... File components/certificate_transparency/log_proof_fetcher.h (right): https://codereview.chromium.org/1100003006/diff/60001/components/certificate_... components/certificate_transparency/log_proof_fetcher.h:37: explicit LogProofFetcher(net::URLRequestContext* request_context); Document the lifetime requirements re: |request_context| https://codereview.chromium.org/1100003006/diff/60001/components/certificate_... File components/certificate_transparency/tree_state_tracker.h (right): https://codereview.chromium.org/1100003006/diff/60001/components/certificate_... components/certificate_transparency/tree_state_tracker.h:31: // knows about. For now, only stores Signed Tree Heads. Comment nit: "For now" suggests there may be more, but you don't hint (via bug or todo) what the more may be https://codereview.chromium.org/1100003006/diff/60001/components/safe_json_pa... File components/safe_json_parser/safe_json_parser.cc (right): https://codereview.chromium.org/1100003006/diff/60001/components/safe_json_pa... components/safe_json_parser/safe_json_parser.cc:41: } As discussed - follow up on the other CL https://codereview.chromium.org/1100003006/diff/60001/net/cert/ct_log_verifie... File net/cert/ct_log_verifier_openssl.cc (right): https://codereview.chromium.org/1100003006/diff/60001/net/cert/ct_log_verifie... net/cert/ct_log_verifier_openssl.cc:112: if (public_key_ == NULL) { if (!public_key_) https://codereview.chromium.org/1100003006/diff/60001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/1100003006/diff/60001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:31: virtual void OnSCTVerified(const ct::SignedCertificateTimestamp* sct, Document when/how this is called https://codereview.chromium.org/1100003006/diff/60001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:32: CTLogVerifier* verifier) {} make this pure virtual https://codereview.chromium.org/1100003006/diff/60001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:38: DISALLOW_COPY_AND_ASSIGN(Observer); delete; unneeded for pure interfaces https://codereview.chromium.org/1100003006/diff/60001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:70: DISALLOW_COPY_AND_ASSIGN(CTVerifier); ditto about unneeded for pure interfaces
NO NEED TO REVIEW! Just FYI as I've addressed the comments and will be breaking off this big change to a series of smaller ones. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/log_proofs_fetcher.cc (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:3: // found in the LICENSE file. On 2015/05/07 21:59:36, David Benjamin wrote: > Since this already has some significant logic now, unit test? Acknowledged - will add to a later patchset. Any examples you can point to of tests using a mock/test URLRequestContext? (I see there's a TestURLRequestContext so will look up myself if you don't have anything off the top of your head) https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:21: const int kMaxSTHSize = 600; On 2015/05/07 21:59:37, David Benjamin wrote: > Probably worth a comment. Notably mention that it's in bytes. Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:34: scoped_ptr<net::URLRequest> LogProofsFetcher::CreateURLRequest( On 2015/05/07 21:59:37, David Benjamin wrote: > Nit: This doesn't match header file order. Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:46: request->set_method("GET"); On 2015/05/07 21:59:36, David Benjamin wrote: > Nit: Somewhat strange separation between CreateRequest and FetchSTH; the former > sets the URL while the latter sets the method. Fixed, CreateURLRequest now sets the method. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:51: raw_request->Start(); On 2015/05/07 21:59:36, David Benjamin wrote: > Could also write this perhaps as > > auto it = inflight_requests.insert(...)->first; > it->first->Start(); Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:60: << " error:" << status.error(); On 2015/05/07 21:59:37, David Benjamin wrote: > I imagine later we'll surface this up through to something interesting? Correct. Particularly, I'd like DomainReliability to collect these metrics and report to us, or collect UMA for that. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:72: request->Read(buffer.get(), buffer->size(), &(it->second->read_bytes)); On 2015/05/07 21:59:36, David Benjamin wrote: > Nit: I don't think those parens are necessary. Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:72: request->Read(buffer.get(), buffer->size(), &(it->second->read_bytes)); On 2015/05/07 21:59:36, David Benjamin wrote: > [Confirmed with mmenke that ignoring URLRequest::Read and checking > request->status().is_io_pending() is preferred for now, since that's what > ResourceLoader does. Making this API sane in the cleanup queue.] Acknowledged. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:83: int bytes_read) { On 2015/05/07 21:59:37, David Benjamin wrote: > bytes_read may be -1 on error. You pull the error code out of request->status(). > (Yeah, this API is kinda messy.) Done, is there anything interesting to do with the error code other than logging though? In case of bytes_read being -1 I'm just cleaning up the request now. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:85: RequestComplete(request, bytes_read); On 2015/05/07 21:59:37, David Benjamin wrote: > There's no guarantee that the entire request will come in in one Read. You need > to keep pumping the read loop until it returns 0. Fixed by creating a CheckReadStatus method to handle the result. Let me know what you think of it. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:93: LOG(WARNING) << "Different requests?!"; On 2015/05/07 21:59:37, David Benjamin wrote: > This can just be a DCHECK, right? If this fails, std::map is kinda broken... Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:96: FetchParams* params(it->second); On 2015/05/07 21:59:36, David Benjamin wrote: > Nit: This can just use = Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:99: // Extract STH json an parse it On 2015/05/07 21:59:37, David Benjamin wrote: > Nit: period at the end of comment. Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:99: // Extract STH json an parse it On 2015/05/07 21:59:37, David Benjamin wrote: > I'd DCHECK that bytes_read is within the buffer's size. Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:103: if (CTLogResponseParser::FillSignedTreeHead(json_sth, sth.get())) { On 2015/05/07 21:59:36, David Benjamin wrote: > This looks like it'll just crash on a null pointer (a test would probably have > noticed ;-) ). sth.get() is null. I think you want to stack-allocate it and pass > in &sth. Or initialize it to new SignedTreeHead if you need to pass it around. Fixed. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:111: LOG(ERROR) << "Invalid STH."; On 2015/05/07 21:59:37, David Benjamin wrote: > (I'm assuming this is all to be routed up to something real later.) It's now passed to a callback. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:137: net::ct::SignedTreeHead* sth) { On 2015/05/07 21:59:37, David Benjamin wrote: > How much do we trust our JSON parser on untrusted sources? (I don't actually > know.) Not a lot, which is why I've switched to the SafeJsonParser. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:139: base::Value* json = json_reader.Read(json_sth); On 2015/05/07 21:59:37, David Benjamin wrote: > This leaks memory. scoped_ptr<base::Value> json(json_reader.Read(json_sth)); > > (That function really should return a scoped_ptr...) Done - irrelevant now. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:140: if (json == NULL) { On 2015/05/07 21:59:37, David Benjamin wrote: > nullptr. Or just !json. Done - irrelevant now. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:144: On 2015/05/07 21:59:37, David Benjamin wrote: > Comment linking to documentation on the format? [I'm assuming RFC 6962] Should be in the ct_log_response_parser. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:147: LOG(WARNING) << "Json value not a dictionary."; On 2015/05/07 21:59:37, David Benjamin wrote: > Json -> JSON, ditto below Done - irrelevant. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:182: LOG(WARNING) << "sha256_root_hash must be exactly 32 bit."; On 2015/05/07 21:59:36, David Benjamin wrote: > bit -> bits N/A. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:187: if (!base::Base64Decode(tree_head_signature, &decoded_signature)) { On 2015/05/07 21:59:37, David Benjamin wrote: > [Incidentally, RFC 6962 fails to mention that this field is base64-encoded.] Filed http://trac.tools.ietf.org/wg/trans/trac/ticket/91. Will also file an errata for RFC6962. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:193: if (!DecodeDigitallySigned(&sp, &(sth->signature))) { On 2015/05/07 21:59:36, David Benjamin wrote: > parens after & unnecessary. Done. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:193: if (!DecodeDigitallySigned(&sp, &(sth->signature))) { On 2015/05/07 21:59:37, David Benjamin wrote: > This should check sp.empty() afterwards, or you'll ignore trailing junk in the > signature. Good point, will do in a separate CL (since the code does not live here anymore). https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/log_proofs_fetcher.cc:202: memcpy(sth->sha256_root_hash, decoded_root_hash.c_str(), On 2015/05/07 21:59:36, David Benjamin wrote: > Nit: I would c_str -> data. It's totally meaningless in C++11 which requires > they be the same, but data makes it clear you don't care about the trailing NUL. Done - N/A anymore. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/new_scts_observer.cc (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/new_scts_observer.cc:27: // fresher STH. On 2015/05/07 21:59:38, David Benjamin wrote: > (I'm assuming this is to be filled in later.) Correct, for now there's no point in even storing them as we don't have the code to check for inclusion of the relevant certificates in CT logs. https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... File components/certificate_transparency/new_scts_observer.h (right): https://codereview.chromium.org/1100003006/diff/20001/components/certificate_... components/certificate_transparency/new_scts_observer.h:27: On 2015/05/07 21:59:38, David Benjamin wrote: > Comment describing what this class does. Done. https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_log_verifier.h File net/cert/ct_log_verifier.h (left): https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_log_verifie... net/cert/ct_log_verifier.h:57: bool SetSignedTreeHead(scoped_ptr<ct::SignedTreeHead> signed_tree_head); On 2015/05/07 21:59:38, David Benjamin wrote: > I'm assume this'll go away in a rebase? I remember reviewing this already. Correct. https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:48: On 2015/05/07 21:59:38, David Benjamin wrote: > Add a comment for when this is called and what |verifier| means here (since you > have both CTVerifier and CTLogVerifier). Maybe verifier -> log_verifier or log? Done. https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:50: CTLogVerifier* verifier) {} On 2015/05/07 21:59:38, David Benjamin wrote: > I wonder if this is better in MultiLogCTVerifier instead. Then you keep the > interface pure. CTVerifier doesn't know that a single verifier secretly > multiplexes between multiple CTLogVerifiers. It seems //net itself only cares > about the Verify hook. You could argue that anything keying off of OnSCTVerified > is a detail of MultiLogCTVerifier's Verify implementation: "hey net stack, here > is how you verify SCTs. Oh, don't worry, I'll deal with checking up with the log > asynchronously". > > I don't feel that strongly either way. Mostly the CTLogVerifier part was a > little confusing. The distinction between CTVerifier and MultiLogCTVerifier is a bit artificial - mostly for describing a clear interface. I don't expect another class implementing CTVerifier. For now, if that's OK with you, let's leave it like this and change later if we see the plumbing does not make sense. https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:61: // called back with notifications. On 2015/05/07 21:59:38, David Benjamin wrote: > Is this true? We're not using an ObserverListThreadSafe, don't need this > cross-threading behavior (only called on IO thread) and the rest of this > interface doesn't have much need for thread-safety. > > I think it's better to use the default //net "everything is single-threaded" > contract. Right, corrected the comment. https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:697: for (auto it = known_logs.begin(); it != known_logs.end(); ++it) On 2015/06/29 11:58:12, Ryan Sleevi (slow through 7-15 wrote: > STYLE: "const auto&" > > alternatively: > for (known_log : known_logs) { > globals_->ct_logs.push_back(known_log); > } Done. https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_thread.h File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.h:131: std::vector<linked_ptr<net::CTLogVerifier>> ct_logs; On 2015/06/29 11:58:12, Ryan Sleevi (slow through 7-15 wrote: > XXX - Replacing linked_ptr<> w/ making net::CTLogVerifier explicitly RefCounted Done. https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.h:165: scoped_ptr<net::CTVerifier::Observer> cert_transparency_observer; On 2015/06/29 11:58:13, Ryan Sleevi (slow through 7-15 wrote: > XXX - sanity check this with eroman to make sure having this for > |proxy_script_fetcher| makes sense Another option is to have a CTVerifier for the proxy_script_fetcher_context that does not have an observer set so it will only validate received SCTs. https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1118: new certificate_transparency::TreeStateTracker( On 2015/06/29 11:58:13, Ryan Sleevi (slow through 7-15 wrote: > XXX Confirm wiring of main_request_context_ IIRC. https://codereview.chromium.org/1100003006/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1278: cert_transparency_verifier_->StopNotifications(); On 2015/06/29 11:58:13, Ryan Sleevi (slow through 7-15 wrote: > XXX To be confirmed with davidben/mmenke (also in the ProfileIOData).
https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:50: CTLogVerifier* verifier) {} On 2015/07/10 13:15:48, Eran wrote: > For now, if that's OK with you, let's leave it like this and change later if we > see the plumbing does not make sense. I seem to recall us discussing this in person and agreeing that it wasn't needed? I do want to echo David's remark of not wanting to pass this along - would prefer multiplexing be coalesced in a subclass. https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:50: CTLogVerifier* verifier) {} On 2015/07/10 13:15:48, Eran wrote: > The distinction between CTVerifier and MultiLogCTVerifier is a bit artificial - > mostly for describing a clear interface. I don't expect another class > implementing CTVerifier. Well, no, it was somewhat intentional, not artificial :) For example, I anticipate we may have separate subclasses for V1 verifications vs V2, although that remains TBD based on how much is changing (the precert aspect is certainly one area of weirdness), such that the MultiLogCertVerifier instantiates an appropriate Verifier subclass based on the version the log supports, so that the MultiLog can handle (transparently) validation of both V1 and V2 SCTs. But it was also to keep the separations cleaner - CTVerifier knows about the crypto and the protocol, whereas MultiLogCTVerifier nows about the conceptual mappings and collations and how to handle missing logs.
https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/1100003006/diff/20001/net/cert/ct_verifier.h#... net/cert/ct_verifier.h:50: CTLogVerifier* verifier) {} On 2015/07/10 13:45:02, Ryan Sleevi (slow through 7-15 wrote: > On 2015/07/10 13:15:48, Eran wrote: > > The distinction between CTVerifier and MultiLogCTVerifier is a bit artificial > - > > mostly for describing a clear interface. I don't expect another class > > implementing CTVerifier. > > Well, no, it was somewhat intentional, not artificial :) > > For example, I anticipate we may have separate subclasses for V1 verifications > vs V2, although that remains TBD based on how much is changing (the precert > aspect is certainly one area of weirdness), such that the MultiLogCertVerifier > instantiates an appropriate Verifier subclass based on the version the log > supports, so that the MultiLog can handle (transparently) validation of both V1 > and V2 SCTs. > > But it was also to keep the separations cleaner - CTVerifier knows about the > crypto and the protocol, whereas MultiLogCTVerifier nows about the conceptual > mappings and collations and how to handle missing logs. Acknowledged - the CTLogVerifier* is not needed here and indeed the TreeStateTracker can hold onto the verifiers so there's no need to pass them in. I agree with the comment about separation between crypto/protocol and conceptual mappings in the MultiLogCTVerifier - it would make it much simpler to upgrade to V2 with this approach.
Description was changed from ========== Certificate Transparency: Fetching of Signed Tree Heads (DRAFT) This change lays out the groundwork for fetching and validating signed tree heads from recognized CT logs. BUG=480867 ========== to ========== Certificate Transparency: Fetching of Signed Tree Heads (DRAFT) This change lays out the groundwork for fetching and validating signed tree heads from recognized CT logs. BUG=480867 ==========
eranm@chromium.org changed reviewers: - davidben@chromium.org, eroman@chromium.org, rsleevi@chromium.org |
