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

From: Jari Aalto <jari.aalto@cante.net>
Date: Mar 14, 2010, 10:19 UTC
Message-ID: <87d3z7dx2y.fsf@jondo.cante.net>
In-Reply-To: <7vtysjs9hi.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 8 quoted lines

> 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.

Show 20 quoted lines

> 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.
>
>> +    --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.

Show 21 quoted lines

>> @@ -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.

Show 11 quoted lines

>  - various callsites of maildomain sub do not have to repeat the fallback
>    default like your patch does.
>
>> @@ -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