Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow custom email sender #695

Merged
merged 4 commits into from
Jul 11, 2022
Merged

Allow custom email sender #695

merged 4 commits into from
Jul 11, 2022

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Jul 7, 2022 •
edited
Loading

Allow the user to provide a custom email sender for alerting.
Relates to trento-project/helm-charts#31

@nelsonkopliku nelsonkopliku mentioned this pull request Jul 7, 2022
Merged
dottorblaster
dottorblaster approved these changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

CDimonaco
CDimonaco approved these changes Jul 7, 2022
View reviewed changes
Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

rtorrero
rtorrero approved these changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

fabriziosestito
fabriziosestito reviewed Jul 7, 2022
View reviewed changes
config/runtime.exs Outdated
@@ -102,6 +102,7 @@ if config_env() == :prod do

config :trento, :alerting,
enabled: System.get_env("ENABLE_ALERTING", "false") == "true",
sender: System.get_env("ALERT_SENDER") || "[email protected]",
Copy link
Member

@fabriziosestito fabriziosestito Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

System.get_env("ALERT_SENDER", "[email protected]")

Copy link
Member Author

@nelsonkopliku nelsonkopliku Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right 😄
Changed also the recipient to

System.get_env("ALERT_RECIPIENT", "[email protected]")

f2faf69

@nelsonkopliku nelsonkopliku force-pushed the allow_custom_email_sender branch from f847d74 to 1523afd Compare July 8, 2022 06:59
@nelsonkopliku nelsonkopliku force-pushed the allow_custom_email_sender branch from 1523afd to 4b65214 Compare July 8, 2022 07:21
@stefanotorresi
Copy link
Member

Since we're touching this code, do you think it could be feasible to make both recipient and sender follow the customary mailbox format name <address> according to https://datatracker.ietf.org/doc/html/rfc5322#section-3.4, so that we users can customize the entire header value in one shot?

@nelsonkopliku
Copy link
Member Author

nelsonkopliku commented Jul 8, 2022 •
edited
Loading

Since we're touching this code, do you think it could be feasible to make both recipient and sender follow the customary mailbox format name <address> according to https://datatracker.ietf.org/doc/html/rfc5322#section-3.4, so that we users can customize the entire header value in one shot?

Not an expert here, but I am wondering whether that could open to some sort of security issue where bad things could be injected 🤔
If so, that would then add the complexity of validation.

It might be interesting allowing that customization, yet I believe the added value is not enough compared to the security risks, if any, and the validation added complexity.

@nelsonkopliku nelsonkopliku merged commit 2ab491e into main Jul 11, 2022
@nelsonkopliku nelsonkopliku deleted the allow_custom_email_sender branch July 11, 2022 13:21
@arbulu89 arbulu89 added the enhancement New feature or request label Jul 14, 2022
@stefanotorresi
Copy link
Member

stefanotorresi commented Jul 18, 2022 •
edited
Loading

Not an expert here, but I am wondering whether that could open to some sort of security issue where bad things could be injected thinking If so, that would then add the complexity of validation.

It might be interesting allowing that customization, yet I believe the added value is not enough compared to the security risks, if any, and the validation added complexity.

I was expecting the validation to be done by Swoosh, but after I looked it up it seems it doesn't support mailbox headers, so I guess the answer to my question is "no, it's not easily feasible". I would never want you to re-implement RFC5322 email header validation, that's what frameworks should do for us. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CDimonaco CDimonaco CDimonaco approved these changes

@rtorrero rtorrero rtorrero approved these changes

@fabriziosestito fabriziosestito fabriziosestito approved these changes

@dottorblaster dottorblaster dottorblaster approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nelsonkopliku @stefanotorresi @dottorblaster @fabriziosestito @rtorrero @CDimonaco @arbulu89