Closed Bug 536301 Opened 10 years ago Closed 9 years ago

e10s HTTPS: securityInfo

Categories

(Core :: Networking: HTTP, defect)

Other Branch
x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0a1+ ---

People

(Reporter: jduell.mcbugs, Assigned: mayhemer)

References

Details

Attachments

(1 file, 7 obsolete files)

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?
Blocks: fennecko
Depends on: 530952
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.
Biesi says we already have some code to serialize mSecurityInfo, which we should use if we need to send it across IPDL.
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.
Assigning to Honza, and renaming, as this bug is really just about securityInfo/lock icon stuff now.
Assignee: nobody → honzab.moz
Summary: e10s HTTPS: make it work → e10s HTTPS: securityInfo
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 :)
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).
Blocks: 559711
Attached patch v1 (obsolete) — Splinter Review
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).
Attachment #446902 - Flags: review?(kaie)
Attachment #446902 - Flags: review?(jduell.mcbugs)
Status: NEW → ASSIGNED
Blocks: 568502
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.
Blocks: 568504
No longer blocks: 568502
Attached patch v2 (obsolete) — Splinter Review
Updated, TabParent now implements nsISecureBrowserUI.
Attachment #446902 - Attachment is obsolete: true
Attachment #448388 - Flags: superreview?(kaie)
Attachment #448388 - Flags: review?(jduell.mcbugs)
Attachment #446902 - Flags: review?(kaie)
Attachment #446902 - Flags: review?(jduell.mcbugs)
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
Attachment #448388 - Flags: superreview?(kaie) → superreview-
Depends on: 570616
Depends on: 569227
Attached patch v3 (obsolete) — Splinter Review
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.
Attachment #448388 - Attachment is obsolete: true
Attachment #450754 - Flags: superreview?(kaie)
Attachment #450754 - Flags: review?(jduell.mcbugs)
Attachment #448388 - Flags: review?(jduell.mcbugs)
Blocks: 570003
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?
Will change it, thanks for comments.
(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.
Honza, could you please add your comment 15 as a comment to the patch?
Attached patch v3.1 (obsolete) — Splinter Review
So, this should be the final patch.
Attachment #450754 - Attachment is obsolete: true
Attachment #451941 - Flags: superreview?(kaie)
Attachment #451941 - Flags: review?(jduell.mcbugs)
Attachment #450754 - Flags: superreview?(kaie)
Attachment #450754 - Flags: review?(jduell.mcbugs)
Comment on attachment 451941 [details] [diff] [review]
v3.1

Thanks, sr=kaie
Attachment #451941 - Flags: superreview?(kaie) → superreview+
Jason: do you want to review this patch or is Kai's sr+ enough to go forward?
Attached patch v3.1 - merged (obsolete) — Splinter Review
Merged to the current e10s tree.
Attachment #451941 - Attachment is obsolete: true
Attachment #453864 - Flags: superreview+
Attachment #453864 - Flags: review?(jduell.mcbugs)
Attachment #451941 - Flags: review?(jduell.mcbugs)
tracking-fennec: --- → 2.0a1+
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.
Attachment #453864 - Flags: review?(jduell.mcbugs) → review+
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.
Attachment #453864 - Flags: review+ → review-
Attached patch patch v.4 (obsolete) — Splinter Review
jduell, hanza, please take a look at this.
Attachment #453864 - Attachment is obsolete: true
Attachment #460000 - Flags: review?
Attachment #460000 - Attachment is patch: true
Attachment #460000 - Attachment mime type: application/octet-stream → text/plain
Attachment #460000 - Flags: review? → review?(jduell.mcbugs)
(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 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...
(In reply to comment #25)
> secInfoSer is null here, and that is cause warning...

I'll fix it in the next patch version.
Comment on attachment 460000 [details] [diff] [review]
patch v.4

waiting for a new patch.
Attachment #460000 - Flags: review?(jduell.mcbugs) → review-
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.
Attached patch v5 (obsolete) — Splinter Review
- 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
Attachment #460000 - Attachment is obsolete: true
Attachment #462034 - Flags: superreview?(jduell.mcbugs)
Attachment #462034 - Flags: review?(doug.turner)
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?
Attachment #462034 - Flags: review?(doug.turner) → review+
(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.
Attached patch v5.1Splinter Review
Updated.
Attachment #462034 - Attachment is obsolete: true
Attachment #462397 - Flags: superreview?(jduell.mcbugs)
Attachment #462397 - Flags: review+
Attachment #462034 - Flags: superreview?(jduell.mcbugs)
Will look at this today, though understanding the lifetime model of PHttpChannel might take a bit. I'll coordinate with Jason here...
Attachment #462397 - Flags: superreview?(jduell.mcbugs) → superreview+
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.
landed on m-c.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 582840
You need to log in before you can comment on or make changes to this bug.