Skip to content

feat: add encryption option for broadcasting #13285

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.

Sign up for GitHub

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

Open
acolombier wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: add encryption option for broadcasting #13285

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97e721c
feat: add encryption option for broadcasting
acolombier Mar 31, 2024
8bef931
fixup! feat: add encryption option for broadcasting
acolombier Jan 19, 2025
823ab0e
fixup! feat: add encryption option for broadcasting
acolombier Jun 28, 2025
968b505
Merge remote-tracking branch 'upstream/main' into feat/icecast-tls-su…
acolombier Mar 1, 2026
952b84d
fixup! feat: add encryption option for broadcasting
acolombier Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 42 additions & 11 deletions src/engine/sidechain/shoutconnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,6 @@ ShoutConnection::ShoutConnection(BroadcastProfilePtr profile,
errorDialog(tr("Error setting non-blocking mode:"),
shout_get_error(m_pShout));
}

#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

@daschuer daschuer Jan 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should keep the reference to the bug.

@daschuer daschuer Jun 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is still pending.

@acolombier acolombier Mar 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

if (shout_set_tls(m_pShout, SHOUT_TLS_DISABLED) != SHOUTERR_SUCCESS) {
errorDialog(tr("Error setting tls mode:"),
shout_get_error(m_pShout));
}
#endif
}

