v7-0002-Review-fixups.patch

From bfa6a26b4610258d510b49a3fbdc8a249a757576 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Mon, 3 Mar 2025 16:17:14 +0100
Subject: [PATCH v7 2/2] Review fixups

---
 configure.ac                             |  2 +-
 doc/src/sgml/installation.sgml           |  2 +-
 doc/src/sgml/libpq.sgml                  | 19 +++++++---
 meson.build                              |  1 +
 src/include/pg_config.h.in               |  3 ++
 src/interfaces/libpq/fe-secure-openssl.c | 46 ++++++++++++++++--------
 src/test/ssl/t/001_ssltests.pl           | 42 ++++++++++++----------
 7 files changed, 76 insertions(+), 39 deletions(-)

diff --git a/configure.ac b/configure.ac
index b6d02f5ecc7..c6f61f7e3a8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1370,7 +1370,7 @@ if test "$with_ssl" = openssl ; then
   # Function introduced in OpenSSL 1.0.2, not in LibreSSL.
   AC_CHECK_FUNCS([SSL_CTX_set_cert_cb])
   # Function introduced in OpenSSL 1.1.1, not in LibreSSL.
-  AC_CHECK_FUNCS([X509_get_signature_info SSL_CTX_set_num_tickets])
+  AC_CHECK_FUNCS([X509_get_signature_info SSL_CTX_set_num_tickets SSL_CTX_set_keylog_callback])
   AC_DEFINE([USE_OPENSSL], 1, [Define to 1 to build with OpenSSL support. (--with-ssl=openssl)])
 elif test "$with_ssl" != no ; then
   AC_MSG_ERROR([--with-ssl must specify openssl])
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index 39fd16651b9..e076cefa3b9 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -301,7 +301,7 @@
       Additionally, <productname>LibreSSL</productname> is supported using the
       <productname>OpenSSL</productname> compatibility layer.  The minimum
       required version is 3.4 (from <systemitem class="osname">OpenBSD</systemitem>
-      version 7.5).
+      version 7.0).
      </para>
     </listitem>
 
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 31b711004aa..cea8acd3ccf 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1922,10 +1922,21 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
       <term><literal>sslkeylogfile</literal></term>
       <listitem>
        <para>
-        This parameter specifies the location where libpq will log keys
-        used in this SSL context. This is useful for debugging postgres
-        protocol using tools like wireshark. This parameter is ignored if an
-        SSL connection is not made. Keys are logged in the NSS format.
+        This parameter specifies the location where <literal>libpq</literal>
+        will log keys used in this SSL context.  This is useful for debugging
+        <productname>PostgreSQL</productname> protocol interations or client
+        connections using network inspection tools like
+        <productname>Wireshark</productname>.  This parameter is ignored if an
+        SSL connection is not made, or if <productname>LibreSSL</productname>
+        is used.  Keys are logged in the <productname>NSS</productname> format.
+
+        <warning>
+         <para>
+          Key logging will expose potentially sensitive information in the
+          keylog file and it should be handled with the same caution as
+          <xref linkend="libpq-connect-sslkey" /> files.
+         </para>
+        </warning>
        </para>
       </listitem>
      </varlistentry>
diff --git a/meson.build b/meson.build
index 13c13748e5d..07c51e7db58 100644
--- a/meson.build
+++ b/meson.build
@@ -1472,6 +1472,7 @@ if sslopt in ['auto', 'openssl']
       # Function introduced in OpenSSL 1.1.1, not in LibreSSL.
       ['X509_get_signature_info'],
       ['SSL_CTX_set_num_tickets'],
+      ['SSL_CTX_set_keylog_callback'],
     ]
 
     are_openssl_funcs_complete = true
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index db6454090d2..64351a3adaa 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -361,6 +361,9 @@
 /* Define to 1 if you have the `SSL_CTX_set_ciphersuites' function. */
 #undef HAVE_SSL_CTX_SET_CIPHERSUITES
 
+/* Define to 1 if you have the `SSL_CTX_set_keylog_callback' function. */
+#undef HAVE_SSL_CTX_SET_KEYLOG_CALLBACK
+
 /* Define to 1 if you have the `SSL_CTX_set_num_tickets' function. */
 #undef HAVE_SSL_CTX_SET_NUM_TICKETS
 
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index b0ca09b6700..fb6c99f4200 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -87,7 +87,6 @@ static pthread_mutex_t ssl_config_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 static PQsslKeyPassHook_OpenSSL_type PQsslKeyPassHook = NULL;
 static int	ssl_protocol_version_to_openssl(const char *protocol);
-static void SSL_CTX_keylog_cb(const SSL *ssl, const char *line);
 
 /* ------------------------------------------------------------ */
 /*			 Procedures common to all secure sessions			*/
@@ -686,12 +685,22 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn *conn,
 /* See pqcomm.h comments on OpenSSL implementation of ALPN (RFC 7301) */
 static unsigned char alpn_protos[] = PG_ALPN_PROTOCOL_VECTOR;
 
-/* This is a callback that writes to a given ssl key log file */
-static void SSL_CTX_keylog_cb(const SSL *ssl, const char *line) {
-	int fd;
-	mode_t old_umask;
-	ssize_t bytes_written;
-	PGconn *conn = SSL_get_app_data(ssl);
+#ifdef HAVE_SSL_CTX_SET_KEYLOG_CALLBACK
+/*
+ * SSL Key Logging callback
+ *
+ * This callback lets the user store all key material to a file for debugging
+ * purposes.  The file will be written using the NSS keylog format.  LibreSSL
+ * 3.5 introduce stub function to set the callback for OpenSSL compatibility,
+ * but the callback is never invoked.
+ */
+static void
+SSL_CTX_keylog_cb(const SSL *ssl, const char *line)
+{
+	int			fd;
+	mode_t		old_umask;
+	ssize_t		rc;
+	PGconn	   *conn = SSL_get_app_data(ssl);
 
 	if (conn == NULL)
 		return;
@@ -700,19 +709,24 @@ static void SSL_CTX_keylog_cb(const SSL *ssl, const char *line) {
 	fd = open(conn->sslkeylogfile, O_WRONLY | O_APPEND | O_CREAT, 0600);
 	umask(old_umask);
 
-	if (fd == -1) {
-		libpq_append_conn_error(conn, "could not open ssl key log file %s: %s", conn->sslkeylogfile, pg_strerror(errno));
+	if (fd == -1)
+	{
+		libpq_append_conn_error(conn, "could not open ssl key log file %s: %s",
+								conn->sslkeylogfile, pg_strerror(errno));
 		return;
 	}
 
-	bytes_written = dprintf(fd, "%s\n", line);
-	if (bytes_written < 0) {
-		libpq_append_conn_error(conn, "could not write to ssl key log file %s: %s", conn->sslkeylogfile, pg_strerror(errno));
-		close(fd);
-		return;
-	}
+	/* line is guaranteed by OpenSSL to be NUL terminated */
+	rc = write(fd, line, strlen(line));
+	if (rc < 0)
+		libpq_append_conn_error(conn, "could not write to ssl key log file %s: %s",
+								conn->sslkeylogfile, pg_strerror(errno));
+	else
+		rc = write(fd, "\n", 1);
+	(void) rc;					/* silence compiler warnings */
 	close(fd);
 }
+#endif
 
 /*
  *	Create per-connection SSL object, and load the client certificate,
@@ -1030,8 +1044,10 @@ initialize_SSL(PGconn *conn)
 	}
 	conn->ssl_in_use = true;
 
+#ifdef HAVE_SSL_CTX_SET_KEYLOG_CALLBACK
 	if (conn->sslkeylogfile && strlen(conn->sslkeylogfile) > 0)
 		SSL_CTX_set_keylog_callback(SSL_context, SSL_CTX_keylog_cb);
+#endif
 
 	/*
 	 * SSL contexts are reference counted by OpenSSL. We can free it as soon
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index ecf9bf7dd93..8cd66a56656 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -147,30 +147,36 @@ my $default_ssl_connstr =
 $common_connstr =
   "$default_ssl_connstr user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
 
-my $tempdir = PostgreSQL::Test::Utils::tempdir;
+SKIP:
+{
+	skip "Keylogging is not supported with LibreSSL" if $libressl;
 
-# Connect should work with a given sslkeylogfile
-$node->connect_ok(
-        "$common_connstr sslrootcert=ssl/root+server_ca.crt sslkeylogfile=$tempdir/key.txt sslmode=require",
-        "connect with server root cert and sslkeylogfile=$tempdir/key.txt");
+	my $tempdir = PostgreSQL::Test::Utils::tempdir;
 
-# Verify the key file exists
-ok(-f "$tempdir/key.txt", "key log file exists");
+	# Connect should work with a given sslkeylogfile
+	$node->connect_ok(
+		"$common_connstr sslrootcert=ssl/root+server_ca.crt sslkeylogfile=$tempdir/key.txt sslmode=require",
+		"connect with server root cert and sslkeylogfile=$tempdir/key.txt");
 
-# Skip permission checks on Windows/Cygwin
-SKIP:
-{
-        skip "Permissions check not enforced on Windows", 1
-        if ($windows_os || $Config::Config{osname} eq 'cygwin');
+	# Verify the key file exists
+	ok(-f "$tempdir/key.txt", "key log file exists");
+
+	# Skip permission checks on Windows/Cygwin
+  SKIP:
+	{
+		skip "Permissions check not enforced on Windows", 1
+		  if ($windows_os || $Config::Config{osname} eq 'cygwin');
 
-        my $mode = (stat("$tempdir/key.txt"))[2];
-        my $permissions_ok = 1;
+		my $mode = (stat("$tempdir/key.txt"))[2];
+		my $permissions_ok = 1;
 
-        if ($mode & 0006) {
-            $permissions_ok = 0;
-        }
+		if ($mode & 0006)
+		{
+			$permissions_ok = 0;
+		}
 
-        ok($permissions_ok, "key log file is not world readble");
+		ok($permissions_ok, "key log file is not world readble");
+	}
 }
 
 # The server should not accept non-SSL connections.
-- 
2.39.3 (Apple Git-146)