[PATCH] git-send-email: allow overriding smtp-encryption config to 'none'

[PATCH] git-send-email: allow overriding smtp-encryption config to 'none'

[Brian Norris]

According to the manpage:

 --smtp-encryption=<encryption>
        Specify the encryption to use, either ssl or tls. Any other value
	reverts to plain SMTP.  Default is the value of
        sendemail.smtpencryption.

However, if I have already set sendemail.smtpencryption but try to override
it with something like 'no', the authentication code block still tries to ask
for a password (and fails).

This patch forces $smtp_encryption to 'none' when a proper encryption type is
not provided, then checks $smtp_encryption before proceeding to authentication.

Example execution:

 $ git send-email --smtp-encryption=no --smtp-domain=<xxx> \
        --smtp-server=<xxx> --smtp-server-port=25 myfile.patch
 Password:
 Command unknown: 'AUTH' at /usr/local/libexec/git-core/git-send-email
 line 1115, <STDIN> line 1.

Tested on Git 1.7.5.4 and 1.7.9.1.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 git-send-email.perl |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index ef30c55..fa0a384 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -385,8 +385,9 @@ foreach my $setting (values %config_bool_settings) {
 	${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));
 }
 
-# 'default' encryption is none -- this only prevents a warning
-$smtp_encryption = '' unless (defined $smtp_encryption);
+# 'default' encryption is none
+$smtp_encryption = 'none' unless (defined $smtp_encryption);
+$smtp_encryption = 'none' unless ($smtp_encryption eq 'tls' || $smtp_encryption eq 'ssl');
 
 # Set CC suppressions
 my(%suppress_cc);
@@ -1113,7 +1114,7 @@ X-Mailer: git-send-email $gitversion
 			    defined $smtp_server_port ? " port=$smtp_server_port" : "";
 		}
 
