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.

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

Send FQDN instead of simple hostname in SMTP HELO command #100

Closed
wants to merge 0 commits into from

Conversation

mguessan
Copy link

@mguessan mguessan commented Sep 3, 2019

Some SMTP servers block messages when HELO command contains a simple hostname instead of a fully qualified domain name.

This path fixes this by using getCanonicalHostName instead of getHostName

@asf-ci
Copy link

asf-ci commented Sep 3, 2019

Can one of the admins verify this patch?

1 similar comment
@asf-ci
Copy link

asf-ci commented Sep 3, 2019

Can one of the admins verify this patch?

@bodewig
Copy link
Member

bodewig commented Sep 3, 2019

this is ok to test

@asf-ci
Copy link

asf-ci commented Sep 3, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Linux/129/

@asf-ci
Copy link

asf-ci commented Sep 3, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Windows/135/

@jaikiran
Copy link
Member

jaikiran commented Sep 3, 2019

The test failures being reported in those runs, for org.apache.tools.mail.MailMessageTest look related. The tests expect a different host name which no longer matches the one being sent.
I haven't yet had a look at the existing code/tests, but we will have to review if this backward incompatibility is OK or if we should make this configurable and default to the previous behaviour.

@asf-ci
Copy link

asf-ci commented Sep 3, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Windows/136/

@asf-ci
Copy link

asf-ci commented Sep 3, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Linux/130/

@mguessan
Copy link
Author

mguessan commented Sep 4, 2019

MailMessageTest is now fixed, however there are still 4 test failures. They don't seem related to this PR.

@twogee
Copy link
Contributor

twogee commented Sep 4, 2019

Looking at the specs, shouldn't the task be made RFC-5321-compliant? For unless I am mistaken, RFC-821 says HELO must use domain...

@twogee
Copy link
Contributor

twogee commented Sep 4, 2019

Looking further, RFC-821 states "The argument field contains the host name of the sender-SMTP." and the formal syntax suggests that "domain" means FQDN. RFC-2821 clarifies

  The argument field contains the fully-qualified domain name
   of the SMTP client if one is available.  In situations in which the
   SMTP client system does not have a meaningful domain name (e.g., when
   its address is dynamically allocated and no reverse mapping record is
   available), the client SHOULD send an address literal (see section
   4.1.3), optionally followed by information that will help to identify
   the client system. 

RFC-5321 deprecates the use of address literals.

@mguessan
Copy link
Author

mguessan commented Sep 9, 2019

Ok, so basically getCanonicalHostName returns exactly what we need, right ?

@twogee
Copy link
Contributor

twogee commented Sep 9, 2019

Yes, and we should change HELO to EHLO.

@asf-ci
Copy link

asf-ci commented Sep 12, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Linux/131/

@asf-ci
Copy link

asf-ci commented Sep 12, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Windows/137/

@asf-ci
Copy link

asf-ci commented Sep 12, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Linux/132/

@asf-ci
Copy link

asf-ci commented Sep 12, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ant%20Github-PR-Windows/138/

@twogee
Copy link
Contributor

twogee commented Sep 12, 2019

May I suggest placing the changes on a separate branch to allow for an easier rebase?

@mguessan
Copy link
Author

mguessan commented Sep 13, 2019

Sure, not very comfortable with git yet...

I will probably have to create a new PR as this one is linked to master on my side (my mistake), right ?

... or not:
https://help.github.com/en/articles/changing-the-base-branch-of-a-pull-request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants