threads / patch / 39989

patchsend-email: provide whitelist of SMTP AUTH mechanisms

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

## tl;dr

11 messages between Jul 31, 2015 and Aug 3, 2015. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Jan Viktorin· Jul 31, 2015, 23:33 UTC · lore

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 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(-)
Show changes to git-send-email.perl +30 −1
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
@@ -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.
+                                     This setting forces to use one of the listed methods.
+                                     Supported: PLAIN LOGIN CRAM-MD5 DIGEST-MD5.
     --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
 
   Automating:
@@ -208,7 +211,7 @@ my ($cover_cc, $cover_to);
 my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, @smtp_server_options);
 my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
-my ($identity, $aliasfiletype, @alias_files, $smtp_domain);
+my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);
 my ($validate, $confirm);
 my (@suppress_cc);
 my ($auto_8bit_encoding);
@@ -239,6 +242,7 @@ my %config_settings = (
     "smtppass" => \$smtp_authpass,
     "smtpsslcertpath" => \$smtp_ssl_cert_path,
     "smtpdomain" => \$smtp_domain,
+    "smtpauth" => \$smtp_auth,
     "to" => \@initial_to,
     "tocmd" => \$to_cmd,
     "cc" => \@initial_cc,
@@ -310,6 +314,7 @@ my $rc = GetOptions("h" => \$help,
 		    "smtp-ssl-cert-path=s" => \$smtp_ssl_cert_path,
 		    "smtp-debug:i" => \$debug_net_smtp,
 		    "smtp-domain:s" => \$smtp_domain,
+		    "smtp-auth=s" => \$smtp_auth,
 		    "identity=s" => \$identity,
 		    "annotate!" => \$annotate,
 		    "no-annotate" => sub {$annotate = 0},
@@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {
 		return 1;
 	}
 
+	# Do not allow arbitrary strings.
+	my ($filtered_auth) = "";
+	foreach ("PLAIN", "LOGIN", "CRAM-MD5", "DIGEST-MD5") {
+		if($smtp_auth && $smtp_auth =~ /\b\Q$_\E\b/i) {
+			$filtered_auth .= $_ . " ";
+		}
+	}
+
+	die "Invalid SMTP AUTH." if length $smtp_auth && !length $filtered_auth;
+
 	# 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) {
+			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
Eric Sunshine· Aug 1, 2015, 09:33 UTC · re: Jan Viktorin · lore

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

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
Jan Viktorin· Aug 1, 2015, 18:19 UTC · re: Eric Sunshine · lore

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

Hello Eric,

thanks for comments. I've described the orignal problem before I tried to fix it:

 https://groups.google.com/forum/#!topic/git-users/PxtiVxAapUU

So, *this patch* was necessary to apply for me to send *this patch* to the mailing list.

Later, I've tried git-send-email (without this patch) on two different PCs with the same distro, same architecture, same git, same perl, same perl libraries. The result was that on the first, it auto-selected DIGEST-MD5 (didn't work) and on the second one, it selected PLAIN (worked). I don't understand it.

More below...

On Sat, 1 Aug 2015 05:33:28 -0400 Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 26 quoted lines
> 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).

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?

Show 15 quoted lines
> 
> 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?

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

Show 18 quoted lines
> 
> > +       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.

Sure. I just wanted to avoid another indentation level. I think, there is no need for optimization at this place. I can rework it, no problem...

Show 9 quoted lines
> 
> > +                       $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 ;).

Show 9 quoted lines
> 
> > +               }
> > +       }
> > +
> > +       die "Invalid SMTP AUTH." if length $smtp_auth && !length
> > $filtered_auth;
> 
> Style: drop capitalization: "invalid..."
> Style: drop period at end
Agree.
> 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...
> 
> (Existing style in the script is not very consistent, but new code
> probably should adhere the above suggestions.)
(Agree.)
> 
> 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.

Note that, the current implementation does not force the user to use only the listed mechanisms. If the $smtp_auth is empty, the original behaviour is preserved...

Show 31 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'
> 
> > +                       my $sasl = 
> > +                               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
-- 
  Jan Viktorin                E-mail: Viktorin@RehiveTech.com
  System Architect            Web:    www.RehiveTech.com
  RehiveTech                  Phone: +420 606 201 868
  Brno, Czech Republic
Eric Sunshine· Aug 2, 2015, 09:41 UTC · re: Jan Viktorin · lore

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:
Show 13 quoted lines
> 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).

