bad behaviour while using git rebase -i -p

[Jan Viktorin]

Hello,

since Git 2.16.1, I've noticed a bad behaviour of git rebase -i -p. It
screws up merge commits created with --log (or config merge.log = true)
in my history. A good merge commit with message like:

Merge branch 'test'

* test:
  c
  b

is changed after rebase (without touching that commit in any way) into:

Merge branch 'test' a git-rebase-p-test.sh test: c b

It seems, like the commit message is interpreted somehow - the '*'
character is expanded to the list of files in the current directory and
the original spacing is removed. This happens during my regular work.

Here is a code that seems to be reproducing this behaviour well:

git init

touch a
git add a
git commit -m a

git checkout -b test
# a new branch made to merge back to master later

touch b
git add b
git commit -m b

touch c
git add c
git commit -m c

git checkout master
git merge --no-edit --log test
git log -1
# everything looks good at this point

export GIT_SEQUENCE_EDITOR='sed "1s/pick/reword/" -i'
# we are rewording only the first commit...
export EDITOR='sed "s/b/x/" -i'
# ...and changing its message from "b" to "x"

git rebase -i HEAD^1 -p
git log -1
# here, you can see the bad merge commit message

Regards
Jan

Re: [PATCH v5] send-email: --batch-size to work around some SMTP server limit

[Jan Viktorin]

On Tue, 23 May 2017 16:46:27 +0900
Junio C Hamano  wrote:

> Ævar Arnfjörð Bjarmason  writes:
> 
> > Looking at this the Nth time now though I wonder about this approach
> > in general. In all your E-Mails I don't think you ever said /what/
> > sort of error you had from the SMTP server, you just said you had a
> > failure or an error, I assume you hit one of the die's in the
> > send_message() function. Can you paste the actual error you get
> > without this patch?

Hello,

I have issues with a company SMTP server that returns:

Net::SMTP::SSL=GLOB(0x20d6510)<<< 451 4.3.0 Please try again later,
rate limited.
4.3.0 Please try again later, rate limited.

Unfortunately, I didn't find out the exact properties of the limit yet.
It seems that sending more then 10 patches at once fails. Thus, I have
to send longer patch sets in 2 rounds:

1. normal git send-email
2. git send-email --no-thread --in-reply-to="" \
 ...

It is not exactly the same as sending all the patches at once.

The xiaoqiang's solution sounds promising to me. However, probably a
more general solution would be to "just" enable sending a whole patch
set in 2 rounds manually. But I didn't find any way how to do it right.

Regards
Jan

> >
> > I wonder if something like this would Just Work for this case without
> > any configuration or command-line options, with the added benefit of
> > just working for anyone with transitory SMTP issues as well (patch
> > posted with -w, full version at
> > https://github.com/avar/git/commit/acb60c4bde50bdcb62b71ed46f49617e2caef84e.patch):
> >   
> 
> Yeah, if the issues users of 163.com are having can be resolved with
> a more general approach like this, that would be very much preferred.
> 
> > Now that's very much a WIP and I don't have a server like that to test 
> > against.
> >
> > Having worked with SMTP a lot in a past life/job, I'd say it's *very*
> > likely that you're just getting a /^4/ error code from 163.com,
> > probably 421, which would make this logic even simpler. I.e. we could
> > just adjust this to back-off for /^4/ instead of trying to handle
> > arbitrary errors.
> >
> > Anyway, I'm not interested in pursuing that WIP patch, and I don't
> > think perfect should be the enemy of the good here. Your patch works
> > for you, doesn't really damage anything else, so if you're not
> > interested in hacking up something like the above I think we should
> > just take it.
> >
> >
> > But I do think it would be very good to get a reply to you / details
> > in the commit message about what error you get exactly in this
> > scenario, see if you get better details with --smtp-debug, and if so
> > paste that (sans any secret info like user/password you don't want to
> > share).  
> 
> Let's wait for a few days to see if xiaoqiang wants to take your
> outline of more general approach and polish it.  I do prefer the "no
> config" solution as xiaoqiang won't be the only 163.com user, but
> Individual Contributors cannot be forced, so ...
> 
> Thanks.

Re: [PATCH v4] send-email: --batch-size to work around some SMTP server limit

[Jan Viktorin]

Hello,

with this patch applied to git 2.12, I could see:

Use of uninitialized value $batch_size in numeric eq (==) at 
/usr/lib/git-core/git-send-email line 1679

when --batch-size is NOT used. See below...

On Sat, 13 May 2017 09:57:26 +0800
xiaoqiang zhao  wrote:

> Some email servers (e.g. smtp.163.com) limit the number emails to be
> sent per session(connection) and this will lead to a faliure when
> sending many messages.
> 
> Teach send-email to disconnect after sending a number of messages
> (configurable via the --batch-size= option), wait for a few
> seconds (configurable via the --relogin-delay= option) and
> reconnect, to work around such a limit.
> 
> Also add this two configure option for git config command.
> 
> Note:
>Re-authentication will happen every $ messages, so it
> will be much more acceptable if you use some form of credential helper
> (e.g. the 'sendemail.smtppass' config option), otherwise you will have
> to retype password every time when asked.
> 
> Signed-off-by: xiaoqiang zhao 
> ---
>  contrib/completion/git-completion.bash |  2 ++
>  git-send-email.perl| 18 ++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index af658995d..29496353a 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2608,6 +2608,8 @@ _git_config ()
>   sendemail.thread
>   sendemail.to
>   sendemail.validate
> + sendemail.smtpbatchsize
> + sendemail.smtprelogindelay
>   showbranch.default
>   status.relativePaths
>   status.showUntrackedFiles
> diff --git a/git-send-email.perl b/git-send-email.perl
> index eea0a517f..071d1ab9d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -81,6 +81,10 @@ git send-email --dump-aliases
>   This setting forces to use one of the 
> listed mechanisms.
>  --smtp-debug<0|1>  * Disable, enable Net::SMTP debug.
>  
> +--batch-size  * send max  message per connection.
> +--relogin-delay   * delay  seconds between two 
> successive login, default to 1,
> + This option can only be used with 
> --batch-size
> +
>Automating:
>  --identity* Use the sendemail. options.
>  --to-cmd  * Email To: via ` \$patch_path`
> @@ -153,6 +157,7 @@ my $have_email_valid = eval { require Email::Valid; 1 };
>  my $have_mail_address = eval { require Mail::Address; 1 };
>  my $smtp;
>  my $auth;
> +my $num_sent = 0;
>  
>  # Regexes for RFC 2047 productions.
>  my $re_token = qr/[^][()<>@,;:\\"\/?.= \000-\037\177-\377]+/;
> @@ -216,6 +221,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 ($batch_size, $relogin_delay);
>  my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);
>  my ($validate, $confirm);
>  my (@suppress_cc);
> @@ -247,6 +253,8 @@ my %config_settings = (
>  "smtppass" => \$smtp_authpass,
>  "smtpdomain" => \$smtp_domain,
>  "smtpauth" => \$smtp_auth,
> +"smtpbatchsize" => \$batch_size,
> +"smtprelogindelay" => \$relogin_delay,
>  "to" => \@initial_to,
>  "tocmd" => \$to_cmd,
>  "cc" => \@initial_cc,
> @@ -358,6 +366,8 @@ $rc = GetOptions(
>   "force" => \$force,
>   "xmailer!" => \$use_xmailer,
>   "no-xmailer" => sub {$use_xmailer = 0},
> + "batch-size=i" => \$batch_size,
> + "relogin-delay=i" => \$relogin_delay,
>);
>  
>  usage() if $help;
> @@ -1664,6 +1674,14 @@ foreach my $t (@files) {
>   }
>   }
>   $message_id = undef;
> + $num_sent++;
> + if ($num_sent == $batch_size) {

This is the line. I think, batch_size can be sometimes uninitialized
while this statement is executed.

Regards
Jan

> + $num_sent = 0;
> + $smtp->quit if defined $smtp;
> + undef $smtp;
> + undef $auth;
> + sleep($relogin_delay);
> + }
>  }
>  
>  # Execute a command (e.g. $to_cmd) to get a list of email addresses

Re: [PATCH v2] send-email: new options to walkaround email server limits

[Jan Viktorin]

Hello, thank you for posting this improvement. I've been missing such feature 
in git. I hope to test it soon.

Jan Viktorin
RehiveTech
Sent from a mobile device
  Původní zpráva  
Od: xiaoqiang zhao
Odesláno: pondělí, 1. května 2017 15:00
Komu: git@vger.kernel.org
Kopie: gits...@pobox.com; vikto...@rehivetech.com; m...@kernel.org; 
pbonz...@redhat.com; min...@mina86.com; artag...@gmail.com
Předmět: [PATCH v2] send-email: new options to walkaround email server limits

Some email server(e.g. smtp.163.com) limits a fixed number emails to
be send per session(connection) and this will lead to a send faliure.

With --batch-size= option, an auto reconnection will occur when
number of sent email reaches  and the problem is solved.

--relogin-delay option will make some delay between two successive
email server login.

