Add support for sending email via AWS SES#5910
Add support for sending email via AWS SES#5910txase wants to merge 2 commits intodani-garcia:mainfrom
Conversation
|
Why does this need a custom implementation? AWS SES also provides an SMTP endpoint which you should already be able to use? |
@septatrix The AWS SES SMTP endpoint only supports static credential authentication (doc link). Static credentials add operational overhead (e.g. generating them the first time, keeping them safe, managing rotations, etc.), and are less secure relative to temporary session credentials. This PR uses the AWS SES SDK that supports temporary session credentials via the SES API instead of the SMTP endpoint. |
|
Note for reviewers: If #5917 is merged, I will update this PR to use the same approach to re-use the reqwest library for SES SDK calls. |
53109d9 to
1fd3cb6
Compare
|
I just pushed up a rebased and updated commit now that #5917 has been merged. This only change is that we now also use the built-in reqwest client implementation for AWS SDK calls, which significantly reduces the packages brought in as dependencies. @dani-garcia @BlackDex: This PR is once again ready for review and merging, thanks! |
src/mail.rs
Outdated
| #[cfg(not(ses))] | ||
| err!("Failed to send email", "Failed to send email using AWS SES: `ses` feature is not enabled"); |
There was a problem hiding this comment.
I would suggest to move this check to the config.rs file around line 927 where it starts with if cfg._enable_smtp {
There we also check if sendmail is enabled that the binary exists or not.
Checking if the feature is enabled their would prevent a system running thinking to use SES while that feature isn't enabled.
There was a problem hiding this comment.
Ahh, good point. I'll fix this up!
There was a problem hiding this comment.
I just noticed that the PR already has a check at line 970-972 of config.rs:
...
} else if cfg.use_aws_ses {
#[cfg(not(ses))]
err!("`USE_AWS_SES` is set, but the `ses` feature is not enabled in this build");
} else {
...This particular code is unreachable. I could change it to unreachable!() to make this more clear, or I could simply delete this line of code. What do you think makes more sense?
There was a problem hiding this comment.
Um, i think it can be removed in a way that this should probably never be called at all.
The validation is done during startup and admin config save, and thus should never ever be able to reach it.
And i see no point in having a redundant check here too.
sorry that i missed the config.rs part though.
There was a problem hiding this comment.
I tried removing the code, and then realized that you have to return from that else if branch. I then tried to figure out syntax to make that else if branch conditionally configured, but I don't think there's a simple syntax to do so without modifying the structure of the entire send_with_selected_transport() function.
Instead, I changed the err!() call to an unreachable!() call. This will at least make it clear to those reading the code that this branch should never be called upon in this circumstance.
Thoughts?
src/mail.rs
Outdated
| #[cfg(not(ses))] | ||
| err!("Failed to send email", "Failed to send email using AWS SES: `ses` feature is not enabled"); |
There was a problem hiding this comment.
Um, i think it can be removed in a way that this should probably never be called at all.
The validation is done during startup and admin config save, and thus should never ever be able to reach it.
And i see no point in having a redundant check here too.
sorry that i missed the config.rs part though.
|
Better use something like https://github.com/lezgomatt/gen_mailer (or https://github.com/lettre/lettre with a custom transport) |
|
|
|
@MOZGIII @septatrix: It sounds like you are proposing an alternative approach to using the AWS SES client directly, as this PR does. At first glance, it looks like lettre itself should be useful for sending an email via SES. There are two possible approaches to do so:
For these reasons I decided to implement the SES transport more straightforwardly as a direct method in mail.rs. |
|
While implementing the trait does not offer any additional value it makes the code cleaner and easier to reason about. The only reason why it is currently possible to just implement and ad-hoc method cleanly is because for each transport the errors are handled differently with a giant match statement. If that code were to be refactored sometime in the future it would be significantly easier to do that with the trait approach |
|
I agree that some of the code could be refactored to simplify if all the transports were lettre async transport implementations. In this PR my goal was to disturb as little existing code as possible. If the maintainers would rather a larger refactor to simplify the code (e.g. making the transport a Box around a dynamic implementation of the lettre async transport trait) as part of this PR, then I can look into it. That could also just as easily be done in a future PR. |
This is clearer to those reading code on expectations.
This code adds AWS SES (Simple Email Service) as an additional (optionally enabled) email transport. It uses the same default AWS SDK config that the s3 file support uses.
Requirements for use
sesorawsfeatureUSE_AWS_SES=trueSMTP_FROM=<sending email address>AWS_PROFILE=<profile name>(If not using a default AWS config profile. You may also use any other standard AWS env vars to configure SDK credentials.)