Show 16 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?
>
> 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).

Show 13 quoted lines
>> > +                       $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),
            ...
Show 5 quoted lines
>> 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.
Show 11 quoted lines
>> 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.

Jan Viktorin· Aug 2, 2015, 16:43 UTC · re: Eric Sunshine · lore

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

Authen::SASL gives:
No SASL mechanism found
 at /usr/share/perl5/vendor_perl/Authen/SASL.pm line 77.
 at /usr/share/perl5/core_perl/Net/SMTP.pm line 207.

The SASL library does not check validity of mechanisms' names (or I did not find it). It just tries to load one that matches both the ours and the server side ones.

I can see one possible weakness of this, however I doubt whether there exists a successful attack vector. Imagine that somebody gives me a malicious .gitconfig with smtpauth = ~/ATTACK and redirects me to a fake mail server that advertises ~/ATTACK as a working mechanism. This might lead to an unwanted execution of ~/ATTACK.pm. Should we consider this to be a threat?

Another thing that confuses me (I mentioned it in the previous e-mail). I forced to use CRAM-MD5, however, it dies with the above errors. The CRAM-MD5 is installed:

/usr/share/perl5/vendor_perl/Authen/SASL/CRAM_MD5.pm /usr/share/perl5/vendor_perl/Authen/SASL/Perl/CRAM_MD5.pm

The same for DIGEST-MD5. On different PC with the same set of libraries, OS, the CRAM-MD5 just works. Why? LOGIN and PLAIN are OK. Environment? (I doubt.)

I would like to include the regex check based on RFC 4422 as I've already mentioned. at least, it filters out the unwanted characters like '/', '.', etc.

Regards Jan

On Sun, 2 Aug 2015 05:41:29 -0400 Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 76 quoted lines
> 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.
-- 
  Jan Viktorin                E-mail: Viktorin@RehiveTech.com
  System Architect            Web:    www.RehiveTech.com
  RehiveTech                  Phone: +420 606 201 868
  Brno, Czech Republic
Junio C Hamano· Aug 2, 2015, 18:28 UTC · re: Jan Viktorin · lore

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

Jan Viktorin <viktorin@rehivetech.com> writes:
Show 13 quoted lines
> Authen::SASL gives:
>
> No SASL mechanism found
>  at /usr/share/perl5/vendor_perl/Authen/SASL.pm line 77.
>  at /usr/share/perl5/core_perl/Net/SMTP.pm line 207.
>
> The SASL library does not check validity of mechanisms'
> names (or I did not find it). It just tries to load one
> that matches both the ours and the server side ones.
> ...
> I would like to include the regex check based on RFC 4422
> as I've already mentioned. at least, it filters out the
> unwanted characters like '/', '.', etc.

Hmm, is there a way to ask Authen::SASL what SASL mechanism the installed system supports? If so, the enhancement you are adding could be

	my @to_use;
	if ($smtp_auth_whitelist is supplied) {
		my @installed = Authen::SASL::list_mechanisms();
                for (@installed) {
                	if ($_ is whitelisted) {
				push @to_use, $_;
			}
		}
	}

and @to_use can later be supplied when we open the connection as the list of mechanisms we allow the library to pick.

Just my $.02
Jan Viktorin· Aug 3, 2015, 10:24 UTC · re: Junio C Hamano · lore

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

On Sun, 02 Aug 2015 11:28:49 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Show 34 quoted lines
> Jan Viktorin <viktorin@rehivetech.com> writes:
> 
> > Authen::SASL gives:
> >
> > No SASL mechanism found
> >  at /usr/share/perl5/vendor_perl/Authen/SASL.pm line 77.
> >  at /usr/share/perl5/core_perl/Net/SMTP.pm line 207.
> >
> > The SASL library does not check validity of mechanisms'
> > names (or I did not find it). It just tries to load one
> > that matches both the ours and the server side ones.
> > ...
> > I would like to include the regex check based on RFC 4422
> > as I've already mentioned. at least, it filters out the
> > unwanted characters like '/', '.', etc.
> 
> Hmm, is there a way to ask Authen::SASL what SASL mechanism the
> installed system supports?  If so, the enhancement you are adding
> could be
> 
> 	my @to_use;
> 	if ($smtp_auth_whitelist is supplied) {
> 		my @installed = Authen::SASL::list_mechanisms();
>                 for (@installed) {
>                 	if ($_ is whitelisted) {
> 				push @to_use, $_;
> 			}
> 		}
> 	}
> 
> and @to_use can later be supplied when we open the connection as the
> list of mechanisms we allow the library to pick.
> 
> Just my $.02

