Hi hackers,
While commit 960869da08 added some information about connections that
have been successfully authenticated, there is no metrics for
connections that have not (or did not reached the authentication stage).
Adding metrics about failed connections attempts could also help, for
example with proper sampling, to:
* detect spikes in failed login attempts
* check if there is a correlation between spikes in successful and
failed connection attempts
While the number of successful connections could also already been
tracked with the ClientAuthentication_hook (and also the ones that
failed the authentication) we are missing metrics about:
* why the connection failed (could be bad password, bad database, bad
user, missing CONNECT privilege...)
* number of times the authentication stage has not been reached
* why the authentication stage has not been reached (bad startup
packets, timeout while processing startup packet,...)
Those missing metrics (in addition to the ones that can be already
gathered) could provide value for:
* security investigations
* anomalies detections
* tracking application misconfigurations
In an attempt to be able to provide those metrics, please find attached
a patch proposal to add new hooks in the connection path, that would be
fired if:
* there is a bad startup packet
* there is a timeout while processing the startup packet
* user does not have CONNECT privilege
* database does not exist
For safety those hooks request the use of a const Port parameter, so
that they could be used only for reporting purpose (for example, we are
working on an extension to record detailed login metrics counters).
Another option could be to add those metrics in the engine itself
(instead of providing new hooks to get them), but the new hooks option
gives more flexibility on how to render and exploit them (there is a lot
of information in the Port Struct that one could be interested with).
I’m adding this patch proposal to the commitfest.
Looking forward to your feedback,
Regards,
Bertrand
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index dde4bc25b1..8e00327a5d 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -567,6 +567,9 @@ int postmaster_alive_fds[2] = {-1, -1};
HANDLE PostmasterHandle;
#endif
+StartupPacketTimeout_hook_type StartupPacketTimeout_hook = NULL;
+BadConnPacket_hook_type BadConnPacket_hook = NULL;
+
/*
* Postmaster main entry point
*/
@@ -4462,8 +4465,11 @@ BackendInitialize(Port *port)
* Stop here if it was bad or a cancel packet. ProcessStartupPacket
* already did any appropriate error reporting.
*/
- if (status != STATUS_OK)
+ if (status != STATUS_OK) {
+ if (BadConnPacket_hook)
+ (*BadConnPacket_hook) (port);
proc_exit(0);
+ }
/*
* Now that we have the user and database name, we can set the process
@@ -5323,6 +5329,11 @@ dummy_handler(SIGNAL_ARGS)
static void
StartupPacketTimeoutHandler(void)
{
+ if (StartupPacketTimeout_hook)
+ (*StartupPacketTimeout_hook) (MyProcPort);
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("timeout while processing startup packet")));
_exit(1);
}
diff --git a/src/backend/utils/init/postinit.c
b/src/backend/utils/init/postinit.c
index 6b9082604f..562ed331bf 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -67,6 +67,14 @@
#include "utils/syscache.h"
#include "utils/timeout.h"
+/*
+ * Hooks to be used when a connection has been refused in case of bad
+ * database name, bad database oid or bad permissions.
+ */
+BadDb_hook_type baddbname_hook = NULL;
+BadDb_hook_type baddboid_hook = NULL;
+BadDb_hook_type baddbperm_hook = NULL;
+
static HeapTuple GetDatabaseTuple(const char *dbname);
static HeapTuple GetDatabaseTupleByOid(Oid dboid);
static void PerformAuthentication(Port *port);
@@ -360,10 +368,14 @@ CheckMyDatabase(const char *name, bool am_superuser, bool
override_allow_connect
if (!am_superuser &&
pg_database_aclcheck(MyDatabaseId, GetUserId(),
ACL_CONNECT)
!= ACLCHECK_OK)
+ {
+ if (baddbperm_hook)
+ (*baddbperm_hook) (MyProcPort);
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied for database
\"%s\"", name),
errdetail("User does not have CONNECT
privilege.")));
+ }
/*
* Check connection limit for this database.
@@ -918,9 +930,13 @@ InitPostgres(const char *in_dbname, Oid dboid, const char
*username,
tuple = GetDatabaseTuple(in_dbname);
if (!HeapTupleIsValid(tuple))
+ {
+ if (baddbname_hook)
+ (*baddbname_hook) (MyProcPort);
ereport(FATAL,
(errcode(ERRCODE_UNDEFINED_DATABASE),
errmsg("database \"%s\" does not
exist", in_dbname)));
+ }
dbform = (Form_pg_database) GETSTRUCT(tuple);
MyDatabaseId = dbform->oid;
MyDatabaseTableSpace = dbform->dattablespace;
@@ -935,9 +951,13 @@ InitPostgres(const char *in_dbname, Oid dboid, const char
*username,
tuple = GetDatabaseTupleByOid(dboid);
if (!HeapTupleIsValid(tuple))
+ {
+ if (baddboid_hook)
+ (*baddboid_hook) (MyProcPort);
ereport(FATAL,
(errcode(ERRCODE_UNDEFINED_DATABASE),
errmsg("database %u does not exist",
dboid)));
+ }
dbform = (Form_pg_database) GETSTRUCT(tuple);
MyDatabaseId = dbform->oid;
MyDatabaseTableSpace = dbform->dattablespace;
diff --git a/src/include/postmaster/postmaster.h
b/src/include/postmaster/postmaster.h
index 90e333ccd2..f42c19a4d1 100644
--- a/src/include/postmaster/postmaster.h
+++ b/src/include/postmaster/postmaster.h
@@ -63,6 +63,19 @@ extern Size ShmemBackendArraySize(void);
extern void ShmemBackendArrayAllocation(void);
#endif
+/* kluge to avoid including libpq/libpq-be.h here */
+struct Port;
+typedef void (*BadDb_hook_type) (const struct Port *);
+extern PGDLLIMPORT BadDb_hook_type baddbname_hook;
+extern PGDLLIMPORT BadDb_hook_type baddboid_hook;
+extern PGDLLIMPORT BadDb_hook_type baddbperm_hook;
+
+typedef void (*BadConnPacket_hook_type) (const struct Port *);
+extern PGDLLIMPORT BadConnPacket_hook_type BadConnPacket_hook;
+
+typedef void (*StartupPacketTimeout_hook_type) (const struct Port *);
+extern PGDLLIMPORT StartupPacketTimeout_hook_type StartupPacketTimeout_hook;
+
/*
* Note: MAX_BACKENDS is limited to 2^18-1 because that's the width reserved
* for buffer references in buf_internals.h. This limitation could be lifted
