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

Coturn #667

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

Coturn #667

wants to merge 35 commits into from

Conversation

goacid
Copy link
Contributor

@goacid goacid commented Jul 3, 2020

Based on PR163 from netaskd : #163

  • Fix prosody configuration
    
  • Fix web configuration
    
  • Fix name : turn => coturn
    
  • Update README.md
    

@holtgrewe
Copy link

Oh, great. Maybe this has a better chance to succeed than the previous PR!

@parruc
Copy link

parruc commented Jul 8, 2020

Really hope this one will be merged. We will be testing this on our infrastructure ASAP and let u know

@goacid
Copy link
Contributor Author

goacid commented Aug 3, 2020

@saghul any reason not to merge ?

coturn.yml Outdated Show resolved Hide resolved
prosody/Dockerfile Outdated Show resolved Hide resolved
@nestorconde
Copy link

I have two questions about this PR.

First, why aren't we adding the turncredentials and turncredentials_secret to jitsi-meet.cfg.lua?, I do not see how mod_turncredentials could work without this.

Second, is out of the scope of this PR implementing some kind of multiplexing base on dns (or protocol) so the turn can work with firewalls that only allow 80 and 443? I'm talking about sth like https://jitsi.github.io/handbook/docs/devops-guide/turn#use-turn-server-on-port-443

@goacid
Copy link
Contributor Author

goacid commented Oct 1, 2020

I have two questions about this PR.

First, why aren't we adding the turncredentials and turncredentials_secret to jitsi-meet.cfg.lua?, I do not see how mod_turncredentials could work without this.

Well this way it works, but maybe possible to make it work in another way, I didn't test.

Second, is out of the scope of this PR implementing some kind of multiplexing base on dns (or protocol) so the turn can work with firewalls that only allow 80 and 443? I'm talking about sth like https://jitsi.github.io/handbook/docs/devops-guide/turn#use-turn-server-on-port-443

I miss this, very interresting. Some work to do to implement.

@nestorconde
Copy link

About the dns multiplexing. I had to update nginx in the web container for ssl_preread_module to work. Executing this in the container should be enough:

#/bin/sh
apt install -y wget
echo "deb http://nginx.org/packages/mainline/debian/ stretch nginx
deb-src http://nginx.org/packages/mainline/debian/ stretch nginx" > /etc/apt/sources.list.d/nginx.list
wget -qO - https://nginx.org/keys/nginx_signing.key | apt-key add -
apt update;
apt install -y nginx" >> test.sh

Then something activate it with something like this in nginx.conf

stream {
map $ssl_preread_server_name $name {
${subdomain}.${domain} web_backend;
turn.${subdomain}.${domain} turn_backend;
}
upstream web_backend {
server 127.0.0.1:4444;
}
upstream turn_backend {
server 127.0.0.1:5349;
}
server {
listen 443;
listen [::]:443;
# since 1.11.5
ssl_preread on;
proxy_pass $name;
# Increase buffer to serve video
proxy_buffer_size 10m;
}
}

And change default so the web nginx listens at 4444 instead of 443

@goacid
Copy link
Contributor Author

goacid commented Oct 2, 2020

Could you implement it based on this PR ?
#163 seems to old from now...

@nestorconde
Copy link

Could you implement it based on this PR ?
#163 seems to old from now...

