Обсуждение: BUG #15789: libpq compilation with OpenSSL 1.1.1b fails on Windows with Visual Studio 2017

The following bug has been logged on the website:

Bug reference:      15789
Logged by:          Sergey Pashkov
Email address:      serpashk@gmail.com
PostgreSQL version: 11.2
Operating system:   Windows 10
Description:

libssh2 and libpq are used in the same product.
It is necessary to use OpenSSL 1.1.1b as it includes EdDSA support required
for libssh2.

Here are parts of our build scripts.

1. Building OpenSSL:
perl Configure VC-WIN64A no-shared no-asm enable-ssl3 enable-ssl3-method
nmake

2. Building libpq:
XCOPY /s /i "%OPENSSL_SRCS%" "%OPENSSL_PATH%"
MKDIR "%OPENSSL_PATH%\lib"
COPY "%THIRD_LIBS%\libssl_64.lib" "%OPENSSL_PATH%\lib\ssleay32.lib"
COPY "%THIRD_LIBS%\libcrypto_64.lib" "%OPENSSL_PATH%\lib\libeay32.lib"

perl mkvcbuild.pl
COPY config_default.pl config.pl
sed -i "s/openssl   => undef/openssl   => \"%OPENSSL_PATH_ESC%\"/g"
config.pl
perl mkvcbuild.pl
msbuild pgsql.sln /t:interfaces\libpq /p:Configuration="Release" /m

The following errors are encountered:

       "C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj" (default
target) (6) ->
       (ClCompile target) ->

c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1467):
error C2037: left
        of 'ptr' specifies undefined struct/union 'bio_st'
[C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj]

c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1467):
error C2198: 'pqs
       ecure_raw_read': too few arguments for call
[C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj]

c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1497):
error C2037: left
        of 'ptr' specifies undefined struct/union 'bio_st'
[C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj]

c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1497):
error C2198: 'pqs
       ecure_raw_write': too few arguments for call
[C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj]

c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1556):
error C2027: use
       of undefined type 'bio_method_st'
[C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj]

c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1559):
error C2027: use
       of undefined type 'bio_method_st'
[C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj]

c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1560):
error C2037: left
        of 'bread' specifies undefined struct/union 'bio_method_st'
[C:\Users\User\AppData\Local\Temp\postgres_64\libpq
       .vcxproj]

c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1561):
error C2037: left
        of 'bwrite' specifies undefined struct/union 'bio_method_st'
[C:\Users\User\AppData\Local\Temp\postgres_64\libp
       q.vcxproj]

c:\users\user\appdata\local\temp\postgres_64\src\interfaces\libpq\fe-secure-openssl.c(1587):
error C2037: left
        of 'ptr' specifies undefined struct/union 'bio_st'
[C:\Users\User\AppData\Local\Temp\postgres_64\libpq.vcxproj]

Headers of the recent OpenSSL versions don't include bio_st definition, as
it was in 1.0.2.

If I manually define the following macros in pg_config.h:

HAVE_BIO_GET_DATA
HAVE_BIO_METH_NEW 

- compilation passes but SSL connection cannot be established anyway.

This question was raised in the following thread:

https://www.postgresql.org/message-id/flat/CAAw-Mseg9JYpp%3DA%3D51HR3rKiQtbvT0MWw%2BaYFwNeJEbdNr%3DCDA%40mail.gmail.com

No solution was proposed.

Thank you!

On Tue, May 28, 2019 at 07:31:55AM +0200, Juan José Santamaría Flecha wrote:
> Can you check if you get a sane compilation with the attached patch?
>
> It is just a compilation issue, once the software is built with the 1.1.x
> libraries you should not have any further problems to use SSL.

Could you add your patch to the upcoming commit fest please?  Here it
is:
https://commitfest.postgresql.org/23/

The scripts in src/tools/msvc/ make efforts for being able to compile
with OpenSSL 1.0.2 which is the latest LTS version of upstream, but we
lack facility to make them more dynamic depending on the version of
OpenSSL so as the compile flags of pg_config.h can be enforced
correctly.  So what you are doing in GetOpenSSLVersion() is something
that we are going to need badly, and OpenSSL has broken many
interfaces between 1.0.2 and 1.1.0.

+   # Startint at version 1.1.0 OpenSSL have changed their library names from:
+   #     libeay to libcrypto
+   #     ssleay to libssl

s/startint/starting/
Are these from the installers we recommend in the docs?  I mean these
ones:
https://slproweb.com/products/Win32OpenSSL.html

+   if (-e "$self->{options}->{openssl}/lib/VC/libssl32MD.lib")
Why not using a version-specific logic here?

+   my ($major, $minor) = $self->GetOpenSSLVersion();
+   if ($major == 1 && $minor == 1)
+   {
+       print $o "#define HAVE_BIO_GET_DATA 1\n";
+       print $o "#define HAVE_BIO_METH_NEW 1\n";
+   }
I think that you are missing HAVE_OPENSSL_INIT_SSL and
HAVE_ASN1_STRING_GET0_DATA here.  Please see commit message of
bde64eb.
--
Michael

On Fri, May 31, 2019 at 04:11:37PM +0200, Juan José Santamaría Flecha wrote:
> Sure, done as  [1]. The attached patch is still for 11, will it be back
> patched?

I don't see a reason why we should not back-patch something like what
you propose where build with OpenSSL 1.1.0 is supported (aka down to
9.5) if that proves to be reliable enough.
--
Michael

On Mon, Jun 03, 2019 at 03:30:49PM +0530, Sandeep Thakkar wrote:
> OpenSSL 1.1.0 and 1.0.2 both are going out of Support in 2019 and PG 9.4 is
> supported till Feb2020.
> Ideally, 1.1.1x support should be backpatched until 9.4

There is no point to patch a stable branch if it has no support for
the OpenSSL version we target.  Now, bb132cdd has added support for
OpenSSL 1.1.0 in 9.4, so there is actually no reason to not patch 9.4
as well and you are right.  Missed that in my initial lookup of the
commit logs as this was applied 7 months after the 9.5~ portions.
--
Michael

On Wed, Jun 05, 2019 at 09:29:21AM +0200, Juan José Santamaría Flecha wrote:
> The patches are intended to support OpenSSL 1.1.x, including 1.1.1x.
>
> The attached patches are meant for all supported versions and HEAD.

I have been looking at this patch, and here are some comments.  First,
I have looked at the differences in what gets installed between 1.0.2
and 1.1.0 and things are rather messy:
- On the Win64 and Win32 installers for 1.0.2, we have both
libssl32.lib and libeay.lib, with the same naming of libraries in
lib/VC/.  This keeps the Postgres scripts more simple, but it causes
conflicts with the installers when trying to manage both Win64 and
Win32, so I cannot blame them for the changes done..
- In 1.1.0~, the situation gets fancy:
-- For Win64 and Win32, we have both now libssl.lib and
libcrypto.lib, which is still consistent.
-- lib/VC/ has been heavily reworked so as we have now for example
libssl64MD.lib & co for Win64 and libssl32MD.lib & co for Win32.

Your patch catches the differences nicely.

sub AddLibrary
{
-       my ($self, $lib, $dbgsuffix) = @_;
+       my ($self, $lib, $slib) = @_;
The only reason why we have the debugging option in the original
interface is for OpenSSL, where Solution.pm checks for the presence of
lib/VC/ssleay32MD.lib before deciding if we should use the debug libs
or not.  I am confused by what you are doing though: what do $slib and
$xlib mean?  For simplicity and compatibility, I am not sure that we
should change the current interface, and just check for the debug libs
that we expect, as done previously.  If the new interface is more
advantageous, the patch needs an effort of documentation, but I would
keep that as a separate improvement.

+sub GetOpenSSLVersion
[...]
+       if ($sslout =~ /(\d+)\.(\d+)\.(\d+)/m)
+       {
+               return ($1, $2)
I really like this interface, but I think that we should return the
three digits instead to help with the decision making so as we don't
need to rework it later on.

+       my ($major, $minor) = $self->GetOpenSSLVersion();
+       if ($major == 1 && $minor == 1)
If one day OpenSSL bumps to 1.2.X, this code would fail. I think that
we should check that the major and minor digits are at least what we
expect them to be.  The same applies to the third digit.
--
Michael

On Thu, Jun 20, 2019 at 4:50 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> - In 1.1.0~, the situation gets fancy:
> -- For Win64 and Win32, we have both now libssl.lib and
> libcrypto.lib, which is still consistent.
> -- lib/VC/ has been heavily reworked so as we have now for example
> libssl64MD.lib & co for Win64 and libssl32MD.lib & co for Win32.
>
> Your patch catches the differences nicely.
>

Great.

> sub AddLibrary
> {
> -       my ($self, $lib, $dbgsuffix) = @_;
> +       my ($self, $lib, $slib) = @_;
> The only reason why we have the debugging option in the original
> interface is for OpenSSL, where Solution.pm checks for the presence of
> lib/VC/ssleay32MD.lib before deciding if we should use the debug libs
> or not.  I am confused by what you are doing though: what do $slib and
> $xlib mean?  For simplicity and compatibility, I am not sure that we
> should change the current interface, and just check for the debug libs
> that we expect, as done previously.  If the new interface is more
> advantageous, the patch needs an effort of documentation, but I would
> keep that as a separate improvement.
>

Since the third parameter is only currently used with OpenSSL
libraries, I thought it would be ok to touch it in this patch. The
reasson behind doing so was to keep the logic for the new fancy
library naming tidy. Documenting better is not a thing I can argue
against, xlib stands for expanded library name as used in
GetAdditionalLinkerDependencies and maybe a comment like this can make
things clearer:

# The parameter $slib is for sufixed run-time library.
# When available it will be added, if not it silently defaults to $lib.
 sub AddLibrary

> +sub GetOpenSSLVersion
> [...]
> If one day OpenSSL bumps to 1.2.X, this code would fail. I think that
> we should check that the major and minor digits are at least what we
> expect them to be.  The same applies to the third digit.

No problem with that. Even more so, it can return 4 values: the 3
digits and the letter. For example, 1.1.1b would be (1, 1,  1, b).

Regards,

Juan José Santamaría Flecha

On Fri, Jun 21, 2019 at 12:23:21PM +0200, Juan José Santamaría Flecha wrote:
> Since the third parameter is only currently used with OpenSSL
> libraries, I thought it would be ok to touch it in this patch. The
> reasson behind doing so was to keep the logic for the new fancy
> library naming tidy. Documenting better is not a thing I can argue
> against, xlib stands for expanded library name as used in
> GetAdditionalLinkerDependencies and maybe a comment like this can make
> things clearer:
>
> # The parameter $slib is for sufixed run-time library.
> # When available it will be added, if not it silently defaults to $lib.
>  sub AddLibrary

That's a sensible argument.  Perhaps your way of doing is much better
at the end.  I still found the patch hard to follow in the changes it
did though, so documenting things for the changes you are doing would
help anybody looking at this code in the future.

> No problem with that. Even more so, it can return 4 values: the 3
> digits and the letter. For example, 1.1.1b would be (1, 1, 1, b).

There are two reasons why we need to know the version of OpenSSL
dynamically:
1) Enforce the correct compilation flags in pg_config.h
2) Select the correct library paths.
OpenSSL keeps API compatibility in their stable branches, so 1. would
not be an issue.  I suspect as well that the installers we use for
Postgres compilation are not going to change their layer in a minor
version update.  So having only 3 digits is enough.  2 likely not.  I
would just keep it at three for now.

Could you send an updated patch?  I would like to look at this stuff
rather sooner than later as OpenSSL 1.0.2 goes EOL in a couple of
months, so there is going to be demand for it.
--
Michael

On Sat, Jun 22, 2019 at 4:44 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> Could you send an updated patch?  I would like to look at this stuff
> rather sooner than later as OpenSSL 1.0.2 goes EOL in a couple of
> months, so there is going to be demand for it.

The updated patches include:

1. Comments for AddLibrary.
2. The function GetOpenSSLVersion returns: major, minor and fix numbers.
3. The version check will default to 1.1.X behaviour for 1.Y.X
versions if Y > 0.

Regards,

Juan José Santamaría Flecha

On Mon, Jun 24, 2019 at 01:44:18PM +0200, Juan José Santamaría Flecha wrote:
> The updated patches include:
>
> 1. Comments for AddLibrary.
> 2. The function GetOpenSSLVersion returns: major, minor and fix numbers.
> 3. The version check will default to 1.1.X behaviour for 1.Y.X
> versions if Y > 0.

Thanks for the new patch set!  I have been looking at that in depths
and I have adjusted the whole logic a bit here and there.  At the end
I found the logic changed in AddLibrary more confusing because the
debugging suffix may change depending on if we use a release-quality
build or not, so I have kept the original interface, and completed it
with a logic allowing the scripts to grab all the libraries it
expects.  This has a small gain when using a non-debug installation as
the library names are the same for Win32 and Win64 for >= 1.1.0.

Also, your patch was not working with other versions of MSVC as the
new routine got stuck into the 2017 class, so I had to move it, and I
found that it was cleaner to just make it return a string made of the
3 first digits and to do direct string comparisons.

Please note that it is not necessary to create versions for the
back-branches yet.  If necessary, I'll do that myself, but first let's
make sure that we agree about the shape of the patch for HEAD.
Attached is an updated version which I would be fine to commit.  I
have tested it with compilation linking to OpenSSL 1.0.2 and 1.1.0 on
Win32 and the build is able to complete.  This applies on HEAD only,
where I have run all my tests.  The patch is properly indented.

What do you think?
--
Michael

I will not have much available time for this list in the next few
weeks, so as quick eye review:

On Tue, Jun 25, 2019 at 10:08 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> Thanks for the new patch set!  I have been looking at that in depths
> and I have adjusted the whole logic a bit here and there.  At the end
> I found the logic changed in AddLibrary more confusing because the
> debugging suffix may change depending on if we use a release-quality
> build or not, so I have kept the original interface, and completed it
> with a logic allowing the scripts to grab all the libraries it
> expects.  This has a small gain when using a non-debug installation as
> the library names are the same for Win32 and Win64 for >= 1.1.0.
>

It actually looks clearer and less intrusive (better) to me too.

> Also, your patch was not working with other versions of MSVC as the
> new routine got stuck into the 2017 class, so I had to move it, and I
> found that it was cleaner to just make it return a string made of the
> 3 first digits and to do direct string comparisons.
>

If you are using a string you will need padding, maybe mimic
OPENSSL_VERSION_NUMBER [1].

> Please note that it is not necessary to create versions for the
> back-branches yet.  If necessary, I'll do that myself, but first let's
> make sure that we agree about the shape of the patch for HEAD.
> Attached is an updated version which I would be fine to commit.  I
> have tested it with compilation linking to OpenSSL 1.0.2 and 1.1.0 on
> Win32 and the build is able to complete.  This applies on HEAD only,
> where I have run all my tests.  The patch is properly indented.
>

There is a typo:

s/versoin/version/

+# made of the three first digits of the OpenSSL versoin, which is enough


Regards,

Juan José Santamaría Flecha

[1] https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_VERSION_NUMBER.html

On 2019-Jun-25, Michael Paquier wrote:

> Attached is an updated version which I would be fine to commit.  I
> have tested it with compilation linking to OpenSSL 1.0.2 and 1.1.0 on
> Win32 and the build is able to complete.  This applies on HEAD only,
> where I have run all my tests.  The patch is properly indented.

In sub GenerateFiles, I would move the stanza for ->{openssl} next to
the one for ->{gss} and keep the simpler lines all together (your patch
leaves ->{nls} as a separate item, which is aesthetically unpleasing).

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

On Tue, Jun 25, 2019 at 09:52:43AM -0400, Alvaro Herrera wrote:
> In sub GenerateFiles, I would move the stanza for ->{openssl} next to
> the one for ->{gss} and keep the simpler lines all together (your patch
> leaves ->{nls} as a separate item, which is aesthetically unpleasing).

Right, thanks.  This makes the flow cleaner.
--
Michael

On Tue, Jun 25, 2019 at 11:43:29AM +0200, Juan José Santamaría Flecha wrote:
> If you are using a string you will need padding, maybe mimic
> OPENSSL_VERSION_NUMBER [1].

For now, I have taken the more simple approach to have three separate
fields for the digit numbers, which is more than enough.  So, after
doing more tweaks and tests on it with the installers for 1.0.2, 1.1.0
and 1.1.1, I have committed the patch to master for now.  The result
is rather cool, as we can now adapt with future versions easily.

I am planning to back-patch the thing down to 9.4 where OpenSSL 1.1.0
is supported, but first let's see if the buildfarm has anything to
say.  I don't expect any issues as we basically don't change the logic
build for 1.0.2, but nobody is never careful enough with this stuff.
--
Michael

On Wed, Jun 26, 2019 at 10:48:03AM +0900, Michael Paquier wrote:
> I am planning to back-patch the thing down to 9.4 where OpenSSL 1.1.0
> is supported, but first let's see if the buildfarm has anything to
> say.  I don't expect any issues as we basically don't change the logic
> build for 1.0.2, but nobody is never careful enough with this stuff.

The buildfarm did not complain, so I have backpatched the thing down
to 9.4.  There were some conflicts, mainly:
- On 9.4, we need to use USE_SSL to enable SSL in the builds.
- In ~9.6, we also need to set HAVE_RAND_OPENSSL for RAND_OpenSSL()
which is a function used by pgcrypto available in OpenSSL 1.1.0 and
newer.  This was removed in v10 as of fe0a0b59, so we can clean up a
bit things here.
--
Michael