Finer SMTP TLS certificate control

[JosefSchoenberger]

This PR adds two new SMTP configurations regarding TLS:

• SMTP_ADDITIONAL_ROOT_CERTS allows an admin to add new root certificates that Vaultwarden accepts for the SMTP server. This can be useful if the SMTP server only offers a self-signed certificate. Example: SMTP_ADDITIONAL_ROOT_CERTS=/etc/ssl/certs/mail1.pem;/etc/ssl/certs/mail2.pem
• SMTP_USE_SYSTEM_ROOT_CERTS disables the system's root certificate store for TLS. This can be used in combination with SMTP_ADDITIONAL_ROOT_CERTS for certificate pinning.

[williamdes]

Very interesting feature!

[BlackDex]

Looks fine by me. This would even work if we want to switch to rustls instead of native.
Just a few comments.

[BlackDex]

I think it is better to have this checked in config.rs.
There it should check upon loading the config if those certs exists or not and and there it may panic.
Causing a panic here is probably not really wanted in my opinion.

Same goes for validating of those certs are valid certs.
So best to move this to the validate_config function there i think.

[JosefSchoenberger]

I agree that panicing only upon sending the first mail is not appropriate.

I'm not sure if I understand you correctly. I'm not familiar with Vaultwarden's codebase, but how should I proceed? I currently see three different options. Which do you prefer? Do you have something else in mind?

1. SMTP_ADDITIONAL_ROOT_CERTS remains remain a static global variable, just that it is moved into config.rs and that verify_config reads it first, such that the Lazy evaluates instantly upon config verification. I dislike this option since (a) it uses a global variable and (b) any calls to update_config never actually reload the certificate (something that I missed in this pull request, too).
2. I can also move the reading and parsing into verify_config and store the resulting certificate list in an internal Option config item upon success. That's fine considering that SMTP_ADDITIONAL_ROOT_CERTS is an Option anyway. The only disadvantage is that the name verify_config becomes misleading as a verification function typically has no side effects.
3. I can also create a new internal gen config option of type Result<Option<Vec<Certificate>>, Box<dyn Error>> in which the generating function read and parse the certificates. Any errors that occur there are then stored as an Box<dyn Error> which is then caught in validate_config. The disadvantage of this approach is that the config is of type Result, yet it is never Err during runtime, so any uses of this config option will always need an additional call to unwrap first.

What is your opinion on that matter? Am I missing something totally obvious?

[BlackDex]

@JosefSchoenberger any interest in following up on the questions? Else i tend to close this PR as it also is possible to add/overwrite allowed certificates as a whole within a container for example.

If you want to continue, then i think it would even make sense to add this same kind of functionality to the reqwest part of the code, and not only link it to SMTP and make it more general.

This could help with company wide transparent proxies maybe. But again, someone can also add custom certs and use the vaultwarden.sh.d startup functionality to add custom CA's to the global CA Store of the container before Vaultwarden actually starts.

[JosefSchoenberger]

Yes, I do. I thought I ended this conversation with a question, hence no further work from me. Anyway, I'm trying the third option now, but there might by some difficulties with complex types in the configuration.

Else i tend to close this PR as it also is possible to add/overwrite allowed certificates as a whole within a container for example.

I think its still better to have these configuration options, setting it up that way is far easier (just setting one or two envs and ro-bind-mounting the certificate file into the container – both works fine in a compose file, for example).

If you want to continue, then i think it would even make sense to add this same kind of functionality to the reqwest part of the code, and not only link it to SMTP and make it more general.

I don't have a lot of time right now. For now, I'd just like to finish the SMTP part. They should have separate configuration variables (the certificates might differ between the mail server and other servers) and the internal setup for reqwest is a bit different, so that works fine as a separate PR, IMHO.

[JosefSchoenberger]

Alright. Option 3 did not work, as lettre's Certificate is not (de)serializable. Anyway, I went with Option 2 now. Feel free to review the first commit again. The second commit is identical to the previous version.