On 08/10/2014 07:48 PM, Craig Ringer wrote:
> Hi all
>
> I just had an idea I wanted to run by you all before turning it into a
> patch.
>
> People seem to get confused when they get auth errors because they
> changed pg_hba.conf but didn't reload.
>
> Should we emit a HINT alongside the main auth error in that case?
>
> Given the amount of confusion that I see around pg_hba.conf from new
> users, I figure anything that makes it less confusing might be a good
> thing if there aren't other consequences.
>
> Interested in a patch?
Given the generally positive reception to this, here's a patch.
The first patch adds an errhint_log , akin to the current errdetail_log,
so we can send a different HINT to the server log than we do to the client.
(Even if DETAIL was appropriate for this info, which it isn't, I can't
use errdetail_log because it's already used for other information in
some of the same error sites.)
The second patch adds a test during errors to report if pg_hba.conf is
stale, or if pg_ident.conf is stale.
Typical output, client:
psql: FATAL: Peer authentication failed for user "fred"
HINT: See the server error log for additional information.
Typical output, server:
LOG: provided user name (fred) and authenticated user name (craig) do
not match
FATAL: Peer authentication failed for user "fred"
DETAIL: Connection matched pg_hba.conf line 84: "local all
all peer"
HINT: pg_hba.conf has been changed since last server configuration
reload. Reload the server configuration to apply the changes.
I've added this to the next CF.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 8b2bf6ae615d716ca9857017fd862386c6bdcd1f Mon Sep 17 00:00:00 2001
From: Craig Ringer <cr...@2ndquadrant.com>
Date: Fri, 17 Oct 2014 11:18:18 +0800
Subject: [PATCH 1/2] Add an errhint_log, akin to errdetail_log
This allows a different HINT to be sent to the server error log
and to the client, which will be useful where there's security
sensitive information that's more appropriate for a HINT than
a DETAIL message.
---
src/backend/utils/error/elog.c | 54 ++++++++++++++++++++++++++++++++----------
src/include/utils/elog.h | 7 ++++++
2 files changed, 48 insertions(+), 13 deletions(-)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 32a9663..59be8a6 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1015,6 +1015,26 @@ errhint(const char *fmt,...)
return 0; /* return value does not matter */
}
+/*
+ * errhint_log --- add a hint_log error message text to the current error
+ */
+int
+errhint_log(const char *fmt,...)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+ MemoryContext oldcontext;
+
+ recursion_depth++;
+ CHECK_STACK_DEPTH();
+ oldcontext = MemoryContextSwitchTo(edata->assoc_context);
+
+ EVALUATE_MESSAGE(edata->domain, hint_log, false, true);
+
+ MemoryContextSwitchTo(oldcontext);
+ recursion_depth--;
+ return 0; /* return value does not matter */
+}
+
/*
* errcontext_msg --- add a context error message text to the current error
@@ -1498,6 +1518,8 @@ CopyErrorData(void)
newedata->detail_log = pstrdup(newedata->detail_log);
if (newedata->hint)
newedata->hint = pstrdup(newedata->hint);
+ if (newedata->hint_log)
+ newedata->hint_log = pstrdup(newedata->hint_log);
if (newedata->context)
newedata->context = pstrdup(newedata->context);
if (newedata->schema_name)
@@ -1536,6 +1558,8 @@ FreeErrorData(ErrorData *edata)
pfree(edata->detail_log);
if (edata->hint)
pfree(edata->hint);
+ if (edata->hint_log)
+ pfree(edata->hint_log);
if (edata->context)
pfree(edata->context);
if (edata->schema_name)
@@ -1618,6 +1642,8 @@ ReThrowError(ErrorData *edata)
newedata->detail_log = pstrdup(newedata->detail_log);
if (newedata->hint)
newedata->hint = pstrdup(newedata->hint);
+ if (newedata->hint_log)
+ newedata->hint_log = pstrdup(newedata->hint_log);
if (newedata->context)
newedata->context = pstrdup(newedata->context);
if (newedata->schema_name)
@@ -2659,8 +2685,11 @@ write_csvlog(ErrorData *edata)
appendCSVLiteral(&buf, edata->detail);
appendStringInfoChar(&buf, ',');
- /* errhint */
- appendCSVLiteral(&buf, edata->hint);
+ /* errhint or errhint_log */
+ if (edata->hint_log)
+ appendCSVLiteral(&buf, edata->hint_log);
+ else
+ appendCSVLiteral(&buf, edata->hint);
appendStringInfoChar(&buf, ',');
/* internal query */
@@ -2777,25 +2806,22 @@ send_message_to_server_log(ErrorData *edata)
if (Log_error_verbosity >= PGERROR_DEFAULT)
{
- if (edata->detail_log)
- {
- log_line_prefix(&buf, edata);
- appendStringInfoString(&buf, _("DETAIL: "));
- append_with_tabs(&buf, edata->detail_log);
- appendStringInfoChar(&buf, '\n');
- }
- else if (edata->detail)
+ const char * const detail
+ = edata->detail_log != NULL ? edata->detail_log : edata->detail;
+ const char * const hint
+ = edata->hint_log != NULL ? edata->hint_log : edata->hint;
+ if (detail)
{
log_line_prefix(&buf, edata);
appendStringInfoString(&buf, _("DETAIL: "));
- append_with_tabs(&buf, edata->detail);
+ append_with_tabs(&buf, detail);
appendStringInfoChar(&buf, '\n');
}
- if (edata->hint)
+ if (hint)
{
log_line_prefix(&buf, edata);
appendStringInfoString(&buf, _("HINT: "));
- append_with_tabs(&buf, edata->hint);
+ append_with_tabs(&buf, hint);
appendStringInfoChar(&buf, '\n');
}
if (edata->internalquery)
@@ -3079,6 +3105,8 @@ send_message_to_frontend(ErrorData *edata)
err_sendstring(&msgbuf, edata->hint);
}
+ /* hint_log is intentionally not used here */
+
if (edata->context)
{
pq_sendbyte(&msgbuf, PG_DIAG_CONTEXT);
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 92073be..3828c93 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -203,6 +203,12 @@ errhint(const char *fmt,...)
the supplied arguments. */
__attribute__((format(PG_PRINTF_ATTRIBUTE, 1, 2)));
+extern int
+errhint_log(const char *fmt,...)
+/* This extension allows gcc to check the format string for consistency with
+ the supplied arguments. */
+__attribute__((format(PG_PRINTF_ATTRIBUTE, 1, 2)));
+
/*
* errcontext() is typically called in error context callback functions, not
* within an ereport() invocation. The callback function can be in a different
@@ -395,6 +401,7 @@ typedef struct ErrorData
char *detail; /* detail error message */
char *detail_log; /* detail error message for server log only */
char *hint; /* hint message */
+ char *hint_log; /* hint message for server log only */
char *context; /* context message */
char *schema_name; /* name of schema */
char *table_name; /* name of table */
--
1.9.3
>From 19a955cb4c9f0caeb99ff4de71d5a5e344932f53 Mon Sep 17 00:00:00 2001
From: Craig Ringer <cr...@2ndquadrant.com>
Date: Fri, 17 Oct 2014 10:15:40 +0800
Subject: [PATCH 2/2] Log a hint if pg_ident.conf or pg_hba.conf changed since
last reload
Users often get tripped up by the fact that you have to reload the
server config to have changes to pg_hba.conf take effect. To help
them out, we now emit a HINT message when authentication fails and
pg_hba.conf or pg_ident.conf are stale, telling them that they
should check the server error log for details.
In the server error log we report that pg_hba.conf or pg_ident.conf
have changed since the last time the server configuration was
reloaded, and that they should reload the config.
No attempt is made to determine whether the change to the config files is
relevant. This is done purely based on timestamp comparisons. If the change
isn't related to the connection issue they're having then at worst they'll
reload the config file and get the same error sans the HINT.
---
src/backend/libpq/auth.c | 68 +++++++++++++++++++++++++++++++++++++++++++-----
src/backend/libpq/hba.c | 8 ++++--
src/include/libpq/auth.h | 2 ++
3 files changed, 69 insertions(+), 9 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index b1974d1..8f682e2 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -16,9 +16,11 @@
#include "postgres.h"
#include <sys/param.h>
+#include <sys/stat.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
+#include <time.h>
#include <unistd.h>
#include "libpq/auth.h"
@@ -30,6 +32,8 @@
#include "miscadmin.h"
#include "replication/walsender.h"
#include "storage/ipc.h"
+#include "utils/guc.h"
+#include "utils/timestamp.h"
/*----------------------------------------------------------------
@@ -41,6 +45,8 @@ static void auth_failed(Port *port, int status, char *logdetail);
static char *recv_password_packet(Port *port);
static int recv_and_check_password_packet(Port *port, char **logdetail);
+static int errhint_if_hba_conf_stale(void);
+
/*----------------------------------------------------------------
* Ident authentication
@@ -282,12 +288,12 @@ auth_failed(Port *port, int status, char *logdetail)
ereport(FATAL,
(errcode(errcode_return),
errmsg(errstr, port->user_name),
- logdetail ? errdetail_log("%s", logdetail) : 0));
+ logdetail ? errdetail_log("%s", logdetail) : 0,
+ errhint_if_hba_conf_stale()));
/* doesn't return */
}
-
/*
* Client authentication starts here. If there is an error, this
* function does not return and the backend process is terminated.
@@ -334,7 +340,8 @@ ClientAuthentication(Port *port)
{
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
- errmsg("connection requires a valid client certificate")));
+ errmsg("connection requires a valid client certificate"),
+ errhint_if_hba_conf_stale()));
}
#else
@@ -378,12 +385,14 @@ ClientAuthentication(Port *port)
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg("pg_hba.conf rejects replication connection for host \"%s\", user \"%s\", %s",
hostinfo, port->user_name,
- port->ssl_in_use ? _("SSL on") : _("SSL off"))));
+ port->ssl_in_use ? _("SSL on") : _("SSL off")),
+ errhint_if_hba_conf_stale()));
#else
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg("pg_hba.conf rejects replication connection for host \"%s\", user \"%s\"",
- hostinfo, port->user_name)));
+ hostinfo, port->user_name),
+ errhint_if_hba_conf_stale()));
#endif
}
else
@@ -394,13 +403,15 @@ ClientAuthentication(Port *port)
errmsg("pg_hba.conf rejects connection for host \"%s\", user \"%s\", database \"%s\", %s",
hostinfo, port->user_name,
port->database_name,
- port->ssl_in_use ? _("SSL on") : _("SSL off"))));
+ port->ssl_in_use ? _("SSL on") : _("SSL off")),
+ errhint_if_hba_conf_stale()));
#else
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg("pg_hba.conf rejects connection for host \"%s\", user \"%s\", database \"%s\"",
hostinfo, port->user_name,
- port->database_name)));
+ port->database_name),
+ errhint_if_hba_conf_stale()));
#endif
}
break;
@@ -453,12 +464,14 @@ ClientAuthentication(Port *port)
errmsg("no pg_hba.conf entry for replication connection from host \"%s\", user \"%s\", %s",
hostinfo, port->user_name,
port->ssl_in_use ? _("SSL on") : _("SSL off")),
+ errhint_if_hba_conf_stale(),
HOSTNAME_LOOKUP_DETAIL(port)));
#else
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg("no pg_hba.conf entry for replication connection from host \"%s\", user \"%s\"",
hostinfo, port->user_name),
+ errhint_if_hba_conf_stale(),
HOSTNAME_LOOKUP_DETAIL(port)));
#endif
}
@@ -471,6 +484,7 @@ ClientAuthentication(Port *port)
hostinfo, port->user_name,
port->database_name,
port->ssl_in_use ? _("SSL on") : _("SSL off")),
+ errhint_if_hba_conf_stale(),
HOSTNAME_LOOKUP_DETAIL(port)));
#else
ereport(FATAL,
@@ -478,6 +492,7 @@ ClientAuthentication(Port *port)
errmsg("no pg_hba.conf entry for host \"%s\", user \"%s\", database \"%s\"",
hostinfo, port->user_name,
port->database_name),
+ errhint_if_hba_conf_stale(),
HOSTNAME_LOOKUP_DETAIL(port)));
#endif
}
@@ -684,6 +699,45 @@ recv_password_packet(Port *port)
return buf.data;
}
+/* See errhint_if_hba_conf_stale */
+static int
+errhint_if_cfg_stale(const char * cfgpath, const char * cfgfile)
+{
+ struct stat statbuf;
+
+ if (stat(cfgpath, &statbuf) == 0)
+ {
+ if (difftime(statbuf.st_mtime, timestamptz_to_time_t(PgReloadTime)) > 0)
+ {
+ errhint("See the server error log for additional information.");
+ return errhint_log("%s has been changed since last server configuration reload. Reload the server configuration to apply the changes.",
+ cfgfile);
+ }
+ }
+ return 0;
+}
+
+/*
+ * If pg_hba.conf has been modified since the last reload, emit a HINT
+ * to the client suggesting that they check the server error log, and
+ * a HINT to the server error log informing the user that pg_hba.conf
+ * has changed since the last reload.
+ */
+static int
+errhint_if_hba_conf_stale()
+{
+ return errhint_if_cfg_stale(HbaFileName, "pg_hba.conf");
+}
+
+/*
+ * Like errhint_if_hba_conf_stale but for pg_ident.conf
+ */
+int
+errhint_if_ident_conf_stale()
+{
+ return errhint_if_cfg_stale(IdentFileName, "pg_ident.conf");
+}
+
/*----------------------------------------------------------------
* MD5 authentication
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 84da823..3265a85 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -28,12 +28,14 @@
#include "catalog/pg_collation.h"
#include "libpq/ip.h"
#include "libpq/libpq.h"
+#include "libpq/auth.h"
#include "postmaster/postmaster.h"
#include "regex/regex.h"
#include "replication/walsender.h"
#include "storage/fd.h"
#include "utils/acl.h"
#include "utils/guc.h"
+#include "utils/timestamp.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -2106,7 +2108,8 @@ check_usermap(const char *usermap_name,
}
ereport(LOG,
(errmsg("provided user name (%s) and authenticated user name (%s) do not match",
- pg_role, auth_user)));
+ pg_role, auth_user),
+ errhint_if_ident_conf_stale()));
return STATUS_ERROR;
}
else
@@ -2126,7 +2129,8 @@ check_usermap(const char *usermap_name,
{
ereport(LOG,
(errmsg("no match in usermap \"%s\" for user \"%s\" authenticated as \"%s\"",
- usermap_name, pg_role, auth_user)));
+ usermap_name, pg_role, auth_user),
+ errhint_if_ident_conf_stale()));
}
return found_entry ? STATUS_OK : STATUS_ERROR;
}
diff --git a/src/include/libpq/auth.h b/src/include/libpq/auth.h
index ace647a..cfc6d9c 100644
--- a/src/include/libpq/auth.h
+++ b/src/include/libpq/auth.h
@@ -22,6 +22,8 @@ extern char *pg_krb_realm;
extern void ClientAuthentication(Port *port);
+extern int errhint_if_ident_conf_stale(void);
+
/* Hook for plugins to get control in ClientAuthentication() */
typedef void (*ClientAuthentication_hook_type) (Port *, int);
extern PGDLLIMPORT ClientAuthentication_hook_type ClientAuthentication_hook;
--
1.9.3
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
