GNOME Bugzilla – Bug 119090
Certificate manager implementation
Last modified: 2008-04-01 21:46:36 UTC
Currently this ui only is able to show the list of installed certificates, but I want some feedback about the design of this solution. you can call the certificate manager from the edit menu. I defined two new gtype interfaces EphyEmbedCertsManager and EphyEmbedX509cert on embed directory. Implemented those interfaces on embed/mozilla directory and added a new method to the interface EphyEmbedSingle called get_certs_manager(...) The certificate manager dialog looks like the mozilla xul certificate manager with a better layout (I think) without the "Other's People" notebook page (Epiphany is not an email client ;-) )
Created attachment 18899 [details] [review] patch for epiphany 0.8.1
Dont laugh at me but ... could you provide a small explanation of what a cert manager is and for what it's used ? The code approach looks fine, but I guess we want to deal with interface design first.
jajaja. I'm laughing at me because I didn't explain what a certManager is ;-) The reponsabilities of the certificate manager are: Retrieve the certificates from the mozilla psm by type (personal, server, CA) (this is implemented). Import / Export certificates (not yet implemented) Remove certificates (not yet implemented)
Typical questions: 1. What is the use case? 2. Could something like this be done using the extensions mechanism? ps. I do think we need to provide a way for extensions to add menu items and perhaps even add a tools menu for this (though it wouldn't be there if no tools are installed).
I'm not sure if this enhancement must be an extension/plugin. I think that SSL certificate management is an integral part of any modern browser. There is already a bug related to the need to create a certificate viewer (Bug #115948), and if the certificate managment is implemented as a plugin, the bug #115948 will reimplement many of the code already created (when finished) for this bug
Can you elaborate on the use case, giving example maybe ? (Note that we arent saying it's not useful, but just we dont know what its use it)
bordoley@msu.edu: > 1. What is the use case? I started to work in this patch becuase i needed to use an SSL certificate that my bank gave to me. So I exported it from mozilla and found no way to import it to epiphany. Maybe in adition to a certificate manager like Mozilla we can make epiphany import the certificate executing the action File->Open (this is just an idea ;-) )
Dave, this looks like necessary evil to me. We cant stop our user to access their bank services or require them to add an extension for it.
Marco: Its fine with me if its necessary. I like you just had no use why you would need this :P. Maybe this should be integrated into the PDM?
Reasons to integrate with PDM: - Less dialogs any other? Reasons to not integrate with PDM: - a user trying get more privacy will erase the data stored (cookies, passwords) can remove the certificates (CA if no personal certificates are installed) assuming that it is data collected by the browser - Basic options in the PDM dialog I'm open to make the changes to integrate it to PDM if people agrees :-)
I'd say to go ahead with separate ui for now. If the code is good enough I guess we can easily integrate later if necessary. I'll try to review the patch more closely in the weekend, I'm very busy with 1.0 bugs :/ Can you update it to cvs btw, I think chpe said it was not applying.
Created attachment 19107 [details] [review] updated patch against CVS 20030811
The updated patch now has the ability to remove certificates, I have not tested it with imported certificates because i have yet to implement the import code. The CA certificates provided by Mozilla apparently can only be removed by root (not a bad idea) but when a non root user removes it mozilla PSM does not signal an error, but neither removes it (The same behaviour happens when using plain mozilla)
On the certificate manager ui (certs-manager.glade), can be seen that each notebook page has an import button, but the import operation is not related to any of the pages, it is a global action of the dialog; my question is: If I place that button as a dialog button (like "Close" and "Help") will this change be compliant with the GNOME HIG?
Some minor stylistic comments: - Funcs should be func () not func(). The same for things like EPHY_EMBED_CERTS_MANAGER(mes->priv->certs_manager); - Please do not put definitions at the top for all funcs but order them so that it's not necessary (we make an exception for init/finalize usually) - Somewhere aligmenents seem screwed looking at the patch, but maybe that's just the patch - ephy_embed_certs_manager_get_authorities_cert_list. I wonder if we could think to shorter names here, no good ideas atm though. + N_("Manager Certificates"), Manage ? G_CONST_RETURN gchararray (*get_title) (EphyEmbedX509cert *cert); We should ever use gresult in embed stuff as return value. + if (NS_FAILED(rv)) + return G_FAILED; These should go on the same line + // WORKAROUND the implementation of the certDB XPCOM component, each string + // returned by FindCertNicknames is a compound string with the following /* */ for commments. I'd add a reference to the bug there too. + /* This constructor is only intended to be used from C++ code */ + MozillaEmbedX509cert * + mozilla_embed_x509cert_new (nsCOMPtr<nsIX509Cert> &moz_cert) + { + MozillaEmbedX509cert *cert; + + cert = MOZILLA_EMBED_X509CERT (g_object_new (MOZILLA_EMBED_X509CERT_TYPE, NULL)); + nsAutoString nickname_string; + moz_cert->GetNickname(nickname_string); + cert->priv->nickname = ToNewUTF8String(nickname_string); + cert->priv->moz_cert = moz_cert; + + return cert; + } Priv stuff should not be set by _new functions. I'd initialize nickname from _init and add a gobject property for moz_cert.
Dave, it would be cool if you could have a look to the glade file and comment about it (and about the HIG problem). Chpe, I'd appreciate if you could do a review too.
Only some nite not already mentioned in Marco's review, look good overall: The certs manager is a modal dialogue, daveb will kill you for that ;) -- no need to be even window-modal, just make it a singleton, like bm and history windows. (?) --- epiphany/embed/ephy-embed-certs-manager.h 2003-08-11 08:23:38.000000000 -0400 + #define IS_EPHY_EMBED_CERTS_MANAGER(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), EPHY_EMBED_CERTS_MANAGER_TYPE)) + #define IS_EPHY_EMBED_CERTS_MANAGER_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), EPHY_EMBED_CERTS_MANAGER_TYPE)) (and in some other places): I know there are still some files which do it like this, but for new code use EPHY_IS_... instead of IS_EPHY_... --- epiphany/embed/mozilla/mozilla-embed-certs-manager.cpp 2003-08-11 08:23:38.000000000 -0400 + list = g_slist_append(list, cert); Use g_slist_prepend, and if the list order _really_ matters, g_slist_reverse() it later: + *l = g_slist_reverse (list); --- epiphany/embed/mozilla/mozilla-embed-x509cert.cpp 2003-08-11 08:23:38.000000000 -0400 + gchararray nickname; (and everywhere else too): Use char* like we do everywhere.
can you provide a copy of the glade file so that i don't have to apply the patch to my tree and i can review the ui. And yes modal anything is banned. :)
Also I'm a little uncomfortable putting this in the edit menu. I think it mighte be worth it to bite the bullet and add a tools menu which could include this and the pdm. Also sort of unrelated, can plugins add menu items?
Pleeease no, Tools menu sucks badly and we have already too many toplevel menus (hard to scan). Personally I dont any problem with: -------- Certificates Personal Data --------- Toolbars Preferences Or maybe it should be available in prefs ? (plugins can merge menu in theory, they need a patch atm)
I will work on updating the code to the conventions on a few days. About the modal dialog thing, i knew that looks ugly, that is is the reason why i placed this comment in the code ( ;-) ) + /* WORKAROUND the dialog is made modal until is implemented + a way to move to front the current open certificate manager */ + ephy_dialog_set_modal(EPHY_DIALOG(dialog), TRUE);
Re: Tools menu ------------- I see no real problem with the tools menu (especially since this seems to be a tool?) I'd rather have more toplevel that are clear with fewer items, rather than just throwing stuff into existing menus for the sake of fewer toplevels. But thats just my opinion, maybe i should shoot off a quick email to calum and seth about this to get their input (Actually if someone else could do this that would be nice, i'm really busy with moving and stuff for the next three weeks.) re: Modal -------------- make it transient to the parent window??
>I see no real problem with the tools menu (especially since this >seems to be a tool?) It's really hard to say what is a tool and what isnt. Bookmarks, history, preferences, toolbar editor are all tools from my point of view (confirmed by the position of prefs in IE and firebird). I think the term is too generic to be a useful categorization. >I'd rather have more toplevel that are clear >with fewer items, rather than just throwing stuff into existing menus >for the sake of fewer toplevels. I just dont see the need of a new toplevel considering that it would have two items and that Edit menu would have only 11 items, less then the View menu (the HIG allow a max of 15). I think it would just make scanning the toplevel menus less efficient without any real simplification gain in the drop down. >But thats just my opinion, maybe i >should shoot off a quick email to calum and seth about this to get >their input (Actually if someone else could do this that would be >nice, i'm really busy with moving and stuff for the next three weeks.) Well it's not urgent ... all post 1.0.
Created attachment 19303 [details] [review] Updated patch based on the recomendations, (not yet corrected the x509cert constructor problem)
Created attachment 19304 [details] Glade file
Sorry for the spam. Reassigning bugs with a target to our next milestone.
Hi Robert, what's the status on this? Marco, I think we can take this now and improve it from there, what do you think? Also, just FYI so there's no needless duplicate work, crispin is working on something which overlaps a little, the cert dialogues [http://patches.theflowerdays.com/cert/].
I'm migrating the implementation to the CVS HEAD, this needed that i updated the required libraries (GTK, libxml, etc..). Also I'm learning how to implement a new XPCOM Service. This new class is needed because Mozilla NSS prompts for the required password used to locally protect the certifates imported. I think than I will have a new patch ready at the end of this weekend
I think I already have the code for the dialog boxes. Assuming you are talking about the nsICertificateDialogs service. I am porting that to Gtk, the only one I have left to do is the ViewCert dialog. I can make that available if you want to look at it. I hope to get the ViewCert ported this weekend, so for the moment you may want to just call the approriate mozilla functions, and when I have these dialogs finished they should be automatically used.
Yes... this is the XPCOM interface, are you implementing dialog for SetPKCS12FilePassword and GetPKCS12FilePassword methods? There is another dialog that mozilla shows the first time that a certificate is imported (Master Password, see Mozilla preferences->Privacy & Security->Master Password->Change Password) and i think that i need to implement it. My current code crash when importing certificates, i think that the mozilla default dialog for this password is causing it
Indeed, it is those that I am implementing: http://patches.theflowerdays.com/cert/ Your mozilla is crashing probably because of the bug in mozilla 1.4 (the GetGtkDomWindow() crash or whatever it is called). IIRC the master password ones use the standard PromptService api which is already gtk'ified.
This dialog that you talk about is the standard window.alert() one. This dialog is called when the user needs to enter the password in order to have access to a installed certificate when a website request it. The dialog than I'm talking about is used to set the master password for the first time (it request the password two times and verifies that both values are equals, and shows a progress bar with an indication about the quality of the password) when no certificate has been imported
For interested people, my patch for the various alerts is at: http://patches.theflowerdays.com/diff.cgi/galeon_cert_dialogs.patch This is very much work in progress, but at least you will be able to see the dialogs in action :) (the patch is against galeon, but the code for epiphany is identical).
Created attachment 20186 [details] [review] Upgraded patch to CVS HEAD 1.1
The updated patch is able to import PKCS12 packaged certificates, (is required to reopen the dialog to see them, for now) an small line is disabled on ephy-window.c - g_object_unref (window->ui_merge); + // g_object_unref (window->ui_merge); This unref is making epiphany crash when any window or tab is closed (This a ugly hack in order to be able to test my code ;-) because the xul master password dialog make epiphany crash) REQUIREMENTS: Mozilla 1.5b or 1.4 branch with patch http://bugzilla.mozilla.org/show_bug.cgi?id=211834 TODO: Implement a call to the View certificate dialog Implement Properties dialog. Any GNOME HIG change required ;-) NLS
I finally looked to the glade file. What's the difference between import/export, they apply one globally and one per tab ? Dave. They are 3 tabs I guess we cant integrate it with the pdm. Mozilla has it in the prefs dialog but I'm not sure if that make sense (also why the pdm would be in the menu and this in prefs ...). Do you think it should be a menu ? I see a bit of a conflict with current naming of Pdm, isnt this personal data too ? Basically I think the whole thing will be a pain :/ What about if we write a short document about the usage and the way this stuff works, it helped in the prefs case. I'm not even sure to understand it fully myself :/ Then maybe we can get Seth to look at it.
Robert, mozilla doesnt appear to have an export button at all, has it been introduced recently ?
Yes, it has one, it is called "backup", I used the "export" name in order to identify to diferent oposite Import and Export, Import and Backup dosn't make sene to me. > I finally looked to the glade file. What's the difference between > import/export, they apply one globally and one per tab ? Mozilla Certificate Database XPCOM component has thow import options: import certifcates based on their type (the tab selected), and it can import any certificate from an PKCS12 file without testing their types, this latest is the method that i'm using (at least based on my experience, i never needed to import select which certificate type i need to import, I use the export/backup option of the source software to do that, for example Mozilla Seamonkey Backup certificates button). The export button will export the selected certificates from the current selected tab, I think that it is not easy to understand that something that is not being shown (certificates selected in another tab) is being exported too. Maybe instead of using three export buttons, one per tab, i could use only one like import, but the placement of the import/export buttons may change it this code is intergrated with the PDM dialog
o my god, i need to check the spelling before posting messages :$. I correct the first paragraph: Yes, it has one, it is called "backup", I used the "export" name in order to identify two oposite actions: Import and Export, Import and Backup doesn't make sence to me.
Created attachment 20406 [details] Dialog screeshot
Created attachment 20968 [details] Proposed PDM integrated UI
Created attachment 20969 [details] Proposed PDM integrated UI (manage CA button)
Created attachment 20970 [details] Certificates used for testing (all passwords for the files are 'test')
I like the idea of putting CA on a separate level, that makes the thing a bit more clear IHMO. Though I'm not very convinced about grouping the two types of certs in a tab, with an optionmenu to choose them. Even if I'm convinced we need to put this in epiphany core at some point, there are so many problems to deal with ... We have identified many of them, which is an important step, but it's hard to find good solutions. Maybe it make sense to start implementing it like a plugin, let it mature and then gradually integrate it in epiphany core. I think we need to start experimenting with it now that we have identified user tasks and problems in mozilla design. I'm not convinced we will get it perfectly right the first time ... Robert/Dave/spark/chpe, what do you think ? Does a plan like this make sense ? Alternative proposals ?
Xan should be involved in this decision too.
I'm working on a document about a proposal to separate the certificate database management from epiphany to an external module, that can be shared by any gnome application The basic idea is to implement a GObjects based API to this certificate database and implement a new PKCS#11 module for epiphany/mozilla See http://developer.netscape.com/docs/manuals/security/pkcs/index.htm ftp://ftp.rsa.com/pub/pkcs/pkcs-11/pkcs11v2.doc Implementing it as a plugin is not a bad idea either
That sounds like a cool idea. I think I've seen secure certificates ui for at least evolution and mono. It would make a lot of sense to share this at desktop level, one more point we gain against mozilla :)
We should start a discussion about that GNOME-wide cert infrastructure on a ML [d-d-l ?] soon, before those other apps have invested too much in their own solution.
I don't use certs, nor does anyone i know, so i guess i don't really have an opinion
I think giving this a first try as a separated entity is a great idea too. If we finally merge with ephy core if will serve as a good testbed, and if it finally makes sense to share with the desktop we'll be on the right track already.
Now that we've decided on our strategy, I've decided to review the patch a little more thouroughly, but only the backend side of thing [i.e. anything that goes in embed/] because this needs to be in epiphany proper even when the manager UI will be an extension. From attachment id=20186: +++ epiphany-new/embed/ephy-embed-certs-manager.c 2003-09-21 14:20:39.000000000 -0400 I think this should be renamed. ephy-certificate-manager.[ch], EphyCertificateManager, EPHY_CERTIFICATE_MANAGER, ephy_certificate_manager etc; same for mozilla side of things. +struct EphyEmbedCertsManagerPrivate +{ + gpointer dummy; +}; Unused, remove. +gresult +ephy_embed_certs_manager_get_certificates (EphyEmbedCertsManager *manager, + GSList **l, + EphyEmbedX509certType type) I've recently gotten rid of gresult; instead of out-parameters we can now use normal return values, i.e GList * ephy_certificate_manager_get_certificates (EphyCertificateManager *manager, EphyX509CertificateType type); and same everywhere else. If you want to indicate failure and no obvious return value can represent them (e.g. NULL), use gboolean return, or an GError out-param. +gresult +ephy_embed_single_get_certs_manager (EphyEmbedSingle *shell, ..._get_certificate_manager +++ epiphany-new/embed/ephy-embed-x509cert.c 2003-09-21 23:04:52.000000000 -0400 Rename ephy-x509-certificate.c, EphyX509Certificate, EPHY_X509_CERTIFICATE, ephy_x509_certificate etc, same for mozilla side of things. +struct EphyEmbedX509certPrivate +{ + gpointer dummy; +}; Unused, remove. +typedef enum +{ + PERSONAL_CERTIFICATE, + SERVER_CERTIFICATE, + CA_CERTIFICATE +} EphyEmbedX509certType; Maybe EPHY_CERTIFICATE_PERSONAL etc. And EphyX509CertificateType. +struct MozillaEmbedCertsManagerPrivate +{ +}; Unused, remove; or do you plan to add sth there? +static void +mozilla_embed_certs_manager_init (MozillaEmbedCertsManager *manager) +{ + manager->priv = g_new0 (MozillaEmbedCertsManagerPrivate, 1); +} If you decided to keep ...Private, use GObject instance-private data here; see any ephy object impl for how it's done. + + g_return_if_fail (object != NULL); + g_return_if_fail (MOZILLA_IS_EMBED_CERTS_MANAGER (object)); + + manager = MOZILLA_EMBED_CERTS_MANAGER (object); + + g_return_if_fail (manager->priv != NULL); Unnecessary, gobject type system guarantees _finalize is only called for objects of the correct type. + g_free (manager->priv); This is unnecessary both if you removed Private and if you kept Private with gobject instance-private data. +enum +{ + PROP_MOZILLA_CERT = 1, + PROP_END +}; Normally we use enum { PROP_0, PROP_MOZILLA_CERT }; etc. +nsCOMPtr<nsIX509Cert> +mozilla_embed_x509cert_get_moz_cert (MozillaEmbedX509cert *cert) +{ + return cert->priv->mozilla_cert; +} I'm not an XPCOM expert, but the XPCOM tutorial recommends against this, I think it will leak. Should probably be already_AddRefed<nsIX509Cert> mozilla_x506_certificate_get_moz_cert (... --- What's left to do: I think there need to be functions to access the GtkNSSDialogs, for example ephy_x509_certificate_view (....) which would show the cert viewer for this certificate.
I have taken notes of the required changes, I am resuming my work on this bug now (i had much work these days not near my computer , I need a laptop '-( ) If the idea to create a PKCS#11 module for mozilla to connect it to an external certificate database works as I think (I hope), no code changes will be needed to epiphany apart from loading the PKCS#11 module, so all this code (if used) will be outside epiphany
Hi Robert, have you been able to work on the patch since? Will it be ready in time for feature freeze on Jan. 12 ? Thanks
Sure, I am working on it but currently I don't have much free time util this friday 19, I hope to have an usable version around 01/12/2004 (I'm returning to work the 01/15/2004)
Mass reassigning of Epiphany bugs to epiphany-maint@b.g.o
Created attachment 54044 [details] [review] Updated patch for CVS - 2005-10-29 This is an updated patch to work with CVS head as of today, I'm made some changes to make the certificate manager an interface on the embed-single object, partially updated to use embed strings, and converted to use file chooser.
Created attachment 54048 [details] [review] Another updated patch for CVS - 2005-10-29 The backend is now a lot cleaner, and I believe it is free of memory leaks, and reference counting is more intuitive. Still to do: - Resplit out the cert manager iface from the embed-single, I now don't like this approach as the iface will grow - Design a user interface The second point is the real blocker, as that will impact what functions need adding to the interface, and where the frontend code lives.
I committed this to cvs disabled by default (enable with --enable-certificate-manager configure switch). Thanks to Robert and Crispin for working on this!