[RFC/PATCH] send-email: die if CA path doesn't exist

[RFC/PATCH] send-email: die if CA path doesn't exist

[John Keeping]

If the CA path isn't found it's most likely to indicate a
misconfiguration, in which case accepting any certificate is unlikely to
be the correct thing to do.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-send-email.perl | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 8e4c0e1..e057051 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1196,8 +1196,7 @@ sub ssl_verify_params {
 		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());
+		die "CA path does not exist.";
 	}
 }
 
-- 
2.6.3.462.gbe2c914

Re: [RFC/PATCH] send-email: die if CA path doesn't exist

[Jeff King]

On Tue, Nov 17, 2015 at 10:12:07PM +0000, John Keeping wrote:

> If the CA path isn't found it's most likely to indicate a
> misconfiguration, in which case accepting any certificate is unlikely to
> be the correct thing to do.

Yeah, this seems like a crazy default for security-sensitive code.

I suspect some people will see breakage from applying this (because
their systems are broken and they did not know it), but that is a good
thing.

For people who know their systems are broken and want to proceed anyway,
what is the appropriate work-around? Obviously it involves disabling
peer verification, but would we want to include instructions for doing
so (either in the error message, or perhaps mentioning it in the commit
message)?

-Peff

Re: [RFC/PATCH] send-email: die if CA path doesn't exist

[John Keeping]

On Fri, Nov 20, 2015 at 06:18:48AM -0500, Jeff King wrote:
> On Tue, Nov 17, 2015 at 10:12:07PM +0000, John Keeping wrote:
> 
> > If the CA path isn't found it's most likely to indicate a
> > misconfiguration, in which case accepting any certificate is unlikely to
> > be the correct thing to do.
> 
> Yeah, this seems like a crazy default for security-sensitive code.
> 
> I suspect some people will see breakage from applying this (because
> their systems are broken and they did not know it), but that is a good
> thing.
> 
> For people who know their systems are broken and want to proceed anyway,
> what is the appropriate work-around? Obviously it involves disabling
> peer verification, but would we want to include instructions for doing
> so (either in the error message, or perhaps mentioning it in the commit
> message)?

The documentation already says:

	Set it to an empty string to disable certificate verification.

It's a bit lost in the middle of a paragraph but I think that is the
best place for the detail of how to disable verification.

Having revisted the patch, I do think the message might be a bit terse,
but I can't think of a reasonably concise way to point at the
--smtp-ssl-cert-path argument as being the culprit.

Maybe we shouldn't worry too much about that, but should instead put the
invalid path into the error message:

	die "CA path \"$smtp_ssl_cert_path\" does not exist.";

Re: [RFC/PATCH] send-email: die if CA path doesn't exist

[Jeff King]

On Fri, Nov 20, 2015 at 07:46:51PM +0000, John Keeping wrote:

> > For people who know their systems are broken and want to proceed anyway,
> > what is the appropriate work-around? Obviously it involves disabling
> > peer verification, but would we want to include instructions for doing
> > so (either in the error message, or perhaps mentioning it in the commit
> > message)?
> 
> The documentation already says:
> 
> 	Set it to an empty string to disable certificate verification.
> 
> It's a bit lost in the middle of a paragraph but I think that is the
> best place for the detail of how to disable verification.
> 
> Having revisted the patch, I do think the message might be a bit terse,
> but I can't think of a reasonably concise way to point at the
> --smtp-ssl-cert-path argument as being the culprit.

Hrm. I was thinking that somebody might not have any clue that
--smtp-ssl-cert-path exists, and with this patch their setup would
suddenly go from working (well, insecure but passing mail) to broken.
They need to know where to look to find that documentation.

But it looks like this code path only triggers if you have set
smtp-ssl-cert-path to something bogus. So anybody who does so at least
knows about the option.

Which makes me wonder what happens when the cert path isn't defined by
Git. The code says:

        if (!defined $smtp_ssl_cert_path) {
                # use the OpenSSL defaults
                return (SSL_verify_mode => SSL_VERIFY_PEER());
        }

What does OpenSSL do when there is no cert? Hopefully it reports a
verification failure (and in that sense your patch is making our code
consistent with that, which is a good thing).

> Maybe we shouldn't worry too much about that, but should instead put the
> invalid path into the error message:
> 
> 	die "CA path \"$smtp_ssl_cert_path\" does not exist.";

Given what I wrote above, yeah, I'd agree that is sufficient (and I do
think mentioning the path is helpful).

-Peff

Re: [RFC/PATCH] send-email: die if CA path doesn't exist

[John Keeping]

