Subject: Re: [PATCH] send-email: If the ca path is not specified, use the defaults

From: Junio C Hamano <[email-masked]>
Date: Wed, 15 Jan 2014 13:30:27 -0800

Ruben Kerkhof <[email-masked]> writes:

>> ... because?  Is it because the cert_path on your platform is
>> different from /etc/ssl/certs?  What platform was this anyway?
>
> This is Fedora rawhide, git-1.8.5.2-1.fc21.x86_64, perl-IO-Socket-SSL-1.962-1.fc21.noarch
>> 
>> I see in the original discussion in your bugzilla entry that
>> "/etc/ssl/certs/" _is_ your ssl cert directory, but I wonder why
>> then this part of the existing codepath does not work for you:
>
> Actually, it's not a directory but a symlink to a directory:
>
> [ruben@vm ~]$ ls -l /etc/ssl/certs
> lrwxrwxrwx. 1 root root 16 Jan 11 12:04 /etc/ssl/certs -> ../pki/tls/certs
>
> Just to rule that out I set smtpsslcertpath = /etc/pki/tls/certs,
> but that doesn't work either.

Yeah, I suspect as much, as "-d" test would dereference symlinks and
would see /etc/ssl/certs is a symlink pointing at a directory.

> ...  I posted the patch to Fedora's bugzilla, since this was
> biting me on Fedora, and Igor took the liberty to forward it.  Not
> that I mind of course, but please don't take this patch as a
> proper fix. I don't pretend to fully understand the code and the
> implications, much less the impact on other platforms.

That is fine, and thanks for starting discussion for a proper fix
(the "thanks" go to both of you).

>> Also, if the above observation is correct, i.e. we are calling
>> IO::Socket::SSL with SSL_ca_path set to a correct directory but
>> somehow your platform is misbehaving and not detecting it as a
>> proper SSL_ca_path, then it could be argued that the code before
>> this patch catered people on platforms with one class of breakage,
>> namely "IO::Socket::SSL does not work with default configuration
>> without SSL_ca_file nor SSL_ca_path", and the code with this patch
>> caters people on platforms with another class of breakage, namely
>> "IO::Socket::SSL does not work when told with SSL_ca_path where the
>> certificate directory is" (or it could be "/etc/ssl/certs is a
>> directory that ought to be usable as SSL_ca_path, but it lacks
>> necessary hash symlinks").  Sort of like robbing Peter to pay Paul,
>> which is not very nice in principle.
>
> I suspect that's exactly the case,...

Actually, there is another possibility.  Perhaps on your system,
even though the current code thinks /etc/ssl/certs/ is usable as
SSL_ca_path, the directory is not meant to be usable as such, and
the default OpenSSL uses the equivalent of SSL_ca_file and uses the
single certificate bundle file without consulting other stuff in the
directory.

And that is not a broken installation at all, which is sort of
consistent with your observation here: 

> ...
> As a last check, I set smtpsslcertpath = /etc/pki/tls/cert.pem in
> ~/.gitconfig and git-send-email works fine now.

Which would mean that the existing code, by blindly defaulting to
/etc/ssl/certs/ and misdiagnosing that the directory is meant to be
used as SSL_ca_path, breaks a set-up that otherwise _should_ work.

I see that the original change that introduced the defaulting to
/etc/ssl/certs/, which is 35035bbf (send-email: be explicit with SSL
certificate verification, 2013-07-18), primarily wanted to avoid
letting Net::SMTP::SSL defaulting to no verification and causing it
to emit warnings of the use of the insecure default.  And I think
the same outcome will result with your patch.  By default, we still
insist on using SSL_VERIFY_PEER(), not SSL_VERIFY_NONE(), which
would avoid defaulting to insecure communication and triggering the
warning.  The way to disable the security by setting ssl_cert_path
to an empty string is unchanged.

Ram (who did 35035bbf), with the patch from Ruben in the thread, do
you get either the warning or SSL failure?  Conceptually, the
resulting code is much better, I think, without blindly defaulting
/etc/ssl/certs and instead of relying on the underlying platform
just working out of the box with its own default, and I would be
happy if we can apply the change without regression to existing
users.