Read and Write SSL3, TLS1 and Cert revocation SSL preferences

Read and Write SSL3, TLS1 and Cert revocation SSL preferences
from local_state on Windows and Mac.

BUG=76551
TEST=testing SSL preferences with options UI
R=wtc
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80635

[ramant (doing other things), 2011-04-04 22:44:16]

Hi Wan-Teh and Matt,
  First cut at combining the code to get SSL preferences from local_state for
Windows and Mac. Wan-Teh and I had thought, we need not migrate the windows and
Mac preferences to local state. The initial values for the SSL preferences will
be true (checked).

  Will submit a separate CL to delete the following files after this is checked
in and is on canary for couple of days.

/net/base/ssl_config_service_mac.cc
/net/base/ssl_config_service_mac.h
/net/base/ssl_config_service_win.cc
/net/base/ssl_config_service_win.h
/net/base/ssl_config_service_mac_unittest.cc
/net/base/ssl_config_service_win_unittest.cc
/chrome/browser/net/ssl_config_service_manager_system.cc

  Will find out from Mark, if we should delete the following two prefs from Mac.


    org.chromium.ssl.ssl3
    org.chromium.ssl.tls1
 
  Would appreciate your comments.

thanks much,
raman

[Mark Mentovai, 2011-04-05 01:16:26]

Wow. It was a mistake to ever have used CFPreferences here, and I wasn’t aware
that this had been done.

You don’t need to remove these prefs if they already exist, you can just let
them live on harmlessly without effect. I don’t think you need to migrate the
old prefs to the new form either.

[wtc, 2011-04-05 18:40:34]

LGTM.

I suggest some changes below, mostly to fix nits.

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/net/ssl_confi...
File chrome/browser/net/ssl_config_service_manager.h (right):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/net/ssl_confi...
chrome/browser/net/ssl_config_service_manager.h:20: // of the manager. Get SSL
preferences from local_state object. On Linux, if
Remove "On Linux" because we will be running the migration
code on all platforms, even though it's not necessary for
Mac and Windows.

Please make sure it is harmless to run the migration code
blindly on Mac and Windows.

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/options/op...
File chrome/browser/ui/options/options_util.cc (left):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/options/op...
chrome/browser/ui/options/options_util.cc:41: #if defined(OS_LINUX) ||
defined(OS_FREEBSD) || defined(OS_OPENBSD)
IMPORTANT: should we keep this ifdef, since Mac and Windows
never had these preferences in user_prefs?

If we keep this ifdef, please add a comment to explain why
these prefs don't exist on Mac and Windows (i.e., historically
they were stored in system settings).

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (left):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.cc:331:
&AdvancedOptionsHandler::HandleUseTLS1Checkbox));
This block of code for OS_WIN (lines 323-331) is exactly the
same as the block of code for OS_LINUX/OS_FREEBSD/OS_OPENBSD
(lines 311-319) above.  Please understand whether they needed
to be separate, or it was simply an oversight.

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/advanced_options_handler.h (left):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.h:70:
defined(OS_OPENBSD)
Please understand why this ifdef has OS_WIN but not OS_MACOSX.
You don't need to explain it to me.  I just wanted to make sure
you fully understand this code.

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/advanced_options_handler.h (right):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.h:70: // user toggles
the "Check SSL Revocation" checkbox.
The names of the checkboxes are not exactly right.  They
should be:
  Check for server certificate revocation
  Use SSL 3.0
  Use TLS 1.0

http://codereview.chromium.org/6693077/diff/4002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6693077/diff/4002/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:285: // and used by NSS/OpenSSL using OSes.
Remove the comment "Currently, these are only present on and
used by NSS/OpenSSL using OSes."

Move these to line 700 below, in the LOCAL STATE section.

http://codereview.chromium.org/6693077/diff/4002/net/base/ssl_config_service.cc
File net/base/ssl_config_service.cc (right):

http://codereview.chromium.org/6693077/diff/4002/net/base/ssl_config_service....
net/base/ssl_config_service.cc:6: #include
"net/base/ssl_config_service_defaults.h"
Nit: please add a blank line after line 5 because
"net/base/ssl_config_service.h" is the primary header for
this class.  This is the style used in the example in the
Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/6693077/diff/4002/net/base/ssl_config_service....
net/base/ssl_config_service.cc:40: return new SSLConfigServiceDefaults;
Please add a comment to note that we don't use the system SSL
configuration any more.

This means we should be able to get rid of the
CreateSystemSSLConfigService method and simplify the code
further.  You don't need to do this cleanup in this CL.

[ramant (doing other things), 2011-04-05 21:01:36]

Thanks very much Wan-Teh. Made all the changes you have suggested.

