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

["Ævar Arnfjörð Bjarmason" <avarab@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?