> On 27 Jan 2020, at 07:01, Michael Paquier <michael@paquier.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
