Horton

Thread

Commits

  1. Skip RSA-PSS ssl test when using LibreSSL.

  2. Ooops ... add required configure support.

  3. Hack one ssl test case to pass with current LibreSSL.

  4. Centralize ssl tests' check for whether we're using LibreSSL.

  5. Re-enable SSL connect_fails tests, and fix related race conditions.

  6. Disable unstable test cases in src/test/ssl/t/001_ssltests.pl.

  1. disabled SSL log_like tests

    Andrew Dunstan <andrew@dunslane.net> — 2025-04-17T14:17:09Z

    Back in 2022 in commit 55828a6b6084 we disabled a bunch of tests due to 
a timing issue. Nothing seems to have been done since ... do we still 
want to keep these commented out lines around? This "temporary" fix 
seems to have stretched the definition of that term more than somewhat.

(noticed when looking into a different issue).


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  2. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-17T14:56:25Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> Back in 2022 in commit 55828a6b6084 we disabled a bunch of tests due to 
> a timing issue. Nothing seems to have been done since ... do we still 
> want to keep these commented out lines around? This "temporary" fix 
> seems to have stretched the definition of that term more than somewhat.

Per my coffee cup, "Nothing is as permanent as a temporary fix".

However, I wonder whether Andres' work at 8b886a4e3 could be used
to improve this, either directly or as inspiration?

			regards, tom lane

  3. Re: disabled SSL log_like tests

    Andrew Dunstan <andrew@dunslane.net> — 2025-04-17T18:37:55Z

    On 2025-04-17 Th 10:56 AM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> Back in 2022 in commit 55828a6b6084 we disabled a bunch of tests due to
>> a timing issue. Nothing seems to have been done since ... do we still
>> want to keep these commented out lines around? This "temporary" fix
>> seems to have stretched the definition of that term more than somewhat.
> Per my coffee cup, "Nothing is as permanent as a temporary fix".
>
> However, I wonder whether Andres' work at 8b886a4e3 could be used
> to improve this, either directly or as inspiration?
>
> 			



I don't think so - these tests are about checking log file contents, not 
a psql problem.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  4. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-18T19:45:58Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-04-17 Th 10:56 AM, Tom Lane wrote:
>> However, I wonder whether Andres' work at 8b886a4e3 could be used
>> to improve this, either directly or as inspiration?

> I don't think so - these tests are about checking log file contents, not 
> a psql problem.

Well, I was mainly leaning on the "inspiration" part: the idea
is to wait for something that must come out after the text we
want to look for.  After digging around a bit I noticed that
002_connection_limits.pl's connect_fails_wait is doing something
that's almost what we want: that test cranks log_min_messages
up to DEBUG2 and then waits for the postmaster's report of
backend exit before believing that it's done.

Awhile later I had the attached patch.  Some notes:

* The commented-out tests in 001_ssltests.pl contained hard-wired
expectations as to certificate serial numbers, which are obsolete now.
I just replaced them with "\d+", but if anyone feels like that's not
good enough, we could continue to check for exact serial numbers and
eat the ensuing maintenance effort.

* I was more than slightly surprised to find that there are a bunch of
other connect_fails callers that are testing log_like or log_unlike
and thereby risking the same type of race condition.  Some of those
tests are relatively new and perhaps just haven't failed *yet*,
but I wonder if we changed something since 2022 that solves this
problem in a different way?  Anyway, after this change any such
caller must set log_min_messages = debug2 or fail.  I think I got
all the relevant test scripts in the attached.

			regards, tom lane

  5. Re: disabled SSL log_like tests

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-18T20:08:43Z

    On Fri, Apr 18, 2025 at 12:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> * The commented-out tests in 001_ssltests.pl contained hard-wired
> expectations as to certificate serial numbers, which are obsolete now.
> I just replaced them with "\d+", but if anyone feels like that's not
> good enough, we could continue to check for exact serial numbers and
> eat the ensuing maintenance effort.

No, I think \d+ should be fine.

(I haven't stared closely at the racing code yet, but I like the
concept of the patch.)

Thanks!
--Jacob

  6. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-18T22:26:35Z

    I wrote:
> * I was more than slightly surprised to find that there are a bunch of
> other connect_fails callers that are testing log_like or log_unlike
> and thereby risking the same type of race condition.  Some of those
> tests are relatively new and perhaps just haven't failed *yet*,
> but I wonder if we changed something since 2022 that solves this
> problem in a different way?

Nope, apparently not.  The failure depends on the kernel's scheduler,
so unsurprisingly it's quite platform-dependent.  But I can reproduce
it reliably on mamba's host (NetBSD 10/macppc) if I enable the
001_ssltests.pl log_like tests without applying the connect_fails
changes.

The fact that mamba hasn't been failing on the other affected
tests is a bit puzzling.  mamba isn't running kerberos or ldap
or oauth_validator, so the lack of failures there is expected.
authentication/t/001_password.pl appears accidentally not vulnerable:
it's not using log_like in any connect_fails tests.  (It is using
log_unlike, so those tests could be giving silent false negatives.)
But authentication/t/003_peer.pl has 8 test cases that look
vulnerable.  I guess there must be some extra dollop of timing
weirdness in the ssl tests that's not there in 003_peer.pl.

