-
Notifications
You must be signed in to change notification settings - Fork 2.4k
chore: add comment about SMTP PLAIN authentication #4741
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.
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
base: main
Are you sure you want to change the base?
Conversation
0878152 to
e905308
Compare
SoloJacobs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I have been thinking whether we should also a validation step to amtool check-config. This should be possible, since the validation done by PlainAuth is straightforward.
|
So I have added a commit that implements validation if TLS is disabled and SMTP PLAIN auth config params are set. But I would consider this somewhat breaking since it will prevent AM starting with some (broken) configs which it previously did accept. |
9f3aea4 to
30a3702
Compare
SoloJacobs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't know about our backwards compatibility requirements. Hopefully one of the maintainers can help out. I think from a
semverperspective this is fine, and catching a notification not being sent out is pretty important in my opinion. - I would definitely allow
localhost. - If you have the time, then I would split the PR. The doc change is ready to merge in my mind.
config/config.go
Outdated
| ec.RequireTLS = new(bool) | ||
| *ec.RequireTLS = c.Global.SMTPRequireTLS | ||
| } | ||
| if (ec.AuthUsername != "" || ec.AuthPassword != "" || ec.AuthPasswordFile != "") && !*ec.RequireTLS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we still allow localhost? This is how PlainAuth checks it:
func isLocalhost(name string) bool {
return name == "localhost" || name == "127.0.0.1" || name == "::1"
} There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the validation and will open another PR for it. Thanks for your feedback @SoloJacobs
Ref: prometheus#1236 Signed-off-by: Christoph Maser <[email protected]>
30a3702 to
5fcde0f
Compare
|
So about that validation. I think a more correct implementation would be to use something like this: But that of cause is different than https://cs.opensource.google/go/go/+/refs/tags/go1.25.4:src/net/smtp/auth.go;l=67 |
|
The validation also seems ok to me. Shame that there is no public function to check that SMTP is safe. |
Ref: #1236