Signed-off-by: xiaoqiang zhao <zxq_yx_...@163.com>
---
git-send-email.perl | 26 +-
1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index eea0a517f..cd9981cc6 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -81,6 +81,10 @@ git send-email --dump-aliases
This setting forces to use one of the listed mechanisms.
--smtp-debug <0|1> * Disable, enable Net::SMTP debug.

+ --batch-size  * send max \$num message per connection.
+ --relogin-delay  * delay \$num seconds between two successive login, 
default to 1,
+ This option can only be used with --batch-size
+
Automating:
--identity  * Use the sendemail. options.
--to-cmd  * Email To: via ` \$patch_path`
@@ -153,6 +157,7 @@ my $have_email_valid = eval { require Email::Valid; 1 };
my $have_mail_address = eval { require Mail::Address; 1 };
my $smtp;
my $auth;
+my $num_sent = 0;

# Regexes for RFC 2047 productions.
my $re_token = qr/[^][()<>@,;:\\"\/?.= \000-\037\177-\377]+/;
@@ -186,6 +191,8 @@ my $format_patch;
my $compose_filename;
my $force = 0;
my $dump_aliases = 0;
+my $batch_size = 0;
+my $relogin_delay = 1;

# Handle interactive edition of files.
my $multiedit;
@@ -358,6 +365,8 @@ $rc = GetOptions(
"force" => \$force,
"xmailer!" => \$use_xmailer,
"no-xmailer" => sub {$use_xmailer = 0},
+"batch-size=i" => \$batch_size,
+"relogin-delay=i" => \$relogin_delay,
);

usage() if $help;
@@ -1158,10 +1167,15 @@ sub smtp_host_string {
# (smtp_user was not specified), and 0 otherwise.

sub smtp_auth_maybe {
-   if (!defined $smtp_authuser || $auth) {
+   if (!defined $smtp_authuser || $num_sent != 0) {
return 1;
}

+   if ($auth && $num_sent == 0) {
+print "Auth use saved password. \n";
+return !!$smtp->auth($smtp_authuser, $smtp_authpass);
+   }
+
# Workaround AUTH PLAIN/LOGIN interaction defect
# with Authen::SASL::Cyrus
eval {
@@ -1187,6 +1201,7 @@ sub smtp_auth_maybe {
'password' => $smtp_authpass
}, sub {
my $cred = shift;
+$smtp_authpass = $cred->{'password'};

if ($smtp_auth) {
my $sasl = Authen::SASL->new(
@@ -1442,6 +1457,15 @@ EOF
}
}

+   $num_sent++;
+   if ($num_sent == $batch_size) {
+$smtp->quit;
+$smtp = undef;
+$num_sent = 0;
+print "Reconnect SMTP server required. \n";
+sleep($relogin_delay);
+   }
+
return 1;
}

-- 
2.13.0.rc1.16.g49e904895

[PATCH v3] 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) 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.

Signed-off-by: Jan Viktorin vikto...@rehivetech.com
---
 Documentation/git-send-email.txt | 11 +++
 git-send-email.perl  | 26 +-
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index f14705e..82c6ae8 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -171,6 +171,17 @@ Sending
to determine your FQDN automatically.  Default is the value of
'sendemail.smtpDomain'.
 
+--smtp-auth=mechs::
+   Whitespace-separated list of allowed SMTP-AUTH mechanisms. This setting
+   forces using only the listed mechanisms. Example:
+
+   $ git send-email --smtp-auth=PLAIN LOGIN GSSAPI ...
+
+   If at least one of the specified mechanisms matches the ones advertised 
by the
+   SMTP server and if it is supported by the utilized SASL library, the 
mechanism
+   is used for authentication. If neither 'sendemail.smtpAuth' nor 
'--smtp-auth'
+   is specified, all mechanisms supported by the SASL library can be used.
+
 --smtp-pass[=password]::
Password for SMTP-AUTH. The argument is optional: If no
argument is specified, then the empty string is used as
diff --git a/git-send-email.perl b/git-send-email.perl
index b660cc2..a7192c4 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -75,6 +75,8 @@ 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.
 --smtp-debug0|1  * Disable, enable Net::SMTP debug.
 
   Automating:
@@ -208,7 +210,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 +241,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 +313,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},
@@ -1130,6 +1134,12 @@ sub smtp_auth_maybe {
Authen::SASL-import(qw(Perl));
};
 
+   # Check mechanism naming as defined in:
+   # https://tools.ietf.org/html/rfc4422#page-8
+   if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) {
+   die invalid smtp auth: '${smtp_auth}';
+   }
+
# TODO: Authentication may fail not because credentials were
# invalid but due to other reasons, in which we should not
# reject credentials.
@@ -1142,6 +1152,20 @@ sub smtp_auth_maybe {
'password' = $smtp_authpass
}, sub {
my $cred = shift;
+
+   if ($smtp_auth) {
+   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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jan Viktorin]

On Sun, 9 Aug 2015 14:13:33 -0400
Eric Sunshine sunsh...@sunshineco.com wrote:

 On Wed, Aug 5, 2015 at 3:17 AM, Jan Viktorin
 vikto...@rehivetech.com wrote:
  Do I understand well that you are complaining about too
  narrow commmit message?
 
 Yes, I'm a complainer. ;-) It's minor, though, not a big deal, and
 certainly not worth a re-roll if that was the only issue. In fact,
 other than the undesirable Supported: line in the documentation, all
 comments on v2 were minor and not demanding of a re-roll.

:)

 
  I am trying to figure out how to write a test. It is
  not very clear to me, what the testing suite does. My
  attempt looks this way at the moment:
 
  1657 do_smtp_auth_test() {
  1658 git send-email \
  1659 --from=Example nob...@example.com \
  1660 --to=some...@example.com \
  1661 --smtp-server=$(pwd)/fake.sendmail \
  1662 --smtp-auth=$1 \
  1663 -v \
  1664 0001-*.patch \
  1665 2errors out
  1666 }
  1667
  1668 test_expect_success $PREREQ 'accepts SMTP AUTH mechanisms (see
  RFC-4422, p. 8)' ' 1669 do_smtp_auth_test PLAIN LOGIN
  CRAM-MD5 DIGEST-MD5 GSSAPI EXTERNAL ANONYMOUS  1670
  do_smtp_auth_test ABCDEFGHIKLMNOPQRSTUVWXYZ 0123456789_-
 
 Wouldn't this one fail the regex check you added which limits the
 length to 20 characters?

Yes, it would fail. But it does not work anyway...

 
  1671 '
  1672
  1673 test_expect_success $PREREQ 'does not accept non-RFC-4422
  strings for SMTP AUTH' ' 1674 test_must_fail
  do_smtp_auth_test ../ATTACK  1675 test_must_fail
  do_smtp_auth_test TOO-LONG-BUT-VALID-STRING  1676
  test_must_fail do_smtp_auth_test no-lower-case-sorry 1677 '
 
  * I do not know yet, what to check after each do_smtp_auth_test
  call.
 
 If you were able somehow to capture the interaction with
 Auth::SASL::Perl, then you'd probably want to test if it received the
 whitelisted mechanisms specified via --smtp-auth, however... (see
 below)

--smtp-debug

 
  * Perhaps, each case should have its own test_expect_success call?
 
 The grouping seems okay as-is.
 
  * Why send-email -v does not generate any output?
 
 As far as I know, git-send-email doesn't accept a -v flag.

True, I confused it with --smtp-debug. However, what I did not
understand was the testing framework. The TAP harness discards
everything (I expected some automatic redirection to a file for each
test.). Later I found the --verbose option that allows to see some
output from tests.

 
(I found a directory 'trash directory.t9001-send-email', however,
  the errors file is always empty.)
 
 Was it empty even for the cases which should have triggered the
 validation regex to invoke die()?
 
  * Is there any other place where the files out, errors are placed?
 
 No.
 
  * I have no idea what the fake.sendmail does (I could see its
  contents but still...). Is it suitable for my tests?
 
 It dumps its command-line arguments to one file (commandline) and
 its stdin to another (msgtxt), but otherwise does no work. This is
 useful for tests which need to make sure that the command-line and/or
 message content gets augmented in some way, but won't help your case
 since it can't capture the script's interaction with
 Authen::SASL::Perl.

I can see it now. Either Perl implementation or a sendmail binary is
used. Unfortunately, this is very unfriendly for such testing.

 
  * Should I check the behaviour '--smtp-auth overrides
sendemail.smtpAuth'?
 
 That would be nice, but there doesn't seem to be a good way to do it
 via an existing testing mechanism since you can't check the
 git-sendemail's interaction with Auth::SASL::Perl. The same holds for
 your question above about what to check after each do_smtp_auth_test()
 call.
 
 One possibility which comes to mind is to create a fake
 Authen::SASL::Perl which merely dumps its input mechanisms to a file,
 and arrange for the Perl search path to find the fake one instead. You
 could then check the output file to see if it reflects your
 expectations. However, this may be overkill and perhaps not worth the
 effort (especially if you're not a Perl programmer).

I think that Authen::SASL::Perl mock would not help. I wanted to create
some fake sendmail (but this is impossible as stated above because
then the perl modules are not used). So the only way would be to
provide some fake socket with a static content on the other side. This
is really an overkill to just test the few lines of code.

So, what more can I do for this feature?

I think that the basic regex test is OK. It can accept lowercase
letters and do an explicit uppercase call. I do not like to rely on
internals of the SASL library. As you could see, the SASL::Perl does
not check its inputs in a very good way and its code is quite unclear
(strange for a library providing security features).

-- 
  Jan ViktorinE-mail: vikto...@rehivetech.com
  System

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

[Jan Viktorin]

Hello Eric, all,

thanks for comments, the coding style will be fixed
in the next version (I cannot find a way how to set
vim to help me with those ifSPACE( issues. I always/often
forget it when writing so I never do it to be consistent.).

Do I understand well that you are complaining about too
narrow commmit message?

I am trying to figure out how to write a test. It is
not very clear to me, what the testing suite does. My
attempt looks this way at the moment:

1657 do_smtp_auth_test() {
1658 git send-email \
1659 --from=Example nob...@example.com \
1660 --to=some...@example.com \
1661 --smtp-server=$(pwd)/fake.sendmail \
1662 --smtp-auth=$1 \
1663 -v \
1664 0001-*.patch \
1665 2errors out
1666 }
1667 
1668 test_expect_success $PREREQ 'accepts SMTP AUTH mechanisms (see RFC-4422, 
p. 8)' '
1669 do_smtp_auth_test PLAIN LOGIN CRAM-MD5 DIGEST-MD5 GSSAPI EXTERNAL 
ANONYMOUS 
1670 do_smtp_auth_test ABCDEFGHIKLMNOPQRSTUVWXYZ 0123456789_-
1671 '
1672 
1673 test_expect_success $PREREQ 'does not accept non-RFC-4422 strings for SMTP 
AUTH' '
1674 test_must_fail do_smtp_auth_test ../ATTACK 
1675 test_must_fail do_smtp_auth_test TOO-LONG-BUT-VALID-STRING 
1676 test_must_fail do_smtp_auth_test no-lower-case-sorry
1677 '

* I do not know yet, what to check after each do_smtp_auth_test call.
* Perhaps, each case should have its own test_expect_success call?
* Why send-email -v does not generate any output?
  (I found a directory 'trash directory.t9001-send-email', however, the
  errors file is always empty.)
* Is there any other place where the files out, errors are placed?
* I have no idea what the fake.sendmail does (I could see its contents
  but still...). Is it suitable for my tests?
* Should I check the behaviour '--smtp-auth overrides
  sendemail.smtpAuth'?

Regards
Jan

On Sun, 2 Aug 2015 14:57:19 -0400
Eric Sunshine sunsh...@sunshineco.com wrote:

 On Sun, Aug 2, 2015 at 12:42 PM, Jan Viktorin
 vikto...@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 vikto...@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-debug0|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

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 gits...@pobox.com wrote:

 Jan Viktorin vikto...@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: vikto...@rehivetech.com
   System Architect  Web:www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] 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) 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.

