Peter Eisentraut <pete...@gmx.net> writes:
> On 10/16/14 11:34 PM, Craig Ringer wrote:
>> psql: FATAL: Peer authentication failed for user "fred"
>> HINT: See the server error log for additional information.
>
> I think this is wrong for many reasons.
>
> I have never seen an authentication system that responds with, hey, what
> you just did didn't get you in, but the administrators are currently in
> the process of making a configuration change, so why don't you check
> that out.
>
> We don't know whether the user has access to the server log. They
> probably don't. Also, it is vastly more likely that the user really
> doesn't have access in the way they chose, so throwing in irrelevant
> hints will be distracting.
>
> Moreover, it will be confusing to regular users if this message
> sometimes shows up and sometimes doesn't, independent of their own state
> and actions.
>
> Finally, the fact that a configuration change is in progress is
> privileged information. Unprivileged users can deduct from the presence
> of this message that administrators are doing something, and possibly
> that they have done something wrong.
>
> I think it's fine to log a message in the server log if the pg_hba.conf
> file needs reloading. But the client shouldn't know about this at all.
These are all valid concerns IMHO.
Attached is the modified version of the original patch by Craig,
addressing the handling of the new hint_log error data field and
removing the client-side HINT.
I'm also moving this to the current CF.
--
Alex
>From 702e0ac31f3d8023ad8c064d90bdf5a8441fddea 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 | 59 ++++++++++++++++++++++++++++++++----------
src/include/utils/elog.h | 7 +++++
2 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
new file mode 100644
index 2316464..da55207
*** a/src/backend/utils/error/elog.c
--- b/src/backend/utils/error/elog.c
*************** errfinish(int dummy,...)
*** 503,508 ****
--- 503,510 ----
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)
*************** errhint(const char *fmt,...)
*** 1015,1020 ****
--- 1017,1042 ----
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
*************** CopyErrorData(void)
*** 1498,1503 ****
--- 1520,1527 ----
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)
*************** FreeErrorData(ErrorData *edata)
*** 1536,1541 ****
--- 1560,1567 ----
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)
*************** ThrowErrorData(ErrorData *edata)
*** 1607,1612 ****
--- 1633,1640 ----
newedata->detail_log = pstrdup(edata->detail_log);
if (edata->hint)
newedata->hint = pstrdup(edata->hint);
+ if (edata->hint_log)
+ newedata->hint_log = pstrdup(edata->hint_log);
if (edata->context)
newedata->context = pstrdup(edata->context);
if (edata->schema_name)
*************** ReThrowError(ErrorData *edata)
*** 1669,1674 ****
--- 1697,1704 ----
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)
*************** write_csvlog(ErrorData *edata)
*** 2710,2717 ****
appendCSVLiteral(&buf, edata->detail);
appendStringInfoChar(&buf, ',');
! /* errhint */
! appendCSVLiteral(&buf, edata->hint);
appendStringInfoChar(&buf, ',');
/* internal query */
--- 2740,2750 ----
appendCSVLiteral(&buf, edata->detail);
appendStringInfoChar(&buf, ',');
! /* errhint or errhint_log */
! if (edata->hint_log)
! appendCSVLiteral(&buf, edata->hint_log);
! else
! appendCSVLiteral(&buf, edata->hint);
appendStringInfoChar(&buf, ',');
/* internal query */
*************** send_message_to_server_log(ErrorData *ed
*** 2828,2852 ****
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)
{
log_line_prefix(&buf, edata);
appendStringInfoString(&buf, _("DETAIL: "));
! append_with_tabs(&buf, edata->detail);
appendStringInfoChar(&buf, '\n');
}
! if (edata->hint)
{
log_line_prefix(&buf, edata);
appendStringInfoString(&buf, _("HINT: "));
! append_with_tabs(&buf, edata->hint);
appendStringInfoChar(&buf, '\n');
}
if (edata->internalquery)
--- 2861,2883 ----
if (Log_error_verbosity >= PGERROR_DEFAULT)
{
! const char *detail = edata->detail_log != NULL
! ? edata->detail_log : edata->detail;
! const char *hint = edata->hint_log != NULL
! ? edata->hint_log : edata->hint;
!
! if (detail)
{
log_line_prefix(&buf, edata);
appendStringInfoString(&buf, _("DETAIL: "));
! append_with_tabs(&buf, detail);
appendStringInfoChar(&buf, '\n');
}
! if (hint)
{
log_line_prefix(&buf, edata);
appendStringInfoString(&buf, _("HINT: "));
! append_with_tabs(&buf, hint);
appendStringInfoChar(&buf, '\n');
}
if (edata->internalquery)
*************** send_message_to_frontend(ErrorData *edat
*** 3130,3135 ****
--- 3161,3168 ----
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
new file mode 100644
index 87438b8..7368fe1
*** a/src/include/utils/elog.h
--- b/src/include/utils/elog.h
*************** errhint(const char *fmt,...)
*** 203,208 ****
--- 203,214 ----
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
*************** typedef struct ErrorData
*** 395,400 ****
--- 401,407 ----
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 */
--
2.1.0
>From 5aac96df52a8d006f70c4fa3f799fc2239ccb57b 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 | 70 +++++++++++++++++++++++++++++++++++++++++++-----
src/backend/libpq/hba.c | 8 ++++--
src/include/libpq/auth.h | 2 ++
3 files changed, 71 insertions(+), 9 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 9ad99ce..b38b3d6
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***************
*** 16,24 ****
--- 16,26 ----
#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,35 ****
--- 32,39 ----
#include "miscadmin.h"
#include "replication/walsender.h"
#include "storage/ipc.h"
+ #include "utils/guc.h"
+ #include "utils/timestamp.h"
/*----------------------------------------------------------------
*************** static void auth_failed(Port *port, int
*** 41,46 ****
--- 45,52 ----
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
*************** auth_failed(Port *port, int status, char
*** 282,293 ****
ereport(FATAL,
(errcode(errcode_return),
errmsg(errstr, port->user_name),
! logdetail ? errdetail_log("%s", logdetail) : 0));
/* doesn't return */
}
-
/*
* Client authentication starts here. If there is an error, this
* function does not return and the backend process is terminated.
--- 288,299 ----
ereport(FATAL,
(errcode(errcode_return),
errmsg(errstr, port->user_name),
! 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.
*************** ClientAuthentication(Port *port)
*** 334,340 ****
{
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
! errmsg("connection requires a valid client certificate")));
}
#else
--- 340,347 ----
{
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
! errmsg("connection requires a valid client certificate"),
! errhint_if_hba_conf_stale()));
}
#else
*************** ClientAuthentication(Port *port)
*** 378,389 ****
(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"))));
#else
ereport(FATAL,
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg("pg_hba.conf rejects replication connection for host \"%s\", user \"%s\"",
! hostinfo, port->user_name)));
#endif
}
else
--- 385,398 ----
(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")),
! 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),
! errhint_if_hba_conf_stale()));
#endif
}
else
*************** ClientAuthentication(Port *port)
*** 394,406 ****
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"))));
#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)));
#endif
}
break;
--- 403,417 ----
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")),
! 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),
! errhint_if_hba_conf_stale()));
#endif
}
break;
*************** ClientAuthentication(Port *port)
*** 453,464 ****
--- 464,477 ----
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
}
*************** ClientAuthentication(Port *port)
*** 471,476 ****
--- 484,490 ----
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,
*************** ClientAuthentication(Port *port)
*** 478,483 ****
--- 492,498 ----
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
}
*************** recv_password_packet(Port *port)
*** 684,689 ****
--- 699,745 ----
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)
+ {
+ /*
+ * We intentionally only log into the server's log, and not
+ * leaking this to the client.
+ */
+ 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
+ * 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
new file mode 100644
index d43c8ff..c5b5d3d
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***************
*** 28,39 ****
--- 28,41 ----
#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"
*************** check_usermap(const char *usermap_name,
*** 2098,2104 ****
}
ereport(LOG,
(errmsg("provided user name (%s) and authenticated user name (%s) do not match",
! pg_role, auth_user)));
return STATUS_ERROR;
}
else
--- 2100,2107 ----
}
ereport(LOG,
(errmsg("provided user name (%s) and authenticated user name (%s) do not match",
! pg_role, auth_user),
! errhint_if_ident_conf_stale()));
return STATUS_ERROR;
}
else
*************** check_usermap(const char *usermap_name,
*** 2118,2124 ****
{
ereport(LOG,
(errmsg("no match in usermap \"%s\" for user \"%s\" authenticated as \"%s\"",
! usermap_name, pg_role, auth_user)));
}
return found_entry ? STATUS_OK : STATUS_ERROR;
}
--- 2121,2128 ----
{
ereport(LOG,
(errmsg("no match in usermap \"%s\" for user \"%s\" authenticated as \"%s\"",
! 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
new file mode 100644
index ace647a..cfc6d9c
*** a/src/include/libpq/auth.h
--- b/src/include/libpq/auth.h
*************** extern char *pg_krb_realm;
*** 22,27 ****
--- 22,29 ----
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;
--
2.1.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
