Load systemd encrypted credentials #902

Merged
witten merged 1 commits from cvlc12/borgmatic:cvlc12-patch-1 into main 2024-10-22 15:58:42 +00:00
Contributor

Try to find an encrypted borg password to use, provided as an encrypted systemd credential

One can save an encrypted password to /etc/credstore.encrypted/borgpw (or other supported path) with e.g.

# systemd-ask-password -n | systemd-creds encrypt - /etc/credstore.encrypted/borgpw

Problems:

  • Is PASSCOMMAND the right tool here, and what if someone is already using it ? I don't think Borg can natively just read the password from a file, although that would be great.
  • If there is no credential, systemd handles this gracefully, but cat throws an error which pollutes the log..

It should be possible to support multiple passwords, but I've been unable to make that work:

If an absolute path referring to a directory is specified, every file in that directory (recursively) will be loaded as a separate credential. The ID for each credential will be the provided ID suffixed with "_$FILENAME" (e.g., "Key_file1"). When loading from a directory, symlinks will be ignored.

This can probably be adapted to work with the user service, however they don't have fallback directories for that yet, so we would need to hardcode a full path in LoadCredentialEncrypted

Try to find an encrypted borg password to use, provided as an encrypted systemd [credential](https://systemd.io/CREDENTIALS/) One can save an encrypted password to `/etc/credstore.encrypted/borgpw` (or other supported path) with e.g. ``` # systemd-ask-password -n | systemd-creds encrypt - /etc/credstore.encrypted/borgpw ``` Problems: - [ ] ~~Is PASSCOMMAND the right tool here, and what if someone is already using it ?~~ I don't think Borg can natively just read the password from a file, although that would be great. - [ ] ~~If there is no credential, systemd handles this gracefully, but cat throws an error which pollutes the log..~~ It should be possible to support multiple passwords, ~~but I've been unable to make that work~~: > If an absolute path referring to a directory is specified, every file in that directory (recursively) will be loaded as a separate credential. The ID for each credential will be the provided ID suffixed with "_$FILENAME" (e.g., "Key_file1"). When loading from a directory, symlinks will be ignored. This can probably be adapted to work with the user service, however [they don't have fallback directories for that yet](https://github.com/systemd/systemd/issues/33887), so we would need to hardcode a full path in `LoadCredentialEncrypted`
cvlc12 added 1 commit 2024-07-31 20:43:34 +00:00
Owner

Thanks for taking the time to submit this PR, and I apologize for the delay in getting to it! The general approach makes sense to me, but I'd recommend perhaps commenting it out by default so that a borgmatic systemd user can make an intentional decision to use it or not. I think my main concern with enabling it by default is that the PASSCOMMAND environment variable can get stomped over by borgmatic if the user also has the corresponding configuration option set.

Is PASSCOMMAND the right tool here, and what if someone is already using it ? I don't think Borg can natively just read the password from a file, although that would be great.

I think PASSCOMMAND is the right tool. If someone is already using it within borgmatic via the encryption_passcommand option, then borgmatic will stomp over it. And yeah, Borg can't natively read the password from a file AFAIK. Might be an interesting PR for Borg though!

If there is no credential, systemd handles this gracefully, but cat throws an error which pollutes the log..

Not ideal, but might be a good enough solution.

It should be possible to support multiple passwords, but I've been unable to make that work:

That's unfortunate, but IMO you could comment around that for now.

This can probably be adapted to work with the user service, however they don't have fallback directories for that yet, so we would need to hardcode a full path in LoadCredentialEncrypted

Do tildes work? Or would the home directory need to be hard-coded too? Might not be a huge issue if it's commented out by default here too.

Thanks for taking the time to submit this PR, and I apologize for the delay in getting to it! The general approach makes sense to me, but I'd recommend perhaps commenting it out by default so that a borgmatic systemd user can make an intentional decision to use it or not. I think my main concern with enabling it by default is that the `PASSCOMMAND` environment variable can get stomped over by borgmatic if the user also has the corresponding configuration option set. > Is PASSCOMMAND the right tool here, and what if someone is already using it ? I don't think Borg can natively just read the password from a file, although that would be great. I think `PASSCOMMAND` is the [right tool](https://borgbackup.readthedocs.io/en/stable/faq.html#how-can-i-specify-the-encryption-passphrase-programmatically). If someone is already using it within borgmatic via the `encryption_passcommand` option, then borgmatic will stomp over it. And yeah, Borg can't natively read the password from a file AFAIK. Might be an interesting PR for Borg though! > If there is no credential, systemd handles this gracefully, but cat throws an error which pollutes the log.. Not ideal, but might be a good enough solution. > It should be possible to support multiple passwords, but I've been unable to make that work: That's unfortunate, but IMO you could comment around that for now. > This can probably be adapted to work with the user service, however they don't have fallback directories for that yet, so we would need to hardcode a full path in LoadCredentialEncrypted Do tildes work? Or would the home directory need to be hard-coded too? Might not be a huge issue if it's commented out by default here too.
cvlc12 force-pushed cvlc12-patch-1 from 62222022c3 to fdd33947e4 2024-09-24 04:52:32 +00:00 Compare
Author
Contributor

Alright, here's a new take on this!

I'm using a new variable BORG_CRED in the service file, so as not to stomp over anyone's previous settings.

Actually it's even easier than that... $CREDENTIALS_DIRECTORY is inherited.. the only necessary step is to load the credential...!

And no need to edit the service file anymore, just uncomment or write down encryption_passcommand: "cat ${CREDENTIALS_DIRECTORY}/borgpw" in a configuration file.

I tried adding this as a second example to the encryption_passcommand section of the main config file, however this might not work as expected, I know nothing about yaml files :)

It should be possible to support multiple passwords, but I've been unable to make that work:

Still working on this

This can probably be adapted to work with the user service, however they don't have fallback directories for that yet, so we would need to hardcode a full path in LoadCredentialEncrypted

Do tildes work? Or would the home directory need to be hard-coded too? Might not be a huge issue if it's commented out by default here too.

For now this does not seem to work. See https://github.com/systemd/systemd/issues/33796

What do you think about this new approach ?

Alright, here's a new take on this! ~~I'm using a new variable `BORG_CRED` in the service file, so as not to stomp over anyone's previous settings.~~ Actually it's even easier than that... $CREDENTIALS_DIRECTORY is inherited.. the only necessary step is to load the credential...! And no need to edit the service file anymore, just uncomment or write down `encryption_passcommand: "cat ${CREDENTIALS_DIRECTORY}/borgpw" ` in a configuration file. I tried adding this as a second example to the `encryption_passcommand` section of the main config file, however this might not work as expected, I know nothing about yaml files :) > It should be possible to support multiple passwords, but I've been unable to make that work: Still working on this > This can probably be adapted to work with the user service, however they don't have fallback directories for that yet, so we would need to hardcode a full path in LoadCredentialEncrypted > > Do tildes work? Or would the home directory need to be hard-coded too? Might not be a huge issue if it's commented out by default here too. For now this does not seem to work. See https://github.com/systemd/systemd/issues/33796 What do you think about this new approach ?
cvlc12 added 1 commit 2024-09-24 05:14:20 +00:00
Author
Contributor

