parse environment variables #886

JasonLG1979 · 2021-11-23T16:25:23Z

Make librespot able to parse environment variables for options and flags. To avoid name collisions environment variables must be prepended with LIBRESPOT_ so option/flag foo-bar becomes LIBRESPOT_FOO_BAR.
Verbose logging mode (-v, --verbose, LIBRESPOT_VERBOSE=) now logs all parsed environment variables and command line arguments (credentials are redacted).

JasonLG1979 · 2021-11-23T18:09:57Z

Haven't done the changelog yet because I didn't want to deal with any merge conflicts between this and #883. After that is merged I'll rebase and edit the changelog.

JasonLG1979 · 2021-11-24T02:41:16Z

This is very handy for daemonizing librespot with systemd as you can specify an environment file and use that as a no cost config file and keep configuration details out of the service unit file. It's also a security win in that case since systemctl status does not require elevated privileges and previously username and password had to be passed as args so anyone could call systemctl status and see the passed creds in plain text.

https://www.freedesktop.org/software/systemd/man/systemd.exec.html#EnvironmentFile=

kingosticks · 2021-11-24T11:52:12Z

In terms of your last comment specifically, other projects use a method where the service unit has something like

ExecStart=/usr/bin/foo --something_generic $FOO_OPTS

and then also define an EnvironmentFile=-/etc/default/foo which contains something like:

FOO_OPTS="--something1 hello --something2 world --secret mypasswordhere"

The benefit of this method is you don't need any special code to handle reading environment variables.

JasonLG1979 · 2021-11-24T16:55:07Z

@kingosticks I know this, see the linked Raspotify issue. And generally that withstanding it's pretty common for command line apps to read environment variables. I don't understand the push back?

JasonLG1979 · 2021-11-24T16:58:55Z

The roll your own var method is a kludge for apps that can't read env vars.

JasonLG1979 · 2021-11-24T17:26:10Z

@kingosticks did you look at the code? It's actually a very small change that moves librespot a small step closer to being a "real" application. I don't think of parsing env vars as some sort of fancy feature, it a pretty basic thing that a great many command line apps are capable of doing.

IMHO this PR isn't about adding a feature but fixing an omission.

kingosticks · 2021-11-25T00:12:33Z

It wasn't meant to be push-back. I was highlighting the alternative because although it might be mentioned in dtcooper/raspotify#460 it's not as clear in your comment here that the specific use-case of hiding arg values can be accomplished without changing librespot. You call that alternative a kludge and that opinion is fine.

This change might actually have as much benefit to those running from the command line (i.e. not a service) as it would allow them to more easily keep librespot settings in their dotfiles.

If/when you make this change, probably need to document somewhere which of the two config methods takes precedence when both are set.

JasonLG1979 · 2021-11-25T00:17:55Z

It wasn't meant to be push-back. I was highlighting the alternative because although it might be mentioned in dtcooper/raspotify#460 it's not as clear in your comment here that the specific use-case of hiding arg values can be accomplished without changing librespot. You call that alternative a kludge and that opinion is fine.

Ok fair enough.

If/when you make this change, probably need to document somewhere which of the two config methods takes precedence when both are set.

If both are present the passed command line arg takes precedence. That seemed like the logical order of things to me.

JasonLG1979 · 2021-11-25T00:23:18Z

fn arg_to_var(arg: &str) -> String {
    // To avoid name collisions environment variables must be prepended
    // with `LIBRESPOT_` so option/flag `foo-bar` becomes `LIBRESPOT_FOO_BAR`.
    format!("LIBRESPOT_{}", arg.to_uppercase().replace("-", "_"))
}

fn env_var_present(arg: &str) -> bool {
    env::var(arg_to_var(arg)).is_ok()
}

fn env_var_opt_str(option: &str) -> Option<String> {
    match env::var(arg_to_var(option)) {
        Ok(value) => Some(value),
        Err(_) => None,
    }
}

   let opt_present = |opt| matches.opt_present(opt) || env_var_present(opt);

   let opt_str = |opt| {
        if matches.opt_present(opt) {
            matches.opt_str(opt)
        } else {
            env_var_opt_str(opt)
        }
    };

env vars aren't even evaluated if command line args are present.

kingosticks · 2021-11-25T00:27:35Z

I know (I did read the code) and I agree that's the correct order. It was a reminder that it needs to be specified in the wiki/whatever.

JasonLG1979 · 2021-11-25T00:35:06Z

The caveat being that flags are set if present (env and command line). The only way to override a flag set with an env var is to remove the env var. You can't override a set flag with another command line arg. If that makes sense?

JasonLG1979 · 2021-11-25T08:16:42Z

It was a reminder that it needs to be specified in the wiki/whatever.

