Add dest option to systemd_creds_encrypt

[mks-h]

SUMMARY

Add support for specifying the output file in systemd_creds_encrypt, since it is the only way to create an encrypted systemd credential stored in a file for use with LoadCredentialEncrypted option in systemd service files. Writing the regular output of systemd_creds_encrypt into a file using the ansible.builtin.copy module does not work, since the output is different to what systemd-creds writes to the file.

Adds the dest option to control the "output" argument of the underlying "systemd-creds" command.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

systemd_creds_encrypt

ADDITIONAL INFORMATION

     --name=name
           When specified with the encrypt command controls the credential name to embed in the encrypted credential data. If not
           specified, the name is chosen automatically from the filename component of the specified output path. If specified as
           empty string no credential name is embedded in the encrypted credential, and no verification of credential name is done
           when the credential is decrypted.

[ansibullbot]

cc @konstruktoid
click here for bot help

[felixfontein]

Thanks for your contribution!

This parameter changes the behavior of the module quite a lot: before the module simply encrypted a secret and returned it. It was up to the user to decide whether the encryption needs to be done, and there is no idempotency check.

With a dest parameter, the common expectation for an Ansible module is that the module performs an idempotency check. This is not done by the module right now. Also check mode is not respected: the destination file is always written, even in check mode.

This likely needs some more changes to the module and to the documentation.

[mks-h]

The last commit is implemented wrong, I'll be redoing it. I'm currently blocked on this bug in Fedora — I can't verify whether systemd-creds cat (or list) command gives any useful metadata about options used to create the credential (such as timestamp, not-after, user). I'll continue on maybe next weekend by trying to run these commands in containers of other distributions.

[russoz]

Hi @mks-h

Thanks for your contribution! I second @felixfontein concerns about idempotency, but also adding a couple of comments here.

[russoz]

I think it would be interesting to add one (or more) testcase that expose what happens when name="".

[konstruktoid]

Perhaps add an decryption test/options as well? That's https://github.com/ansible-collections/community.general/blob/main/plugins/modules/systemd_creds_decrypt.py on the other hand

[konstruktoid]

or https://github.com/ansible-collections/community.general/pull/10549/files#diff-b0129a525eebd2b35f800b7d9ef6b272d5824976e0b9e42f5050892c87774542R144 is perhaps enough

[russoz]

Hi @mks-h thanks for your continued work on this.

Left another couple of comments, and @konstruktoid also left comments in the test, but I must confess I did not delve into those. If you could be as kind as to take a look :-)

Other than these minor things, it is looking good to me.

[russoz]

I think this note might be better placed in dest below - both cases described required that dest is set.

Also, maybe a more affirmative wording might help. Something like (inserted be low in dest's description):

- When this is set and O(name=""), then the credential_name is XXXXX (not clear to me the right answer here)
- When this is set and O(name) is not passed, then credential name will take the value of O(dest).
- When this is set and O(name) is also set with a non-empty name, then credential name will take the value of O(name) (???? will it???)

Or similar - take this with a pinch of salt.

[russoz]

What do you think?

[mks-h]

The only way I can prove idempotence is to create a new secret each time, and see if the resulting file differs from the existing one. This should result in the files being the same so long as the "timestamp" is explicitly provided. If it isn't, each encryption will result in a different file, since the default is current time.

[felixfontein]

How about decrypting the encrypted file and comparing its contents with the original file?

In any case, I'm wondering whether this should better be a new module (systemd_creds_encrypt_file) instead of a new part of the systemd_creds_encrypt module, since the new functionality behaves quite different from the existing.

[mks-h]

How about decrypting the encrypted file and comparing its contents with the original file?

This only verifies the contents, not the attributes. There's no way to read the embedded attributes from a secret. Plus, I'd say it is safer to not decrypt sensitive data — the principle of least privilege, in a way.

In any case, I'm wondering whether this should better be a new module (systemd_creds_encrypt_file) instead of a new part of the systemd_creds_encrypt module, since the new functionality behaves quite different from the existing.

The current functionality doesn't change, it only extends to work with more options of the original command.

[felixfontein]

In any case, I'm wondering whether this should better be a new module (systemd_creds_encrypt_file) instead of a new part of the systemd_creds_encrypt module, since the new functionality behaves quite different from the existing.

The current functionality doesn't change, it only extends to work with more options of the original command.

There is a very fundamental change: before the module was doing an action. Now it is managing a file. That's quite different.

[mks-h]

Or it can still just do an action, if you don't use the dest option.

Either way, I'm afraid I'm out of ideas on how to verify that the existing secret was created with the same parameters. The credentials may contain salt, because their hashes are different each time you create one — even if you specify all the same parameters explicitly.

[felixfontein]

Please note that this PR now has a conflict since we reformatted all code in the collection (see #10999).