Skip to content
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

Allow server host to decide peer to peer behavior #28

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

sjoerd108
Copy link

Ties in with the PR over at CrewLink that adds support for TURN/STUN servers.

This PR, when combined with the CrewLink PR allows the Voice Server's host to decide how CrewLink users connect to one another. The template config file explains all the properties and the addition to README.md should point users in the right direction for setting up their own TURN server.

The CrewLink server will send its peer config over websockets as soon as a client connects. The client then verifies that the config is valid and applies it. If no config file is made then the defaults will be used.

I've also changed the part about removing the 'https://' component from Heroku URLs as that is no longer needed (in fact it would break).

@MattJustMatt
Copy link

Looks good to me. This is a re(lay) nice feature :D

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@sjoerd108
Copy link
Author

sjoerd108 commented Dec 8, 2020

@djmattyg007 Thanks for checking the readme, I've addressed the points you mentioned.

@sjoerd108 sjoerd108 marked this pull request as draft December 13, 2020 23:55
@sjoerd108
Copy link
Author

Converted to draft in order to look into possibilities for bundled TURN servers.

@sjoerd108 sjoerd108 marked this pull request as ready for review January 1, 2021 13:47
@sjoerd108
Copy link
Author

I've done a lot of testing with a possible integrated TURN server using node-turn. Sadly, node-turn does not provide an implementation that works well enough for this. I've had various issues but the main one is that after 40 seconds, the peer connection gets marked as failed, despite CrewLink seemingly working. I think it'd be best to fix these issues either in node turn or use another implementation and making it a separate PR.

@Fabiryn
Copy link

Fabiryn commented Jan 1, 2021

Hey, I also tested the turn implementation. It works fine after the initial setup of a coturn server. But maybe we should add a simple config example for coturn or even a docker-compose example?

The coturn config is somewhat complicated.

docker-compose.yml
version: '3.7'

services:
crewlink:
    image: ottomated/crewlink-server:latest
    depends_on:
    - turn
    ports:
    - "9736:9736"
    restart: always
    volumes:
    - ./config/peerConfig.yml:/app/config/peerConfig.yml
    environment:
    - ADDRESS=example.com

turn:
    image: instrumentisto/coturn:latest
    network_mode: host
    restart: always
    volumes:
    - ./config/coturn.conf:/my/coturn.conf
    #- /path/to/cert:/certs/cert.pem # optional
    #- /path/to/privkey.pem:/certs/privkey.pem # optional
    #- /path/to/dhparam.pem:/certs/dhparam.pem # optional
    command: ["-c /my/coturn.conf"]
coturn.conf
# Coturn TURN SERVER configuration file
#
# Boolean values note: where a boolean value is supposed to be used,
# you can use '0', 'off', 'no', 'false', or 'f' as 'false,
# and you can use '1', 'on', 'yes', 'true', or 't' as 'true'
# If the value is missing, then it means 'true' by default.
#


# TURN listener port for UDP and TCP (Default: 3478).
# Note: actually, TLS & DTLS sessions can connect to the
# "plain" TCP & UDP port(s), too - if allowed by configuration.
#
listening-port=3478

# Lower and upper bounds of the UDP relay endpoints:
# (default values are 49152 and 65535)
#
min-port=49152
max-port=65535

# Uncomment to use fingerprints in the TURN messages.
# By default the fingerprints are off.
#
fingerprint

# Uncomment to use long-term credential mechanism.
# By default no credentials mechanism is used (any user allowed).
#
lt-cred-mech

# 'Static' user accounts for the long term credentials mechanism, only.
# This option cannot be used with TURN REST API.
# 'Static' user accounts are NOT dynamically checked by the turnserver process,
# so they can NOT be changed while the turnserver is running.
#
user=crewlink:server
#user=username1:key1
#user=username2:key2
# OR:
#user=username1:password1
#user=username2:password2
#
# Keys must be generated by turnadmin utility. The key value depends
# on user name, realm, and password:
#
# Example:
# $ turnadmin -k -u ninefingers -r north.gov -p youhavetoberealistic
# Output: 0xbc807ee29df3c9ffa736523fb2c4e8ee
# ('0x' in the beginning of the key is what differentiates the key from
# password. If it has 0x then it is a key, otherwise it is a password).
#
# The corresponding user account entry in the config file will be:
#
#user=ninefingers:0xbc807ee29df3c9ffa736523fb2c4e8ee
# Or, equivalently, with open clear password (less secure):
#user=ninefingers:youhavetoberealistic
#

# The default realm to be used for the users when no explicit
# origin/realm relationship is found in the database, or if the TURN
# server is not using any database (just the commands-line settings
# and the userdb file). Must be used with long-term credentials
# mechanism or with TURN REST API.
#
# Note: If the default realm is not specified, then realm falls back to the host domain name.
#       If the domain name string is empty, or set to '(None)', then it is initialized as an empty string.
#
realm=crewlink

