Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14125003:
Do not roll back to SSL 3.0 for Google properties. (Closed)
|
Created:
7 years, 8 months ago by thaidn_google Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, balfanz_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDo not roll back to SSL 3.0 for Google properties.
SSL 3.0 fallback for Google properties can be enabled again with --enable-unrestricted-ssl3-fallback.
Delete the obsolete SSL 3.0 fallback on TLS decompression failure.
BUG=230171
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195335
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add command line switches to control the enforcement and learning of minimum SSL versions. #Patch Set 3 : Remove learning mode. Enforce TLS for Google's properties. #
Total comments: 21
Patch Set 4 : disable #
Total comments: 26
Patch Set 5 : Adress rsleevi and agl's comments. #Patch Set 6 : Cleanup #
Total comments: 20
Patch Set 7 : Address rsleevi's comments. #Patch Set 8 : Cleanup before sending for reviews. #
Total comments: 17
Patch Set 9 : Final touch. #Patch Set 10 : Address wtc's comments. #Patch Set 11 : Cleanup before landing(?). #
Total comments: 10
Patch Set 12 : Change flag to --enable-ssl3-fallback #Patch Set 13 : Fix a bug that prevents TLS 1.1 -> TLS 1.0 fallback. #
Total comments: 28
Patch Set 14 : Change switch to --enable-unrestricted-ssl3-fallback; Remove obsolete TLS decompression test cases. #
Total comments: 6
Patch Set 15 : Cleanup. #Messages
Total messages: 40 (0 generated)
Hi Adam, This CL implements "The client MUST NOT roll back to an older version than the server has indicated, even if the connection handshake failed. That is, if the server indicates support for TLS 1.1, but the connection fails, then the client MUST NOT attempt to connect to the server using TLS 1.0, but allow the connection to fail." While it looks like Pettersen suggests that client only remembers server's version until the handshake is successful, I decide to make it one step further: Chrome will persist server's version as a part of HSTS. For current preloaded HSTS domains I use TLSv1.0 as the minimum version for Google's properties, and use SSLv3.0 for the rest. For these domains Chrome would never update the preloaded versions, regardless of versions used in handshakes. That means even if Chrome uses TLSv1.1 to connect to www.google.com, the minimum version that it would use for new connections is still TLSv1.0. For non-preloaded domains Chrome will extract the maximum version from successful handshakes, and use it as the minimum protocol for subsequent connections. There is a risk of DoS here: if Chrome extracts version from an unauthenticated handshake session (i.e., before it gets and verifies Finished) then MITM attackers can set the version to TLSv1.2, and deny user access to any non TLSv1.2 servers permanently. Currently I extract the version within SSLClientSocketNSS::DoVerifyCertComplete, but I don't know if the handshake is finished at that point. Please advise. Anyway if you think this CL is too radical, I can revise it to Pettersen's original suggestion. Cheers Thai.
Some explanations and questions. https://codereview.chromium.org/14125003/diff/1/chrome/browser/net/transport_... File chrome/browser/net/transport_security_persister.h (right): https://codereview.chromium.org/14125003/diff/1/chrome/browser/net/transport_... chrome/browser/net/transport_security_persister.h:80: // "ssl_version": integer should be ssl_version_min. will fix in another patchset. https://codereview.chromium.org/14125003/diff/1/net/http/transport_security_s... File net/http/transport_security_state.h (right): https://codereview.chromium.org/14125003/diff/1/net/http/transport_security_s... net/http/transport_security_state.h:287: friend class SSLClientSocketNSS; This is ugly. Should I make |EnableHost| public? https://codereview.chromium.org/14125003/diff/1/net/http/transport_security_s... File net/http/transport_security_state_static.h (right): https://codereview.chromium.org/14125003/diff/1/net/http/transport_security_s... net/http/transport_security_state_static.h:383: {25, true, "\013pinningtest\007appspot\003com", false, kTestPins, DOMAIN_APPSPOT_COM, SSL_CONNECTION_VERSION_TLS1 }, Some lines have been already longer than 80 chars. I don't make them worse.
This is a fairly aggressive change, but perhaps it would be interesting to dip our toe into the dev channel with it. At a high level, I would drop the persistence changes in this CL: don't try to learn minimum versions at all, just use the preloaded table for now. I think that will remove half the code and most of the complexity. I'm going to avoid a review on the small scale for now in light of the annoying, large changes that I've suggested :) I also wonder about a command line flag to disable this so that we can ask bug reporters to try with the flag and see if that solves their problem. https://codereview.chromium.org/14125003/diff/1/chrome/browser/net/transport_... File chrome/browser/net/transport_security_persister.cc (right): https://codereview.chromium.org/14125003/diff/1/chrome/browser/net/transport_... chrome/browser/net/transport_security_persister.cc:82: const char kSSLVersion30[] = "sslv3.0"; We already have a convention for these strings I'm afraid: "ssl3", "tls1", "tls1.1", "tls1.2".
Adam, I've added a switch to control the enforcement of minimum SSL versions for preloaded domains. I don't remove the learning versions code, but I turn it off with another switch. I think it may be useful to map this switch (and the above as well) to some flag in configuration policy, and turn it on for Googlers to see how it affects us. This CL is more complex than I expect (and it is a somewhat steep learning curve for me :-P), so please let me know if you have any suggestions to make it simpler (rather than removing the learning mode). Thanks! Thai.
Since it's unclear whether requiring >= TLS 1.0 for Google sites is feasible, this isn't the first step. The first step would just involve something like the net/http/http_network_transaction.[cc|h] change and the command line flag. I didn't think of this yesterday, but we don't need to change the preloaded JSON at this point either - we can just say that sites with preloaded pins need >= TLS 1.0. That sort of minimal change can run in dev/beta and we'll see how much breakage results before doing anything else.
On 2013/04/12 14:32:52, agl wrote: > Since it's unclear whether requiring >= TLS 1.0 for Google sites is feasible, > this isn't the first step. > > The first step would just involve something like the > net/http/http_network_transaction.[cc|h] change and the command line flag. I > didn't think of this yesterday, but we don't need to change the preloaded JSON > at this point either - we can just say that sites with preloaded pins need >= > TLS 1.0. > > That sort of minimal change can run in dev/beta and we'll see how much breakage > results before doing anything else. Okay. I don't think we really want to enforce TLS for all preloaded sites. Maybe just for Google's properties? I will have a PS ready soon.
On Fri, Apr 12, 2013 at 4:29 PM, <thaidn@google.com> wrote: > Okay. I don't think we really want to enforce TLS for all preloaded sites. > Maybe > just for Google's properties? Sure. It would be possible to test whether it's a Google property by testing the pin values for now, rather than adding a value to the JSON. Cheers AGL
Adam, I've made the changes that you suggested. Please review PS 3. Cheers Thai.
https://codereview.chromium.org/14125003/diff/19001/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right): https://codereview.chromium.org/14125003/diff/19001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:227: // * |ssl_version_min_learning_enabled| is false. This comment line looks to be a left-over. https://codereview.chromium.org/14125003/diff/19001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:266: bool tmp; I'd use a real variable name rather than just |tmp| - we do above. https://codereview.chromium.org/14125003/diff/19001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:322: bool tmp; ditto. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction.cc:187: server_ssl_config_.version_fallback; I don't think version_fallback makes sense here. It's not like we can have fallen back at this point. |sni_available| is designed to remove HSTS requirements when SNI isn't sent (i.e. SSLv3) and we didn't want to do that under the attackers control - i.e. when falling back. In this case, since we're setting TLSv1 to be the minimum version then SNI is always sent. Thus I think this should always just be true. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction.h:89: SSLConfig& server_ssl_config() { return server_ssl_config_; } server_ssl_config_for_testing() (If it's a testing function then let's make it really clear.) Alternatively, it looks like the style here is to use FRIEND_TEST_ALL_PREFIXES for the tests. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:28: class SimpleProxyConfigService : public ProxyConfigService { Looks like this is the third copy of this. Probably time to give it it's own file and header. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:57: public: indentation of this class is incorrect. https://codereview.chromium.org/14125003/diff/19001/net/ssl/ssl_config_servic... File net/ssl/ssl_config_service.cc (right): https://codereview.chromium.org/14125003/diff/19001/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.cc:190: new_config.disabled_cipher_suites) || This looks like a stray space.
p.s. needs a change description.
Chris makes a good point on the bug: there may be MITM proxies that only implement SSLv3. Preventing fallback would allow them to continue to work, but setting the minimum version breaks them. Perhaps the change should disable fallback rather than change the min version (grep the code for where version_fallback is set).
https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction.cc:190: if (TransportSecurityState::IsGooglePinnedProperty(host, sni_available)) { nit: net/ style is to omit braces on one-line if statements. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction.h:89: SSLConfig& server_ssl_config() { return server_ssl_config_; } On 2013/04/15 15:23:51, agl wrote: > server_ssl_config_for_testing() > > (If it's a testing function then let's make it really clear.) > > Alternatively, it looks like the style here is to use FRIEND_TEST_ALL_PREFIXES > for the tests. +1 to either FRIEND_TEST_ALL_PREFIXES or friending the test fixture and adding a protected getter to the test fixture for the individual tests. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:110: SSL_PROTOCOL_VERSION_TLS1); style: EXPECT_* macros follow the EXPECT_*(expected, actual), but your EXPECT_EQ for SSL_PROTOCOL_VERSION* are in the form of EXPECT_*(actual, expected). https://codereview.chromium.org/14125003/diff/19001/net/ssl/ssl_config_service.h File net/ssl/ssl_config_service.h (right): https://codereview.chromium.org/14125003/diff/19001/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.h:103: // preloaded HSTS entries. Comment nit: Chromium comments discourage the use of pronouns - see https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... "True if the enforcement of the minimum SSL/TLS version for preloaded HSTS entries is disabled."
Thanks for the reviews, Adam and Ryan! I've made the changes that you suggested. I also implement cbentzel@'s suggestion as well. Please take another look. https://codereview.chromium.org/14125003/diff/19001/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right): https://codereview.chromium.org/14125003/diff/19001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:227: // * |ssl_version_min_learning_enabled| is false. On 2013/04/15 15:23:51, agl wrote: > This comment line looks to be a left-over. Done. https://codereview.chromium.org/14125003/diff/19001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:266: bool tmp; On 2013/04/15 15:23:51, agl wrote: > I'd use a real variable name rather than just |tmp| - we do above. Done. https://codereview.chromium.org/14125003/diff/19001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:322: bool tmp; On 2013/04/15 15:23:51, agl wrote: > ditto. Done. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction.cc:187: server_ssl_config_.version_fallback; On 2013/04/15 15:23:51, agl wrote: > I don't think version_fallback makes sense here. It's not like we can have > fallen back at this point. > > |sni_available| is designed to remove HSTS requirements when SNI isn't sent > (i.e. SSLv3) and we didn't want to do that under the attackers control - i.e. > when falling back. > > In this case, since we're setting TLSv1 to be the minimum version then SNI is > always sent. Thus I think this should always just be true. Done. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction.cc:190: if (TransportSecurityState::IsGooglePinnedProperty(host, sni_available)) { On 2013/04/15 18:03:28, Ryan Sleevi wrote: > nit: net/ style is to omit braces on one-line if statements. Done. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction.h:89: SSLConfig& server_ssl_config() { return server_ssl_config_; } On 2013/04/15 15:23:51, agl wrote: > server_ssl_config_for_testing() > > (If it's a testing function then let's make it really clear.) > > Alternatively, it looks like the style here is to use FRIEND_TEST_ALL_PREFIXES > for the tests. Done. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:28: class SimpleProxyConfigService : public ProxyConfigService { On 2013/04/15 15:23:51, agl wrote: > Looks like this is the third copy of this. Probably time to give it it's own > file and header. Done. https://codereview.chromium.org/14125003/diff/19001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:57: public: On 2013/04/15 15:23:51, agl wrote: > indentation of this class is incorrect. Done. https://codereview.chromium.org/14125003/diff/19001/net/ssl/ssl_config_servic... File net/ssl/ssl_config_service.cc (right): https://codereview.chromium.org/14125003/diff/19001/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.cc:190: new_config.disabled_cipher_suites) || On 2013/04/15 15:23:51, agl wrote: > This looks like a stray space. Done.
https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:33: net::test_spdy3::SpdySessionDependencies session_deps; Please let me know if you think I shouldn't reuse this class.
I think this is pretty close to landing, although rsleevi should take a look first. This review is only cursory because I've got to run in a sec. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction.cc:1226: if (server_ssl_config_.version_max == SSL_PROTOCOL_VERSION_TLS1 && Can't think chunk live before the switch() and thus remove the need to duplicate it below? https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction.h:89: SSLConfig& server_ssl_config_for_testing() { return server_ssl_config_; } Looks like rsleevi want's a friend test I'm afraid :)
https://codereview.chromium.org/14125003/diff/5003/chrome/common/chrome_switc... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/14125003/diff/5003/chrome/common/chrome_switc... chrome/common/chrome_switches.cc:324: // For preloaded Google's properties the minimum version will be TLS 1.0. s/'s// s/will be/is/ https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction.cc:1226: if (server_ssl_config_.version_max == SSL_PROTOCOL_VERSION_TLS1 && On 2013/04/16 15:19:24, agl wrote: > Can't think chunk live before the switch() and thus remove the need to duplicate > it below? Yeah, I think this switch can/should be re-organized a little as part of this uint16 version_before = server_ssl_config_.version_max; uint16 version_max = version_before; switch (error) { case ...: case ...: if (cond_a) { version_max = X } break; case ...: case ...: if (cond_b) { version_max = y; } break; } if (version_max <= SSL3 && Google) { // It would be good to UMA this here, right? Or are we // assuming this will always fail (since UMA => Google) // If it doesn't make sense, then just change the if/else // into an "if (x && !(version_max... && Google))" } else if (version_max != version_before) { server_ssl_config_.version_max = version_max; net_log_.AddEvent(...) server_ssl_config_.version_fallback = true; ResetConn...() error = OK; } https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction.h:89: SSLConfig& server_ssl_config_for_testing() { return server_ssl_config_; } On 2013/04/16 15:19:24, agl wrote: > Looks like rsleevi want's a friend test I'm afraid :) Either friend the test or friend the harness. I would recommend friending the harness (eg: friend class HttpNetworkTransactionTest) over the individual test. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:33: net::test_spdy3::SpdySessionDependencies session_deps; On 2013/04/16 00:39:12, thaidn_google wrote: > Please let me know if you think I shouldn't reuse this class. You shouldn't re-use this class :) You could use something similar to http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_network_layer_u... for these bits, AIUI. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:35: // |ssl_data1| contains the data for the first SSL handshake that will comment nit: you intermix comments of "first SSL handshake" (when talking about TLS) and "first handshake" (when talking about SSL3) I would suggest consistently saying "first handshake", and then making sure the comment explains *which* version you expect to be negotiated. Further, it seems like the # of handshakes is coupled to the TLS versions you're using (eg: TLS 1.1 -> TLS 1.0 -> SSL3), which will then create issues if/when we change the default to include TLS 1.2 (since now you have an ADDITIONAL set of fallbacks) Seems like this test harness should explicitly configure its supported TLS version to 1.0, so that there is only ever one fallback - to SSL3. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:70: rv = callback.WaitForResult(); We're trying to phase out this pattern in existing code. You can either do: int rv = callback.GetResult(trans->Start(...)) or int rv = trans->Start(); rv = callback.GetResult(rv); The former is preferable. The point of this is that the assertion that rv == ERR_IO_PENDING isn't strictly a requirement of the code under test. For example, you could just as well change line 38/48 to be SYNCHRONOUS completions and the test (should) still function correctly. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:71: EXPECT_EQ(net::ERR_SSL_PROTOCOL_ERROR, rv); You're in net (line 18), so you don't need the net:: prefix anywhere in this file. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:165: // fail. This comment seems entirely incorrect (and thus creates issues for the comment on 171-173). Where is it ever set that the failure is due to client certs? You're just flagging a protocol error. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:173: // Chrome will attempt to handshake 3 times, but there are just 2 handshake nit: s/Chrome/Chromium/ https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:180: new HttpNetworkTransaction(DEFAULT_PRIORITY, session)); style: indentation https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:185: callback.callback(), BoundNetLog()); style: indentation https://codereview.chromium.org/14125003/diff/5003/net/ssl/ssl_config_service.h File net/ssl/ssl_config_service.h (right): https://codereview.chromium.org/14125003/diff/5003/net/ssl/ssl_config_service... net/ssl/ssl_config_service.h:58: static uint16 SSLProtocolVersionFromString(const std::string& version_str); This seems like a really awkward place to put this, given that we already have net::SSLVersionToString in http://src.chromium.org/viewvc/chrome/trunk/src/net/ssl/ssl_cipher_suite_name... Seems like these should live there?
agl, rsleevi: thanks for the reviews. I've made all the changes that you suggest. Please take another look. https://codereview.chromium.org/14125003/diff/5003/chrome/common/chrome_switc... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/14125003/diff/5003/chrome/common/chrome_switc... chrome/common/chrome_switches.cc:324: // For preloaded Google's properties the minimum version will be TLS 1.0. On 2013/04/16 19:55:26, Ryan Sleevi wrote: > s/'s// > s/will be/is/ Done. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction.cc:1226: if (server_ssl_config_.version_max == SSL_PROTOCOL_VERSION_TLS1 && On 2013/04/16 19:55:26, Ryan Sleevi wrote: > On 2013/04/16 15:19:24, agl wrote: > > Can't think chunk live before the switch() and thus remove the need to > duplicate > > it below? > > Yeah, I think this switch can/should be re-organized a little as part of this > > uint16 version_before = server_ssl_config_.version_max; > uint16 version_max = version_before; > switch (error) { > case ...: > case ...: > if (cond_a) { > version_max = X > } > break; > case ...: > case ...: > if (cond_b) { > version_max = y; > } > break; > } > > if (version_max <= SSL3 && Google) { > // It would be good to UMA this here, right? Or are we > // assuming this will always fail (since UMA => Google) > // If it doesn't make sense, then just change the if/else > // into an "if (x && !(version_max... && Google))" > } else if (version_max != version_before) { > server_ssl_config_.version_max = version_max; > net_log_.AddEvent(...) > server_ssl_config_.version_fallback = true; > ResetConn...() > error = OK; > } Done. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction.h:89: SSLConfig& server_ssl_config_for_testing() { return server_ssl_config_; } On 2013/04/16 19:55:26, Ryan Sleevi wrote: > On 2013/04/16 15:19:24, agl wrote: > > Looks like rsleevi want's a friend test I'm afraid :) > > Either friend the test or friend the harness. > > I would recommend friending the harness (eg: > friend class HttpNetworkTransactionTest) over the individual test. Done. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:33: net::test_spdy3::SpdySessionDependencies session_deps; On 2013/04/16 19:55:26, Ryan Sleevi wrote: > On 2013/04/16 00:39:12, thaidn_google wrote: > > Please let me know if you think I shouldn't reuse this class. > > You shouldn't re-use this class :) > > You could use something similar to > http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_network_layer_u... > for these bits, AIUI. Done. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:70: rv = callback.WaitForResult(); On 2013/04/16 19:55:26, Ryan Sleevi wrote: > We're trying to phase out this pattern in existing code. > > You can either do: > > int rv = callback.GetResult(trans->Start(...)) or > > int rv = trans->Start(); > rv = callback.GetResult(rv); > > The former is preferable. > > The point of this is that the assertion that rv == ERR_IO_PENDING isn't strictly > a requirement of the code under test. For example, you could just as well change > line 38/48 to be SYNCHRONOUS completions and the test (should) still function > correctly. Done. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:71: EXPECT_EQ(net::ERR_SSL_PROTOCOL_ERROR, rv); On 2013/04/16 19:55:26, Ryan Sleevi wrote: > You're in net (line 18), so you don't need the net:: prefix anywhere in this > file. Done. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:165: // fail. On 2013/04/16 19:55:26, Ryan Sleevi wrote: > This comment seems entirely incorrect (and thus creates issues for the comment > on 171-173). > > Where is it ever set that the failure is due to client certs? You're just > flagging a protocol error. Done. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:173: // Chrome will attempt to handshake 3 times, but there are just 2 handshake On 2013/04/16 19:55:26, Ryan Sleevi wrote: > nit: s/Chrome/Chromium/ Done. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:180: new HttpNetworkTransaction(DEFAULT_PRIORITY, session)); On 2013/04/16 19:55:26, Ryan Sleevi wrote: > style: indentation Done. https://codereview.chromium.org/14125003/diff/5003/net/http/http_network_tran... net/http/http_network_transaction_ssl_unittest.cc:185: callback.callback(), BoundNetLog()); On 2013/04/16 19:55:26, Ryan Sleevi wrote: > style: indentation Done. https://codereview.chromium.org/14125003/diff/5003/net/ssl/ssl_config_service.h File net/ssl/ssl_config_service.h (right): https://codereview.chromium.org/14125003/diff/5003/net/ssl/ssl_config_service... net/ssl/ssl_config_service.h:58: static uint16 SSLProtocolVersionFromString(const std::string& version_str); It turns out that I don't need them here anymore. I just move them back to their original place. On 2013/04/16 19:55:26, Ryan Sleevi wrote: > This seems like a really awkward place to put this, given that we already have > net::SSLVersionToString in > http://src.chromium.org/viewvc/chrome/trunk/src/net/ssl/ssl_cipher_suite_name... > > Seems like these should live there?
Apologies for the back-and-forth, but I'm trying to make sure I pay close attention to the CL and give it a detailed review. A lot of comments about the unit tests - mostly cosmetic, hopefully, but they will hopefully make these tests easier to maintain and understand. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction.cc:1220: uint16 version_before = server_ssl_config_.version_max; So after reading the code (which, yes, I realize I pseudo-coded), I realized you don't need version_before. If you swap lines 1259/1260 (so that assignment comes after the logging), you can get away with just tracking version_max (the new max) and server_ssl_config_.version_max (aka version_before) https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:64: MockClientSocketFactory mock_socket_factory; Seems like all your tests just end up creating a mock_socket_factory and assigning it to session_params_ (eg: instead of lines 64 & 79) Should you just add this to the test harness and add it to params on line 39/40? Then you only have to do mock_socket_factory_.AddSSLSocketDataProvider(...) mock_socket_factory_.AddSocketDataProvider(...) https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:68: // |HttpNetworkTransaction|. comment style nit: While the || notation isn't formalized, AFAIK, Chromium code only uses it to refer to variable names, and not to things like type names/class names or enum values and the like. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:72: // previous connection was attempted with TLS 1.0. Since that behavior is comment nit: Two bits: 1) This is explaining a concept introduced on lines 66-68, but not yet explained. It would be good to move the explanation of what is "the SSL 3.0 fallback logic" into that comment. 2) Correctness nit: It's not unconditionally going to rollback to SSL3.0 - only if it's enabled. An easier way to address these comment nits might be to move the test description up to line 62 and provide the high-level summary of the test, and then briefly explain points that matter (for line 62) // Tests that the HttpNetworkTransaction does not attempt to // fall back to SSL 3.0 when a TLS 1.0 handshake fails and // the site is pinned to the Google pin list (indicating that // it is a Google site). then line 66 // |ssl_data| is for the first handshake (TLS 1.0), which // will fail for protocol reasons (eg: simulating a version // rollback attack). // Because SSL 3.0 fallback should never even be attempted, // only one simulated SSL handshake is supported. This generally applies to all tests in this file. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:76: net::StaticSocketDataProvider data(NULL, 0, NULL, 0); style: net:: namespace issues still https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:85: TestCompletionCallback callback; style nit: Move the TestCompletionCallback declaration closer to its first use (eg: line 90/91) Of course, if you move lines 87-89 below, as suggested, this may be unnecessary. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:89: ssl_config.version_max = SSL_PROTOCOL_VERSION_TLS1; Seems like this could be done in the test harness SetUp() when setting up the SSLConfigService to be a (class local) MockSSLConfigService? It's such a small amount of code to mock (eg: see https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config... ) that you could get away with class TLS1OnlySSLConfigService : public SSLConfigService { public: TLS1OnlySSLConfigService() { ssl_config_.version_min = SSL_PROTOCOL_VERSION_SSL3; ssl_config_.version_max = SSL_PROTOCOL_VERSION_TLS1; } virtual void GetSSLConfig(SSLConfig* config) OVERRIDE { *config = ssl_config_; } private: SSLConfig ssl_config_; }; I suggest this approach because your tests are also (subtlely) dependent upon version_min being SSL_PROTOCOL_VERSION_SSL3 (to enable fallback), which it may not always be so. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:118: // caller of the |HttpNetworkTransaction|. With the comments from line 72 incorporated (eg: including a high level description of this test on line 102), you could be brief and just say // |ssl_data2| contains the handshake result for an SSL 3.0 // handshake which will be attempted after the TLS 1.0 // handshake fails. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:123: // only one handshake data. comment nit: I'm generally not fond of inter-test comments cross-referencing eachother, because it means the developer has to skip around reading code. Further, this comment makes me think that Google_SSLVersionMinPreloadedEnabled perhaps would be served by having an extra SSL data (that should be unconsumed - and this can be confirmed by asking the SSLSocketDataProvider/MockSocketFactory how many sockets it vended) This is because if the behaviour being tested in Google_SSLVersionMinPreloadedEnabled ever breaks (and Google properties begin SSL fallbacks, for example), then based on your comment here, that test will start crashing (bringing down all of net_unittests), rather than failing gracefully. I'm not sure if I feel strongly enough on this fact to request it - because the developer should be running things locally first - but it's enough to highlight how significantly nasty the failure mode is. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:201: ssl_config.version_max); Is wrapping really necessary here?
Ryan, Thanks for the thorough review. I've learned a lot from your comments. I've revised my approach to address your concerns, please take another look. Thanks! https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction.cc:1220: uint16 version_before = server_ssl_config_.version_max; Nice catch :-). On 2013/04/17 01:16:36, Ryan Sleevi wrote: > So after reading the code (which, yes, I realize I pseudo-coded), I realized you > don't need version_before. > > If you swap lines 1259/1260 (so that assignment comes after the logging), you > can get away with just tracking version_max (the new max) and > server_ssl_config_.version_max (aka version_before) https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:64: MockClientSocketFactory mock_socket_factory; Nice tip. Thanks! On 2013/04/17 01:16:36, Ryan Sleevi wrote: > Seems like all your tests just end up creating a mock_socket_factory and > assigning it to session_params_ (eg: instead of lines 64 & 79) > > Should you just add this to the test harness and add it to params on line 39/40? > Then you only have to do > > mock_socket_factory_.AddSSLSocketDataProvider(...) > mock_socket_factory_.AddSocketDataProvider(...) https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:68: // |HttpNetworkTransaction|. Good to know. Thanks! On 2013/04/17 01:16:36, Ryan Sleevi wrote: > comment style nit: While the || notation isn't formalized, AFAIK, Chromium code > only uses it to refer to variable names, and not to things like type names/class > names or enum values and the like. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:72: // previous connection was attempted with TLS 1.0. Since that behavior is I hope the modified comments LGTY :-). On 2013/04/17 01:16:36, Ryan Sleevi wrote: > comment nit: Two bits: > 1) This is explaining a concept introduced on lines 66-68, but not yet > explained. It would be good to move the explanation of what is "the SSL 3.0 > fallback logic" into that comment. > 2) Correctness nit: It's not unconditionally going to rollback to SSL3.0 - only > if it's enabled. > > An easier way to address these comment nits might be to move the test > description up to line 62 and provide the high-level summary of the test, and > then briefly explain points that matter > > (for line 62) > // Tests that the HttpNetworkTransaction does not attempt to > // fall back to SSL 3.0 when a TLS 1.0 handshake fails and > // the site is pinned to the Google pin list (indicating that > // it is a Google site). > > then line 66 > // |ssl_data| is for the first handshake (TLS 1.0), which > // will fail for protocol reasons (eg: simulating a version > // rollback attack). > // Because SSL 3.0 fallback should never even be attempted, > // only one simulated SSL handshake is supported. > > This generally applies to all tests in this file. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:76: net::StaticSocketDataProvider data(NULL, 0, NULL, 0); On 2013/04/17 01:16:36, Ryan Sleevi wrote: > style: net:: namespace issues still Done. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:85: TestCompletionCallback callback; On 2013/04/17 01:16:36, Ryan Sleevi wrote: > style nit: Move the TestCompletionCallback declaration closer to its first use > (eg: line 90/91) > > Of course, if you move lines 87-89 below, as suggested, this may be unnecessary. Done. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:89: ssl_config.version_max = SSL_PROTOCOL_VERSION_TLS1; On 2013/04/17 01:16:36, Ryan Sleevi wrote: > Seems like this could be done in the test harness SetUp() when setting up the > SSLConfigService to be a (class local) MockSSLConfigService? > > It's such a small amount of code to mock (eg: see > https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_config... > ) that you could get away with > > class TLS1OnlySSLConfigService : public SSLConfigService { > public: > TLS1OnlySSLConfigService() { > ssl_config_.version_min = SSL_PROTOCOL_VERSION_SSL3; > ssl_config_.version_max = SSL_PROTOCOL_VERSION_TLS1; > } > > virtual void GetSSLConfig(SSLConfig* config) OVERRIDE { > *config = ssl_config_; > } > > private: > SSLConfig ssl_config_; > }; > > > I suggest this approach because your tests are also (subtlely) dependent upon > version_min being SSL_PROTOCOL_VERSION_SSL3 (to enable fallback), which it may > not always be so. Done. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:118: // caller of the |HttpNetworkTransaction|. On 2013/04/17 01:16:36, Ryan Sleevi wrote: > With the comments from line 72 incorporated (eg: including a high level > description of this test on line 102), you could be brief and just say > > // |ssl_data2| contains the handshake result for an SSL 3.0 > // handshake which will be attempted after the TLS 1.0 > // handshake fails. Done. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:123: // only one handshake data. I agree. On 2013/04/17 01:16:36, Ryan Sleevi wrote: > comment nit: I'm generally not fond of inter-test comments cross-referencing > eachother, because it means the developer has to skip around reading code. > > Further, this comment makes me think that Google_SSLVersionMinPreloadedEnabled > perhaps would be served by having an extra SSL data (that should be unconsumed - > and this can be confirmed by asking the SSLSocketDataProvider/MockSocketFactory > how many sockets it vended) > > This is because if the behaviour being tested in > Google_SSLVersionMinPreloadedEnabled ever breaks (and Google properties begin > SSL fallbacks, for example), then based on your comment here, that test will > start crashing (bringing down all of net_unittests), rather than failing > gracefully. > > I'm not sure if I feel strongly enough on this fact to request it - because the > developer should be running things locally first - but it's enough to highlight > how significantly nasty the failure mode is. https://codereview.chromium.org/14125003/diff/61001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:201: ssl_config.version_max); On 2013/04/17 01:16:36, Ryan Sleevi wrote: > Is wrapping really necessary here? Done.
Mod nits, I think this LGTM. Adam or Wan-Teh, I wasn't sure if you wanted to take a last pass here. https://codereview.chromium.org/14125003/diff/74001/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/14125003/diff/74001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref.cc:67: std::string SSLConfig::SSLProtocolVersionToString(uint16 version) { I believe you should revert these changes - it's no longer a member of SSLConfig::. https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:90: TEST_F(HttpNetworkTransactionSSLTest, Google_SSLVersionMinPreloadedEnabled) { One test naming nit - I would suggest using the site as the suffix, rather than the prefix, so that you can do --gtest_filter=SSLVersionMinPreloaded* and run all three tests easily
Review comments on patch set 8: I reviewed the CL before you uploaded patch set 9, so my comments are on patch set 8. 1. Optional: we should come up with a better name than "ssl_version_min_preloaded_disabled". I suggest some ideas below. 2. I suggest an alternative way to implement this change in http_network_transaction.cc. I marked that comment with "IMPORTANT". https://codereview.chromium.org/14125003/diff/74001/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/14125003/diff/74001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref.cc:310: version_max_str); Why reformat these two lines? https://codereview.chromium.org/14125003/diff/74001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/14125003/diff/74001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:327: "disable-ssl-version-min-preloaded"; The CL's description says this option is --ssl-version-min-preloaded-disabled. Please update the CL's description. https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... File net/http/http_network_transaction.cc (left): https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... net/http/http_network_transaction.cc:1249: server_ssl_config_.version_min == SSL_PROTOCOL_VERSION_SSL3) { BUG?: The original code uses server_ssl_config_.version_min here. Did you intend to change it to version_max? Since we no longer do TLS compression, this code is now obsolete and can be simply deleted. https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... net/http/http_network_transaction.cc:176: if (request_->load_flags & LOAD_DISABLE_CERT_REVOCATION_CHECKING) { IMPORTANT: the name "ssl_version_min_preloaded_disabled" suggests that we can simply change server_ssl_config_.version_min here, as soon as request_->url is available and we can call TransportSecurityState::IsGooglePinnedProperty(). I think this alternative implementation would be better. https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... net/http/http_network_transaction.cc:1253: // Do not fallback to SSL 3.0 on Google properties. I suggest moving this comment before the if statement to avoid mixing comments with conditional expressions. This change is borderline "optimizing Chrome for Google servers". I think we should clarify this in this comment. Perhaps we will experiment with Google servers first and expand this to other servers after we have gained enough experience. Perhaps there is a high risk of breaking the servers so we are uncomfortable doing this in general. https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... net/http/http_network_transaction.cc:1255: !server_ssl_config_.ssl_version_min_preloaded_disabled && I think "hsts" should be part of this member's name. I had no idea what "preloaded" means. It is a little confusing to use "version_min" in this member's name because it is actually used in conjunction with version_max here. This is another reason to consider the alternative implementation. Perhaps this member should be named something like "strict_ssl_version_fallback_disabled". https://codereview.chromium.org/14125003/diff/74001/net/socket/socket_test_ut... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/14125003/diff/74001/net/socket/socket_test_ut... net/socket/socket_test_util.h:522: size_t next_index() { return next_index_; } Nit: move this getter method to be next to the related ResetNextIndex() method.
Will address wtc@'s comments in a follow up PS. https://codereview.chromium.org/14125003/diff/74001/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/14125003/diff/74001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref.cc:67: std::string SSLConfig::SSLProtocolVersionToString(uint16 version) { Oh right. Good catch! This should be caught if I was able to compile unit_tests (it failed for some unrelated reasons). On 2013/04/17 18:01:27, Ryan Sleevi wrote: > I believe you should revert these changes - it's no longer a member of > SSLConfig::. > https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:90: TEST_F(HttpNetworkTransactionSSLTest, Google_SSLVersionMinPreloadedEnabled) { On 2013/04/17 18:01:27, Ryan Sleevi wrote: > One test naming nit - I would suggest using the site as the suffix, rather than > the prefix, so that you can do > > --gtest_filter=SSLVersionMinPreloaded* and run all three tests easily Done.
wtc: please take another look. https://codereview.chromium.org/14125003/diff/74001/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/14125003/diff/74001/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref.cc:310: version_max_str); On 2013/04/17 19:49:50, wtc wrote: > > Why reformat these two lines? Done. https://codereview.chromium.org/14125003/diff/74001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/14125003/diff/74001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:327: "disable-ssl-version-min-preloaded"; I change it to --enable-ssl3-version-fallback, and update the CL's description. On 2013/04/17 19:49:50, wtc wrote: > > The CL's description says this option is > --ssl-version-min-preloaded-disabled. Please update the CL's > description. https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... net/http/http_network_transaction.cc:176: if (request_->load_flags & LOAD_DISABLE_CERT_REVOCATION_CHECKING) { This was my original approach. Chris pointed out that there may be MITM proxies that only implement SSLv3. Preventing fallback would allow them to continue to work, but setting the minimum version breaks them. On 2013/04/17 19:49:50, wtc wrote: > > IMPORTANT: the name "ssl_version_min_preloaded_disabled" > suggests that we can simply change server_ssl_config_.version_min > here, as soon as request_->url is available and we can > call TransportSecurityState::IsGooglePinnedProperty(). > > I think this alternative implementation would be better. https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... net/http/http_network_transaction.cc:1253: // Do not fallback to SSL 3.0 on Google properties. On 2013/04/17 19:49:50, wtc wrote: > > I suggest moving this comment before the if statement to > avoid mixing comments with conditional expressions. > > This change is borderline "optimizing Chrome for Google > servers". I think we should clarify this in this comment. > Perhaps we will experiment with Google servers first and > expand this to other servers after we have gained enough > experience. Perhaps there is a high risk of breaking the > servers so we are uncomfortable doing this in general. Done. https://codereview.chromium.org/14125003/diff/74001/net/http/http_network_tra... net/http/http_network_transaction.cc:1255: !server_ssl_config_.ssl_version_min_preloaded_disabled && I rename it to ssl3_version_fallback_enabled. I chose the old name because it was part of a bigger plan to induce minimum SSL version from HSTS preloaded configuration. On 2013/04/17 19:49:50, wtc wrote: > > I think "hsts" should be part of this member's name. I had > no idea what "preloaded" means. > > It is a little confusing to use "version_min" in this member's > name because it is actually used in conjunction with version_max > here. This is another reason to consider the alternative > implementation. > > Perhaps this member should be named something like > "strict_ssl_version_fallback_disabled". https://codereview.chromium.org/14125003/diff/74001/net/socket/socket_test_ut... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/14125003/diff/74001/net/socket/socket_test_ut... net/socket/socket_test_util.h:522: size_t next_index() { return next_index_; } On 2013/04/17 19:49:50, wtc wrote: > > Nit: move this getter method to be next to the related > ResetNextIndex() method. Done.
Review comments on patch set 11: this still needs work. https://codereview.chromium.org/14125003/diff/87004/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right): https://codereview.chromium.org/14125003/diff/87004/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:226: // * |ssl3_version_fallback_enabled| is false; Nit: this is the last bullet item, so it can end with a period. https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... File net/http/http_network_transaction.cc (left): https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... net/http/http_network_transaction.cc:1221: case ERR_SSL_VERSION_OR_CIPHER_MISMATCH: It may be clearer to retain the original switch statement. https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... net/http/http_network_transaction.cc:1243: if (version_max == SSL_PROTOCOL_VERSION_SSL3 && BUG?: I think this should be if (version_max > SSL_PROTOCOL_VERSION_SSL3 || https://codereview.chromium.org/14125003/diff/87004/net/ssl/ssl_config_service.h File net/ssl/ssl_config_service.h (right): https://codereview.chromium.org/14125003/diff/87004/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.h:95: // fails. This description does not reflect the actual meaning of this boolean member. It fails to describe the IsGooglePinnedProperty aspect. https://codereview.chromium.org/14125003/diff/87004/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.h:96: bool ssl3_version_fallback_enabled; Remove "version". "ssl3_fallback_enabled" is sufficient. (The |version_fallback| member below was originally named |ssl3_fallback|. I generalized it to |version_fallback| in https://chromiumcodereview.appspot.com/10377022.)
https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... File net/http/http_network_transaction.cc (left): https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... net/http/http_network_transaction.cc:1221: case ERR_SSL_VERSION_OR_CIPHER_MISMATCH: On 2013/04/17 23:22:05, wtc wrote: > > It may be clearer to retain the original switch statement. Done. https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... net/http/http_network_transaction.cc:1243: if (version_max == SSL_PROTOCOL_VERSION_SSL3 && No, it isn't a bug (otherwise tests have failed). The statements inside the if clause will cause the fallback. So we need to test two things: * The protocol that we are going to use for the fallback would be SSL_PROTOCOL_VERSION_SSL3; and * Either fallback is enabled or this is not a Google site. On 2013/04/17 23:22:05, wtc wrote: > > BUG?: I think this should be > if (version_max > SSL_PROTOCOL_VERSION_SSL3 || https://codereview.chromium.org/14125003/diff/87004/net/ssl/ssl_config_service.h File net/ssl/ssl_config_service.h (right): https://codereview.chromium.org/14125003/diff/87004/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.h:95: // fails. On 2013/04/17 23:22:05, wtc wrote: > > This description does not reflect the actual meaning of this > boolean member. It fails to describe the IsGooglePinnedProperty > aspect. Done. https://codereview.chromium.org/14125003/diff/87004/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.h:96: bool ssl3_version_fallback_enabled; On 2013/04/17 23:22:05, wtc wrote: > > Remove "version". "ssl3_fallback_enabled" is sufficient. > > (The |version_fallback| member below was originally named > |ssl3_fallback|. I generalized it to |version_fallback| in > https://chromiumcodereview.appspot.com/10377022.) Done.
Patch set 13 LGTM. The main issue is that I still find "ssl3_fallback_enabled" confusing. I think we should qualify it with something like "permissive" or "unrestricted" to reflect more accurately what this setting actually controls, at least before we expand this to all sites. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref.cc (right): https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref.cc:183: BooleanPrefMember ssl3_verlback_enabled_; Typo: ssl3_verlback_enabled_ => ssl3_fallback_enabled_ https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref.cc:223: ssl3_versionk_enabled_.Init( Typo: ssl3_versionk_enabled_ => ssl3_fallback_enabled_ https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref.cc:268: default_config.ssl3_fallback_enabled); Nit: you may be able to format this call in two lines now: registry->RegisterBooleanPref(refs::kEnableSSL3Fallback, default_config.ssl3_fallback_enabled); https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref.cc:331: ssl3_fallback_enabled_.GetValue(); This should fit on one line now: config->ssl3_fallback_enabled = ssl3_fallback_enabled_.GetValue(); https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right): https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:226: // * |ssl3_fallback_enabled| is false. I suggest rewording this comment as follows (please fold long lines as appropriate): // Test that // * without command-line settings for minimum and maximum SSL versions, // SSL 3.0 ~ default_version_max() are enabled; // * without --enable-ssl3-fallback, |ssl3_fallback_enabled| is false. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:256: prefs::kEnableSSL3Fallback)); This should now fit on one line. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:268: &ssl3_fallback_enabled)); This call should fit in two lines now. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:311: const PrefService::Preference* ssl3_version_fallback_pref = ssl3_version_fallback_pref => ssl3_fallback_pref https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:324: &ssl3_fallback_enabled)); This should fit in two lines now. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/prefs/comm... File chrome/browser/prefs/command_line_pref_store.cc (right): https://codereview.chromium.org/14125003/diff/92003/chrome/browser/prefs/comm... chrome/browser/prefs/command_line_pref_store.cc:57: prefs::kEnableSSL3Fallback, true }, This should fit in one line now. https://codereview.chromium.org/14125003/diff/92003/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/14125003/diff/92003/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:623: // Enables SSL 3.0 fallback. This comment should say something like "Enables permissive SSL 3.0 fallback" or "Enables unrestricted SSL 3.0 fallback". We probably should just duplicate the comment from ssl_config_service.h. https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... net/http/http_network_transaction.cc:1248: (version_max == SSL_PROTOCOL_VERSION_SSL3 && Delete "(version_max == SSL_PROTOCOL_VERSION_SSL3 &&" and the corresponding closing ")". This is implied by how C/C++ evaluates the two operands of the || operator, and the fact that SSL_PROTOCOL_VERSION_SSL3 is the smallest SSL version used by Chrome. https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... File net/http/http_network_transaction_ssl_unittest.cc (right): https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:26: class TLS1OnlySSLConfigService : public SSLConfigService { It seems wrong to call this SSLConfigService "TLS1 only" because it still enables SSL3 (see line 29). Perhaps this class should be named TLS10SSLConfigService https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... net/http/http_network_transaction_ssl_unittest.cc:106: // * SSL 3.0 fallback is disabled. All instances of "SSL 3.0 fallback" in this file should be changed to something like "Permissive SSL 3.0 fallback" or "Unrestricted SSL 3.0 fallback" to be more accurate. https://codereview.chromium.org/14125003/diff/92003/net/socket/socket_test_ut... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/14125003/diff/92003/net/socket/socket_test_ut... net/socket/socket_test_util.h:534: void ResetNextIndex() { Nit and optional: we should rename this method "reset_next_index" for a consistent style. The Style Guide allows lowercase letters for very short inlined functions. https://codereview.chromium.org/14125003/diff/92003/net/ssl/ssl_config_servic... File net/ssl/ssl_config_service.cc (right): https://codereview.chromium.org/14125003/diff/92003/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.cc:161: new_config.ssl3_fallback_enabled) || Please add this test after the false_start_enabled test to match the declaration order of these members. https://codereview.chromium.org/14125003/diff/92003/net/ssl/ssl_config_service.h File net/ssl/ssl_config_service.h (right): https://codereview.chromium.org/14125003/diff/92003/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.h:98: // for all sites. I suggest describing the "true" case before the "false" case, and rewording the "false" case slightly to make the two cases more symmetrical, as follows: // If |ssl3_fallback_enabled| is true, SSL 3.0 fallback will be enabled // for all sites. // If |ssl3_fallback_enabled| is false, SSL 3.0 fallback will be disabled // for a site pinned to the Google pin list (indicating that it is a // Google site). https://codereview.chromium.org/14125003/diff/92003/net/ssl/ssl_config_servic... net/ssl/ssl_config_service.h:99: bool ssl3_fallback_enabled; NOTE: I still think this member name is confusing. I think something like "permissive_ssl3_fallback_enabled" or "ssl3_fallback_unrestricted" would be more accurate. https://codereview.chromium.org/14125003/diff/92003/net/ssl/ssl_config_servic... File net/ssl/ssl_config_service_unittest.cc (right): https://codereview.chromium.org/14125003/diff/92003/net/ssl/ssl_config_servic... net/ssl/ssl_config_service_unittest.cc:74: initial_config.ssl3_fallback_enabled = false; Set this after line 71 (initial_config.false_start_enabled = false). https://codereview.chromium.org/14125003/diff/92003/net/ssl/ssl_config_servic... net/ssl/ssl_config_service_unittest.cc:101: mock_service->SetSSLConfig(initial_config); Do this after lines 86-88 (which deals with false_start_enabled).
https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... net/http/http_network_transaction.cc:1243: if (version_max == SSL_PROTOCOL_VERSION_SSL3 && On 2013/04/18 00:05:50, thaidn_google wrote: > No, it isn't a bug (otherwise tests have failed). The unit test HTTPSRequestTest.TLSv1Fallback in net/url_request/url_request_unittest.cc should have caught this. Do you know why that test didn't fail? Perhaps you didn't run all the tests in net_unittests?
wtc: I've addressed your comments. Please take another look. There is one problem: looks like I messed up with my git branches, and now this CL contains some unrelated changes from other CLs. I don't know if this causes any problem for merging etc., but if it does I will try to upload from the original branch. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right): https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:226: // * |ssl3_fallback_enabled| is false. On 2013/04/18 18:15:34, wtc wrote: > I suggest rewording this comment as follows (please fold long > lines as appropriate): > > // Test that > // * without command-line settings for minimum and maximum SSL versions, > // SSL 3.0 ~ default_version_max() are enabled; > // * without --enable-ssl3-fallback, |ssl3_fallback_enabled| is false. Done. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:256: prefs::kEnableSSL3Fallback)); After the rename, it can't be on one line. On 2013/04/18 18:15:34, wtc wrote: > > This should now fit on one line. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:268: &ssl3_fallback_enabled)); After the rename, it can't be on two lines. On 2013/04/18 18:15:34, wtc wrote: > > This call should fit in two lines now. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:311: const PrefService::Preference* ssl3_version_fallback_pref = On 2013/04/18 18:15:34, wtc wrote: > > ssl3_version_fallback_pref => ssl3_fallback_pref Done. https://codereview.chromium.org/14125003/diff/92003/chrome/browser/net/ssl_co... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:324: &ssl3_fallback_enabled)); After the rename, it can't be on two lines. On 2013/04/18 18:15:34, wtc wrote: > > This should fit in two lines now. https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... net/http/http_network_transaction.cc:1248: (version_max == SSL_PROTOCOL_VERSION_SSL3 && Nice catch. Thanks! On 2013/04/18 18:15:34, wtc wrote: > > Delete "(version_max == SSL_PROTOCOL_VERSION_SSL3 &&" and the > corresponding closing ")". > > This is implied by how C/C++ evaluates the two operands of > the || operator, and the fact that SSL_PROTOCOL_VERSION_SSL3 > is the smallest SSL version used by Chrome.
On 2013/04/18 18:23:09, wtc wrote: > https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... > File net/http/http_network_transaction.cc (right): > > https://codereview.chromium.org/14125003/diff/87004/net/http/http_network_tra... > net/http/http_network_transaction.cc:1243: if (version_max == > SSL_PROTOCOL_VERSION_SSL3 && > > On 2013/04/18 00:05:50, thaidn_google wrote: > > No, it isn't a bug (otherwise tests have failed). > > The unit test HTTPSRequestTest.TLSv1Fallback in > net/url_request/url_request_unittest.cc should have caught > this. Do you know why that test didn't fail? Perhaps you > didn't run all the tests in net_unittests? I didn't run all the tests in net_unittests. Now I'm running them and I get a few failures: [ FAILED ] HTTPSOCSPTest.Valid [ FAILED ] HTTPSOCSPTest.Revoked [ FAILED ] HTTPSOCSPTest.Invalid [ FAILED ] HTTPSEVCRLSetTest.MissingCRLSetAndInvalidOCSP [ FAILED ] HTTPSEVCRLSetTest.MissingCRLSetAndGoodOCSP [ FAILED ] HTTPSEVCRLSetTest.ExpiredCRLSet [ FAILED ] HTTPSEVCRLSetTest.FreshCRLSet [ FAILED ] HTTPSEVCRLSetTest.ExpiredCRLSetAndRevokedNonEVCert [ FAILED ] HTTPSCRLSetTest.ExpiredCRLSet They look unrelated to what I am doing. Here is the stack trace: OCSP server started on 127.0.0.1:35480... Traceback (most recent call last): File "/usr/local/google/home/thaidn/p/chrome/src/net/tools/testserver/testserver.py", line 2089, in <module> sys.exit(ServerRunner().main()) File "/usr/local/google/home/thaidn/p/chrome/src/net/tools/testserver/testserver_base.py", line 189, in main self.server = self.create_server(server_data) File "/usr/local/google/home/thaidn/p/chrome/src/net/tools/testserver/testserver.py", line 1925, in create_server self.options.tls_intolerant) File "/usr/local/google/home/thaidn/p/chrome/src/net/tools/testserver/testserver.py", line 132, in __init__ self.private_key = tlslite.api.parsePEMKey(pem_cert_and_key, private=True) File "/usr/local/google/home/thaidn/p/chrome/src/third_party/tlslite/tlslite/utils/keyfactory.py", line 146, in parsePEMKey key = OpenSSL_RSAKey.parse(s, passwordCallback) File "/usr/local/google/home/thaidn/p/chrome/src/third_party/tlslite/tlslite/utils/OpenSSL_RSAKey.py", line 141, in parse raise SyntaxError() SyntaxError: None Seems like it couldn't read the keys? BTW I talk to rsleevi@, and he assures that the PS will land clearly. It will create a bit more work for you to review. Could you please ignore the unrelated changes (which should be clear from the context), and focus on my changes? Thanks!
I mark a few unrelated changes that may be confusing. https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... net/http/http_network_transaction.cc:1084: } Not me. https://codereview.chromium.org/14125003/diff/92003/net/http/http_network_tra... net/http/http_network_transaction.cc:1137: } Not me. https://codereview.chromium.org/14125003/diff/107001/net/http/http_network_tr... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/14125003/diff/107001/net/http/http_network_tr... net/http/http_network_transaction.cc:780: request_headers_.HasHeader(HttpRequestHeaders::kProxyAuthorization); Not me. https://codereview.chromium.org/14125003/diff/107001/net/socket/socket_test_u... File net/socket/socket_test_util.h (right): https://codereview.chromium.org/14125003/diff/107001/net/socket/socket_test_u... net/socket/socket_test_util.h:746: bool peer_closed_connection_; not me.
LGTM. There's some diff noise (I'm assuming), but I'm ignoring that. One tiny typo. Will leave for wtc to hit the CQ button since he had a few comments last time. https://codereview.chromium.org/14125003/diff/107001/chrome/browser/net/ssl_c... File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right): https://codereview.chromium.org/14125003/diff/107001/chrome/browser/net/ssl_c... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:226: // * without --enable-unrestricted_ssl3-fallback, s/_/-/
Patch set 14 LGTM. Thanks! Please make the changes Adam and I suggested in the review. Please change the CL's description: 1. Nit: change "Does not" to "Do not". 2. Mention that you deleted the obsolete SSL 3.0 fallback on TLS decompression failure. https://codereview.chromium.org/14125003/diff/107001/net/ssl/ssl_config_servi... File net/ssl/ssl_config_service.h (right): https://codereview.chromium.org/14125003/diff/107001/net/ssl/ssl_config_servi... net/ssl/ssl_config_service.h:97: // disabled for a site pinned to the Google pin list (indicating that Nit: can you fill up this line?
On 2013/04/19 01:20:43, thaidn_google wrote: > > There is one problem: looks like I messed up with my git branches, and now this > CL contains some unrelated changes from other CLs. This is normal. If you update/sync your work tree to the tip of the Chromium trunk, you may pick up changes made by others, and the next patch set you upload will include those changes. You can avoid this problem by not updating/syncing your work tree unless absolutely necesssary. But in general we are used to mentally filter out such unrelated changes.
Thanks for the review, guys! Please commit it for me. https://codereview.chromium.org/14125003/diff/107001/chrome/browser/net/ssl_c... File chrome/browser/net/ssl_config_service_manager_pref_unittest.cc (right): https://codereview.chromium.org/14125003/diff/107001/chrome/browser/net/ssl_c... chrome/browser/net/ssl_config_service_manager_pref_unittest.cc:226: // * without --enable-unrestricted_ssl3-fallback, On 2013/04/19 14:41:52, agl wrote: > s/_/-/ Done. https://codereview.chromium.org/14125003/diff/107001/net/ssl/ssl_config_servi... File net/ssl/ssl_config_service.h (right): https://codereview.chromium.org/14125003/diff/107001/net/ssl/ssl_config_servi... net/ssl/ssl_config_service.h:97: // disabled for a site pinned to the Google pin list (indicating that On 2013/04/19 17:45:18, wtc wrote: > > Nit: can you fill up this line? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thaidn@google.com/14125003/113002
Presubmit check for 14125003-113002 failed and returned exit status 1.
INFO:root:Found 17 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/browser/prefs/command_line_pref_store.cc
Presubmit checks took 2.5s to calculate.
thestig: could you review the changes to src/chrome in this CL or recommend another reviewer (who knows about prefs)? Thanks.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thaidn@google.com/14125003/113002
Message was sent while issue was closed.
Change committed as 195335 |
