Demote the Use SSL 3.0 and Use TLS 1.0 options from Under The Hood to

Demote the Use SSL 3.0 and Use TLS 1.0 options from Under The Hood to
command line switches.

BUG=93554
TEST=Use the --disable-tls1 and --disable-ssl3 options and verify they work.

[agl, 2011-08-23 22:24:40]

LGTM for the SSL changes, although you should see if wtc thinks otherwise.

[kevers, 2011-08-24 14:12:31]

LGTM for web UI changes.

[Chris Palmer, 2011-08-24 18:24:27]

Having kevers' LGTM, I just need an official LGTM from you, wtc, to land this
patch. Would you be so kind? :) Thanks!

[wtc, 2011-08-24 19:37:30]

LGTM.  I suggest some changes below before you check this in.

High-level comments:

1. When we add TLS 1.1 and 1.2 support to NSS, it will be
useful to have the "Use TLS 1.1" and "Use TLS 1.2" checkboxes.
So I am worried about the work to add those checkboxes.

As long as we keep the "Check for server certificate revocation"
checkbox around, it will be easy -- we can just imitate the
code for that checkbox.

2. I see another SSL/TLS preference, ssl.cipher_suites.blacklist,
which is even lower-level than the ssl.ssl3.enabled and
ssl.tls1.enabled preferences you removed.  It would be
good to find out how the ssl.cipher_suites.blacklist
preference is managed.  You don't need to do that in this
changelist.

Low-level comments follow.

http://codereview.chromium.org/7685019/diff/1/chrome/browser/net/ssl_config_s...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

http://codereview.chromium.org/7685019/diff/1/chrome/browser/net/ssl_config_s...
chrome/browser/net/ssl_config_service_manager_pref.cc:141: std::vector<uint16>
disabled_cipher_suites_;
It seems strange that ssl3_enabled_ and tls1_enabled_
are gone but disabled_cipher_suites_ is still here.

Is the disabled_cipher_suites_ preference managed in a
different way?  Perhaps it can only be changed by a group
policy.

http://codereview.chromium.org/7685019/diff/1/chrome/browser/net/ssl_config_s...
chrome/browser/net/ssl_config_service_manager_pref.cc:209:
!(CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableTLS1));
Nit: omit the outer parentheses ().

Line 207 and line 209 (continuation lines) should be indented
with four spaces.

Nit: Since the command-line options do not change, it seems that
we can call CommandLine::ForCurrentProcess()->HasSwitch()
just once in the constructor (line 155) and save the
results in class members.

http://codereview.chromium.org/7685019/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7685019/diff/1/chrome/common/chrome_switches.c...
chrome/common/chrome_switches.cc:294: const char kDisableSSL3[]                 
 = "disable-ssl3";
I don't think I will be using this command-line option.
We probably don't need to add it.

http://codereview.chromium.org/7685019/diff/1/chrome/common/chrome_switches.c...
chrome/common/chrome_switches.cc:332: // Disable TLS v1 (usually for testing
purposes).
Nit: v1 => 1.0

[Chris Palmer, 2011-08-24 22:57:04]

Thank you very much Wan-Teh! I'll fix the nits tomorrow and upload a
new version of the CL.

I also don't know about the cipher suites blacklist feature — but I'd
like to. If I get some free time maybe I'll look into it...


