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

From: Jeff King <peff@peff.net>
Date: Feb 16, 2012, 00:49 UTC
Message-ID: <20120216004903.GA21170@sigill.intra.peff.net>
In-Reply-To: <CAN8TOE_BnkOcMQRTY-GWrHozYD0+0giWn2LtjB8AVnP_DzA+Sg@mail.gmail.com>

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

Show 5 quoted lines

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

Show 13 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

> 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