feat: add encryption option for broadcasting by acolombier · Pull Request #13285 · mixxxdj/mixxx

acolombier · 2024-05-25T22:01:31Z

This attempts to address #8301 and #8706

Couple of notes:

Behaviour is pretty random and unstable with the current libshout version, but far more stable and expectable with [Don't merge] libshout idjc 2.4.6 #4723
Tested with an Icecast server 2.4.4
SHOUT_TLS seems hardcoded to 1 so I was tempted to remove the flag entirely
This touches on section of the code quite off our current C++ standard, but I've decided to stay as aligned as possible with the rest of the file style when possible

JoergAtGithub · 2025-01-04T16:02:01Z

There is a user confirmation, that this PR works: #8301 (comment)

daschuer · 2025-01-04T17:17:26Z

+    default: // Required
+        result = shout_set_tls(m_pShout, SHOUT_TLS_AUTO_NO_PLAIN);
+        break;
+    }


We have these values:

#define SHOUT_TLS_DISABLED ( 0) /* Do not use TLS at all */ #define SHOUT_TLS_AUTO ( 1) /* Autodetect which TLS mode to use if any */ #define SHOUT_TLS_AUTO_NO_PLAIN ( 2) /* Like SHOUT_TLS_AUTO_NO_PLAIN but does not allow plain connections */ #define SHOUT_TLS_RFC2818 ( 11) /* Use TLS for transport layer like HTTPS [RFC2818] does. */ #define SHOUT_TLS_RFC2817 ( 12) /* Use TLS via HTTP Upgrade:-header [RFC2817]. */

Since SHOUT_TLS_AUTO seems to be broken, we should not offer it. How is the situation with the explicit mode selection, does it work? How does "Auto" work?

Is it still broken? It seems to be working fine on 2.4.6.

Explicit mode works well as well, but it require the user to know which RFC/implementation the server is using (startTLS vs full TLS), which depends of how the broadcast server is configured. Reading #8301, I don't think we want the user to have to know/care about this level of details

During my test, I tried edge/ingress level TLS (RFC2818), and container level (RFC2817), explicit option worked well as long as you picked the right one. Auto was able to detect the right setup nicely.

@daschuer Are you sure, that this is still broken in 2.4.6 ?

I don't have tested this. We can merge this PR once we have version guards for 2.4.6.
I am really concerned for regressions form this PR so let's move forward with this safety net.
Once a version guard shout_version() is in place we can merge this.

I am really concerned for regressions form this PR so let's move forward with this safety net.

There is no regression risk in this PR.
Before, TLS was explicitly disabled.
Now it can be explicitly enabled or disabled.

Only risk is for a user to experience regression when enabling TLS, but they have the option to get back to TLS being explicitly disabled.

Are you asking to force user with <2.4.6 to stay stuck with TLS disabled?

Also #13285 (comment)

daschuer · 2025-01-04T17:18:07Z

-#ifdef SHOUT_TLS
-    // Libshout defaults to SHOUT_TLS_AUTO if build with SHOUT_TLS
-    // Sometimes autodetection fails, resulting into no metadata send
-    // https://github.com/mixxxdj/mixxx/issues/9599


We should keep the reference to the bug.

This is still pending.

This comment is now irrelevant since we've agreed not to use SHOUT_TLS_AUTO

daschuer · 2025-01-04T17:32:18Z

+    case EncryptionMode::Disabled:
+        m_encryptionMode = EncryptionMode::Disabled;
+        break;
+    default:


What is the migration path? I think we should keep "Disabled" after upgrade. We may consider to keep the "not set" state that we have the chance to ask the user in pop up box in a later PR (or in this if you like)

I think that's a fair approach, not sure how to deal with the prompt tho. Do we want to consider adding a version attribute on the BroadcastProfile document root?

I don't mind, my idea was to just check if this new option exists or not.

Updated - the migration path will now default to disable, which is the current behaviour as we don't support TLS in 2.6

daschuer · 2025-06-28T23:41:19Z

Behaviour is pretty random and unstable with the current libshout version, but far more stable and expectable with #4723

It sound like we should enable the code only for libshout-idjc 2.4.6 and above, right?

