Is there a way to ensure this code is only included in "Test" builds?

The hijinks of TestRootCerts is to try to avoid shipping, in chrome.exe, a way to override the root certs (plus, as currently implemented, TestRootCerts has threading concerns related to how the CryptoAPI hooking on Windows works).

Looking forward thinking (e.g. Q4/Q1, design doc desperately needing to be written), one of the goals with transitioning to our PKI library is to ensure that 'root cert' management is done per-profile (particularly to allow cloud sync). From this perspective, managing the root store is a capability that would be granted (e.g. to the browser process) to influence the network process, but wouldn't be granted to a renderer process or equivalent. For unittests, we could grant that capability to the test.

Without wanting to overly complicate things, this would be the most forward-friendly design for the network service. That is, 'root store' is a concept implemented by the network service, you can get access to a profile's root store, and the browser process tickles the appropriate profile as necessary. From an implementation side in the network process, the service vended could just tickle the process-wide store for now, and defer the profile-isolation to the PKI library work.

How does that sound?

There's already a network_service_test.mojom.  While the interface is in content/public/common, the implementation is in content/public/test, and is only included in test targets.  It's basically a service that's created in the same process as the network service (So it has access to globals).

Of course, the code that makes the global cert store is in Chrome itself (Well, on Windows, I guess We use some system API instead?).  I'm not going to make it per profile, simply because that doesn't work without deeper changes.

I may just go the minimal route (Individual tests manually injecting the test certs, rather than anything meant for mass deployment) and let people who know where they want this to go do all the lifting here.  I figure if we're going to change it anyways, better to write a simple couple hour CL that works and isn't too hairy (Unless used in every test that starts an SSL server) may be better than spending a lot of time on something that will need to be completely replaced.

Longer term, could even just stuff them in SSLConfig, which is already per profile (Though it currently uses global prefs).  Or could just reuse the same mojo pipe as that will use.

I don't think we can easily stuff them in SSLConfig (plus, that seems like a pretty gross layering thing). And almost every test ends up needing to inject test certs, so it seems like we'll need to cross this bridge.

My hope is that servicification won't simply introduce (deferred) costs for cleanups later, although I totally understand and appreciate the desire to do the quick thing. I just think it may be a road we know we don't want to go down :)

I don't think I have sufficient understanding of where you want to go here to do something cleaner.  Also, most tests don't use or need HTTPS.  If it were all tests, we'd just bake it into the test fixture (Which I suppose I could do anyways - with basically the same approach, just in the test fixture instead).

Also worth noting I'm sufficiently unfamiliar with SSLConfig to understand why have extra supported token key binding params in it is a good idea, but bonus SSL root certs is not (i.e., I'm not a good person to be making long term new design decisions in this space).

I'm not sure I understand your remark about 'bake it into the test fixture'. However, most tests using EmbeddedTestServer for HTTPS purposes (e.g. to be able to meet secure origin checks / permission gates, such as powerful features) need to inject.

The overall request is:
1) Don't expose in Chrome, if at all possible
2) If we do expose in Chrome, don't expose the API surface as a global, but per-profile

It can't be hidden from Chrome (Or at least content/), because net/ doesn't know about the network process, and we have to inject code into the network process.  I suppose we could inject the embedded test server itself into the network process, and then wrap that with a Mojo interface for handling requests, but I see no advantage to doing that.  And if we go that route, it potentially makes doing things per-NetworkContext even weirder, since the EmbeddedTestServer doesn't know what a URLRequestContext is, currently.

I think rewriting the 5 TestRootCerts class to change that is rather out of scope here, particularly as tests need to work with the network service disabled as well.  Refactoring every single test that uses the embedded test server to work with per-URLRequestContext root certs seems way out of scope here, and doesn't seem like something that should block getting Chrome's SSLConfigService working when using an out-of-process network service.

Moreover, exposing it as a per-URLRequestContext value (Remember that not all URLREquestContext/NetworkContexts have a BrowserContext (Profile is the wrong term here, as this includes content/ tests)) is significantly more intrusive in tests than as a global, since they'd all have to directly inject it themselves into any URLRequestContext that they're using.  It also has to be done with a sync call, to avoid any races.

