{"thread":{"id":"39996","subject":"[PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms","startedAt":"2015-08-02T16:42:49Z","lastAt":"2015-08-12T00:01:50Z","messageCount":10,"participants":["Jan Viktorin","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"267289","messageId":"1438533769-17460-1-git-send-email-viktorin@rehivetech.com","threadId":"39996","inReplyTo":null,"subject":"[PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Jan Viktorin","fromEmail":"viktorin@rehivetech.com","sentAt":"2015-08-02T16:42:49Z","receivedAt":"2015-08-02T16:42:49Z","isPatch":true,"sender":{"key":"viktorin@rehivetech.com","avatar":null},"body":"When sending an e-mail, the client and server must\nagree on an authentication mechanism. Some servers\n(due to misconfiguration or a bug) deny valid\ncredentials for certain mechanisms. In this patch,\na new option --smtp-auth and configuration entry\nsmtpauth are introduced. If smtp_auth is defined,\nit works as a whitelist of allowed mechanisms for\nauthentication selected from the ones supported by\nthe installed SASL perl library.\n\nSigned-off-by: Jan Viktorin \n---\nChanges v1 -> v2:\n - check user input by regex\n - added documentation\n - still missing a test\n\n Documentation/git-send-email.txt | 8 ++++++++\n git-send-email.perl | 25 ++++++++++++++++++++++++-\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 7ae467b..c237c80 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -171,6 +171,14 @@ Sending\n \tto determine your FQDN automatically. Default is the value of\n \t'sendemail.smtpDomain'.\n \n+--smtp-auth=::\n+\tSpecify allowed SMTP-AUTH mechanisms. This setting forces using only\n+\tthe listed mechanisms. Separate allowed mechanisms by a whitespace.\n+\tExample: PLAIN LOGIN GSSAPI. If at least one of the specified mechanisms\n+\tmatchs those advertised by the SMTP server and it is supported by the SASL\n+\tlibrary we use, it is used for authentication. If neither of 'sendemail.smtpAuth'\n+\tor '--smtp-auth' is specified, all mechanisms supported on client can be used.\n+\n --smtp-pass[=]::\n \tPassword for SMTP-AUTH. The argument is optional: If no\n \targument is specified, then the empty string is used as\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex ae9f869..ebc1e90 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -75,6 +75,9 @@ git send-email [options] \n Pass an empty string to disable certificate\n verification.\n --smtp-domain * The domain name sent to HELO/EHLO handshake\n+ --smtp-auth * Space separated list of allowed AUTH methods.\n+ This setting forces to use one of the listed methods.\n+ Supported: PLAIN LOGIN CRAM-MD5 DIGEST-MD5.\n --smtp-debug <0|1> * Disable, enable Net::SMTP debug.\n \n Automating:\n@@ -208,7 +211,7 @@ my ($cover_cc, $cover_to);\n my ($to_cmd, $cc_cmd);\n my ($smtp_server, $smtp_server_port, @smtp_server_options);\n my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);\n-my ($identity, $aliasfiletype, @alias_files, $smtp_domain);\n+my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);\n my ($validate, $confirm);\n my (@suppress_cc);\n my ($auto_8bit_encoding);\n@@ -239,6 +242,7 @@ my %config_settings = (\n \"smtppass\" => \\$smtp_authpass,\n \"smtpsslcertpath\" => \\$smtp_ssl_cert_path,\n \"smtpdomain\" => \\$smtp_domain,\n+ \"smtpauth\" => \\$smtp_auth,\n \"to\" => \\@initial_to,\n \"tocmd\" => \\$to_cmd,\n \"cc\" => \\@initial_cc,\n@@ -310,6 +314,7 @@ my $rc = GetOptions(\"h\" => \\$help,\n \t\t \"smtp-ssl-cert-path=s\" => \\$smtp_ssl_cert_path,\n \t\t \"smtp-debug:i\" => \\$debug_net_smtp,\n \t\t \"smtp-domain:s\" => \\$smtp_domain,\n+\t\t \"smtp-auth=s\" => \\$smtp_auth,\n \t\t \"identity=s\" => \\$identity,\n \t\t \"annotate!\" => \\$annotate,\n \t\t \"no-annotate\" => sub {$annotate = 0},\n@@ -1136,6 +1141,10 @@ sub smtp_auth_maybe {\n \t\tAuthen::SASL->import(qw(Perl));\n \t};\n \n+\tif($smtp_auth !~ /^(\\b[A-Z0-9-_]{1,20}\\s*)*$/) {\n+\t\tdie \"invalid smtp auth: '${smtp_auth}'\";\n+\t}\n+\n \t# TODO: Authentication may fail not because credentials were\n \t# invalid but due to other reasons, in which we should not\n \t# reject credentials.\n@@ -1148,6 +1157,20 @@ sub smtp_auth_maybe {\n \t\t'password' => $smtp_authpass\n \t}, sub {\n \t\tmy $cred = shift;\n+\n+\t\tif($smtp_auth) {\n+\t\t\tmy $sasl = Authen::SASL->new(\n+\t\t\t\tmechanism => $smtp_auth,\n+\t\t\t\tcallback => {\n+\t\t\t\t\tuser => $cred->{'username'},\n+\t\t\t\t\tpass => $cred->{'password'},\n+\t\t\t\t\tauthname => $cred->{'username'},\n+\t\t\t\t}\n+\t\t\t);\n+\n+\t\t\treturn !!$smtp->auth($sasl);\n+\t\t}\n+\n \t\treturn !!$smtp->auth($cred->{'username'}, $cred->{'password'});\n \t});\n \n-- \n2.5.0\n"},{"id":"267296","messageId":"CAPig+cQwFxVtO1C_RAumGP6_et21ggORB4jhpcUtBYNznNH1qA@mail.gmail.com","threadId":"39996","inReplyTo":"1438533769-17460-1-git-send-email-viktorin@rehivetech.com","subject":"Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-02T18:57:19Z","receivedAt":"2015-08-02T18:57:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 2, 2015 at 12:42 PM, Jan Viktorin wrote:\n> When sending an e-mail, the client and server must\n> agree on an authentication mechanism. Some servers\n> (due to misconfiguration or a bug) deny valid\n> credentials for certain mechanisms. In this patch,\n> a new option --smtp-auth and configuration entry\n> smtpauth are introduced. If smtp_auth is defined,\n> it works as a whitelist of allowed mechanisms for\n> authentication selected from the ones supported by\n> the installed SASL perl library.\n\nNit: This would read a bit more nicely if wrapped to 70-72 columns.\n\n> Signed-off-by: Jan Viktorin \n> ---\n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> index 7ae467b..c237c80 100644\n> --- a/Documentation/git-send-email.txt\n> +++ b/Documentation/git-send-email.txt\n> @@ -171,6 +171,14 @@ Sending\n> +--smtp-auth=::\n> + Specify allowed SMTP-AUTH mechanisms. This setting forces using only\n> + the listed mechanisms. Separate allowed mechanisms by a whitespace.\n\nPerhaps:\n\n Whitespace-separated list of allowed SMTP-AUTH mechanisms.\n\n> + Example: PLAIN LOGIN GSSAPI. If at least one of the specified mechanisms\n> + matchs those advertised by the SMTP server and it is supported by the SASL\n\ns/matchs/matches/\n\n> + library we use, it is used for authentication. If neither of 'sendemail.smtpAuth'\n> + or '--smtp-auth' is specified, all mechanisms supported on client can be used.\n\ns/neither of/neither/\ns/or/nor/\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index ae9f869..ebc1e90 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -75,6 +75,9 @@ git send-email [options] \n> Pass an empty string to disable certificate\n> verification.\n> --smtp-domain * The domain name sent to HELO/EHLO handshake\n> + --smtp-auth * Space separated list of allowed AUTH methods.\n\ns/Space separated/Space-separated/\n\n> + This setting forces to use one of the listed methods.\n> + Supported: PLAIN LOGIN CRAM-MD5 DIGEST-MD5.\n\nSince you're no longer checking explicitly for these mechanisms, you\nprobably want to drop the \"Supported:\" line.\n\n> --smtp-debug <0|1> * Disable, enable Net::SMTP debug.\n>\n> Automating:\n> @@ -1136,6 +1141,10 @@ sub smtp_auth_maybe {\n> Authen::SASL->import(qw(Perl));\n> };\n>\n> + if($smtp_auth !~ /^(\\b[A-Z0-9-_]{1,20}\\s*)*$/) {\n> + die \"invalid smtp auth: '${smtp_auth}'\";\n> + }\n\nStyle: space after 'if'\n\n> # TODO: Authentication may fail not because credentials were\n> # invalid but due to other reasons, in which we should not\n> # reject credentials.\n> @@ -1148,6 +1157,20 @@ sub smtp_auth_maybe {\n> 'password' => $smtp_authpass\n> }, sub {\n> my $cred = shift;\n> +\n> + if($smtp_auth) {\n\nStyle: space after 'if'\n\n> + my $sasl = Authen::SASL->new(\n> + mechanism => $smtp_auth,\n> + callback => {\n> + user => $cred->{'username'},\n> + pass => $cred->{'password'},\n> + authname => $cred->{'username'},\n> + }\n> + );\n> +\n> + return !!$smtp->auth($sasl);\n> + }\n> +\n> return !!$smtp->auth($cred->{'username'}, $cred->{'password'});\n> });\n>\n> --\n> 2.5.0\n"},{"id":"267496","messageId":"20150805091747.242e8fa1@jvn","threadId":"39996","inReplyTo":"CAPig+cQwFxVtO1C_RAumGP6_et21ggORB4jhpcUtBYNznNH1qA@mail.gmail.com","subject":"Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Jan Viktorin","fromEmail":"viktorin@rehivetech.com","sentAt":"2015-08-05T07:17:47Z","receivedAt":"2015-08-05T07:17:47Z","isPatch":true,"sender":{"key":"viktorin@rehivetech.com","avatar":null},"body":"Hello Eric, all,\n\nthanks for comments, the coding style will be fixed\nin the next version (I cannot find a way how to set\nvim to help me with those if( issues. I always/often\nforget it when writing so I never do it to be consistent.).\n\nDo I understand well that you are complaining about too\nnarrow commmit message?\n\nI am trying to figure out how to write a test. It is\nnot very clear to me, what the testing suite does. My\nattempt looks this way at the moment:\n\n1657 do_smtp_auth_test() {\n1658 git send-email \\\n1659 --from=\"Example \" \\\n1660 --to=someone@example.com \\\n1661 --smtp-server=\"$(pwd)/fake.sendmail\" \\\n1662 --smtp-auth=\"$1\" \\\n1663 -v \\\n1664 0001-*.patch \\\n1665 2>errors >out\n1666 }\n1667 \n1668 test_expect_success $PREREQ 'accepts SMTP AUTH mechanisms (see RFC-4422, p. 8)' '\n1669 do_smtp_auth_test \"PLAIN LOGIN CRAM-MD5 DIGEST-MD5 GSSAPI EXTERNAL ANONYMOUS\" &&\n1670 do_smtp_auth_test \"ABCDEFGHIKLMNOPQRSTUVWXYZ 0123456789_-\"\n1671 '\n1672 \n1673 test_expect_success $PREREQ 'does not accept non-RFC-4422 strings for SMTP AUTH' '\n1674 test_must_fail do_smtp_auth_test \"../ATTACK\" &&\n1675 test_must_fail do_smtp_auth_test \"TOO-LONG-BUT-VALID-STRING\" &&\n1676 test_must_fail do_smtp_auth_test \"no-lower-case-sorry\"\n1677 '\n\n* I do not know yet, what to check after each do_smtp_auth_test call.\n* Perhaps, each case should have its own test_expect_success call?\n* Why send-email -v does not generate any output?\n (I found a directory 'trash directory.t9001-send-email', however, the\n errors file is always empty.)\n* Is there any other place where the files out, errors are placed?\n* I have no idea what the fake.sendmail does (I could see its contents\n but still...). Is it suitable for my tests?\n* Should I check the behaviour '--smtp-auth overrides\n sendemail.smtpAuth'?\n\nRegards\nJan\n\nOn Sun, 2 Aug 2015 14:57:19 -0400\nEric Sunshine wrote:\n\n> On Sun, Aug 2, 2015 at 12:42 PM, Jan Viktorin\n> wrote:\n> > When sending an e-mail, the client and server must\n> > agree on an authentication mechanism. Some servers\n> > (due to misconfiguration or a bug) deny valid\n> > credentials for certain mechanisms. In this patch,\n> > a new option --smtp-auth and configuration entry\n> > smtpauth are introduced. If smtp_auth is defined,\n> > it works as a whitelist of allowed mechanisms for\n> > authentication selected from the ones supported by\n> > the installed SASL perl library.\n> \n> Nit: This would read a bit more nicely if wrapped to 70-72 columns.\n> \n> > Signed-off-by: Jan Viktorin \n> > ---\n> > diff --git a/Documentation/git-send-email.txt\n> > b/Documentation/git-send-email.txt index 7ae467b..c237c80 100644\n> > --- a/Documentation/git-send-email.txt\n> > +++ b/Documentation/git-send-email.txt\n> > @@ -171,6 +171,14 @@ Sending\n> > +--smtp-auth=::\n> > + Specify allowed SMTP-AUTH mechanisms. This setting forces\n> > using only\n> > + the listed mechanisms. Separate allowed mechanisms by a\n> > whitespace.\n> \n> Perhaps:\n> \n> Whitespace-separated list of allowed SMTP-AUTH mechanisms.\n> \n> > + Example: PLAIN LOGIN GSSAPI. If at least one of the\n> > specified mechanisms\n> > + matchs those advertised by the SMTP server and it is\n> > supported by the SASL\n> \n> s/matchs/matches/\n> \n> > + library we use, it is used for authentication. If neither\n> > of 'sendemail.smtpAuth'\n> > + or '--smtp-auth' is specified, all mechanisms supported on\n> > client can be used.\n> \n> s/neither of/neither/\n> s/or/nor/\n> \n> > diff --git a/git-send-email.perl b/git-send-email.perl\n> > index ae9f869..ebc1e90 100755\n> > --- a/git-send-email.perl\n> > +++ b/git-send-email.perl\n> > @@ -75,6 +75,9 @@ git send-email [options] > rev-list options > Pass an empty string to disable certificate\n> > verification.\n> > --smtp-domain * The domain name sent to\n> > HELO/EHLO handshake\n> > + --smtp-auth * Space separated list of\n> > allowed AUTH methods.\n> \n> s/Space separated/Space-separated/\n> \n> > + This setting forces to use\n> > one of the listed methods.\n> > + Supported: PLAIN LOGIN\n> > CRAM-MD5 DIGEST-MD5.\n> \n> Since you're no longer checking explicitly for these mechanisms, you\n> probably want to drop the \"Supported:\" line.\n> \n> > --smtp-debug <0|1> * Disable, enable Net::SMTP\n> > debug.\n> >\n> > Automating:\n> > @@ -1136,6 +1141,10 @@ sub smtp_auth_maybe {\n> > Authen::SASL->import(qw(Perl));\n> > };\n> >\n> > + if($smtp_auth !~ /^(\\b[A-Z0-9-_]{1,20}\\s*)*$/) {\n> > + die \"invalid smtp auth: '${smtp_auth}'\";\n> > + }\n> \n> Style: space after 'if'\n> \n> > # TODO: Authentication may fail not because credentials were\n> > # invalid but due to other reasons, in which we should not\n> > # reject credentials.\n> > @@ -1148,6 +1157,20 @@ sub smtp_auth_maybe {\n> > 'password' => $smtp_authpass\n> > }, sub {\n> > my $cred = shift;\n> > +\n> > + if($smtp_auth) {\n> \n> Style: space after 'if'\n> \n> > + my $sasl = Authen::SASL->new(\n> > + mechanism => $smtp_auth,\n> > + callback => {\n> > + user => $cred->{'username'},\n> > + pass => $cred->{'password'},\n> > + authname =>\n> > $cred->{'username'},\n> > + }\n> > + );\n> > +\n> > + return !!$smtp->auth($sasl);\n> > + }\n> > +\n> > return !!$smtp->auth($cred->{'username'},\n> > $cred->{'password'}); });\n> >\n> > --\n> > 2.5.0\n\n\n\n-- \n Jan Viktorin E-mail: Viktorin@RehiveTech.com\n System Architect Web: www.RehiveTech.com\n RehiveTech Phone: +420 606 201 868\n Brno, Czech Republic\n"},{"id":"267694","messageId":"CAPig+cRenkDWeQWR_QFvy_mrH=n5=hz6kaB3PMd_LLbPWN3U1g@mail.gmail.com","threadId":"39996","inReplyTo":"CAPig+cQwFxVtO1C_RAumGP6_et21ggORB4jhpcUtBYNznNH1qA@mail.gmail.com","subject":"Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-09T17:19:58Z","receivedAt":"2015-08-09T17:19:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 2, 2015 at 2:57 PM, Eric Sunshine wrote:\n> On Sun, Aug 2, 2015 at 12:42 PM, Jan Viktorin wrote:\n>> @@ -1136,6 +1141,10 @@ sub smtp_auth_maybe {\n>> Authen::SASL->import(qw(Perl));\n>> };\n>>\n>> + if($smtp_auth !~ /^(\\b[A-Z0-9-_]{1,20}\\s*)*$/) {\n>> + die \"invalid smtp auth: '${smtp_auth}'\";\n>> + }\n>\n> Style: space after 'if'\n\nBy the way, I notice that Authen::SASL::Perl implementation itself\nnormalizes the incoming mechanism to uppercase, if necessary:\n\n $mechanism =~ s/^\\s*\\b(.*)\\b\\s*$/$1/g;\n $mechanism =~ s/-/_/g;\n $mechanism = uc $mechanism;\n\nSince it doesn't require uppercase, it's not clear how much benefit\nthere is to adding a strict regex check to git-send-email.\n"},{"id":"267695","messageId":"CAPig+cQvu82JzdDDBuzeTvhhEfhULky-q8D0OPqH-Nzmev_bdA@mail.gmail.com","threadId":"39996","inReplyTo":"CAPig+cRenkDWeQWR_QFvy_mrH=n5=hz6kaB3PMd_LLbPWN3U1g@mail.gmail.com","subject":"Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-09T17:45:45Z","receivedAt":"2015-08-09T17:45:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 9, 2015 at 1:19 PM, Eric Sunshine wrote:\n> On Sun, Aug 2, 2015 at 2:57 PM, Eric Sunshine wrote:\n>> On Sun, Aug 2, 2015 at 12:42 PM, Jan Viktorin wrote:\n>>> @@ -1136,6 +1141,10 @@ sub smtp_auth_maybe {\n>>> Authen::SASL->import(qw(Perl));\n>>> };\n>>>\n>>> + if($smtp_auth !~ /^(\\b[A-Z0-9-_]{1,20}\\s*)*$/) {\n>>> + die \"invalid smtp auth: '${smtp_auth}'\";\n>>> + }\n>>\n>> Style: space after 'if'\n>\n> By the way, I notice that Authen::SASL::Perl implementation itself\n> normalizes the incoming mechanism to uppercase, if necessary:\n>\n> $mechanism =~ s/^\\s*\\b(.*)\\b\\s*$/$1/g;\n> $mechanism =~ s/-/_/g;\n> $mechanism = uc $mechanism;\n>\n> Since it doesn't require uppercase, it's not clear how much benefit\n> there is to adding a strict regex check to git-send-email.\n\nHmm, perhaps I was looking at the wrong chunk of code. You had already\nreferenced the real code here[1], and it doesn't appear to do any case\ntransformation (it only replaces \"-\" with \"_\").\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/275161\n"},{"id":"267696","messageId":"CAPig+cQ0fSc+rjzgDyaw4xvCPCswJLDcQSmbxXnxG-uc6zB0qA@mail.gmail.com","threadId":"39996","inReplyTo":"20150805091747.242e8fa1@jvn","subject":"Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-09T18:13:33Z","receivedAt":"2015-08-09T18:13:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 5, 2015 at 3:17 AM, Jan Viktorin wrote:\n> Do I understand well that you are complaining about too\n> narrow commmit message?\n\nYes, I'm a complainer. ;-) It's minor, though, not a big deal, and\ncertainly not worth a re-roll if that was the only issue. In fact,\nother than the undesirable \"Supported:\" line in the documentation, all\ncomments on v2 were minor and not demanding of a re-roll.\n\n> I am trying to figure out how to write a test. It is\n> not very clear to me, what the testing suite does. My\n> attempt looks this way at the moment:\n>\n> 1657 do_smtp_auth_test() {\n> 1658 git send-email \\\n> 1659 --from=\"Example \" \\\n> 1660 --to=someone@example.com \\\n> 1661 --smtp-server=\"$(pwd)/fake.sendmail\" \\\n> 1662 --smtp-auth=\"$1\" \\\n> 1663 -v \\\n> 1664 0001-*.patch \\\n> 1665 2>errors >out\n> 1666 }\n> 1667\n> 1668 test_expect_success $PREREQ 'accepts SMTP AUTH mechanisms (see RFC-4422, p. 8)' '\n> 1669 do_smtp_auth_test \"PLAIN LOGIN CRAM-MD5 DIGEST-MD5 GSSAPI EXTERNAL ANONYMOUS\" &&\n> 1670 do_smtp_auth_test \"ABCDEFGHIKLMNOPQRSTUVWXYZ 0123456789_-\"\n\nWouldn't this one fail the regex check you added which limits the\nlength to 20 characters?\n\n> 1671 '\n> 1672\n> 1673 test_expect_success $PREREQ 'does not accept non-RFC-4422 strings for SMTP AUTH' '\n> 1674 test_must_fail do_smtp_auth_test \"../ATTACK\" &&\n> 1675 test_must_fail do_smtp_auth_test \"TOO-LONG-BUT-VALID-STRING\" &&\n> 1676 test_must_fail do_smtp_auth_test \"no-lower-case-sorry\"\n> 1677 '\n>\n> * I do not know yet, what to check after each do_smtp_auth_test call.\n\nIf you were able somehow to capture the interaction with\nAuth::SASL::Perl, then you'd probably want to test if it received the\nwhitelisted mechanisms specified via --smtp-auth, however... (see\nbelow)\n\n> * Perhaps, each case should have its own test_expect_success call?\n\nThe grouping seems okay as-is.\n\n> * Why send-email -v does not generate any output?\n\nAs far as I know, git-send-email doesn't accept a -v flag.\n\n> (I found a directory 'trash directory.t9001-send-email', however, the\n> errors file is always empty.)\n\nWas it empty even for the cases which should have triggered the\nvalidation regex to invoke die()?\n\n> * Is there any other place where the files out, errors are placed?\n\nNo.\n\n> * I have no idea what the fake.sendmail does (I could see its contents\n> but still...). Is it suitable for my tests?\n\nIt dumps its command-line arguments to one file (\"commandline\") and\nits stdin to another (\"msgtxt\"), but otherwise does no work. This is\nuseful for tests which need to make sure that the command-line and/or\nmessage content gets augmented in some way, but won't help your case\nsince it can't capture the script's interaction with\nAuthen::SASL::Perl.\n\n> * Should I check the behaviour '--smtp-auth overrides\n> sendemail.smtpAuth'?\n\nThat would be nice, but there doesn't seem to be a good way to do it\nvia an existing testing mechanism since you can't check the\ngit-sendemail's interaction with Auth::SASL::Perl. The same holds for\nyour question above about what to check after each do_smtp_auth_test()\ncall.\n\nOne possibility which comes to mind is to create a fake\nAuthen::SASL::Perl which merely dumps its input mechanisms to a file,\nand arrange for the Perl search path to find the fake one instead. You\ncould then check the output file to see if it reflects your\nexpectations. However, this may be overkill and perhaps not worth the\neffort (especially if you're not a Perl programmer).\n"},{"id":"267744","messageId":"20150810120642.2a0baac2@jvn","threadId":"39996","inReplyTo":"CAPig+cQ0fSc+rjzgDyaw4xvCPCswJLDcQSmbxXnxG-uc6zB0qA@mail.gmail.com","subject":"Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Jan Viktorin","fromEmail":"viktorin@rehivetech.com","sentAt":"2015-08-10T10:06:42Z","receivedAt":"2015-08-10T10:06:42Z","isPatch":true,"sender":{"key":"viktorin@rehivetech.com","avatar":null},"body":"On Sun, 9 Aug 2015 14:13:33 -0400\nEric Sunshine wrote:\n\n> On Wed, Aug 5, 2015 at 3:17 AM, Jan Viktorin\n> wrote:\n> > Do I understand well that you are complaining about too\n> > narrow commmit message?\n> \n> Yes, I'm a complainer. ;-) It's minor, though, not a big deal, and\n> certainly not worth a re-roll if that was the only issue. In fact,\n> other than the undesirable \"Supported:\" line in the documentation, all\n> comments on v2 were minor and not demanding of a re-roll.\n\n:)\n\n> \n> > I am trying to figure out how to write a test. It is\n> > not very clear to me, what the testing suite does. My\n> > attempt looks this way at the moment:\n> >\n> > 1657 do_smtp_auth_test() {\n> > 1658 git send-email \\\n> > 1659 --from=\"Example \" \\\n> > 1660 --to=someone@example.com \\\n> > 1661 --smtp-server=\"$(pwd)/fake.sendmail\" \\\n> > 1662 --smtp-auth=\"$1\" \\\n> > 1663 -v \\\n> > 1664 0001-*.patch \\\n> > 1665 2>errors >out\n> > 1666 }\n> > 1667\n> > 1668 test_expect_success $PREREQ 'accepts SMTP AUTH mechanisms (see\n> > RFC-4422, p. 8)' ' 1669 do_smtp_auth_test \"PLAIN LOGIN\n> > CRAM-MD5 DIGEST-MD5 GSSAPI EXTERNAL ANONYMOUS\" && 1670\n> > do_smtp_auth_test \"ABCDEFGHIKLMNOPQRSTUVWXYZ 0123456789_-\"\n> \n> Wouldn't this one fail the regex check you added which limits the\n> length to 20 characters?\n\nYes, it would fail. But it does not work anyway...\n\n> \n> > 1671 '\n> > 1672\n> > 1673 test_expect_success $PREREQ 'does not accept non-RFC-4422\n> > strings for SMTP AUTH' ' 1674 test_must_fail\n> > do_smtp_auth_test \"../ATTACK\" && 1675 test_must_fail\n> > do_smtp_auth_test \"TOO-LONG-BUT-VALID-STRING\" && 1676\n> > test_must_fail do_smtp_auth_test \"no-lower-case-sorry\" 1677 '\n> >\n> > * I do not know yet, what to check after each do_smtp_auth_test\n> > call.\n> \n> If you were able somehow to capture the interaction with\n> Auth::SASL::Perl, then you'd probably want to test if it received the\n> whitelisted mechanisms specified via --smtp-auth, however... (see\n> below)\n\n--smtp-debug\n\n> \n> > * Perhaps, each case should have its own test_expect_success call?\n> \n> The grouping seems okay as-is.\n> \n> > * Why send-email -v does not generate any output?\n> \n> As far as I know, git-send-email doesn't accept a -v flag.\n\nTrue, I confused it with --smtp-debug. However, what I did not\nunderstand was the testing framework. The TAP harness discards\neverything (I expected some automatic redirection to a file for each\ntest.). Later I found the --verbose option that allows to see some\noutput from tests.\n\n> \n> > (I found a directory 'trash directory.t9001-send-email', however,\n> > the errors file is always empty.)\n> \n> Was it empty even for the cases which should have triggered the\n> validation regex to invoke die()?\n> \n> > * Is there any other place where the files out, errors are placed?\n> \n> No.\n> \n> > * I have no idea what the fake.sendmail does (I could see its\n> > contents but still...). Is it suitable for my tests?\n> \n> It dumps its command-line arguments to one file (\"commandline\") and\n> its stdin to another (\"msgtxt\"), but otherwise does no work. This is\n> useful for tests which need to make sure that the command-line and/or\n> message content gets augmented in some way, but won't help your case\n> since it can't capture the script's interaction with\n> Authen::SASL::Perl.\n\nI can see it now. Either Perl implementation or a sendmail binary is\nused. Unfortunately, this is very unfriendly for such testing.\n\n> \n> > * Should I check the behaviour '--smtp-auth overrides\n> > sendemail.smtpAuth'?\n> \n> That would be nice, but there doesn't seem to be a good way to do it\n> via an existing testing mechanism since you can't check the\n> git-sendemail's interaction with Auth::SASL::Perl. The same holds for\n> your question above about what to check after each do_smtp_auth_test()\n> call.\n> \n> One possibility which comes to mind is to create a fake\n> Authen::SASL::Perl which merely dumps its input mechanisms to a file,\n> and arrange for the Perl search path to find the fake one instead. You\n> could then check the output file to see if it reflects your\n> expectations. However, this may be overkill and perhaps not worth the\n> effort (especially if you're not a Perl programmer).\n\nI think that Authen::SASL::Perl mock would not help. I wanted to create\nsome fake sendmail (but this is impossible as stated above because\nthen the perl modules are not used). So the only way would be to\nprovide some fake socket with a static content on the other side. This\nis really an overkill to just test the few lines of code.\n\nSo, what more can I do for this feature?\n\nI think that the basic regex test is OK. It can accept lowercase\nletters and do an explicit uppercase call. I do not like to rely on\ninternals of the SASL library. As you could see, the SASL::Perl does\nnot check its inputs in a very good way and its code is quite unclear\n(strange for a library providing security features).\n\n-- \n Jan Viktorin E-mail: Viktorin@RehiveTech.com\n System Architect Web: www.RehiveTech.com\n RehiveTech Phone: +420 606 201 868\n Brno, Czech Republic\n"},{"id":"267804","messageId":"CAPig+cSU_rgRvfETfY7TiY6X0B6Tt6N0+GMVgSsYH-6zMteAgg@mail.gmail.com","threadId":"39996","inReplyTo":"20150810120642.2a0baac2@jvn","subject":"Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-10T23:43:24Z","receivedAt":"2015-08-10T23:43:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 10, 2015 at 6:06 AM, Jan Viktorin wrote:\n> On Sun, 9 Aug 2015 14:13:33 -0400\n> Eric Sunshine wrote:\n>> One possibility which comes to mind is to create a fake\n>> Authen::SASL::Perl which merely dumps its input mechanisms to a file,\n>> and arrange for the Perl search path to find the fake one instead. You\n>> could then check the output file to see if it reflects your\n>> expectations. However, this may be overkill and perhaps not worth the\n>> effort (especially if you're not a Perl programmer).\n>\n> I think that Authen::SASL::Perl mock would not help. I wanted to create\n> some fake sendmail (but this is impossible as stated above because\n> then the perl modules are not used). So the only way would be to\n> provide some fake socket with a static content on the other side. This\n> is really an overkill to just test the few lines of code.\n\nAgreed.\n\n> So, what more can I do for this feature?\n\nI don't have any further suggestions. Other than the unwanted\n\"Supported:\" line in the documentation and the couple style issues[1],\nthe patch seems sufficiently complete, as-is. The validation regex\ngets a \"meh\" from me merely because it's not clear how beneficial it\nwill be in practice, but that's not an outright objection; I don't\nfeel strongly about it either way.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/275150\n\n> I think that the basic regex test is OK. It can accept lowercase\n> letters and do an explicit uppercase call. I do not like to rely on\n> internals of the SASL library. As you could see, the SASL::Perl does\n> not check its inputs in a very good way and its code is quite unclear\n> (strange for a library providing security features).\n"},{"id":"267883","messageId":"1439336384-1445-1-git-send-email-viktorin@rehivetech.com","threadId":"39996","inReplyTo":"1438533769-17460-1-git-send-email-viktorin@rehivetech.com","subject":"[PATCH v3] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Jan Viktorin","fromEmail":"viktorin@rehivetech.com","sentAt":"2015-08-11T23:39:44Z","receivedAt":"2015-08-11T23:39:44Z","isPatch":true,"sender":{"key":"viktorin@rehivetech.com","avatar":null},"body":"When sending an e-mail, the client and server must agree on an\nauthentication mechanism. Some servers (due to misconfiguration\nor a bug) deny valid credentials for certain mechanisms. In this\npatch, a new option --smtp-auth and configuration entry smtpAuth\nare introduced. If smtp_auth is defined, it works as a whitelist\nof allowed mechanisms for authentication selected from the ones\nsupported by the installed SASL perl library.\n\nSigned-off-by: Jan Viktorin \n---\n Documentation/git-send-email.txt | 11 +++++++++++\n git-send-email.perl | 26 +++++++++++++++++++++++++-\n 2 files changed, 36 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex f14705e..82c6ae8 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -171,6 +171,17 @@ Sending\n \tto determine your FQDN automatically. Default is the value of\n \t'sendemail.smtpDomain'.\n \n+--smtp-auth=::\n+\tWhitespace-separated list of allowed SMTP-AUTH mechanisms. This setting\n+\tforces using only the listed mechanisms. Example:\n+\n+\t$ git send-email --smtp-auth=\"PLAIN LOGIN GSSAPI\" ...\n+\n+\tIf at least one of the specified mechanisms matches the ones advertised by the\n+\tSMTP server and if it is supported by the utilized SASL library, the mechanism\n+\tis used for authentication. If neither 'sendemail.smtpAuth' nor '--smtp-auth'\n+\tis specified, all mechanisms supported by the SASL library can be used.\n+\n --smtp-pass[=]::\n \tPassword for SMTP-AUTH. The argument is optional: If no\n \targument is specified, then the empty string is used as\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex b660cc2..a7192c4 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -75,6 +75,8 @@ git send-email [options] \n Pass an empty string to disable certificate\n verification.\n --smtp-domain * The domain name sent to HELO/EHLO handshake\n+ --smtp-auth * Space-separated list of allowed AUTH methods.\n+ This setting forces to use one of the listed methods.\n --smtp-debug <0|1> * Disable, enable Net::SMTP debug.\n \n Automating:\n@@ -208,7 +210,7 @@ my ($cover_cc, $cover_to);\n my ($to_cmd, $cc_cmd);\n my ($smtp_server, $smtp_server_port, @smtp_server_options);\n my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);\n-my ($identity, $aliasfiletype, @alias_files, $smtp_domain);\n+my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);\n my ($validate, $confirm);\n my (@suppress_cc);\n my ($auto_8bit_encoding);\n@@ -239,6 +241,7 @@ my %config_settings = (\n \"smtppass\" => \\$smtp_authpass,\n \"smtpsslcertpath\" => \\$smtp_ssl_cert_path,\n \"smtpdomain\" => \\$smtp_domain,\n+ \"smtpauth\" => \\$smtp_auth,\n \"to\" => \\@initial_to,\n \"tocmd\" => \\$to_cmd,\n \"cc\" => \\@initial_cc,\n@@ -310,6 +313,7 @@ my $rc = GetOptions(\"h\" => \\$help,\n \t\t \"smtp-ssl-cert-path=s\" => \\$smtp_ssl_cert_path,\n \t\t \"smtp-debug:i\" => \\$debug_net_smtp,\n \t\t \"smtp-domain:s\" => \\$smtp_domain,\n+\t\t \"smtp-auth=s\" => \\$smtp_auth,\n \t\t \"identity=s\" => \\$identity,\n \t\t \"annotate!\" => \\$annotate,\n \t\t \"no-annotate\" => sub {$annotate = 0},\n@@ -1130,6 +1134,12 @@ sub smtp_auth_maybe {\n \t\tAuthen::SASL->import(qw(Perl));\n \t};\n \n+\t# Check mechanism naming as defined in:\n+\t# https://tools.ietf.org/html/rfc4422#page-8\n+\tif ($smtp_auth !~ /^(\\b[A-Z0-9-_]{1,20}\\s*)*$/) {\n+\t\tdie \"invalid smtp auth: '${smtp_auth}'\";\n+\t}\n+\n \t# TODO: Authentication may fail not because credentials were\n \t# invalid but due to other reasons, in which we should not\n \t# reject credentials.\n@@ -1142,6 +1152,20 @@ sub smtp_auth_maybe {\n \t\t'password' => $smtp_authpass\n \t}, sub {\n \t\tmy $cred = shift;\n+\n+\t\tif ($smtp_auth) {\n+\t\t\tmy $sasl = Authen::SASL->new(\n+\t\t\t\tmechanism => $smtp_auth,\n+\t\t\t\tcallback => {\n+\t\t\t\t\tuser => $cred->{'username'},\n+\t\t\t\t\tpass => $cred->{'password'},\n+\t\t\t\t\tauthname => $cred->{'username'},\n+\t\t\t\t}\n+\t\t\t);\n+\n+\t\t\treturn !!$smtp->auth($sasl);\n+\t\t}\n+\n \t\treturn !!$smtp->auth($cred->{'username'}, $cred->{'password'});\n \t});\n \n-- \n2.5.0\n"},{"id":"267884","messageId":"20150812000150.GA41558@flurp.local","threadId":"39996","inReplyTo":"1439336384-1445-1-git-send-email-viktorin@rehivetech.com","subject":"Re: [PATCH v3] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-12T00:01:50Z","receivedAt":"2015-08-12T00:01:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 12, 2015 at 01:39:44AM +0200, Jan Viktorin wrote:\n> When sending an e-mail, the client and server must agree on an\n> authentication mechanism. Some servers (due to misconfiguration\n> or a bug) deny valid credentials for certain mechanisms. In this\n> patch, a new option --smtp-auth and configuration entry smtpAuth\n> are introduced. If smtp_auth is defined, it works as a whitelist\n> of allowed mechanisms for authentication selected from the ones\n> supported by the installed SASL perl library.\n> \n> Signed-off-by: Jan Viktorin \n> ---\n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> index f14705e..82c6ae8 100644\n> --- a/Documentation/git-send-email.txt\n> +++ b/Documentation/git-send-email.txt\n> @@ -171,6 +171,17 @@ Sending\n> \tto determine your FQDN automatically. Default is the value of\n> \t'sendemail.smtpDomain'.\n> \n> +--smtp-auth=::\n> +\tWhitespace-separated list of allowed SMTP-AUTH mechanisms. This setting\n> +\tforces using only the listed mechanisms. Example:\n> +\n> +\t$ git send-email --smtp-auth=\"PLAIN LOGIN GSSAPI\" ...\n> +\n> +\tIf at least one of the specified mechanisms matches the ones advertised by the\n> +\tSMTP server and if it is supported by the utilized SASL library, the mechanism\n> +\tis used for authentication. If neither 'sendemail.smtpAuth' nor '--smtp-auth'\n> +\tis specified, all mechanisms supported by the SASL library can be used.\n\nUnfortuantely, this won't format correctly in Asciidoc. The following squash-in fixes it...\n\n---- 8< ----\nSubject: [PATCH] fixup! send-email: provide whitelist of SMTP AUTH mechanisms\n\n---\n Documentation/git-send-email.txt | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 82c6ae8..9e4f130 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -174,13 +174,15 @@ Sending\n --smtp-auth=::\n \tWhitespace-separated list of allowed SMTP-AUTH mechanisms. This setting\n \tforces using only the listed mechanisms. Example:\n-\n-\t$ git send-email --smtp-auth=\"PLAIN LOGIN GSSAPI\" ...\n-\n-\tIf at least one of the specified mechanisms matches the ones advertised by the\n-\tSMTP server and if it is supported by the utilized SASL library, the mechanism\n-\tis used for authentication. If neither 'sendemail.smtpAuth' nor '--smtp-auth'\n-\tis specified, all mechanisms supported by the SASL library can be used.\n++\n+------\n+$ git send-email --smtp-auth=\"PLAIN LOGIN GSSAPI\" ...\n+------\n++\n+If at least one of the specified mechanisms matches the ones advertised by the\n+SMTP server and if it is supported by the utilized SASL library, the mechanism\n+is used for authentication. If neither 'sendemail.smtpAuth' nor '--smtp-auth'\n+is specified, all mechanisms supported by the SASL library can be used.\n \n --smtp-pass[=]::\n \tPassword for SMTP-AUTH. The argument is optional: If no\n-- \n2.5.0.276.gf5e568e\n"}]}