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@xxxxxxxxx/
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?
