Setting min/max TLS protocol in clientside libpq

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Setting min/max TLS protocol in clientside libpq

Daniel Gustafsson
Responding to the recent thread on bumping the default TLS version, I realized
that we don't have a way to set the minimum/maximum TLS protocol version in
clientside libpq.  Setting the maximum protocol version obviously not terribly
important (possibly with the exception of misbehaving middle-boxes and
testing), but the minimum version can be quite useful to avoid misbehaving
and/or misconfigured servers etc.

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).

This can today be achieved by editing the local openssl configuration, but
having an override in libpq to tighten down the connection parameters make it
far easier for the user/application IMO.

cheers ./daniel


libpq_minmaxproto.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Artur Zakirov
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


Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

cary huang
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

Hello

I have applied the patch and did some basic testing with various combination of min and max TLS protocol versions. Overall the functionality works and the chosen TLS protocol aligns with the min/max TLS protocol settings on the PG server side.

I agree with Arthur that it makes sense to check the validity of "conn->sslmaxprotocolversion" first before checking if it is larger than "conn->sslminprotocolversion"

A small suggestion here. I see that PG server defaults TLS min version to be TLSv1.2 and max version to none. So by default the server accepts TLSv1.2 and above. I think on the client side, it also makes sense to have the same defaults as the server. In the patch, if the client does not supply "sslminprotocolversion", it will run to "else" statement and sets TLS min version to "INT_MIN", which is a huge negative number and of course openssl won't set it. I think this else statement can be enhanced a little to set "sslminprotocolversion" to TLSv1.2 by default to match the server and provide some warning message that TLS minimum has defaulted to TLSv1.2.

Cary
HighGo Software Canada
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Michael Paquier-2
On Thu, Jan 02, 2020 at 09:46:44PM +0000, cary huang wrote:
> I agree with Arthur that it makes sense to check the validity of
> "conn->sslmaxprotocolversion" first before checking if it is larger
> than "conn->sslminprotocolversion"

Here I don't agree.  Why not just let OpenSSL handle things with
SSL_CTX_set_min_proto_version?  We don't bother about that in the
backend code for that reason on top of keeping the code more simple
with less error handling.  And things are cleaner when it comes to
this libpq patch by giving up with the INT_MIN hack.

> A small suggestion here. I see that PG server defaults TLS min
> version to be TLSv1.2 and max version to none. So by default the
> server accepts TLSv1.2 and above. I think on the client side, it
> also makes sense to have the same defaults as the server.

Yeah, that makes sense.  Even more now that I have just removed
support for OpenSSL 0.9.8 and 1.0.0 ;)

There could be an argument to lower down the default if we count for
backends built with OpenSSL versions older than libpq, but I am not
ready to buy that there would be many of those.

> In the patch, if the client does not supply "sslminprotocolversion",
> it will run to "else" statement and sets TLS min version to "INT_MIN",
> which is a huge negative number and of course openssl won't set
> it. I think this else statement can be enhanced a little to set
> "sslminprotocolversion" to TLSv1.2 by default to match the server
> and provide some warning message that TLS minimum has defaulted to
> TLSv1.2.

In this patch fe-secure-openssl.c has just done a copy-paste of
SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version
present in be-secure-openssl.c.  That's not good.  Could you refactor
that please as a separate file?  For example openssl-protocol.c in
src/common/?  src/common/ stuff is built with -fPIC since 7143b3e so
there is no need to include directly the source files in the
Makefile.  A shame you cannot do that for
ssl_protocol_version_to_openssl(), so for that a note would be welcome
on top of the former backend routine and the one you are adding.

The patch has conflicts with libpq-int.h as far as I can see.  That
should be easy enough to solve.

The patch should have tests in src/test/ssl/, like for invalid values,
incorrect combinations leading to failures, etc.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Magnus Hagander-2
On Mon, Jan 6, 2020 at 7:02 AM Michael Paquier <[hidden email]> wrote:

>
> On Thu, Jan 02, 2020 at 09:46:44PM +0000, cary huang wrote:
> > I agree with Arthur that it makes sense to check the validity of
> > "conn->sslmaxprotocolversion" first before checking if it is larger
> > than "conn->sslminprotocolversion"
>
> Here I don't agree.  Why not just let OpenSSL handle things with
> SSL_CTX_set_min_proto_version?  We don't bother about that in the
> backend code for that reason on top of keeping the code more simple
> with less error handling.  And things are cleaner when it comes to
> this libpq patch by giving up with the INT_MIN hack.
>
> > A small suggestion here. I see that PG server defaults TLS min
> > version to be TLSv1.2 and max version to none. So by default the
> > server accepts TLSv1.2 and above. I think on the client side, it
> > also makes sense to have the same defaults as the server.
>
> Yeah, that makes sense.  Even more now that I have just removed
> support for OpenSSL 0.9.8 and 1.0.0 ;)
>
> There could be an argument to lower down the default if we count for
> backends built with OpenSSL versions older than libpq, but I am not
> ready to buy that there would be many of those.