I didn't find a way how to determine what mechanisms are supported by SASL. This is a way how it looks for a mechanism (I think) on new():

Authen/SASL/Perl.pm
 57   my @mpkg = sort {
 58     $b->_order <=> $a->_order
 59   } grep {
 60     my $have = $have{$_} ||= (eval "require $_;" and $_->can('_secflags')) ? 1 : -1;
 61     $have > 0 and $_->_secflags(@sec) == @sec
 62   } map {
 63     (my $mpkg = __PACKAGE__ . "::$_") =~ s/-/_/g;
 64     $mpkg;
 65   } split /[^-\w]+/, $parent->mechanism
 66     or croak "No SASL mechanism found\n";

It just loads a package based on the names we provide. So it seems, the library has no clue about the existing mechanisms. This would be possible by reading the proper directory with packages which seems to be quite wierd anyway.

-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic
Junio C Hamano· Aug 3, 2015, 19:53 UTC · re: Jan Viktorin · lore

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

Jan Viktorin <viktorin@rehivetech.com> writes:
> I didn't find a way how to determine what mechanisms are supported by SASL.
Ok, forget the suggested approach, then X-<.
Junio C Hamano· Aug 2, 2015, 18:10 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 16 quoted lines
> 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).

Both are very good points. I do not think we should be limiting the user input; instead, we (1) either let the Perl module emit proper diagnosis (e.g. it may say "There is no valid auth method in the list you gave me, which was 'PLIAN LOGIN CROM-MD5'") and do nothing, or (2) catch the error from the Perl module and then guess what happened after the fact (e.g. the module may say in its die() message something that is understandable by the program but not by the user, and "eval { ... module call ... }; if ($@) { ... HERE ... }" would massage what it can learn in HERE from $@ into what the end user would understand. It may be that it is sufficient to have something as simple as this:

	my $msg = "$@"
	if ($smtp_auth is used to customize) {
		$msg .= "\nYour customized <$smtp_auth> might be misspelt?"
	}
	die $msg;
in "HERE" above.
Show 6 quoted lines
> ...
> 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.

Yes, exactly. What happens if we go the route of doing nothing here? What are the horrible diagnostic message the end user would not understand?

brian m. carlson· Aug 1, 2015, 16:49 UTC · re: Jan Viktorin · lore

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

On Sat, Aug 01, 2015 at 01:33:37AM +0200, Jan Viktorin wrote:
> +	# Do not allow arbitrary strings.
> +	my ($filtered_auth) = "";
> +	foreach ("PLAIN", "LOGIN", "CRAM-MD5", "DIGEST-MD5") {

On my system, GSSAPI is also available, and it does indeed work, as I'm not prompted for a password. (I have only PLAIN and GSSAPI available server-side, and AUTH is required.)

It may be better to simply force the text to upper case, as that would allow us not to have to change Git if Authen::SASL::Perl implements new mechanisms.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
Jan Viktorin· Aug 1, 2015, 18:21 UTC · re: brian m. carlson · lore

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

Hello Brian,

thanks for your note. I think, I will remove the check of list of mechanisms and put there a regex check.

On Sat, 1 Aug 2015 16:49:59 +0000 "brian m. carlson" <sandals@crustytoothpaste.net> wrote:

Show 12 quoted lines
> On Sat, Aug 01, 2015 at 01:33:37AM +0200, Jan Viktorin wrote:
> > +	# Do not allow arbitrary strings.
> > +	my ($filtered_auth) = "";
> > +	foreach ("PLAIN", "LOGIN", "CRAM-MD5", "DIGEST-MD5") {
> 
> On my system, GSSAPI is also available, and it does indeed work, as
> I'm not prompted for a password.  (I have only PLAIN and GSSAPI
> available server-side, and AUTH is required.)
> 
> It may be better to simply force the text to upper case, as that would
> allow us not to have to change Git if Authen::SASL::Perl implements
> new mechanisms.
-- 
  Jan Viktorin                E-mail: Viktorin@RehiveTech.com
  System Architect            Web:    www.RehiveTech.com
  RehiveTech                  Phone: +420 606 201 868
  Brno, Czech Republic

← back to recent threads