diff mbox series

[v2,2/3] git-send-email: die on invalid smtp_encryption

Message ID 20210411125431.28971-3-sir@cmpwn.com (mailing list archive)
State New
Headers show
Series git-send-email: improve SSL configuration | expand

Commit Message

Drew DeVault April 11, 2021, 12:54 p.m. UTC
Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
 Documentation/git-send-email.txt | 4 ++--
 git-send-email.perl              | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Ævar Arnfjörð Bjarmason April 11, 2021, 2:20 p.m. UTC | #1
On Sun, Apr 11 2021, Drew DeVault wrote:

> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
>  Documentation/git-send-email.txt | 4 ++--
>  git-send-email.perl              | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index c17c3b400a..520b355e50 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -171,8 +171,8 @@ Sending
>  	Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables
>  	generic SSL/TLS support and is typically used on port 465.  'tls'
>  	enables in-band STARTTLS support and is typically used on port 25 or
> -	587.  Use whichever option is recommended by your mail provider.  Any
> -	other value reverts to plain SMTP.  Default is the value of
> +	587.  Use whichever option is recommended by your mail provider.  Leave
> +	empty to disable encryption and use plain SMTP.  Default is the value of
>  	`sendemail.smtpEncryption`.
>  
>  --smtp-domain=<FQDN>::
> diff --git a/git-send-email.perl b/git-send-email.perl
> index f5bbf1647e..bda5211f0d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -495,6 +495,9 @@ sub read_config {
>  
>  # 'default' encryption is none -- this only prevents a warning
>  $smtp_encryption = '' unless (defined $smtp_encryption);
> +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
> +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
> +}

Having not tested this but just eyeballed the code, I'm fairly sure
you're adding a logic error here, or is $smtp_encryption guaranteed to
be defined at this point?

We start with it as undef, then read the config, then the CLI
options. If we've got neither it'll still be undef here, no?

Thus the string comparison will emit a warning.

Maybe I'm overly used to regexen in Perl, but I'd also find this more
readable as:

    $smtp_encryption !~ /^(?:|ssl|tls)$/s

Or something like:

    my @valid_smtp_encryption = ('', qw(ssl tls));
    if (!grep { $_ eq $smtp_encryption } @valid_smtp_encryption) {
        ....
    }
Drew DeVault April 11, 2021, 2:21 p.m. UTC | #2
On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote:
> >  # 'default' encryption is none -- this only prevents a warning
> >  $smtp_encryption = '' unless (defined $smtp_encryption);
> > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
> > +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
> > +}
>
> Having not tested this but just eyeballed the code, I'm fairly sure
> you're adding a logic error here, or is $smtp_encryption guaranteed to
> be defined at this point?

I will admit to being ignorant of much of Perl's semantics, but I had
assumed that the line prior to my additions addresses this:

$smtp_encryption = '' unless (defined $smtp_encryption);

> $smtp_encryption !~ /^(?:|ssl|tls)$/s

Yeah, that would probably be better.
Ævar Arnfjörð Bjarmason April 11, 2021, 2:30 p.m. UTC | #3
On Sun, Apr 11 2021, Drew DeVault wrote:

> On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote:
>> >  # 'default' encryption is none -- this only prevents a warning
>> >  $smtp_encryption = '' unless (defined $smtp_encryption);
>> > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
>> > +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
>> > +}
>>
>> Having not tested this but just eyeballed the code, I'm fairly sure
>> you're adding a logic error here, or is $smtp_encryption guaranteed to
>> be defined at this point?
>
> I will admit to being ignorant of much of Perl's semantics, but I had
> assumed that the line prior to my additions addresses this:
>
> $smtp_encryption = '' unless (defined $smtp_encryption);

You're right, I just misread the diff. Nevermind.
Ævar Arnfjörð Bjarmason April 11, 2021, 3:06 p.m. UTC | #4
On Sun, Apr 11 2021, Ævar Arnfjörð Bjarmason wrote:

> On Sun, Apr 11 2021, Drew DeVault wrote:
>
>> On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote:
>>> >  # 'default' encryption is none -- this only prevents a warning
>>> >  $smtp_encryption = '' unless (defined $smtp_encryption);
>>> > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
>>> > +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
>>> > +}
>>>
>>> Having not tested this but just eyeballed the code, I'm fairly sure
>>> you're adding a logic error here, or is $smtp_encryption guaranteed to
>>> be defined at this point?
>>
>> I will admit to being ignorant of much of Perl's semantics, but I had
>> assumed that the line prior to my additions addresses this:
>>
>> $smtp_encryption = '' unless (defined $smtp_encryption);
>
> You're right, I just misread the diff. Nevermind.

So on a second reading.

So first, I've been sitting on some fairly extensive send-email patches
locally, but have been trying to focus on re-rolling some of my
outstanding stuff.

But I just sent two patches directly relevant to this series as
https://lore.kernel.org/git/cover-0.2-00000000000-20210411T144128Z-avarab@gmail.com/

Something felt a bit wrong about the approach in your series, I wasn't
quite sure what initially, but here it is;

So, the only reason we have that "encryption is none -- this only
prevents a warning" so late in the file (as opposed to setting it to ''
when we declare the variable) was because of the
smtp.{smtpssl,smtpencryption} interaction, i.e. we relied on it being
undef to see if we needed to parse the secondary variable.

But with it gone with my small series (it already didn't work) we can
get rid of that special case.

But, on the specifics of the "felt funny":

 1. Your 2/3 changes a long standing existing "any other value = no
    encryption" to "die on unrecognized". I happen to think this is
    probably a good idea, but let's be explicit in the commit message,
    e.g.:

        We don't think it's a good idea to silently degrade to
        non-encrypted as we've been promising just because your version
        doesn't support something, let's die instead.

 2. If we're breaking the "any other value" we should not be documenting
    the "or nothing", the distinction between "" and undef on the
    Perl-level was just a leaky implementation detail.

    But let's not conflate that with how we present something to the
    user. It's not the same to not set a variable v.s. setting it to the
    empty string.

    With my 2-part series it's even more trivial to detect that, but
    even on top of master you just move your check above the "set to
    empty unless defined".

 3. While I'm very much leaning to #1 being a good idea, I'm very much
    leaning towards introducing this "starttls" alias being a bad idea
    for the same reason.
    
    I.e. let's not create a new 'starttls' if we can avoid it explicitly
    because we used to have the long-standing "anything unrecognized is
    empty == no encryption" behavior.

    A lot of users read documentation for the latest version online, but
    may have an older version installed.

    To the extent that anyone cares about the transport security of
    git-send-email (I'm kind of "meh" on it, but if we're making
    sendemail.smtpEncryption parsing strict you probably disagree), then
    such silent downgrading seems worse than just not accepting starttls
    at all. I.e. to have a new behavior of something like:

        if (defined $smtp_encryption) {
        	die "we call 'starttls' 'tls' for historical reasons, sorry!" if $smtp_encryption eq 'starttls';
		die "unknown mode '$smtp_encryption'" unless $smtp_encryption =~ /^(?:ssl|tls)$/s;
	} else {
		$smtp_encryption = '';
	}

    I.e. I get that it's confusing, but isn't it enough to address the
    TLS v.s. STARTTLS confusion in the docs, as opposed to supporting it
    in the config format, which as noted will silently downgrade on
    older versions?
Drew DeVault April 11, 2021, 3:18 p.m. UTC | #5
On Sun Apr 11, 2021 at 11:06 AM EDT, Ævar Arnfjörð Bjarmason wrote:
> 3. While I'm very much leaning to #1 being a good idea, I'm very much
> leaning towards introducing this "starttls" alias being a bad idea
> for the same reason.
>     
> i.e. let's not create a new 'starttls' if we can avoid it explicitly
> because we used to have the long-standing "anything unrecognized is
> empty == no encryption" behavior.
>
> A lot of users read documentation for the latest version online, but
> may have an older version installed.

I feel quite strongly that the options here are a grave failure of
usability, and that it needs to be corrected. I help people troubleshoot
git send-email problems quite often, and this is a recurring error.
However, you make a good point in that someone might see some online
documentation which does not match their git version and end up with a
surprisingly unencrypted connection.

As a compromise, let's consider making this a gradual change. We can
start by clarifying the docs and forbiding the use of any value other
than 'ssl' or 'tls'. If an unknown value is set, the user is not getting
the encryption they expected anyway, and this should cause an error.

Then we can leave the issue aside for some agreed upon period of time to
allow the change to proliferate in the ecosystem, and then revisit this
at some point in the future to rename the options to make more sense.

Does this seem like a reasonable compromise?
Ævar Arnfjörð Bjarmason April 11, 2021, 7:56 p.m. UTC | #6
On Sun, Apr 11 2021, Drew DeVault wrote:

> On Sun Apr 11, 2021 at 11:06 AM EDT, Ævar Arnfjörð Bjarmason wrote:
>> 3. While I'm very much leaning to #1 being a good idea, I'm very much
>> leaning towards introducing this "starttls" alias being a bad idea
>> for the same reason.
>>     
>> i.e. let's not create a new 'starttls' if we can avoid it explicitly
>> because we used to have the long-standing "anything unrecognized is
>> empty == no encryption" behavior.
>>
>> A lot of users read documentation for the latest version online, but
>> may have an older version installed.
>
> I feel quite strongly that the options here are a grave failure of
> usability, and that it needs to be corrected. I help people troubleshoot
> git send-email problems quite often, and this is a recurring error.
> However, you make a good point in that someone might see some online
> documentation which does not match their git version and end up with a
> surprisingly unencrypted connection.
>
> As a compromise, let's consider making this a gradual change. We can
> start by clarifying the docs and forbiding the use of any value other
> than 'ssl' or 'tls'. If an unknown value is set, the user is not getting
> the encryption they expected anyway, and this should cause an error.
>
> Then we can leave the issue aside for some agreed upon period of time to
> allow the change to proliferate in the ecosystem, and then revisit this
> at some point in the future to rename the options to make more sense.
>
> Does this seem like a reasonable compromise?

I suggest we don't compromise and just go with whatever you're OK with
:)

I really don't care enough about #1 and #3 in my E-Mail to in any way
push for it, sorry if it came off that way.

I just wanted to check your assumptions when reviewing the series. I do
think that it would make sense to more prominently note something to the
effect of "this was documented to do X all along, now we do Y, but
that's OK because ABC", and to note why the new starttls = plaintext on
older versions is OK, maybe it's just fine. I really don't know.

Isn't it pretty common in any case that SMTP servers in the wild just
refuse plaintext these days when dealing with auth'd connections? I
don't know.

I do think it makes sense to fixup for my suggested #2, i.e. not leaking
the internal detail of the "empty string".
Drew DeVault April 12, 2021, 12:33 p.m. UTC | #7
On Sun Apr 11, 2021 at 3:56 PM EDT, Ævar Arnfjörð Bjarmason wrote:
> I suggest we don't compromise and just go with whatever you're OK with :)

Well, if you're giving me an opportunity to not drag this out into a
multi-phase rollout, then I'll take it :)

Another option is to forbid an unknown value (which is almost certainly
(1) wrong and (2) causing users to unexpectedly use plaintext when they
expected encryption), file a CVE, and pitch it as a security fix - then
we can expect a reasonably quick rollout of the change to the ecosystem
at large.
Ævar Arnfjörð Bjarmason April 12, 2021, 1:16 p.m. UTC | #8
On Mon, Apr 12 2021, Drew DeVault wrote:

> On Sun Apr 11, 2021 at 3:56 PM EDT, Ævar Arnfjörð Bjarmason wrote:
>> I suggest we don't compromise and just go with whatever you're OK with :)
>
> Well, if you're giving me an opportunity to not drag this out into a
> multi-phase rollout, then I'll take it :)

Just to be clear even if I was insisting on that I'm still just one guy
on the ML reviewing your patch.

As a first approximation the opinion of regular contributors counts for
more when the topic is some tricky interaction of code they wrote/are
familiar with.

In this case we're just discussing the general interaction of security,
optional switches, software versioning and how SMTP servers in the wild
work.

I'd think someone who e.g. needs to regularly deal with SMTP servers in
the wild would have a much better idea of those trade-offs than someone
(like me) who happens to have some existing patches in git.git to
git-send-email.perl.

> Another option is to forbid an unknown value (which is almost certainly
> (1) wrong and (2) causing users to unexpectedly use plaintext when they
> expected encryption), file a CVE, and pitch it as a security fix - then
> we can expect a reasonably quick rollout of the change to the ecosystem
> at large.

I think anyone would agree that in retrospect "unknown is plaintext" for
the "what encryption do you want" option is at best a something
approaching a shotgun to your foot of a UI pattern.

But I think it falls far short of a CVE. We *do* prominently document
it, a potential CVE would be if we had silent degration to plaintext
(well, in a mode whose inherent workings aren't to be vulnerable to that
attack, as STARTTLS is...).

FWIW since my upthread <87zgy4egtp.fsf@evledraar.gmail.com> I tried
sending mail through GMail's plain-text smtp gateway as an authenticated
user.

Testing with:

    nc smtp.gmail.com 25
    openssl s_client -connect smtp.gmail.com:465

It will emit a 530 if you try to AUTH in plain-text (telling you to use
STARTTLS), it will also only say "AUTH" in the EHLO response to the
latter.

And indeed Net::SMTP picks up on this, and doesn't even send your
user/password:
https://metacpan.org/release/libnet/source/lib/Net/SMTP.pm#L169

So this hypothetical degradation of the connection and sending auth over
plain-text I suggested in upthread #3 seems to mostly/entirely be a
non-issue as far as e.g. accidentally sending your password on some open
WiFi network goes due to a local misconfiguration.

