Re: OpenSSL 1.1 breaks configure and more

Hi,

as reported by Debian's OpenSSL maintainers, PostgreSQL is failing to
build against a snapshot of the upcoming 1.1.0 version. The report was
for 9.5.3, but I can reproduce it in HEAD as well:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=828510
> OpenSSL 1.1.0 is about to released. During a rebuild of all packages using
> OpenSSL this package fail to build. A log of that build can be found at:
> https://breakpoint.cc/openssl-1.1-rebuild-2016-05-29/Attempted/postgresql-9.5_9.5.3-1_amd64-20160529-1510
>
> On https://wiki.openssl.org/index.php/1.1_API_Changes you can see various of the
> reasons why it might fail. There are also updated man pages at
> https://www.openssl.org/docs/manmaster/ that should contain useful information.
>
> There is a libssl-dev package available in experimental that contains a recent
> snapshot, I suggest you try building against that to see if everything works.

$ ./configure --with-openssl
checking for CRYPTO_new_ex_data in -lcrypto... yes
checking for SSL_library_init in -lssl... no
configure: error: library 'ssl' is required for OpenSSL

I can get one step further by tweaking configure.in and running
autoreconf, but then compilation fails further down:

- AC_CHECK_LIB(ssl, SSL_library_init, [], [AC_MSG_ERROR([library 'ssl' is required for OpenSSL])])
+ AC_CHECK_LIB([ssl], [SSL_library_init])

make -C common all
make[4]: Verzeichnis „/home/cbe/projects/postgresql/pg/master/src/backend/access/common“ wird betreten
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -O2 -I../../../../src/include -D_GNU_SOURCE -c -o printtup.o printtup.c
In file included from ../../../../src/include/libpq/libpq-be.h:25:0,
from ../../../../src/include/libpq/libpq.h:21,
from printtup.c:19:
/usr/include/openssl/ssl.h:1740:26: error: expected identifier or ‘(’ before numeric constant
__owur const COMP_METHOD *SSL_get_current_compression(SSL *s);
^

Christoph

Christoph Berg <myon(at)debian(dot)org> writes:
> as reported by Debian's OpenSSL maintainers, PostgreSQL is failing to
> build against a snapshot of the upcoming 1.1.0 version.

The errors you report make it sound like they broke API compatibility
wholesale. Was that really their intent? If so, where are the changes
documented?

regards, tom lane

On 06/27/2016 05:24 PM, Tom Lane wrote:
> Christoph Berg <myon(at)debian(dot)org> writes:
>> as reported by Debian's OpenSSL maintainers, PostgreSQL is failing to
>> build against a snapshot of the upcoming 1.1.0 version.
>
> The errors you report make it sound like they broke API compatibility
> wholesale. Was that really their intent? If so, where are the changes
> documented?

I do not see that they have documented the removal of the
SSL_library_init symbol anywhere. They changed the function into a macro
in the following commit. I guess we have to check for some other symbol,
like SSL_new.

https://github.com/openssl/openssl/commit/7fa792d14d06cdaca18f225b1d2d8daf8ed24fd7

They have also, which is in the release notes, broken API compatibility
when they made the BIO and BIO_METHOD structs opaque. This will probably
require some ugly ugly #ifs or compatibility macros from us.

They also seem to have broken our OpenSSL thread safety callback when
they added their new threading API and removed the CRYPTO_LOCK define. I
have reported this in their issue tracker
(https://github.com/openssl/openssl/issues/1260).

In addition to this there are a couple of deprecated functions
(DH_generate_parameters() and OPENSSL_config()), but they look pretty
easy to handle.

I think much of the above is missing from the release notes I have
found. I hope they will be more complete at the time of the release. I
am working on a patch to handle these API changes.

- https://www.openssl.org/news/openssl-1.1.0-notes.html
- https://wiki.openssl.org/index.php/1.1_API_Changes

Andreas

Re: Andreas Karlsson 2016-06-27 <8a0a5959-0b83-3dc8-d9e7-66ce8c1c5bc7(at)proxel(dot)se>
> > The errors you report make it sound like they broke API compatibility
> > wholesale. Was that really their intent? If so, where are the changes
> > documented?
>
> I do not see that they have documented the removal of the SSL_library_init
> symbol anywhere. They changed the function into a macro in the following
> commit. I guess we have to check for some other symbol, like SSL_new.

I'm not an autoconf expert, but as said in the original mail, I could
get the SSL_library_init check to work, even if that's a macro now:

> - AC_CHECK_LIB(ssl, SSL_library_init, [], [AC_MSG_ERROR([library 'ssl' is required for OpenSSL])])
> + AC_CHECK_LIB([ssl], [SSL_library_init])

(I haven't investigated if that's due to the quoting, the removal of
the extra arguments, or simply because I reran autoreconf.)

> I think much of the above is missing from the release notes I have found. I
> hope they will be more complete at the time of the release. I am working on
> a patch to handle these API changes.
>
> - https://www.openssl.org/news/openssl-1.1.0-notes.html
> - https://wiki.openssl.org/index.php/1.1_API_Changes

Nod, I was also disappointed when browsing the API changes document,
given it didn't mention any of the problems I was seeing.

Christoph

On 06/27/2016 08:12 PM, Christoph Berg wrote:
> Re: Andreas Karlsson 2016-06-27 <8a0a5959-0b83-3dc8-d9e7-66ce8c1c5bc7(at)proxel(dot)se>
>>> The errors you report make it sound like they broke API compatibility
>>> wholesale. Was that really their intent? If so, where are the changes
>>> documented?
>>
>> I do not see that they have documented the removal of the SSL_library_init
>> symbol anywhere. They changed the function into a macro in the following
>> commit. I guess we have to check for some other symbol, like SSL_new.
>
> I'm not an autoconf expert, but as said in the original mail, I could
> get the SSL_library_init check to work, even if that's a macro now:

Yes, we could do that, but I do not think we should check for the
existence of a backwards compatibility macro. Actually I think we may
want to skip much of the OpenSSL initialization code when compiling
against OpenSSL 1.1 since they have now added automatic initialization
of the library. Instead I think we should check for something we
actually will use like SSL_CTX_new().

Andreas

On Tue, Jun 28, 2016 at 3:21 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Yes, we could do that, but I do not think we should check for the existence
> of a backwards compatibility macro. Actually I think we may want to skip
> much of the OpenSSL initialization code when compiling against OpenSSL 1.1
> since they have now added automatic initialization of the library. Instead I
> think we should check for something we actually will use like SSL_CTX_new().

Agreed. Changing the routine being checked may be a good idea in this
case, and we surely want to check for something that is used in the
frontend and the backend. So why not something more generic like
SSL_read or SSL_write?
--
Michael

Hi,

Here is an initial set of patches related to OpenSSL 1.1. Everything
should still build fine on older OpenSSL versions (and did when I tested
with 1.0.2h).

0001-Fixes-for-compiling-with-OpenSSL-1.1.patch

This patch fixes the code so it builds with OpenSSL 1.1 (except the
CRYPTO_LOCK issue I have reported to the OpenSSL team).

- Makes our configure script check for SSL_new instead
- Uses functions instead of direct access to struct members

0002-Define-CRYPTO_LOCK-for-OpenSSL-1.1-compat.patch

Fix for the removal of the CRYPTO_LOCK define. I am trying to convince
them to add the define back. :)

0003-Remove-OpenSSL-1.1-deprecation-warnings.patch

Silence all warnings. This commit changes more things and is not
necessary for getting PostgreSQL to build against 1.1.

- Silences deprecation other warnings related to that OpenSSL 1.1 now
1) automatically initializes the library and 2) no longer uses the
locking callback.
- Silences deprecation warning when generating DH parameters.

Andreas

On Fri, Jul 1, 2016 at 9:27 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Hi,
>
> Here is an initial set of patches related to OpenSSL 1.1. Everything should
> still build fine on older OpenSSL versions (and did when I tested with
> 1.0.2h).
>
> 0001-Fixes-for-compiling-with-OpenSSL-1.1.patch
>
> This patch fixes the code so it builds with OpenSSL 1.1 (except the
> CRYPTO_LOCK issue I have reported to the OpenSSL team).
>
> - Makes our configure script check for SSL_new instead
> - Uses functions instead of direct access to struct members
>
> 0002-Define-CRYPTO_LOCK-for-OpenSSL-1.1-compat.patch
>
> Fix for the removal of the CRYPTO_LOCK define. I am trying to convince them
> to add the define back. :)
>
> 0003-Remove-OpenSSL-1.1-deprecation-warnings.patch
>
> Silence all warnings. This commit changes more things and is not necessary
> for getting PostgreSQL to build against 1.1.
>
> - Silences deprecation other warnings related to that OpenSSL 1.1 now 1)
> automatically initializes the library and 2) no longer uses the locking
> callback.
> - Silences deprecation warning when generating DH parameters.

Those patches are going to need a careful review by looking at the
areas they are changing, and a backpatch. On Arch there is no test
package available except in AUR. And that's the pre3 release, OpenSSL
folks are on pre5 now with their beta2. It would be annoying to
compile it manually, but if there is no other way... Is Debian up to
date with 1.1.0 beta2 in its snapshot packages?
--
Michael

On Fri, Jul 1, 2016 at 4:08 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Fri, Jul 1, 2016 at 9:27 AM, Andreas Karlsson <andreas(at)proxel(dot)se>
> wrote:
> > Hi,
> >
> > Here is an initial set of patches related to OpenSSL 1.1. Everything
> should
> > still build fine on older OpenSSL versions (and did when I tested with
> > 1.0.2h).
> >
> > 0001-Fixes-for-compiling-with-OpenSSL-1.1.patch
> >
> > This patch fixes the code so it builds with OpenSSL 1.1 (except the
> > CRYPTO_LOCK issue I have reported to the OpenSSL team).
> >
> > - Makes our configure script check for SSL_new instead
> > - Uses functions instead of direct access to struct members
> >
> > 0002-Define-CRYPTO_LOCK-for-OpenSSL-1.1-compat.patch
> >
> > Fix for the removal of the CRYPTO_LOCK define. I am trying to convince
> them
> > to add the define back. :)
> >
> > 0003-Remove-OpenSSL-1.1-deprecation-warnings.patch
> >
> > Silence all warnings. This commit changes more things and is not
> necessary
> > for getting PostgreSQL to build against 1.1.
> >
> > - Silences deprecation other warnings related to that OpenSSL 1.1 now 1)
> > automatically initializes the library and 2) no longer uses the locking
> > callback.
> > - Silences deprecation warning when generating DH parameters.
>
> Those patches are going to need a careful review by looking at the
> areas they are changing, and a backpatch. On Arch there is no test
> package available except in AUR. And that's the pre3 release, OpenSSL
> folks are on pre5 now with their beta2. It would be annoying to
> compile it manually, but if there is no other way... Is Debian up to
> date with 1.1.0 beta2 in its snapshot packages?
>

Debian testing is still on 1.0.2h.
Debian experimental is on 1.1.0pre5.

Not sure here beta2 enters the discussion, it's not mentioned anywhere on
their site?

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

On Fri, Jul 1, 2016 at 5:02 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Debian testing is still on 1.0.2h.
> Debian experimental is on 1.1.0pre5.
>
> Not sure here beta2 enters the discussion, it's not mentioned anywhere on
> their site?

Thanks. From the main page of openssl.org, pre5 is beta2.
--
Michael

On Fri, Jul 1, 2016 at 10:10 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Fri, Jul 1, 2016 at 5:02 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
> > Debian testing is still on 1.0.2h.
> > Debian experimental is on 1.1.0pre5.
> >
> > Not sure here beta2 enters the discussion, it's not mentioned anywhere on
> > their site?
>
> Thanks. From the main page of openssl.org, pre5 is beta2.
>
>
Hah. And it's not mentioned on their download page. I see they continue
down their path of confusing version numbering.

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

Re: Andreas Karlsson 2016-07-01 <688a438c-ccc2-0431-7100-26e418fc3bca(at)proxel(dot)se>
> Hi,
>
> Here is an initial set of patches related to OpenSSL 1.1. Everything should
> still build fine on older OpenSSL versions (and did when I tested with
> 1.0.2h).

Hi Andreas,

thanks for the patches. I applied all there patches on top of HEAD
(10c0558f). The server builds and passes "make check", pgcrypto still
needs work, though:

./configure --with-openssl
make world

gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -O2 -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -c -o openssl.o openssl.c
openssl.c:205:13: error: field ‘ctx’ has incomplete type
EVP_MD_CTX ctx;
^
openssl.c: In function ‘digest_free’:
openssl.c:253:2: warning: implicit declaration of function ‘EVP_MD_CTX_cleanup’ [-Wimplicit-function-declaration]
EVP_MD_CTX_cleanup(&digest->ctx);
^
openssl.c: In function ‘init_openssl_rand’:
openssl.c:990:24: warning: implicit declaration of function ‘RAND_SSLeay’ [-Wimplicit-function-declaration]
RAND_set_rand_method(RAND_SSLeay());
^
openssl.c:990:24: warning: passing argument 1 of ‘RAND_set_rand_method’ makes pointer from integer without a cast [-Wint-conversion]
In file included from openssl.c:40:0:
/usr/include/openssl/rand.h:41:5: note: expected ‘const RAND_METHOD * {aka const struct rand_meth_st *}’ but argument is of type ‘int’
int RAND_set_rand_method(const RAND_METHOD *meth);
^
openssl.c: In function ‘px_get_pseudo_random_bytes’:
openssl.c:1017:2: warning: ‘RAND_pseudo_bytes’ is deprecated [-Wdeprecated-declarations]
res = RAND_pseudo_bytes(dst, count);
^
In file included from openssl.c:40:0:
/usr/include/openssl/rand.h:51:5: note: declared here
DEPRECATEDIN_1_1_0(int RAND_pseudo_bytes(unsigned char *buf, int num))
^
openssl.c: In function ‘digest_block_size’:
openssl.c:222:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
openssl.c: In function ‘digest_result_size’:
openssl.c:214:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
<eingebaut>: die Regel für Ziel „openssl.o“ scheiterte
make[2]: *** [openssl.o] Fehler 1
make[2]: Verzeichnis „/home/cbe/projects/postgresql/pg/master/contrib/pgcrypto“ wird verlassen

ii libssl-dev:amd64 1.1.0~pre5-4 amd64 Secure Sockets Layer toolkit - development files
ii libssl1.0.0:amd64 1.0.2d-1 amd64 Secure Sockets Layer toolkit - shared libraries
ii libssl1.0.2:amd64 1.0.2h-1 amd64 Secure Sockets Layer toolkit - shared libraries
ii libssl1.1:amd64 1.1.0~pre5-4 amd64 Secure Sockets Layer toolkit - shared libraries
ii openssl 1.0.2h-1 amd64 Secure Sockets Layer toolkit - cryptographic utility

Christoph

On 07/01/2016 11:41 AM, Christoph Berg wrote:
> thanks for the patches. I applied all there patches on top of HEAD
> (10c0558f). The server builds and passes "make check", pgcrypto still
> needs work, though:

Thanks, I had forgotten pgcrypto.

When fixing pgcrypto I noticed that the OpenSSL team has deprecated
RAND_pseudo_bytes() and recommend using RAND_bytes() instead (see
302d38e3f73d5fd2ba2fd30bb7798778cb9f18dd).

As far as I can tell the only difference is that RAND_bytes() adds an
error to the error queue if there is not enough entropy for generating
secure data. And since we already always use strong random with the
Fortuna algorithm, why not just drop px_get_pseudo_random_bytes()? It
feels like a potential security problem with to me unclear benefit.

I also found that client CA loading is broken in OpenSSL 1.1-pre5
(reported as https://github.com/openssl/openssl/pull/1279). This might
be good to be aware of when testing my patches.

Attached a new set of patches:

0001-Fixes-for-compiling-with-OpenSSL-1.1-v2.patch

The fixes necessary to build with OpenSSL 1.1. Mostly fixes surrounding
direct access to struct fields.

0002-Remove-OpenSSL-1.1-deprecation-warnings-v2.patch

Fix deprecation warnings. Mostly trusting OpenSSL 1.1 to handle
threading and initialization automatically.

0003-Remove-px_get_pseudo_random_bytes-v2.patch

Remove the px_get_pseudo_random_bytes() from pgcrypto. Also silcences
deprecation warning about RAND_pseudo_bytes().

0004-Define-CRYPTO_LOCK-for-OpenSSL-1.1-compat-v2.patch

Useful if you want to play around with
0001-Fixes-for-compiling-with-OpenSSL-1.1-v2.patch before they release a
new version where CRYPTO_LOCK is added back. See
https://github.com/openssl/openssl/issues/1260

Andreas

Thanks for this effort.

> static BIO_METHOD *
> my_BIO_s_socket(void)
> {
> - if (!my_bio_initialized)
> + if (!my_bio_methods)
> {
> - memcpy(&my_bio_methods, BIO_s_socket(), sizeof(BIO_METHOD));
> - my_bio_methods.bread = my_sock_read;
> - my_bio_methods.bwrite = my_sock_write;
> - my_bio_initialized = true;
> + BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket();
> +#if SSLEAY_VERSION_NUMBER >= 0x10100000L
> + my_bio_methods = BIO_meth_new(BIO_TYPE_SOCKET, "pgsocket");
> + BIO_meth_set_write(my_bio_methods, my_sock_write);
> + BIO_meth_set_read(my_bio_methods, my_sock_read);
> + BIO_meth_set_gets(my_bio_methods, BIO_meth_get_gets(biom));
> + BIO_meth_set_ctrl(my_bio_methods, BIO_meth_get_ctrl(biom));
> + BIO_meth_set_create(my_bio_methods, BIO_meth_get_create(biom));
> + BIO_meth_set_destroy(my_bio_methods, BIO_meth_get_destroy(biom));
> + BIO_meth_set_callback_ctrl(my_bio_methods, BIO_meth_get_callback_ctrl(biom));
> +#else
> + my_bio_methods = malloc(sizeof(BIO_METHOD));
> + memcpy(my_bio_methods, biom, sizeof(BIO_METHOD));
> + my_bio_methods->bread = my_sock_read;
> + my_bio_methods->bwrite = my_sock_write;
> +#endif

Generally, version number tests sprinkled all over the place are not
terribly nice. I think it would be better to get configure to define a
symbol like HAVE_BIO_METH_NEW. Not sure about the other hunks in this
patch; perhaps HAVE_BIO_SET_DATA, and #define both those macros if not.

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

On 07/02/2016 02:28 AM, Alvaro Herrera wrote:
>> static BIO_METHOD *
>> my_BIO_s_socket(void)
>> {
>> - if (!my_bio_initialized)
>> + if (!my_bio_methods)
>> {
>> - memcpy(&my_bio_methods, BIO_s_socket(), sizeof(BIO_METHOD));
>> - my_bio_methods.bread = my_sock_read;
>> - my_bio_methods.bwrite = my_sock_write;
>> - my_bio_initialized = true;
>> + BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket();
>> +#if SSLEAY_VERSION_NUMBER >= 0x10100000L
>> + my_bio_methods = BIO_meth_new(BIO_TYPE_SOCKET, "pgsocket");
>> + BIO_meth_set_write(my_bio_methods, my_sock_write);
>> + BIO_meth_set_read(my_bio_methods, my_sock_read);
>> + BIO_meth_set_gets(my_bio_methods, BIO_meth_get_gets(biom));
>> + BIO_meth_set_ctrl(my_bio_methods, BIO_meth_get_ctrl(biom));
>> + BIO_meth_set_create(my_bio_methods, BIO_meth_get_create(biom));
>> + BIO_meth_set_destroy(my_bio_methods, BIO_meth_get_destroy(biom));
>> + BIO_meth_set_callback_ctrl(my_bio_methods, BIO_meth_get_callback_ctrl(biom));
>> +#else
>> + my_bio_methods = malloc(sizeof(BIO_METHOD));
>> + memcpy(my_bio_methods, biom, sizeof(BIO_METHOD));
>> + my_bio_methods->bread = my_sock_read;
>> + my_bio_methods->bwrite = my_sock_write;
>> +#endif
>
> Generally, version number tests sprinkled all over the place are not
> terribly nice. I think it would be better to get configure to define a
> symbol like HAVE_BIO_METH_NEW. Not sure about the other hunks in this
> patch; perhaps HAVE_BIO_SET_DATA, and #define both those macros if not.

Agreed, that it is not nice. I followed what the previous code did, but
I do not like the inflation of this kind of #ifs with my OpenSSL 1.1
patches. I will try to see if I can figure out some good symbols.

Essentially the API changes which require ifdefs are:

- Opaque struts (we see an example above with the BIO struct)
- Renaming of RAND_SSLeay()
- Deprecation of DH_generate_parameters()
- Automatic initialization
- Automatic handling of threading

I do not like the idea of having a define per struct they have made
opaque in 1.1, but I think one define for all structs could be fine
(something like HAVE_OPENSSL_OPAQUE_STRUCTS). What do you think?

Andreas

On 07/02/2016 02:45 AM, Andreas Karlsson wrote:
> On 07/02/2016 02:28 AM, Alvaro Herrera wrote:
>> Generally, version number tests sprinkled all over the place are not
>> terribly nice. I think it would be better to get configure to define a
>> symbol like HAVE_BIO_METH_NEW. Not sure about the other hunks in this
>> patch; perhaps HAVE_BIO_SET_DATA, and #define both those macros if not.
>
> Agreed, that it is not nice. I followed what the previous code did, but
> I do not like the inflation of this kind of #ifs with my OpenSSL 1.1
> patches. I will try to see if I can figure out some good symbols.
>
> Essentially the API changes which require ifdefs are:
>
> - Opaque struts (we see an example above with the BIO struct)
> - Renaming of RAND_SSLeay()
> - Deprecation of DH_generate_parameters()
> - Automatic initialization
> - Automatic handling of threading
>
> I do not like the idea of having a define per struct they have made
> opaque in 1.1, but I think one define for all structs could be fine
> (something like HAVE_OPENSSL_OPAQUE_STRUCTS). What do you think?

Looking at my code again I noticed it is just the BIO and BIO_METHOD
structs which needed #ifs. The rest could be handled with changing the
code to work in both old and new versions. If it is just two structs it
might be fine to have two symbols, hm ..

Andreas

Re: Andreas Karlsson 2016-07-02 <a5f4b79e-a9ea-200d-e17e-2da3ad187e5b(at)proxel(dot)se>
> On 07/01/2016 11:41 AM, Christoph Berg wrote:
> > thanks for the patches. I applied all there patches on top of HEAD
> > (10c0558f). The server builds and passes "make check", pgcrypto still
> > needs work, though:
>
> Thanks, I had forgotten pgcrypto.

pgcrypto works now as well, thanks!

Re: Alvaro Herrera 2016-07-02 <20160702002846(dot)GA376611(at)alvherre(dot)pgsql>
> Generally, version number tests sprinkled all over the place are not
> terribly nice. I think it would be better to get configure to define a
> symbol like HAVE_BIO_METH_NEW. Not sure about the other hunks in this
> patch; perhaps HAVE_BIO_SET_DATA, and #define both those macros if not.

Otoh these symbols are strictly version-dependant on OpenSSL, it's not
like one of the symbols would appear or disappear for other reasons
(like different TLS implementation, or different operating system).

Once we decide (in 10 years?) that the minimum supported OpenSSL
version is >= 1.1, we can just drop the version checks. If these are
converted to feature tests now, it will be much harder to remember at
which point they can be dropped.

Christoph

On Fri, 1 Jul 2016 02:27:03 +0200
Andreas Karlsson <andreas(at)proxel(dot)se> wrote:

> 0003-Remove-OpenSSL-1.1-deprecation-warnings.patch
>
> Silence all warnings. This commit changes more things and is not
> necessary for getting PostgreSQL to build against 1.1.

This patch breaks feature, which exists in PostgreSQL since 8.2 -
support for SSL ciphers, provided by loadable modules such as Russian
national standard (GOST) algorithms, and support for cryptographic
hardware tokens (which are also supported by loadble modules called
engines in OpenSSL).

Call for OPENSSL_config was added to PostgreSQL for this purpose - it
loads default OpenSSL configuration file, where such things as crypto
hardware modules can be configured.

If we wish to keep this functionality, we need to explicitely call

OPENSSL_init_ssl(INIT_LOAD_CONFIG,NULL) instead of deprecated
OPENSSL_config in 1.1.0

On 07/05/2016 11:13 AM, Victor Wagner wrote:
> On Fri, 1 Jul 2016 02:27:03 +0200
> Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>> 0003-Remove-OpenSSL-1.1-deprecation-warnings.patch
>>
>> Silence all warnings. This commit changes more things and is not
>> necessary for getting PostgreSQL to build against 1.1.
>
> This patch breaks feature, which exists in PostgreSQL since 8.2 -
> support for SSL ciphers, provided by loadable modules such as Russian
> national standard (GOST) algorithms, and support for cryptographic
> hardware tokens (which are also supported by loadble modules called
> engines in OpenSSL).
>
> Call for OPENSSL_config was added to PostgreSQL for this purpose - it
> loads default OpenSSL configuration file, where such things as crypto
> hardware modules can be configured.
>
> If we wish to keep this functionality, we need to explicitely call
>
> OPENSSL_init_ssl(INIT_LOAD_CONFIG,NULL) instead of deprecated
> OPENSSL_config in 1.1.0

Thanks for testing the patches!

I have attached a new set of patches which this is fixed. I have also
skipped the last patch since OpenSSL has fixed the two issues I have
mentioned earlier in this thread.

Andreas

On 07/05/2016 04:46 PM, Andreas Karlsson wrote:
> @@ -280,8 +287,9 @@ px_find_digest(const char *name, PX_MD **res)
> digest = px_alloc(sizeof(*digest));
> digest->algo = md;
>
> - EVP_MD_CTX_init(&digest->ctx);
> - if (EVP_DigestInit_ex(&digest->ctx, digest->algo, NULL) == 0)
> + digest->ctx = EVP_MD_CTX_create();
> + EVP_MD_CTX_init(digest->ctx);
> + if (EVP_DigestInit_ex(digest->ctx, digest->algo, NULL) == 0)
> return -1;
>
> h = px_alloc(sizeof(*h));

Now that we're calling EVP_MD_CTX_create((), which allocates memory, are
we risking memory leaks? It has always been part of the contract that
you have to call px_md_free(), for any context returned by
px_find_digest(), but I wonder just how careful we have been about that.
Before this, you would probably get away with it without leaking, if the
digest implementation didn't allocate any extra memory or other resources.

At least pg_digest and try_unix_std functions call px_find_digest(), and
then do more palloc()s which could elog() if you run out of memory,
leaking th digest struct. Highly unlikely, but I think it would be
fairly straightforward to reorder those calls to eliminate the risk, so
we probably should.

> @@ -854,6 +858,25 @@ load_dh_buffer(const char *buffer, size_t len)
> return dh;
> }
>
> +static DH *
> +generate_dh_params(int prime_len, int generator)
> +{
> +#if SSLEAY_VERSION_NUMBER >= 0x00908000L
> + DH *dh;
> +
> + if ((dh = DH_new()) == NULL)
> + return NULL;
> +
> + if (DH_generate_parameters_ex(dh, prime_len, generator, NULL))
> + return dh;
> +
> + DH_free(dh);
> + return NULL;
> +#else
> + return DH_generate_parameters(prime_len, generator, NULL, NULL);
> +#endif
> +}
> +

I think now would be a good time to drop support for OpenSSL versions
older than 0.9.8. OpenSSL don't even support 0.9.8 anymore, although
there are probably distributions out there that still provide patches
for it. But OpenSSL 0.9.7 and older are really not interesting for
PostgreSQL 10 anymore, I think.

- Heikki

On 8/26/16 5:31 AM, Heikki Linnakangas wrote:
> I think now would be a good time to drop support for OpenSSL versions
> older than 0.9.8. OpenSSL don't even support 0.9.8 anymore, although
> there are probably distributions out there that still provide patches
> for it. But OpenSSL 0.9.7 and older are really not interesting for
> PostgreSQL 10 anymore, I think.

CentOS 5 currently ships 0.9.8e. That's usually the oldest OS we want
to support eagerly.

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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> On 8/26/16 5:31 AM, Heikki Linnakangas wrote:
>> I think now would be a good time to drop support for OpenSSL versions
>> older than 0.9.8. OpenSSL don't even support 0.9.8 anymore, although
>> there are probably distributions out there that still provide patches
>> for it. But OpenSSL 0.9.7 and older are really not interesting for
>> PostgreSQL 10 anymore, I think.

> CentOS 5 currently ships 0.9.8e. That's usually the oldest OS we want
> to support eagerly.

Also, I get this on fully-up-to-date OS X (El Capitan):

$ openssl version
OpenSSL 0.9.8zh 14 Jan 2016

Worth noting though is that without -Wno-deprecated-declarations, you
find that Apple has sprinkled the entire OpenSSL API with deprecation
warnings. That suggests that their plan for the future is to drop it
rather than update it. Should we be thinking ahead to that?

regards, tom lane

PS: I still have 0.9.7 on some of my buildfarm critters. But I could
either update them, or stop using --with-openssl there.

On 08/26/2016 07:44 PM, Tom Lane wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>> On 8/26/16 5:31 AM, Heikki Linnakangas wrote:
>>> I think now would be a good time to drop support for OpenSSL versions
>>> older than 0.9.8. OpenSSL don't even support 0.9.8 anymore, although
>>> there are probably distributions out there that still provide patches
>>> for it. But OpenSSL 0.9.7 and older are really not interesting for
>>> PostgreSQL 10 anymore, I think.
>
>> CentOS 5 currently ships 0.9.8e. That's usually the oldest OS we want
>> to support eagerly.
>
> Also, I get this on fully-up-to-date OS X (El Capitan):
>
> $ openssl version
> OpenSSL 0.9.8zh 14 Jan 2016

Ok, sold, let's remove support for OpenSSL < 0.9.8.

> Worth noting though is that without -Wno-deprecated-declarations, you
> find that Apple has sprinkled the entire OpenSSL API with deprecation
> warnings. That suggests that their plan for the future is to drop it
> rather than update it. Should we be thinking ahead to that?

Yeah, they want people to move to their own SSL library [1]. I doubt
they will actually remove it any time soon, but who knows. It would be a
good project for someone with an OS X system and some spare time, to
write a patch to build with OS X's native SSL library instead of
OpenSSL. The code is structured nicely to enable that now.

[1] I couldn't find any official statement, but lots of blog posts
saying the same thing.

- Heikki

Heikki Linnakangas <hlinnaka(at)iki(dot)fi> writes:
> Yeah, they want people to move to their own SSL library [1].

> [1] I couldn't find any official statement, but lots of blog posts
> saying the same thing.

As I recall, the deprecation warning messages said that in so many words.
That probably counts as an official statement ...

regards, tom lane

On 08/26/2016 07:04 PM, Heikki Linnakangas wrote:
> On 08/26/2016 07:44 PM, Tom Lane wrote:
>> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>>> On 8/26/16 5:31 AM, Heikki Linnakangas wrote:
>>>> I think now would be a good time to drop support for OpenSSL versions
>>>> older than 0.9.8. OpenSSL don't even support 0.9.8 anymore, although
>>>> there are probably distributions out there that still provide patches
>>>> for it. But OpenSSL 0.9.7 and older are really not interesting for
>>>> PostgreSQL 10 anymore, I think.
>>
>>> CentOS 5 currently ships 0.9.8e. That's usually the oldest OS we want
>>> to support eagerly.
>>
>> Also, I get this on fully-up-to-date OS X (El Capitan):
>>
>> $ openssl version
>> OpenSSL 0.9.8zh 14 Jan 2016
>
> Ok, sold, let's remove support for OpenSSL < 0.9.8.

I have attached a patch which removes the < 0.9.8 compatibility code.
Should we also add a version check to configure? We do not have any such
check currently.

Andreas

On Sat, Aug 27, 2016 at 2:04 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 08/26/2016 07:44 PM, Tom Lane wrote:
>> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
>> Also, I get this on fully-up-to-date OS X (El Capitan):
>>
>> $ openssl version
>> OpenSSL 0.9.8zh 14 Jan 2016
>
>
> Ok, sold, let's remove support for OpenSSL < 0.9.8.

Yes I think it's a wiser plan to not brush up newer versions than that.

>> Worth noting though is that without -Wno-deprecated-declarations, you
>> find that Apple has sprinkled the entire OpenSSL API with deprecation
>> warnings. That suggests that their plan for the future is to drop it
>> rather than update it. Should we be thinking ahead to that?
>
>
> Yeah, they want people to move to their own SSL library [1]. I doubt they
> will actually remove it any time soon, but who knows. It would be a good
> project for someone with an OS X system and some spare time, to write a
> patch to build with OS X's native SSL library instead of OpenSSL. The code
> is structured nicely to enable that now.
>
> [1] I couldn't find any official statement, but lots of blog posts saying
> the same thing.

As well on El Capitan:
$ ssh -V
OpenSSH_6.9p1, LibreSSL 2.1.8

So could it be possible that it would be a switch from openssl to
libressl instead?
--
Michael

On 8/26/16 9:26 PM, Andreas Karlsson wrote:
> I have attached a patch which removes the < 0.9.8 compatibility code.
> Should we also add a version check to configure? We do not have any such
> check currently.

I think that is not necessary.

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

On 08/27/2016 05:15 PM, Peter Eisentraut wrote:
> On 8/26/16 9:26 PM, Andreas Karlsson wrote:
>> I have attached a patch which removes the < 0.9.8 compatibility code.
>> Should we also add a version check to configure? We do not have any such
>> check currently.
>
> I think that is not necessary.

I was going to change the configure test to check for a different
function that we use, that's only present in 0.9.8 and later. But the
only such functions were related to ECDH, and the use of those functions
is inside "#ifndef OPENSSL_NO_ECDH", so they're not suitable for the
autoconf test. So I gave up. If you try to build with 0.9.7, you'll get
compilation errors because of those ECDH symbols, and with 0.9.6,
probably on some other symbols.

Pushed with some small doc fixes, thanks Andreas! I'll continue
reviewing the rest of the patches.

- Heikki

On 08/29/2016 08:22 PM, Heikki Linnakangas wrote:
> On 08/27/2016 05:15 PM, Peter Eisentraut wrote:
>> On 8/26/16 9:26 PM, Andreas Karlsson wrote:
>>> I have attached a patch which removes the < 0.9.8 compatibility code.
>>> Should we also add a version check to configure? We do not have any such
>>> check currently.
>>
>> I think that is not necessary.
>
> I was going to change the configure test to check for a different
> function that we use, that's only present in 0.9.8 and later. But the
> only such functions were related to ECDH, and the use of those functions
> is inside "#ifndef OPENSSL_NO_ECDH", so they're not suitable for the
> autoconf test. So I gave up. If you try to build with 0.9.7, you'll get
> compilation errors because of those ECDH symbols, and with 0.9.6,
> probably on some other symbols.
>
> Pushed with some small doc fixes, thanks Andreas! I'll continue
> reviewing the rest of the patches.

Buildfarm animals "locust" and "prairiedog" are not happy with this.
They seem to be using OpenSSL 0.9.7, as they failed with errors related
to those ECDH calls:

be-secure-openssl.c: In function 'initialize_ecdh':
be-secure-openssl.c:978: error: 'EC_KEY' undeclared (first use in this
function)
be-secure-openssl.c:978: error: (Each undeclared identifier is reported
only once
be-secure-openssl.c:978: error: for each function it appears in.)
be-secure-openssl.c:978: error: 'ecdh' undeclared (first use in this
function)
be-secure-openssl.c:979: warning: ISO C90 forbids mixed declarations and
code
be-secure-openssl.c:986: warning: implicit declaration of function
'EC_KEY_new_by_curve_name'
be-secure-openssl.c:991: error: 'SSL_OP_SINGLE_ECDH_USE' undeclared
(first use in this function)
be-secure-openssl.c:992: warning: implicit declaration of function
'SSL_CTX_set_tmp_ecdh'
be-secure-openssl.c:993: warning: implicit declaration of function
'EC_KEY_free'

I only now noticed that Tom said upthread that he still has a buildfarm
critter using 0.9.7 (that's prairiedog). Sorry for the breakage.

It would be easy to put the version check back to still support 0.9.7,
most of the changes in this commit was thanks to removing support for
0.9.6. But that'd complicate the upcoming 1.1.0 support patch slightly,
so let's stick to the plan and drop the support for <= 0.9.7

Tom, Rémi, can you fix locust and prairiedog, please, by updating
OpenSSL or removing --with-openssl?

- Heikki

Heikki Linnakangas <hlinnaka(at)iki(dot)fi> writes:
> Buildfarm animals "locust" and "prairiedog" are not happy with this.
> They seem to be using OpenSSL 0.9.7, as they failed with errors related
> to those ECDH calls:

prairiedog definitely is, and since locust is also an ancient OS X
version, that's not too surprising also. (I imagine gaur/pademelon
will fall over too, next time I turn it on.)

> Tom, Rmi, can you fix locust and prairiedog, please, by updating
> OpenSSL or removing --with-openssl?

Roger, will do (probably just the latter for today).

regards, tom lane

> Le 29 août 2016 à 19:46, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> a écrit :
>
>
> Tom, Rémi, can you fix locust and prairiedog, please, by updating OpenSSL or removing --with-openssl?
>

Hi,

Should be OK for locust on next build.

Rémi

On 08/29/2016 07:22 PM, Heikki Linnakangas wrote:
> Pushed with some small doc fixes, thanks Andreas! I'll continue
> reviewing the rest of the patches.

Thanks!

Andreas

On 08/26/2016 11:31 AM, Heikki Linnakangas wrote:
> On 07/05/2016 04:46 PM, Andreas Karlsson wrote:
>> @@ -280,8 +287,9 @@ px_find_digest(const char *name, PX_MD **res)
>> digest = px_alloc(sizeof(*digest));
>> digest->algo = md;
>>
>> - EVP_MD_CTX_init(&digest->ctx);
>> - if (EVP_DigestInit_ex(&digest->ctx, digest->algo, NULL) == 0)
>> + digest->ctx = EVP_MD_CTX_create();
>> + EVP_MD_CTX_init(digest->ctx);
>> + if (EVP_DigestInit_ex(digest->ctx, digest->algo, NULL) == 0)
>> return -1;
>>
>> h = px_alloc(sizeof(*h));
>
> Now that we're calling EVP_MD_CTX_create((), which allocates memory, are
> we risking memory leaks? It has always been part of the contract that
> you have to call px_md_free(), for any context returned by
> px_find_digest(), but I wonder just how careful we have been about that.
> Before this, you would probably get away with it without leaking, if the
> digest implementation didn't allocate any extra memory or other resources.
>
> At least pg_digest and try_unix_std functions call px_find_digest(), and
> then do more palloc()s which could elog() if you run out of memory,
> leaking th digest struct. Highly unlikely, but I think it would be
> fairly straightforward to reorder those calls to eliminate the risk, so
> we probably should.

Since px_find_digest() calls palloc() later in the function there is a
slim possibility of memory leaks. How do we generally handle that things
not allocated with palloc() may leak when something calls elog()?

I have attached new versions of the patches which are rebased on master,
with slightly improves error handling in px_find_digest(), and handles
the deprecation of ASN1_STRING_data().

Andreas

On 08/30/2016 03:26 AM, Andreas Karlsson wrote:
> On 08/26/2016 11:31 AM, Heikki Linnakangas wrote:
>> On 07/05/2016 04:46 PM, Andreas Karlsson wrote:
>>> @@ -280,8 +287,9 @@ px_find_digest(const char *name, PX_MD **res)
>>> digest = px_alloc(sizeof(*digest));
>>> digest->algo = md;
>>>
>>> - EVP_MD_CTX_init(&digest->ctx);
>>> - if (EVP_DigestInit_ex(&digest->ctx, digest->algo, NULL) == 0)
>>> + digest->ctx = EVP_MD_CTX_create();
>>> + EVP_MD_CTX_init(digest->ctx);
>>> + if (EVP_DigestInit_ex(digest->ctx, digest->algo, NULL) == 0)
>>> return -1;
>>>
>>> h = px_alloc(sizeof(*h));
>>
>> Now that we're calling EVP_MD_CTX_create((), which allocates memory, are
>> we risking memory leaks? It has always been part of the contract that
>> you have to call px_md_free(), for any context returned by
>> px_find_digest(), but I wonder just how careful we have been about that.
>> Before this, you would probably get away with it without leaking, if the
>> digest implementation didn't allocate any extra memory or other resources.
>>
>> At least pg_digest and try_unix_std functions call px_find_digest(), and
>> then do more palloc()s which could elog() if you run out of memory,
>> leaking th digest struct. Highly unlikely, but I think it would be
>> fairly straightforward to reorder those calls to eliminate the risk, so
>> we probably should.
>
> Since px_find_digest() calls palloc() later in the function there is a
> slim possibility of memory leaks.

Yep. That palloc() would be easy to move to before the EVP_MD_CTX_new()
call. And some of the calls to px_find_digest() could likewise be
rearranged. But there are some more complicated callers. pgp_encrypt(),
for example, builds a pipeline of multiple "mbuf" filters, and one of
those filters uses px_find_digest().

> How do we generally handle that things
> not allocated with palloc() may leak when something calls elog()?

There's the ResourceOwner mechanism, see src/backend/utils/resowner/.
That would be the proper way to do this. Call
RegisterResourceReleaseCallback() when the context is allocated, and
have the callback free it. One pitfall to watch out for is that
RegisterResourceReleaseCallback() itself calls palloc(), and can error
out, so you have to do things in such an order that you don't leak in
that case either.

Want to take a stab at that?

Another approach is put each allocated context in a list or array in a
global variable, and to register a callback to be called at
end-of-(sub)transaction, which closes all the contexts. But the resource
owner mechanism is probably easier.

There's also PG_TRY-CATCH, that you could maybe use in the callers of
px_find_digest(), to make sure they call px_free_digest() even on error.
But that also seems difficult to use with the pgp_encrypt() pipeline.

> I have attached new versions of the patches which are rebased on master,
> with slightly improves error handling in px_find_digest(), and handles
> the deprecation of ASN1_STRING_data().

Thanks!

PS. I just remembered that I've wanted to refactor the pgcrypto calls
for symmetric encryption to use the newer EVP API for some time, and
even posted a patch for that
(https://www.postgresql.org/message-id/561274F1.1030000@iki.fi). I
dropped the ball back then, but I think I'll go ahead and do that now,
once we get these other OpenSSL changes in.

- Heikki

On 08/30/2016 08:42 AM, Heikki Linnakangas wrote:
> There's the ResourceOwner mechanism, see src/backend/utils/resowner/.
> That would be the proper way to do this. Call
> RegisterResourceReleaseCallback() when the context is allocated, and
> have the callback free it. One pitfall to watch out for is that
> RegisterResourceReleaseCallback() itself calls palloc(), and can error
> out, so you have to do things in such an order that you don't leak in
> that case either.
>
> Want to take a stab at that?
>
> Another approach is put each allocated context in a list or array in a
> global variable, and to register a callback to be called at
> end-of-(sub)transaction, which closes all the contexts. But the resource
> owner mechanism is probably easier.
>
> There's also PG_TRY-CATCH, that you could maybe use in the callers of
> px_find_digest(), to make sure they call px_free_digest() even on error.
> But that also seems difficult to use with the pgp_encrypt() pipeline.

Sure, I have attached a patch where I try to use it.

> PS. I just remembered that I've wanted to refactor the pgcrypto calls
> for symmetric encryption to use the newer EVP API for some time, and
> even posted a patch for that
> (https://www.postgresql.org/message-id/561274F1.1030000@iki.fi). I
> dropped the ball back then, but I think I'll go ahead and do that now,
> once we get these other OpenSSL changes in.

Nice!

Andreas

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> On 08/30/2016 08:42 AM, Heikki Linnakangas wrote:
>> PS. I just remembered that I've wanted to refactor the pgcrypto calls
>> for symmetric encryption to use the newer EVP API for some time, and
>> even posted a patch for that
>> (https://www.postgresql.org/message-id/561274F1.1030000@iki.fi). I
>> dropped the ball back then, but I think I'll go ahead and do that now,
>> once we get these other OpenSSL changes in.

> Nice!

Judging by the number of people who have popped up recently with their
own OpenSSL 1.1 patches, I think there is going to be a lot of demand for
back-patching some sort of 1.1 support into our back branches. All this
talk of refactoring does not sound very back-patchable. Should we be
thinking of what we can extract that is back-patchable?

regards, tom lane

On 09/05/2016 02:23 AM, Tom Lane wrote:
> Judging by the number of people who have popped up recently with their
> own OpenSSL 1.1 patches, I think there is going to be a lot of demand for
> back-patching some sort of 1.1 support into our back branches. All this
> talk of refactoring does not sound very back-patchable. Should we be
> thinking of what we can extract that is back-patchable?

My idea is that the first of my four patches contains the minimum
changes needed to add support for 1.1 and tries to do as little
refactoring as possible while the other patches refactor things. I am
not sure about if anything of the other patches should be backpatched.

Andreas

On Mon, Sep 5, 2016 at 9:32 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> On 09/05/2016 02:23 AM, Tom Lane wrote:
>>
>> Judging by the number of people who have popped up recently with their
>> own OpenSSL 1.1 patches, I think there is going to be a lot of demand for
>> back-patching some sort of 1.1 support into our back branches. All this
>> talk of refactoring does not sound very back-patchable. Should we be
>> thinking of what we can extract that is back-patchable?
>
> My idea is that the first of my four patches contains the minimum changes
> needed to add support for 1.1 and tries to do as little refactoring as
> possible while the other patches refactor things. I am not sure about if
> anything of the other patches should be backpatched.

From what I can see of the 4 patches proposed, those are not that much
invasive, so a backpatch of those is really doable. But yes let's keep
the refactoring only for HEAD. That's definitely the safest approach.
--
Michael

On 09/05/2016 03:12 AM, Andreas Karlsson wrote:
> On 08/30/2016 08:42 AM, Heikki Linnakangas wrote:
>> There's the ResourceOwner mechanism, see src/backend/utils/resowner/.
>> That would be the proper way to do this. Call
>> RegisterResourceReleaseCallback() when the context is allocated, and
>> have the callback free it. One pitfall to watch out for is that
>> RegisterResourceReleaseCallback() itself calls palloc(), and can error
>> out, so you have to do things in such an order that you don't leak in
>> that case either.
>>
>> Want to take a stab at that?
>>
>> Another approach is put each allocated context in a list or array in a
>> global variable, and to register a callback to be called at
>> end-of-(sub)transaction, which closes all the contexts. But the resource
>> owner mechanism is probably easier.
>>
>> There's also PG_TRY-CATCH, that you could maybe use in the callers of
>> px_find_digest(), to make sure they call px_free_digest() even on error.
>> But that also seems difficult to use with the pgp_encrypt() pipeline.
>
> Sure, I have attached a patch where I try to use it.

Thanks! Unfortunately the callback mechanism is a bit more complicated
to use than that. The list of registered callbacks is global, so the
callback gets called for every ResourceOwner that's closed, not just the
one that was active when you registered it. Also, unregistering the
callback from within the callback is not safe. I fixed those things in
the (first) attached patch.

On 09/05/2016 03:23 AM, Tom Lane wrote:
> Judging by the number of people who have popped up recently with their
> own OpenSSL 1.1 patches, I think there is going to be a lot of demand for
> back-patching some sort of 1.1 support into our back branches. All this
> talk of refactoring does not sound very back-patchable. Should we be
> thinking of what we can extract that is back-patchable?

Yes, I think you're right.

The minimum set of changes is the first of the attached patches. It
includes Andreas' first patch, with the configure changes and other
changes needed to compile, and a working version of the resourceowner
callback mechanism to make sure we don't leak OpenSSL handles in pgcrypto.

Maybe we could get away without the resourceowner mechanism, and just
accept the minor memory leaks on the rare error case (out-of-memory
might be the only situation where that happen). Or #ifdef it so that we
keep the old embed-in-palloced-struct approach for OpenSSL versions
older than 1.1. But on the whole, I'd prefer to keep the code the same
in all branches.

The second patch attached here includes Andreas' second and third
patches, to silence deprecation warnings. That's not strictly required,
but seems safe enough that I think we should back-patch that too. It
needs one additional #ifdef version check in generate_dh_params(), if we
want it to still work with OpenSSL 0.9.7, but that's easy. I'm assuming
we want to still support it in back-branches, even though we just
dropped it from master.

- Heikki

On 09/05/2016 02:52 PM, Heikki Linnakangas wrote:
> On 09/05/2016 03:23 AM, Tom Lane wrote:
>> Judging by the number of people who have popped up recently with their
>> own OpenSSL 1.1 patches, I think there is going to be a lot of demand for
>> back-patching some sort of 1.1 support into our back branches. All this
>> talk of refactoring does not sound very back-patchable. Should we be
>> thinking of what we can extract that is back-patchable?
>
> Yes, I think you're right.

I planned to commit this today, but while reading through it and
testing, I ended up doing a bunch more changes, so this deserves another
round of review.

Changes since last version:

* Added more error checks to the my_BIO_s_socket() function. Check for
NULL result from malloc(). Check the return code of BIO_meth_set_*()
functions; looking at OpenSSL sources, they always succeed, but all the
test/example programs that come with OpenSSL do check them.

* Use BIO_get_new_index() to get the index number for the wrapper BIO.

* Also call BIO_meth_set_puts(). It was missing in previous patch versions.

* Fixed src/test/ssl test suite to also work with OpenSSL 1.1.0.

* Changed all references (in existing code) to SSLEAY_VERSION_NUMBER
into OPENSSL_VERSION_NUMBER, for consistency.

* Squashed all into one patch.

I intend to apply this to all supported branches, so please have a look!
This is now against REL9_6_STABLE, but there should be little difference
between branches in the code that this touches.

- Heikki

On Tue, Sep 13, 2016 at 1:51 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> I planned to commit this today, but while reading through it and testing, I
> ended up doing a bunch more changes, so this deserves another round of
> review.

OK, I am giving it a try. Note to people using OSX: at least for brew
there is the formula openssl(at)1(dot)1 that you can use with the following
flags:
CFLAGS="-I/usr/local/opt/openssl(at)1(dot)1/include"
LDFLAGS="-L/usr/local/opt/openssl(at)1(dot)1/lib"
Postgres is not the only broken thing, so they kept the formula
openssl to 1.0.2.

> Changes since last version:
>
> * Added more error checks to the my_BIO_s_socket() function. Check for NULL
> result from malloc(). Check the return code of BIO_meth_set_*() functions;
> looking at OpenSSL sources, they always succeed, but all the test/example
> programs that come with OpenSSL do check them.
>
> * Use BIO_get_new_index() to get the index number for the wrapper BIO.
>
> * Also call BIO_meth_set_puts(). It was missing in previous patch versions.
>
> * Fixed src/test/ssl test suite to also work with OpenSSL 1.1.0.
>
> * Changed all references (in existing code) to SSLEAY_VERSION_NUMBER into
> OPENSSL_VERSION_NUMBER, for consistency.
>
> * Squashed all into one patch.
>
> I intend to apply this to all supported branches, so please have a look!
> This is now against REL9_6_STABLE, but there should be little difference
> between branches in the code that this touches.

I just took a look at this patch, testing it on the way with 1.1.0 and
1.0.2. And it looks in pretty good shape.

+ ResourceOwner owner;
+ struct OSSLDigest *next;
+ struct OSSLDigest *prev;
This could be done as well with a list of pg_list, the cost being a
couple of extra calls to switch memory contexts, but it would simplify
free_openssldigest when cleaning up an entry. I guessed you already
thought about that but discarded it?
--
Michael

On 09/12/2016 06:51 PM, Heikki Linnakangas wrote:
> Changes since last version:
>
> * Added more error checks to the my_BIO_s_socket() function. Check for
> NULL result from malloc(). Check the return code of BIO_meth_set_*()
> functions; looking at OpenSSL sources, they always succeed, but all the
> test/example programs that come with OpenSSL do check them.
>
> * Use BIO_get_new_index() to get the index number for the wrapper BIO.
>
> * Also call BIO_meth_set_puts(). It was missing in previous patch versions.
>
> * Fixed src/test/ssl test suite to also work with OpenSSL 1.1.0.
>
> * Changed all references (in existing code) to SSLEAY_VERSION_NUMBER
> into OPENSSL_VERSION_NUMBER, for consistency.
>
> * Squashed all into one patch.
>
> I intend to apply this to all supported branches, so please have a look!
> This is now against REL9_6_STABLE, but there should be little difference
> between branches in the code that this touches.

This patch no longer seems to apply to head after the removed support of
0.9.6. Is that intentional?

Andreas

On 09/15/2016 02:03 AM, Andreas Karlsson wrote:
> On 09/12/2016 06:51 PM, Heikki Linnakangas wrote:
>> Changes since last version:
>>
>> * Added more error checks to the my_BIO_s_socket() function. Check for
>> NULL result from malloc(). Check the return code of BIO_meth_set_*()
>> functions; looking at OpenSSL sources, they always succeed, but all the
>> test/example programs that come with OpenSSL do check them.
>>
>> * Use BIO_get_new_index() to get the index number for the wrapper BIO.
>>
>> * Also call BIO_meth_set_puts(). It was missing in previous patch
>> versions.
>>
>> * Fixed src/test/ssl test suite to also work with OpenSSL 1.1.0.
>>
>> * Changed all references (in existing code) to SSLEAY_VERSION_NUMBER
>> into OPENSSL_VERSION_NUMBER, for consistency.
>>
>> * Squashed all into one patch.
>>
>> I intend to apply this to all supported branches, so please have a look!
>> This is now against REL9_6_STABLE, but there should be little difference
>> between branches in the code that this touches.
>
> This patch no longer seems to apply to head after the removed support of
> 0.9.6. Is that intentional?

Never mind. I just failed at reading.

Now for a review:

It looks generally good but I think I saw one error. In
fe-secure-openssl.c your code still calls SSL_library_init() in OpenSSL
1.1. I think it should be enough to just call
OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL) like you do in be-secure.

Andreas

On 09/15/2016 03:16 AM, Andreas Karlsson wrote:
> Now for a review:
>
> It looks generally good but I think I saw one error. In
> fe-secure-openssl.c your code still calls SSL_library_init() in OpenSSL
> 1.1. I think it should be enough to just call
> OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL) like you do in be-secure.

Ok, fixed that, and committed. Thanks everyone!

I backpatched this to 9.5, but not further than that. The functions this
modified were moved around in 9.5, so the patch wouldn't apply as is. It
wouldn't be difficult to back-patch further if there's demand, but I'm
not eager to do that until someone complains.

- Heikki

On Thu, Sep 15, 2016 at 8:57 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> I backpatched this to 9.5, but not further than that. The functions this
> modified were moved around in 9.5, so the patch wouldn't apply as is. It
> wouldn't be difficult to back-patch further if there's demand, but I'm not
> eager to do that until someone complains.

Not going older than 9.5 may be fine:
https://www.openssl.org/blog/blog/2014/12/23/the-new-release-strategy/
https://wiki.freebsd.org/OpenSSL
As far as I can see 1.0.2 would be supported until Dec 2019, so that
would just overlap with 9.4's EOL.
--
Michael

Re: Michael Paquier 2016-09-15 <CAB7nPqQu1GpMzkB4S6XO0_+1cAUx==RDVF70vCmDytuA=nCHiQ(at)mail(dot)gmail(dot)com>
> On Thu, Sep 15, 2016 at 8:57 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> > I backpatched this to 9.5, but not further than that. The functions this
> > modified were moved around in 9.5, so the patch wouldn't apply as is. It
> > wouldn't be difficult to back-patch further if there's demand, but I'm not
> > eager to do that until someone complains.
>
> Not going older than 9.5 may be fine:
> https://www.openssl.org/blog/blog/2014/12/23/the-new-release-strategy/
> https://wiki.freebsd.org/OpenSSL
> As far as I can see 1.0.2 would be supported until Dec 2019, so that
> would just overlap with 9.4's EOL.

I'm afraid it's not that easy - Debian 9 (stretch) will release at the
beginning of next year, and apt.postgresql.org will want to build
9.2/9.3/9.4 for that distribution. I guess yum.postgresql.org will
have the same problem with the next Fedora release.

Christoph

Christoph Berg wrote:
> Re: Michael Paquier 2016-09-15 <CAB7nPqQu1GpMzkB4S6XO0_+1cAUx==RDVF70vCmDytuA=nCHiQ(at)mail(dot)gmail(dot)com>
> > On Thu, Sep 15, 2016 at 8:57 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> > > I backpatched this to 9.5, but not further than that. The functions this
> > > modified were moved around in 9.5, so the patch wouldn't apply as is. It
> > > wouldn't be difficult to back-patch further if there's demand, but I'm not
> > > eager to do that until someone complains.
> >
> > Not going older than 9.5 may be fine:
> > https://www.openssl.org/blog/blog/2014/12/23/the-new-release-strategy/
> > https://wiki.freebsd.org/OpenSSL
> > As far as I can see 1.0.2 would be supported until Dec 2019, so that
> > would just overlap with 9.4's EOL.
>
> I'm afraid it's not that easy - Debian 9 (stretch) will release at the
> beginning of next year, and apt.postgresql.org will want to build
> 9.2/9.3/9.4 for that distribution. I guess yum.postgresql.org will
> have the same problem with the next Fedora release.

I suppose some interested party could grab the patch that Heikki
committed to the new branches and produce a back-patch that can be
applied to the older branches.

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

On 09/15/2016 05:33 PM, Christoph Berg wrote:
> Re: Michael Paquier 2016-09-15 <CAB7nPqQu1GpMzkB4S6XO0_+1cAUx==RDVF70vCmDytuA=nCHiQ(at)mail(dot)gmail(dot)com>
>> On Thu, Sep 15, 2016 at 8:57 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>>> I backpatched this to 9.5, but not further than that. The functions this
>>> modified were moved around in 9.5, so the patch wouldn't apply as is. It
>>> wouldn't be difficult to back-patch further if there's demand, but I'm not
>>> eager to do that until someone complains.
>>
>> Not going older than 9.5 may be fine:
>> https://www.openssl.org/blog/blog/2014/12/23/the-new-release-strategy/
>> https://wiki.freebsd.org/OpenSSL
>> As far as I can see 1.0.2 would be supported until Dec 2019, so that
>> would just overlap with 9.4's EOL.
>
> I'm afraid it's not that easy - Debian 9 (stretch) will release at the
> beginning of next year, and apt.postgresql.org will want to build
> 9.2/9.3/9.4 for that distribution. I guess yum.postgresql.org will
> have the same problem with the next Fedora release.

Can you elaborate? Are you saying that Debian 9 (strect) will not ship
OpenSSL 1.0.2 anymore, and will require using OpenSSL 1.1.0?

- Heikki

Re: Heikki Linnakangas 2016-09-15 <7e4991a9-410f-5e1f-2a3a-e918e4a4bbbb(at)iki(dot)fi>
> > I'm afraid it's not that easy - Debian 9 (stretch) will release at the
> > beginning of next year, and apt.postgresql.org will want to build
> > 9.2/9.3/9.4 for that distribution. I guess yum.postgresql.org will
> > have the same problem with the next Fedora release.
>
> Can you elaborate? Are you saying that Debian 9 (strect) will not ship
> OpenSSL 1.0.2 anymore, and will require using OpenSSL 1.1.0?

I thought that was the plan, but upon asking on #debian-devel, it
seems it's not set yet. I'll ask the maintainers directly and report
back.

Christoph

On 09/15/2016 05:38 PM, Alvaro Herrera wrote:
> I suppose some interested party could grab the patch that Heikki
> committed to the new branches and produce a back-patch that can be
> applied to the older branches.

Here is the result of backporting the sum of the two patches on top of
REL9_4_STABLE. Not sure if we need this, but if we do we can apply this
patch.

Andreas

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> On 09/15/2016 05:38 PM, Alvaro Herrera wrote:
>> I suppose some interested party could grab the patch that Heikki
>> committed to the new branches and produce a back-patch that can be
>> applied to the older branches.

> Here is the result of backporting the sum of the two patches on top of
> REL9_4_STABLE. Not sure if we need this, but if we do we can apply this
> patch.

If someone's done the legwork, I think we would be well advised to
back-patch. Maybe not bother with 9.1 though.

regards, tom lane

Re: To Heikki Linnakangas 2016-09-15 <20160915213406(dot)2mjlhcg7px3saynq(at)msg(dot)df7cb(dot)de>
> > Can you elaborate? Are you saying that Debian 9 (strect) will not ship
> > OpenSSL 1.0.2 anymore, and will require using OpenSSL 1.1.0?
>
> I thought that was the plan, but upon asking on #debian-devel, it
> seems it's not set yet. I'll ask the maintainers directly and report
> back.

The plan is to ship only OpenSSL 1.1 in Stretch. (The list of packages
not yet ported is enormous, though, so I'm not yet sure it will really
happen.)

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=827061

Re: Tom Lane 2016-09-16 <17025(dot)1473977329(at)sss(dot)pgh(dot)pa(dot)us>
> > Here is the result of backporting the sum of the two patches on top of
> > REL9_4_STABLE. Not sure if we need this, but if we do we can apply this
> > patch.
>
> If someone's done the legwork, I think we would be well advised to
> back-patch. Maybe not bother with 9.1 though.

Thanks for the patch!

I just tried to apply it to 9.2. There was a conflict in configure.in which was
trivial to resolve.

Another conflict in contrib/pgcrypto/pgcrypto.c was not applicable
because the code doesn't seem to exist (didn't try very hard though).

Ignoring the contrib conflict, it still didn't compile:

/home/cbe/projects/postgresql/debian/9.2/build/../src/backend/libpq/be-secure.c: In function ‘secure_write’:
/home/cbe/projects/postgresql/debian/9.2/build/../src/backend/libpq/be-secure.c:342:17: error: dereferencing pointer to incomplete type ‘SSL {aka struct ssl_st}’
if (port->ssl->state != SSL_ST_OK)
^~
/home/cbe/projects/postgresql/debian/9.2/build/../src/backend/libpq/be-secure.c:342:28: error: ‘SSL_ST_OK’ undeclared (first use in this function)
if (port->ssl->state != SSL_ST_OK)
^~~~~~~~~

Christoph

On 09/16/2016 04:11 PM, Christoph Berg wrote:
> Thanks for the patch!
>
> I just tried to apply it to 9.2. There was a conflict in configure.in which was
> trivial to resolve.
>
> Another conflict in contrib/pgcrypto/pgcrypto.c was not applicable
> because the code doesn't seem to exist (didn't try very hard though).
>
> Ignoring the contrib conflict, it still didn't compile:
>
> /home/cbe/projects/postgresql/debian/9.2/build/../src/backend/libpq/be-secure.c: In function ‘secure_write’:
> /home/cbe/projects/postgresql/debian/9.2/build/../src/backend/libpq/be-secure.c:342:17: error: dereferencing pointer to incomplete type ‘SSL {aka struct ssl_st}’
> if (port->ssl->state != SSL_ST_OK)
> ^~
> /home/cbe/projects/postgresql/debian/9.2/build/../src/backend/libpq/be-secure.c:342:28: error: ‘SSL_ST_OK’ undeclared (first use in this function)
> if (port->ssl->state != SSL_ST_OK)
> ^~~~~~~~~

This is related to the renegotiation which was first fixed and later
removed in the 9.4 cycle, but intentionally not backported. It seems
like OpenSSL refactored the state machine in 1.1 which is why the code
above breaks.

I am not entirely sure I follow what the old code in 9.3 and 9.2 is
strying to do and why it messes directly with the state of the statemachine.

Andreas

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> On 09/15/2016 05:38 PM, Alvaro Herrera wrote:
>> I suppose some interested party could grab the patch that Heikki
>> committed to the new branches and produce a back-patch that can be
>> applied to the older branches.

> Here is the result of backporting the sum of the two patches on top of
> REL9_4_STABLE. Not sure if we need this, but if we do we can apply this
> patch.

I've pushed this into 9.4 with trivial corrections (fix merge failure
against a later patch, and sync the autoconf output files with the
actual contents of configure.in). I've tested it locally against
openssl 1.0.1e and 1.1.0e, but not anything older. What I did to test
was to copy the 9.5-branch src/test/ssl/ stuff into 9.4 and run it.
I saw failures on the tests for Subject Alternative Name, which is
unsurprising since we added that support as a feature in 9.5, but
everything else passed. Unless the buildfarm turns up problems,
I think we're ok there.

I tried to push the code into 9.3, and saw the same problems Christoph
mentioned for 9.2: it compiles fine against 1.0.1e, but the references
to port->ssl->state don't work with 1.1. The reason that's OK in 9.4
is not that we removed SSL negotiation; that didn't happen until 9.5.
Rather, it's because this 9.4 commit got rid of the bogus code:

Author: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Branch: master Release: REL9_4_BR [31cf1a1a4] 2013-10-10 23:45:20 -0300

Rework SSL renegotiation code

If we want to go any further back with 1.1 support, we have a range
of options:

1. Back-patch that patch, probably also including the followup adjustments
in 86029b31e and 36a3be654.

2. Add #if's to use 31cf1a1a4's coding with OpenSSL >= 1.1, while keeping
the older code for use when built against older OpenSSLs.

3. Conditionally disable renegotiation altogether with OpenSSL >= 1.1,
thus adopting 9.5 not 9.4 behavior when using newer OpenSSL.

I think #3 would be fairly weird unless we also changed 9.4 similarly.
But there's some argument for doing that: we don't really have any field
experience with using renegotiation with OpenSSL 1.1, so we don't know
that what is in the 9.4 branch right now actually works with 1.1.
On the other hand, it would also be the most work of these options,
since we'd have to do things like adding conditional behavior in guc.c.

Thoughts?

For the archives' sake, attached is the 9.3-adapted version of the
patch so far.

regards, tom lane

I wrote:
> If we want to go any further back with 1.1 support, we have a range
> of options:
> 1. Back-patch that patch, probably also including the followup adjustments
> in 86029b31e and 36a3be654.
> 2. Add #if's to use 31cf1a1a4's coding with OpenSSL >= 1.1, while keeping
> the older code for use when built against older OpenSSLs.
> 3. Conditionally disable renegotiation altogether with OpenSSL >= 1.1,
> thus adopting 9.5 not 9.4 behavior when using newer OpenSSL.

> I think #3 would be fairly weird unless we also changed 9.4 similarly.
> But there's some argument for doing that: we don't really have any field
> experience with using renegotiation with OpenSSL 1.1, so we don't know
> that what is in the 9.4 branch right now actually works with 1.1.
> On the other hand, it would also be the most work of these options,
> since we'd have to do things like adding conditional behavior in guc.c.

I did some simple testing and can say that at least the successful path
in the 9.4 code seems to be fine with 1.1; in particular I see no sign
of the misbehavior discussed in
https://www.postgresql.org/message-id/flat/20150126101405.GA31719%40awork2.anarazel.de
Given Heikki's opinion that that was an OpenSSL bug, perhaps they
fixed the bug. Certainly we don't seem to have committed any of
the workaround patches discussed in that thread.

At this point I'd be inclined to reject option #3: aside from being
more work, it'd be a pain to document, and confusing for users.
I have a slight preference for #1 over #2 --- we'd intended to back-patch
Alvaro's fixes once they had survived some field use, and I know of no
evidence that 9.4 is worse than the older branches.

regards, tom lane

On 04/16/2017 03:14 AM, Tom Lane wrote:
> 1. Back-patch that patch, probably also including the followup adjustments
> in 86029b31e and 36a3be654.
>
> 2. Add #if's to use 31cf1a1a4's coding with OpenSSL >= 1.1, while keeping
> the older code for use when built against older OpenSSLs.
>
> 3. Conditionally disable renegotiation altogether with OpenSSL >= 1.1,
> thus adopting 9.5 not 9.4 behavior when using newer OpenSSL.
>
> [...]
>
> Thoughts?

Given that I cannot recall seeing any complaints about the behavior of
9.4 compared to 9.3 I am leaning towards #1. That way there are fewer
different versions of our OpenSSL code.

Andreas

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> On 04/16/2017 03:14 AM, Tom Lane wrote:
>> 1. Back-patch that patch, probably also including the followup adjustments
>> in 86029b31e and 36a3be654.

> Given that I cannot recall seeing any complaints about the behavior of
> 9.4 compared to 9.3 I am leaning towards #1. That way there are fewer
> different versions of our OpenSSL code.

Yeah, I was thinking about that point too. Barring objections I'll
do #1 and then move forward with the openssl 1.1 backport.

regards, tom lane