[Bug 29458] TLS-connection branch

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=29458

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Merge TLS-connection branch |TLS-connection branch
             Status|NEW                         |ASSIGNED
           Severity|normal                      |enhancement
         AssignedTo|telepathy-bugs at lists.freede |cosimoc at gnome.org
                   |sktop.org                   |

--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-08-09 09:50:42 PDT ---
In general I prefer the first review round for a branch to be a series of
coherent patches (i.e. I prefer it if people use git rebase --interactive to
squash trivial fixes into the patch that they fix, before the first review
round; ideally, make check should pass and the code should make sense after
each commit).

As a first review pass here I've mostly gone through the whole diff without
looking at individual patches, so you're free to rebase where appropriate.
There are changes in this branch that shouldn't be there, so please do rebase
them out.

Irrelevant changes
==================

> --- a/src/ft-channel.h
> +++ b/src/ft-channel.h
> ...
> -#include <extensions/_gen/svc.h>
> -#include <extensions/_gen/interfaces.h>
> -#include <extensions/_gen/enums.h>
> +#include <extensions/extensions.h>

I'm sure this change is good and all, but it's not part of your feature. This
sort of thing could usefully have been fast-tracked in as part of a small
bugfix branch (Telepathy people tend to call that branch "trivia", which is
probably my fault).

> -  g_assert ((priv->state == JS_STATE_PENDING_CREATED) ||
> -      (priv->state == JS_STATE_ENDED));
> +  g_assert ((priv->state == (JingleState) JS_STATE_PENDING_CREATED) ||
> +      (priv->state == (JingleState) JS_STATE_ENDED));

Likewise, and also, I don't like scattering casts through our code if we don't
have to; they clutter the code (hurting clarity), and can mask genuine bugs
that the compiler would have warned us about. I'll open a separate bug for the
gcc 4.5 stuff.

I'm going to assume that all changes in ft-channel.h, jingle-session.c,
muc-channel.c, media-factory.c, tubes-channel.c are irrelevant to this branch.

Architectural thoughts
======================

I'm unhappy about subclassing WockyTLSHandler in the Channel; it means you
can't subclass GabbleBaseChannel, so you need a lot of boilerplate. The
many-one and one-one relationships also don't seem right this way.

Is there a reason why the *channel manager* can't be the WockyTLSHandler and do
most of the work? If you did that, I think the architecture would make more
sense:

* it could avoid creating the Channel until verify_async() is called, meaning
the Channel could put itself on the bus, create its certificate, etc. in
constructed() rather than having a separate method to do that
* it could create a Channel every time verify_async() is called, if for some
insane reason Wocky calls it multiple times

The channel manager should manage the channel(s), rather than being a thin shim
around a channel that manages itself :-)

Minor details
=============

