> On 10 Apr 2024, at 09:31, Peter Eisentraut <pe...@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 to be conditional. We could > check for a representative function like OPENSSL_init_ssl() in > configure/meson, or we could just let 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 but a behavior change. Let's > make this association clearer in the code. For example, add a version check > or assertion about this 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 not removing 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
v7-0005-Remove-pg_strong_random-initialization.patch
Description: Binary data
v7-0004-Support-SSL_R_VERSION_TOO_LOW-on-LibreSSL.patch
Description: Binary data
v7-0003-Support-disallowing-SSL-renegotiation-in-LibreSSL.patch
Description: Binary data
v7-0002-Remove-support-for-OpenSSL-1.0.2.patch
Description: Binary data
v7-0001-Doc-Use-past-tense-for-things-which-happened-in-t.patch
Description: Binary data