Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1058003004:
Forget SSL error exceptions when good certs seen for regular requests. (Closed)
DescriptionForget SSL error exceptions when good certs seen for regular requests.
Chrome remembers decisions by the user to proceed through SSL errors.
However, it remembers this even after a good certificate has been seen
for the given host. This change removes all previous exceptions for a
given host after a good certificate is seen on a regular request,
but not on redirects.
In the SSLPolicy, this adds a call to RevokeUserAllowExceptions() on the
SSLHostStateDelegate when a request response begins. This removes all
prior exceptions for the specified host. However, there is currently no
similar plumbing for redirects, so until that plumbing is built,
revocation will not occur when a valid cert for a host is seen on
redirect.
BUG=473390
Committed: https://crrev.com/5a586e5039cf30c3071b14a8e1df105f08c6b06a
Cr-Commit-Position: refs/heads/master@{#326332}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changed browser test to use TestRootCerts #
Total comments: 16
Patch Set 3 : Tests related to unsafe resources w/user exceptions #
Total comments: 9
Patch Set 4 : Rebase on ToT #Patch Set 5 : Addressed histogram comments #
Total comments: 5
Patch Set 6 : Nit from felt #
Total comments: 2
Patch Set 7 : Updated histogram description #Patch Set 8 : Rebase on ToT #
Total comments: 4
Patch Set 9 : Nits from davdben #Patch Set 10 : Rebase on ToT #Patch Set 11 : Fix Android WebView SSLHostStateDelegate #Patch Set 12 : Another WebView compile error #Patch Set 13 : Yet Another Webview Fix (should be the last, I swear) #Messages
Total messages: 57 (15 generated)
jww@chromium.org changed reviewers: + davidben@chromium.org, felt@chromium.org
This is the implementation of the revocation we discussed the other day. I know the testing layering seems a bit funny, but in order to test the actual functionality, there needs to be an implementation of SSLHostStateDelegate, which only exists in //chrome. Thus, I put the test over there.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1058003004/diff/1/net/test/spawned_test_serve... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/1058003004/diff/1/net/test/spawned_test_serve... net/test/spawned_test_server/base_test_server.h:260: void set_ssl_options(const SSLOptions& ssl_options); Drive by not-LGTM on these. TBH, I'm surprised 254-258 exist, those definitely run hinky w/ the BaseTestServer trying to be immutable-once-init'd.
Ok, so not-LGT marky mark is seen as an OK, not the Not LGTM it's supposed to be.
Any suggestions on how to make this look better? What about a specific "set_cert_response" or something similar on BaseTestServer? On Thu, Apr 2, 2015, 5:21 PM <rsleevi@chromium.org> wrote: > Ok, so not-LGT marky mark is seen as an OK, not the Not LGTM it's supposed > to > be. > > https://codereview.chromium.org/1058003004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/03 00:37:15, jww wrote: > Any suggestions on how to make this look better? What about a specific > "set_cert_response" or something similar on BaseTestServer? It "sounds" like you want a MockCertVerifier if you just want to induce errors. Otherwise, you need to restart the test server in between requests and use your new config (which generally means changing ports, which means it no longer origin-matches, which means doing something hinky at a lower layer anyways)
On 2015/04/03 00:54:50, Ryan Sleevi wrote: > On 2015/04/03 00:37:15, jww wrote: > > Any suggestions on how to make this look better? What about a specific > > "set_cert_response" or something similar on BaseTestServer? > > It "sounds" like you want a MockCertVerifier if you just want to induce errors. > Otherwise, you need to restart the test server in between requests and use your > new config (which generally means changing ports, which means it no longer > origin-matches, which means doing something hinky at a lower layer anyways) Thanks! I'm trying to use MockCertVerifier, as you suggested, and I'm able to set a MockCertVerifier on the URLRequestContext, however the browser test never seems to call Verify() on the MockCertVerifier. It appears that it's calling Verify() on CertVerifyProc instead, which doesn't ever call into CertVerifier/MockCertVerifier. Any suggestions on what I'm missing here?
On 2015/04/03 22:57:44, jww wrote: > On 2015/04/03 00:54:50, Ryan Sleevi wrote: > > On 2015/04/03 00:37:15, jww wrote: > > > Any suggestions on how to make this look better? What about a specific > > > "set_cert_response" or something similar on BaseTestServer? > > > > It "sounds" like you want a MockCertVerifier if you just want to induce > errors. > > Otherwise, you need to restart the test server in between requests and use > your > > new config (which generally means changing ports, which means it no longer > > origin-matches, which means doing something hinky at a lower layer anyways) > > Thanks! I'm trying to use MockCertVerifier, as you suggested, and I'm able to > set a MockCertVerifier on the URLRequestContext, however the browser test never > seems to call Verify() on the MockCertVerifier. It appears that it's calling > Verify() on CertVerifyProc instead, which doesn't ever call into > CertVerifier/MockCertVerifier. Any suggestions on what I'm missing here? Ah, question partially answered and new questions raised... it's being called from MultiThreadedCertVerifier::Verify(). Which raises the question, why doesn't set_cert_verifier() on URLRequestContext() change the CertVerifier from MultiThreatedCertVerifier to MockCertVerifier? Next week on "As the Net Stack Turns"!
On 2015/04/03 23:06:34, jww wrote: > Ah, question partially answered and new questions raised... it's being called > from MultiThreadedCertVerifier::Verify(). Which raises the question, why doesn't > set_cert_verifier() on URLRequestContext() change the CertVerifier from > MultiThreatedCertVerifier to MockCertVerifier? Next week on "As the Net Stack > Turns"! You can only do that before the URLRequest is initialized. That is, the CertVerifier is owned by the URLRequestContext's storage, and passed in to the NetworkSessionParams. Once the HttpNetworkSession is initialized, the verifier is immutable (because the verifier participates in all of the pooling logic) That said, not sure if you can muck with it - David would know. Either way, the TestServer stuff won't "work" / isn't "compatible", so if you need to go down that route, you're going to have to add magic URLs to the test server to dynamically re-configure itself, AND deal with situations that you can't refer to file paths (I mean, the TestSErver is meant to hide all that from you). Alternatively, you could set up test servers on multiple local IPs running different configs, then use the RuleBasedHostResolver to interpose and redirect a given URL (e.g. example.com) to 127.0.0.1 before, run the test, then update the resolver to then redirect it to 127.0.0.2 (different test server, different config), and run the second part of the test. That *might* work. Or socket pools might get in the way.
On 2015/04/03 23:26:18, Ryan Sleevi wrote: > On 2015/04/03 23:06:34, jww wrote: > > Ah, question partially answered and new questions raised... it's being called > > from MultiThreadedCertVerifier::Verify(). Which raises the question, why > doesn't > > set_cert_verifier() on URLRequestContext() change the CertVerifier from > > MultiThreatedCertVerifier to MockCertVerifier? Next week on "As the Net Stack > > Turns"! > > You can only do that before the URLRequest is initialized. That is, the > CertVerifier is owned by the URLRequestContext's storage, and passed in to the > NetworkSessionParams. Once the HttpNetworkSession is initialized, the verifier > is immutable (because the verifier participates in all of the pooling logic) > > That said, not sure if you can muck with it - David would know. > > Either way, the TestServer stuff won't "work" / isn't "compatible", so if you > need to go down that route, you're going to have to add magic URLs to the test > server to dynamically re-configure itself, AND deal with situations that you > can't refer to file paths (I mean, the TestSErver is meant to hide all that from > you). I'm surprised to hear that. It certainly seems to be calling into MultiThreadedCertVerifier::Verify(), so it seems like if I'm somehow able to shim MockCertVerifier in there, it would work. But I'm sure there's a lot I'm not understanding here :-) > > Alternatively, you could set up test servers on multiple local IPs running > different configs, then use the RuleBasedHostResolver to interpose and redirect > a given URL (e.g. http://example.com) to 127.0.0.1 before, run the test, then update > the resolver to then redirect it to 127.0.0.2 (different test server, different > config), and run the second part of the test. > > That *might* work. Or socket pools might get in the way. Ugh. Thanks.
On 2015/04/03 23:31:31, jww wrote: > I'm surprised to hear that. It certainly seems to be calling into > MultiThreadedCertVerifier::Verify(), so it seems like if I'm somehow able to > shim MockCertVerifier in there, it would work. But I'm sure there's a lot I'm > not understanding here :-) It's not calling into MTCV by bouncing through the URLRequest's get_cert_verifier(), it's going through the HttpNetworkSession, which is already initialized. To better explain why it doesn't work - for mobile, we have a single IPC that is sent from the device when it starts the TestServer that passes along the config. If you want to change a TestServer's config, you need to create a new TestServer (which sets up a new IPC channel to the host and sends the new config) Restarting a TestServer generates a new ephemeral port for that test server, thus a new origin. But it's also the API contract - TestServer is immutable after construction. Violations of that create issues :/
Man, this is a nuisance. So there's TestRootCerts which you can fiddle with. But that doesn't work because MTCV has a cache, so even if you add/remove the root, I'm not sure it'll notice. One option is to get the MockCertVerifier injected early, all the way at startup. If we're going to inject something global like that, doing it at browser startup seems sanest... but IOThread::Globals doesn't seem to have a story for that. I wouldn't be too unhappy if browser tests started to have more principled injection mechanisms. Another option is to make MTCV's cache not bite us. There's CertDatabase::GetInstance()->NotifyObserversOfCACertChanged. That'll clear MTCV's cache. Having TestRootCerts call that when it changes things seems... actually pretty sensible in itself. How about that? You could run the connection first with the test server's root removed (or maybe we add a new root to the test server which it doesn't auto-install). Then add it back and connect again. Ryan, does this pass the "doesn't make Sleevi scream too loudly" sniff test? https://codereview.chromium.org/1058003004/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1058003004/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:2152: You should probably flush the socket pools here for good measure. Otherwise you might just reuse an old socket. Though I don't know if the test server does keep-alives? This is how net-internals does it: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... That'll need to all run on the IO thread, so do a PostTaskAndReply with the reply being the QuitClosure of a base::RunLoop and then spin the RunLoop.
On 2015/04/07 21:08:43, David Benjamin wrote: > Man, this is a nuisance. So there's TestRootCerts which you can fiddle with. But > that doesn't work because MTCV has a cache, so even if you add/remove the root, > I'm not sure it'll notice. > > One option is to get the MockCertVerifier injected early, all the way at > startup. If we're going to inject something global like that, doing it at > browser startup seems sanest... but IOThread::Globals doesn't seem to have a > story for that. I wouldn't be too unhappy if browser tests started to have more > principled injection mechanisms. > > Another option is to make MTCV's cache not bite us. There's > CertDatabase::GetInstance()->NotifyObserversOfCACertChanged. That'll clear > MTCV's cache. Having TestRootCerts call that when it changes things seems... > actually pretty sensible in itself. > > How about that? You could run the connection first with the test server's root > removed (or maybe we add a new root to the test server which it doesn't > auto-install). Then add it back and connect again. Ryan, does this pass the > "doesn't make Sleevi scream too loudly" sniff test? > > https://codereview.chromium.org/1058003004/diff/1/chrome/browser/ssl/ssl_brow... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > https://codereview.chromium.org/1058003004/diff/1/chrome/browser/ssl/ssl_brow... > chrome/browser/ssl/ssl_browser_tests.cc:2152: > You should probably flush the socket pools here for good measure. Otherwise you > might just reuse an old socket. Though I don't know if the test server does > keep-alives? > > This is how net-internals does it: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > That'll need to all run on the IO thread, so do a PostTaskAndReply with the > reply being the QuitClosure of a base::RunLoop and then spin the RunLoop. I started trying to implement Ryan's suggestion of using RuleBasedHostObserver, but that's turning out to be trickier than I expected, so I'm happy to take whatever approach y'all think is best. Ryan, what do you think of David's suggestion?
On 2015/04/07 21:38:06, jww wrote: > On 2015/04/07 21:08:43, David Benjamin wrote: > > Man, this is a nuisance. So there's TestRootCerts which you can fiddle with. > But > > that doesn't work because MTCV has a cache, so even if you add/remove the > root, > > I'm not sure it'll notice. > > > > One option is to get the MockCertVerifier injected early, all the way at > > startup. If we're going to inject something global like that, doing it at > > browser startup seems sanest... but IOThread::Globals doesn't seem to have a > > story for that. I wouldn't be too unhappy if browser tests started to have > more > > principled injection mechanisms. > > > > Another option is to make MTCV's cache not bite us. There's > > CertDatabase::GetInstance()->NotifyObserversOfCACertChanged. That'll clear > > MTCV's cache. Having TestRootCerts call that when it changes things seems... > > actually pretty sensible in itself. > > > > How about that? You could run the connection first with the test server's root > > removed (or maybe we add a new root to the test server which it doesn't > > auto-install). Then add it back and connect again. Ryan, does this pass the > > "doesn't make Sleevi scream too loudly" sniff test? > > > > > https://codereview.chromium.org/1058003004/diff/1/chrome/browser/ssl/ssl_brow... > > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > > > > https://codereview.chromium.org/1058003004/diff/1/chrome/browser/ssl/ssl_brow... > > chrome/browser/ssl/ssl_browser_tests.cc:2152: > > You should probably flush the socket pools here for good measure. Otherwise > you > > might just reuse an old socket. Though I don't know if the test server does > > keep-alives? > > > > This is how net-internals does it: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > That'll need to all run on the IO thread, so do a PostTaskAndReply with the > > reply being the QuitClosure of a base::RunLoop and then spin the RunLoop. > > I started trying to implement Ryan's suggestion of using RuleBasedHostObserver, > but that's turning out to be trickier than I expected, so I'm happy to take > whatever approach y'all think is best. Ryan, what do you think of David's > suggestion? David's suggestion of using TestRootCerts is pretty much working for me, so at this point, that's my preference :-) I should have it uploaded shortly, and hopefully it won't stink too bad to Ryan.
https://codereview.chromium.org/1058003004/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1058003004/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:2152: On 2015/04/07 21:08:43, David Benjamin wrote: > You should probably flush the socket pools here for good measure. Otherwise you > might just reuse an old socket. Though I don't know if the test server does > keep-alives? > > This is how net-internals does it: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > That'll need to all run on the IO thread, so do a PostTaskAndReply with the > reply being the QuitClosure of a base::RunLoop and then spin the RunLoop. Done. https://codereview.chromium.org/1058003004/diff/1/net/test/spawned_test_serve... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/1058003004/diff/1/net/test/spawned_test_serve... net/test/spawned_test_server/base_test_server.h:260: void set_ssl_options(const SSLOptions& ssl_options); On 2015/04/03 00:21:12, Ryan Sleevi wrote: > Drive by not-LGTM on these. TBH, I'm surprised 254-258 exist, those definitely > run hinky w/ the BaseTestServer trying to be immutable-once-init'd. Gone.
I'll give my blessing and LGTM on this. Kudos to David for the creative solution that doesn't make me sad panda :) https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:230: void RootCertsChangedOnIO(net::URLRequestContextGetter* context_getter) { s/OnIO/OnIOThread/ https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:230: void RootCertsChangedOnIO(net::URLRequestContextGetter* context_getter) { DANGER WILL ROBINSON: Passing a naked pointer around when you need to keep the ref alive. const scoped_refptr<net::URLRequestContextGetter>& context_getter https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:237: void RootCertsChanged(WebContents* contents) { Document https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:238: net::CertDatabase::GetInstance()->NotifyObserversOfCACertChanged(NULL); Running this on not-IO thread? Sounds super sketch. This should be happening on IO IINM. https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:2172: ASSERT_TRUE(root_certs != NULL); ASSERT_TRUE(root_certs) https://codereview.chromium.org/1058003004/diff/20001/net/test/spawned_test_s... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/1058003004/diff/20001/net/test/spawned_test_s... net/test/spawned_test_server/base_test_server.h:262: bool LoadTestRootCert() const WARN_UNUSED_RESULT; STYLE: Reorder the .cc if you reorder the .h
https://codereview.chromium.org/1058003004/diff/20001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/1058003004/diff/20001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:118: backend_->RevokeUserAllowExceptions(info->url().host()); Can you record how often this happens w/ UMA?
https://codereview.chromium.org/1058003004/diff/20001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy_backend.h (right): https://codereview.chromium.org/1058003004/diff/20001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy_backend.h:30: // Revokes all allow exceptions by by the user for |host|. nit: by by
David pointed out to me on hangouts that this is all terribly thread-racy, but that's not a problem you've caused, nor is it one that has a good solution. Just, uh, be aware that here be (somewhat unavoidable) dragons.
jww@chromium.org changed reviewers: + mpearson@chromium.org
mpearson@chromium.org: Can you review the histograms.xml change? felt@, can you take another look at the ssl* changes? Thanks! https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:230: void RootCertsChangedOnIO(net::URLRequestContextGetter* context_getter) { On 2015/04/09 20:55:48, Ryan Sleevi wrote: > s/OnIO/OnIOThread/ Done. https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:230: void RootCertsChangedOnIO(net::URLRequestContextGetter* context_getter) { On 2015/04/09 20:55:48, Ryan Sleevi wrote: > DANGER WILL ROBINSON: Passing a naked pointer around when you need to keep the > ref alive. > > const scoped_refptr<net::URLRequestContextGetter>& context_getter Done. https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:237: void RootCertsChanged(WebContents* contents) { On 2015/04/09 20:55:48, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:238: net::CertDatabase::GetInstance()->NotifyObserversOfCACertChanged(NULL); On 2015/04/09 20:55:48, Ryan Sleevi wrote: > Running this on not-IO thread? Sounds super sketch. This should be happening on > IO IINM. I'ved moved it to RootCertsChangedOnIOThread(). https://codereview.chromium.org/1058003004/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:2172: ASSERT_TRUE(root_certs != NULL); On 2015/04/09 20:55:48, Ryan Sleevi wrote: > ASSERT_TRUE(root_certs) Done. https://codereview.chromium.org/1058003004/diff/20001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/1058003004/diff/20001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:118: backend_->RevokeUserAllowExceptions(info->url().host()); On 2015/04/09 21:05:07, felt wrote: > Can you record how often this happens w/ UMA? Hm, this is going to happen every time a good cert is seen, so I'm not sure that's interesting. What seems interesting to me is when we see revoke a set of exceptions, and there already *is* a set of exceptions made. I'll add in the logic for that, so let me know if you disagree. https://codereview.chromium.org/1058003004/diff/20001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy_backend.h (right): https://codereview.chromium.org/1058003004/diff/20001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy_backend.h:30: // Revokes all allow exceptions by by the user for |host|. On 2015/04/09 21:57:28, felt wrote: > nit: by by Done. https://codereview.chromium.org/1058003004/diff/20001/net/test/spawned_test_s... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/1058003004/diff/20001/net/test/spawned_test_s... net/test/spawned_test_server/base_test_server.h:262: bool LoadTestRootCert() const WARN_UNUSED_RESULT; On 2015/04/09 20:55:48, Ryan Sleevi wrote: > STYLE: Reorder the .cc if you reorder the .h Done.
https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:11: #include "base/metrics/histogram.h" histogram_macros is more appropriate https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:109: enum SSLGoodCertSeenEvent { 1. This is an odd place for this enum. This is style-acceptable? 2. Please put the useful standard warning label about enums that a recorded in UMA (don't deleted or reorder or renumber; add new entry only at end) 3. Please explicitly do =0, =1, etc. https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:126: UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.good_cert_seen", event, This sounds more like a BOOLEAN histogram. Why are you using an enum here? Do you have the expectation that it needs to expand in the future? If so, how? https://codereview.chromium.org/1058003004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1058003004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12747: + exception for a bad certificate. Recorded any time a new connection is made This first sentence is a bit wrong (it doesn't track how often anything happens) and slightly backwards. Perhaps something like this: Records whether the user has already given an exception for a bad certificate on this site.
https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:11: #include "base/metrics/histogram.h" On 2015/04/17 16:20:36, Mark P wrote: > histogram_macros is more appropriate Done. https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:109: enum SSLGoodCertSeenEvent { On 2015/04/17 16:20:36, Mark P wrote: > 1. This is an odd place for this enum. This is style-acceptable? You're right, it probably should be at the top in an anonymous namespace. Done. > 2. Please put the useful standard warning label about enums that a recorded in > UMA (don't deleted or reorder or renumber; add new entry only at end) Done. > 3. Please explicitly do =0, =1, etc. Done. https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:126: UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.good_cert_seen", event, On 2015/04/17 16:20:36, Mark P wrote: > This sounds more like a BOOLEAN histogram. Why are you using an enum here? Do > you have the expectation that it needs to expand in the future? If so, how? In the past, when I've had boolean uma measurements, I've been asked to make an enum anyway for clarity in the reporting (see SSLCertificateDecisionsDidRevoke for example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). Personally, I like that approach as well, but if you feel strongly that it should be a straight boolean instead, let me know. https://codereview.chromium.org/1058003004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1058003004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12747: + exception for a bad certificate. Recorded any time a new connection is made On 2015/04/17 16:20:36, Mark P wrote: > This first sentence is a bit wrong (it doesn't track how often anything happens) > and slightly backwards. Perhaps something like this: > Records whether the user has already given an exception for a bad certificate on > this site. Hm, that's not entirely accurate either because it's only recorded when a good certificate is seen (not when the cert has an error). I've updated it to: "Records when a good certificate is seen after a user already gave an exception for a bad certificate for the same host."
https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:2150: EXPECT_GT(img_width, 100); This is an odd way to verify that something loaded successfully. Is this a pattern used elsewhere in tests? https://codereview.chromium.org/1058003004/diff/80001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/1058003004/diff/80001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:136: backend_->RevokeUserAllowExceptions(info->url().host()); is there a specific reason why this needs to be done regardless of the HasAllowException return value?
https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:2150: EXPECT_GT(img_width, 100); On 2015/04/17 19:58:03, felt wrote: > This is an odd way to verify that something loaded successfully. Is this a > pattern used elsewhere in tests? Yes, this is taken from the test "SSLUITest.TestUnsafeContents". https://codereview.chromium.org/1058003004/diff/80001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/1058003004/diff/80001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:136: backend_->RevokeUserAllowExceptions(info->url().host()); On 2015/04/17 19:58:03, felt wrote: > is there a specific reason why this needs to be done regardless of the > HasAllowException return value? Nope. Moved into the if block.
minor comments, otherwise histograms.xml lgtm https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/1058003004/diff/40001/content/browser/ssl/ssl... content/browser/ssl/ssl_policy.cc:126: UMA_HISTOGRAM_ENUMERATION("interstitial.ssl.good_cert_seen", event, On 2015/04/17 18:14:33, jww wrote: > On 2015/04/17 16:20:36, Mark P wrote: > > This sounds more like a BOOLEAN histogram. Why are you using an enum here? > Do > > you have the expectation that it needs to expand in the future? If so, how? > > In the past, when I've had boolean uma measurements, I've been asked to make an > enum anyway for clarity in the reporting (see SSLCertificateDecisionsDidRevoke > for example: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). > > Personally, I like that approach as well, but if you feel strongly that it > should be a straight boolean instead, let me know. Acknowledged. https://codereview.chromium.org/1058003004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1058003004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:13081: + Records when a good certificate is seen after a user already gave an This still sounds awkward. Consider Records -> Emitted after a -> whether the
https://codereview.chromium.org/1058003004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1058003004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:13081: + Records when a good certificate is seen after a user already gave an On 2015/04/17 20:26:19, Mark P wrote: > This still sounds awkward. Consider > Records -> Emitted > after a -> whether the Done.
https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:2150: EXPECT_GT(img_width, 100); On 2015/04/17 20:16:06, jww wrote: > On 2015/04/17 19:58:03, felt wrote: > > This is an odd way to verify that something loaded successfully. Is this a > > pattern used elsewhere in tests? > > Yes, this is taken from the test "SSLUITest.TestUnsafeContents". The brittleness of this test makes me a little squicky. How to make it better -- hm -- could you check that it's equal precisely 114 to remove at least that margin of error?
On 2015/04/17 20:38:55, felt wrote: > https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_browser_tests.cc:2150: EXPECT_GT(img_width, 100); > On 2015/04/17 20:16:06, jww wrote: > > On 2015/04/17 19:58:03, felt wrote: > > > This is an odd way to verify that something loaded successfully. Is this a > > > pattern used elsewhere in tests? > > > > Yes, this is taken from the test "SSLUITest.TestUnsafeContents". > > The brittleness of this test makes me a little squicky. How to make it better -- > hm -- could you check that it's equal precisely 114 to remove at least that > margin of error? No, I tried that. Unfortunately, it's not consistent. IMO, this test is no more brittle than TestUnsafeContents, so we're not really adding brittleness. But what I really care about is the script check anyway, so if you're really concerned, I'm happy to take out the img check entirely.
On 2015/04/17 21:31:22, jww wrote: > On 2015/04/17 20:38:55, felt wrote: > > > https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... > > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > > > > https://codereview.chromium.org/1058003004/diff/80001/chrome/browser/ssl/ssl_... > > chrome/browser/ssl/ssl_browser_tests.cc:2150: EXPECT_GT(img_width, 100); > > On 2015/04/17 20:16:06, jww wrote: > > > On 2015/04/17 19:58:03, felt wrote: > > > > This is an odd way to verify that something loaded successfully. Is this a > > > > pattern used elsewhere in tests? > > > > > > Yes, this is taken from the test "SSLUITest.TestUnsafeContents". > > > > The brittleness of this test makes me a little squicky. How to make it better > -- > > hm -- could you check that it's equal precisely 114 to remove at least that > > margin of error? > > No, I tried that. Unfortunately, it's not consistent. IMO, this test is no more > brittle than TestUnsafeContents, so we're not really adding brittleness. But > what I really care about is the script check anyway, so if you're really > concerned, I'm happy to take out the img check entirely. It's not consistent? Woah. That's... bizarre. OK, if TestUnsafeContents is not flaky then lgtm.
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1058003004/#ps120001 (title: "Updated histogram description")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1058003004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org, mpearson@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1058003004/#ps140001 (title: "Rebase on ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1058003004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
On 2015/04/17 23:44:03, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) Oops, just realized that I need OWNER approval for content/public/browser/ssl_host_state_delegate.h :-) davidben@, can you take a look? Thanks!
content lgtm with comments https://codereview.chromium.org/1058003004/diff/140001/content/browser/ssl/ss... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/1058003004/diff/140001/content/browser/ssl/ss... content/browser/ssl/ssl_policy.cc:36: END_OF_SSL_GOOD_CERT_SEEN_EVENT = 2 Nit: I think we'd normally call this SSL_GOOD_CERT_SEEN_EVENT_MAX https://codereview.chromium.org/1058003004/diff/140001/content/browser/ssl/ss... File content/browser/ssl/ssl_policy_backend.cc (right): https://codereview.chromium.org/1058003004/diff/140001/content/browser/ssl/ss... content/browser/ssl/ssl_policy_backend.cc:41: bool SSLPolicyBackend::HasAllowException(const std::string& host) { Should this account for a NULL delegate? The others do.
https://codereview.chromium.org/1058003004/diff/140001/content/browser/ssl/ss... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/1058003004/diff/140001/content/browser/ssl/ss... content/browser/ssl/ssl_policy.cc:36: END_OF_SSL_GOOD_CERT_SEEN_EVENT = 2 On 2015/04/21 18:09:00, David Benjamin wrote: > Nit: I think we'd normally call this SSL_GOOD_CERT_SEEN_EVENT_MAX Done. https://codereview.chromium.org/1058003004/diff/140001/content/browser/ssl/ss... File content/browser/ssl/ssl_policy_backend.cc (right): https://codereview.chromium.org/1058003004/diff/140001/content/browser/ssl/ss... content/browser/ssl/ssl_policy_backend.cc:41: bool SSLPolicyBackend::HasAllowException(const std::string& host) { On 2015/04/21 18:09:00, David Benjamin wrote: > Should this account for a NULL delegate? The others do. Yikes! Yup.
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org, mpearson@chromium.org, davidben@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1058003004/#ps180001 (title: "Rebase on ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1058003004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
jww@chromium.org changed reviewers: + torne@chromium.org
torne@chromium.org: Can you review the android_webview/ additions? Thanks!
android_webview LGTM
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org, mpearson@chromium.org, davidben@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1058003004/#ps240001 (title: "Yet Another Webview Fix (should be the last, I swear)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1058003004/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/5a586e5039cf30c3071b14a8e1df105f08c6b06a Cr-Commit-Position: refs/heads/master@{#326332} |
