git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] Remove dependency on deprecated Net::SMTP::SSL
@ 2016-11-20 21:18 Mike Fisher
  2016-11-20 21:53 ` brian m. carlson
  2016-11-21  5:37 ` Torsten Bögershausen
  0 siblings, 2 replies; 4+ messages in thread
From: Mike Fisher @ 2016-11-20 21:18 UTC (permalink / raw)
  To: git

Refactor send_message() to remove dependency on deprecated
Net::SMTP::SSL:

<http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>

Signed-off-by: Mike Fisher <mfisher@csh.rit.edu>
---
  git-send-email.perl | 54 
+++++++++++++++++++++++++----------------------------
  1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index da81be4..fc166c5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1330,15 +1330,17 @@ Message-Id: $message_id
  		print $sm "$header\n$message";
  		close $sm or die $!;
  	} else {
-
  		if (!defined $smtp_server) {
  			die "The required SMTP server is not properly defined."
  		}

+		require Net::SMTP;
+		$smtp_domain ||= maildomain();
+		my $smtp_ssl = 0;
+
  		if ($smtp_encryption eq 'ssl') {
  			$smtp_server_port ||= 465; # ssmtp
-			require Net::SMTP::SSL;
-			$smtp_domain ||= maildomain();
+			$smtp_ssl = 1;
  			require IO::Socket::SSL;

  			# Suppress "variable accessed once" warning.
@@ -1347,37 +1349,31 @@ Message-Id: $message_id
  				$IO::Socket::SSL::DEBUG = 1;
  			}

-			# Net::SMTP::SSL->new() does not forward any SSL options
  			IO::Socket::SSL::set_client_defaults(
  				ssl_verify_params());
-			$smtp ||= Net::SMTP::SSL->new($smtp_server,
-						      Hello => $smtp_domain,
-						      Port => $smtp_server_port,
-						      Debug => $debug_net_smtp);
  		}
  		else {
-			require Net::SMTP;
-			$smtp_domain ||= maildomain();
  			$smtp_server_port ||= 25;
-			$smtp ||= Net::SMTP->new($smtp_server,
-						 Hello => $smtp_domain,
-						 Debug => $debug_net_smtp,
-						 Port => $smtp_server_port);
-			if ($smtp_encryption eq 'tls' && $smtp) {
-				require Net::SMTP::SSL;
-				$smtp->command('STARTTLS');
-				$smtp->response();
-				if ($smtp->code == 220) {
-					$smtp = Net::SMTP::SSL->start_SSL($smtp,
-									  ssl_verify_params())
-						or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
-					$smtp_encryption = '';
-					# Send EHLO again to receive fresh
-					# supported commands
-					$smtp->hello($smtp_domain);
-				} else {
-					die "Server does not support STARTTLS! ".$smtp->message;
-				}
+		}
+
+		$smtp ||= Net::SMTP->new($smtp_server,
+					 Hello => $smtp_domain,
+					 Port => $smtp_server_port,
+					 Debug => $debug_net_smtp,
+					 SSL => $smtp_ssl);
+
+		if ($smtp_encryption eq 'tls' && $smtp) {
+			$smtp->command('STARTTLS');
+			$smtp->response();
+			if ($smtp->code == 220) {
+				$smtp->starttls(ssl_verify_params())
+					or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
+				$smtp_encryption = '';
+				# Send EHLO again to receive fresh
+				# supported commands
+				$smtp->hello($smtp_domain);
+			} else {
+				die "Server does not support STARTTLS! ".$smtp->message;
  			}
  		}

-- 
2.9.3 (Apple Git-75)


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL
  2016-11-20 21:18 [PATCH] Remove dependency on deprecated Net::SMTP::SSL Mike Fisher
@ 2016-11-20 21:53 ` brian m. carlson
  2017-01-13 14:59   ` Renato Botelho
  2016-11-21  5:37 ` Torsten Bögershausen
  1 sibling, 1 reply; 4+ messages in thread
From: brian m. carlson @ 2016-11-20 21:53 UTC (permalink / raw)
  To: Mike Fisher; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

On Sun, Nov 20, 2016 at 04:18:16PM -0500, Mike Fisher wrote:
> Refactor send_message() to remove dependency on deprecated
> Net::SMTP::SSL:
> 
> <http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>

As much as I hate to say this, I think this is going to cause
compatibility problems.  Net::SMTP is part of core Perl (as of v5.7.3),
but the version you want to rely on (which you did not provide an
explicit dependency on) is from October 2014.

That basically means that no Perl on a Red Hat or CentOS system is going
to provide that support, since RHEL 7 was released in June 2014.
Providing an updated Git on those platforms would require replacing the
system Perl or parts of it, which would be undesirable.  This would
affect Debian 7 as well.

We currently support Perl 5.8 [0], so if you want to remove support for
Net::SMTP::SSL, I'd recommend a solution that works with that version.

[0] I personally believe we should drop support for Perl older than
5.10.1 (if not newer), but that's my opinion and it isn't shared by
other list regulars.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL
  2016-11-20 21:18 [PATCH] Remove dependency on deprecated Net::SMTP::SSL Mike Fisher
  2016-11-20 21:53 ` brian m. carlson
@ 2016-11-21  5:37 ` Torsten Bögershausen
  1 sibling, 0 replies; 4+ messages in thread
From: Torsten Bögershausen @ 2016-11-21  5:37 UTC (permalink / raw)
  To: Mike Fisher, git



>On 20/11/16 22:18, Mike Fisher  wrote:

Thanks for contributing to Git.
One comment on the head line:
 >Refactor send_message() to remove dependency on deprecated
Net::SMTP::SSL
The word "refactor" may be used in other way: Re-structure the code,
and use the same API.


"Remove dependency on deprecated Net::SMTP::SSL"

> Refactor send_message() to remove dependency on deprecated
> Net::SMTP::SSL:
Is there a security risk with require Net::SMTP::SSL ?
If yes, the commit message should state this.
If no:
Even if it is deprecated, is it still in use somewhere ?
Does it hurt someone, is there any OS release where the old code doesn't work 
anymore ?
Or is it "only" nice to have ?
Since when does Net::SMTP include Net::SMTP::SSL ?
On which system has the change been tested ?

I think the commit message could and should give more information like this.

My comments may be over-critical.
Lets see if other people from the list know more than me.

>
> <http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>
>
> Signed-off-by: Mike Fisher <mfisher@csh.rit.edu>
> ---
>  git-send-email.perl | 54 +++++++++++++++++++++++++----------------------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index da81be4..fc166c5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1330,15 +1330,17 @@ Message-Id: $message_id
>          print $sm "$header\n$message";
>          close $sm or die $!;
>      } else {
> -
I can see one refactoring, that is the removal of an empty line.

>
>          if (!defined $smtp_server) {
>              die "The required SMTP server is not properly defined."
>          }
>
> +        require Net::SMTP;
> +        $smtp_domain ||= maildomain();
> +        my $smtp_ssl = 0;
> +
>          if ($smtp_encryption eq 'ssl') {
>              $smtp_server_port ||= 465; # ssmtp
> -            require Net::SMTP::SSL;
> -            $smtp_domain ||= maildomain();
> +            $smtp_ssl = 1;
>              require IO::Socket::SSL;
>
>              # Suppress "variable accessed once" warning.
> @@ -1347,37 +1349,31 @@ Message-Id: $message_id
>                  $IO::Socket::SSL::DEBUG = 1;
>              }
>
> -            # Net::SMTP::SSL->new() does not forward any SSL options
>              IO::Socket::SSL::set_client_defaults(
>                  ssl_verify_params());
> -            $smtp ||= Net::SMTP::SSL->new($smtp_server,
> -                              Hello => $smtp_domain,
> -                              Port => $smtp_server_port,
> -                              Debug => $debug_net_smtp);
>          }
>          else {
> -            require Net::SMTP;
> -            $smtp_domain ||= maildomain();
>              $smtp_server_port ||= 25;
> -            $smtp ||= Net::SMTP->new($smtp_server,
> -                         Hello => $smtp_domain,
> -                         Debug => $debug_net_smtp,
> -                         Port => $smtp_server_port);
> -            if ($smtp_encryption eq 'tls' && $smtp) {
> -                require Net::SMTP::SSL;
> -                $smtp->command('STARTTLS');
> -                $smtp->response();
> -                if ($smtp->code == 220) {
> -                    $smtp = Net::SMTP::SSL->start_SSL($smtp,
> -                                      ssl_verify_params())
> -                        or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> -                    $smtp_encryption = '';
> -                    # Send EHLO again to receive fresh
> -                    # supported commands
> -                    $smtp->hello($smtp_domain);
> -                } else {
> -                    die "Server does not support STARTTLS! ".$smtp->message;
> -                }
> +        }
> +
> +        $smtp ||= Net::SMTP->new($smtp_server,
> +                     Hello => $smtp_domain,
> +                     Port => $smtp_server_port,
> +                     Debug => $debug_net_smtp,
> +                     SSL => $smtp_ssl);
> +
> +        if ($smtp_encryption eq 'tls' && $smtp) {
> +            $smtp->command('STARTTLS');
> +            $smtp->response();
> +            if ($smtp->code == 220) {
> +                $smtp->starttls(ssl_verify_params())
> +                    or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> +                $smtp_encryption = '';
> +                # Send EHLO again to receive fresh
> +                # supported commands
> +                $smtp->hello($smtp_domain);
> +            } else {
> +                die "Server does not support STARTTLS! ".$smtp->message;
>              }
>          }
>


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL
  2016-11-20 21:53 ` brian m. carlson
@ 2017-01-13 14:59   ` Renato Botelho
  0 siblings, 0 replies; 4+ messages in thread
From: Renato Botelho @ 2017-01-13 14:59 UTC (permalink / raw)
  To: brian m. carlson; +Cc: Mike Fisher, git

> On 20 Nov 2016, at 19:53, brian m. carlson <sandals@crustytoothpaste.net> wrote:
> 
> On Sun, Nov 20, 2016 at 04:18:16PM -0500, Mike Fisher wrote:
>> Refactor send_message() to remove dependency on deprecated
>> Net::SMTP::SSL:
>> 
>> <http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>
> 
> As much as I hate to say this, I think this is going to cause
> compatibility problems.  Net::SMTP is part of core Perl (as of v5.7.3),
> but the version you want to rely on (which you did not provide an
> explicit dependency on) is from October 2014.
> 
> That basically means that no Perl on a Red Hat or CentOS system is going
> to provide that support, since RHEL 7 was released in June 2014.
> Providing an updated Git on those platforms would require replacing the
> system Perl or parts of it, which would be undesirable.  This would
> affect Debian 7 as well.
> 
> We currently support Perl 5.8 [0], so if you want to remove support for
> Net::SMTP::SSL, I'd recommend a solution that works with that version.
> 
> [0] I personally believe we should drop support for Perl older than
> 5.10.1 (if not newer), but that's my opinion and it isn't shared by
> other list regulars.
> -- 
> brian m. carlson / brian with sandals: Houston, Texas, US
> +1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
> OpenPGP: https://keybase.io/bk2204

Net::SMTP::SSL is marked as DEPRECATED on FreeBSD ports tree and will be removed in 2017-03-31. When it happens users will not be able to run git-send-email anymore. I’m considering to add Mike’s patch to FreeBSD ports tree as an alternative but it would be good to have a official solution for this problem.

FreeBSD bug report can be found at https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214335

--
Renato Botelho


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-20 21:18 [PATCH] Remove dependency on deprecated Net::SMTP::SSL Mike Fisher
2016-11-20 21:53 ` brian m. carlson
2017-01-13 14:59   ` Renato Botelho
2016-11-21  5:37 ` Torsten Bögershausen

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox