Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

On Fri, Apr 14, 2023 at 7:20 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> And again with the attachment.

After some sleep... From inspection I think the final EOF branch could
be masked by the new branch, if verification has failed but was already
ignored.

To test that, I tried hanging up on the client partway through the
server handshake, and I got some strange results. With the patch, using
sslmode=require and OpenSSL 1.0.1, I see:

connection to server at "127.0.0.1", port 50859 failed: SSL error:
certificate verify failed: self signed certificate

Which is wrong -- we shouldn't care about the self-signed failure if
we're not using verify-*. I was going to suggest a patch like the following:

> if (r == -1)
> - libpq_append_conn_error(conn, "SSL SYSCALL error: %s",
> - SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
> + {
> + /*
> + * If we get an X509 error here without an error in the
> + * socket layer it means that verification failed without
> + * it being a protocol error. A common cause is trying to
> + * a default system CA which is missing or broken.
> + */
> + if (!save_errno && vcode != X509_V_OK)
> + libpq_append_conn_error(conn, "SSL error: certificate verify failed: %s",
> + X509_verify_cert_error_string(vcode));
> + else
> + libpq_append_conn_error(conn, "SSL SYSCALL error: %s",
> + SOCK_STRERROR(save_errno, sebuf, sizeof(sebuf)));
> + }
> else
> libpq_append_conn_error(conn, "SSL SYSCALL error: EOF detected");

But then I tested my case against PG15, and I didn't get the EOF message
I expected:

connection to server at "127.0.0.1", port 50283 failed: SSL SYSCALL
error: Success

So it appears that this (hanging up on the client during the handshake)
is _also_ a case where we could get a SYSCALL error with a zero errno,
and my patch doesn't actually fix the misleading error message.

That makes me worried, but I don't really have a concrete suggestion to
make it better, yet. I'm not opposed to pushing this as a fix for the
tests, but I suspect we'll have to iterate on this more.

Thanks,
--Jacob