[PATCH 1/3] send-email: pass Debug to Net::SMTP::SSL::new

[PATCH 1/3] send-email: pass Debug to Net::SMTP::SSL::new

[Thomas Rast]

We forgot to pass the Debug option through to Net::SMTP::SSL->new --
which is the same as Net::SMTP->new.  This meant that with security
set to SSL, we would never enable debug output.

Pass through the flag.

Signed-off-by: Thomas Rast <tr@thomasrast.ch>
---
 git-send-email.perl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 3782c3b..f7468b6 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1217,6 +1217,7 @@ sub send_message {
 			$smtp ||= Net::SMTP::SSL->new($smtp_server,
 						      Hello => $smtp_domain,
 						      Port => $smtp_server_port,
+						      Debug => $debug_net_smtp,
 						      ssl_verify_params());
 		}
 		else {
-- 
1.8.5.rc3.5.g2a1fe2f

[PATCH 2/3] send-email: --smtp-ssl-cert-path takes an argument

[Thomas Rast]

35035bb (send-email: be explicit with SSL certificate verification,
2013-07-18) forgot to specify that --smtp-ssl-cert-path takes a string
argument.  This means that the option could not actually be used as
intended.  Presumably noone noticed because it's much easier to set it
through configs anyway.

Add the required "=s".

Signed-off-by: Thomas Rast <tr@thomasrast.ch>
---
 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index f7468b6..9f31c68 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -291,7 +291,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-ssl-cert-path=s" => \$smtp_ssl_cert_path,
 		    "smtp-debug:i" => \$debug_net_smtp,
 		    "smtp-domain:s" => \$smtp_domain,
 		    "identity=s" => \$identity,
-- 
1.8.5.rc3.5.g2a1fe2f

[PATCH 3/3] send-email: set SSL options through IO::Socket::SSL::set_client_defaults

[Thomas Rast]

When --smtp-encryption=ssl, we use a Net::SMTP::SSL connection,
passing its ->new all the options that would otherwise go to
Net::SMTP->new (most options) and IO::Socket::SSL->start_SSL (for the
SSL options).

However, while Net::SMTP::SSL replaces the underlying socket class
with an SSL socket, it does nothing to allow passing options to that
socket.  So the SSL-relevant options are lost.

Fortunately there is an escape hatch: we can directly set the options
with IO::Socket::SSL::set_client_defaults.  They will then persist
within the IO::Socket::SSL module.

Signed-off-by: Thomas Rast <tr@thomasrast.ch>
---
 git-send-email.perl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 9f31c68..2016d9c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1214,11 +1214,14 @@ sub send_message {
 			$smtp_server_port ||= 465; # ssmtp
 			require Net::SMTP::SSL;
 			$smtp_domain ||= maildomain();
+			require IO::Socket::SSL;
+			# Net::SMTP::SSL->new() does not forward any SSL options
+			IO::Socket::SSL::set_client_defaults(
+				ssl_verify_params());
 			$smtp ||= Net::SMTP::SSL->new($smtp_server,
 						      Hello => $smtp_domain,
 						      Port => $smtp_server_port,
-						      Debug => $debug_net_smtp,
-						      ssl_verify_params());
+						      Debug => $debug_net_smtp);
 		}
 		else {
 			require Net::SMTP;
-- 
1.8.5.rc3.5.g2a1fe2f

Re: [PATCH 3/3] send-email: set SSL options through IO::Socket::SSL::set_client_defaults

[Ramkumar Ramachandra]

Thomas Rast wrote:
> When --smtp-encryption=ssl, we use a Net::SMTP::SSL connection,
> passing its ->new all the options that would otherwise go to
> Net::SMTP->new (most options) and IO::Socket::SSL->start_SSL (for the
> SSL options).
>
> However, while Net::SMTP::SSL replaces the underlying socket class
> with an SSL socket, it does nothing to allow passing options to that
> socket.  So the SSL-relevant options are lost.

Both [1/3] and [2/3] look good. However, I'm curious about this one:
Net::SMTP::SSL inherits from IO::Socket::SSL, where new() is defined.
In the documentation for IO::Socket::SSL,

  $ perldoc IO::Socket::SSL

I can see examples where SSL_verify_mode and SSL_ca_path are passed to
new(). So, I'm not sure what this patch is about.

Re: [PATCH 3/3] send-email: set SSL options through IO::Socket::SSL::set_client_defaults

[Thomas Rast]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Thomas Rast wrote:
>> When --smtp-encryption=ssl, we use a Net::SMTP::SSL connection,
>> passing its ->new all the options that would otherwise go to
>> Net::SMTP->new (most options) and IO::Socket::SSL->start_SSL (for the
>> SSL options).
>>
>> However, while Net::SMTP::SSL replaces the underlying socket class
>> with an SSL socket, it does nothing to allow passing options to that
>> socket.  So the SSL-relevant options are lost.
>
> Both [1/3] and [2/3] look good. However, I'm curious about this one:
> Net::SMTP::SSL inherits from IO::Socket::SSL, where new() is defined.
> In the documentation for IO::Socket::SSL,
>
>   $ perldoc IO::Socket::SSL
>
> I can see examples where SSL_verify_mode and SSL_ca_path are passed to
> new(). So, I'm not sure what this patch is about.

Net::SMTP::SSL is merely steals all the code from Net::SMTP into a class
that has IO::Socket::SSL as its first inheritance line.

This works because Net::SMTP (no SSL) inherits from IO::Socket::INET
instead, and uses SUPER:: methods to access the latter's features.  So
by effectively replacing IO::Socket::INET with IO::Socket::SSL,
Net::SMTP::SSL can apply all of Net::SMTP's code on an SSL socket.

However!

That SUPER:: access does not pass anything SSLey.  In particular,
Net::SMTP::SSL->new (which is just the same as Net::SMTP->new) runs this
to initialize its socket:

    $obj = $type->SUPER::new(
      PeerAddr => ($host = $h),
      PeerPort => $arg{Port} || 'smtp(25)',
      LocalAddr => $arg{LocalAddr},
      LocalPort => $arg{LocalPort},
      Proto     => 'tcp',
      Timeout   => defined $arg{Timeout}
      ? $arg{Timeout}
      : 120
      )

Note the conspicuous absence of any kind of SSL arguments, or any kind
of args-I-don't-know-myself passthrough.

If you _do_ specify SSL arguments (i.e. key-value style arguments that
would normally be accepted by IO::Socket::SSL->new) to
Net::SMTP::SSL->new, they will simply be ignored, because of how the
key-value argument passing treats the argument list as a hash.

Does that clarify it?

This is all assuming I got the details vaguely correct, and the source
snippets are from my perl v5.18.1 installed by opensuse 13.1.

It turns out the server I was trying to talk to on Sunday had an expired
certificate, and despite the code from 35035bb, my efforts to set
SSL_VERIFY_NONE were futile.  Until I noticed the set_client_defaults()
trick.  So I'm pretty convinced the patch does *something* right.

-- 
Thomas Rast
tr@thomasrast.ch