It's not clear to me if this should block canary on !ChromeOS platforms, and I don't know the prevalence of this feature on ChromeOS (I couldn't find any histograms to check). However, I also couldn't find a bug tracking the resolution of this, so thought it'd be good to file regardless.

Pavol, can you PTAL?

Sorry for the delay.
The prevalence is pretty high on EDU which is our main Chrome OS managed customer base. Unfortunately we don't have metrics for this -- we have metrics for policies, but these are set through the "OpenNetworkConfiguration" policy which is a JSON blob containing all kinds of things.
Definitely a TODO to improve metrics here - filed bug 870590. In the meantime, we could ask the server-side device management folks to see if they can give us an overview. But let's assume that prevalence is high and this is an important feature for EDU customers, because they run MITM proxies inspecting/filtering SSL traffic :/

Anyway, this must mean that the PolicyProvidedTrustRootsRegularUserTest.AllowedForRegularUser[1] browsertest must be failing/disabled for network service, correct?

I remember that we had thoughts about this usecase before but can't find that particular discussion at the moment :/

Anyway, I'm assuming that cert verification in the Network Service _only_ happens using cert_verified_builtin, not by calling into NSS anymore. Is this correct?

If yes, would it be possible to have a way for chrome to configure a list of additional CA/server certificates with a trust_anchor flag for each in the network service (ideally in a way that would apply to the scope of a chrome Profile, if that's possible), and having the Network Service signal back when one of these has been used in the path of a successful cert verification event?

[1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/user_network_configuration_updater_factory_browsertest.cc?rcl=2b4900391fa6ad04eaab57fb957e29f7a20cc686&l=196

> Anyway, I'm assuming that cert verification in the Network Service _only_ happens using cert_verified_builtin, not by calling into NSS anymore. Is this correct?

No. Today, when the Network Service is enabled, it's 100% within the NS and thus this policy does not work.

Work is being done to move this out, which would enable the Chrome side to handle the verification as well as signal (albeit through a layering violation) when one of these additional trust anchors is used.

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

commit 73bdca66c203a89fbffb99c85e632184375071f1
Author: Ryan Sleevi <rsleevi@chromium.org>
Date: Wed Aug 22 16:34:23 2018

Move additional_trust_anchors to CertVerifier::Config

The set of additional trust anchors (read: Enterprise-configured
trust) presently is passed as a parameter to the
CertVerifier::Verify() call, except it is logically part of the
system configuration. As it does not vary between calls, move it
to the CertVerifier::Config and update the PolicyCertVerifier to
pass this as part of the config.

Bug: 848277,  862043 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ibeb084cf05411a1055f0fcfc5e6e987a96eb03e9
Reviewed-on: https://chromium-review.googlesource.com/1167472
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585058}
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/android_webview/browser/net/aw_url_request_context_getter_unittest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/chrome/browser/chromeos/policy/policy_cert_verifier.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/chrome/browser/chromeos/policy/policy_cert_verifier.h
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/chrome/browser/chromeos/policy/policy_cert_verifier_unittest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/chrome/browser/chromeos/policy/user_network_configuration_updater_factory_browsertest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/chrome/browser/extensions/api/platform_keys/verify_trust_api.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/chrome/browser/net/trial_comparison_cert_verifier.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/chrome/browser/net/trial_comparison_cert_verifier_unittest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/content/browser/web_package/signed_exchange_handler.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/net/cert/caching_cert_verifier_unittest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/net/cert/cert_verifier.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/net/cert/cert_verifier.h
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/net/cert/cert_verifier_unittest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/net/cert/multi_threaded_cert_verifier.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/net/cert/multi_threaded_cert_verifier_unittest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/net/cert_net/nss_ocsp_unittest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/net/quic/crypto/proof_verifier_chromium.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/services/network/ignore_errors_cert_verifier_unittest.cc
[modify] https://crrev.com/73bdca66c203a89fbffb99c85e632184375071f1/services/network/ssl_config_service_mojo_unittest.cc

I can take a look at this

So, do I understand correctly that the current plan is to do Cert Verification in the Network Service?

We have a few things to consider here:
(1) Respect policy-provided trust anchors.
Caveat: These can change at runtime. Currently, the PolicyCertService creates a PolicyCertVerifier and updates its trust anchors set based on notifications from a PolicyCertificateProvider[1].

(2) Respect policy-provided trust anchors in all user network contexts
The PolicyCertVerifier mentioned in (1) ends up in the Profile's main network context (initialized in ProfileIOData::Init), and is IIUC inherited into all network contexts derived from that.

(3) Signal back to browser code that a policy-provided trust anchor has been used. This is implemented using PolicyCertVerifier's anchor_used_callback[2].

(4) Respect policy-provided untrusted intermediates (for device and user policy).
Currently, as NSS is being used for cert verification, this is done by simply putting them into NSS as temp certs (using TempCertsCacheNSS[3]).
I think that if NSS is being used in the Network Service for cert verification and the Network Service runs in the same process, (4) should continue working out of the box (whether that's a good thing or not... :-) ).

So, (1) would mean sending the trust anchors over to the network service during NetworkContext creation, and on every update, correct? I was not sure if SSLConfig / SSLConfigClient would be the right mojo structures to transfer this information (?)
Maybe an easy way would be a top-level function in NetworkService (to set the trust anchors) and NetworkServiceClient (to notify), which would be fine if there's one NetworkService per Profile and we don't need to use different set of trust anchors per NetworkContext (which we don't at the moment, but may want to do that in the future). A nice property of that would be that (2) would work out of the box I guess.

[1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/policy_cert_service.cc?rcl=395e56f1f58492347fdad75595cd85463fd68ee3&l=106
[2] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/policy_cert_verifier.h?rcl=395e56f1f58492347fdad75595cd85463fd68ee3&l=37
[3] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/temp_certs_cache_nss.h?q=tempcertscacheNSS&sq=package:chromium&dr=CSs&l=16

Thanks Pavol for the background.

Yep cert verification is running in the network process at launch. On desktop, this is always a separate process. Thanks for the pointer re 4, as NSS will be run in a different process that'll have to be initialized there instead.

SSLConfig should be good to pass this, as it's logically where we pass other information for each NetworkContext related to certificates and it handles dynamic updates.

(per our chat): SSLConfig is updated per NetworkContext; the browser would loop through all storage partitions for a particular profile and get the NetworkContext pointer from them. profile_network_context_service.cc shows how we do this.

"NSS will be run in a different process" --- could you please explain in more detail?

I've imagined that either the NetworkService runs in the same process as the browser, or in a separate process. So the NSS calls for cert verification would be issued from one of the two processes. Or is there a third process to consider?

Note that we do use NSS directly in the browser for a few things (e.g. pki-related extension APIs, cert listing), and I don't know what happens if two processes use NSS to access the same NSS softoken databases.

( long-term, we(Ryan and I) want to get rid of NSS altogether on Chrome OS :-) )

To clarify, by "different process" I was referring to different than browser. i.e. the network process.

Are the NSS databases system-wide? i.e. if the browser writes to it, would the network process see that change? I tried commenting out the TempCertsCacheNSS usage in PolicyCertService::OnPolicyProvidedCertsChanged but the PolicyProvidedTrustAnchors* tests still passed, so I assume there's no integration test coverage. Are there manual tests one can follow?

Re TempCertsCacheNSS:
WebviewClientCertsLoginTest.SigninFrameIntermediateAuthorityKnown and WebviewClientCertsLoginTest.SigninFrameIntermediateAuthorityUnknown should test that this works fine when delivered through device policy (for matching client certs on the sign-in screen).

I'll see if I can find a test for this being correctly applied from user policy -- otherwise I'll write one, as it's a pretty subtle thing.

Re NSS databases:
(However, this is getting slightly off topic here because it is not too related to ONC-pushed CA certificates)
Unfortunately, NSS has global scope, so anything that is loaded in NSS is visible to all NSS calls in the process.
We load the "system slot" (a PKCS#11 token provided by libchaps) on Chrome OS start around here:
https://cs.chromium.org/chromium/src/chrome/browser/net/nss_context_chromeos.cc?q=nss_context_chromeos&sq=package:chromium&g=0&l=29
The system slot contains device-wide client certificates.

We load the "user public slot", which is a NSS softoken i.e. a sqlite database, here:
https://cs.chromium.org/chromium/src/crypto/nss_util.cc?q=nss+public+slot&sq=package:chromium&g=0&l=448

And we load the "user private slot", which is another PKCS#11 token provided by libchaps, here:
https://cs.chromium.org/chromium/src/crypto/nss_util.cc?q=nss+public+slot&sq=package:chromium&g=0&l=470

I *think* only the user's public slot should be relevant in practice for cert verification, as it can contain trusted/untrusted CA/server certificates the user has imported through chrome://settings/certificates.

I have some ideas about this but I'd like Ryan to confirm if he thinks that making things from the user's public slot available in NSS in the network service process while we still use NSS is the way to go.

Last off topic Q:
If a client certificate is used in the TLS connection, I'm assuming that the network service asks the embedder(is that a word?) to sign the challenge? so that would still happen in the browser process?

Re WebViewClientCertsLoginTest.*: I haven't looked through these tests closely, but they are passing now with network service. Either the addition of certs to NSS in the browser is affecting the network process (yay), or the tests need to be modified for network service.

Re NSS scope: I'm still not following: will adding certs to NSS in one process (e.g. browser) affect other processes (e.g. network)?

Re client certs: can you point to the code that does this now? Is it just putting certs into the NSS db?

(ok looking through the code, I have some guesses. please correct if I'm wrong, as I'm learning this code).

It looks like slots are handles to opens of the NSS database, which means we'll have to open them in the network process as well.

For client certs, looks like this is handled in the browser process already when network service is in use. This is through the NetworkServiceClient::OnCertificateRequested mojo call.

Re WebViewClientCertsLoginTest.*: 
Ah, these care about client cert discovery/selection. As that's handled in the browser process over NetworkServiceClient::OnCertificateRequested, it makes sense that it works with the network service.
I've filed bug 897233 to write a test for untrusted authorities in cert verification, which I hope to work on on monday.

Re NSS scope: I'm still not following: will adding certs to NSS in one process (e.g. browser) affect other processes (e.g. network)?
 -> Nope, that should not happen.

Re opens of the NSS database:
If done this way, I think the network service process will only have to open the user's public slot, as the other ones are currently practically only used for client certificates.
I'll see if we have a browsertest verifying that CA certs imported into the user's NSS database are respected.

(btw, was this a point of discussion for Network Service on linux?)

However, I don't know if it's OK to open the NSS sqlite database (what we use as "public slot") from two processes at once. I'd assume yes, but Ryan would know for sure.

Re client certs:
Yup, looks like this is fine - OnCertificateRequested will be called to select a client cert which is then handled in the browser, and afterwards SSLPrivateKey.Sign will be called on what the browser returned, so actually requesting the challenge signature in the backend NSS/extension) will also happen in the browser process.

From Comment #15
> Are the NSS databases system-wide? i.e. if the browser writes to it, would the network process see that change? I tried commenting out the TempCertsCacheNSS usage in PolicyCertService::OnPolicyProvidedCertsChanged but the PolicyProvidedTrustAnchors* tests still passed, so I assume there's no integration test coverage. Are there manual tests one can follow?

From Comment #19:
> However, I don't know if it's OK to open the NSS sqlite database (what we use as "public slot") from two processes at once. I'd assume yes, but Ryan would know for sure.


No. Both of these assumptions are incorrect, at least for the problem at hand. As stated in past discussions, the ChromeOS situation is complex enough to warrant bringing the verification out of process. While NSS nominally supports dual-opening, it does *not* support cross-process notifications of changes. Changes in one process will not be propagated to another process in a reliable fashion. This is why, for example, that installing machine certs for Googlers requires restarting Chrome. This is a limitation and design of the underlying platform. This also applies, to a lesser extent, to private slots, as public objects are not periodically polled from those slots.

Alternative designs - such as interfacing exclusively with Chaps as the storage mechanism - have been proposed, but these represent significant more complexity for the efforts compared to Network S13N. Having a 'simple' inter-process verification avoids a significant amount of this complexity, and keeps the ChromeOS security boundaries (in which the browser process manages interactions with these slots, their configuration, their updates, and their display) in a single process. I would again like to advocate that this design is one which can be supported with far less complexity and fewer design issues than attempting multi-process concurrent modification, interaction, and display - and with far less security risk of TOCTOU issues.

Thanks for clearing that up Ryan! Makes sense that NSS is not montirong the filesystem to watch for DB changes.

Agree that in the current situation doing the verification in the browser process sounds far easier
 In an offline chat I remember jam@ saying that this has been rejected for now - John, would you mind telling us the reason?

Ryan, if we only used cert_verifier_builtin within the network service and sent over the set of trust anchors and the set of untrusted authorities from the browser process to the network service (keeping it up to date), would that still pose a security risk in your eyes? What would a TOCTOU issue look loke in this case?

Switching to cert_verifier_builtin on ChromeOS would be a significantly more risky change, especially if coupled with Network S13N, since we'd lose visibility on the performance and compatibility issues, and we anticipate that it will introduce compatibility issues (which is why we were measuring for macOS & Linux). That said, we're not able to measure for compatibility anymore because that code was in //chrome and tied to Chrome reporting, so even the compatibility breakage measurement has to be migrated to work with the Network Service.

Setting aside the compatibility issues we expect, and the lack of ability to measure anymore, it would be less of a risk than the NSS usage today, which is why we'd like to get there. However, getting rid of NSS is predicated on those other elements first and being able to quantify the data to make sure the new verifier is not adding risk. The TOCTOU issue that would seemingly manifest with both (that is, NSS-in-NS or builtin-in-NS) is if the browser process is expecting to keep track of what the policy-pushed authorities are, as it does today, then the NS may not have processed that message. That flow gets better if the NS informs the browser as to what's going on, or better when all the NSS usage is centralized in a single process.

Note that  Issue 880835  is capturing some of the Windows sandboxing issues (today, for the browser process) that would manifest if/when the Network Service is sandboxed in a manner similar to the issue here with using NSS multi-process - namely, multiple updating. That issue is exploring moving client cert processing out, but as both bugs capture, client cert processing and cert verification are pretty intrinsically linked and only really work effectively when kept together.

A WIP is at https://chromium-review.googlesource.com/c/chromium/src/+/1292830.

Ryan: a few questions to help me understand the cross-process NSS issues:
-do we have any automated tests for these scenarios?
-do you have manual test scenarios to see if the network process gets notified (or not) for NSS changes?
-for the "reliable" part of your comment: when does it not work?

Depending on above, one option could be to run the network service in-process on ChromeOS until NSS stops being used. Is there a timeline for that?

Regarding  bug 880835 , my understanding is that this is happening without the network service/process (and client certs are always processed in the browser, even with network service), so I'm not sure I understand how it's related here?

> Ryan: a few questions to help me understand the cross-process NSS issues:
> -do we have any automated tests for these scenarios?

Can you clarify what you mean by "these scenarios"? Are you speaking of using NSS cross-process? If so, the answer is no, because it's explicitly not supported by Chrome or by NSS. We don't tend to add negative tests for things that aren't supported - that tends to be confused/conflated as "supporting", and has the same overhead and maintenance burdens.

> -do you have manual test scenarios to see if the network process gets notified (or not) for NSS changes?

No. It's explicitly not supported by NSS.

> -for the "reliable" part of your comment: when does it not work?

It's explicitly outside the scope of the NSS API. NSS does not guarantee it will work, discourages it from being used as such, and only makes the guarantee with the process restart. When it does work, it is not because it is a supported feature - it is happenstance of a complex system and MUST NOT be relied upon. There is no intent to support this at this time within NSS - it's outside of the supported use cases today.

> Regarding  bug 880835 ,

We can follow-up there. My statement is that the issues that exist are a function of the sandbox that exists and is desired for the Network Service, and those two functions (certificate verification and client certificates) both have the same needs re: sandboxing. This is similar, because the NSS usage (of client certificates and certificate trust stores) equally requires that they be colocated and with the same security and sandboxing needs (i.e. none). Any solution for Windows - for example, that attempts to move those two functions to a utility process, as suggested by Will and David - would be related to this issue, as the NSS functionality would need to be colocated. That is, a solution for  Bug 880835  for the long-term would be similar to the solution for this issue.

"these scenarios" = test steps to show what would fail if NSS is used in the browser and network process on ChromeOS. As an example, the above patch fixes the tests for an admin pushing new trust anchors. What other scenarios exist that would fail with the network service running in a separate process (e.g. Importing on one of the chrome://certificate-manager tabs)? Above you mentioned machine certs, but afaik that's the same thing as client certs, which is still read in the browser process with the network service/process.

Regarding NSS and multi-process, can you give more background about what it doesn't support in our usage on ChromeOS? I ask because links like
https://wiki.mozilla.org/NSS_Shared_DB
https://wiki.mozilla.org/NSS_Shared_DB_Howto
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/developer_guide/che-nsslib
give the impression that NSS supports usage from multiple processes.

One scenario I'd expect to fail would be the user manually importing trust anchors, i.e. navigating to chrome://certificate-manager, importing a CA certificate and editing its trust settings to be trusted for web navigations.

I'd expect that the other process (i.e. the process hosting the network service) would not see the added trust anchor.

Any attempts at concurrent modification can corrupt the database. Modifications in one process - which includes trust settings, intermediate certificates, and loading/unloading smart cards - are not visible in other processes.

I'm aware that those pages are confusing. That's a rather sore point that I'm going to be careful to avoid documenting too extensively on a public bug. It would be best to describe those as an "unfunded, aspirational goal" written 10 years ago, but never tested nor implemented. Much of those pages were written in the midst of a large-scale refactor of the NSS codebase that was abandoned half-way through incomplete, as the entire team doing that refactor stopped working on NSS.

It's more productive to speak about what is "officially" supported - which is the shared configuration of smart cards - than to speak about what is not. As it particularly relates to ChromeOS, modifications are not visible across process boundaries. Chrome has a number of places that make modifications; explicit modifications (such as chrome://certificate-manager) and implicit modifications (such as policy certificates) are two examples.

I'm very concerned with an approach to talk about how to manually test "these scenarios". Because they're not officially supported, it doesn't matter whether or not they work now - I'm stating explicitly they're not supported upstream. Considering that any breakages would require contributions to NSS, and the NSS team is itself not staffed or equipped to support this case (at Google or elsewhere), it's not at all acceptable to use this APIs and expect if they do break, someone will fix.

I want to stress: This is not supported by the NSS team. It is not staffed, by Google or any other company, to support. No such tests exist for such functionality. Those pages document aspirational goals, not usable. Even using NSS across multiple threads, as Chrome does, is best described as "not well supported" - which is why significant care is taken. This is, broadly, why replacing NSS is a useful and valuable goal. However, it's not at all desirable to couple with S13N, especially for performance and security reasons.

It also would place serious limitations on teams supporting ChromeOS and ARC++, as the APIs that would be expected to work no longer function reliably, and inevitably, those cost significant energy debugging and diagnosing. By keeping the implementation simpler, by colocating these activities, we enable other teams to continue to move rapidly.

Thanks again for being clear on this point, Ryan.
To clarify, I was not trying to give a test example in the sense of "if this works, everything is fine" - rather trying to illustrate things that would visibly fail. Understood that things may fail invisibly / not reproducibly if we decided to rely on unsupported behavior.

I agree that verification in the chrome process seems like the most straightforward solution for this, and I thought that's where we're heading originally. Was it rejected due to the expected additional latency of two inter-service IPC hops, or due to layering considerations (or something else)?

In case verification in the browser process is not possible or desirable for reasons that I don't know, *maybe* an alternative could be to use NSS in the network process but not open any shared databases, and instead attempt to replicate all available certificates in the browser process using NSS temporary certificates in the network service process.
However, it sounds like this is non-trivial and NSS experts may know reasons why it wouldn't work (aside from the TOCTOU issues mentioned in Comment #22 - which IIUC could manifest in *any* situation where the browser process maintains the authority over the cert database, but it's being used in another process, and aside from the sandboxing issues which I don't really have insight into),

I'm not sure that latter option (pushing configuration replication down from browser -> network) would work, especially when we talk about the path forward to migrating off NSS. Or at least, it seems like it'd be significantly more effort involved here, because it'd start violating the NSS API assumptions.

For example, in the context of ARC++, we'd talked about the harmonization on Chaps for coordination of certificate stores as the canonical source of truth (migrating from the home-dir softoken, which is essential to moving off NSS). Under the current 'single-process' design, the notification flow for changes to the Chaps DB don't require explicit coordination (because of the token notification). 
This seems like it would require:
  - In addition to any NSS API calls that might modify the store, explicit synchronization points with the Network Service
  - Alternatively, implicit synchronization points using the token observation APIs (which don't work / aren't supported for softoken, and which caused sufficient number of crashes for !softoken that Firefox stopped using that API entirely)
  - Alternatively, some sort of additional coordination via DBus so that the NS can know about (Chrome || ARC++) modifications, which introduces the risk we're trying to work around with just the Chrome & ARC++ case, and seemingly amplifies it (needing to coordinate sequencing across the three processes, rather than the two)

None of these seem like a particular desirable amount of complexity - the first outsources it entirely to developers needing to know Chrome-specific usage constraints for the NSS API (which has impacted ChromeOS in the past, and why we've tried to eliminate), the second is buggy enough that even Mozilla agreed it was more cost-effective to avoid than to try to support, because of the complexity of the bugs, and the third introduces both high complexity and an awful failure mode (the FIFO problem that worsens the ChromeOS experience).

Pavol/Ryan: please file new bugs for any regressions related to NSS. This bug is for ONC-pushed certs, which the patch above fixes, so let's keep it focused on that. You can use this template to create new bugs with network service:
https://bugs.chromium.org/p/chromium/issues/entry?template=Defect&components=Internals>Services>Network&labels=Proj-Servicification,Proj-Servicification-Canary&cc=jam@chromium.org,dxie@chromium.org,dougt@chromium.org

My comments for the new bugs:
-for the scenario in comment 26, please also add the commands that need to be run to generate the public/private certs and how to run the server with them
-feel free to add Restrict-View-Google if you don't want the description of NSS internals to be public
-specific scenarios would be helpful, i.e. I'm still not sure why smart cards are being mentioned still, I thought that's for client certs and that's not read in the network process. also afaik the network process doesn't do any modification of NSS database. If these assumptions are incorrect, please let us know via bugs
-I agree that we don't want to couple network service with NSS removal. Depending on above bugs, we can run it in-process until then

It would probably be good to set up a meeting with Pavol. I don't think the proposed approach in Comment #30 sounds like folks are on the same page, nor does it sound like a reasonable path forward. It puts a disproportionate burden on folks to "prove that this change breaks", when the issue is that the way that the API is being used is not supported by upstream, and puts significant burden on a variety of teams to try to test and maintain as a result.

As it relates to the subsequent points:
- As I stated, that Wiki is not accurate or reflective of "support". The best way to show that is that almost all of the content was written in 2008, but it didn't even remotely come to being close to true until early 2018, and has caused a number of bugs since.

- Smart cards are not exclusive to client certificates. As past design docs tried to carefully explain, smart cards also deliver trusted certificates and additional path-building certificates. Holistically, the statement is "On all platforms, smart cards participate in certificate verification. Certificate verification relies on the correct functioning of smart cards, if they are present." That hopefully makes it easier to cover.

- The issue is that having two processes - one performing modification and one expected to observe modification - is not supported. The Browser Process would perform modification. Those changes are expected to propagate to the Network Process. The NSS APIs do not test nor support this model without restarting the Network Process every time a change is made. Further, "changes" may be made in the browser process explicitly or implicitly (i.e. internal to NSS) based on the APIs being used. There are significant issues reliably detecting the 'internal' changes, and to propagate all 'external' changes would require developers no longer use the NSS APIs as they have, but also know they need to force sync points (process restarts) with the Network Service.

I think if there are further questions on those basics, it would be useful to setup a sync meeting.

Linking this bug (ONC-pushed certificates) and the other issue raised above (NSS updates in the browser won't show up in the network process) is only true if the solution to both of them is to run all of CV in the browser. As mentioned in comment 23, if it turns out that indeed it's not possible with reasonable engineering (*) to make the network process be in sync with the browser's view of NSS, then the network service will run in-process on ChromeOS until NSS stops being used. This is similar to how network service has to run in-process on Android WebView without Android system changes. That patch linked above in comment 23 would still be needed regardless, which is why I'm saying these two issues don't have to be conflated.

Asking for repro scenarios is not the same as asking for a significant burden to test/maintain a big change to NSS. This is only to get a rough cost of the different solutions. i.e. to understand how many endpoints exist that add certificates to the store that would be accessed by the network service. 

[*] Per bug 897035, it looks like there are only 2 (chrome://certificate-manager and the enterprise extensions API). There's also a developer-facing chrome://net-internals/#chromeos. If you know of more instances that would impact the network process, please let me know. Otherwise having 3 places that update the network service doesn't seem like a burden, especially since per bug 897035 there are plans to make them all use the same code to update certs, which would be a single choke point.

It may be better to think of this as a discussion about "thread-safety". Here, we're talking about "process-safety".

When something is thread-hostile, we have a guarantee that, at best, it's only safe to read from if it's never written to (during the process lifetime). Global, statically initialized variables can be both thread-hostile and 'safe' in that regard - because they follow a defined pattern by the time someone can read them, they've been initialized, and they 'can't' be written to. However, if multiple threads are reading and writing, then we say it's unsafe - even "benign" data races are anything but.

Approaches for dealing with thread-hostility, especially in third-party libraries, come down to either making the library thread-safe or making the usage thread-safe. Making the library thread-safe means that the library now supports thread-safety, and will need to work to provide and guarantee that contract going forward. Alternatively, consumers can attempt to either guard all activity with the library on multiple threads (e.g. external locks), or, depending on the contract, may need to guarantee everything is done in a single thread.

Finally, it helps to consider 'volatile' - the sort of memory address that might be changed by your program, or might be changed by an underlying act of the hardware.

The same can be said about process-hostility. NSS is process-hostile, with limited support for a sort of 'static global' initialization, that allows it to read common configuration. Attempting to read and modify across different processes is not supported - in the sense of the library is hostile to those efforts. Discussions about how to have NSS support this use case involve significant effort for the NSS maintainers. It may be that such feedback is not being internalized, because an explicit list of the complexity is not provided, but I'd be happy to provide a number of folks on Chrome and NSS who can happily agree that "This is complex beyond the point of desirable". Even if we imagined it was possible 'tomorrow', our NSS dependencies for Linux and ChromeOS mean that we will not, as a matter of function, be able to reliably depend on that 'process-friendly' version for several years (because we treat it as an OS library, because it's part of the LSB). Let's ignore making NSS process-friendly, especially for the timeframes.

So now we're discussing whether to do 'locking' - in this case, Mojo synchronization - or whether to allocate things to a single 'process'. The 'locking' approach is tempting, because it says that if we can isolate any and every NSS call, and add synchronization, this should be doable with less effort or impact than allocating to a single 'process'. That conclusion, however, would be incorrect, for several reasons. One reason is that the folks tasked with maintaining this code - the Chrome Networking and Enterprise teams - have already explored such routes in the past, and generally found the maintenance burden higher than the alternatives being proposed. On MacOS, efforts to synchronize all accesses (due to the underlying OS being thread-hostile through bugs) ended up being a complex continuous maintenance cost, and as new developers contributed code assuming they could 'just' rely on the OS APIs, would find themselves introducing new crashes and divergent behaviour. Similarly, efforts to ensure NSS was used on a single-thread (as it is, to an extent, also /thread-hostile/ when smart cards are involved) have similarly caused significant P0 breakages for ChromeOS and Linux users when new developers thought they could 'just' use the NSS APIs.

Thus, the safe approach - one that does not present undue risk, that conforms to the API  contracts, and which can be most easily generalized for new developers and those attempting to understand the code - is to accept that this code is process-and-thread hostile, and to keep activity located in one process, and on one thread.

Finally, one remaining note to highlight here is that this memory is best treated as 'volatile'. That is, it is inadequate to examine NSS usage by itself for explicit modification APIs - modifications may result as part of hardware event handling (smart cards, chaps) or other internal state resynchronization. This is also part of the thread-and-process hostile.

I'm happy to bring forward a number of developers who have had to support and maintain this code in Chrome and NSS who will no doubt agree with this summary. This is why attempting to examine where are explicit Chrome calls (why? Because the 'volatility') is not a productive or useful path forward.

I hope, with this additional information, it's clearer why the path being proposed in Comment #32 is not a good path forward - and will increase development cost (for mental models and reasoning, let alone supporting) and introduce bugs (in the same way that ignoring 'thread-hostility' introduces bugs, eventually).

Finally, as I fear it will be suggested, it's not useful to treat ChromeOS as 'different' than Linux in this regard. We've made a concerted effort over several years to attempt to align their models and reasoning, precisely because of the code complexity and overhead involved with treating them differently. It also allows developers to more productively iterate and test on Linux without having to go through the ChromeOS provisioning-and-debugging phase. Solutions that are effective will acknowledge they have the same needs - through their use of the common library - even if certain specific elements may diverge (for example, third-party PKCS#11 modules in the case of Linux).

Finally, it's important to note that our NSS transition plan involves multiple stages. On Linux, NSS will *never* fully go away. On ChromeOS, the move to the NSS verifier is *not* sufficient in-and-of-itself to address the above issues. Even as we move to Chrome's verifier, we will sill interact with NSS on a variety of levels, as the effort to wholly move these activities to ChromeOS has not been staffed for four years now. pmarko@ is interested in tackling some of this, but this will be a multi-quarter effort that will affect a host of critical ChromeOS services (notably, chaps and wpa_supplicant), and due to the need to support users upgrading from older versions of ChromeOS, would still involve NSS for several years after that, even in a migratory capacity.

If the goal is to eventually get the NS out of process - for Linux, or for ChromeOS - then we will need to address these issues. We have one solution that is low overhead for multiple teams, and addresses current and future issues. As the decision seems to be that, despite us now attempting to S13N-ify ChromeOS, we're not going to pursue that, a solution that addresses the constraints above needs to be found. Those problems cannot be ignored - they simply shift additional burden to other developers and teams to support and deal with the issues that are caused by 'thread-unsafe' and 'process-unsafe' usage.

>  Otherwise having 3 places that update the network service doesn't seem like a burden, especially since per bug 897035 there are plans to make them all use the same code to update certs, which would be a single choke point.

I defer to Pavol, but I believe the proposed approach will make it much more difficult to accomplish that goal of Issue 897035.

That bug only looks at the ChromeOS side, which is a 'subset' of the Linux functionality (but they work the same and use the same infrastructure).

- Issue 897035 identifies 7 sources (S1.1 - S1.4, S2.1 - S2.2, S3.1), not 2.
- On Linux, and as being worked on for ARC++, there may be S1.5 - S1.* (Linux smart cards or ARC++ bridge points)
- There are additional sources relevant to current certification behaviours. For example, S3.2 (not listed there) are APIs for extensions to verify certificates themselves, which introduces them into the NSS process cache
- There is a tight coupling on Linux/ChromeOS between the verification of certificates and the selection of client certificates. The selection of a client certificate (and building a certificate path) is influenced by the verification of the server certificate. For example, if the server has a chain of "Root -> Intermediate -> Server Cert", and requests a client certificate from "Root", and the client only has "Client Certificate", then the "Intermediate" used for certificate verification is used to establish that there's a path from "Root -> Intermediate -> Client Certificate". This means it is a bidirectional path - it does not flow solely from one direction.
- The browser process, on Linux-SmartCard and for ChromeOS-Chaps (the necessary component to moving away from NSS), notifications for changes is not reliably distributed to all listening processes. Smart cards are largely 'single process' (and this was the discussion re: chaps in 29), and thus having both processes speak to the same card can create issues. Further, while NSS implicitly maintains state to track these events, the 'explicit' APIs to receive notifications (as an NSS consumer) are buggy - that is, a solution that says "Listen to NSS changes and propagate those across processes" is not a viable or reliable path.

These same issues exist on other platforms, like Windows and macOS. We don't maintain explicit tests for this, because generally speaking, on all platforms, such interactions are 'process-hostile' and to a lesser extent 'thread-hostile'. This is why it's important to colocate client certificates with certificate verification and certificate database modification - they are part of the same logical operation and with many of the same system dependencies (libraries, hardware, interaction models).

Some notes:
-your points about NSS database not being modified by multiple processes are noted and to be clear, I'm not advocating for that.
-in-process network service is fully supported: we will need to use it in production environments (e.g. WebView, Clank, Headless)
-re the "7 sources", what I asked is the number of places which modify the NSS store in a way that is used by the network process. So for example, policy-pushed certs and client cert don't matter, because the former already work with the patch above (as they don't go through the NSS store) and the latter work because client certs are always processed in the browser process
-I do appreciate you bringing up these issues, as there were no test coverage so I wasn't aware of them until you brought them up. I still think that they should be covered by another bug (or several, depending on how they manifest to the user). If there's no solution that is found which keeps the network service updated that is acceptable, then the network service will run in the browser until these issues are fixed (*). Regardless, the patch above will still be needed.
-While looking through the code, I saw net::CertDatabase::Observer. I tried connecting that notification to the network service, and that seemed to fix a new CA being added in chrome://certificate-manager not being noticed. Is there a scenario where listening to this in the browser is not reliable? Repro steps would be helpful.
-I'm still not sure why smart cards are being referenced, as client certs always happen in the browser even with the network service. The example above would still work with an out-of-process network service, since the new cert which is added to the store would be used by client certs (as those are in the browser). Is there a use case I'm missing?

*issues being fixed in the future doesn't imply a specific solution. i.e. if after NS launch, CV moves to the browser process because it's required by sandboxing, that'd be one possibility. NSS not being used for storage is another.

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

commit c626ad1cb4b95c14dc084ae2827f3b8d143a28aa
Author: John Abd-El-Malek <jam@chromium.org>
Date: Fri Oct 26 20:47:17 2018

Support policy-pushed trust anchors on ChromeOS with network service.

Also use CertVerifyProcChromeOS.

Bug:  862043 ,  887007 
Change-Id: I42bc36d58065db35cc70a1a2c587affec25a955a
Reviewed-on: https://chromium-review.googlesource.com/c/1292830
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603187}
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/DEPS
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/chromeos/policy/policy_cert_service.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/chromeos/policy/policy_cert_service.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/chromeos/policy/policy_cert_service_factory.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/chromeos/policy/policy_cert_service_factory.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/chromeos/policy/user_network_configuration_updater_factory_browsertest.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/io_thread.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/net/profile_network_context_service.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/net/profile_network_context_service.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/profiles/profile_io_data.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/profiles/profile_io_data.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/browser/ui/ash/session_controller_client_unittest.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/chrome/test/BUILD.gn
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/content/browser/network_service_client.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/content/browser/network_service_client.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/content/public/browser/content_browser_client.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/BUILD.gn
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/OWNERS
[rename] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/cert_verifier_with_trust_anchors.cc
[rename] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/cert_verifier_with_trust_anchors.h
[rename] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/cert_verifier_with_trust_anchors_unittest.cc
[rename] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/cert_verify_proc_chromeos.cc
[rename] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/cert_verify_proc_chromeos.h
[rename] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/cert_verify_proc_chromeos_unittest.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/network_context.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/network_context.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/public/mojom/network_context.mojom
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/public/mojom/network_service.mojom
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/test/test_network_context.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/test/test_network_service_client.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/test/test_network_service_client.h
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/services/network/url_loader_unittest.cc
[modify] https://crrev.com/c626ad1cb4b95c14dc084ae2827f3b8d143a28aa/testing/buildbot/filters/mojo.fyi.chromeos.network_browser_tests.filter

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

commit b344c2ab5db368f53026708b1085a0dee4e0fb6c
Author: John Abd-El-Malek <jam@chromium.org>
Date: Fri Oct 26 20:51:26 2018

Notify the network process when the certificate database changes

Bug:  862043 
Change-Id: I78c19396fbacfdf0bc1ee2bffa07b61e75f0ee4b
Reviewed-on: https://chromium-review.googlesource.com/c/1300793
Reviewed-by: Doug Turner <dougt@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603189}
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/content/browser/network_service_client.cc
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/content/browser/network_service_client.h
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/content/browser/utility_process_host.cc
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/content/public/common/content_switches.cc
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/content/public/common/content_switches.h
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/content/public/test/network_service_test_helper.cc
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/services/network/network_service.cc
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/services/network/network_service.h
[modify] https://crrev.com/b344c2ab5db368f53026708b1085a0dee4e0fb6c/services/network/public/mojom/network_service.mojom

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

commit be4a3ee09cf6395b33723c6b2d319b5328ac38f3
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Fri Oct 26 21:56:06 2018

Revert "Support policy-pushed trust anchors on ChromeOS with network service."

This reverts commit c626ad1cb4b95c14dc084ae2827f3b8d143a28aa.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 603187 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2M2MjZhZDFjYjRiOTVjMTRkYzA4NGFlMjgyN2YzYjhkMTQzYTI4YWEM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/8527

Sample Failed Step: compile

Original change's description:
> Support policy-pushed trust anchors on ChromeOS with network service.
> 
> Also use CertVerifyProcChromeOS.
> 
> Bug:  862043 ,  887007 
> Change-Id: I42bc36d58065db35cc70a1a2c587affec25a955a
> Reviewed-on: https://chromium-review.googlesource.com/c/1292830
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Pavol Marko <pmarko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#603187}

Change-Id: I67243629f4caf63da6e1e943dd9321a4b0e9f7a7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  862043 ,  887007 
Reviewed-on: https://chromium-review.googlesource.com/c/1303175
Cr-Commit-Position: refs/heads/master@{#603202}
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/DEPS
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
[rename] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc
[rename] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/net/cert_verify_proc_chromeos.h
[rename] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/net/cert_verify_proc_chromeos_unittest.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/policy/policy_cert_service.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/policy/policy_cert_service.h
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/policy/policy_cert_service_factory.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/policy/policy_cert_service_factory.h
[rename] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/policy/policy_cert_verifier.cc
[rename] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/policy/policy_cert_verifier.h
[rename] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/policy/policy_cert_verifier_unittest.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/chromeos/policy/user_network_configuration_updater_factory_browsertest.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/io_thread.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/net/profile_network_context_service.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/net/profile_network_context_service.h
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/profiles/profile_io_data.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/profiles/profile_io_data.h
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/browser/ui/ash/session_controller_client_unittest.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/chrome/test/BUILD.gn
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/content/browser/network_service_client.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/content/browser/network_service_client.h
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/content/public/browser/content_browser_client.h
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/BUILD.gn
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/OWNERS
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/network_context.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/network_context.h
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/public/mojom/network_context.mojom
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/public/mojom/network_service.mojom
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/test/test_network_context.h
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/test/test_network_service_client.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/test/test_network_service_client.h
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/services/network/url_loader_unittest.cc
[modify] https://crrev.com/be4a3ee09cf6395b33723c6b2d319b5328ac38f3/testing/buildbot/filters/mojo.fyi.chromeos.network_browser_tests.filter

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

commit 21bca7c96239e737e64a1455c7330cb448cad312
Author: John Abd-El-Malek <jam@chromium.org>
Date: Fri Oct 26 22:13:33 2018

Support policy-pushed trust anchors on ChromeOS with network service.

Also use CertVerifyProcChromeOS.

TBR=nasko@chromium.org, pmarko@chromium.org

Bug:  862043 ,  887007 
Change-Id: I5250a59fc18073ef4997dcb881392e79d080ded2
Reviewed-on: https://chromium-review.googlesource.com/c/1292830
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#603187}
Reviewed-on: https://chromium-review.googlesource.com/c/1303052
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603214}
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/DEPS
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/chromeos/policy/policy_cert_service.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/chromeos/policy/policy_cert_service.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/chromeos/policy/policy_cert_service_factory.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/chromeos/policy/policy_cert_service_factory.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/chromeos/policy/user_network_configuration_updater_factory_browsertest.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/io_thread.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/net/profile_network_context_service.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/net/profile_network_context_service.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/profiles/profile_io_data.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/profiles/profile_io_data.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/browser/ui/ash/session_controller_client_unittest.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/chrome/test/BUILD.gn
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/content/browser/network_service_client.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/content/browser/network_service_client.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/content/public/browser/content_browser_client.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/BUILD.gn
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/OWNERS
[rename] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/cert_verifier_with_trust_anchors.cc
[rename] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/cert_verifier_with_trust_anchors.h
[rename] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/cert_verifier_with_trust_anchors_unittest.cc
[rename] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/cert_verify_proc_chromeos.cc
[rename] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/cert_verify_proc_chromeos.h
[rename] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/cert_verify_proc_chromeos_unittest.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/network_context.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/network_context.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/public/mojom/network_context.mojom
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/public/mojom/network_service.mojom
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/test/test_network_context.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/test/test_network_service_client.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/test/test_network_service_client.h
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/services/network/url_loader_unittest.cc
[modify] https://crrev.com/21bca7c96239e737e64a1455c7330cb448cad312/testing/buildbot/filters/mojo.fyi.chromeos.network_browser_tests.filter

Please let me summarize the direction you seem to be taking for Chrome OS at the moment:
(1) Forward the policy-provided trust anchors to the network service
    (this is fine IMO)
(2) Forward policy-provided untrusted CA/server certs to the network service
    This has not been done yet, and will have to be done, but I can take care of that in bug 897233.
(2) Open the user's NSS softoken ("public slot") from the network service
    - if the network service runs in-process, this will be a no-op as it's already been done by profile_io_data.cc
    - if the network service runs out-of process, this will result in two processes accessing the softoken.
      While only the chrome process will make notifications, these will not become automatically visible in the network service process. You have prepared https://chromium-review.googlesource.com/c/chromium/src/+/1300793 to explicitly notify the network service process.
      This still relies on NSS relisting all certificates from disk e.g. in PK11_ListCerts and using them in server cert verification.
      To be clear: I do *not* know if this is fine with NSS. Ryan (who has, to my understanding, the biggest knowledge of NSS internals/API contracts in all of chromium) says it is not.

This IMO is still an issue if it can lead to race conditions etc. - I've LGTMed the CL in Comment #36/#39 as a policy OWNER (and because I think the policy trust anchors handling itself is fine), and tried to advise against doing this for now, but ultimately I'm not an OWNER of the net/crypto parts.

Other than this, the solution (in-network-service verification on Chrome OS) is probably fine, due to the following assumptions:
(*) chrome will be aware of all modifications of certificate stores, and will be able to forward the new information to the network service
 -> This is true on Chrome OS atm IMO.
    Disclaimer: I don't think ARC++ can currently add server/CA certs anywhere, and if this will be implemented, we'll have to think about this case carefully.
(*) smartcard-provided CA certificates do not currently exist on Chrome OS
    There is bug 859137 as a feature request to support them, and when we do, it will most probably be implemented in a very similar way to policy-pushed certificates, so I am assuming we can just send them over too. (This implies that we get notified by the smartcard driver extension that new CA certs are available instead of having to poll them).

So there's still the issue of opening the public slot softoken from two processes and hoping that changes will be reflected which I'd like to avoid if possible, and tbh I don't trust NSS to be comfortable shipping in this state.
I'll try to prototype a CL which operates with temp (in-memory) certs on the network service side only, and sends the current set of certificates over on updates, avoiding this issue.

Actually, splitted that part off into bug 899623.

Closing this bug as ONC pushed CA certs now work with network service. If there are repros of other bugs, please file new bugs.

Please provide steps if it requires manual verification. Thanks.!

Closing the issue as verified as per #c42. Thanks.!