[PATCH v2 0/3] git-send-email: improve SSL configuration

[PATCH v2 0/3] git-send-email: improve SSL configuration

[Drew DeVault]

Following feedback on v1, this splits the changes up into 3 commits,
including a new change which makes any value other than 'ssl',
'starttls', or nothing into an error. This also removes 'ssl/tls' in
favor of 'ssl' to avoid confusion, and avoids taking a stance on the
matter of the deprecation of either approach.

Drew DeVault (3):
  git-send-email(1): improve smtp-encryption docs
  git-send-email: die on invalid smtp_encryption
  git-send-email: rename 'tls' to 'starttls'

 Documentation/git-send-email.txt | 9 +++++++--
 git-send-email.perl              | 9 ++++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] git-send-email(1): improve smtp-encryption docs

[Drew DeVault]

This clarifies the meaning of the 'ssl' and 'tls' values for this
option, which respectively enable SSL/TLS, i.e. a standard "modern" SSL
approach; and STARTTLS, i.e. opportunistic in-band TLS.

Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
 Documentation/git-send-email.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 93708aefea..c17c3b400a 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -168,8 +168,11 @@ Sending
 	unspecified, choosing the envelope sender is left to your MTA.
 
 --smtp-encryption=<encryption>::
-	Specify the encryption to use, either 'ssl' or 'tls'.  Any other
-	value reverts to plain SMTP.  Default is the value of
+	Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables
+	generic SSL/TLS support and is typically used on port 465.  'tls'
+	enables in-band STARTTLS support and is typically used on port 25 or
+	587.  Use whichever option is recommended by your mail provider.  Any
+	other value reverts to plain SMTP.  Default is the value of
 	`sendemail.smtpEncryption`.
 
 --smtp-domain=<FQDN>::
-- 
2.31.1

[PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Drew DeVault]

Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
 Documentation/git-send-email.txt | 4 ++--
 git-send-email.perl              | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index c17c3b400a..520b355e50 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -171,8 +171,8 @@ Sending
 	Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables
 	generic SSL/TLS support and is typically used on port 465.  'tls'
 	enables in-band STARTTLS support and is typically used on port 25 or
-	587.  Use whichever option is recommended by your mail provider.  Any
-	other value reverts to plain SMTP.  Default is the value of
+	587.  Use whichever option is recommended by your mail provider.  Leave
+	empty to disable encryption and use plain SMTP.  Default is the value of
 	`sendemail.smtpEncryption`.
 
 --smtp-domain=<FQDN>::
diff --git a/git-send-email.perl b/git-send-email.perl
index f5bbf1647e..bda5211f0d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -495,6 +495,9 @@ sub read_config {
 
 # 'default' encryption is none -- this only prevents a warning
 $smtp_encryption = '' unless (defined $smtp_encryption);
+if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
+	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
+}
 
 # Set CC suppressions
 my(%suppress_cc);
-- 
2.31.1

[PATCH v2 3/3] git-send-email: rename 'tls' to 'starttls'

[Drew DeVault]

The name 'tls' is misleading. The 'ssl' option enables a generic
"modern" encryption stack which might very well use TLS; but the 'tls'
option enables STARTTLS support, which works entirely differently.

This renames the canonical option to 'starttls', to make this
distinction more obvious, and adds 'tls' as an alias for starttls, to
avoid breaking config files.

Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
 Documentation/git-send-email.txt |  6 ++++--
 git-send-email.perl              | 10 +++++++---
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 520b355e50..f8cea9e1f9 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -168,12 +168,14 @@ Sending
 	unspecified, choosing the envelope sender is left to your MTA.
 
 --smtp-encryption=<encryption>::
-	Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables
-	generic SSL/TLS support and is typically used on port 465.  'tls'
+	Specify the encryption to use, either 'ssl' or 'starttls'. 'ssl' enables
+	generic SSL/TLS support and is typically used on port 465.  'starttls'
 	enables in-band STARTTLS support and is typically used on port 25 or
 	587.  Use whichever option is recommended by your mail provider.  Leave
 	empty to disable encryption and use plain SMTP.  Default is the value of
 	`sendemail.smtpEncryption`.
++
+'tls' is an alias for 'starttls' for legacy reasons.
 
 --smtp-domain=<FQDN>::
 	Specifies the Fully Qualified Domain Name (FQDN) used in the
