Thread

Commits

Fix compilation with older OpenSSL versions
- a430c7b4afe8 12.0 landed
- 4e6f101e921c 13.0 landed

PostgreSQL12 and older versions of OpenSSL

Victor Wagner <vitus@wagner.pp.ru> — 2019-09-24T07:18:59Z

Dear hackers,

PostgreSQL 12 documentation states, that minimum required version of
OpenSSL is 0.9.8. However, I was unable to сompile current
PGPRO_12_STABLE with OpenSSL 0.9.8j (from SLES 11sp4).


-fno-strict-aliasing -fwrapv -g -O2 -I../../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2   -c -o be-secure-openssl.o be-secure-openssl.c
be-secure-openssl.c: In function ‘SSL_CTX_set_min_proto_version’:
be-secure-openssl.c:1340: error: ‘SSL_OP_NO_TLSv1_1’ undeclared (first use in this function)
be-secure-openssl.c:1340: error: (Each undeclared identifier is reported only once
be-secure-openssl.c:1340: error: for each function it appears in.)
be-secure-openssl.c:1344: error: ‘SSL_OP_NO_TLSv1_2’ undeclared (first use in this function)
be-secure-openssl.c: In function ‘SSL_CTX_set_max_proto_version’:
be-secure-openssl.c:1361: error: ‘SSL_OP_NO_TLSv1_1’ undeclared (first use in this function)
be-secure-openssl.c:1365: error: ‘SSL_OP_NO_TLSv1_2’ undeclared (first use in this function)
make: *** [be-secure-openssl.o] Error 1


Problem is that some code in src/backend/libpq/be-secure-openssl.c
assumes that if preprocessor symbols TLS1_1_VERSION and TLS1_2_VERSION
are defined in the openssl headers, corresponding versions of TLS are
supported by the library.

It is not so. Here is exempt from tls1.h header file from the openssl
0.9.8j

#define TLS1_VERSION                    0x0301
#define TLS1_1_VERSION                  0x0302
#define TLS1_2_VERSION                  0x0303
/* TLS 1.1 and 1.2 are not supported by this version of OpenSSL, so
 * TLS_MAX_VERSION indicates TLS 1.0 regardless of the above
 * definitions. (s23_clnt.c and s23_srvr.c have an OPENSSL_assert()
 * check that would catch the error if TLS_MAX_VERSION was too low.)
 */
#define TLS_MAX_VERSION                 TLS1_VERSION

Replacing all 

#ifdef TLS1_1_VERSION

with

#if defined(TLS1_1_VERSION) && TLS1_1_VERSION <= TLS_MAX_VERSION

and analogue for TLS1_2_VERSION fixes the problem.

Really, problem is that symbol SSL_OP_NO_TLSv1_1 (and 1_2 accordingly)
might be undefined even if TLS1_1_VERSION defined. 

Replacing 

#ifdef TLS1_1_VERSION

with 

#ifdef SSL_OP_NO_TLSv1_1 

seems to be correct solution for two of three #ifdef TLS1_1_VERSION
statements in be-secure-openssl.c, because this symbol is used inside
#ifdef block.

But there is third (first from start of file) one.
...
                case PG_TLS1_1_VERSION:
#ifdef TLS1_1_VERSION
                        return TLS1_1_VERSION;
#else
                        break;
#endif
...
(line 1290). In this case check for TLS1_1_VERSION <= TLS_MAX_VERSION
seems to be more self-explanatory, than check for somewhat unrelated 
symbol SSL_OP_NO_TLSv1_1
 

--

Re: PostgreSQL12 and older versions of OpenSSL

Michael Paquier <michael@paquier.xyz> — 2019-09-24T09:49:17Z

On Tue, Sep 24, 2019 at 10:18:59AM +0300, Victor Wagner wrote:
> PostgreSQL 12 documentation states, that minimum required version of
> OpenSSL is 0.9.8. However, I was unable to сompile current
> PGPRO_12_STABLE with OpenSSL 0.9.8j (from SLES 11sp4).

I can reproduce that with REL_12_STABLE and the top of
OpenSSL_0_9_8-stable fromx OpenSSL's git.

