New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SSL support? #217

Closed
alxpettit opened this Issue Feb 2, 2019 · 14 comments

Comments

Projects
None yet
4 participants
@alxpettit
Copy link

alxpettit commented Feb 2, 2019

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

@Et0h

This comment has been minimized.

Copy link
Contributor

Et0h commented Feb 3, 2019

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

This comment has been minimized.

Copy link
Member

albertosottile commented Feb 3, 2019

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 comment has been minimized.

Copy link

Mikaela commented Feb 4, 2019

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.

@alxpettit

This comment has been minimized.

Copy link
Author

alxpettit commented Feb 19, 2019

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

@alxpettit alxpettit closed this Feb 19, 2019

@albertosottile

This comment has been minimized.

Copy link
Member

albertosottile commented Feb 19, 2019

@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.

@alxpettit

This comment has been minimized.

Copy link
Author

alxpettit commented Feb 19, 2019

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

This comment has been minimized.

Copy link
Member

albertosottile commented Feb 19, 2019

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.

@alxpettit

This comment has been minimized.

Copy link
Author

alxpettit commented Feb 19, 2019

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

This comment has been minimized.

Copy link
Member

albertosottile commented Feb 19, 2019

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

This comment has been minimized.

Copy link
Member

albertosottile commented Feb 19, 2019

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.

@alxpettit

This comment has been minimized.

Copy link
Author

alxpettit commented Feb 19, 2019

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

@alxpettit

This comment has been minimized.

Copy link
Author

alxpettit commented Feb 19, 2019

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.

@Et0h

This comment has been minimized.

Copy link
Contributor

Et0h commented Feb 19, 2019

@alxpettit I can see the benefits, but there were a few reasons why I did not think we needed a TLS/SSL-only option:

  1. Syncplay is not intended to be used for sharing sensitive data. In the message Syncplay displays at the top of the notification window it explicitly states: "Do not use Syncplay to send sensitive information". Syncplay is not intended to be a secure chat facility, and indeed in the future we might allow for chat logging.

  2. Syncplay does not share your IP address with other users and does not require registration. As such, the only information transmitted is that someone going under a certain alias was playing a certain file or making a certain change to the playlist. If a user don't want their viewing habits associated with them then they can just avoid using an identifiable username. For those on one of the public servers (or any server with 'room isolation' enabled) only those in the same room can see any of this information, adding an additional layer of privacy.

  3. Many people still use older versions of Syncplay without SSL/TLS support. Not only does the default build of Syncplay for Windows not have SSL/TLS support yet but neither does the legacy release for macOS. At present Syncplay has excellent backwards compatibility, which means viewing sessions are not delayed by people having to download a newer version of Syncplay.

  4. Anyone can fork their own version with SSL/TLS-only support. Because Syncplay is open source we do not have to cater for every use case.

@alxpettit

This comment has been minimized.

Copy link
Author

alxpettit commented Feb 19, 2019

I mean, yeah, but...

  1. Some people take their security pretty seriously and prefer to have their idle chats secure, and it is pretty convenient to use the Syncplay chat feature for those idle chats.

The fact that SSL is so secure and we can just take it and plug it into other software systems pretty easily -- means that tightening the security of Syncplay to the point where it could be used for highly sensitive conversations, is hardly unfeasible -- even if such use cases are unimportant and aren't the main target case. And I find in the post-Snowden environment, that security-by-default is a very good attitude to take.

  1. Continuously changing your nick is pretty inconvenient, and the main point of SSL encryption is to secure data while it's in motion. If your ISP is spying on you, they know A) where the packets are coming from, B) what their contents are, and C) where they're going to.

Not having the server share private data , is not going to protect that data if a malicious pipe itself can sidechannel and rewrite any data it wants. Client-to-server encryption protects the data in the pipe, not at any of the nodes. And that's fine(ish) because anyone can set up their own server if they don't trust the ones available.

If we assume that all pipes are compromised (a fair assumption, given our post-Snowden era), and we already know all data is forwarded to each node through a pipe, then any one node that does not use encryption will compromise the data.

Which means anyone in that use case will have to constantly monitor whether anyone is connecting with a non-SSL version, or patch it themselves.

  1. Small groups of people using a private server can easily make sure they're all up to date.

It's also notable that I doubt anyone who isn't criminally lazy is going to be using the pre-SSL version, say, 2 years from now. At which point, obligate SSL could presumably be deployed on the major servers, if the devs decided to do so. But anyway, this feature isn't meant to target the major servers (at least, in the immediate future).

  1. Any feature can be added to open source software -- but that's not really an important point, because forking for every little feature is inefficient as well (I mean... just look what happened to Gnome2). But I will agree that maybe I'm the only one who likes to use a private server for themselves and their friends, and who likes to be as certain as possible that those private conversations, however irrelevant they might be, don't get read by the NSA and such.

I'm not asking you to make adjustments to the program just for my sake. The feature creep in a program that tries to accommodate every use case, no matter how obscure, would of course be horrific. I just didn't think my use case was all that unique or obscure. But I could be wrong! And if I am, fair enough. :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment