Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2021433004:
Cert - protobufs to serialize and deserialize CertVerifierCache. (Closed)
|
Created:
4 years, 6 months ago by ramant (doing other things) Modified:
4 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, eroman, jbudorick Base URL:
https://chromium.googlesource.com/chromium/src.git@Add_support_for_walking_1999733002 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCert - protobufs to serialize and deserialize CertVerifierCache.
BUG=607354
R=rsleevi@chromium.org, mef@chromium.org,
Committed: https://crrev.com/18f5a46af8dcde2ae88873d03d2908949678af75
Cr-Commit-Position: refs/heads/master@{#402020}
Patch Set 1 #Patch Set 2 : Added histogram for cache size #
Total comments: 20
Patch Set 3 : #
Total comments: 12
Patch Set 4 : Fix comments and more tests #
Total comments: 53
Patch Set 5 : Fix comments for Patch Set 4 #Patch Set 6 : Move code to cronet #
Total comments: 5
Patch Set 7 : removed changes to ios files #Patch Set 8 : delete //net/data/ssl/... #
Total comments: 38
Patch Set 9 : Fix comments #Patch Set 10 : Rebase TOT #
Total comments: 20
Patch Set 11 : Fix comments #
Total comments: 64
Patch Set 12 : Minor fix to comment #
Total comments: 4
Patch Set 13 : Fix comments #
Total comments: 6
Patch Set 14 : Fix comments #Depends on Patchset: Messages
Total messages: 90 (43 generated)
rtenneti@chromium.org changed reviewers: + rch@chromium.org
rtenneti@chromium.org changed reviewers: + rsleevi@chromium.org
Hi Ryan H and Ryan S, Extracted protobuf code of the following CL into a separate CL. It serializes and de-serializes the new CertVerifierCache structure (most of the code is same as the following CL). https://codereview.chromium.org/1892033002/ This CL is dependent CLs: 1994353002, 2000503002, 1991653002 and 1999733002. This code is not yet enabled.
I think my biggest concern here right now is with the documentation; I haven't looked over the implementation in depth because I wanted to focus on the documentation/interfaces and make sure we're on good footing. I'm trying to be direct here in the concerns, to avoid any miscommunication, although I do fear it may come off harsher than I intend. Please don't mistake it for frustration/anger/any emotional reaction - I just want to make sure it's clear where the disagreements are. I've tried to suggest paths forward, but I think you really want to take careful review over this (perhaps with rch@) and make sure your documentation is updated, consistent, and with the right layering. That is, don't talk about how a given X "is" used - talk about how to use it right. I think that will be very important for this CL (and in general) Also, as a general review, consider avoiding pronouns in comments - you can read more about https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M . by avoiding terms like "we" and "you", if generally results in comments that are clearer, more direct, and without the tendency for layering violations https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... File net/extras/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.cc:34: std::find(serialized_certs->begin(), serialized_certs->end(), encoded); EFFICIENCY: These |encoded| strings are going to be between 1-3K each, and will share in common a good portion of the first 12-30 bytes (up until the serial number), which will likely force cache misses and force the entire string to be loaded. Just calling it out. (For example, you could consider using a hash_map to map cert->index, and just linearly increment that way, without the need for std::distance or std::find) Recall that the cache stores 256 entries, of full certificates, so that will be average 3-4 certificates an entry (not to mention the CertVerifyResult verify_chain being another set). Even with a Google-service bias (where we stuff 100+ SANs in a cert), this could be less than ideal. It really depends on defining for your use case, and the only way I can see this being the 'right' case is for situations where it's talking to Google services and maybe 10-15 entries. I think if we look at 'real world' browsing, even on mobile, this will likely have 100 or so entries, non-overlapping - and the memory pressures this begins to put (which isn't about the O(N) complexities but the L1/L2 caching) will likely make this not very performant - AND it will force evictions of other data from the cache lines. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... File net/extras/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:7: // deals with writing that data out to disk as needed and loading it at startup. 1) The first sentence isn't accurate - the CertVerifierCachePersister doesn't do this 2) The second sentence isn't accurate - this isn't a singleton, doesn't handle those cases. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:14: // that data to disk. Lines 9 - 14 are a clear layering violation - you shouldn't talk about how this class is used (by higher layers) - you should focus on documenting how this class is used. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:20: #include <vector> Unused https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:22: #include "base/compiler_specific.h" What's being used from this header? https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:32: typedef std::vector<std::string> CertVector; Unused. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... File net/extras/cert/proto/cert_verification.proto (right): https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... net/extras/cert/proto/cert_verification.proto:11: message CertVerificationSHA1HashValue { Unused? https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... net/extras/cert/proto/cert_verification.proto:16: // Each unique certficate gets an unique number. This doesn't seem like a really good comment - and seems to be duplicated (lines 22, 27). https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... net/extras/cert/proto/cert_verification.proto:48: Weird style here for whitespace https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... net/extras/cert/proto/cert_verification.proto:77: // MultiThreadedCertVerifier's CertVerifierCache. Outdated
https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... File net/extras/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.cc:34: std::find(serialized_certs->begin(), serialized_certs->end(), encoded); On 2016/05/30 18:37:18, Ryan Sleevi (OOO til 6-6) wrote: > EFFICIENCY: These |encoded| strings are going to be between 1-3K each, and will > share in common a good portion of the first 12-30 bytes (up until the serial > number), which will likely force cache misses and force the entire string to be > loaded. Just calling it out. > > (For example, you could consider using a hash_map to map cert->index, and just > linearly increment that way, without the need for std::distance or std::find) > > Recall that the cache stores 256 entries, of full certificates, so that will be > average 3-4 certificates an entry (not to mention the CertVerifyResult > verify_chain being another set). Even with a Google-service bias (where we stuff > 100+ SANs in a cert), this could be less than ideal. > > It really depends on defining for your use case, and the only way I can see this > being the 'right' case is for situations where it's talking to Google services > and maybe 10-15 entries. I think if we look at 'real world' browsing, even on > mobile, this will likely have 100 or so entries, non-overlapping - and the > memory pressures this begins to put (which isn't about the O(N) complexities but > the L1/L2 caching) will likely make this not very performant - AND it will force > evictions of other data from the cache lines. Done. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... File net/extras/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:7: // deals with writing that data out to disk as needed and loading it at startup. On 2016/05/30 18:37:19, Ryan Sleevi (OOO til 6-6) wrote: > 1) The first sentence isn't accurate - the CertVerifierCachePersister doesn't do > this > 2) The second sentence isn't accurate - this isn't a singleton, doesn't handle > those cases. Done. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:14: // that data to disk. On 2016/05/30 18:37:19, Ryan Sleevi (OOO til 6-6) wrote: > Lines 9 - 14 are a clear layering violation - you shouldn't talk about how this > class is used (by higher layers) - you should focus on documenting how this > class is used. Done. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:20: #include <vector> On 2016/05/30 18:37:19, Ryan Sleevi (OOO til 6-6) wrote: > Unused Done. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:22: #include "base/compiler_specific.h" On 2016/05/30 18:37:19, Ryan Sleevi (OOO til 6-6) wrote: > What's being used from this header? Done. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:32: typedef std::vector<std::string> CertVector; On 2016/05/30 18:37:18, Ryan Sleevi (OOO til 6-6) wrote: > Unused. Done. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... File net/extras/cert/proto/cert_verification.proto (right): https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... net/extras/cert/proto/cert_verification.proto:11: message CertVerificationSHA1HashValue { On 2016/05/30 18:37:20, Ryan Sleevi (OOO til 6-6) wrote: > Unused? Done. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... net/extras/cert/proto/cert_verification.proto:16: // Each unique certficate gets an unique number. On 2016/05/30 18:37:19, Ryan Sleevi (OOO til 6-6) wrote: > This doesn't seem like a really good comment - and seems to be duplicated (lines > 22, 27). Took a cut at the comment. Done. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... net/extras/cert/proto/cert_verification.proto:48: On 2016/05/30 18:37:19, Ryan Sleevi (OOO til 6-6) wrote: > Weird style here for whitespace Done. https://codereview.chromium.org/2021433004/diff/20001/net/extras/cert/proto/c... net/extras/cert/proto/cert_verification.proto:77: // MultiThreadedCertVerifier's CertVerifierCache. On 2016/05/30 18:37:20, Ryan Sleevi (OOO til 6-6) wrote: > Outdated Done.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... File net/extras/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.cc:59: DCHECK(false); DCHECK(false) is an anti-pattern NOTREACHED() is the idiomatic form. Same throughout. https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... File net/extras/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:22: class NET_EXPORT_PRIVATE CertVerifierCachePersister { This doesn't seem like it needs to be a class anymore, now that the iterator interface is cleaned up and no friends are used (the protobuf class) SerializeCertVerifierCache(const CachingCertVerifier& verifier); (void/bool?) DeserializeCertVerifierCache(const TheProtobufClass&, CachingCertVerifier* verifier); https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:24: CertVerifierCachePersister(CachingCertVerifier* verifier); STYLE: Explicit https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:28: // write the hierarchical structure into |data|. |cache_| is not a recursive structure. Also, you're talking about implementation details. You're not accessing |cache_|, you're accessing it's iterator. Talk in terms of public interfaces // Iterates through |verifier_|'s cache and serializes the data to |data|. This can be used to populate a new CachingCertVerifier with // |LoadCache()| https://codereview.chromium.org/2021433004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2021433004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25630: + The number of CertVerificationCache's entries that were deserialized. GRAMMAR: This feels weird (notably, "CertVerificationCache's entries"). This also feels like it all three might be at the wrong layer - the time spent (and number of entries deserialized) seems not relevant as a whole, but to a specific use (in this case, presumably, a profile's serialization/deserialization time) Because there is no limit on how many CertVerificationCaches there are, nor as to how many CachingCertVerifiers there are (and of course, there are many - per profile, URLRequestContext, etc), then the metric you're collecting here is not as valuable. Moving this to be dealt with in your #5 case seems more relevant to what you're trying to measure, since you're concerned about a very specific callsites' time spent, right? https://codereview.chromium.org/2021433004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25637: + The amount of time spent deserializing and populating CertVerifiers' cache. GRAMMAR: Either caches should be plural (to reflect the plurality of CertVerifiers), or you should make CertVerifiers' singular - and "populating a CertVerifier's cache." Same with below
rtenneti@chromium.org changed reviewers: + asvitkine@chromium.org
Many many thanks Ryan for the comments. Made all the changes. PTAL. Won't submit until other CL's have been submitted. https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... File net/extras/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.cc:59: DCHECK(false); On 2016/06/08 21:38:10, Ryan Sleevi wrote: > DCHECK(false) is an anti-pattern > > NOTREACHED() is the idiomatic form. > > Same throughout. Done. https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... File net/extras/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:22: class NET_EXPORT_PRIVATE CertVerifierCachePersister { On 2016/06/08 21:38:10, Ryan Sleevi wrote: > This doesn't seem like it needs to be a class anymore, now that the iterator > interface is cleaned up and no friends are used > > (the protobuf class) SerializeCertVerifierCache(const CachingCertVerifier& > verifier); > > (void/bool?) DeserializeCertVerifierCache(const TheProtobufClass&, > CachingCertVerifier* verifier); Done. https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:24: CertVerifierCachePersister(CachingCertVerifier* verifier); On 2016/06/08 21:38:10, Ryan Sleevi wrote: > STYLE: Explicit Deleted the class. https://codereview.chromium.org/2021433004/diff/60001/net/extras/cert/cert_ve... net/extras/cert/cert_verifier_cache_persister.h:28: // write the hierarchical structure into |data|. On 2016/06/08 21:38:10, Ryan Sleevi wrote: > |cache_| is not a recursive structure. > > Also, you're talking about implementation details. You're not accessing > |cache_|, you're accessing it's iterator. Talk in terms of public interfaces > > // Iterates through |verifier_|'s cache and serializes the data to |data|. This > can be used to populate a new CachingCertVerifier with > // |LoadCache()| Done. https://codereview.chromium.org/2021433004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2021433004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25630: + The number of CertVerificationCache's entries that were deserialized. On 2016/06/08 21:38:10, Ryan Sleevi wrote: > GRAMMAR: This feels weird (notably, "CertVerificationCache's entries"). This > also feels like it all three might be at the wrong layer - the time spent (and > number of entries deserialized) seems not relevant as a whole, but to a specific > use (in this case, presumably, a profile's serialization/deserialization time) > > Because there is no limit on how many CertVerificationCaches there are, nor as > to how many CachingCertVerifiers there are (and of course, there are many - per > profile, URLRequestContext, etc), then the metric you're collecting here is not > as valuable. > > Moving this to be dealt with in your #5 case seems more relevant to what you're > trying to measure, since you're concerned about a very specific callsites' time > spent, right? Deleted this. Done. https://codereview.chromium.org/2021433004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25637: + The amount of time spent deserializing and populating CertVerifiers' cache. On 2016/06/08 21:38:10, Ryan Sleevi wrote: > GRAMMAR: Either caches should be plural (to reflect the plurality of > CertVerifiers), or you should make CertVerifiers' singular - and "populating a > CertVerifier's cache." > > Same with below Done.
Patchset #4 (id:80001) has been deleted
histograms lgtm - didn't look at much else https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... File net/extras/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Nit: No (c) for new files. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:8: // WARNING: This is experimental code, please don't use it. This is a strange comment. What does it mean not to use it? Does it mean it shouldn't be used from outside the network stack or at all? Is there a plan to transition away from this state? If so, suggest adding a crbug here with more details.
https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... File net/extras/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:30: EncodedCertMap* encoded_certs, DESIGN: This seems like a memory-intensive serialization pattern, because you end up keeping a copy of |encoded| in both |serialized_certs| and |encoded_certs| (and their names aren't very helpful). Why not simply remove |serialized_certs| entirely: auto result = encoded_certs->insert({ encoded, encoded_certs->size() + 1}); return result.first->second; In case it's not obvious, if |encoded| already exists in the map, then |result| will return the iterator to that as part of the first item. If it doesn't, it'll be added - with a value of encoded_certs->size() + 1. In either event, you'll get an iterator back in result.first, and then you can get the number by doing result.first->second; https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:37: EncodedCertMap::const_iterator it = encoded_certs->find(encoded); Improve documentation For example: // Determine if |cert_handle| was already serialized. If so, simply // return a reference to that entry. Otherwise, add a new entry to // the set of certs to be serialized (|serialized_certs|). https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:52: bool SerializeCertificate(const scoped_refptr<X509Certificate>& certificate, Pass as a naked pointer here since you're not retaining references, per https://www.chromium.org/developers/smart-pointer-guidelines That is, X509Certificate* https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:77: // |proto_certificate|. This comment can be substantially improved // Deserializes |proto_certificate| using the certificate database // provided in |deserialized_der_certs|. Returns the parsed certificate // on success, or nullptr if deserialization failed. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:91: return nullptr; Do error handling first if (cert_number >= deserialized_der_certs.size()) return nullptr; der_cert_pieces[i] = ... https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:97: // Update |proto_request_param| with RequestParams data from |params|. Again, documentation can be improved. // Serializes |params| into |proto_request_param|, updating |encoded_certs| // with the set of raw certificates that will be needed to deserialize // the certificate in |proto_request_param| via DeserializeCertificate(). https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:127: // certificates. See above for how this wording can be improved. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:169: } no braces on single-line conditionals https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:172: if (!proto_result.has_verified_cert() || !proto_result.has_cert_status()) { no braces on single-line conditionals https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:180: if (!result->verified_cert || !result->verified_cert.get()) { no braces on single-line conditionals https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:185: const ::std::string& public_key_hash = proto_result.public_key_hashes(i); Don't do ::std, just do std:: https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:241: base::TimeTicks::Now() - start_time); Again, I think this is the wrong layer for histograming https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:255: for (int i = 0; i < proto_cert_cache.certs_size(); ++i) Seems like you can improve the comments throughout this function. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:265: detected_corrupted_data = true; DESIGN: Why do you continue deserializing once data is corrupted? NAMING: "detected_" seems superfluous here - that is, detection is implied by a variable existing. bool data_corrupted = false; Seems similarly expressive and clear (if you decide to keep it) https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:283: if (!certificate || !certificate.get()) { !certificate.get() is redundant with !certificate - they do the same thing if (!certificate) return false; (or update data_corrupted) https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:288: std::string hostname(proto_request_params.hostname()); Why make a copy like this? Or with line 290? Why not just pass in straight from the Protobuf and avoid the extra copy? https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:299: if (!cert || !cert.get()) { See above about .get() https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:327: detected_corrupted_data = true; BUG: AddEntry failing does not mean data was corrupt. It means the Verifier might be full or have a fresher entry. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:332: base::TimeTicks::Now() - load_cache_start_time); Ditto https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... File net/extras/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:6: // CachingCertVerifier's cache. This comment is out of date https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:8: // WARNING: This is experimental code, please don't use it. On 2016/06/09 21:46:31, Alexei Svitkine (slow) wrote: > This is a strange comment. > > What does it mean not to use it? Does it mean it shouldn't be used from outside > the network stack or at all? Is there a plan to transition away from this state? > If so, suggest adding a crbug here with more details. +1 to crbug with more details. The context is this is experimental code which we're cautious about supporting long term, and it's only intended for specific (non-Chromium, but using Cronet) products to use. To that end: Raman, is there any reason this code can't live in Cronet for now, and then, if we decide to make it a part of Chrome for Android, move it down here with appropriate disclaimers? https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:26: NET_EXPORT_PRIVATE CertVerificationCache Naming wise, this seems like a non-ideal name for a public type in net. Can you namespace this to indicate it's scope? For example, sync uses sync_pb::[Type of Protobuf] to indicate when types are protobuf-specific. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:26: NET_EXPORT_PRIVATE CertVerificationCache Is NET_EXPORT_PRIVATE the right API, if you're planning on using this from Cronet? https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:29: // Populates |verifier|'s cache. Returns true if the |proto_cert_cache| is Naming: Here, naming the variable after the type is a style anti-pattern ("proto_cert_cache"). "cert_cache" seems to be a better name, with the type itself being indicative it's a protobuf.
Description was changed from ========== Cert - protobufs to serialize and deserialize CertVerifierCache. ========== to ========== Cert - protobufs to serialize and deserialize CertVerifierCache. BUG=607354 ==========
Patchset #5 (id:120001) has been deleted
PTAL. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... File net/extras/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:30: EncodedCertMap* encoded_certs, On 2016/06/09 22:38:41, Ryan Sleevi wrote: > DESIGN: This seems like a memory-intensive serialization pattern, because you > end up keeping a copy of |encoded| in both |serialized_certs| and > |encoded_certs| (and their names aren't very helpful). > > Why not simply remove |serialized_certs| entirely: > > auto result = encoded_certs->insert({ encoded, encoded_certs->size() + 1}); > return result.first->second; > > In case it's not obvious, if |encoded| already exists in the map, then |result| > will return the iterator to that as part of the first item. If it doesn't, it'll > be added - with a value of encoded_certs->size() + 1. In either event, you'll > get an iterator back in result.first, and then you can get the number by doing > result.first->second; Excellent point. Fixed it. Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:37: EncodedCertMap::const_iterator it = encoded_certs->find(encoded); On 2016/06/09 22:38:41, Ryan Sleevi wrote: > Improve documentation > > For example: > // Determine if |cert_handle| was already serialized. If so, simply > // return a reference to that entry. Otherwise, add a new entry to > // the set of certs to be serialized (|serialized_certs|). Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:52: bool SerializeCertificate(const scoped_refptr<X509Certificate>& certificate, On 2016/06/09 22:38:42, Ryan Sleevi wrote: > Pass as a naked pointer here since you're not retaining references, per > https://www.chromium.org/developers/smart-pointer-guidelines > > That is, X509Certificate* Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:77: // |proto_certificate|. On 2016/06/09 22:38:41, Ryan Sleevi wrote: > This comment can be substantially improved > > // Deserializes |proto_certificate| using the certificate database > // provided in |deserialized_der_certs|. Returns the parsed certificate > // on success, or nullptr if deserialization failed. Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:91: return nullptr; On 2016/06/09 22:38:41, Ryan Sleevi wrote: > Do error handling first > > if (cert_number >= deserialized_der_certs.size()) > return nullptr; > der_cert_pieces[i] = ... Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:97: // Update |proto_request_param| with RequestParams data from |params|. On 2016/06/09 22:38:41, Ryan Sleevi wrote: > Again, documentation can be improved. > > // Serializes |params| into |proto_request_param|, updating |encoded_certs| > // with the set of raw certificates that will be needed to deserialize > // the certificate in |proto_request_param| via DeserializeCertificate(). Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:127: // certificates. On 2016/06/09 22:38:42, Ryan Sleevi wrote: > See above for how this wording can be improved. Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:169: } On 2016/06/09 22:38:41, Ryan Sleevi wrote: > no braces on single-line conditionals Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:172: if (!proto_result.has_verified_cert() || !proto_result.has_cert_status()) { On 2016/06/09 22:38:41, Ryan Sleevi wrote: > no braces on single-line conditionals Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:180: if (!result->verified_cert || !result->verified_cert.get()) { On 2016/06/09 22:38:41, Ryan Sleevi wrote: > no braces on single-line conditionals Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:185: const ::std::string& public_key_hash = proto_result.public_key_hashes(i); On 2016/06/09 22:38:41, Ryan Sleevi wrote: > Don't do ::std, just do std:: Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:241: base::TimeTicks::Now() - start_time); On 2016/06/09 22:38:41, Ryan Sleevi wrote: > Again, I think this is the wrong layer for histograming Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:255: for (int i = 0; i < proto_cert_cache.certs_size(); ++i) On 2016/06/09 22:38:42, Ryan Sleevi wrote: > Seems like you can improve the comments throughout this function. Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:265: detected_corrupted_data = true; On 2016/06/09 22:38:41, Ryan Sleevi wrote: > DESIGN: Why do you continue deserializing once data is corrupted? If an entry is corrupted, ignore that entry and continue to the next one. Changed it to return (and not continue with deserialization) if an error is detected. > NAMING: "detected_" seems superfluous here - that is, detection is implied by a > variable existing. Deleted this variable. Changed the code to return if there is an error. > > bool data_corrupted = false; > > Seems similarly expressive and clear (if you decide to keep it) https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:283: if (!certificate || !certificate.get()) { On 2016/06/09 22:38:42, Ryan Sleevi wrote: > !certificate.get() is redundant with !certificate - they do the same thing > > if (!certificate) > return false; > > (or update data_corrupted) Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:288: std::string hostname(proto_request_params.hostname()); On 2016/06/09 22:38:41, Ryan Sleevi wrote: > Why make a copy like this? Or with line 290? Why not just pass in straight from > the Protobuf and avoid the extra copy? Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:299: if (!cert || !cert.get()) { On 2016/06/09 22:38:41, Ryan Sleevi wrote: > See above about .get() Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:327: detected_corrupted_data = true; On 2016/06/09 22:38:42, Ryan Sleevi wrote: > BUG: AddEntry failing does not mean data was corrupt. It means the Verifier > might be full or have a fresher entry. Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.cc:332: base::TimeTicks::Now() - load_cache_start_time); On 2016/06/09 22:38:41, Ryan Sleevi wrote: > Ditto Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... File net/extras/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/06/09 21:46:31, Alexei Svitkine (slow) wrote: > Nit: No (c) for new files. Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:6: // CachingCertVerifier's cache. On 2016/06/09 22:38:42, Ryan Sleevi wrote: > This comment is out of date thanks. Deleted it. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:8: // WARNING: This is experimental code, please don't use it. On 2016/06/09 22:38:42, Ryan Sleevi wrote: > On 2016/06/09 21:46:31, Alexei Svitkine (slow) wrote: > > This is a strange comment. > > > > What does it mean not to use it? Does it mean it shouldn't be used from > outside > > the network stack or at all? Is there a plan to transition away from this > state? > > If so, suggest adding a crbug here with more details. > > +1 to crbug with more details. The context is this is experimental code which > we're cautious about supporting long term, and it's only intended for specific > (non-Chromium, but using Cronet) products to use. > > To that end: Raman, is there any reason this code can't live in Cronet for now, > and then, if we decide to make it a part of Chrome for Android, move it down > here with appropriate disclaimers? Added a TODO to move it. Will do in the next CL. Updated the bug. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:8: // WARNING: This is experimental code, please don't use it. On 2016/06/09 21:46:31, Alexei Svitkine (slow) wrote: > This is a strange comment. > > What does it mean not to use it? Does it mean it shouldn't be used from outside > the network stack or at all? Is there a plan to transition away from this state? > If so, suggest adding a crbug here with more details. Added a crbug. Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:26: NET_EXPORT_PRIVATE CertVerificationCache On 2016/06/09 22:38:42, Ryan Sleevi wrote: > Naming wise, this seems like a non-ideal name for a public type in net. > > Can you namespace this to indicate it's scope? For example, sync uses > sync_pb::[Type of Protobuf] to indicate when types are protobuf-specific. Done. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:26: NET_EXPORT_PRIVATE CertVerificationCache On 2016/06/09 22:38:42, Ryan Sleevi wrote: > Is NET_EXPORT_PRIVATE the right API, if you're planning on using this from > Cronet? Will change it when we move this code to cronet. https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:29: // Populates |verifier|'s cache. Returns true if the |proto_cert_cache| is On 2016/06/09 22:38:42, Ryan Sleevi wrote: > Naming: Here, naming the variable after the type is a style anti-pattern > ("proto_cert_cache"). "cert_cache" seems to be a better name, with the type > itself being indicative it's a protobuf. Done.
Description was changed from ========== Cert - protobufs to serialize and deserialize CertVerifierCache. BUG=607354 ========== to ========== Cert - protobufs to serialize and deserialize CertVerifierCache. BUG=607354 R=rsleevi@chromium.org ==========
rtenneti@chromium.org changed reviewers: - asvitkine@chromium.org
https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... File net/extras/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/100001/net/extras/cert/cert_v... net/extras/cert/cert_verifier_cache_persister.h:26: NET_EXPORT_PRIVATE CertVerificationCache On 2016/06/10 06:13:52, ramant wrote: > Will change it when we move this code to cronet. I'll follow-up in person with you tomorrow to understand the motivations for not moving it now, before it's committed, which seems the correct step for this CL.
Patchset #6 (id:160001) has been deleted
Description was changed from ========== Cert - protobufs to serialize and deserialize CertVerifierCache. BUG=607354 R=rsleevi@chromium.org ========== to ========== Cert - protobufs to serialize and deserialize CertVerifierCache. BUG=607354 R=rsleevi@chromium.org, mef@chromium.org, ==========
rtenneti@chromium.org changed reviewers: + mef@chromium.org
https://codereview.chromium.org/2021433004/diff/180001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2021433004/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:705: "//net/data/ssl/certificates/root_ca_cert.pem", I want to reiterate: This should NOT be done. If depending on //net:test_support is not enough, then that's a bug, and it MUST be fixed before landing this.
https://codereview.chromium.org/2021433004/diff/180001/components/cronet.gypi File components/cronet.gypi (right): https://codereview.chromium.org/2021433004/diff/180001/components/cronet.gypi... components/cronet.gypi:774: # Protobuf compiler / generator for certificate verifcation protocol Hi Misha, This persistence code is not needed by ios platform. Removed ios BUILD changes. Should we make changes to ios platform also? thanks raman https://codereview.chromium.org/2021433004/diff/180001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2021433004/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:705: "//net/data/ssl/certificates/root_ca_cert.pem", On 2016/06/14 21:52:32, Ryan Sleevi wrote: > I want to reiterate: This should NOT be done. > > If depending on //net:test_support is not enough, then that's a bug, and it MUST > be fixed before landing this. jbudorick@ is working on a fix for this. Will fix asap.
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2021433004/diff/180001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2021433004/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:705: "//net/data/ssl/certificates/root_ca_cert.pem", On 2016/06/14 22:38:19, ramant wrote: > On 2016/06/14 21:52:32, Ryan Sleevi wrote: > > I want to reiterate: This should NOT be done. > > > > If depending on //net:test_support is not enough, then that's a bug, and it > MUST > > be fixed before landing this. > > jbudorick@ is working on a fix for this. Will fix asap. Depending on //net:test_support was not sufficient. Fix is here: https://codereview.chromium.org/2065313002
jbudorick@chromium.org changed reviewers: - jbudorick@chromium.org
Hi Ryan Sleevi, Will wait for your changes and jbudorick@ changes to land. PTAL. https://codereview.chromium.org/2021433004/diff/180001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2021433004/diff/180001/components/cronet/andr... components/cronet/android/BUILD.gn:705: "//net/data/ssl/certificates/root_ca_cert.pem", On 2016/06/15 13:53:19, jbudorick wrote: > On 2016/06/14 22:38:19, ramant wrote: > > On 2016/06/14 21:52:32, Ryan Sleevi wrote: > > > I want to reiterate: This should NOT be done. > > > > > > If depending on //net:test_support is not enough, then that's a bug, and it > > MUST > > > be fixed before landing this. > > > > jbudorick@ is working on a fix for this. Will fix asap. > > Depending on //net:test_support was not sufficient. Fix is here: > https://codereview.chromium.org/2065313002 Done.
https://codereview.chromium.org/2021433004/diff/220001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2021433004/diff/220001/components/cronet/andr... components/cronet/android/BUILD.gn:109: proto_library("cronet_cert_proto") { Can you restrict the visibility to the template? Is that something GN allows? The context is making sure we don't end up with J-Random-User depending on this target, since it's really an implementation detail (AFAICT) https://codereview.chromium.org/2021433004/diff/220001/components/cronet/andr... components/cronet/android/BUILD.gn:134: "//third_party/protobuf:protobuf_lite", Doesn't the proto_library take on that dep, rather than the target itself? https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:242: return cert_cache; Note: You're mutating |cert_cache| throughout your serialization, even if it fails. That means you potentially end up serializing junk data in a way that can seriously mess with your deserialization. If serialization fails, shouldn't you fail-closed and serialize nothing? https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:321: base::Time::FromInternalValue(cache_entry.verification_time())); BUG?: Not checking if this deserializes properly into a valid time https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister_unittest.cc (right): https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:30: int VerifyCert(scoped_refptr<net::X509Certificate> certificate, Either pass as raw pointer (X509Certificate*) or std::move on line 38. You shouldn't pass by-val unless you're taking ownership, same as unique_ptr https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:52: EXPECT_TRUE(cert.get()); This will crash the test if it's false, so you're not gaining anything from this. You can only ASSERT_TRUE() if your return is void or a ::testing::AssertionResult https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:52: EXPECT_TRUE(cert.get()); STYLE: Don't use .get() for boolean comparisons. Just EXPECT_TRUE(cert); (but really, ASSERT_TRUE(cert); https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:55: EXPECT_TRUE(net::IsCertificateError(error)); Ditto about this doing nothing https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:60: DCHECK_EQ(1, cert_cache.cache_entry_size()); Don't DCHECK in tests. That crashes the test. My overall recommendation is don't make a utility function like this, for the reasons demonstrated above. Do the needful in the tests themselves, or restructure this so you can use ASSERT_NO_FATAL_FAILURE with the subroutines. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:73: net::CachingCertVerifier verifier_; This doesn't really need to be a test harness. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:93: // Restore the cache data for an existing entry shouldn't fail. GRAMMAR: Restoring? GRAMMAR: Avoid double negative (should not fail -> should succeed)? https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:94: EXPECT_TRUE(DeserializeCertVerifierCache(cert_cache, &verifier_)); This is unclear to me why this test exists. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:100: ASSERT_NE(static_cast<net::X509Certificate*>(NULL), ok_cert.get()); STYLE: Don't do this. ASSERT_TRUE(ok_cert); https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:120: // Create a new Verifier and restoring the data into it should succeed. Perhaps this comment belongs on line 96 instead https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:131: ASSERT_NE(net::ERR_IO_PENDING, error); You're calling .GetResult (line 125), so it's impossible for this assertion to ever fail. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:137: TEST_F(CertVerifierCachePersisterTest, RestoreMultipleEntriesIntoNewVerifier) { Why isn't this combined into the previous test? https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:217: TEST_F(CertVerifierCachePersisterTest, DeserializeCorruptedCerts) { But what's the expected state of |verifier2| here? Some certs but not all? Or no certs. As it stands, you call AddEntry on each iteration, so if deserialization fails partially, you'll deserialize bad data into |verifier2|. It's a test that highlights an undefined part of your API implementation, and reveals your DeserializeCertVerifierCache has side-effects, which is probably undesirable. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:370: DeserializeCorruptedRequestParamsCertNumbers) { Do lines 217-370 need to be structured as they are? Couldn't you use an iterative approach with in-place protobuf structures that you deserialize? That is, something like const SomeTestData data[] = { {a, a, a}, {b, b, b}, {c, c, c}, {d, d, d} }; for (const auto& test_data : data) { SCOPED_TEST(...); ASSERT_FALSE(Deserialize....); } That is, the above assumes you can do struct-initialization of the protobufs, and I don't know if you can, but if you can, it sure beats the hell out of the serialize-mangle-deserialize. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:547: cronet_pb::CertVerificationCertificate* certificate = All of these corrupt tests seem to share the common pattern I mentioned above.
Patchset #9 (id:240001) has been deleted
Hi Ryan, This CL is still waiting for jbudorick's CL to land. Made all the changes including the changes we had talked off line. PTAL. thanks raman https://codereview.chromium.org/2021433004/diff/220001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2021433004/diff/220001/components/cronet/andr... components/cronet/android/BUILD.gn:109: proto_library("cronet_cert_proto") { On 2016/06/15 23:31:31, Ryan Sleevi wrote: > Can you restrict the visibility to the template? Is that something GN allows? > > The context is making sure we don't end up with J-Random-User depending on this > target, since it's really an implementation detail (AFAICT) Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/andr... components/cronet/android/BUILD.gn:134: "//third_party/protobuf:protobuf_lite", On 2016/06/15 23:31:31, Ryan Sleevi wrote: > Doesn't the proto_library take on that dep, rather than the target itself? Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:242: return cert_cache; On 2016/06/15 23:31:32, Ryan Sleevi wrote: > Note: You're mutating |cert_cache| throughout your serialization, even if it > fails. That means you potentially end up serializing junk data in a way that can > seriously mess with your deserialization. > > If serialization fails, shouldn't you fail-closed and serialize nothing? Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:321: base::Time::FromInternalValue(cache_entry.verification_time())); At line 270 was doing !cache_entry.has_verification_time(). Added check for is_null. Added a check for verification_time not being in future. Any other checks? Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister_unittest.cc (right): https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:30: int VerifyCert(scoped_refptr<net::X509Certificate> certificate, On 2016/06/15 23:31:32, Ryan Sleevi wrote: > Either pass as raw pointer (X509Certificate*) or std::move on line 38. You > shouldn't pass by-val unless you're taking ownership, same as unique_ptr Thanks. Missed it. Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:52: EXPECT_TRUE(cert.get()); On 2016/06/15 23:31:33, Ryan Sleevi wrote: > STYLE: Don't use .get() for boolean comparisons. > > Just > EXPECT_TRUE(cert); > > (but really, > ASSERT_TRUE(cert); Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:52: EXPECT_TRUE(cert.get()); On 2016/06/15 23:31:33, Ryan Sleevi wrote: > This will crash the test if it's false, so you're not gaining anything from > this. > > You can only ASSERT_TRUE() if your return is void or a > ::testing::AssertionResult Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:55: EXPECT_TRUE(net::IsCertificateError(error)); On 2016/06/15 23:31:33, Ryan Sleevi wrote: > Ditto about this doing nothing Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:60: DCHECK_EQ(1, cert_cache.cache_entry_size()); On 2016/06/15 23:31:32, Ryan Sleevi wrote: > Don't DCHECK in tests. That crashes the test. > > My overall recommendation is don't make a utility function like this, for the > reasons demonstrated above. Do the needful in the tests themselves, or > restructure this so you can use ASSERT_NO_FATAL_FAILURE with the subroutines. Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:73: net::CachingCertVerifier verifier_; On 2016/06/15 23:31:32, Ryan Sleevi wrote: > This doesn't really need to be a test harness. Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:93: // Restore the cache data for an existing entry shouldn't fail. On 2016/06/15 23:31:33, Ryan Sleevi wrote: > GRAMMAR: Restoring? > GRAMMAR: Avoid double negative (should not fail -> should succeed)? Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:94: EXPECT_TRUE(DeserializeCertVerifierCache(cert_cache, &verifier_)); On 2016/06/15 23:31:32, Ryan Sleevi wrote: > This is unclear to me why this test exists. My intention was to test if cert verifier cache already had an entry, restoring an entry from deserialization shouldn't cause issues. Deleted it. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:100: ASSERT_NE(static_cast<net::X509Certificate*>(NULL), ok_cert.get()); On 2016/06/15 23:31:32, Ryan Sleevi wrote: > STYLE: Don't do this. > > ASSERT_TRUE(ok_cert); Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:120: // Create a new Verifier and restoring the data into it should succeed. On 2016/06/15 23:31:32, Ryan Sleevi wrote: > Perhaps this comment belongs on line 96 instead Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:131: ASSERT_NE(net::ERR_IO_PENDING, error); On 2016/06/15 23:31:33, Ryan Sleevi wrote: > You're calling .GetResult (line 125), so it's impossible for this assertion to > ever fail. Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:137: TEST_F(CertVerifierCachePersisterTest, RestoreMultipleEntriesIntoNewVerifier) { On 2016/06/15 23:31:32, Ryan Sleevi wrote: > Why isn't this combined into the previous test? Done. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:217: TEST_F(CertVerifierCachePersisterTest, DeserializeCorruptedCerts) { On 2016/06/15 23:31:32, Ryan Sleevi wrote: > But what's the expected state of |verifier2| here? Some certs but not all? Or no > certs. > > As it stands, you call AddEntry on each iteration, so if deserialization fails > partially, you'll deserialize bad data into |verifier2|. > > It's a test that highlights an undefined part of your API implementation, and > reveals your DeserializeCertVerifierCache has side-effects, which is probably > undesirable. Changed the code to populate CachingCertVerifier only if all the data looks good. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:370: DeserializeCorruptedRequestParamsCertNumbers) { On 2016/06/15 23:31:33, Ryan Sleevi wrote: > Do lines 217-370 need to be structured as they are? Couldn't you use an > iterative approach with in-place protobuf structures that you deserialize? > > That is, something like > const SomeTestData data[] = { {a, a, a}, > {b, b, b}, > {c, c, c}, > {d, d, d} }; > for (const auto& test_data : data) { > SCOPED_TEST(...); > ASSERT_FALSE(Deserialize....); > } > > That is, the above assumes you can do struct-initialization of the protobufs, > and I don't know if you can, but if you can, it sure beats the hell out of the > serialize-mangle-deserialize. I agree. All the protobuf related tests, I had seen were doing set... methods. https://codereview.chromium.org/2021433004/diff/220001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:547: cronet_pb::CertVerificationCertificate* certificate = On 2016/06/15 23:31:32, Ryan Sleevi wrote: > All of these corrupt tests seem to share the common pattern I mentioned above. Acknowledged.
Patchset #9 (id:260001) has been deleted
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021433004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/20 18:19:13, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. jbudorick@ changes have landed. Verified that this CL builds ok on trybots. Can you please take another look? thanks raman
https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:11: #include "base/metrics/histogram_macros.h" Unused? https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:17: #include "net/cert/cert_type.h" Why are you including this? Seems unused, but is also very Linux/ChromeOS specific https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:19: #include "net/cert/x509_cert_types.h" Unused https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:30: bool SerializeCertHandle(const net::X509Certificate::OSCertHandle& cert_handle, Document https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.h:8: #include "base/macros.h" Unused? https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.h:9: #include "components/cronet/cert/proto/cert_verification.pb.h" Forward declare, to avoid build system dependency issues. (Namely, if another target depends on your target, whether or not this .pb.h is generated in time is... complicated. Avoid the issue entirely :D) https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister_unittest.cc (right): https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:94: net::CertVerifyResult verifyier1_result1; spelling: verifier1_result https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:103: // Verify www.example2.com host's certificate. example.com is a special (reserved) name, but example2.com is not If you want different hostnames, use www.example.com and www2.example.com (that is, make the subdomain vary) https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:150: // Corrupted |cert_entry| in the serialized data, should fail when deserialized. Grammar: This is not a properly constructed sentence. Alternatives could be: // A corrupted cert_entry in the serialized data should fail to be deserialized. // Should not deserialize a corrupted cert_entry. (Note, no | | because cert_entry isn't a local variable) https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:277: request_params->set_hostname(""); How is this different than NoHostname?
Hi Ryan, Thanks very for the comments. PTAL. Will delete the test if it is overkill. thanks raman https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:11: #include "base/metrics/histogram_macros.h" On 2016/06/21 01:22:10, Ryan Sleevi wrote: > Unused? Done. https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:17: #include "net/cert/cert_type.h" On 2016/06/21 01:22:10, Ryan Sleevi wrote: > Why are you including this? Seems unused, but is also very Linux/ChromeOS > specific Done. https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:19: #include "net/cert/x509_cert_types.h" On 2016/06/21 01:22:10, Ryan Sleevi wrote: > Unused Done. https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:30: bool SerializeCertHandle(const net::X509Certificate::OSCertHandle& cert_handle, On 2016/06/21 01:22:10, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.h:8: #include "base/macros.h" On 2016/06/21 01:22:10, Ryan Sleevi wrote: > Unused? Done. https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.h:9: #include "components/cronet/cert/proto/cert_verification.pb.h" On 2016/06/21 01:22:10, Ryan Sleevi wrote: > Forward declare, to avoid build system dependency issues. (Namely, if another > target depends on your target, whether or not this .pb.h is generated in time > is... complicated. Avoid the issue entirely :D) Done. https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister_unittest.cc (right): https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:94: net::CertVerifyResult verifyier1_result1; On 2016/06/21 01:22:10, Ryan Sleevi wrote: > spelling: verifier1_result Done. https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:103: // Verify www.example2.com host's certificate. On 2016/06/21 01:22:10, Ryan Sleevi wrote: > http://example.com is a special (reserved) name, but http://example2.com is not > > If you want different hostnames, use http://www.example.com and http://www2.example.com (that > is, make the subdomain vary) Done. https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:150: // Corrupted |cert_entry| in the serialized data, should fail when deserialized. On 2016/06/21 01:22:10, Ryan Sleevi wrote: > Grammar: This is not a properly constructed sentence. > > Alternatives could be: > // A corrupted cert_entry in the serialized data should fail to be deserialized. > // Should not deserialize a corrupted cert_entry. > > (Note, no | | because cert_entry isn't a local variable) Done. https://codereview.chromium.org/2021433004/diff/300001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:277: request_params->set_hostname(""); On 2016/06/21 01:22:10, Ryan Sleevi wrote: > How is this different than NoHostname? One checks request_params.has_hostname() and the other checks request_params.hostname().empty(). bool CertVerificationRequestParams::has_hostname() const { return (_has_bits_[0] & 0x00000002u) != 0; } Is this overkill? Can delete the test.
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021433004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:36: NOTREACHED(); After re-reading this, all of these NOTREACHED()'s should go away - there's nothing about your API or developer contract that ensures these won't be reached - they absolutely can be. NOTREACHED() is meant to indicate this code is unreachable - but you're layering on top of other APIs, and their contract is that this code is reachable (that is, these methods can fail) https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:60: for (size_t i = 0; i < intermediate_ca_certs.size(); ++i) { You no longer use i, so you could just use for (const auto& intermediate : intermediate_ca_certs) { if (!SerializeCertHandle(intermediate, serialized_certs, &cert_number)) { } } https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:109: for (const scoped_refptr<net::X509Certificate>& cert : can use "const auto&" here https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:253: for (auto cert : serialized_certs_) { const auto& cert https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:342: // Deserialize |request_params|'s certificate using the certificate s/ / / https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:378: if (verification_time.is_null() || verification_time >= base::Time::Now()) It's unclear why you're making this check (which is not to say it's wrong, but it's just not clear why you don't allow future entries - you should document) https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.h:6: #define COMPONENTS_CRONET_CERT_CERT_VERIFIER_CACHE_PERSISTER_H_ s/Persister/Serializer/, to reflect your naming of the methods? You're not actually doing any persistence here in these methods. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister_unittest.cc (right): https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:48: ASSERT_TRUE(net::IsCertificateError(error)); BUG/SURPRISE: What part of the contract of VerifyCert guarantees it'll be an error? You're passed in a CachingCertVerifier - there's no way you can know whether |cert_name| should be valid or invalid. Documenting this method could be clearer, but it's not clear to me that this is the right check you should be doing. Put differently: Why does it matter what the value of |error| is for the functioning of this test? If it matters, you should document it as a condition to VerifyCert. If it doesn't matter, you should remove this assert. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:101: ASSERT_TRUE(net::IsCertificateError(error)); Same comments apply throughout for the checks here https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:131: // www.example.com. I highlighted before, but these comments (130-131) do not match what you're doing in the test. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:134: EXPECT_EQ(verifier2_result1.cert_status, verifier1_result1.cert_status); Document why you're checking here. For example, why don't you make sure that they have the same error result? Why don't you test that the overall CertVerifyResults are equal? Why only test the CertStatus? (This is where having an operator== for CertVerifyResult may help) https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:141: nullptr, &verifier2_result2, callback.callback(), &request, Why don't you use the VerifyCert helper here? What's different or the same? https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:151: // Should not deserialize a corrupted cert_entry. You updated this description, but failed to update all of the others like I requested. I only highlighted the first instance. see HERE1 below https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:169: // deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:188: // deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:211: // fail when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:236: // when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:261: // when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:286: // when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:311: // fail when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:336: // fail when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:364: // fail when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:392: // |request_params| in the serialized data, should fail when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:422: EXPECT_TRUE(net::IsCertificateError(error)); Why don't you use the VerifyCert() helper like you do in other tests? https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:447: // deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:494: // when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:519: // deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:545: // deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:571: // deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:601: // should fail when deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:630: // deserialized. HERE1 https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:657: // deserialized. HERE1
Patchset #12 (id:340001) has been deleted
Patchset #12 (id:360001) has been deleted
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021433004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Hi Ryan, Made all the changes. Updated the comments. Added operator== method for cert_verify_result and used that to verify the result. Unfortunately, git mv uploaded a new file. PTAL. thanks for your comments, raman https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.cc (right): https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:36: NOTREACHED(); On 2016/06/21 20:31:02, Ryan Sleevi wrote: > After re-reading this, all of these NOTREACHED()'s should go away - there's > nothing about your API or developer contract that ensures these won't be reached > - they absolutely can be. NOTREACHED() is meant to indicate this code is > unreachable - but you're layering on top of other APIs, and their contract is > that this code is reachable (that is, these methods can fail) Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:60: for (size_t i = 0; i < intermediate_ca_certs.size(); ++i) { On 2016/06/21 20:31:02, Ryan Sleevi wrote: > You no longer use i, so you could just use > > for (const auto& intermediate : intermediate_ca_certs) { > if (!SerializeCertHandle(intermediate, serialized_certs, &cert_number)) { > } > } Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:109: for (const scoped_refptr<net::X509Certificate>& cert : On 2016/06/21 20:31:02, Ryan Sleevi wrote: > can use "const auto&" here Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:253: for (auto cert : serialized_certs_) { On 2016/06/21 20:31:02, Ryan Sleevi wrote: > const auto& cert Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:342: // Deserialize |request_params|'s certificate using the certificate On 2016/06/21 20:31:02, Ryan Sleevi wrote: > s/ / / Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.cc:378: if (verification_time.is_null() || verification_time >= base::Time::Now()) On 2016/06/21 20:31:02, Ryan Sleevi wrote: > It's unclear why you're making this check (which is not to say it's wrong, but > it's just not clear why you don't allow future entries - you should document) What do you think of my comment? Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister.h (right): https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister.h:6: #define COMPONENTS_CRONET_CERT_CERT_VERIFIER_CACHE_PERSISTER_H_ On 2016/06/21 20:31:02, Ryan Sleevi wrote: > s/Persister/Serializer/, to reflect your naming of the methods? You're not > actually doing any persistence here in these methods. Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_persister_unittest.cc (right): https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:48: ASSERT_TRUE(net::IsCertificateError(error)); On 2016/06/21 20:31:03, Ryan Sleevi wrote: > BUG/SURPRISE: What part of the contract of VerifyCert guarantees it'll be an > error? You're passed in a CachingCertVerifier - there's no way you can know > whether |cert_name| should be valid or invalid. > > Documenting this method could be clearer, but it's not clear to me that this is > the right check you should be doing. > > Put differently: Why does it matter what the value of |error| is for the > functioning of this test? If it matters, you should document it as a condition > to VerifyCert. If it doesn't matter, you should remove this assert. I am not an expert in this area. Was copying other cert verification tests. Deleted the check for IsCertificateError. Added comment that cert verifcation will succeed. wdyt? Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:101: ASSERT_TRUE(net::IsCertificateError(error)); On 2016/06/21 20:31:02, Ryan Sleevi wrote: > Same comments apply throughout for the checks here Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:131: // www.example.com. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > I highlighted before, but these comments (130-131) do not match what you're > doing in the test. Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:134: EXPECT_EQ(verifier2_result1.cert_status, verifier1_result1.cert_status); On 2016/06/21 20:31:03, Ryan Sleevi wrote: > Document why you're checking here. > > For example, why don't you make sure that they have the same error result? Why > don't you test that the overall CertVerifyResults are equal? Why only test the > CertStatus? > > (This is where having an operator== for CertVerifyResult may help) Added operator== to CertVerifyResult and used it here. Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:141: nullptr, &verifier2_result2, callback.callback(), &request, On 2016/06/21 20:31:03, Ryan Sleevi wrote: > Why don't you use the VerifyCert helper here? What's different or the same? www2.example.com has certificate chain. VerifyCert verifies ok_cert certificate (a different cert chain). Added a comment. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:151: // Should not deserialize a corrupted cert_entry. On 2016/06/21 20:31:04, Ryan Sleevi wrote: > You updated this description, but failed to update all of the others like I > requested. I only highlighted the first instance. > > see HERE1 below Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:169: // deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:188: // deserialized. On 2016/06/21 20:31:02, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:211: // fail when deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:236: // when deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:261: // when deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:286: // when deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:311: // fail when deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:336: // fail when deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:364: // fail when deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:392: // |request_params| in the serialized data, should fail when deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:422: EXPECT_TRUE(net::IsCertificateError(error)); On 2016/06/21 20:31:03, Ryan Sleevi wrote: > Why don't you use the VerifyCert() helper like you do in other tests? In this test, we are verifying with trust_anchors. Added a comment. wdyt? https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:447: // deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:494: // when deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:519: // deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:545: // deserialized. On 2016/06/21 20:31:02, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:571: // deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:601: // should fail when deserialized. On 2016/06/21 20:31:04, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:630: // deserialized. On 2016/06/21 20:31:03, Ryan Sleevi wrote: > HERE1 Done. https://codereview.chromium.org/2021433004/diff/320001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_persister_unittest.cc:657: // deserialized. On 2016/06/21 20:31:02, Ryan Sleevi wrote: > HERE1 Done.
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Patchset #12 (id:380001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021433004/390001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Patchset #12 (id:390001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021433004/410001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021433004/430001
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Patchset #13 (id:430001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021433004/450001
Patchset #12 (id:410001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % nit https://codereview.chromium.org/2021433004/diff/450001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_serializer.cc (right): https://codereview.chromium.org/2021433004/diff/450001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_serializer.cc:24: typedef std::map<std::string, size_t> SerializedCertMap; Maybe some documentation here (about what the size_t is) https://codereview.chromium.org/2021433004/diff/450001/net/cert/cert_verify_r... File net/cert/cert_verify_result.cc (right): https://codereview.chromium.org/2021433004/diff/450001/net/cert/cert_verify_r... net/cert/cert_verify_result.cc:44: common_name_fallback_used == other.common_name_fallback_used; Could also do: return verified_cert->Equals(other.verified_cert.get()) && std::tie(cert_status, has_md2, has_md4, has_md5, has_sha1, public_key_hashes, is_issued_by_known_root, is_issued_by_additional_trust_anchor, common_name_fallback_used) == std::tie(other.cert_status, other.has_md2, other.has_md4, other.has_md5, other.has_sha1, other.public_key_hashes, other.is_issued_by_known_root, other.is_issued_by_additional_trust_anchor, other.common_name_fallback_used) Avoids a lot of the repetitive &&/==
Thanks very much Ryan S. for the code review. Hi Misha, Can you look at components/cronet changes? thanks raman https://codereview.chromium.org/2021433004/diff/450001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_serializer.cc (right): https://codereview.chromium.org/2021433004/diff/450001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_serializer.cc:24: typedef std::map<std::string, size_t> SerializedCertMap; On 2016/06/22 22:32:20, Ryan Sleevi wrote: > Maybe some documentation here (about what the size_t is) Done. https://codereview.chromium.org/2021433004/diff/450001/net/cert/cert_verify_r... File net/cert/cert_verify_result.cc (right): https://codereview.chromium.org/2021433004/diff/450001/net/cert/cert_verify_r... net/cert/cert_verify_result.cc:44: common_name_fallback_used == other.common_name_fallback_used; On 2016/06/22 22:32:20, Ryan Sleevi wrote: > Could also do: > > return verified_cert->Equals(other.verified_cert.get()) && > std::tie(cert_status, has_md2, has_md4, has_md5, has_sha1, public_key_hashes, > is_issued_by_known_root, is_issued_by_additional_trust_anchor, > common_name_fallback_used) == > std::tie(other.cert_status, other.has_md2, other.has_md4, other.has_md5, > other.has_sha1, other.public_key_hashes, other.is_issued_by_known_root, > other.is_issued_by_additional_trust_anchor, other.common_name_fallback_used) > > > Avoids a lot of the repetitive &&/== Done.
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2021433004/470001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm (I trust sleevi)
lgtm with a couple of nits. This could be useful for iOS as well as Android, right? https://codereview.chromium.org/2021433004/diff/470001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2021433004/diff/470001/components/cronet/andr... components/cronet/android/BUILD.gn:132: ":cronet_cert_proto", How much does it increase the binary size? https://codereview.chromium.org/2021433004/diff/470001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_serializer.cc (right): https://codereview.chromium.org/2021433004/diff/470001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_serializer.cc:76: for (int i = 0; i < certificate.cert_numbers_size(); i++) { nit: pre-increment ++i https://codereview.chromium.org/2021433004/diff/470001/components/cronet/cert... File components/cronet/cert/proto/cert_verification.proto (right): https://codereview.chromium.org/2021433004/diff/470001/components/cronet/cert... components/cronet/cert/proto/cert_verification.proto:57: // and it's validy period. validity period -> time of verification?
On 2016/06/24 19:16:20, mef wrote: > lgtm with a couple of nits. > > This could be useful for iOS as well as Android, right? No. We MUST NOT ship this on iOS. If there is a reason encouraging it, it's likely the wrong reason, and we should discuss more. Remember: The only reason we're doing this is because Android forces certificate verifications in-process, and the in-process initialization cost of the pinning blacklist from the system takes several hundred ms. On iOS, these concerns don't exist. On iOS, certificate verification is handled by the system - which handles it all in a single daemon process (securityd) without any of those initialization overheads.
On 2016/06/24 19:19:20, Ryan Sleevi wrote: > On 2016/06/24 19:16:20, mef wrote: > > lgtm with a couple of nits. > > > > This could be useful for iOS as well as Android, right? > > No. We MUST NOT ship this on iOS. If there is a reason encouraging it, it's > likely the wrong reason, and we should discuss more. > > Remember: The only reason we're doing this is because Android forces certificate > verifications in-process, and the in-process initialization cost of the pinning > blacklist from the system takes several hundred ms. > > On iOS, these concerns don't exist. On iOS, certificate verification is handled > by the system - which handles it all in a single daemon process (securityd) > without any of those initialization overheads. Ah, thanks for clarification! In this case I think we should move it from components/cronet into components/cronet/android, WDYT?
On 2016/06/24 19:22:16, mef wrote: > Ah, thanks for clarification! In this case I think we should move it from > components/cronet into components/cronet/android, WDYT? The narrower the scope, the merrier :)
The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Moved cronet/cert to cronet/android/cert. https://codereview.chromium.org/2021433004/diff/470001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2021433004/diff/470001/components/cronet/andr... components/cronet/android/BUILD.gn:132: ":cronet_cert_proto", On 2016/06/24 19:16:20, mef wrote: > How much does it increase the binary size? In Debug build, cronet.so size has increased from 5.123MB to 5.143MB - a 20k increase. Protobuf code adds around 3k of .cc code and 2k of .h code. https://codereview.chromium.org/2021433004/diff/470001/components/cronet/cert... File components/cronet/cert/cert_verifier_cache_serializer.cc (right): https://codereview.chromium.org/2021433004/diff/470001/components/cronet/cert... components/cronet/cert/cert_verifier_cache_serializer.cc:76: for (int i = 0; i < certificate.cert_numbers_size(); i++) { On 2016/06/24 19:16:20, mef wrote: > nit: pre-increment ++i Done. https://codereview.chromium.org/2021433004/diff/470001/components/cronet/cert... File components/cronet/cert/proto/cert_verification.proto (right): https://codereview.chromium.org/2021433004/diff/470001/components/cronet/cert... components/cronet/cert/proto/cert_verification.proto:57: // and it's validy period. On 2016/06/24 19:16:20, mef wrote: > validity period -> time of verification? Done.
The CQ bit was unchecked by rtenneti@chromium.org
The CQ bit was checked by rtenneti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rsleevi@chromium.org, mef@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/2021433004/#ps490001 (title: "Fix comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Cert - protobufs to serialize and deserialize CertVerifierCache. BUG=607354 R=rsleevi@chromium.org, mef@chromium.org, ========== to ========== Cert - protobufs to serialize and deserialize CertVerifierCache. BUG=607354 R=rsleevi@chromium.org, mef@chromium.org, ==========
Message was sent while issue was closed.
Committed patchset #14 (id:490001)
Message was sent while issue was closed.
Description was changed from ========== Cert - protobufs to serialize and deserialize CertVerifierCache. BUG=607354 R=rsleevi@chromium.org, mef@chromium.org, ========== to ========== Cert - protobufs to serialize and deserialize CertVerifierCache. BUG=607354 R=rsleevi@chromium.org, mef@chromium.org, Committed: https://crrev.com/18f5a46af8dcde2ae88873d03d2908949678af75 Cr-Commit-Position: refs/heads/master@{#402020} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/18f5a46af8dcde2ae88873d03d2908949678af75 Cr-Commit-Position: refs/heads/master@{#402020} |