Alright, even got the multiple passwords to work..

# mkdir /etc/credstore.encrypted/borgpw
# systemd-ask-password -n | systemd-creds encrypt --name=borgpw_1 - /etc/credstore.encrypted/borgpw/1
# systemd-ask-password -n | systemd-creds encrypt --name=borgpw_2 - /etc/credstore.encrypted/borgpw/2

The service file needs to have

LoadCredentialEncrypted=borgpw:/etc/credstore.encrypted/borgpw

Then passwords are available as

encryption_passcommand: "cat ${CREDENTIALS_DIRECTORY}/borgpw_1"
...
encryption_passcommand: "cat ${CREDENTIALS_DIRECTORY}/borgpw_2 

Only problem is that the full path needs to be hardcoded in the service file. From man systemd-exec(5):

If an absolute path referring to a directory is specified, every file in that directory (recursively) will be loaded as a separate credential. The ID for each credential will be the provided ID suffixed with "_$FILENAME" (e.g., "Key_file1"). When loading from a directory, symlinks will be ignored.

Slightly less straightforward, but definitely can be useful. Should we go for this approach instead ?

Alright, even got the multiple passwords to work.. ``` # mkdir /etc/credstore.encrypted/borgpw # systemd-ask-password -n | systemd-creds encrypt --name=borgpw_1 - /etc/credstore.encrypted/borgpw/1 # systemd-ask-password -n | systemd-creds encrypt --name=borgpw_2 - /etc/credstore.encrypted/borgpw/2 ``` The service file needs to have ``` LoadCredentialEncrypted=borgpw:/etc/credstore.encrypted/borgpw ``` Then passwords are available as ``` encryption_passcommand: "cat ${CREDENTIALS_DIRECTORY}/borgpw_1" ... encryption_passcommand: "cat ${CREDENTIALS_DIRECTORY}/borgpw_2 ``` Only problem is that the full path needs to be hardcoded in the service file. From man systemd-exec(5): > If an absolute path referring to a directory is specified, every file in that directory (recursively) will be loaded as a separate credential. The ID for each credential will be the provided ID suffixed with "_$FILENAME" (e.g., "Key_file1"). When loading from a directory, symlinks will be ignored. Slightly less straightforward, but definitely can be useful. Should we go for this approach instead ?
Owner

