Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Aug 1, 2015, 09:33 UTC
- Message-ID
- <CAPig+cT842GAFFM-wfjSU1ZiOevDCOPNDWxux6-vqtdr=3F4qw@mail.gmail.com>
- In-Reply-To
- <1438385617-29159-1-git-send-email-viktorin@rehivetech.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/
Show 14 quoted lines
> 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.
Show 9 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?
> + 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?
Show 9 quoted lines
> # 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'
Show 17 quoted lines
> + 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