> On 10 Apr 2024, at 09:31, Peter Eisentraut <peter@eisentraut.org> wrote:
> I think it might be better to separate this into two steps:
Fair enough.
> 1. Move to 1.1.0. This is an API update. Change OPENSSL_API_COMPAT, and remove a bunch of code that no longer needs
tobe conditional. We could check for a representative function like OPENSSL_init_ssl() in configure/meson, or we could
justlet the compilation fail with older versions.
The attached 0002 bumps the minimum required version to 1.1.0 with a hard error
in autoconf/meson, and removes all the 1.0.2 support code.
I think the documentation for PQinitOpenSSL should be reworded from "you have
to do this, unless you run 1.1.0 or later" to "If you run 1.1.0, you need to do
this). As it is now the important bit of the paragrapg is at the end rather
than in the beginning. Trying this I didn't find a wording which seemed like
an improvement though, suggestions are welcome.
> 2. Move to 1.1.1. I understand this has to do with the fork-safety of pg_strong_random(), and it's not an API change
buta behavior change. Let's make this association clearer in the code. For example, add a version check or assertion
aboutthis into pg_strong_random() itself.
0005 moves the fork safety init inline with calling pg_strong_random, and
removes it for everyone else. This allows 1.1.0 to be supported as we
effectively are at the 1.1.0 API level, at the cost of calls for strong random
being slower on 1.1.0. An unscientific guess based on packaged OpenSSL
versions and the EOL and ELS/LTS status of 1.1.0, is that the number of
production installs of postgres 17 using OpenSSL 1.1.0 is close to zero.
> I don't know how LibreSSL interacts with either of these two points. That's something that could be clearer.
The oldest LibreSSL we have in the buildfarm is 3.2 (from OpenBSD 6.8), which
the attached version supports and passes tests with. LibreSSL has been
providing fork safety since 2.0.2 which is well into the past.
> * doc/src/sgml/libpq.sgml
>
> This small documentation patch could be committed forthwith.
Agreed, separated into 0001 in the attached and can be committed regardless of
the remaining ones.
> * src/backend/libpq/be-secure-openssl.c
>
> +#include <openssl/bn.h>
>
> This patch doesn't appear to add anything, so why does it need a new include?
As mentioned downthread, an indirect inclusion was removed so we need to
explicitly include it.
> Could the additions of SSL_OP_NO_CLIENT_RENEGOTIATION and SSL_R_VERSION_TOO_LOW be separate patches?
They can, done in the attached.
SSL_R_VERSION_TOO_LOW was introduced quite recently in LibreSSL 3.6.3 (OpenBSD
7.2), but splitting the check for TOO_LOW and TOO_HIGH into two seems pretty
uncontroversial and a good way to get ever so slightly better error handling on
recent LibreSSL.
SSL renegotiation has been supported for much longer in LibreSSL so adding that
to make OpenSSL and LibreSSL support slightly more on par seems seems like a
good idea regardless of version bump.
> * src/common/hmac_openssl.c
>
> There appears to be some unrelated refactoring happening here?
I assume you mean changing back to FRONTEND from USE_RESOWNER_FOR_HMAC, the
latter was added recently in 38698dd38 and have never shipped, so it seemed
more backpatch-friendly to move back. Given the amount of changes it probably
won't move the needle though so reverted.
> * src/include/common/openssl.h
>
> Is the comment no longer applicable to OpenSSL, only to LibreSSL?
OpenSSL has since 0.9.8 defined TLS_MAX_VERSION which points highest version
TLS protocol supported, but AFAIK there is no such construction in LibreSSL.
Assuming I didn't totally misunderstand the comment of course.
> * src/port/pg_strong_random.c
>
> I would prefer to remove pg_strong_random_init() if it's no longer useful. I mean, if we leave it as is, and we are
notremoving any callers, then we are effectively continuing to support OpenSSL <1.1.1, right?
The attached removes pg_strong_random_init and instead calls it explicitly for
1.1.0 users by checking the OpenSSL version.
Is the attached split in line with how you were thinking about it?
--
Daniel Gustafsson