Re: Setting min/max TLS protocol in clientside libpq

Hello,

On 2019/12/04 2:37, Daniel Gustafsson wrote:
> The attached patch implements two new connection string variables for minimum
> and maximum TLS protocol version, mimicking how it's done in the backend. This
> does duplicate a bit of code from be-secure-openssl.c to cope with older
> versions of OpenSSL, but it seemed a too trivial duplication to create
> common/openssl.c (but others might disagree).

I've looked at the patch and I have a couple comments.

> + if (ssl_max_ver < ssl_min_ver)
> + {
> + printfPQExpBuffer(&conn->errorMessage,
> + libpq_gettext("invalid maximum SSL version specified, must be higher than minimum SSL version: %s\n"),
> + conn->sslmaxprotocolversion);
> + return -1;
> + }
> +
> + if (ssl_max_ver == -1)
> + {
> + printfPQExpBuffer(&conn->errorMessage,
> + libpq_gettext("invalid maximum SSL version specified: %s\n"),
> + conn->sslmaxprotocolversion);
> + return -1;
> + }

I think we should raise the error "invalid maximum SSL version
specified" earlier. If ssl_protocol_version_to_openssl() returns -1 and
ssl_min_ver is valid we never reach the condition "ssl_max_ver == -1".
Also it might confuse users to get the error "invalid maximum SSL
version specified, must be higher than minimum SSL version" instead of
former one.

Secondly I think the error "invalid maximum SSL version specified"
itself might confuse users, in the case if the input is good but a build
doesn't support desired version. So I think it is better to do two
checks here: check for a correct input and check if a build supports it.
In the second case we may raise "SSL version %s not supported by this
build". It is actually like backend does: guc.c checks for correct input
using ssl_protocol_versions_info and ssl_protocol_version_to_openssl()
checks if a build supports the version.

--
Arthur