I will note it in the change log when the time comes and in the wiki.

kingosticks · 2021-11-25T09:33:37Z

The caveat being that flags are set if present (env and command line). The only way to override a flag set with an env var is to remove the env var. You can't override a set flag with another command line arg. If that makes sense?

Yes, I was thinking about that too. I think it has the potential to muddy the water when debugging as we often ask for the exact command used to invoke librespot in order to understand the settings they are running with. You can imagine someone setting an envvar, forgetting about it and later wondering why it's doing something odd. A solution would be to ensure there is (at least verbose) logging for every setting value. Or, have a mode that just prints the configured value for every setting and exits. The latter is quite nice as you can ask for that self-contained output in the bug report template etc.

JasonLG1979 · 2021-11-25T09:38:20Z

Adding a trace level message would be pretty easy. An arg that outputs all args is kinda meta though. It's kinda like altering what you're trying to observe.

JasonLG1979 · 2021-11-25T09:40:56Z

Meta not necessarily in a bad way but just in a way that it's an arg about arg.

kingosticks · 2021-11-25T10:01:42Z

You could see it that way but that's not an argument against doing it. It's similar to help.

Anyways, it's out of scope here. Just something to keep in mind for later if we find ourselves getting tricked by the multiple sources for options. No need to fix something that isn't a problem (yet)!

JasonLG1979 · 2021-11-25T10:29:07Z

I'm not arguing against it but I agree that we can cross that bridge if we come to it.

Make librespot able to parse environment variables for options and flags. To avoid name collisions environment variables must be prepended with `LIBRESPOT_` so option/flag `foo-bar` becomes `LIBRESPOT_FOO_BAR`.

JasonLG1979 · 2021-11-25T15:39:34Z

There the change log is updated and as noted after this is merged I will update the wiki.

JasonLG1979 · 2021-11-25T15:42:05Z

You guys better step up your game because so far that Unreleased changelog section is all me 💪 🤣

JasonLG1979 · 2021-11-25T20:57:15Z

@kingosticks I just re-read your comment about the alternative method aka "the arg bucket".

To clarify the result would be:

/usr/bin/foo --something_generic --something1 hello --something2 world --secret mypasswordhere

And your "secret" being completely visible to any and all user with systemctl status foo. All you've managed to do is move it from the unit file to an environment file to make it easy to change. It's not secure at all even if you locked down the environment file because you've passed it as an arg and args are shown in plain text when you use systemctl status.

ashthespy · 2021-11-25T21:38:47Z

And your "secret" being completely visible to any and all user with systemctl status foo. All you've managed to do is move it from the unit file to an environment file to make it easy to change. It's not secure at all even if you locked down the environment file because you've passed it as an arg and args are shown in plain text when you use systemctl status.

and echo $LIBRESPOT_PASSWORD?

PS: I like french windows, and a red door on my bike shed :-D
Nah, in all seriousness, given our use case, I don't think we should aim at anything more than opportunistic credit leaking, which you seem to have tackled for the raspotify use case.

JasonLG1979 · 2021-11-25T21:40:36Z

and echo $LIBRESPOT_PASSWORD?

That won't do anything. The vars set in a service file aren't visible from the outside.

ashthespy · 2021-11-25T21:44:34Z

That won't do anything. The vars set in a service file aren't visible from the outside.

TIL!

JasonLG1979 · 2021-11-25T21:48:44Z

My point was not to bikeshed but to clarify that the secret in the example is anything but a secret. The example seems to imply that it is. All the environment file does in that case is make it easier to change the args. It makes a faux config file.

JasonLG1979 · 2021-11-25T21:50:02Z

I'm also not implying that proper env vars are bullet proof security. They're just marginally better. And they make for a better faux config file.

ashthespy · 2021-11-25T21:53:25Z

My point was not to bikeshed but to clarify that the secret in the example is anything but a secret. The example seems to imply that it is. All the environment file does in that case is make it easier to change the args. It makes a faux config file.

Sorry, my comment was a drive-by - no offense meant!
FWIW, I got fed up with all the flags and moved to reading a config file on my fork, not for security, but usability/"singe source of truth".

EDIT:
If work is going into making librespot - the binary better, it might be time to just bite the bullet and convert it to librespot - the daemon? I feel we are hesitant to commit to the daemon, making life with the binary a bit convoluted?

JasonLG1979 · 2021-11-25T22:23:35Z

The systemd docs make a big deal about environment variables being a bad idea for passing secrets. Not sure we need to buy into that level of paranoia. I think there's a limit to how secret this data needs to be. I guess if someone cares enough they can add support for something like .netrc or keyrings.

I was mainly just trying to keep it out of the view of your average Lookie-loo. Realistically we're not talking about state secrets.

