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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Aug 1, 2015 at 2:19 PM, Jan Viktorin <viktorin@rehivetech.com> wrote:
> 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).

>> > 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).

>> > +                       $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),
            ...

>> 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.

>> 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.