Nice use of borgmatic's environment variables feature here. πŸ˜„ I honestly think either of these approaches could work. If you have a use case for multiple passphrases, then maybe go with that.

Nice use of borgmatic's environment variables feature here. πŸ˜„ I honestly think either of these approaches could work. If you have a use case for multiple passphrases, then maybe go with that.
witten reviewed 2024-10-01 21:02:12 +00:00
@ -235,1 +235,4 @@
example: "secret-tool lookup borg-repository repo-name"
description: |
Uncomment to use an encrypted systemd service credential (/etc/credstore.encrypted/borgpw).
example: "cat ${CREDENTIALS_DIRECTORY}/borgpw"
Owner

I don't think you can have two descriptions and two examples for a given option. So maybe just work the additional description into the existing one, maybe including the new example there? And/or relying on documentation rather than the schema to document this feature?

BTW you can test this with scripts/dev-docs if you have Docker Compose installed. Even just running tests with tox will exercise the code path that interprets the schema. (No Docker Compose required that way.)

More info at: https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/

I don't think you can have two descriptions and two examples for a given option. So maybe just work the additional description into the existing one, maybe including the new example there? And/or relying on documentation rather than the schema to document this feature? BTW you can test this with `scripts/dev-docs` if you have Docker Compose installed. Even just running tests with `tox` will exercise the code path that interprets the schema. (No Docker Compose required that way.) More info at: https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/
Author
Contributor

Indeed the tox tests failed, but they seem to run OK with one description but two examples. Does that look ok ?

Indeed the tox tests failed, but they seem to run OK with one description but two examples. Does that look ok ?
Owner

Unfortunately I don't think it works. Even it tests pass, the following won't work with those changes:

$ borgmatic config generate -d test.yaml --overwrite
Generating a configuration file at: test.yaml
...
  File "/home/witten/.local/pipx/venvs/borgmatic/lib/python3.12/site-packages/ruamel/yaml/constructor.py", line 278, in check_mapping_key
    raise DuplicateKeyError(*args)
ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping
  in "/home/witten/code/borgmatic/borgmatic/config/schema.yaml", line 228, column 9
found duplicate key "example" with value "cat ${CREDENTIALS_DIRECTORY}/borgpw" (original value: "secret-tool lookup borg-repository repo-name")
  in "/home/witten/code/borgmatic/borgmatic/config/schema.yaml", line 236, column 9
...

That error happens because standard YAML doesn't allow duplicate keys. What I recommend doing instead is to work any secondary examples into the description. It's not ideal, but at least it'd be valid YAML. πŸ˜„ Alternatively, or in addition, your documentation changes might be sufficient for documenting the systemd approach.

