536301 - e10s HTTPS: securityInfo

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

This may be very easy: it may just work.   Right now my patch for bug 530952 doesn't allow HTTPS in HttpChannelChild::Init, but I should uncomment the check and see how we do.  It's likely to work fine for vanilla HTTPS requests.

Can anyone think of cases that might be tricky here?  All the HTTPS/NSS logic will be entirely in chrome.  Redirects from HTTP->HTTPS (or vice versa) will presumably need to notify the child so that the lock icon can be displayed or removed.  Anything else?

Comment 1

[Jason Duell]

From necko notes:

To determine if Lock Icon needed, content process will need handle to securityInfo, which is currently pointed to by nsHttpChannel.  Shouldn't need to actually read security info--just let child know whether it's present.

Comment 2

[Jason Duell]

Biesi says we already have some code to serialize mSecurityInfo, which we should use if we need to send it across IPDL.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Correct, it implements nsISerializable. See also http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsSerializationHelper.h if necessary.

Comment 4

[Benjamin Smedberg]

I think we should try to avoid exposing mSecurityInfo the content process as anything other than a handle... I don't think that the content process actually needs access to the data, although I guess it's possible that scriptsecuritymanager uses it somehow.

Comment 5

[Jason Duell]

Assigning to Honza, and renaming, as this bug is really just about securityInfo/lock icon stuff now.

Comment 6

[Jason Duell]

One thing that I assume is part of this bug (or a new bug if not):

nsNSSSocketInfo::EnsureDocShellDependentStuffKnown() tries to determine "if SSL error reporting is needed".  It currently does so by trying to get the DocShell out of its mCallbacks, which winds up being HttpChannelParent.

This isn't going to work in e10s--we need to figure out some other way to query this info.  We can pass along whatever flag/data we need during AsyncOpen and set it in the HttpChannelParent.  But not the whole docshell :)

Comment 7

[Jason Duell]

To trigger the EnsureDocShellDependentStuffKnown query, go to either http://wiki.mozilla.org or http://bugzilla.mozilla.org.

This was crashing (bug 561830) but is getting fixed to simply return NULL (but the functionality is still not there, obviously).

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1

Roughly tested (missing mochitests to check the corner cases).  TabParent::RecvnotifySecurityChange now gets all info needed, we now can attach proper listeners to it.

nsIAssociatedContentSecurity interface is used to store different security state sub-requests count, those are determined on the child process.  But, those numbers must be persisted to cache (used by the session store).  Therefore I push them to the parent process through the channel.

The EV status cannot be determined by the child process (a need for nss be initialized and access to the cert database of the profile).  I moved determination of the EV status to the parent process (TabParent).

Comment 9

[Honza Bambas (:mayhemer)]

I believe part of this bug should also be to let TabParent implement nsISecureBrowserUI to let browser.xml from Firefox and Fennec to bind to it.

Changing each browser.xml should happen in separate bugs.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v2

Updated, TabParent now implements nsISecureBrowserUI.

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 448388 [details] [diff] [review]
v2

Some minor change requests and a question.


