Обсуждение: Setting min/max TLS protocol in clientside libpq

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

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

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.
Overallthe functionality works and the chosen TLS protocol aligns with the min/max TLS protocol settings on the PG
serverside.
 

I agree with Arthur that it makes sense to check the validity of "conn->sslmaxprotocolversion" first before checking if
itis 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
defaultthe server accepts TLSv1.2 and above. I think on the client side, it also makes sense to have the same defaults
asthe server. In the patch, if the client does not supply "sslminprotocolversion", it will run to "else" statement and
setsTLS min version to "INT_MIN", which is a huge negative number and of course openssl won't set it. I think this else
statementcan be enhanced a little to set "sslminprotocolversion" to TLSv1.2 by default to match the server and provide
somewarning message that TLS minimum has defaulted to TLSv1.2.
 

Cary
HighGo Software Canada

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

On Mon, Jan 6, 2020 at 7:02 AM Michael Paquier <michael@paquier.xyz> 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/

Magnus Hagander <magnus@hagander.net> 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

On Mon, Jan 06, 2020 at 09:37:54AM -0500, Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> 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

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 <michael@paquier.xyz> 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

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

> On 11 Jan 2020, at 03:49, Michael Paquier <michael@paquier.xyz> 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

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 11 Jan 2020, at 03:49, Michael Paquier <michael@paquier.xyz> 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

> On 14 Jan 2020, at 15:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>>>> On 11 Jan 2020, at 03:49, Michael Paquier <michael@paquier.xyz> 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

> On 14 Jan 2020, at 16:15, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 14 Jan 2020, at 15:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Daniel Gustafsson <daniel@yesql.se> writes:
>>>>> On 11 Jan 2020, at 03:49, Michael Paquier <michael@paquier.xyz> 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

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

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

> On 16 Jan 2020, at 04:22, Michael Paquier <michael@paquier.xyz> 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

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

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

> On 17 Jan 2020, at 03:38, Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.

Agreed.

Attached is a v5 of the patch which hopefully address all the comments raised,
sorry for the delay.

cheers ./daniel

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.

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.