By "injecting into the test fixture itself", I was thinking content::BrowserTestBase, so all browser tests get it. That does mean adding a sync mojo message to all tests when the network service is enabled, though we currently already do that, anyways, for HostResolver injection, to avoid doing real DNS requests.

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

commit bd131f2f5352b20469f0866e5377077967fa04a4
Author: Matt Menke <mmenke@chromium.org>
Date: Thu Aug 24 19:47:11 2017

Install test root certs in NetworkServiceTestHelper.

This allows browser tests that use EmbeddedTestServers of TYPE_HTTPS
to avoid running into SSL errors when the network service is enabled.

Also enable a small number of content_browsertests that were using
HTTPS servers, though most remain disabled, since most such tests
cover other things that aren't hooked up yet.

Also fix base::DIR_SOURCE_ROOT in content/shell on OSX subprocesses.

Bug:  757088 
Change-Id: I8aeb3b253934f21b0b198e4c4f157a198bcc88c8
Reviewed-on: https://chromium-review.googlesource.com/624067
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497161}
[modify] https://crrev.com/bd131f2f5352b20469f0866e5377077967fa04a4/content/public/test/network_service_test_helper.cc
[modify] https://crrev.com/bd131f2f5352b20469f0866e5377077967fa04a4/content/shell/app/paths_mac.h
[modify] https://crrev.com/bd131f2f5352b20469f0866e5377077967fa04a4/content/shell/app/paths_mac.mm
[modify] https://crrev.com/bd131f2f5352b20469f0866e5377077967fa04a4/content/shell/app/shell_main_delegate.cc
[modify] https://crrev.com/bd131f2f5352b20469f0866e5377077967fa04a4/net/test/embedded_test_server/embedded_test_server.cc
[modify] https://crrev.com/bd131f2f5352b20469f0866e5377077967fa04a4/net/test/embedded_test_server/embedded_test_server.h
[modify] https://crrev.com/bd131f2f5352b20469f0866e5377077967fa04a4/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

Note that this still isn't quite fixed - on OSX, Chrome uses a different binary to launch browser_tests from the one to launch services, and the services binary does not get the NetworkServiceTestHelper.  On all other platforms that run Chrome browser_tests (And for content_browsertests on OSX as well), the binary is the same.

Not sure about the reasons for this.  I feel like I've done enough digging into certs and OSX weirdness this for now, so taking a break from this for the moment, though leaving it assigned to me.

[rsleevi]:  So it turns out that NetworkServiceTestHelper isn't actually hooked up on OSX, and doing so requires mucking with how child processes are started in browser_tests to use a separate binary from what they use in production, which is something I'm rather reluctant to muck with.

Are we planning to keep "ignore-certificate-errors-spki-list" around?  It occurs to me that I could just hook up that command line flag to the network service, and use that instead, appending it to the command line in tests.  We're going to have to hook it up to the network service anyways, unless it goes away in the next year or two.

If we do that, we could either pass the command line flag to the network service, or add a method to the NetworkContext or NetworkService to set the ignore cert errors SPKI list manually (Either at startup, or during creation).  Using a command line flag is nice in that it makes the API a little less obvious, discouraging use, and lets us set it in the browser test fixture, though you don't seem to be a fan of that approach.

The other options would require each test that uses a test server inject the SPKI list directly.  Making it a method used on creation of the NetworkService or a NetworkContext would require globals.  Making it a runtime method would make it easier to set for tests, and would require tests wait until the NetworkService had received the message before using the test server, to avoid a race.

Any way we go would at least help us move forward in getting rid of the current cert magic, in favor of using the same logic as "ignore-certificate-errors-spki-list".

We should not add the flag itself to the NetworkService. The flag is meant to be explicitly accompanied with other security mitigations (namely, --user-data-dir, the same as we require for other security-disabling flags). The flag is meant to enable automated testing, so arguably, this is within scope, although that flag does not enable the necessary control that we exercise via other tests.

