threads / patch / 34355
Subject: [PATCH v2 0/2] Squelch warning from send-email
Ramkumar RamachandraHi,
Since nobody stepped up to write it, I wrote [2/2] myself. It will be tested when I send out this series.
Thanks.
Ramkumar Ramachandra (2): send-email: squelch warning from Net::SMTP::SSL send-email: introduce sendemail.smtpsslcertpath
Documentation/config.txt | 3 +++ Documentation/git-send-email.txt | 4 ++++ git-send-email.perl | 20 ++++++++++++++++++-- 3 files changed, 25 insertions(+), 2 deletions(-)
-- 1.8.3.2.722.g3244e19.dirty
Ramkumar Ramachandra[PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL
Due to a recent change in the Net::SMTP::SSL module, send-email emits the following ugly warning everytime a email is sent via SSL:
******************************************************************* Using the default of SSL_verify_mode of SSL_VERIFY_NONE for client is deprecated! Please set SSL_verify_mode to SSL_VERIFY_PEER together with SSL_ca_file|SSL_ca_path for verification. If you really don't want to verify the certificate and keep the connection open to Man-In-The-Middle attacks please set SSL_verify_mode explicitly to SSL_VERIFY_NONE in your application. *******************************************************************
Fix this by explicitly specifying SSL_verify_mode => SSL_VERIFY_NONE in Net::SMTP::SSL->start_SSL().
Helped-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com> --- git-send-email.perl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl index ecbf56f..758100d 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1193,10 +1193,12 @@ X-Mailer: git-send-email $gitversion Debug => $debug_net_smtp); if ($smtp_encryption eq 'tls' && $smtp) { require Net::SMTP::SSL; + use IO::Socket::SSL qw(SSL_VERIFY_NONE); $smtp->command('STARTTLS'); $smtp->response(); if ($smtp->code == 220) { - $smtp = Net::SMTP::SSL->start_SSL($smtp) + $smtp = Net::SMTP::SSL->start_SSL($smtp, + SSL_verify_mode => SSL_VERIFY_NONE) or die "STARTTLS failed! ".$smtp->message; $smtp_encryption = ''; # Send EHLO again to receive fresh
-- 1.8.3.2.723.gad7967b.dirty
Torsten BögershausenRe: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL
On 2013-07-05 14.05, Ramkumar Ramachandra wrote:
> Due to a recent change in the Net::SMTP::SSL module, send-email emits
> the following ugly warning everytime a email is sent via SSL:
>
> *******************************************************************
> Using the default of SSL_verify_mode of SSL_VERIFY_NONE for client
> is deprecated! Please set SSL_verify_mode to SSL_VERIFY_PEER
> together with SSL_ca_file|SSL_ca_path for verification.
> If you really don't want to verify the certificate and keep the
> connection open to Man-In-The-Middle attacks please set
> SSL_verify_mode explicitly to SSL_VERIFY_NONE in your application.
> *******************************************************************
>
> Fix this by explicitly specifying SSL_verify_mode => SSL_VERIFY_NONE in
> Net::SMTP::SSL->start_SSL().
>
> Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> git-send-email.perl | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index ecbf56f..758100d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1193,10 +1193,12 @@ X-Mailer: git-send-email $gitversion
> Debug => $debug_net_smtp);
> if ($smtp_encryption eq 'tls' && $smtp) {
> require Net::SMTP::SSL;
> + use IO::Socket::SSL qw(SSL_VERIFY_NONE);
> $smtp->command('STARTTLS');
> $smtp->response();
> if ($smtp->code == 220) {
> - $smtp = Net::SMTP::SSL->start_SSL($smtp)
> + $smtp = Net::SMTP::SSL->start_SSL($smtp,
> + SSL_verify_mode => SSL_VERIFY_NONE)
> or die "STARTTLS failed! ".$smtp->message;
> $smtp_encryption = '';
> # Send EHLO again to receive fresh
> Hm, this doesn't work on my system, and t9001 fails:
"SSL_VERIFY_PEER" is not exported by the IO::Socket::SSL module "SSL_VERIFY_NONE" is not exported by the IO::Socket::SSL module Can't continue after import errors at /Users/tb/projects/git/git.pu/git-send-email line 1090 /Torsten
brian m. carlsonRe: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL
On Sat, Jul 06, 2013 at 04:28:00PM +0200, Torsten Bögershausen wrote:
> On 2013-07-05 14.05, Ramkumar Ramachandra wrote:
> > Due to a recent change in the Net::SMTP::SSL module, send-email emits
> > the following ugly warning everytime a email is sent via SSL:
> >
> > *******************************************************************
> > Using the default of SSL_verify_mode of SSL_VERIFY_NONE for client
> > is deprecated! Please set SSL_verify_mode to SSL_VERIFY_PEER
> > together with SSL_ca_file|SSL_ca_path for verification.
> > If you really don't want to verify the certificate and keep the
> > connection open to Man-In-The-Middle attacks please set
> > SSL_verify_mode explicitly to SSL_VERIFY_NONE in your application.
> > *******************************************************************
> >
> > Fix this by explicitly specifying SSL_verify_mode => SSL_VERIFY_NONE in
> > Net::SMTP::SSL->start_SSL().
> >
> > Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> > ---
> > git-send-email.perl | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index ecbf56f..758100d 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1193,10 +1193,12 @@ X-Mailer: git-send-email $gitversion
> > Debug => $debug_net_smtp);
> > if ($smtp_encryption eq 'tls' && $smtp) {
> > require Net::SMTP::SSL;
> > + use IO::Socket::SSL qw(SSL_VERIFY_NONE);
> > $smtp->command('STARTTLS');
> > $smtp->response();
> > if ($smtp->code == 220) {
> > - $smtp = Net::SMTP::SSL->start_SSL($smtp)
> > + $smtp = Net::SMTP::SSL->start_SSL($smtp,
> > + SSL_verify_mode => SSL_VERIFY_NONE)
> > or die "STARTTLS failed! ".$smtp->message;
> > $smtp_encryption = '';
> > # Send EHLO again to receive fresh
> >
> Hm, this doesn't work on my system, and t9001 fails:
>
> "SSL_VERIFY_PEER" is not exported by the IO::Socket::SSL module
> "SSL_VERIFY_NONE" is not exported by the IO::Socket::SSL module
> Can't continue after import errors at /Users/tb/projects/git/git.pu/git-send-email line 1090What version of IO::Socket::SSL do you have, and what source did you get it from?
perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'
This should be available in 1.31 or later. It might end up that we need to adjust the use/require statement to require 1.31.
-- 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
Torsten BögershausenRe: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL
On 2013-07-06 16.32, brian m. carlson wrote:
> perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'
Mac OS X, 10.6: (I think this perl we use for git:) /usr/bin/perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";' 1.22
(And this is in my path:) which perl /opt/local/bin/perl
perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";' Can't locate IO/Socket/SSL.pm in @INC (@INC contains: /sw/lib/perl5 /sw/lib/perl5/darwin /opt/local/lib/perl5/site_perl/5.8.9/darwin-2level /opt/local/lib/perl5/site_perl/5.8.9 /opt/local/lib/perl5/site_perl /opt/local/lib/perl5/vendor_perl/5.8.9/darwin-2level /opt/local/lib/perl5/vendor_perl/5.8.9 /opt/local/lib/perl5/vendor_perl /opt/local/lib/perl5/5.8.9/darwin-2level /opt/local/lib/perl5/5.8.9 .).
/Torsten
Ramkumar RamachandraRe: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL
Torsten Bögershausen wrote:
> /usr/bin/perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";' > 1.22
This is ancient! (I have 1.84). Is it not possible to do an ssl-verify-peer in older versions (is it exported as something else)? The older versions don't display the warning anyway, and this series is about squelching the warning in newer versions. Does
require IO::Socket::SSL qw(SSL_VERIFY_NONE SSL_VERIFY_PEER) or print "warning: not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL"
sound reasonable?
brian m. carlsonRe: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL
On Sun, Jul 14, 2013 at 07:19:10PM +0530, Ramkumar Ramachandra wrote:
> Torsten Bögershausen wrote: > > /usr/bin/perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";' > > 1.22 > > This is ancient! (I have 1.84). Is it not possible to do an > ssl-verify-peer in older versions (is it exported as something else)? > The older versions don't display the warning anyway, and this series > is about squelching the warning in newer versions. Does > > require IO::Socket::SSL qw(SSL_VERIFY_NONE SSL_VERIFY_PEER) or print > "warning: not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL"
require doesn't take a list of symbols to import, and the import dies if it fails. You need:
require IO::Socket::SSL;
eval {
IO::Socket::SSL->import(qw(SSL_VERIFY_NONE SSL_VERIFY_PEER));
};
if ($@) {
warn "Not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL\n";
# Do something different here.
}I didn't stick the require in the eval because git-send-email will fail in this case anyway if you don't have it, since Net::SMTP::SSL requires it. Let me know if you want a patch for this on top of the existing two in this series and I'll provide one.
-- 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
Ramkumar RamachandraRe: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL
brian m. carlson wrote:
> I didn't stick the require in the eval because git-send-email will fail > in this case anyway if you don't have it, since Net::SMTP::SSL requires > it. Let me know if you want a patch for this on top of the existing two > in this series and I'll provide one.
Yeah, that'd be nice. I'm not enjoying this Perl-wrestling very much.
Torsten BögershausenRe: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL
On 2013-07-14 19.03, brian m. carlson wrote:
> On Sun, Jul 14, 2013 at 07:19:10PM +0530, Ramkumar Ramachandra wrote:
>> Torsten Bögershausen wrote:
>>> /usr/bin/perl -MIO::Socket::SSL -e 'print "$IO::Socket::SSL::VERSION\n";'
>>> 1.22
>>
>> This is ancient! (I have 1.84). Is it not possible to do an
>> ssl-verify-peer in older versions (is it exported as something else)?
>> The older versions don't display the warning anyway, and this series
>> is about squelching the warning in newer versions. Does
>>
>> require IO::Socket::SSL qw(SSL_VERIFY_NONE SSL_VERIFY_PEER) or print
>> "warning: not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL"
>
> require doesn't take a list of symbols to import, and the import dies if
> it fails. You need:
>
> require IO::Socket::SSL;
> eval {
> IO::Socket::SSL->import(qw(SSL_VERIFY_NONE SSL_VERIFY_PEER));
> };
> if ($@) {
> warn "Not using SSL_VERIFY_PEER due to outdated IO::Socket::SSL\n";
> # Do something different here.
> }
>
> I didn't stick the require in the eval because git-send-email will fail
> in this case anyway if you don't have it, since Net::SMTP::SSL requires
> it. Let me know if you want a patch for this on top of the existing two
> in this series and I'll provide one.
> Please send a patch if possible. I can volonteer to test it here, but it can take a couple of days to respond.
Junio C HamanoRe: [PATCH v2 1/2] send-email: squelch warning from Net::SMTP::SSL
Torsten Bögershausen <tboegi@web.de> writes:
>> I didn't stick the require in the eval because git-send-email will fail >> in this case anyway if you don't have it, since Net::SMTP::SSL requires >> it. Let me know if you want a patch for this on top of the existing two >> in this series and I'll provide one. >> > Please send a patch if possible. > I can volonteer to test it here, but it can take a couple of days to respond.
Thanks all.
brian m. carlson[PATCH] send-email: improve SSL certificate verification
The SSL and TLS code for SMTP is non-trivial, so refactor it into a separate function for ease of use. Handle both files and directories as sources for CA certificates. Also add handling for older version of IO::Socket::SSL that do not support the SSL_VERIFY_PEER and SSL_VERIFY_NONE constants; in this case, print a warning and inform the user of this fact.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> ---
This is completely untested. I used perl -c, but that's it.
git-send-email.perl | 50 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 14 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl index 095b6fb..11fb2d0 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1083,6 +1083,37 @@ sub smtp_auth_maybe { return $auth; } +sub ssl_verify_params { + require IO::Socket::SSL; + eval { + IO::Socket::SSL->import(qw/SSL_VERIFY_PEER SSL_VERIFY_NONE/); + }; + if ($@) { + print STDERR "Not using SSL_VERIFY_PEER due to out-of-date IO::Socket::SSL.\n"; + return; + } + + if (!defined $smtp_ssl_cert_path) { + $smtp_ssl_cert_path ||= "/etc/ssl/certs"; + } + + if (!$smtp_ssl_cert_path) { + return (SSL_verify_mode => SSL_VERIFY_NONE()); + } + elsif (-d $smtp_ssl_cert_path) { + return (SSL_verify_mode => SSL_VERIFY_PEER(), + SSL_ca_path => $smtp_ssl_cert_path); + } + elsif (-f $smtp_ssl_cert_path) { + return (SSL_verify_mode => SSL_VERIFY_PEER(), + SSL_ca_file => $smtp_ssl_cert_path); + } + else { + print STDERR "Not using SSL_VERIFY_PEER because the CA path does not exist.\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. @@ -1187,7 +1218,8 @@ X-Mailer: git-send-email $gitversion $smtp_domain ||= maildomain(); $smtp ||= Net::SMTP::SSL->new($smtp_server, Hello => $smtp_domain, - Port => $smtp_server_port); + Port => $smtp_server_port, + ssl_verify_params()); } else { require Net::SMTP; @@ -1203,19 +1235,9 @@ X-Mailer: git-send-email $gitversion $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
-- 1.8.3.2.923.g2a18ff8.dirty -- 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
Torsten BögershausenRe: [PATCH] send-email: improve SSL certificate verification
[snip] I wasn't sure where to apply the patch, so I manually copy/paste it on top of pu: commit 6b1ca0f4d443ee8716857b871b0513ae85c9f112 Merge: bce90ab f351fcf
Thanks, t9001 passes on Mac OS X 10.6. To be sure I didn't messed it up, please see the diff below. When it shows up on pu, I can re-test of course.
diff --git a/git-send-email.perl b/git-send-email.perl index a9a6661..a965b8e 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -28,7 +28,7 @@ use File::Temp qw/ tempdir tempfile /; use File::Spec::Functions qw(catfile); use Error qw(:try); use Git; -use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE); +#use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE); Getopt::Long::Configure qw/ pass_through /; @@ -1092,19 +1092,34 @@ sub smtp_auth_maybe { # Helper to come up with SSL/TLS certification validation params # and warn when doing no verification sub ssl_verify_params { - if ($smtp_ssl_verify == 0) { - return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_NONE); + require IO::Socket::SSL; + eval { + IO::Socket::SSL->import(qw/SSL_VERIFY_PEER SSL_VERIFY_NONE/); + }; + if ($@) { + print STDERR "Not using SSL_VERIFY_PEER due to out-of-date IO::Socket::SSL.\n"; + return; } - 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 { - return (SSL_verify_mode => IO::Socket::SSL->SSL_VERIFY_PEER, + if (!defined $smtp_ssl_cert_path) { + $smtp_ssl_cert_path ||= "/etc/ssl/certs"; + } + + if (!$smtp_ssl_cert_path) { + return (SSL_verify_mode => SSL_VERIFY_NONE()); + } + elsif (-d $smtp_ssl_cert_path) { + return (SSL_verify_mode => SSL_VERIFY_PEER(), SSL_ca_path => $smtp_ssl_cert_path); } + elsif (-f $smtp_ssl_cert_path) { + return (SSL_verify_mode => SSL_VERIFY_PEER(), + SSL_ca_file => $smtp_ssl_cert_path); + } + else { + print STDERR "Not using SSL_VERIFY_PEER because the CA path does not exist.\n"; + return (SSL_verify_mode => SSL_VERIFY_NONE()); + } } # Returns 1 if the message was sent, and 0 otherwise. @@ -1229,13 +1244,8 @@ X-Mailer: git-send-email $gitversion if ($smtp->code == 220) { $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 - $smtp->hello($smtp_domain); - } else { - die "Server does not support STARTTLS! ".$smtp->message; + or die "STARTTLS failed! ".$smtp->message; + } } }
brian m. carlsonRe: [PATCH] send-email: improve SSL certificate verification
On Tue, Jul 16, 2013 at 04:33:55AM +0200, Torsten Bögershausen wrote:
> [snip] > I wasn't sure where to apply the patch, so I manually copy/paste it > on top of pu: > commit 6b1ca0f4d443ee8716857b871b0513ae85c9f112 > Merge: bce90ab f351fcf > > Thanks, t9001 passes on Mac OS X 10.6. > To be sure I didn't messed it up, please see the diff below. > When it shows up on pu, I can re-test of course.
Yeah, that 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
Junio C HamanoRe* [PATCH] send-email: improve SSL certificate verification
Torsten Bögershausen <tboegi@web.de> writes:
> I wasn't sure where to apply the patch, so I manually copy/paste it > on top of pu: > commit 6b1ca0f4d443ee8716857b871b0513ae85c9f112 > Merge: bce90ab f351fcf > > Thanks, t9001 passes on Mac OS X 10.6. > To be sure I didn't messed it up, please see the diff below. > When it shows up on pu, I can re-test of course.
As the history of rr/send-email-ssl-verify needs rewriting to squash this change in, here is a single patch with which I would propose to replace all the commits accumulated on that branch.
Ramkumar, as you will still be the primary author of the resulting commit, I tentatively added a line for your sign-off even though you haven't signed off _this_ version yet, so that I would not forget. If this version looks good, please tell us you would.
Torsten, I have Tested-by with your name, again so that I would not forget, but obviously this one hasn't been tested by you yet. If this tests OK, please tell us so.
Thanks.
-- >8 -- From: Ramkumar Ramachandra <artagnon@gmail.com> Subject: send-email: be explicit with SSL certificate verification
When initiating an SSL connection without explicitly specifying the SSL certificate verification mode, Net::SMTP::SSL defaults to no verification, but recent versions of the module gives a warning against this use of the default.
Enable certificate verification by default, using /etc/ssl/certs as the default path for certificates of certificate authorities. This path can be overriden by the --smtp-ssl-cert-path command line option and the sendemail.smtpSSLCertPath configuration variable.
Passing an empty string as the path for CA certificates path disables the SSL certificate verification explicitly, which does not trigger the warning from recent versions of Net::SMTP::SSL.
(PROVISO) Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com> Helped-by: Brian M. Carlson <sandals@crustytoothpaste.net> (PROVISO) Tested-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/config.txt | 4 ++++ Documentation/git-send-email.txt | 6 ++++++ git-send-email.perl | 41 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 48 insertions(+), 3 deletions(-)
Documentation/config.txt, Documentation/git-send-email.txt, git-send-email.perl
diff --git a/Documentation/config.txt b/Documentation/config.txt index 6e53fc5..4de154c 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2022,6 +2022,10 @@ sendemail.smtpencryption:: sendemail.smtpssl:: Deprecated alias for 'sendemail.smtpencryption = ssl'. +sendemail.smtpsslcertpath:: + Path to ca-certificates (either a directory or a single file). + Set it to an empty string to disable certificate verification. + sendemail.<identity>.*:: Identity-specific versions of the 'sendemail.*' parameters found below, taking precedence over those when the this diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 40a9a9a..f0e57a5 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -198,6 +198,12 @@ must be used for each option. --smtp-ssl:: Legacy alias for '--smtp-encryption ssl'. +--smtp-ssl-cert-path:: + Path to ca-certificates (either a directory or a single file). + Set it to an empty string to disable certificate verification. + Defaults to the value set to the 'sendemail.smtpsslcertpath' + configuration variable, if set, or `/etc/ssl/certs` otherwise. + --smtp-user=<user>:: Username for SMTP-AUTH. Default is the value of 'sendemail.smtpuser'; if a username is not specified (with '--smtp-user' or 'sendemail.smtpuser'), diff --git a/git-send-email.perl b/git-send-email.perl index bd13cc8..60eaed3 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -69,6 +69,9 @@ sub usage { --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'. + --smtp-ssl-cert-path <str> * Path to ca-certificates (either directory or file). + Pass an empty string to disable certificate + verification. --smtp-domain <str> * The domain name sent to HELO/EHLO handshake --smtp-debug <0|1> * Disable, enable Net::SMTP debug. @@ -194,7 +197,7 @@ sub do_edit { 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_authuser, $smtp_encryption, $smtp_ssl_cert_path); my ($identity, $aliasfiletype, @alias_files, $smtp_domain); my ($validate, $confirm); my (@suppress_cc); @@ -222,6 +225,7 @@ sub do_edit { "smtpserveroption" => \@smtp_server_options, "smtpuser" => \$smtp_authuser, "smtppass" => \$smtp_authpass, + "smtpsslcertpath" => \$smtp_ssl_cert_path, "smtpdomain" => \$smtp_domain, "to" => \@initial_to, "tocmd" => \$to_cmd, @@ -302,6 +306,7 @@ sub signal_handler { "smtp-pass:s" => \$smtp_authpass, "smtp-ssl" => sub { $smtp_encryption = 'ssl' }, "smtp-encryption=s" => \$smtp_encryption, + "smtp-ssl-cert-path" => \$smtp_ssl_cert_path, "smtp-debug:i" => \$debug_net_smtp, "smtp-domain:s" => \$smtp_domain, "identity=s" => \$identity, @@ -1089,6 +1094,34 @@ sub smtp_auth_maybe { return $auth; } +sub ssl_verify_params { + eval { + require IO::Socket::SSL; + IO::Socket::SSL->import(qw/SSL_VERIFY_PEER SSL_VERIFY_NONE/); + }; + if ($@) { + print STDERR "Not using SSL_VERIFY_PEER due to out-of-date IO::Socket::SSL.\n"; + return; + } + + if (!defined $smtp_ssl_cert_path) { + $smtp_ssl_cert_path = "/etc/ssl/certs"; + } + + if ($smtp_ssl_cert_path eq "") { + return (SSL_verify_mode => SSL_VERIFY_NONE()); + } elsif (-d $smtp_ssl_cert_path) { + return (SSL_verify_mode => SSL_VERIFY_PEER(), + SSL_ca_path => $smtp_ssl_cert_path); + } elsif (-f $smtp_ssl_cert_path) { + return (SSL_verify_mode => SSL_VERIFY_PEER(), + SSL_ca_file => $smtp_ssl_cert_path); + } else { + print STDERR "Not using SSL_VERIFY_PEER because the CA path does not exist.\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. @@ -1194,7 +1227,8 @@ sub send_message { $smtp_domain ||= maildomain(); $smtp ||= Net::SMTP::SSL->new($smtp_server, Hello => $smtp_domain, - Port => $smtp_server_port); + Port => $smtp_server_port, + ssl_verify_params()); } else { require Net::SMTP; @@ -1207,7 +1241,8 @@ sub send_message { $smtp->command('STARTTLS'); $smtp->response(); if ($smtp->code == 220) { - $smtp = Net::SMTP::SSL->start_SSL($smtp) + $smtp = Net::SMTP::SSL->start_SSL($smtp, + ssl_verify_params()) or die "STARTTLS failed! ".$smtp->message; $smtp_encryption = ''; # Send EHLO again to receive fresh
-- 1.8.3.3-992-gf0e5e44
Ramkumar RamachandraRe: Re* [PATCH] send-email: improve SSL certificate verification
Junio C Hamano wrote:
> Ramkumar, as you will still be the primary author of the resulting > commit, I tentatively added a line for your sign-off even though you > haven't signed off _this_ version yet, so that I would not forget. > If this version looks good, please tell us you would.
Yeah, it seems to work fine for me. Sign-off is fine.
Thanks for putting it together.
Ramkumar Ramachandra[PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
Use the ca-certificates in /etc/ssl/certs by default (that's where most distributions put it). SSL_VERIFY_NONE is now the fallback mode.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com> --- Documentation/config.txt | 3 +++ Documentation/git-send-email.txt | 4 ++++ git-send-email.perl | 22 ++++++++++++++++++---- 3 files changed, 25 insertions(+), 4 deletions(-)
Documentation/config.txt, Documentation/git-send-email.txt, git-send-email.perl
diff --git a/Documentation/config.txt b/Documentation/config.txt index b4d4887..b216a63 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2048,6 +2048,9 @@ sendemail.smtpencryption:: sendemail.smtpssl:: Deprecated alias for 'sendemail.smtpencryption = ssl'. +sendemail.smtpsslcertpath:: + Path to ca-certificates. Defaults to `/etc/ssl/certs`. + sendemail.<identity>.*:: Identity-specific versions of the 'sendemail.*' parameters found below, taking precedence over those when the this diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 40a9a9a..605f263 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-cert-path:: + Path to ca-certificates. Defaults to `/etc/ssl/certs`, or + 'sendemail.smtpsslcertpath'. + --smtp-user=<user>:: Username for SMTP-AUTH. Default is the value of 'sendemail.smtpuser'; if a username is not specified (with '--smtp-user' or 'sendemail.smtpuser'), diff --git a/git-send-email.perl b/git-send-email.perl index 758100d..026bcbc 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -69,6 +69,8 @@ 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'. + --smtp-ssl-cert-path <str> * Path to ca-certificates. Defaults to + /etc/ssl/certs. --smtp-domain <str> * The domain name sent to HELO/EHLO handshake --smtp-debug <0|1> * Disable, enable Net::SMTP debug. @@ -195,6 +197,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 ($identity, $aliasfiletype, @alias_files, $smtp_domain); my ($validate, $confirm); my (@suppress_cc); @@ -220,6 +223,7 @@ my %config_settings = ( "smtpserveroption" => \@smtp_server_options, "smtpuser" => \$smtp_authuser, "smtppass" => \$smtp_authpass, + "smtpsslcertpath" => \$smtp_ssl_cert_path, "smtpdomain" => \$smtp_domain, "to" => \@initial_to, "tocmd" => \$to_cmd, @@ -1193,13 +1197,23 @@ X-Mailer: git-send-email $gitversion Debug => $debug_net_smtp); if ($smtp_encryption eq 'tls' && $smtp) { require Net::SMTP::SSL; - use IO::Socket::SSL qw(SSL_VERIFY_NONE); + use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE); $smtp->command('STARTTLS'); $smtp->response(); if ($smtp->code == 220) { - $smtp = Net::SMTP::SSL->start_SSL($smtp, - SSL_verify_mode => SSL_VERIFY_NONE) - or die "STARTTLS failed! ".$smtp->message; + # 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_encryption = ''; # Send EHLO again to receive fresh # supported commands
-- 1.8.3.2.723.gad7967b.dirty
Eric SunshineRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
On Fri, Jul 5, 2013 at 8:05 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Use the ca-certificates in /etc/ssl/certs by default (that's where most
> distributions put it). SSL_VERIFY_NONE is now the fallback mode.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 758100d..026bcbc 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1193,13 +1197,23 @@ X-Mailer: git-send-email $gitversion
> Debug => $debug_net_smtp);
> if ($smtp_encryption eq 'tls' && $smtp) {
> require Net::SMTP::SSL;
> - use IO::Socket::SSL qw(SSL_VERIFY_NONE);
> + use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
> $smtp->command('STARTTLS');
> $smtp->response();
> if ($smtp->code == 220) {
> - $smtp = Net::SMTP::SSL->start_SSL($smtp,
> - SSL_verify_mode => SSL_VERIFY_NONE)
> - or die "STARTTLS failed! ".$smtp->message;
> + # Attempt to use a ca-certificate by default
> + $smtp_ssl_cert_path |= "/etc/ssl/certs";You're going to want to use logical ||= here. Bitwise |= on a string does not do what you expect[1]:
my $s = '/usr/local/etc/ssl/certs'; $s |= '/etc/ssl/certs'; print $s, "\n";
Outputs: /uws/oooowts/ssl/certs
[1]: http://perldoc.perl.org/perlop.html#Bitwise-String-Operators
> + 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_encryption = '';
> # Send EHLO again to receive fresh
> # supported commandsRamkumar RamachandraRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
Eric Sunshine wrote:
>> + $smtp_ssl_cert_path |= "/etc/ssl/certs"; > > You're going to want to use logical ||= here. Bitwise |= on a string > does not do what you expect[1]: > > my $s = '/usr/local/etc/ssl/certs'; > $s |= '/etc/ssl/certs'; > print $s, "\n"; > > Outputs: /uws/oooowts/ssl/certs
Thanks; much appreciated. My Perl is quite weak.
brian m. carlsonRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
On Fri, Jul 05, 2013 at 05:35:47PM +0530, Ramkumar Ramachandra wrote:
> @@ -1193,13 +1197,23 @@ X-Mailer: git-send-email $gitversion
> Debug => $debug_net_smtp);
> if ($smtp_encryption eq 'tls' && $smtp) {
> require Net::SMTP::SSL;
> - use IO::Socket::SSL qw(SSL_VERIFY_NONE);
> + use IO::Socket::SSL qw(SSL_VERIFY_PEER SSL_VERIFY_NONE);
> $smtp->command('STARTTLS');
> $smtp->response();
> if ($smtp->code == 220) {
> - $smtp = Net::SMTP::SSL->start_SSL($smtp,
> - SSL_verify_mode => SSL_VERIFY_NONE)
> - or die "STARTTLS failed! ".$smtp->message;
> + # 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;
> + }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.
-- 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
Ramkumar RamachandraRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
brian m. carlson wrote:
> 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.
Thanks. On a related note, how do I find out about all these things? I tried
$ perldoc Net::SMTP::SSL
but it was completely useless. The only reason I got this far is because you literally told me what to do. Do I have to resort to reading the sources? If so, how do I bring up the relevant functions quickly? (I'm looking for something like the elisp M-x find-function).
brian m. carlsonRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
On Fri, Jul 05, 2013 at 06:23:58PM +0530, Ramkumar Ramachandra wrote:
> Thanks. On a related note, how do I find out about all these things? I tried > > $ perldoc Net::SMTP::SSL > > but it was completely useless. The only reason I got this far is > because you literally told me what to do. Do I have to resort to > reading the sources? If so, how do I bring up the relevant functions > quickly? (I'm looking for something like the elisp M-x find-function).
Since Net::SMTP::SSL inherits from IO::Socket::SSL, you can do "perldoc IO::Socket::SSL", which is significantly more helpful. If you're looking for the source of a module, use "perldoc -lm Net::SMTP::SSL"; that will print the .pm file for the module.
-- 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
Junio C HamanoRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
"brian m. carlson" <sandals@crustytoothpaste.net> 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
John KeepingRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> 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?
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
> 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);
> + }
> +}
> +Junio C HamanoRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
John Keeping <john@keeping.me.uk> writes:
> On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote: >> "brian m. carlson" <sandals@crustytoothpaste.net> 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? > > 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
Interesting. That frees us from saying "we assume /etc/ssl/cacerts is the default location, and let the users override it".
To help those "I do not want verification because I know my server does not present valid certificate, I know my server is internal and trustable, and I do not bother to fix it" people, we can let them specify an empty string (or any non-directory) as the CACertPath, and structure the code like so?
if (defined $smtp_ssl_cert_path && -d $smtp_ssl_cert_path) {
return (SSL_verify_mode => SSL_VERIFY_PEER,
SSL_ca_path => $smtp_ssl_cert_path);
} elsif (defined $smtp_ssl_cert_path) {
return (SSL_verify_mode => SSL_VERIFY_NONE);
} else {
return (SSL_verify_mode => SSL_VERIFY_PEER);
}John KeepingRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
On Fri, Jul 05, 2013 at 11:30:11AM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > On Fri, Jul 05, 2013 at 10:20:11AM -0700, Junio C Hamano wrote:
> >> "brian m. carlson" <sandals@crustytoothpaste.net> 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?
> >
> > 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
>
> Interesting. That frees us from saying "we assume /etc/ssl/cacerts
> is the default location, and let the users override it".
>
> To help those "I do not want verification because I know my server
> does not present valid certificate, I know my server is internal and
> trustable, and I do not bother to fix it" people, we can let them
> specify an empty string (or any non-directory) as the CACertPath,
> and structure the code like so?
>
> if (defined $smtp_ssl_cert_path && -d $smtp_ssl_cert_path) {
> return (SSL_verify_mode => SSL_VERIFY_PEER,
> SSL_ca_path => $smtp_ssl_cert_path);
> } elsif (defined $smtp_ssl_cert_path) {
> return (SSL_verify_mode => SSL_VERIFY_NONE);
> } else {
> return (SSL_verify_mode => SSL_VERIFY_PEER);
> }I'd rather have '$smtp_ssl_cert_path ne ""' in the first if condition (instead of the '-d $smtp_ssl_cert_path') but that seems reasonable and agrees with my reading of the documentation.
Perhaps a complete solution could allow CA files as well:
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);
}Junio C HamanoRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
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]?
> 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?John KeepingRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
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 --
Documentation/git-send-email.txt, git-send-email.perl
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?
Junio C HamanoRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
John Keeping <john@keeping.me.uk> writes:
> @@ -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);I do not see any "use IO::Socket::SSL" anywhere after applying this patch. Is this expected to work?
John KeepingRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
On Sat, Jul 06, 2013 at 09:12:31PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > @@ -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);
>
> I do not see any "use IO::Socket::SSL" anywhere after applying this
> patch. Is this expected to work?I don't get any errors about unknown variables when running it. Do we get IO::Socket::SSL imported through Net::SMTP::SSL, which extends it?
brian m. carlsonRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
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
Jeff KingRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
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:
> > +# 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
Junio C HamanoRe: [PATCH v2 2/2] send-email: introduce sendemail.smtpsslcertpath
Jeff King <peff@peff.net> writes:
> 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:
>> > +# 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.A very good point. Thanks.
Also it appears that people seem to be seeing different behaviours depending on the versions of IO::Socket::SSL they have; we may need to conditionalize what our code does depending on $PACKAGE::Version after we do that lazy loading.