Allow passing database password as environment variables. #546

New Issue

essembeh · 2022-06-12T20:56:35Z

essembeh commented

2022-06-12 20:56:35 +00:00

What I'm trying to do and why

I'm using borgmatic as a cronjob of a kubernetes cluster to backup files and a MySQL database.
My MySQL passwords are defined as Kubernetes Secrets and injected as env var to MySQL container.
I configure the borgmatic job with a config file defined in a ConfigMap (Kubernetes object).

I don't like having to write down my MySQL password directly in the Yaml file, I would like to inject it as a Secret ie. an environment variable directly in the container.

I would like to have a password_env key in addition to password for the 3 database hooks configuration nodes where I could give the environment variable name where the password will be injected.

I made a quick development and it works for my needs, maybe someone might find it useful too.
See: #545

Thanks

#### What I'm trying to do and why I'm using borgmatic as a cronjob of a kubernetes cluster to backup files and a MySQL database. My MySQL passwords are defined as Kubernetes Secrets and injected as env var to MySQL container. I configure the borgmatic job with a config file defined in a ConfigMap (Kubernetes object). I don't like having to write down my MySQL password directly in the Yaml file, I would like to inject it as a Secret ie. an environment variable directly in the container. I would like to have a `password_env` key in addition to `password` for the 3 database hooks configuration nodes where I could give the environment variable name where the password will be injected. I made a quick development and it works for my needs, maybe someone might find it useful too. See: https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/545 Thanks

witten commented

2022-06-14 17:46:46 +00:00

