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

> On 14 Apr 2023, at 01:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Good points, it should of course be SOCK_ERRNO.  The attached saves off errno
>> and reinstates it to avoid clobbering.  Will test it on Windows in the morning
>> as well.
> 
> I think instead of this:
> 
> +                    SOCK_ERRNO_SET(save_errno);
> 
> you could just do this:
> 
>                         libpq_append_conn_error(conn, "SSL SYSCALL error: %s",
> -                                           SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
> +                                           SOCK_STRERROR(save_errno, sebuf, sizeof(sebuf)));
> 
> Although ... we're already assuming that SSL_get_error and ERR_get_error
> don't clobber errno.  Maybe SSL_get_verify_result doesn't either.
> Or we could make it look like this:
> 
> +    SOCK_ERRNO_SET(0);
>     ERR_clear_error();
>     r = SSL_connect(conn->ssl);
>     if (r <= 0)
> +       int            save_errno = SOCK_ERRNO;
>        int            err = SSL_get_error(conn->ssl, r);
>        unsigned long ecode;
> 
>        ...
> 
> -                                           SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
> +                                           SOCK_STRERROR(save_errno, sebuf, sizeof(sebuf)));
> 
> to remove all doubt.

I mainly put save_errno back into SOCK_ERRNO for greppability, I don't have any
strong opinions either way so I went with the latter suggestion.  Attached v3
does the above change and passes the tests both with a broken and working
system CA pool.  Unless objections from those with failing local envs I propose
this is pushed to close the open item.

--
Daniel Gustafsson