add support for system mta though sendmail

[soruh]

This PR enables using lettre's SendmailTransport in Vaultwarden.

It adds two configuration options:
- USE_SENDMAIL wether to use sendmail instead of directly connecting to a SMTP server
- SENDMAIL_COMMAND what sendmail command to use. By default the one from the $PATH is used, but I think there are cases where it makes sense to specify a different one

Supporting the system MTA allows for more flexible mail configuration without too much effort and does not require packaging any sendmail client as assumed in #2198.

While there are other usecases like sending through a local mailserver without having to create a dedicated SMTP Account as mentioned in #2198, my specific usecase for this is the following:

I have msmtp configured with an account that all applications use without me having to paste the SMTP configuration and credentials into each of their configs and allows me to only have to maintain the one SMTP configuration instead of potentially forgetting one (not fun).

Testing can be done as follows:

> sudo pacman -S msmtp-mta
> sudo tee /etc/msmtprc << EOF
defaults

account default
auth on
from <from>
host <hostname>
password <passowrd>
tls on
user <username>
EOF
> tee .env << EOF
USE_SENDMAIL = true
SMTP_FROM = "bitwarden@tld"
EOF
> cargo run [...] # run vaultwarden

log into vaultwarden, click
User icon in top right > account settings > security > Two-step login > Email > Manage > Send Email

[BlackDex]

I think there are some adjustments missing here:

vaultwarden/src/config.rs

Lines 1046 to 1049 in 9991563

	pub fn mail_enabled(&self) -> bool {
	let inner = &self.inner.read().unwrap().config;
	inner._enable_smtp && inner.smtp_host.is_some()
	}

[soruh]

I think there are some adjustments missing here:

vaultwarden/src/config.rs

Lines 1046 to 1049 in 9991563

	pub fn mail_enabled(&self) -> bool {
	let inner = &self.inner.read().unwrap().config;
	inner._enable_smtp && inner.smtp_host.is_some()
	}

Fixed in 41faff5

[BlackDex]

Could you btw also add these variables to the .env.template with the same comments :)

[soruh]

Oh, I missed that. Might be because it was hidden :)

[BlackDex]

Ok, i just tested this locally, and it seems to work fine.

Though it does add extra config items, i do think this could be useful for some people.
I know it has been requested in the past. And since there are more and more standalone builds made of Vaultwarden, like for Arch Linux, Alpine, Nixos, FreeBSD, Void etc... this could be useful.

There is just one item i think we may need to address. If someone provides SENDMAIL_COMMAND i think it should be the full path to the executable, and therefor, i think we should validate if that file exists or not during the config validation. This will prevent users from configuring this variable, but a non working environment.

Other then that, LGTM.

[soruh]

Thank you, that's a good point. I'll add that when I get home.

[soruh]

It looks like the SENDMAIL_COMMAND is ultimately passed into tokio::process::Command so if it's an absolute path it will just be executed but if it's not it will be looked up in the $PATH. I see two options to check validity:

• force the command to be an absolute path and check if the file exists and is executable
• allow commands in the $PATH and figure out a way to check if the command is valid (like spawning a tokio::process::Command with the command and checking for ENOENT)

I think the latter is definitely more involved, but there could be use cases where it's needed. Do you have any preference, or a better way to do this?

[soruh]

A third option would be to actually iterate through the PATH as suggested here but that doesn't seem reasonable to me.

[soruh]

I think I would prefer forcing an absolute path

[BlackDex]

I would say the first. Make a path mandatory to be absolute.
For one, it feels safer to me and if someone wants to override it, they probably know the full path.

Checking if it is executable is going to be difficult. That isn't supported on Windows. And probably to much i think.

[soruh]

Great, I agree. I'll implement that when I'm home then.

[soruh]

07f754b adds the check. If you think the check for executability is too complicated I can remove it again.

[soruh]

I have just pushed 82452b3 which adds the check using the which crate. If there is a consensus on what to do we can either revert it for the old behavior or keep it.

[soruh]

I feel like it would be unfortunate for this to get stuck on something that feels pretty minor to me. I would really like this to be in the next version when it is released as I need it myself :). Is there a way a consensus can be found for this? Maybe a compromise of not using the which crate for now, but having the configuration description be more adamant about having sendmail installed would be acceptable?

[BlackDex]

I might have some time to check this this evening (UTC). But you do need to rebase this PR it looks like.

[soruh]

Do you know if there is a way for github to automatically squash PR commits or should I clean up the commit history a bit?

[BlackDex]

Not that i know. I do stuff like that via the cli with a reset --soft.

[soruh]

Thank's alot for your help on this @BlackDex!

[soruh]

Curious that git thinks there is a conflict here. I think it assumes my new function is the same as the function added on main. I've rebased it now...

[dani-garcia]

Yeah, I had trouble with git conflicts while trying to batch merge all the PR's manually, not sure why. Ended up merging them one by one.

We've just relicensed the project as AGPL but this PR was started before the license change was merged, so i'd appreciate if you can add your comment to the relicensing agreement, even if it's just a small formality by now:
#2450

Thanks for your contribution!

[soruh]

done