Not having thought about it in much detail, but it's a fairly common
scenario to have a much newer version of libpq (and the platform it's
built on) than the server. E.g. a v12 libpq against a v9.6 postgres
server is very common. For example, debian based systems will
auto-upgrade your libpq, but not your server (for obvious reasons).
And it's also quite common to upgrade platforms for the application
much more frequently than the database server platform.

--
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/


Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Tom Lane-2
Magnus Hagander <[hidden email]> writes:
> Not having thought about it in much detail, but it's a fairly common
> scenario to have a much newer version of libpq (and the platform it's
> built on) than the server. E.g. a v12 libpq against a v9.6 postgres
> server is very common. For example, debian based systems will
> auto-upgrade your libpq, but not your server (for obvious reasons).
> And it's also quite common to upgrade platforms for the application
> much more frequently than the database server platform.

Yeah, there's a reason why we expect pg_dump and psql to function with
ancient server versions.  We shouldn't break this scenario with
careless rejiggering of libpq's connection defaults.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Michael Paquier-2
On Mon, Jan 06, 2020 at 09:37:54AM -0500, Tom Lane wrote:

> Magnus Hagander <[hidden email]> writes:
>> Not having thought about it in much detail, but it's a fairly common
>> scenario to have a much newer version of libpq (and the platform it's
>> built on) than the server. E.g. a v12 libpq against a v9.6 postgres
>> server is very common. For example, debian based systems will
>> auto-upgrade your libpq, but not your server (for obvious reasons).
>> And it's also quite common to upgrade platforms for the application
>> much more frequently than the database server platform.
>
> Yeah, there's a reason why we expect pg_dump and psql to function with
> ancient server versions.  We shouldn't break this scenario with
> careless rejiggering of libpq's connection defaults.
Good points.  Let's not do that then.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Daniel Gustafsson
In reply to this post by Michael Paquier-2
Thanks for review everyone!  A v2 of the patch which I believe addresses all
concerns raised is attached.

> On 6 Jan 2020, at 07:01, Michael Paquier <[hidden email]> wrote:
>
> On Thu, Jan 02, 2020 at 09:46:44PM +0000, cary huang wrote:
>> I agree with Arthur that it makes sense to check the validity of
>> "conn->sslmaxprotocolversion" first before checking if it is larger
>> than "conn->sslminprotocolversion"
>
> Here I don't agree.  Why not just let OpenSSL handle things with
> SSL_CTX_set_min_proto_version?  We don't bother about that in the
> backend code for that reason on top of keeping the code more simple
> with less error handling.  And things are cleaner when it comes to
> this libpq patch by giving up with the INT_MIN hack.
I looked into this and it turns out that OpenSSL does nothing to prevent the
caller from setting a nonsensical protocol range like min=tlsv1.3,max=tlsv1.1.
Thus, it's quite easy to screw up the backend server config and get it to start
properly, but with quite unrelated error messages as a result on connection.

Since I think this needs to be dealt with for both backend and frontend (if
this is accepted), I removed it from this patch to return to it in a separate
thread.

>> In the patch, if the client does not supply "sslminprotocolversion",
>> it will run to "else" statement and sets TLS min version to "INT_MIN",
>> which is a huge negative number and of course openssl won't set
>> it. I think this else statement can be enhanced a little to set
>> "sslminprotocolversion" to TLSv1.2 by default to match the server
>> and provide some warning message that TLS minimum has defaulted to
>> TLSv1.2.
>
> In this patch fe-secure-openssl.c has just done a copy-paste of
> SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version
> present in be-secure-openssl.c.  That's not good.  Could you refactor
> that please as a separate file?
Done.  I opted for a more generic header to make usage of the code easier, not
sure if thats ok.

One thing I noticed when looking at it is that we now have sha2_openssl.c and
openssl_protocol.c in src/common.  For easier visual grouping of OpenSSL
functionality, it makes sense to me to rename sha2_openssl.c to openssl_sha2.c,
but that might just be pointless churn.

> The patch should have tests in src/test/ssl/, like for invalid values,
> incorrect combinations leading to failures, etc.

Also done.

cheers ./daniel





