Closed
Bug 94974
Opened 23 years ago
Closed 8 years ago
Provide an interface for embeddors to access security info..
Categories
(Core Graveyard :: Embedding: APIs, defect)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
mozilla1.1alpha
People
(Reporter: chak, Assigned: chpe)
Details
(Keywords: embed, topembed-)
Attachments
(5 files, 1 obsolete file)
1.25 KB,
patch
|
rpotts
:
review+
|
Details | Diff | Splinter Review |
23 years ago
6.32 KB,
patch
|
Details | Diff | Splinter Review | |
6.32 KB,
patch
|
Details | Diff | Splinter Review | |
15.25 KB,
patch
|
Details | Diff | Splinter Review | |
1.95 KB,
patch
|
dveditz
:
review+
|
Details | Diff | Splinter Review |
We need to determine an appropriate PSM interface to expose to embeddors using which they can query for security info such as cert info. etc.
Reporter | ||
Comment 1•23 years ago
|
||
Without having to come up with any new interface[s] here's what i propose: Allow embeddors to get access to nsISSLStatus via nsIWebBrowser by doing the following: 1. In our nsIWebBrowser impl we create/maintain nsISecureBrowserUI 2. We can do a QI on nsISecureBrowserUI to get nsISSLStatusProvider 3. We call nsISSLStatusProvider::GetSSLStatus() which returns an nsISSLStatus which we pass it on to the embeddors Using nsISSLStatus the embeddor can get the security info. such as the cert info, cipher strength etc. Terry : Please let me know what you think of this approach....Thanks
Comment 2•23 years ago
|
||
are we already doing #1 and #2? is nsISSLStatus everything someone would want to know about a page from a security standpoint? what about encryption level? Also, if we go this route, do we want to drag in nsIX509Cert; sounds scary?
Reporter | ||
Comment 3•23 years ago
|
||
> are we already doing #1 and #2? Actually we just do #1 for getting OnSecurityChange() working. We'd have to add code to #2 and #3 >is nsISSLStatus everything someone would want to know about a page from a >security standpoint? >what about encryption level? Contrary to its name nsISSLStatus provides a lot more info than just SSL status. The methods in it are : GetCipherName() GetSecretKeyLength()[using which we can determine the encryption strenght] >Also, if we go this route, do we want to drag in nsIX509Cert; sounds scary? As you've probably seen nsISSLStatus also has GetServerCert() which returns nsIX509Cert using which embeddors can impl the ViewCert feature if they choose to.
Comment 4•23 years ago
|
||
What you describe (steps 1-3) is exactly what the security overlay for PageInfo does to get the information it needs to display. I have no problem doing it that way.
Reporter | ||
Comment 5•23 years ago
|
||
Reporter | ||
Comment 6•23 years ago
|
||
Reporter | ||
Comment 7•23 years ago
|
||
Reporter | ||
Comment 8•23 years ago
|
||
Hi Terry : Could you please r= this when you get a chance...Thanks
Updated•23 years ago
|
QA Contact: mdunn → depstein
Comment 9•23 years ago
|
||
r=thayes for the portions that use the ISSLStatus interface.
Reporter | ||
Comment 10•23 years ago
|
||
Rick/Jud : Can i get an r= on the following attachment: http://bugzilla.mozilla.org/attachment.cgi?id=47214&action=view Can one of your sr= it as well........thanks
Updated•23 years ago
|
Attachment #47214 -
Flags: review+
Comment 11•23 years ago
|
||
Comment on attachment 47214 [details] [diff] [review] Modify nsWebBrowser::GetInterface() to be able to get nsISSLStatus r=rpotts. looks good to me.
Reporter | ||
Comment 12•23 years ago
|
||
sr=rpotts [After getting Rick's OK this morning]
Reporter | ||
Comment 13•23 years ago
|
||
Fix checked in...
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 14•23 years ago
|
||
This is bad. This checkin made the main tree dependent upon building PSM which is still an optional component. (In fact, it still doesn't build on BeOS.) In theory, we can add a -DENABLE_CRYPTO define but then we run the risk of PSM not being pluggable into builds that weren't built with that flag.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 15•23 years ago
|
||
Hi Chris : This checkin did not introduce any new dependency on PSM - we've already had this dependency when we added the code to create an instance of |nsISecureBrowserUI| in nsWebBrowser::Create() Thanks Chak
Comment 16•23 years ago
|
||
Chak, your patch added a #include of nsISSLStatus.h, which is a PSM header. That's the problem cls is citing. We need to avoid directly coupling that way. At the least, #ifdef the #include and the code that uses its definitions. Even better if we can use XPCOM to do a runtime binding. This needs to get fixed pronto. /be
Updated•23 years ago
|
Keywords: mozilla0.9.5
Updated•23 years ago
|
Keywords: mozilla0.9.5
Reporter | ||
Comment 17•23 years ago
|
||
Got it. I'll work with cls to get this taken care of..Thanks Brendan
Reporter | ||
Comment 18•23 years ago
|
||
Change to nsWebBrowser being backed out by Waterson for the time being...
Comment 19•23 years ago
|
||
The problem is that |nsISSLStatus| is defined by the PSM module, not in the base code. |nsISecureBrowserUI| is defined in the base level code, and we use the XPCOM runtime binding to avoid adding the dependency. Until now the only user of |nsISSLStatus| was another portion of PSM itself (the UI layer which corresponds with the work the Chak is doing). Seems like #define is the easiest way out it this case, since we know whether security features are being included at the point where we build the embedding test application.
Comment 20•23 years ago
|
||
The problem with the define is that you will no longer be able to "add-on" psm to existing builds. If we use that define, either you were compiled with psm or you weren't. If you weren't, then you're missing some psm support. And requiring everyone to build psm is not an option. This leads right back into the interface/implementation split I posted about a few weeks ago. In theory, using an interface does not mean that you need to build the implementation of said interface. So, in order to get around problems like this and to avoid adding defines that will further damage our modularity story, I'm proposing that we do create the master list of IDLs/interfaces and export them from a single place in the tree.
Comment 21•23 years ago
|
||
cls: there's no necessity to centralize interfaces in the source tree. As with the autoconfig changes to modules/libpref that couple it to ldap (alecf can cite the bug), the alternative technique we should use is for the consumer to publish its own "callback" interface that the producer can implement. The producer then configures the consumer to use that interface (implemented by a singleton, or with whatever multiplicity makes sense for the particular case; for "remove pref providers" such as LDAP, a singleton is fine). We have the tools, we're just not using them. Source tree centralization has nothing to do with the problem; it's not the solution. /be
Comment 22•23 years ago
|
||
s/remove pref providers/remote pref providers/ /be
Comment 23•23 years ago
|
||
I was going to post something similar to what brendan said: I think that moving interfaces into their own directory do not actually solve architectural issues, nor promote proper architecture. The proper solution is what brendan is saying: if you're interacting with a component but don't want that component to depend on you, you need that component to provide an interface for you to implement... I have a few places where I am fixing this, like bug 101995. Maybe a similar approach can be used here?
Comment 24•23 years ago
|
||
As Brendan says, we have the tools. However the organization of the components does not allow what you need at the moment. |ISSLStatus| is defined in an optional component (PSM), so it is impossible for another component to use it without adding an unwanted dependency. There are two solutions to this: 1) move |ISSLStatus| to a "core" location, possibly alongside |ISecureBrowserUI|, or 2) break PSM up into parts, with |ISSLStatus| being defined in one (core) piece, and the rest of PSM compiled optionally.
3: have the embedding code provide an nsIEmbeddingSecurityInfo interface, and have the PSM code implement it. I like 3 best of all.
Reporter | ||
Comment 27•23 years ago
|
||
->1.0 since there's still some dependency issues with PSM
Target Milestone: mozilla0.9.7 → mozilla1.0
Updated•22 years ago
|
Updated•22 years ago
|
QA Contact: depstein → carosendahl
Comment 29•20 years ago
|
||
(In reply to comment #28) > Setting to 1.1 1.1 has come and gone. Should this just be marked WONTFIX then?
Assignee | ||
Comment 30•20 years ago
|
||
I propose to just allow embedders to get the nsISecureBrowserUI from nsWebBrowser with GetInterface. This will not add any additional PSM dependency on nsWebBrowser, since it already uses nsISecureBrowserUI (mSecurityUI variable). The app can then QI it to nsISSLStatusProvider and get the nsISSLStatus from that, if the embedding app depends on PSM. Embedders (Epiphany and Galeon) already use the nsISecureBrowserUI interface (currently by instantiating its implementation (nsSecureBrowserUIImpl) again, which has the nasty side-effect of always showing TWO instances of the security warning dialogues). nsIEmbeddingSecurityInfo as suggested in comment 25 may be a nice idea, but embedders really need this interface _now_.
Assignee | ||
Comment 31•20 years ago
|
||
Updated patch with comments from caillon. Asking for r/sr now.
Attachment #170730 -
Attachment is obsolete: true
Attachment #170844 -
Flags: superreview?(darin)
Attachment #170844 -
Flags: review?(dveditz)
Comment 32•20 years ago
|
||
Comment on attachment 170844 [details] [diff] [review] updated patch for GI from nsWebBrowser to nsISecureBrowserUI nsISecureBrowserUI is not frozen. That interface should really be frozen before this API is relied upon in any products.
Comment 33•20 years ago
|
||
i don't even understand what that interface does. that interface should not be frozen without detailed comments (at the very least). i'd also like to have a chance to comment on the interface before it freezes.
Comment 34•20 years ago
|
||
>nsISecureBrowserUI is not frozen. That interface should really be frozen
>before this API is relied upon in any products.
It would be nice if we (galeon and epiphany) didn't have to rely on non-frozen
API's but there is a lot of stuff that you just can't do if you limit yourself
to frozen APIs.
This bug isn't something that 'would be nice' it is vital that a solution be
found, as there doesn't seem to be anyway for the embeddors to work out if the
security state is 'broken' except by using nsISecureBrowserUI. This information
is crucial for embeddors for displaying the lock icon correctly, and it's much
better to have a non-frozen interface for this, than non at all.
Comment 35•20 years ago
|
||
I agree, but a parallel task should be to freeze nsISecureBrowserUI. If you want a stable API, then freezing this interface should be important to you.
Comment 36•20 years ago
|
||
Comment on attachment 170844 [details] [diff] [review] updated patch for GI from nsWebBrowser to nsISecureBrowserUI Failing any knights on white chargers coming in to implement a frozen embedding security iface I'm OK with this. r=dveditz but dougt should probably look too.
Attachment #170844 -
Flags: review?(dveditz) → review+
Comment 37•20 years ago
|
||
Ugh.. this is very ugly. We're exposing an interface to an unfrozen interface (nsISecureBrowserUI), so that we can QI that interface pointer to another unfrozen interface (nsISSLStatusProvider), so we can call a method on that interface to access the unfrozen nsISSLStatus. Someone should really sit down and create a proper, documented API for accessing this stuff. Unless this API is frozen and properly documented, we can't promise that it will remain intact in future versions of Mozilla since future developers may be unaware of an embedder depending on this API.
Comment 38•20 years ago
|
||
I agree that we have to create a well documented frozen interface to "fix" this bug. I am not sure if what we have is the right interface or if it is the wrong interface. I think the basic requirement would be to get the x509 cert. I think in the mix content case, this patch will fail to show the embedder the true state of the page. For example what if multiple ssl pipes loaded the current page being displayed by the web browser. I think that it would be cleaner if from a nsIWebBrowser you could get a set of http channels that loaded the page -- or at least something that represented that load information. Then off of this object you can get security info.
Comment 39•20 years ago
|
||
chpe, crispin: i understand that you just want something to work, but is there any chance that you might be willing to invest some time to build a proper API for this?
Comment 40•19 years ago
|
||
really, no response? anyone still care about this patch?
Assignee | ||
Comment 41•19 years ago
|
||
Sorry for not commenting ealier, it just slipped off my radar. > i understand that you just want something to work, but is there > any chance that you might be willing to invest some time to build a proper API > for this? I'm willing to invest some time to implement an API which addresses the embedders (Epiphany and Galeon) needs. So let's take a look at what we currently use nsISecureBrowserUI [http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsISecureBrowserUI.idl] for: 1) get security state and short description ("tooltip"): nsISecureBrowerUI::GetState and ::GetTooltipText. 2) QI it to nsISSLStatusProvider to get the nsISSLStatus [http://lxr.mozilla.org/seamonkey/source/security/manager/ssl/public/nsISSLStatus.idl] off of it. With the info from nsISSLStatus, we build our security information dialogue / show the certificate info. If I was to build this from scratch, here's what I would want: I need to get the security information for the current page: SSL version, ciphers and keylengths used, server certificate, and client certificate (BTW, I haven't found any way to get the client certificate). Having that, I can build the security description ("tooltip") myself, avoiding i18n issues with the current way (::GetTooltipText). Now about how to make that into an API: should we simply define an interface (nsISSLStatus2 ?), and make nsWebBrowser's GI hand out that object ? However, comment 16 ff suggest that that's not possible without makeing nsWebBrowser depend on PSM.
Comment 42•19 years ago
|
||
Some issues to consider: SSL is not the only possible security provider. It is possible someone could eventually implement SASL security layers, for instance. Some possible URL schemes could have an object security model instead of a transport security model. This, along with the fact that PSM isn't necessarily installed, is the reason that the tooltip is provided by PSM as a string. There is also the issue that we need to be able to extend SSL without necessarily changing the consumers of it. The strength of a given keylength depends on the cipher--ECC ciphers have shorter keylengths for a given strength than do RSA or DSA ciphers. Kerberos over SSL does not use X.509 server certificates.
Comment 43•19 years ago
|
||
Any updates? Gecko 1.8 is closing in fast.
Assignee | ||
Comment 44•19 years ago
|
||
(In reply to comment #43) > Any updates? Gecko 1.8 is closing in fast. I'm not sure how to proceed here. Comment 42 implies that just handing out nsISSLStatus from nsWebBrowser's GI isn't acceptable. So we could make it hand out an object you can GI or QI to the actual security info, among them nsISSLStatus. But that's just like it would be if we just made nsWebBrowser:GI hand out the nsISecureBrowserUI object (as detailed in comment 41).
Assignee | ||
Comment 45•19 years ago
|
||
Comment on attachment 170844 [details] [diff] [review] updated patch for GI from nsWebBrowser to nsISecureBrowserUI This patch doesn't apply anymore since the fastback changes, cancelling sr request. Thanks to the fastback work, I can now get the nsISecureBrowserUI object from the docshell; but I still think a frozen way to do it would make sense.
Attachment #170844 -
Flags: superreview?(darin)
Comment 46•19 years ago
|
||
Especially since nsIDocShell is in bad need of a housecleaning...
Updated•17 years ago
|
Assignee: chak → nobody
Status: REOPENED → NEW
QA Contact: carosendahl → apis
Assignee | ||
Comment 48•17 years ago
|
||
Yes, I'd still like to have this available on a frozen interface, not just the docshell.
Comment 49•17 years ago
|
||
So what's holding this back?
Comment 50•8 years ago
|
||
Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.
Status: NEW → RESOLVED
Closed: 23 years ago → 8 years ago
Resolution: --- → INCOMPLETE
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•