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

Few debian updates #5729

Merged
merged 10 commits into from
Apr 8, 2020
Merged

Few debian updates #5729

merged 10 commits into from
Apr 8, 2020

Conversation

damencho
Copy link
Member

@damencho damencho commented Apr 6, 2020

No description provided.

@damencho damencho requested a review from saghul April 6, 2020 21:27
@damencho
Copy link
Member Author

damencho commented Apr 6, 2020

turnserver still does not work on Debian10 because of permissions ... #5714

@damencho
Copy link
Member Author

damencho commented Apr 6, 2020

I will push that commit from the other PR here just to test it.

@damencho
Copy link
Member Author

damencho commented Apr 7, 2020

Finally I made it work :)

@damencho
Copy link
Member Author

damencho commented Apr 7, 2020

@saghul I think this and the meta-package PR are all the known problems so far fixed. I was able to repro several times the re-ordering of the installing packets on Debian10 and with these PRs it is fixed.
Also, I tested with apache, with Nginx already listening on 443 ... and we don't need --no-recommends instructions no more.

saghul
saghul previously approved these changes Apr 7, 2020
Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

LGTM! Question: didn’t we want to drop support for prosody-trunk?

@paulmenzel
Copy link
Contributor

Commit debian: Update coturn udp port to non-privileged one. misses the explanation, why this change is good. The whole point of using the TURN server Coturn is for users behind Firewalls with only ports 80 and 443 open, isn’t it?

@saghul
Copy link
Member

saghul commented Apr 7, 2020

The whole point of using the TURN server Coturn is for users behind Firewalls with only ports 80 and 443 open, isn’t it?

No, it isn't. If 2 participants are behind symmetric NATs they will need TURN to etablish a P2P session.

The reuse of ports 80 / 443 is is indeed nice to try and circumvent spartan firewalls, but the problem here is that Debian 10 doesn't have the capability to bind to priviledged ports set on their unit file, and hacking it ourselves is just not nice.

This is just our default, you are welcome to make changes to your installation of course.

@paulmenzel
Copy link
Contributor

I’d prefer if the scripts would create a drop-in snippet for Coturn.

$ cat /etc/systemd/system/turnserver.service.d/allow-bind-443.conf
[Service]
AmbientCapabilities=CAP_NET_BIND_SERVICE

(systemctl edit coturn.service would be for manually creating the drop-in.)

@saghul
Copy link
Member

saghul commented Apr 7, 2020

We have spent countless hours trying to get this right, we'll just land it as is.

@paulmenzel
Copy link
Contributor

I am sorry, that you had to spend so much time with this, and thank you for supporting Jitsi users.

What is the actual problem of sharing the port? The commit message and pull request description do not mention this.

@julienfastre
Copy link
Contributor

The issue is here: #5529

TLDR: There are two problems:

  • the turnserver user does not have any permission to bind on port 443/udp (because this is a privilegied port) ;
  • the turnserver user does not have sufficient permissions to read the letsencrypt certificates.


# create a directory to store certs if it does not exists
if [ ! -d "$COTURN_CERT_DIR" ]; then
mkdir -p /etc/coturn/certs
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the variable COTURN_CERT_DIR here?


for domain in $RENEWED_DOMAINS; do
case $domain in
jitsi-meet.example.com)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment that this is updated/replaced with the correct domain by the install script?

@paulmenzel
Copy link
Contributor

The issue is here: #5529

TLDR: There are two problems:

Thank you for the summary.

* the `turnserver` user does not have any permission to bind on port 443/udp (because this is a privilegied port) ;

Instead of changing to an unprivileged port, I am proposing to extend Coturn’s service unit to give the needed capability to bind to a privileged port to keep “The reuse of ports 80 / 443 is is indeed nice to try and circumvent spartan firewalls,” working.

* the `turnserver` user does not have sufficient permissions to read the letsencrypt certificates.

Yes, fixed by commit add scripts for deploying coturn with certbot.

@saghul
Copy link
Member

saghul commented Apr 7, 2020