On Wed, Aug 24, 2011 at 12:37 PM,  <wtc@chromium.org> wrote:
> LGTM.  I suggest some changes below before you check this in.
>
> High-level comments:
>
> 1. When we add TLS 1.1 and 1.2 support to NSS, it will be
> useful to have the "Use TLS 1.1" and "Use TLS 1.2" checkboxes.
> So I am worried about the work to add those checkboxes.
>
> As long as we keep the "Check for server certificate revocation"
> checkbox around, it will be easy -- we can just imitate the
> code for that checkbox.
>
> 2. I see another SSL/TLS preference, ssl.cipher_suites.blacklist,
> which is even lower-level than the ssl.ssl3.enabled and
> ssl.tls1.enabled preferences you removed.  It would be
> good to find out how the ssl.cipher_suites.blacklist
> preference is managed.  You don't need to do that in this
> changelist.
>
> Low-level comments follow.
>
>
>
http://codereview.chromium.org/7685019/diff/1/chrome/browser/net/ssl_config_s...
> File chrome/browser/net/ssl_config_service_manager_pref.cc (right):
>
>
http://codereview.chromium.org/7685019/diff/1/chrome/browser/net/ssl_config_s...
> chrome/browser/net/ssl_config_service_manager_pref.cc:141:
> std::vector<uint16> disabled_cipher_suites_;
> It seems strange that ssl3_enabled_ and tls1_enabled_
> are gone but disabled_cipher_suites_ is still here.
>
> Is the disabled_cipher_suites_ preference managed in a
> different way?  Perhaps it can only be changed by a group
> policy.
>
>
http://codereview.chromium.org/7685019/diff/1/chrome/browser/net/ssl_config_s...
> chrome/browser/net/ssl_config_service_manager_pref.cc:209:
> !(CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableTLS1));
> Nit: omit the outer parentheses ().
>
> Line 207 and line 209 (continuation lines) should be indented
> with four spaces.
>
> Nit: Since the command-line options do not change, it seems that
> we can call CommandLine::ForCurrentProcess()->HasSwitch()
> just once in the constructor (line 155) and save the
> results in class members.
>
> http://codereview.chromium.org/7685019/diff/1/chrome/common/chrome_switches.cc
> File chrome/common/chrome_switches.cc (right):
>
>
http://codereview.chromium.org/7685019/diff/1/chrome/common/chrome_switches.c...
> chrome/common/chrome_switches.cc:294: const char kDisableSSL3[]
>         = "disable-ssl3";
> I don't think I will be using this command-line option.
> We probably don't need to add it.
>
>
http://codereview.chromium.org/7685019/diff/1/chrome/common/chrome_switches.c...
> chrome/common/chrome_switches.cc:332: // Disable TLS v1 (usually for
> testing purposes).
> Nit: v1 => 1.0
>
> http://codereview.chromium.org/7685019/
>



-- 
They who can give up general-purpose computing to obtain a little
temporary safety, deserve neither general-purpose computing nor
safety.

[Ryan Sleevi, 2011-08-24 23:36:10]

On 2011/08/24 22:57:04, Chris Palmer wrote:
> Thank you very much Wan-Teh! I'll fix the nits tomorrow and upload a
> new version of the CL.
> 
> I also don't know about the cipher suites blacklist feature — but I'd
> like to. If I get some free time maybe I'll look into it...
> 

http://crbug.com/58831

It is meant to be managed by enterprise policy. Use case is for something like
PCI-DSS, FIPS, general security.

[Chris Palmer, 2011-08-25 22:17:54]

Fixed wtc's nits, except for creating a new member to store CommandLine. I think
we have the sufficient LGTMs to commit this one now?

[wtc, 2011-08-26 18:33:27]

LGTM on Patch Set 2.

palmer: given the new info provided by rsleevi, I highly
recommend that you look into an alternative method he
used for the cipher suite blacklist.

In http://code.google.com/p/chromium/issues/detail?id=58831#c16
he mentioned something called CommandLinePrefStore, which
he used to parse a command-line option into the cipher
suite list blacklist preference.  See http://crrev.com/93778.
This seems like a good way to parse your command-line options,
which still keeping the "ssl.ssl3.enabled" and
"ssl.tls1.enabled" preferences internally.

[Chris Evans, 2011-08-26 20:09:27]

I'm going to re-trybot this now and then land on behalf of Chris P. I'll then
nominate him for provisional committer. Thanks!

On 2011/08/26 18:33:27, wtc wrote:
> LGTM on Patch Set 2.
> 
> palmer: given the new info provided by rsleevi, I highly
> recommend that you look into an alternative method he
> used for the cipher suite blacklist.
> 
> In http://code.google.com/p/chromium/issues/detail?id=58831#c16
> he mentioned something called CommandLinePrefStore, which
> he used to parse a command-line option into the cipher
> suite list blacklist preference.  See http://crrev.com/93778.
> This seems like a good way to parse your command-line options,
> which still keeping the "ssl.ssl3.enabled" and
> "ssl.tls1.enabled" preferences internally.