[PATCH v2 0/2] Squelch warning from send-email

[PATCH v2 0/2] Squelch warning from send-email

[Ramkumar Ramachandra]

Hi,

Since nobody stepped up to write it, I wrote [2/2] myself.  It will be
tested when I send out this series.

Thanks.

Ramkumar Ramachandra (2):
  send-email: squelch warning from Net::SMTP::SSL
  send-email: introduce sendemail.smtpsslcertpath

 Documentation/config.txt         |  3 +++
 Documentation/git-send-email.txt |  4 ++++
 git-send-email.perl              | 20 ++++++++++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

-- 
1.8.3.2.722.g3244e19.dirty

[PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL

[Ramkumar Ramachandra]

Due to a recent change in the Net::SMTP::SSL module, send-email emits
the following ugly warning everytime a email is sent via SSL:

*******************************************************************
 Using the default of SSL_verify_mode of SSL_VERIFY_NONE for client
 is deprecated! Please set SSL_verify_mode to SSL_VERIFY_PEER
 together with SSL_ca_file|SSL_ca_path for verification.
 If you really don't want to verify the certificate and keep the
 connection open to Man-In-The-Middle attacks please set
 SSL_verify_mode explicitly to SSL_VERIFY_NONE in your application.
*******************************************************************

Fix this by explicitly specifying SSL_verify_mode => SSL_VERIFY_NONE in
Net::SMTP::SSL->start_SSL().

Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-send-email.perl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index ecbf56f..758100d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1193,10 +1193,12 @@ X-Mailer: git-send-email $gitversion
 						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
+				use IO::Socket::SSL qw(SSL_VERIFY_NONE);
 				$smtp->command('STARTTLS');
 				$smtp->response();
 				if ($smtp->code == 220) {
-					$smtp = Net::SMTP::SSL->start_SSL($smtp)
+					$smtp = Net::SMTP::SSL->start_SSL($smtp,
+									  SSL_verify_mode => SSL_VERIFY_NONE)
 						or die "STARTTLS failed! ".$smtp->message;
 					$smtp_encryption = '';
 					# Send EHLO again to receive fresh
-- 
1.8.3.2.723.gad7967b.dirty

Re: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL

[Torsten Bögershausen]

On 2013-07-05 14.05, Ramkumar Ramachandra wrote:
> Due to a recent change in the Net::SMTP::SSL module, send-email emits
> the following ugly warning everytime a email is sent via SSL:
> 
> *******************************************************************
>  Using the default of SSL_verify_mode of SSL_VERIFY_NONE for client
>  is deprecated! Please set SSL_verify_mode to SSL_VERIFY_PEER
>  together with SSL_ca_file|SSL_ca_path for verification.
>  If you really don't want to verify the certificate and keep the
>  connection open to Man-In-The-Middle attacks please set
>  SSL_verify_mode explicitly to SSL_VERIFY_NONE in your application.
> *******************************************************************
> 
> Fix this by explicitly specifying SSL_verify_mode => SSL_VERIFY_NONE in
> Net::SMTP::SSL->start_SSL().
> 
> Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  git-send-email.perl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index ecbf56f..758100d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1193,10 +1193,12 @@ X-Mailer: git-send-email $gitversion
>  						 Debug => $debug_net_smtp);
>  			if ($smtp_encryption eq 'tls' && $smtp) {
>  				require Net::SMTP::SSL;
> +				use IO::Socket::SSL qw(SSL_VERIFY_NONE);
>  				$smtp->command('STARTTLS');
>  				$smtp->response();
>  				if ($smtp->code == 220) {
> -					$smtp = Net::SMTP::SSL->start_SSL($smtp)
> +					$smtp = Net::SMTP::SSL->start_SSL($smtp,
> +									  SSL_verify_mode => SSL_VERIFY_NONE)
>  						or die "STARTTLS failed! ".$smtp->message;
>  					$smtp_encryption = '';
>  					# Send EHLO again to receive fresh
> 
Hm, this doesn't work on my system, and t9001 fails:

"SSL_VERIFY_PEER" is not exported by the IO::Socket::SSL module
 "SSL_VERIFY_NONE" is not exported by the IO::Socket::SSL module
Can't continue after import errors at /Users/tb/projects/git/git.pu/git-send-email line 1090
/Torsten

Re: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL

[brian m. carlson]

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

On Sat, Jul 06, 2013 at 04:28:00PM +0200, Torsten Bögershausen wrote:
> On 2013-07-05 14.05, Ramkumar Ramachandra wrote:
> > Due to a recent change in the Net::SMTP::SSL module, send-email emits
> > the following ugly warning everytime a email is sent via SSL:
> > 
> > *******************************************************************
> >  Using the default of SSL_verify_mode of SSL_VERIFY_NONE for client
> >  is deprecated! Please set SSL_verify_mode to SSL_VERIFY_PEER
> >  together with SSL_ca_file|SSL_ca_path for verification.
> >  If you really don't want to verify the certificate and keep the
> >  connection open to Man-In-The-Middle attacks please set
> >  SSL_verify_mode explicitly to SSL_VERIFY_NONE in your application.
> > *******************************************************************
> > 
> > Fix this by explicitly specifying SSL_verify_mode => SSL_VERIFY_NONE in
> > Net::SMTP::SSL->start_SSL().
> > 
> > Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> > ---
> >  git-send-email.perl | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index ecbf56f..758100d 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1193,10 +1193,12 @@ X-Mailer: git-send-email $gitversion
> >  						 Debug => $debug_net_smtp);
> >  			if ($smtp_encryption eq 'tls' && $smtp) {
> >  				require Net::SMTP::SSL;
> > +				use IO::Socket::SSL qw(SSL_VERIFY_NONE);
> >  				$smtp->command('STARTTLS');
> >  				$smtp->response();
> >  				if ($smtp->code == 220) {
> > -					$smtp = Net::SMTP::SSL->start_SSL($smtp)
> > +					$smtp = Net::SMTP::SSL->start_SSL($smtp,
> > +									  SSL_verify_mode => SSL_VERIFY_NONE)
> >  						or die "STARTTLS failed! ".$smtp->message;
> >  					$smtp_encryption = '';
> >  					# Send EHLO again to receive fresh
> > 
> Hm, this doesn't work on my system, and t9001 fails:
> 
> "SSL_VERIFY_PEER" is not exported by the IO::Socket::SSL module
>  "SSL_VERIFY_NONE" is not exported by the IO::Socket::SSL module
> Can't continue after import errors at /Users/tb/projects/git/git.pu/git-send-email line 1090

What version of IO::Socket::SSL do you have, and what source did you get
it from?

perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'

This should be available in 1.31 or later.  It might end up that we need
to adjust the use/require statement to require 1.31.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL

[Torsten Bögershausen]

On 2013-07-06 16.32, brian m. carlson wrote:
> perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'

Mac OS X, 10.6:
(I think this perl we use for git:)
/usr/bin/perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'
1.22

(And this is in my path:)
 which perl
/opt/local/bin/perl

perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'
Can't locate IO/Socket/SSL.pm in @INC (@INC contains: /sw/lib/perl5 /sw/lib/perl5/darwin /opt/local/lib/perl5/site_perl/5.8.9/darwin-2level /opt/local/lib/perl5/site_perl/5.8.9 /opt/local/lib/perl5/site_perl /opt/local/lib/perl5/vendor_perl/5.8.9/darwin-2level /opt/local/lib/perl5/vendor_perl/5.8.9 /opt/local/lib/perl5/vendor_perl /opt/local/lib/perl5/5.8.9/darwin-2level /opt/local/lib/perl5/5.8.9 .).

/Torsten

Re: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL

[Ramkumar Ramachandra]

Torsten Bögershausen wrote:
> /usr/bin/perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'
> 1.22

This is ancient!  (I have 1.84).  Is it not possible to do an
ssl-verify-peer in older versions (is it exported as something else)?
The older versions don't display the warning anyway, and this series
is about squelching the warning in newer versions.  Does

  require IO::Socket::SSL qw(SSL_VERIFY_NONE SSL_VERIFY_PEER) or print
"warning: not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL"

sound reasonable?

Re: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL

[brian m. carlson]

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

On Sun, Jul 14, 2013 at 07:19:10PM +0530, Ramkumar Ramachandra wrote:
> Torsten Bögershausen wrote:
> > /usr/bin/perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'
> > 1.22
> 
> This is ancient!  (I have 1.84).  Is it not possible to do an
> ssl-verify-peer in older versions (is it exported as something else)?
> The older versions don't display the warning anyway, and this series
> is about squelching the warning in newer versions.  Does
> 
>   require IO::Socket::SSL qw(SSL_VERIFY_NONE SSL_VERIFY_PEER) or print
> "warning: not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL"

require doesn't take a list of symbols to import, and the import dies if
it fails.  You need:

require IO::Socket::SSL;
eval {
	IO::Socket::SSL->import(qw(SSL_VERIFY_NONE SSL_VERIFY_PEER));
};
if ($@) {
	warn "Not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL\n";
	# Do something different here.
}

I didn't stick the require in the eval because git-send-email will fail
in this case anyway if you don't have it, since Net::SMTP::SSL requires
it.  Let me know if you want a patch for this on top of the existing two
in this series and I'll provide one.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL

[Ramkumar Ramachandra]

brian m. carlson wrote:
> I didn't stick the require in the eval because git-send-email will fail
> in this case anyway if you don't have it, since Net::SMTP::SSL requires
> it.  Let me know if you want a patch for this on top of the existing two
> in this series and I'll provide one.

Yeah, that'd be nice.  I'm not enjoying this Perl-wrestling very much.

Re: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL

[Torsten Bögershausen]

On 2013-07-14 19.03, brian m. carlson wrote:
> On Sun, Jul 14, 2013 at 07:19:10PM +0530, Ramkumar Ramachandra wrote:
>> Torsten Bögershausen wrote:
>>> /usr/bin/perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'
>>> 1.22
>>
>> This is ancient!  (I have 1.84).  Is it not possible to do an
>> ssl-verify-peer in older versions (is it exported as something else)?
>> The older versions don't display the warning anyway, and this series
>> is about squelching the warning in newer versions.  Does
>>
>>   require IO::Socket::SSL qw(SSL_VERIFY_NONE SSL_VERIFY_PEER) or print
>> "warning: not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL"
> 
> require doesn't take a list of symbols to import, and the import dies if
> it fails.  You need:
> 
> require IO::Socket::SSL;
> eval {
> 	IO::Socket::SSL->import(qw(SSL_VERIFY_NONE SSL_VERIFY_PEER));
> };
> if ($@) {
> 	warn "Not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL\n";
> 	# Do something different here.
> }
> 
> I didn't stick the require in the eval because git-send-email will fail
> in this case anyway if you don't have it, since Net::SMTP::SSL requires
> it.  Let me know if you want a patch for this on top of the existing two
> in this series and I'll provide one.
> 
Please send a patch if possible.
I can volonteer to test it here, but it can take a couple of days to respond.

Re: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

>> I didn't stick the require in the eval because git-send-email will fail
>> in this case anyway if you don't have it, since Net::SMTP::SSL requires
>> it.  Let me know if you want a patch for this on top of the existing two
>> in this series and I'll provide one.
>> 
> Please send a patch if possible.
> I can volonteer to test it here, but it can take a couple of days to respond.

Thanks all.

[PATCH] send-email: improve SSL certificate verification

[brian m. carlson]

The SSL and TLS code for SMTP is non-trivial, so refactor it into a separate
function for ease of use.  Handle both files and directories as sources for CA
certificates.  Also add handling for older version of IO::Socket::SSL that do
not support the SSL_VERIFY_PEER and SSL_VERIFY_NONE constants; in this case,
print a warning and inform the user of this fact.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---

This is completely untested.  I used perl -c, but that's it.

 git-send-email.perl | 50 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 095b6fb..11fb2d0 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1083,6 +1083,37 @@ sub smtp_auth_maybe {
 	return $auth;
 }
 
+sub ssl_verify_params {
+	require IO::Socket::SSL;
+	eval {
+		IO::Socket::SSL->import(qw/SSL_VERIFY_PEER SSL_VERIFY_NONE/);
+	};
+	if ($@) {
+		print STDERR "Not using SSL_VERIFY_PEER due to out-of-date IO::Socket::SSL.\n";
+		return;
+	}
+
+	if (!defined $smtp_ssl_cert_path) {
+		$smtp_ssl_cert_path ||= "/etc/ssl/certs";
+	}
+
+	if (!$smtp_ssl_cert_path) {
+		return (SSL_verify_mode => SSL_VERIFY_NONE());
+	}
+	elsif (-d $smtp_ssl_cert_path) {
+		return (SSL_verify_mode => SSL_VERIFY_PEER(),
+			SSL_ca_path => $smtp_ssl_cert_path);
+	}
+	elsif (-f $smtp_ssl_cert_path) {
+		return (SSL_verify_mode => SSL_VERIFY_PEER(),
+			SSL_ca_file => $smtp_ssl_cert_path);
+	}
+	else {
+		print STDERR "Not using SSL_VERIFY_PEER because the CA path does not exist.\n";
+		return (SSL_verify_mode => SSL_VERIFY_NONE());
+	}
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -1187,7 +1218,8 @@ X-Mailer: git-send-email $gitversion
 			$smtp_domain ||= maildomain();
 			$smtp ||= Net::SMTP::SSL->new($smtp_server,
 						      Hello => $smtp_domain,
-						      Port => $smtp_server_port);
+						      Port => $smtp_server_port,
+							  ssl_verify_params());
 		}
 		else {
 			require Net::SMTP;
@@ -1203,19 +1235,9 @@ X-Mailer: git-send-email $gitversion
 				$smtp->command('STARTTLS');
 				$smtp->response();
 				if ($smtp->code == 220) {
-					# Attempt to use a ca-certificate by default
-					$smtp_ssl_cert_path |= "/etc/ssl/certs";
-					if (-d $smtp_ssl_cert_path) {
-						$smtp = Net::SMTP::SSL->start_SSL($smtp,
-										  SSL_verify_mode => SSL_VERIFY_PEER,
-										  SSL_ca_path => $smtp_ssl_cert_path)
-							or die "STARTTLS failed! ".$smtp->message;
-					} else {
-						print STDERR "warning: Using SSL_VERIFY_NONE.  See sendemail.smtpsslcertpath.\n";
-						$smtp = Net::SMTP::SSL->start_SSL($smtp,
-										  SSL_verify_mode => SSL_VERIFY_NONE)
-							or die "STARTTLS failed! ".$smtp->message;
-					}
+					$smtp = Net::SMTP::SSL->start_SSL($smtp,
+									  ssl_verify_params())
+						or die "STARTTLS failed! ".$smtp->message;
 					$smtp_encryption = '';
 					# Send EHLO again to receive fresh
 					# supported commands
-- 
1.8.3.2.923.g2a18ff8.dirty


-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

Re: [PATCH] send-email: improve SSL certificate verification

[Torsten Bögershausen]

[snip]
I wasn't sure where to apply the patch, so I manually copy/paste it
on top of pu:
commit 6b1ca0f4d443ee8716857b871b0513ae85c9f112
Merge: bce90ab f351fcf

Thanks, t9001 passes on Mac OS X 10.6.
To be sure I didn't messed it up, please see the diff below.
When it shows up on pu, I can re-test of course.



diff --git a/git-send-email.perl b/git-send-email.perl
index a9a6661..a965b8e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -28,7 +28,7 @@ use File::Temp qw/ tempdir tempfile /;
 use File::Spec::Functions qw(catfile);
 use Error qw(:try);
 use Git;
-use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
+#use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
 
 Getopt::Long::Configure qw/ pass_through /;
 
@@ -1092,19 +1092,34 @@ sub smtp_auth_maybe {
 # Helper to come up with SSL/TLS certification validation params
 # and warn when doing no verification
 sub ssl_verify_params {
-    if ($smtp_ssl_verify == 0) {
-        return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_NONE);
+    require IO::Socket::SSL;
+    eval {
+        IO::Socket::SSL->import(qw/SSL_VERIFY_PEER SSL_VERIFY_NONE/);
+    };
+    if ($@) {
+        print STDERR "Not using SSL_VERIFY_PEER due to out-of-date IO::Socket::SSL.\n";
+        return;
     }
 
-    if (! defined $smtp_ssl_cert_path) {
-        return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_PEER);
-    } elsif (-f $smtp_ssl_cert_path) {
-        return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_PEER,
-            SSL_ca_file => $smtp_ssl_cert_path);
-    } else {
-        return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_PEER,
+    if (!defined $smtp_ssl_cert_path) {
+        $smtp_ssl_cert_path ||= "/etc/ssl/certs";
+    }
+
+    if (!$smtp_ssl_cert_path) {
+        return (SSL_verify_mode => SSL_VERIFY_NONE());
+    }
+    elsif (-d $smtp_ssl_cert_path) {
+        return (SSL_verify_mode => SSL_VERIFY_PEER(),
             SSL_ca_path => $smtp_ssl_cert_path);
     }
+    elsif (-f $smtp_ssl_cert_path) {
+        return (SSL_verify_mode => SSL_VERIFY_PEER(),
+            SSL_ca_file => $smtp_ssl_cert_path);
+    }
+    else {
+        print STDERR "Not using SSL_VERIFY_PEER because the CA path does not exist.\n";
+        return (SSL_verify_mode => SSL_VERIFY_NONE());
+    }
 }
 
 # Returns 1 if the message was sent, and 0 otherwise.
@@ -1229,13 +1244,8 @@ X-Mailer: git-send-email $gitversion
                 if ($smtp->code == 220) {
                     $smtp = Net::SMTP::SSL->start_SSL($smtp,
                                       ssl_verify_params())
-                        or die "STARTTLS failed! ".$smtp->message;
-                    $smtp_encryption = '';
-                    # Send EHLO again to receive fresh
-                    # supported commands
-                    $smtp->hello($smtp_domain);
-                } else {
-                    die "Server does not support STARTTLS! ".$smtp->message;
+                        or die "STARTTLS failed! ".$smtp->message;
+
                 }
             }
         }

Re: [PATCH] send-email: improve SSL certificate verification

[brian m. carlson]

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

On Tue, Jul 16, 2013 at 04:33:55AM +0200, Torsten Bögershausen wrote:
> [snip]
> I wasn't sure where to apply the patch, so I manually copy/paste it
> on top of pu:
> commit 6b1ca0f4d443ee8716857b871b0513ae85c9f112
> Merge: bce90ab f351fcf
> 
> Thanks, t9001 passes on Mac OS X 10.6.
> To be sure I didn't messed it up, please see the diff below.
> When it shows up on pu, I can re-test of course.

Yeah, that looks fine.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re* [PATCH] send-email: improve SSL certificate verification

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> I wasn't sure where to apply the patch, so I manually copy/paste it
> on top of pu:
> commit 6b1ca0f4d443ee8716857b871b0513ae85c9f112
> Merge: bce90ab f351fcf
>
> Thanks, t9001 passes on Mac OS X 10.6.
> To be sure I didn't messed it up, please see the diff below.
> When it shows up on pu, I can re-test of course.

As the history of rr/send-email-ssl-verify needs rewriting to squash
this change in, here is a single patch with which I would propose to
replace all the commits accumulated on that branch.

Ramkumar, as you will still be the primary author of the resulting
commit, I tentatively added a line for your sign-off even though you
haven't signed off _this_ version yet, so that I would not forget.
If this version looks good, please tell us you would.

Torsten, I have Tested-by with your name, again so that I would not
forget, but obviously this one hasn't been tested by you yet.  If
this tests OK, please tell us so.

Thanks.

-- >8 --
From: Ramkumar Ramachandra <artagnon@gmail.com>
Subject: send-email: be explicit with SSL certificate verification

When initiating an SSL connection without explicitly specifying the
SSL certificate verification mode, Net::SMTP::SSL defaults to no
verification, but recent versions of the module gives a warning
against this use of the default.

Enable certificate verification by default, using /etc/ssl/certs as
the default path for certificates of certificate authorities.  This
path can be overriden by the --smtp-ssl-cert-path command line
option and the sendemail.smtpSSLCertPath configuration variable.

Passing an empty string as the path for CA certificates path disables
the SSL certificate verification explicitly, which does not trigger
the warning from recent versions of Net::SMTP::SSL.

(PROVISO) Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Helped-by: Brian M. Carlson <sandals@crustytoothpaste.net>
(PROVISO) Tested-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt         |  4 ++++
 Documentation/git-send-email.txt |  6 ++++++
 git-send-email.perl              | 41 +++++++++++++++++++++++++++++++++++++---
 3 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e53fc5..4de154c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2022,6 +2022,10 @@ sendemail.smtpencryption::
 sendemail.smtpssl::
 	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.
+
 sendemail.<identity>.*::
 	Identity-specific versions of the 'sendemail.*' parameters
 	found below, taking precedence over those when the this
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 40a9a9a..f0e57a5 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -198,6 +198,12 @@ must be used for each option.
 --smtp-ssl::
 	Legacy alias for '--smtp-encryption ssl'.
 
+--smtp-ssl-cert-path::
+	Path to ca-certificates (either a directory or a single file).
+	Set it to an empty string to disable certificate verification.
+	Defaults to the value set to the 'sendemail.smtpsslcertpath'
+	configuration variable, if set, or `/etc/ssl/certs` otherwise.
+
 --smtp-user=<user>::
 	Username for SMTP-AUTH. Default is the value of 'sendemail.smtpuser';
 	if a username is not specified (with '--smtp-user' or 'sendemail.smtpuser'),
diff --git a/git-send-email.perl b/git-send-email.perl
index bd13cc8..60eaed3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -69,6 +69,9 @@ sub usage {
     --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
     --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
+    --smtp-ssl-cert-path    <str>  * Path to ca-certificates (either directory or file).
+                                     Pass an empty string to disable certificate
+                                     verification.
     --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake
     --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
 
@@ -194,7 +197,7 @@ sub do_edit {
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
 my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, @smtp_server_options);
-my ($smtp_authuser, $smtp_encryption);
+my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
 my ($identity, $aliasfiletype, @alias_files, $smtp_domain);
 my ($validate, $confirm);
 my (@suppress_cc);
@@ -222,6 +225,7 @@ sub do_edit {
     "smtpserveroption" => \@smtp_server_options,
     "smtpuser" => \$smtp_authuser,
     "smtppass" => \$smtp_authpass,
+    "smtpsslcertpath" => \$smtp_ssl_cert_path,
     "smtpdomain" => \$smtp_domain,
     "to" => \@initial_to,
     "tocmd" => \$to_cmd,
@@ -302,6 +306,7 @@ sub signal_handler {
 		    "smtp-pass:s" => \$smtp_authpass,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
+		    "smtp-ssl-cert-path" => \$smtp_ssl_cert_path,
 		    "smtp-debug:i" => \$debug_net_smtp,
 		    "smtp-domain:s" => \$smtp_domain,
 		    "identity=s" => \$identity,
@@ -1089,6 +1094,34 @@ sub smtp_auth_maybe {
 	return $auth;
 }
 
+sub ssl_verify_params {
+	eval {
+		require IO::Socket::SSL;
+		IO::Socket::SSL->import(qw/SSL_VERIFY_PEER SSL_VERIFY_NONE/);
+	};
+	if ($@) {
+		print STDERR "Not using SSL_VERIFY_PEER due to out-of-date IO::Socket::SSL.\n";
+		return;
+	}
+
+	if (!defined $smtp_ssl_cert_path) {
+		$smtp_ssl_cert_path = "/etc/ssl/certs";
+	}
+
+	if ($smtp_ssl_cert_path eq "") {
+		return (SSL_verify_mode => SSL_VERIFY_NONE());
+	} elsif (-d $smtp_ssl_cert_path) {
+		return (SSL_verify_mode => SSL_VERIFY_PEER(),
+			SSL_ca_path => $smtp_ssl_cert_path);
+	} elsif (-f $smtp_ssl_cert_path) {
+		return (SSL_verify_mode => SSL_VERIFY_PEER(),
+			SSL_ca_file => $smtp_ssl_cert_path);
+	} else {
+		print STDERR "Not using SSL_VERIFY_PEER because the CA path does not exist.\n";
+		return (SSL_verify_mode => SSL_VERIFY_NONE());
+	}
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -1194,7 +1227,8 @@ sub send_message {
 			$smtp_domain ||= maildomain();
 			$smtp ||= Net::SMTP::SSL->new($smtp_server,
 						      Hello => $smtp_domain,
-						      Port => $smtp_server_port);
+						      Port => $smtp_server_port,
+						      ssl_verify_params());
 		}
 		else {
 			require Net::SMTP;
@@ -1207,7 +1241,8 @@ sub send_message {
 				$smtp->command('STARTTLS');
 				$smtp->response();
 				if ($smtp->code == 220) {
-					$smtp = Net::SMTP::SSL->start_SSL($smtp)
+					$smtp = Net::SMTP::SSL->start_SSL($smtp,
+									  ssl_verify_params())
 						or die "STARTTLS failed! ".$smtp->message;
 					$smtp_encryption = '';
 					# Send EHLO again to receive fresh
-- 
1.8.3.3-992-gf0e5e44

Re: Re* [PATCH] send-email: improve SSL certificate verification

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar, as you will still be the primary author of the resulting
> commit, I tentatively added a line for your sign-off even though you
> haven't signed off _this_ version yet, so that I would not forget.
> If this version looks good, please tell us you would.

Yeah, it seems to work fine for me.  Sign-off is fine.

Thanks for putting it together.

[PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Ramkumar Ramachandra]

Use the ca-certificates in /etc/ssl/certs by default (that's where most
distributions put it).  SSL_VERIFY_NONE is now the fallback mode.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt         |  3 +++
 Documentation/git-send-email.txt |  4 ++++
 git-send-email.perl              | 22 ++++++++++++++++++----
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b4d4887..b216a63 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2048,6 +2048,9 @@ sendemail.smtpencryption::
 sendemail.smtpssl::
 	Deprecated alias for 'sendemail.smtpencryption = ssl'.
 
+sendemail.smtpsslcertpath::
+	Path to ca-certificates.  Defaults to `/etc/ssl/certs`.
+
 sendemail.<identity>.*::
 	Identity-specific versions of the 'sendemail.*' parameters
 	found below, taking precedence over those when the this
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 40a9a9a..605f263 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -198,6 +198,10 @@ must be used for each option.
 --smtp-ssl::
 	Legacy alias for '--smtp-encryption ssl'.
 
+--smtp-ssl-cert-path::
+	Path to ca-certificates.  Defaults to `/etc/ssl/certs`, or
+	'sendemail.smtpsslcertpath'.
+
 --smtp-user=<user>::
 	Username for SMTP-AUTH. Default is the value of 'sendemail.smtpuser';
 	if a username is not specified (with '--smtp-user' or 'sendemail.smtpuser'),
diff --git a/git-send-email.perl b/git-send-email.perl
index 758100d..026bcbc 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -69,6 +69,8 @@ git send-email [options] <file | directory | rev-list options >
     --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
     --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
+    --smtp-ssl-cert-path    <str>  * Path to ca-certificates.  Defaults to
+                                     /etc/ssl/certs.
     --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake
     --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
 
@@ -195,6 +197,7 @@ my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
 my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, @smtp_server_options);
 my ($smtp_authuser, $smtp_encryption);
+my ($smtp_ssl_cert_path);
 my ($identity, $aliasfiletype, @alias_files, $smtp_domain);
 my ($validate, $confirm);
 my (@suppress_cc);
@@ -220,6 +223,7 @@ my %config_settings = (
     "smtpserveroption" => \@smtp_server_options,
     "smtpuser" => \$smtp_authuser,
     "smtppass" => \$smtp_authpass,
+    "smtpsslcertpath" => \$smtp_ssl_cert_path,
     "smtpdomain" => \$smtp_domain,
     "to" => \@initial_to,
     "tocmd" => \$to_cmd,
@@ -1193,13 +1197,23 @@ X-Mailer: git-send-email $gitversion
 						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
-				use IO::Socket::SSL qw(SSL_VERIFY_NONE);
+				use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
 				$smtp->command('STARTTLS');
 				$smtp->response();
 				if ($smtp->code == 220) {
-					$smtp = Net::SMTP::SSL->start_SSL($smtp,
-									  SSL_verify_mode => SSL_VERIFY_NONE)
-						or die "STARTTLS failed! ".$smtp->message;
+					# Attempt to use a ca-certificate by default
+					$smtp_ssl_cert_path |= "/etc/ssl/certs";
+					if (-d $smtp_ssl_cert_path) {
+						$smtp = Net::SMTP::SSL->start_SSL($smtp,
+										  SSL_verify_mode => SSL_VERIFY_PEER,
+										  SSL_ca_path => $smtp_ssl_cert_path)
+							or die "STARTTLS failed! ".$smtp->message;
+					} else {
+						print STDERR "warning: Using SSL_VERIFY_NONE.  See sendemail.smtpsslcertpath.\n";
+						$smtp = Net::SMTP::SSL->start_SSL($smtp,
+										  SSL_verify_mode => SSL_VERIFY_NONE)
+							or die "STARTTLS failed! ".$smtp->message;
+					}
 					$smtp_encryption = '';
 					# Send EHLO again to receive fresh
 					# supported commands
-- 
1.8.3.2.723.gad7967b.dirty

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Eric Sunshine]

On Fri, Jul 5, 2013 at 8:05 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Use the ca-certificates in /etc/ssl/certs by default (that's where most
> distributions put it).  SSL_VERIFY_NONE is now the fallback mode.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 758100d..026bcbc 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1193,13 +1197,23 @@ X-Mailer: git-send-email $gitversion
>                                                  Debug => $debug_net_smtp);
>                         if ($smtp_encryption eq 'tls' && $smtp) {
>                                 require Net::SMTP::SSL;
> -                               use IO::Socket::SSL qw(SSL_VERIFY_NONE);
> +                               use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
>                                 $smtp->command('STARTTLS');
>                                 $smtp->response();
>                                 if ($smtp->code == 220) {
> -                                       $smtp = Net::SMTP::SSL->start_SSL($smtp,
> -                                                                         SSL_verify_mode => SSL_VERIFY_NONE)
> -                                               or die "STARTTLS failed! ".$smtp->message;
> +                                       # Attempt to use a ca-certificate by default
> +                                       $smtp_ssl_cert_path |= "/etc/ssl/certs";

You're going to want to use logical ||= here. Bitwise |= on a string
does not do what you expect[1]:

  my $s = '/usr/local/etc/ssl/certs';
  $s |= '/etc/ssl/certs';
  print $s, "\n";

Outputs: /uws/oooowts/ssl/certs

[1]: http://perldoc.perl.org/perlop.html#Bitwise-String-Operators

> +                                       if (-d $smtp_ssl_cert_path) {
> +                                               $smtp = Net::SMTP::SSL->start_SSL($smtp,
> +                                                                                 SSL_verify_mode => SSL_VERIFY_PEER,
> +                                                                                 SSL_ca_path => $smtp_ssl_cert_path)
> +                                                       or die "STARTTLS failed! ".$smtp->message;
> +                                       } else {
> +                                               print STDERR "warning: Using SSL_VERIFY_NONE.  See sendemail.smtpsslcertpath.\n";
> +                                               $smtp = Net::SMTP::SSL->start_SSL($smtp,
> +                                                                                 SSL_verify_mode => SSL_VERIFY_NONE)
> +                                                       or die "STARTTLS failed! ".$smtp->message;
> +                                       }
>                                         $smtp_encryption = '';
>                                         # Send EHLO again to receive fresh
>                                         # supported commands

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Ramkumar Ramachandra]

Eric Sunshine wrote:
>> +                                       $smtp_ssl_cert_path |= "/etc/ssl/certs";
>
> You're going to want to use logical ||= here. Bitwise |= on a string
> does not do what you expect[1]:
>
>   my $s = '/usr/local/etc/ssl/certs';
>   $s |= '/etc/ssl/certs';
>   print $s, "\n";
>
> Outputs: /uws/oooowts/ssl/certs

Thanks; much appreciated.  My Perl is quite weak.

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[brian m. carlson]

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

On Fri, Jul 05, 2013 at 05:35:47PM +0530, Ramkumar Ramachandra wrote:
> @@ -1193,13 +1197,23 @@ X-Mailer: git-send-email $gitversion
>  						 Debug => $debug_net_smtp);
>  			if ($smtp_encryption eq 'tls' && $smtp) {
>  				require Net::SMTP::SSL;
> -				use IO::Socket::SSL qw(SSL_VERIFY_NONE);
> +				use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
>  				$smtp->command('STARTTLS');
>  				$smtp->response();
>  				if ($smtp->code == 220) {
> -					$smtp = Net::SMTP::SSL->start_SSL($smtp,
> -									  SSL_verify_mode => SSL_VERIFY_NONE)
> -						or die "STARTTLS failed! ".$smtp->message;
> +					# Attempt to use a ca-certificate by default
> +					$smtp_ssl_cert_path |= "/etc/ssl/certs";
> +					if (-d $smtp_ssl_cert_path) {
> +						$smtp = Net::SMTP::SSL->start_SSL($smtp,
> +										  SSL_verify_mode => SSL_VERIFY_PEER,
> +										  SSL_ca_path => $smtp_ssl_cert_path)
> +							or die "STARTTLS failed! ".$smtp->message;
> +					} else {
> +						print STDERR "warning: Using SSL_VERIFY_NONE.  See sendemail.smtpsslcertpath.\n";
> +						$smtp = Net::SMTP::SSL->start_SSL($smtp,
> +										  SSL_verify_mode => SSL_VERIFY_NONE)
> +							or die "STARTTLS failed! ".$smtp->message;
> +					}

You've covered the STARTTLS case, but not the SSL one right above it.
Someone using smtps on port 465 will still see the warning.  You can
pass SSL_verify_mode to Net::SMTP::SSL->new just like you pass it to
start_SSL.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Ramkumar Ramachandra]

brian m. carlson wrote:
> You've covered the STARTTLS case, but not the SSL one right above it.
> Someone using smtps on port 465 will still see the warning.  You can
> pass SSL_verify_mode to Net::SMTP::SSL->new just like you pass it to
> start_SSL.

Thanks.  On a related note, how do I find out about all these things?  I tried

  $ perldoc Net::SMTP::SSL

but it was completely useless.  The only reason I got this far is
because you literally told me what to do.  Do I have to resort to
reading the sources?  If so, how do I bring up the relevant functions
quickly? (I'm looking for something like the elisp M-x find-function).

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[brian m. carlson]

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

On Fri, Jul 05, 2013 at 06:23:58PM +0530, Ramkumar Ramachandra wrote:
> Thanks.  On a related note, how do I find out about all these things?  I tried
> 
>   $ perldoc Net::SMTP::SSL
> 
> but it was completely useless.  The only reason I got this far is
> because you literally told me what to do.  Do I have to resort to
> reading the sources?  If so, how do I bring up the relevant functions
> quickly? (I'm looking for something like the elisp M-x find-function).

Since Net::SMTP::SSL inherits from IO::Socket::SSL, you can do "perldoc
IO::Socket::SSL", which is significantly more helpful.  If you're
looking for the source of a module, use "perldoc -lm Net::SMTP::SSL";
that will print the .pm file for the module.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> You've covered the STARTTLS case, but not the SSL one right above it.
> Someone using smtps on port 465 will still see the warning.  You can
> pass SSL_verify_mode to Net::SMTP::SSL->new just like you pass it to
> start_SSL.

OK, will a fix-up look like this on top of 1/2 and 2/2?

 git-send-email.perl | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 52028ba..3b80340 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1093,6 +1093,25 @@ sub smtp_auth_maybe {
 	return $auth;
 }
 
+# Helper to come up with SSL/TLS certification validation params
+# and warn when doing no verification
+sub ssl_verify_params {
+	use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
+
+	if (!defined $smtp_ssl_cert_path) {
+		$smtp_ssl_cert_path = "/etc/ssl/certs";
+	}
+
+	if (-d $smtp_ssl_cert_path) {
+		return (SSL_verify_mode => SSL_VERIFY_PEER,
+			SSL_ca_path => $smtp_ssl_cert_path);
+	} else {
+		print STDERR "warning: Using SSL_VERIFY_NONE.  " .
+		    "See sendemail.smtpsslcertpath.\n";
+		return (SSL_verify_mode => SSL_VERIFY_NONE);
+	}
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -1195,12 +1214,11 @@ sub send_message {
 		if ($smtp_encryption eq 'ssl') {
 			$smtp_server_port ||= 465; # ssmtp
 			require Net::SMTP::SSL;
-			use IO::Socket::SSL qw(SSL_VERIFY_NONE);
 			$smtp_domain ||= maildomain();
 			$smtp ||= Net::SMTP::SSL->new($smtp_server,
 						      Hello => $smtp_domain,
 						      Port => $smtp_server_port,
-						      SSL_verify_mode => SSL_VERIFY_NONE);
+						      ssl_verify_params());
 		}
 		else {
 			require Net::SMTP;
@@ -1210,23 +1228,12 @@ sub send_message {
 						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
-				use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
 				$smtp->command('STARTTLS');
 				$smtp->response();
 				if ($smtp->code == 220) {
-					# Attempt to use a ca-certificate by default
-					$smtp_ssl_cert_path ||= "/etc/ssl/certs";
-					if (-d $smtp_ssl_cert_path) {
-						$smtp = Net::SMTP::SSL->start_SSL($smtp,
-										  SSL_verify_mode => SSL_VERIFY_PEER,
-										  SSL_ca_path => $smtp_ssl_cert_path)
-							or die "STARTTLS failed! ".$smtp->message;
-					} else {
-						print STDERR "warning: Using SSL_VERIFY_NONE.  See sendemail.smtpsslcertpath.\n";
-						$smtp = Net::SMTP::SSL->start_SSL($smtp,
-										  SSL_verify_mode => SSL_VERIFY_NONE)
-							or die "STARTTLS failed! ".$smtp->message;
-					}
+					$smtp = Net::SMTP::SSL->start_SSL($smtp,
+									  ssl_verify_params())
+					    or die "STARTTLS failed! ".$smtp->message;
 					$smtp_encryption = '';
 					# Send EHLO again to receive fresh
 					# supported commands

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[John Keeping]

On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > You've covered the STARTTLS case, but not the SSL one right above it.
> > Someone using smtps on port 465 will still see the warning.  You can
> > pass SSL_verify_mode to Net::SMTP::SSL->new just like you pass it to
> > start_SSL.
> 
> OK, will a fix-up look like this on top of 1/2 and 2/2?

According to IO::Socket::SSL [1], if neither SSL_ca_file nor SSL_ca_path
is specified then builtin defaults will be used, so I wonder if we
should pass SSL_VERIFY_PEER regardless (possibly with a switch for
SSL_VERIFY_NONE if people really need that).

[1] http://search.cpan.org/~sullr/IO-Socket-SSL-1.951/lib/IO/Socket/SSL.pm

>  git-send-email.perl | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 52028ba..3b80340 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1093,6 +1093,25 @@ sub smtp_auth_maybe {
>  	return $auth;
>  }
>  
> +# Helper to come up with SSL/TLS certification validation params
> +# and warn when doing no verification
> +sub ssl_verify_params {
> +	use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
> +
> +	if (!defined $smtp_ssl_cert_path) {
> +		$smtp_ssl_cert_path = "/etc/ssl/certs";
> +	}
> +
> +	if (-d $smtp_ssl_cert_path) {
> +		return (SSL_verify_mode => SSL_VERIFY_PEER,
> +			SSL_ca_path => $smtp_ssl_cert_path);
> +	} else {
> +		print STDERR "warning: Using SSL_VERIFY_NONE.  " .
> +		    "See sendemail.smtpsslcertpath.\n";
> +		return (SSL_verify_mode => SSL_VERIFY_NONE);
> +	}
> +}
> +

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>> 
>> > You've covered the STARTTLS case, but not the SSL one right above it.
>> > Someone using smtps on port 465 will still see the warning.  You can
>> > pass SSL_verify_mode to Net::SMTP::SSL->new just like you pass it to
>> > start_SSL.
>> 
>> OK, will a fix-up look like this on top of 1/2 and 2/2?
>
> According to IO::Socket::SSL [1], if neither SSL_ca_file nor SSL_ca_path
> is specified then builtin defaults will be used, so I wonder if we
> should pass SSL_VERIFY_PEER regardless (possibly with a switch for
> SSL_VERIFY_NONE if people really need that).
>
> [1] http://search.cpan.org/~sullr/IO-Socket-SSL-1.951/lib/IO/Socket/SSL.pm

Interesting.  That frees us from saying "we assume /etc/ssl/cacerts
is the default location, and let the users override it".

To help those "I do not want verification because I know my server
does not present valid certificate, I know my server is internal and
trustable, and I do not bother to fix it" people, we can let them
specify an empty string (or any non-directory) as the CACertPath,
and structure the code like so?

        if (defined $smtp_ssl_cert_path && -d $smtp_ssl_cert_path) {
                return (SSL_verify_mode => SSL_VERIFY_PEER,
                        SSL_ca_path => $smtp_ssl_cert_path);
        } elsif (defined $smtp_ssl_cert_path) {
                return (SSL_verify_mode => SSL_VERIFY_NONE);
        } else {
                return (SSL_verify_mode => SSL_VERIFY_PEER);
        }

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[John Keeping]

On Fri, Jul 05, 2013 at 11:30:11AM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote:
> >> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >> 
> >> > You've covered the STARTTLS case, but not the SSL one right above it.
> >> > Someone using smtps on port 465 will still see the warning.  You can
> >> > pass SSL_verify_mode to Net::SMTP::SSL->new just like you pass it to
> >> > start_SSL.
> >> 
> >> OK, will a fix-up look like this on top of 1/2 and 2/2?
> >
> > According to IO::Socket::SSL [1], if neither SSL_ca_file nor SSL_ca_path
> > is specified then builtin defaults will be used, so I wonder if we
> > should pass SSL_VERIFY_PEER regardless (possibly with a switch for
> > SSL_VERIFY_NONE if people really need that).
> >
> > [1] http://search.cpan.org/~sullr/IO-Socket-SSL-1.951/lib/IO/Socket/SSL.pm
> 
> Interesting.  That frees us from saying "we assume /etc/ssl/cacerts
> is the default location, and let the users override it".
> 
> To help those "I do not want verification because I know my server
> does not present valid certificate, I know my server is internal and
> trustable, and I do not bother to fix it" people, we can let them
> specify an empty string (or any non-directory) as the CACertPath,
> and structure the code like so?
> 
>         if (defined $smtp_ssl_cert_path && -d $smtp_ssl_cert_path) {
>                 return (SSL_verify_mode => SSL_VERIFY_PEER,
>                         SSL_ca_path => $smtp_ssl_cert_path);
>         } elsif (defined $smtp_ssl_cert_path) {
>                 return (SSL_verify_mode => SSL_VERIFY_NONE);
>         } else {
>                 return (SSL_verify_mode => SSL_VERIFY_PEER);
>         }

I'd rather have '$smtp_ssl_cert_path ne ""' in the first if condition
(instead of the '-d $smtp_ssl_cert_path') but that seems reasonable and
agrees with my reading of the documentation.

Perhaps a complete solution could allow CA files as well:

	if (defined $smtp_ssl_cert_path) {
		if ($smtp_ssl_cert_path eq "") {
			return (SSL_verify_mode => SSL_VERIFY_NONE);
		} elsif (-f $smtp_ssl_cert_path) {
			return (SSL_verify_mode => SSL_VERIFY_PEER,
				SSL_ca_file => $smtp_ssl_cert_path);
		} else {
			return (SSL_verify_mode => SSL_VERIFY_PEER,
				SSL_ca_path => $smtp_ssl_cert_path);
		}
	} else {
		return (SSL_verify_mode => SSL_VERIFY_PEER);
	}

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> I'd rather have '$smtp_ssl_cert_path ne ""' in the first if condition
> (instead of the '-d $smtp_ssl_cert_path') ...

I agree.  The signal for "no certs" should be an explicit "nonsense"
value like an empty string, not just a string that does not name an
expected filesystem object.  Otherwise people can misspell paths and
disable the validation by accident.

> Perhaps a complete solution could allow CA files as well.

Yes, that would be a good idea.  Care to roll into a "fixup!" patch
against [2/2]?

> 	if (defined $smtp_ssl_cert_path) {
> 		if ($smtp_ssl_cert_path eq "") {
> 			return (SSL_verify_mode => SSL_VERIFY_NONE);
> 		} elsif (-f $smtp_ssl_cert_path) {
> 			return (SSL_verify_mode => SSL_VERIFY_PEER,
> 				SSL_ca_file => $smtp_ssl_cert_path);
> 		} else {
> 			return (SSL_verify_mode => SSL_VERIFY_PEER,
> 				SSL_ca_path => $smtp_ssl_cert_path);
> 		}
> 	} else {
> 		return (SSL_verify_mode => SSL_VERIFY_PEER);
> 	}

Two things that worry me a bit are:

 (1) At the end user UI level, it may look nice to accept some form
     of --no-option-name to say "I have been using SSL against my
     server with handrolled cert, and I want to keep using the
     verify-none option"; "--ssl-cert-path=" looks somewhat ugly.
     The same goes for [sendemail] ssl_cert_path = "" config.

 (2) How loudly does the new code barf when no configuration is done
     (i.e. we just pass SSL_VERIFY_PEER and let the system default
     CA path to take effect) and the server cert does not validate?
     The warning that triggered this thread, if we had the
     configuration mechanism we have been designing together, would
     have been a good reminder for the user to use it, but would we
     give a similar (less noisy is fine, as long as it is clear)
     diagnostic message?

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[John Keeping]

On Fri, Jul 05, 2013 at 11:25:36PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > I'd rather have '$smtp_ssl_cert_path ne ""' in the first if condition
> > (instead of the '-d $smtp_ssl_cert_path') ...
> 
> I agree.  The signal for "no certs" should be an explicit "nonsense"
> value like an empty string, not just a string that does not name an
> expected filesystem object.  Otherwise people can misspell paths and
> disable the validation by accident.
> 
> > Perhaps a complete solution could allow CA files as well.
> 
> Yes, that would be a good idea.  Care to roll into a "fixup!" patch
> against [2/2]?

Here's a patch that should do that.  However, when testing this I
couldn't get the "SSL_verify_mode" warning to disappear and
git-send-email kept connecting to my untrusted server - this was using
the SSL code path not the TLS upgrade one.

I think this is caused by the SSL_verify_mode argument not getting all
the way down to the configure_SSL function in IO::Socket::SSL but I
can't see what the code in git-send-email is doing wrong.  Can any Perl
experts point out what's going wrong?

Also, I tried Brian's "IO::Socket::SSL->import(qw(SSL_VERIFY_PEER
SSL_VERIFY_NONE));" but that produced a warning message about the uses
of SSL_VERIFY_PEER and SSL_VERIFY_NONE following that statement, so I
ended up qualifying each use in the code below.

-- >8 --
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 605f263..b56c215 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -198,6 +198,10 @@ must be used for each option.
 --smtp-ssl::
 	Legacy alias for '--smtp-encryption ssl'.
 
+--smtp-ssl-verify::
+--no-smtp-ssl-verify::
+	Enable SSL certificate verification.  Defaults to on.
+
 --smtp-ssl-cert-path::
 	Path to ca-certificates.  Defaults to `/etc/ssl/certs`, or
 	'sendemail.smtpsslcertpath'.
diff --git a/git-send-email.perl b/git-send-email.perl
index 3b80340..cbe85aa 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -69,8 +69,10 @@ git send-email [options] <file | directory | rev-list options >
     --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
     --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
+    --[no-]smtp-ssl-verify         * Enable SSL certificate verification.
+                                     Default on.
     --smtp-ssl-cert-path    <str>  * Path to ca-certificates.  Defaults to
-                                     /etc/ssl/certs.
+                                     a platform-specific value.
     --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake
     --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
 
@@ -197,7 +199,7 @@ my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
 my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, @smtp_server_options);
 my ($smtp_authuser, $smtp_encryption);
-my ($smtp_ssl_cert_path);
+my ($smtp_ssl_verify, $smtp_ssl_cert_path);
 my ($identity, $aliasfiletype, @alias_files, $smtp_domain);
 my ($validate, $confirm);
 my (@suppress_cc);
@@ -214,6 +216,7 @@ my %config_bool_settings = (
     "suppressfrom" => [\$suppress_from, undef],
     "signedoffbycc" => [\$signed_off_by_cc, undef],
     "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
+    "smtpsslverify" => [\$smtp_ssl_verify, 1],
     "validate" => [\$validate, 1],
     "multiedit" => [\$multiedit, undef],
     "annotate" => [\$annotate, undef]
@@ -306,6 +309,8 @@ my $rc = GetOptions("h" => \$help,
 		    "smtp-pass:s" => \$smtp_authpass,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
+		    "smtp-ssl-cert-path=s" => \$smtp_ssl_cert_path,
+		    "smtp-ssl-verify!" => \$smtp_ssl_verify,
 		    "smtp-debug:i" => \$debug_net_smtp,
 		    "smtp-domain:s" => \$smtp_domain,
 		    "identity=s" => \$identity,
@@ -1096,19 +1101,18 @@ sub smtp_auth_maybe {
 # Helper to come up with SSL/TLS certification validation params
 # and warn when doing no verification
 sub ssl_verify_params {
-	use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
-
-	if (!defined $smtp_ssl_cert_path) {
-		$smtp_ssl_cert_path = "/etc/ssl/certs";
+	if ($smtp_ssl_verify == 0) {
+		return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_NONE);
 	}
 
-	if (-d $smtp_ssl_cert_path) {
-		return (SSL_verify_mode => SSL_VERIFY_PEER,
-			SSL_ca_path => $smtp_ssl_cert_path);
+	if (! defined $smtp_ssl_cert_path) {
+		return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_PEER);
+	} elsif (-f $smtp_ssl_cert_path) {
+		return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_PEER,
+			SSL_ca_file => $smtp_ssl_cert_path);
 	} else {
-		print STDERR "warning: Using SSL_VERIFY_NONE.  " .
-		    "See sendemail.smtpsslcertpath.\n";
-		return (SSL_verify_mode => SSL_VERIFY_NONE);
+		return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_PEER,
+			SSL_ca_path => $smtp_ssl_cert_path);
 	}
 }
 
-- 8< --

> > 	if (defined $smtp_ssl_cert_path) {
> > 		if ($smtp_ssl_cert_path eq "") {
> > 			return (SSL_verify_mode => SSL_VERIFY_NONE);
> > 		} elsif (-f $smtp_ssl_cert_path) {
> > 			return (SSL_verify_mode => SSL_VERIFY_PEER,
> > 				SSL_ca_file => $smtp_ssl_cert_path);
> > 		} else {
> > 			return (SSL_verify_mode => SSL_VERIFY_PEER,
> > 				SSL_ca_path => $smtp_ssl_cert_path);
> > 		}
> > 	} else {
> > 		return (SSL_verify_mode => SSL_VERIFY_PEER);
> > 	}
> 
> Two things that worry me a bit are:
> 
>  (1) At the end user UI level, it may look nice to accept some form
>      of --no-option-name to say "I have been using SSL against my
>      server with handrolled cert, and I want to keep using the
>      verify-none option"; "--ssl-cert-path=" looks somewhat ugly.
>      The same goes for [sendemail] ssl_cert_path = "" config.
> 
>  (2) How loudly does the new code barf when no configuration is done
>      (i.e. we just pass SSL_VERIFY_PEER and let the system default
>      CA path to take effect) and the server cert does not validate?
>      The warning that triggered this thread, if we had the
>      configuration mechanism we have been designing together, would
>      have been a good reminder for the user to use it, but would we
>      give a similar (less noisy is fine, as long as it is clear)
>      diagnostic message?

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> @@ -1096,19 +1101,18 @@ sub smtp_auth_maybe {
>  # Helper to come up with SSL/TLS certification validation params
>  # and warn when doing no verification
>  sub ssl_verify_params {
> -	use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
> -
> -	if (!defined $smtp_ssl_cert_path) {
> -		$smtp_ssl_cert_path = "/etc/ssl/certs";
> +	if ($smtp_ssl_verify == 0) {
> +		return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_NONE);

I do not see any "use IO::Socket::SSL" anywhere after applying this
patch.  Is this expected to work?

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[John Keeping]

On Sat, Jul 06, 2013 at 09:12:31PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > @@ -1096,19 +1101,18 @@ sub smtp_auth_maybe {
> >  # Helper to come up with SSL/TLS certification validation params
> >  # and warn when doing no verification
> >  sub ssl_verify_params {
> > -	use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
> > -
> > -	if (!defined $smtp_ssl_cert_path) {
> > -		$smtp_ssl_cert_path = "/etc/ssl/certs";
> > +	if ($smtp_ssl_verify == 0) {
> > +		return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_NONE);
> 
> I do not see any "use IO::Socket::SSL" anywhere after applying this
> patch.  Is this expected to work?

I don't get any errors about unknown variables when running it.  Do we
get IO::Socket::SSL imported through Net::SMTP::SSL, which extends it?

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[brian m. carlson]

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

On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote:
> +# Helper to come up with SSL/TLS certification validation params
> +# and warn when doing no verification
> +sub ssl_verify_params {
> +	use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);

You might as well put this at the top of the file, because all use
statements happen at compile time anyway, regardless of their location.
If you want to lazy-load this, you need to do:

require IO::Socket::SSL;
IO::Socket::SSL->import(qw(SSL_VERIFY_PEER SSL_VERIFY_NONE));

which is equivalent to "use" except that it happens at runtime.

Otherwise, it looks fine.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Jeff King]

On Fri, Jul 05, 2013 at 08:29:48PM +0000, brian m. carlson wrote:

> On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote:
> > +# Helper to come up with SSL/TLS certification validation params
> > +# and warn when doing no verification
> > +sub ssl_verify_params {
> > +	use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
> 
> You might as well put this at the top of the file, because all use
> statements happen at compile time anyway, regardless of their location.
> If you want to lazy-load this, you need to do:
> 
> require IO::Socket::SSL;
> IO::Socket::SSL->import(qw(SSL_VERIFY_PEER SSL_VERIFY_NONE));
> 
> which is equivalent to "use" except that it happens at runtime.

I think we _must_ lazy load this, or else we are breaking git-send-email
users on platforms that do not have IO::Socket::SSL (and do not plan on
using SSL themselves).

The same goes for the "use" in patch 1/2.

-Peff

Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 05, 2013 at 08:29:48PM +0000, brian m. carlson wrote:
>
>> On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote:
>> > +# Helper to come up with SSL/TLS certification validation params
>> > +# and warn when doing no verification
>> > +sub ssl_verify_params {
>> > +	use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
>> 
>> You might as well put this at the top of the file, because all use
>> statements happen at compile time anyway, regardless of their location.
>> If you want to lazy-load this, you need to do:
>> 
>> require IO::Socket::SSL;
>> IO::Socket::SSL->import(qw(SSL_VERIFY_PEER SSL_VERIFY_NONE));
>> 
>> which is equivalent to "use" except that it happens at runtime.
>
> I think we _must_ lazy load this, or else we are breaking git-send-email
> users on platforms that do not have IO::Socket::SSL (and do not plan on
> using SSL themselves).
>
> The same goes for the "use" in patch 1/2.

A very good point.  Thanks.

Also it appears that people seem to be seeing different behaviours
depending on the versions of IO::Socket::SSL they have; we may need
to conditionalize what our code does depending on $PACKAGE::Version
after we do that lazy loading.