> [scriptable, uuid(8DB92DDE-799F-4d33-80F7-459CAC800DC9)]
> interface nsIAssociatedContentSecurity : nsISupports
> {
>   attribute long countSubRequestsHighSecurity;
>   attribute long countSubRequestsLowSecurity;
>   attribute long countSubRequestsBrokenSecurity;
>   attribute long countSubRequestsNoSecurity;
>+  void flush();

Please change the UUID whenever you change an interface.


+NS_IMETHODIMP
+HttpChannelChild::GetCountSubRequestsHighSecurity(PRInt32 *aSubRequestsHighSecurity)
+{
+  nsCOMPtr<nsIAssociatedContentSecurity> assoc =
+      do_QueryInterface(mSecurityInfo);
+  if (!assoc)
+    return NS_OK;
+
+  return assoc->GetCountSubRequestsHighSecurity(aSubRequestsHighSecurity);
+}

You add 8 Get/Set functions. In each of them, if "assoc" is null, you allow it and continue without warning.

But is it allowed to have no associated content security?

If there is none, could this be an inconsistency?
If mSecurityInfo != null, is it really ok if QueryInterface fails?
Please try to answer that question (to yourself), before you commit the code.


+  PRInt32 hi, low, broken, no;
+  assoc->GetCountSubRequestsHighSecurity(&hi);
+  assoc->GetCountSubRequestsLowSecurity(&low);
+  assoc->GetCountSubRequestsBrokenSecurity(&broken);
+  assoc->GetCountSubRequestsNoSecurity(&no);

Today the implementation of those function is unconditional, but who knows what will happen in the future? Maybe the functions will change and might even fails?

In other words, I'd sleep better if you initialize:
 hi=low=broken=no=0; 


If you address my comments, and you are not worried about inconsistencies, r/sr=kaie

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v3

Merged, updated.  Now http channel QI's to nsIAssociatedContentSecurity only when there really is this interface provided by the security info object.  I believe consumers (not the channel) should handle when it is missing.

Comment 13

[Kai Engert (:KaiE:)]

Thanks for making these changes. I'm mostly happy, just the following minor points:


I find variable names secInfoSer, secInfoSerialization too similar, which makes it difficult to read. One of them is a string, and the variable should say so. I propose, rename secInfoSerialization to secInfoAsString.


I find the purpose of aUseSSLStatus difficult to grasp from the interface. On sender side you call it aUseSecuritystatus, on receiverside aUseSSLStatus. I propose to name it the same on both sender and recipient. I propose the name aExtractContainedSSLStatus.

On the other hand, I don't understand why you need that variable at all. Is there a reason you don't always attempt to extract on the receiving side?

Comment 14

[Honza Bambas (:mayhemer)]

Will change it, thanks for comments.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to comment #13)
> On the other hand, I don't understand why you need that variable at all. Is
> there a reason you don't always attempt to extract on the receiving side?

Forgot to explain: security UI internally holds 4 states: secure, mixed, broken, no security.  In cases of secure, mixed and broken it holds reference to a valid SSL status object.  But, in case of the 'broken' state it doesn't return the SSL status object (returns null), in contrary to the 'mixed' state for which it returns it.

However, mixed and broken states are both reported to the upper level (the level the patch works on) as nsIWebProgressListener::STATE_IS_BROKEN, i.e. states are merged, so I cannot determine, if to return the status object or not.

In other words aUseSSLStatus actually represents the 'broken' state.

Comment 16

[Kai Engert (:KaiE:)]

Honza, could you please add your comment 15 as a comment to the patch?

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: v3.1

So, this should be the final patch.

Comment 18

[Kai Engert (:KaiE:)]

Comment on attachment 451941 [details] [diff] [review]
v3.1

Thanks, sr=kaie

Comment 19

[Honza Bambas (:mayhemer)]

Jason: do you want to review this patch or is Kai's sr+ enough to go forward?

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v3.1 - merged

Merged to the current e10s tree.

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 453864 [details] [diff] [review]
v3.1 - merged

+      nsresult rv = provider->GetSSLStatus(getter_AddRefs(supports));
+      if (NS_SUCCEEDED(rv) && supports) {

Would we ever get one without the other?

What kind of tests do we have for this?  Has it passed try?

Otherwise looks fine.

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 453864 [details] [diff] [review]
v3.1 - merged

when updating this to m-c, i noticed some problems:

1) TabParent didn't implement the secure browser ui.  I am not sure if it needs to do that, or if it just needs to implement the provider.  I implemented both, and it compiles, etc.

2) The method names in pidl should be in mixed case.

Comment 23

[Doug Turner (:dougt)]

Attachment: patch v.4

jduell, hanza, please take a look at this.

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to comment #22)
> Comment on attachment 453864 [details] [diff] [review]
> v3.1 - merged
> 
> when updating this to m-c, i noticed some problems:
> 
> 1) TabParent didn't implement the secure browser ui.  

Not sure what you mean, but this [1] and this [2] seem to be the implementation, don't they?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=453864&action=diff#a/dom/ipc/TabParent.cpp_sec1
[2] https://bugzilla.mozilla.org/attachment.cgi?id=453864&action=diff#a/dom/ipc/TabParent.h_sec2

