mattermost / mattermost-plugin-calls

https://www.mattermost.com
Other
96 stars 54 forks source link

Turn server config #49

Closed justvanbloom closed 2 years ago

justvanbloom commented 2 years ago

Would be nice if we can insert shared cred and user-xx to make it use coturn.

streamer45 commented 2 years ago

Hi @justvanbloom,

are you looking to use a TURN server to relay the media to clients unable to connect directly?

We definitely have plans to support clients behind very strict firewalls but nothing has been decided yet on how exactly we want to implement it. While adding support for external TURN servers would be easy enough it's not something I am particularly excited about, especially as we are trying to avoid using 3rd party services where possible. So it may take a while before we support this functionality but it's definitely on the roadmap.

justvanbloom commented 2 years ago

Uh. Okay. Perhaphs i can mod it to use a external turn. Yes clients behind corporate fw are unable to use calls right now

Sent via Commodore 64 in use with Dataphone s21-23d

Am 26.04.2022 um 13:54 schrieb Claudio Costa @.***>:

 Hi @justvanbloom,

are you looking to use a TURN server to relay the media to clients unable to connect directly?

We definitely have plans to support clients behind very strict firewalls but nothing has been decided yet on how exactly we want to implement it. While adding support for external TURN servers would be easy enough it's not something I am particularly excited about, especially as we are trying to avoid using 3rd party services where possible. So it may take a while before we support this functionality but it's definitely on the roadmap.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

Asgoret commented 2 years ago

@streamer45 Would be great If u move it little up on roadmap and make in this year...or early :)

streamer45 commented 2 years ago

@Asgoret is your main requirement being able to set an external TURN server that you'll provide on your own or have an all in one solution?

Asgoret commented 2 years ago

@streamer45 external provided by our own. In my opinion it's not worth it to do all features all-in-one. Some things can make by external solutions, you just need to create such opportunity. It will cost less hours to program and will be good metric for necessity of such feature. If somebody will use it as external and want in all-in-one they can create request.

streamer45 commented 2 years ago

Alright, thanks for the context. I created an internal ticket (MM-44486) to keep track of this and hopefully include it sometime soon.

Asgoret commented 2 years ago

Ok. Thanks)

gecame commented 2 years ago

Hi @all, @streamer45, +10 for this enhancement 👍 It's a pretty common VoIP problem and affects far more than just some corporate networks with strict firewalls. Actually, a significant fraction of mobile networks (and increasingly more and more home ISPs) use so-called symmetric NATs (e.g. link1, link2) - these don't allow directly sending media end-to-end across network boundaries. Users behind such a mobile network/ISP will suffer from one-way audio or no audio at all without helpers such as TURN / ICE, even if they are connecting to a less restricted network. The good thing is that support for TURN and ICE comes "for free" with essentially all current browsers via WebRTC (link). By giving the option of defining a TURN server address, sites running Mattermost could then decide whether, where and how to set up their own TURN server(s) (such as coturn).

streamer45 commented 2 years ago

Thanks for the feedback @gecame. I definitely want to unblock this for anyone using the plugin so will make an effort to prioritize and hopefully it won't be too long before we get it in.

streamer45 commented 2 years ago

@justvanbloom, @Asgoret, @gecame I have an implementation to pass basic TURN server configs more or less ready.

Would any of you be willing to give a test build a try and let me know if it works as expected?

gecame commented 2 years ago

Great, many thanks @streamer45 ! I'll pass it on to our Mattermost expert, we will let you know 👍

ThiefMaster commented 2 years ago

Yes, please :)

Asgoret commented 2 years ago

@streamer45 yeah, why not?)

streamer45 commented 2 years ago

Alright then, here's a test build with the addition.

com.mattermost.calls-0.6.1-turn.tar.gz

There will be a new setting available in the Admin Console, to be filled as such:

image

Let me know if you get a chance to test it, thanks :)

ThiefMaster commented 2 years ago

small suggestion for the sake of convenience: paste the example json in here so people don't have to type all it manually ;)

streamer45 commented 2 years ago

small suggestion for the sake of convenience: paste the example json in here so people don't have to type all it manually ;)

[{
    "urls": ["turn:turn.example.com:3478"],
    "username": "username",
    "credential": "password"
}]
ThiefMaster commented 2 years ago

I have this in my coturn config (since this is what I use for my Matrix server as well):

# 'Static' authentication secret value (a string) for TURN REST API only.
static-auth-secret=<long shared secret here>

Is this supported? Or do I have to use the username/password option?

Edit: Found this snippet to generate a (temporary) username/password from the shared secret, so I'll try that...

secret=staticAuthSecretHere
u=$((`date +%s` + 3600)):test
p=$(echo -n $u | openssl dgst -hmac $secret -sha1 -binary | base64)
echo -e "username: $u\npassword: $p"
streamer45 commented 2 years ago

Anything more advanced than static username/password would likely need an interaction with some kind of API but that's beyond what we are doing here (just passing configs to the clients). Eventually that's what we want to do but it's certainly more involved and needs some consideration before going into implementation.

ThiefMaster commented 2 years ago

What e.g. matrix clients do is making an API call to the server which then returns a (short-lived) credential using the HMAC: https://github.com/matrix-org/synapse/blob/master/synapse/rest/client/voip.py

