94974 - Provide an interface for embeddors to access security info..

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[Chak Nanga]

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.

Comment 1

[Chak Nanga]

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

[Judson Valeski]

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?

Comment 3

[Chak Nanga]

> 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

[Terry Hayes]

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.

Comment 5

[Chak Nanga]

Attachment: Modify nsWebBrowser::GetInterface() to be able to get nsISSLStatus

Comment 6

[Chak Nanga]

Attachment: Patch to MfcEmbed to get and display SSL info such as cert details etc.

Comment 7

[Chak Nanga]

Attachment: Updated patch to MfcEmbed which includes the new SecurityInfoDlg.* files

Comment 8

[Chak Nanga]

Hi Terry : Could you please r= this when you get a chance...Thanks

Comment 9

[Terry Hayes]

r=thayes for the portions that use the ISSLStatus interface.

Comment 10

[Chak Nanga]

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

Comment 11

[rpotts (gone)]

Comment on attachment 47214 [details] [diff] [review]
Modify nsWebBrowser::GetInterface() to be able to get nsISSLStatus

r=rpotts.

looks good to me.

Comment 12

[Chak Nanga]

sr=rpotts [After getting Rick's OK this morning]

Comment 13

[Chak Nanga]

Fix checked in...

Comment 14

[hacker formerly known as [e-mail]]

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.

Comment 15

[Chak Nanga]

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

[Brendan Eich [:brendan]]

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

Comment 17

[Chak Nanga]

Got it. I'll work with cls to get this taken care of..Thanks Brendan

Comment 18

[Chak Nanga]

Change to nsWebBrowser being backed out by Waterson for the time being...

Comment 19

[Terry Hayes]

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

[hacker formerly known as [e-mail]]

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

[Brendan Eich [:brendan]]

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

[Brendan Eich [:brendan]]

s/remove pref providers/remote pref providers/

/be

Comment 23

[Alec Flett]

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

[Terry Hayes]

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.

Comment 25

[Mike Shaver (:shaver emeritus)]

3: have the embedding code provide an nsIEmbeddingSecurityInfo interface, and
have the PSM code implement it.

I like 3 best of all.

Comment 26

[Chak Nanga]

->0.9.7

Comment 27

[Chak Nanga]

->1.0 since there's still some dependency issues with PSM

Comment 28

[Chak Nanga]

Setting to 1.1

Comment 29

[Worcester12345]

(In reply to comment #28)
> Setting to 1.1

1.1 has come and gone. Should this just be marked WONTFIX then?

Comment 30

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: allow GI to get nsISecureBrowserUI from nsWebBrowser

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_.

Comment 31

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch for GI from nsWebBrowser to nsISecureBrowserUI

Updated patch with comments from caillon. Asking for r/sr now.

Comment 32

[Darin Fisher]

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

[timeless]

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

[Crispin Flowerday (not receiving bug mail)]

>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

[Darin Fisher]

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

[Daniel Veditz [:dveditz]]

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.

Comment 37

[Darin Fisher]

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

[Doug Turner (:dougt)]

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

[Darin Fisher]

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

[Darin Fisher]

really, no response?  anyone still care about this patch?

Comment 41

[Christian Persch (GNOME) (away; not receiving bug mail)]

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

[John G. Myers]

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

[Darin Fisher]

Any updates?  Gecko 1.8 is closing in fast.

Comment 44

[Christian Persch (GNOME) (away; not receiving bug mail)]

(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).

Comment 45

[Christian Persch (GNOME) (away; not receiving bug mail)]

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.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Especially since nsIDocShell is in bad need of a housecleaning...

Comment 47

[Reed Loden [:reed]]

chpe, is this bug still valid?

Comment 48

[Christian Persch (GNOME) (away; not receiving bug mail)]

Yes, I'd still like to have this available on a frozen interface, not just the docshell.

Comment 49

[Boris Zbarsky [:bzbarsky]]

So what's holding this back?

Comment 50

[Benjamin Smedberg]

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.