{"thread":{"id":"55476","subject":"[PATCH v2 0/3] git-send-email: improve SSL configuration","startedAt":"2021-04-11T12:54:46Z","lastAt":"2021-05-01T09:17:27Z","messageCount":24,"participants":["Drew DeVault","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"421590","messageId":"20210411125431.28971-1-sir@cmpwn.com","threadId":"55476","inReplyTo":null,"subject":"[PATCH v2 0/3] git-send-email: improve SSL configuration","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2021-04-11T12:54:28Z","receivedAt":"2021-04-11T12:54:46Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"Following feedback on v1, this splits the changes up into 3 commits,\nincluding a new change which makes any value other than 'ssl',\n'starttls', or nothing into an error. This also removes 'ssl/tls' in\nfavor of 'ssl' to avoid confusion, and avoids taking a stance on the\nmatter of the deprecation of either approach.\n\nDrew DeVault (3):\n  git-send-email(1): improve smtp-encryption docs\n  git-send-email: die on invalid smtp_encryption\n  git-send-email: rename 'tls' to 'starttls'\n\n Documentation/git-send-email.txt | 9 +++++++--\n git-send-email.perl              | 9 ++++++++-\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\n-- \n2.31.1\n\n"},{"id":"421591","messageId":"20210411125431.28971-2-sir@cmpwn.com","threadId":"55476","inReplyTo":"20210411125431.28971-1-sir@cmpwn.com","subject":"[PATCH v2 1/3] git-send-email(1): improve smtp-encryption docs","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2021-04-11T12:54:29Z","receivedAt":"2021-04-11T12:54:46Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"This clarifies the meaning of the 'ssl' and 'tls' values for this\noption, which respectively enable SSL/TLS, i.e. a standard \"modern\" SSL\napproach; and STARTTLS, i.e. opportunistic in-band TLS.\n\nSigned-off-by: Drew DeVault <sir@cmpwn.com>\n---\n Documentation/git-send-email.txt | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 93708aefea..c17c3b400a 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -168,8 +168,11 @@ Sending\n \tunspecified, choosing the envelope sender is left to your MTA.\n \n --smtp-encryption=<encryption>::\n-\tSpecify the encryption to use, either 'ssl' or 'tls'.  Any other\n-\tvalue reverts to plain SMTP.  Default is the value of\n+\tSpecify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables\n+\tgeneric SSL/TLS support and is typically used on port 465.  'tls'\n+\tenables in-band STARTTLS support and is typically used on port 25 or\n+\t587.  Use whichever option is recommended by your mail provider.  Any\n+\tother value reverts to plain SMTP.  Default is the value of\n \t`sendemail.smtpEncryption`.\n \n --smtp-domain=<FQDN>::\n-- \n2.31.1\n\n"},{"id":"421593","messageId":"20210411125431.28971-3-sir@cmpwn.com","threadId":"55476","inReplyTo":"20210411125431.28971-1-sir@cmpwn.com","subject":"[PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2021-04-11T12:54:30Z","receivedAt":"2021-04-11T12:54:47Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"Signed-off-by: Drew DeVault <sir@cmpwn.com>\n---\n Documentation/git-send-email.txt | 4 ++--\n git-send-email.perl              | 3 +++\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex c17c3b400a..520b355e50 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -171,8 +171,8 @@ Sending\n \tSpecify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables\n \tgeneric SSL/TLS support and is typically used on port 465.  'tls'\n \tenables in-band STARTTLS support and is typically used on port 25 or\n-\t587.  Use whichever option is recommended by your mail provider.  Any\n-\tother value reverts to plain SMTP.  Default is the value of\n+\t587.  Use whichever option is recommended by your mail provider.  Leave\n+\tempty to disable encryption and use plain SMTP.  Default is the value of\n \t`sendemail.smtpEncryption`.\n \n --smtp-domain=<FQDN>::\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex f5bbf1647e..bda5211f0d 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -495,6 +495,9 @@ sub read_config {\n \n # 'default' encryption is none -- this only prevents a warning\n $smtp_encryption = '' unless (defined $smtp_encryption);\n+if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"tls\") {\n+\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\\n\");\n+}\n \n # Set CC suppressions\n my(%suppress_cc);\n-- \n2.31.1\n\n"},{"id":"421592","messageId":"20210411125431.28971-4-sir@cmpwn.com","threadId":"55476","inReplyTo":"20210411125431.28971-1-sir@cmpwn.com","subject":"[PATCH v2 3/3] git-send-email: rename 'tls' to 'starttls'","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2021-04-11T12:54:31Z","receivedAt":"2021-04-11T12:54:48Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"The name 'tls' is misleading. The 'ssl' option enables a generic\n\"modern\" encryption stack which might very well use TLS; but the 'tls'\noption enables STARTTLS support, which works entirely differently.\n\nThis renames the canonical option to 'starttls', to make this\ndistinction more obvious, and adds 'tls' as an alias for starttls, to\navoid breaking config files.\n\nSigned-off-by: Drew DeVault <sir@cmpwn.com>\n---\n Documentation/git-send-email.txt |  6 ++++--\n git-send-email.perl              | 10 +++++++---\n 2 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 520b355e50..f8cea9e1f9 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -168,12 +168,14 @@ Sending\n \tunspecified, choosing the envelope sender is left to your MTA.\n \n --smtp-encryption=<encryption>::\n-\tSpecify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables\n-\tgeneric SSL/TLS support and is typically used on port 465.  'tls'\n+\tSpecify the encryption to use, either 'ssl' or 'starttls'. 'ssl' enables\n+\tgeneric SSL/TLS support and is typically used on port 465.  'starttls'\n \tenables in-band STARTTLS support and is typically used on port 25 or\n \t587.  Use whichever option is recommended by your mail provider.  Leave\n \tempty to disable encryption and use plain SMTP.  Default is the value of\n \t`sendemail.smtpEncryption`.\n++\n+'tls' is an alias for 'starttls' for legacy reasons.\n \n --smtp-domain=<FQDN>::\n \tSpecifies the Fully Qualified Domain Name (FQDN) used in the\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex bda5211f0d..3f125bc2b8 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -495,8 +495,12 @@ sub read_config {\n \n # 'default' encryption is none -- this only prevents a warning\n $smtp_encryption = '' unless (defined $smtp_encryption);\n-if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"tls\") {\n-\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\\n\");\n+if ($smtp_encryption eq \"tls\") {\n+\t# \"tls\" is an alias for starttls for legacy reasons\n+\t$smtp_encryption = \"starttls\";\n+};\n+if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"starttls\") {\n+\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'starttls', or nothing.\\n\");\n }\n \n # Set CC suppressions\n@@ -1541,7 +1545,7 @@ sub send_message {\n \t\t\t\t\t\t Hello => $smtp_domain,\n \t\t\t\t\t\t Debug => $debug_net_smtp,\n \t\t\t\t\t\t Port => $smtp_server_port);\n-\t\t\tif ($smtp_encryption eq 'tls' && $smtp) {\n+\t\t\tif ($smtp_encryption eq 'starttls' && $smtp) {\n \t\t\t\tif ($use_net_smtp_ssl) {\n \t\t\t\t\t$smtp->command('STARTTLS');\n \t\t\t\t\t$smtp->response();\n-- \n2.31.1\n\n"},{"id":"421595","messageId":"87h7kcgbct.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"20210411125431.28971-2-sir@cmpwn.com","subject":"Re: [PATCH v2 1/3] git-send-email(1): improve smtp-encryption docs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T14:11:14Z","receivedAt":"2021-04-11T14:11:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 11 2021, Drew DeVault wrote:\n\nSubject nit: 1/3 is \"git-send-email(1):\", the rest\n\"git-send-email:\". I'd suggest just \"send-email:\", we usually omit\n\"git-\" from the subcommand, and don't use man sections to refer to our\nown software.\n\n> This clarifies the meaning of the 'ssl' and 'tls' values for this\n> option, which respectively enable SSL/TLS, i.e. a standard \"modern\" SSL\n> approach; and STARTTLS, i.e. opportunistic in-band TLS.\n>\n> Signed-off-by: Drew DeVault <sir@cmpwn.com>\n> ---\n>  Documentation/git-send-email.txt | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> index 93708aefea..c17c3b400a 100644\n> --- a/Documentation/git-send-email.txt\n> +++ b/Documentation/git-send-email.txt\n> @@ -168,8 +168,11 @@ Sending\n>  \tunspecified, choosing the envelope sender is left to your MTA.\n>  \n>  --smtp-encryption=<encryption>::\n> -\tSpecify the encryption to use, either 'ssl' or 'tls'.  Any other\n> -\tvalue reverts to plain SMTP.  Default is the value of\n> +\tSpecify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables\n\nStarting a sentance with a quoted lower-case word makes for hard\nreading. Maybe:\n\n    When set to 'ssl' ...\n\nOr something? \n\n> +\tgeneric SSL/TLS support and is typically used on port 465.  'tls'\n> +\tenables in-band STARTTLS support and is typically used on port 25 or\n> +\t587.  Use whichever option is recommended by your mail provider.  Any\n> +\tother value reverts to plain SMTP.  Default is the value of\n>  \t`sendemail.smtpEncryption`.\n>  \n>  --smtp-domain=<FQDN>::\n\n"},{"id":"421596","messageId":"87eefggb2q.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"20210411125431.28971-4-sir@cmpwn.com","subject":"Re: [PATCH v2 3/3] git-send-email: rename 'tls' to 'starttls'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T14:17:17Z","receivedAt":"2021-04-11T14:17:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 11 2021, Drew DeVault wrote:\n\n> The name 'tls' is misleading. The 'ssl' option enables a generic\n> \"modern\" encryption stack which might very well use TLS; but the 'tls'\n> option enables STARTTLS support, which works entirely differently.\n>\n> This renames the canonical option to 'starttls', to make this\n> distinction more obvious, and adds 'tls' as an alias for starttls, to\n> avoid breaking config files.\n>\n> Signed-off-by: Drew DeVault <sir@cmpwn.com>\n> ---\n>  Documentation/git-send-email.txt |  6 ++++--\n>  git-send-email.perl              | 10 +++++++---\n>  2 files changed, 11 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> index 520b355e50..f8cea9e1f9 100644\n> --- a/Documentation/git-send-email.txt\n> +++ b/Documentation/git-send-email.txt\n> @@ -168,12 +168,14 @@ Sending\n>  \tunspecified, choosing the envelope sender is left to your MTA.\n>  \n>  --smtp-encryption=<encryption>::\n> -\tSpecify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables\n> -\tgeneric SSL/TLS support and is typically used on port 465.  'tls'\n> +\tSpecify the encryption to use, either 'ssl' or 'starttls'. 'ssl' enables\n> +\tgeneric SSL/TLS support and is typically used on port 465.  'starttls'\n>  \tenables in-band STARTTLS support and is typically used on port 25 or\n>  \t587.  Use whichever option is recommended by your mail provider.  Leave\n>  \tempty to disable encryption and use plain SMTP.  Default is the value of\n>  \t`sendemail.smtpEncryption`.\n> ++\n> +'tls' is an alias for 'starttls' for legacy reasons.\n>  \n>  --smtp-domain=<FQDN>::\n>  \tSpecifies the Fully Qualified Domain Name (FQDN) used in the\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index bda5211f0d..3f125bc2b8 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -495,8 +495,12 @@ sub read_config {\n>  \n>  # 'default' encryption is none -- this only prevents a warning\n>  $smtp_encryption = '' unless (defined $smtp_encryption);\n> -if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"tls\") {\n> -\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\\n\");\n> +if ($smtp_encryption eq \"tls\") {\n> +\t# \"tls\" is an alias for starttls for legacy reasons\n> +\t$smtp_encryption = \"starttls\";\n> +};\n\nNeedless trailing \";\".\n\nThis and the preceding patch would be more readable if it was\nre-arranged in some way as to not rewrite the newly introduced lines\nbetween 2 and 3, maybe:\n\n{\n\tmy $tls_name = \"tls\";\n        if (....)\n}\n\nThen you'd only need to change \"tls\" to \"starttls\" there.\n\n> +if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"starttls\") {\n> +\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'starttls', or nothing.\\n\");\n>  }\n>  \n>  # Set CC suppressions\n> @@ -1541,7 +1545,7 @@ sub send_message {\n>  \t\t\t\t\t\t Hello => $smtp_domain,\n>  \t\t\t\t\t\t Debug => $debug_net_smtp,\n>  \t\t\t\t\t\t Port => $smtp_server_port);\n> -\t\t\tif ($smtp_encryption eq 'tls' && $smtp) {\n> +\t\t\tif ($smtp_encryption eq 'starttls' && $smtp) {\n\nAnd this could use the same variable.\n"},{"id":"421597","messageId":"87blakgaxr.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"20210411125431.28971-3-sir@cmpwn.com","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T14:20:16Z","receivedAt":"2021-04-11T14:20:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 11 2021, Drew DeVault wrote:\n\n> Signed-off-by: Drew DeVault <sir@cmpwn.com>\n> ---\n>  Documentation/git-send-email.txt | 4 ++--\n>  git-send-email.perl              | 3 +++\n>  2 files changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\n> index c17c3b400a..520b355e50 100644\n> --- a/Documentation/git-send-email.txt\n> +++ b/Documentation/git-send-email.txt\n> @@ -171,8 +171,8 @@ Sending\n>  \tSpecify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables\n>  \tgeneric SSL/TLS support and is typically used on port 465.  'tls'\n>  \tenables in-band STARTTLS support and is typically used on port 25 or\n> -\t587.  Use whichever option is recommended by your mail provider.  Any\n> -\tother value reverts to plain SMTP.  Default is the value of\n> +\t587.  Use whichever option is recommended by your mail provider.  Leave\n> +\tempty to disable encryption and use plain SMTP.  Default is the value of\n>  \t`sendemail.smtpEncryption`.\n>  \n>  --smtp-domain=<FQDN>::\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index f5bbf1647e..bda5211f0d 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -495,6 +495,9 @@ sub read_config {\n>  \n>  # 'default' encryption is none -- this only prevents a warning\n>  $smtp_encryption = '' unless (defined $smtp_encryption);\n> +if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"tls\") {\n> +\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\\n\");\n> +}\n\nHaving not tested this but just eyeballed the code, I'm fairly sure\nyou're adding a logic error here, or is $smtp_encryption guaranteed to\nbe defined at this point?\n\nWe start with it as undef, then read the config, then the CLI\noptions. If we've got neither it'll still be undef here, no?\n\nThus the string comparison will emit a warning.\n\nMaybe I'm overly used to regexen in Perl, but I'd also find this more\nreadable as:\n\n    $smtp_encryption !~ /^(?:|ssl|tls)$/s\n\nOr something like:\n\n    my @valid_smtp_encryption = ('', qw(ssl tls));\n    if (!grep { $_ eq $smtp_encryption } @valid_smtp_encryption) {\n        ....\n    }\n\n"},{"id":"421598","messageId":"CAKYMAEJQOA3.25YK6UYSYFHXQ@taiga","threadId":"55476","inReplyTo":"87blakgaxr.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2021-04-11T14:21:34Z","receivedAt":"2021-04-11T14:21:41Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote:\n> >  # 'default' encryption is none -- this only prevents a warning\n> >  $smtp_encryption = '' unless (defined $smtp_encryption);\n> > +if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"tls\") {\n> > +\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\\n\");\n> > +}\n>\n> Having not tested this but just eyeballed the code, I'm fairly sure\n> you're adding a logic error here, or is $smtp_encryption guaranteed to\n> be defined at this point?\n\nI will admit to being ignorant of much of Perl's semantics, but I had\nassumed that the line prior to my additions addresses this:\n\n$smtp_encryption = '' unless (defined $smtp_encryption);\n\n> $smtp_encryption !~ /^(?:|ssl|tls)$/s\n\nYeah, that would probably be better.\n"},{"id":"421599","messageId":"CAKYMSVQ9ZM9.32H3OS6V9CLK9@taiga","threadId":"55476","inReplyTo":"87eefggb2q.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] git-send-email: rename 'tls' to 'starttls'","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2021-04-11T14:22:14Z","receivedAt":"2021-04-11T14:22:19Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"On Sun Apr 11, 2021 at 10:17 AM EDT, Ævar Arnfjörð Bjarmason wrote:\n> >  # 'default' encryption is none -- this only prevents a warning\n> >  $smtp_encryption = '' unless (defined $smtp_encryption);\n> > -if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"tls\") {\n> > -\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\\n\");\n> > +if ($smtp_encryption eq \"tls\") {\n> > +\t# \"tls\" is an alias for starttls for legacy reasons\n> > +\t$smtp_encryption = \"starttls\";\n> > +};\n>\n> Needless trailing \";\".\n>\n> This and the preceding patch would be more readable if it was\n> re-arranged in some way as to not rewrite the newly introduced lines\n> between 2 and 3, maybe:\n>\n> {\n> my $tls_name = \"tls\";\n> if (....)\n> }\n\nI disagree that this would be an improvement. It would make the patches\na bit more readlable on their own, but the resulting code would\nintroduce this bizzare variable which doesn't make sense out of context.\n"},{"id":"421600","messageId":"878s5ogagz.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"CAKYMAEJQOA3.25YK6UYSYFHXQ@taiga","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T14:30:20Z","receivedAt":"2021-04-11T14:30:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 11 2021, Drew DeVault wrote:\n\n> On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote:\n>> >  # 'default' encryption is none -- this only prevents a warning\n>> >  $smtp_encryption = '' unless (defined $smtp_encryption);\n>> > +if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"tls\") {\n>> > +\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\\n\");\n>> > +}\n>>\n>> Having not tested this but just eyeballed the code, I'm fairly sure\n>> you're adding a logic error here, or is $smtp_encryption guaranteed to\n>> be defined at this point?\n>\n> I will admit to being ignorant of much of Perl's semantics, but I had\n> assumed that the line prior to my additions addresses this:\n>\n> $smtp_encryption = '' unless (defined $smtp_encryption);\n\nYou're right, I just misread the diff. Nevermind.\n"},{"id":"421602","messageId":"patch-1.2-ee041188e55-20210411T144128Z-avarab@gmail.com","threadId":"55476","inReplyTo":"cover-0.2-00000000000-20210411T144128Z-avarab@gmail.com","subject":"[PATCH 1/2] send-email: remove non-working support for \"sendemail.smtpssl\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T14:43:19Z","receivedAt":"2021-04-11T14:43:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the already dead code to support \"sendemail.smtssl\" by finally\nremoving the dead code supporting the configuration option.\n\nIn f6bebd121ac (git-send-email: add support for TLS via\nNet::SMTP::SSL, 2008-06-25) the --smtp-ssl command-line option was\ndocumented as deprecated, later in 65180c66186 (List send-email config\noptions in config.txt., 2009-07-22) the \"sendemail.smtpssl\"\nconfiguration option was also documented as such.\n\nThen in in 3ff15040e22 (send-email: fix regression in\nsendemail.identity parsing, 2019-05-17) I unintentionally removed\nsupport for it by introducing a bug in read_config().\n\nAs can be seen from the diff context we've already returned unless\n$enc i defined, so it's not possible for us to reach the \"elsif\"\nbranch here. This code was therefore already dead since Git v2.23.0.\n\nSo let's just remove it instead of fixing the bug, clearly nobody's\ncared enough to complain.\n\nThe --smtp-ssl option is still deprecated, if someone cares they can\nfollow-up and remove that too, but unlike the config option that one\ncould still be in use in the wild. I'm just removing this code that's\nprovably unused already.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/config/sendemail.txt | 3 ---\n git-send-email.perl                | 6 +-----\n 2 files changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt\nindex cbc5af42fdf..50baa5d6bfb 100644\n--- a/Documentation/config/sendemail.txt\n+++ b/Documentation/config/sendemail.txt\n@@ -8,9 +8,6 @@ sendemail.smtpEncryption::\n \tSee linkgit:git-send-email[1] for description.  Note that this\n \tsetting is not subject to the 'identity' mechanism.\n \n-sendemail.smtpssl (deprecated)::\n-\tDeprecated alias for 'sendemail.smtpEncryption = ssl'.\n-\n sendemail.smtpsslcertpath::\n \tPath to ca-certificates (either a directory or a single file).\n \tSet it to an empty string to disable certificate verification.\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex f5bbf1647e3..877c7dd1a21 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -374,11 +374,7 @@ sub read_config {\n \t\tmy $enc = Git::config(@repo, $setting);\n \t\treturn unless defined $enc;\n \t\treturn if $configured->{$setting}++;\n-\t\tif (defined $enc) {\n-\t\t\t$smtp_encryption = $enc;\n-\t\t} elsif (Git::config_bool(@repo, \"$prefix.smtpssl\")) {\n-\t\t\t$smtp_encryption = 'ssl';\n-\t\t}\n+\t\t$smtp_encryption = $enc;\n \t}\n }\n \n-- \n2.31.1.623.g88b15a793d\n\n"},{"id":"421603","messageId":"cover-0.2-00000000000-20210411T144128Z-avarab@gmail.com","threadId":"55476","inReplyTo":"20210411125431.28971-1-sir@cmpwn.com","subject":"[PATCH 0/2] send-email: simplify smtp.{smtpssl,smtpencryption} parsing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T14:43:18Z","receivedAt":"2021-04-11T14:43:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in 1/2 I unintentionally broke the deprecated\nsendemail.smtpssl configuration parsing a while ago. Since nothing\nactually uses it let's remove it.\n\nThis doesn't conflict with Drew's\nhttp://lore.kernel.org/git/20210411125431.28971-1-sir@cmpwn.com\nseries, but as I'll reply to there knowing that we can do this might\nsimplify some things for it, if it were to be based on top of this.\n\nÆvar Arnfjörð Bjarmason (2):\n  send-email: remove non-working support for \"sendemail.smtpssl\"\n  send-email: refactor sendemail.smtpencryption config parsing\n\n Documentation/config/sendemail.txt |  3 ---\n git-send-email.perl                | 13 +------------\n 2 files changed, 1 insertion(+), 15 deletions(-)\n\n-- \n2.31.1.623.g88b15a793d\n\n"},{"id":"421604","messageId":"patch-2.2-2de5edcf8f4-20210411T144128Z-avarab@gmail.com","threadId":"55476","inReplyTo":"cover-0.2-00000000000-20210411T144128Z-avarab@gmail.com","subject":"[PATCH 2/2] send-email: refactor sendemail.smtpencryption config parsing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T14:43:20Z","receivedAt":"2021-04-11T14:43:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"With the removal of the support for sendemail.smtpssl in the preceding\ncommit the parsing of sendemail.smtpencryption is no longer special,\nand can by moved to %config_settings.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-send-email.perl | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 877c7dd1a21..da28c6e8b4b 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -268,6 +268,7 @@ sub do_edit {\n );\n \n my %config_settings = (\n+    \"smtpencryption\" => \\$smtp_encryption,\n     \"smtpserver\" => \\$smtp_server,\n     \"smtpserverport\" => \\$smtp_server_port,\n     \"smtpserveroption\" => \\@smtp_server_options,\n@@ -368,14 +369,6 @@ sub read_config {\n \t\t\t$$target = $v;\n \t\t}\n \t}\n-\n-\tif (!defined $smtp_encryption) {\n-\t\tmy $setting = \"$prefix.smtpencryption\";\n-\t\tmy $enc = Git::config(@repo, $setting);\n-\t\treturn unless defined $enc;\n-\t\treturn if $configured->{$setting}++;\n-\t\t$smtp_encryption = $enc;\n-\t}\n }\n \n # sendemail.identity yields to --identity. We must parse this\n-- \n2.31.1.623.g88b15a793d\n\n"},{"id":"421605","messageId":"875z0sg8t9.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"878s5ogagz.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T15:06:10Z","receivedAt":"2021-04-11T15:06:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 11 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> On Sun, Apr 11 2021, Drew DeVault wrote:\n>\n>> On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote:\n>>> >  # 'default' encryption is none -- this only prevents a warning\n>>> >  $smtp_encryption = '' unless (defined $smtp_encryption);\n>>> > +if ($smtp_encryption ne \"\" && $smtp_encryption ne \"ssl\" && $smtp_encryption ne \"tls\") {\n>>> > +\tdie __(\"Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\\n\");\n>>> > +}\n>>>\n>>> Having not tested this but just eyeballed the code, I'm fairly sure\n>>> you're adding a logic error here, or is $smtp_encryption guaranteed to\n>>> be defined at this point?\n>>\n>> I will admit to being ignorant of much of Perl's semantics, but I had\n>> assumed that the line prior to my additions addresses this:\n>>\n>> $smtp_encryption = '' unless (defined $smtp_encryption);\n>\n> You're right, I just misread the diff. Nevermind.\n\nSo on a second reading.\n\nSo first, I've been sitting on some fairly extensive send-email patches\nlocally, but have been trying to focus on re-rolling some of my\noutstanding stuff.\n\nBut I just sent two patches directly relevant to this series as\nhttps://lore.kernel.org/git/cover-0.2-00000000000-20210411T144128Z-avarab@gmail.com/\n\nSomething felt a bit wrong about the approach in your series, I wasn't\nquite sure what initially, but here it is;\n\nSo, the only reason we have that \"encryption is none -- this only\nprevents a warning\" so late in the file (as opposed to setting it to ''\nwhen we declare the variable) was because of the\nsmtp.{smtpssl,smtpencryption} interaction, i.e. we relied on it being\nundef to see if we needed to parse the secondary variable.\n\nBut with it gone with my small series (it already didn't work) we can\nget rid of that special case.\n\nBut, on the specifics of the \"felt funny\":\n\n 1. Your 2/3 changes a long standing existing \"any other value = no\n    encryption\" to \"die on unrecognized\". I happen to think this is\n    probably a good idea, but let's be explicit in the commit message,\n    e.g.:\n\n        We don't think it's a good idea to silently degrade to\n        non-encrypted as we've been promising just because your version\n        doesn't support something, let's die instead.\n\n 2. If we're breaking the \"any other value\" we should not be documenting\n    the \"or nothing\", the distinction between \"\" and undef on the\n    Perl-level was just a leaky implementation detail.\n\n    But let's not conflate that with how we present something to the\n    user. It's not the same to not set a variable v.s. setting it to the\n    empty string.\n\n    With my 2-part series it's even more trivial to detect that, but\n    even on top of master you just move your check above the \"set to\n    empty unless defined\".\n\n 3. While I'm very much leaning to #1 being a good idea, I'm very much\n    leaning towards introducing this \"starttls\" alias being a bad idea\n    for the same reason.\n    \n    I.e. let's not create a new 'starttls' if we can avoid it explicitly\n    because we used to have the long-standing \"anything unrecognized is\n    empty == no encryption\" behavior.\n\n    A lot of users read documentation for the latest version online, but\n    may have an older version installed.\n\n    To the extent that anyone cares about the transport security of\n    git-send-email (I'm kind of \"meh\" on it, but if we're making\n    sendemail.smtpEncryption parsing strict you probably disagree), then\n    such silent downgrading seems worse than just not accepting starttls\n    at all. I.e. to have a new behavior of something like:\n\n        if (defined $smtp_encryption) {\n        \tdie \"we call 'starttls' 'tls' for historical reasons, sorry!\" if $smtp_encryption eq 'starttls';\n\t\tdie \"unknown mode '$smtp_encryption'\" unless $smtp_encryption =~ /^(?:ssl|tls)$/s;\n\t} else {\n\t\t$smtp_encryption = '';\n\t}\n\n    I.e. I get that it's confusing, but isn't it enough to address the\n    TLS v.s. STARTTLS confusion in the docs, as opposed to supporting it\n    in the config format, which as noted will silently downgrade on\n    older versions?\n"},{"id":"421607","messageId":"CAKZTYI6U0WY.36DC3N1E4R7D2@taiga","threadId":"55476","inReplyTo":"875z0sg8t9.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2021-04-11T15:18:36Z","receivedAt":"2021-04-11T15:18:43Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"On Sun Apr 11, 2021 at 11:06 AM EDT, Ævar Arnfjörð Bjarmason wrote:\n> 3. While I'm very much leaning to #1 being a good idea, I'm very much\n> leaning towards introducing this \"starttls\" alias being a bad idea\n> for the same reason.\n>     \n> i.e. let's not create a new 'starttls' if we can avoid it explicitly\n> because we used to have the long-standing \"anything unrecognized is\n> empty == no encryption\" behavior.\n>\n> A lot of users read documentation for the latest version online, but\n> may have an older version installed.\n\nI feel quite strongly that the options here are a grave failure of\nusability, and that it needs to be corrected. I help people troubleshoot\ngit send-email problems quite often, and this is a recurring error.\nHowever, you make a good point in that someone might see some online\ndocumentation which does not match their git version and end up with a\nsurprisingly unencrypted connection.\n\nAs a compromise, let's consider making this a gradual change. We can\nstart by clarifying the docs and forbiding the use of any value other\nthan 'ssl' or 'tls'. If an unknown value is set, the user is not getting\nthe encryption they expected anyway, and this should cause an error.\n\nThen we can leave the issue aside for some agreed upon period of time to\nallow the change to proliferate in the ecosystem, and then revisit this\nat some point in the future to rename the options to make more sense.\n\nDoes this seem like a reasonable compromise?\n"},{"id":"421612","messageId":"xmqqk0p8hc5v.fsf@gitster.g","threadId":"55476","inReplyTo":"patch-1.2-ee041188e55-20210411T144128Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] send-email: remove non-working support for \"sendemail.smtpssl\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-11T19:08:28Z","receivedAt":"2021-04-11T19:08:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> So let's just remove it instead of fixing the bug, clearly nobody's\n> cared enough to complain.\n\nHmph, is that a safe assumption?  They may have just assumed that\nyou did not break it and kept using plaintext without knowing?  If\nwe do not give a warning when sending over an unencrypted channel in\nred flashing letters, that is more likely explanation than nobody\ncaring that we saw no breakage reports, no?\n"},{"id":"421615","messageId":"8735vwfvln.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"xmqqk0p8hc5v.fsf@gitster.g","subject":"Re: [PATCH 1/2] send-email: remove non-working support for \"sendemail.smtpssl\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T19:51:32Z","receivedAt":"2021-04-11T19:51:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 11 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> So let's just remove it instead of fixing the bug, clearly nobody's\n>> cared enough to complain.\n>\n> Hmph, is that a safe assumption?  They may have just assumed that\n> you did not break it and kept using plaintext without knowing?  If\n> we do not give a warning when sending over an unencrypted channel in\n> red flashing letters, that is more likely explanation than nobody\n> caring that we saw no breakage reports, no?\n\nMaybe, I think in either case this patch series makes senes. We were\nalready 11 years into a stated deprecation period of that variable, now\nit's 13.\n\nIf we're going to e.g. emit some notice about it I think the parsing\nsimplification this series gives us makes sense, we can always add a\ntrivial patch on top to make it die if it sees the old variable.\n\nI don't think that's needed, do you?\n"},{"id":"421616","messageId":"87zgy4egtp.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"CAKZTYI6U0WY.36DC3N1E4R7D2@taiga","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-11T19:56:02Z","receivedAt":"2021-04-11T19:56:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 11 2021, Drew DeVault wrote:\n\n> On Sun Apr 11, 2021 at 11:06 AM EDT, Ævar Arnfjörð Bjarmason wrote:\n>> 3. While I'm very much leaning to #1 being a good idea, I'm very much\n>> leaning towards introducing this \"starttls\" alias being a bad idea\n>> for the same reason.\n>>     \n>> i.e. let's not create a new 'starttls' if we can avoid it explicitly\n>> because we used to have the long-standing \"anything unrecognized is\n>> empty == no encryption\" behavior.\n>>\n>> A lot of users read documentation for the latest version online, but\n>> may have an older version installed.\n>\n> I feel quite strongly that the options here are a grave failure of\n> usability, and that it needs to be corrected. I help people troubleshoot\n> git send-email problems quite often, and this is a recurring error.\n> However, you make a good point in that someone might see some online\n> documentation which does not match their git version and end up with a\n> surprisingly unencrypted connection.\n>\n> As a compromise, let's consider making this a gradual change. We can\n> start by clarifying the docs and forbiding the use of any value other\n> than 'ssl' or 'tls'. If an unknown value is set, the user is not getting\n> the encryption they expected anyway, and this should cause an error.\n>\n> Then we can leave the issue aside for some agreed upon period of time to\n> allow the change to proliferate in the ecosystem, and then revisit this\n> at some point in the future to rename the options to make more sense.\n>\n> Does this seem like a reasonable compromise?\n\nI suggest we don't compromise and just go with whatever you're OK with\n:)\n\nI really don't care enough about #1 and #3 in my E-Mail to in any way\npush for it, sorry if it came off that way.\n\nI just wanted to check your assumptions when reviewing the series. I do\nthink that it would make sense to more prominently note something to the\neffect of \"this was documented to do X all along, now we do Y, but\nthat's OK because ABC\", and to note why the new starttls = plaintext on\nolder versions is OK, maybe it's just fine. I really don't know.\n\nIsn't it pretty common in any case that SMTP servers in the wild just\nrefuse plaintext these days when dealing with auth'd connections? I\ndon't know.\n\nI do think it makes sense to fixup for my suggested #2, i.e. not leaking\nthe internal detail of the \"empty string\".\n"},{"id":"421676","messageId":"CALQY92B6OVL.2Z59Y6W51BU4Y@taiga","threadId":"55476","inReplyTo":"87zgy4egtp.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2021-04-12T12:33:42Z","receivedAt":"2021-04-12T12:33:56Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"On Sun Apr 11, 2021 at 3:56 PM EDT, Ævar Arnfjörð Bjarmason wrote:\n> I suggest we don't compromise and just go with whatever you're OK with :)\n\nWell, if you're giving me an opportunity to not drag this out into a\nmulti-phase rollout, then I'll take it :)\n\nAnother option is to forbid an unknown value (which is almost certainly\n(1) wrong and (2) causing users to unexpectedly use plaintext when they\nexpected encryption), file a CVE, and pitch it as a security fix - then\nwe can expect a reasonably quick rollout of the change to the ecosystem\nat large.\n"},{"id":"421680","messageId":"87o8ejej8m.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"CALQY92B6OVL.2Z59Y6W51BU4Y@taiga","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T13:16:09Z","receivedAt":"2021-04-12T13:16:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Apr 12 2021, Drew DeVault wrote:\n\n> On Sun Apr 11, 2021 at 3:56 PM EDT, Ævar Arnfjörð Bjarmason wrote:\n>> I suggest we don't compromise and just go with whatever you're OK with :)\n>\n> Well, if you're giving me an opportunity to not drag this out into a\n> multi-phase rollout, then I'll take it :)\n\nJust to be clear even if I was insisting on that I'm still just one guy\non the ML reviewing your patch.\n\nAs a first approximation the opinion of regular contributors counts for\nmore when the topic is some tricky interaction of code they wrote/are\nfamiliar with.\n\nIn this case we're just discussing the general interaction of security,\noptional switches, software versioning and how SMTP servers in the wild\nwork.\n\nI'd think someone who e.g. needs to regularly deal with SMTP servers in\nthe wild would have a much better idea of those trade-offs than someone\n(like me) who happens to have some existing patches in git.git to\ngit-send-email.perl.\n\n> Another option is to forbid an unknown value (which is almost certainly\n> (1) wrong and (2) causing users to unexpectedly use plaintext when they\n> expected encryption), file a CVE, and pitch it as a security fix - then\n> we can expect a reasonably quick rollout of the change to the ecosystem\n> at large.\n\nI think anyone would agree that in retrospect \"unknown is plaintext\" for\nthe \"what encryption do you want\" option is at best a something\napproaching a shotgun to your foot of a UI pattern.\n\nBut I think it falls far short of a CVE. We *do* prominently document\nit, a potential CVE would be if we had silent degration to plaintext\n(well, in a mode whose inherent workings aren't to be vulnerable to that\nattack, as STARTTLS is...).\n\nFWIW since my upthread <87zgy4egtp.fsf@evledraar.gmail.com> I tried\nsending mail through GMail's plain-text smtp gateway as an authenticated\nuser.\n\nTesting with:\n\n    nc smtp.gmail.com 25\n    openssl s_client -connect smtp.gmail.com:465\n\nIt will emit a 530 if you try to AUTH in plain-text (telling you to use\nSTARTTLS), it will also only say \"AUTH\" in the EHLO response to the\nlatter.\n\nAnd indeed Net::SMTP picks up on this, and doesn't even send your\nuser/password:\nhttps://metacpan.org/release/libnet/source/lib/Net/SMTP.pm#L169\n\nSo this hypothetical degradation of the connection and sending auth over\nplain-text I suggested in upthread #3 seems to mostly/entirely be a\nnon-issue as far as e.g. accidentally sending your password on some open\nWiFi network goes due to a local misconfiguration.\n\nAs long as the SMTP server is functional enough to say it doesn't\nsupport AUTH on plain-text you'll be OK. I'm assuming that these days\nwith the push for \"SSL everywhere\" most/all big providers/MTAs have\nmoved away from supporing plain-text auth by default.\n"},{"id":"421883","messageId":"CAML4RYHKQ6U.35902JHAIZYY@taiga","threadId":"55476","inReplyTo":"87o8ejej8m.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2021-04-13T12:12:47Z","receivedAt":"2021-04-13T12:12:55Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"Can I get one of the maintainers to chime in on this thread and explain,\nin their opinion, what this patchset needs before it is acceptable? I'm\nnot sure where I should go from this discussion.\n"},{"id":"421905","messageId":"87lf9m2rj3.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"CAML4RYHKQ6U.35902JHAIZYY@taiga","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-13T14:22:24Z","receivedAt":"2021-04-13T14:22:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 13 2021, Drew DeVault wrote:\n\n> Can I get one of the maintainers to chime in on this thread and explain,\n> in their opinion, what this patchset needs before it is acceptable? I'm\n> not sure where I should go from this discussion.\n\nGit just has the one maintainer, Junio C Hamano.\n\nUltimately getting your patch in is up to his whimsy.\n\nMaybe he'll reply, but in an attempt to save him some time (which I\nunderstand I'm taking too much of these days):\n\nGetting your patch in is ultimately up to you.\n\nSo you submitted a patch, got some feedback/review.\n\nThrough some combination of this thread (mainly\n<875z0sg8t9.fsf@evledraar.gmail.com> and\n<87o8ejej8m.fsf@evledraar.gmail.com>) I suggested making some changes in a v3.\n\nI.e. cleaning up some of the semantics (config docs/handling leaking\nimplementation details) and improving the commit message(s) to summarize\nthe trade-offs, why this approach is safe/isn't safe/why it's OK in the\ncases it's not etc.\n\nSo, whatever Junio or anyone else thinks of my opinion I think it's fair\nto say that at this point he's most likely to skim this thread and see\nthat there's some outstanding feedback that hasn't been addressed.\n\n\"Addressed\" means either re-rolling into a v3, or deciding that nothing\n(or only part of the feedback) needs to be changed and/or\naddressed.\n\nBoth/some of those/that are perfectly acceptable approaches, but in\neither case making things easily digestable to Junio will help your\nseries along.\n\n"},{"id":"421935","messageId":"xmqqtuo9kgo0.fsf@gitster.g","threadId":"55476","inReplyTo":"CAML4RYHKQ6U.35902JHAIZYY@taiga","subject":"Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-13T21:39:43Z","receivedAt":"2021-04-13T21:39:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Drew DeVault\" <sir@cmpwn.com> writes:\n\n> Can I get one of the maintainers to chime in on this thread and explain,\n> in their opinion, what this patchset needs before it is acceptable? I'm\n> not sure where I should go from this discussion.\n\nWhat you called \"compromise\" in the upthread didn't even smelled\nlike a compromise but the safest way forward.  My reasoning goes\n(thinking aloud, so that you and Ævar can correct me if I am talking\nnonsense and discount my input based on it):\n\n - If we were designing this from scratch, we would have called the\n   smtps:// tunnelled transport SSL/TLS and in-place upgrade\n   transport STARTTLS, like e-mail providers and client programs do,\n   but unfortunately we didn't.  We ended up with 'ssl' vs 'tls'.\n\n - We could introduce and advertise STARTTLS as a synonym to 'tls',\n   but then those whose send-email does not understand STARTTLS but\n   read about the new way of spelling would end up having no\n   encryption due to another earlier mistake we made, i.e. an\n   unrecognised option value silently turns into no encryption.\n\n - To avoid the above problem, the first phase is not to change the\n   status quo that 'ssl' vs 'tls' are the only two choices.  What we\n   do is to make the program error out if we see an unrecognised\n   value given to the option.  We release this to the wild, and wait\n   for the current versions of send-email that turns unrecognised\n   words into no-encryption die out.  It may take several years,\n   though.\n\n - After waiting, we add 'starttls' as a synonym to 'tls'.  We may\n   also add 'ssl/tls' as a synonym to 'ssl'.  Unfortunately 'tls'\n   alone cannot be repurposed as a synonym for smtps:// without\n   another deprecation dance, and it is not in scope of the\n   transtion.\n\nAm I on the same page as you two?  \n"},{"id":"423398","messageId":"87a6peyfry.fsf@evledraar.gmail.com","threadId":"55476","inReplyTo":"8735vwfvln.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/2] send-email: remove non-working support for \"sendemail.smtpssl\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-01T09:15:43Z","receivedAt":"2021-05-01T09:17:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Apr 11 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> On Sun, Apr 11 2021, Junio C Hamano wrote:\n>\n>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>>\n>>> So let's just remove it instead of fixing the bug, clearly nobody's\n>>> cared enough to complain.\n>>\n>> Hmph, is that a safe assumption?  They may have just assumed that\n>> you did not break it and kept using plaintext without knowing?  If\n>> we do not give a warning when sending over an unencrypted channel in\n>> red flashing letters, that is more likely explanation than nobody\n>> caring that we saw no breakage reports, no?\n>\n> Maybe, I think in either case this patch series makes senes. We were\n> already 11 years into a stated deprecation period of that variable, now\n> it's 13.\n>\n> If we're going to e.g. emit some notice about it I think the parsing\n> simplification this series gives us makes sense, we can always add a\n> trivial patch on top to make it die if it sees the old variable.\n>\n> I don't think that's needed, do you?\n\nJunio: *Poke*. Was going thorugh my outstanding patches, I still think\nit makes sense to just pick this up. Especially with the related\ndiscussion later about how common in-the-wild service providers would\njust not support AUTH non-encrypted, so in practice I think it's even\nless likely that anyone saw silent breakage as a result of this\nalready-deprecated variable being ignored.\n"}]}