Skip to content

chore: add comment about SMTP PLAIN authentication - #4741

Merged
SuperQ merged 1 commit into
prometheus:mainfrom
TheMeier:issues/1236
Dec 5, 2025
Merged

chore: add comment about SMTP PLAIN authentication#4741
SuperQ merged 1 commit into
prometheus:mainfrom
TheMeier:issues/1236

Conversation

@TheMeier

Copy link
Copy Markdown
Contributor

Ref: #1236

@SoloJacobs SoloJacobs left a comment

Copy link
Copy Markdown
Contributor

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.

@TheMeier

Copy link
Copy Markdown
Contributor Author

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.
I can also break out that commit into another PR.

@SoloJacobs SoloJacobs left a comment

Copy link
Copy Markdown
Contributor

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 semver perspective 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.
Comment thread config/config.go Outdated
@@ -451,6 +451,9 @@ func (c *Config) UnmarshalYAML(unmarshal func(any) error) error {
ec.RequireTLS = new(bool)
*ec.RequireTLS = c.Global.SMTPRequireTLS
}
if (ec.AuthUsername != "" || ec.AuthPassword != "" || ec.AuthPasswordFile != "") && !*ec.RequireTLS {

Copy link
Copy Markdown
Contributor

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"                                  
} 

Copy link
Copy Markdown
Contributor Author

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 <christoph.maser+github@gmail.com>
@TheMeier

Copy link
Copy Markdown
Contributor Author

So about that validation. I think a more correct implementation would be to use something like this:

			if !*ec.RequireTLS && (ec.AuthUsername != "" || ec.AuthPassword != "" || ec.AuthPasswordFile != "") {
				if ip := net.ParseIP(ec.Smarthost.Host); ip != nil && !ip.IsLoopback() {
					return errors.New("PLAIN SMTP authentication without TLS can only be used with loopback (aka localhost) addresses")
				}
			}

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 issue I see here is, whatever validation we implement is prone to become different at some point if the implementation in net/smtp changes in the future. So I also don't feel very happy to just copy their isLocalhost implementation.

@TheMeier
TheMeier requested a review from SoloJacobs November 18, 2025 17:43
@SoloJacobs

Copy link
Copy Markdown
Contributor

The validation also seems ok to me. Shame that there is no public function to check that SMTP is safe.

@SuperQ
SuperQ merged commit 913b386 into prometheus:main Dec 5, 2025
7 checks passed
@SoloJacobs SoloJacobs mentioned this pull request Dec 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants