threads / rfc / 40821

RFC patchsend-email: die if CA path doesn't exist

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

## tl;dr

11 messages between Nov 17, 2015 and Nov 25, 2015. Diffs are folded; open one to read it.

replies: 10people: 2as markdown or json

John Keeping· Nov 17, 2015, 22:12 UTC · lore

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

Show changes to git-send-email.perl +1 −2

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

Jeff King· Nov 20, 2015, 11:18 UTC · re: John Keeping · lore

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

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

John Keeping· Nov 20, 2015, 19:46 UTC · re: Jeff King · lore

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

On Fri, Nov 20, 2015 at 06:18:48AM -0500, Jeff King wrote:

Show 17 quoted lines

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

Jeff King· Nov 24, 2015, 19:58 UTC · re: John Keeping · lore

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

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

Show 16 quoted lines

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

John Keeping· Nov 24, 2015, 22:17 UTC · re: Jeff King · lore

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

On Tue, Nov 24, 2015 at 02:58:43PM -0500, Jeff King wrote:

Show 39 quoted lines

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

Show changes to git-send-email.perl +4 −1

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.

Jeff King· Nov 24, 2015, 22:28 UTC · re: John Keeping · lore

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

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

Show 19 quoted lines

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

Show 9 quoted lines

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

John Keeping· Nov 24, 2015, 23:10 UTC · re: Jeff King · lore

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

On Tue, Nov 24, 2015 at 05:28:21PM -0500, Jeff King wrote:

Show 24 quoted lines

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

Show 11 quoted lines

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

John Keeping· Nov 24, 2015, 23:31 UTC · re: John Keeping · lore

[PATCH v2] 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 <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(-)

Show changes to git-send-email.perl +1 −2

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

Jeff King· Nov 24, 2015, 23:35 UTC · re: John Keeping · lore

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

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

John Keeping· Nov 25, 2015, 10:19 UTC · re: Jeff King · lore

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

On Tue, Nov 24, 2015 at 06:35:36PM -0500, Jeff King wrote:

Show 15 quoted lines

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

Jeff King· Nov 25, 2015, 10:23 UTC · re: John Keeping · lore

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

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

Show 15 quoted lines

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