Add SSL support?

[crabdancing]

It looks like Python's Twisted already supports SSL. How difficult would it be to refactor to add this?

[Et0h]

In the long term it seems like SSL (or equivalent) would be a nice feature to have, but none of the core Syncplay developers have sufficient SSL experience to really move this forwards. We do not want broken SSL support (or for SSL support to come at the expense of usability), so we would need someone with a good track record of SSL development to come up with an implementation which worked for Syncplay and to then not just implement it but also to maintain it.

There is also the question of how to handle backwards compatibility, and whether if we are moving to SSL we should actually be moving to secure websockets to allow for browser-based implementations of Syncplay in the future.

As per #107 (comment) and #107 (comment) @ccxcz has done some interesting initial work on 'endpoints' implementation that could theoretically move us closer to SSL, but in its current form it does not offer a continuity of experience (i.e. we do not yet have something which is "Syncplay just works as you would expect, but it now has SSL support").

[albertosottile]

I would like to add that twisted currently does not provide an SSL6ServerEndpoint as per the latest documentation (link). While this absence does not prevent us to support TLS, it definitely complicates things further.

[Mikaela]

This may be a bit offtopic, but I have encrypted connection to my Syncplay server with Yggdrasil network. There are packages for Debian & RHEL based distributions, macOS and Windows, all instances get inter-Yggdrasil-IPv6 address that is based on their keys and within everything is end-to-end encrypted.

While supporting TLS natively would be nice, I would wish my current setup to keep working, because otherwise I would need to bother a lot more with getting valid SSL certificates and how to automate LetsEncrypt certificate getting to Syncplay and reloading the certificate and all kinds of complications there may be.

For example ZNC currently reloads the certificate every time when a client is connecting, which is not very efficient. On the other hand some versions of Mumble's server (murmur) only reload certificate on restart and there are unlucky cases where the server goes down for people, because automagic restarts it for renewing the certificate. On Mumble I have heard that there is git version reloading certificate on SIGUSR1 or SIGUSR2, I am not sure which and I have no idea if it has been released yet.

I don't know if Syncplay clients keep working even if the server restarts in the middle of playing.

[crabdancing]

It looks like SSL support changes have been merged, so I'll go ahead and close this! ^.^

[albertosottile]

@alxpettit Could you give a look at the wiki page I prepared for this feature?
https://github.com/Syncplay/syncplay/wiki/TLS-support

Please let me know if there is anything that you would change or that you think is not clear enough. Thanks.

[crabdancing]

One thing I would change is to have separate arguments passing locations for the private key and cert chain, since the names of those files aren't very well standardized. All other server apps that make use of SSL that I've ever used (nginx, apache, ngircd...) allow the user to separately designate the name and path of each file, which is less elegant, but provides more control.

Also, rather than using chain.pem and cert.pem, they just use fullchain.pem.

Staying within user expectations should help to reduce confusion and problems, if possible.

Edit: oh, I see it needs python-certifi now. I'll go ahead and add that to the optdepends in the AUR package for Syncplay :)

Edit 2: also, it looks like we're now up to like 6 different options for servers. It might be about time to implement some sort of server config file, as an alternative to args (also helps for cases where the server has to run on an insecure system and the password is visible to anyone who can see the process running because it's passed as an arg)... May I suggest YAML format? How do people feel about that? Let me know!

[albertosottile]

As much as I would love to get rid of the extra chain, I am afraid that is not currently possible with Twisted (and believe me, I tried). See documentation and also this post.

Hence, we would have to provide three parameters for three different files to customize them, and I do not really like this option. That is one of the reasons we are providing a wiki/instruction page, so that server admins can read it if they need help configuring their server. Later down the road, once the protocol has been tested, we could use a configuration file for this feature and for other server arguments.

Edit: I was thinking about your "edit 2" while you were writing it. We'll probably give it a try in the future.

[crabdancing]

Actually, adding YAML would probably add an additional prereq, wouldn't it? May be better to go with JSON. Although I personally hate editing JSON config files by hand. Hmm. Maybe some simple INI-type thing that could be implemented in house with like 10 lines of code?

[albertosottile]

Edit: oh, I see it needs python-certifi now. I'll go ahead and add that to the optdepends in the AUR package for Syncplay :)

It actually needs also twisted[tls] and I am not sure if AUR provides also dependencies for extras declared in python packages. For the record, the [tls] extra also installs (besides the usual twisted dependencies):

'pyopenssl >= 16.0.0',
'service_identity',
'idna >= 0.6, != 2.3'

[albertosottile]

Maybe some simple INI-type thing that could be implemented in house with like 10 lines of code?

We are already using configparserfor client configuration files, so we will probably use that. But, I would not hold my breath for the release of this feature. The server CLI so far has been working great and I am afraid we have some other stuff to address before introducing a server configuration file.

[crabdancing]

Yeah, it's not super important or anything either. Just something to put on the checklist. But it is pretty easy to implement it, I might go ahead and do a pull request for it. Although I already am working on a pull request for a event handler system, so I'll probably shelve it for now. :P

[crabdancing]

Oh, @albertosottile! Something just occured to me: there should probably be an option for the server to force SSL-only connections. The rationale being that if a single person connects without SSL support, then all communications are being transferred unencrypted over their connection, which may be a problem if any sensitive data is shared by other people.