Closed
Bug 1512598
Opened 6 years ago
Closed 5 years ago
Provide baseline for starting PSM in an "isolated" mode on the Socket Process
Categories
(Core :: Security: PSM, enhancement, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
| Tracking | Status | |
|---|---|---|
| firefox65 | --- | affected |
People
(Reporter: mayhemer, Assigned: dragana)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [psm-assigned])
Attachments
(1 file, 4 obsolete files)
|
(deleted),
patch
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
|
|
Reporter | |
Comment 2•6 years ago
|
||
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•6 years 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•6 years 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•6 years ago
|
||
- 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•6 years ago
|
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
|
Assignee | |
Updated•6 years ago
|
Blocks: socket-proc
|
|
Assignee | |
Updated•6 years ago
|
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 7•6 years ago
|
||
|
|
Assignee | |
Comment 8•6 years 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)
|
|
Assignee | |
Comment 10•6 years ago
|
||
|
||
Updated•6 years ago
|
Attachment #9050055 -
Attachment is obsolete: true
|
|
||
Updated•6 years ago
|
Attachment #9058426 -
Attachment description: 1512598 - NSS initialization without nsNSSComponent for the socket process. r=keeler → 1512598 - NSS initialization without nsNSSComponent for the socket process. r=keeler r=kershaw
|
||
Comment 11•6 years ago
|
||
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.
Flags: needinfo?(dd.mozilla)
|
|
Assignee | |
Comment 12•5 years ago
|
||
This is not for m.-c. this is a separate project.
Flags: needinfo?(dd.mozilla)
|
||
Comment 13•5 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
|
|
||
Updated•5 years ago
|
Attachment #9058426 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•