Wow, borgmatic on Kubernetes. Never thought I'd see that! Thank you for taking the time to file this and submit the PR. It seems like a totally viable solution for your use case, although I'm also thinking about a few other tickets with related use cases (#370, #382). Specifically, they involve using environment variables for more configuration options than just database password.

So, at the risk of increasing scope, what do you think of a more generalized solution that could be used for other options as well? Something like:

hooks:
    postgresql_databases:
        - name: test
          username: ${MY_DB_USERNAME}
          password: ${MY_DB_PASSWORD}

... where MY_DB_USERNAME and MY_DB_PASSWORD are arbitrary environment variables that would get interpolated into the option's value. The same approach could be used for the repository encryption passphrase:

storage:
    encryption_passphrase: ${MY_PASSPHRASE}

Open to other ideas for syntax as well.

I'd be happy to take a stab at this. I just wanted to see if it'd solve your use case first. Thanks!

Wow, borgmatic on Kubernetes. Never thought I'd see that! Thank you for taking the time to file this and submit the PR. It seems like a totally viable solution for your use case, although I'm also thinking about a few other tickets with related use cases (#370, #382). Specifically, they involve using environment variables for more configuration options than just database password. So, at the risk of increasing scope, what do you think of a more generalized solution that could be used for other options as well? Something like: ```bash hooks: postgresql_databases: - name: test username: ${MY_DB_USERNAME} password: ${MY_DB_PASSWORD} ``` ... where `MY_DB_USERNAME` and `MY_DB_PASSWORD` are arbitrary environment variables that would get interpolated into the option's value. The same approach could be used for the repository encryption passphrase: ```bash storage: encryption_passphrase: ${MY_PASSPHRASE} ``` Open to other ideas for syntax as well. I'd be happy to take a stab at this. I just wanted to see if it'd solve your use case first. Thanks!

witten referenced this issue

2022-06-14 17:47:26 +00:00

Using environment variables for databases hooks #382

witten referenced this issue

2022-06-14 17:47:51 +00:00

Optionally pass through PostgreSQL environment variables in database hook #370

essembeh commented

2022-06-14 19:51:13 +00:00

Thanks for your answer @witten

I understand the idea of using this env resolution mecanism for other configuration nodes, I was too focus on my initial need.

One thing I don't like in my first implementation is that I bypass the syntax validation because variables values are not checked by the schema validation.
Without saying that my solution duplicates every key you want to have in environment (password and password_env).

I just thought of a maybe better solution, more convenient.
Maybe I could try using https://pypi.org/project/envsubst/ first to resolve all variables in config file, on start, before the schema validation so that you are sure the config file is valid and you don't need to change anything else in the code to be able to use env variables for any node?

If you like the idea, I could update my PR with that?

Thanks for your answer @witten I understand the idea of using this env resolution mecanism for other configuration nodes, I was too focus on my initial need. One thing I don't like in my first implementation is that I bypass the syntax validation because variables values are not checked by the schema validation. Without saying that my solution duplicates every key you want to have in environment (`password` and `password_env`). I just thought of a maybe better solution, more convenient. Maybe I could try using https://pypi.org/project/envsubst/ first to resolve all variables in config file, on start, before the schema validation so that you are sure the config file is valid and you don't need to change anything else in the code to be able to use env variables for any node? If you like the idea, I could update my PR with that?

witten commented

2022-06-14 21:08:02 +00:00

Yeah, I think it makes sense not to bypass schema validation. There's already precedent for applying logical changes to the loaded configuration before validating it, specifically here: https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/master/borgmatic/config/validate.py#L101

Those lines apply any command-line overrides to configuration and normalize/upgrade any "old" configuration from a prior schema—all before validation is applied. I could see interpolating environment variables around there, too.

As for the actual substitution, there's precedent for that as well! Specifically, some very basic variable interpolation within command hooks: https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/master/borgmatic/hooks/command.py#L13

That's one reason I was suggesting the ${FOO} syntax. It's kind of like the existing hook variable interpolation—with the addition of $ to indicate it's an environment variable. ($FOO syntax is possible too, but I'd worry that'd break on things like passphrases that contain a literal $ character.)

As for envsubst, that looks like it hasn't seen any updates recently. I wonder if a similar (less featureful) approach like the command hook interpolation above would work here?

Anyway, if you're still interested, I'd welcome an updated PR. That should probably include updating/adding tests as well. Thanks for your patience here!

Yeah, I think it makes sense not to bypass schema validation. There's already precedent for applying logical changes to the loaded configuration before validating it, specifically here: https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/master/borgmatic/config/validate.py#L101 Those lines apply any [command-line overrides](https://torsion.org/borgmatic/docs/how-to/make-per-application-backups/#configuration-overrides) to configuration and normalize/upgrade any "old" configuration from a prior schema—all before validation is applied. I could see interpolating environment variables around there, too. As for the actual substitution, there's precedent for that as well! Specifically, some very basic [variable interpolation](https://torsion.org/borgmatic/docs/how-to/add-preparation-and-cleanup-steps-to-backups/#variable-interpolation) within command hooks: https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/master/borgmatic/hooks/command.py#L13 That's one reason I was suggesting the `${FOO}` syntax. It's kind of like the existing hook variable interpolation—with the addition of `$` to indicate it's an environment variable. (`$FOO` syntax is possible too, but I'd worry that'd break on things like passphrases that contain a literal `$` character.) As for `envsubst`, that looks like it hasn't seen any updates recently. I wonder if a similar (less featureful) approach like the command hook interpolation above would work here? Anyway, if you're still interested, I'd welcome an updated PR. That should probably include updating/adding tests as well. Thanks for your patience here!

essembeh commented

2022-06-14 22:08:59 +00:00

Thanks for your quick answers.

I'll have a look to both source files you pointed.

Envsubst is a common and basic linux cli tool, part or GNU gettext

I had a look to the python project and it does not seem active recently but on the other hand it does only basic things, that may explain it.

I would propose to either

use this project as dependency (i don't like it to be honest)
implement the same re.sub logic
use envsubst as an external process, this would only work on linux, idk if borgmatic supports windows, but a runtime error if you use variables and envsubst is not available in your OS looks good to me.

In addition, I would implement a --envsubst (name TBD) to enable the variable resolution so that it does not change the current behavior ?

Thanks for your quick answers. I'll have a look to both source files you pointed. Envsubst is a common and basic linux cli tool, [part or GNU gettext](https://www.gnu.org/software/gettext/manual/html_node/envsubst-Invocation.html) I had a look to the python project and it does not seem active recently but on the other hand it does [only basic things](https://github.com/ashafer01/python-envsubst/blob/master/envsubst.py), that may explain it. I would propose to either * use this project as dependency (i don't like it to be honest) * implement the same `re.sub` logic * use `envsubst` as an external process, this would only work on linux, idk if borgmatic supports windows, but a runtime error if you use variables and `envsubst` is not available in your OS looks good to me. In addition, I would implement a `--envsubst` (name TBD) to enable the variable resolution so that it does not change the current behavior ?

witten commented

2022-06-14 22:52:31 +00:00

My personal bias is for the re.sub() approach, because:

Precedent (not always a good reason, admittedly).
borgmatic doesn't officially support Windows, but there are a number of Mac users ... and envsubst isn't a built-in on macOS. I'd prefer not to have Linux-only features unless absolutely necessary.
I generally try to limit dependencies, whether on third-party Python libraries or external shell commands. When borgmatic does have to take a dependency, ideally it's on something that's actively maintained—if only for security updates!
I don't know that we need all the fancy interpolation of something like envsubst (at least of the Python library). Just a basic one-for-one substitution IMO would be enough. (Feel free to tell me about use cases I haven't considered though.)

Anyway, on the topic of a flag to enable this behavior, that sounds totally reasonable. I could also see it being enabled by default (so, without a flag) if the interpolation syntax doesn't seem too likely to collide with literal values. Like, I could foresee encryption_passphrase: !@a#$b!@c#$ causing problems. But there's probably nobody with encryption_passphrase: !@a#${b}!@c#$ in their configuration file. Probably. 😄

My personal bias is for the `re.sub()` approach, because: * Precedent (not always a good reason, admittedly). * borgmatic doesn't officially support Windows, but there are a number of Mac users ... and `envsubst` isn't a built-in on macOS. I'd prefer not to have Linux-only features unless absolutely necessary. * I generally try to limit dependencies, whether on third-party Python libraries or external shell commands. When borgmatic does have to take a dependency, *ideally* it's on something that's actively maintained—if only for security updates! * I don't know that we need all the fancy interpolation of something like `envsubst` (at least of the Python library). Just a basic one-for-one substitution IMO would be enough. (Feel free to tell me about use cases I haven't considered though.) Anyway, on the topic of a flag to enable this behavior, that sounds totally reasonable. I could also see it being enabled by default (so, without a flag) if the interpolation syntax doesn't seem too likely to collide with literal values. Like, I could foresee `encryption_passphrase: !@a#$b!@c#$` causing problems. But there's probably nobody with `encryption_passphrase: !@a#${b}!@c#$` in their configuration file. Probably. 😄

👍 1

witten added the

waiting for response

label 2022-06-15 23:13:25 +00:00

essembeh referenced this issue

2022-06-16 16:54:51 +00:00

Allow database password to be read from environ #545

essembeh referenced a pull request that will close this issue

2022-06-16 16:55:33 +00:00

Allow environment variable resolution in configuration file #548

essembeh commented

2022-06-16 17:00:43 +00:00

@witten please have a look to the new PR I just submitted,

I hesitated between using os.path.expandvars and the re.sub implentation.
I prefered the second one because expandvars also resolves $FOO and you mentionned it would be better not to.
Also, using re.sub allow to implement shell-like default values like ${FOO:-bar}, feature that os.path.expandvars does not support.

I implemented some basic unit tests, but did not update any documentation.

Thanks

@witten please have a look to the new [PR I just submitted](https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/548), I hesitated between using [`os.path.expandvars`](https://docs.python.org/3/library/os.path.html?highlight=expandvars#os.path.expandvars) and the `re.sub` implentation. I prefered the second one because `expandvars` also resolves `$FOO` and you mentionned it would be better not to. Also, using `re.sub` allow to implement shell-like default values like `${FOO:-bar}`, feature that `os.path.expandvars` does not support. I implemented some basic unit tests, but did not update any documentation. Thanks

witten closed this issue

2022-06-16 18:18:15 +00:00

witten referenced this issue from a commit

2022-06-16 18:18:15 +00:00

Allow environment variable resolution in configuration file

witten referenced this issue from a commit

2022-06-16 18:36:30 +00:00

Code style, rename command-line flag, and move new code into its own file (#546)

witten referenced this issue from a commit

2022-06-16 22:31:17 +00:00

Document environment variable interpolation (#546).

witten commented

2022-06-16 22:44:41 +00:00

New docs are up! https://torsion.org/borgmatic/docs/how-to/provide-your-passwords/

Please let me know if you see anything in them that's inaccurate or could be improved. Thanks again!

New docs are up! https://torsion.org/borgmatic/docs/how-to/provide-your-passwords/ Please let me know if you see anything in them that's inaccurate or could be improved. Thanks again!

witten removed the

waiting for response

label 2022-06-16 23:23:19 +00:00

witten referenced this issue

2022-06-16 23:26:20 +00:00

Optionally pass through PostgreSQL environment variables in database hook #370

witten referenced this issue

2022-06-16 23:30:02 +00:00

Using environment variables for databases hooks #382

essembeh commented

2022-06-17 07:58:15 +00:00

The documentation is very clear.
Just a missing detail, that I also missed in the unit tests ... you can escape an env var with \

But then I realized that my implementation had an issue, as you noticed I use a negative lookbehind to ignore escaped env vars like \${FOO}.
But when you escape an env var, you expect the \ to be removed like in shell

> echo \${FOO}
${FOO}

So that is your password is abc${foo}def you can escape it in your config file

password: abc\${foo}def

I tried to fix this in #549
I do not use a negative lookbehind anymore, to be able to catch the escaped env var and then unescape it. I added a unit test for that purpose.

I also added a test with multiple env var in the same string :)

The documentation is very clear. Just a missing detail, that I also missed in the unit tests ... you can escape an env var with `\` But then I realized that my implementation had an issue, as you noticed I use a negative lookbehind to ignore escaped env vars like `\${FOO}`. But when you escape an env var, you expect the `\` to be removed like in shell ```sh > echo \${FOO} ${FOO} ``` So that is your password is `abc${foo}def` you can escape it in your config file ```yaml password: abc\${foo}def ``` I tried to fix this in https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/549 I do not use a negative lookbehind anymore, to be able to catch the escaped env var and then unescape it. I added a unit test for that purpose. I also added a test with multiple env var in the same string :)

witten commented

2022-06-23 17:10:39 +00:00

Awesome, good catch! Really appreciate it.

witten referenced this issue from a commit

2022-06-23 17:16:12 +00:00

Fix escaped environment variable in configuration (#546).

witten referenced this issue from a commit

2022-06-23 17:28:18 +00:00

Update documentation with environment variable escaping (#546).

witten commented

2022-06-24 17:01:02 +00:00

Released in borgmatic 1.6.4!