# Flag to prevent stdout log messages.
# By default, all log messages go to both stdout and to
# the configured log file. With this option everything will
# go to the configured log only (unless the log file itself is stdout).
#
#no-stdout-log

# Option to set the log file name.
# By default, the turnserver tries to open a log file in
# /var/log, /var/tmp, /tmp and the current directory
# (Whichever file open operation succeeds first will be used).
# With this option you can set the definite log file name.
# The special names are "stdout" and "-" - they will force everything
# to the stdout. Also, the "syslog" name will force everything to
# the system log (syslog).
# In the runtime, the logfile can be reset with the SIGHUP signal
# to the turnserver process.
#
log-file=stdout

# Turn OFF the CLI support.
# By default it is always ON.
# See also options cli-ip and cli-port.
#
no-cli

@sjoerd108
Copy link
Author

@Fabiryn I wasn't referring to Coturn, that works perfectly fine. If you revert the fork back to commit 6dde958 you'll notice an integrated TURN server, which would have been really easy to set up, that's what I was referring to with "integrated TURN server". The addition to the dockerfile would be good but things like the TURN username and password would need to be random for each instance. Running the relay itself is probably best kept optional due to the fact that it's fairly easy to obtain the TURN password with this implementation.

@Fabiryn
Copy link

Fabiryn commented Jan 1, 2021

@sjoerd108 no I meant in generall. I didn't test the node implementation. I setup the coturn server simular to the config example. Do you have a good solution in mind to randomly generate user and password and pass it on to the crewlink-server?

@tystuyfzand
Copy link
Contributor

@sjoerd108 coturn has an API that's made specifically for this which you could look into!

See https://stackoverflow.com/questions/35766382/coturn-how-to-use-turn-rest-api

@sjoerd108
Copy link
Author

@tystuyfzand Thanks for the suggestion. I'm aware of the API. I suppose it would allow function similar to the integrated relay with temporary user credentials when paired with a Dockerfile. It still wouldn't completely prevent unauthorized access to the TURN relay however as you could make a websocket connection to the server and get a set of credentials, so I'm inclined to not make it a default option if this was implemented.

@tystuyfzand
Copy link
Contributor

From what I understand, you can generate a set of credentials when the user connects with a set timeout, then expire them when they disconnect - each user can get their own set of credentials. It'd at least make it somewhat authenticated, but perhaps not quite as much as you'd like?

@sjoerd108
Copy link
Author

sjoerd108 commented Jan 2, 2021

Pretty much, yeah. It would harden things, but one can still obtain and abuse temporary TURN credentials. Bandwidth cost aside, it's not hard to imagine what an attacker might do with a server that relays packets to wherever you like, so in my opinion it's not something that should be made default. That said, I'd like to look into implementing Coturn's API for credential rotation.

@pengi
Copy link

pengi commented Jan 2, 2021

Earlier we had a lot of peer-to-peer connection issues when playing, so I set up a coturn server, and been using a custom build of this branch for a couple of rounds, and this TURN patch seems to solve all of the issues we had related to connection.

I agree that there are a lot of security related issues when scaling up the use of those featues, and I would love to see that being improved in the future too.

But since this patch doesn't enable TURN server settings by default, the default settings would be unchanged from previous version, and when running a public crewlink server, I can see that TURN server config would be disabled, not just by security, but also due to bandwidth.

I can't however decline that this patch works fine for the use of a private crewlink server for a smaller community, where coturn and crewlink-server is running on two limited VPSs that would hit the threshold when abused, if anyone would even find it.

So I would love to have this patch merged in the official build, to define how the crewlink client and server would communicate TURN server settings, and then have the TURN configuration as opt-in for now, since it really helps with the connection issues we, and many else, has. This patch contains some great work regarding improvement of the really cool CrewLink software.

I think the best way to progress regarding the security impliciations would be to keep a clear note in the example configuartion file describing the implications, then if and when someone solves it, it can be added as another PR. Doing so, third parties that are prepared for the security implications can still enjoy the really nice software CrewLink with the benefits of having a TURN server setup.

@djmattyg007
Copy link

Please don't let perfect be the enemy of good. If this patch is working in its current state, a lot of people would appreciate it being released for general use. Integrating with Coturn's API is a nice-to-have that should be handled in a new PR.

@sjoerd108
Copy link
Author

I agree with @djmattyg007 and @pengi here and I am not going to set these PRs back to draft status. I'll be working on a Coturn API implementation for another PR.

I've added a warning about the use of TURN servers to the config example like @pengi suggested (and copied said warning to the readme as well).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants