pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
"openssl" should not be optional
9+ messages / 2 participants
[nested] [flat]

* "openssl" should not be optional
@ 2025-09-24 11:14  Christoph Berg <myon@debian.org>
  0 siblings, 1 reply; 9+ messages in thread

From: Christoph Berg @ 2025-09-24 11:14 UTC (permalink / raw)
  To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Debian's reproducible-builds machinery has discovered a problem in the
SSL tests: When building with SSL support, but /usr/bin/openssl
missing (i.e "libssl-dev" installed, but "openssl" missing), the tests
fail in subtle ways:

checking for openssl... no
configure: using openssl: openssl not found
checking for openssl/ssl.h... yes
checking for openssl/err.h... yes

build/src/test/ssl/tmp_check/log/regress_log_001_ssltests:
Can't exec "x509": No such file or directory at t/001_ssltests.pl line 751.
couldn't run " x509" to get client cert serialno at t/001_ssltests.pl line 775.

build/src/test/ssl/tmp_check/log/regress_log_003_sslinfo:
[08:42:02.209](0.029s) ok 11 - ssl_client_serial() compared with pg_stat_ssl
psql:<stdin>:1: ERROR:  invalid X.509 field name: "invalid"
[08:42:02.238](0.029s) ok 12 - ssl_client_dn_field() for an invalid field

Full build log: https://reproduce.debian.net/amd64-pull184/api/v1/builds/66623/log

The problem does not show up on the normal Debian build daemons. While
the build environment there is fairly minimal, it does have "openssl"
preinstalled. So I cannot yet say if this problem is new in PG18, or
just never got detected in older branches.

While it is probably possible to skip the tests when the configure
probe did not find the openssl binary, IMHO the configure check should
already fail. That's more robust and easier.

Attached is a WIP patch that implements that for autoconf.

Christoph

Attachments:

  [text/x-diff] v1-0001-openssl-should-not-be-optional.patch (1.2K, ../../aNPSp1-RIAs3skZm@msg.df7cb.de/2-v1-0001-openssl-should-not-be-optional.patch)
  download | inline diff:
From c6146aba7f09df30f4effe304f45f27bcb9975ac Mon Sep 17 00:00:00 2001
From: Christoph Berg <myon@debian.org>
Date: Wed, 24 Sep 2025 11:11:35 +0000
Subject: [PATCH v1] "openssl" should not be optional

---
 configure.ac | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index e44943aa6fe..ac0dccdeab5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1576,10 +1576,13 @@ if test "$with_gssapi" = yes ; then
 	[AC_CHECK_HEADERS(gssapi_ext.h, [], [AC_MSG_ERROR([gssapi_ext.h header file is required for GSSAPI])])])
 fi
 
-PGAC_PATH_PROGS(OPENSSL, openssl)
-pgac_openssl_version="$($OPENSSL version 2> /dev/null || echo openssl not found)"
-AC_MSG_NOTICE([using openssl: $pgac_openssl_version])
 if test "$with_ssl" = openssl ; then
+  PGAC_PATH_PROGS(OPENSSL, openssl)
+  if test -z "$OPENSSL"; then
+    AC_MSG_ERROR([openssl not found])
+  fi
+  pgac_openssl_version="$($OPENSSL version 2> /dev/null || echo openssl not found)"
+  AC_MSG_NOTICE([using openssl: $pgac_openssl_version])
   AC_CHECK_HEADER(openssl/ssl.h, [], [AC_MSG_ERROR([header file <openssl/ssl.h> is required for OpenSSL])])
   AC_CHECK_HEADER(openssl/err.h, [], [AC_MSG_ERROR([header file <openssl/err.h> is required for OpenSSL])])
 fi
-- 
2.51.0

^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: "openssl" should not be optional
@ 2025-09-24 11:22  Daniel Gustafsson <daniel@yesql.se>
  parent: Christoph Berg <myon@debian.org>
  0 siblings, 1 reply; 9+ messages in thread

From: Daniel Gustafsson @ 2025-09-24 11:22 UTC (permalink / raw)
  To: Christoph Berg <myon@debian.org>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

> On 24 Sep 2025, at 13:14, Christoph Berg <myon@debian.org> wrote:

> While it is probably possible to skip the tests when the configure
> probe did not find the openssl binary, IMHO the configure check should
> already fail. That's more robust and easier.

It seems a bit restrictive to require the openssl binary which is test-only,
since we allow building with ssl but without TAP support (which is where the
openssl binary is used).

--
Daniel Gustafsson






^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: "openssl" should not be optional
@ 2025-09-24 11:37  Christoph Berg <myon@debian.org>
  parent: Daniel Gustafsson <daniel@yesql.se>
  0 siblings, 1 reply; 9+ messages in thread

From: Christoph Berg @ 2025-09-24 11:37 UTC (permalink / raw)
  To: Daniel Gustafsson <daniel@yesql.se>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Re: Daniel Gustafsson
> It seems a bit restrictive to require the openssl binary which is test-only,
> since we allow building with ssl but without TAP support (which is where the
> openssl binary is used).

Ok, but then the error messages should be better. This was found
because a fellow Debian developer was smart enough to spot the extra
space in that " x509" error message... (And another one knew about
this difference between the different build environments.)

Christoph





^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: "openssl" should not be optional
@ 2025-09-24 11:40  Daniel Gustafsson <daniel@yesql.se>
  parent: Christoph Berg <myon@debian.org>
  0 siblings, 1 reply; 9+ messages in thread

From: Daniel Gustafsson @ 2025-09-24 11:40 UTC (permalink / raw)
  To: Christoph Berg <myon@debian.org>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

> On 24 Sep 2025, at 13:37, Christoph Berg <myon@debian.org> wrote:
> 
> Re: Daniel Gustafsson
>> It seems a bit restrictive to require the openssl binary which is test-only,
>> since we allow building with ssl but without TAP support (which is where the
>> openssl binary is used).
> 
> Ok, but then the error messages should be better. This was found
> because a fellow Debian developer was smart enough to spot the extra
> space in that " x509" error message... (And another one knew about
> this difference between the different build environments.)

If we make it optional and skip the relevant tests then there wouldn't be any
errors messages?  I do agree that all messaging around should be very clear
though, so it's obvious why tests were skipped.

Do you feel like expanding your patch or should I?

--
Daniel Gustafsson






^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: "openssl" should not be optional
@ 2025-09-24 11:51  Christoph Berg <myon@debian.org>
  parent: Daniel Gustafsson <daniel@yesql.se>
  0 siblings, 1 reply; 9+ messages in thread

From: Christoph Berg @ 2025-09-24 11:51 UTC (permalink / raw)
  To: Daniel Gustafsson <daniel@yesql.se>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Re: Daniel Gustafsson
> If we make it optional and skip the relevant tests then there wouldn't be any
> errors messages?

Or that, sure.

> Do you feel like expanding your patch or should I?

TBH I know very little about how TAP interfaces with the build system,
so that's better with you.

Thanks,
Christoph





^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: "openssl" should not be optional
@ 2025-09-24 13:57  Daniel Gustafsson <daniel@yesql.se>
  parent: Christoph Berg <myon@debian.org>
  0 siblings, 1 reply; 9+ messages in thread

From: Daniel Gustafsson @ 2025-09-24 13:57 UTC (permalink / raw)
  To: Christoph Berg <myon@debian.org>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

> On 24 Sep 2025, at 13:51, Christoph Berg <myon@debian.org> wrote:

>> Do you feel like expanding your patch or should I?
> 
> TBH I know very little about how TAP interfaces with the build system,
> so that's better with you.

Looking at this I was reminded that we already handle this by using a fallback
and the test worked all along.  The message for this was quite poorly worded
though, and used a warning instead of a note.  The attached will try to detect
openssl being missing before trying to run it, and will skip the warning
message if the fallback is used (which really isn't a warning in the first
place).

The ERROR in 003_sslinfo is intentional, we are testing that processing fails
by passing an invalid value.

--
Daniel Gustafsson

Attachments:

  [application/octet-stream] v2-0001-Avoid-warnings-in-ssl-tests-when-openssl-isn-t-av.patch (2.1K, ../../DB9931FA-8A28-4958-BC00-3667F527FCFC@yesql.se/2-v2-0001-Avoid-warnings-in-ssl-tests-when-openssl-isn-t-av.patch)
  download | inline diff:
From d50005e65f9b29b3d157e65ca7b6579c13b60bc6 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 24 Sep 2025 15:48:23 +0200
Subject: [PATCH v2] Avoid warnings in ssl tests when openssl isn't available

---
 src/test/ssl/t/001_ssltests.pl | 42 +++++++++++++++-------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index b2eb18d3e81..eaee88d027e 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -748,33 +748,29 @@ TODO:
 
 # pg_stat_ssl
 
-my $serialno = `$ENV{OPENSSL} x509 -serial -noout -in ssl/client.crt`;
-if ($? == 0)
-{
-	# OpenSSL prints serial numbers in hexadecimal and converting the serial
-	# from hex requires a 64-bit capable Perl as the serialnumber is based on
-	# the current timestamp. On 32-bit fall back to checking for it being an
-	# integer like how we do when grabbing the serial fails.
-	if ($Config{ivsize} == 8)
-	{
-		no warnings qw(portable);
+# If the openssl program isn't available, or fails to run, fall back to a
+# generic integer match rather than skipping the test.
+my $serialno = '\d+';
 
-		$serialno =~ s/^serial=//;
-		$serialno =~ s/\s+//g;
-		$serialno = hex($serialno);
-	}
-	else
+if ($ENV{OPENSSL} ne '')
+{
+	$serialno = `$ENV{OPENSSL} x509 -serial -noout -in ssl/client.crt`;
+	if ($? == 0)
 	{
-		$serialno = '\d+';
+		# OpenSSL prints serial numbers in hexadecimal and converting the serial
+		# from hex requires a 64-bit capable Perl as the serialnumber is based on
+		# the current timestamp. On 32-bit fall back to checking for it being an
+		# integer like how we do when grabbing the serial fails.
+		if ($Config{ivsize} == 8)
+		{
+			no warnings qw(portable);
+
+			$serialno =~ s/^serial=//;
+			$serialno =~ s/\s+//g;
+			$serialno = hex($serialno);
+		}
 	}
 }
-else
-{
-	# OpenSSL isn't functioning on the user's PATH. This probably isn't worth
-	# skipping the test over, so just fall back to a generic integer match.
-	warn "couldn't run \"$ENV{OPENSSL} x509\" to get client cert serialno";
-	$serialno = '\d+';
-}
 
 command_like(
 	[
-- 
2.39.3 (Apple Git-146)

=

^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: "openssl" should not be optional
@ 2025-09-24 15:13  Christoph Berg <myon@debian.org>
  parent: Daniel Gustafsson <daniel@yesql.se>
  0 siblings, 1 reply; 9+ messages in thread

From: Christoph Berg @ 2025-09-24 15:13 UTC (permalink / raw)
  To: Daniel Gustafsson <daniel@yesql.se>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Re: Daniel Gustafsson
> Looking at this I was reminded that we already handle this by using a fallback
> and the test worked all along.  The message for this was quite poorly worded
> though, and used a warning instead of a note.  The attached will try to detect
> openssl being missing before trying to run it, and will skip the warning
> message if the fallback is used (which really isn't a warning in the first
> place).

Thanks, I just built the postgresql-18 again with this patch (and
openssl not installed [*]). It passes fine now.

In the meantime, I also got the report that postgresql-17 is not
failing in that environment, so the problem is new in 18.

> The ERROR in 003_sslinfo is intentional, we are testing that processing fails
> by passing an invalid value.

Ah, I was mentioning that in the original report because it only
showed up in the failing log, but that's just because the non-failing
build does not go scraping the test log files. That made the problem
look bigger than it actually was.

Thanks,
Christoph

[*] future builds will have openssl as build-dependency.





^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: "openssl" should not be optional
@ 2025-09-24 15:49  Daniel Gustafsson <daniel@yesql.se>
  parent: Christoph Berg <myon@debian.org>
  0 siblings, 1 reply; 9+ messages in thread

From: Daniel Gustafsson @ 2025-09-24 15:49 UTC (permalink / raw)
  To: Christoph Berg <myon@debian.org>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

> On 24 Sep 2025, at 17:13, Christoph Berg <myon@debian.org> wrote:
> 
> Re: Daniel Gustafsson
>> Looking at this I was reminded that we already handle this by using a fallback
>> and the test worked all along.  The message for this was quite poorly worded
>> though, and used a warning instead of a note.  The attached will try to detect
>> openssl being missing before trying to run it, and will skip the warning
>> message if the fallback is used (which really isn't a warning in the first
>> place).
> 
> Thanks, I just built the postgresql-18 again with this patch (and
> openssl not installed [*]). It passes fine now.
> 
> In the meantime, I also got the report that postgresql-17 is not
> failing in that environment, so the problem is new in 18.

That's odd, off the cuff I don't see anything materially different around this
but I'll do some more digging.  It will at least be fixed by 18.1.

--
Daniel Gustafsson






^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: "openssl" should not be optional
@ 2025-09-24 17:27  Christoph Berg <myon@debian.org>
  parent: Daniel Gustafsson <daniel@yesql.se>
  0 siblings, 0 replies; 9+ messages in thread

From: Christoph Berg @ 2025-09-24 17:27 UTC (permalink / raw)
  To: Daniel Gustafsson <daniel@yesql.se>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Re: Daniel Gustafsson
> > In the meantime, I also got the report that postgresql-17 is not
> > failing in that environment, so the problem is new in 18.
> 
> That's odd, off the cuff I don't see anything materially different around this
> but I'll do some more digging.  It will at least be fixed by 18.1.

Because I had already added openssl to 17's build-dependencies and
forgot to port that change to 18 *blush*:

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

Christoph





^ permalink  raw  reply  [nested|flat] 9+ messages in thread


end of thread, other threads:[~2025-09-24 17:27 UTC | newest]

Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-09-24 11:14 "openssl" should not be optional Christoph Berg <myon@debian.org>
2025-09-24 11:22 ` Daniel Gustafsson <daniel@yesql.se>
2025-09-24 11:37   ` Christoph Berg <myon@debian.org>
2025-09-24 11:40     ` Daniel Gustafsson <daniel@yesql.se>
2025-09-24 11:51       ` Christoph Berg <myon@debian.org>
2025-09-24 13:57         ` Daniel Gustafsson <daniel@yesql.se>
2025-09-24 15:13           ` Christoph Berg <myon@debian.org>
2025-09-24 15:49             ` Daniel Gustafsson <daniel@yesql.se>
2025-09-24 17:27               ` Christoph Berg <myon@debian.org>

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox