From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Drew DeVault <sir@cmpwn.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption
Date: Sun, 11 Apr 2021 17:06:10 +0200 [thread overview]
Message-ID: <875z0sg8t9.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <878s5ogagz.fsf@evledraar.gmail.com>
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?
next prev parent reply other threads:[~2021-04-11 15:06 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-11 12:54 [PATCH v2 0/3] git-send-email: improve SSL configuration Drew DeVault
2021-04-11 12:54 ` [PATCH v2 1/3] git-send-email(1): improve smtp-encryption docs Drew DeVault
2021-04-11 14:11 ` Ævar Arnfjörð Bjarmason
2021-04-11 12:54 ` [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption Drew DeVault
2021-04-11 14:20 ` Ævar Arnfjörð Bjarmason
2021-04-11 14:21 ` Drew DeVault
2021-04-11 14:30 ` Ævar Arnfjörð Bjarmason
2021-04-11 15:06 ` Ævar Arnfjörð Bjarmason [this message]
2021-04-11 15:18 ` Drew DeVault
2021-04-11 19:56 ` Ævar Arnfjörð Bjarmason
2021-04-12 12:33 ` Drew DeVault
2021-04-12 13:16 ` Ævar Arnfjörð Bjarmason
2021-04-13 12:12 ` Drew DeVault
2021-04-13 14:22 ` Ævar Arnfjörð Bjarmason
2021-04-13 21:39 ` Junio C Hamano
2021-04-11 12:54 ` [PATCH v2 3/3] git-send-email: rename 'tls' to 'starttls' Drew DeVault
2021-04-11 14:17 ` Ævar Arnfjörð Bjarmason
2021-04-11 14:22 ` Drew DeVault
2021-04-11 14:43 ` [PATCH 0/2] send-email: simplify smtp.{smtpssl,smtpencryption} parsing Ævar Arnfjörð Bjarmason
2021-04-11 14:43 ` [PATCH 1/2] send-email: remove non-working support for "sendemail.smtpssl" Ævar Arnfjörð Bjarmason
2021-04-11 19:08 ` Junio C Hamano
2021-04-11 19:51 ` Ævar Arnfjörð Bjarmason
2021-05-01 9:15 ` Ævar Arnfjörð Bjarmason
2021-04-11 14:43 ` [PATCH 2/2] send-email: refactor sendemail.smtpencryption config parsing Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875z0sg8t9.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=sir@cmpwn.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).