> It is not so. Here is exempt from tls1.h header file from the openssl
> 0.9.8j
> 
> #define TLS1_VERSION                    0x0301
> #define TLS1_1_VERSION                  0x0302
> #define TLS1_2_VERSION                  0x0303
> /* TLS 1.1 and 1.2 are not supported by this version of OpenSSL, so
>  * TLS_MAX_VERSION indicates TLS 1.0 regardless of the above
>  * definitions. (s23_clnt.c and s23_srvr.c have an OPENSSL_assert()
>  * check that would catch the error if TLS_MAX_VERSION was too low.)
>  */
> #define TLS_MAX_VERSION                 TLS1_VERSION

Indeed, we rely currently on a false assumption that the version is
supported if the object is defined.  That's clearly wrong.

> Replacing all 
> 
> #ifdef TLS1_1_VERSION
> 
> with
> 
> #if defined(TLS1_1_VERSION) && TLS1_1_VERSION <= TLS_MAX_VERSION
> 
> and analogue for TLS1_2_VERSION fixes the problem.

That sounds like a plan.  

> Really, problem is that symbol SSL_OP_NO_TLSv1_1 (and 1_2 accordingly)
> might be undefined even if TLS1_1_VERSION defined. 
> 
> Replacing 
> 
> #ifdef TLS1_1_VERSION
> 
> with 
> 
> #ifdef SSL_OP_NO_TLSv1_1

Hmm.  Wouldn't it be better to check if the maximum version of TLS is
supported and if SSL_OP_NO_TLSv1_1 is defined (same for 1.2)?

> But there is third (first from start of file) one.
> ...
>                 case PG_TLS1_1_VERSION:
> #ifdef TLS1_1_VERSION
>                         return TLS1_1_VERSION;
> #else
>                         break;
> #endif
> ...
> (line 1290). In this case check for TLS1_1_VERSION <= TLS_MAX_VERSION
> seems to be more self-explanatory, than check for somewhat unrelated 
> symbol SSL_OP_NO_TLSv1_1

That sounds right.  Victor, would you like to write a patch?
--
Michael

Re: PostgreSQL12 and older versions of OpenSSL

Victor Wagner <vitus@wagner.pp.ru> — 2019-09-24T10:07:31Z

On Tue, 24 Sep 2019 18:49:17 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> On Tue, Sep 24, 2019 at 10:18:59AM +0300, Victor Wagner wrote:
> > PostgreSQL 12 documentation states, that minimum required version of
> > OpenSSL is 0.9.8. However, I was unable to сompile current
> > PGPRO_12_STABLE with OpenSSL 0.9.8j (from SLES 11sp4).
> 
> I can reproduce that with REL_12_STABLE and the top of
> OpenSSL_0_9_8-stable fromx OpenSSL's git.
> 
> > Replacing all 
> > 
> > #ifdef TLS1_1_VERSION
> > 
> > with
> > 
> > #if defined(TLS1_1_VERSION) && TLS1_1_VERSION <= TLS_MAX_VERSION
> > 
> > and analogue for TLS1_2_VERSION fixes the problem.
> 
> That sounds like a plan.  
[skip] 
> > ...
> > (line 1290). In this case check for TLS1_1_VERSION <=
> > TLS_MAX_VERSION seems to be more self-explanatory, than check for
> > somewhat unrelated symbol SSL_OP_NO_TLSv1_1
> 
> That sounds right.  Victor, would you like to write a patch?

I'm attaching patch which uses solution mentioned above.
It seems that chedk for SSL_OP_NO_TLSvX_Y is redundant if 
we are checking for TLS_MAX_VERSION.
--

Re: PostgreSQL12 and older versions of OpenSSL

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-09-24T15:13:07Z

On 2019-Sep-24, Victor Wagner wrote:

> Dear hackers,
> 
> PostgreSQL 12 documentation states, that minimum required version of
> OpenSSL is 0.9.8. However, I was unable to сompile current
> PGPRO_12_STABLE with OpenSSL 0.9.8j (from SLES 11sp4).

(Nice branch name.)  I wonder if we should really continue to support
OpenSSL 0.9.8.  That branch was abandoned by the OpenSSL dev group in
2015 ... and I wouldn't want to assume that there are no security
problems fixed in the meantime.  Why shouldn't we drop support for that
going forward, raising our minimum required OpenSSL version to be at
least something in the 1.0 branch?

(I'm not entirely sure about minor version numbers in OpenSSL -- it
seems 1.0.2 is still being maintained, but 1.0.0 itself was also
abandoned in 2016, as was 1.0.1.  As far as I understand they use the
alphabetical sequence *after* the three-part version number in the way
we use minor number; so 1.0.1u (2016) is the last there, and 1.0.2t is a
recent one in the maintained branch.

Along the same lines, 0.9.8j was released in Jan 2009.  The last in
0.9.8 was 0.9.8zi in December 2015.)