diff --git a/git-send-email.perl b/git-send-email.perl
index bda5211f0d..3f125bc2b8 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -495,8 +495,12 @@ sub read_config {
 
 # 'default' encryption is none -- this only prevents a warning
 $smtp_encryption = '' unless (defined $smtp_encryption);
-if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
-	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
+if ($smtp_encryption eq "tls") {
+	# "tls" is an alias for starttls for legacy reasons
+	$smtp_encryption = "starttls";
+};
+if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "starttls") {
+	die __("Invalid smtp_encryption configuration: expected 'ssl', 'starttls', or nothing.\n");
 }
 
 # Set CC suppressions
@@ -1541,7 +1545,7 @@ sub send_message {
 						 Hello => $smtp_domain,
 						 Debug => $debug_net_smtp,
 						 Port => $smtp_server_port);
-			if ($smtp_encryption eq 'tls' && $smtp) {
+			if ($smtp_encryption eq 'starttls' && $smtp) {
 				if ($use_net_smtp_ssl) {
 					$smtp->command('STARTTLS');
 					$smtp->response();
-- 
2.31.1

Re: [PATCH v2 1/3] git-send-email(1): improve smtp-encryption docs

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 11 2021, Drew DeVault wrote:

Subject nit: 1/3 is "git-send-email(1):", the rest
"git-send-email:". I'd suggest just "send-email:", we usually omit
"git-" from the subcommand, and don't use man sections to refer to our
own software.

> This clarifies the meaning of the 'ssl' and 'tls' values for this
> option, which respectively enable SSL/TLS, i.e. a standard "modern" SSL
> approach; and STARTTLS, i.e. opportunistic in-band TLS.
>
> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
>  Documentation/git-send-email.txt | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 93708aefea..c17c3b400a 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -168,8 +168,11 @@ Sending
>  	unspecified, choosing the envelope sender is left to your MTA.
>  
>  --smtp-encryption=<encryption>::
> -	Specify the encryption to use, either 'ssl' or 'tls'.  Any other
> -	value reverts to plain SMTP.  Default is the value of
> +	Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables

Starting a sentance with a quoted lower-case word makes for hard
reading. Maybe:

    When set to 'ssl' ...

Or something? 

> +	generic SSL/TLS support and is typically used on port 465.  'tls'
> +	enables in-band STARTTLS support and is typically used on port 25 or
> +	587.  Use whichever option is recommended by your mail provider.  Any
> +	other value reverts to plain SMTP.  Default is the value of
>  	`sendemail.smtpEncryption`.
>  
>  --smtp-domain=<FQDN>::

Re: [PATCH v2 3/3] git-send-email: rename 'tls' to 'starttls'

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 11 2021, Drew DeVault wrote:

> The name 'tls' is misleading. The 'ssl' option enables a generic
> "modern" encryption stack which might very well use TLS; but the 'tls'
> option enables STARTTLS support, which works entirely differently.
>
> This renames the canonical option to 'starttls', to make this
> distinction more obvious, and adds 'tls' as an alias for starttls, to
> avoid breaking config files.
>
> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
>  Documentation/git-send-email.txt |  6 ++++--
>  git-send-email.perl              | 10 +++++++---
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 520b355e50..f8cea9e1f9 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -168,12 +168,14 @@ Sending
>  	unspecified, choosing the envelope sender is left to your MTA.
>  
>  --smtp-encryption=<encryption>::
> -	Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables
> -	generic SSL/TLS support and is typically used on port 465.  'tls'
> +	Specify the encryption to use, either 'ssl' or 'starttls'. 'ssl' enables
> +	generic SSL/TLS support and is typically used on port 465.  'starttls'
>  	enables in-band STARTTLS support and is typically used on port 25 or
>  	587.  Use whichever option is recommended by your mail provider.  Leave
>  	empty to disable encryption and use plain SMTP.  Default is the value of
>  	`sendemail.smtpEncryption`.
> ++
> +'tls' is an alias for 'starttls' for legacy reasons.
>  
>  --smtp-domain=<FQDN>::
>  	Specifies the Fully Qualified Domain Name (FQDN) used in the
> diff --git a/git-send-email.perl b/git-send-email.perl
> index bda5211f0d..3f125bc2b8 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -495,8 +495,12 @@ sub read_config {
>  
>  # 'default' encryption is none -- this only prevents a warning
>  $smtp_encryption = '' unless (defined $smtp_encryption);
> -if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
> -	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
> +if ($smtp_encryption eq "tls") {
> +	# "tls" is an alias for starttls for legacy reasons
> +	$smtp_encryption = "starttls";
> +};

Needless trailing ";".

This and the preceding patch would be more readable if it was
re-arranged in some way as to not rewrite the newly introduced lines
between 2 and 3, maybe:

{
	my $tls_name = "tls";
        if (....)
}

Then you'd only need to change "tls" to "starttls" there.

> +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "starttls") {
> +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'starttls', or nothing.\n");
>  }
>  
>  # Set CC suppressions
> @@ -1541,7 +1545,7 @@ sub send_message {
>  						 Hello => $smtp_domain,
>  						 Debug => $debug_net_smtp,
>  						 Port => $smtp_server_port);
> -			if ($smtp_encryption eq 'tls' && $smtp) {
> +			if ($smtp_encryption eq 'starttls' && $smtp) {

And this could use the same variable.

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 11 2021, Drew DeVault wrote:

> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
>  Documentation/git-send-email.txt | 4 ++--
>  git-send-email.perl              | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index c17c3b400a..520b355e50 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -171,8 +171,8 @@ Sending
>  	Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables
>  	generic SSL/TLS support and is typically used on port 465.  'tls'
>  	enables in-band STARTTLS support and is typically used on port 25 or
> -	587.  Use whichever option is recommended by your mail provider.  Any
> -	other value reverts to plain SMTP.  Default is the value of
> +	587.  Use whichever option is recommended by your mail provider.  Leave
> +	empty to disable encryption and use plain SMTP.  Default is the value of
>  	`sendemail.smtpEncryption`.
>  
>  --smtp-domain=<FQDN>::
> diff --git a/git-send-email.perl b/git-send-email.perl
> index f5bbf1647e..bda5211f0d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -495,6 +495,9 @@ sub read_config {
>  
>  # 'default' encryption is none -- this only prevents a warning
>  $smtp_encryption = '' unless (defined $smtp_encryption);
> +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
> +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
> +}

Having not tested this but just eyeballed the code, I'm fairly sure
you're adding a logic error here, or is $smtp_encryption guaranteed to
be defined at this point?

We start with it as undef, then read the config, then the CLI
options. If we've got neither it'll still be undef here, no?

Thus the string comparison will emit a warning.

Maybe I'm overly used to regexen in Perl, but I'd also find this more
readable as:

    $smtp_encryption !~ /^(?:|ssl|tls)$/s

Or something like:

    my @valid_smtp_encryption = ('', qw(ssl tls));
    if (!grep { $_ eq $smtp_encryption } @valid_smtp_encryption) {
        ....
    }

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Drew DeVault]

On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote:
> >  # 'default' encryption is none -- this only prevents a warning
> >  $smtp_encryption = '' unless (defined $smtp_encryption);
> > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
> > +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
> > +}
>
> Having not tested this but just eyeballed the code, I'm fairly sure
> you're adding a logic error here, or is $smtp_encryption guaranteed to
> be defined at this point?

I will admit to being ignorant of much of Perl's semantics, but I had
assumed that the line prior to my additions addresses this:

$smtp_encryption = '' unless (defined $smtp_encryption);

> $smtp_encryption !~ /^(?:|ssl|tls)$/s

Yeah, that would probably be better.

Re: [PATCH v2 3/3] git-send-email: rename 'tls' to 'starttls'

[Drew DeVault]

On Sun Apr 11, 2021 at 10:17 AM EDT, Ævar Arnfjörð Bjarmason wrote:
> >  # 'default' encryption is none -- this only prevents a warning
> >  $smtp_encryption = '' unless (defined $smtp_encryption);
> > -if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
> > -	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
> > +if ($smtp_encryption eq "tls") {
> > +	# "tls" is an alias for starttls for legacy reasons
> > +	$smtp_encryption = "starttls";
> > +};
>
> Needless trailing ";".
>
> This and the preceding patch would be more readable if it was
> re-arranged in some way as to not rewrite the newly introduced lines
> between 2 and 3, maybe:
>
> {
> my $tls_name = "tls";
> if (....)
> }

I disagree that this would be an improvement. It would make the patches
a bit more readlable on their own, but the resulting code would
introduce this bizzare variable which doesn't make sense out of context.

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 11 2021, Drew DeVault wrote:

> On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote:
>> >  # 'default' encryption is none -- this only prevents a warning
>> >  $smtp_encryption = '' unless (defined $smtp_encryption);
>> > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
>> > +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
>> > +}
>>
>> Having not tested this but just eyeballed the code, I'm fairly sure
>> you're adding a logic error here, or is $smtp_encryption guaranteed to
>> be defined at this point?
>
> I will admit to being ignorant of much of Perl's semantics, but I had
> assumed that the line prior to my additions addresses this:
>
> $smtp_encryption = '' unless (defined $smtp_encryption);

You're right, I just misread the diff. Nevermind.

[PATCH 0/2] send-email: simplify smtp.{smtpssl,smtpencryption} parsing

[Ævar Arnfjörð Bjarmason]

As noted in 1/2 I unintentionally broke the deprecated
sendemail.smtpssl configuration parsing a while ago. Since nothing
actually uses it let's remove it.

This doesn't conflict with Drew's
http://lore.kernel.org/git/20210411125431.28971-1-sir@cmpwn.com
series, but as I'll reply to there knowing that we can do this might
simplify some things for it, if it were to be based on top of this.

Ævar Arnfjörð Bjarmason (2):
  send-email: remove non-working support for "sendemail.smtpssl"
  send-email: refactor sendemail.smtpencryption config parsing

 Documentation/config/sendemail.txt |  3 ---
 git-send-email.perl                | 13 +------------
 2 files changed, 1 insertion(+), 15 deletions(-)

-- 
2.31.1.623.g88b15a793d

[PATCH 1/2] send-email: remove non-working support for "sendemail.smtpssl"

[Ævar Arnfjörð Bjarmason]

Remove the already dead code to support "sendemail.smtssl" by finally
removing the dead code supporting the configuration option.

In f6bebd121ac (git-send-email: add support for TLS via
Net::SMTP::SSL, 2008-06-25) the --smtp-ssl command-line option was
documented as deprecated, later in 65180c66186 (List send-email config
options in config.txt., 2009-07-22) the "sendemail.smtpssl"
configuration option was also documented as such.

Then in in 3ff15040e22 (send-email: fix regression in
sendemail.identity parsing, 2019-05-17) I unintentionally removed
support for it by introducing a bug in read_config().

As can be seen from the diff context we've already returned unless
$enc i defined, so it's not possible for us to reach the "elsif"
branch here. This code was therefore already dead since Git v2.23.0.

So let's just remove it instead of fixing the bug, clearly nobody's
cared enough to complain.

The --smtp-ssl option is still deprecated, if someone cares they can
follow-up and remove that too, but unlike the config option that one
could still be in use in the wild. I'm just removing this code that's
provably unused already.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/sendemail.txt | 3 ---
 git-send-email.perl                | 6 +-----
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt
index cbc5af42fdf..50baa5d6bfb 100644
--- a/Documentation/config/sendemail.txt
+++ b/Documentation/config/sendemail.txt
@@ -8,9 +8,6 @@ sendemail.smtpEncryption::
 	See linkgit:git-send-email[1] for description.  Note that this
 	setting is not subject to the 'identity' mechanism.
 
-sendemail.smtpssl (deprecated)::
-	Deprecated alias for 'sendemail.smtpEncryption = ssl'.
-
 sendemail.smtpsslcertpath::
 	Path to ca-certificates (either a directory or a single file).
 	Set it to an empty string to disable certificate verification.
diff --git a/git-send-email.perl b/git-send-email.perl
index f5bbf1647e3..877c7dd1a21 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -374,11 +374,7 @@ sub read_config {
 		my $enc = Git::config(@repo, $setting);
 		return unless defined $enc;
 		return if $configured->{$setting}++;
-		if (defined $enc) {
-			$smtp_encryption = $enc;
-		} elsif (Git::config_bool(@repo, "$prefix.smtpssl")) {
-			$smtp_encryption = 'ssl';
-		}
+		$smtp_encryption = $enc;
 	}
 }
 
-- 
2.31.1.623.g88b15a793d

[PATCH 2/2] send-email: refactor sendemail.smtpencryption config parsing

[Ævar Arnfjörð Bjarmason]

With the removal of the support for sendemail.smtpssl in the preceding
commit the parsing of sendemail.smtpencryption is no longer special,
and can by moved to %config_settings.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 877c7dd1a21..da28c6e8b4b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -268,6 +268,7 @@ sub do_edit {
 );
 
 my %config_settings = (
+    "smtpencryption" => \$smtp_encryption,
     "smtpserver" => \$smtp_server,
     "smtpserverport" => \$smtp_server_port,
     "smtpserveroption" => \@smtp_server_options,
@@ -368,14 +369,6 @@ sub read_config {
 			$$target = $v;
 		}
 	}
-
-	if (!defined $smtp_encryption) {
-		my $setting = "$prefix.smtpencryption";
-		my $enc = Git::config(@repo, $setting);
-		return unless defined $enc;
-		return if $configured->{$setting}++;
-		$smtp_encryption = $enc;
-	}
 }
 
 # sendemail.identity yields to --identity. We must parse this
