Add support for reading password from a different file

[dvaerum]

Hey fho, I hope you are open for pull requests 😁

I started to use rspamd-iscan, but I am handling my secret separately from my config file. So I make this little patch that allows rspamd-iscan to read the password for RspamdPasswordFile and ImapPasswordFile from a separate file.
I hope that you will accept it.

[fho]

Hello @dvaerum,

I hope you are open for pull requests 😁

Yes, I'm, thanks for your PR! :-)
When supporting reading credentials from another file, I think we should aim for making it work nicely together with systemd credentials (https://systemd.io/CREDENTIALS/).
This means either:

• support to specify the credentials file via the environment variable CREDENTIALS_DIRECTORY or
• support specifying the credentials via a command-line parameter.

Because rspamd-iscan doesn't support env. variables currently but has cmdline parameters, we should add a parameter to specify a separate credentials file (rspamd-iscan --credentials-file).
The file could also be in TOML format and support to specify the IMAP and rspamd credentials.

What do you think about the approach?

Is there any special reason that the PR introduces to read the IMAP and rspamd credentials from different files, instead of the same?

[dvaerum]

Yes, I'm, thanks for your PR! :-) When supporting reading credentials from another file, I think we should aim for making it work nicely together with systemd credentials (https://systemd.io/CREDENTIALS/). This means either:

* support to specify the credentials file via the environment variable `CREDENTIALS_DIRECTORY` or

This does sound like a good idea, but I am a little unsure about the names of the files in the directory, but I guess that one could just use ImapPassword and RspamdPassword for file names. When if they exist they overwrite what is in the config.toml file as an example.

* support specifying the credentials via a command-line parameter.

Because rspamd-iscan doesn't support env. variables currently but has cmdline parameters, we should add a parameter to specify a separate credentials file (rspamd-iscan --credentials-file).

I am not sure if I understand what you are thinking here?

The file could also be in TOML format and support to specify the IMAP and rspamd credentials.

Are you thinking of using a toml-file for storing credencials?

What do you think about the approach?

I like the idea of implementing support for CREDENTIALS_DIRECTORY, maybe having an argument --credentials-directory that takes its default value from the environment variable CREDENTIALS_DIRECTORY, but the argument idea is mostly for having more options for configuration.

Is there any special reason that the PR introduces to read the IMAP and rspamd credentials from different files, instead of the same?

I use sops-nix in NixOS to handle my secrets, and it writes each secret to one file, so for me this is just the easiest way to handle it, without having a bash script read the files and reformat them. sops-nix does seem to support some type of template, which would solve this, but the last time I tried to use it, it did not succeed 😅

[fho]

Yes, I'm, thanks for your PR! :-) When supporting reading credentials from another file, I think we should aim for making it work nicely together with systemd credentials (https://systemd.io/CREDENTIALS/). This means either:

* support to specify the credentials file via the environment variable `CREDENTIALS_DIRECTORY` or

This does sound like a good idea, but I am a little unsure about the names of the files in the directory, but I guess that one could just use ImapPassword and RspamdPassword for file names. When if they exist they overwrite what is in the config.toml file as an example.

Reusing the fieldNames from the toml file also makes sense to me, great idea!
I think we should also support reading ImapUser from a credential file.
It isn't as sensitive but would allow to keep the entire IMAP auth data external to e.g. reuse the same config for different IMAP users.

* support specifying the credentials via a command-line parameter.

Because rspamd-iscan doesn't support env. variables currently but has cmdline parameters, we should add a parameter to specify a separate credentials file (rspamd-iscan --credentials-file).

I am not sure if I understand what you are thinking here?

https://systemd.io/CREDENTIALS/ mentions that ideally applications would simply use the credentials from the environment variable $CREDENTIALS_DIRECTORY when it is defined. An alternative is to pass the path via a cmdline parameter to the application.
I had in mind, to prevent that we introduce a 3rd variant for configuration (cmdline paramters, toml file, environment variable), we pass the path via a command line parameter.

The file could also be in TOML format and support to specify the IMAP and rspamd credentials.

Are you thinking of using a toml-file for storing credencials?

Yes, was thinking about sharing a single file for all credentials.

What do you think about the approach?

I like the idea of implementing support for CREDENTIALS_DIRECTORY, maybe having an argument --credentials-directory that takes its default value from the environment variable CREDENTIALS_DIRECTORY, but the argument idea is mostly for having more options for configuration.

Is there any special reason that the PR introduces to read the IMAP and rspamd credentials from different files, instead of the same?

I use sops-nix in NixOS to handle my secrets, and it writes each secret to one file, so for me this is just the easiest way to handle it, without having a bash script read the files and reformat them. sops-nix does seem to support some type of template, which would solve this, but the last time I tried to use it, it did not succeed 😅

I see! I thought having a single file instead of multiple with credentials is easier to maintain.
But seems the other way when using the right tools.
Using a single credential per file seems also the preferred way for the the systemd credentials support.
I agree with you that using a single per credential would then be better!

[dvaerum]

Okay, in summary, we want something like this?

• Use --credentials-directory to set the directory.
• If the credentials directory is defined, overwrite the settings RspamdURL, RspamdPassword, ImapUser and ImapPassword in the TOML config with the content of a file with the same name if the file exists.

[fho]

@dvaerum Yes

[dvaerum]

@fho have a look 😉

[fho]

@dvaerum thanks!
I only managed to have a quick look and it looks good. I'll have more thoroughly look soonish.
It would be amazing if you could also add a testcase, otherwise I can also AI generate one during my review. :-)

[dvaerum]

I will take a look at making some tests. I am currently running the code on my server, so it cannot be completely broken 😉

[dvaerum]

@fho I have added some simple unittests for the patch.

[fho]

@dvaerum I'll have a look soon

[fho]

@dvaerum sorry, took a bit long, thanks for the PR! :-)

[dvaerum]

No worries, I was already using the code so no rush from my side 😁 but don’t get me wrong I am happy to see it merged 😉