-raman

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/net/ssl_confi...
File chrome/browser/net/ssl_config_service_manager.h (right):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/net/ssl_confi...
chrome/browser/net/ssl_config_service_manager.h:20: // of the manager. Get SSL
preferences from local_state object. On Linux, if
On 2011/04/05 18:40:37, wtc wrote:
> Remove "On Linux" because we will be running the migration
> code on all platforms, even though it's not necessary for
> Mac and Windows.
> 
> Please make sure it is harmless to run the migration code
> blindly on Mac and Windows.

Done.

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/options/op...
File chrome/browser/ui/options/options_util.cc (left):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/options/op...
chrome/browser/ui/options/options_util.cc:41: #if defined(OS_LINUX) ||
defined(OS_FREEBSD) || defined(OS_OPENBSD)
On 2011/04/05 18:40:37, wtc wrote:
> IMPORTANT: should we keep this ifdef, since Mac and Windows
> never had these preferences in user_prefs?
> 
> If we keep this ifdef, please add a comment to explain why
> these prefs don't exist on Mac and Windows (i.e., historically
> they were stored in system settings).

Hi Wan-Teh,
  We access these preferences in user_pref (or we do user_prefs->HasPrefPath)
during migration (though migration doesn't have any impact). Added a comment to
delete this code when we delete the migration code.

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (left):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.cc:331:
&AdvancedOptionsHandler::HandleUseTLS1Checkbox));
On 2011/04/05 18:40:37, wtc wrote:
> This block of code for OS_WIN (lines 323-331) is exactly the
> same as the block of code for OS_LINUX/OS_FREEBSD/OS_OPENBSD
> (lines 311-319) above.  Please understand whether they needed
> to be separate, or it was simply an oversight.

We knew we are going to cleanup. We added all Linux specific code with a
separate #ifdef. (We should have combined under one #ifdef)

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/advanced_options_handler.h (left):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.h:70:
defined(OS_OPENBSD)
On 2011/04/05 18:40:37, wtc wrote:
> Please understand why this ifdef has OS_WIN but not OS_MACOSX.
> You don't need to explain it to me.  I just wanted to make sure
> you fully understand this code.

We used to have special code in advanced_option_handler* for Windows, but not
for Mac. thanks for the keen eye Wan-Teh.

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/advanced_options_handler.h (right):

http://codereview.chromium.org/6693077/diff/4002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/advanced_options_handler.h:70: // user toggles
the "Check SSL Revocation" checkbox.
On 2011/04/05 18:40:37, wtc wrote:
> The names of the checkboxes are not exactly right.  They
> should be:
>   Check for server certificate revocation
>   Use SSL 3.0
>   Use TLS 1.0

Done.

http://codereview.chromium.org/6693077/diff/4002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6693077/diff/4002/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:285: // and used by NSS/OpenSSL using OSes.
On 2011/04/05 18:40:37, wtc wrote:
> Remove the comment "Currently, these are only present on and
> used by NSS/OpenSSL using OSes."
> 
> Move these to line 700 below, in the LOCAL STATE section.

Done.

http://codereview.chromium.org/6693077/diff/4002/net/base/ssl_config_service.cc
File net/base/ssl_config_service.cc (right):

http://codereview.chromium.org/6693077/diff/4002/net/base/ssl_config_service....
net/base/ssl_config_service.cc:6: #include
"net/base/ssl_config_service_defaults.h"
On 2011/04/05 18:40:37, wtc wrote:
> Nit: please add a blank line after line 5 because
> "net/base/ssl_config_service.h" is the primary header for
> this class.  This is the style used in the example in the
> Style Guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Done.

http://codereview.chromium.org/6693077/diff/4002/net/base/ssl_config_service....
net/base/ssl_config_service.cc:40: return new SSLConfigServiceDefaults;
On 2011/04/05 18:40:37, wtc wrote:
> Please add a comment to note that we don't use the system SSL
> configuration any more.
> 
> This means we should be able to get rid of the
> CreateSystemSSLConfigService method and simplify the code
> further.  You don't need to do this cleanup in this CL.

Done.

[wtc, 2011-04-06 00:33:28]

LGTM.

http://codereview.chromium.org/6693077/diff/10001/net/base/ssl_config_service.cc
File net/base/ssl_config_service.cc (right):

http://codereview.chromium.org/6693077/diff/10001/net/base/ssl_config_service...
net/base/ssl_config_service.cc:43: // further.
Please omit the second sentence, which promises "real soon",
unless someone really is going to do that.  You can change
that into a TODO comment, without "real soon".

Please discuss with mattm how this can be simplified.  It
seems that the indirection of SSLConfigServiceManager can
be eliminated...