Unfortunately ... it sometimes fails even with the connect_fails
changes, for example

#   Failed test 'intermediate client certificate is missing: log matches'
#   at /home/tgl/pgsql/src/test/ssl/../../../src/test/perl/PostgreSQL/Test/Cluster.pm line 2666.
#                   '2025-04-18 17:59:19.358 EDT [1460] DEBUG:  assigned pm child slot 2 for backend
# 2025-04-18 17:59:19.359 EDT [1460] DEBUG:  forked new client backend, pid=599 socket=8
# 2025-04-18 17:59:19.369 EDT [599] [unknown] LOG:  connection received: host=localhost port=63709
# 2025-04-18 17:59:19.436 EDT [1460] DEBUG:  releasing pm child slot 1
# 2025-04-18 17:59:19.436 EDT [1460] DEBUG:  client backend (PID 25401) exited with exit code 0
# '
#     doesn't match '(?^:Client certificate verification failed at depth 0: unable to get local issuer certificate)'

What I think happened here is that a previous backend hadn't exited
yet when we start the test, and when its report does come out,
connect_fails prematurely stops waiting.  (In the above, evidently
the child process we want to wait for is 599, but we're fooled by
a delayed report for 25401.)  So my v1 patch needs work.

Maybe we can make the test compare the PIDs in the "forked new client
backend" and "client backend exited" log messages.  Stay tuned...

			regards, tom lane

  7. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-18T23:26:32Z

    I wrote:
> What I think happened here is that a previous backend hadn't exited
> yet when we start the test, and when its report does come out,
> connect_fails prematurely stops waiting.  (In the above, evidently
> the child process we want to wait for is 599, but we're fooled by
> a delayed report for 25401.)  So my v1 patch needs work.

> Maybe we can make the test compare the PIDs in the "forked new client
> backend" and "client backend exited" log messages.  Stay tuned...

Okay, this version seems more reliable.

			regards, tom lane

  8. Re: disabled SSL log_like tests

    Andrew Dunstan <andrew@dunslane.net> — 2025-04-19T11:47:26Z

    On 2025-04-18 Fr 7:26 PM, Tom Lane wrote:
> I wrote:
>> What I think happened here is that a previous backend hadn't exited
>> yet when we start the test, and when its report does come out,
>> connect_fails prematurely stops waiting.  (In the above, evidently
>> the child process we want to wait for is 599, but we're fooled by
>> a delayed report for 25401.)  So my v1 patch needs work.
>> Maybe we can make the test compare the PIDs in the "forked new client
>> backend" and "client backend exited" log messages.  Stay tuned...
> Okay, this version seems more reliable.
>

+See C<log_check(...)>.  CAUTION: use of either option requires that
+the server's log_min_messages be at least DEBUG2, and that no other
+client backend is launched concurrently.  These requirements allow
+C<connect_fails> to wait to see the postmaster-log report of backend
+exit, without which there is a race condition as to whether we will
+see the expected backend log output.


That seems a little fragile. I can imagine test authors easily 
forgetting this. Is it worth sanity checking to make sure 
log_min_messages is appropriately set?


cheers


andrew




>
--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  9. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-19T14:31:47Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-04-18 Fr 7:26 PM, Tom Lane wrote:
> +See C<log_check(...)>.  CAUTION: use of either option requires that
> +the server's log_min_messages be at least DEBUG2, and that no other
> +client backend is launched concurrently.  These requirements allow
> +C<connect_fails> to wait to see the postmaster-log report of backend
> +exit, without which there is a race condition as to whether we will
> +see the expected backend log output.

> That seems a little fragile. I can imagine test authors easily 
> forgetting this. Is it worth sanity checking to make sure 
> log_min_messages is appropriately set?

Setting log_min_messages is not so easily forgotten, because
connect_fails will just hang until timeout if you didn't.

I'm more worried about the "no other backend" requirement.
I think v2 is reasonably proof against that, but whether it's
sufficiently bulletproof to withstand the buildfarm environment
remains to be seen.  I wish there were a better way to
determine the backend PID for a failed connection...

			regards, tom lane

  10. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-04-22T19:33:42Z

    I wrote:
> The fact that mamba hasn't been failing on the other affected
> tests is a bit puzzling.  mamba isn't running kerberos or ldap
> or oauth_validator, so the lack of failures there is expected.
> authentication/t/001_password.pl appears accidentally not vulnerable:
> it's not using log_like in any connect_fails tests.  (It is using
> log_unlike, so those tests could be giving silent false negatives.)
> But authentication/t/003_peer.pl has 8 test cases that look
> vulnerable.  I guess there must be some extra dollop of timing
> weirdness in the ssl tests that's not there in 003_peer.pl.