I think this would be a good approach here as well; that way you can either use a static username and password, or if you prefer not exposing permanently-valid credentials to clients (which is not great security-wise) you use the shared secret so it's up to the Mattermost server to generate the credentials which expire after a few hours.

Anyway, for testing purposes, generating the HMAC manually worked fine.

I just tested a bit with @gecame and even behind his mobile provider's NAT everything worked nicely :)

streamer45 commented 2 years ago

This is part of the why I didn't allow TURN servers in the first place cause static credentials are pretty weak. But we are on the same page. The next iteration would likely be what you describe, I just need to see if there's an API standard to do this or if it's up to the turn implementation. Are you using coturn as TURN server?

gecame commented 2 years ago

I can only endorse this, security-wise it would be a great improvement! And many thanks for the work done so far, this will really boost our Mattermost community :-)

ThiefMaster commented 2 years ago

Yes, I'm using coturn

ThiefMaster commented 2 years ago

It looks like there's at least a draft standard for this: https://datatracker.ietf.org/doc/html/draft-uberti-behave-turn-rest-00

streamer45 commented 2 years ago

Cool, thanks guys, I think we can work with that one to start with. I'll file a ticket for this improvement and let you know.

streamer45 commented 2 years ago

Some good news. I have worked on a simple implementation of the expected authentication scheme. As expected, this required adding a couple of new TURN specific settings, as follows:

image

Here's a test build for you to try. If working as expected I am happy to go ahead with the changes and release them officially.

com.mattermost.calls-0.6.2-turnauth.tar.gz

ThiefMaster commented 2 years ago

I'm getting this error when try to start a call after installing the new version:

image

            if (window.callsClient) return void mn('calls client is already initialized');
            const i = [
              ...Va(n.getState()) // it happened in this line
            ];
            try {
              const e = await t().get(''.concat(An(), '/turn-credentials'));
              i.push(...e.data)
            } catch (e) {
              mn(e)
            }
streamer45 commented 2 years ago

@ThiefMaster , I updated the attached build which should work now.

ThiefMaster commented 2 years ago

Thanks, now the request to get the credentials fails even though I did set the static secret:

{"message":"TURNStaticAuthSecret should be set","status_code":403}
streamer45 commented 2 years ago

Is the change reflected in config.json (PluginSettings.Plugins.com.mattermost.calls)? Bear in mind the config system is a bit brittle in the sense that it doesn't really show an error if something fails to get set due to a validation error.

ThiefMaster commented 2 years ago

Yes:

    "com.mattermost.calls": {
      "allowenablecalls": true,
      "defaultenabled": true,
      "icehostoverride": "",
      "iceservers": "stun:stun.global.calls.mattermost.com:3478",
      "iceserversconfigs": "[{\n\t\"urls\": [\"turn:turn.xxx.net:3478?transport=udp\", \"turn:turn.xxx.net:3478?transport=tcp\"]\n}]",
      "maxcallparticipants": 0,
      "rtcdserviceurl": "",
      "turncredentialsexpirationminutes": "1440",
      "turnstaticauthsecret": "xxxxxxxxxxxxxxxxxxxxxxx",
      "udpserverport": 8443
    },

I also tried restarting the Mattermost server, but same error

streamer45 commented 2 years ago

@ThiefMaster Not sure if related but I'd try changing:

"turncredentialsexpirationminutes": "1440",

to

"turncredentialsexpirationminutes": 1440,

And restart. Value there should be a number (not string) which may be causing the parsing to fail. In the mean time I'll check why it got there in the first place.

ThiefMaster commented 2 years ago

That was it! I hadn't touched the field so it was probably still the default value. After changing the value and saving the settings it's working fine now.

streamer45 commented 2 years ago

Yeah, something funny going on with non-string defaults. Will fix that, thanks for bearing with me.

ThiefMaster commented 2 years ago

When testing I noticed this in my turnserver log file:

ERROR: A peer IP 10.76.60.33 denied in the range: 10.0.0.0-10.255.255.255

That IP address is the internal IP my Mattermost pod has on OpenShift/Kubernetes (so clearly not an IP that's useful to my TURN server running elsewhere). This didn't happen during my tests last week - did something change regarding what data is sent to the TURN server?

streamer45 commented 2 years ago

Not really, we are just generating credentials on the fly now, if static ones are not set. But other than that, any communication to TURN is done entirely by your browser.

ThiefMaster commented 2 years ago

Now that I tried it again it no longer happens... so there's no "local mattermost server IP" or similar information ever passed to the TURN server?

streamer45 commented 2 years ago

Now that I tried it again it no longer happens... so there's no "local mattermost server IP" or similar information ever passed to the TURN server?

Okay, I see what you mean now. Yes, we are setting up the other end of the connection (Mattermost side) to generate TURN candidates as well. That's likely where the IP is getting from.

demogorgonz commented 2 years ago

Some good news. I have worked on a simple implementation of the expected authentication scheme. As expected, this required adding a couple of new TURN specific settings, as follows:

image

Here's a test build for you to try. If working as expected I am happy to go ahead with the changes and release them officially.

com.mattermost.calls-0.6.2-turnauth.tar.gz

Works like a charm!

Used turns in config like:

[{
"urls": [turn:turn.example.com:443]
}]

Thanks @streamer45 !

streamer45 commented 2 years ago

@justvanbloom @Asgoret @gecame

Letting you know that support for this has finally landed in the latest v0.7.0 release. Please let us know if you encounter any issue.