(BTW even if unit/integrations tests pass with the duplicate key, I'd expect end-to-end tests to properly catch it and fail.)

Unfortunately I don't think it works. Even it tests pass, the following won't work with those changes: ``` $ borgmatic config generate -d test.yaml --overwrite Generating a configuration file at: test.yaml ... File "/home/witten/.local/pipx/venvs/borgmatic/lib/python3.12/site-packages/ruamel/yaml/constructor.py", line 278, in check_mapping_key raise DuplicateKeyError(*args) ruamel.yaml.constructor.DuplicateKeyError: while constructing a mapping in "/home/witten/code/borgmatic/borgmatic/config/schema.yaml", line 228, column 9 found duplicate key "example" with value "cat ${CREDENTIALS_DIRECTORY}/borgpw" (original value: "secret-tool lookup borg-repository repo-name") in "/home/witten/code/borgmatic/borgmatic/config/schema.yaml", line 236, column 9 ... ``` That error happens because standard YAML doesn't allow duplicate keys. What I recommend doing instead is to work any secondary examples into the description. It's not ideal, but at least it'd be valid YAML. πŸ˜„ Alternatively, or in addition, your documentation changes might be sufficient for documenting the systemd approach. (BTW even if unit/integrations tests pass with the duplicate key, I'd expect [end-to-end tests](https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/#end-to-end-tests) to properly catch it and fail.)
Author
Contributor

Ok, so I removed the example and linked to the docs. I used the following to fit the URL in:

https://yaml.org/spec/1.2-old/spec.html#style/flow/double-quoted

Ok, so I removed the example and linked to the docs. I used the following to fit the URL in: https://yaml.org/spec/1.2-old/spec.html#style/flow/double-quoted
Owner

Sorry to keep going back and forth on this, but unfortunately I don't think this works either! Here's what ends up in the generated config file:

# The standard output of this command is used to unlock the encryption
# key. Only use on repositories that were initialized with
# passcommand/repokey/keyfile encryption. Note that if both
# encryption_passcommand and encryption_passphrase are set, then
# encryption_passphrase takes precedence. This can also be used to
# access encrypted systemd service credentials (see "https://torsion\
# .org/borgmatic/docs/how-to/provide-your-passwords/#using-systemd-\
# service-credentials"). Defaults to not set.
# encryption_passcommand: secret-tool lookup borg-repository repo-name

I think the problem is that that the double quotes are already embedded in a |-defined string. (YAML is kind of complex...) Rather than try to fight YAML on this, I'd instead recommend doing something like:

            ...
            encryption_passphrase takes precedence. This can also be used to
            access encrypted systemd service credentials. Defaults to not set.
            For more details, see:
            https://torsion.org/borgmatic/docs/how-to/provide-your-passwords/

It's not ideal in that it doesn't have the direct link to the section in question, but it has the distinct benefit of resulting in a working URL in the generated config.

Sorry to keep going back and forth on this, but unfortunately I don't think this works either! Here's what ends up in the generated config file: ```yaml # The standard output of this command is used to unlock the encryption # key. Only use on repositories that were initialized with # passcommand/repokey/keyfile encryption. Note that if both # encryption_passcommand and encryption_passphrase are set, then # encryption_passphrase takes precedence. This can also be used to # access encrypted systemd service credentials (see "https://torsion\ # .org/borgmatic/docs/how-to/provide-your-passwords/#using-systemd-\ # service-credentials"). Defaults to not set. # encryption_passcommand: secret-tool lookup borg-repository repo-name ``` I think the problem is that that the double quotes are already embedded in a `|`-defined string. (YAML is kind of complex...) Rather than try to fight YAML on this, I'd instead recommend doing something like: ``` ... encryption_passphrase takes precedence. This can also be used to access encrypted systemd service credentials. Defaults to not set. For more details, see: https://torsion.org/borgmatic/docs/how-to/provide-your-passwords/ ``` It's not ideal in that it doesn't have the direct link to the section in question, but it has the distinct benefit of resulting in a working URL in the generated config.
cvlc12 marked this conversation as resolved
cvlc12 force-pushed cvlc12-patch-1 from 125401e416 to 0da21cd4cb 2024-10-03 17:26:59 +00:00 Compare
Author
Contributor

Nice use of borgmatic's environment variables feature here. πŸ˜„ I honestly think either of these approaches could work. If you have a use case for multiple passphrases, then maybe go with that.

It's probably nice to go with the multiple password approach and hardcode the path for now while they sort a couple things out (https://github.com/systemd/systemd/issues/34542, https://github.com/systemd/systemd/issues/33887). And then eventually update it to be more generic.

Is 'borgpw' ok for the credential folder name ? Or should it just be borgmatic ?

> Nice use of borgmatic's environment variables feature here. πŸ˜„ I honestly think either of these approaches could work. If you have a use case for multiple passphrases, then maybe go with that. It's probably nice to go with the multiple password approach and hardcode the path for now while they sort a couple things out (https://github.com/systemd/systemd/issues/34542, https://github.com/systemd/systemd/issues/33887). And then eventually update it to be more generic. Is 'borgpw' ok for the credential folder name ? Or should it just be borgmatic ?
cvlc12 added 1 commit 2024-10-03 19:19:01 +00:00
cvlc12 added 1 commit 2024-10-03 19:26:30 +00:00
cvlc12 force-pushed cvlc12-patch-1 from 521a75320c to aefc9d61ba 2024-10-03 19:28:51 +00:00 Compare
cvlc12 added 1 commit 2024-10-04 09:51:40 +00:00
cvlc12 added 1 commit 2024-10-04 09:56:19 +00:00
cvlc12 force-pushed cvlc12-patch-1 from 665cf85b9e to 4e833e147a 2024-10-04 09:57:10 +00:00 Compare
cvlc12 force-pushed cvlc12-patch-1 from 4e833e147a to 7eb925dfbc 2024-10-04 10:25:39 +00:00 Compare
cvlc12 changed title from WIP: Load systemd encrypted credentials to Load systemd encrypted credentials 2024-10-04 10:27:25 +00:00
Author
Contributor

Alright, sorry for doing this in so many stages, but I believe this last take is the right approach. This allows using either one password, or multiple.

The hardcoded path for multiple passwords is necessary for now but can probably be removed later on.

Removed WIP from the title as I think it's ready now

Alright, sorry for doing this in so many stages, but I believe this last take is the right approach. This allows using either one password, or multiple. The hardcoded path for multiple passwords is necessary for now but can probably be removed later on. Removed WIP from the title as I think it's ready now
cvlc12 force-pushed cvlc12-patch-1 from 7eb925dfbc to fd91eebeda 2024-10-04 10:32:38 +00:00 Compare
Owner

Is 'borgpw' ok for the credential folder name ? Or should it just be borgmatic ?

Personally, I'd prefer borgmatic just because these are the systemd credentials for the borgmatic systemd service (even if borgmatic happens to pass them to Borg).

I'll have a look at the rest of the changes!

> Is 'borgpw' ok for the credential folder name ? Or should it just be borgmatic ? Personally, I'd prefer `borgmatic` just because these are the systemd credentials for the borgmatic systemd service (even if borgmatic happens to pass them to Borg). I'll have a look at the rest of the changes!
witten reviewed 2024-10-08 18:19:47 +00:00
@ -32,0 +57,4 @@
```yaml
encryption_passcommand: "cat ${CREDENTIALS_DIRECTORY}/borgpw"
encryption_passcommand: "cat ${CREDENTIALS_DIRECTORY}/borg_backupserver1"
Owner

This does make me wonder if you can just put all credentials inside a directory, even in the single credential case. Then your instructions/procedure would be the same regardless of the number of credentials.

Do not feel strongly.

This does make me wonder if you can just put _all_ credentials inside a directory, even in the single credential case. Then your instructions/procedure would be the same regardless of the number of credentials. Do not feel strongly.
Author
Contributor

Yes I considered that as well. I decided not to because I guess most people just use a single password, and the "foldername_filename" thing systemd came up with for the credential name when using multiple credentials might throw some people off. But I don't mind reconsidering if you think that's easier.

Yes I considered that as well. I decided not to because I guess most people just use a single password, and the "foldername_filename" thing systemd came up with for the credential name when using multiple credentials might throw some people off. But I don't mind reconsidering if you think that's easier.
Author
Contributor

The other reason is that it makes it easier to propagate the password/credential if using only one, since the path is not hardcoded. So borgmatic.pw will automatically be checked for in e.g. /run/credstore.encrypted in addition to /etc/credstore.encrypted/, etc..

The other reason is that it makes it easier to propagate the password/credential if using only one, since the path is not hardcoded. So `borgmatic.pw` will automatically be checked for in e.g. `/run/credstore.encrypted` in addition to `/etc/credstore.encrypted/`, etc..
Owner

I'm totally fine with it as is!

I'm totally fine with it as is!
cvlc12 marked this conversation as resolved
witten reviewed 2024-10-08 18:21:23 +00:00
@ -29,6 +29,37 @@ For example, to ask the *Pass* password manager to provide the passphrase:
encryption_passcommand: pass path/to/borg-repokey
```
### Using systemd service credentials
Owner

Nice docs! I appreciate you writing this up.

Nice docs! I appreciate you writing this up.
cvlc12 marked this conversation as resolved
cvlc12 added 1 commit 2024-10-13 13:23:45 +00:00
cvlc12 added 1 commit 2024-10-13 13:38:05 +00:00
cvlc12 force-pushed cvlc12-patch-1 from 12879bda2e to 5280de86ff 2024-10-13 13:42:26 +00:00 Compare
witten approved these changes 2024-10-22 15:57:49 +00:00
witten left a comment
Owner

I'm gonna merge this just so we don't have to do another round of back and forth, and then I'll adjust the URL in the config file as per my comment. Thanks so much for sticking with this!

I'm gonna merge this just so we don't have to do another round of back and forth, and then I'll adjust the URL in the config file as per my comment. Thanks so much for sticking with this!
witten merged commit ed957a940a into main 2024-10-22 15:58:42 +00:00
Author
Contributor

I'm gonna merge this just so we don't have to do another round of back and forth, and then I'll adjust the URL in the config file as per my comment. Thanks so much for sticking with this!

Great ! Thanks

> I'm gonna merge this just so we don't have to do another round of back and forth, and then I'll adjust the URL in the config file as per my comment. Thanks so much for sticking with this! Great ! Thanks
Owner

Released in borgmatic 1.9.0!

Released in borgmatic 1.9.0!
Owner

It sounds like this change errors when encrypted credentials aren't present on disk: #929

Is that expected, and can you think of any work-arounds other than leaving it commented out by default? Thanks.

It sounds like this change errors when encrypted credentials aren't present on disk: #929 Is that expected, and can you think of any work-arounds other than leaving it commented out by default? Thanks.
Author
Contributor

I can confirm that credentials are gracefully skipped when missing:
nov. 07 16:23:30 host (sd-mkdcre[3448]: Couldn't read inherited credential 'borgmatic.pw', skipping: No such file or directory

However this is not the case with the credential folder
LoadCredentialEncrypted=borgmatic:/etc/credstore.encrypted/borgmatic/

If the folder is empty, everything is gracefully ignored. But if the folder is missing, then it breaks...

I've filed an issue to systemd https://github.com/systemd/systemd/issues/35077

I see two options,

  1. Forcefully create the /etc/credstore.encrypted/borgmatic/ dir
  2. Comment out the folder from the service file and have people only use it after creating the folder. Or just remove the option for multiple credentials until the systemd folks sort out the feature.

Anyway, the missing borgmatic.pw credential should not error out, and there should be no error at all if the the borgmatic folder exists.

I can confirm that credentials are gracefully skipped when missing: `nov. 07 16:23:30 host (sd-mkdcre[3448]: Couldn't read inherited credential 'borgmatic.pw', skipping: No such file or directory` However this is not the case with the credential **folder** `LoadCredentialEncrypted=borgmatic:/etc/credstore.encrypted/borgmatic/` If the folder is empty, everything is gracefully ignored. But if the folder is missing, then it breaks... I've filed an issue to systemd https://github.com/systemd/systemd/issues/35077 I see two options, 1. Forcefully create the `/etc/credstore.encrypted/borgmatic/` dir 2. Comment out the folder from the service file and have people only use it after creating the folder. Or just remove the option for multiple credentials until the systemd folks sort out the feature. Anyway, the missing borgmatic.pw credential should not error out, and there should be no error at all if the the borgmatic folder exists.
cvlc12 deleted branch cvlc12-patch-1 2024-11-08 21:08:18 +00:00
Sign in to join this conversation.
No Reviewers
No Milestone
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#902
No description provided.