{"thread":{"id":"39989","subject":"[PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","startedAt":"2015-07-31T23:33:37Z","lastAt":"2015-08-03T19:53:20Z","messageCount":11,"participants":["Jan Viktorin","Eric Sunshine","brian m. carlson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"267245","messageId":"1438385617-29159-1-git-send-email-viktorin@rehivetech.com","threadId":"39989","inReplyTo":null,"subject":"[PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Jan Viktorin","fromEmail":"viktorin@rehivetech.com","sentAt":"2015-07-31T23:33:37Z","receivedAt":"2015-07-31T23:33:37Z","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) denies valid\ncredentials for certain mechanisms. In this patch,\na new option --smtp-auth and configuration entry\nsmtpauth are introduced.\n\nIf smtp_auth is defined, it works as a whitelist\nof allowed mechanisms for authentication. There\nare four mechanisms supported: PLAIN, LOGIN,\nCRAM-MD5, DIGEST-MD5. However, their availability\ndepends on the installed SASL library.\n\nSigned-off-by: Jan Viktorin \n---\n git-send-email.perl | 31 ++++++++++++++++++++++++++++++-\n 1 file changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex ae9f869..b00ed9d 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@@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {\n \t\treturn 1;\n \t}\n \n+\t# Do not allow arbitrary strings.\n+\tmy ($filtered_auth) = \"\";\n+\tforeach (\"PLAIN\", \"LOGIN\", \"CRAM-MD5\", \"DIGEST-MD5\") {\n+\t\tif($smtp_auth && $smtp_auth =~ /\\b\\Q$_\\E\\b/i) {\n+\t\t\t$filtered_auth .= $_ . \" \";\n+\t\t}\n+\t}\n+\n+\tdie \"Invalid SMTP AUTH.\" if length $smtp_auth && !length $filtered_auth;\n+\n \t# Workaround AUTH PLAIN/LOGIN interaction defect\n \t# with Authen::SASL::Cyrus\n \teval {\n@@ -1148,6 +1163,20 @@ sub smtp_auth_maybe {\n \t\t'password' => $smtp_authpass\n \t}, sub {\n \t\tmy $cred = shift;\n+\n+\t\tif($filtered_auth) {\n+\t\t\tmy $sasl = Authen::SASL->new(\n+\t\t\t\tmechanism => $filtered_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":"267266","messageId":"CAPig+cT842GAFFM-wfjSU1ZiOevDCOPNDWxux6-vqtdr=3F4qw@mail.gmail.com","threadId":"39989","inReplyTo":"1438385617-29159-1-git-send-email-viktorin@rehivetech.com","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-01T09:33:28Z","receivedAt":"2015-08-01T09:33:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 31, 2015 at 7:33 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) denies valid\n\ns/denies/deny/\n\n> credentials for certain mechanisms. In this patch,\n> a new option --smtp-auth and configuration entry\n> smtpauth are introduced.\n>\n> If smtp_auth is defined, it works as a whitelist\n> of allowed mechanisms for authentication. There\n> are four mechanisms supported: PLAIN, LOGIN,\n> CRAM-MD5, DIGEST-MD5. However, their availability\n> depends on the installed SASL library.\n>\n> Signed-off-by: Jan Viktorin \n> ---\n> git-send-email.perl | 31 ++++++++++++++++++++++++++++++-\n> 1 file changed, 30 insertions(+), 1 deletion(-)\n\nAt the very least, you will also want to update the documentation\n(Documentation/git-send-email.txt) and, if possible, add new tests\n(t/t9001-send-email.sh).\n\nMore below.\n\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index ae9f869..b00ed9d 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {\n> return 1;\n> }\n>\n> + # Do not allow arbitrary strings.\n\nCan you explain why this restriction is needed. What are the\nconsequences of not limiting the input to this \"approved\" list?\n\n> + my ($filtered_auth) = \"\";\n\nStyle: unnecessary parentheses\n\n> + foreach (\"PLAIN\", \"LOGIN\", \"CRAM-MD5\", \"DIGEST-MD5\") {\n\nThis might read more nicely and be easier to maintain if written as:\n\n foreach (qw/PLAIN LOGIN CRAM-MD5 DIGEST-MD5/) {\n\n> + if($smtp_auth && $smtp_auth =~ /\\b\\Q$_\\E\\b/i) {\n\nStyle: space after 'if'\n\nAlso, why not lift the 'if ($smtp_auth)' check outside the loop since\nits value never changes and there's no need to iterate over the list\nif $smtp_auth is empty.\n\n> + $filtered_auth .= $_ . \" \";\n\nStyle question: Would this be more naturally expressed with\n'filtered_auth' as an array onto which items are pushed, rather than\nas a string? At the point of use, the string can be recreated via\njoin().\n\nNot a big deal; just wondering.\n\n> + }\n> + }\n> +\n> + die \"Invalid SMTP AUTH.\" if length $smtp_auth && !length $filtered_auth;\n\nStyle: drop capitalization: \"invalid...\"\nStyle: drop period at end\nStyle: add \"\\n\" at end in order to suppress printing of the\n perl line number and input line number which aren't\n very meaningful for a user error\n\n(Existing style in the script is not very consistent, but new code\nprobably should adhere the above suggestions.)\n\nAlso, don't you want to warn the user about tokens that don't match\none of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather than\ndropping them silently?\n\n> # Workaround AUTH PLAIN/LOGIN interaction defect\n> # with Authen::SASL::Cyrus\n> eval {\n> @@ -1148,6 +1163,20 @@ sub smtp_auth_maybe {\n> 'password' => $smtp_authpass\n> }, sub {\n> my $cred = shift;\n> +\n> + if($filtered_auth) {\n\nStyle: space after 'if'\n\n> + my $sasl = Authen::SASL->new(\n> + mechanism => $filtered_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":"267270","messageId":"20150801164959.GC488564@vauxhall.crustytoothpaste.net","threadId":"39989","inReplyTo":"1438385617-29159-1-git-send-email-viktorin@rehivetech.com","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-08-01T16:49:59Z","receivedAt":"2015-08-01T16:49:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Aug 01, 2015 at 01:33:37AM +0200, Jan Viktorin wrote:\n> +\t# Do not allow arbitrary strings.\n> +\tmy ($filtered_auth) = \"\";\n> +\tforeach (\"PLAIN\", \"LOGIN\", \"CRAM-MD5\", \"DIGEST-MD5\") {\n\nOn my system, GSSAPI is also available, and it does indeed work, as I'm\nnot prompted for a password. (I have only PLAIN and GSSAPI available\nserver-side, and AUTH is required.)\n\nIt may be better to simply force the text to upper case, as that would\nallow us not to have to change Git if Authen::SASL::Perl implements new\nmechanisms.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"267271","messageId":"20150801201950.5d8c1951@jvn","threadId":"39989","inReplyTo":"CAPig+cT842GAFFM-wfjSU1ZiOevDCOPNDWxux6-vqtdr=3F4qw@mail.gmail.com","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Jan Viktorin","fromEmail":"viktorin@rehivetech.com","sentAt":"2015-08-01T18:19:50Z","receivedAt":"2015-08-01T18:19:50Z","isPatch":true,"sender":{"key":"viktorin@rehivetech.com","avatar":null},"body":"Hello Eric,\n\nthanks for comments. I've described the orignal problem before I tried\nto fix it:\n\n https://groups.google.com/forum/#!topic/git-users/PxtiVxAapUU\n\nSo, *this patch* was necessary to apply for me to send *this patch* to\nthe mailing list.\n\nLater, I've tried git-send-email (without this patch) on two different\nPCs with the same distro, same architecture, same git, same perl, same\nperl libraries. The result was that on the first, it auto-selected\nDIGEST-MD5 (didn't work) and on the second one, it selected PLAIN\n(worked). I don't understand it.\n\nMore below...\n\nOn Sat, 1 Aug 2015 05:33:28 -0400\nEric Sunshine wrote:\n\n> On Fri, Jul 31, 2015 at 7:33 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) denies valid\n> \n> s/denies/deny/\n> \n> > credentials for certain mechanisms. In this patch,\n> > a new option --smtp-auth and configuration entry\n> > smtpauth are introduced.\n> >\n> > If smtp_auth is defined, it works as a whitelist\n> > of allowed mechanisms for authentication. There\n> > are four mechanisms supported: PLAIN, LOGIN,\n> > CRAM-MD5, DIGEST-MD5. However, their availability\n> > depends on the installed SASL library.\n> >\n> > Signed-off-by: Jan Viktorin \n> > ---\n> > git-send-email.perl | 31 ++++++++++++++++++++++++++++++-\n> > 1 file changed, 30 insertions(+), 1 deletion(-)\n> \n> At the very least, you will also want to update the documentation\n> (Documentation/git-send-email.txt) and, if possible, add new tests\n> (t/t9001-send-email.sh).\n\nI will update the documentation when it is clear, how the smtp-auth\nworks.\n\nI have no idea, how to test the feature. I can see something like\nfake.sendmail in the file. How does it work? I can image a test whether\nuser inserts valid values. What more?\n\n> \n> More below.\n> \n> > diff --git a/git-send-email.perl b/git-send-email.perl\n> > index ae9f869..b00ed9d 100755\n> > --- a/git-send-email.perl\n> > +++ b/git-send-email.perl\n> > @@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {\n> > return 1;\n> > }\n> >\n> > + # Do not allow arbitrary strings.\n> \n> Can you explain why this restriction is needed. What are the\n> consequences of not limiting the input to this \"approved\" list?\n\nThis is more a check of an arbitrary user input then a check\nof an \"approved list\". It should be also used to inform user\nabout invalid methods (however, I didn't implemented it yet).\n\n> \n> > + my ($filtered_auth) = \"\";\n> \n> Style: unnecessary parentheses\n> \n> > + foreach (\"PLAIN\", \"LOGIN\", \"CRAM-MD5\", \"DIGEST-MD5\") {\n> \n> This might read more nicely and be easier to maintain if written as:\n> \n> foreach (qw/PLAIN LOGIN CRAM-MD5 DIGEST-MD5/) {\n> \n> > + if($smtp_auth && $smtp_auth =~ /\\b\\Q$_\\E\\b/i) {\n> \n> Style: space after 'if'\n> \n> Also, why not lift the 'if ($smtp_auth)' check outside the loop since\n> its value never changes and there's no need to iterate over the list\n> if $smtp_auth is empty.\n\nSure. I just wanted to avoid another indentation level. I think, there\nis no need for optimization at this place. I can rework it, no\nproblem...\n\n> \n> > + $filtered_auth .= $_ . \" \";\n> \n> Style question: Would this be more naturally expressed with\n> 'filtered_auth' as an array onto which items are pushed, rather than\n> as a string? At the point of use, the string can be recreated via\n> join().\n> \n> Not a big deal; just wondering.\n\nI am not a Perl programmer. Yesterday, I've discovered for the first\ntime that Perl uses a dot for concatenation... I have no idea what\nhappens when passing an array to Authen::SASL->new(). Moreover, the\nPerl arrays syntax rules scare me a bit ;).\n\n> \n> > + }\n> > + }\n> > +\n> > + die \"Invalid SMTP AUTH.\" if length $smtp_auth && !length\n> > $filtered_auth;\n> \n> Style: drop capitalization: \"invalid...\"\n> Style: drop period at end\n\nAgree.\n\n> Style: add \"\\n\" at end in order to suppress printing of the\n> perl line number and input line number which aren't\n> very meaningful for a user error\n\nAnother hidden Perl suprise, I guess...\n\n> \n> (Existing style in the script is not very consistent, but new code\n> probably should adhere the above suggestions.)\n\n(Agree.)\n\n> \n> Also, don't you want to warn the user about tokens that don't match\n> one of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather than\n> dropping them silently?\n\nYes, this would be great (as I've already mentioned). It's a question\nwhether to include the check for the mechanisms or whether to leave\nthe $smtp_auth variable as it is... Maybe just validate by a regex?\n\nThe naming rules are defiend here:\n\n https://tools.ietf.org/html/rfc4422#page-8\n\nSo, this looks to me as a better way.\n\nNote that, the current implementation does not force the user to use\nonly the listed mechanisms. If the $smtp_auth is empty, the original\nbehaviour is preserved...\n\n> \n> > # Workaround AUTH PLAIN/LOGIN interaction defect\n> > # with Authen::SASL::Cyrus\n> > eval {\n> > @@ -1148,6 +1163,20 @@ sub smtp_auth_maybe {\n> > 'password' => $smtp_authpass\n> > }, sub {\n> > my $cred = shift;\n> > +\n> > + if($filtered_auth) {\n> \n> Style: space after 'if'\n> \n> > + my $sasl = \n> > + mechanism => $filtered_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":"267272","messageId":"20150801202156.57debcc3@jvn","threadId":"39989","inReplyTo":"20150801164959.GC488564@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Jan Viktorin","fromEmail":"viktorin@rehivetech.com","sentAt":"2015-08-01T18:21:56Z","receivedAt":"2015-08-01T18:21:56Z","isPatch":true,"sender":{"key":"viktorin@rehivetech.com","avatar":null},"body":"Hello Brian,\n\nthanks for your note. I think, I will remove the check\nof list of mechanisms and put there a regex check.\n\nOn Sat, 1 Aug 2015 16:49:59 +0000\n\"brian m. carlson\" wrote:\n\n> On Sat, Aug 01, 2015 at 01:33:37AM +0200, Jan Viktorin wrote:\n> > +\t# Do not allow arbitrary strings.\n> > +\tmy ($filtered_auth) = \"\";\n> > +\tforeach (\"PLAIN\", \"LOGIN\", \"CRAM-MD5\", \"DIGEST-MD5\") {\n> \n> On my system, GSSAPI is also available, and it does indeed work, as\n> I'm not prompted for a password. (I have only PLAIN and GSSAPI\n> available server-side, and AUTH is required.)\n> \n> It may be better to simply force the text to upper case, as that would\n> allow us not to have to change Git if Authen::SASL::Perl implements\n> new mechanisms.\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":"267278","messageId":"CAPig+cQwgYYYYsszaRdJDwFLLB0PmiDQ_WTa+Nzzoq0U1zuMiA@mail.gmail.com","threadId":"39989","inReplyTo":"20150801201950.5d8c1951@jvn","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-02T09:41:29Z","receivedAt":"2015-08-02T09:41:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 1, 2015 at 2:19 PM, Jan Viktorin wrote:\n> On Sat, 1 Aug 2015 05:33:28 -0400 Eric Sunshine wrote:\n>> On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin\n>> wrote:\n>> At the very least, you will also want to update the documentation\n>> (Documentation/git-send-email.txt) and, if possible, add new tests\n>> (t/t9001-send-email.sh).\n>\n> I will update the documentation when it is clear, how the smtp-auth\n> works.\n>\n> I have no idea, how to test the feature. I can see something like\n> fake.sendmail in the file. How does it work? I can image a test whether\n> user inserts valid values. What more?\n\nThat's what I was thinking. You could test if the die() is triggered\nor if it emits warnings for bad values (assuming you implement that\nfeature). As for testing the actual authentication, I'm not sure you\ncan (and don't see any such testing in the script).\n\n>> > diff --git a/git-send-email.perl b/git-send-email.perl\n>> > index ae9f869..b00ed9d 100755\n>> > --- a/git-send-email.perl\n>> > +++ b/git-send-email.perl\n>> > @@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {\n>> > return 1;\n>> > }\n>> >\n>> > + # Do not allow arbitrary strings.\n>>\n>> Can you explain why this restriction is needed. What are the\n>> consequences of not limiting the input to this \"approved\" list?\n>\n> This is more a check of an arbitrary user input then a check\n> of an \"approved list\". It should be also used to inform user\n> about invalid methods (however, I didn't implemented it yet).\n\nWhat I was really asking was whether this sort of checking really\nbelongs in git-send-email or if it is better left to Net::SMTP (and\nAuthen::SASL) to do so since they are in better positions to know what\nis valid and what is not. If the Perl module(s) generate suitable\ndiagnostics for bad input, then it makes sense to leave the checking\nto them. If not, then I can understand your motivation for\ngit-send-email doing the checking instead in order to emit\nuser-friendly diagnostics.\n\nSo, that's what I meant when I asked 'What are the consequences of not\nlimiting the input to this \"approved\" list?'.\n\nThe other reason I asked was that it increases maintenance costs for\nus to maintain a list of \"approved\" mechanisms, since the list needs\nto be updated when new ones are implemented (and, as brian pointed\nout, some may already exist which are not in your list).\n\n>> > + $filtered_auth .= $_ . \" \";\n>>\n>> Style question: Would this be more naturally expressed with\n>> 'filtered_auth' as an array onto which items are pushed, rather than\n>> as a string? At the point of use, the string can be recreated via\n>> join().\n>>\n>> Not a big deal; just wondering.\n>\n> I am not a Perl programmer. Yesterday, I've discovered for the first\n> time that Perl uses a dot for concatenation... I have no idea what\n> happens when passing an array to Authen::SASL->new(). Moreover, the\n> Perl arrays syntax rules scare me a bit ;).\n\nYou wouldn't pass the array to Authen::SASL, instead you would use\njoin() to transform the array back into a space-separated string. It's\nprobably moot (since you probably shouldn't be doing the filtering\nmanually), but the code would look something like this:\n\n my @filtered_auth;\n ...\n foreach (...) {\n if (...) {\n push @filtered_auth, $_;\n }\n }\n ...\n if (@filtered_auth) {\n my $sasl = Authen::SASL->new(\n mechanism => join(' ', @filtered_auth),\n ...\n\n>> Style: add \"\\n\" at end in order to suppress printing of the\n>> perl line number and input line number which aren't\n>> very meaningful for a user error\n>\n> Another hidden Perl suprise, I guess...\n\nYes.\n\n>> Also, don't you want to warn the user about tokens that don't match\n>> one of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather than\n>> dropping them silently?\n>\n> Yes, this would be great (as I've already mentioned). It's a question\n> whether to include the check for the mechanisms or whether to leave\n> the $smtp_auth variable as it is... Maybe just validate by a regex?\n>\n> The naming rules are defiend here:\n> https://tools.ietf.org/html/rfc4422#page-8\n> So, this looks to me as a better way.\n\nMaybe. This leads back to my original question of whether it's really\ngit-send-email's responsibility to do validation or if that can be\nleft to Net::SMTP/Authen::SASL. If the Perl module(s) emit suitable\ndiagnostics for bad input, then validation can be omitted from\ngit-send-email.\n"},{"id":"267290","messageId":"20150802184353.2a5da936@jvn","threadId":"39989","inReplyTo":"CAPig+cQwgYYYYsszaRdJDwFLLB0PmiDQ_WTa+Nzzoq0U1zuMiA@mail.gmail.com","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Jan Viktorin","fromEmail":"viktorin@rehivetech.com","sentAt":"2015-08-02T16:43:53Z","receivedAt":"2015-08-02T16:43:53Z","isPatch":true,"sender":{"key":"viktorin@rehivetech.com","avatar":null},"body":"Authen::SASL gives:\n\nNo SASL mechanism found\n at /usr/share/perl5/vendor_perl/Authen/SASL.pm line 77.\n at /usr/share/perl5/core_perl/Net/SMTP.pm line 207.\n\nThe SASL library does not check validity of mechanisms'\nnames (or I did not find it). It just tries to load one\nthat matches both the ours and the server side ones.\n\nI can see one possible weakness of this, however I doubt\nwhether there exists a successful attack vector. Imagine\nthat somebody gives me a malicious .gitconfig with\nsmtpauth = ~/ATTACK and redirects me to a fake mail\nserver that advertises ~/ATTACK as a working mechanism.\nThis might lead to an unwanted execution of ~/ATTACK.pm.\nShould we consider this to be a threat?\n\nAnother thing that confuses me (I mentioned it in the\nprevious e-mail). I forced to use CRAM-MD5, however, it\ndies with the above errors. The CRAM-MD5 is installed:\n\n/usr/share/perl5/vendor_perl/Authen/SASL/CRAM_MD5.pm\n/usr/share/perl5/vendor_perl/Authen/SASL/Perl/CRAM_MD5.pm\n\nThe same for DIGEST-MD5. On different PC with the same\nset of libraries, OS, the CRAM-MD5 just works. Why? LOGIN\nand PLAIN are OK. Environment? (I doubt.)\n\nI would like to include the regex check based on RFC 4422\nas I've already mentioned. at least, it filters out the\nunwanted characters like '/', '.', etc.\n\nRegards\nJan\n\nOn Sun, 2 Aug 2015 05:41:29 -0400\nEric Sunshine wrote:\n\n> On Sat, Aug 1, 2015 at 2:19 PM, Jan Viktorin\n> wrote:\n> > On Sat, 1 Aug 2015 05:33:28 -0400 Eric Sunshine\n> > wrote:\n> >> On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin\n> >> wrote:\n> >> At the very least, you will also want to update the documentation\n> >> (Documentation/git-send-email.txt) and, if possible, add new tests\n> >> (t/t9001-send-email.sh).\n> >\n> > I will update the documentation when it is clear, how the smtp-auth\n> > works.\n> >\n> > I have no idea, how to test the feature. I can see something like\n> > fake.sendmail in the file. How does it work? I can image a test\n> > whether user inserts valid values. What more?\n> \n> That's what I was thinking. You could test if the die() is triggered\n> or if it emits warnings for bad values (assuming you implement that\n> feature). As for testing the actual authentication, I'm not sure you\n> can (and don't see any such testing in the script).\n> \n> >> > diff --git a/git-send-email.perl b/git-send-email.perl\n> >> > index ae9f869..b00ed9d 100755\n> >> > --- a/git-send-email.perl\n> >> > +++ b/git-send-email.perl\n> >> > @@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {\n> >> > return 1;\n> >> > }\n> >> >\n> >> > + # Do not allow arbitrary strings.\n> >>\n> >> Can you explain why this restriction is needed. What are the\n> >> consequences of not limiting the input to this \"approved\" list?\n> >\n> > This is more a check of an arbitrary user input then a check\n> > of an \"approved list\". It should be also used to inform user\n> > about invalid methods (however, I didn't implemented it yet).\n> \n> What I was really asking was whether this sort of checking really\n> belongs in git-send-email or if it is better left to Net::SMTP (and\n> Authen::SASL) to do so since they are in better positions to know what\n> is valid and what is not. If the Perl module(s) generate suitable\n> diagnostics for bad input, then it makes sense to leave the checking\n> to them. If not, then I can understand your motivation for\n> git-send-email doing the checking instead in order to emit\n> user-friendly diagnostics.\n> \n> So, that's what I meant when I asked 'What are the consequences of not\n> limiting the input to this \"approved\" list?'.\n> \n> The other reason I asked was that it increases maintenance costs for\n> us to maintain a list of \"approved\" mechanisms, since the list needs\n> to be updated when new ones are implemented (and, as brian pointed\n> out, some may already exist which are not in your list).\n>\n> (...)\n>\n> >> Also, don't you want to warn the user about tokens that don't match\n> >> one of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather\n> >> than dropping them silently?\n> >\n> > Yes, this would be great (as I've already mentioned). It's a\n> > question whether to include the check for the mechanisms or whether\n> > to leave the $smtp_auth variable as it is... Maybe just validate by\n> > a regex?\n> >\n> > The naming rules are defiend here:\n> > https://tools.ietf.org/html/rfc4422#page-8\n> > So, this looks to me as a better way.\n> \n> Maybe. This leads back to my original question of whether it's really\n> git-send-email's responsibility to do validation or if that can be\n> left to Net::SMTP/Authen::SASL. If the Perl module(s) emit suitable\n> diagnostics for bad input, then validation can be omitted from\n> git-send-email.\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":"267293","messageId":"xmqq614xa7de.fsf@gitster.dls.corp.google.com","threadId":"39989","inReplyTo":"CAPig+cQwgYYYYsszaRdJDwFLLB0PmiDQ_WTa+Nzzoq0U1zuMiA@mail.gmail.com","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-02T18:10:53Z","receivedAt":"2015-08-02T18:10:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> What I was really asking was whether this sort of checking really\n> belongs in git-send-email or if it is better left to Net::SMTP (and\n> Authen::SASL) to do so since they are in better positions to know what\n> is valid and what is not. If the Perl module(s) generate suitable\n> diagnostics for bad input, then it makes sense to leave the checking\n> to them. If not, then I can understand your motivation for\n> git-send-email doing the checking instead in order to emit\n> user-friendly diagnostics.\n>\n> So, that's what I meant when I asked 'What are the consequences of not\n> limiting the input to this \"approved\" list?'.\n>\n> The other reason I asked was that it increases maintenance costs for\n> us to maintain a list of \"approved\" mechanisms, since the list needs\n> to be updated when new ones are implemented (and, as brian pointed\n> out, some may already exist which are not in your list).\n\nBoth are very good points. I do not think we should be limiting the\nuser input; instead, we (1) either let the Perl module emit proper\ndiagnosis (e.g. it may say \"There is no valid auth method in the\nlist you gave me, which was 'PLIAN LOGIN CROM-MD5'\") and do nothing,\nor (2) catch the error from the Perl module and then guess what\nhappened after the fact (e.g. the module may say in its die() message\nsomething that is understandable by the program but not by the user,\nand \"eval { ... module call ... }; if ($@) { ... HERE ... }\" would\nmassage what it can learn in HERE from $@ into what the end user\nwould understand. It may be that it is sufficient to have something\nas simple as this:\n\n\tmy $msg = \"$@\"\n\tif ($smtp_auth is used to customize) {\n\t\t$msg .= \"\\nYour customized <$smtp_auth> might be misspelt?\"\n\t}\n\tdie $msg;\n\nin \"HERE\" above.\n\n> ...\n> Maybe. This leads back to my original question of whether it's really\n> git-send-email's responsibility to do validation or if that can be\n> left to Net::SMTP/Authen::SASL. If the Perl module(s) emit suitable\n> diagnostics for bad input, then validation can be omitted from\n> git-send-email.\n\nYes, exactly. What happens if we go the route of doing nothing\nhere? What are the horrible diagnostic message the end user would\nnot understand?\n"},{"id":"267295","messageId":"xmqqwpxd8rz2.fsf@gitster.dls.corp.google.com","threadId":"39989","inReplyTo":"20150802184353.2a5da936@jvn","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-02T18:28:49Z","receivedAt":"2015-08-02T18:28:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Viktorin writes:\n\n> Authen::SASL gives:\n>\n> No SASL mechanism found\n> at /usr/share/perl5/vendor_perl/Authen/SASL.pm line 77.\n> at /usr/share/perl5/core_perl/Net/SMTP.pm line 207.\n>\n> The SASL library does not check validity of mechanisms'\n> names (or I did not find it). It just tries to load one\n> that matches both the ours and the server side ones.\n> ...\n> I would like to include the regex check based on RFC 4422\n> as I've already mentioned. at least, it filters out the\n> unwanted characters like '/', '.', etc.\n\nHmm, is there a way to ask Authen::SASL what SASL mechanism the\ninstalled system supports? If so, the enhancement you are adding\ncould be\n\n\tmy @to_use;\n\tif ($smtp_auth_whitelist is supplied) {\n\t\tmy @installed = Authen::SASL::list_mechanisms();\n for (@installed) {\n \tif ($_ is whitelisted) {\n\t\t\t\tpush @to_use, $_;\n\t\t\t}\n\t\t}\n\t}\n\nand @to_use can later be supplied when we open the connection as the\nlist of mechanisms we allow the library to pick.\n\nJust my $.02\n"},{"id":"267306","messageId":"20150803122432.21066d73@pcviktorin.fit.vutbr.cz","threadId":"39989","inReplyTo":"xmqqwpxd8rz2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Jan Viktorin","fromEmail":"viktorin@rehivetech.com","sentAt":"2015-08-03T10:24:32Z","receivedAt":"2015-08-03T10:24:32Z","isPatch":true,"sender":{"key":"viktorin@rehivetech.com","avatar":null},"body":"On Sun, 02 Aug 2015 11:28:49 -0700\nJunio C Hamano wrote:\n\n> Jan Viktorin writes:\n> \n> > Authen::SASL gives:\n> >\n> > No SASL mechanism found\n> > at /usr/share/perl5/vendor_perl/Authen/SASL.pm line 77.\n> > at /usr/share/perl5/core_perl/Net/SMTP.pm line 207.\n> >\n> > The SASL library does not check validity of mechanisms'\n> > names (or I did not find it). It just tries to load one\n> > that matches both the ours and the server side ones.\n> > ...\n> > I would like to include the regex check based on RFC 4422\n> > as I've already mentioned. at least, it filters out the\n> > unwanted characters like '/', '.', etc.\n> \n> Hmm, is there a way to ask Authen::SASL what SASL mechanism the\n> installed system supports? If so, the enhancement you are adding\n> could be\n> \n> \tmy @to_use;\n> \tif ($smtp_auth_whitelist is supplied) {\n> \t\tmy @installed = Authen::SASL::list_mechanisms();\n> for (@installed) {\n> \tif ($_ is whitelisted) {\n> \t\t\t\tpush @to_use, $_;\n> \t\t\t}\n> \t\t}\n> \t}\n> \n> and @to_use can later be supplied when we open the connection as the\n> list of mechanisms we allow the library to pick.\n> \n> Just my $.02\n\nI didn't find a way how to determine what mechanisms are supported by SASL.\nThis is a way how it looks for a mechanism (I think) on new():\n\nAuthen/SASL/Perl.pm\n\n 57 my @mpkg = sort {\n 58 $b->_order <=> $a->_order\n 59 } grep {\n 60 my $have = $have{$_} ||= (eval \"require $_;\" and $_->can('_secflags')) ? 1 : -1;\n 61 $have > 0 and $_->_secflags(@sec) == @sec\n 62 } map {\n 63 (my $mpkg = __PACKAGE__ . \"::$_\") =~ s/-/_/g;\n 64 $mpkg;\n 65 } split /[^-\\w]+/, $parent->mechanism\n 66 or croak \"No SASL mechanism found\\n\";\n\nIt just loads a package based on the names we provide. So it seems, the library\nhas no clue about the existing mechanisms. This would be possible by reading the\nproper directory with packages which seems to be quite wierd anyway.\n\n-- \n Jan Viktorin E-mail: Viktorin@RehiveTech.com\n System Architect Web: www.RehiveTech.com\n RehiveTech\n Brno, Czech Republic\n"},{"id":"267337","messageId":"xmqqk2tc87yn.fsf@gitster.dls.corp.google.com","threadId":"39989","inReplyTo":"20150803122432.21066d73@pcviktorin.fit.vutbr.cz","subject":"Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-03T19:53:20Z","receivedAt":"2015-08-03T19:53:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Viktorin writes:\n\n> I didn't find a way how to determine what mechanisms are supported by SASL.\n\nOk, forget the suggested approach, then X-<.\n"}]}