Anyway I suppose it's not impossible that third parties are still
maintaining their 1.0.0 branch, but I doubt anyone cares for 0.9.8 with
Postgres 12 ... particularly since SUSE themselves suggest not to use
the packaged OpenSSL for their stuff but rather stick to NSS.  That
said, in 2014 (!!) SUSE released OpenSSL 1.0.1 separately, for use with
SLES 11:
https://www.suse.com/c/introducing-the-suse-linux-enterprise-11-security-module/
Who would use the already obsolete SLES 11 (general support ended in
March 2019, though extended support ends in 2022) with Postgres 12?
That seems insane.

All that being said, I don't oppose to this patch, since it seems a
quick way to get out of the immediate trouble.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: PostgreSQL12 and older versions of OpenSSL

Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-24T15:25:30Z

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> ... I wonder if we should really continue to support
> OpenSSL 0.9.8.

Fair question, but post-rc1 is no time to be moving that goalpost
for the v12 branch.

> Anyway I suppose it's not impossible that third parties are still
> maintaining their 1.0.0 branch,

Another data point on that is that Red Hat is still supporting
1.0.1e in RHEL6.  I don't think we should assume that just because
OpenSSL upstream has dropped support for a branch, it no longer
exists in the wild.

Having said that, if it makes our lives noticeably easier to
drop support for 0.9.8 in HEAD, I won't stand in the way.

(We should survey the buildfarm and see what the older critters
are running, perhaps.)

			regards, tom lane

Re: PostgreSQL12 and older versions of OpenSSL

Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-24T16:43:17Z

Victor Wagner <vitus@wagner.pp.ru> writes:
> I'm attaching patch which uses solution mentioned above.
> It seems that chedk for SSL_OP_NO_TLSvX_Y is redundant if 
> we are checking for TLS_MAX_VERSION.

One thing I'm wondering is if it's safe to assume that TLS_MAX_VERSION
will be defined whenever these other symbols are.  Looking in an
0.9.8x install tree, that doesn't seem to define any of them; while
in 1.0.1e I see

./tls1.h:#define TLS1_1_VERSION                 0x0302
./tls1.h:#define TLS1_2_VERSION                 0x0303
./tls1.h:#define TLS_MAX_VERSION                        TLS1_2_VERSION

So the patch seems okay for these two versions, but I have no data about
intermediate OpenSSL versions.

BTW, the spacing in this patch seems rather random.

			regards, tom lane

Re: PostgreSQL12 and older versions of OpenSSL

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-09-24T21:52:48Z

On 2019-09-24 09:18, Victor Wagner wrote:
> Problem is that some code in src/backend/libpq/be-secure-openssl.c
> assumes that if preprocessor symbols TLS1_1_VERSION and TLS1_2_VERSION
> are defined in the openssl headers, corresponding versions of TLS are
> supported by the library.
> 
> It is not so. Here is exempt from tls1.h header file from the openssl
> 0.9.8j
> 
> #define TLS1_VERSION                    0x0301
> #define TLS1_1_VERSION                  0x0302
> #define TLS1_2_VERSION                  0x0303
> /* TLS 1.1 and 1.2 are not supported by this version of OpenSSL, so
>  * TLS_MAX_VERSION indicates TLS 1.0 regardless of the above
>  * definitions. (s23_clnt.c and s23_srvr.c have an OPENSSL_assert()
>  * check that would catch the error if TLS_MAX_VERSION was too low.)
>  */
> #define TLS_MAX_VERSION                 TLS1_VERSION

That's not actually what this file looks like in the upstream release.
It looks like the packagers must have patched in the protocol codes for
TLS 1.1 and 1.2 themselves.  Then they should also add the corresponding
SSL_OP_NO_* flags.  AFAICT, these pairs of symbols are always added
together in upstream commits.

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

Re: PostgreSQL12 and older versions of OpenSSL

Michael Paquier <michael@paquier.xyz> — 2019-09-25T06:55:14Z

On Tue, Sep 24, 2019 at 11:25:30AM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> ... I wonder if we should really continue to support
>> OpenSSL 0.9.8.
> 
> Fair question, but post-rc1 is no time to be moving that goalpost
> for the v12 branch.

