(#17827) Properly format SMTP HELO when sending tagmail #1351

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@ahpook
Contributor

ahpook commented Dec 28, 2012

Previously, the tagmail report sending code did not initialize
Net::SMTP.start with a 'helo' option, which causes securely-
configured SMTP servers to reject the mail.

This commit adds settings to control the destination SMTP port
and the value for SMTP HELO, which default to 25 and our fqdn,
respectively.

Report and original patch from Jim Pirzyk.

(#17827) Properly format SMTP HELO when sending tagmail
Previously, the tagmail report sending code did not initialize
Net::SMTP.start with a 'helo' option, which causes securely-
configured SMTP servers to reject the mail.

This commit adds settings to control the destination SMTP port
and the value for SMTP HELO, which default to 25 and our fqdn,
respectively.

Report and original patch from Jim Pirzyk.
@slippycheeze

This comment has been minimized.

Show comment
Hide comment
@slippycheeze

slippycheeze Dec 28, 2012

Contributor

You mean "incorrectly configured SMTP servers", not securely, and you should probably document why this might be interesting to users in the configuration description. Just having a strictly factual statement of effect, but no meat on why makes this difficult for users to understand.

Part of the reason I emphasise "incorrectly configured" is because some-but-not-all of the SMTP servers that require the HELO / EHLO name to be an FQDN will also reject an unresolvable, or even "no MX exists for", string.

Given that using an internal name, like FQDN on an internal machine, might reject just as much as using the RFC-specified "opaque string for loopback detection" would.

Contributor

slippycheeze commented Dec 28, 2012

You mean "incorrectly configured SMTP servers", not securely, and you should probably document why this might be interesting to users in the configuration description. Just having a strictly factual statement of effect, but no meat on why makes this difficult for users to understand.

Part of the reason I emphasise "incorrectly configured" is because some-but-not-all of the SMTP servers that require the HELO / EHLO name to be an FQDN will also reject an unresolvable, or even "no MX exists for", string.

Given that using an internal name, like FQDN on an internal machine, might reject just as much as using the RFC-specified "opaque string for loopback detection" would.

@ahpook

This comment has been minimized.

Show comment
Hide comment
@ahpook

ahpook Dec 28, 2012

Contributor

My experience with HELO checking might be different to yours Daniel -- in Postfix at least, there's a range of helo restrictions varying from the are-you-malware low bar to entry ('must exist', 'must look like a fqdn') to the pretty-clearly-insane ('look up the MX record for the provided hostname and perform some action against it'). So I don't think it's true that all helo restrictions are inherently misconfigurations.

Point taken, and commit added, for adding 'why' to the description though.

Contributor

ahpook commented Dec 28, 2012

My experience with HELO checking might be different to yours Daniel -- in Postfix at least, there's a range of helo restrictions varying from the are-you-malware low bar to entry ('must exist', 'must look like a fqdn') to the pretty-clearly-insane ('look up the MX record for the provided hostname and perform some action against it'). So I don't think it's true that all helo restrictions are inherently misconfigurations.

Point taken, and commit added, for adding 'why' to the description though.

lib/puppet/defaults.rb
+ :default => Facter["fqdn"].value,
+ :desc => "The name by which we identify ourselves in SMTP HELO for reports.
+ If you send to a smtpserver which does strict HELO checking (as with Postfix's
+ `smtpd_helo_restrictions` access controls), you may to ensure this resolves.",

This comment has been minimized.

@slippycheeze

slippycheeze Dec 28, 2012

Contributor

I thing you have a tyop in there: "you may to ensure"

@slippycheeze

slippycheeze Dec 28, 2012

Contributor

I thing you have a tyop in there: "you may to ensure"

@slippycheeze

This comment has been minimized.

Show comment
Hide comment
@slippycheeze

slippycheeze Dec 28, 2012

Contributor

My experience with HELO checking might be different to yours Daniel -- in
Postfix at least, there's a range of helo restrictions varying from the
are-you-malware low bar to entry ('must exist', 'must look like a fqdn') to
the pretty-clearly-insane ('look up the MX record for the provided hostname
and perform some action against it'). So I don't think it's true that all
helo restrictions are inherently misconfigurations.

"Looks like FQDN" and "must resolve" are both violations of the spec -
at least, the traditional 821 and 822 pair; they happen to work
because of a coincidence of implementation of most current MTA and
MUAs, but they are definitely not quite right. This has led to
various problems over the years when, eg, a laptop uses the "FQDN" of
"${hostname}", and is rejected by some-but-not-all SMTP servers.

Anyway, yeah, it is a popular, if incorrect, check, and supporting
working around it is a win. :0

Contributor

slippycheeze commented Dec 28, 2012

My experience with HELO checking might be different to yours Daniel -- in
Postfix at least, there's a range of helo restrictions varying from the
are-you-malware low bar to entry ('must exist', 'must look like a fqdn') to
the pretty-clearly-insane ('look up the MX record for the provided hostname
and perform some action against it'). So I don't think it's true that all
helo restrictions are inherently misconfigurations.

"Looks like FQDN" and "must resolve" are both violations of the spec -
at least, the traditional 821 and 822 pair; they happen to work
because of a coincidence of implementation of most current MTA and
MUAs, but they are definitely not quite right. This has led to
various problems over the years when, eg, a laptop uses the "FQDN" of
"${hostname}", and is rejected by some-but-not-all SMTP servers.

Anyway, yeah, it is a popular, if incorrect, check, and supporting
working around it is a win. :0

@puppetcla

This comment has been minimized.

Show comment
Hide comment
@puppetcla

puppetcla Mar 21, 2013

CLA Signed by ahpook on 2012-08-16 21:00:00 -0700

CLA Signed by ahpook on 2012-08-16 21:00:00 -0700

@adrienthebo

This comment has been minimized.

Show comment
Hide comment
@adrienthebo

adrienthebo May 24, 2013

Contributor

@ahpook @daniel-pittman I'm dragging this pull request back from the dead to see if we can get a resolution on this. In my uninformed opinion, this seems like a pretty harmless change. Merging this shouldn't break anything, and while it may be working around the deficiencies/configuration of mail servers it's a small change and could make the lives of some users a bit easier. Could we get a definitive yay/nay on this?

Thanks!

Contributor

adrienthebo commented May 24, 2013

@ahpook @daniel-pittman I'm dragging this pull request back from the dead to see if we can get a resolution on this. In my uninformed opinion, this seems like a pretty harmless change. Merging this shouldn't break anything, and while it may be working around the deficiencies/configuration of mail servers it's a small change and could make the lives of some users a bit easier. Could we get a definitive yay/nay on this?

Thanks!

@ahpook

This comment has been minimized.

Show comment
Hide comment
@ahpook

ahpook May 25, 2013

Contributor

i favor taking it obvs. i interpret daniel's last comment "supporting working around [mail server settings] is a win" as support.

Contributor

ahpook commented May 25, 2013

i favor taking it obvs. i interpret daniel's last comment "supporting working around [mail server settings] is a win" as support.

@adrienthebo

This comment has been minimized.

Show comment
Hide comment
@adrienthebo

adrienthebo May 29, 2013

Contributor

summary: merged into master in 8dbe436; this should be released in 3.3.0. There was a syntax incompatibility with ruby 1.8.7, so I rebased this on master and made the syntax amendment. Thanks for the contribution!

Contributor

adrienthebo commented May 29, 2013

summary: merged into master in 8dbe436; this should be released in 3.3.0. There was a syntax incompatibility with ruby 1.8.7, so I rebased this on master and made the syntax amendment. Thanks for the contribution!

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