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