search

notandy / ympd

Standalone MPD Web GUI written in C, utilizing Websockets and Bootstrap/JS
http://www.ympd.org
GNU General Public License v2.0
515 stars 143 forks source link

Support for password-protected mpd #105

Closed magnatlu closed 6 years ago

anthraxx commented 8 years ago

currently the systemd file and ympd.default does not reflect this change, maybe it should also be included. @magnatlu could you also add the changes to the systemd stuff and environment file? :yum:

SuperBFG7 commented 8 years ago

I'm not sure this makes sense: supplying passwords on the command line is bad practice, since anybody logged in to the system can then see the password...

anthraxx commented 8 years ago

@SuperBFG7 the same applies for any kind of configuration file if there is no special user and 600 file permissions. Not that its good, but ncmpcpp has the same "issue".

If we want to do this "properly" then it would make more sense to make ympd configuration file aware (f.e. reading /etc/ympd.conf) by default and provide a cli parameter to use some other config file (f.e. via --config or similar). In case the cli parameter for config files is provided, it would still make sense to first process the global ympd.config and just override the values by the cli parameter passed config. This way a system wide packaging could create a system ympd user with /bin/nologin shell and set 600 on the /etc/ympd.conf file. If another user wants to one-time connect to a different mpd, then he can pass a config file parameter and set the deferred config files perms to 600. Bonus points if ympd complains about config files that are world readable.

SuperBFG7 commented 8 years ago

Yes, but securing the config file with 600 is possible, whereas the command line is always public. No need for a special user (root can read everything anyway). Maybe instead reading the password just read the location to a password file on the command line? Then the user can decide if he wants it 600 secured or not. Would be easier than moving all of the parameters to a config I guess.

btw. one time connects to other mpd servers can be done much easier ad-hoc in the web GUI

anthraxx commented 8 years ago

@SuperBFG7 Its obvious (to us) that its always public, thats exactly why I explained my recommendation how to do it "properly" :yum:

Maybe you don't want to start ympd as root if its not required (even when it can drop privileges) therefor it would make totally sense to add a new user! You only really need to start as root if you want to bind to a port <1024. But thats a distribution/packaging topic for each distribution on its own anyway and is beyond the scope of the projects and source itself.

If there is handling to read a file anyway its not that much more work to possibly read all parameters, however the gain of convenience is high as one could store multiple configs for different environments rather then writing wrapper startup scripts, store aliases somewhere or scroll through history. Why not doing it awesome instead of good-enough? :stuck_out_tongue_winking_eye: Also I would still recommend at least a warning upon world readable config file if it can contain a password because users tend to not think about this and its highly recommended to have sane defaults or at least warn the user and make him aware that he may leak his password. Not every normal user has in mind that this may have security implications.

Cool, did not really notice that you can one-time connect in the web GUI, thanks :)

magnatlu commented 8 years ago

I didn't notice password configuration in web UI nor any other way to set it. Since ympd is targeted mostly at RPis, I don't think it is much of a security risk - those are usually zero-or-single user platform. I definitely agree passing password as a command line parameter is very bad practice, but it was sufficient and acceptable in my case. I guess it won't be merged back to official release anyway.

anthraxx commented 8 years ago

@magnatlu you should not assume that is exclusively used on single-user systems or embedded devices. Its always recommended to follow sane defaults, no matter what the major target audience may be! If nobody wants to implement the configuration file thingie, I would volunteer to do it around Christmas holidays in the way i proposed it. :smile:

magnatlu commented 8 years ago

@anthraxx Since I have little experience in C, I don't feel competent enough to parse a simple config file without causing half a dozen buffer overflows and massive memory leaks.

anthraxx commented 8 years ago

@magnatlu no problem, thank you either way for creating this patch and contributing therefor. At the end its still the developers decision... so maybe he could give us a small hint what he wants... if he is happy with config files I could do that (or he wants to do that itself, event better :yum: )