HTTPD output: Added support for HTTP Basic authentication #1968

Mrkvak · 2024-01-17T19:24:19Z

I want to have a publicly available streaming MPD server available from internet. However I want to protect the output with password - if not for anything else then for legal reasons (the moment someone other than me opens the stream I break the law).

This adds new optional "password" option for "httpd" output in mpd.conf. If it is specified and non-empty, it requires HTTP Basic authentication with arbitrary user and set password.

(It's been some time since I've wrote anything in C++, so please check the code if I haven't made any stupid mistakes. I've tested it and it seems to be working as intended in both cases.)

src/output/plugins/httpd/HttpdClient.cxx

src/output/plugins/httpd/HttpdOutputPlugin.cxx

src/output/plugins/httpd/HttpdClient.hxx

src/output/plugins/httpd/HttpdClient.cxx

MaxKellermann · 2024-01-17T19:37:19Z

src/output/plugins/httpd/HttpdClient.cxx

@@ -102,6 +112,15 @@ HttpdClient::HandleLine(const char *line) noexcept
 			return true;
 		}

+		if (StringEqualsCaseASCII(line, "Authorization: Basic ", 21)) {
+			auto encoded_pwd_length = strlen(line) - 21;
+			size_t debase64_size = CalculateBase64OutputSize(encoded_pwd_length);


This is redundant code - there's a DecodeBase64() overload which returns AllocatedArray. You can then cast this buffer to std::string_view (e.g. via ToStringView() from util/SpanCast.hxx) and operate on this buffer.

This is not only simpler and doesn't add redundant code, but is also faster because it reduces the two strlen() calls in your code to just one.

Fair point. Should be done, hopefully correctly.

src/output/plugins/httpd/HttpdClient.cxx

naglis · 2024-01-18T08:21:25Z

doc/mpdconf.example

@@ -283,6 +283,7 @@ input {
 #	bitrate		"128"			# do not define if quality is defined
 #	format		"44100:16:1"
 #	max_clients	"0"			# optional 0=no limit
+#	password		"secretPassword"	# protect stream with HTTP Basic authentication (user can be anything)


user can be anything

Note that username cannot contain :, since then splitting username and password would result in part of the username being considered as part of the password.

Also, according to RFC 7617:

The user-id and password MUST NOT contain any control characters (see "CTL" in Appendix B.1 of [RFC5234]).

Perhaps we should enforce some hardcoded username, like mpd, to avoid misuse?

I've explicitly written in documentation can contain all printable characters except colon (the code does not check it, though, but I don't think it should be necessary).

doc/mpdconf.example

src/output/plugins/httpd/HttpdClient.cxx

kingosticks · 2024-01-18T09:32:34Z

I'm sorry to be that guy but I really don't understand why you'd bother with the massive potential headache of supporting this "security" in mpd when it would be easier and safer for users to use an existing reverse-proxy server. They'd get something well tested with support for a variety of auth mechanisms. Am I missing something?

MaxKellermann · 2024-01-18T14:40:33Z

when it would be easier and safer for users to use an existing reverse-proxy server

The whole idea of the httpd output plugin is that MPD gives you something with batteries included; it may have fewer features than other solutions (Icecast or nginx), but you can do most things with just MPD and nothing else.

Of course, this may replicate features that are available with more complex solutions, but MPD's httpd may be the sweet spot for some users. Others want a fully-featured solution which comes with complexity.

For those people who want to make a stream available but want some protection, like an unencrypted password on an unencrypted HTTP connection - then it's fine to add this feature. It's weak, but for some people, it's just the right amount of security. Sure, somebody could sniff the password from the wire, and then they can listen to the stream. But the MPD user can't be accused of copyright violation by broadcasting copyrighted works - it's not a broadcast if it's protected, even if the protection is weak.

I understand your complaint, and I know this plugin and this PR is very imperfect, but it suits some kind of MPD users. Not me, and not you. But that's okay.

kingosticks · 2024-01-18T16:16:36Z

That's fair enough. My only further comment is that it's not obvious how secure this is trying to be. Maybe a note in the appropriate documentation would help. We obviously can't rely on GitHub comments.

MaxKellermann · 2024-01-18T16:21:43Z

Maybe a note in the appropriate documentation would help.

Agree. The presence of this feature should not pretend it's truly secure, when it cannot possibly be (because the transport is not secure).

bubbleguuum · 2024-01-20T09:33:24Z

Basic authentication is fairly secure when used in conjunction with https.
Otherwise Digest authentication is better, but more complicated to implement.

MaxKellermann · 2024-03-11T12:28:45Z

Looks like @Mrkvak is not interested in fixing the issues, and I only wasted my time with the code review.

Mrkvak · 2024-03-11T12:45:25Z

Damn, sorrry, I've totally forgot this is still open.
@MaxKellermann can you please reopen it? I'll look at it later today.

Mrkvak · 2024-03-12T21:04:16Z

@kingosticks @MaxKellermann basically explained my motivation behind this. If someone sniffs the password, they can also sniff the audio stream. My goal was to have MPD not publicly broadcasting when accessible from network/internet. Nothing more, nothing less.
I've added clear warning that the password is transmitted over unencrypted connection - it should be obvious, but I think it's still a good idea.

I think all feedback (thanks a lot for the review, by the way) was implemented, I've tested the code and it seems to be working.

MaxKellermann · 2024-03-15T17:39:03Z

Ouch, the PR branch now contains merge commits. This is a big mess that cannot be reviewed. Please fix this.

Mrkvak · 2024-03-15T17:56:52Z

Sigh... I guess that's my fault for trying to use github gui. Should be rebased on MPD/master without any pesky merge commits.

MaxKellermann · 2024-03-21T13:09:24Z

There are now commits which obviously fix mistakes in earlier commits of this PR, e.g. "working version". I don't want to read known-bad commits and find all those mistakes again, only to see later that those mistakes have been fixed already. If you find a mistake in a commit that is not yet merged, please amend the commit instead of adding a fixup commit.

Mrkvak · 2024-03-25T22:57:23Z

Squashed

src/output/plugins/httpd/HttpdClient.cxx

MaxKellermann · 2024-04-03T18:11:04Z

src/output/plugins/httpd/HttpdClient.cxx

 #include "net/SocketError.hxx"
 #include "net/UniqueSocketDescriptor.hxx"
 #include "util/SpanCast.hxx"
+#include "util/StringCompare.hxx"


Why include this header?

~~Missed leftover, sorry.~~
Actually no, it's to include StringAfterPrefixIgnoreCase

src/output/plugins/httpd/HttpdClient.cxx

MaxKellermann · 2024-04-03T18:13:37Z

src/output/plugins/httpd/HttpdClient.cxx

@@ -41,6 +50,10 @@ HttpdClient::BeginResponse() noexcept
 {
 	assert(state != State::RESPONSE);

+	if (!head_method) {


Why the head_method check?

I don't think it's necessary to password protect HEAD requests, is it?

You wrote extra code to implement a special case to allow HEAD requests. What justifies this extra complexity?
Your answer isn't good because extra complexity should be justified, not the other way round.

src/output/plugins/httpd/HttpdClient.cxx

MaxKellermann · 2024-04-03T18:14:23Z

src/output/plugins/httpd/HttpdClient.hxx

+	/**
+	 * Should we reject this request as unauthorized?
+	 */
+	bool should_reject_unauthorized = false;


This field should only exist when the feature is available

MaxKellermann · 2024-04-03T18:14:33Z

src/output/plugins/httpd/HttpdClient.cxx

@@ -169,7 +200,8 @@ HttpdClient::HttpdClient(HttpdOutput &_httpd, UniqueSocketDescriptor _fd,
 			 bool _metadata_supported)
 	:BufferedSocket(_fd.Release(), _loop),
 	 httpd(_httpd),
-	 metadata_supported(_metadata_supported)
+	 metadata_supported(_metadata_supported),
+	 provided_password{""}


What's the point of this initializer?

I don't think it's needed after the last simplification, sorry I missed it.

MaxKellermann · 2024-04-03T18:15:28Z

src/output/plugins/httpd/HttpdClient.hxx

+	/**
+	 * Provided password (using HTTP basic auth)
+	 */
+	std::string provided_password;


This field should only exist when the feature is available

MaxKellermann · 2024-04-03T18:16:02Z

src/output/plugins/httpd/HttpdInternal.hxx

+	/**
+	 * The configured password.
+	 */
+	char const *password;


This field should only exist when the feature is available and it should be const

src/output/plugins/httpd/HttpdInternal.hxx

…upport

MaxKellermann · 2024-04-08T06:24:29Z

"If you find a mistake in a commit that is not yet merged, please amend the commit instead of adding a fixup commit."

MaxKellermann requested changes Jan 17, 2024

View reviewed changes

naglis reviewed Jan 18, 2024

View reviewed changes

MaxKellermann closed this Mar 11, 2024

MaxKellermann reopened this Mar 11, 2024

Mrkvak force-pushed the feature/http-basic-auth branch 2 times, most recently from 358c240 to d1ad6d1 Compare March 15, 2024 17:51

Added support for HTTP basic authentication for streaming server

01c8d5c

Mrkvak force-pushed the feature/http-basic-auth branch from d1ad6d1 to 01c8d5c Compare March 25, 2024 22:14

MaxKellermann requested changes Apr 3, 2024

View reviewed changes

Mrkvak added 3 commits April 3, 2024 21:49

Implemented feedback on pull request

9fbf7f1

All httpd output variables are now behind HAVE_BASE64 #ifdef

710432c

Make clear in documentation httpd basic authentication needs base64 s…

6863f2d

…upport

MaxKellermann added the waiting Waiting for more information from reporter label May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTPD output: Added support for HTTP Basic authentication #1968

HTTPD output: Added support for HTTP Basic authentication #1968

Mrkvak commented Jan 17, 2024

MaxKellermann Jan 17, 2024

Mrkvak Mar 12, 2024

naglis Jan 18, 2024

Mrkvak Mar 12, 2024

kingosticks commented Jan 18, 2024

MaxKellermann commented Jan 18, 2024

kingosticks commented Jan 18, 2024

MaxKellermann commented Jan 18, 2024

bubbleguuum commented Jan 20, 2024

MaxKellermann commented Mar 11, 2024

Mrkvak commented Mar 11, 2024

Mrkvak commented Mar 12, 2024

MaxKellermann commented Mar 15, 2024

Mrkvak commented Mar 15, 2024

MaxKellermann commented Mar 21, 2024

Mrkvak commented Mar 25, 2024

MaxKellermann Apr 3, 2024

Mrkvak Apr 3, 2024 •

edited

MaxKellermann Apr 3, 2024

Mrkvak Apr 3, 2024

MaxKellermann Apr 3, 2024

MaxKellermann Apr 3, 2024

MaxKellermann Apr 3, 2024

Mrkvak Apr 3, 2024

MaxKellermann Apr 3, 2024

MaxKellermann Apr 3, 2024

MaxKellermann commented Apr 8, 2024

HTTPD output: Added support for HTTP Basic authentication #1968

Are you sure you want to change the base?

HTTPD output: Added support for HTTP Basic authentication #1968

Conversation

Mrkvak commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingosticks commented Jan 18, 2024

MaxKellermann commented Jan 18, 2024

kingosticks commented Jan 18, 2024

MaxKellermann commented Jan 18, 2024

bubbleguuum commented Jan 20, 2024

MaxKellermann commented Mar 11, 2024

Mrkvak commented Mar 11, 2024

Mrkvak commented Mar 12, 2024

MaxKellermann commented Mar 15, 2024

Mrkvak commented Mar 15, 2024

MaxKellermann commented Mar 21, 2024

Mrkvak commented Mar 25, 2024

Choose a reason for hiding this comment

Mrkvak Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKellermann commented Apr 8, 2024

Mrkvak Apr 3, 2024 •

edited