Calling global settings in hidden code, away from the rest of network stack configuration seems like an anti-pattern. While we're fixing this, could we hook this up in a more discoverable place?
I'm not sure how we handle the initial set on browser start, but we'll also need to handle NetworkService crashes/restarts.
SystemNetworkContextManager::OnNetworkServiceCreated is currently the main place for re-configuring the NetworkService after a crash.
Thanks for the pointer as to how to handle crashes! That's super helpful to know.
No disagreement here that it is an anti-pattern, as is the lack of the integration tests that could have flagged this. This design goes back to when all certificate verification was done via a single static method (X509Certificate::Verify).
I'm making this explicitly part of the certificate verification configuration, as with the flags passed in Verify(), as they are not call-specific but context-specific (i.e. associated with a given CertVerifier, not a single call to ::Verify). This is what we'd talked about when discussing the SSLConfigService, and how it's really "two" configurations - SSL and cert verification. I'm making the latter an explicit API, which also resolves this issue.
If we're switching to per-NetworkContext, rather than global state, hooking up is more complicated. The logic would then need to be hooked up for the system, profile, and safebrowsing NetworkContexts, each of which have their own code. Or, perhaps more sanely, CreateDefaultNetworkContext params would need to plug in a new pipe specifically for this data whenever a NetworkContext is created. (New pipes are necessary because NetworkContexts are invalidated on NetworkService crash, and there's no way for a consumer to watch for this happening, other than by having its own pipe to each NetworkContext).
The goal is not to split it to per-NetworkContext - that is, from the observable API, it behaves global - but to no longer make it per-call parameters, and instead a rather explicit per-verifier configuration. This at least makes it clearer that it's a configuration parameter, and where/how it's set and updated.
If this is blocking canary, we should do the simplest thing that makes it work as is today (i.e. a global call on NetworkService mojom). Any improvements related to the new per-verifier configuration can be done later.
Doing this per NetworkService should be a quick fix.
hey ryan, can you provide an update on this bug? We are getting very close to launch a canary experiment with network service and hope to have this resolved before then.
Comment 1 by mmenke@chromium.org
, Jun 20 2018