Yeah.  I worked in the past with SUSE-based appliances, and I recall
that those folks have been maintaining their own patched version of
OpenSSL 0.9.8 with a bunch of custom patches, some of them coming from
newer versions of upstream to take care of security issues with 0.9.8.
So even if they call their version 0.9.8j, I think that they include
much more security-related fixes than their version string suggests.
I don't know at which extent though.

>> Anyway I suppose it's not impossible that third parties are still
>> maintaining their 1.0.0 branch,
> 
> Another data point on that is that Red Hat is still supporting
> 1.0.1e in RHEL6.  I don't think we should assume that just because
> OpenSSL upstream has dropped support for a branch, it no longer
> exists in the wild.
> 
> Having said that, if it makes our lives noticeably easier to
> drop support for 0.9.8 in HEAD, I won't stand in the way.

Agreed.  There is an argument for dropping support for OpenSSL 0.9.8
in 13~, but I don't agree of doing that in 12.  Let's just fix the
issue.
--
Michael

Re: PostgreSQL12 and older versions of OpenSSL

Michael Paquier <michael@paquier.xyz> — 2019-09-26T04:53:14Z

On Tue, Sep 24, 2019 at 11:52:48PM +0200, Peter Eisentraut wrote:
> That's not actually what this file looks like in the upstream release.
> It looks like the packagers must have patched in the protocol codes for
> TLS 1.1 and 1.2 themselves.  Then they should also add the corresponding
> SSL_OP_NO_* flags.  AFAICT, these pairs of symbols are always added
> together in upstream commits.

Yes, they did so.  I see those three fields as of 6287fa5 from
upstream which is the release tag for 0.9.8j:
#define TLS1_VERSION            0x0301
#define TLS1_VERSION_MAJOR      0x03
#define TLS1_VERSION_MINOR      0x01

However if you look at the top branch OpenSSL_0_9_8-stable (7474341),
then you would notice that ssl/tls1.h does something completely
different and defines TLS_MAX_VERSION.  So everything is in line to
say that the version of OpenSSL in SUSE labelled 0.9.8j is using
something compatible with the latest version of upstream 0.9.8zi.  I
think that we should just stick for simplicity with the top of their
branch instead of trying to be compatible with 0.9.8j because Postgres
12 has not been released yet, hence if one tries to compile Postgres
12 with OpenSSL 0.9.8j then they would get a compilation failure, and
we could just tell them to switch to the latest version of upstream
for 0.9.8.   That's something they should really do anyway to take
care of various security issues on this branch.  Well, if that happens
they should rather upgrade to at least 1.0.2 anyway :)

So I agree with the proposal to rely on the presence of
TLS_MAX_VERSION, and base our decision-making on that.
--
Michael

Re: PostgreSQL12 and older versions of OpenSSL

Michael Paquier <michael@paquier.xyz> — 2019-09-26T05:25:48Z

On Tue, Sep 24, 2019 at 12:43:17PM -0400, Tom Lane wrote:
> One thing I'm wondering is if it's safe to assume that TLS_MAX_VERSION
> will be defined whenever these other symbols are.  Looking in an
> 0.9.8x install tree, that doesn't seem to define any of them; while
> in 1.0.1e I see

Yeah, I could personally live with the argument of simplicity and just
say that trying to compile v12 with any version older than 0.9.8zc or
any version that does not have those symbols just does not work, and
that one needs to use the top of the released versions.

> ./tls1.h:#define TLS1_1_VERSION                 0x0302
> ./tls1.h:#define TLS1_2_VERSION                 0x0303
> ./tls1.h:#define TLS_MAX_VERSION                        TLS1_2_VERSION
>
> So the patch seems okay for these two versions, but I have no data about
> intermediate OpenSSL versions.

More precisely, all those fields have been added by this upstream
commit, so the fields are present since 0.9.8zc:
commit: c6a876473cbff0fd323c8abcaace98ee2d21863d
author: Bodo Moeller <bodo@openssl.org>
date: Wed, 15 Oct 2014 04:18:29 +0200
Support TLS_FALLBACK_SCSV.

> BTW, the spacing in this patch seems rather random.

Indeed.

Now that I think about it, another method would be to rely on the fact
that a given version of OpenSSL does not support TLS 1.1 and 1.2.  So
we could also just add checks based on OPENSSL_VERSION_NUMBER and be
done with it.  And actually, looking at their tree TLS 1.1 and 2.2 are
not supported in 1.0.0 either.  1.0.1, 1.0.2, 1.1.0 and HEAD do
support them, but not TLS 1.3.