I'm not sure what you meant by "current cert magic" - I thought we had agreed that was a viable and clean enough path forward - thus, not magic, but part of the design.

From a layering perspective, it seems that the right way, particularly to support how testing is done and the layering is done (embedder-providers, not network-service-provides) is to allow the embedder to specify how certificate will be verified (certificate verification as a service provided by the NetworkService client). Absent that, we're simply taking higher layers and smooshing them into lower layers - when that separation was intentional.

That said, with the understanding that "NetworkService represents //content, not //net, and should not be used for folks looking for //net-like loading", then I would greatly prefer explicitly configuring the list as a configuration option and API, rather than implicitly (via the command-line). The handling of the command-line should still require the concept of ensuring that "the critical command-line flag is set" (which in //chrome, is user-data-dir, but in other embedders, may be different) seems like it's a concept that belongs outside the Network Service, but inside content.

"current cert magic" means using the globals that you wanted me not to use in the first place.

On an unrelated issue, "user-data-dir" is a Chrome switch (Declared and implemented in chrome), so having "ignore-certificate-errors-spki-list" in content/ with a note about it requiring user-data-dir seems like a bit of a layering violation.

Anyhow, I'll go ahead with an actual API, then.  If we want to replace it with injecting a mojo interface down the line, I'm fine with that (Though we'll want to be sure not to add IPCs - dealing with ChromeOS is going to be where things may get complicated).

Just driving by, and a bit late, but just fyi some tests expect verification to fail, so just ignoring all errors for the test roots wouldn't work. (Ex, https://chromium.googlesource.com/chromium/src/+/e14d8f6b14aae2ab3883ea0f4483b33caadd5086 which was added to verify that SetURLRequestContextForNSSHttpIO doesn't get broken again during servicification)

Yeah, the comments weren't updated when it was moved from //chrome down into //content, which was supposed to update it as a 'required flag to be present'.

If we want the network service to be usable for others, then we'll need to revisit how we do certificate verification. Our design and efforts have been around "embedder supplies and layers", while putting it in process means "All possible implementations are in process, embedder configures"

So does comment #13 mean we're going to have to inject the entire test cert, not just its sha-256, and do full validation?

Yeah, I'd say so. If we don't inject the test root cert and do full validation, then there's no way to guarantee the cert verification code is hooked up and configured properly.

Short of injecting globals, it looks like we don't have any cross-platform way of adding additional root certs.  CertVerifyProc::VerifyInternal takes additional trust anchors (Which I assume is what I'd theoretically want to add to), but it's only supported on iOS.  So if I go with pushing in the entire root cert, I guess I'm stuck with using the globals?

*Only supported with NSS, rather.

So if I'm not going to use the globals in production code, and am not going to fool with how subprocesses are launch on Mac, I guess that means I'll need to implement |additional_trust_anchors| support on all platforms that run browser_tests, and just use that?  Looks like we'd need to create OS-specific objects from certs on each verification, which seems potentially expensive.

Since I only care about this for tests, I'm fine with doing that, but I thought I'd ask first.  We could also create a PlatformCertStore or somesuch, and pass that in to Verify instead of / in addition to a CertificateList, and have a wrapping cert verifier manage it.

I think Ryan's suggestion was to use the globals for now, but design the API so that when the new non-global trust store stuff is ready it can be plugged in without having to redo all that. I don't think it's worth trying to retrofit all the existing verifiers to support it.

The globals need to be configured before any network requests have been made (And maybe before any URLRequestContexts have been created), right?  I don't believe such a time period current exists.  And changing that would be more significantly invasive than changing browser_tests to use a different child binary on OSX.

Content's mojo connector is only set up after all the threads are set up, so we can't talk to the network service until SetUpOnMainThread in a browser_test, but by that point, we've already started issuing network requests, or at least a dozen things have already grabbed on to the network stack.

I think the TestRootCerts only need to be added before any connections that need those certs. afaik it's fine for unrelated requests to have been made.

