Message-ID: <65e643ec.050a0220.39f33.8bdf@mx.google.com>
Sender: Michael Banck <mbanck@gmail.com>
Date: Mon, 4 Mar 2024 22:58:03 +0100
From: Michael Banck <mbanck@gmx.net>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Abhijit Menon-Sen <ams@toroid.org>,
	pgsql-hackers@lists.postgresql.org,
	=?utf-8?B?5oiQ5LmL54SV?= <zhcheng@ceresdata.com>
Subject: Re: [PATCH] Exponential backoff for auth_delay
References: <658c4eab.050a0220.731ff.011e@mx.google.com>
 <65965e9d.050a0220.919a6.f9e1@mx.google.com>
 <ZaYimdRlGS5tdzjy@toroid.org>
 <65aa8265.170a0220.1ecef.91df@mx.google.com>
 <65aa8439.050a0220.31e77.0071@mx.google.com>
 <da09dacb-7158-4af1-99f8-6609070b7553@enterprisedb.com>
 <65e62440.170a0220.f6a7c.6f7f@mx.google.com>
 <CA+Tgmob=v5We_JUvuEC6oBro4nud6aMPrFG2QB-14B99yzKFMw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="hoZxPH4CaxYzWscb"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+Tgmob=v5We_JUvuEC6oBro4nud6aMPrFG2QB-14B99yzKFMw@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/65e643ec.050a0220.39f33.8bdf%40mx.google.com>
Precedence: bulk


--hoZxPH4CaxYzWscb
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

On Mon, Mar 04, 2024 at 03:50:07PM -0500, Robert Haas wrote:
> I agree that two GUCs here seems to be one more than necessary, but I
> wonder whether we couldn't just say 0 means no exponential backoff and
> any other value is the maximum time. 

Alright, I have changed it so that auth_delay.milliseconds and
auth_delay.max_milliseconds are the only GUCs, their default being 0. If
the latter is 0, the former's value is always taken. If the latter is
non-zero and larger than the former, exponential backoff is applied with
the latter's value as maximum delay.

If the latter is smaller than the former then auth_delay just sets the
delay to the latter, I don't think this is problem or confusing, or
should this be considered a misconfiguration?

> The idea that 0 means unlimited doesn't seem useful in practice. 

Yeah, that was more how it was coded than a real policy decision, so
let's do away with it.

V5 attached.


Michael

--hoZxPH4CaxYzWscb
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
 filename="v5-0001-Add-optional-exponential-backoff-to-auth_delay-co.patch"
Content-Transfer-Encoding: 8bit

From 3563d77061480b7e022255b968a39086b0cc8814 Mon Sep 17 00:00:00 2001
From: Michael Banck <michael.banck@credativ.de>
Date: Wed, 27 Dec 2023 15:55:39 +0100
Subject: [PATCH v5] Add optional exponential backoff to auth_delay contrib
 module.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This adds the new auth_delay.max_milliseconds GUC. If set (its default is 0),
auth_delay adds exponential backoff with this GUC's value as maximum delay.

The exponential backoff is tracked per remote host and doubled for every failed
login attempt (i.e., wrong password, not just missing pg_hba line or database)
and reset to auth_delay.milliseconds after a successful authentication or when
no authentication attempts have been made for 5*max_milliseconds from that
host.

Authors: Michael Banck, based on an earlier patch by 成之焕
Reviewed-by: Abhijit Menon-Sen, Tomas Vondra
Discussion: https://postgr.es/m/AHwAxACqIwIVOEhs5YejpqoG.1.1668569845751.Hmail.zhcheng@ceresdata.com
---
 contrib/auth_delay/auth_delay.c  | 216 ++++++++++++++++++++++++++++++-
 doc/src/sgml/auth-delay.sgml     |  31 ++++-
 src/tools/pgindent/typedefs.list |   1 +
 3 files changed, 244 insertions(+), 4 deletions(-)

diff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index ff0e1fd461..5fb123d133 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -14,24 +14,50 @@
 #include <limits.h>
 
 #include "libpq/auth.h"
+#include "miscadmin.h"
 #include "port.h"
+#include "storage/dsm_registry.h"
+#include "storage/ipc.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
 #include "utils/guc.h"
 #include "utils/timestamp.h"
 
 PG_MODULE_MAGIC;
 
+#define MAX_CONN_RECORDS 100
+
 /* GUC Variables */
 static int	auth_delay_milliseconds = 0;
+static int	auth_delay_max_milliseconds = 0;
 
 /* Original Hook */
 static ClientAuthentication_hook_type original_client_auth_hook = NULL;
 
+typedef struct AuthConnRecord
+{
+	char		remote_host[NI_MAXHOST];
+	double		sleep_time;		/* in milliseconds */
+	TimestampTz last_failed_auth;
+} AuthConnRecord;
+
+static shmem_startup_hook_type shmem_startup_next = NULL;
+static AuthConnRecord *acr_array = NULL;
+
+static AuthConnRecord *auth_delay_find_acr_for_host(char *remote_host);
+static AuthConnRecord *auth_delay_find_free_acr(void);
+static double auth_delay_increase_delay_after_failed_conn_auth(Port *port);
+static void auth_delay_cleanup_conn_record(Port *port);
+static void auth_delay_expire_conn_records(Port *port);
+
 /*
  * Check authentication
  */
 static void
 auth_delay_checks(Port *port, int status)
 {
+	double		delay = auth_delay_milliseconds;
+
 	/*
 	 * Any other plugins which use ClientAuthentication_hook.
 	 */
@@ -39,20 +65,190 @@ auth_delay_checks(Port *port, int status)
 		original_client_auth_hook(port, status);
 
 	/*
-	 * Inject a short delay if authentication failed.
+	 * We handle both STATUS_ERROR and STATUS_OK - the third option
+	 * (STATUS_EOF) is disregarded.
+	 *
+	 * In case of STATUS_ERROR we inject a short delay, optionally with
+	 * exponential backoff.
+	 */
+	if (status == STATUS_ERROR)
+	{
+		if (auth_delay_max_milliseconds > 0)
+		{
+			/*
+			 * Delay by 2^n seconds after each authentication failure from a
+			 * particular host, where n is the number of consecutive
+			 * authentication failures.
+			 */
+			delay = auth_delay_increase_delay_after_failed_conn_auth(port);
+
+			/*
+			 * Clamp delay to a maximum of auth_delay_max_milliseconds.
+			 */
+			delay = Min(delay, auth_delay_max_milliseconds);
+		}
+
+		if (delay > 0)
+		{
+			elog(DEBUG1, "Authentication delayed for %g seconds due to auth_delay", delay / 1000.0);
+			pg_usleep(1000L * (long) delay);
+		}
+
+		/*
+		 * Expire delays from other hosts after auth_delay_max_milliseconds *
+		 * 5.
+		 */
+		auth_delay_expire_conn_records(port);
+	}
+
+	/*
+	 * Remove host-specific delay if authentication succeeded.
+	 */
+	if (status == STATUS_OK)
+		auth_delay_cleanup_conn_record(port);
+}
+
+static double
+auth_delay_increase_delay_after_failed_conn_auth(Port *port)
+{
+	AuthConnRecord *acr = NULL;
+
+	acr = auth_delay_find_acr_for_host(port->remote_host);
+
+	if (!acr)
+	{
+		acr = auth_delay_find_free_acr();
+
+		if (!acr)
+		{
+			/*
+			 * No free space, MAX_CONN_RECORDS reached. Wait for the
+			 * configured maximum amount.
+			 */
+			elog(LOG, "auth_delay: host connection list full, waiting maximum amount");
+			return auth_delay_max_milliseconds;
+		}
+		strcpy(acr->remote_host, port->remote_host);
+	}
+	if (acr->sleep_time == 0)
+		acr->sleep_time = (double) auth_delay_milliseconds;
+	else
+		acr->sleep_time *= 2;
+
+	/*
+	 * Set current timestamp for later expiry.
 	 */
-	if (status != STATUS_OK)
+	acr->last_failed_auth = GetCurrentTimestamp();
+
+	return acr->sleep_time;
+}
+
+static AuthConnRecord *
+auth_delay_find_acr_for_host(char *remote_host)
+{
+	int			i;
+
+	for (i = 0; i < MAX_CONN_RECORDS; i++)
+	{
+		if (strcmp(acr_array[i].remote_host, remote_host) == 0)
+			return &acr_array[i];
+	}
+
+	return NULL;
+}
+
+static AuthConnRecord *
+auth_delay_find_free_acr(void)
+{
+	int			i;
+
+	for (i = 0; i < MAX_CONN_RECORDS; i++)
+	{
+		if (!acr_array[i].remote_host[0])
+			return &acr_array[i];
+	}
+
+	return 0;
+}
+
+static void
+auth_delay_cleanup_conn_record(Port *port)
+{
+	AuthConnRecord *acr = NULL;
+
+	acr = auth_delay_find_acr_for_host(port->remote_host);
+	if (acr == NULL)
+		return;
+
+	port->remote_host[0] = '\0';
+
+	acr->sleep_time = 0.0;
+	acr->last_failed_auth = 0.0;
+}
+
+static void
+auth_delay_expire_conn_records(Port *port)
+{
+	int			i;
+	TimestampTz now = GetCurrentTimestamp();
+
+	for (i = 0; i < MAX_CONN_RECORDS; i++)
 	{
-		pg_usleep(1000L * auth_delay_milliseconds);
+		/*
+		 * Do not expire the host from which the current authentication
+		 * failure originated.
+		 */
+		if (strcmp(acr_array[i].remote_host, port->remote_host) == 0)
+			continue;
+
+		if (acr_array[i].last_failed_auth > 0 && (long) ((now - acr_array[i].last_failed_auth) / 1000) > 5 * auth_delay_max_milliseconds)
+		{
+			acr_array[i].remote_host[0] = '\0';
+			acr_array[i].sleep_time = 0.0;
+			acr_array[i].last_failed_auth = 0.0;
+		}
 	}
 }
 
+/*
+ * Set up shared memory
+ */
+
+static void
+auth_delay_init_state(void *ptr)
+{
+	Size		shm_size;
+	AuthConnRecord *array = (AuthConnRecord *) ptr;
+
+	shm_size = sizeof(AuthConnRecord) * MAX_CONN_RECORDS;
+
+	memset(array, 0, shm_size);
+}
+
+static void
+auth_delay_shmem_startup(void)
+{
+	bool		found;
+	Size		shm_size;
+
+	if (shmem_startup_next)
+		shmem_startup_next();
+
+	shm_size = sizeof(AuthConnRecord) * MAX_CONN_RECORDS;
+	acr_array = GetNamedDSMSegment("auth_delay", shm_size, auth_delay_init_state, &found);
+}
+
 /*
  * Module Load Callback
  */
 void
 _PG_init(void)
 {
+	if (!process_shared_preload_libraries_in_progress)
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("auth_delay must be loaded via shared_preload_libraries")));
+
 	/* Define custom GUC variables */
 	DefineCustomIntVariable("auth_delay.milliseconds",
 							"Milliseconds to delay before reporting authentication failure",
@@ -66,9 +262,23 @@ _PG_init(void)
 							NULL,
 							NULL);
 
+	DefineCustomIntVariable("auth_delay.max_milliseconds",
+							"Maximum delay for exponential backoff",
+							NULL,
+							&auth_delay_max_milliseconds,
+							0,
+							0, INT_MAX / 1000,
+							PGC_SIGHUP,
+							GUC_UNIT_MS,
+							NULL, NULL, NULL);
+
 	MarkGUCPrefixReserved("auth_delay");
 
 	/* Install Hooks */
 	original_client_auth_hook = ClientAuthentication_hook;
 	ClientAuthentication_hook = auth_delay_checks;
+
+	/* Set up shared memory */
+	shmem_startup_next = shmem_startup_hook;
+	shmem_startup_hook = auth_delay_shmem_startup;
 }
diff --git a/doc/src/sgml/auth-delay.sgml b/doc/src/sgml/auth-delay.sgml
index 0571f2a99d..e3c182cd45 100644
--- a/doc/src/sgml/auth-delay.sgml
+++ b/doc/src/sgml/auth-delay.sgml
@@ -16,6 +16,19 @@
   connection slots.
  </para>
 
+ <para>
+  It is optionally possible to let <filename>auth_delay</filename> wait longer
+  on each successive authentication failure if the configuration parameter
+  <varname>auth_delay.max_milliseconds</varname> is set.  In this case,
+  <filename>auth_delay</filename> will start with a delay of
+  <varname>auth_delay.milliseconds</varname> and double the delay after each
+  consecutive authentication failure from a particular host, up to the given
+  <varname>auth_delay.max_milliseconds</varname>. If the host authenticates
+  successfully or after a timeout of five times
+  <varname>auth_delay.max_milliseconds</varname>, the delay is reset to
+  <varname>auth_delay.milliseconds</varname>.
+ </para>
+
  <para>
   In order to function, this module must be loaded via
   <xref linkend="guc-shared-preload-libraries"/> in <filename>postgresql.conf</filename>.
@@ -39,6 +52,21 @@
      </para>
     </listitem>
    </varlistentry>
+   <varlistentry>
+    <term>
+     <varname>auth_delay.max_milliseconds</varname> (<type>integer</type>)
+     <indexterm>
+      <primary><varname>auth_delay.max_milliseconds</varname> configuration parameter</primary>
+     </indexterm>
+    </term>
+    <listitem>
+     <para>
+      The maximum delay in milliseconds, implying exponential backoff between
+      each successive failed attempt.  The default is 0, meaning exponential
+      backoff is not active.
+     </para>
+    </listitem>
+   </varlistentry>
   </variablelist>
 
   <para>
@@ -50,7 +78,8 @@
 # postgresql.conf
 shared_preload_libraries = 'auth_delay'
 
-auth_delay.milliseconds = '500'
+auth_delay.milliseconds = '125'
+auth_delay.max_milliseconds = '20000'
 </programlisting>
  </sect2>
 
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 95ae7845d8..dd9fb9e530 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -166,6 +166,7 @@ AttrMap
 AttrMissing
 AttrNumber
 AttributeOpts
+AuthConnRecord
 AuthRequest
 AuthToken
 AutoPrewarmSharedState
-- 
2.39.2


--hoZxPH4CaxYzWscb--