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

[John Keeping <john@keeping.me.uk>]

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?