Migrate Wocky to GIO-TLS #5
Conversation
|
@noonien-d would you mind taking a look? I have no idea what is going on, I can only press "Merge" and (optionally) tag a release. |
| #include <sys/types.h> | ||
| #include <sys/stat.h> | ||
| #include <unistd.h> | ||
| #include <dirent.h> | ||
|
|
||
| /* DANWFIXME: allow configuring compression options */ |
There was a problem hiding this comment.
Part of original patch :)
There was a problem hiding this comment.
But ok, perhaps we can replace that with more valuable comment like - use GIO_USE_TLS and G_TLS_GNUTLS_PRIORITY to set the SSL options.
On the other hand there are more places like that across the original patch, so I'd rather leave it as is for future cleanup, at the moment main question is whether we go full GIO-TLS or not. I'm reading the patches for further cleanup - like removing of direct openssl backend (it's available via GIO) so those markers are the places of potential future attention.
There was a problem hiding this comment.
I will test gio-tls for sailfish the next days.
There was a problem hiding this comment.
I had just left comments wherever the patched version didn't support everything the original did. It is entirely possible that nothing uses the functionality that is no longer there.
There was a problem hiding this comment.
Wocky compiles when dropping the last commit, but gabble-0.18 doesn't compile with wocky-master. Sailfish needs gabble-0.18 due to an old telepathy-glib :-/
So I guess I'll stick to openssl for my Sailfish build, that shouldn't be a hindrance for wocky-master development ...
There was a problem hiding this comment.
Ok, let me post my current testing branch
https://github.com/rufferson/telepathy-gabble/tree/test-merged
which includes your and mine PRs as well as other minor cleanup patches I've cherry-picked (including GIO-TLS one)
There was a problem hiding this comment.
I see, when including Adjust callbacks to Wocky GIO-TLS changes gabble-18.4 builds with gio-tls on Sailfish SDK. I will take a deeper look these days.
There was a problem hiding this comment.
We need to apply the callback patch to gabble master, don't we?
There was a problem hiding this comment.
when we update submodule tip, yes
| } | ||
|
|
||
| void | ||
| wocky_tls_session_add_crl (WockyTLSSession *session, const gchar *crl_path) | ||
| { | ||
| int n = 0; |
There was a problem hiding this comment.
This (now empty function) seems to be used in wocky-connector-test only, otherwise it could be an security issue. Might be a reason for the test failing, though.
There was a problem hiding this comment.
Right, that was also called out in original patch here https://bugs.freedesktop.org/show_bug.cgi?id=31447#c15
|
So besides the todos/comments and failing tests it seems okay for me. It would be nice of you keep on working to finish this code. There are also some tab/whitespace errors introduced in wocky-jingle-session.c, wocky-jabber-auth.c and wocky-connector.c, found by make check. They should get fixed before merging. |
|
interesting results, I'm still working on failing tests and so far following observations:
Upd: Actually with openssl almost all tests succeed now, with exception of several remaining SASL failures and several error mismatch (woky/gio tls error mapping). With gnutls server implementation for some reason cannot support tls1.2 |
* Localize variables in wocky-connector.c and wocky-jabber-auth.c * Add fallthrough marker to wocky-jingle-session.c * remove double const in wocky-data-form.c * fix pointer dereference in wocky-sasl-digest-md5.c
This is how gio TLS does it, among other reasons because it lets you use the SNI extension to tell the server which certificate it should present. https://bugs.freedesktop.org/show_bug.cgi?id=31447
to match gio TLS, and because there's not much use in the separation anyway https://bugs.freedesktop.org/show_bug.cgi?id=31447
GTlsConnection can only wrap pollable input/output streams, so implement that here to make some of the test cases work. https://bugs.freedesktop.org/show_bug.cgi?id=31447
A few minor things, marked DANWFIXME, are unimplemented https://bugs.freedesktop.org/show_bug.cgi?id=31447
|
Ok se eventually gnutls server implementation does not support properly TLSv1.3 and that's what GIO forces it to use on client side. while it does work with openssl: Interestingly the chosen cipher (TLS_AES_256_GCM_SHA384) was also advertised by gnutls-serv in debug as available but it was unable to select it. It does work with TLS1.2 though - and indeed if I disable TLS1.3 gnutls backend behaves identically to openssl (sasl fails ob both, still trying to figure why): |
|
and here's why sasl test fails. Apart of this only CRL fails - for known reason (no support for custom CRL in GIO) |
* Update test certificate to use SHA2 to avoid INSECURE error * Add certificate refresh dependency to Makefile * Add SASL SCRAM worng password test workaround * Suppress CRL verification tests as not supported by GIO-TLS * Fix summarise-tests.py to handle deprecations and new syntax
|
PASS: wocky-test-sasl-auth |
|
So I think I'm good with this one, I've already started stockpiling next MR on top to cleanup/address Dan's spotlights, remove remnants of old code (openssl) and add additional test case (weak hash algo) |
|
Are you done with this one? @noonien-d can I consider this PR as reviewed? Edit: I just noticed |
This contains long past due patch made by @danwinship in 2010 as https://bugs.freedesktop.org/show_bug.cgi?id=31447.
After I restricted my jabber server to TLS1.2+ telepathy stopped connecting to it, it was unable to read content via TLS1.3 stream (eg. it sent stream start, server responded with stream start, but wocky doesn't see it). The best I could do was to revive this old patch and do some cleanups to make it work. And it does work actually now.
Testing is a mess though, I've updated resolver to match latest glib spec but TLS tests are failing more often than not.