libpq_minmaxproto_v2.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Michael Paquier-2
On Fri, Jan 10, 2020 at 12:01:36AM +0100, Daniel Gustafsson wrote:
> I looked into this and it turns out that OpenSSL does nothing to prevent the
> caller from setting a nonsensical protocol range like min=tlsv1.3,max=tlsv1.1.
> Thus, it's quite easy to screw up the backend server config and get it to start
> properly, but with quite unrelated error messages as a result on connection.

FWIW, here is the error produced, and that's confusing:
$ psql -d "host=localhost sslmode=require"
psql: error: could not connect to server: SSL error: tlsv1 alert
internal error

> Since I think this needs to be dealt with for both backend and frontend (if
> this is accepted), I removed it from this patch to return to it in a separate
> thread.

HEAD and back branches only care about the backend, so I think that we
should address this part first as your patch would I guess reuse the
interface we finish by using for the backend.  Looking at OpenSSL, I
agree that there is no internal logic to perform sanity checks on the
min/max bounds.  Still I can see that OpenSSL 1.1.0 has added some
"get" routines for SSL_CTX_set_min/max_proto_version:
https://www.openssl.org/docs/man1.1.0/man3/SSL_CTX_set_min_proto_version.html

Hmmmmeuh.  It would be perfect to rely only on OpenSSL for that part
to bring some sanity, and compare the results fetched from the SSL
context so as we don't have to worry about special cases in with the
GUC reload if the parameter is not set, or the parameter value is not
supported.  Now, OpenSSL <= 1.0.2 cannot do that, and you can get the
values set only after doing the set, so adding the compatibility
argument it is much more tempting to use our
ssl_protocol_version_to_openssl() wrapper and complain iff:
- both the min and max are supported values.
- min/max are incompatible.
And the check needs to be done before attempting to set the min/max
protos so as you don't finish with an incorrect intermediate state.
Daniel, are you planning to start a new thread?

> One thing I noticed when looking at it is that we now have sha2_openssl.c and
> openssl_protocol.c in src/common.  For easier visual grouping of OpenSSL
> functionality, it makes sense to me to rename sha2_openssl.c to openssl_sha2.c,
> but that might just be pointless churn.

Databases like consistency, and so do I, so no issues from me to do a
rename of the sha2.c file.  That makes sense with the addition of the
new file.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Daniel Gustafsson
> On 11 Jan 2020, at 03:49, Michael Paquier <[hidden email]> wrote:

> Hmmmmeuh.  It would be perfect to rely only on OpenSSL for that part
> to bring some sanity, and compare the results fetched from the SSL
> context so as we don't have to worry about special cases in with the
> GUC reload if the parameter is not set, or the parameter value is not
> supported.

I'm not convinced about this, but since there is a thread opened for discussing
the range check let's take it over there.

> Daniel, are you planning to start a new thread?

I was going to, but you beat me to it =)

>> One thing I noticed when looking at it is that we now have sha2_openssl.c and
>> openssl_protocol.c in src/common.  For easier visual grouping of OpenSSL
>> functionality, it makes sense to me to rename sha2_openssl.c to openssl_sha2.c,
>> but that might just be pointless churn.
>
> Databases like consistency, and so do I, so no issues from me to do a
> rename of the sha2.c file.  That makes sense with the addition of the
> new file.

Done in the attached v3.

cheers ./daniel


