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

"brian m. carlson" [off-list ref] 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

On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote:

"brian m. carlson" [off-list ref] writes:

quoted

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

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

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:

quoted

+# 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