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

Ignore 502 error on helo - fixes communication with some servers #32

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

qnikst
Copy link
Contributor

@qnikst qnikst commented Nov 23, 2021

Fixes #31.

@MangoIV
Copy link
Collaborator

MangoIV commented Jan 7, 2025

can you provide reasoning why this is always okay? I have found some mixed results when looking this up - i.e. I saw that servers should normally never not provide HELO but the reason was that clients might be incompatible - so I don't quite know if that makes this required.
Thank you :)

@qnikst
Copy link
Contributor Author

qnikst commented Jan 8, 2025

According to RFC 2821 old servers may support HELO and reply promptly on that (with HELO):

Older client SMTP systems MAY, as discussed above, use HELO (as specified in RFC 821) instead of EHLO, and servers MUST support the HELO command and reply properly to it. In any event, a client MUST issue HELO or EHLO before starting a mail transaction.

So we have few options:

  1. Drop such servers (I’m not sure it’s ok)
  2. Try such servers as in commit above but to not fail. Maybe we need better policy - try EHLO and then only if server does not know it issue HELO
  3. Provide an ability for the user to define that the server is old and we should try HELO.

I find the option 2 the most appropriate. However change of the logic to issue EHLO first looks like a sane improvement

@MangoIV
Copy link
Collaborator

MangoIV commented Jan 8, 2025

try EHLO and then only if server does not know it issue HELO

do you want to implement that? otherwise I think this is fine and I'll merge as is.

@qnikst
Copy link
Contributor Author

qnikst commented Jan 9, 2025

No, not sure that I’ll do it soon. I think it’s fine to merge, as it improves the status quo!

@MangoIV
Copy link
Collaborator

MangoIV commented Jan 9, 2025

Thank you.

@MangoIV MangoIV merged commit 22b782c into dpwright:master Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS fails to authorise if the server stopped supporting HELO command
2 participants