libpq_minmaxproto_v3.patch (26K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Tom Lane-2
Daniel Gustafsson <[hidden email]> writes:
>> On 11 Jan 2020, at 03:49, Michael Paquier <[hidden email]> wrote:
>>> One thing I noticed when looking at it is that we now have sha2_openssl.c and
>>> openssl_protocol.c in src/common.  For easier visual grouping of OpenSSL
>>> functionality, it makes sense to me to rename sha2_openssl.c to openssl_sha2.c,
>>> but that might just be pointless churn.

>> Databases like consistency, and so do I, so no issues from me to do a
>> rename of the sha2.c file.  That makes sense with the addition of the
>> new file.

> Done in the attached v3.

I'm kind of down on renaming files unless there is a *really* strong
reason for it.  It makes back-patching more difficult and it makes
it much harder to follow the git history.  And, seeing that there is
also a src/common/sha2.c, it seems to me that renaming sha2_openssl.c
will just break consistency in a different way.

Maybe the problem is you've got the new file's name backwards.
Maybe it should be protocol_openssl.c.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Daniel Gustafsson
> On 14 Jan 2020, at 15:49, Tom Lane <[hidden email]> wrote:
>
> Daniel Gustafsson <[hidden email]> writes:
>>>> On 11 Jan 2020, at 03:49, Michael Paquier <[hidden email]> wrote:
>>>> One thing I noticed when looking at it is that we now have sha2_openssl.c and
>>>> openssl_protocol.c in src/common.  For easier visual grouping of OpenSSL
>>>> functionality, it makes sense to me to rename sha2_openssl.c to openssl_sha2.c,
>>>> but that might just be pointless churn.
>
>>> Databases like consistency, and so do I, so no issues from me to do a
>>> rename of the sha2.c file.  That makes sense with the addition of the
>>> new file.
>
>> Done in the attached v3.
>
> I'm kind of down on renaming files unless there is a *really* strong
> reason for it.  It makes back-patching more difficult and it makes
> it much harder to follow the git history.  And, seeing that there is
> also a src/common/sha2.c, it seems to me that renaming sha2_openssl.c
> will just break consistency in a different way.
>
> Maybe the problem is you've got the new file's name backwards.
> Maybe it should be protocol_openssl.c.

Thats a very good argument, I’ll send a v4 with protocol_openssl.c when back at the computer.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Daniel Gustafsson
> On 14 Jan 2020, at 16:15, Daniel Gustafsson <[hidden email]> wrote:

>
>> On 14 Jan 2020, at 15:49, Tom Lane <[hidden email]> wrote:
>>
>> Daniel Gustafsson <[hidden email]> writes:
>>>>> On 11 Jan 2020, at 03:49, Michael Paquier <[hidden email]> wrote:
>>>>> One thing I noticed when looking at it is that we now have sha2_openssl.c and
>>>>> openssl_protocol.c in src/common.  For easier visual grouping of OpenSSL
>>>>> functionality, it makes sense to me to rename sha2_openssl.c to openssl_sha2.c,
>>>>> but that might just be pointless churn.
>>
>>>> Databases like consistency, and so do I, so no issues from me to do a
>>>> rename of the sha2.c file.  That makes sense with the addition of the
>>>> new file.
>>
>>> Done in the attached v3.
>>
>> I'm kind of down on renaming files unless there is a *really* strong
>> reason for it.  It makes back-patching more difficult and it makes
>> it much harder to follow the git history.  And, seeing that there is
>> also a src/common/sha2.c, it seems to me that renaming sha2_openssl.c
>> will just break consistency in a different way.
>>
>> Maybe the problem is you've got the new file's name backwards.
>> Maybe it should be protocol_openssl.c.
>
> Thats a very good argument, I’ll send a v4 with protocol_openssl.c when back at the computer.
Files renamed to match existing naming convention, the rest of the patch left
unchanged.

cheers ./daniel


libpq_minmaxproto_v4.patch (25K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Michael Paquier-2
On Tue, Jan 14, 2020 at 11:01:00PM +0100, Daniel Gustafsson wrote:
> Files renamed to match existing naming convention, the rest of the patch left
> unchanged.

+   if ((pg_strcasecmp("tlsv1", protocol) == 0) || pg_strcasecmp("tlsv1.0", protocol) == 0)
+       return TLS1_VERSION;
"TLSv1.0" is not a supported grammar in the backend.  So I would just
drop it in libpq.  It is also not documented.

+ * Portions Copyright (c) 2018-2020, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *       src/common/protocol_openssl.c
It is a nobrainer to just use those lines for copyright notices
instead:
 * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California

+     <varlistentry id="libpq-connect-sslminprotocolversion"
xreflabel="sslminprotocolversion">
+      <term><literal>sslminprotocolversion</literal></term>
Got to wonder if we had better not use underscores for those new
parameter names as they are much longer than their cousins.
Underscores would make things more inconsistent.

+       if (ssl_min_ver == -1)
+       {
+           printfPQExpBuffer(&conn->errorMessage,
+                             libpq_gettext("invalid minimum protocol version specified: %s\n"),
+                             conn->sslminprotocolversion);
+           return -1;
+       }
[...]
+       if (ssl_max_ver == -1)
+       {
+           printfPQExpBuffer(&conn->errorMessage,
+                             libpq_gettext("invalid or unsupported maximum protocol version specified: %s\n"),
+                             conn->sslmaxprotocolversion);
+           return -1;
+       }
Error messages for the min/max are inconsistent.  I would just use
"unsupported", because...

Following with your complain on the other thread about the GUC
handling for the min/max protocol parameter. Shouldn't you validate
the supported values in connectOptions2() like what's done for the
other parameters?  This way, you can make the difference between an
invalid value and an unsupported value with two different error
strings.  By validating the values at an earlier stage, you save a
couple of cycles for the application.

+        <literal>TLSv1.3</literal>.  The supported protocols depend on the
+        version of <productname>OpenSSL</productname> used, older versions
+        doesn't support the modern protocol versions.
Incorrect grammar => s/doesn't/don't/.

It would be good to mention that the default is no value, meaning that
the minimum and/or the maximum are not enforced in the SSL context.

+   if (conn->sslminprotocolversion)
+   {
[...]
+   if (conn->sslmaxprotocolversion)
+   {
You are missing two checks for empty strings here (aka strlen == 0).
These should be treated the same as no value to enforce the protocol
to.  (Let's not add an alias for "any".)

+ * Convert TLS protocol versionstring to OpenSSL values
Space needed here => "version string".

A nit, perhaps unnecessary, but I would use "TLSv1.1", etc. in the
values harcoded for libpq.  That's easier to grep after, and
consistent with the existing conventions even if you apply a
case-insensitive comparison.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Michael Paquier-2
On Wed, Jan 15, 2020 at 02:58:09PM +0900, Michael Paquier wrote:
> On Tue, Jan 14, 2020 at 11:01:00PM +0100, Daniel Gustafsson wrote:
>> Files renamed to match existing naming convention, the rest of the patch left
>> unchanged.
>
> [previous review]

One thing I remembered after sleeping on it is that we can split the
patch into two parts: the refactoring pieces and the addition of the
options for libpq.  The previous review mostly impacts the libpq part,
and the split is straight-forward, so attached is a patch for only the
refactoring pieces with some fixes and tweaks.  I have tested it with
and without OpenSSL, using 1.0.2 and 1.1.0 on Linux and Windows
(MSVC).  Those tests have allowed me to find an error in the previous
patch that I missed: the new files openssl.h and protocol_openssl.c
still declared SSL_CTX_set_min/max_proto_version as static functions,
so compilation was broken when trying to use OpenSSL <= 1.0.2.

If that looks fine, I would like to get that part committed first.
Daniel, any thoughts?
--
Michael

openssl-proto-refactor-v1.patch (7K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Daniel Gustafsson
> On 16 Jan 2020, at 04:22, Michael Paquier <[hidden email]> wrote:
>
> On Wed, Jan 15, 2020 at 02:58:09PM +0900, Michael Paquier wrote:
>> On Tue, Jan 14, 2020 at 11:01:00PM +0100, Daniel Gustafsson wrote:
>>> Files renamed to match existing naming convention, the rest of the patch left
>>> unchanged.
>>
>> [previous review]
>
> One thing I remembered after sleeping on it is that we can split the
> patch into two parts: the refactoring pieces and the addition of the
> options for libpq.

Correct, they are mostly independent (the refactoring doesn't make a lot of
sense without the follow-up patch, but the min/max patch can be kept more
readable without the refactoring in it as well).

> The previous review mostly impacts the libpq part,
> and the split is straight-forward, so attached is a patch for only the
> refactoring pieces with some fixes and tweaks.  I have tested it with
> and without OpenSSL, using 1.0.2 and 1.1.0 on Linux and Windows
> (MSVC).  Those tests have allowed me to find an error in the previous
> patch that I missed: the new files openssl.h and protocol_openssl.c
> still declared SSL_CTX_set_min/max_proto_version as static functions,
> so compilation was broken when trying to use OpenSSL <= 1.0.2.

Doh .. thanks.

> If that looks fine, I would like to get that part committed first.
> Daniel, any thoughts?

The patch looks fine to me, I don't an issue with splitting it into a
refactoring patch and a TLS min/max version patch.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Michael Paquier-2
On Thu, Jan 16, 2020 at 09:56:01AM +0100, Daniel Gustafsson wrote:
> The patch looks fine to me, I don't an issue with splitting it into a
> refactoring patch and a TLS min/max version patch.

Thanks, committed the refactoring part then.  If the buildfarm breaks
for a reason or another, the window to look at is narrower than if we
had the full set of changes, and the git history is cleaner.  I
noticed as well a compilation warning when compiling with OpenSSL
1.0.2 from protocol_openssl.c because of missing declarations of the
two routines because the header declaration was incorrect.

Could you please rebase and fix the remaining pieces of the patch?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Setting min/max TLS protocol in clientside libpq

Michael Paquier-2
On Fri, Jan 17, 2020 at 10:09:54AM +0900, Michael Paquier wrote:
> Could you please rebase and fix the remaining pieces of the patch?

And while I remember, you may want to add checks for incorrect bounds
when validating the values in fe-connect.c...  The same arguments as
for the backend part apply because we'd want to make the
implementation a maximum pluggable with all SSL libraries.
--
Michael

signature.asc (849 bytes) Download Attachment