> I am not sure if it needs
> to do that, or if it just needs to implement the provider.  I implemented both,
> and it compiles, etc.
> 
> 2) The method names in pidl should be in mixed case.

You mean upper camel case? 


(In reply to comment #23)
> Created attachment 460000 [details] [diff] [review]
> patch v.4
> 
> jduell, hanza, please take a look at this.

Looks good, thanks Doug.

We have tests for mixed content, I'll try to run them (adjust them) locally.

Comment 25

[Oleg Romashin (:romaxa)]

Comment on attachment 460000 [details] [diff] [review]
patch v.4

I've found one permanent warnings with this patch:
WARNING: NS_ENSURE_TRUE(classInfo) failed: file xpcom/io/nsBinaryStream.cpp, line 285
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file netwerk/base/src/nsSerializationHelper.cpp, line 67


 HttpChannelParent::OnStartRequest
>+  nsCOMPtr<nsISerializable> secInfoSer =
>+      do_QueryInterface(secInfoSupp);
>+
>+  nsCString secInfoSerialization;
>+  NS_SerializeToString(secInfoSer, secInfoSerialization);

secInfoSer is null here, and that is cause warning...

Comment 26

[Honza Bambas (:mayhemer)]

(In reply to comment #25)
> secInfoSer is null here, and that is cause warning...

I'll fix it in the next patch version.

Comment 27

[Doug Turner (:dougt)]

Comment on attachment 460000 [details] [diff] [review]
patch v.4

waiting for a new patch.

Comment 28

[Honza Bambas (:mayhemer)]

Bug 572980 breaks basic logic of this patch.  We need the top level document channel pair live after OnStopRequest.  We need to store vital important data to the nsHttpChannel running on the parent process and source for that data is running on the content process long after the document has load.

We have one of these options:
1. let the top level document channel live after OnStopRequest
2. introduce new protocol for communicating the data from content process to parent process and do more modifications to otherwise working code

IMHO the first option is simpler to implement and cost the same or less as the second option.

Comment 29

[Honza Bambas (:mayhemer)]

Attachment: v5

- based on v4
- fixed the warning (checking non-null secInfo object)
- I leave the doc loading channel live until it is released on the content process; I need this for communicating security state counters
- as discussed at the last e10s meeting, we will land this w/o automated tests

Comment 30

[Doug Turner (:dougt)]

Comment on attachment 462034 [details] [diff] [review]
v5


>+      nsCOMPtr<nsISSLStatusProvider> provider = 
>+        do_QueryInterface(secureUI);

Put this on one line.

>-NS_IMPL_ISUPPORTS3(TabParent, nsITabParent, nsIWebProgress, nsIAuthPromptProvider)
>+  NS_IMPL_ISUPPORTS5(TabParent, nsITabParent, nsIWebProgress, nsIAuthPromptProvider, nsISSLStatusProvider, nsISecureBrowserUI)


no spaces in front of this macro


>+    nsCOMPtr<nsISupports> secInfoSupports;
>+    nsresult rv = NS_DeserializeObject(aSecInfoAsString, getter_AddRefs(secInfoSupports));
>+

do you actually have to test the result here, or is testing the outvar enough?

Comment 31

[Honza Bambas (:mayhemer)]

(In reply to comment #30)
> do you actually have to test the result here, or is testing the outvar enough?

I have to test rv, see the implementation, and I believe it's a common practice.

Comment 32

[Honza Bambas (:mayhemer)]

Attachment: v5.1

Updated.

Comment 33

[[e-mail]]

Will look at this today, though understanding the lifetime model of PHttpChannel might take a bit. I'll coordinate with Jason here...

Comment 34

[Jason Duell]

landed on e10s:

http://hg.mozilla.org/projects/electrolysis/rev/5936477a4dc8

Comment 35

[Jason Duell]

By the way, the lifecycle change in this patch only keeps PHttpChannel around for lifetime of HttpChannelChild iff channel is a document load.  Most channels will still destroy PHttpChannel at OnStopRequest.  I think that's fine.

Comment 36

[Jason Duell]

landed on m-c.