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

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

[Jan Viktorin]

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

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

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

[Eric Sunshine]

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

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

[Jan Viktorin]

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:

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

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

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

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

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

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

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

[Jan Viktorin]

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:

> 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

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

[Eric Sunshine]

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

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

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

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

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

[Jan Viktorin]

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:

> 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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

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

[Junio C Hamano]

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

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

[Jan Viktorin]

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

> 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

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

[Junio C Hamano]

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