From: John Keeping <john@keeping.me.uk>
To: Junio C Hamano <gitster@pobox.com>
Cc: "brian m. carlson" <sandals@crustytoothpaste.net>,
Ramkumar Ramachandra <artagnon@gmail.com>,
Git List <git@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
Date: Sat, 6 Jul 2013 12:46:00 +0100 [thread overview]
Message-ID: <20130706114600.GP9161@serenity.lan> (raw)
In-Reply-To: <7v1u7c6w7z.fsf@alter.siamese.dyndns.org>
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?
next prev parent reply other threads:[~2013-07-06 11:46 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-05 12:05 [PATCH v2 0/2] Squelch warning from send-email Ramkumar Ramachandra
2013-07-05 12:05 ` [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL Ramkumar Ramachandra
2013-07-06 14:28 ` Torsten Bögershausen
2013-07-06 14:32 ` brian m. carlson
2013-07-06 15:49 ` Torsten Bögershausen
2013-07-14 13:49 ` Ramkumar Ramachandra
2013-07-14 17:03 ` brian m. carlson
2013-07-14 21:49 ` Ramkumar Ramachandra
2013-07-15 3:07 ` Torsten Bögershausen
2013-07-15 4:15 ` Junio C Hamano
2013-07-16 0:15 ` [PATCH] send-email: improve SSL certificate verification brian m. carlson
2013-07-16 2:33 ` Torsten Bögershausen
2013-07-16 2:35 ` brian m. carlson
2013-07-18 16:53 ` Re* " Junio C Hamano
2013-07-18 17:36 ` Ramkumar Ramachandra
2013-07-05 12:05 ` [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath Ramkumar Ramachandra
2013-07-05 12:33 ` Eric Sunshine
2013-07-05 12:36 ` Ramkumar Ramachandra
2013-07-05 12:45 ` brian m. carlson
2013-07-05 12:53 ` Ramkumar Ramachandra
2013-07-05 13:01 ` brian m. carlson
2013-07-05 17:20 ` Junio C Hamano
2013-07-05 17:47 ` John Keeping
2013-07-05 18:30 ` Junio C Hamano
2013-07-05 18:43 ` John Keeping
2013-07-06 6:25 ` Junio C Hamano
2013-07-06 11:46 ` John Keeping [this message]
2013-07-07 4:12 ` Junio C Hamano
2013-07-07 9:02 ` John Keeping
2013-07-05 20:29 ` brian m. carlson
2013-07-07 5:54 ` Jeff King
2013-07-07 10:01 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130706114600.GP9161@serenity.lan \
--to=john@keeping.me.uk \
--cc=artagnon@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=sandals@crustytoothpaste.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).