On Fri, Jul 05, 2013 at 11:25:36PM -0700, Junio C Hamano wrote:
> John Keeping <[email protected]> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