If we don't have time to simplify the code, I guess we
can at least rename this method CreateSSLConfigServiceDefaults
to reflect the current implementation.  Note: it is
tempting to rename this method CreateDefaultSSLConfigService,
but I think it will be easily confused with
SSLConfigServiceManager::CreateDefaultManager.

[ramant (doing other things), 2011-04-06 01:30:03]

http://codereview.chromium.org/6693077/diff/10001/net/base/ssl_config_service.cc
File net/base/ssl_config_service.cc (right):

http://codereview.chromium.org/6693077/diff/10001/net/base/ssl_config_service...
net/base/ssl_config_service.cc:43: // further.
On 2011/04/06 00:33:28, wtc wrote:
> Please omit the second sentence, which promises "real soon",
> unless someone really is going to do that.  You can change
> that into a TODO comment, without "real soon".
> 
> Please discuss with mattm how this can be simplified.  It
> seems that the indirection of SSLConfigServiceManager can
> be eliminated...
> 
> If we don't have time to simplify the code, I guess we
> can at least rename this method CreateSSLConfigServiceDefaults
> to reflect the current implementation.  Note: it is
> tempting to rename this method CreateDefaultSSLConfigService,
> but I think it will be easily confused with
> SSLConfigServiceManager::CreateDefaultManager.

Done.

[wtc, 2011-04-06 02:10:03]

LGTM.  Please go ahead and check this in.  Thanks.

[Miranda Callahan, 2011-04-06 12:38:24]

On 2011/04/06 02:10:03, wtc wrote:
> LGTM.  Please go ahead and check this in.  Thanks.

Does this change mean that we can remove willchan's comment in io_thread.h?

    // NOTE(willchan): This request context is unusable until a system
    // SSLConfigService is provided that doesn't rely on
    // Profiles. Do NOT use this yet.

[wtc, 2011-04-06 14:28:54]

On 2011/04/06 12:38:24, Miranda Callahan wrote:
>
> Does this change mean that we can remove willchan's comment in io_thread.h?
> 
>     // NOTE(willchan): This request context is unusable until a system
>     // SSLConfigService is provided that doesn't rely on
>     // Profiles. Do NOT use this yet.

I think so.  But I think we also need to fix this TODO in io_thread.cc:

  // TODO(willchan): Use the real SSLConfigService.
  globals_->ssl_config_service =
      net::SSLConfigService::CreateSystemSSLConfigService();

We should have willchan update io_thread.{h,cc}.

[Miranda Callahan, 2011-04-06 19:37:43]

On 2011/04/06 14:28:54, wtc wrote:
> On 2011/04/06 12:38:24, Miranda Callahan wrote:
> >
> > Does this change mean that we can remove willchan's comment in io_thread.h?
> > 
> >     // NOTE(willchan): This request context is unusable until a system
> >     // SSLConfigService is provided that doesn't rely on
> >     // Profiles. Do NOT use this yet.
> 
> I think so.  But I think we also need to fix this TODO in io_thread.cc:
> 
>   // TODO(willchan): Use the real SSLConfigService.
>   globals_->ssl_config_service =
>       net::SSLConfigService::CreateSystemSSLConfigService();
> 
> We should have willchan update io_thread.{h,cc}.

Will, is this something you see yourself getting to soon? I'm trying to decide
whether to wait for this checkin before finishing my PromoResourceServer CL, so
I can use the system_url_request_context.

[willchan no longer on Chromium, 2011-04-07 09:56:50]

On Wed, Apr 6, 2011 at 9:37 PM, <mirandac@chromium.org> wrote:

> On 2011/04/06 14:28:54, wtc wrote:
>
>> On 2011/04/06 12:38:24, Miranda Callahan wrote:
>> >
>> > Does this change mean that we can remove willchan's comment in
>> io_thread.h?
>> >
>> >     // NOTE(willchan): This request context is unusable until a system
>> >     // SSLConfigService is provided that doesn't rely on
>> >     // Profiles. Do NOT use this yet.
>>
>
>  I think so.  But I think we also need to fix this TODO in io_thread.cc:
>>
>
>    // TODO(willchan): Use the real SSLConfigService.
>>   globals_->ssl_config_service =
>>       net::SSLConfigService::CreateSystemSSLConfigService();
>>
>
>  We should have willchan update io_thread.{h,cc}.
>>
>
> Will, is this something you see yourself getting to soon? I'm trying to
> decide
> whether to wait for this checkin before finishing my PromoResourceServer
> CL, so
> I can use the system_url_request_context.


I wouldn't wait for it. It should be a quick fix to plumb
system_url_request_context after this finishes.


>
>
> http://codereview.chromium.org/6693077/
>