After pushing this patch, the probable explanation for the "extra
timing weirdness" finally penetrated my brain: the error reports
we are looking for are cases that are detected by OpenSSL.  So
it's plausible that OpenSSL has told the connecting client to
go away before it returns the error indication to the backend's
calling code, which would be what would log the message.  That is
what provides the window for a race condition.  You still need
a bunch of assumptions about the kernel's subsequent scheduling
of psql and the TAP script versus the backend, but the whole
thing is certainly dependent on psql getting the word sooner
than the backend.

(Not all of the tests disabled by 55828a6b6 meet this description,
but a bunch of them do, and I believe that I just zapped every
log_like condition in the script rather than trying to be selective
about which ones were known to have failed.)

It seems at least plausible that the Kerberos tests could have
a similar problem.  I'm less sure about LDAP or OAuth.  But
authentication/t/003_peer.pl would not, because elog.c emits errors
to the log before sending them to the client.  So that explains
the lack of buildfarm reports from mamba, and it's unclear that
we have any other animals with the right timing behavior to
see this.

			regards, tom lane

  11. Re: disabled SSL log_like tests

    Thomas Munro <thomas.munro@gmail.com> — 2025-05-05T19:39:09Z

    If you run the not-yet-enabled-by-default OpenBSD CI task on master,
ssl/001_ssltests fails in "intermediate client certificate is
untrusted", recently uncommented by commit e0f373ee.  I think it might
be telling us that LibreSSL's x509_store_ctx_get_current_cert() is
giving us the client certificate (ie chain depth 0) instead of the
intermediate certificate, even though X509_STORE_CTX_get_error_depth()
returned 1 as expected.  I don't know why it would do that, given the
documentation:

X509_STORE_CTX_get_current_cert() returns the certificate in ctx which
caused the error or NULL if no certificate is relevant.

The explanation is probably in here somewhere, but I don't understand
these things:

https://github.com/openbsd/src/blob/master/lib/libcrypto/x509/x509_vfy.c
https://github.com/openssl/openssl/blob/master/crypto/x509/x509_vfy.c

[17:55:28.888] #   Failed test 'intermediate client certificate is
untrusted: log matches'
[17:55:28.888] #   at
/home/postgres/postgres/src/test/perl/PostgreSQL/Test/Cluster.pm line
2667.
[17:55:28.888] #                   '2025-05-05 17:55:28.353 UTC
[10009][postmaster] DEBUG:  assigned pm child slot 1 for backend
[17:55:28.888] # 2025-05-05 17:55:28.354 UTC [10009][postmaster]
DEBUG:  forked new client backend, pid=27624 socket=8
[17:55:28.888] # 2025-05-05 17:55:28.355 UTC [27624][not initialized]
[[unknown]][:0] LOG:  connection received: host=localhost port=11357
[17:55:28.888] # 2025-05-05 17:55:28.374 UTC [27624][not initialized]
[[unknown]][:0] LOG:  could not accept SSL connection: certificate
verify failed
[17:55:28.888] # 2025-05-05 17:55:28.374 UTC [27624][not initialized]
[[unknown]][:0] DETAIL:  Client certificate verification failed at
depth 1: unable to get local issuer certificate.
[17:55:28.888] #     Failed certificate data (unverified): subject
"/CN=ssltestuser", serial number 2315702411956921344, issuer "/CN=Test
CA for PostgreSQL SSL regression test client certs".
[17:55:28.888] # 2025-05-05 17:55:28.374 UTC [27624][not initialized]
[[unknown]][:0] DEBUG:  SSL connection from DN:"(anonymous)"
CN:"(anonymous)"
[17:55:28.888] # 2025-05-05 17:55:28.377 UTC [10009][postmaster]
DEBUG:  releasing pm child slot 1
[17:55:28.888] # 2025-05-05 17:55:28.377 UTC [10009][postmaster]
DEBUG:  client backend (PID 27624) exited with exit code 0
[17:55:28.888] # '
[17:55:28.888] #     doesn't match '(?^:Failed certificate data
\(unverified\): subject "/CN=Test CA for PostgreSQL SSL regression
test client certs", serial number \d+, issuer "/CN=Test root CA for
PostgreSQL SSL regression test suite")'
[17:55:28.888] # Looks like you failed 1 test of 240.

https://cirrus-ci.com/task/4708964002168832?logs=test_world#L345
https://api.cirrus-ci.com/v1/artifact/task/4708964002168832/testrun/build/testrun/ssl/001_ssltests/log/regress_log_001_ssltests
https://api.cirrus-ci.com/v1/artifact/task/4708964002168832/testrun/build/testrun/ssl/001_ssltests/log/001_ssltests_primary.log

  12. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-05T22:45:24Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> If you run the not-yet-enabled-by-default OpenBSD CI task on master,
> ssl/001_ssltests fails in "intermediate client certificate is
> untrusted", recently uncommented by commit e0f373ee.

Yeah, I see that too.  But I also see three failures in 002_scram.pl,
which presumably were there before e0f373ee.  (Tested on OpenBSD 7.6
and 7.7.)  The buildfarm's OpenBSD animals haven't caught this
because they don't run this test suite :-(.  Yes they build with
--with-openssl, but one of them lacks --enable-tap-tests and the
other two aren't filling PG_TEST_EXTRA.

