From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 02 Aug 2015 18:57:19 GMT
Subject: Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms
Message-ID: <CAPig+cQwFxVtO1C_RAumGP6_et21ggORB4jhpcUtBYNznNH1qA@mail.gmail.com>
In-Reply-To: <1438533769-17460-1-git-send-email-viktorin@rehivetech.com>

On Sun, Aug 2, 2015 at 12:42 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) deny valid
> 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 selected from the ones supported by
> the installed SASL perl library.

Nit: This would read a bit more nicely if wrapped to 70-72 columns.

> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> ---
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 7ae467b..c237c80 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -171,6 +171,14 @@ Sending
> +--smtp-auth=<mechs>::
> +       Specify allowed SMTP-AUTH mechanisms. This setting forces using only
> +       the listed mechanisms. Separate allowed mechanisms by a whitespace.

Perhaps:

    Whitespace-separated list of allowed SMTP-AUTH mechanisms.

> +       Example: PLAIN LOGIN GSSAPI. If at least one of the specified mechanisms
> +       matchs those advertised by the SMTP server and it is supported by the SASL

s/matchs/matches/

> +       library we use, it is used for authentication. If neither of 'sendemail.smtpAuth'
> +       or '--smtp-auth' is specified, all mechanisms supported on client can be used.

s/neither of/neither/
s/or/nor/

> diff --git a/git-send-email.perl b/git-send-email.perl
> index ae9f869..ebc1e90 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -75,6 +75,9 @@ git send-email [options] <file | directory | rev-list options >
>                                       Pass an empty string to disable certificate
>                                       verification.
>      --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake
> +    --smtp-auth             <str>  * Space separated list of allowed AUTH methods.

s/Space separated/Space-separated/

> +                                     This setting forces to use one of the listed methods.
> +                                     Supported: PLAIN LOGIN CRAM-MD5 DIGEST-MD5.

Since you're no longer checking explicitly for these mechanisms, you
probably want to drop the "Supported:" line.

>      --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
>
>    Automating:
> @@ -1136,6 +1141,10 @@ sub smtp_auth_maybe {
>                 Authen::SASL->import(qw(Perl));
>         };
>
> +       if($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) {
> +               die "invalid smtp auth: '${smtp_auth}'";
> +       }

Style: space after 'if'

>         # TODO: Authentication may fail not because credentials were
>         # invalid but due to other reasons, in which we should not
>         # reject credentials.
> @@ -1148,6 +1157,20 @@ sub smtp_auth_maybe {
>                 'password' => $smtp_authpass
>         }, sub {
>                 my $cred = shift;
> +
> +               if($smtp_auth) {

Style: space after 'if'

> +                       my $sasl = Authen::SASL->new(
> +                               mechanism => $smtp_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