Re: Setting min/max TLS protocol in clientside libpq

> On 27 Jan 2020, at 07:01, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Fri, Jan 24, 2020 at 12:19:31PM +0100, Daniel Gustafsson wrote:
>> Attached is a v5 of the patch which hopefully address all the comments raised,
>> sorry for the delay.
>
> Thanks for the new version.

Thanks for review and hackery!

> psql: error: could not connect to server: invalid or unsupported
> maximum protocol version specified: TLSv1.3
> Running the regression tests with OpenSSL 1.0.1, 1.0.2 or 1.1.0 fails,
> because TLSv1.3 (TLS1_3_VERSION) is not supported in those versions.
> I think that it is better to just rely on TLSv1.2 for all that,
> knowing that the server default for the minimum bound is v1.2.

Yes, of course, brainfade on my part.

> + {"sslminprotocolversion", "PGSSLMINPROTOCOLVERSION", NULL,
> NULL,
> + "SSL-Minimum-Protocol-Version", "", /*
> sizeof("TLSv1.x") */ 7,
> + offsetof(struct pg_conn, sslminprotocolversion)},
> +
> + {"sslmaxprotocolversion", "PGSSLMAXPROTOCOLVERSION", NULL,
> NULL,
> + "SSL-Maximum-Protocol-Version", "", /*
> sizeof("TLSv1.x") */ 7,
> Missing a zero-terminator in the count here. And actually
> gssencmode is wrong as well.. I'll report that on a different
> thread.

Nice catch, I plead guilty to copy-pasting and transferring the error.

> +bool
> +pq_verify_ssl_protocol_option(const char *protocolversion)
> [...]
> +bool
> +pq_verify_ssl_protocol_range(const char *min, const char *max)
> Both routines are just used in fe-connect.c to validate the connection
> parameters, so it is better to keep them static and in fe-connect.c in
> my opinion.

Ok. I prefer to keep the TLS code collected in fe-secure.c, but I don't have
strong enough opinions to kick up a fuzz.

> Hm. I am not sure that having a separate section "Client Protocol
> Usage" brings much, so I have removed this one, and added an extra
> sentence for the maximum protocol regarding its value for testing or
> protocol compatibility.

I'm not convinced, this forces the reader to know what to look for (the
connection parameters) rather than being informed. If anything, I think we
need more explanatory sections in the docs.

> The regression tests of postgres_fdw failed because of the new
> parameters. One update later, they run fine.

Doh, thanks.

> So, attached is an updated version of the patch that I have spent a
> couple of hours polishing. What do you think?

Overall a +1 on this version, thanks for picking it up!

cheers ./daniel