Inconsistent error handling in the openssl init code
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t40083psql -h localhost -U postgresBuilt from patchset v3 (message #3), July 28, 2026 at 04:38 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t40083_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t40083_3 && git checkout t40083_3Patchset v3 (message #3) is on t40083_3
The errorhandling in be_tls_init(), and functions called from it, set the
appropriate elevel by the isServerStart. ssl_protocol_version_to_openssl() is
however erroring out unconditionally with ERROR on invalid TLS versions. The
attached patch adds isServerStart handling to the TLS version handling as well,
to make be_tls_init() consistent in its errorhandling.
cheers ./daniel
Attachments:
openssl_tlsver.patchapplication/octet-stream; name=openssl_tlsver.patch; x-unix-mode=0644Download+23-10
On Wed, Feb 06, 2019 at 11:18:22PM +0100, Daniel Gustafsson wrote:
The errorhandling in be_tls_init(), and functions called from it, set the
appropriate elevel by the isServerStart. ssl_protocol_version_to_openssl() is
however erroring out unconditionally with ERROR on invalid TLS versions. The
attached patch adds isServerStart handling to the TLS version handling as well,
to make be_tls_init() consistent in its errorhandling.
(Adding Peter Eisentraut in CC)
Good catch, this is an oversight from commit e73e67c7, which affects
only HEAD. The comment at the top of ssl_protocol_version_to_openssl
becomes incorrect as the function would not throw an error in a reload
context.
The current comment is that:
* If a version is passed that is not supported by the current OpenSSL
* version, then we throw an error, so that subsequent code can assume it's
* working with a supported version.
Which we could change to that:
..., then we throw an error as FATAL if isServerStart is true so as it
won't return. Otherwise, errors are logged as LOG level and return -1
to indicate trouble, preserving the old SSL state if any.
Peter, could you take care of it? Or should I?
--
Michael
On 7 Feb 2019, at 05:12, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Feb 06, 2019 at 11:18:22PM +0100, Daniel Gustafsson wrote:
The errorhandling in be_tls_init(), and functions called from it, set the
appropriate elevel by the isServerStart. ssl_protocol_version_to_openssl() is
however erroring out unconditionally with ERROR on invalid TLS versions. The
attached patch adds isServerStart handling to the TLS version handling as well,
to make be_tls_init() consistent in its errorhandling.(Adding Peter Eisentraut in CC)
Good catch, this is an oversight from commit e73e67c7, which affects
only HEAD. The comment at the top of ssl_protocol_version_to_openssl
becomes incorrect as the function would not throw an error in a reload
context.
Doh, managed to completely overlook that. The attached updated patch also
fixes the comment, thanks!
cheers ./daniel
On Thu, Feb 07, 2019 at 10:03:30AM +0100, Daniel Gustafsson wrote:
Doh, managed to completely overlook that. The attached updated patch also
fixes the comment, thanks!
That looks fine to me. Could you just add it to the next CF as a bug
fix so as we don't forget? I am pretty sure that Peter will look at
that soon.
--
Michael
On 8 Feb 2019, at 01:10, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Feb 07, 2019 at 10:03:30AM +0100, Daniel Gustafsson wrote:
Doh, managed to completely overlook that. The attached updated patch also
fixes the comment, thanks!That looks fine to me. Could you just add it to the next CF as a bug
fix so as we don't forget? I am pretty sure that Peter will look at
that soon.
Done, thanks! I took the liberty to mark you as reviewer since you’ve already
spent time looking at the patch.
cheers ./daniel
On Fri, Feb 08, 2019 at 09:36:59AM +0100, Daniel Gustafsson wrote:
Done, thanks! I took the liberty to mark you as reviewer since you’ve already
spent time looking at the patch.
Thanks. Please note that I can take care of the patch in a couple of
days if need be.
--
Michael
On 08/02/2019 11:01, Michael Paquier wrote:
On Fri, Feb 08, 2019 at 09:36:59AM +0100, Daniel Gustafsson wrote:
Done, thanks! I took the liberty to mark you as reviewer since you’ve already
spent time looking at the patch.Thanks. Please note that I can take care of the patch in a couple of
days if need be.
Fixed, thanks.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 8 Feb 2019, at 12:01, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 08/02/2019 11:01, Michael Paquier wrote:
On Fri, Feb 08, 2019 at 09:36:59AM +0100, Daniel Gustafsson wrote:
Done, thanks! I took the liberty to mark you as reviewer since you’ve already
spent time looking at the patch.Thanks. Please note that I can take care of the patch in a couple of
days if need be.Fixed, thanks.
Thanks, I’ve closed it in the commitfest with you as committer.
cheers ./daniel