Provides initial TLS support to aio-talk

[rseabra]

Rebased my commits to e requested DC0.
Adds initial TLS support to aio-talk.
TODO: provide way to configure allowed algorithms.

• Fix aio-talk should support eturnal's tls transport #7595

[Zoey2936]

Since a review was requested from me, I want to give some general thoughts on this:

From my understanding this implementation is done for people who use this container independent of AIO.
For me this is fine, but is this wanted?
Like the request to add an env to change the file name of the remote borg binary was denied, because of added maintenance required. (#6340 / #6687)
But is this not the same in this case? I would say “supporting” independent use of single AIO containers would require even more work. Additionally, this doesn't even add a benefit to AIO itself in this case.

In case there are future plans to integrate this fully in AIO, then ignore my words above, but it would be complicated to transfer the certs from caddy to talk, also talk would need to be restarted after cert renewals and for reverse proxy setups this would only work with self-signed certs.

I will look later in the implementation.

[rseabra]

Hi @Zoey2936 ,

Regardless of being used alone or integrated in the "all in one" package, TLS needs to be supported because turn server is exposed to Internet.

Having no support for TLS TURN, should be viewed as a vulnerability, (as it is by SOC teams, I've been there).

In this PR I've been extremely careful to:

1. not break the current usage of the container
2. improve it by adding TLS support
3. adding missing documentation

I'm still going to add some variables for algorithm fine-tunning (as enterprise usage usually needs that), as without that I can't use it where I need it used, but that will come at a later PR.

Yes, there are challenges with sharing certificates, for the time being a good solution is to map them as volume until a better (and integrated) solution comes along.

Still, even if an integrated solution is found, it would be extremely useful to still allow independent usage of the container, as it simplifies a lot the process for other kinds of installations.

I hope I've clarified enough, and I am available to talk about any other issues one may find.

Best regards,
Rui Seabra

[pabzm]

@rseabra Thank you for the contribution! To be able to properly review this I'd like to ask you to split the technical topics into separate PRs, or at least in separate commits.

Also, could you please explain why you made the signalling port, timeouts, etc. configurable? It looks like you have a certain use case, of which we'd like to know.

And please remove the quadlet-related details. We don't include documentation on podman-based setups as of know, and this would not be a good place to start it, from my point of view.

[rseabra]

Hi @pabzm ,

@rseabra Thank you for the contribution!

YW, I just had an itch that needed scratching! :)

To be able to properly review this I'd like to ask you to split the technical topics into separate PRs, or at least in separate commits.

Oh, I just didn't have the time to checkout the docs to learn how to fine-tune the criptographic algorithms, specially in eturnal, and thought that would also only be worth it if TLS get's to be supported.

Also, could you please explain why you made the signalling port, timeouts, etc. configurable? It looks like you have a certain use case, of which we'd like to know.

In a word: completeness. Those parameter sounded like potentially relevant performance parameters that, should the need come up, it did no harm to add support for it.

After all, it's just setting up variables with default values rather than having them hard-coded.

And please remove the quadlet-related details. We don't include documentation on podman-based setups as of know, and this would not be a good place to start it, from my point of view.

Ok. Done.

Let me know what else. :)

Best regards,
Rui

[pabzm]

Please remove the timeout- and replay-port-related parts from this PR and post both change-sets as individual PRs instead. We'd like to keep these topics apart, since we're not sure if we would like to merge them, yet.

And we want to avoid behavioural changes if possible, thus my comment on $TALK_PORT.

Additionally please make sure that all commits have a signed-off-by-line. Thank you!

[pabzm]

This is a breaking change in behaviour, which we would like to avoid.

Please revert $TALK_PORT to being a required variable without a default.

$TALK_TLS_PORT should not be required (as that would be a breaking change, too), but it should not have a default value, neither. Instead it shouldn't be activated if unset.

[rseabra]

This is not a breaking change, it merely makes it unnecessary to specify the default port.

As you can see, the standard port is the same accepted argument and inside the start.sh script a more clear variable name called STUN_PORT is used.

It is set to either TALK_PORT as provided, or the default port, and then it's STUN_PORT that's actually used.

This doesn't break anything, it just makes it more comfortable to use: you only need the parameter if you're not using the default port.

[pabzm]

Oh, and the actual documentation for this container is living here and would need to be updated, too (in case you might be able to do that): https://github.com/nextcloud/spreed/blob/main/docs/quick-install.md

[rseabra]

Hi @pabzm,

I really don't understand your review, I'd really like to accommodate but it would help me a lot to swallow the pill if I could understand the reasoning and I don't get your objections, at all.

1. defining the default port value for TALK_PORT doesn't break anything, it actually fixes breaking if the default port is not specified

I don't see why one wouldn't want to have default values for the ports, you're just making it mandatory in deployment time to have an explicitly defined variable when it could be smarter (and more comfortable to deploy)

This doesn't break anything, it merely let's it work with the default port if the standard port is not specified.

2. TALK_TLS_PORT is not a required parameter, the actual check is the definition of the certificate paths

The same thing goes for the TLS port, which not only is not required, it will not be enabled without providing a certificate with private and public keys, which is the actual check as done here: 3343131#diff-2251ec8a5310d7028953402eed81f74318fd30a3f3e0b7d3775153fb077ca47eR54

There is no point in trying to use the TLS port if no certificate is given, which is why that is the real check.

3. the timeouts are already there, they're default values, what is the problem with giving the possibility to define them?

4. quick install is a quick install document, the full gory details of configuration should not be in a quick install document but in a component's document

That document specifically even says...

«This guide explains the steps and instructions required to install the High-performance backend using Docker and how to quickly configure the basics to get started.»

Can you please reconsider these points? I've been very careful not to break anything and just make deployment easier and more flexible, as breaking anything is what wouldn't make any sense.

Thanks,
Rui