{"thread":{"id":"29639","subject":"[PATCH] git-send-email: allow overriding smtp-encryption config to 'none'","startedAt":"2012-02-15T21:42:58Z","lastAt":"2012-02-18T06:24:19Z","messageCount":6,"participants":["Brian Norris","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"184792","messageId":"1329342178-14540-1-git-send-email-computersforpeace@gmail.com","threadId":"29639","inReplyTo":null,"subject":"[PATCH] git-send-email: allow overriding smtp-encryption config to 'none'","fromName":"Brian Norris","fromEmail":"computersforpeace@gmail.com","sentAt":"2012-02-15T21:42:58Z","receivedAt":"2012-02-15T21:42:58Z","isPatch":true,"sender":{"key":"computersforpeace@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2268082?v=4"},"body":"According to the manpage:\n\n --smtp-encryption=<encryption>\n        Specify the encryption to use, either ssl or tls. Any other value\n\treverts to plain SMTP.  Default is the value of\n        sendemail.smtpencryption.\n\nHowever, if I have already set sendemail.smtpencryption but try to override\nit with something like 'no', the authentication code block still tries to ask\nfor a password (and fails).\n\nThis patch forces $smtp_encryption to 'none' when a proper encryption type is\nnot provided, then checks $smtp_encryption before proceeding to authentication.\n\nExample execution:\n\n $ git send-email --smtp-encryption=no --smtp-domain=<xxx> \\\n        --smtp-server=<xxx> --smtp-server-port=25 myfile.patch\n Password:\n Command unknown: 'AUTH' at /usr/local/libexec/git-core/git-send-email\n line 1115, <STDIN> line 1.\n\nTested on Git 1.7.5.4 and 1.7.9.1.\n\nSigned-off-by: Brian Norris <computersforpeace@gmail.com>\n---\n git-send-email.perl |    7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex ef30c55..fa0a384 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -385,8 +385,9 @@ foreach my $setting (values %config_bool_settings) {\n \t${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));\n }\n \n-# 'default' encryption is none -- this only prevents a warning\n-$smtp_encryption = '' unless (defined $smtp_encryption);\n+# 'default' encryption is none\n+$smtp_encryption = 'none' unless (defined $smtp_encryption);\n+$smtp_encryption = 'none' unless ($smtp_encryption eq 'tls' || $smtp_encryption eq 'ssl');\n \n # Set CC suppressions\n my(%suppress_cc);\n@@ -1113,7 +1114,7 @@ X-Mailer: git-send-email $gitversion\n \t\t\t    defined $smtp_server_port ? \" port=$smtp_server_port\" : \"\";\n \t\t}\n \n-\t\tif (defined $smtp_authuser) {\n+\t\tif (defined $smtp_authuser && $smtp_encryption ne 'none') {\n \t\t\t# Workaround AUTH PLAIN/LOGIN interaction defect\n \t\t\t# with Authen::SASL::Cyrus\n \t\t\teval {\n-- \n1.7.5.4\n"},{"id":"184793","messageId":"20120215220629.GA17672@sigill.intra.peff.net","threadId":"29639","inReplyTo":"1329342178-14540-1-git-send-email-computersforpeace@gmail.com","subject":"Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-15T22:06:29Z","receivedAt":"2012-02-15T22:06:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 15, 2012 at 01:42:58PM -0800, Brian Norris wrote:\n\n> According to the manpage:\n> \n>  --smtp-encryption=<encryption>\n>         Specify the encryption to use, either ssl or tls. Any other value\n> \treverts to plain SMTP.  Default is the value of\n>         sendemail.smtpencryption.\n> \n> However, if I have already set sendemail.smtpencryption but try to override\n> it with something like 'no', the authentication code block still tries to ask\n> for a password (and fails).\n\nSounds reasonable.\n\n> This patch forces $smtp_encryption to 'none' when a proper encryption type is\n> not provided, then checks $smtp_encryption before proceeding to authentication.\n\nDefaulting everything except \"ssl\" or \"tls\" to \"none\" seems risky to me.\nIf I am understanding your patch correctly, then doing this:\n\n  git send-email --smtp-encryption=SSL\n\nwill silently treat that as \"don't do encryption\", which could have\nsurprising security implications for the user. I chose all-caps as it is\nan obvious mistake to make. We probably should treat it the same as\nlowercase \"ssl\", but the same argument applies to other typos like\n\"tsl\".\n\nIt seems like a much safer default would be to die() on an invalid\nencryption specifier.\n\n-Peff\n"},{"id":"184797","messageId":"CAN8TOE_BnkOcMQRTY-GWrHozYD0+0giWn2LtjB8AVnP_DzA+Sg@mail.gmail.com","threadId":"29639","inReplyTo":"20120215220629.GA17672@sigill.intra.peff.net","subject":"Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'","fromName":"Brian Norris","fromEmail":"computersforpeace@gmail.com","sentAt":"2012-02-15T23:49:59Z","receivedAt":"2012-02-15T23:49:59Z","isPatch":true,"sender":{"key":"computersforpeace@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2268082?v=4"},"body":"On Wed, Feb 15, 2012 at 2:06 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Feb 15, 2012 at 01:42:58PM -0800, Brian Norris wrote:\n>\n>> According to the manpage:\n>>\n>>  --smtp-encryption=<encryption>\n>>         Specify the encryption to use, either ssl or tls. Any other value\n>>       reverts to plain SMTP.  Default is the value of\n>>         sendemail.smtpencryption.\n>>\n>> However, if I have already set sendemail.smtpencryption but try to override\n>> it with something like 'no', the authentication code block still tries to ask\n>> for a password (and fails).\n>\n> Sounds reasonable.\n\nAn error like\n  Command unknown: 'AUTH' at /usr/local/libexec/git-core/git-send-email\nis reasonable?\n\n>> This patch forces $smtp_encryption to 'none' when a proper encryption type is\n>> not provided, then checks $smtp_encryption before proceeding to authentication.\n>\n> Defaulting everything except \"ssl\" or \"tls\" to \"none\" seems risky to me.\n> If I am understanding your patch correctly, then doing this:\n>\n>  git send-email --smtp-encryption=SSL\n>\n> will silently treat that as \"don't do encryption\", which could have\n> surprising security implications for the user. I chose all-caps as it is\n> an obvious mistake to make. We probably should treat it the same as\n> lowercase \"ssl\", but the same argument applies to other typos like\n> \"tsl\".\n\nWell, git-send-email already doesn't handle typos or capitalization\ncorrectly, AFAICT. So nothing new here.\n\n> It seems like a much safer default would be to die() on an invalid\n> encryption specifier.\n\nFine. But then we need to define a behavior that means 'no\nencryption.' Like 'none' instead of just saying 'anything but tls or\nssl.'\n\nNow that I look at this again, I think part of the issue I have is\nthat there is no way to override *smtp-user* via command-line, in\norder to do unencrypted, unauthenticated email. So the\n*authentication* not the encryption is really my main problem...I'll\ntake another look and try a new patch.\n\nThanks,\nBrian\n"},{"id":"184800","messageId":"20120216004903.GA21170@sigill.intra.peff.net","threadId":"29639","inReplyTo":"CAN8TOE_BnkOcMQRTY-GWrHozYD0+0giWn2LtjB8AVnP_DzA+Sg@mail.gmail.com","subject":"Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-16T00:49:03Z","receivedAt":"2012-02-16T00:49:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 15, 2012 at 03:49:59PM -0800, Brian Norris wrote:\n\n> > Sounds reasonable.\n> \n> An error like\n>   Command unknown: 'AUTH' at /usr/local/libexec/git-core/git-send-email\n> is reasonable?\n\nSorry, no, I meant your goal of allowing overriding config sounds like a\nreasonable thing to want. But from reading your message below, it seems\nthat is not actually the problem you are trying to solve.\n\n> > Defaulting everything except \"ssl\" or \"tls\" to \"none\" seems risky to me.\n> > If I am understanding your patch correctly, then doing this:\n> >\n> >  git send-email --smtp-encryption=SSL\n> >\n> > will silently treat that as \"don't do encryption\", which could have\n> > surprising security implications for the user. I chose all-caps as it is\n> > an obvious mistake to make. We probably should treat it the same as\n> > lowercase \"ssl\", but the same argument applies to other typos like\n> > \"tsl\".\n> \n> Well, git-send-email already doesn't handle typos or capitalization\n> correctly, AFAICT. So nothing new here.\n\nHmm. From your description and the patch, I thought that was something\nintroduced by your patch. But looking at the existing code, it seems\nlike that is already the case. IOW, I don't understand why\n\"--smtp-encryption=none\" does not already work looking at the current\ncode.\n\nSo being more careful about typos is an improvement we could make, but\nit is not a feature that would need to be part of a bugfix patch.\n\n> > It seems like a much safer default would be to die() on an invalid\n> > encryption specifier.\n> \n> Fine. But then we need to define a behavior that means 'no\n> encryption.' Like 'none' instead of just saying 'anything but tls or\n> ssl.'\n\nRight. I meant that you should introduce \"none\" as an explicit \"no, I\ndon't want this\" and die when the flag is not one of {ssl, tls, none}.\n\n> Now that I look at this again, I think part of the issue I have is\n> that there is no way to override *smtp-user* via command-line, in\n> order to do unencrypted, unauthenticated email. So the\n> *authentication* not the encryption is really my main problem...I'll\n> take another look and try a new patch.\n\nAh, I see. I misunderstood the original problem you were trying to solve\n(I thought your example was \"see? Encryption is off, so the server won't\ndo AUTH, demonstrating that the patch works.\").\n\nOverriding the smtp user from the config is a separate issue, and I\ndon't think that is currently possible. The usual way to spell an option\nlike that in git is \"--no-smtp-user\", but it seems that we use perl's\nGetOptions, which does not understand that syntax. So you'd have to add\na \"--no-smtp-user\" by hand.\n\n-Peff\n"},{"id":"184919","messageId":"CAN8TOE-vek=ooq4DRcNF0iCg+rJMt6SUhMi4+_dOWaRJ44KLLA@mail.gmail.com","threadId":"29639","inReplyTo":"20120216004903.GA21170@sigill.intra.peff.net","subject":"Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'","fromName":"Brian Norris","fromEmail":"computersforpeace@gmail.com","sentAt":"2012-02-18T05:27:44Z","receivedAt":"2012-02-18T05:27:44Z","isPatch":true,"sender":{"key":"computersforpeace@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2268082?v=4"},"body":"On Wed, Feb 15, 2012 at 4:49 PM, Jeff King <peff@peff.net> wrote:\n> Ah, I see. I misunderstood the original problem you were trying to solve\n> (I thought your example was \"see? Encryption is off, so the server won't\n> do AUTH, demonstrating that the patch works.\").\n\nYeah, I got a little bit off track on what my actual goal was...\n\n> Overriding the smtp user from the config is a separate issue, and I\n> don't think that is currently possible. The usual way to spell an option\n> like that in git is \"--no-smtp-user\", but it seems that we use perl's\n> GetOptions, which does not understand that syntax. So you'd have to add\n> a \"--no-smtp-user\" by hand.\n\nI think the \"--no-smtp-user\" is what I really wanted. I've written a\ndifferent patch that actually targets the user name properly, but I've\nalso found a current solution that can work for scripting purposes:\njust redirect the $GIT_CONFIG environment variable to /dev/null\ntemporarily. Perhaps I'll send my new patch sometime, but it's not\npressing and I'm not sure what kind of use it would actually get.\n\nThanks for the pointers.\n\nBrian\n"},{"id":"184920","messageId":"20120218062418.GA3818@sigill.intra.peff.net","threadId":"29639","inReplyTo":"CAN8TOE-vek=ooq4DRcNF0iCg+rJMt6SUhMi4+_dOWaRJ44KLLA@mail.gmail.com","subject":"Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-18T06:24:19Z","receivedAt":"2012-02-18T06:24:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 17, 2012 at 09:27:44PM -0800, Brian Norris wrote:\n\n> > Overriding the smtp user from the config is a separate issue, and I\n> > don't think that is currently possible. The usual way to spell an option\n> > like that in git is \"--no-smtp-user\", but it seems that we use perl's\n> > GetOptions, which does not understand that syntax. So you'd have to add\n> > a \"--no-smtp-user\" by hand.\n> \n> I think the \"--no-smtp-user\" is what I really wanted. I've written a\n> different patch that actually targets the user name properly, but I've\n> also found a current solution that can work for scripting purposes:\n> just redirect the $GIT_CONFIG environment variable to /dev/null\n> temporarily.\n\nJust FYI, the fact that doing so works is somewhat accidental. Long ago,\nGIT_CONFIG was respected everywhere as an override to stop reading any\nother config. Later, it was dropped, but retained its meaning only for\nthe git-config command, mostly for historical reasons (although these\ndays one would do better to use \"git config -f $file\" instead).\n\nSo the reason it works for git-send-email is that send-email in turn\ncalls git-config to actually look at config values, because send-email\nis a perl script and not a C program. In other words, the fact that\nGIT_CONFIG is respected is a coincidence of some implementation\ndecisions, not an intended behavior.\n\nI don't think we have any plans for those implementation details to\nchange in the near future.  So by all means, use it if you like for the\ntime being. But know that it's not a behavior which is guaranteed not to\nchange in future versions.\n\n> Perhaps I'll send my new patch sometime, but it's not pressing and I'm\n> not sure what kind of use it would actually get.\n\nI think the ideal case would be a patch that teaches the send-email\noption parsing code to understand a \"--no-*\" counterpart for every\noption, without having to modify each option individually. I haven't\nlooked at how easy or hard that would be, though.\n\n-Peff\n"}]}