I would still prefer relying on TLS_MAX_VERSION though, as that's more
portable for future decisions, like the introduction of TLS1_3_VERSION
for which we have already some logic in be-secure-openssl.c.  And
updating this stuff would very likely get forgotten once OpenSSL adds
support for TLS 1.3...

There is another issue in the patch:
-#ifdef TLS1_3_VERSION
+#if defined(TLS1_3_VERSION)  &&  TLS1_2_VERSION <= TLS_MAX_VERSION
The second part of the if needs to use TLS1_3_VERSION.

I would also add more brackets around the extra conditions for
readability.
--
Michael

Re: PostgreSQL12 and older versions of OpenSSL

Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-26T06:03:12Z

Michael Paquier <michael@paquier.xyz> writes:
> Now that I think about it, another method would be to rely on the fact
> that a given version of OpenSSL does not support TLS 1.1 and 1.2.  So
> we could also just add checks based on OPENSSL_VERSION_NUMBER and be
> done with it.

No, that way madness lies.  We *know* that there are lots of
vendor-patched versions of OpenSSL out there, so that the nominal
version number isn't really going to tell us what the package can do.

What I'm concerned about at the moment is Peter's comment upthread
that what we seem to be dealing with here is a broken vendor patch,
not any officially-released OpenSSL version at all.  Is it our job
to work around that situation, rather than pushing the vendor to
fix their patch?

			regards, tom lane

Re: PostgreSQL12 and older versions of OpenSSL

Michael Paquier <michael@paquier.xyz> — 2019-09-26T06:56:46Z

On Thu, Sep 26, 2019 at 02:03:12AM -0400, Tom Lane wrote:
> What I'm concerned about at the moment is Peter's comment upthread
> that what we seem to be dealing with here is a broken vendor patch,
> not any officially-released OpenSSL version at all.  Is it our job
> to work around that situation, rather than pushing the vendor to
> fix their patch?

Yes, rather broken.  SUSE got the header visibly right, at least the
version string is not.  The best solution in our favor would be that
they actually fix their stuff :)

And OpenSSL is also to blame by not handling those flags consistently
in a stable branch..
--
Michael

Re: PostgreSQL12 and older versions of OpenSSL

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-09-26T14:43:33Z

On 2019-09-26 06:53, Michael Paquier wrote:
> So I agree with the proposal to rely on the presence of
> TLS_MAX_VERSION, and base our decision-making on that.

But then there is this:

commit 04cd70c6899c6b36517b2b07d7a12b2cceba1bef
Author: Kurt Roeckx <kurt@roeckx.be>
Date:   Tue Sep 18 22:17:14 2018

    Deprecate TLS_MAX_VERSION, DTLS_MAX_VERSION and DTLS_MIN_VERSION

    Fixes: #7183

    Reviewed-by: Matt Caswell <matt@openssl.org>
    GH: #7260

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

Re: PostgreSQL12 and older versions of OpenSSL

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-09-26T16:24:22Z

Here is my proposed patch, currently completely untested.

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

Re: PostgreSQL12 and older versions of OpenSSL

Michael Paquier <michael@paquier.xyz> — 2019-09-27T00:37:26Z

On Thu, Sep 26, 2019 at 04:43:33PM +0200, Peter Eisentraut wrote:
> On 2019-09-26 06:53, Michael Paquier wrote:
> > So I agree with the proposal to rely on the presence of
> > TLS_MAX_VERSION, and base our decision-making on that.
> 
> But then there is this:
> 
> commit 04cd70c6899c6b36517b2b07d7a12b2cceba1bef
> Author: Kurt Roeckx <kurt@roeckx.be>
> Date:   Tue Sep 18 22:17:14 2018
> 
>     Deprecate TLS_MAX_VERSION, DTLS_MAX_VERSION and DTLS_MIN_VERSION
> 
>     Fixes: #7183
> 
>     Reviewed-by: Matt Caswell <matt@openssl.org>
>     GH: #7260

Ouch.  I missed that part, thanks!  That's included as part of current
HEAD, so even 1.1.1 still has the flags.
--
Michael

Re: PostgreSQL12 and older versions of OpenSSL

Michael Paquier <michael@paquier.xyz> — 2019-09-27T01:51:01Z

On Thu, Sep 26, 2019 at 06:24:22PM +0200, Peter Eisentraut wrote:
> Here is my proposed patch, currently completely untested.