I think CertVerifyProcMac accesses globals on worker threads, doesn't it?  It uses a LazyInstance, so grabbing its pointer should be safe, but it dereferences other pointers that are not protected by locks.

At least I think CertVerifyProcs are called on worker threads, by MultiThreadedCertVerifier?

Sure I can just add locks as necessary, but all this retrofitting has me a bit concerned about correctmess.

re: Comment 20 - It's not possible to support |additional_trust_anchors| across platforms. If it was, we would have :) That is, fundamentally, the only way to support |additional_trust_anchors| is by shared global, because on (macOS, NSS, Windows), the trust settings are a shared global.

I agree with comment #20 as to the approach, and sadly, comment #25 with the concerns.

This is why I've suggested the CertVerifier interface, as the API contract of //net (and //content) is designed to have the embedder supply the certificate verification interaction - it is not a 'fundamental' part of //net or //content, just various implementations for various purposes. I believe this is 'long term' the right approach, so if we're evaluating the cost of 'short term fix' versus 'long term solution', I believe that we should be trying to move closer to that goal, overall. This is true even if the TestServer did not (directly) depend on the TestRootStore - as we break that dependency, the embedder will be responsible for configuring the verification.

Regarding #19 - no, it's not expensive, no, we should not supply a PlatformCertStore.

Regarding #22, 'a' solution is to simply not allow network requests to be made until some other 'no, I'm really done with setup' phase has been completed (after the threads). Is that viable?

rsleevi:  Ah.  I had assumed the "TestRootCerts::GetInstance()->FixupSecTrustRef()" call on OSX had all the additional logic needed to check against a specific set of additional root certs.  And I assumed the HCERTSTORE that the Windows method takes also allowed for separate sets of certs to check against.

If we wanted to delay initialization, I suppose we might be able to make a private content API, and have the browser test fixture could delay NetworkService initialization.  I'm a bit concerned that would render browser tests a little too unlike production behavior, though (Even if we just limited it to SSL tests, then all our SSL tests would use a bogus startup path).  We could delay network startup in production, but that seems a bit concerning as well.

https://cs.chromium.org/chromium/src/net/cert/test_root_certs_win.cc?rcl=c91bb424052f0e46dbe31d9ce9375a50a0aec56d&l=90 is the logic that intercepts DLL calls on Windows (using a 'defined' API, but process-wide) to support test root certs. The HCERTSTORE bits are about ensuring changes in the Chromium side flush the OS caches.

To make sure I understand the state of the world:
- We don't have a good path to supply a 'testing' network instance (e.g. with a test cert verifier) that is configured by the unit test, right? A MockCertVerifier is supposed to be friendly towards modification after creation (well, as long as it's done on the IO thread), so exposing that for a testing cert verifier would let *most* tests work.
- At least some tests require the true TestRootCerts, because of the OS integration - like Matt mentioned.

Most of out network service unittests just run both mojo services in process, so the problem is just browser tests, when the network stack is in a separate process.  Unfortunately, injecting test-only code into the network process on OSX requires more significant changes than I'm willing to make (OSX uses a separate service binary, and currently browser tests use the production service binary, so we'd need to make a test-only separate service binary, and make sure everything knows to use it).

Until this is resolved, we either can't hook up any SSL-related classes on the OOP-network-stack path, or we can't have any integration tests that they're hooked up properly in Chrome on OSX.

Apologies, applied the wrong component in bulk.

It looks like neither --ignore-certificate-errors (which some existing browser tests use) nor MockCertVerifier work with the network service. Is this bug meant to include those things, or should I file a separate bug for them?

ignore-cerficitate-errors is production code, and this is more about the test-only logic that EmbeddedTestServer does to add certs.  Of course, it's possible that adding support for those two would give us a simple path forward on this bug.

Still not fixed on Mac.

@Matt: the Mac bug is 843324 right? (that covers test certs, and other test code)

 Issue 843324  was filed two ago.  Regardless, yea, it's a dupe.

Ok cool, I'll mark this bug as fixed then and leave the other mac-specific one open