borgmatic-collective/borgmatic
borgmatic-collective/borgmatic
10
66
Fork 27
Code Issues 42 Pull Requests 6 Actions Packages Releases 140 Activity

Allow passing database password as environment variables. #546

New Issue
Closed
opened 2022-06-12 20:56:35 +00:00 by essembeh · 10 comments
essembeh commented 2022-06-12 20:56:35 +00:00
Contributor
Copy Link

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
Owner
Copy Link

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!
essembeh commented 2022-06-14 19:51:13 +00:00
Author
Contributor
Copy Link

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
Owner
Copy Link

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
Author
Contributor
Copy Link

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
Owner
Copy Link

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 commented 2022-06-16 17:00:43 +00:00
Author
Contributor
Copy Link

@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 commented 2022-06-16 22:44:41 +00:00
Owner
Copy Link

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
essembeh commented 2022-06-17 07:58:15 +00:00
Author
Contributor
Copy Link

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
Owner
Copy Link

Awesome, good catch! Really appreciate it.

Awesome, good catch! Really appreciate it.
witten commented 2022-06-24 17:01:02 +00:00
Owner
Copy Link

Released in borgmatic 1.6.4!

Released in borgmatic 1.6.4!
👍 1
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
diivi import_bot witten
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#546
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?