The SCRAM failures are a bit discouraging ...

[18:16:33.259](0.565s) not ok 26 - SCRAM with SSL and channel_binding=require, server certificate uses 'rsassaPss'
[18:16:33.261](0.002s) 
[18:16:33.261](0.000s) #   Failed test 'SCRAM with SSL and channel_binding=require, server certificate uses 'rsassaPss''
#   at t/002_scram.pl line 161.
[18:16:33.262](0.001s) #          got: '2'
#     expected: '0'
[18:16:33.264](0.002s) not ok 27 - SCRAM with SSL and channel_binding=require, server certificate uses 'rsassaPss': no stderr
[18:16:33.265](0.001s) 
[18:16:33.265](0.000s) #   Failed test 'SCRAM with SSL and channel_binding=require, server certificate uses 'rsassaPss': no stderr'
#   at t/002_scram.pl line 161.
[18:16:33.266](0.001s) #          got: 'psql: error: connection to server at "127.0.0.1", port 10442 failed: SSL error: sslv3 alert handshake failure'
#     expected: ''
[18:16:33.268](0.002s) not ok 28 - SCRAM with SSL and channel_binding=require, server certificate uses 'rsassaPss': log matches
[18:16:33.269](0.001s) 
[18:16:33.269](0.000s) #   Failed test 'SCRAM with SSL and channel_binding=require, server certificate uses 'rsassaPss': log matches'
#   at /home/tgl/pgsql/src/test/ssl/../../../src/test/perl/PostgreSQL/Test/Cluster.pm line 2607.
[18:16:33.270](0.001s) #                   '2025-05-05 18:16:33.222 EDT [71478] [unknown] LOG:  connection received: host=localhost port=42632
# 2025-05-05 18:16:33.244 EDT [71478] [unknown] LOG:  could not accept SSL connection: missing rsa certificate
# '
#     doesn't match '(?^:connection authenticated: identity="ssltestuser" method=scram-sha-256)'

			regards, tom lane

  13. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-07T01:18:11Z

    I wrote:
> Yeah, I see that too.  But I also see three failures in 002_scram.pl,
> which presumably were there before e0f373ee.  (Tested on OpenBSD 7.6
> and 7.7.)  The buildfarm's OpenBSD animals haven't caught this
> because they don't run this test suite :-(.

I dug into this with gdb, and it seems like it's an omission in
LibreSSL's RSA-PSS support.  We're requesting a signature algorithm
with code 2054, which in their source is SIGALG_RSA_PSS_RSAE_SHA512,
and that leads them to this sigalgs[] table entry in
lib/libssl/ssl_sigalgs.c:

	{
		.value = SIGALG_RSA_PSS_RSAE_SHA512,
		.key_type = EVP_PKEY_RSA,
		.md = EVP_sha512,
		.security_level = 5,
		.flags = SIGALG_FLAG_RSA_PSS,
	},

The problem is that the private key has key_type EVP_PKEY_RSA_PSS
which is not equal to EVP_PKEY_RSA, so ssl_sigalg_pkey_ok() in the
same file rejects this entry as not compatible.  In fact, there
are *no* entries in sigalgs[] that can match a PSS private key
according to ssl_sigalg_pkey_ok().  So while perhaps
SIGALG_RSA_PSS_RSAE_SHA512 is the wrong thing for us to request,
I do not see a right thing.

Looking around a little more, there are places like
SSL_get_signature_type_nid() that do things like this:

	*nid = sigalg->key_type;
	if (sigalg->key_type == EVP_PKEY_RSA &&
	    (sigalg->flags & SIGALG_FLAG_RSA_PSS))
		*nid = EVP_PKEY_RSA_PSS;

So it seems like this might be a simple oversight in
ssl_sigalg_pkey_ok(), which doesn't make any such correction:

	if (sigalg->key_type != EVP_PKEY_id(pkey))
		return 0;

Anyone know anything about where to submit LibreSSL bugs?

			regards, tom lane

  14. Re: disabled SSL log_like tests

    Thomas Munro <thomas.munro@gmail.com> — 2025-05-07T03:52:22Z

    On Wed, May 7, 2025 at 1:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So it seems like this might be a simple oversight in
> ssl_sigalg_pkey_ok(), which doesn't make any such correction:
>
>         if (sigalg->key_type != EVP_PKEY_id(pkey))
>                 return 0;

Nice detective work.

> Anyone know anything about where to submit LibreSSL bugs?

I think it's done with sendbug on an OpenBSD box, or perhaps you can
just write a normal email to the bugs@openbsd.org or
libressl@openbsd.org list, based on:

https://www.openbsd.org/mail.html

  15. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-07T04:34:12Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Wed, May 7, 2025 at 1:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Anyone know anything about where to submit LibreSSL bugs?