-		if (defined $smtp_authuser) {
+		if (defined $smtp_authuser && $smtp_encryption ne 'none') {
 			# Workaround AUTH PLAIN/LOGIN interaction defect
 			# with Authen::SASL::Cyrus
 			eval {
-- 
1.7.5.4

Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'

[Jeff King]

On Wed, Feb 15, 2012 at 01:42:58PM -0800, Brian Norris wrote:

> According to the manpage:
> 
>  --smtp-encryption=<encryption>
>         Specify the encryption to use, either ssl or tls. Any other value
> 	reverts to plain SMTP.  Default is the value of
>         sendemail.smtpencryption.
> 
> However, if I have already set sendemail.smtpencryption but try to override
> it with something like 'no', the authentication code block still tries to ask
> for a password (and fails).

Sounds reasonable.

> This patch forces $smtp_encryption to 'none' when a proper encryption type is
> not provided, then checks $smtp_encryption before proceeding to authentication.

Defaulting everything except "ssl" or "tls" to "none" seems risky to me.
If I am understanding your patch correctly, then doing this:

  git send-email --smtp-encryption=SSL

will silently treat that as "don't do encryption", which could have
surprising security implications for the user. I chose all-caps as it is
an obvious mistake to make. We probably should treat it the same as
lowercase "ssl", but the same argument applies to other typos like
"tsl".

It seems like a much safer default would be to die() on an invalid
encryption specifier.

-Peff

Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'

[Brian Norris]

On Wed, Feb 15, 2012 at 2:06 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 15, 2012 at 01:42:58PM -0800, Brian Norris wrote:
>
>> According to the manpage:
>>
>>  --smtp-encryption=<encryption>
>>         Specify the encryption to use, either ssl or tls. Any other value
>>       reverts to plain SMTP.  Default is the value of
>>         sendemail.smtpencryption.
>>
>> However, if I have already set sendemail.smtpencryption but try to override
>> it with something like 'no', the authentication code block still tries to ask
>> for a password (and fails).
>
> Sounds reasonable.

An error like
  Command unknown: 'AUTH' at /usr/local/libexec/git-core/git-send-email
is reasonable?

>> This patch forces $smtp_encryption to 'none' when a proper encryption type is
>> not provided, then checks $smtp_encryption before proceeding to authentication.
>
> Defaulting everything except "ssl" or "tls" to "none" seems risky to me.
> If I am understanding your patch correctly, then doing this:
>
>  git send-email --smtp-encryption=SSL
>
> will silently treat that as "don't do encryption", which could have
> surprising security implications for the user. I chose all-caps as it is
> an obvious mistake to make. We probably should treat it the same as
> lowercase "ssl", but the same argument applies to other typos like
> "tsl".

Well, git-send-email already doesn't handle typos or capitalization
correctly, AFAICT. So nothing new here.

> It seems like a much safer default would be to die() on an invalid
> encryption specifier.

Fine. But then we need to define a behavior that means 'no
encryption.' Like 'none' instead of just saying 'anything but tls or
ssl.'

Now that I look at this again, I think part of the issue I have is
that there is no way to override *smtp-user* via command-line, in
order to do unencrypted, unauthenticated email. So the
*authentication* not the encryption is really my main problem...I'll
take another look and try a new patch.

Thanks,
Brian

Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'

[Jeff King]

On Wed, Feb 15, 2012 at 03:49:59PM -0800, Brian Norris wrote:

> > Sounds reasonable.
> 
> An error like
>   Command unknown: 'AUTH' at /usr/local/libexec/git-core/git-send-email
> is reasonable?

Sorry, no, I meant your goal of allowing overriding config sounds like a
reasonable thing to want. But from reading your message below, it seems
that is not actually the problem you are trying to solve.

> > Defaulting everything except "ssl" or "tls" to "none" seems risky to me.
> > If I am understanding your patch correctly, then doing this:
> >
> >  git send-email --smtp-encryption=SSL
> >
> > will silently treat that as "don't do encryption", which could have
> > surprising security implications for the user. I chose all-caps as it is
> > an obvious mistake to make. We probably should treat it the same as
> > lowercase "ssl", but the same argument applies to other typos like
> > "tsl".
> 
> Well, git-send-email already doesn't handle typos or capitalization
> correctly, AFAICT. So nothing new here.

Hmm. From your description and the patch, I thought that was something
introduced by your patch. But looking at the existing code, it seems
like that is already the case. IOW, I don't understand why
"--smtp-encryption=none" does not already work looking at the current
code.

So being more careful about typos is an improvement we could make, but
it is not a feature that would need to be part of a bugfix patch.

> > It seems like a much safer default would be to die() on an invalid
> > encryption specifier.
> 
> Fine. But then we need to define a behavior that means 'no
> encryption.' Like 'none' instead of just saying 'anything but tls or
> ssl.'

Right. I meant that you should introduce "none" as an explicit "no, I
don't want this" and die when the flag is not one of {ssl, tls, none}.

> Now that I look at this again, I think part of the issue I have is
> that there is no way to override *smtp-user* via command-line, in
> order to do unencrypted, unauthenticated email. So the
> *authentication* not the encryption is really my main problem...I'll
> take another look and try a new patch.

Ah, I see. I misunderstood the original problem you were trying to solve
(I thought your example was "see? Encryption is off, so the server won't
do AUTH, demonstrating that the patch works.").

Overriding the smtp user from the config is a separate issue, and I
don't think that is currently possible. The usual way to spell an option
like that in git is "--no-smtp-user", but it seems that we use perl's
GetOptions, which does not understand that syntax. So you'd have to add
a "--no-smtp-user" by hand.

-Peff

Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'

[Brian Norris]

On Wed, Feb 15, 2012 at 4:49 PM, Jeff King <peff@peff.net> wrote:
> Ah, I see. I misunderstood the original problem you were trying to solve
> (I thought your example was "see? Encryption is off, so the server won't
> do AUTH, demonstrating that the patch works.").

Yeah, I got a little bit off track on what my actual goal was...

> Overriding the smtp user from the config is a separate issue, and I
> don't think that is currently possible. The usual way to spell an option
> like that in git is "--no-smtp-user", but it seems that we use perl's
> GetOptions, which does not understand that syntax. So you'd have to add
> a "--no-smtp-user" by hand.

I think the "--no-smtp-user" is what I really wanted. I've written a
different patch that actually targets the user name properly, but I've
also found a current solution that can work for scripting purposes:
just redirect the $GIT_CONFIG environment variable to /dev/null
temporarily. Perhaps I'll send my new patch sometime, but it's not
pressing and I'm not sure what kind of use it would actually get.

Thanks for the pointers.

Brian

Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'

[Jeff King]

On Fri, Feb 17, 2012 at 09:27:44PM -0800, Brian Norris wrote:

> > Overriding the smtp user from the config is a separate issue, and I
> > don't think that is currently possible. The usual way to spell an option
> > like that in git is "--no-smtp-user", but it seems that we use perl's
> > GetOptions, which does not understand that syntax. So you'd have to add
> > a "--no-smtp-user" by hand.
> 
> I think the "--no-smtp-user" is what I really wanted. I've written a
> different patch that actually targets the user name properly, but I've
> also found a current solution that can work for scripting purposes:
> just redirect the $GIT_CONFIG environment variable to /dev/null
> temporarily.

Just FYI, the fact that doing so works is somewhat accidental. Long ago,
GIT_CONFIG was respected everywhere as an override to stop reading any
other config. Later, it was dropped, but retained its meaning only for
the git-config command, mostly for historical reasons (although these
days one would do better to use "git config -f $file" instead).

So the reason it works for git-send-email is that send-email in turn
calls git-config to actually look at config values, because send-email
is a perl script and not a C program. In other words, the fact that
GIT_CONFIG is respected is a coincidence of some implementation
decisions, not an intended behavior.

I don't think we have any plans for those implementation details to
change in the near future.  So by all means, use it if you like for the
time being. But know that it's not a behavior which is guaranteed not to
change in future versions.

> Perhaps I'll send my new patch sometime, but it's not pressing and I'm
> not sure what kind of use it would actually get.

I think the ideal case would be a patch that teaches the send-email
option parsing code to understand a "--no-*" counterpart for every
option, without having to modify each option individually. I haven't
looked at how easy or hard that would be, though.

-Peff