GNOME Bugzilla – Bug 626848
Allow interactive TLS certificate verification
Last modified: 2010-08-24 13:28:00 UTC
Hi, as the new Channel.Type.ServerTLSConnection and Authentication.TLSCertificate objects are now merged as drafts in telepathy-spec, I implemented the new interactive TLS verification in Empathy. The branch is here [1], I will attach a squashed patch too. Note that to make this work, this gabble branch has to be merged too [2]. Review welcome. [1] http://git.collabora.co.uk/?p=user/cosimoc/empathy.git;a=shortlog;h=refs/heads/tls-connection [2] https://bugs.freedesktop.org/show_bug.cgi?id=29458
Created attachment 167812 [details] [review] squashed diff configure.ac | 4 +- data/Empathy.Auth.client | 6 + data/Makefile.am | 9 +- ...esktop.Telepathy.Client.Empathy.Auth.service.in | 3 + extensions/Authentication_TLS_Certificate.xml | 304 +++++++++ extensions/Channel_Type_Server_TLS_Connection.xml | 69 ++ extensions/misc.xml | 2 + libempathy-gtk/Makefile.am | 4 + libempathy-gtk/empathy-tls-dialog.c | 315 +++++++++ libempathy-gtk/empathy-tls-dialog.h | 68 ++ libempathy-gtk/gcr-simple-certificate.c | 131 ++++ libempathy-gtk/gcr-simple-certificate.h | 58 ++ libempathy/Makefile.am | 8 + libempathy/empathy-auth-factory.c | 209 ++++++ libempathy/empathy-auth-factory.h | 66 ++ libempathy/empathy-debug.c | 1 + libempathy/empathy-debug.h | 1 + libempathy/empathy-server-tls-handler.c | 298 ++++++++ libempathy/empathy-server-tls-handler.h | 73 ++ libempathy/empathy-tls-certificate.c | 557 +++++++++++++++ libempathy/empathy-tls-certificate.h | 78 +++ libempathy/empathy-tls-verifier.c | 708 ++++++++++++++++++++ libempathy/empathy-tls-verifier.h | 78 +++ src/Makefile.am | 6 +- src/empathy-auth-helper.c | 195 ++++++ 25 files changed, 3247 insertions(+), 4 deletions(-)
Review of attachment 167812 [details] [review]: I tried building using gnutls 2.6.5 (in my jhbuild) and got this error: empathy-tls-verifier.c: In function ‘verification_output_to_reason’: empathy-tls-verifier.c:118: error: ‘GNUTLS_CERT_NOT_ACTIVATED’ undeclared (first use in this function) empathy-tls-verifier.c:118: error: (Each undeclared identifier is reported only once empathy-tls-verifier.c:118: error: for each function it appears in.) empathy-tls-verifier.c:120: error: ‘GNUTLS_CERT_EXPIRED’ undeclared (first use in this function) Building with gnutls 2.8.5 (from my system) worked fine. So we should depend on the minimum version required. We should then ask to GNOME to bump the external dep but that shouldn't be an issue. Be sure that "make check" and "make distcheck" pass. Did you try to valgrind the auth client? ::: configure.ac @@ +155,3 @@ gio-unix-2.0 >= $GLIB_REQUIRED gnome-keyring-1 >= $KEYRING_REQUIRED + gnutls Set the minimal version. ::: libempathy-gtk/empathy-tls-dialog.c @@ +128,3 @@ + "verified.\n")); + + switch (reason) Some of those are already in empathy_status_reason_get_default_message(). Some kind of sharing would be good if possible. @@ +236,3 @@ + * easily store the new CA cert in $XDG_CONFIG_DIR/telepathy/certs in that + * case. For the other errors, we probably need a smarter/more powerful + * certificate storage. Open a bug please. ::: libempathy-gtk/gcr-simple-certificate.c @@ +17,3 @@ + * License along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA + * 02111-1307, USA. If this file is an unchanged copy from gnome-keyring we should make it clear in the copyright header. ::: libempathy/empathy-auth-factory.c @@ +63,3 @@ + DEBUG ("Failed to create a server TLS handler; error %s", + error->message); + g_error_free (error); We should use tp_handle_channels_context_fail() in this case as we are not handling the channel. @@ +91,3 @@ + * at the same time, for the same connection/account. + */ + g_assert (g_list_length (channels) == 1); This makes you remotely crashable. Use tp_handle_channels_context_fail(). @@ +107,3 @@ + + out: + tp_handle_channels_context_accept (context); I think we should wait that the EmpathyServerTLSHandler has been created before calling _accept. @@ +169,3 @@ + + if (priv->handler != NULL) + g_object_unref (priv->handler); Should be in _dispose and use tp_clear_object() ::: libempathy/empathy-server-tls-handler.c @@ +147,3 @@ + + priv->async_init_res = g_simple_async_result_new (G_OBJECT (self), + callback, user_data, NULL); source_tag ? @@ +150,3 @@ + + /* call GetAll() on the channel properties */ + tp_cli_dbus_properties_call_get_all (priv->channel, Can't you use tp_channel_borrow_immutable_properties() ? ::: libempathy/empathy-tls-certificate.c @@ +67,3 @@ +G_DEFINE_TYPE_WITH_CODE (EmpathyTLSCertificate, empathy_tls_certificate, + G_TYPE_OBJECT, + G_IMPLEMENT_INTERFACE (G_TYPE_ASYNC_INITABLE, async_initable_iface_init)); Did you check with smcv if this is the right design? This object should move to tp-glib once the spec is stable so we'd better get it right. Shouldn't we inherit from TpProxy and use tp_proxy_prepare_async() instead? @@ +83,3 @@ + retval = FALSE; + + return retval; This line contain trailing spaces. "make check" should detect those. @@ +115,3 @@ + { + g_simple_async_result_set_from_error (priv->async_init_res, error); + g_simple_async_result_complete_in_idle (priv->async_init_res); I *think* the pattern is to use _complete (not in idle) when completing from a callback. @@ +154,3 @@ + + priv->async_init_res = g_simple_async_result_new (G_OBJECT (self), + callback, user_data, NULL); no source_tag? @@ +375,3 @@ + return retval; +} + Please document public API. This will make things easier when we'll move this object to tp-glib. @@ +409,3 @@ + +void +empathy_tls_certificate_accept (EmpathyTLSCertificate *self) Shouldn't be this function async to let user known about success/failure of the operation? @@ +422,3 @@ + +void +empathy_tls_certificate_reject (EmpathyTLSCertificate *self, async? ::: libempathy/empathy-tls-verifier.c @@ +80,3 @@ +verification_output_to_reason (gint res, + guint verify_output, + EmpTLSCertificateRejectReason *reason) if you don't check reason != NULL before using it then add an assertion @@ +92,3 @@ + { + case GNUTLS_E_INSUFFICIENT_CREDENTIALS: + *reason = EMP_TLS_CERTIFICATE_REJECT_REASON_UNTRUSTED; Aren't these errors in tp-glib now? @@ +224,3 @@ + gint res = 0; + + /* this is taken from GnuTLS */ The copyright header should reflect this. @@ +268,3 @@ + certified_hostname, priv->hostname); + + /* TODO: pass-through the expected hostname in the reject details */ fix it or open a bug. @@ +534,3 @@ + g_free (user_certs_dir); + + /* TODO: do the CRL too */ Do it or file a bug about it. @@ +609,3 @@ + + if (priv->trusted_ca_list != NULL) + g_ptr_array_unref (priv->trusted_ca_list); You can use tp_clear_pointer() @@ +684,3 @@ + EmpathyTLSVerifierPriv *priv = GET_PRIV (self); + + priv->verify_result = g_simple_async_result_new (G_OBJECT (self), return_if_fail if verifly_result != NULL @@ +694,3 @@ +empathy_tls_verifier_verify_finish (EmpathyTLSVerifier *self, + GAsyncResult *res, + EmpTLSCertificateRejectReason *reason, check if reason != NULL before using it. ::: src/empathy-auth-helper.c @@ +167,3 @@ + empathy_gtk_init (); + gnutls_global_init (); + g_set_application_name (_("Empathy authentication helper")); Why is this app called an helper? It's more a client. @@ +188,3 @@ + DEBUG ("Empathy auth client started."); + + gtk_main (); This client should stop itself after a while if it's done handling things except if EMPATHY_PERSIST as been defined. See empathy-av.c
(In reply to comment #2) Guillame, thanks for the review! > Be sure that "make check" and "make distcheck" pass. Yeah, I fixed this in my branch (709119042e1c45e0aafb38659ac6ea7396c8aa0c, 460043daa52d76ffdc90b2f9cf6432b12c850a0f and 37d64d2ec686d48a88db3f94b92e88df213b11b7) > Did you try to valgrind the auth client? Not yet. Will do. > ::: configure.ac > @@ +155,3 @@ > gio-unix-2.0 >= $GLIB_REQUIRED > gnome-keyring-1 >= $KEYRING_REQUIRED > + gnutls > > Set the minimal version. Done (1f436c6ef319671d80b1c35bd498dd4893c6919b) > ::: libempathy-gtk/empathy-tls-dialog.c > @@ +128,3 @@ > + "verified.\n")); > + > + switch (reason) > > Some of those are already in empathy_status_reason_get_default_message(). Some > kind of sharing would be good if possible. Hmm, those are different, as here in the dialog we're poking at this [1] kind of errors, not the connection errors, so I think it's not something we can do. > @@ +236,3 @@ > + * easily store the new CA cert in $XDG_CONFIG_DIR/telepathy/certs in that > + * case. For the other errors, we probably need a smarter/more powerful > + * certificate storage. > > Open a bug please. Ok. Can I do it after merging? > ::: libempathy-gtk/gcr-simple-certificate.c > @@ +17,3 @@ > + * License along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA > + * 02111-1307, USA. > > If this file is an unchanged copy from gnome-keyring we should make it clear in > the copyright header. Done in 1fba1db04fde57223ace7332111d146a124a643a > ::: libempathy/empathy-auth-factory.c > @@ +63,3 @@ > + DEBUG ("Failed to create a server TLS handler; error %s", > + error->message); > + g_error_free (error); > > We should use tp_handle_channels_context_fail() in this case as we are not > handling the channel. > @@ +107,3 @@ > + > + out: > + tp_handle_channels_context_accept (context); > > I think we should wait that the EmpathyServerTLSHandler has been created before > calling _accept. > @@ +169,3 @@ > + > + if (priv->handler != NULL) > + g_object_unref (priv->handler); > > Should be in _dispose and use tp_clear_object() All fixed in d9428d22891d01b25b08f23278317daa78871274 > ::: libempathy/empathy-server-tls-handler.c > @@ +147,3 @@ > + > + priv->async_init_res = g_simple_async_result_new (G_OBJECT (self), > + callback, user_data, NULL); > > source_tag ? > > @@ +150,3 @@ > + > + /* call GetAll() on the channel properties */ > + tp_cli_dbus_properties_call_get_all (priv->channel, > > Can't you use tp_channel_borrow_immutable_properties() ? All fixed in 92c282488352fd9067fdfbd1d9e96b668a2ff19c > ::: libempathy/empathy-tls-certificate.c > @@ +67,3 @@ > +G_DEFINE_TYPE_WITH_CODE (EmpathyTLSCertificate, empathy_tls_certificate, > + G_TYPE_OBJECT, > + G_IMPLEMENT_INTERFACE (G_TYPE_ASYNC_INITABLE, async_initable_iface_init)); > > Did you check with smcv if this is the right design? This object should move to > tp-glib once the spec is stable so we'd better get it right. Shouldn't we > inherit from TpProxy and use tp_proxy_prepare_async() instead? As discussed on IRC, TpProxy subclassing doesn't seem to be ready for this kind of usage yet, and smcv wasn't around today. Will ask him when he's back. > @@ +115,3 @@ > + { > + g_simple_async_result_set_from_error (priv->async_init_res, error); > + g_simple_async_result_complete_in_idle (priv->async_init_res); > > I *think* the pattern is to use _complete (not in idle) when completing from a > callback. Shouldn't really matter, anyway I fixed this in 4472cbd53db695b41146ec4c9345ee4ab3197b04 > @@ +154,3 @@ > + > + priv->async_init_res = g_simple_async_result_new (G_OBJECT (self), > + callback, user_data, NULL); > > no source_tag? Fixed in d20fcc3aedd0b76b3ea0307bb69324121f49b555 > @@ +375,3 @@ > + return retval; > +} > + > > Please document public API. This will make things easier when we'll move this > object to tp-glib. Will do once I talk to smcv about the design of this object, so that I don't do useless work. > @@ +409,3 @@ > + > +void > +empathy_tls_certificate_accept (EmpathyTLSCertificate *self) > > Shouldn't be this function async to let user known about success/failure of the > operation? > > @@ +422,3 @@ > + > +void > +empathy_tls_certificate_reject (EmpathyTLSCertificate *self, > > async? Fixed in d20fcc3aedd0b76b3ea0307bb69324121f49b555 > ::: libempathy/empathy-tls-verifier.c > @@ +80,3 @@ > +verification_output_to_reason (gint res, > + guint verify_output, > + EmpTLSCertificateRejectReason *reason) > > if you don't check reason != NULL before using it then add an assertion Fixed in f735801461c5748ee212eaba80c894f33ecf3f10 > @@ +92,3 @@ > + { > + case GNUTLS_E_INSUFFICIENT_CREDENTIALS: > + *reason = EMP_TLS_CERTIFICATE_REJECT_REASON_UNTRUSTED; > > Aren't these errors in tp-glib now? No, these are reject reasons for the TLS certificate, not TpConnection errors. > @@ +224,3 @@ > + gint res = 0; > + > + /* this is taken from GnuTLS */ > > The copyright header should reflect this. Done in 0a173bc247669513992889723e93eb2de4cc0de9 > @@ +268,3 @@ > + certified_hostname, priv->hostname); > + > + /* TODO: pass-through the expected hostname in the reject details */ > > fix it or open a bug. I fixed this in f0793af4b460ffcde3df89f77590f6130c2b8b9e and the two following commits. > @@ +609,3 @@ > + > + if (priv->trusted_ca_list != NULL) > + g_ptr_array_unref (priv->trusted_ca_list); > > You can use tp_clear_pointer() Done in a272c04575698908bff1363f3d652cc67876e599 > @@ +684,3 @@ > + EmpathyTLSVerifierPriv *priv = GET_PRIV (self); > + > + priv->verify_result = g_simple_async_result_new (G_OBJECT (self), > > return_if_fail if verifly_result != NULL Fixed in 9005c6a1357b612d9a3bce36dd6326da487ab728 > @@ +694,3 @@ > +empathy_tls_verifier_verify_finish (EmpathyTLSVerifier *self, > + GAsyncResult *res, > + EmpTLSCertificateRejectReason *reason, > > check if reason != NULL before using it. Done as part of f0793af4b460ffcde3df89f77590f6130c2b8b9e > ::: src/empathy-auth-helper.c > @@ +167,3 @@ > + empathy_gtk_init (); > + gnutls_global_init (); > + g_set_application_name (_("Empathy authentication helper")); > > Why is this app called an helper? It's more a client. I renamed it in 5286587e932047f9b32001429a395d8caf193b26 > @@ +188,3 @@ > + DEBUG ("Empathy auth client started."); > + > + gtk_main (); > > This client should stop itself after a while if it's done handling things > except if EMPATHY_PERSIST as been defined. See empathy-av.c Done in 526dbcddf5a3969a8714b49441e4dd62927837f3 I also added some other commits to avoid showing the error banner in the contact list if the user cancels the dialog, using the 'user-requested' property in the error details hash table. [1] file:///media/Source/telepathy-spec/doc/spec/Authentication_TLS_Certificate.html#TLS_Certificate_Reject_Reason
Created attachment 168215 [details] [review] squashed diff v2 configure.ac | 5 +- data/Empathy.Auth.client | 6 + data/Makefile.am | 9 +- ...esktop.Telepathy.Client.Empathy.Auth.service.in | 3 + extensions/Authentication_TLS_Certificate.xml | 304 ++++++++ extensions/Channel_Type_Server_TLS_Connection.xml | 69 ++ extensions/Makefile.am | 2 + extensions/misc.xml | 2 + libempathy-gtk/Makefile.am | 4 + libempathy-gtk/empathy-tls-dialog.c | 367 ++++++++++ libempathy-gtk/empathy-tls-dialog.h | 69 ++ libempathy-gtk/gcr-simple-certificate.c | 134 ++++ libempathy-gtk/gcr-simple-certificate.h | 61 ++ libempathy/Makefile.am | 8 + libempathy/empathy-auth-factory.c | 251 +++++++ libempathy/empathy-auth-factory.h | 66 ++ libempathy/empathy-debug.c | 1 + libempathy/empathy-debug.h | 1 + libempathy/empathy-server-tls-handler.c | 275 ++++++++ libempathy/empathy-server-tls-handler.h | 73 ++ libempathy/empathy-tls-certificate.c | 603 ++++++++++++++++ libempathy/empathy-tls-certificate.h | 89 +++ libempathy/empathy-tls-verifier.c | 737 ++++++++++++++++++++ libempathy/empathy-tls-verifier.h | 79 +++ libempathy/empathy-utils.c | 15 +- libempathy/empathy-utils.h | 3 +- po/POTFILES.in | 2 + src/Makefile.am | 6 +- src/empathy-accounts-dialog.c | 4 +- src/empathy-auth-client.c | 268 +++++++ src/empathy-main-window.c | 11 +- 31 files changed, 3517 insertions(+), 10 deletions(-)
(In reply to comment #3) > > ::: configure.ac > > @@ +155,3 @@ > > gio-unix-2.0 >= $GLIB_REQUIRED > > gnome-keyring-1 >= $KEYRING_REQUIRED > > + gnutls > > > > Set the minimal version. > > Done (1f436c6ef319671d80b1c35bd498dd4893c6919b) I'll ask to GNOME if bumping the external dep is OK as it currently depends on 2.4.2. > > @@ +236,3 @@ > > + * easily store the new CA cert in $XDG_CONFIG_DIR/telepathy/certs in that > > + * case. For the other errors, we probably need a smarter/more powerful > > + * certificate storage. > > > > Open a bug please. > > Ok. Can I do it after merging? sure. > > ::: libempathy/empathy-tls-certificate.c > > @@ +67,3 @@ > > +G_DEFINE_TYPE_WITH_CODE (EmpathyTLSCertificate, empathy_tls_certificate, > > + G_TYPE_OBJECT, > > + G_IMPLEMENT_INTERFACE (G_TYPE_ASYNC_INITABLE, async_initable_iface_init)); > > > > Did you check with smcv if this is the right design? This object should move to > > tp-glib once the spec is stable so we'd better get it right. Shouldn't we > > inherit from TpProxy and use tp_proxy_prepare_async() instead? > > As discussed on IRC, TpProxy subclassing doesn't seem to be ready for this kind > of usage yet, and smcv wasn't around today. Will ask him when he's back. Cool. Please do check. > > Please document public API. This will make things easier when we'll move this > > object to tp-glib. > > Will do once I talk to smcv about the design of this object, so that I don't do > useless work. cool. I think that's the last merge blockers. Except that it looks all good.
(In reply to comment #5) > (In reply to comment #3) > > > ::: configure.ac > > > @@ +155,3 @@ > > > gio-unix-2.0 >= $GLIB_REQUIRED > > > gnome-keyring-1 >= $KEYRING_REQUIRED > > > + gnutls > > > > > > Set the minimal version. > > > > Done (1f436c6ef319671d80b1c35bd498dd4893c6919b) > > I'll ask to GNOME if bumping the external dep is OK as it currently depends on > 2.4.2. That's ok :)
GNOME 2.32 now depends on gnutls 2.8.5 so that's fine.
Created attachment 168628 [details] [review] squashed diff v3 Updated the patchset after talking with Simon on IRC about TpProxy subclassing. We agreed it's best for EmpathyTLSCertificate to inherit from TpProxy, have a non-async constructor and an async _prepare() method, so I implemented this behaviour. configure.ac | 5 +- data/Empathy.Auth.client | 6 + data/Makefile.am | 9 +- ...esktop.Telepathy.Client.Empathy.Auth.service.in | 3 + extensions/Authentication_TLS_Certificate.xml | 304 ++++++++ extensions/Channel_Type_Server_TLS_Connection.xml | 69 ++ extensions/Makefile.am | 2 + extensions/misc.xml | 2 + libempathy-gtk/Makefile.am | 4 + libempathy-gtk/empathy-tls-dialog.c | 367 ++++++++++ libempathy-gtk/empathy-tls-dialog.h | 69 ++ libempathy-gtk/gcr-simple-certificate.c | 134 ++++ libempathy-gtk/gcr-simple-certificate.h | 61 ++ libempathy/Makefile.am | 8 + libempathy/empathy-auth-factory.c | 251 +++++++ libempathy/empathy-auth-factory.h | 66 ++ libempathy/empathy-debug.c | 1 + libempathy/empathy-debug.h | 1 + libempathy/empathy-server-tls-handler.c | 290 ++++++++ libempathy/empathy-server-tls-handler.h | 73 ++ libempathy/empathy-tls-certificate.c | 511 ++++++++++++++ libempathy/empathy-tls-certificate.h | 95 +++ libempathy/empathy-tls-verifier.c | 737 ++++++++++++++++++++ libempathy/empathy-tls-verifier.h | 79 +++ libempathy/empathy-utils.c | 15 +- libempathy/empathy-utils.h | 3 +- po/POTFILES.in | 2 + src/Makefile.am | 6 +- src/empathy-accounts-dialog.c | 4 +- src/empathy-auth-client.c | 268 +++++++ src/empathy-main-window.c | 11 +- 31 files changed, 3446 insertions(+), 10 deletions(-)
Review of attachment 168628 [details] [review]: ::: libempathy/empathy-tls-certificate.c @@ +117,3 @@ +{ + EmpathyTLSCertificatePriv *priv = GET_PRIV (self); + You should check if priv->async_prepare_res is NULL. If not you can raise a PENDING error (or maybe even assert in that case as it's not meant to be prepared twice).
I fixed this and merged my branch to master after Guillaume acked it on IRC. Closing as FIXED.