Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

Junio C Hamano [off-list ref] writes:

So please explain why asking your local MTA (either mailhost or localhost)
how it is configured to identify to other MTA is a good way to obtain the
HELO domain you should give $smtp_server.  Your answer could be "I expect
that most people use $smtp_server set to 'mailhost' or 'localhost' and
have the MTA configured to talk with the outside world." (which by the way
I do not think is true for most people).  Or it could be "Xsmtp MTA that
is used by many people to send out mails through their ISP's mailserver
uses the same trick to solve this issue".

The code is based on Perl library Test::Reporter

        http://search.cpan.org/~dagolden/Test-Reporter-1.55/lib/Test/Reporter.pm

        /usr/share/perl5/Test/Reporter/Mail/Util.pm ==> sub _maildomain ()

    Function _maildomain() in Test::Reporter uses much more intesive
    tests by scanning all kinds of files, like checking sendmail(1),
    smail(1) etc. configuration files before querying the MTA. I used
    only a portion of it, those parts that I was confortable with to
    handle.

Where to read the FQDN?

    git-send-email contacts MTA to see that it thinks the caller host is
    named, because -- to my knowledge -- the FQDN information is not
    readily available elsewhere in all systems. The MTA does reverse IP
    lookup (DNS) for git-send-email to read.

Why FQDN is passed to MTA?

    Tightly configured MTAs require that the caller gives a real DNS
    domain name that corresponds the IP address in the handshake. This
    prevents spammers from trying to hide their identity.

Side note.  Apparently this seems to be a common issue.  For example,
msmtp has this configurable via command line and configuration option:

   domain argument
          Use  this  command to set the argument of the SMTP EHLO (or LMTP
          LHLO) command.  The default is localhost (stupid, but  working).
          Possible  choices  are  the  domain  part  of  your mail address
          (provider.example for joe@provider.example) or the fully  quali‐
          fied domain name of your host (if available).

Interestingly, one of the suggestions above is to derive it from the From
address.

In any case, I want to hear your justification.

quoted

+    --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake

I think this addition makes sense. I suspect that we would also want to
have sendemail.smtpdomain configuration variable.  They of course need to
be documented.

Sure.

+my $MAIL_DOMAIN;			# See maildomain()
Why uppercase? 

It might be good to convert all global variables to de facto UPPERCASE.
What do you think? (a separate patch?).

+        return $MAIL_DOMAIN if $MAIL_DOMAIN;
+
+	my $maildomain;
Looks like a funny indent here...

SP vs. TAB in diff. Fixed.

Please drop extra SP immediately after '('

Done.

But as I already said, I tend to think the logic implemented by this part
is of dubious validity.

Does the above explation address this? I concluded that if the Perl
Module developers use MTA to find out the FQDN, it must be a working
solution. Especially as the code is in the Test:: module libraries.

quoted

@@ -917,6 +962,8 @@ X-Mailer: git-send-email $gitversion
 		}
 	}
 
+	my $maildomain;
+
 	if ($dry_run) {
 		# We don't want to send the email.
 	} elsif ($smtp_server =~ m#^/#) {

@@ -936,13 +983,18 @@ X-Mailer: git-send-email $gitversion

...
+			$maildomain = maildomain() || "localhost.localdomain";
+			$smtp ||= Net::SMTP::SSL->new($smtp_server,
+						      Hello => $maildomain,
+						      Port => $smtp_server_port);

Why not use the same structure as how lifetime is defined for $smtp
variable?  IOW,

			$mail_domain || = maildomain();
			$smtp ||= ...

This is now completely different. See new maildomain().

without an extra local $maildomain variable?  If you do so, then

 - You can lose the $maildomain variable local to this function;

It's needed in the error message (see variable's scope):

		if (!$smtp) {
			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
			    "VALUES: server=$smtp_server ",
			    "encryption=$smtp_encryption ",
!!			    "maildomain=$maildomain",
			    defined $smtp_server_port ? "port=$smtp_server_port" : "";

 - "return what the user configured" does not have to be in the maildomain
   sub;

Refactored.

 - the maildomain sub can return "localhost.localdomain" as a fallback
   default; and

Refactored.
 

 - various callsites of maildomain sub do not have to repeat the fallback
   default like your patch does.

quoted

@@ -962,9 +1014,10 @@ X-Mailer: git-send-email $gitversion
 		}
 
 		if (!$smtp) {
-			die "Unable to initialize SMTP properly. Check config. ",
+			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",

This part is a good addition, but it needs to be in the earlier patch, no?

The --smtp-debug was introduced in the current patch along with maildomain().

See next message for a reworked patch.
Jari