Support environment variables in config.yml

[BladeWDR]

It doesn't seem like the YAML parser you're using supports environment variable expansion.

How difficult would it be to implement this?

Granted, my arrs are not publicly exposed, but I do store my dotfiles on GitHub, and I'd prefer not to have my API keys for them stored there for anyone to see.

Being able to load them from a .env file or system wide environment variables would be a nice enhancement.

Something like this:

radarr:
  uri: https://radarr.domain.xyz
  use_ssl: true
  api_token: ${RADARR_API_KEY}
sonarr:
  uri: https://sonarr.domain.xyz
  use_ssl: true
  api_token: ${SONARR_API_KEY}

I just started playing with this today, nice little project! :)

[Dark-Alex-17]

Thank you! I'm so glad you're enjoying it!

I've looked around and I can't find a crate for my specific use case that doesn't have a lot of additional bloat to do what you described with the environment variables. That said, I can easily implement it manually and thanks to Rust's type system, it wouldn't be security vulnerability.

But before I even do that, I wanted to ask: What's the use case for doing this? Docker images I'm guessing? Otherwise, it seems like it would be more useful to support predefined environment variables (e.g. MANAGARR_RADARR_API_KEY) for configuration and then fall back to the configuration file if none is found.

Would that be preferable? To just allow environment variable overrides/exclusive environment variable configs? Or is there a greater need for defining arbitrary environment variables within the YAML that I've just never encountered?

[BladeWDR]

Simply because I don't like leaving passwords lying around in plaintext, even if it's low risk.

If we can set the environment variables globally and do it that way that also works. It just seemed to me that from a consistency standpoint (and ease of use for less advanced users) that keeping everything in the config file would make more sense.

Either method is fine by me, honestly.

[Dark-Alex-17]

Hmm...I know that this is a pretty standard pattern (thinking of things like kubeconfigs), but I do think an argument could be made for only the API keys as environment variables. Seems like that could work too.

Alternatively, if you use a password manager at all, you could theoretically just save the entire config as a password and use that with process substitution to start Managarr; i.e. something like

managarr --config <(some-pass-manager show managarr-config)

I've not tried using process substitution with the --config flag so I don't know offhand if that works for certain. But internally it's using a PathBuf, so I think it should work.

[tangowithfoxtrot]

Regarding API keys in the config file, I'll admit that I'm also not a huge fan, despite it, admittedly, being a pretty common practice.

Drawing from the comparison with kubeconfigs that you mentioned, @Dark-Alex-17, I can think of two ways that this concern could be mitigated:

1. Support base64-encoded values, similar to a lot of k8s tooling. Granted, this is still not great from a security standpoint, but it at least provides an option to not store them in their non-encoded form, making them less susceptible to a pattern-based grep for API keys. This would be a simple-ish approach, I think.

2. Support environment variable interpolation in the config files. As you mentioned, I don't think there's a simple crate that does this for us, but we should be able to make use of std::env::Vars() to retrieve the environment variables. I imagine those values could then be matched against before the config file is deserialized. I haven't looked at the config code yet, so I could be absolutely be wrong though 😅

#\2 would probably be more involved than supporting base64-encoded values, but I'd also prefer to have a way to avoid keys existing in my config files at all, if possible. If you'd like, I can take a shot at implementing either approach and putting together a PR for it 😄 Lmk if you'd find either or both of those approaches to be acceptable.

[Dark-Alex-17]

I see no good reason not to add that functionality since it is technically more secure. However, my naive implementation of this is to parse the YAML file and replace all ${} with the corresponding environment variables that were fetched via std::env::Vars(). The only issue is I don't want someone to be able to do any kind of code injection, be it YAML, Rust, or otherwise, that would exploit that interpolation. I imagine this is one of the reasons the serde_yaml crate doesn't directly support environment variables, since preventing that and scrubbing the incoming environment variable data may be challenging.

What are you thinking?

Oh, and for easy reference, this is the code to load the config YAML 😄 :

pub(super) fn load_config(path: &str) -> Result<AppConfig> {
  let file = File::open(path).map_err(|e| anyhow!(e))?;
  let reader = BufReader::new(file);
  let config = serde_yaml::from_reader(reader)?;
  Ok(config)
}

[tangowithfoxtrot]

Thanks for the pointers!

Originally, yes, I was thinking what you suggested: do a sort of find-and-replace in the YAML values, but it didn't feel as clean as making config deserializers would. I took a shot at doing that in #30.

I'm not fully sure of what, if anything, would need to be done to protect against any sort of command injection attacks. My initial thought is that as long as we're not evaluating or executing what's in the environment variable values, we should be safe? But we could probably add some tests to feel more confident about that.

[Dark-Alex-17]

I hope you don't mind me pushing to your branch. I tried to think of a better way to share the changes I was playing with but it was too many to leave in comments. ☺️

[tangowithfoxtrot]

Not at all 😁 I'm checking the changes locally now.