ShoutConnection::~ShoutConnection() {
Expand Down Expand Up @@ -237,6 +227,24 @@ void ShoutConnection::updateFromPreferences() {
serverUrl.setUserName(login);
}

#ifdef SHOUT_TLS
BroadcastProfile::EncryptionMode encryptionMode = m_pProfile->getEncryptionMode();

int result;
switch (encryptionMode) {
case BroadcastProfile::EncryptionMode::Disabled:
result = shout_set_tls(m_pShout, SHOUT_TLS_DISABLED);
break;
default: // Required
result = shout_set_tls(m_pShout, SHOUT_TLS_AUTO_NO_PLAIN);
break;
}

@daschuer daschuer Jan 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@acolombier acolombier Jan 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@JoergAtGithub JoergAtGithub Jun 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@daschuer daschuer Mar 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@acolombier acolombier Mar 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

@acolombier acolombier Mar 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also #13285 (comment)

if (result != SHOUTERR_SUCCESS) {
errorDialog(tr("Error setting TLS mode:"),
shout_get_error(m_pShout));
}
#endif

kLogger.debug() << "Using server URL:" << serverUrl;

QByteArray baPassword = m_pProfile->getPassword().toLatin1();
Expand Down Expand Up @@ -603,6 +611,29 @@ bool ShoutConnection::processConnect() {
kLogger.warning()
<< "processConnect() socket error."
<< "Is socket already in use?";
} else if (m_iShoutStatus == SHOUTERR_NOLOGIN) {
m_lastErrorStr = "Invalid login details";
kLogger.warning()
<< "processConnect() failed with no invalid or missing login details:"
<< m_iShoutStatus << shout_get_error(m_pShout);
#ifdef SHOUT_TLS
} else if (m_iShoutStatus == SHOUTERR_NOTLS) {
DEBUG_ASSERT(m_pProfile &&
m_pProfile->getEncryptionMode() ==
BroadcastProfile::EncryptionMode::Required);
m_lastErrorStr = "The server doesn't not provide the required TLS encryption";
kLogger.warning()
<< "processConnect() failed with no valid TLS:"
<< m_iShoutStatus << shout_get_error(m_pShout);
} else if (m_iShoutStatus == SHOUTERR_TLSBADCERT) {
DEBUG_ASSERT(m_pProfile &&
m_pProfile->getEncryptionMode() !=
BroadcastProfile::EncryptionMode::Disabled);
m_lastErrorStr = "The server TLS certificate couldn't be verified";
kLogger.warning()
<< "processConnect() failed with a bad TLS certificate:"
<< m_iShoutStatus << shout_get_error(m_pShout);
#endif
} else if (timeout >= kConnectRetries) {
// Not translated, because shout_get_error() returns also English only
m_lastErrorStr = QStringLiteral("Connection establishment time-out");
Expand Down Expand Up @@ -987,7 +1018,7 @@ void ShoutConnection::run() {
errorDialog(tr("Can't connect to streaming server"),
m_lastErrorStr + "\n\n" +
tr("Please check your connection to the Internet and "
"verify that your username and password are "
"verify that your username, password and encryption mode are "
"correct."));
return;
}
Expand Down
14 changes: 14 additions & 0 deletions src/preferences/broadcastprofile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ constexpr const char* kProfileName = "ProfileName";
constexpr const char* kReconnectFirstDelay = "ReconnectFirstDelay";
constexpr const char* kReconnectPeriod = "ReconnectPeriod";
constexpr const char* kServertype = "Servertype";
constexpr const char* kEncryptionMode = "EncryptionMode";
constexpr const char* kStreamDesc = "StreamDesc";
constexpr const char* kStreamGenre = "StreamGenre";
constexpr const char* kStreamName = "StreamName";
Expand Down Expand Up @@ -241,6 +242,7 @@ void BroadcastProfile::copyValuesTo(BroadcastProfilePtr other) {
other->setServertype(this->getServertype());
other->setLogin(this->getLogin());
other->setPassword(this->getPassword());
other->setEncryptionMode(this->getEncryptionMode());

other->setEnableReconnect(this->getEnableReconnect());
other->setReconnectPeriod(this->getReconnectPeriod());
Expand Down Expand Up @@ -277,6 +279,7 @@ void BroadcastProfile::copyValuesTo(BroadcastProfilePtr other) {

void BroadcastProfile::adoptDefaultValues() {
m_secureCredentials = false;
m_encryptionMode = EncryptionMode::Required;
m_enabled = false;

m_host = QString();
Expand Down Expand Up @@ -348,6 +351,13 @@ bool BroadcastProfile::loadValues(const QString& filename) {
m_host = selectCleanNodeString(doc, kHost, &fixedStrings);
m_port = XmlParse::selectNodeInt(doc, kPort);
m_serverType = selectCleanNodeString(doc, kServertype, &fixedStrings);
switch (static_cast<EncryptionMode>(XmlParse::selectNodeInt(doc, kEncryptionMode))) {
case EncryptionMode::Required:
m_encryptionMode = EncryptionMode::Required;
break;
default:

@daschuer daschuer Jan 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

@acolombier acolombier Jan 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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?

@daschuer daschuer Jan 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@acolombier acolombier Jun 28, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

m_encryptionMode = EncryptionMode::Disabled;
}

m_login = selectCleanNodeString(doc, kLogin, &fixedStrings);
if (m_secureCredentials) {
Expand Down Expand Up @@ -433,6 +443,10 @@ bool BroadcastProfile::save(const QString& filename) {
XmlParse::addElement(doc, docRoot, kHost, m_host);
XmlParse::addElement(doc, docRoot, kPort, QString::number(m_port));
XmlParse::addElement(doc, docRoot, kServertype, m_serverType);
XmlParse::addElement(doc,
docRoot,
kEncryptionMode,
QString::number(static_cast<int>(m_encryptionMode)));

XmlParse::addElement(doc, docRoot, kLogin, m_login);
if (m_secureCredentials) {
Expand Down
13 changes: 13 additions & 0 deletions src/preferences/broadcastprofile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ class BroadcastProfile : public QObject {
STATUS_CONNECTED = 2, // On Air
STATUS_FAILURE = 3 // Happens when disconnected by an error
};
enum class EncryptionMode {
Required,
Disabled
};
Q_ENUM(EncryptionMode);

explicit BroadcastProfile(const QString& profileName,
QObject* parent = nullptr);
Expand Down Expand Up @@ -73,6 +78,13 @@ class BroadcastProfile : public QObject {
QString getPassword() const;
void setPassword(const QString& value);

EncryptionMode getEncryptionMode() const {
return m_encryptionMode;
}
void setEncryptionMode(EncryptionMode value) {
m_encryptionMode = value;
}

bool getEnableReconnect() const;
void setEnableReconnect(bool value);

Expand Down Expand Up @@ -164,6 +176,7 @@ class BroadcastProfile : public QObject {
void errorDialog(const QString& text, const QString& detailedError);

bool m_secureCredentials;
EncryptionMode m_encryptionMode;

QString m_filename;

Expand Down
12 changes: 12 additions & 0 deletions src/preferences/dialog/dlgprefbroadcast.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@ DlgPrefBroadcast::DlgPrefBroadcast(QWidget* parent,
comboBoxEncodingChannels->addItem(tr("Stereo"),
static_cast<int>(EncoderSettings::ChannelMode::STEREO));

// Encryption mode combobox
encryptionModeComboBox->addItem(tr("Required"),
static_cast<int>(BroadcastProfile::EncryptionMode::Required));
encryptionModeComboBox->addItem(tr("Disabled"),
static_cast<int>(BroadcastProfile::EncryptionMode::Disabled));
Comment thread
acolombier marked this conversation as resolved.

connect(checkBoxEnableReconnect,
#if QT_VERSION >= QT_VERSION_CHECK(6, 7, 0)
&QCheckBox::checkStateChanged,
Expand Down Expand Up @@ -463,6 +469,10 @@ void DlgPrefBroadcast::getValuesFromProfile(BroadcastProfilePtr profile) {
// Password
password->setText(profile->getPassword());

// Encryption mode
encryptionModeComboBox->setCurrentIndex(encryptionModeComboBox->findData(
static_cast<int>(profile->getEncryptionMode())));

// Enable automatic reconnect
bool enableReconnect = profile->getEnableReconnect();
checkBoxEnableReconnect->setChecked(enableReconnect);
Expand Down Expand Up @@ -577,6 +587,8 @@ void DlgPrefBroadcast::setValuesToProfile(BroadcastProfilePtr profile) {
profile->setPort(port->text().toInt());
profile->setLogin(login->text());
profile->setPassword(password->text());
profile->setEncryptionMode(static_cast<BroadcastProfile::EncryptionMode>(
encryptionModeComboBox->currentData().toInt()));
profile->setEnableReconnect(checkBoxEnableReconnect->isChecked());
profile->setReconnectFirstDelay(spinBoxFirstDelay->value());
profile->setReconnectPeriod(spinBoxReconnectPeriod->value());
Expand Down
55 changes: 44 additions & 11 deletions src/preferences/dialog/dlgprefbroadcastdlg.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<x>0</x>
<y>0</y>
<width>715</width>
<height>1012</height>
<height>1248</height>
</rect>
</property>
<property name="windowTitle">
Expand Down Expand Up @@ -158,7 +158,16 @@
<item>
<widget class="QWidget" name="widgetReconnectControls" native="true">
<layout class="QGridLayout" name="gridLayout_5">
<property name="margin">
<property name="leftMargin">
<number>0</number>
</property>
<property name="topMargin">
<number>0</number>
</property>
<property name="rightMargin">
<number>0</number>
</property>
<property name="bottomMargin">
<number>0</number>
</property>
<item row="0" column="0">
Expand Down Expand Up @@ -598,9 +607,9 @@
<string notr="true">&lt;!DOCTYPE HTML PUBLIC &quot;-//W3C//DTD HTML 4.0//EN&quot; &quot;http://www.w3.org/TR/REC-html40/strict.dtd&quot;&gt;
&lt;html&gt;&lt;head&gt;&lt;meta name=&quot;qrichtext&quot; content=&quot;1&quot; /&gt;&lt;style type=&quot;text/css&quot;&gt;
p, li { white-space: pre-wrap; }
&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;

@daschuer daschuer Mar 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be an unrelated change.

&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';&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'; font-size:13pt;&quot;&gt;&lt;br /&gt;&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>
</property>
</widget>
</item>
Expand Down Expand Up @@ -674,6 +683,26 @@ p, li { white-space: pre-wrap; }
<string>Server connection</string>
</property>
<layout class="QGridLayout">
<item row="3" column="0">
<layout class="QHBoxLayout" name="horizontalLayout_3">
<item>
<widget class="QLabel" name="label_21">
<property name="sizePolicy">
<sizepolicy hsizetype="Preferred" vsizetype="Preferred">
<horstretch>0</horstretch>
<verstretch>0</verstretch>
</sizepolicy>
</property>
<property name="text">
<string>Encryption</string>
</property>
</widget>
</item>
<item>
<widget class="QComboBox" name="encryptionModeComboBox"/>
</item>
</layout>
</item>
<item row="0" column="0">
<layout class="QGridLayout" name="gridLayoutSC">
<item row="0" column="0">
Expand Down Expand Up @@ -787,7 +816,16 @@ p, li { white-space: pre-wrap; }
<item row="1" column="0">
<widget class="QWidget" name="groupPasswordStorage" native="true">
<layout class="QHBoxLayout" name="horizontalLayout_2">
<property name="margin">
<property name="leftMargin">
<number>0</number>
</property>
<property name="topMargin">
<number>0</number>
</property>
<property name="rightMargin">
<number>0</number>
</property>
<property name="bottomMargin">
<number>0</number>
</property>
<item>
Expand Down Expand Up @@ -869,25 +907,21 @@ p, li { white-space: pre-wrap; }
<tabstop>btnDisconnectAll</tabstop>
<tabstop>btnCreateConnection</tabstop>
<tabstop>connectOnApply</tabstop>

<tabstop>comboBoxServerType</tabstop>
<tabstop>mountpoint</tabstop>
<tabstop>host</tabstop>
<tabstop>port</tabstop>
<tabstop>login</tabstop>
<tabstop>password</tabstop>
<tabstop>rbPasswordCleartext</tabstop>

<tabstop>checkBoxEnableReconnect</tabstop>
<tabstop>spinBoxFirstDelay</tabstop>
<tabstop>spinBoxReconnectPeriod</tabstop>
<tabstop>checkBoxLimitReconnects</tabstop>
<tabstop>spinBoxMaximumRetries</tabstop>

<tabstop>comboBoxEncodingBitrate</tabstop>
<tabstop>comboBoxEncodingFormat</tabstop>
<tabstop>comboBoxEncodingChannels</tabstop>

<tabstop>stream_public</tabstop>
<tabstop>stream_name</tabstop>
<tabstop>stream_website</tabstop>
Expand All @@ -896,7 +930,6 @@ p, li { white-space: pre-wrap; }
<tabstop>stream_IRC</tabstop>
<tabstop>stream_AIM</tabstop>
<tabstop>stream_ICQ</tabstop>

<tabstop>metadata_format</tabstop>
<tabstop>enableCustomMetadata</tabstop>
<tabstop>custom_artist</tabstop>
Expand Down
4 changes: 4 additions & 0 deletions src/test/broadcastprofile_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ TEST(BroadcastProfileTest, SetGetValues) {
profile.setHost(hostname);
ASSERT_TRUE(profile.getHost() == hostname);

BroadcastProfile::EncryptionMode mode = BroadcastProfile::EncryptionMode::Required;
profile.setEncryptionMode(mode);
ASSERT_TRUE(profile.getEncryptionMode() == mode);

int port = 1238;
profile.setPort(port);
ASSERT_EQ(profile.getPort(), port);
Expand Down
Loading