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

ASSIGNED
Assigned to

Status

()

enhancement
P1
normal
ASSIGNED
5 months ago
7 days ago

People

(Reporter: mayhemer, Assigned: dragana)

Tracking

(Blocks 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 affected)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 months ago
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
(Reporter)

Comment 1

4 months ago
Posted patch wip, backup (obsolete) — Splinter Review
(Reporter)

Comment 2

4 months ago
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #9030283 - Attachment is obsolete: true
Attachment #9030412 - Flags: review?(dkeeler)
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.
Attachment #9030412 - Flags: review?(dkeeler) → review-
(Reporter)

Comment 4

4 months ago
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?
(Reporter)

Updated

4 months ago
Flags: needinfo?(dkeeler)
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).
Flags: needinfo?(dkeeler)
Priority: -- → P1
Whiteboard: [psm-assigned]
(Reporter)

Comment 6

3 months ago
Posted patch v2 - wipSplinter Review
  • 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

Attachment #9030412 - Attachment is obsolete: true
(Reporter)

Updated

3 months ago
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

a month ago
Blocks: socket-proc
(Assignee)

Updated

a month ago
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 8

27 days ago

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?

Flags: needinfo?(dkeeler)

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?)

Flags: needinfo?(dkeeler)
Attachment #9050055 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.