MPD
MPD copied to clipboard
MusicPlayerDaemon
HTTPD output: Added support for HTTP Basic authentication
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.)
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?
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.
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.
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).
Basic authentication is fairly secure when used in conjunction with https. Otherwise Digest authentication is better, but more complicated to implement.
Looks like @Mrkvak is not interested in fixing the issues, and I only wasted my time with the code review.
Damn, sorrry, I've totally forgot this is still open. @MaxKellermann can you please reopen it? I'll look at it later today.
@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.
Ouch, the PR branch now contains merge commits. This is a big mess that cannot be reviewed. Please fix this.
Sigh... I guess that's my fault for trying to use github gui. Should be rebased on MPD/master without any pesky merge commits.
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.
"If you find a mistake in a commit that is not yet merged, please amend the commit instead of adding a fixup commit."