JasonLG1979 · 2021-11-25T22:30:02Z

Not having garbage littering the service file is the primary reason. As you mention it makes updating much easier when/if args are added/removed and/or change names.

JasonLG1979 · 2021-11-26T05:07:40Z

@kingosticks

Args are expended. Environment variables are not. Whatever you set as env vars is not visible. In your example you are passing the vars as args. That is why they are visible. You can set vars with a env var file and they will not be visible as long as you don't pass them as args.

JasonLG1979 · 2021-11-26T17:42:37Z

This has kinda gone off the rails a bit. But I think I would be correct in say that everyone so far agrees that this is a good addition @roderickvd what do you think? Ready to merge?

roderickvd · 2021-11-26T18:48:23Z

Yeah I was lurking to see how this all unfolded 😆

You guys better step up your game because so far that Unreleased changelog section is all me 💪 🤣

I'll review it soon, this evening I'm keen to iron out the last things in my new-api WIP and push it out to you guys. Didn't know it was a contest but expect more than a few LOC 😜

JasonLG1979 · 2021-11-26T20:01:36Z

I'll review it soon, this evening I'm keen to iron out the last things in my new-api WIP and push it out to you guys. Didn't know it was a contest but expect more than a few LOC

I'm more of the cleanup crew at this point, while you guys are the builders. You guys do the heavy lifting and then I go in and smooth the rough edges and put a little polish on it.

I look forward to the new (new-new?) API. I think that once that is ironed out and maybe libmdns is cleaned up (if it even needs to be?) librespot has the opportunity to be a really next level stuff. I personally look forward to the audio back end bits when we move to supporting more than just vorbis.

JasonLG1979 · 2021-11-27T22:35:14Z

There @kingosticks -v / --verbose now logs the env vars and command line args. The output looks like this:

$ export LIBRESPOT_NORMALISATION_PREGAIN=-3.0
$ export LIBRESPOT_AUTOPLAY=

$ librespot -u username -v --bitrate 320

[2021-11-27T22:30:34Z INFO  librespot] librespot 0.3.1 e2c3808 (Built on 2021-11-27, Build ID: TgKYi39V, Profile: release)
[2021-11-27T22:30:34Z TRACE librespot] Environment variable(s):
[2021-11-27T22:30:34Z TRACE librespot] 		LIBRESPOT_NORMALISATION_PREGAIN=-3.0
[2021-11-27T22:30:34Z TRACE librespot] 		LIBRESPOT_AUTOPLAY=
[2021-11-27T22:30:34Z TRACE librespot] Command line argument(s):
[2021-11-27T22:30:34Z TRACE librespot] 		-u XXXXXXXX
[2021-11-27T22:30:34Z TRACE librespot] 		-v 
[2021-11-27T22:30:34Z TRACE librespot] 		--bitrate 320

JasonLG1979 · 2021-11-27T22:35:57Z

Creds are redacted.

Verbose logging mode (`-v`, `--verbose`) now logs all parsed environment variables and command line arguments (credentials are redacted).

roderickvd · 2021-11-28T15:59:18Z

Again I'm following this, please let me know when you're ready for final review and are all in agreement.

JasonLG1979 · 2021-11-28T17:53:13Z

I'm ready. I'm still waiting for @kingosticks as he mentioned the idea of logging all the command line args and env vars so I added that.

JasonLG1979 · 2021-11-28T18:30:08Z

Anyone that would like to see an implementation of this can checkout https://github.com/dtcooper/raspotify/tree/new-config

JasonLG1979 · 2021-11-28T18:31:12Z

Specifically:

https://github.com/dtcooper/raspotify/blob/new-config/raspotify/lib/systemd/system/raspotify.service

And

https://github.com/dtcooper/raspotify/blob/new-config/raspotify/etc/raspotify/conf

kingosticks · 2021-11-28T18:37:47Z

Sorry, I've not got time to actually look at the code right now but the example in the comment looks great. Nice one.

JasonLG1979 · 2021-11-28T19:20:59Z

@kingosticks the code isn't particularly clever. It just loops though the vars and args and outputs a trace level log message if there are any.

JasonLG1979 · 2021-12-01T16:58:17Z

@roderickvd and @kingosticks and @ashthespy what say you? Merge?

ashthespy · 2021-12-01T17:27:10Z

LGTM :-) Thanks for your work!

roderickvd

Just one small thing and we're good to merge!

src/main.rs

The alternative would be clunky.