As long as the SMTP server is functional enough to say it doesn't
support AUTH on plain-text you'll be OK. I'm assuming that these days
with the push for "SSL everywhere" most/all big providers/MTAs have
moved away from supporing plain-text auth by default.
Drew DeVault April 13, 2021, 12:12 p.m. UTC | #9
Can I get one of the maintainers to chime in on this thread and explain,
in their opinion, what this patchset needs before it is acceptable? I'm
not sure where I should go from this discussion.
Ævar Arnfjörð Bjarmason April 13, 2021, 2:22 p.m. UTC | #10
On Tue, Apr 13 2021, Drew DeVault wrote:

> Can I get one of the maintainers to chime in on this thread and explain,
> in their opinion, what this patchset needs before it is acceptable? I'm
> not sure where I should go from this discussion.

Git just has the one maintainer, Junio C Hamano.

Ultimately getting your patch in is up to his whimsy.

Maybe he'll reply, but in an attempt to save him some time (which I
understand I'm taking too much of these days):

Getting your patch in is ultimately up to you.

So you submitted a patch, got some feedback/review.

Through some combination of this thread (mainly
<875z0sg8t9.fsf@evledraar.gmail.com> and
<87o8ejej8m.fsf@evledraar.gmail.com>) I suggested making some changes in a v3.

I.e. cleaning up some of the semantics (config docs/handling leaking
implementation details) and improving the commit message(s) to summarize
the trade-offs, why this approach is safe/isn't safe/why it's OK in the
cases it's not etc.

So, whatever Junio or anyone else thinks of my opinion I think it's fair
to say that at this point he's most likely to skim this thread and see
that there's some outstanding feedback that hasn't been addressed.

"Addressed" means either re-rolling into a v3, or deciding that nothing
(or only part of the feedback) needs to be changed and/or
addressed.

Both/some of those/that are perfectly acceptable approaches, but in
either case making things easily digestable to Junio will help your
series along.
Junio C Hamano April 13, 2021, 9:39 p.m. UTC | #11
"Drew DeVault" <sir@cmpwn.com> writes:

> Can I get one of the maintainers to chime in on this thread and explain,
> in their opinion, what this patchset needs before it is acceptable? I'm
> not sure where I should go from this discussion.

What you called "compromise" in the upthread didn't even smelled
like a compromise but the safest way forward.  My reasoning goes
(thinking aloud, so that you and Ævar can correct me if I am talking
nonsense and discount my input based on it):

 - If we were designing this from scratch, we would have called the
   smtps:// tunnelled transport SSL/TLS and in-place upgrade
   transport STARTTLS, like e-mail providers and client programs do,
   but unfortunately we didn't.  We ended up with 'ssl' vs 'tls'.

 - We could introduce and advertise STARTTLS as a synonym to 'tls',
   but then those whose send-email does not understand STARTTLS but
   read about the new way of spelling would end up having no
   encryption due to another earlier mistake we made, i.e. an
   unrecognised option value silently turns into no encryption.

 - To avoid the above problem, the first phase is not to change the
   status quo that 'ssl' vs 'tls' are the only two choices.  What we
   do is to make the program error out if we see an unrecognised
   value given to the option.  We release this to the wild, and wait
   for the current versions of send-email that turns unrecognised
   words into no-encryption die out.  It may take several years,
   though.

 - After waiting, we add 'starttls' as a synonym to 'tls'.  We may
   also add 'ssl/tls' as a synonym to 'ssl'.  Unfortunately 'tls'
   alone cannot be repurposed as a synonym for smtps:// without
   another deprecation dance, and it is not in scope of the
   transtion.

Am I on the same page as you two?
diff mbox series

Patch

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index c17c3b400a..520b355e50 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -171,8 +171,8 @@  Sending
 	Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables
 	generic SSL/TLS support and is typically used on port 465.  'tls'
 	enables in-band STARTTLS support and is typically used on port 25 or
-	587.  Use whichever option is recommended by your mail provider.  Any
-	other value reverts to plain SMTP.  Default is the value of
+	587.  Use whichever option is recommended by your mail provider.  Leave
+	empty to disable encryption and use plain SMTP.  Default is the value of
 	`sendemail.smtpEncryption`.
 
 --smtp-domain=<FQDN>::
diff --git a/git-send-email.perl b/git-send-email.perl
index f5bbf1647e..bda5211f0d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -495,6 +495,9 @@  sub read_config {
 
 # 'default' encryption is none -- this only prevents a warning
 $smtp_encryption = '' unless (defined $smtp_encryption);
+if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
+	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
+}
 
 # Set CC suppressions
 my(%suppress_cc);