I'll certainly try but unfortunately in the short term I'm too busy to dedicate enough time to it :(

@goacid
Copy link
Contributor Author

goacid commented Nov 26, 2020

@saghul : when TURN will be merged, ENABLE_STUN_TURN need be false by default

@saghul
Copy link
Member

saghul commented Nov 26, 2020

when TURN will be merged, ENABLE_STUN_TURN need be false by default

No way. Why do you think that is the case?

@goacid
Copy link
Contributor Author

goacid commented Nov 26, 2020

when TURN will be merged, ENABLE_STUN_TURN need be false by default

No way. Why do you think that is the case?

My bad, I'm rebasing this PR and indeed, it is not the case.

@goacid
Copy link
Contributor Author

goacid commented Nov 26, 2020

@saghul : is there a chance that is PR will be merged ?

@saghul
Copy link
Member

saghul commented Nov 26, 2020

Sure thing! I just need to find the time to properly test it, which I haven't managed to do yet. Btw, did you add Let's Encrypt support to the TURN setup?

@@ -48,6 +48,9 @@ RUN \
&& rm -rf /tmp/pkg /var/cache/apt

RUN patch -d /usr/lib/prosody/modules/muc -p0 < /prosody-plugins/muc_owner_allow_kick.patch
RUN \
curl -4so /prosody-plugins/mod_turncredentials.lua \
https://raw.githubusercontent.com/netaskd/mod_turncredentials/master/mod_turncredentials.lua
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed prosody-plugins package already contains mod_turncredentials?
https://raw.githubusercontent.com/jitsi/jitsi-meet/master/resources/prosody-plugins/mod_turncredentials.lua

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well exact, I can now remove this

@dzeroc
Copy link

dzeroc commented Nov 26, 2020

Hi,

We have faced many situations with only 80 and 443 ports allowed, so we followed this guide[0] with nginx in front of containers.
Now we are trying to put nginx stream configuration in web container but there would be invasive changes on .env.
There should be a correlation between TURN_PORT and HTTPS_PORT or maybe adding another variable.
Are you going to consider to have turn listening possibly on 443/tcp?

[0]https://jitsi.github.io/handbook/docs/devops-guide/turn

@goacid
Copy link
Contributor Author

goacid commented Nov 27, 2020

Hi,

We have faced many situations with only 80 and 443 ports allowed, so we followed this guide[0] with nginx in front of containers.
Now we are trying to put nginx stream configuration in web container but there would be invasive changes on .env.
There should be a correlation between TURN_PORT and HTTPS_PORT or maybe adding another variable.
Are you going to consider to have turn listening possibly on 443/tcp?

[0]https://jitsi.github.io/handbook/docs/devops-guide/turn

I considere that turn run in another port that the one use by jisti-meet web.
If someone can implement nginx configuration to handle both turn/web that will be perfect.

@goacid
Copy link
Contributor Author

goacid commented Nov 27, 2020

Sure thing! I just need to find the time to properly test it, which I haven't managed to do yet. Btw, did you add Let's Encrypt support to the TURN setup?

Done

@dzeroc
Copy link

dzeroc commented Nov 27, 2020

I considere that turn run in another port that the one use by jisti-meet web.
If someone can implement nginx configuration to handle both turn/web that will be perfect.

OK. I can work on it.
I assume that if TURN_PORT is set as 443, turn container will bind 5349 (default port) on host and web container will bind 443 and HTTPS_PORT (set different from 443) on host.

@goacid
Copy link
Contributor Author

goacid commented Nov 30, 2020

I considere that turn run in another port that the one use by jisti-meet web.
If someone can implement nginx configuration to handle both turn/web that will be perfect.

OK. I can work on it.
I assume that if TURN_PORT is set as 443, turn container will bind 5349 (default port) on host and web container will bind 443 and HTTPS_PORT (set different from 443) on host.

@nestorconde (see earlier msg) post about handle web and coturn on the same nginx and so same port
#163

saghul and others added 16 commits December 17, 2020 14:52
* a81ad73 prosody: add support for lobby
* baed605 web: fix removing closed captions button if transcription is enabled
* edecacd etherpad: add ability to use a external server
* a7563d4 jvb: use JVB_TCP_PORT for exposing the port
* b235ea1 prosody: disable s2s module
* 1d428a8 prosody: use a 2-stage build
* 613c26c misc: working on latest
* 4d72ee3 release: stable-4627-1
* 22b7063 examples: update Traefik v1 example
* 1381b08 prosody: fix installing dependdencies
* 2900c11 misc: add extra line to tag message
* c57a84b misc: working on latest
* 5ceaf5f web: add IPv6 support
* aff3775 xmpp: allow recorders to bypass lobby
* ad5625b jvb: switch to WebSocket based bridge channels
* 8110336 web: add ability to configure the nginx resolver
* 2f47518 jicofo: no auth URL in JWT auth mode
* c149463 web: build config.js on each boot
* c792bbc base: update frep
* bec928c prosody: configure lobby on the guest domain is necessary
* bcbd977 jicofo: pass XMPP_MUC_DOMAIN through docker-compose.yml
* 8f9caa4 jicofo: set XMPP_MUC_COMPONENT_PREFIX
* 2a0120d web: set security headers also for non HTTPS
* e6586f2 jvb: set LOCAL_ADDRESS to the correct local IP (jitsi#630)
* 97f5e75 base: optimize size
* b78c89e misc: minor Dockerfile Improvements
* a754519 misc: working on latest
* a81ad73 prosody: add support for lobby
* baed605 web: fix removing closed captions button if transcription is enabled
* edecacd etherpad: add ability to use a external server
* a7563d4 jvb: use JVB_TCP_PORT for exposing the port
* b235ea1 prosody: disable s2s module
* 1d428a8 prosody: use a 2-stage build
* 613c26c misc: working on latest
* 4d72ee3 release: stable-4627-1
* 22b7063 examples: update Traefik v1 example
* 1381b08 prosody: fix installing dependdencies
* 2900c11 misc: add extra line to tag message
* c57a84b misc: working on latest
* 5ceaf5f web: add IPv6 support
* aff3775 xmpp: allow recorders to bypass lobby
* ad5625b jvb: switch to WebSocket based bridge channels
* 8110336 web: add ability to configure the nginx resolver
* 2f47518 jicofo: no auth URL in JWT auth mode
* c149463 web: build config.js on each boot
* c792bbc base: update frep
* bec928c prosody: configure lobby on the guest domain is necessary
* bcbd977 jicofo: pass XMPP_MUC_DOMAIN through docker-compose.yml
* 8f9caa4 jicofo: set XMPP_MUC_COMPONENT_PREFIX
* 2a0120d web: set security headers also for non HTTPS
* e6586f2 jvb: set LOCAL_ADDRESS to the correct local IP (jitsi#630)
* 97f5e75 base: optimize size
* b78c89e misc: minor Dockerfile Improvements
* a754519 misc: working on latest
@goacid
Copy link
Contributor Author

goacid commented Jan 21, 2021

@saghul is there something I can do to help merge it

@jforjava
Copy link

@saghul is there something I can do to help merge it

Long awaited PR, I always hope it is merged asap. But maybe Jitsi team forget it now.

@saghul
Copy link
Member

saghul commented Mar 18, 2021

A couple of quick observations:

  • Don't use Alpine, use our base Debian image please (this is non-negotiable)
  • Use a different domain for Let's Encrypt, and SNI on the web container to direct traffic at the TURN server, so we can run it on port 443.

@aporquez
Copy link

@goacid do you need help with the change?

@goacid
Copy link
Contributor Author

goacid commented Apr 18, 2021

Yes ! not a lot of time at this moment, any help is welcome

turn/Dockerfile Outdated
Comment on lines 1 to 6
ARG VERSION
FROM instrumentisto/coturn:${VERSION:-latest}

RUN apk add --no-cache openssl
RUN apk add --no-cache certbot
RUN apk add --no-cache bash
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARG JITSI_REPO=jitsi
ARG BASE_TAG=latest
FROM ${JITSI_REPO}/base:${BASE_TAG}

RUN
apt-dpkg-wrap apt-get update &&
apt-dpkg-wrap apt-get install -y coturn &&
apt-cleanup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thansk !
@saghul ask
Use a different domain for Let's Encrypt, and SNI on the web container to direct traffic at the TURN server, so we can run it on port 443.
@nestorconde I think you work on that ?

@w4tsn
Copy link

w4tsn commented May 7, 2021

I've started out work on the Let's Encrypt part with the attached patch file. Unfortunately I don't quite understand all the details reading the docs. Especially the prosody configuration and let's encrypt hook. I'll add what else I spot via review.

0001-Add-TURN-configuration-to-web-container.txt

Hope this is of any help.

{{ if .Env.TURN_HOST }}
{ type = "{{ .Env.TURN_PROTO | default "turns" }}",
host = "{{ .Env.TURN_HOST }}",
port = {{ .Env.TURN_PORT | default "3478" }},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
port = {{ .Env.TURN_PORT | default "3478" }},
{{ if not (.Env.DISABLE_HTTPS | default "0" | toBool) }}
port = "443",
{{ else }}
port = {{ .Env.TURN_PORT | default "3478" }},
{{ end }}

We can hard code the 443 port here if TLS / Let's Encrypt is enabled / used

@pierreozoux pierreozoux mentioned this pull request May 25, 2021
# run coturn server with API auth method enabled.
turnserver -n \
--verbose \
--prod \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coturn/coturn@520e172

--no-software-attribute Production mode: hide the software version.

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.