> I think it's done with sendbug on an OpenBSD box, or perhaps you can
> just write a normal email to the bugs@openbsd.org or
> libressl@openbsd.org list, based on:
> https://www.openbsd.org/mail.html

Thanks, I'll look into reporting it tomorrow.  In the meantime,
I couldn't help noticing that the backtraces went through
lib/libssl/tls13_legacy.c, which doesn't give a warm feeling
about how supported they think our usage is (and perhaps also
explains why they didn't detect this bug themselves).  This is
evidently because we set up the SSL context with SSLv23_method(),
per this comment in be_tls_init():

     * We use SSLv23_method() because it can negotiate use of the highest
     * mutually supported protocol version, while alternatives like
     * TLSv1_2_method() permit only one specific version.  Note that we don't
     * actually allow SSL v2 or v3, only TLS protocols (see below).

This choice seems to be more than 20 years old, though the above
comment defending it dates only to 2014.  I wonder if it's time to
revisit that idea.

			regards, tom lane

  16. Re: disabled SSL log_like tests

    Thomas Munro <thomas.munro@gmail.com> — 2025-05-07T04:58:03Z

    On Wed, May 7, 2025 at 4:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thanks, I'll look into reporting it tomorrow.  In the meantime,
> I couldn't help noticing that the backtraces went through
> lib/libssl/tls13_legacy.c, which doesn't give a warm feeling
> about how supported they think our usage is (and perhaps also
> explains why they didn't detect this bug themselves).  This is
> evidently because we set up the SSL context with SSLv23_method(),
> per this comment in be_tls_init():

Oh, interesting.  I also wondered if the problem I reported might be
related to the separate legacy code paths in x509_vfy.c.

  17. Re: disabled SSL log_like tests

    Daniel Gustafsson <daniel@yesql.se> — 2025-05-07T06:54:48Z

    > On 7 May 2025, at 06:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Thomas Munro <thomas.munro@gmail.com> writes:
>> On Wed, May 7, 2025 at 1:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Anyone know anything about where to submit LibreSSL bugs?
> 
>> I think it's done with sendbug on an OpenBSD box, or perhaps you can
>> just write a normal email to the bugs@openbsd.org or
>> libressl@openbsd.org list, based on:
>> https://www.openbsd.org/mail.html

Bugs are frequently reported, and dealt with, on the libressl mailing list.

> Thanks, I'll look into reporting it tomorrow.  In the meantime,
> I couldn't help noticing that the backtraces went through
> lib/libssl/tls13_legacy.c, which doesn't give a warm feeling
> about how supported they think our usage is (and perhaps also
> explains why they didn't detect this bug themselves).  This is
> evidently because we set up the SSL context with SSLv23_method(),
> per this comment in be_tls_init():
> 
>     * We use SSLv23_method() because it can negotiate use of the highest
>     * mutually supported protocol version, while alternatives like
>     * TLSv1_2_method() permit only one specific version.  Note that we don't
>     * actually allow SSL v2 or v3, only TLS protocols (see below).
> 
> This choice seems to be more than 20 years old, though the above
> comment defending it dates only to 2014.  I wonder if it's time to
> revisit that idea.

The use of SSLv23_method() comes from us supporting 1.0.2, the replacement
TLS_method() was introduced in 1.1.0 and SSLv23_method() was turned into an
alias of TLS_method() in OpenSSL commit 32ec41539b5.

Since we no longer support 1.0.2 we can apply something like the (lightly
tested) attached which should be a no-op as we already use TLS_method() but via
an alias.

There's likely more OpenSSL code we can modernize, hopefully we can make some
progress on that during the v19 cycle.

--
Daniel Gustafsson

  18. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-07T16:04:39Z

    Daniel Gustafsson <daniel@yesql.se> writes:
>> On 7 May 2025, at 06:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I couldn't help noticing that the backtraces went through
>> lib/libssl/tls13_legacy.c, which doesn't give a warm feeling
>> about how supported they think our usage is (and perhaps also
>> explains why they didn't detect this bug themselves).

> Since we no longer support 1.0.2 we can apply something like the (lightly
> tested) attached which should be a no-op as we already use TLS_method() but via
> an alias.

Yeah, I saw that SSLv23_method() was merely an alias for TLS_method()
in LibreSSL as well.  That means unfortunately that your proposal is
just cosmetic and doesn't get us out of using code that they're
calling "legacy".  I wonder what it would take to get to the "modern"
code paths.

			regards, tom lane

  19. Re: disabled SSL log_like tests

    Daniel Gustafsson <daniel@yesql.se> — 2025-05-07T17:53:38Z

    > On 7 May 2025, at 18:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 7 May 2025, at 06:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I couldn't help noticing that the backtraces went through
>>> lib/libssl/tls13_legacy.c, which doesn't give a warm feeling
>>> about how supported they think our usage is (and perhaps also
>>> explains why they didn't detect this bug themselves).
> 
>> Since we no longer support 1.0.2 we can apply something like the (lightly
>> tested) attached which should be a no-op as we already use TLS_method() but via
>> an alias.
> 
> Yeah, I saw that SSLv23_method() was merely an alias for TLS_method()
> in LibreSSL as well.  That means unfortunately that your proposal is
> just cosmetic and doesn't get us out of using code that they're
> calling "legacy".  I wonder what it would take to get to the "modern"
> code paths.

AFAICT (it's not documented what I can see) the Libressl folks consider code
inherited from OpenSSL legacy.  Using current OpenSSL API's and moving away
from deprecated API's is probably our best bet.  On that note, TLS_method is
the current API to use in both OpenSSL and Libressl according to their
respective documentations.

A separate be-secure-libressl.c could move to use their libtls instead of the
libssl OpenSSL compatibility library, which may be an interesting excercise but
a very different project from what is discussed here.

--
Daniel Gustafsson

  20. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-07T19:29:39Z

    I filed a report with the LibreSSL folks, and the answer was basically
"yeah, we know RSA-PSS doesn't work; there are multiple things that
need fixing" [1].  So it seems like we'd better find a way to skip
that test case when using LibreSSL.  Not sure what the most
appropriate check would look like.

			regards, tom lane

[1] https://marc.info/?l=libressl&m=174664225002441&w=2

  21. Re: disabled SSL log_like tests

    Daniel Gustafsson <daniel@yesql.se> — 2025-05-07T19:33:18Z

    > On 7 May 2025, at 21:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I filed a report with the LibreSSL folks, and the answer was basically
> "yeah, we know RSA-PSS doesn't work; there are multiple things that
> need fixing" [1].  So it seems like we'd better find a way to skip
> that test case when using LibreSSL.  Not sure what the most
> appropriate check would look like.

I haven't looked at the test in question yet, but we do skip some SSL tests if
running against libressl already so I assume this will be able to follow the
same pattern.

--
Daniel Gustafsson

  22. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-07T21:27:11Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> Oh, interesting.  I also wondered if the problem I reported might be
> related to the separate legacy code paths in x509_vfy.c.

I poked at this a little more, and apparently the code to look at is
actually in the sibling file x509_verify.c.  Here's the backtrace
(in OpenBSD 7.7) from where we construct the incorrect report:

#0  verify_cb (ok=0, ctx=0x17d83e77c00) at be-secure-openssl.c:1213
#1  0x0000017ce17483dc in x509_verify (ctx=0x17d1886f700, leaf=0x17d188882a0, 
    name=0x0) at /usr/src/lib/libcrypto/x509/x509_verify.c:1271
#2  0x0000017ce16ed683 in _lcry_X509_verify_cert (ctx=0x17d83e77c00)
    at /usr/src/lib/libcrypto/x509/x509_vfy.c:645
#3  0x0000017d44ebc4e3 in ssl_verify_cert_chain (s=0x17d83e96500, 
    certs=0x17d83e5fce0) at /usr/src/lib/libssl/ssl_cert.c:437
#4  0x0000017d44eb3d13 in tls13_client_certificate_recv (ctx=0x17d18881f00, 
    cbs=<optimized out>) at /usr/src/lib/libssl/tls13_server.c:918
#5  0x0000017d44ee8d66 in tls13_handshake_recv_action (ctx=0x17d18881f00, 
    action=0x17d44ef6dd0 <state_machine+352>)
    at /usr/src/lib/libssl/tls13_handshake.c:549
#6  tls13_handshake_perform (ctx=0x17d18881f00)
    at /usr/src/lib/libssl/tls13_handshake.c:412
#7  0x0000017d44eb18c7 in tls13_legacy_accept (ssl=0x17d83e96500)
    at /usr/src/lib/libssl/tls13_legacy.c:434
#8  0x0000017a8bbd6f3a in be_tls_open_server (port=0x17d6d91f9b0)
    at be-secure-openssl.c:494
#9  0x0000017a8bbc41c8 in secure_open_server (port=0x17d6d91f9b0)
    at be-secure.c:136
#10 0x0000017a8be9d79e in ProcessStartupPacket (port=0x17d6d91f9b0, 
    ssl_done=false, gss_done=false) at backend_startup.c:614

The code in x509_verify() is about as convoluted as can be:
it seems to be dealing with two different verify-callback APIs
(and naturally, the one we are using is the legacy one).
There is some stuff there that saves and restores an error
condition, but fails to save/restore current_cert, so the first
thing I thought was that that was the problem --- but it's not,
the current_cert doesn't change anyway between the save and the
restore.  So I failed to identify exactly where the problem is,
but I wonder if it'd behave better if we switched to their
new-style callback API.

			regards, tom lane

  23. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-07T21:54:31Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> I haven't looked at the test in question yet, but we do skip some SSL tests if
> running against libressl already so I assume this will be able to follow the
> same pattern.

Ah, thanks for the tip.  I propose the attached, which disables the
RSA-PSS test altogether on LibreSSL, and modifies the
intermediate-cert test to accept the result we're actually getting
on LibreSSL.  We could revert that one if anyone can figure out
how to make it better, but I don't wish to put any more time into
it myself.

			regards, tom lane

  24. Re: disabled SSL log_like tests

    Daniel Gustafsson <daniel@yesql.se> — 2025-05-08T11:20:06Z

    > On 7 May 2025, at 23:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>> I haven't looked at the test in question yet, but we do skip some SSL tests if
>> running against libressl already so I assume this will be able to follow the
>> same pattern.
> 
> Ah, thanks for the tip.  I propose the attached, which disables the
> RSA-PSS test altogether on LibreSSL, and modifies the
> intermediate-cert test to accept the result we're actually getting
> on LibreSSL.  We could revert that one if anyone can figure out
> how to make it better, but I don't wish to put any more time into
> it myself.

LGTM for now.

> +# Determine whether this build uses OpenSSL or LibreSSL. As a heuristic, the
> +# HAVE_SSL_CTX_SET_CERT_CB macro isn't defined for LibreSSL.
> +my $libressl = not check_pg_config("#define HAVE_SSL_CTX_SET_CERT_CB 1");

Longer term it would be nice to move this into SSL::Server and have the module
export a function or symbol which returns the underlying library and version,
but that's not for this patch.

> +
> +# As of 5/2025, LibreSSL doesn't actually work for RSA-PSS certificates.

Should we add a link to the relevant thread for future readers?  OpenBSD refer
to MARC for archiving which I believe is stable enough for an inclusion.

	https://marc.info/?l=libressl&m=174664225002441&w=2

--
Daniel Gustafsson

  25. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-08T13:49:37Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 7 May 2025, at 23:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +# Determine whether this build uses OpenSSL or LibreSSL. As a heuristic, the
>> +# HAVE_SSL_CTX_SET_CERT_CB macro isn't defined for LibreSSL.
>> +my $libressl = not check_pg_config("#define HAVE_SSL_CTX_SET_CERT_CB 1");

> Longer term it would be nice to move this into SSL::Server and have the module
> export a function or symbol which returns the underlying library and version,
> but that's not for this patch.

I was feeling itchy about having two copies of code that looks none
too set-in-stone.  Maybe we should just do that.  Any preferences
on the API?

>> +# As of 5/2025, LibreSSL doesn't actually work for RSA-PSS certificates.

> Should we add a link to the relevant thread for future readers?  OpenBSD refer
> to MARC for archiving which I believe is stable enough for an inclusion.

WFM, I'll make it so.

			regards, tom lane

  26. Re: disabled SSL log_like tests

    Daniel Gustafsson <daniel@yesql.se> — 2025-05-08T15:05:20Z

    > On 8 May 2025, at 15:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 7 May 2025, at 23:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> +# Determine whether this build uses OpenSSL or LibreSSL. As a heuristic, the
>>> +# HAVE_SSL_CTX_SET_CERT_CB macro isn't defined for LibreSSL.
>>> +my $libressl = not check_pg_config("#define HAVE_SSL_CTX_SET_CERT_CB 1");
> 
>> Longer term it would be nice to move this into SSL::Server and have the module
>> export a function or symbol which returns the underlying library and version,
>> but that's not for this patch.
> 
> I was feeling itchy about having two copies of code that looks none
> too set-in-stone.  Maybe we should just do that.  Any preferences
> on the API?

There is already SSL::Server::ssl_library() which returns the underlying
library, but it's not smart enough to differentiate between which flavour of
OpenSSL compatible library is being used (OpenSSL, Libressl, BoringSSL etc) as
it's only returning a hardcoded string as of now.  My plan was to expand that
at some point.

--
Daniel Gustafsson

  27. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-08T20:24:31Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 8 May 2025, at 15:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I was feeling itchy about having two copies of code that looks none
>> too set-in-stone.  Maybe we should just do that.  Any preferences
>> on the API?

> There is already SSL::Server::ssl_library() which returns the underlying
> library, but it's not smart enough to differentiate between which flavour of
> OpenSSL compatible library is being used (OpenSSL, Libressl, BoringSSL etc) as
> it's only returning a hardcoded string as of now.  My plan was to expand that
> at some point.

Hm.  There is this bit in 001_ssltests.pl:

my $result = $node->safe_psql('postgres', "SHOW ssl_library");
is($result, $ssl_server->ssl_library(), 'ssl_library parameter');

which would break.  Admittedly that's not a very exciting test,
so I wouldn't feel bad about dropping it, but maybe someone else
would.

Also, it seems like ssl_library is mainly intended to distinguish
which "backend" module is in use, so having the one string "OpenSSL"
seems to match up with the one backend "OpenSSL.pm".  What we're
talking about here feels like a finer subdivision.  I'm not quite
sure how it ought to fit into that "backend" structure.

We could deal with the RSA-PSS issue pretty cleanly by inventing a
backend method "supports_rsa_pss", but the other thing we're trying
to hack around doesn't seem like it has such a clean definition.

			regards, tom lane

  28. Re: disabled SSL log_like tests

    Daniel Gustafsson <daniel@yesql.se> — 2025-05-08T20:48:27Z

    > On 8 May 2025, at 22:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 8 May 2025, at 15:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I was feeling itchy about having two copies of code that looks none
>>> too set-in-stone.  Maybe we should just do that.  Any preferences
>>> on the API?
> 
>> There is already SSL::Server::ssl_library() which returns the underlying
>> library, but it's not smart enough to differentiate between which flavour of
>> OpenSSL compatible library is being used (OpenSSL, Libressl, BoringSSL etc) as
>> it's only returning a hardcoded string as of now.  My plan was to expand that
>> at some point.
> 
> Hm.  There is this bit in 001_ssltests.pl:
> 
> my $result = $node->safe_psql('postgres', "SHOW ssl_library");
> is($result, $ssl_server->ssl_library(), 'ssl_library parameter');
> 
> which would break.  Admittedly that's not a very exciting test,
> so I wouldn't feel bad about dropping it, but maybe someone else
> would.

I have no problems dropping that, it's rather uninteresting.

> Also, it seems like ssl_library is mainly intended to distinguish
> which "backend" module is in use, so having the one string "OpenSSL"
> seems to match up with the one backend "OpenSSL.pm".  What we're
> talking about here feels like a finer subdivision.  I'm not quite
> sure how it ought to fit into that "backend" structure.

The backend concept was mostly intended to match up with the underlying library.
It get's a bit murky as OpenSSL tough since it's a library, but also a popular API
compatibility target implemented by multiple libraries (Libressl, Boringssl,
Wolfssl come to mind).

Maybe the ssl_library function should return a hash with backend => 'OpenSSL'
and library => <the actual implementation used>?  Then the test author can
decide which level of compatibility they want?  If we were to end up with a
Libressl libtls implementation in libpq we'd still have to test with Libressl
against the OpenSSL compat layer in libssl since it could act as both.  Not a
bridge we have to cross today but might be worth at least keeping in mind when
designing something to not make it impossible in the future.

--
Daniel Gustafsson

  29. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-09T00:15:24Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> Maybe the ssl_library function should return a hash with backend => 'OpenSSL'
> and library => <the actual implementation used>?

I don't love doing it exactly like that: seems like it adds notational
complexity for little gain.  Also, it forces ssl_library to expend
work detecting things the current caller may not care about.

I was thinking about just transposing the existing test down to the
backend layer, more or less as attached.  Not wedded to these names
of course.

> If we were to end up with a
> Libressl libtls implementation in libpq we'd still have to test with Libressl
> against the OpenSSL compat layer in libssl since it could act as both.  Not a
> bridge we have to cross today but might be worth at least keeping in mind when
> designing something to not make it impossible in the future.

Right.  I think the attached would be amenable to that.

Further down the road, it seems inevitable that we'll need to have a
way of detecting the SSL library version --- for example, assuming
the LibreSSL folk eventually fix their RSA-PSS code, we'll need a
version-dependent test.  That could be another new backend method,
I guess.

			regards, tom lane

  30. Re: disabled SSL log_like tests

    Daniel Gustafsson <daniel@yesql.se> — 2025-05-09T07:24:57Z

    > On 9 May 2025, at 02:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:

>> If we were to end up with a
>> Libressl libtls implementation in libpq we'd still have to test with Libressl
>> against the OpenSSL compat layer in libssl since it could act as both.  Not a
>> bridge we have to cross today but might be worth at least keeping in mind when
>> designing something to not make it impossible in the future.
> 
> Right.  I think the attached would be amenable to that.

It will be a bit awkward to ask "are you libressl" if we ever add support for
something not OpenSSL based, but we could always revisit should that happen.

> Further down the road, it seems inevitable that we'll need to have a
> way of detecting the SSL library version --- for example, assuming
> the LibreSSL folk eventually fix their RSA-PSS code, we'll need a
> version-dependent test.  That could be another new backend method,
> I guess.

Agreed.

--
Daniel Gustafsson

  31. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-09T14:03:51Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 9 May 2025, at 02:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Right.  I think the attached would be amenable to that.

> It will be a bit awkward to ask "are you libressl" if we ever add support for
> something not OpenSSL based, but we could always revisit should that happen.

I was imagining that unrelated backends would simply make the method
report constant-false.  But in any case, redesigning this in the light
of any future requirements doesn't seem like a big deal.  What I'm
mainly after for today is just to ensure that these various which-
library-is-it tests are visibly interconnected, so that we don't
forget to check them all when things change.

			regards, tom lane

  32. Re: disabled SSL log_like tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-09T16:42:38Z

    Pushed all that stuff.  The SSL tests pass for me now on OpenBSD 7.7,
and hopefully the CI environment will be happy too.

			regards, tom lane

  33. Re: disabled SSL log_like tests

    Daniel Gustafsson <daniel@yesql.se> — 2025-05-09T16:47:54Z

    > On 9 May 2025, at 18:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Pushed all that stuff.  The SSL tests pass for me now on OpenBSD 7.7,
> and hopefully the CI environment will be happy too.

Thanks!

--
Daniel Gustafsson