I have tested compilation of REL_12_STABLE with the top of OpenSSL
0.9.8, 1.0.0, 1.0.1, 1.0.2, 1.1.0 and 1.1.1.  Our SSL tests also pass
in all the setups I have tested.

Your patch does not issue a ereport(LOG/FATAL) in the event of a
failure with SSL_CTX_set_max_proto_version(), which is something done
when ssl_protocol_version_to_openssl()'s result is -1.  Wouldn't it be
better to report that properly to the user?

Some more nits about the patch I have.  Would it be worth copying the
comment from min_proto_version() to SSL_CTX_set_max_proto_version()?
I would add a newline before the comment block as well.

Note: We have a failure with ssl/t/002_scram.pl because of the
introduction of the recent channel_binding parameter if you try to run 
the SSL tests on HEAD with at least 0.9.8 as we forgot to add a
conditional check for HAVE_X509_GET_SIGNATURE_NID as c3d41cc did.
I'll send a patch for that separately.  That's why I have checked the
patch only with REL_12_STABLE.
--
Michael

Re: PostgreSQL12 and older versions of OpenSSL

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-09-27T13:50:57Z

On 2019-09-27 03:51, Michael Paquier wrote:
> I have tested compilation of REL_12_STABLE with the top of OpenSSL
> 0.9.8, 1.0.0, 1.0.1, 1.0.2, 1.1.0 and 1.1.1.  Our SSL tests also pass
> in all the setups I have tested.

great

> Your patch does not issue a ereport(LOG/FATAL) in the event of a
> failure with SSL_CTX_set_max_proto_version(), which is something done
> when ssl_protocol_version_to_openssl()'s result is -1.  Wouldn't it be
> better to report that properly to the user?

Our SSL_CTX_set_max_proto_version() is a reimplementation of a function
that exists in newer versions of OpenSSL, so it has a specific error
behavior.  Our implementation should probably not diverge from it too much.

> Some more nits about the patch I have.  Would it be worth copying the
> comment from min_proto_version() to SSL_CTX_set_max_proto_version()?
> I would add a newline before the comment block as well.

ok

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

Re: PostgreSQL12 and older versions of OpenSSL

Michael Paquier <michael@paquier.xyz> — 2019-09-27T14:20:58Z

On Fri, Sep 27, 2019 at 03:50:57PM +0200, Peter Eisentraut wrote:
> On 2019-09-27 03:51, Michael Paquier wrote:
>> Your patch does not issue a ereport(LOG/FATAL) in the event of a
>> failure with SSL_CTX_set_max_proto_version(), which is something done
>> when ssl_protocol_version_to_openssl()'s result is -1.  Wouldn't it be
>> better to report that properly to the user?
> 
> Our SSL_CTX_set_max_proto_version() is a reimplementation of a function
> that exists in newer versions of OpenSSL, so it has a specific error
> behavior.  Our implementation should probably not diverge from it too much.

I agree with this point.  Now my argument is about logging LOG or
FATAL within be_tls_init() after the two OpenSSL functions (or our
wrappers) SSL_CTX_set_min/max_proto_version are called.
--
Michael

Re: PostgreSQL12 and older versions of OpenSSL

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-09-28T20:52:18Z

On 2019-09-27 16:20, Michael Paquier wrote:
> On Fri, Sep 27, 2019 at 03:50:57PM +0200, Peter Eisentraut wrote:
>> On 2019-09-27 03:51, Michael Paquier wrote:
>>> Your patch does not issue a ereport(LOG/FATAL) in the event of a
>>> failure with SSL_CTX_set_max_proto_version(), which is something done
>>> when ssl_protocol_version_to_openssl()'s result is -1.  Wouldn't it be
>>> better to report that properly to the user?
>>
>> Our SSL_CTX_set_max_proto_version() is a reimplementation of a function
>> that exists in newer versions of OpenSSL, so it has a specific error
>> behavior.  Our implementation should probably not diverge from it too much.
> 
> I agree with this point.  Now my argument is about logging LOG or
> FATAL within be_tls_init() after the two OpenSSL functions (or our
> wrappers) SSL_CTX_set_min/max_proto_version are called.

committed with that

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

Re: PostgreSQL12 and older versions of OpenSSL

Michael Paquier <michael@paquier.xyz> — 2019-09-29T01:47:55Z

On Sat, Sep 28, 2019 at 10:52:18PM +0200, Peter Eisentraut wrote:
> committed with that

Thanks, LGTM.
--
Michael