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

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