1512598 - Provide baseline for starting PSM in an "isolated" mode on the Socket Process

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[Honza Bambas (:mayhemer)]

Take parts from https://bugzilla.mozilla.org/attachment.cgi?id=9029844&action=edit

includes:
- recognize the socket process and bypass initiation of various parts that are not necessary/available on the socket process
- allow PSM socket providers to run on the socket process

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: wip, backup

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

Purpose of this patch is to only provide a basic support for running PSM on the socket process (have the socket providers up, mainly).  This just unblocks some of the other work blocking the meta bug 1484751.

With this patch we can load secure pages, but only because certificate validation is completely bypassed.  The UI will not show any certificate/security state.

This is intended to land only on the larch branch for now.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 9030412 [details] [diff] [review]
v1

Review of attachment 9030412 [details] [diff] [review]:
-----------------------------------------------------------------

I have a recommendation for a slightly different approach - let me know what you think.

::: security/manager/ssl/nsNSSComponent.h
@@ +124,5 @@
>    // it will never complete) so we use this boolean to keep track of if we
>    // should wait.
>    bool mLoadLoadableRootsTaskDispatched;
> +
> +  // Whether we are running on an isolated process and have to IPC to the parent

Sharing the implementation of nsNSSComponent between the main process and the network process probably won't serve us well in the long run. nsNSSComponent is basically half things that need to run in the main process (loading roots, configuring the certificate verifier, etc.) and half things that need to run in the network process (configuring NSS callbacks, setting ciphersuite prefs, etc.). The one thing they have in common is that they need to initialize NSS, but even in that case we want to initialize NSS in different ways, so that's not even really shared.

I think for the network process you'll want to do something more like the last half of this function: https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/security/manager/ssl/nsNSSComponent.cpp#127 (but with the addition of also configuring the NSS callbacks).

In fact, I would just update that function (and maybe name it something a bit more general). That way, things like https://searchfox.org/mozilla-central/rev/adcc169dcf58c2e45ba65c4ed5661d666fc3ac74/security/manager/ssl/nsNSSModule.cpp#92 will still work.

Comment 4

[Honza Bambas (:mayhemer)]

Thanks for the review and feedback.  However I want to clarify what you suggest first, I see it as follows:

- instead of adding a number of |if (mIsIsolated)| rather have a whole second class/singleton-object to do the necessary work only for the socket process (and content process(es))
- so, we have the component doing the necessary work for the chrome process (bind to the profile) and a second component that does the network bind part
- with socket process (SP) =off let the chrome part also instantiate the net part, both on the parent process, both sharing the same instance of NSS bound to the profile
- with SP=on let only the chrome part do its init part, the net part on the socket process will create the child/parent channel on its own, using its own instance of NSS in-mem only

Is it that?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Yes, but I think even having a second class/singleton would be unnecessary - it should be sufficient to extend EnsureNSSInitializedChromeOrContent (with maybe some refactoring of nsNSSComponent into more granular functions that both can call).

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v2 - wip

• some methods renamed, as EnsureNSSInitializedChromeOrContent -> EnsureNSSInitializedProcessSpecific
• cert verification is turned off when sockets live on the socket process (as a workaround to allow https pages to load w/o any sec info associated)
• I have added (split) init and shutdown methods to be invoked on chrome or socket process respectively (or all only on the chrome process when socket process is disabled)

The next problem this patch has is SharedSSLState object livetime; it seems to need to live on both the chrome and socket process and also may need to be synced (serialized) between the two. I don't know what all it's used for and how it flows, so hard to say what strategy to choose

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: Bug 1512598 - Init nss for the socket process.

Comment 8

[Dragana Damjanovic [:dragana]]

In your comment on phabricator you suggested that the socket process does not need nsNSSComponent. So I was working on such a approach but I hit issue at:

https://searchfox.org/mozilla-central/source/security/manager/ssl/nsNSSCallbacks.cpp#1071

this is called from the socket process.
Can I forward this to the parent process async?

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Yes - I think that would be a good idea. (I mean, I don't think it's possible right now, but that's why you're writing this code, right?)

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: 1512598 - NSS initialization without nsNSSComponent for the socket process. r=keeler r=kershaw

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:dragana, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 12

[Dragana Damjanovic [:dragana]]

This is not for m.-c. this is a separate project.

Comment 13

[Kershaw Chang [:kershaw]]

https://hg.mozilla.org/projects/larch/rev/70705acfab026fca13c600ece7e02d16fb0fdc74

Landed to larch.