Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1727133002:
Expose TLS settings in the Security panel overview, and call out individual obsolete settings. (Closed)
DescriptionExpose TLS settings in the Security panel overview, and call out individual obsolete settings.
BUG=551728
TEST=For all tests below, first open DevTools to the Security panel overview.
1) Visit google.com and check that there is a green bullet point with
the following summary and description:
- Secure Connection
- The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM).
2) Visit cbc.badssl.com and check that there is a gray info bullet
point with the following summary and description:
- Obsolete Connection Settings
- The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1).
3) Visit cbc.badssl.com/mixed/script/ and check that there are two gray
info bullet points: "Obsolete TLS cipher suite" and
"Blocked mixed content".
4) Visit static-rsa.badssl.com and check that there is a gray info bullet point
with the following summary and description:
- Obsolete Connection Settings
- The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM).
5) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info
bullet point with the following summary and description:
- Obsolete Connection Settings
- The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1).
Committed: https://crrev.com/3e2c33e434d9832c8d4883c9264adb5be0e9bf68
Cr-Commit-Position: refs/heads/master@{#414341}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address estark@'s initial nits. #Patch Set 3 : Always expose TLS settings. #
Total comments: 13
Patch Set 4 : Reintroduce IsSecureTLSCipherSuite() as its negative and update tests. #
Total comments: 40
Patch Set 5 : Respond to comments; simplify a lot of the strings; take out IsObsoleteTLSCipherSuite(). #
Total comments: 4
Patch Set 6 : Add some tests. #
Total comments: 24
Patch Set 7 : Rebase and update tests. #
Total comments: 2
Patch Set 8 : EXPECT_EQ(TRUE, is_aead); → EXPECT_TRUE(is_aead); #Patch Set 9 : Place "Modern SSL" comment on same line as enum value. #Patch Set 10 : Update SecurityStateModelTest.* unit tests. #Patch Set 11 : Try removing NET_EXPORT from the ObsoleteSSLMask enum. #Patch Set 12 : Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. #Patch Set 13 : Rebasing. #Patch Set 14 : Remove NOTREACHED() that is now reached. #Patch Set 15 : Move "was there real TLS?" check above "turn TLS details into strings" section. #Patch Set 16 : Also check that connection_status is not zero, which is the case for 3 browser tests. #Messages
Total messages: 105 (56 generated)
lgarron@chromium.org changed reviewers: + davidben@chromium.org, estark@chromium.org
estark@: could you review this draft CL? davidben@: Presumably, we want the string to show something more useful that than this CL. How do you feel about showing the generic string most of the time, and special-casing a string for M50 that specifically calls out when DHE is in use? Also, how do both of you feel about showing the cipher suite (e.g. "ECDHE_RSA with HMAC-SHA1 ") in the message?
Oh cool, this is even easier than I thought it was going to be, I didn't realize we already had kSecurityStateInfo. Looks good but can you add some tests before landing? I don't have strong feelings one way or the other about showing the ciphersuite, except that if we show the ciphersuite we should also show the protocol version, since either one of those can cause the message to show up. https://codereview.chromium.org/1727133002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14324: + <message name="IDS_OBSOLETE_PROTOCOL_AND_CIPHERSUITE" desc="Summary phrase for a site that uses a modern, secure TLS protocol version and ciphersuite."> Both of these desc attributes should say "obsolete" instead of "modern, secure" https://codereview.chromium.org/1727133002/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1727133002/diff/1/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1411: ); formatting looks weird here
On 2016/02/24 at 02:10:51, estark wrote: > Oh cool, this is even easier than I thought it was going to be, I didn't realize we already had kSecurityStateInfo. > > Looks good but can you add some tests before landing? > > I don't have strong feelings one way or the other about showing the ciphersuite, except that if we show the ciphersuite we should also show the protocol version, since either one of those can cause the message to show up. > > https://codereview.chromium.org/1727133002/diff/1/chrome/app/generated_resour... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/1727133002/diff/1/chrome/app/generated_resour... > chrome/app/generated_resources.grd:14324: + <message name="IDS_OBSOLETE_PROTOCOL_AND_CIPHERSUITE" desc="Summary phrase for a site that uses a modern, secure TLS protocol version and ciphersuite."> > Both of these desc attributes should say "obsolete" instead of "modern, secure" > > https://codereview.chromium.org/1727133002/diff/1/chrome/browser/ui/browser.cc > File chrome/browser/ui/browser.cc (right): > > https://codereview.chromium.org/1727133002/diff/1/chrome/browser/ui/browser.c... > chrome/browser/ui/browser.cc:1411: ); > formatting looks weird here Yeah, I need to add tests and make sure the message show up at the end instead of the middle of the bullets: https://bugs.chromium.org/p/chromium/issues/detail?id=551728#c11 So, there's a subtlety about cipher suites. The only way to have a modern cipher suite is to use TLS 1.2. Therefore, an outdated protocol also implies an outdated cipher suite (at the moment), and fixing the warning will require upgrading the protocol if it's outdated. (davidben@, can you back me up my claim?) However, an outdated cipher suite does not imply an outdated protocol, so we can't just say "outdated protocol *and* cipher suite*. So, there are two options: 1) Just say outdated cipher suite. 2) Say outdated cipher suite, and also say outdated protocol if the protocol outdated. I've opted for 1) because it's simpler, and it's actually what we used to do in the connection tab. Would you prefer 2)?
On 2016/02/24 02:02:15, lgarron wrote: > davidben@: Presumably, we want the string to show something more useful that > than this CL. How do you feel about showing the generic string most of the time, > and special-casing a string for M50 that specifically calls out when DHE is in > use? Seems reasonable. Given that M50 is branching imminently, we can defer figuring out DHE stuff until M51. I'm thinking that should also include a console message since it's an imminent removal rather than (we're going to keep frowning at you until you behave). :-) We probably do want it to include text or point to somewhere that says what we actually want you to use. (Maybe some TLS "best practices" document?) But I'm also okay figuring that out later since the text was there before without useful instructions. > Also, how do both of you feel about showing the cipher suite (e.g. "ECDHE_RSA > with HMAC-SHA1 > ") in the message? No strong opinions on my end. Seems reasonable enough. > I don't have strong feelings one way or the other about showing the ciphersuite, > except that if we show the ciphersuite we should also show the protocol version, > since either one of those can cause the message to show up. Strictly speaking, it can just be a cipher suite check because you don't get AEADs before TLS 1.2, but calling out low versions sounds also reasonable. Getting rid of TLS 1.0/1.1 is arguably more important than getting rid of non-AEAD ciphers because that affects downgrade. We're dependent on a MD5/SHA1 concatenation right now. (Of course, I don't expect we'll be able to remove any of this for a loooooooong time if ever. :-( And there's a lovely OpenSSL bug that means a lot of TLS 1.2 OpenSSLs also can't sign anything but SHA-1, so we're still not home free if we got rid of TLS 1.0/1.1. I need to go yell at Ubuntu about backporting that fix...)
Description was changed from ========== Surface an obsolete cipher suite warning in the Security panel overview. BUG=551728 ========== to ========== Surface an obsolete cipher suite warning in the Security panel overview. BUG=551728 TEST=Visit cbc.badssl.com, open DevTools to the Security panel, and make sure the Overview shows a gray bullet point saying "Obsolete TLS Settings". ==========
Did anything ever happen with this CL? M50 seems to have branched with the change to the OIB but still with the obsolete/modern regression.
On 2016/03/22 at 18:36:59, davidben wrote: > Did anything ever happen with this CL? M50 seems to have branched with the change to the OIB but still with the obsolete/modern regression. I de-prioritized it after learning it was not as critical for you as I initially thought. I consider this "important, but not urgent" for Security panel now. Would you like it in M51 due to DHE??
On 2016/03/22 23:05:55, lgarron wrote: > On 2016/03/22 at 18:36:59, davidben wrote: > > Did anything ever happen with this CL? M50 seems to have branched with the > change to the OIB but still with the obsolete/modern regression. > > I de-prioritized it after learning it was not as critical for you as I initially > thought. I consider this "important, but not urgent" for Security panel now. > Would you like it in M51 due to DHE?? Oh, sorry, I might have been unclear. This is unrelated to DHE. Deprecating DHE (I'll send the email shortly, got distracted staring at assembly) is a "this is going to be removed imminently" thing. I was planning on putting a JS console message, because that's what the Blink deprecation process says. That should be a JS console message because it's a fairly strong signal. Your site *will* break. I don't expect that site administrators will idly open DevTools AND idly go to the security panel. (I don't expect they will idly open DevTools either, but it's the best surface we've got that doesn't start messing with UI for users. And it aligns with Blink practice elsewhere.) Modern vs. obsolete[*] is different. In parallel with stuff that's finally low enough that we can remove, we should also nudge people towards best practices. Not everything that is "obsolete" is "deprecated". "Obsolete" is everything that's not "modern" or ECDHE + AEAD. This is consistent the HTTPS transparency report's definition of "modern". IF an admin is proactive enough to open DevTools AND go to the security panel, I think it's reasonable to point out best practices to them. The old OIB information, although confusing, did this. I think this something we lost by moving to the security panel. (Not to disparage what you've done with the security panel. The security panel is great! :-) ) [*] Feel free to quibble on the exact wording.
On 2016/03/22 at 23:13:24, davidben wrote: > On 2016/03/22 23:05:55, lgarron wrote: > > On 2016/03/22 at 18:36:59, davidben wrote: > > > Did anything ever happen with this CL? M50 seems to have branched with the > > change to the OIB but still with the obsolete/modern regression. > > > > I de-prioritized it after learning it was not as critical for you as I initially > > thought. I consider this "important, but not urgent" for Security panel now. > > Would you like it in M51 due to DHE?? > > Oh, sorry, I might have been unclear. This is unrelated to DHE. > > Deprecating DHE (I'll send the email shortly, got distracted staring at assembly) is a "this is going to be removed imminently" thing. I was planning on putting a JS console message, because that's what the Blink deprecation process says. That should be a JS console message because it's a fairly strong signal. Your site *will* break. I don't expect that site administrators will idly open DevTools AND idly go to the security panel. (I don't expect they will idly open DevTools either, but it's the best surface we've got that doesn't start messing with UI for users. And it aligns with Blink practice elsewhere.) > > Modern vs. obsolete[*] is different. In parallel with stuff that's finally low enough that we can remove, we should also nudge people towards best practices. Not everything that is "obsolete" is "deprecated". "Obsolete" is everything that's not "modern" or ECDHE + AEAD. This is consistent the HTTPS transparency report's definition of "modern". IF an admin is proactive enough to open DevTools AND go to the security panel, I think it's reasonable to point out best practices to them. The old OIB information, although confusing, did this. I think this something we lost by moving to the security panel. (Not to disparage what you've done with the security panel. The security panel is great! :-) ) > > [*] Feel free to quibble on the exact wording. Sorry for conflating them; you pinged me about DHE, so I assumed this ping was related. Is it okay if I get to this next week?
Description was changed from ========== Surface an obsolete cipher suite warning in the Security panel overview. BUG=551728 TEST=Visit cbc.badssl.com, open DevTools to the Security panel, and make sure the Overview shows a gray bullet point saying "Obsolete TLS Settings". ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Add separators to the info explanations if there are multiple ones. - Always place info explanations at the end. BUG=551728 TEST=For all 5 test below test, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure TLS connection - The connection to this site is encrypted and authenticated using a strong protocol version (QUIC), key exchange (ECDHE_RSA), and cipher suite (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray "info" bullet point with the following summary and description: - Obsolete TLS cipher suite - The connection to this site uses a modern protocol (TLS 1.2), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1 ). 3) Visit dh2048.badssl.com and check that there is a gray "info" bullet point with the following summary and description: - Obsolete TLS key exchange - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (DHE_RSA), and a modern cipher suite (AES_128_GCM ). 4) Visit rc4.badssl.comcom and check that there are only two bullet points, and neither mentions TLS directly. 5) Visit cbc.badssl.com/mixed/script/ and check that there are two gray "info" bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". There should be a light gray line below each. ==========
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Add separators to the info explanations if there are multiple ones. - Always place info explanations at the end. BUG=551728 TEST=For all 5 test below test, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure TLS connection - The connection to this site is encrypted and authenticated using a strong protocol version (QUIC), key exchange (ECDHE_RSA), and cipher suite (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray "info" bullet point with the following summary and description: - Obsolete TLS cipher suite - The connection to this site uses a modern protocol (TLS 1.2), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1 ). 3) Visit dh2048.badssl.com and check that there is a gray "info" bullet point with the following summary and description: - Obsolete TLS key exchange - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (DHE_RSA), and a modern cipher suite (AES_128_GCM ). 4) Visit rc4.badssl.comcom and check that there are only two bullet points, and neither mentions TLS directly. 5) Visit cbc.badssl.com/mixed/script/ and check that there are two gray "info" bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". There should be a light gray line below each. ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Add separators to the info explanations if there are multiple ones. - Always place info explanations at the end. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure TLS connection - The connection to this site is encrypted and authenticated using a strong protocol version (QUIC), key exchange (ECDHE_RSA), and cipher suite (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS cipher suite - The connection to this site uses a modern protocol (TLS 1.2), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1 ). 3) Visit dh2048.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS key exchange - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (DHE_RSA), and a modern cipher suite (AES_128_GCM ). 4) Visit rc4.badssl.com and check that there are only two bullet points, and neither mentions TLS directly. 5) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". There should be a light gray line below each. 6) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS key exchange and cipher suite - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1). 7) Visit https://tls-v1.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS protocol and cipher suite - The connection to this site uses an obsolete protocol (TLS 1.0), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_128_CBC with HMAC-SHA1). ==========
Patchset #3 (id:40001) has been deleted
davidben@: Could you review? Based on developer feedback, I've tried to find a good way to: 1) always show the protocol, key exchange, and cipher suite in the Security panel overview, and 2) highlight the obsolete settings. See https://crbug.com/551728#c16 for screenshots. https://codereview.chromium.org/1727133002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:14324: + <message name="IDS_OBSOLETE_PROTOCOL_AND_CIPHERSUITE" desc="Summary phrase for a site that uses a modern, secure TLS protocol version and ciphersuite."> On 2016/02/24 at 02:10:50, estark wrote: > Both of these desc attributes should say "obsolete" instead of "modern, secure" Done. https://codereview.chromium.org/1727133002/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1727133002/diff/1/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:1411: ); On 2016/02/24 at 02:10:50, estark wrote: > formatting looks weird here The last I've moved the final ); onto the previous line. https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14345: The connection to this site is encrypted and authenticated using a strong protocol (<ph name="PROTOCOL_VERSION">$1<ex>TLS 1.2</ex></ph>), key exchange (<ph name="KEY_EXCHANGE">$2<ex>ECDHE_RSA</ex></ph>), and cipher suite (<ph name="CIPHER_SUTE">$3<ex>AES_128_GCM</ex></ph>). I'm referring to "TLS", but the connection might actually be QUIC. Do you think it's okay to gloss over that detail? https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.cc (right): https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:365: int ObsoleteSSLStatus(int connection_status) { I think that this is the right level of abstraction (a single function call to get all 3 mask bits, including protocol). I would move this function to another file like net/ssl/obsolete_ssl_stats.{h, cc}, but that requires exposing GetCipherProperties() in ssl_cipher_suite_names.h. The values in the switch statements would also become even-more-magic numbers, given that the cipher suite listing is in ssl_cipher_suite_names.cc Do you have a preference?
Also note that I haven't updated tests. I'm gonna start with a dry run just to see what breaks.
The CQ bit was checked by lgarron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1727133002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1727133002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
(Catching up on everything from IETF, so I only gave this a cursory look right now.) https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14345: The connection to this site is encrypted and authenticated using a strong protocol (<ph name="PROTOCOL_VERSION">$1<ex>TLS 1.2</ex></ph>), key exchange (<ph name="KEY_EXCHANGE">$2<ex>ECDHE_RSA</ex></ph>), and cipher suite (<ph name="CIPHER_SUTE">$3<ex>AES_128_GCM</ex></ph>). On 2016/04/09 03:22:50, lgarron wrote: > I'm referring to "TLS", but the connection might actually be QUIC. Do you think > it's okay to gloss over that detail? *shrug* The description string doesn't end up in the UI anyway. https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14353: <message name="IDS_TWO_OBSOLETE_TLS_SETTINGS" desc="Summary phrase for a site that uses two of an outdated TLS protocol, key exchange, or ciphersuite." translateable="false"> Nit: you say ciphersuite sometimes and cipher suite other times. https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14354: Obsolete TLS <ph name="FIRST_SETTING">$1<ex>protocol</ex></ph> and <ph name="SECOND_SETTING">$2<ex>cipher suite</ex></ph> cipher suite -> bulk cipher or maybe just "cipher". "cipher suite" refers to the entire combination. (Hence "suite".) Ditto elsewhere. https://codereview.chromium.org/1727133002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1727133002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:29: #include "base/strings/utf_string_conversions.h" Duplicate include https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.h (left): https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:58: NET_EXPORT bool IsSecureTLSCipherSuite(uint16_t cipher_suite); This function has a ton of callers you need to update.
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Add separators to the info explanations if there are multiple ones. - Always place info explanations at the end. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure TLS connection - The connection to this site is encrypted and authenticated using a strong protocol version (QUIC), key exchange (ECDHE_RSA), and cipher suite (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS cipher suite - The connection to this site uses a modern protocol (TLS 1.2), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1 ). 3) Visit dh2048.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS key exchange - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (DHE_RSA), and a modern cipher suite (AES_128_GCM ). 4) Visit rc4.badssl.com and check that there are only two bullet points, and neither mentions TLS directly. 5) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". There should be a light gray line below each. 6) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS key exchange and cipher suite - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1). 7) Visit https://tls-v1.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS protocol and cipher suite - The connection to this site uses an obsolete protocol (TLS 1.0), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure TLS connection - The connection to this site is encrypted and authenticated using a strong protocol version (QUIC), key exchange (ECDHE_RSA), and cipher suite (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS cipher suite - The connection to this site uses a modern protocol (TLS 1.2), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1 ). 3) Visit dh2048.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS key exchange - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (DHE_RSA), and a modern cipher suite (AES_128_GCM ). 4) Visit rc4.badssl.com and check that there are only two bullet points, and neither mentions TLS directly. 5) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". There should be a light gray line below each. 6) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS key exchange and cipher suite - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1). 7) Visit https://tls-v1.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS protocol and cipher suite - The connection to this site uses an obsolete protocol (TLS 1.0), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_128_CBC with HMAC-SHA1). ==========
davidben@: This is now ready for full review. Could you review? See https://crbug.com/551728#c17 (comment 17, not comment 16) for updated screenshots. https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14345: The connection to this site is encrypted and authenticated using a strong protocol (<ph name="PROTOCOL_VERSION">$1<ex>TLS 1.2</ex></ph>), key exchange (<ph name="KEY_EXCHANGE">$2<ex>ECDHE_RSA</ex></ph>), and cipher suite (<ph name="CIPHER_SUTE">$3<ex>AES_128_GCM</ex></ph>). On 2016/04/11 at 16:47:01, davidben (OOO 4-4 to 4-11) wrote: > On 2016/04/09 03:22:50, lgarron wrote: > > I'm referring to "TLS", but the connection might actually be QUIC. Do you think > > it's okay to gloss over that detail? > > *shrug* The description string doesn't end up in the UI anyway. I'm not sure what you mean. It shows up in the first screenshot at https://crbug.com/551728#c16 (for www.google.com). https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14353: <message name="IDS_TWO_OBSOLETE_TLS_SETTINGS" desc="Summary phrase for a site that uses two of an outdated TLS protocol, key exchange, or ciphersuite." translateable="false"> On 2016/04/11 at 16:47:01, davidben (OOO 4-4 to 4-11) wrote: > Nit: you say ciphersuite sometimes and cipher suite other times. Good catch, fixed. (I used to write "ciphersuite" and recently switched to "cipher suite", but I'm still getting used to the more popular usage.) https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14354: Obsolete TLS <ph name="FIRST_SETTING">$1<ex>protocol</ex></ph> and <ph name="SECOND_SETTING">$2<ex>cipher suite</ex></ph> On 2016/04/11 at 16:47:01, davidben (OOO 4-4 to 4-11) wrote: > cipher suite -> bulk cipher or maybe just "cipher". "cipher suite" refers to the entire combination. (Hence "suite".) Ditto elsewhere. The origin view in the Security panel say "Cipher Suite" rather than "Cipher". I'd like this to be consistent. Would you prefer to change both to "Cipher"? https://codereview.chromium.org/1727133002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1727133002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:29: #include "base/strings/utf_string_conversions.h" On 2016/04/11 at 16:47:01, davidben (OOO 4-4 to 4-11) wrote: > Duplicate include *shakes fist at C++ and shakes hand with `goimports`* (Removed.) https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.cc (right): https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:365: int ObsoleteSSLStatus(int connection_status) { On 2016/04/09 at 03:22:50, lgarron wrote: > I think that this is the right level of abstraction (a single function call to get all 3 mask bits, including protocol). > > I would move this function to another file like net/ssl/obsolete_ssl_stats.{h, cc}, but that requires exposing GetCipherProperties() in ssl_cipher_suite_names.h. The values in the switch statements would also become even-more-magic numbers, given that the cipher suite listing is in ssl_cipher_suite_names.cc > > Do you have a preference? ping https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.h (left): https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:58: NET_EXPORT bool IsSecureTLSCipherSuite(uint16_t cipher_suite); On 2016/04/11 at 16:47:01, davidben (OOO 4-4 to 4-11) wrote: > This function has a ton of callers you need to update. Yeah, I left them out for this patch because most of them are tests. I've updated them in the latest patch.
davidben@: This is now ready for full review. Could you review? See https://crbug.com/551728#c17 (comment 17, not comment 16) for updated screenshots. https://codereview.chromium.org/1727133002/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1727133002/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:29: #include "base/strings/utf_string_conversions.h" On 2016/04/11 at 16:47:01, davidben (OOO 4-4 to 4-11) wrote: > Duplicate include *shakes fist at C++ and shakes hand with `goimports`* (Removed.) https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.cc (right): https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:365: int ObsoleteSSLStatus(int connection_status) { On 2016/04/09 at 03:22:50, lgarron wrote: > I think that this is the right level of abstraction (a single function call to get all 3 mask bits, including protocol). > > I would move this function to another file like net/ssl/obsolete_ssl_stats.{h, cc}, but that requires exposing GetCipherProperties() in ssl_cipher_suite_names.h. The values in the switch statements would also become even-more-magic numbers, given that the cipher suite listing is in ssl_cipher_suite_names.cc > > Do you have a preference? ping https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.h (left): https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:58: NET_EXPORT bool IsSecureTLSCipherSuite(uint16_t cipher_suite); On 2016/04/11 at 16:47:01, davidben (OOO 4-4 to 4-11) wrote: > This function has a ton of callers you need to update. Yeah, I left them out for this patch because most of them are tests. I've updated them in the latest patch.
davidben@: Gentle ping. :-) I don't mind taking time for this if we still need to iterate, but it would be nice to land this by Tuesday if it's almost ready.
lgarron@chromium.org changed reviewers: + pfeldman@chromium.org
pfeldman, could you review //content (2 files) and //third_part/WebKit (2 files)?
lgarron@chromium.org changed reviewers: + msw@chromium.org
msw@chromium.org: Could you also review //chrome (2 files)?
On 2016/04/15 22:33:33, lgarron wrote: > mailto:msw@chromium.org: Could you also review //chrome (2 files)? Sorry, I don't think I'm a qualified to review those changes to browser.cc
On 2016/04/15 at 22:40:27, msw wrote: > On 2016/04/15 22:33:33, lgarron wrote: > > mailto:msw@chromium.org: Could you also review //chrome (2 files)? > > Sorry, I don't think I'm a qualified to review those changes to browser.cc Hmm, you reviewed some of the surrounding code. estark@ is best-equipped to review those changes (she wrote the surrounding code), but she's not an OWNER of them. Should I find another owner, or would it be okay to have her review that part and TBR you?
On 2016/04/15 22:50:07, lgarron wrote: > On 2016/04/15 at 22:40:27, msw wrote: > > On 2016/04/15 22:33:33, lgarron wrote: > > > mailto:msw@chromium.org: Could you also review //chrome (2 files)? > > > > Sorry, I don't think I'm a qualified to review those changes to browser.cc > > Hmm, you reviewed some of the surrounding code. > estark@ is best-equipped to review those changes (she wrote the surrounding > code), but she's not an OWNER of them. > > Should I find another owner, or would it be okay to have her review that part > and TBR you? I can do a fairly stampy OWNERS review after estark@ does a proper review.
This looks pretty reasonable to me, mostly nits inline. Didn't look too closely at the //net stuff. I don't think we should land this without tests though (ideally both devtools tests and unit- or browser-testing Browser::GetSecurityStyle). https://codereview.chromium.org/1727133002/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:14347: <message name="IDS_CIPHERSUITE_WITH_MAC" desc="Description of a cipher suite that contains a seperate cipher and MAC." translateable="false"> seperate -> separate https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1320: content::SecurityStyle Browser::GetSecurityStyle( This function is getting pretty huge. We should probably move it in to chrome/browser/ssl, either as a utility function in its own file, or maybe as a method on ChromeSecurityStateModelClient. That can be a separate CL though. Filed bug 604339. https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1425: const base::string16 cipher_suite_name = Why do these all need to be string16s? https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1438: } else if (security_info.cert_id != 0) { Can you please explain this check in a comment? It doesn't look like we check for a 0 cert_id elsewhere, so I'm wondering why we need to here. https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1441: Hmmmm, reading this constructing-the-string code hurts my brain. It would probably help if you separated it out into a helper function. Other optional suggestions for possible simplifications: 1.) Are you sure we need the summary string to be so descriptive? Could it just be "Obsolete TLS settings" or "Secure TLS connection", rather than actually specifying the obsolete settings in the summary? 2.) Maybe factor out yet another helper function for the repetitive code in lines 1442-1451, 1453-1463, and 1465-1474. Or, this totally might be personal preference, but I think I might find this easier to read if you defined more strings, and had OBSOLETE_PROTOCOL, MODERN_PROTOCOL, OBSOLETE_KEY_EXCHANGE, MODERN_KEY_EXCHANGE, OBSOLETE_CIPHER, MODERN_CIPHER, instead of combining obsolete/modern with protocol/key exchange/cipher for each one. https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/website_settings.cc:68: #include "net/ssl/ssl_cipher_suite_names.h" Duplicate of the line above it. https://codereview.chromium.org/1727133002/diff/80001/components/security_sta... File components/security_state/security_state_model.h (right): https://codereview.chromium.org/1727133002/diff/80001/components/security_sta... components/security_state/security_state_model.h:107: // True if the protocol version and ciphersuite for the connection Update comment. https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:1273: IsObsoleteTLSCipherSuite(channel_info.cipherSuite)) { I thought this change would be unnecessary based on patch set 4's description, but it doesn't look like patch set 4 actually reintroduces IsObsoleteTLSCipherSuite, so now I'm confused. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:301: if (ssl_version < net::SSL_CONNECTION_VERSION_TLS1_2) { nit: no curly braces https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.h (right): https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:58: // |OBSOLETE_SSL_CIPHER_SUITE| indicates to obsolete cipher. "to" -> "an", maybe? not sure what it's supposed to say but it doesn't make sense :) also looks like it should be |OBSOLETE_SSL_MASK_CIPHER|, not |OBSOLETE_SSL_CIPHER_SUITE| https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:78: // the |cipher_suite| and returns true if either the key exchange of the cipher "of the cipher" -> "or the cipher" https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names_unittest.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names_unittest.cc:167: // Cartesian combos optional nit: you might be able to use TEST_P with INSTANTIATE_TEST_CASE_P to write this test more easily, here's an example: https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_conn... (I think the main advantage in this case is that it would be easier to review for correctness.) https://codereview.chromium.org/1727133002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1727133002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:578: console.log(parent); remove
https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:1273: IsObsoleteTLSCipherSuite(channel_info.cipherSuite)) { On 2016/04/18 11:46:44, estark wrote: > I thought this change would be unnecessary based on patch set 4's description, > but it doesn't look like patch set 4 actually reintroduces > IsObsoleteTLSCipherSuite, so now I'm confused. Also, presumably there's a corresponding change needed in ssl_client_socket_openssl.cc?
https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14345: The connection to this site is encrypted and authenticated using a strong protocol (<ph name="PROTOCOL_VERSION">$1<ex>TLS 1.2</ex></ph>), key exchange (<ph name="KEY_EXCHANGE">$2<ex>ECDHE_RSA</ex></ph>), and cipher suite (<ph name="CIPHER_SUTE">$3<ex>AES_128_GCM</ex></ph>). On 2016/04/12 02:25:07, lgarron wrote: > On 2016/04/11 at 16:47:01, davidben (OOO 4-4 to 4-11) wrote: > > On 2016/04/09 03:22:50, lgarron wrote: > > > I'm referring to "TLS", but the connection might actually be QUIC. Do you > think > > > it's okay to gloss over that detail? > > > > *shrug* The description string doesn't end up in the UI anyway. > > I'm not sure what you mean. It shows up in the first screenshot at > https://crbug.com/551728#c16 (for http://www.google.com). Oh, I see. Why not "Secure connection"? I don't terribly mind "Secure TLS connection" for QUIC, but I'm sure someone will file a bug for you here (just like all the "however" stuff was just asking for reports), and the rest of the UI just says "connection". ("Your connection to this site is private", etc.) https://codereview.chromium.org/1727133002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:14354: Obsolete TLS <ph name="FIRST_SETTING">$1<ex>protocol</ex></ph> and <ph name="SECOND_SETTING">$2<ex>cipher suite</ex></ph> On 2016/04/12 02:25:07, lgarron wrote: > On 2016/04/11 at 16:47:01, davidben (OOO 4-4 to 4-11) wrote: > > cipher suite -> bulk cipher or maybe just "cipher". "cipher suite" refers to > the entire combination. (Hence "suite".) Ditto elsewhere. > > The origin view in the Security panel say "Cipher Suite" rather than "Cipher". > I'd like this to be consistent. Would you prefer to change both to "Cipher"? Yeah, let's change them both to "cipher". "Cipher suite" is a very specific TLS-specific notion. We shouldn't muddy the waters by using it for the AEAD / bulk cipher + MAC. https://en.wikipedia.org/wiki/Cipher_suite https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.cc (right): https://codereview.chromium.org/1727133002/diff/60001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:365: int ObsoleteSSLStatus(int connection_status) { On 2016/04/12 02:25:08, lgarron wrote: > On 2016/04/09 at 03:22:50, lgarron wrote: > > I think that this is the right level of abstraction (a single function call to > get all 3 mask bits, including protocol). > > > > I would move this function to another file like net/ssl/obsolete_ssl_stats.{h, > cc}, but that requires exposing GetCipherProperties() in > ssl_cipher_suite_names.h. The values in the switch statements would also become > even-more-magic numbers, given that the cipher suite listing is in > ssl_cipher_suite_names.cc > > > > Do you have a preference? > > ping I'm a little sad about this connection_status thing, but I don't have a clear alternate suggestion if you insist on including the protocol. If not, the protocol check is redundant. Though I suppose we will be happier if you're TLS 1.2 with an insecure cipher suite than TLS 1.1 since we can get a tiny bit closer to ridding ourselves of SHA-1. https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:1273: IsObsoleteTLSCipherSuite(channel_info.cipherSuite)) { On 2016/04/18 11:47:35, estark wrote: > On 2016/04/18 11:46:44, estark wrote: > > I thought this change would be unnecessary based on patch set 4's description, > > but it doesn't look like patch set 4 actually reintroduces > > IsObsoleteTLSCipherSuite, so now I'm confused. > > Also, presumably there's a corresponding change needed in > ssl_client_socket_openssl.cc? BoringSSL does the False Start check internally. That NPN has a callback is due to some silly NSS politics if I recall. Aside: ssl_client_socket_nss.cc is doomed to be deleted real soon now once https://codereview.chromium.org/1882433002/ lands. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.h (right): https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:52: // to make it very clear that any obsolete bit makes the whole mask obsolete. I wouldn't bother with the second sentence. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:66: // which ones are "obsolete". Nit: no quotes (you already used it without quotes in line 50) https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:80: NET_EXPORT bool IsObsoleteTLSCipherSuite(int cipher_suite); I realize I was the one who said you needed to add this back, but I didn't realize how far we were along to killing the NSS code, so I guess it's not actually going to be needed. Nit: If you do keep this around, why the negation? IsSecureTLSCipherSuite is a fine name and IsObsoleteTLSCipherSuite causes an unfortunate negation. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names_unittest.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names_unittest.cc:13: int MakeConnectionStatus(int version, uint16_t cipher_suite) { Nit: Move this after line 25 and remove the net:: prefixes. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names_unittest.cc:123: uint16_t cipher_suite = 0xc02f; /* TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 */ Nit: static const uint16_t kModernCipherSuite = ...; (Both because it's a constant and so it's clear from reading the call sites that this cipher is already modern.) https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names_unittest.cc:152: 0xc5; /* TLS_DH_anon_WITH_CAMELLIA_256_CBC_SHA256 */ Nit: Let's do RSA with AES_128_CBC which is a much much more plausible input. :-)
https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:1273: IsObsoleteTLSCipherSuite(channel_info.cipherSuite)) { On 2016/04/19 17:47:01, davidben wrote: > On 2016/04/18 11:47:35, estark wrote: > > On 2016/04/18 11:46:44, estark wrote: > > > I thought this change would be unnecessary based on patch set 4's > description, > > > but it doesn't look like patch set 4 actually reintroduces > > > IsObsoleteTLSCipherSuite, so now I'm confused. > > > > Also, presumably there's a corresponding change needed in > > ssl_client_socket_openssl.cc? > > BoringSSL does the False Start check internally. That NPN has a callback is due > to some silly NSS politics if I recall. > > Aside: ssl_client_socket_nss.cc is doomed to be deleted real soon now once > https://codereview.chromium.org/1882433002/ lands. Oh, gotcha, ok. Confusion about patch set 4 description not matching the code still stands though.
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure TLS connection - The connection to this site is encrypted and authenticated using a strong protocol version (QUIC), key exchange (ECDHE_RSA), and cipher suite (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS cipher suite - The connection to this site uses a modern protocol (TLS 1.2), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1 ). 3) Visit dh2048.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS key exchange - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (DHE_RSA), and a modern cipher suite (AES_128_GCM ). 4) Visit rc4.badssl.com and check that there are only two bullet points, and neither mentions TLS directly. 5) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". There should be a light gray line below each. 6) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS key exchange and cipher suite - The connection to this site uses a modern protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher suite (AES_256_CBC with HMAC-SHA1). 7) Visit https://tls-v1.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete TLS protocol and cipher suite - The connection to this site uses an obsolete protocol (TLS 1.0), a modern key exchange (ECDHE_RSA), and an obsolete cipher suite (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit dh2048.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (DHE_RSA), and a strong cipher (AES_128_GCM). 4) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 5) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 6) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 7) Visit https://tls-v1.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ==========
davidben@, estark@: Thanks for the good suggestions! I've simplified some stuff. Could you take another look when you have time? estark@: src/chrome/browser/ui/test/ and src/chrome/browser/ui/tests/ look pretty barren and it doesn't seem to be tested through either of its callees. I also don't know of a way to control SSL settings for layout test. Before I ask around, do you happen to know a good place to start? https://codereview.chromium.org/1727133002/diff/80001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/80001/chrome/app/generated_re... chrome/app/generated_resources.grd:14347: <message name="IDS_CIPHERSUITE_WITH_MAC" desc="Description of a cipher suite that contains a seperate cipher and MAC." translateable="false"> On 2016/04/18 at 11:46:44, estark wrote: > seperate -> separate Done! https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1425: const base::string16 cipher_suite_name = On 2016/04/18 at 11:46:44, estark wrote: > Why do these all need to be string16s? GetStringFUTF8() as well as GetStringFUTF16() take `string16`s. In particular, I have so many arguments that I have to pass a vector of replacements rather than positional arguments, and we only have the GetStringFUTF16() variant of the function that takes a vector [1] Ergo, I have to construct a std::vector<base::string16>. I didn't feel that this warranted a comment; do you think it would worth adding one? [1] https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/l10n/l10n_... https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1438: } else if (security_info.cert_id != 0) { On 2016/04/18 at 11:46:44, estark wrote: > Can you please explain this check in a comment? It doesn't look like we check for a 0 cert_id elsewhere, so I'm wondering why we need to here. The reason is net errors. If we don't include this check, visiting dh480.badssl.com will result in: "The connection to this site uses an obsolete protocol (???), an obsolete key exchange (NULL), and an obsolete cipher suite (NULL with NULL)." I've added a comment: > We avoid trying to show TLS details when we couldn't even establish a TLS > connection (e.g. for net errors). We check the cert_id to see if there > was a connection. https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1441: On 2016/04/18 at 11:46:44, estark wrote: > Hmmmm, reading this constructing-the-string code hurts my brain. It would probably help if you separated it out into a helper function. Other optional suggestions for possible simplifications: > > 1.) Are you sure we need the summary string to be so descriptive? Could it just be "Obsolete TLS settings" or "Secure TLS connection", rather than actually specifying the obsolete settings in the summary? > > 2.) Maybe factor out yet another helper function for the repetitive code in lines 1442-1451, 1453-1463, and 1465-1474. Or, this totally might be personal preference, but I think I might find this easier to read if you defined more strings, and had OBSOLETE_PROTOCOL, MODERN_PROTOCOL, OBSOLETE_KEY_EXCHANGE, MODERN_KEY_EXCHANGE, OBSOLETE_CIPHER, MODERN_CIPHER, instead of combining obsolete/modern with protocol/key exchange/cipher for each one. I've simplified things considerably, taking your string change suggestions from 1. and 2. I could still factor it out, but I think it's not bad right now. https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/1727133002/diff/80001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/website_settings.cc:68: #include "net/ssl/ssl_cipher_suite_names.h" On 2016/04/18 at 11:46:44, estark wrote: > Duplicate of the line above it. Removed. https://codereview.chromium.org/1727133002/diff/80001/components/security_sta... File components/security_state/security_state_model.h (right): https://codereview.chromium.org/1727133002/diff/80001/components/security_sta... components/security_state/security_state_model.h:107: // True if the protocol version and ciphersuite for the connection On 2016/04/18 at 11:46:44, estark wrote: > Update comment. Done. https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/socket/ssl_client_s... net/socket/ssl_client_socket_nss.cc:1273: IsObsoleteTLSCipherSuite(channel_info.cipherSuite)) { On 2016/04/19 at 21:31:47, estark wrote: > On 2016/04/19 17:47:01, davidben wrote: > > On 2016/04/18 11:47:35, estark wrote: > > > On 2016/04/18 11:46:44, estark wrote: > > > > I thought this change would be unnecessary based on patch set 4's > > description, > > > > but it doesn't look like patch set 4 actually reintroduces > > > > IsObsoleteTLSCipherSuite, so now I'm confused. > > > > > > Also, presumably there's a corresponding change needed in > > > ssl_client_socket_openssl.cc? > > > > BoringSSL does the False Start check internally. That NPN has a callback is due > > to some silly NSS politics if I recall. > > > > Aside: ssl_client_socket_nss.cc is doomed to be deleted real soon now once > > https://codereview.chromium.org/1882433002/ lands. > > Oh, gotcha, ok. Confusion about patch set 4 description not matching the code still stands though. Sorry, I made the patch description concise to the point of vagueness. :-( "Reintroduce IsSecureTLSCipherSuite() as its negative" meant "Introduce IsObsoleteTLSCipherSuite() to substitute for the purpose for which we used to have IsSecureTLSCipherSuite()." Anyhow, I'm glad we don't have to worry about it; the latest patch removes it. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:301: if (ssl_version < net::SSL_CONNECTION_VERSION_TLS1_2) { On 2016/04/18 at 11:46:44, estark wrote: > nit: no curly braces Done. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.h (right): https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:52: // to make it very clear that any obsolete bit makes the whole mask obsolete. On 2016/04/19 at 17:47:01, davidben wrote: > I wouldn't bother with the second sentence. Done. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:58: // |OBSOLETE_SSL_CIPHER_SUITE| indicates to obsolete cipher. On 2016/04/18 at 11:46:45, estark wrote: > "to" -> "an", maybe? not sure what it's supposed to say but it doesn't make sense :) > > also looks like it should be |OBSOLETE_SSL_MASK_CIPHER|, not |OBSOLETE_SSL_CIPHER_SUITE| This comment was more relevant before I added OBSOLETE_SSL_MASK_PROTOCOL. I've just gone ahead and removed the comment. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:66: // which ones are "obsolete". On 2016/04/19 at 17:47:01, davidben wrote: > Nit: no quotes (you already used it without quotes in line 50) Ack, but I've removed line 50, so I'll keep the quotes here for now. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:78: // the |cipher_suite| and returns true if either the key exchange of the cipher On 2016/04/18 at 11:46:44, estark wrote: > "of the cipher" -> "or the cipher" Good catch; function is now removed, though. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:80: NET_EXPORT bool IsObsoleteTLSCipherSuite(int cipher_suite); On 2016/04/19 at 17:47:01, davidben wrote: > I realize I was the one who said you needed to add this back, but I didn't realize how far we were along to killing the NSS code, so I guess it's not actually going to be needed. > > Nit: If you do keep this around, why the negation? IsSecureTLSCipherSuite is a fine name and IsObsoleteTLSCipherSuite causes an unfortunate negation. I was struggling with what to call this. Since the file with the only call site is gone now, this is finally moot. :-) https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names_unittest.cc (right): https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names_unittest.cc:13: int MakeConnectionStatus(int version, uint16_t cipher_suite) { On 2016/04/19 at 17:47:01, davidben wrote: > Nit: Move this after line 25 and remove the net:: prefixes. Done. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names_unittest.cc:123: uint16_t cipher_suite = 0xc02f; /* TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 */ On 2016/04/19 at 17:47:01, davidben wrote: > Nit: static const uint16_t kModernCipherSuite = ...; > > (Both because it's a constant and so it's clear from reading the call sites that this cipher is already modern.) Done. I'll also rename the vars below with that convention. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names_unittest.cc:152: 0xc5; /* TLS_DH_anon_WITH_CAMELLIA_256_CBC_SHA256 */ On 2016/04/19 at 17:47:01, davidben wrote: > Nit: Let's do RSA with AES_128_CBC which is a much much more plausible input. :-) Done. https://codereview.chromium.org/1727133002/diff/80001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names_unittest.cc:167: // Cartesian combos On 2016/04/18 at 11:46:45, estark wrote: > optional nit: you might be able to use TEST_P with INSTANTIATE_TEST_CASE_P to write this test more easily, here's an example: https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_conn... > > (I think the main advantage in this case is that it would be easier to review for correctness.) INSTANTIATE_TEST_CASE_P looks nice, but it seems to be written with the assumption that you're testing a variety of inputs for the *same* correct behaviour. In this case, I would want to use gtest's `Combine()` functionality with different expected outputs. I could compute the expected output based on the input, but that feels like the tail wagging the dog. I don't see an idiomatic way to call INSTANTIATE_TEST_CASE_P on custom tuples of input without basically replicating these calls, so I hope it's okay if I leave them as-is? https://codereview.chromium.org/1727133002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1727133002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:578: console.log(parent); On 2016/04/18 at 11:46:45, estark wrote: > remove Done.
On 2016/04/25 23:56:54, lgarron wrote: > davidben@, estark@: Thanks for the good suggestions! I've simplified some stuff. > Could you take another look when you have time? > > estark@: src/chrome/browser/ui/test/ and src/chrome/browser/ui/tests/ look > pretty barren and it doesn't seem to be tested through either of its callees. I > also don't know of a way to control SSL settings for layout test. Before I ask > around, do you happen to know a good place to start? Can you do something like LayoutTests/http/tests/inspector/security/security-blocked-mixed-content.html to simulate DevTools loading a page with info explanations and test that it displays it properly? Also, https://codereview.chromium.org/1919773005/ is an example of how to mock connection parameters; can you do something like that in browser_browsertest.cc to test this change? (You might want to just patch that CL on to a branch, rebase your changes on that branch, and then let mine land first, so that you don't have to rewrite all that boilerplate I wrote and can just reuse it.)
net lgtm https://codereview.chromium.org/1727133002/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:14437: + Secure Connection Nit: Previously this was sentence-case rather than title-case ("connection" was lowercase). I don't have opinions on which way it goes but wanted to make sure this was an intentional rather than accidental change. https://codereview.chromium.org/1727133002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:14443: + Obsolete Connection Settings Ditto.
Ping. Is this CL just waiting on tests?
On 2016/05/20 at 22:19:57, estark wrote: > Ping. Is this CL just waiting on tests? Yep. I was focusing on launching hstspreload and am sheriff now, but I want to write tests once I have time to get back to writing Chromium code.
On 2016/05/20 22:19:57, estark wrote: > Ping. Is this CL just waiting on tests? Is this ready for devtools review?
estark@: Could you review now? A layout test is not useful enough, since the synthesized description string is generated at a higher level that we can't control from layout tests. After trying to see if I can attach it to the SecurityHandler (not a great idea), I've opted to just test the string as part of SecurityStyleChangedObserverNonsecureConnection. https://codereview.chromium.org/1727133002/diff/100001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1727133002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:14437: + Secure Connection On 2016/04/29 at 21:44:13, davidben wrote: > Nit: Previously this was sentence-case rather than title-case ("connection" was lowercase). I don't have opinions on which way it goes but wanted to make sure this was an intentional rather than accidental change. Intentional, since all the others are title case right now. https://codereview.chromium.org/1727133002/diff/100001/chrome/app/generated_r... chrome/app/generated_resources.grd:14443: + Obsolete Connection Settings On 2016/04/29 at 21:44:13, davidben wrote: > Ditto. Intentional, since all the others are title case right now. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:160: "The connection to this site is encrypted and authenticated using a " estark@, do you know if it's okay to hardcode these parameter assumptions into a test? I don't want to drop this EXPECT_EQ unnecessarily, but it seems like the embedded test server isn't really set up for us to specify/sanity check SSL settings. An alternative would be to rewrite BrowserTestNonsecureURLRequest to allow specifying arbitrary TLS settings instead of hardcoding obsolete ones.
https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:111: } else { no need for else + indentation after return https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:112: // We avoid trying to show TLS details when we couldn't even establish a TLS nit: avoid "we" in comments https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:113: // connection (e.g. for net errors). We check the cert_id to see if there nit: |cert_id| https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:115: if (security_info.cert_id != 0) { prefer early return: if (security_info.cert_id == 0) return; ... https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:144: return; no return necessary here https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:160: "The connection to this site is encrypted and authenticated using a " On 2016/06/14 00:59:42, lgarron wrote: > estark@, do you know if it's okay to hardcode these parameter assumptions into a > test? I don't want to drop this EXPECT_EQ unnecessarily, but it seems like the > embedded test server isn't really set up for us to specify/sanity check SSL > settings. > > An alternative would be to rewrite BrowserTestNonsecureURLRequest to allow > specifying arbitrary TLS settings instead of hardcoding obsolete ones. Why hardcode the string instead of instantiating it with GetStringFUTF8? Also, instead of hardcoding the parameters, perhaps you could grab them from ChromeSecurityStateModelClient::FromWebContents(web_contents)->GetSecurityInfo()? https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:866: const int obsoleteTLSVersion = net::SSL_CONNECTION_VERSION_TLS1_1; should be named kObsoleteTLSVersion, I think (and line 868 as well) https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:868: const uint16_t obsoleteTLSJobCipherSuite = 0xc013; Why the "Job" in the name? https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1003: ASSERT_EQ(net::SSL_CONNECTION_VERSION_TLS1_1, obsoleteTLSVersion); These should both be EXPECT_EQ, unless I'm missing something. (ASSERT is for when the test would crash if it continued after failing an expectation, for example to check that a pointer is non-null before dereferencing it later on in the test.) https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1004: ASSERT_EQ(0xc013, obsoleteTLSJobCipherSuite); Huh. This strikes me as a little weird (asserting that the constants have particular values). Any reason not to instantiate the parameterized string with the constants? https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1007: "The connection to this site uses an obsolete protocol (TLS 1.1), " ditto about instantiating the parameterized string https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:12: #include <vector> All these new #includes seem unnecessary since they are the only changes in the file. Are they supposed to be in chrome_security_state_model_client.cc perhaps?
friendly ping. Are you still planning to work on this, Lucas?
friendly ping. (Someone on IRC was just asking why the "obsolete" indicator was lost.)
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit dh2048.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (DHE_RSA), and a strong cipher (AES_128_GCM). 4) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 5) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 6) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 7) Visit https://tls-v1.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 4) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 5) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 6) Visit https://tls-v1.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ==========
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 4) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 5) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 6) Visit https://tls-v1.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 4) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 5) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 6) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ==========
estark@, could you review? I think I've addressed everything you brought up. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:111: } else { On 2016/06/15 at 04:46:08, estark wrote: > no need for else + indentation after return Done. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:112: // We avoid trying to show TLS details when we couldn't even establish a TLS On 2016/06/15 at 04:46:08, estark wrote: > nit: avoid "we" in comments Done. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:113: // connection (e.g. for net errors). We check the cert_id to see if there On 2016/06/15 at 04:46:08, estark wrote: > nit: |cert_id| Done. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:115: if (security_info.cert_id != 0) { On 2016/06/15 at 04:46:08, estark wrote: > prefer early return: > > if (security_info.cert_id == 0) > return; > ... Done. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:144: return; On 2016/06/15 at 04:46:08, estark wrote: > no return necessary here Done. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:160: "The connection to this site is encrypted and authenticated using a " On 2016/06/15 at 04:46:08, estark wrote: > On 2016/06/14 00:59:42, lgarron wrote: > > estark@, do you know if it's okay to hardcode these parameter assumptions into a > > test? I don't want to drop this EXPECT_EQ unnecessarily, but it seems like the > > embedded test server isn't really set up for us to specify/sanity check SSL > > settings. > > > > An alternative would be to rewrite BrowserTestNonsecureURLRequest to allow > > specifying arbitrary TLS settings instead of hardcoding obsolete ones. > > Why hardcode the string instead of instantiating it with GetStringFUTF8? > > Also, instead of hardcoding the parameters, perhaps you could grab them from ChromeSecurityStateModelClient::FromWebContents(web_contents)->GetSecurityInfo()? Thanks for the tip. I've switched to instantiating the string based on the web_contents's security info. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:866: const int obsoleteTLSVersion = net::SSL_CONNECTION_VERSION_TLS1_1; On 2016/06/15 at 04:46:08, estark wrote: > should be named kObsoleteTLSVersion, I think (and line 868 as well) Done. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:868: const uint16_t obsoleteTLSJobCipherSuite = 0xc013; On 2016/06/15 at 04:46:08, estark wrote: > Why the "Job" in the name? Because it's the cipher suite used by the [URLRequest]ObsoleteTLSJob class. I've changed it to kObsoleteCipherSuite. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1003: ASSERT_EQ(net::SSL_CONNECTION_VERSION_TLS1_1, obsoleteTLSVersion); On 2016/06/15 at 04:46:08, estark wrote: > These should both be EXPECT_EQ, unless I'm missing something. (ASSERT is for when the test would crash if it continued after failing an expectation, for example to check that a pointer is non-null before dereferencing it later on in the test.) Hmm, I had the impression that EXPECT was for stuff we actually want to test, and ASSERT is to sanity-check stuff that would invalidate what we're actually trying to test. https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#as... seems to support that. I guess we might add more to the end of the test some day, and in that case it might be good to report multiple failures, so I've switched to EXPECT_EQ. https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1004: ASSERT_EQ(0xc013, obsoleteTLSJobCipherSuite); On 2016/06/15 at 04:46:08, estark wrote: > Huh. This strikes me as a little weird (asserting that the constants have particular values). Any reason not to instantiate the parameterized string with the constants? Okay, okay. :-) https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ui/brow... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1727133002/diff/120001/chrome/browser/ui/brow... chrome/browser/ui/browser.cc:12: #include <vector> On 2016/06/15 at 04:46:09, estark wrote: > All these new #includes seem unnecessary since they are the only changes in the file. Are they supposed to be in chrome_security_state_model_client.cc perhaps? Oh my, that's embarrassing. I'm gonna have a strong word with my rebasing skills. (Yes. I've gone ahead and [re]moved them from here.)
c/b/ssl and components/security_state lgtm https://codereview.chromium.org/1727133002/diff/140001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/1727133002/diff/140001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:179: EXPECT_EQ(TRUE, is_aead); nit: EXPECT_TRUE(is_aead)
https://codereview.chromium.org/1727133002/diff/140001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc (right): https://codereview.chromium.org/1727133002/diff/140001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:179: EXPECT_EQ(TRUE, is_aead); On 2016/08/08 18:01:38, estark wrote: > nit: EXPECT_TRUE(is_aead) Done.
lgarron@chromium.org changed reviewers: + felt@chromium.org
felt@: Could you review the 1-line change in chrome/browser/ui/website_settings/website_settings.cc?
The CQ bit was checked by lgarron@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...
change to chrome/browser/ui/website_settings/website_settings.cc lgtm
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 4) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 5) Visit usbank.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 6) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 4) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 5) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 6) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ==========
Patchset #10 (id:200001) has been deleted
The CQ bit was checked by lgarron@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...
estark@: Could you review the changes to c/s/security_state_model_unittest.cc? Pretty straightforward, but I'd rather be safe than sorry. (I didn't catch them the first time. :-( )
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
components/security_state/security_state_model_unittest.cc lgtm but looks like there are some red bots :(
The CQ bit was checked by lgarron@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lgarron@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lgarron@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org, felt@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1727133002/#ps270001 (title: "Expose all TLS settings in the Security panel overview, and call out individual obsolete settings.")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org, davidben@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1727133002/#ps290001 (title: "Rebasing.")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lgarron@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lgarron@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 #12 (id:250001) has been deleted
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. Also includes minor formatting changes in the DevTools Security panel overview: - Wrap descriptions at 400px. - Split the protocol-provided explanations from the locally-constructed extra explanations (can currently only be blocked mixed content). BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 4) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 5) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 6) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 4) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 5) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 6) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ==========
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit rc4.badssl.com and check that there are only two bullet points, and neither is "Secure Connection" of "Obsolete Connection Settings". 4) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 5) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 6) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 4) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 5) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ==========
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" with a white background and "Blocked mixed content" with gray background. 4) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 5) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". 4) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 5) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org, davidben@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1727133002/#ps350001 (title: "Also check that connection_status is not zero, which is the case for 3 browser tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Expose all TLS settings in the Security panel overview, and call out individual obsolete settings. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". 4) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 5) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose TLS settings in the Security panel overview, and call out individual obsolete settings. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". 4) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 5) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ==========
Message was sent while issue was closed.
Committed patchset #16 (id:350001)
Message was sent while issue was closed.
Description was changed from ========== Expose TLS settings in the Security panel overview, and call out individual obsolete settings. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". 4) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 5) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). ========== to ========== Expose TLS settings in the Security panel overview, and call out individual obsolete settings. BUG=551728 TEST=For all tests below, first open DevTools to the Security panel overview. 1) Visit google.com and check that there is a green bullet point with the following summary and description: - Secure Connection - The connection to this site is encrypted and authenticated using a strong protocol (QUIC), a strong key exchange (ECDHE_RSA), and a strong cipher (AES_128_GCM). 2) Visit cbc.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_256_CBC with HMAC-SHA1). 3) Visit cbc.badssl.com/mixed/script/ and check that there are two gray info bullet points: "Obsolete TLS cipher suite" and "Blocked mixed content". 4) Visit static-rsa.badssl.com and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses a strong protocol (TLS 1.2), an obsolete key exchange (RSA), and a strong cipher (AES_256_GCM). 5) Visit https://tls-v1-0.badssl.com:1010/ and check that there is a gray info bullet point with the following summary and description: - Obsolete Connection Settings - The connection to this site uses an obsolete protocol (TLS 1.0), a strong key exchange (ECDHE_RSA), and an obsolete cipher (AES_128_CBC with HMAC-SHA1). Committed: https://crrev.com/3e2c33e434d9832c8d4883c9264adb5be0e9bf68 Cr-Commit-Position: refs/heads/master@{#414341} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/3e2c33e434d9832c8d4883c9264adb5be0e9bf68 Cr-Commit-Position: refs/heads/master@{#414341} |
