Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 2, 2015, 09:41 UTC
Message-ID: <CAPig+cQwgYYYYsszaRdJDwFLLB0PmiDQ_WTa+Nzzoq0U1zuMiA@mail.gmail.com>
In-Reply-To: <20150801201950.5d8c1951@jvn>

On Sat, Aug 1, 2015 at 2:19 PM, Jan Viktorin <viktorin@rehivetech.com> wrote:

Show 13 quoted lines

> On Sat, 1 Aug 2015 05:33:28 -0400 Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin
>> <viktorin@rehivetech.com> wrote:
>> At the very least, you will also want to update the documentation
>> (Documentation/git-send-email.txt) and, if possible, add new tests
>> (t/t9001-send-email.sh).
>
> I will update the documentation when it is clear, how the smtp-auth
> works.
>
> I have no idea, how to test the feature. I can see something like
> fake.sendmail in the file. How does it work? I can image a test whether
> user inserts valid values. What more?

That's what I was thinking. You could test if the die() is triggered or if it emits warnings for bad values (assuming you implement that feature). As for testing the actual authentication, I'm not sure you can (and don't see any such testing in the script).

Show 16 quoted lines

>> > diff --git a/git-send-email.perl b/git-send-email.perl
>> > index ae9f869..b00ed9d 100755
>> > --- a/git-send-email.perl
>> > +++ b/git-send-email.perl
>> > @@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {
>> >                 return 1;
>> >         }
>> >
>> > +       # Do not allow arbitrary strings.
>>
>> Can you explain why this restriction is needed. What are the
>> consequences of not limiting the input to this "approved" list?
>
> This is more a check of an arbitrary user input then a check
> of an "approved list". It should be also used to inform user
> about invalid methods (however, I didn't implemented it yet).

What I was really asking was whether this sort of checking really belongs in git-send-email or if it is better left to Net::SMTP (and Authen::SASL) to do so since they are in better positions to know what is valid and what is not. If the Perl module(s) generate suitable diagnostics for bad input, then it makes sense to leave the checking to them. If not, then I can understand your motivation for git-send-email doing the checking instead in order to emit user-friendly diagnostics.

So, that's what I meant when I asked 'What are the consequences of not limiting the input to this "approved" list?'.

The other reason I asked was that it increases maintenance costs for us to maintain a list of "approved" mechanisms, since the list needs to be updated when new ones are implemented (and, as brian pointed out, some may already exist which are not in your list).

Show 13 quoted lines

>> > +                       $filtered_auth .= $_ . " ";
>>
>> Style question: Would this be more naturally expressed with
>> 'filtered_auth' as an array onto which items are pushed, rather than
>> as a string? At the point of use, the string can be recreated via
>> join().
>>
>> Not a big deal; just wondering.
>
> I am not a Perl programmer. Yesterday, I've discovered for the first
> time that Perl uses a dot for concatenation... I have no idea what
> happens when passing an array to Authen::SASL->new(). Moreover, the
> Perl arrays syntax rules scare me a bit ;).

You wouldn't pass the array to Authen::SASL, instead you would use join() to transform the array back into a space-separated string. It's probably moot (since you probably shouldn't be doing the filtering manually), but the code would look something like this:

    my @filtered_auth;
    ...
    foreach (...) {
        if (...) {
            push @filtered_auth, $_;
        }
    }
    ...
    if (@filtered_auth) {
        my $sasl = Authen::SASL->new(
            mechanism => join(' ', @filtered_auth),
            ...

Show 5 quoted lines

>> Style: add "\n" at end in order to suppress printing of the
>>     perl line number and input line number which aren't
>>     very meaningful for a user error
>
> Another hidden Perl suprise, I guess...

Yes.

Show 11 quoted lines

>> Also, don't you want to warn the user about tokens that don't match
>> one of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather than
>> dropping them silently?
>
> Yes, this would be great (as I've already mentioned). It's a question
> whether to include the check for the mechanisms or whether to leave
> the $smtp_auth variable as it is... Maybe just validate by a regex?
>
> The naming rules are defiend here:
>  https://tools.ietf.org/html/rfc4422#page-8
> So, this looks to me as a better way.

Maybe. This leads back to my original question of whether it's really git-send-email's responsibility to do validation or if that can be left to Net::SMTP/Authen::SASL. If the Perl module(s) emit suitable diagnostics for bad input, then validation can be omitted from git-send-email.