What is the actual problem of sharing the port? The commit message and pull request description do not mention this.

The problem is not with sharing, it's with coturn not being able to bind to a priviledged port by default, on Debian 10. This is not the case on Ubuntu for example.

saghul
saghul previously approved these changes Apr 7, 2020
@damencho
Copy link
Member Author

damencho commented Apr 8, 2020

Oh, there are more cases here with this turn ... if we skip its configuration, actually it is installed ... we may need to just configure it on another port (change prosody config) ...

@paulmenzel
Copy link
Contributor

Oh, there are more cases here with this turn ...

Do you mean problems? Which are those?

if we skip its configuration, actually it is installed ...

What do you mean? The package coturn was always installed, wasn’t it?

we may need to just configure it on another port (change prosody config) ...

Why? To fix what problems?

PS: It’s quite hard to help in any way with these terse (short) comments.

@damencho
Copy link
Member Author

damencho commented Apr 8, 2020

Yeah, sorry for those. They were more like self note and @saul notes.

Even when we skip configuring coturn it is installed as it is dependency of this package. And we still advertise it.
So what I was thinking last night is to always configuure it and depending on whether there is another nginx site listening on 443 either multiplex or configure coturn and advertise the non 4443 port and for tcp and add instructions the this port may need port forwarding.

@damencho
Copy link
Member Author

damencho commented Apr 8, 2020

I was also to leave and coturn and for apache on the non standard port.

@damencho
Copy link
Member Author

damencho commented Apr 8, 2020

I did a few more changes. Hope those are the last one. I will be testing those now.

@damencho
Copy link
Member Author

damencho commented Apr 8, 2020

Jenkins, skip ci.

@damencho
Copy link
Member Author

damencho commented Apr 8, 2020

@paulmenzel I think everything is covered now. If you can take a look. @saghul will also take a look and approve. But I think this is it and this will go to stable fixing all discovered issues with the packaging.

Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Awesome work @damencho !

@damencho damencho merged commit ffdd4f2 into master Apr 8, 2020
@damencho damencho deleted the deb-up branch April 8, 2020 18:06
Fair-Exchange pushed a commit to Fair-Exchange/safechat that referenced this pull request Apr 9, 2020
* debian: Update coturn udp port to non-privileged one.

* debian: Turnserver config requires jitsi-meet-web-config files.

* doc: Updates doc, removing `--no-install-recommends`.

* debian: Moves checks and configs to default to prosody 0.11.

* debian: Disable room locking on internal muc.

* add scripts for deploying coturn with certbot

* turnserver: Removes unused variable showing error.

* debian: updates let's encrypt and coturn scripts.

* debian: Detect failure to retrieve external ip address.

* debian: Always configure turn when the turnserver package is installed.

Co-authored-by: Julien Fastré <[email protected]>
@Yetangitu
Copy link
Contributor

Yetangitu commented Apr 17, 2020

Please have a look at my comment in the 'Move UDP port to 4446' thread: #5714 (comment)

STUN and TURN have officially designated ports, 3478 and (TLS) 5349. Some sites already have turn/stun-servers and the accompanying router and firewall configurations, generally set for those official ports. I suggest using these ports instead of 4446/4445.

I just submitted a PR for this: #6172

@damencho
Copy link
Member Author

We are discussing this already, to drop the nginx multiplexing and just leave a documentation how to do it. And when this happen we will install and configure turnserver by default even with apache and leave ports on their defaults. We just haven't been able to work on that.

@RubensRainelli
Copy link

@damencho please fix it ASAP since it broke me 2 servers while reloading NGINX

@damencho
Copy link
Member Author

What did broke is more important, as it should not break anything at the moment.

@RubensRainelli
Copy link

And it will break if I didn't made my own fix xD

@damencho
Copy link
Member Author

The current stable detects and if there is already a host listening to 443 the multiplexing is skipped.

@paulmenzel
Copy link
Contributor

@RubensRainelli, can you please create an issue with more context and refer to this merge/pull request as the “culprit”?

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