On Tue, Nov 24, 2015 at 02:58:43PM -0500, Jeff King wrote:
> On Fri, Nov 20, 2015 at 07:46:51PM +0000, John Keeping wrote:
> 
> > > For people who know their systems are broken and want to proceed anyway,
> > > what is the appropriate work-around? Obviously it involves disabling
> > > peer verification, but would we want to include instructions for doing
> > > so (either in the error message, or perhaps mentioning it in the commit
> > > message)?
> > 
> > The documentation already says:
> > 
> > 	Set it to an empty string to disable certificate verification.
> > 
> > It's a bit lost in the middle of a paragraph but I think that is the
> > best place for the detail of how to disable verification.
> > 
> > Having revisted the patch, I do think the message might be a bit terse,
> > but I can't think of a reasonably concise way to point at the
> > --smtp-ssl-cert-path argument as being the culprit.
> 
> Hrm. I was thinking that somebody might not have any clue that
> --smtp-ssl-cert-path exists, and with this patch their setup would
> suddenly go from working (well, insecure but passing mail) to broken.
> They need to know where to look to find that documentation.
> 
> But it looks like this code path only triggers if you have set
> smtp-ssl-cert-path to something bogus. So anybody who does so at least
> knows about the option.
> 
> Which makes me wonder what happens when the cert path isn't defined by
> Git. The code says:
> 
>         if (!defined $smtp_ssl_cert_path) {
>                 # use the OpenSSL defaults
>                 return (SSL_verify_mode => SSL_VERIFY_PEER());
>         }
> 
> What does OpenSSL do when there is no cert? Hopefully it reports a
> verification failure (and in that sense your patch is making our code
> consistent with that, which is a good thing).

I suspect it's not very useful; I originally got here after setting up
git-send-email to talk to a server with a certificate signed by a
corporate CA and had to resort to the perl debugger to figure out where
it was going wrong.  There isn't even any output with --smtp-debug when
the SSL handshake fails.

The error message is (all on one line):

Unable to initialize SMTP properly. Check config and use --smtp-debug.
VALUES: server=<redacted> encryption=ssl hello=<redacted>
port=465 at /usr/libexec/git-core/git-send-email line 1357.

I wonder if we should do this to help debug SSL issues:

-- >8 --
diff --git a/git-send-email.perl b/git-send-email.perl
index e057051..6d4e0ee 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1317,6 +1317,10 @@ Message-Id: $message_id
 			require Net::SMTP::SSL;
 			$smtp_domain ||= maildomain();
 			require IO::Socket::SSL;
+			if ($debug_net_smtp) {
+				no warnings 'once';
+				$IO::Socket::SSL::DEBUG = 1;
+			}
 			# Net::SMTP::SSL->new() does not forward any SSL options
 			IO::Socket::SSL::set_client_defaults(
 				ssl_verify_params());
-- 8< --

> > Maybe we shouldn't worry too much about that, but should instead put the
> > invalid path into the error message:
> > 
> > 	die "CA path \"$smtp_ssl_cert_path\" does not exist.";
> 
> Given what I wrote above, yeah, I'd agree that is sufficient (and I do
> think mentioning the path is helpful).

I'll change it to this in a re-roll.

Re: [RFC/PATCH] send-email: die if CA path doesn't exist

[Jeff King]

On Tue, Nov 24, 2015 at 10:17:08PM +0000, John Keeping wrote:

> I wonder if we should do this to help debug SSL issues:
> 
> -- >8 --
> diff --git a/git-send-email.perl b/git-send-email.perl
> index e057051..6d4e0ee 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1317,6 +1317,10 @@ Message-Id: $message_id
>  			require Net::SMTP::SSL;
>  			$smtp_domain ||= maildomain();
>  			require IO::Socket::SSL;
> +			if ($debug_net_smtp) {
> +				no warnings 'once';
> +				$IO::Socket::SSL::DEBUG = 1;
> +			}
>  			# Net::SMTP::SSL->new() does not forward any SSL options
>  			IO::Socket::SSL::set_client_defaults(
>  				ssl_verify_params());
> -- 8< --

That certainly looks like a reasonable thing to be doing, assuming that
the output from IO::Socket::SSL is generally helpful.

> > > Maybe we shouldn't worry too much about that, but should instead put the
> > > invalid path into the error message:
> > > 
> > > 	die "CA path \"$smtp_ssl_cert_path\" does not exist.";
> > 
> > Given what I wrote above, yeah, I'd agree that is sufficient (and I do
> > think mentioning the path is helpful).
> 
> I'll change it to this in a re-roll.

Thanks.

-Peff

Re: [RFC/PATCH] send-email: die if CA path doesn't exist

[John Keeping]

On Tue, Nov 24, 2015 at 05:28:21PM -0500, Jeff King wrote:
> On Tue, Nov 24, 2015 at 10:17:08PM +0000, John Keeping wrote:
> 
> > I wonder if we should do this to help debug SSL issues:
> > 
> > -- >8 --
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index e057051..6d4e0ee 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1317,6 +1317,10 @@ Message-Id: $message_id
> >  			require Net::SMTP::SSL;
> >  			$smtp_domain ||= maildomain();
> >  			require IO::Socket::SSL;
> > +			if ($debug_net_smtp) {
> > +				no warnings 'once';
> > +				$IO::Socket::SSL::DEBUG = 1;
> > +			}
> >  			# Net::SMTP::SSL->new() does not forward any SSL options
> >  			IO::Socket::SSL::set_client_defaults(
> >  				ssl_verify_params());
> > -- 8< --
> 
> That certainly looks like a reasonable thing to be doing, assuming that
> the output from IO::Socket::SSL is generally helpful.

It's a bit verbose for errors, but it does let you know what went wrong:

DEBUG: .../IO/Socket/SSL.pm:1796: SSL connect attempt failed error:14090086:SSL routines:ssl3_get_server_certificate:certificate verify failed
DEBUG: .../IO/Socket/SSL.pm:673: fatal SSL error: SSL connect attempt failed error:14090086:SSL routines:ssl3_get_server_certificate:certificate verify failed
DEBUG: .../IO/Socket/SSL.pm:1780: IO::Socket::IP configuration failed

It doesn't print anything when the SSL connection is established
successfully, but I don't think that's a problem and if we jump to
level 2 it starts logging things like:

DEBUG: .../IO/Socket/SSL.pm:687: waiting for fd to become ready: SSL wants a read first
DEBUG: .../IO/Socket/SSL.pm:707: socket ready, retrying connect
DEBUG: .../IO/Socket/SSL.pm:677: ssl handshake in progress

without adding anything useful.

> > > > Maybe we shouldn't worry too much about that, but should instead put the
> > > > invalid path into the error message:
> > > > 
> > > > 	die "CA path \"$smtp_ssl_cert_path\" does not exist.";
> > > 
> > > Given what I wrote above, yeah, I'd agree that is sufficient (and I do
> > > think mentioning the path is helpful).
> > 
> > I'll change it to this in a re-roll.
> 
> Thanks.

[PATCH v2] send-email: die if CA path doesn't exist

[John Keeping]

If the CA path isn't found it's most likely to indicate a
misconfiguration, in which case accepting any certificate is unlikely to
be the correct thing to do.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
Changes since v1:
- add missing path to error message
- remove trailing '.' on error message since die appends "at
  /path/to/git-send-email line ..."

 git-send-email.perl | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 8e4c0e1..68c93fa 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1196,8 +1196,7 @@ sub ssl_verify_params {
 		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());
+		die "CA path \"$smtp_ssl_cert_path\" does not exist";
 	}
 }
 
-- 
2.6.3.462.gbe2c914

Re: [PATCH v2] send-email: die if CA path doesn't exist

[Jeff King]

On Tue, Nov 24, 2015 at 11:31:40PM +0000, John Keeping wrote:

> If the CA path isn't found it's most likely to indicate a
> misconfiguration, in which case accepting any certificate is unlikely to
> be the correct thing to do.

Thanks.

> Changes since v1:
> - add missing path to error message
> - remove trailing '.' on error message since die appends "at
>   /path/to/git-send-email line ..."

It won't if the error message ends with a newline. We seem to be wildly
inconsistent about that in send-email, though.

-Peff

Re: [PATCH v2] send-email: die if CA path doesn't exist

[John Keeping]

On Tue, Nov 24, 2015 at 06:35:36PM -0500, Jeff King wrote:
> On Tue, Nov 24, 2015 at 11:31:40PM +0000, John Keeping wrote:
> 
> > If the CA path isn't found it's most likely to indicate a
> > misconfiguration, in which case accepting any certificate is unlikely to
> > be the correct thing to do.
> 
> Thanks.
> 
> > Changes since v1:
> > - add missing path to error message
> > - remove trailing '.' on error message since die appends "at
> >   /path/to/git-send-email line ..."
> 
> It won't if the error message ends with a newline. We seem to be wildly
> inconsistent about that in send-email, though.

Interesting.  I think in this case it would definitely be better to add
the newline and avoid printing the location in the script, but it may
make more sense to have a separate pass over git-send-email.perl and fix
all of the die() calls.

I suspect that everything except the equivalent of BUG() should be
suppressing the location in a user-facing script like this.

Re: [PATCH v2] send-email: die if CA path doesn't exist

[Jeff King]

On Wed, Nov 25, 2015 at 10:19:09AM +0000, John Keeping wrote:

> > > Changes since v1:
> > > - add missing path to error message
> > > - remove trailing '.' on error message since die appends "at
> > >   /path/to/git-send-email line ..."
> > 
> > It won't if the error message ends with a newline. We seem to be wildly
> > inconsistent about that in send-email, though.
> 
> Interesting.  I think in this case it would definitely be better to add
> the newline and avoid printing the location in the script, but it may
> make more sense to have a separate pass over git-send-email.perl and fix
> all of the die() calls.
> 
> I suspect that everything except the equivalent of BUG() should be
> suppressing the location in a user-facing script like this.

Yeah, I think I'd agree. Your patch is merged to next, so we'd want a
separate patch to fix. And I agree that a whole pass over the script
probably makes sense.

In past projects I have also used a $SIG{__DIE__} handler to massage
errors into a nicer format, but it unfortunately gets pretty deep into
Perl voodoo.

-Peff