* commit 'e66cc5508cee0413829aa347c7a31bd0293eb856': parse environment variables (librespot-org#886) Harden systemd service, update restart policy (librespot-org#888) Improve `--device ?` functionality for the alsa backend # Conflicts: # src/main.rs

Make librespot able to parse environment variables for options and flags. To avoid name collisions environment variables must be prepended with `LIBRESPOT_` so option/flag `foo-bar` becomes `LIBRESPOT_FOO_BAR`. Verbose logging mode (`-v`, `--verbose`) logs all parsed environment variables and command line arguments (credentials are redacted).

Fix up for librespot-org#886 Closes: librespot-org#898 And... * Don't silently ignore non-Unicode while parsing env vars. * Iterating over `std::env::args` will panic! on invalid unicode. Let's not do that. `getopts` will catch missing args and exit if those args are required after our error message about the arg not being valid unicode. * Gaurd against empty strings. There are a few places while parsing options strings that we don't immediately evaluate their validity let's at least makes sure that they are not empty if present. * `args` is only used in `get_setup` it doesn't need to be in main. * Nicer help header. * Get rid of `use std::io::{stderr, Write};` and just use `rpassword::prompt_password_stderr`. * Get rid of `get_credentials` it was clunky, ugly and only used once. There is no need for it to be a separate function. * Handle an empty password prompt and password prompt parsing errors. * + Other random misc clean ups.

roderickvd added the enhancement label Nov 23, 2021

JasonLG1979 mentioned this pull request Nov 24, 2021

The current config situation is insecure. dtcooper/raspotify#460

Closed

JasonLG1979 added 2 commits November 25, 2021 09:33

parse environment variables

a239eb2

Make librespot able to parse environment variables for options and flags. To avoid name collisions environment variables must be prepended with `LIBRESPOT_` so option/flag `foo-bar` becomes `LIBRESPOT_FOO_BAR`.

Merge branch 'librespot-org:dev' into parse_env_var

e2c3808

roderickvd self-assigned this Nov 26, 2021

JasonLG1979 added 3 commits November 27, 2021 20:00

log all parsed environment variables and command line arguments

ca783cf

Verbose logging mode (`-v`, `--verbose`) now logs all parsed environment variables and command line arguments (credentials are redacted).

Merge branch 'dev' into parse_env_var

819c4f8

Use to_str instead of into_string

7205db0

JasonLG1979 mentioned this pull request Nov 30, 2021

Spotify does not detect Librespot as a device #876

Closed

roderickvd previously requested changes Dec 1, 2021

View reviewed changes

src/main.rs Show resolved Hide resolved

roderickvd merged commit e66cc55 into librespot-org:dev Dec 1, 2021

JasonLG1979 deleted the parse_env_var branch December 1, 2021 20:30

JasonLG1979 mentioned this pull request Dec 5, 2021

Fix Command line arguments incorrectly echoed in TRACE #899

Merged

doris1347 mentioned this pull request Dec 11, 2022

How I got everything working dupontgu/retro-ipod-spotify-client#69

Open

parse environment variables #886

parse environment variables #886

Uh oh!

Conversation

JasonLG1979 commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonLG1979 commented Nov 23, 2021

Uh oh!

JasonLG1979 commented Nov 24, 2021

Uh oh!

kingosticks commented Nov 24, 2021

Uh oh!

JasonLG1979 commented Nov 24, 2021

Uh oh!

JasonLG1979 commented Nov 24, 2021

Uh oh!

JasonLG1979 commented Nov 24, 2021

Uh oh!

kingosticks commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonLG1979 commented Nov 25, 2021

Uh oh!

kingosticks commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021

Uh oh!

kingosticks commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kingosticks commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashthespy commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021

Uh oh!

ashthespy commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021

Uh oh!

JasonLG1979 commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashthespy commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonLG1979 commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonLG1979 commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonLG1979 commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonLG1979 commented Nov 26, 2021

Uh oh!

roderickvd commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JasonLG1979 commented Nov 26, 2021

Uh oh!

JasonLG1979 commented Nov 23, 2021 •

edited

Loading

JasonLG1979 commented Nov 25, 2021 •

edited

Loading

JasonLG1979 commented Nov 25, 2021 •

edited

Loading

JasonLG1979 commented Nov 25, 2021 •

edited

Loading

JasonLG1979 commented Nov 25, 2021 •

edited

Loading

ashthespy commented Nov 25, 2021 •

edited

Loading

JasonLG1979 commented Nov 25, 2021 •

edited

Loading

JasonLG1979 commented Nov 25, 2021 •

edited

Loading

JasonLG1979 commented Nov 26, 2021 •

edited

Loading

roderickvd commented Nov 26, 2021 •

edited

Loading

JasonLG1979 commented Nov 27, 2021 •

edited

Loading

JasonLG1979 commented Nov 28, 2021 •

edited

Loading