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

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 <redacted>
---
 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

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

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:

quoted

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.";

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

quoted

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

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:

quoted

quoted

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< --

quoted

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.

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

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

quoted

quoted

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

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:

quoted

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.

quoted

quoted

quoted

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.

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 <redacted>
---
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

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

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:

quoted

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.

quoted

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.

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

quoted

quoted

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