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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin <viktorin@rehivetech.com> wrote:
> When sending an e-mail, the client and server must
> agree on an authentication mechanism. Some servers
> (due to misconfiguration or a bug) denies valid

s/denies/deny/

> credentials for certain mechanisms. In this patch,
> a new option --smtp-auth and configuration entry
> smtpauth are introduced.
>
> If smtp_auth is defined, it works as a whitelist
> of allowed mechanisms for authentication. There
> are four mechanisms supported: PLAIN, LOGIN,
> CRAM-MD5, DIGEST-MD5. However, their availability
> depends on the installed SASL library.
>
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> ---
>  git-send-email.perl | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)

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

More below.

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

> +       my ($filtered_auth) = "";

Style: unnecessary parentheses

> +       foreach ("PLAIN", "LOGIN", "CRAM-MD5", "DIGEST-MD5") {

This might read more nicely and be easier to maintain if written as:

    foreach (qw/PLAIN LOGIN CRAM-MD5 DIGEST-MD5/) {

> +               if($smtp_auth && $smtp_auth =~ /\b\Q$_\E\b/i) {

Style: space after 'if'

Also, why not lift the 'if ($smtp_auth)' check outside the loop since
its value never changes and there's no need to iterate over the list
if $smtp_auth is empty.

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

> +               }
> +       }
> +
> +       die "Invalid SMTP AUTH." if length $smtp_auth && !length $filtered_auth;

Style: drop capitalization: "invalid..."
Style: drop period at end
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

(Existing style in the script is not very consistent, but new code
probably should adhere the above suggestions.)

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?

>         # Workaround AUTH PLAIN/LOGIN interaction defect
>         # with Authen::SASL::Cyrus
>         eval {
> @@ -1148,6 +1163,20 @@ sub smtp_auth_maybe {
>                 'password' => $smtp_authpass
>         }, sub {
>                 my $cred = shift;
> +
> +               if($filtered_auth) {

Style: space after 'if'

> +                       my $sasl = Authen::SASL->new(
> +                               mechanism => $filtered_auth,
> +                               callback => {
> +                                       user => $cred->{'username'},
> +                                       pass => $cred->{'password'},
> +                                       authname => $cred->{'username'},
> +                               }
> +                       );
> +
> +                       return !!$smtp->auth($sasl);
> +               }
> +
>                 return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
>         });
>
> --
> 2.5.0