-- 
2.31.1.623.g88b15a793d

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 11 2021, Ævar Arnfjörð Bjarmason wrote:

> On Sun, Apr 11 2021, Drew DeVault wrote:
>
>> On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote:
>>> >  # 'default' encryption is none -- this only prevents a warning
>>> >  $smtp_encryption = '' unless (defined $smtp_encryption);
>>> > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
>>> > +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
>>> > +}
>>>
>>> Having not tested this but just eyeballed the code, I'm fairly sure
>>> you're adding a logic error here, or is $smtp_encryption guaranteed to
>>> be defined at this point?
>>
>> I will admit to being ignorant of much of Perl's semantics, but I had
>> assumed that the line prior to my additions addresses this:
>>
>> $smtp_encryption = '' unless (defined $smtp_encryption);
>
> You're right, I just misread the diff. Nevermind.

So on a second reading.

So first, I've been sitting on some fairly extensive send-email patches
locally, but have been trying to focus on re-rolling some of my
outstanding stuff.

But I just sent two patches directly relevant to this series as
https://lore.kernel.org/git/cover-0.2-00000000000-20210411T144128Z-avarab@gmail.com/

Something felt a bit wrong about the approach in your series, I wasn't
quite sure what initially, but here it is;

So, the only reason we have that "encryption is none -- this only
prevents a warning" so late in the file (as opposed to setting it to ''
when we declare the variable) was because of the
smtp.{smtpssl,smtpencryption} interaction, i.e. we relied on it being
undef to see if we needed to parse the secondary variable.

