Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
547603002:
Certificate Transparency: Code for unpacking EV cert hashes whitelist (Closed)
|
Created:
6 years, 3 months ago by Eran Messeri Modified:
6 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, Dmitry Lomov (no reviews), waffles, Ben Laurie (Chromium) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionCertificate Transparency: Code for unpacking EV cert hashes whitelist
Re-submission of https://codereview.chromium.org/462543002/ (note that the Fingerprint256 changes have been broken off to a separate change).
Note for the build cop: Please attempt to contact me if there's a need to roll back.
BUG=339128
Committed: https://crrev.com/efbd3137115a35c938a0cb8fa54d7c4b33403afb
Cr-Commit-Position: refs/heads/master@{#301642}
Patch Set 1 #Patch Set 2 : Windows compilation fixes. #Patch Set 3 : Adding call to GetLastError #Patch Set 4 : Not emitting histogram entry if fingerprinting failed. #Patch Set 5 : Adding TODO for Windows Vista issues #Patch Set 6 : Correctly fingerprint cert on Vista, XP #
Total comments: 18
Patch Set 7 : Created a class for the EV whitelist, enabled in the component updater #Patch Set 8 : Unit-test fixes. #Patch Set 9 : Merging with head (resolving histograms.xml conflicts) #Patch Set 10 : Minor documentation fix. #Patch Set 11 : Adding missing EV initialization to all tests in url_request #
Total comments: 26
Patch Set 12 : Avoiding globals in favour of passing the SSLConfigService around #
Total comments: 22
Patch Set 13 : Switching to static methods on SSLConfigService #Patch Set 14 : Catching up with base/files change on master #
Total comments: 36
Patch Set 15 : Addressing review comments #Patch Set 16 : Attempting to address ambiguity in c'tor selection #
Total comments: 25
Patch Set 17 : Addressing review comments. #
Total comments: 4
Patch Set 18 : quic verifier, storing struct in the vector #Patch Set 19 : Using uint64_t rather than an array in the whitelist vector #
Total comments: 6
Patch Set 20 : Addressing final comments #Patch Set 21 : Removing unnecessary const #Messages
Total messages: 42 (6 generated)
The CQ bit was checked by eranm@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@chromium.org/547603002/40001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
eranm@chromium.org changed reviewers: + mpearson@chromium.org, rsleevi@chromium.org
Please review this re-submission of https://codereview.chromium.org/462543002/. Ryan - Wan-Teh reviewed the previous issue and approved patchset 8, but as he's away, could you please review the changes? I've simply ignored the test for Windows Vista. The code that uses the failing function (CalculateFingerprint256) is a no-op if the function fails.
On 2014/09/08 13:00:23, Eran wrote: > Please review this re-submission of https://codereview.chromium.org/462543002/. > Ryan - Wan-Teh reviewed the previous issue and approved patchset 8, but as he's > away, could you please review the changes? > > I've simply ignored the test for Windows Vista. The code that uses the failing > function (CalculateFingerprint256) is a no-op if the function fails. Update: The code now correctly fingerprints the certificate on all Windows platforms Chrome is supporting (by using a different Cryptographic Service Provider on Vista and falling back to NSS's hashing implementation on Windows XP, since I could not find a CSP for Windows XP that supports SHA-256).
histograms.xml lgtm
https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelist.h File net/cert/ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:24: // whitelist in a blocking thread. First glance: This comment needs serious rewording, but I'm not sure what to suggest. What it doesn't tell me is much about this class or its use. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:26: // This class is declared here so it can be tested. Delete this comment (and then line 25) https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:29: BitStreamReader(const char* source, size_t length); Design choice: Why not base::StringPiece as our preferred structure of bytes & length. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:33: bool ReadUnaryEncoding(uint64* out); 1) #include <stdint.h> 2) uint64_t as the type 3) newline before line 34. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:38: bool ReadBits(uint8 num_bits, uint64* out); 1) uint8_t, uint64_t 2) newline before line 39 https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:52: // Index of the last bit read within |current_byte_|. Since bits are read newline between 51/52 https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:63: std::set<std::string>* uncompressed_list); From an API design, it seems a very design-limiting choice to use global functions like this, and to force the data type as an std::set<std::string>* It seems like you're better off exposing the EV whitelist as a class, with a series of helper methods to determine whether or not a given certificate is whitelisted. On top of that base (pure-virtual) interface, you have two concrete implementations: - One which can read data from a file on disk - The Golumb coding, the internal structure of the data, etc is entirely opaque in this interface (e.g. it's part of the .cc). - From an API consumer standpoint, you just check whether or not a certificate is in the whitelist. - One which can be used for testing https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:77: NET_EXPORT void SetEVWhitelistFromFile( This being a global in the //net layer is also a reason I'd not LGTM. Using the abstraction above avoids this. That is, put differently, there is an EXTREMELY high bar to introduce any new globals in //net, especially when those globals may have complex life-time/threading (e.g. in the case of a component update that happens after the fact) https://codereview.chromium.org/547603002/diff/100001/net/cert/x509_certifica... File net/cert/x509_certificate_win.cc (right): https://codereview.chromium.org/547603002/diff/100001/net/cert/x509_certifica... net/cert/x509_certificate_win.cc:326: if (!CryptAcquireContext(&csp_provider, 1) Not LGTM. There's zero reason to go through a CSP for this, and significant overhead. 2) The code is bugged for XP SP3 3) Don't remark about the library in use (NSS or BoringSSL is opaque to you here) TL;DR: return crytpo::SHA256HashString(der_cert);
eranm@chromium.org changed reviewers: + sorin@chromium.org
Sorin - Added you as reviewer for the component_updater change (which essentially enables using the fetched data). Ryan - as discussed offline, there's now a class for the EVCertsWhitelist data and is kept in the same way the CRLSet is. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelist.h File net/cert/ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:24: // whitelist in a blocking thread. On 2014/09/08 19:48:00, Ryan Sleevi wrote: > First glance: This comment needs serious rewording, but I'm not sure what to > suggest. What it doesn't tell me is much about this class or its use. Tried re-wording it a bit, although I think that the name is representative enough of what the class does so only little documentation is necessary. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:26: // This class is declared here so it can be tested. On 2014/09/08 19:48:00, Ryan Sleevi wrote: > Delete this comment (and then line 25) Done. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:29: BitStreamReader(const char* source, size_t length); On 2014/09/08 19:48:00, Ryan Sleevi wrote: > Design choice: Why not base::StringPiece as our preferred structure of bytes & > length. Done. Note that the implication is copying the data to a member field. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:33: bool ReadUnaryEncoding(uint64* out); On 2014/09/08 19:48:00, Ryan Sleevi wrote: > 1) #include <stdint.h> > 2) uint64_t as the type > 3) newline before line 34. Done. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:38: bool ReadBits(uint8 num_bits, uint64* out); On 2014/09/08 19:48:00, Ryan Sleevi wrote: > 1) uint8_t, uint64_t > 2) newline before line 39 Done. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:52: // Index of the last bit read within |current_byte_|. Since bits are read On 2014/09/08 19:48:00, Ryan Sleevi wrote: > newline between 51/52 Done. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:63: std::set<std::string>* uncompressed_list); On 2014/09/08 19:48:00, Ryan Sleevi wrote: > From an API design, it seems a very design-limiting choice to use global > functions like this, and to force the data type as an std::set<std::string>* > > It seems like you're better off exposing the EV whitelist as a class, with a > series of helper methods to determine whether or not a given certificate is > whitelisted. > > On top of that base (pure-virtual) interface, you have two concrete > implementations: > - One which can read data from a file on disk > - The Golumb coding, the internal structure of the data, etc is entirely > opaque in this interface (e.g. it's part of the .cc). > - From an API consumer standpoint, you just check whether or not a certificate > is in the whitelist. > - One which can be used for testing Done - created a class for the EV whitelist data. https://codereview.chromium.org/547603002/diff/100001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:77: NET_EXPORT void SetEVWhitelistFromFile( On 2014/09/08 19:48:00, Ryan Sleevi wrote: > This being a global in the //net layer is also a reason I'd not LGTM. Using the > abstraction above avoids this. > > That is, put differently, there is an EXTREMELY high bar to introduce any new > globals in //net, especially when those globals may have complex > life-time/threading (e.g. in the case of a component update that happens after > the fact) As we discussed offline, I've switched to the model used for CRLSets - there's still a global, but it's kept in the SSLConfigService class. I have added a global for the EV whitelist to that class, PTAL. https://codereview.chromium.org/547603002/diff/100001/net/cert/x509_certifica... File net/cert/x509_certificate_win.cc (right): https://codereview.chromium.org/547603002/diff/100001/net/cert/x509_certifica... net/cert/x509_certificate_win.cc:326: if (!CryptAcquireContext(&csp_provider, On 2014/09/08 19:48:00, Ryan Sleevi wrote: > 1) Not LGTM. > > There's zero reason to go through a CSP for this, and significant overhead. > > 2) The code is bugged for XP SP3 > 3) Don't remark about the library in use (NSS or BoringSSL is opaque to you > here) > > TL;DR: > > return crytpo::SHA256HashString(der_cert); Done, I'm happy to get rid of this CryptAcquireContext shenanigans. Is there *any* benefit to using CryptHashCertificate over crypto::SHA256HashString? Not that I want to use it, I'm just curious why the SHA-1 fingerprinting is done that way.
Apologies for continuing to push back on design, and for the high latency in my reviews doing so. I realize some of the //net design assumptions have not been well documented, and you're getting tripped up by a "Do as I say, not as I do" thing. One thing worth pointing out here is that the EV whitelist only makes sense/has meaning in a user-visible URLRequestContext, of which there is only one (the main URLRequestContext). Arguably, the EVCertsWhitelist could be a data member on the SSLConfigService, and you could set up that SSLConfigService via the SSLConfigServiceManager in //chrome/browser/net, which could glue up the SSLConfigService to the component updater responsible for fetching the EVCertWhitelist. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... File net/cert/ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.cc:11: #include "base/lazy_instance.h" Unused? https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.cc:13: #include "net/ssl/ssl_config_service.h" Layering violation to depend on net/ssl from net/cert (//net/cert should only pickup //net/base, //crypto, and //base) https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.cc:180: std::set<std::string> whitelist_; This is a lot of overhead for a large number of entries. Are there data structures we can use to optimize this? For example, could the uncompressed whitelist be a sorted continuous buffer that you then used bsearch over? https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.cc:187: if (!base::ReadFileToString(compressed_whitelist_file, &compressed_list)) { So, this method is bad because it does disk IO on whatever thread it's called in, and then calls the SSLConfigService method (which should only ever be on the IO thread) So, the way I read it, you're blocking the IO thread. That's Very Very Bad. Structurally, like I mentioned in the header, I think the "Parse a file and build a concrete whitelist" is something that lives outside of //net in your higher layer. There, you use the BrowserThread aspects, as needed, to deserialize the file (on a worker thread), parse it (worker thread?), then send the parsed result back to the IO thread to update this global. All of that lives outside of //net. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelist.h File net/cert/ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:30: class NET_EXPORT_PRIVATE BitStreamReader { Why do you have this in a public header in an ::internal namespace? It seems unrelated. Move it to it's own header, and I don't think it belongs in _internal. _EXPORT_PRIVATE is a sufficient signal that this is an internal implementation detail, while still being in a .h for testing. If you're actually discretely unit testing this (which you should) https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:66: : public base::RefCountedThreadSafe<EVCertsWhitelist> { There's a really, really high-barrier for introducing new ref-counted classes. Can you explain why you made this RefCounted? It's a big design constraint to do so. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:82: virtual bool Update(const std::string& compressed_whitelist) = 0; I don't think this method really belongs/makes sense. 1) It doesn't really fit the virtual interface that I see EVCertsWhitelist having, which is meant for 'accessing' the whitelist. 2) Because this is pure virtual, every concrete class is going to have to implement uncompressing compressed_whitelist. That doesn't sound like the right thing for an interface? That is, updating the list (and lines 89/90, uncompressing) are more details a concrete implementation. I would instead focus your interface simply on accessing (lines 70-71? Is 73-75 needed?), with a concrete implementation that handles the details of compression/uncompression. Put differently, the interface you want to thread from //chrome into //net is simply "here's how to access the list", with "here's how to update the list" living in //chrome (or //components, w/e), since that's the only layer that should ever be updating the list. Did that make sense? https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:98: UncompressesWhitelistCorrectly); Generally, pick one or the other here. If you friend the fixture (line 92), then you should make protected methods that mess with this class, and remove lines 93-98. Otherwise, you explicitly list tests like you do. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:110: const base::FilePath& compressed_whitelist_file); If you did keep this method (still uncomfortable with it, it's the wrong layer it feels), forward declare base::FilePath. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:114: NET_EXPORT EVCertsWhitelist* GetEmptyEVCertsWhitelist(); If it's for testing, why isn't it NET_EXPORT_PRIVATE? Note that if you take my comments above and separate this into a simple 'testing' interface, then you'd simply create a dummy fake EVCertsWhitelist in your test .cc, since you'd only have one (two?) methods to implement ContainsCertificateHash (always returns false), and MAYBE IsValid() (returns true?) https://codereview.chromium.org/547603002/diff/200001/net/cert/x509_certifica... File net/cert/x509_certificate_win.cc (right): https://codereview.chromium.org/547603002/diff/200001/net/cert/x509_certifica... net/cert/x509_certificate_win.cc:329: // to get SHA-256 capabilities, which introduces significant overhead. So, I should have been clearer on the comment. The reason why CryptHashCertificate is used above is that it's just a shim for CertGetCertificateContextProperty(), but in a foward-compatible way. That is, Windows maintains a SHA-1 hash for all certificates, always, internally, but they reserve the right to change that (but largely, wont) I'd simply update this comment // Use crypto::SHA256HashString for two reasons: // * < Windows Vista does not have universal SHA-256 support // * It's more efficient for > Windows Vista (less overhead) https://codereview.chromium.org/547603002/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/547603002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:3443: } This is the wrong layer to be logging this. This sort of logic likely belongs somewhere much higher - perhaps the ResourceLoader - or much lower - within the CertVerifier.
lgtm component updater lgtm. Thank you! https://codereview.chromium.org/547603002/diff/200001/chrome/browser/componen... File chrome/browser/component_updater/ev_whitelist_component_installer.cc (right): https://codereview.chromium.org/547603002/diff/200001/chrome/browser/componen... chrome/browser/component_updater/ev_whitelist_component_installer.cc:72: FROM_HERE, I think the arguments will fit on line 71. Or consider not using a named local variable, since we have to vertically align parameters anyway.
Ryan, please take another look - the code now avoids global variables. There are still two open issues: * Use of set vs bsearching an array. * The right place to do the check for presence of cert in the whitelist. Sorin, you may want to take another look at the component updater changes, as I've passed in a parameter to be updated. https://codereview.chromium.org/547603002/diff/200001/chrome/browser/componen... File chrome/browser/component_updater/ev_whitelist_component_installer.cc (right): https://codereview.chromium.org/547603002/diff/200001/chrome/browser/componen... chrome/browser/component_updater/ev_whitelist_component_installer.cc:72: FROM_HERE, On 2014/09/12 19:46:54, Sorin Jianu wrote: > I think the arguments will fit on line 71. Or consider not using a named local > variable, since we have to vertically align parameters anyway. Done. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... File net/cert/ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.cc:11: #include "base/lazy_instance.h" On 2014/09/11 23:44:38, Ryan Sleevi (expect_delays) wrote: > Unused? Done. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.cc:13: #include "net/ssl/ssl_config_service.h" On 2014/09/11 23:44:38, Ryan Sleevi (expect_delays) wrote: > Layering violation to depend on net/ssl from net/cert > > (//net/cert should only pickup //net/base, //crypto, and //base) Done. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.cc:180: std::set<std::string> whitelist_; On 2014/09/11 23:44:38, Ryan Sleevi wrote: > This is a lot of overhead for a large number of entries. > > Are there data structures we can use to optimize this? For example, could the > uncompressed whitelist be a sorted continuous buffer that you then used bsearch > over? There'll be ~600K entries in this set. I will benchmark memory overhead vs. added runtime of bsearching over an array so we'll have data to justify one over the other. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.cc:187: if (!base::ReadFileToString(compressed_whitelist_file, &compressed_list)) { On 2014/09/11 23:44:38, Ryan Sleevi (expect_delays) wrote: > So, this method is bad because it does disk IO on whatever thread it's called > in, and then calls the SSLConfigService method (which should only ever be on the > IO thread) > > So, the way I read it, you're blocking the IO thread. That's Very Very Bad. > > Structurally, like I mentioned in the header, I think the "Parse a file and > build a concrete whitelist" is something that lives outside of //net in your > higher layer. There, you use the BrowserThread aspects, as needed, to > deserialize the file (on a worker thread), parse it (worker thread?), then send > the parsed result back to the IO thread to update this global. All of that lives > outside of //net. Done: This method is called on a blocking pool task (see chrome/browser/component_updater/ev_whitelist_component_installer.cc), so IIUC it is not blocking anything. I've added another task dispatching that does the actual setting on the SSLConfigService on the IO thread. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelist.h File net/cert/ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:30: class NET_EXPORT_PRIVATE BitStreamReader { On 2014/09/11 23:44:38, Ryan Sleevi wrote: > Why do you have this in a public header in an ::internal namespace? It seems > unrelated. > > Move it to it's own header, and I don't think it belongs in _internal. > _EXPORT_PRIVATE is a sufficient signal that this is an internal implementation > detail, while still being in a .h for testing. If you're actually discretely > unit testing this (which you should) Short answer is that it's in a public header in an internal namespace for unit testing it (no net_export moniker anymore since it's not under net anymore). Should it live in a separate header? not in a namespace? I'll happily move it to a more suitable location. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:66: : public base::RefCountedThreadSafe<EVCertsWhitelist> { On 2014/09/11 23:44:38, Ryan Sleevi wrote: > There's a really, really high-barrier for introducing new ref-counted classes. > Can you explain why you made this RefCounted? It's a big design constraint to do > so. (also mentioned offline) It's ref-counted because, IIUC, that will allow safe updating of the whitelist. I.e., the whitelist instance could be used by the SSLClientSocketNSS, while a new whitelist is set on the IO thread. Are you suggesting that by setting the list only on the IO thread we could avoid reference counting? https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:82: virtual bool Update(const std::string& compressed_whitelist) = 0; On 2014/09/11 23:44:38, Ryan Sleevi wrote: > I don't think this method really belongs/makes sense. > > 1) It doesn't really fit the virtual interface that I see EVCertsWhitelist > having, which is meant for 'accessing' the whitelist. > 2) Because this is pure virtual, every concrete class is going to have to > implement uncompressing compressed_whitelist. That doesn't sound like the right > thing for an interface? > > That is, updating the list (and lines 89/90, uncompressing) are more details a > concrete implementation. > > I would instead focus your interface simply on accessing (lines 70-71? Is 73-75 > needed?), with a concrete implementation that handles the details of > compression/uncompression. > > Put differently, the interface you want to thread from //chrome into //net is > simply "here's how to access the list", with "here's how to update the list" > living in //chrome (or //components, w/e), since that's the only layer that > should ever be updating the list. Did that make sense? Yes - as you recommended, moved the implementation to //chrome/browser/net, this interface has no update method anymore. A new instance is constructed each time the list is updated (and IsValid is still there so the list is not replaced if it's not valid). https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:98: UncompressesWhitelistCorrectly); On 2014/09/11 23:44:38, Ryan Sleevi wrote: > Generally, pick one or the other here. If you friend the fixture (line 92), then > you should make protected methods that mess with this class, and remove lines > 93-98. Otherwise, you explicitly list tests like you do. Done. Picked FRIEND_TEST_ALL_PREFIXES because the only protected method I want to test is UncompressEVWhitelist. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:110: const base::FilePath& compressed_whitelist_file); On 2014/09/11 23:44:38, Ryan Sleevi wrote: > If you did keep this method (still uncomfortable with it, it's the wrong layer > it feels), forward declare base::FilePath. That now lives in //chrome/browser/net, forward-declared base::FilePath. https://codereview.chromium.org/547603002/diff/200001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:114: NET_EXPORT EVCertsWhitelist* GetEmptyEVCertsWhitelist(); On 2014/09/11 23:44:39, Ryan Sleevi wrote: > If it's for testing, why isn't it NET_EXPORT_PRIVATE? > > Note that if you take my comments above and separate this into a simple > 'testing' interface, then you'd simply create a dummy fake EVCertsWhitelist in > your test .cc, since you'd only have one (two?) methods to implement > > ContainsCertificateHash (always returns false), and MAYBE IsValid() (returns > true?) Removed this method, per your suggestion there's a DefaultEVCertsWhitelist subclass that behaves as you described and (as the name suggests) is the default for SSLConfig instances. https://codereview.chromium.org/547603002/diff/200001/net/cert/x509_certifica... File net/cert/x509_certificate_win.cc (right): https://codereview.chromium.org/547603002/diff/200001/net/cert/x509_certifica... net/cert/x509_certificate_win.cc:329: // to get SHA-256 capabilities, which introduces significant overhead. On 2014/09/11 23:44:39, Ryan Sleevi wrote: > So, I should have been clearer on the comment. The reason why > CryptHashCertificate is used above is that it's just a shim for > CertGetCertificateContextProperty(), but in a foward-compatible way. > > That is, Windows maintains a SHA-1 hash for all certificates, always, > internally, but they reserve the right to change that (but largely, wont) > > I'd simply update this comment > // Use crypto::SHA256HashString for two reasons: > // * < Windows Vista does not have universal SHA-256 support > // * It's more efficient for > Windows Vista (less overhead) Done, thanks for the explanation. https://codereview.chromium.org/547603002/diff/200001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/547603002/diff/200001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:3443: } On 2014/09/11 23:44:39, Ryan Sleevi wrote: > This is the wrong layer to be logging this. > > This sort of logic likely belongs somewhere much higher - perhaps the > ResourceLoader - or much lower - within the CertVerifier. Re putting this logic in the CertVerifier, that makes sense, but means adding another parameter to the CertVerifier::Verify method, and there's already a TODO there with your name on it to remove the CRLSet. Where did you want to remove the CRLSet to? Perhaps I can supply the CRLSet and the EVCertsWhitelist in the same manner. Re ResourceLoader, could you clarify exactly where? Also, the ultimate goal is to take away the EV state if the cert is not in the whitelist, don't we want this reflected in the cert_status field?
Thank you Eran! https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:393: void RegisterComponentsForUpdate(net::SSLConfigService* ssl_config_service) { Eran, I see this additional parameter is dependent on the the profile. Will this have some side effects as a result of the user changing profiles? https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1469: RegisterComponentsForUpdate(profile_->GetSSLConfigService()); I don't know how user switching and profiles work, I am just raising a concern due to additional dependency injected here. https://codereview.chromium.org/547603002/diff/220001/chrome/browser/componen... File chrome/browser/component_updater/ev_whitelist_component_installer.h (right): https://codereview.chromium.org/547603002/diff/220001/chrome/browser/componen... chrome/browser/component_updater/ev_whitelist_component_installer.h:28: EVWhitelistComponentInstallerTraits( needs an explicit declarator.
rsleevi@chromium.org changed reviewers: + haavardm@opera.com
Adding haavard from Opera so that he can comment more on Opera's interest (or lack) in this, so we can get layering right. With respect to layering, as mentioned in email, I think you're on the right track, and we should just closer ape the CRLSet design. I still need to think more about the interaction w/ respect to the CertVerifier, CTVerifier, and the SSLClientSocket interactions. See https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config... fr the CRLSEt bits https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:431: RegisterEVWhitelistComponent(cus, ssl_config_service); CRLSets, which have a similar purpose as this, don't require passing in the SSLCS. See 415-426 https://codereview.chromium.org/547603002/diff/220001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/220001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:33: class BitStreamReader { Split into separate files? https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelist.h File net/cert/ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:39: NET_EXPORT EVCertsWhitelist* GetDefaultEVCertsWhitelist(); No need for this function w/ the integration into SSLConfigService https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... File net/cert/ct_ev_whitelist_unittest.cc (right): https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist_unittest.cc:19: EXPECT_TRUE(whitelist->ContainsCertificateHash("")); "" -> std::string https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... File net/cert/x509_certificate_win.cc (right): https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... net/cert/x509_certificate_win.cc:322: DCHECK_NE(static_cast<DWORD>(0), cert->cbCertEncoded); just use 0u ? https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... net/cert/x509_certificate_win.cc:325: DWORD sha256_size = sizeof(sha256.data); No longer the correct type now, is it? https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:3427: if (server_cert_verify_result_.cert_status & CERT_STATUS_IS_EV) { BUG: Need this logic for OpenSSL. Easy to forget to change both places. https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:3428: if (ssl_config_.ev_certs_whitelist->IsValid()) { Should ev_certs_whitelist just be optional, the same as it is with CRLSets?
On 2014/10/01 20:15:43, Ryan Sleevi wrote: > Adding haavard from Opera so that he can comment more on Opera's interest (or > lack) in this, so we can get layering right. Missed that you added me here. We are interested, but don't think I'm able to give any meaningful comments about this until Monday. > > With respect to layering, as mentioned in email, I think you're on the right > track, and we should just closer ape the CRLSet design. > > I still need to think more about the interaction w/ respect to the CertVerifier, > CTVerifier, and the SSLClientSocket interactions. > > See > https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config... > fr the CRLSEt bits > > https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:431: RegisterEVWhitelistComponent(cus, > ssl_config_service); > CRLSets, which have a similar purpose as this, don't require passing in the > SSLCS. See 415-426 > > https://codereview.chromium.org/547603002/diff/220001/chrome/browser/net/pack... > File chrome/browser/net/packed_ct_ev_whitelist.h (right): > > https://codereview.chromium.org/547603002/diff/220001/chrome/browser/net/pack... > chrome/browser/net/packed_ct_ev_whitelist.h:33: class BitStreamReader { > Split into separate files? > > https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelist.h > File net/cert/ct_ev_whitelist.h (right): > > https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... > net/cert/ct_ev_whitelist.h:39: NET_EXPORT EVCertsWhitelist* > GetDefaultEVCertsWhitelist(); > No need for this function w/ the integration into SSLConfigService > > https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... > File net/cert/ct_ev_whitelist_unittest.cc (right): > > https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... > net/cert/ct_ev_whitelist_unittest.cc:19: > EXPECT_TRUE(whitelist->ContainsCertificateHash("")); > "" -> std::string > > https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... > File net/cert/x509_certificate_win.cc (right): > > https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... > net/cert/x509_certificate_win.cc:322: DCHECK_NE(static_cast<DWORD>(0), > cert->cbCertEncoded); > just use 0u ? > > https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... > net/cert/x509_certificate_win.cc:325: DWORD sha256_size = sizeof(sha256.data); > No longer the correct type now, is it? > > https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... > File net/socket/ssl_client_socket_nss.cc (right): > > https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_nss.cc:3427: if > (server_cert_verify_result_.cert_status & CERT_STATUS_IS_EV) { > BUG: Need this logic for OpenSSL. Easy to forget to change both places. > > https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_nss.cc:3428: if > (ssl_config_.ev_certs_whitelist->IsValid()) { > Should ev_certs_whitelist just be optional, the same as it is with CRLSets?
On 2014/10/01 20:15:43, Ryan Sleevi wrote: > Adding haavard from Opera so that he can comment more on Opera's interest (or > lack) in this, so we can get layering right. Missed that you added me here. We are interested, but don't think I'm able to give any meaningful comments about this until Monday. > > With respect to layering, as mentioned in email, I think you're on the right > track, and we should just closer ape the CRLSet design. > > I still need to think more about the interaction w/ respect to the CertVerifier, > CTVerifier, and the SSLClientSocket interactions. > > See > https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config... > fr the CRLSEt bits > > https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:431: RegisterEVWhitelistComponent(cus, > ssl_config_service); > CRLSets, which have a similar purpose as this, don't require passing in the > SSLCS. See 415-426 > > https://codereview.chromium.org/547603002/diff/220001/chrome/browser/net/pack... > File chrome/browser/net/packed_ct_ev_whitelist.h (right): > > https://codereview.chromium.org/547603002/diff/220001/chrome/browser/net/pack... > chrome/browser/net/packed_ct_ev_whitelist.h:33: class BitStreamReader { > Split into separate files? > > https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelist.h > File net/cert/ct_ev_whitelist.h (right): > > https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... > net/cert/ct_ev_whitelist.h:39: NET_EXPORT EVCertsWhitelist* > GetDefaultEVCertsWhitelist(); > No need for this function w/ the integration into SSLConfigService > > https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... > File net/cert/ct_ev_whitelist_unittest.cc (right): > > https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... > net/cert/ct_ev_whitelist_unittest.cc:19: > EXPECT_TRUE(whitelist->ContainsCertificateHash("")); > "" -> std::string > > https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... > File net/cert/x509_certificate_win.cc (right): > > https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... > net/cert/x509_certificate_win.cc:322: DCHECK_NE(static_cast<DWORD>(0), > cert->cbCertEncoded); > just use 0u ? > > https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... > net/cert/x509_certificate_win.cc:325: DWORD sha256_size = sizeof(sha256.data); > No longer the correct type now, is it? > > https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... > File net/socket/ssl_client_socket_nss.cc (right): > > https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_nss.cc:3427: if > (server_cert_verify_result_.cert_status & CERT_STATUS_IS_EV) { > BUG: Need this logic for OpenSSL. Easy to forget to change both places. > > https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_nss.cc:3428: if > (ssl_config_.ev_certs_whitelist->IsValid()) { > Should ev_certs_whitelist just be optional, the same as it is with CRLSets?
Quick update regarding the use of set vs bsearch: I've benchmarked and using bsearch to search in a sorted array of 120K entries (each 8 bytes long) is twice as fast as finding in a set. I don't see how using std::set under these circumstances makes sense, so I'll migrate the code to use bsearch and upload a new patchset. Also, from an offline discussion with Ryan on the issue of using the EVCertsWhitelist in the CertVerifier, the conclusion was that for now we'll check the presence in the whitelist in SSLClientSocket* (NSS, OpenSSL) and change once we figure out a way to pass it into the CertVerifier.
Ryan, Sorin, PTAL. As Ryan suggested I'm now storing the EV certs whitelist in a similar manner to the CRLSets, so no passing around of the SSLConfigService is necessary. https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:393: void RegisterComponentsForUpdate(net::SSLConfigService* ssl_config_service) { On 2014/10/01 18:50:02, Sorin Jianu wrote: > Eran, I see this additional parameter is dependent on the the profile. Will this > have some side effects as a result of the user changing profiles? Removed this, as I've reverted back to the approach of setting a global state on the SSLConfigService, similar to CRLSets. To answer your question, though, I *think* that it would not have side effects, as there are some objects that are being copied from one profile to another because they seem to be global configuration options (see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...) https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:431: RegisterEVWhitelistComponent(cus, ssl_config_service); On 2014/10/01 20:15:42, Ryan Sleevi wrote: > CRLSets, which have a similar purpose as this, don't require passing in the > SSLCS. See 415-426 Done. https://codereview.chromium.org/547603002/diff/220001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1469: RegisterComponentsForUpdate(profile_->GetSSLConfigService()); On 2014/10/01 18:50:02, Sorin Jianu wrote: > I don't know how user switching and profiles work, I am just raising a concern > due to additional dependency injected here. As mentioned, removed. https://codereview.chromium.org/547603002/diff/220001/chrome/browser/componen... File chrome/browser/component_updater/ev_whitelist_component_installer.h (right): https://codereview.chromium.org/547603002/diff/220001/chrome/browser/componen... chrome/browser/component_updater/ev_whitelist_component_installer.h:28: EVWhitelistComponentInstallerTraits( On 2014/10/01 18:50:02, Sorin Jianu wrote: > needs an explicit declarator. Not anymore since I've reverted to the default c'tor. https://codereview.chromium.org/547603002/diff/220001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/220001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:33: class BitStreamReader { On 2014/10/01 20:15:42, Ryan Sleevi wrote: > Split into separate files? Done. https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelist.h File net/cert/ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:39: NET_EXPORT EVCertsWhitelist* GetDefaultEVCertsWhitelist(); On 2014/10/01 20:15:42, Ryan Sleevi wrote: > No need for this function w/ the integration into SSLConfigService Done, removed. https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... File net/cert/ct_ev_whitelist_unittest.cc (right): https://codereview.chromium.org/547603002/diff/220001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist_unittest.cc:19: EXPECT_TRUE(whitelist->ContainsCertificateHash("")); On 2014/10/01 20:15:42, Ryan Sleevi wrote: > "" -> std::string Removed the whole test since the default instance is not necessary. https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... File net/cert/x509_certificate_win.cc (right): https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... net/cert/x509_certificate_win.cc:322: DCHECK_NE(static_cast<DWORD>(0), cert->cbCertEncoded); On 2014/10/01 20:15:43, Ryan Sleevi wrote: > just use 0u ? Done. https://codereview.chromium.org/547603002/diff/220001/net/cert/x509_certifica... net/cert/x509_certificate_win.cc:325: DWORD sha256_size = sizeof(sha256.data); On 2014/10/01 20:15:43, Ryan Sleevi wrote: > No longer the correct type now, is it? Done. https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:3427: if (server_cert_verify_result_.cert_status & CERT_STATUS_IS_EV) { On 2014/10/01 20:15:43, Ryan Sleevi wrote: > BUG: Need this logic for OpenSSL. Easy to forget to change both places. For now I copied the logic as-is to ssl_client_socket_openssl, once there's a clear plan on how to unify policy-related checks I'll move the code. https://codereview.chromium.org/547603002/diff/220001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:3428: if (ssl_config_.ev_certs_whitelist->IsValid()) { On 2014/10/01 20:15:43, Ryan Sleevi wrote: > Should ev_certs_whitelist just be optional, the same as it is with CRLSets? Done. Note that the IsValid method on the EVCertsWhitelist remained because the EVCertsWhitelist instances are immutable, so in order to figure out if we should replace the existing EVCertsWhitelist instance with a newly-created one (from fresh data) the IsValid method is used.
On 2014/10/01 20:15:43, Ryan Sleevi wrote: > Adding haavard from Opera so that he can comment more on Opera's interest (or > lack) in this, so we can get layering right. A few high level comments from me (A little late probably, but throwing it in anyway hoping I don't cause too much noise) Since Opera doesn't compile the chrome/ directory: Is it necessary to have the EV whitelist decompression algorithm in chrome/browser/? Since zip or other standard compression algorithms won't be able to compress this (as the data seems random to those algorithms), it would be nice if users of the content api also had access to this. You probably already have thought about these next comments and have good reasons not for doing it, bit I'll ask anyway: * Instead of adding this as a separate list adding all this extra code to Chromium, would be possible to set up a "special" chromium ct log (or Opera specific for that sake) which contains these certs, and chromium checks this CT log as default only for EV certs not having SCT extension? * Would it possible to use the cert dates (probably "not after" as you cannot trust the not before dates), and introduce EV certs in the same fashion you handle the deprecation of SHA-1, instead of adding this list? It would not be as clear cut, but would eventually lead to CT log requirement for EV certs. > With respect to layering, as mentioned in email, I think you're on the right > track, and we should just closer ape the CRLSet design. Perhaps some of the ideas here also could be used on CRLSet? I'm thinking especially using Golomb coding, since the CRLset also basically is a set of hashes. > > I still need to think more about the interaction w/ respect to the CertVerifier, > CTVerifier, and the SSLClientSocket interactions. >
On 2014/10/06 11:42:59, haavardm wrote: > On 2014/10/01 20:15:43, Ryan Sleevi wrote: > > Adding haavard from Opera so that he can comment more on Opera's interest (or > > lack) in this, so we can get layering right. > > A few high level comments from me (A little late probably, but throwing it in > anyway hoping I don't cause too much noise) > > Since Opera doesn't compile the chrome/ directory: Is it necessary to have the > EV whitelist decompression algorithm in chrome/browser/? Since zip or other > standard compression algorithms won't be able to compress this (as the data > seems random to those algorithms), it would be nice if users of the content api > also had access to this. I'll happily move it around. What do you think about moving it to content/ once you have concrete plans to implement the CT requirement for EV certs, in a separate patch? > > You probably already have thought about these next comments and have good > reasons not for doing it, bit I'll ask anyway: > > * Instead of adding this as a separate list adding all this extra code to > Chromium, would be possible to set up a "special" chromium ct log (or Opera > specific for that sake) which contains these certs, and chromium checks this CT > log as default only for EV certs not having SCT extension? Technically yes, there are two issues though: (1) The code to check inclusion in logs is not in yet :) (2) The benefit to a centrally-distributed whitelist is that each client does not have to wait for a log's response to complete the SSL connection establishment. You could argue that the inclusion check should be done in the background, but for EV certs that would mean updating the EV status some time after the site has already loaded (potentially even after they user left the site). It's also technically complex to update the EV state after the certificate status bits have been passed onto the UI (from what I've seen). > > * Would it possible to use the cert dates (probably "not after" as you cannot > trust the not before dates), and introduce EV certs in the same fashion you > handle the deprecation of SHA-1, instead of adding this list? It would not be as > clear cut, but would eventually lead to CT log requirement for EV certs. I believe the answer is yes - and that is probably the approach we would have taken if it wasn't for the fact we could whitelist all EV certs we know of relatively cheaply (the compressed whitelist is ~600K). In terms of cert issuance volumes, the CAs accounting for over 90% (95%?) have stated they will implement CT and we have seen implementations of various CAs coming online, so I don't think there's a concern here that a major part of EV certificates will break when we require CT for EV. > > > > With respect to layering, as mentioned in email, I think you're on the right > > track, and we should just closer ape the CRLSet design. > > Perhaps some of the ideas here also could be used on CRLSet? I'm thinking > especially using Golomb coding, since the CRLset also basically is a set of > hashes. The Golomb code is agl@'s idea :) I think that the CRLSet will not benefit from Golomb coding because it deals with many more certificates than this whitelist (~120K EV certificates will ultimately be included in the list). > > > > > I still need to think more about the interaction w/ respect to the > CertVerifier, > > CTVerifier, and the SSLClientSocket interactions. > >
On 2014/10/06 12:32:04, Eran wrote: > On 2014/10/06 11:42:59, haavardm wrote: > > On 2014/10/01 20:15:43, Ryan Sleevi wrote: > > > Adding haavard from Opera so that he can comment more on Opera's interest > (or > > > lack) in this, so we can get layering right. > > > > A few high level comments from me (A little late probably, but throwing it in > > anyway hoping I don't cause too much noise) > > > > Since Opera doesn't compile the chrome/ directory: Is it necessary to have the > > EV whitelist decompression algorithm in chrome/browser/? Since zip or other > > standard compression algorithms won't be able to compress this (as the data > > seems random to those algorithms), it would be nice if users of the content > api > > also had access to this. > I'll happily move it around. What do you think about moving it to content/ once > you have concrete plans to implement the CT requirement for EV certs, in a > separate patch? The only risk is if the code gets so entangled with src/chrome code that it's hard to split out later. However I don't see that happening here, so it should work for us if that is what fit you the best. > > > > You probably already have thought about these next comments and have good > > reasons not for doing it, bit I'll ask anyway: > > > > * Instead of adding this as a separate list adding all this extra code to > > Chromium, would be possible to set up a "special" chromium ct log (or Opera > > specific for that sake) which contains these certs, and chromium checks this > CT > > log as default only for EV certs not having SCT extension? > Technically yes, there are two issues though: > (1) The code to check inclusion in logs is not in yet :) > (2) The benefit to a centrally-distributed whitelist is that each client does > not have to wait for a log's response to complete the SSL connection > establishment. You could argue that the inclusion check should be done in the > background, but for EV certs that would mean updating the EV status some time > after the site has already loaded (potentially even after they user left the > site). It's also technically complex to update the EV state after the > certificate status bits have been passed onto the UI (from what I've seen). > > > > * Would it possible to use the cert dates (probably "not after" as you cannot > > trust the not before dates), and introduce EV certs in the same fashion you > > handle the deprecation of SHA-1, instead of adding this list? It would not be > as > > clear cut, but would eventually lead to CT log requirement for EV certs. > I believe the answer is yes - and that is probably the approach we would have > taken if it wasn't for the fact we could whitelist all EV certs we know of > relatively cheaply (the compressed whitelist is ~600K). > In terms of cert issuance volumes, the CAs accounting for over 90% (95%?) have > stated they will implement CT and we have seen implementations of various CAs > coming online, so I don't think there's a concern here that a major part of EV > certificates will break when we require CT for EV. > > > > > > > With respect to layering, as mentioned in email, I think you're on the right > > > track, and we should just closer ape the CRLSet design. > > > > Perhaps some of the ideas here also could be used on CRLSet? I'm thinking > > especially using Golomb coding, since the CRLset also basically is a set of > > hashes. > The Golomb code is agl@'s idea :) I think that the CRLSet will not benefit from > Golomb coding because it deals with many more certificates than this whitelist > (~120K EV certificates will ultimately be included in the list). Well, thinking about it, CRLset contains more data than just hash data anyway. > > > > > > I still need to think more about the interaction w/ respect to the > > CertVerifier, > > > CTVerifier, and the SSLClientSocket interactions. > > >
On Oct 6, 2014 5:32 AM, <eranm@chromium.org> wrote: > > On 2014/10/06 11:42:59, haavardm wrote: >> >> On 2014/10/01 20:15:43, Ryan Sleevi wrote: >> > Adding haavard from Opera so that he can comment more on Opera's interest > > (or >> >> > lack) in this, so we can get layering right. > > >> A few high level comments from me (A little late probably, but throwing it in >> anyway hoping I don't cause too much noise) > > >> Since Opera doesn't compile the chrome/ directory: Is it necessary to have the >> EV whitelist decompression algorithm in chrome/browser/? Since zip or other >> standard compression algorithms won't be able to compress this (as the data >> seems random to those algorithms), it would be nice if users of the content > > api >> >> also had access to this. > > I'll happily move it around. What do you think about moving it to content/ once > you have concrete plans to implement the CT requirement for EV certs, in a > separate patch? > > >> You probably already have thought about these next comments and have good >> reasons not for doing it, bit I'll ask anyway: > > >> * Instead of adding this as a separate list adding all this extra code to >> Chromium, would be possible to set up a "special" chromium ct log (or Opera >> specific for that sake) which contains these certs, and chromium checks this > > CT >> >> log as default only for EV certs not having SCT extension? > > Technically yes, there are two issues though: > (1) The code to check inclusion in logs is not in yet :) > (2) The benefit to a centrally-distributed whitelist is that each client does > not have to wait for a log's response to complete the SSL connection > establishment. You could argue that the inclusion check should be done in the > background, but for EV certs that would mean updating the EV status some time > after the site has already loaded (potentially even after they user left the > site). It's also technically complex to update the EV state after the > certificate status bits have been passed onto the UI (from what I've seen). > To be clear, its a non-starter to add any latency to TLS establishment. Any online checks would do that, as well as add the privacy concerns of disclosing the sites you visit to the log, a critical failure of OCSP. > >> * Would it possible to use the cert dates (probably "not after" as you cannot >> trust the not before dates), and introduce EV certs in the same fashion you >> handle the deprecation of SHA-1, instead of adding this list? It would not be > > as >> >> clear cut, but would eventually lead to CT log requirement for EV certs. > > I believe the answer is yes - and that is probably the approach we would have > taken if it wasn't for the fact we could whitelist all EV certs we know of > relatively cheaply (the compressed whitelist is ~600K). > In terms of cert issuance volumes, the CAs accounting for over 90% (95%?) have > stated they will implement CT and we have seen implementations of various CAs > coming online, so I don't think there's a concern here that a major part of EV > certificates will break when we require CT for EV. > > The answer is actually no. The cert dates aren't trust worthy - they are controlled by the CA. Whether compelled or compromised, a CA could backdate certs and get another three years of MITM compromise. The SHA-1 deprecation doesn't use the notBefore, but the notAfter, and with a hard date for rollover. So it is different. > >> > With respect to layering, as mentioned in email, I think you're on the right >> > track, and we should just closer ape the CRLSet design. > > >> Perhaps some of the ideas here also could be used on CRLSet? I'm thinking >> especially using Golomb coding, since the CRLset also basically is a set of >> hashes. > > The Golomb code is agl@'s idea :) I think that the CRLSet will not benefit from > Golomb coding because it deals with many more certificates than this whitelist > (~120K EV certificates will ultimately be included in the list). > Correct. Adam has detailed the math somewhere on imperialviolet (sorry, on mobile) > >> > >> > I still need to think more about the interaction w/ respect to the >> CertVerifier, >> > CTVerifier, and the SSLClientSocket interactions. >> > > > > > > https://codereview.chromium.org/547603002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/06 17:07:42, Ryan Sleevi wrote: > On Oct 6, 2014 5:32 AM, <mailto:eranm@chromium.org> wrote: > > > > On 2014/10/06 11:42:59, haavardm wrote: > >> > >> On 2014/10/01 20:15:43, Ryan Sleevi wrote: > >> > Adding haavard from Opera so that he can comment more on Opera's > interest > > > > (or > >> > >> > lack) in this, so we can get layering right. > > > > > >> A few high level comments from me (A little late probably, but throwing > it in > >> anyway hoping I don't cause too much noise) > > > > > >> Since Opera doesn't compile the chrome/ directory: Is it necessary to > have the > >> EV whitelist decompression algorithm in chrome/browser/? Since zip or > other > >> standard compression algorithms won't be able to compress this (as the > data > >> seems random to those algorithms), it would be nice if users of the > content > > > > api > >> > >> also had access to this. > > > > I'll happily move it around. What do you think about moving it to > content/ once > > you have concrete plans to implement the CT requirement for EV certs, in a > > separate patch? > > > > > >> You probably already have thought about these next comments and have good > >> reasons not for doing it, bit I'll ask anyway: > > > > > >> * Instead of adding this as a separate list adding all this extra code to > >> Chromium, would be possible to set up a "special" chromium ct log (or > Opera > >> specific for that sake) which contains these certs, and chromium checks > this > > > > CT > >> > >> log as default only for EV certs not having SCT extension? > > > > Technically yes, there are two issues though: > > (1) The code to check inclusion in logs is not in yet :) > > (2) The benefit to a centrally-distributed whitelist is that each client > does > > not have to wait for a log's response to complete the SSL connection > > establishment. You could argue that the inclusion check should be done in > the > > background, but for EV certs that would mean updating the EV status some > time > > after the site has already loaded (potentially even after they user left > the > > site). It's also technically complex to update the EV state after the > > certificate status bits have been passed onto the UI (from what I've > seen). > > > > To be clear, its a non-starter to add any latency to TLS establishment. Any > online checks would do that, as well as add the privacy concerns of > disclosing the sites you visit to the log, a critical failure of OCSP. > > > > >> * Would it possible to use the cert dates (probably "not after" as you > cannot > >> trust the not before dates), and introduce EV certs in the same fashion > you > >> handle the deprecation of SHA-1, instead of adding this list? It would > not be > > > > as > >> > >> clear cut, but would eventually lead to CT log requirement for EV certs. > > > > I believe the answer is yes - and that is probably the approach we would > have > > taken if it wasn't for the fact we could whitelist all EV certs we know of > > relatively cheaply (the compressed whitelist is ~600K). > > In terms of cert issuance volumes, the CAs accounting for over 90% (95%?) > have > > stated they will implement CT and we have seen implementations of various > CAs > > coming online, so I don't think there's a concern here that a major part > of EV > > certificates will break when we require CT for EV. > > > > > > The answer is actually no. > > The cert dates aren't trust worthy - they are controlled by the CA. Whether > compelled or compromised, a CA could backdate certs and get another three > years of MITM compromise. > > The SHA-1 deprecation doesn't use the notBefore, but the notAfter, and with > a hard date for rollover. So it is different. I did actually say notAfter for this exact reason, but I realize it would take a few years to catch all certs this way. > > > > >> > With respect to layering, as mentioned in email, I think you're on the > right > >> > track, and we should just closer ape the CRLSet design. > > > > > >> Perhaps some of the ideas here also could be used on CRLSet? I'm thinking > >> especially using Golomb coding, since the CRLset also basically is a set > of > >> hashes. > > > > The Golomb code is agl@'s idea :) I think that the CRLSet will not > benefit from > > Golomb coding because it deals with many more certificates than this > whitelist > > (~120K EV certificates will ultimately be included in the list). > > > > Correct. Adam has detailed the math somewhere on imperialviolet (sorry, on > mobile) > > > > >> > > >> > I still need to think more about the interaction w/ respect to the > >> CertVerifier, > >> > CTVerifier, and the SSLClientSocket interactions. > >> > > > > > > > > > > > https://codereview.chromium.org/547603002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Ryan, are there any other outstanding issue with this patch?
https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... File chrome/browser/net/bit_stream_reader.cc (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.cc:22: res++; Why keep the temporary? You could change line 20 to *out = 0; while (...) ++(*out); return true; https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.cc:30: return false; API: Your API contract says they shouldn't call with num_bits > 64. Should this be a DCHECK here (indicating API failure), with some higher-layer doing the >64 comparison (as part of handling untrusted input) https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.cc:39: *out = res; ditto the temporary again https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.cc:46: return (source_.length() - (current_byte_ + 1)) * 8 + current_bit_ + 1; BUG: integer overflow issues here. Should you use the checked_numerics from base? https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... File chrome/browser/net/bit_stream_reader.h (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.h:18: // whitelist in a blocking thread. This comment needs rewording. You shouldn't discuss how it's used (a layering violation) or about the implementation details ("in a blocking thread") // A class for reading individual bits from a packed buffer. Bits are read MSB-first from the // stream. // It is limited to 64-bit reads and is inefficient as a design choice. This class should not // be used frequently. // // [There probably needs to be something here about how this reader encodes data, otherwise, why is it just not a byte, etc] https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.h:50: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... File chrome/browser/net/bit_stream_reader_unittest.cc (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader_unittest.cc:16: TEST(BitStreamReaderTest, CanReadSingleByte) { Need more negative tests (for out of bounds conditions). Like the overflow issue mentioned https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:25: namespace internal { There's no need for this to be in the internal namespace, is there? It shouldbe in the unnamed namespace? https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:34: void SetEVWhitelistFromFile(const base::FilePath& compressed_whitelist_file) { The documentation for this method needs to be updated to describe a bit of it's guarantees. e.g. is this meant to be called from a file worker and dispatch to the IO thread? Good to document. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:88: uint64_t curr_diff = read_prefix * M + r; Overflow? https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:101: int hash_val_comp(const void* v1, const void* v2) { Group your methods from the unnamed namespace into a single location at the top of the file. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:114: whitelist_ = new char[new_whitelist.size() * kCertHashLength]; LEAK: You never deallocate this. What if someone updates the EV whitelist during normal Chrome operation? https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:22: class PackedEVCertsWhitelist : public net::ct::EVCertsWhitelist { Document https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:24: explicit PackedEVCertsWhitelist(const std::string& compressed_whitelist); Document https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:42: std::vector<std::string>* uncompressed_list); Why is this protected+static? Private+static plus friend (potentially using a test fixture & "friend class PackedEVCertsWhitelistTest") https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:52: friend class base::RefCountedThreadSafe<net::ct::EVCertsWhitelist>; This isn't necessary - the base class must already friend this. https://codereview.chromium.org/547603002/diff/260001/net/cert/ct_ev_whitelist.h File net/cert/ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/260001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:8: #include <stdint.h> Unused? https://codereview.chromium.org/547603002/diff/260001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:39: NET_EXPORT EVCertsWhitelist* GetDefaultEVCertsWhitelist(); I'd rather nuke this and let NULL be valid, if at all possible.
PTAL. I'm breaking out the Fingerprint256 changes to a separate patch anyway. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... File chrome/browser/net/bit_stream_reader.cc (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.cc:22: res++; On 2014/10/20 19:18:24, Ryan Sleevi wrote: > Why keep the temporary? > > You could change line 20 to > *out = 0; > while (...) > ++(*out); > return true; Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.cc:30: return false; On 2014/10/20 19:18:24, Ryan Sleevi wrote: > API: Your API contract says they shouldn't call with num_bits > 64. Should this > be a DCHECK here (indicating API failure), with some higher-layer doing the >64 > comparison (as part of handling untrusted input) Done. In practice I always read a fixed amount so no untrusted input in this case. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.cc:39: *out = res; On 2014/10/20 19:18:24, Ryan Sleevi wrote: > ditto the temporary again Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.cc:46: return (source_.length() - (current_byte_ + 1)) * 8 + current_bit_ + 1; On 2014/10/20 19:18:24, Ryan Sleevi wrote: > BUG: integer overflow issues here. Should you use the checked_numerics from > base? Added a DCHECK that source_.length() is greater than current_byte_. current_byte_ should never exceed source_.length() and if they are equal (which is covered by the condition at the beginning of the method) then there are no more bits to read. Casting both source_.length() and current_byte_ to signed values just for the negation would be cumbersome and we can get the same protection against underflow with the DCHECK. As for the potential for overflow, I've added a DCHECK in the c'tor that the source_.length() never exceeds UINT32_MAX. In practice creating a StringPiece instance so big that it's length could get anywhere near uint64 limit is likely to result in problems elsewhere... https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... File chrome/browser/net/bit_stream_reader.h (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.h:18: // whitelist in a blocking thread. On 2014/10/20 19:18:24, Ryan Sleevi wrote: > This comment needs rewording. You shouldn't discuss how it's used (a layering > violation) or about the implementation details ("in a blocking thread") > > // A class for reading individual bits from a packed buffer. Bits are read > MSB-first from the > // stream. > // It is limited to 64-bit reads and is inefficient as a design choice. This > class should not > // be used frequently. > // > // [There probably needs to be something here about how this reader encodes > data, otherwise, why is it just not a byte, etc] Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.h:50: }; On 2014/10/20 19:18:25, Ryan Sleevi wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... File chrome/browser/net/bit_stream_reader_unittest.cc (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader_unittest.cc:16: TEST(BitStreamReaderTest, CanReadSingleByte) { On 2014/10/20 19:18:25, Ryan Sleevi wrote: > Need more negative tests (for out of bounds conditions). Like the overflow issue > mentioned I've added another test for empty stream. As for hitting out of bounds condition, the DCHECK for the size of the input StringPiece should handle that (is there a way to test that a DCHECK is triggered?). https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:25: namespace internal { On 2014/10/20 19:18:25, Ryan Sleevi wrote: > There's no need for this to be in the internal namespace, is there? It shouldbe > in the unnamed namespace? Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:34: void SetEVWhitelistFromFile(const base::FilePath& compressed_whitelist_file) { On 2014/10/20 19:18:25, Ryan Sleevi wrote: > The documentation for this method needs to be updated to describe a bit of it's > guarantees. e.g. is this meant to be called from a file worker and dispatch to > the IO thread? Good to document. Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:88: uint64_t curr_diff = read_prefix * M + r; On 2014/10/20 19:18:25, Ryan Sleevi wrote: > Overflow? Good catch, added checks for validity of data and a test verifying this method returns false when encountering a value that would lead to overflow. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:101: int hash_val_comp(const void* v1, const void* v2) { On 2014/10/20 19:18:25, Ryan Sleevi wrote: > Group your methods from the unnamed namespace into a single location at the top > of the file. Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:114: whitelist_ = new char[new_whitelist.size() * kCertHashLength]; On 2014/10/20 19:18:25, Ryan Sleevi wrote: > LEAK: You never deallocate this. What if someone updates the EV whitelist during > normal Chrome operation? Good catch, done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:22: class PackedEVCertsWhitelist : public net::ct::EVCertsWhitelist { On 2014/10/20 19:18:25, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:24: explicit PackedEVCertsWhitelist(const std::string& compressed_whitelist); On 2014/10/20 19:18:25, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:42: std::vector<std::string>* uncompressed_list); On 2014/10/20 19:18:25, Ryan Sleevi wrote: > Why is this protected+static? > > Private+static plus friend (potentially using a test fixture & "friend class > PackedEVCertsWhitelistTest") Done. https://codereview.chromium.org/547603002/diff/260001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:52: friend class base::RefCountedThreadSafe<net::ct::EVCertsWhitelist>; On 2014/10/20 19:18:25, Ryan Sleevi wrote: > This isn't necessary - the base class must already friend this. Done. https://codereview.chromium.org/547603002/diff/260001/net/cert/ct_ev_whitelist.h File net/cert/ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/260001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:8: #include <stdint.h> On 2014/10/20 19:18:25, Ryan Sleevi wrote: > Unused? Done. https://codereview.chromium.org/547603002/diff/260001/net/cert/ct_ev_whitelis... net/cert/ct_ev_whitelist.h:39: NET_EXPORT EVCertsWhitelist* GetDefaultEVCertsWhitelist(); On 2014/10/20 19:18:25, Ryan Sleevi wrote: > I'd rather nuke this and let NULL be valid, if at all possible. Done.
https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:25: scoped_refptr<net::ct::EVCertsWhitelist> new_whitelist) { pass refcounted pointers as const-ref https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:74: static const uint64_t M = static_cast<uint64_t>(1) << kGolombMParameterBits; needs a better name? Line 82 surprises me when reading it. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:112: whitelist_ = new char[new_whitelist.size() * kCertHashLength]; Is there a reason that you're manually managing the memory like this? Note that this is a code-smell for overflow issues - new_whitelist.size() * kCertHashLength causing an overflow (if new_whitelist.size() > (max / kCertHashLength), bad things) Our normal pattern is to use something like std::vector<char[8]>, and then allocate the vector to be new_whitelist.size(). vector will ensure that new_whitelist.size() < (size / sizeof(elem)). https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:14: #include "base/memory/ref_counted.h" Unusued? https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:15: #include "base/strings/string_piece.h" Unused? https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:26: // * Repeating Golomb-coded number which is the numberic difference of the s/numeric/ https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:44: virtual bool IsValid() const override; latest Chromium style is you can drop the "virtual" from these (the 'override' is sufficient) https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:47: virtual ~PackedEVCertsWhitelist(); latest Chromium style is ~PackedEVCertsWhitelist() override; https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:55: UncompressesWhitelistCorrectly); These can / should all be private friends, not protected friends. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:73: uint32_t whitelist_size_; DISALLOW_COPY_AND_ASSIGN
Got rid of manual memory management, addressed all other comments, reverted x509_certificate* changes that already went into master. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:25: scoped_refptr<net::ct::EVCertsWhitelist> new_whitelist) { On 2014/10/21 22:50:25, Ryan Sleevi wrote: > pass refcounted pointers as const-ref Done. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:74: static const uint64_t M = static_cast<uint64_t>(1) << kGolombMParameterBits; On 2014/10/21 22:50:25, Ryan Sleevi wrote: > needs a better name? Line 82 surprises me when reading it. Can you suggest one? I've added a comment to clarify that it's the tunable parameter used by the Golomb coding, but couldn't think of a better name. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:112: whitelist_ = new char[new_whitelist.size() * kCertHashLength]; On 2014/10/21 22:50:25, Ryan Sleevi wrote: > Is there a reason that you're manually managing the memory like this? > > Note that this is a code-smell for overflow issues - new_whitelist.size() * > kCertHashLength causing an overflow (if new_whitelist.size() > (max / > kCertHashLength), bad things) > > Our normal pattern is to use something like std::vector<char[8]>, and then > allocate the vector to be new_whitelist.size(). vector will ensure that > new_whitelist.size() < (size / sizeof(elem)). Switched to using vector and std::binary_search. It wasn't clear to me if your comment is about the manual memory management or the usage of a raw type (char*). I've used raw memory access here since bsearch, which was used later on, requires a buffer. But binary_search can do the job equally well. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.h (right): https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:14: #include "base/memory/ref_counted.h" On 2014/10/21 22:50:26, Ryan Sleevi wrote: > Unusued? Done. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:15: #include "base/strings/string_piece.h" On 2014/10/21 22:50:25, Ryan Sleevi wrote: > Unused? Done. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:26: // * Repeating Golomb-coded number which is the numberic difference of the On 2014/10/21 22:50:25, Ryan Sleevi wrote: > s/numeric/ Done. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:44: virtual bool IsValid() const override; On 2014/10/21 22:50:25, Ryan Sleevi wrote: > latest Chromium style is you can drop the "virtual" from these (the 'override' > is sufficient) Done. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:47: virtual ~PackedEVCertsWhitelist(); On 2014/10/21 22:50:25, Ryan Sleevi wrote: > latest Chromium style is > > ~PackedEVCertsWhitelist() override; Done. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:55: UncompressesWhitelistCorrectly); On 2014/10/21 22:50:25, Ryan Sleevi wrote: > These can / should all be private friends, not protected friends. Done. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.h:73: uint32_t whitelist_size_; On 2014/10/21 22:50:25, Ryan Sleevi wrote: > DISALLOW_COPY_AND_ASSIGN Done.
https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:112: whitelist_ = new char[new_whitelist.size() * kCertHashLength]; On 2014/10/22 10:53:26, Eran wrote: > On 2014/10/21 22:50:25, Ryan Sleevi wrote: > > Is there a reason that you're manually managing the memory like this? > > > > Note that this is a code-smell for overflow issues - new_whitelist.size() * > > kCertHashLength causing an overflow (if new_whitelist.size() > (max / > > kCertHashLength), bad things) > > > > Our normal pattern is to use something like std::vector<char[8]>, and then > > allocate the vector to be new_whitelist.size(). vector will ensure that > > new_whitelist.size() < (size / sizeof(elem)). > > Switched to using vector and std::binary_search. > > It wasn't clear to me if your comment is about the manual memory management or > the usage of a raw type (char*). > I've used raw memory access here since bsearch, which was used later on, > requires a buffer. But binary_search can do the job equally well. > FWIW, using std::binary_search is twice is slow as using bsearch :/ std::map vs bsearch: std::map takes 140% more time than bseacrh. std::map vs std::binary_search: std::map takes only 20% more than binary_search over a vector. I don't think that performance here matters that much, just FYI.
Still need to update the QUIC code - https://chromium.googlesource.com/chromium/src/+/master/net/quic/crypto/proof... Otherwise, I think we're almost there, but I think you went the wrong direction based on the previous comments. I've attempted to clarify below, let me know if this helps. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:74: static const uint64_t M = static_cast<uint64_t>(1) << kGolombMParameterBits; On 2014/10/22 10:53:26, Eran wrote: > On 2014/10/21 22:50:25, Ryan Sleevi wrote: > > needs a better name? Line 82 surprises me when reading it. > > Can you suggest one? I've added a comment to clarify that it's the tunable > parameter used by the Golomb coding, but couldn't think of a better name. even just something like kGolombParameterM or something to make it more explicit than a single letter :) https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:112: whitelist_ = new char[new_whitelist.size() * kCertHashLength]; On 2014/10/22 10:53:26, Eran wrote: > On 2014/10/21 22:50:25, Ryan Sleevi wrote: > > Is there a reason that you're manually managing the memory like this? > > > > Note that this is a code-smell for overflow issues - new_whitelist.size() * > > kCertHashLength causing an overflow (if new_whitelist.size() > (max / > > kCertHashLength), bad things) > > > > Our normal pattern is to use something like std::vector<char[8]>, and then > > allocate the vector to be new_whitelist.size(). vector will ensure that > > new_whitelist.size() < (size / sizeof(elem)). > > Switched to using vector and std::binary_search. > > It wasn't clear to me if your comment is about the manual memory management or > the usage of a raw type (char*). Well, both :) More aptly the raw memory management AND the lack of explicit sizes. > I've used raw memory access here since bsearch, which was used later on, > requires a buffer. But binary_search can do the job equally well. > I think this misses where I was originally going. You know the contents of the vector are fixed (it's a vector of 8 byte hashes), so vector<char[8]> is a totally legal definition. You'll get the same memory layout as char[8*size], but with the added consistency checks for overflow as part of vector allocation. Put differently, explicitly: std::vector<char[8]> whitelist_; whitelist_.resize(new_whitelist.size()); for (size_t i = 0; i < new_whitelist.size(); ++i) { memcpy(&whitelist_[i], new_whitelist[i].data(), kCertHashLength); } is_whitelist_valid_ = true; Then you can still use bsearch, as &whitelist_[0] will be fully contiguous. https://codereview.chromium.org/547603002/diff/320001/chrome/browser/net/bit_... File chrome/browser/net/bit_stream_reader.h (right): https://codereview.chromium.org/547603002/diff/320001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.h:46: uint64_t current_byte_; this should be a size_t (since it deals with byte offsets) https://codereview.chromium.org/547603002/diff/320001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/320001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:94: result.push_back(std::string(hash_bytes, 8)); This 8 is duplicated on lines 68 and 66. Perhaps this should be a constant (kHashSize) ?
Addressed comments, changed the quic verifier. Note that to store a char[8] in the vector I had to wrap it in a struct. I moved the constants outside of packed_ev_certs_whitelist.cc into the header file (as static private consts) so they can be used in the array definition. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:74: static const uint64_t M = static_cast<uint64_t>(1) << kGolombMParameterBits; On 2014/10/22 19:26:58, Ryan Sleevi wrote: > On 2014/10/22 10:53:26, Eran wrote: > > On 2014/10/21 22:50:25, Ryan Sleevi wrote: > > > needs a better name? Line 82 surprises me when reading it. > > > > Can you suggest one? I've added a comment to clarify that it's the tunable > > parameter used by the Golomb coding, but couldn't think of a better name. > > even just something like kGolombParameterM or something to make it more explicit > than a single letter :) Done, used your suggested name. https://codereview.chromium.org/547603002/diff/300001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:112: whitelist_ = new char[new_whitelist.size() * kCertHashLength]; On 2014/10/22 19:26:58, Ryan Sleevi wrote: > On 2014/10/22 10:53:26, Eran wrote: > > On 2014/10/21 22:50:25, Ryan Sleevi wrote: > > > Is there a reason that you're manually managing the memory like this? > > > > > > Note that this is a code-smell for overflow issues - new_whitelist.size() * > > > kCertHashLength causing an overflow (if new_whitelist.size() > (max / > > > kCertHashLength), bad things) > > > > > > Our normal pattern is to use something like std::vector<char[8]>, and then > > > allocate the vector to be new_whitelist.size(). vector will ensure that > > > new_whitelist.size() < (size / sizeof(elem)). > > > > Switched to using vector and std::binary_search. > > > > It wasn't clear to me if your comment is about the manual memory management or > > the usage of a raw type (char*). > > Well, both :) More aptly the raw memory management AND the lack of explicit > sizes. > > > I've used raw memory access here since bsearch, which was used later on, > > requires a buffer. But binary_search can do the job equally well. > > > > I think this misses where I was originally going. You know the contents of the > vector are fixed (it's a vector of 8 byte hashes), so vector<char[8]> is a > totally legal definition. You'll get the same memory layout as char[8*size], but > with the added consistency checks for overflow as part of vector allocation. > > Put differently, explicitly: > > std::vector<char[8]> whitelist_; > > whitelist_.resize(new_whitelist.size()); > for (size_t i = 0; i < new_whitelist.size(); ++i) { > memcpy(&whitelist_[i], new_whitelist[i].data(), kCertHashLength); > } > is_whitelist_valid_ = true; > > Then you can still use bsearch, as &whitelist_[0] will be fully contiguous. Done, although in a slightly different way: std::vector<char[8]> whitelist_; may be a legal definition with some compilers, but since arrays are not copyable or assignable, they have to be stored in a struct and that struct used as the template parameter for the vector. So that's what I did (see struct TruncatedHash in the header file). It's not pretty, but it works. https://codereview.chromium.org/547603002/diff/320001/chrome/browser/net/bit_... File chrome/browser/net/bit_stream_reader.h (right): https://codereview.chromium.org/547603002/diff/320001/chrome/browser/net/bit_... chrome/browser/net/bit_stream_reader.h:46: uint64_t current_byte_; On 2014/10/22 19:26:58, Ryan Sleevi wrote: > this should be a size_t (since it deals with byte offsets) Done. https://codereview.chromium.org/547603002/diff/320001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/320001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:94: result.push_back(std::string(hash_bytes, 8)); On 2014/10/22 19:26:58, Ryan Sleevi wrote: > This 8 is duplicated on lines 68 and 66. Perhaps this should be a constant > (kHashSize) ? Done.
Per offline discussion, using uint64_t in the vector rather than an array.
Post-weekend friendly ping.
LGTM mod two things. https://codereview.chromium.org/547603002/diff/360001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/360001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:51: base::Callback<void(void)> assign_cb = you can use base::Closure, but that's no big deal. https://codereview.chromium.org/547603002/diff/360001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:100: result.push_back(std::string(hash_bytes, kCertHashLength)); Why convert to an std::string, when you have it as a uint64_t here? That is, why can't this just be an std::vector<uint64_t>? Did I miss something? https://codereview.chromium.org/547603002/diff/360001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:130: return bsearch(certificate_hash.c_str(), Isn't this where you should be ensuring big endian conversion? Otherwise, you're just directly casting certificate_hash to a uint64_t
Addressed all comments, attempting submission now. https://codereview.chromium.org/547603002/diff/360001/chrome/browser/net/pack... File chrome/browser/net/packed_ct_ev_whitelist.cc (right): https://codereview.chromium.org/547603002/diff/360001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:51: base::Callback<void(void)> assign_cb = On 2014/10/27 17:09:48, Ryan Sleevi wrote: > you can use base::Closure, but that's no big deal. Done. https://codereview.chromium.org/547603002/diff/360001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:100: result.push_back(std::string(hash_bytes, kCertHashLength)); On 2014/10/27 17:09:48, Ryan Sleevi wrote: > Why convert to an std::string, when you have it as a uint64_t here? > > That is, why can't this just be an std::vector<uint64_t>? Did I miss something? No reason - changed. https://codereview.chromium.org/547603002/diff/360001/chrome/browser/net/pack... chrome/browser/net/packed_ct_ev_whitelist.cc:130: return bsearch(certificate_hash.c_str(), On 2014/10/27 17:09:48, Ryan Sleevi wrote: > Isn't this where you should be ensuring big endian conversion? Otherwise, you're > just directly casting certificate_hash to a uint64_t Good catch, done.
The CQ bit was checked by eranm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/547603002/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/efbd3137115a35c938a0cb8fa54d7c4b33403afb Cr-Commit-Position: refs/heads/master@{#301642} |
