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@xxxxxxxxxxxxxx> wrote: > On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin > <viktorin@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxx System Architect Web: 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html