Signed-off-by: Jan Viktorin vikto...@rehivetech.com
---
Changes v1 - v2:
  - check user input by regex
  - added documentation
  - still missing a test

 Documentation/git-send-email.txt |  8 
 git-send-email.perl  | 25 -
 2 files changed, 32 insertions(+), 1 deletion(-)

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
to determine your FQDN automatically.  Default is the value of
'sendemail.smtpDomain'.
 
+--smtp-auth=mechs::
+   Specify allowed SMTP-AUTH mechanisms. This setting forces using only
+   the listed mechanisms. Separate allowed mechanisms by a whitespace.
+   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
+   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.
+
 --smtp-pass[=password]::
Password for SMTP-AUTH. The argument is optional: If no
argument is specified, then the empty string is used as
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.
+ This setting forces to use one of the 
listed methods.
+ Supported: PLAIN LOGIN CRAM-MD5 
DIGEST-MD5.
 --smtp-debug0|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},
@@ -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}';
+   }
+
# 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) {
+   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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 sunsh...@sunshineco.com wrote:

 On Sat, Aug 1, 2015 at 2:19 PM, Jan Viktorin
 vikto...@rehivetech.com wrote:
  On Sat, 1 Aug 2015 05:33:28 -0400 Eric Sunshine
  sunsh...@sunshineco.com wrote:
  On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin
  vikto...@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 ViktorinE-mail: vikto...@rehivetech.com
  System ArchitectWeb:www.RehiveTech.com
  RehiveTech  Phone: +420 606 201 868
  Brno, Czech Republic
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 sunsh...@sunshineco.com wrote:

 On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin
 vikto...@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 vikto...@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

[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 vikto...@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-debug0|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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html