acolombier · 2025-06-29T18:30:17Z

Yes, I consider this PR depending on libshout-idjc 2.4.6, and on the TLS RFC being correctly detected on this version

daschuer · 2025-06-29T18:44:34Z

OK, so you need to check shout_version() ...

daschuer · 2026-02-15T10:31:50Z

Did you consider to add the version check to make this mergeable dependently?

acolombier · 2026-02-15T18:24:13Z

Yes, and I consider this a dead end.
As you right fully pointed in here, there is likely a small minority that would be able to benefit this. I myself use purposefully a virtual environment on 24.04 to ensure we support our minimum requirements (tho I run a much newer system, on why I also build Mixxx for my leisure). This means the CI tests (and my development setup) wouldn't be able to test and guarantee the behaviour.
As you also rightfully point in the comment linkled above, this feature would be made available to 2.7, which will bump support to 26.04. (2.7 will be released at least 6 months from 2.6, which is still not GA)

daschuer · 2026-02-16T01:17:01Z

I have not understand what you propose.

Why not add the version guard and have this PR from our to do list. Main user with the latest libshout-idjc version can benefit from it.

acolombier · 2026-02-16T23:41:46Z

Main user with the latest libshout-idjc version can benefit from it.

Please tell me how to do that on Ubuntu 24.04.

daschuer · 2026-02-17T07:29:50Z

With the version guard an Ubuntu 24.04 user can still build main. In short Ubuntu 25.04 LTS will be released and users will automatically benefit from this feature here.

By the way, can you explain the issue you had:

Behaviour is pretty random and unstable with the current libshout version.

Which version did you test? What was the faulty behaviour?

daschuer · 2026-02-17T07:35:02Z

It is probably this:

libshout 2.4.2 (20190429)
* Rewrote TLS handling code and statemachine (#2244, #2298, #2301, #2303).
* Support OPTIONS for RFC 2817 TLS mode.
* Allow manual inspection of server's TLS certificate.

acolombier · 2026-02-17T09:06:12Z

In short Ubuntu 25.04 LTS will be released and users will automatically benefit from this feature here.

This is exactly my point. main a.k.a 2.7 will be released with the minimum requirement of 26.04. SO instead of releasing a disable feature, let's make sure it is enabled during the alpha (and eventually the beta) to gather as much feedback as possible.

Which version did you test? What was the faulty behaviour?

This was 18 months ago, but I believe this would have been 2.4.1. Rewrote TLS handling code and statemachine sounds familiar to what would have been the issue, but I don't recall exactly what was the symptoms.

daschuer · 2026-02-17T12:27:15Z

Ah ok. So we have a common agreement to get this one merged as soon as possible.

If you add the version guard now, I consider this ready for merge. A final test is missing if cause.

…pport

daschuer

Is there a reason to not add a version guard? I think this will remove the deadlock with this PR.

daschuer · 2026-03-01T19:18:04Z

+    default: // Required
+        result = shout_set_tls(m_pShout, SHOUT_TLS_AUTO_NO_PLAIN);
+        break;
+    }


I don't have tested this. We can merge this PR once we have version guards for 2.4.6.
I am really concerned for regressions form this PR so let's move forward with this safety net.
Once a version guard shout_version() is in place we can merge this.

daschuer · 2026-03-01T19:19:03Z

-&lt;/style&gt;&lt;/head&gt;&lt;body style=&quot; font-family:'.SF NS Text'; font-size:13pt; font-weight:400; font-style:normal;&quot;&gt;
-&lt;p style=&quot; margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;&quot;&gt;&lt;span style=&quot; font-family:'Ubuntu'; font-size:10pt;&quot;&gt;  &lt;/span&gt;&lt;/p&gt;
-&lt;p style=&quot;-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; font-family:'Lucida Grande';&quot;&gt;&lt;br /&gt;&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>
+&lt;/style&gt;&lt;/head&gt;&lt;body style=&quot; font-family:'Fira Sans Semi-Light'; font-size:10pt; font-weight:400; font-style:normal;&quot;&gt;


This seems to be an unrelated change.

acolombier · 2026-03-01T21:09:59Z