+test_connect_fails(
+       $common_connstr,
+       "sslrootcert=ssl/root+server_ca.crt sslmode=require
sslminprotocolversion=TLSv1.3 sslmaxprotocolversion=tlsv1.2",
+       qr/invalid protocol version range/,
+       "connect with an incorrect range of TLS protocol versions
leaving no versions allowed");
+
+test_connect_fails(
+       $common_connstr,
+       "sslrootcert=ssl/root+server_ca.crt sslmode=require
sslminprotocolversion=TLSv1.3 sslmaxprotocolversion=tlsv1",
+       qr/invalid protocol version range/,
+       "connect with an incorrect range of TLS protocol versions
leaving no versions allowed");
This is testing twice pattern the same thing, and I am not sure if is
is worth bothering about the special case with TLSv1 (using just a
comparison with pg_strcasecmp you don't actually need those special
checks..).

Tests should make sure that a failure happens when an incorrect value
is set for sslminprotocolversion and sslmaxprotocolversion.

For readability, I think that it is better to consider NULL or empty
values for the parameters to be valid.  They are actually valid
values, because they just get ignored when creating the connection.

Adding an assertion within the routine for the protocol range check to
make sure that the values are valid makes the code more robust.

+       {"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.

+# Test min/mix protocol versions
Typo here.

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

+       if (*(min + strlen("TLSv1.")) > *(max + strlen("TLSv1.")))
+               return false;
It is enough to use pg_strcasecmp() here.

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.

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

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

> 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

On Mon, Jan 27, 2020 at 09:49:09AM +0100, Daniel Gustafsson wrote:
>> On 27 Jan 2020, at 07:01, Michael Paquier <michael@paquier.xyz> wrote:
> 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.

They are parameter-related, so fe-connect.c made the most sense to me.
The routine checking after the range makes the code more readable IMO
even if we only use it in one place.

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

Thanks.  I have committed the bulk of the changes.  As mentioned
previously, I still have doubts about the value of the new section for
the new protocol usage.  Once reworded a bit, I finish with the
attached, and the following paragraph for libpq.sgml:
+ <sect2>
+  <title>Client Protocol Usage</title>
+  <para>
+   When connecting using SSL, the client and server negotiate which protocol
+   to use for the connection. <productname>PostgreSQL</productname> supports
+   <literal>TLSv1</literal>, <literal>TLSv1.1</literal>,
+   <literal>TLSv1.2</literal> and <literal>TLSv1.3</literal>, but the
+   protocols available depend on the version of
+   <productname>OpenSSL</productname> that the client and the backend are
+   using. The minimum requested version can be specified with
+   <literal>sslminprotocolversion</literal>, which will ensure that the
+   connection uses that protocol version or higher. The maximum requested
+   version can be specified with <literal>sslmaxprotocolversion</literal>.
+  </para>
+ </sect2>

Now, we already mention in the docs which values the min and max
bounds support, and that the version of OpenSSL used by the backend
and the frontend are impacted by that depending on what version of
OpenSSL one or the other link to.  The remaining piece is that the
client and the server negotiate the protocol they use, which is an
obvious fact, at least to me..
--
Michael

> On 28 Jan 2020, at 04:53, Michael Paquier <michael@paquier.xyz> wrote:

> Now, we already mention in the docs which values the min and max
> bounds support, and that the version of OpenSSL used by the backend
> and the frontend are impacted by that depending on what version of
> OpenSSL one or the other link to.  The remaining piece is that the
> client and the server negotiate the protocol they use, which is an
> obvious fact, at least to me..

You don't really qualify as the target audience for basic, yet not always
universally known/understood, sections in the documentation though =) I've
heard enough complaints that it's complicated to set up that I think we need to
make the docs more digestable, but if noone else +1's then lets drop it.

cheers ./daniel

On Tue, Jan 28, 2020 at 11:29:39AM +0100, Daniel Gustafsson wrote:
> You don't really qualify as the target audience for basic, yet not always
> universally known/understood, sections in the documentation though =)

Likely I don't.

> I've heard enough complaints that it's complicated to set up that I
> think we need to make the docs more digestable, but if noone else
> +1's then lets drop it.

Sure.  Now I am pretty sure that we would need a bit more than just
saying that the SSL protocol is negotiated between the backend and
libpq if we add a new section.
--
Michael

Can we reconsider whether we really want to name the new settings like 
"sslminprotocolversion", or whether we could add some underscores, both 
for readability and for consistency with the server-side options?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 24 Apr 2020, at 12:56, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> Can we reconsider whether we really want to name the new settings like "sslminprotocolversion", or whether we could
addsome underscores, both for readability and for consistency with the server-side options? 

That was brought up by Michael in the thread, but none of us followed up on it
it seems.  The current name was chosen to be consistent with the already
existing ssl* client-side settings, but I don't really have strong opinions on
if that makes sense or not.  Perhaps use ssl_m{in|max}_protocolversion to make
it more readable?

The attached renames the userfacing setting, but keeps the environment variable
without underscores as most settings have env vars without underscores.

cheers ./daniel

On Fri, Apr 24, 2020 at 02:03:04PM +0200, Daniel Gustafsson wrote:
> That was brought up by Michael in the thread, but none of us followed up on it
> it seems.  The current name was chosen to be consistent with the already
> existing ssl* client-side settings, but I don't really have strong opinions on
> if that makes sense or not.  Perhaps use ssl_m{in|max}_protocolversion to make
> it more readable?

There was no hard push in favor of this comment so I did not insist,
but I am not wedded to the existing connection parameter names.

-       {"sslminprotocolversion", "PGSSLMINPROTOCOLVERSION", NULL, NULL,
+       {"ssl_min_protocolversion", "PGSSLMINPROTOCOLVERSION", NULL, NULL,
Shouldn't that actually be "ssl_min_protocol_version" with one extra
underscore?

> The attached renames the userfacing setting, but keeps the environment variable
> without underscores as most settings have env vars without underscores.

There are two in this case: PG_COLOR and PG_COLORS.  For readability
it could make sense to use something like PG_SSL_MIN_PROTOCOL_VERSION
or PGSSL_MIN_PROTOCOL_VERSION, but like Daniel I'd rather keep the env
variables without underscores.
--
Michael

On 2020-04-24 14:03, Daniel Gustafsson wrote:
>> On 24 Apr 2020, at 12:56, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>>
>> Can we reconsider whether we really want to name the new settings like "sslminprotocolversion", or whether we could
addsome underscores, both for readability and for consistency with the server-side options?
 
> 
> That was brought up by Michael in the thread, but none of us followed up on it
> it seems.  The current name was chosen to be consistent with the already
> existing ssl* client-side settings, but I don't really have strong opinions on
> if that makes sense or not.  Perhaps use ssl_m{in|max}_protocolversion to make
> it more readable?

The names on the backend side are ssl_{min|max|_protocol_version.

> The attached renames the userfacing setting, but keeps the environment variable
> without underscores as most settings have env vars without underscores.

Keeping the environment variable as is seems fine (also consistent with 
"channel_binding").

I would, however, prefer to also rename the internal symbols.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 26 Apr 2020, at 14:01, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-04-24 14:03, Daniel Gustafsson wrote:
>>> On 24 Apr 2020, at 12:56, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>>>
>>> Can we reconsider whether we really want to name the new settings like "sslminprotocolversion", or whether we could
addsome underscores, both for readability and for consistency with the server-side options? 
>> That was brought up by Michael in the thread, but none of us followed up on it
>> it seems.  The current name was chosen to be consistent with the already
>> existing ssl* client-side settings, but I don't really have strong opinions on
>> if that makes sense or not.  Perhaps use ssl_m{in|max}_protocolversion to make
>> it more readable?
>
> The names on the backend side are ssl_{min|max|_protocol_version.

That was the preferred name by Michael too elsewhere in the thread, so went
ahead and made it so.

>> The attached renames the userfacing setting, but keeps the environment variable
>> without underscores as most settings have env vars without underscores.
>
> Keeping the environment variable as is seems fine (also consistent with "channel_binding").
>
> I would, however, prefer to also rename the internal symbols.

Done in the attached v2.

cheers ./daniel

On Sun, Apr 26, 2020 at 11:20:01PM +0200, Daniel Gustafsson wrote:
> That was the preferred name by Michael too elsewhere in the thread, so went
> ahead and made it so.

Thanks Daniel.

>> I would, however, prefer to also rename the internal symbols.
>
> Done in the attached v2.

What you have here looks fine to me.  Peter, what do you think?
--
Michael

On 2020-04-27 07:45, Michael Paquier wrote:
> On Sun, Apr 26, 2020 at 11:20:01PM +0200, Daniel Gustafsson wrote:
>> That was the preferred name by Michael too elsewhere in the thread, so went
>> ahead and made it so.
> 
> Thanks Daniel.
> 
>>> I would, however, prefer to also rename the internal symbols.
>>
>> Done in the attached v2.
> 
> What you have here looks fine to me.  Peter, what do you think?

This looks good to me, except that

xreflabel="ssl-min-protocol-version"

etc. needs to be changed to use underscores.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Apr 29, 2020 at 10:33:26PM +0200, Peter Eisentraut wrote:
> This looks good to me, except that
>
> xreflabel="ssl-min-protocol-version"
>
> etc. needs to be changed to use underscores.

Indeed, thanks.  I have fixed this part and applied the patch.
--
Michael