But with it gone with my small series (it already didn't work) we can
get rid of that special case.

But, on the specifics of the "felt funny":

 1. Your 2/3 changes a long standing existing "any other value = no
    encryption" to "die on unrecognized". I happen to think this is
    probably a good idea, but let's be explicit in the commit message,
    e.g.:

        We don't think it's a good idea to silently degrade to
        non-encrypted as we've been promising just because your version
        doesn't support something, let's die instead.

 2. If we're breaking the "any other value" we should not be documenting
    the "or nothing", the distinction between "" and undef on the
    Perl-level was just a leaky implementation detail.

    But let's not conflate that with how we present something to the
    user. It's not the same to not set a variable v.s. setting it to the
    empty string.

    With my 2-part series it's even more trivial to detect that, but
    even on top of master you just move your check above the "set to
    empty unless defined".

 3. While I'm very much leaning to #1 being a good idea, I'm very much
    leaning towards introducing this "starttls" alias being a bad idea
    for the same reason.
    
    I.e. let's not create a new 'starttls' if we can avoid it explicitly
    because we used to have the long-standing "anything unrecognized is
    empty == no encryption" behavior.

    A lot of users read documentation for the latest version online, but
    may have an older version installed.

    To the extent that anyone cares about the transport security of
    git-send-email (I'm kind of "meh" on it, but if we're making
    sendemail.smtpEncryption parsing strict you probably disagree), then
    such silent downgrading seems worse than just not accepting starttls
    at all. I.e. to have a new behavior of something like:

        if (defined $smtp_encryption) {
        	die "we call 'starttls' 'tls' for historical reasons, sorry!" if $smtp_encryption eq 'starttls';
		die "unknown mode '$smtp_encryption'" unless $smtp_encryption =~ /^(?:ssl|tls)$/s;
	} else {
		$smtp_encryption = '';
	}

    I.e. I get that it's confusing, but isn't it enough to address the
    TLS v.s. STARTTLS confusion in the docs, as opposed to supporting it
    in the config format, which as noted will silently downgrade on
    older versions?

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Drew DeVault]

