Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2620203003:
Add initial version of captive portal list checking. (Closed)
DescriptionAdd initial version of captive portal list checking.
This CL adds captive portal certificate list checking feature. When an SSL
error occurs, the feature checks the certificate chain's SPKI hashes to a
list of hashes that are known to be served by captive portals. The list is
embedded as a resource and currently only contains a single hash (the hash
of the leaf cert of captive-portal.badssl.com). Follow up CLs will introduce
a component updater component to dynamically update the list of known captive
portal SPKI hashes.
BUG=640835
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2620203003
Cr-Commit-Position: refs/heads/master@{#448796}
Committed: https://chromium.googlesource.com/chromium/src/+/b0785808c1a3d64ec590bf17d41db45c7b0d8b16
Patch Set 1 #
Total comments: 29
Patch Set 2 : estark comments #
Total comments: 20
Patch Set 3 : Add more tests #Patch Set 4 : Fix build #Patch Set 5 : More tests and rebase #
Total comments: 20
Patch Set 6 : rsleevi comments #
Total comments: 19
Patch Set 7 : estark and rsleevi comments #
Total comments: 8
Patch Set 8 : bauerb comments #
Total comments: 2
Patch Set 9 : Fix Android tests #
Messages
Total messages: 43 (17 generated)
meacer@chromium.org changed reviewers: + estark@chromium.org
estark: This isn't yet ready for landing, but sending you to get early feedback. In particular, we shouldn't be loading the proto in the UI thread so that needs to be fixed. Let me know if the general idea looks okay.
Looks pretty reasonable to me! My main questions (about which I rambled inline) are: 1. Are we sure we're okay with checking every cert in the chain against the captive portal list, and 2. What happens on the captive portal interstitial when the landing URL is empty? https://codereview.chromium.org/2620203003/diff/1/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:629: <if expr="not is_android and not is_ios"> Can we not do the captive portal list on mobile for some reason? https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (left): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:1869: IN_PROC_BROWSER_TEST_F(SSLUITest, DISABLED_TestCNInvalidStickiness) { Not flaky anymore? https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:129: "sha256/2zCMVDKgnKec0721Sp1zVh2yiHeW/LJK4STkNnEa1og="; Hmmmmm. This is unfortunate, since okay.pem will get updated as it expires, etc. and it sucks for the person doing that to have to come here to update the hash. Maybe instead you can issue a request to the test server and grab the hash from the SSLInfo on that request... or something. Will think on that a little more. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:166: // Returns true if one of the cert hashes in |ssl_info| is of a captive portal Haven't read the implementation yet, but this makes it sound like we check every cert in the chain against the captive portal list. Is that intentional? I'm wondering if it's safer and equally effective to just check the leaf cert. (In case we somehow accidentally were to end up with an intermediate or root on the captive portal list.) .... or after reading a little below, maybe this means that every different hash algorithm of the leaf cert is checked in the captive portal list. So, plz clarify? https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:193: // SPKI hashes belonging to certs treated as captive portal portals. Populated nit: extra "portal" https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:259: for (const net::HashValue& hash_value : ssl_info.public_key_hashes) { Ok, now that I got down here, it looks like we're comparing the hashes of several algorithms for each cert in the chain. So I think the comment above should be clarified (line 166), and also want to make sure you're intending to check every cert in the chain. Is this mostly for convenience, so that we don't have to calculate the SPKI hashes of the leaf cert here? https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:323: std::string SSLErrorHandler::GetHistogramNameForTesting() { Ah, this is a good idea, I always end up repeating the histogram name literal in the tests... https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:359: g_config.Pointer()->IsKnownCaptivePortalCert(ssl_info_)) { Maybe this should be Finchable? It would be good to have a Finch feature as a kill switch, plus if we roll it out 50/50 we could see via UMA the difference in captive portal interstitials shown for users who are checking the captive portal list and those who aren't. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:361: ShowCaptivePortalInterstitial(GURL()); How will this work without an empty landing URL? Does that mean the Connect button (or whatever it's called) won't go anywhere? https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler_unittest.cc:564: EXPECT_EQ(1u, ssl_info().public_key_hashes.size()); nit: might want to add a test case for when there's more than one public_key_hash https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler_unittest.cc:579: base::RunLoop().RunUntilIdle(); This is a little confusing, why do we run the run loop and then check the same assertions again? (Maybe add a comment?)
Responding to some comments with no code changes. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:629: <if expr="not is_android and not is_ios"> On 2017/01/12 18:54:43, estark wrote: > Can we not do the captive portal list on mobile for some reason? I eventually want to enable this for mobile as well, but that complicates things a bit (e.g. the captive portal interstitial isn't compiled on mobile). So I was planning to do it as a follow up CL. Does that sound okay? https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (left): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:1869: IN_PROC_BROWSER_TEST_F(SSLUITest, DISABLED_TestCNInvalidStickiness) { On 2017/01/12 18:54:43, estark wrote: > Not flaky anymore? I was looking into re-enabling this in another CL. It failed once in about 200 runs. Keeping it disabled in this CL. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:166: // Returns true if one of the cert hashes in |ssl_info| is of a captive portal On 2017/01/12 18:54:43, estark wrote: > Haven't read the implementation yet, but this makes it sound like we check every > cert in the chain against the captive portal list. > > Is that intentional? I'm wondering if it's safer and equally effective to just > check the leaf cert. (In case we somehow accidentally were to end up with an > intermediate or root on the captive portal list.) It's intentional for this draft in the sense that I wanted to make sure leaf is the first or the last cert in the chain. If we can guarantee, we should just check the leaf. Though I wonder if there are any intermediates that we can also mark as captive portal (haven't seen any so far) > > .... > > > or after reading a little below, maybe this means that every different hash > algorithm of the leaf cert is checked in the captive portal list. So, plz > clarify? Yes, it is checking both hashes. This is another thing I wanted to clarify, do we always have both hashes? If so, we can always check the sha256 version. Also note that it's checking the serialized hashes via ToString(), rather than the actual hash. The reason is code simplicity: Since the hashes are put in a set, we'll have to pull in comparison operators for net::HashValue if we use the actual hashes. Let me know if you prefer using std::unorderd_set<net::HashValue> instead. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:259: for (const net::HashValue& hash_value : ssl_info.public_key_hashes) { On 2017/01/12 18:54:43, estark wrote: > Ok, now that I got down here, it looks like we're comparing the hashes of > several algorithms for each cert in the chain. So I think the comment above > should be clarified (line 166), and also want to make sure you're intending to > check every cert in the chain. Is this mostly for convenience, so that we don't > have to calculate the SPKI hashes of the leaf cert here? Can we not assume the first or the last two of the hashes belong to the leaf? If yes I don't think we need to re-calculate the hashes, right? https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:323: std::string SSLErrorHandler::GetHistogramNameForTesting() { On 2017/01/12 18:54:43, estark wrote: > Ah, this is a good idea, I always end up repeating the histogram name literal in > the tests... It always works until it doesn't. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:359: g_config.Pointer()->IsKnownCaptivePortalCert(ssl_info_)) { On 2017/01/12 18:54:43, estark wrote: > Maybe this should be Finchable? > > It would be good to have a Finch feature as a kill switch, plus if we roll it > out 50/50 we could see via UMA the difference in captive portal interstitials > shown for users who are checking the captive portal list and those who aren't. I was hoping to avoid another finch for this file, but being able to experiment sounds like a good idea. Will do this. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:361: ShowCaptivePortalInterstitial(GURL()); On 2017/01/12 18:54:43, estark wrote: > How will this work without an empty landing URL? Does that mean the Connect > button (or whatever it's called) won't go anywhere? Empty landing URL takes the user to the captive portal ping URL (www.gstatic.com/generate_204). If the captive portal is a proper one, it should then show its own login page there. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler_unittest.cc:579: base::RunLoop().RunUntilIdle(); On 2017/01/12 18:54:43, estark wrote: > This is a little confusing, why do we run the run loop and then check the same > assertions again? (Maybe add a comment?) This is in case SSLErrorHandler incorrectly starts a timer, in which case we'll want to wait until the end.
https://codereview.chromium.org/2620203003/diff/1/chrome/browser/browser_reso... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/browser_reso... chrome/browser/browser_resources.grd:629: <if expr="not is_android and not is_ios"> On 2017/01/12 23:54:02, Mustafa Emre Acer wrote: > On 2017/01/12 18:54:43, estark wrote: > > Can we not do the captive portal list on mobile for some reason? > > I eventually want to enable this for mobile as well, but that complicates things > a bit (e.g. the captive portal interstitial isn't compiled on mobile). So I was > planning to do it as a follow up CL. Does that sound okay? Yep, sgtm https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:166: // Returns true if one of the cert hashes in |ssl_info| is of a captive portal On 2017/01/12 23:54:03, Mustafa Emre Acer wrote: > On 2017/01/12 18:54:43, estark wrote: > > Haven't read the implementation yet, but this makes it sound like we check > every > > cert in the chain against the captive portal list. > > > > Is that intentional? I'm wondering if it's safer and equally effective to just > > check the leaf cert. (In case we somehow accidentally were to end up with an > > intermediate or root on the captive portal list.) > > It's intentional for this draft in the sense that I wanted to make sure leaf is > the first or the last cert in the chain. If we can guarantee, we should just > check the leaf. > > Though I wonder if there are any intermediates that we can also mark as captive > portal (haven't seen any so far) > > > > > .... > > > > > > or after reading a little below, maybe this means that every different hash > > algorithm of the leaf cert is checked in the captive portal list. So, plz > > clarify? > > Yes, it is checking both hashes. This is another thing I wanted to clarify, do > we always have both hashes? If so, we can always check the sha256 version. > > Also note that it's checking the serialized hashes via ToString(), rather than > the actual hash. The reason is code simplicity: Since the hashes are put in a > set, we'll have to pull in comparison operators for net::HashValue if we use the > actual hashes. Let me know if you prefer using std::unorderd_set<net::HashValue> > instead. Hmmm. From my reading of ssl_info.h, I would guess we can't assume anything about the order of the hashes or what algorithms are present. Nor do I see any nicely exposed way to calculate an SPKI hash. So I guess what you're doing is the best option, we should just make sure to only add leaf certs to the captive portal list. (Is there somewhere we can document that?) (I think comparing the strings is fine.)
Description was changed from ========== Add initial version of captive portal list checking. BUG=640835 ========== to ========== Add initial version of captive portal list checking. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The more I read about it, the more I feel it's okay to load the resource on the UI thread. This is still missing finch config, but I think otherwise is ready for a full review. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:129: "sha256/2zCMVDKgnKec0721Sp1zVh2yiHeW/LJK4STkNnEa1og="; On 2017/01/12 18:54:43, estark wrote: > Hmmmmm. This is unfortunate, since okay.pem will get updated as it expires, etc. > and it sucks for the person doing that to have to come here to update the hash. How often does it change? If it's, say, yearly, I don't think it's that big of a maintenance burden, as the test will catch the error. I added the commands to update the value. > > Maybe instead you can issue a request to the test server and grab the hash from > the SSLInfo on that request... or something. Will think on that a little more. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:166: // Returns true if one of the cert hashes in |ssl_info| is of a captive portal On 2017/01/13 23:40:23, estark wrote: > On 2017/01/12 23:54:03, Mustafa Emre Acer wrote: > > On 2017/01/12 18:54:43, estark wrote: > > > Haven't read the implementation yet, but this makes it sound like we check > > every > > > cert in the chain against the captive portal list. > > > > > > Is that intentional? I'm wondering if it's safer and equally effective to > just > > > check the leaf cert. (In case we somehow accidentally were to end up with an > > > intermediate or root on the captive portal list.) > > > > It's intentional for this draft in the sense that I wanted to make sure leaf > is > > the first or the last cert in the chain. If we can guarantee, we should just > > check the leaf. > > > > Though I wonder if there are any intermediates that we can also mark as > captive > > portal (haven't seen any so far) > > > > > > > > .... > > > > > > > > > or after reading a little below, maybe this means that every different hash > > > algorithm of the leaf cert is checked in the captive portal list. So, plz > > > clarify? > > > > Yes, it is checking both hashes. This is another thing I wanted to clarify, do > > we always have both hashes? If so, we can always check the sha256 version. > > > > Also note that it's checking the serialized hashes via ToString(), rather than > > the actual hash. The reason is code simplicity: Since the hashes are put in a > > set, we'll have to pull in comparison operators for net::HashValue if we use > the > > actual hashes. Let me know if you prefer using > std::unorderd_set<net::HashValue> > > instead. > > Hmmm. From my reading of ssl_info.h, I would guess we can't assume anything > about the order of the hashes or what algorithms are present. Nor do I see any > nicely exposed way to calculate an SPKI hash. So I guess what you're doing is > the best option, we should just make sure to only add leaf certs to the captive > portal list. (Is there somewhere we can document that?) > > (I think comparing the strings is fine.) Added a note to tls_error_assistant.proto. In the future, we can also do some validation while generating the binary version of that proto, but that will be complicated (e.g. will need to add the full PEM). https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:193: // SPKI hashes belonging to certs treated as captive portal portals. Populated On 2017/01/12 18:54:43, estark wrote: > nit: extra "portal" Done. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:259: for (const net::HashValue& hash_value : ssl_info.public_key_hashes) { On 2017/01/12 23:54:03, Mustafa Emre Acer wrote: > On 2017/01/12 18:54:43, estark wrote: > > Ok, now that I got down here, it looks like we're comparing the hashes of > > several algorithms for each cert in the chain. So I think the comment above > > should be clarified (line 166), and also want to make sure you're intending to > > check every cert in the chain. Is this mostly for convenience, so that we > don't > > have to calculate the SPKI hashes of the leaf cert here? > > Can we not assume the first or the last two of the hashes belong to the leaf? If > yes I don't think we need to re-calculate the hashes, right? Added sha256 check and clarified comment in line 166. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler.cc:361: ShowCaptivePortalInterstitial(GURL()); On 2017/01/12 23:54:03, Mustafa Emre Acer wrote: > On 2017/01/12 18:54:43, estark wrote: > > How will this work without an empty landing URL? Does that mean the Connect > > button (or whatever it's called) won't go anywhere? > > Empty landing URL takes the user to the captive portal ping URL > (http://www.gstatic.com/generate_204). If the captive portal is a proper one, it should > then show its own login page there. Actually, this was buggy (the text was broken). I'm now setting the URL explicitly. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler_unittest.cc:564: EXPECT_EQ(1u, ssl_info().public_key_hashes.size()); On 2017/01/12 18:54:44, estark wrote: > nit: might want to add a test case for when there's more than one > public_key_hash Changed this test to use multiple hashes. https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_erro... chrome/browser/ssl/ssl_error_handler_unittest.cc:579: base::RunLoop().RunUntilIdle(); On 2017/01/12 23:54:03, Mustafa Emre Acer wrote: > On 2017/01/12 18:54:43, estark wrote: > > This is a little confusing, why do we run the run loop and then check the same > > assertions again? (Maybe add a comment?) > > This is in case SSLErrorHandler incorrectly starts a timer, in which case we'll > want to wait until the end. Added a comment.
lgtm with a few comments/nits. Are you planning to add the Finch flag in this CL, or a follow-up? https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/1/chrome/browser/ssl/ssl_brow... chrome/browser/ssl/ssl_browser_tests.cc:129: "sha256/2zCMVDKgnKec0721Sp1zVh2yiHeW/LJK4STkNnEa1og="; On 2017/01/20 21:29:59, Mustafa Emre Acer wrote: > On 2017/01/12 18:54:43, estark wrote: > > Hmmmmm. This is unfortunate, since okay.pem will get updated as it expires, > etc. > > and it sucks for the person doing that to have to come here to update the > hash. > > How often does it change? If it's, say, yearly, I don't think it's that big of a > maintenance burden, as the test will catch the error. > > I added the commands to update the value. Yeah I think it's relatively rare, so including the commands seems like a good compromise. > > > > > Maybe instead you can issue a request to the test server and grab the hash > from > > the SSLInfo on that request... or something. Will think on that a little more. > https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:237: // Returns true if the timer has been started. nit: "timer" => "interstitial delay timer" (otherwise it mostly repeats the method name and isn't that useful) https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3911: // Navigate to an unsafe site. Proceed with interstitial page to indicate The "Proceed with..." part doesn't apply, right? (copy-paste error?) https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3924: // Check that the histogram for the captive portal cert was recorded. optional nit: I usually throw in an ExpectTotalCount() also to make sure no other buckets were recorded unexpectedly. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:143: bundle.GetRawDataResource(IDR_TLS_ERROR_ASSISTANT_PB).CopyToString(binary_pb); Could you explain more why it's okay to load it on the UI thread? https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:195: // SPKI hashes belonging to certs treated as captive portals. Populated the nit: "Populated" => "Null until" (to be clear that it can be null, not just an empty set) https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:45: // an SSL validation error. The display of the interstitial might be delayed by optional clarification request while you're here: this class also actually shows the interstitial, right? If so, I think it might be useful to add "and showing it" to the end of the first sentence. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:52: // If there is a name mismatch error and a corresponding suggested URL result Hmm, these lines 52-56 have rotted a bit; for example they don't mention how clock errors fit in. Consider rewriting as: Based on the result of these checks, SSLErrorHandler will show a customized interstitial, redirect to a different suggested URL, or, if all else fails, show the normal SSL interstitial. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:67: // Events for UMA. Public for testing. Since these are histogrammed, they should have an explict `= 0` on line 69 and the standard warning about not removing or reordering any of them. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:104: nit: no blank line here so that GetHistogramName is in the "Testing methods" block (or give them all blank lines) https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:593: histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(), ditto optional nit about adding an ExpectTotalCount() call
Oh, forgot to mention, before you land can you update the CL description with some more detail?
On 2017/01/21 01:33:27, estark wrote: > Oh, forgot to mention, before you land can you update the CL description with > some more detail? I was adding some more tests and now I'm in the middle of a large SSLErrorHandler refactoring. So probably not landing until next week :)
The CQ bit was checked by meacer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
PTAL? https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:237: // Returns true if the timer has been started. On 2017/01/20 23:31:18, estark wrote: > nit: "timer" => "interstitial delay timer" > (otherwise it mostly repeats the method name and isn't that useful) Done. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3911: // Navigate to an unsafe site. Proceed with interstitial page to indicate On 2017/01/20 23:31:18, estark wrote: > The "Proceed with..." part doesn't apply, right? (copy-paste error?) Done. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3924: // Check that the histogram for the captive portal cert was recorded. On 2017/01/20 23:31:18, estark wrote: > optional nit: I usually throw in an ExpectTotalCount() also to make sure no > other buckets were recorded unexpectedly. Done. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:143: bundle.GetRawDataResource(IDR_TLS_ERROR_ASSISTANT_PB).CopyToString(binary_pb); On 2017/01/20 23:31:18, estark wrote: > Could you explain more why it's okay to load it on the UI thread? I couldn't find an authoritative answer to this, but there are a couple of other places where this is done with an explicit DCHECK_CURRENTLY_ON(BrowserThread::UI): - ChromeOSCreditsHandler::StartOnUIThread: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/about_ui.cc?rcl=... - NTPResourceCache::GetNewTabHTML which eventually calls https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/ntp/ntp_resource... Not to mention, we already load html templates on the UI thread at SecurityInterstitialPage::GetHTMLContents. It seems most places that call GetRawDataResource do so to load a resource that eventually gets displayed in the UI, and they do it in the UI thread. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:195: // SPKI hashes belonging to certs treated as captive portals. Populated the On 2017/01/20 23:31:18, estark wrote: > nit: "Populated" => "Null until" (to be clear that it can be null, not just an > empty set) Done. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:45: // an SSL validation error. The display of the interstitial might be delayed by On 2017/01/20 23:31:18, estark wrote: > optional clarification request while you're here: this class also actually shows > the interstitial, right? If so, I think it might be useful to add "and showing > it" to the end of the first sentence. Done. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:52: // If there is a name mismatch error and a corresponding suggested URL result On 2017/01/20 23:31:18, estark wrote: > Hmm, these lines 52-56 have rotted a bit; for example they don't mention how > clock errors fit in. Consider rewriting as: > > Based on the result of these checks, SSLErrorHandler will show a customized > interstitial, redirect to a different suggested URL, or, if all else fails, show > the normal SSL interstitial. Done, thanks for the suggestion. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:67: // Events for UMA. Public for testing. On 2017/01/20 23:31:18, estark wrote: > Since these are histogrammed, they should have an explict `= 0` on line 69 and > the standard warning about not removing or reordering any of them. Done. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:104: On 2017/01/20 23:31:18, estark wrote: > nit: no blank line here so that GetHistogramName is in the "Testing methods" > block > > (or give them all blank lines) Done. https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:593: histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(), On 2017/01/20 23:31:18, estark wrote: > ditto optional nit about adding an ExpectTotalCount() call Done.
meacer@chromium.org changed reviewers: + rsleevi@chromium.org
Added some more tests and rebased. rsleevi: Can you also please take a second look? Thanks.
A quick scan, but I mostly skipped the tests this round https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/ssl/ssl_error_assistant/ssl_error_assistant.asciipb (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/resourc... chrome/browser/resources/ssl/ssl_error_assistant/ssl_error_assistant.asciipb:16: sha256_hash: "sha256/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=" Is this an example? Or test data? I mean, it's technically possible for this to happen, however unwise. (I mention this because I know of at least two bugs that assumed "magic test value will never happen in the wild" which, given probability, ended up happening in the wild) https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:134: // Sha256 fingerprint of okay.pem's Subject Public Key Information. s/okay.pem/ok_cert.pem/ Hardcoding to ok_cert.pem makes it annoying to re-generate certs (e.x. https://bugs.chromium.org/p/chromium/issues/detail?id=249850 ) and ended up tripping people up. As much as possible, it'd be great to avoid hardcoding it (making it programatically computed), or making it automatically update whenever ok_cert.pem is regenerated. Thoughts? https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_assistant.proto (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_assistant.proto:17: // NOTE: Only leaf certs must be added here. When I read this outloud, it sounds weird, but it may be totally valid. Alternative wordings might be NOTE: Only leaf certificates are supported. NOTE: Certificates other than leaf certificates are not supported. Basically, I'm trying to parse the "must" here, to understand what happens if that must is violated (e.g. does it crash or is it just not supported) https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:210: std::unique_ptr<std::unordered_set<std::string>> captive_portal_spki_hashes_; Have you considered making/requiring the PB be sorted? That would/should allow you to avoid allocating at all for this (you could search the pb w/o the CopyToString) Although I'm not sure how much the cost is to load a resource, so maybe the savings isn't as great... https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:275: LoadCaptivePortalCertHashes(error_assistant_proto); Can resources be loaded on the UI thread? I thought they had to go through the File thread? https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:291: captive_portal_spki_hashes_->end()) { nit: I tend to encourage writing fewer compound conditionals, if it helps clarity if (hash_value.tag != net::HASH_VALUE_SHA256) { continue; } if (...->find != ...->end()) { return true; } But that's a nit pedantry, not a must-fix :) https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:114: static void ResetConfigForTesting(); Usually when I see a much of ForTesting() (or FRIEND_TEST_ALL_PREFIXES), it's a sign that abstraction details might be leaking or the class might be doing too much. static methods are also usually a warning sign that there's too much intrinsic state - hidden globals and the like. I don't think it's grounds to block this CL, but maybe give a noodle about this class and how it might be better tested using public interfaces. I don't think this is urgent, but I think the growth here might be a sign of growing complexity that could/should be simplified. I also don't always practice what I preach here - because I'm a big fan of hiding details from public interfaces - so I realize it's a tough balance to find :) https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:194: template <net::CertStatus kCertStatus> I haven't really seen template parameters follow the constant naming scheme of k*, even if they do end up compile-time constants. That feels a little weird, but more of a nit than a hard block - it may very well be the thing-with-precedent and I just haven't seen it. Perhaps consider parameter-naming style? https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:202: SSLErrorHandler::ResetConfigForTesting(); Shouldn't you also be doing this in TearDown? https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:265: SSLErrorHandler::ResetConfigForTesting(); Shouldn't you also be doing this in TearDown?
https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/ssl/ssl_error_assistant/ssl_error_assistant.asciipb (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/resourc... chrome/browser/resources/ssl/ssl_error_assistant/ssl_error_assistant.asciipb:16: sha256_hash: "sha256/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=" On 2017/02/01 22:06:06, Ryan Sleevi wrote: > Is this an example? Or test data? I mean, it's technically possible for this to > happen, however unwise. > > (I mention this because I know of at least two bugs that assumed "magic test > value will never happen in the wild" which, given probability, ended up > happening in the wild) It's magic test data and I thought we would publish if we ever hit this in the wild :) But in all seriousness, it's not needed anymore now that we have a test site, so removed. https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:134: // Sha256 fingerprint of okay.pem's Subject Public Key Information. On 2017/02/01 22:06:06, Ryan Sleevi wrote: > s/okay.pem/ok_cert.pem/ > > Hardcoding to ok_cert.pem makes it annoying to re-generate certs (e.x. > https://bugs.chromium.org/p/chromium/issues/detail?id=249850 ) and ended up > tripping people up. As much as possible, it'd be great to avoid hardcoding it > (making it programatically computed), or making it automatically update whenever > ok_cert.pem is regenerated. > > Thoughts? estark mentioned this too. I'm now computing this from the server's cert. https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_assistant.proto (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_assistant.proto:17: // NOTE: Only leaf certs must be added here. On 2017/02/01 22:06:06, Ryan Sleevi wrote: > When I read this outloud, it sounds weird, but it may be totally valid. > > Alternative wordings might be > > NOTE: Only leaf certificates are supported. > NOTE: Certificates other than leaf certificates are not supported. > > Basically, I'm trying to parse the "must" here, to understand what happens if > that must is violated (e.g. does it crash or is it just not supported) Technically intermediates and roots can also be added, and they will trigger a captive portal interstitial. The warning was to discourage doing that, but since it's also not an invalid thing to do not sure if unsupported is the right word -- it seems possible to me that a captive portal vendor owns an intermediate and that we decide to add it here? https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:210: std::unique_ptr<std::unordered_set<std::string>> captive_portal_spki_hashes_; On 2017/02/01 22:06:06, Ryan Sleevi wrote: > Have you considered making/requiring the PB be sorted? That would/should allow > you to avoid allocating at all for this (you could search the pb w/o the > CopyToString) > > Although I'm not sure how much the cost is to load a resource, so maybe the > savings isn't as great... I wanted to avoid requiring the PB be sorted because of the additional presubmit checks it would require and the binary search code that will run on the PB's fields. I'm also not sure how we can get rid of the CopyToString since it'll still be required to load the proto, maybe I'm missing something? https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:275: LoadCaptivePortalCertHashes(error_assistant_proto); On 2017/02/01 22:06:06, Ryan Sleevi wrote: > Can resources be loaded on the UI thread? I thought they had to go through the > File thread? This was going to be one of my main questions to you (previous discussion with estark here: https://codereview.chromium.org/2620203003/diff/20001/chrome/browser/ssl/ssl_...). It looks like there are various places where resources are loaded on the UI thread, and it seems to make sense to do so given that they are eventually used on the UI thread (e.g. we already do this in SecurityInterstitialPage::GetHTMLContents), so I couldn't convince myself that this needs to hop onto the FILE thread. https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:291: captive_portal_spki_hashes_->end()) { On 2017/02/01 22:06:06, Ryan Sleevi wrote: > nit: I tend to encourage writing fewer compound conditionals, if it helps > clarity > > if (hash_value.tag != net::HASH_VALUE_SHA256) { > continue; > } > if (...->find != ...->end()) { > return true; > } > > But that's a nit pedantry, not a must-fix :) Done. https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:114: static void ResetConfigForTesting(); On 2017/02/01 22:06:06, Ryan Sleevi wrote: > Usually when I see a much of ForTesting() (or FRIEND_TEST_ALL_PREFIXES), it's a > sign that abstraction details might be leaking or the class might be doing too > much. static methods are also usually a warning sign that there's too much > intrinsic state - hidden globals and the like. > > I don't think it's grounds to block this CL, but maybe give a noodle about this > class and how it might be better tested using public interfaces. I don't think > this is urgent, but I think the growth here might be a sign of growing > complexity that could/should be simplified. I also don't always practice what I > preach here - because I'm a big fan of hiding details from public interfaces - > so I realize it's a tough balance to find :) Agreed, this class accumulated some cruft since it was introduced for the captive portal interstitial. Currently it's tricky to properly test without exposing its internals as it's only created between the error and the interstitial, which is also why it has a singleton config. I've been meaning to refactor it but will have to think more about how to do it properly. https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:194: template <net::CertStatus kCertStatus> On 2017/02/01 22:06:06, Ryan Sleevi wrote: > I haven't really seen template parameters follow the constant naming scheme of > k*, even if they do end up compile-time constants. That feels a little weird, > but more of a nit than a hard block - it may very well be the > thing-with-precedent and I just haven't seen it. > > Perhaps consider parameter-naming style? Wasn't sure about it either so I did a code search before sending the CL, but now that I look at it again all the places that use this style are either blink or v8, so changing back to parameter style. https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:202: SSLErrorHandler::ResetConfigForTesting(); On 2017/02/01 22:06:06, Ryan Sleevi wrote: > Shouldn't you also be doing this in TearDown? Done. https://codereview.chromium.org/2620203003/diff/100001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:265: SSLErrorHandler::ResetConfigForTesting(); On 2017/02/01 22:06:06, Ryan Sleevi wrote: > Shouldn't you also be doing this in TearDown? Done.
LGTM (and largely deferring to estark's review, although I did review all the files) https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:345: SSLErrorHandler::ResetConfigForTesting(); I believe you also should do this in TearDown(), although it's mostly pedantry in a world of isolated tests (just clearer symmetry) Alternatively, a scoped resetter would also work, but that's probably more hassle :) https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:4014: // predefined certificates. You could use a MockCertVerifyProc to return any cert you want as a 'verified' cert, but Emily may know more if that doesn't work :) https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:285: captive_portal_spki_hashes_ = LoadCaptivePortalCertHashes(proto); The zero-copy solution I was suggesting previously was some combination of base::StringPiece data = bundle.GetRawDataResource(IDR_SSL_ERROR_ASSISTANT_PB); ArrayInputStream stream(data.data(), data.size()); SSLErrorAssistantConfig proto; proto.ParseFromZeroCopyStream(&stream); https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:292: if (captive_portal_spki_hashes_->find(hash_value.ToString()) != If you wanted to avoid forcing every hash to be string-coerced, you could namespace std { template <> struct hash<net::SHA256HashValue> { size_t operator()(const net::SHA256HashValue& hash) { return base::StringPieceHash()(base::StringPiece(hash.data(), hash.size())); } } } // namespace std std::unique_ptr<std::unordered_set<net::SHA256HashValue>> captive_portal_spki_hashes_; I mean, the hash function is super-grotty, but it avoids copying (which is why not forwarding to std::hash<string>()). Mostly, it's a question of how often you expect this to be called - and given that it's only for errors, it may not be worth optimizing. But at least worth calling out if you wanted :)
Sorry to be so slow! lgtm https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3927: IN_PROC_BROWSER_TEST_F(SSLUITest, CaptivePortalCertificateList_Disabled) { optional nit: for this and the other tests, I like having a one-sentence description of the test above this line, e.g. "Tests that the captive portal certificate list is not used when the feature is disabled via Finch." (especially in this file where there are gazillions of tests... maybe we should look at splitting them up/organizing them at some point) https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3929: scoped_feature_list.InitFromCommandLine( optional nit: I'd find InitAndDisableFeature() a bit easier to read (here and below) https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:4013: // embeded test server, but the test server can only serve a limited number of typo: embeded -> embedded https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:4014: // predefined certificates. On 2017/02/02 02:24:36, Ryan Sleevi wrote: > You could use a MockCertVerifyProc to return any cert you want as a 'verified' > cert, but Emily may know more if that doesn't work :) I had originally suggested a MockCertVerifier to Mustafa, but I walked that back because I had a vague memory that it was against the rules for a CertVerifier to return a leaf cert that is different than the one passed in. (i.e. it would violate the CertVerifyResult guarantee at [1]) But now that I'm looking in this file, I see pre-existing examples of MockCertVerifiers returning certs different than what was passed in, so I guess it's not a hard-and-fast rule. [1] https://cs.chromium.org/chromium/src/net/cert/cert_verify_result.h?q=CertVeri... https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:4015: template <int net_error> Just curious, why templatize the test fixture instead of making it a parameterized test? Is it because the expectations are different depending on the error so you don't want to choose different expectations based on the parameter value? https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:45: // for an SSL validation error and actually display it. The display of the nit: display => displaying
https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:345: SSLErrorHandler::ResetConfigForTesting(); On 2017/02/02 02:24:36, Ryan Sleevi wrote: > I believe you also should do this in TearDown(), although it's mostly pedantry > in a world of isolated tests (just clearer symmetry) > > Alternatively, a scoped resetter would also work, but that's probably more > hassle :) Done. https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3927: IN_PROC_BROWSER_TEST_F(SSLUITest, CaptivePortalCertificateList_Disabled) { On 2017/02/03 07:24:07, estark wrote: > optional nit: for this and the other tests, I like having a one-sentence > description of the test above this line, e.g. "Tests that the captive portal > certificate list is not used when the feature is disabled via Finch." > (especially in this file where there are gazillions of tests... maybe we should > look at splitting them up/organizing them at some point) Done. https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3929: scoped_feature_list.InitFromCommandLine( On 2017/02/03 07:24:07, estark wrote: > optional nit: I'd find InitAndDisableFeature() a bit easier to read (here and > below) I initially wanted to use it too, but it requires exposing the feature in SSLErrorHandler. That would normally be okay, but the feature is protected by BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION) which will require pulling a bunch of headers (feature list and buildflags) to SSLErrorHandler header. I kind of want to avoid that, but don't feel strongly if you really want this :) https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:4013: // embeded test server, but the test server can only serve a limited number of On 2017/02/03 07:24:07, estark wrote: > typo: embeded -> embedded Done. https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:4014: // predefined certificates. On 2017/02/03 07:24:07, estark wrote: > On 2017/02/02 02:24:36, Ryan Sleevi wrote: > > You could use a MockCertVerifyProc to return any cert you want as a 'verified' > > cert, but Emily may know more if that doesn't work :) > > I had originally suggested a MockCertVerifier to Mustafa, but I walked that back > because I had a vague memory that it was against the rules for a CertVerifier to > return a leaf cert that is different than the one passed in. (i.e. it would > violate the CertVerifyResult guarantee at [1]) > > But now that I'm looking in this file, I see pre-existing examples of > MockCertVerifiers returning certs different than what was passed in, so I guess > it's not a hard-and-fast rule. > > [1] > https://cs.chromium.org/chromium/src/net/cert/cert_verify_result.h?q=CertVeri... Converted the tests to use MockCertVerifier. https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:4015: template <int net_error> On 2017/02/03 07:24:07, estark wrote: > Just curious, why templatize the test fixture instead of making it a > parameterized test? Is it because the expectations are different depending on > the error so you don't want to choose different expectations based on the > parameter value? Parametrizing this class added complexity: - If we keep this class as is and make the interceptor take a net_error parameter, we'd need to hold a reference to the interceptor and post the desired net_error parameter to the IO thread. - The other option is to pass net_error to the test's constructor, which would require two different test classes along with their constructors. The current implementation avoids adding that extra code (it typedefs two classes instead, saving a whopping 5 lines of code). This is moot now that the test is using MockCertVerifier. https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:285: captive_portal_spki_hashes_ = LoadCaptivePortalCertHashes(proto); On 2017/02/02 02:24:37, Ryan Sleevi wrote: > The zero-copy solution I was suggesting previously was some combination of > > base::StringPiece data = bundle.GetRawDataResource(IDR_SSL_ERROR_ASSISTANT_PB); > ArrayInputStream stream(data.data(), data.size()); > SSLErrorAssistantConfig proto; > proto.ParseFromZeroCopyStream(&stream); Done, thanks for pointing to this. https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:292: if (captive_portal_spki_hashes_->find(hash_value.ToString()) != On 2017/02/02 02:24:36, Ryan Sleevi wrote: > If you wanted to avoid forcing every hash to be string-coerced, you could > > namespace std { > template <> > struct hash<net::SHA256HashValue> { > size_t operator()(const net::SHA256HashValue& hash) { > return base::StringPieceHash()(base::StringPiece(hash.data(), hash.size())); > } > } > } // namespace std > > std::unique_ptr<std::unordered_set<net::SHA256HashValue>> > captive_portal_spki_hashes_; > > I mean, the hash function is super-grotty, but it avoids copying (which is why > not forwarding to std::hash<string>()). Mostly, it's a question of how often you > expect this to be called - and given that it's only for errors, it may not be > worth optimizing. But at least worth calling out if you wanted :) I looked into providing a hash function, but was a bit wary of adding it to std namespace because I wasn't sure if it's frowned up in Chrome codebase :) (there aren't any examples under chrome/, after all) I think I'll leave this as is, given that it's only done for errors. (And just for completeness' sake: Another option is to use binary search and pull HashToArrayComparator out from cert_verify_proc.cc for that purpose.) https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/2620203003/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:45: // for an SSL validation error and actually display it. The display of the On 2017/02/03 07:24:07, estark wrote: > nit: display => displaying Done.
estark: You might want to take a look at the new tests in ssl_browser_tests.cc since they are now using MockCertVerifier.
meacer@chromium.org changed reviewers: + bauerb@chromium.org
bauerb: PTAL at browser_resources.grd? Thanks.
LGTM w/ nits: https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:117: #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION) Nit: Move this before USE_NSS_CERTS so the #ifdef conditions are alphabetical? https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:292: EXPECT_TRUE( ASSERT_TRUE? If this fails, the subsequent checks are not going to be of much use either. https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3938: "" /* enabled */, "CaptivePortalCertificateList" /* disabled */); Nit: Use an empty std::string() constructor to save a (surprisingly large) number of instructions. https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:154: std::unique_ptr<std::unordered_set<std::string>> hashes( Nit: auto hashes = base::MakeUnique<std::unordered_set<std::string>> so you save one mention of the type? Or really, just return the set by value? With return value optimizations and std::move(), this shouldn't actually make copies.
https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:117: #if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION) On 2017/02/07 10:53:03, Bernhard Bauer wrote: > Nit: Move this before USE_NSS_CERTS so the #ifdef conditions are alphabetical? Done. https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:292: EXPECT_TRUE( On 2017/02/07 10:53:03, Bernhard Bauer wrote: > ASSERT_TRUE? If this fails, the subsequent checks are not going to be of much > use either. Can't ASSERT_TRUE here since the function returns a value and ASSERT_TRUE tries to return early from the function. https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3938: "" /* enabled */, "CaptivePortalCertificateList" /* disabled */); On 2017/02/07 10:53:03, Bernhard Bauer wrote: > Nit: Use an empty std::string() constructor to save a (surprisingly large) > number of instructions. Done. https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/2620203003/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:154: std::unique_ptr<std::unordered_set<std::string>> hashes( On 2017/02/07 10:53:03, Bernhard Bauer wrote: > Nit: auto hashes = base::MakeUnique<std::unordered_set<std::string>> so you save > one mention of the type? Or really, just return the set by value? With return > value optimizations and std::move(), this shouldn't actually make copies. auto'ed. Returning by value wouldn't work, since this needs to be assigned to a unique ptr, right? (would need to reset captive_portal_spki_hashes_ separately and then assign to this value) Or am I misunderstanding?
latest ssl_browser_tests.cc lgtm https://codereview.chromium.org/2620203003/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:4068: mock_cert_verifier()->AddResultForCert(cert, verify_result, net_result); Ohh, I was confused about this, I thought you needed to return a cert that was different than the cert that was being verified... this makes much more sense, so ignore everything I said about MockCertVerifier, ever.
https://codereview.chromium.org/2620203003/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2620203003/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:4068: mock_cert_verifier()->AddResultForCert(cert, verify_result, net_result); On 2017/02/07 21:07:58, estark wrote: > Ohh, I was confused about this, I thought you needed to return a cert that was > different than the cert that was being verified... this makes much more sense, > so ignore everything I said about MockCertVerifier, ever. I see what you meant. Yeah, serving the same cert seems to work fine.
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2620203003/#ps160001 (title: "bauerb comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, rsleevi@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2620203003/#ps180001 (title: "Fix Android tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by meacer@chromium.org
Description was changed from ========== Add initial version of captive portal list checking. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add initial version of captive portal list checking. This CL adds captive portal certificate list checking feature. When an SSL error occurs, the feature checks the certificate chain's SPKI hashes to a list of hashes that are known to be served by captive portals. The list is embedded as a resource and currently only contains a single hash (the hash of the leaf cert of captive-portal.badssl.com). Follow up CLs will introduce a component updater component to dynamically update the list of known captive portal SPKI hashes. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486506810401390,
"parent_rev": "93fa43b36cf37af0b142fbe70d7e1cc3f05bca33", "commit_rev":
"b0785808c1a3d64ec590bf17d41db45c7b0d8b16"}
Message was sent while issue was closed.
Description was changed from ========== Add initial version of captive portal list checking. This CL adds captive portal certificate list checking feature. When an SSL error occurs, the feature checks the certificate chain's SPKI hashes to a list of hashes that are known to be served by captive portals. The list is embedded as a resource and currently only contains a single hash (the hash of the leaf cert of captive-portal.badssl.com). Follow up CLs will introduce a component updater component to dynamically update the list of known captive portal SPKI hashes. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add initial version of captive portal list checking. This CL adds captive portal certificate list checking feature. When an SSL error occurs, the feature checks the certificate chain's SPKI hashes to a list of hashes that are known to be served by captive portals. The list is embedded as a resource and currently only contains a single hash (the hash of the leaf cert of captive-portal.badssl.com). Follow up CLs will introduce a component updater component to dynamically update the list of known captive portal SPKI hashes. BUG=640835 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2620203003 Cr-Commit-Position: refs/heads/master@{#448796} Committed: https://chromium.googlesource.com/chromium/src/+/b0785808c1a3d64ec590bf17d41d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/b0785808c1a3d64ec590bf17d41d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
