Could you explain where you found it reading from global prefs? That's probably a bug, because it's supposed to be getting them from cloud policy (and we've got it documented as such)

We actually _want_ SSLConfigService to support per-profile prefs, to support things like cloud policies. This is especially important since our work towards isolating the certificate validation stack (controlled via the SSLConfig) is explicitly with a goal to support per-profile isolation on validation.

I think we should be passing along a config service, not the actual prefs, since it is supposed to support dynamic, cloud-policy updated state. This is especially important for things like SHA-1 deprecation, root cert deployment, or CT policies.

Matt and I chatted offline.

Turns out my assumption was incorrect, and the tests worked for reasons other than what I assumed.

The two places we initialize an SSLConfigServiceManager(Pref) are:
- https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_impl.cc?rcl=b8e90b623d45c0588550935f29dc2c0e080798aa&l=609
- https://cs.chromium.org/chromium/src/chrome/browser/io_thread.cc?rcl=b8e90b623d45c0588550935f29dc2c0e080798aa&l=354

The IOThread initializes the (global, shared) manager, and then per-profile there's an instantiation - but it just happens to copy from the g_browser_process local_state, rather than using the users' profile state (and thus, transitively, per-user cloud policy)

The assumption about things working/not working is that the g_browser_process local state properly picks up machine-level enterprise policies - and ChromeOS persists those (locally) so that it's available at next boot - but doesn't pick up per-user settings. It also works because these support dynamic_refresh (aka pref observers), and thus aren't static bools we push down but dynamically updatable.

This means that supporting per-profile/per-user settings would be _new_ development not yet supported, although it's new development we want for both upcoming features and to better support our existing policies.

For documentation sake, the current policies related to TLS/networking are:

- CertificateTransparencyEnforcementDisabledForUrls
- [hidden] CertificateTransparencyEnforcementEnabledForUrls
(These are both used in //components/certificate_transparency/ct_policy_manager.cc )

- EnableOnlineRevocationChecks
- RequireOnlineRevocationChecksForLocalAnchors
- EnableSha1ForLocalAnchors
- EnableCommonNameFallbackForLocalAnchors


We also have 'static' policies
- TLS 1.3 variant configuration (controlled via Finch)
- SSL version minimum and maximum (controlled via command-line)
- Disabled ciphersuites (controlled via command-line)

The static ones are because a bad config can prevent future updates, hence the added 'manual' configuration.

In terms of future work, the dynamic_refresh policies are ones we 'wanted' to support per-profile (especially for ChromeOS multi-profile). The CT one is a feature request, the others were due to limits of the OS APIs (which are no longer relevant as we move to our own validator).


I think we're good with Matt's proposal, with the caveat that we do want to have a path to 'push' config to a URLRequestContext (or equivalent), or potentially at a profile-level granularity.

(SSLVersionMax also affects it, as an escape hatch for enterprises with buggy middleboxes to turn off TLS 1.3. We've also had others in the past for temporarily undoing various deprecations as enterprises tend to be a little behind on these things.)

SSLConfigService::ProcessConfigUpdate ignores the case where only the tls13_variant of the SSLConfig changes.  Is that a bug?

The same is also true of signed_cert_timestamps_enabled (Which nothing seems to set to false?)

I'm not sure why signed_cert_timestamps_enabled should be of any interest to SSLConfigService? There are loads of fields that don't participate in SSLConfigService. tls13_variant perhaps should (+svaldez), though I'm guessing this only matters if we get some enterprise settings pushed and tls13_variant doesn't participate in enterprise settings. SSLConfigService itself has problems since it simultaneously serves both Finch, command-line flags, and enterprise settings.

(If you're adding this to the network service, please avoid ossifying it. This area is kind of a mess. SSLConfig is simutaneously URLRequestContext-level configuration and socket-level configuration.)

Yeah, I think that was an oversight, wrote up a CL to fix it.
https://chromium-review.googlesource.com/c/619834

Actually, we could probably just avoid this check altogether. The only caller is SSLConfigServiceManagerPrefs which listens for just the pref types it cares about. Supposing prefs is reasonable enough to ignore no-op settings changes on their end, nearly every applicable pref change will trigger an SSLConfig change. The filtering is just an optimization that easily misfires.

I'm guessing this dates to when we paid attention to platform SSL settings and signals are noisier?

@davidben: I don't believe prefs is reasonable in the case of AD syncs... but yeah, things used to be noisy, hence the filtering for no-ops :)

I had thought signed_cert_timestamps_enabled meant whether we enabled cert timestamps, as opposed to being a property of a connection.  The entire class is a mess, making it impossible to pick out what is part of global SSL configuration, and what is not.

Since we create the config from prefs, and need to pass it over to the NetworkService, I'll be sending the subset of SSLConfig that comes from prefs via a Mojo message - just the values SSLConfigService::ProcessConfigUpdate cares about, though.

We could use a Mojo-ified pref service (Which I don't think is available yet in production?)  But I'm reluctant to expose two pref services to the network service, and if we're only going to expose one, it would probably be the per-user prefs.

I believe I have a bug open for removing sct_enabled, but I can't find it. It used to be that we'd set that to false (on some platforms), which would disable requesting OCSP stapling info. However, we now request OCSP stapling on all platforms, unconditionally, and we resolved the issue w/ servers barfing on the extension, hence it's got no value :)

#c8: Not sure I follow the AD sync concern. The only property I'm thinking of is: if a pref used to be "Hello" and get set back to "Hello", will it run the callback? If no, it should be safe to remove the filtering. If yes, we should fix that (because it can be uniformly fixed once for all prefs), and then remove the filtering.

#c9: Indeed. Everything around SSLConfig is a mess, which is why it really should not be ossified. This mess extends all the way down to the socket pools, though I've cleaned up most of that.

> cleaned up most of that

Where by that I mean, "it still doesn't work, but at least we're not bifurcating the socket pools anymore".

With my change, we'll have a matching Mojo structure, but it will be easy to add/remove fields.  So changing it will be a bit more effort, but shouldn't be too bad.  I'll also be using the pref monitoring/SSLConfig creating stuff in components, so no redundant code there, either.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dca1c68b46759c54bf1bcfe70389a176a7b6f45f

commit dca1c68b46759c54bf1bcfe70389a176a7b6f45f
Author: Steven Valdez <svaldez@chromium.org>
Date: Thu Aug 17 23:10:22 2017

Add tls13_variant to ProcessConfigUpdate.

SSLConfigServicePref calls this function when any of the
prefs it uses changes. tls13_variant is one of these, so it should
trigger notifications. This change should be a no-op. These prefs
are set based on:
- Field trials (n/a because field trials set default values)
- Command-line flags (n/a because command-line flags are set
  early enough to not need a change signal)
- Admin policy (n/a because tls13_variant is not attached to
   any admin policies)

Bug:  755309 
Change-Id: I5cf86510755660cdc14dfc3e96481328c73de71a
Reviewed-on: https://chromium-review.googlesource.com/619834
Commit-Queue: Steven Valdez <svaldez@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495371}
[modify] https://crrev.com/dca1c68b46759c54bf1bcfe70389a176a7b6f45f/net/ssl/ssl_config_service.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0b07a98f2ba6ec6908ef1ea6ff5755607f2c22a

commit d0b07a98f2ba6ec6908ef1ea6ff5755607f2c22a
Author: Matt Menke <mmenke@chromium.org>
Date: Wed Aug 23 02:09:29 2017

Delete TestingIOThreadState.

This class provided a bogus IOThread for unit testing.

Of the 3 tests and one test fixture that were instantiating a
TestingIOThreadState, only one test was actually using it.  That one
test was just testing that it was creating a particular object, not
that the object was correctly hooked up, so doesn't seem worth keeping.

The main motivation of getting rid of this class, beyond it's limited
utility, is to make migration to the network service, which
will involve replacing the IOThread class, simpler.

Bug:  755309 
Change-Id: I5e8bae00e17d2da70e4951a530c0b1546bca9218
Reviewed-on: https://chromium-review.googlesource.com/619178
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Vitaly Buka <vitalybuka@chromium.org>
Reviewed-by: Randy Smith <rdsmith@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496557}
[delete] https://crrev.com/37014d9d8dbfb91fd95c09bb54cadcf506616b62/chrome/browser/data_use_measurement/chrome_data_use_ascriber_service_unittest.cc
[modify] https://crrev.com/d0b07a98f2ba6ec6908ef1ea6ff5755607f2c22a/chrome/browser/io_thread.cc
[modify] https://crrev.com/d0b07a98f2ba6ec6908ef1ea6ff5755607f2c22a/chrome/browser/io_thread.h
[modify] https://crrev.com/d0b07a98f2ba6ec6908ef1ea6ff5755607f2c22a/chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
[modify] https://crrev.com/d0b07a98f2ba6ec6908ef1ea6ff5755607f2c22a/chrome/browser/ui/toolbar/app_menu_model_unittest.cc
[modify] https://crrev.com/d0b07a98f2ba6ec6908ef1ea6ff5755607f2c22a/chrome/browser/ui/views/frame/test_with_browser_view.cc
[modify] https://crrev.com/d0b07a98f2ba6ec6908ef1ea6ff5755607f2c22a/chrome/browser/ui/views/frame/test_with_browser_view.h
[modify] https://crrev.com/d0b07a98f2ba6ec6908ef1ea6ff5755607f2c22a/chrome/test/BUILD.gn
[delete] https://crrev.com/37014d9d8dbfb91fd95c09bb54cadcf506616b62/chrome/test/base/testing_io_thread_state.cc
[delete] https://crrev.com/37014d9d8dbfb91fd95c09bb54cadcf506616b62/chrome/test/base/testing_io_thread_state.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21b9cc61281b5a66d69275a3be8cc7d588d3efd4

commit 21b9cc61281b5a66d69275a3be8cc7d588d3efd4
Author: Matt Menke <mmenke@chromium.org>
Date: Wed Aug 23 15:45:45 2017

Make SystemNetworkContextManager owned by the BrowserProcess.

It had been a singleton, but it will need to depend on
BrowserProcess::local_state() to support the functionality needed
by HttpAuthPreferences and SSLConfServiceManagerPref. Singletons
also tend to make unit tests sad.

With the launch of the network service, SystemNetworkContextManager
will replace IOThread (And NetLog will be removed from the
BrowserProcess as well), so this long term this result in no bloat
increase in the BrowserProcess class.

Bug:  755309 
Change-Id: Ideef75978ef7d37bc679334844662ec40380bb20
Reviewed-on: https://chromium-review.googlesource.com/619182
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Randy Smith <rdsmith@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496693}
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/browser_process.h
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/browser_process_impl.h
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/io_thread.cc
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/io_thread.h
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/io_thread_unittest.cc
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/net/network_context_configuration_browsertest.cc
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/net/profile_network_context_service.cc
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/net/system_network_context_manager.cc
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/browser/net/system_network_context_manager.h
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/test/base/testing_browser_process.cc
[modify] https://crrev.com/21b9cc61281b5a66d69275a3be8cc7d588d3efd4/chrome/test/base/testing_browser_process.h

Not going to get to this in the current quarter.  Next one looks not too likely, either, though we'll see.

Apologies, applied the wrong component in bulk.

Matt and I chatted a bit today, in the context of tests, and one of the things resulting from this is that the Network Service is enabling SHA-1 by default, because it's only disabled when prefs are wired up to enable it for enterprises.

In the context of the SSLConfigService, this means the following Enterprise Policies are also not wired up:

EnableOnlineRevocationChecks
RequireOnlineRevocationChecksForLocalAnchors
EnableSha1ForLocalAnchors
EnableCommonNameFallbackForLocalAnchors

These all propagate through policy, and thus to prefs on the SSLConfigService

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2fcac67f0fcddbd23af9349577e8f8175217f15

commit c2fcac67f0fcddbd23af9349577e8f8175217f15
Author: Ryan Sleevi <rsleevi@chromium.org>
Date: Thu Dec 21 19:14:16 2017

Add end-to-end tests for cert-related enterprise policies

Enterprises have the capability to enable SHA-1 support for
locally installed CAs, as well as to allow certificates that
only support commonNames (no SANs). While unit tests exist for
these behaviours, no test existed for end-to-end integration
that the SSLConfigService, as configured by the IOThread,
actually propogated these flags through.

This adds end-to-end tests to ensure that both SHA-1 support
and commonName support are disabled by default, and that
enterprise policies, if set, appropriately enable the support.

BUG= 755309 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I0664943c5f7a865700a8287b26c25d53c131a02c
Reviewed-on: https://chromium-review.googlesource.com/837945
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525760}
[modify] https://crrev.com/c2fcac67f0fcddbd23af9349577e8f8175217f15/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/c2fcac67f0fcddbd23af9349577e8f8175217f15/net/BUILD.gn
[add] https://crrev.com/c2fcac67f0fcddbd23af9349577e8f8175217f15/net/data/ssl/certificates/common_name_only.pem
[add] https://crrev.com/c2fcac67f0fcddbd23af9349577e8f8175217f15/net/data/ssl/certificates/sha1_leaf.pem
[modify] https://crrev.com/c2fcac67f0fcddbd23af9349577e8f8175217f15/net/data/ssl/scripts/ee.cnf
[modify] https://crrev.com/c2fcac67f0fcddbd23af9349577e8f8175217f15/net/data/ssl/scripts/generate-test-certs.sh
[modify] https://crrev.com/c2fcac67f0fcddbd23af9349577e8f8175217f15/net/test/embedded_test_server/embedded_test_server.cc
[modify] https://crrev.com/c2fcac67f0fcddbd23af9349577e8f8175217f15/net/test/embedded_test_server/embedded_test_server.h
[modify] https://crrev.com/c2fcac67f0fcddbd23af9349577e8f8175217f15/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Matt: is it possible for you to finish it this month or next? Thanks

I hope so - I don't think this will be a big task, as long as I don't mind disabling tests on OSX (Which is why I didn't land in the first place).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de13b3b58abd2a3b259a6f859a361717b00a18ce

commit de13b3b58abd2a3b259a6f859a361717b00a18ce
Author: Matt Menke <mmenke@chromium.org>
Date: Thu May 03 17:52:38 2018

Hook up BasicTestServer's certs to NetworkServiceTestHelper.

It has another cert for OCSP tests not used by the EmbeddedTestServer.

Bug:  755309 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ic43314cf9f8e2978848a75764a4323ce34b0006a
Reviewed-on: https://chromium-review.googlesource.com/1033192
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555804}
[modify] https://crrev.com/de13b3b58abd2a3b259a6f859a361717b00a18ce/content/public/test/network_service_test_helper.cc
[modify] https://crrev.com/de13b3b58abd2a3b259a6f859a361717b00a18ce/net/test/spawned_test_server/base_test_server.cc
[modify] https://crrev.com/de13b3b58abd2a3b259a6f859a361717b00a18ce/net/test/spawned_test_server/base_test_server.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5958d32c83f59f30d570299288881f97e57cc954

commit 5958d32c83f59f30d570299288881f97e57cc954
Author: Matt Menke <mmenke@chromium.org>
Date: Mon May 21 21:52:57 2018

Add NetworkService interfaces to set the SSLConfig.

And make SSLConfigServiceMonitor use them.

BUG:  755309 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I72f16fdd74eecf2a9b565895fc97d90654384b16
Reviewed-on: https://chromium-review.googlesource.com/1022691
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560355}
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/browsing_data/browsing_data_channel_id_helper_unittest.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/io_thread.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/io_thread.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/net/default_network_context_params.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/net/network_context_configuration_browsertest.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/net/system_network_context_manager.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/net/system_network_context_manager.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/prefs/chrome_command_line_pref_store_ssl_manager_unittest.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/profiles/off_the_record_profile_impl.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/profiles/off_the_record_profile_impl.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/profiles/profile.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/profiles/profile_impl.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/profiles/profile_impl.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/profiles/profile_io_data.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/profiles/profile_io_data.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/ssl/ssl_config_service_manager.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/ssl/ssl_config_service_manager_pref.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/ssl/ssl_config_service_manager_pref_unittest.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/ui/app_list/test/fake_profile.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/browser/ui/app_list/test/fake_profile.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/test/base/testing_profile.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/chrome/test/base/testing_profile.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/net/ssl/ssl_config_service.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/net/ssl/ssl_config_service.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/BUILD.gn
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/OWNERS
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/network_context.cc
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/public/mojom/BUILD.gn
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/public/mojom/network_context.mojom
[add] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/public/mojom/ssl_config.mojom
[add] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/ssl_config_service_mojo.cc
[add] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/ssl_config_service_mojo.h
[add] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/ssl_config_service_mojo_unittest.cc
[add] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/ssl_config_type_converter.cc
[add] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/services/network/ssl_config_type_converter.h
[modify] https://crrev.com/5958d32c83f59f30d570299288881f97e57cc954/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter