git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
John Keeping <john@keeping.me.uk>
Date
Jul 6, 2013, 11:46 UTC
Message-ID
<20130706114600.GP9161@serenity.lan>
In-Reply-To
<7v1u7c6w7z.fsf@alter.siamese.dyndns.org>
On Fri, Jul 05, 2013 at 11:25:36PM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> 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?
Previous: Junio C HamanoNext: Junio C Hamano
Message 27 of 32 in “Squelch warning from send-email”
  1. 0/2 Squelch warning from send-emailRamkumar Ramachandra, Jul 5, 2013
  2. 1/2 send-email: squelch warning from Net::SMTP::SSLRamkumar Ramachandra, Jul 5, 2013
  3. Torsten BögershausenJul 6, 2013
  4. brian m. carlsonJul 6, 2013
  5. Torsten BögershausenJul 6, 2013
  6. Ramkumar RamachandraJul 14, 2013
  7. brian m. carlsonJul 14, 2013
  8. Ramkumar RamachandraJul 14, 2013
  9. Torsten BögershausenJul 15, 2013
  10. Junio C HamanoJul 15, 2013
  11. send-email: improve SSL certificate verificationbrian m. carlson, Jul 16, 2013
  12. Torsten BögershausenJul 16, 2013
  13. brian m. carlsonJul 16, 2013
  14. Re* [PATCH] send-email: improve SSL certificate verificationJunio C Hamano, Jul 18, 2013
  15. Ramkumar RamachandraJul 18, 2013
  16. 2/2 send-email: introduce sendemail.smtpsslcertpathRamkumar Ramachandra, Jul 5, 2013
  17. Eric SunshineJul 5, 2013
  18. Ramkumar RamachandraJul 5, 2013
  19. brian m. carlsonJul 5, 2013
  20. Ramkumar RamachandraJul 5, 2013
  21. brian m. carlsonJul 5, 2013
  22. Junio C HamanoJul 5, 2013
  23. John KeepingJul 5, 2013
  24. Junio C HamanoJul 5, 2013
  25. John KeepingJul 5, 2013
  26. Junio C HamanoJul 6, 2013
  27. John KeepingJul 6, 2013
  28. Junio C HamanoJul 7, 2013
  29. John KeepingJul 7, 2013
  30. brian m. carlsonJul 5, 2013
  31. Jeff KingJul 7, 2013
  32. Junio C HamanoJul 7, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.