On Mon, Aug 21, 2023 at 10:49:16AM -0700, Jacob Champion wrote: > On Sun, Aug 20, 2023 at 4:58 PM Michael Paquier <mich...@paquier.xyz> wrote: > > Attached is a v3 to do these two things, with adjustments for two SSL > > tests. Any objections about it? > > (Sorry for the long weekend delay.) No objections; you may want to > adjust the comment above the test block in t/001_password.pl, as well.
There are additionally two more comments in the SSL tests that could be removed, I guess. Here's a v4, with Robert's latest suggestion added. > I will ask -- more as a rhetorical question than something to resolve > for this patch, since the topic is going to come back with a vengeance > for OAuth -- what purpose the consistency here is serving. If the OP > wants to notice when a connection that should be using strong > authentication is not, is it helpful to make that connection "look the > same" in the logs? I understand we've been carrying the language > "trust authentication method" for a long time, but is that really the > only hang-up, or would there be pushback if I tried to change that > too, sometime in the future? I am not sure that we need to change this historic term, TBH. Perhaps it would be shorter to just rip off the trust method from the tree with a deprecation period but that's not something I'm much in favor off either (I use it daily for my own stuff, as one example). Another, more conservative approach may be to make it a developer-only option and discourage more its use in the docs. -- Michael
From b6eacc5084b1b7b1f711d756a1b1bc96c358394f Mon Sep 17 00:00:00 2001
From: Jacob Champion <champio...@gmail.com>
Date: Wed, 16 Aug 2023 14:48:49 -0700
Subject: [PATCH v4] log_connections: add entries for "trust" connections
Allow DBAs to audit unexpected "trust" connections. Previously, you had
to notice the absence of a "connection authenticated" line between the
"connection received" and "connection authorized" entries. Now it's
called out explicitly.
---
src/backend/libpq/auth.c | 16 ++++++++++++++++
src/test/authentication/t/001_password.pl | 11 +++++++----
src/test/ssl/t/001_ssltests.pl | 6 ++----
3 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 315a24bb3f..f6a42257b4 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -645,6 +645,22 @@ ClientAuthentication(Port *port)
#endif
}
+ if (Log_connections && (status == STATUS_OK) &&
+ !MyClientConnectionInfo.authn_id)
+ {
+ /*
+ * Normally, if log_connections is set, the call to set_authn_id()
+ * will log the connection. However, if that function is never
+ * called, perhaps because the trust method is in use, then we
+ * handle the logging here instead.
+ */
+ ereport(LOG,
+ errmsg("connection authenticated: user=\"%s\" method=%s "
+ "(%s:%d)",
+ port->user_name, hba_authname(port->hba->auth_method),
+ port->hba->sourcefile, port->hba->linenumber));
+ }
+
if (ClientAuthentication_hook)
(*ClientAuthentication_hook) (port, status);
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 12552837a8..9ba4308689 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -136,13 +136,16 @@ SKIP:
# Create a database to test regular expression.
$node->safe_psql('postgres', "CREATE database regex_testdb;");
-# For "trust" method, all users should be able to connect. These users are not
-# considered to be authenticated.
+# For "trust" method, all users should be able to connect.
reset_pg_hba($node, 'all', 'all', 'trust');
test_conn($node, 'user=scram_role', 'trust', 0,
- log_unlike => [qr/connection authenticated:/]);
+ log_like => [
+ qr/connection authenticated: user="scram_role" method=trust/
+ ]);
test_conn($node, 'user=md5_role', 'trust', 0,
- log_unlike => [qr/connection authenticated:/]);
+ log_like => [
+ qr/connection authenticated: user="md5_role" method=trust/
+ ]);
# SYSTEM_USER is null when not authenticated.
$res = $node->safe_psql('postgres', "SELECT SYSTEM_USER IS NULL;");
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 76442de063..ce17a760fa 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -800,8 +800,7 @@ $node->connect_ok(
"$common_connstr user=ssltestuser sslcert=ssl/client.crt "
. sslkey('client.key'),
"auth_option clientcert=verify-full succeeds with matching username and Common Name",
- # verify-full does not provide authentication
- log_unlike => [qr/connection authenticated:/],);
+ log_like => [qr/connection authenticated: user="ssltestuser" method=trust/],);
$node->connect_fails(
"$common_connstr user=anotheruser sslcert=ssl/client.crt "
@@ -818,8 +817,7 @@ $node->connect_ok(
"$common_connstr user=yetanotheruser sslcert=ssl/client.crt "
. sslkey('client.key'),
"auth_option clientcert=verify-ca succeeds with mismatching username and Common Name",
- # verify-full does not provide authentication
- log_unlike => [qr/connection authenticated:/],);
+ log_like => [qr/connection authenticated: user="yetanotheruser" method=trust/],);
# intermediate client_ca.crt is provided by client, and isn't in server's ssl_ca_file
switch_server_cert($node, certfile => 'server-cn-only', cafile => 'root_ca');
--
2.40.1
signature.asc
Description: PGP signature