On Sun Apr 11, 2021 at 11:06 AM EDT, Ævar Arnfjörð Bjarmason wrote:
> 3. While I'm very much leaning to #1 being a good idea, I'm very much
> leaning towards introducing this "starttls" alias being a bad idea
> for the same reason.
>     
> i.e. let's not create a new 'starttls' if we can avoid it explicitly
> because we used to have the long-standing "anything unrecognized is
> empty == no encryption" behavior.
>
> A lot of users read documentation for the latest version online, but
> may have an older version installed.

I feel quite strongly that the options here are a grave failure of
usability, and that it needs to be corrected. I help people troubleshoot
git send-email problems quite often, and this is a recurring error.
However, you make a good point in that someone might see some online
documentation which does not match their git version and end up with a
surprisingly unencrypted connection.

As a compromise, let's consider making this a gradual change. We can
start by clarifying the docs and forbiding the use of any value other
than 'ssl' or 'tls'. If an unknown value is set, the user is not getting
the encryption they expected anyway, and this should cause an error.

Then we can leave the issue aside for some agreed upon period of time to
allow the change to proliferate in the ecosystem, and then revisit this
at some point in the future to rename the options to make more sense.

Does this seem like a reasonable compromise?

Re: [PATCH 1/2] send-email: remove non-working support for "sendemail.smtpssl"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> So let's just remove it instead of fixing the bug, clearly nobody's
> cared enough to complain.