> @@ -1599,6 +1620,36 @@ connector_error_disconnect (GabbleConnection *self,
> +      channel = GABBLE_SERVER_TLS_CHANNEL
> +        (gabble_server_tls_manager_get_handler (self->priv->server_tls_manager));

Why does this need a cast?

Could the TLS manager do the heavy lifting here, to keep the messy stuff out of
GabbleConnection? Something like this, perhaps:

TpConnectionStatusReason reason;
GHashTable *details;

if (gabble_server_tls_manager_get_rejection (self->priv->server_tls_manager,
        &dbus_error, &details, &reason))
  {
    tp_base_connection_disconnect_with_dbus_error (base,
      dbus_error, details, reason);
    g_free (dbus_error);
    g_hash_table_unref (details);
    return;
  }

> +++ b/src/error.c
> +typedef struct {
> +  GabbleTLSCertificateRejectReason tls_reason;
> +  TpConnectionStatusReason tp_reason;
> +} TLSToTpReasonMap;

I think this might be clearer as a switch() in the function
gabble_tls_reason_to_conn_reason. A switch() is also O(1) instead of O(n), if
you like micro-optimizations.

If the TLS manager does the heavy lifting as I suggested above, then this could
all go in the TLS manager's source file, and the TLS manager's API to the
Connection would just be to return the right error.

> +++ b/src/tls-certificate.h
> +#include <wocky/wocky-tls.h>

This doesn't seem to be necessary for the header, only for the implementation?

> +void
> +gabble_tls_certificate_register (GabbleTLSCertificate *self)
> +{
> +  DBusGConnection *bus;
> +
> +  /* register on the bus */
> +  bus = tp_get_bus ();

I'd prefer new code to take a TpDBusDaemon as a construct-only property (or
obtain it from its TpBaseConnection/GabbleConnection, if it has one), and use
tp_dbus_daemon_register_object() on it. (The same applies to the channel.)

Given that a GabbleTLSCertificate knows its own object path and immutable
properties, why would it be necessary to have a delay between creation and
export? You could just export it in the constructed() GObject pseudo-method.

> +  DEBUG ("Created TLS certificate, type %s, at path %s, with data %p",
> +      cert_type, object_path, cert_data);

This is in gabble_tls_certificate_new(), so it won't run if an object is
created by calling g_object_new() directly (e.g. in a subclass). If this debug
message is useful, the init(), constructor() or constructed() would be the
place to put it. I realise you don't have any subclasses here, but having code
other than g_object_new() in a _new() wrapper is a bad habit to get into.

> +/* public */
> +GabbleTLSCertificate *
> +gabble_tls_certificate_new (const gchar *object_path,

Why is it necessary to say this is public? The comment doesn't seem very
useful.

> +gabble_tls_certificate_reject (GabbleSvcAuthenticationTLSCertificate *cert,
...
> +  g_assert (self->priv->cert_state == GABBLE_TLS_CERTIFICATE_STATE_PENDING);

Don't assert on D-Bus input (and call sequences are input). Calling one of
Accept() or Reject() while in a state other than PENDING should send an
appropriate error to the caller, not crash Gabble.

> +  retval = g_object_new (GABBLE_TYPE_TLS_CERTIFICATE,
> +      "object-path", object_path, "certificate-chain-data",
> +      cert_data, "certificate-type", cert_type, NULL);

Please break lines in logical places when varargs are paired or otherwise
linked:

    g_object_new (TYPE,
        "first-prop", first_value,
        "second-prop", second_value,
        NULL);

(In Telepathy style, this even applies if the number of pairs is 0 or 1.)

> +    guint in_Reason,
> +    const gchar *in_Error,
> +    GHashTable *in_Details,

The codegen generates these names as a cheap way to get non-colliding parameter
names that aren't C keywords. Actual code written by actual people should use
conventional parameter names (I'd use reason, error and details here).

> +      /* FIXME: this should be generated for us. */
> +      array_of_ay_get_type (),

Don't you get GABBLE_ARRAY_TYPE_CERTIFICATE_DATA_LIST in
extensions/_gen/gtypes.h?

> +static void gabble_server_tls_channel_verify_async (WockyTLSHandler *handler,
> +    WockyTLSSession *tls_session, const gchar *peername,
> +    GAsyncReadyCallback callback, gpointer user_data);

Why's this forward-declared? I try to avoid static forward declarations unless
they're really needed. (channel_iface_init is really needed, because it's used
in G_DEFINE_TYPE_WITH_CODE.)

> +static const gchar *gabble_server_tls_channel_interfaces[] = {
> +  GABBLE_IFACE_CHANNEL_TYPE_SERVER_TLS_CONNECTION,
> +  NULL
> +};

The channel type doesn't need to appear in Interfaces.

> +gabble_server_tls_channel_init (GabbleServerTLSChannel *self)
> +  self->priv->closed = TRUE;

This business with setting closed initially, and clearing it on export, seems
rather astonishing. What's your intention here? Why is starting with
closed=FALSE insufficient?

> +  if (self->priv->server_cert != NULL)
> +    g_object_unref (self->priv->server_cert);

tp_clear_object (&self->priv->server_cert) is less code :-)

Releasing references in finalize isn't right; they should be released in
dispose. Only things that don't hold a reference should be released in
finalize.

> +GabbleTLSCertificate *
> +gabble_server_tls_channel_get_certificate (GabbleServerTLSChannel *self)
> +{
> +  if (self->priv->tls_state_changed)
> +    return self->priv->server_cert;
> +  else
> +    return NULL;
> +}

What's going on here? The server cert exists, regardless...

> +static void
> +tls_certificate_rejected_cb (GabbleSvcAuthenticationTLSCertificate *cert,
> +    guint reason,

Shouldn't this be a member of WockyTlsCertError?

> +  g_object_unref (self->priv->async_result);
> +  self->priv->async_result = NULL;

tp_clear_object() is my new favourite hammer :-P

> +cert_type_to_str (WockyTLSCertType type)

This could usefully use a switch().

Isn't "X509" meant to be "x509" per telepathy-spec? Either change the
implementation or the spec (and I think the CertificateType needs to be
machine-readable rather than human-readable, so the spec's right and the
implementation's wrong).

Similarly, isn't "OpenPGP" meant to be "pgp"?

Will WOCKY_TLS_CERT_TYPE_NONE ever appear here? It probably shouldn't...

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.