Finer SMTP TLS certificate control #4385
Conversation
|
Very interesting feature! |
| static SMTP_ADDITIONAL_ROOT_CERTS: Lazy<Option<Vec<Certificate>>> = Lazy::new(|| { | ||
| Some( | ||
| CONFIG | ||
| .smtp_additional_root_certs()? | ||
| .split(';') | ||
| .filter(|path| !path.is_empty()) | ||
| .map(|path| { | ||
| let cert = std::fs::read(path) | ||
| .unwrap_or_else(|e| panic!("Error loading additional SMTP root certificate file {path}.\n{e}")); | ||
| Certificate::from_pem(&cert).unwrap_or_else(|e| panic!("Error decoding certificate file {path}.\n{e}")) | ||
| }) | ||
| .collect(), | ||
| ) | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
SMTP_ADDITIONAL_ROOT_CERTSremains remain a static global variable, just that it is moved intoconfig.rsand thatverify_configreads it first, such that theLazyevaluates instantly upon config verification. I dislike this option since (a) it uses a global variable and (b) any calls toupdate_confignever actually reload the certificate (something that I missed in this pull request, too).- I can also move the reading and parsing into
verify_configand store the resulting certificate list in an internalOptionconfig item upon success. That's fine considering thatSMTP_ADDITIONAL_ROOT_CERTSis anOptionanyway. The only disadvantage is that the nameverify_configbecomes misleading as a verification function typically has no side effects. - I can also create a new internal
genconfig option of typeResult<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 anBox<dyn Error>which is then caught invalidate_config. The disadvantage of this approach is that the config is of typeResult, yet it is neverErrduring runtime, so any uses of this config option will always need an additional call tounwrapfirst.
What is your opinion on that matter? Am I missing something totally obvious?
This PR adds two new SMTP configurations regarding TLS:
SMTP_ADDITIONAL_ROOT_CERTSallows 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.pemSMTP_USE_SYSTEM_ROOT_CERTSdisables the system's root certificate store for TLS. This can be used in combination withSMTP_ADDITIONAL_ROOT_CERTSfor certificate pinning.