Hmph, is that a safe assumption?  They may have just assumed that
you did not break it and kept using plaintext without knowing?  If
we do not give a warning when sending over an unencrypted channel in
red flashing letters, that is more likely explanation than nobody
caring that we saw no breakage reports, no?

Re: [PATCH 1/2] send-email: remove non-working support for "sendemail.smtpssl"

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 11 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> So let's just remove it instead of fixing the bug, clearly nobody's
>> cared enough to complain.
>
> Hmph, is that a safe assumption?  They may have just assumed that
> you did not break it and kept using plaintext without knowing?  If
> we do not give a warning when sending over an unencrypted channel in
> red flashing letters, that is more likely explanation than nobody
> caring that we saw no breakage reports, no?

Maybe, I think in either case this patch series makes senes. We were
already 11 years into a stated deprecation period of that variable, now
it's 13.

If we're going to e.g. emit some notice about it I think the parsing
simplification this series gives us makes sense, we can always add a
trivial patch on top to make it die if it sees the old variable.

I don't think that's needed, do you?

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 11 2021, Drew DeVault wrote:

> On Sun Apr 11, 2021 at 11:06 AM EDT, Ævar Arnfjörð Bjarmason wrote:
>> 3. While I'm very much leaning to #1 being a good idea, I'm very much
>> leaning towards introducing this "starttls" alias being a bad idea
>> for the same reason.
>>     
>> i.e. let's not create a new 'starttls' if we can avoid it explicitly
>> because we used to have the long-standing "anything unrecognized is
>> empty == no encryption" behavior.
>>
>> A lot of users read documentation for the latest version online, but
>> may have an older version installed.
>
> I feel quite strongly that the options here are a grave failure of
> usability, and that it needs to be corrected. I help people troubleshoot
> git send-email problems quite often, and this is a recurring error.
> However, you make a good point in that someone might see some online
> documentation which does not match their git version and end up with a
> surprisingly unencrypted connection.
>
> As a compromise, let's consider making this a gradual change. We can
> start by clarifying the docs and forbiding the use of any value other
> than 'ssl' or 'tls'. If an unknown value is set, the user is not getting
> the encryption they expected anyway, and this should cause an error.
>
> Then we can leave the issue aside for some agreed upon period of time to
> allow the change to proliferate in the ecosystem, and then revisit this
> at some point in the future to rename the options to make more sense.
>
> Does this seem like a reasonable compromise?