Is there a reason to not add a version guard?

Yes - as per our policy, we will drop support for Ubuntu 24.04 in less than two months, so this extra work, as well as extra branching risk which is expected to even be removed before the beta starts.

I think this will remove the deadlock with this PR.

IMO, removing the deadlock with this solution, means keeping the deadlock in #4723 so this will leave a lot of user waiting for #8301 behind (many user will still be on 24.04 and won't migrate till summer Point Release or even more, some are still on 22.04 - e.g PopOS pre-cosmic).
While we won't be supporting them officially, we will likely provide a backport like we did for 22.04 and leaving them without TLS connection in 2026 is not serious.

daschuer · 2026-03-01T21:28:45Z

All this discussion is also extra work. Isn't it only a runtime call in addition to #ifdef SHOUT_TLS we have already?

While we won't be supporting them officially, we will likely provide a backport like we did for 22.04 and leaving them without TLS connection in 2026 is not serious.

Not TLS due to the version guard?

With runtime version guard:

We have reliable broadcasting independent from the users libshout-idjc version
We can collect real world experiences experiences with rolling Linux distros.
We can merge this independent of the decision we take in [Don't merge] libshout idjc 2.4.6 #4723

Without the version guard:

We need to wait.
Users on old unsupported distros may suffer regressions

acolombier · 2026-03-01T22:09:57Z

All this discussion is also extra work.

Agreed. The best would be to merge #4723 then this PR.

Not TLS due to the version guard?

Since we refuse to update our already vendored deps, TLS won't be available. This also means we would need to keep the branch in place in our code.

We have reliable broadcasting independent from the users libshout-idjc version

Calling unencrypted traffic - including broadcaster credentials - a "reliable broadcasting" experience is quite controversial. I certainly do not register myself, and wouldn't want to broadcast with Mixxx.
Perhaps we could ask users on the issue if they would prefer to see their broadcasting credential compromised or if they prefer to take some risk during an apha, then beta period, which will likely last month and where we will be able to fix forward.

Without the version guard:
* We need to wait.

With this guard, we give another reason not to update this version, which has been live for years and hasn't show any regression.

Here is my proposal:

Either we keep the deadlock till 26.04 is out and the min version libshout version is 2.4.6
We ask the community if they would prefer to have an alpha version with TLS - and thus a safe way to broadcast - but the risk of an alpha, which is instability risk

Happy to quickly get a post ready on Discourse (maybe we even have survey enabled?) to remove the deadlock.
Curious to hear @mixxxdj/developers 's take here as this feature seems to be well expected?

daschuer · 2026-03-01T22:34:55Z

Ah got it. You think our vendored version has no TLS. This is not true. It has. That's why the compile time version guard is not sufficient.

The compile time version guard only detects which headers have been used during compiling. This may not match the *.so the user has. That's why we need the runtime version guard as well, in any case, independent from the rest of the update strategy.

acolombier · 2026-03-01T22:58:35Z

You think our vendored version has no TLS. This is not true. It has.

It does, but it suffers from an issue that have lead us to disable it.
As I said in the top of this PR, I have tested this feature with the currently shipped version (2.4.1), and it does behave randomly, particularly fails after a few connecti/disconnect cycle, which I assume are the original symptoms leading to force disable TLS.

This may not match the *.so the user has.

Appreciate I often get confused with our CMakeFile, but in this case, I believe it is clear that, in some case that we have decided due to two issues, we ship our vendored fork of the lib
. That is both the headers, which impact compilation, and the forked library (not sure if it is dynamically linked into a *.so or statically linked FWIW)

The whole reason I am pushing for vendoring 2.4.6 is that we can simplify that to:

User have a version greater to 2.4.6, great then can use that (no need for version guard)
User have version less than 2.4.1, we give them our vendored version

This means that our theoretical 24.04 backport will be shipped with the vendor lib, but the 26.04 won't, tho will have the version constrain in the .deb manifest.

daschuer · 2026-03-02T09:14:45Z

I think your latest post is unrelated to the runtime version guard. Let me explain.

We have here:

#ifdef SHOUT_TLS

This does check if the header used at compile time has SHOUT_TLS defined. Unfortunately this is not enough for a working encryption support, because we need in addition a certain version. Something similar to this pseudo code.

if (shout_version() >= 2.4.6)

This is my only demand here, because is behaves rock solid under all conditions with only a minimal change.
Why not just add this her and merge?

acolombier · 2026-03-02T12:07:10Z

Thanks, I understood what you ask and I know how to implement it. But this does not address my concern.

Again. doing this means that this feature would result in a no-op, except for user building Mixxx themselve with libshout > 2.4.6.
Non technically confident user would be yet again left behind, and wouldn't even be able to install the .deb build artifact since it is built with Ubuntu 24.04, which would use our vendored version.

Let's please stop discussing about bringing any guards to this PR.

daschuer · 2026-03-02T12:30:42Z

Your concerns are a compete different topic.

We need the guard in any case if we want to release before all < 2.4.6 have disappeared. This is because distros sometime rip of libraries in our lib folder. It is just a safety guard that died not hard IMHO her. The no opt behaviour is desired for libshout >= 2.4.6 If users are left behind or not is a business decision. We should be first technically correct here.

feat: add encryption option for broadcasting

97e721c

github-actions Bot added engine ui code quality preferences labels May 25, 2024

JoergAtGithub added the needs review label Jun 17, 2024

acolombier mentioned this pull request Jun 19, 2024

Support SSL/TLS for streaming #8301

Open

acolombier added this to the 2.6-beta milestone Sep 19, 2024

daschuer mentioned this pull request Sep 22, 2024

~WORKAROUND!~Mixxx Not pushing Metadata to Shoutcast 1.x+2.x server (windows 10) #13681

Open

JoergAtGithub requested a review from daschuer January 4, 2025 16:04

daschuer requested changes Jan 4, 2025

View reviewed changes

fixup! feat: add encryption option for broadcasting

8bef931

acolombier marked this pull request as draft February 17, 2025 23:54

acolombier removed this from the 2.6-beta milestone Feb 23, 2025

acolombier removed the needs review label Feb 27, 2025

fixup! feat: add encryption option for broadcasting

823ab0e

acolombier marked this pull request as ready for review June 28, 2025 21:43

acolombier requested a review from daschuer June 28, 2025 21:43

acolombier mentioned this pull request Feb 15, 2026

[Don't merge] libshout idjc 2.4.6 #4723

Draft

Merge remote-tracking branch 'upstream/main' into feat/icecast-tls-su…

968b505

…pport

acolombier linked an issue Mar 1, 2026 that may be closed by this pull request

Support SSL/TLS for streaming #8301

Open

daschuer requested changes Mar 1, 2026

View reviewed changes

fixup! feat: add encryption option for broadcasting

952b84d

acolombier mentioned this pull request Jul 3, 2026

Icecast 2 broadcasting works on Windows 11, but track metadata is never sent to Icecast #16311

Open

Uh oh!

Uh oh!

Conversation

acolombier commented May 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoergAtGithub commented Jan 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschuer commented Jun 28, 2025

Uh oh!

acolombier commented Jun 29, 2025

Uh oh!

daschuer commented Jun 29, 2025

Uh oh!

daschuer commented Feb 15, 2026

Uh oh!

acolombier commented Feb 15, 2026

Uh oh!

daschuer commented Feb 16, 2026

Uh oh!

acolombier commented Feb 16, 2026

Uh oh!

daschuer commented Feb 17, 2026

Uh oh!

daschuer commented Feb 17, 2026

Uh oh!

acolombier commented Feb 17, 2026

Uh oh!

daschuer commented Feb 17, 2026

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acolombier commented Mar 1, 2026

Uh oh!

daschuer commented Mar 1, 2026

Uh oh!

acolombier commented Mar 1, 2026

Uh oh!

daschuer commented Mar 1, 2026

Uh oh!

acolombier commented Mar 1, 2026

Uh oh!

daschuer commented Mar 2, 2026

Uh oh!

acolombier commented Mar 2, 2026

Uh oh!

daschuer commented Mar 2, 2026

Uh oh!

acolombier commented May 25, 2024 •

edited

Loading