I suggest we don't compromise and just go with whatever you're OK with
:)

I really don't care enough about #1 and #3 in my E-Mail to in any way
push for it, sorry if it came off that way.

I just wanted to check your assumptions when reviewing the series. I do
think that it would make sense to more prominently note something to the
effect of "this was documented to do X all along, now we do Y, but
that's OK because ABC", and to note why the new starttls = plaintext on
older versions is OK, maybe it's just fine. I really don't know.

Isn't it pretty common in any case that SMTP servers in the wild just
refuse plaintext these days when dealing with auth'd connections? I
don't know.

I do think it makes sense to fixup for my suggested #2, i.e. not leaking
the internal detail of the "empty string".

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Drew DeVault]

On Sun Apr 11, 2021 at 3:56 PM EDT, Ævar Arnfjörð Bjarmason wrote:
> I suggest we don't compromise and just go with whatever you're OK with :)

Well, if you're giving me an opportunity to not drag this out into a
multi-phase rollout, then I'll take it :)

Another option is to forbid an unknown value (which is almost certainly
(1) wrong and (2) causing users to unexpectedly use plaintext when they
expected encryption), file a CVE, and pitch it as a security fix - then
we can expect a reasonably quick rollout of the change to the ecosystem
at large.

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 12 2021, Drew DeVault wrote:

> On Sun Apr 11, 2021 at 3:56 PM EDT, Ævar Arnfjörð Bjarmason wrote:
>> I suggest we don't compromise and just go with whatever you're OK with :)
>
> Well, if you're giving me an opportunity to not drag this out into a
> multi-phase rollout, then I'll take it :)

Just to be clear even if I was insisting on that I'm still just one guy
on the ML reviewing your patch.

As a first approximation the opinion of regular contributors counts for
more when the topic is some tricky interaction of code they wrote/are
familiar with.

In this case we're just discussing the general interaction of security,
optional switches, software versioning and how SMTP servers in the wild
work.

I'd think someone who e.g. needs to regularly deal with SMTP servers in
the wild would have a much better idea of those trade-offs than someone
(like me) who happens to have some existing patches in git.git to
git-send-email.perl.

> Another option is to forbid an unknown value (which is almost certainly
> (1) wrong and (2) causing users to unexpectedly use plaintext when they
> expected encryption), file a CVE, and pitch it as a security fix - then
> we can expect a reasonably quick rollout of the change to the ecosystem
> at large.

I think anyone would agree that in retrospect "unknown is plaintext" for
the "what encryption do you want" option is at best a something
approaching a shotgun to your foot of a UI pattern.

But I think it falls far short of a CVE. We *do* prominently document
it, a potential CVE would be if we had silent degration to plaintext
(well, in a mode whose inherent workings aren't to be vulnerable to that
attack, as STARTTLS is...).

FWIW since my upthread <87zgy4egtp.fsf@evledraar.gmail.com> I tried
sending mail through GMail's plain-text smtp gateway as an authenticated
user.

Testing with:

    nc smtp.gmail.com 25
    openssl s_client -connect smtp.gmail.com:465

It will emit a 530 if you try to AUTH in plain-text (telling you to use
STARTTLS), it will also only say "AUTH" in the EHLO response to the
latter.

And indeed Net::SMTP picks up on this, and doesn't even send your
user/password:
https://metacpan.org/release/libnet/source/lib/Net/SMTP.pm#L169

So this hypothetical degradation of the connection and sending auth over
plain-text I suggested in upthread #3 seems to mostly/entirely be a
non-issue as far as e.g. accidentally sending your password on some open
WiFi network goes due to a local misconfiguration.

As long as the SMTP server is functional enough to say it doesn't
support AUTH on plain-text you'll be OK. I'm assuming that these days
with the push for "SSL everywhere" most/all big providers/MTAs have
moved away from supporing plain-text auth by default.

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Drew DeVault]

Can I get one of the maintainers to chime in on this thread and explain,
in their opinion, what this patchset needs before it is acceptable? I'm
not sure where I should go from this discussion.

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 13 2021, Drew DeVault wrote:

> Can I get one of the maintainers to chime in on this thread and explain,
> in their opinion, what this patchset needs before it is acceptable? I'm
> not sure where I should go from this discussion.

Git just has the one maintainer, Junio C Hamano.

Ultimately getting your patch in is up to his whimsy.

Maybe he'll reply, but in an attempt to save him some time (which I
understand I'm taking too much of these days):

Getting your patch in is ultimately up to you.

So you submitted a patch, got some feedback/review.

Through some combination of this thread (mainly
<875z0sg8t9.fsf@evledraar.gmail.com> and
<87o8ejej8m.fsf@evledraar.gmail.com>) I suggested making some changes in a v3.

I.e. cleaning up some of the semantics (config docs/handling leaking
implementation details) and improving the commit message(s) to summarize
the trade-offs, why this approach is safe/isn't safe/why it's OK in the
cases it's not etc.

So, whatever Junio or anyone else thinks of my opinion I think it's fair
to say that at this point he's most likely to skim this thread and see
that there's some outstanding feedback that hasn't been addressed.

"Addressed" means either re-rolling into a v3, or deciding that nothing
(or only part of the feedback) needs to be changed and/or
addressed.

Both/some of those/that are perfectly acceptable approaches, but in
either case making things easily digestable to Junio will help your
series along.

Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

[Junio C Hamano]

"Drew DeVault" <sir@cmpwn.com> writes:

> Can I get one of the maintainers to chime in on this thread and explain,
> in their opinion, what this patchset needs before it is acceptable? I'm
> not sure where I should go from this discussion.

What you called "compromise" in the upthread didn't even smelled
like a compromise but the safest way forward.  My reasoning goes
(thinking aloud, so that you and Ævar can correct me if I am talking
nonsense and discount my input based on it):

 - If we were designing this from scratch, we would have called the
   smtps:// tunnelled transport SSL/TLS and in-place upgrade
   transport STARTTLS, like e-mail providers and client programs do,
   but unfortunately we didn't.  We ended up with 'ssl' vs 'tls'.

 - We could introduce and advertise STARTTLS as a synonym to 'tls',
   but then those whose send-email does not understand STARTTLS but
   read about the new way of spelling would end up having no
   encryption due to another earlier mistake we made, i.e. an
   unrecognised option value silently turns into no encryption.

 - To avoid the above problem, the first phase is not to change the
   status quo that 'ssl' vs 'tls' are the only two choices.  What we
   do is to make the program error out if we see an unrecognised
   value given to the option.  We release this to the wild, and wait
   for the current versions of send-email that turns unrecognised
   words into no-encryption die out.  It may take several years,
   though.

 - After waiting, we add 'starttls' as a synonym to 'tls'.  We may
   also add 'ssl/tls' as a synonym to 'ssl'.  Unfortunately 'tls'
   alone cannot be repurposed as a synonym for smtps:// without
   another deprecation dance, and it is not in scope of the
   transtion.

Am I on the same page as you two?  

Re: [PATCH 1/2] send-email: remove non-working support for "sendemail.smtpssl"

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 11 2021, Ævar Arnfjörð Bjarmason wrote:

> On Sun, Apr 11 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> So let's just remove it instead of fixing the bug, clearly nobody's
>>> cared enough to complain.
>>
>> Hmph, is that a safe assumption?  They may have just assumed that
>> you did not break it and kept using plaintext without knowing?  If
>> we do not give a warning when sending over an unencrypted channel in
>> red flashing letters, that is more likely explanation than nobody
>> caring that we saw no breakage reports, no?
>
> Maybe, I think in either case this patch series makes senes. We were
> already 11 years into a stated deprecation period of that variable, now
> it's 13.
>
> If we're going to e.g. emit some notice about it I think the parsing
> simplification this series gives us makes sense, we can always add a
> trivial patch on top to make it die if it sees the old variable.
>
> I don't think that's needed, do you?

Junio: *Poke*. Was going thorugh my outstanding patches, I still think
it makes sense to just pick this up. Especially with the related
discussion later about how common in-the-wild service providers would
just not support AUTH non-encrypted, so in practice I think it's even
less likely that anyone saw silent breakage as a result of this
already-deprecated variable being ignored.