HINT: pg_hba.conf changed since last config reload
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?
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
On 2014-08-10 19:48:29 +0800, Craig Ringer wrote:
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.
I think we could/would only emit that to the server log because of
security concerns. It very well might be interesting for an attacker to
know that an outdated hba.conf is still being used... Would that still
provide enough benefits?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Andres Freund (andres@2ndquadrant.com) wrote:
On 2014-08-10 19:48:29 +0800, Craig Ringer wrote:
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.I think we could/would only emit that to the server log because of
security concerns. It very well might be interesting for an attacker to
know that an outdated hba.conf is still being used... Would that still
provide enough benefits?
I'd think that'd be useful even if it's only in the main log.
To Craig's point on addressing user confusion (when the user is really
an admin trying to work through changes), a HINT along the lines of
"this incident has been logged with details to the PostgreSQL log file"
or something might help. It amazes me how often just telling people to
go *look at the server log file* helps...
Thanks,
Stephen
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
Attachments:
0001-Add-an-errhint_log-akin-to-errdetail_log.patchtext/x-patch; name=0001-Add-an-errhint_log-akin-to-errdetail_log.patchDownload
>From 8b2bf6ae615d716ca9857017fd862386c6bdcd1f Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@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
0002-Log-a-hint-if-pg_ident.conf-or-pg_hba.conf-changed-s.patchtext/x-patch; name=0002-Log-a-hint-if-pg_ident.conf-or-pg_hba.conf-changed-s.patchDownload
>From 19a955cb4c9f0caeb99ff4de71d5a5e344932f53 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@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
On 10/16/2014 11:34 PM, Craig Ringer wrote:
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.
The patch behaves as you describe. I feel that this feature would be
useful , and you implemented the suggestions given that requested the
reload notice but be sent to the client but instead just a hint about
checking the server log.
You follow the pattern set with detail_log which makes sense. The
variable name "hint_log" doesn't make it obvious to me that
the hint goes to the server log, but not the client. The comment for
errhint_log should maybe explicitly say that.
One question about the code:
Does errfinish (elog.c at around line 505) need to free hint_log ? (I
would assume it does)
Other than that the patch looks good to me.
---------
Something else I noticed while testing. This isn't introduced by your
patch but I am wondering if it an existing bug if I setup my
configuration like this:
#data_directory = 'ConfigDir' # use data in another directory
# (change requires restart)
hba_file = 'ConfigDir/pg_hba2.conf' # host-based authentication file
and start postgres like
./postgres -D ../data
it looks for pg2hba2.conf at bin/ConfigDir/pg_hba2.conf (relative to
the bin directory I started it from)
Then if I change my pg_hba.conf and do a reload I get the following in
the log
LOG: parameter "hba_file" cannot be changed without restarting the server
LOG: configuration file
"/usr/local/pgsql95git/bin/../data/postgresql.conf" contains errors;
unaffected changes were applied
set_config_option is comparing the relative path with the absolute path.
Steve
(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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 6, 2014 at 5:46 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
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.
I agree.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 6, 2014 at 05:46:42PM -0500, Peter Eisentraut wrote:
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.
Should we do this for postgresql.conf too?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Nov 27, 2014 at 8:49 AM, Bruce Momjian <bruce@momjian.us> wrote:
On Thu, Nov 6, 2014 at 05:46:42PM -0500, Peter Eisentraut wrote:
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.Should we do this for postgresql.conf too?
It doesn't really make sense; or at least, the exact same thing
doesn't make any sense. If an authentication attempt fails
unexpectedly, it might be because of a pg_hba.conf change that wasn't
reloaded, so it makes sense to try to tip off the DBA. But it can't
really be because of a pending postgresql.conf change that hasn't been
reloaded, because those don't generally affect authentication.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Peter Eisentraut <peter_e@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
Attachments:
0001-Add-an-errhint_log-akin-to-errdetail_log-v2.patchtext/x-diffDownload
>From 702e0ac31f3d8023ad8c064d90bdf5a8441fddea Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@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
0002-Log-a-hint-if-pg_ident.conf-or-pg_hba.conf-changed-v2.patchtext/x-diffDownload
>From 5aac96df52a8d006f70c4fa3f799fc2239ccb57b Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@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
On 12/15/2014 11:38 AM, Alex Shulgin wrote:
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
The updated patch removes the client message. I feel this addresses
Peter's concern. The updated patch also addresses the missing free I
mentioned in my original review.
The patch applies cleanly to head,
One thing I'm noticed while testing is that if you do the following
1. Edit pg_hba.conf to allow a connection from somewhere
2. Attempt to connect, you get an error + the hind in the server log
3. Make another change to pg_hba.conf and introduce an error in the file
4. Attempt to reload the config files, pg_hba.conf the reload fails
because of the above error
5. Attempt to connect you still can't connect since we have the original
pg_hba.conf loaded
You don't get the warning in step 5 since we update PgReloadTime even if
the reload didn't work.
Is this enough of a concern to justify the extra complexity in tracking
the reload time of the pg_hba and pg_ident times directly?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Steve Singer <steve@ssinger.info> writes:
On 12/15/2014 11:38 AM, Alex Shulgin wrote:
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. -- AlexThe updated patch removes the client message. I feel this addresses
Peter's concern. The updated patch also addresses the missing free I
mentioned in my original review.The patch applies cleanly to head,
One thing I'm noticed while testing is that if you do the following
1. Edit pg_hba.conf to allow a connection from somewhere
2. Attempt to connect, you get an error + the hind in the server log
3. Make another change to pg_hba.conf and introduce an error in the file
4. Attempt to reload the config files, pg_hba.conf the reload fails
because of the above error
5. Attempt to connect you still can't connect since we have the
original pg_hba.conf loadedYou don't get the warning in step 5 since we update PgReloadTime even
if the reload didn't work.Is this enough of a concern to justify the extra complexity in
tracking the reload time of the pg_hba and pg_ident times directly?
I don't think so. The scenario this patch relies on assumes that the
DBA will remember to look in the log if something goes wrong, and in
your case there would be a message like the following:
WARNING: pg_hba.conf not reloaded
So an extra hint about file timestamp is unneeded.
--
Alex
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/19/2014 11:41 PM, Alex Shulgin wrote:
I don't think so. The scenario this patch relies on assumes that the
DBA will remember to look in the log if something goes wrong
Well, actually, the whole point was that the user who's connecting
(likely also the "DBA") will see a HINT telling them that there's more
in the logs.
But that got removed.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Craig Ringer <craig@2ndquadrant.com> writes:
On 12/19/2014 11:41 PM, Alex Shulgin wrote:
I don't think so. The scenario this patch relies on assumes that the
DBA will remember to look in the log if something goes wrongWell, actually, the whole point was that the user who's connecting
(likely also the "DBA") will see a HINT telling them that there's more
in the logs.But that got removed.
While it sounds useful at first glance, I believe Peter's arguments
upthread provide enough justification to avoid sending the hint to the
client.
--
Alex
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/19/2014 10:41 AM, Alex Shulgin wrote:
I don't think so. The scenario this patch relies on assumes that the
DBA will remember to look in the log if something goes wrong, and in
your case there would be a message like the following:WARNING: pg_hba.conf not reloaded
So an extra hint about file timestamp is unneeded.
That makes sense to me.
I haven't found any new issues with this patch.
I think it is ready for a committer.
--
Alex
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2014-12-15 19:38:16 +0300, Alex Shulgin wrote:
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 not a big fan of this implementation. We're adding a fair bit of
infrastructure (i.e. server-only hints) where in other places we use
NOTICE for similar hints.
Why don't we just add emit a NOTICE or WARNING in the relevant place
saying that pg_hba.conf is outdated? Then the server won't log those if
configured appropriately, which doesn't seem like a bad thing. Note that
<= ERROR messages aren't sent to the client during authentication.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-01-16 18:01:24 +0100, Andres Freund wrote:
Why don't we just add emit a NOTICE or WARNING in the relevant place
saying that pg_hba.conf is outdated? Then the server won't log those if
configured appropriately, which doesn't seem like a bad thing. Note that
<= ERROR messages aren't sent to the client during authentication.
I think a 'WARNING: client authentication failed/succeeded with a
outdated pg_hba.conf in effect' would actually be a good way to present
this. It's not only failed logins where this is relevant.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
Why don't we just add emit a NOTICE or WARNING in the relevant place
saying that pg_hba.conf is outdated? Then the server won't log those if
configured appropriately, which doesn't seem like a bad thing. Note that
<= ERROR messages aren't sent to the client during authentication.
I think people felt that sending that information to the client wouldn't
be a good idea security-wise. But I'd phrase it as "why not just emit
a LOG message?". I agree that new logging infrastructure seems like
overkill.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-01-16 12:21:13 -0500, Tom Lane wrote:
Andres Freund <andres@2ndquadrant.com> writes:
Why don't we just add emit a NOTICE or WARNING in the relevant place
saying that pg_hba.conf is outdated? Then the server won't log those if
configured appropriately, which doesn't seem like a bad thing. Note that
<= ERROR messages aren't sent to the client during authentication.I think people felt that sending that information to the client wouldn't
be a good idea security-wise.
It won't if issued during the right phase of the authentication:
/*
* client_min_messages is honored only after we complete the
* authentication handshake. This is required both for security
* reasons and because many clients can't handle NOTICE messages
* during authentication.
*/
if (ClientAuthInProgress)
output_to_client = (elevel >= ERROR);
else
output_to_client = (elevel >= client_min_messages ||
elevel == INFO);
}
Surely deserves a comment on the emitting site.
But I'd phrase it as "why not just emit a LOG message?".
Well, LOGs can be sent to the client just the same, no? Just requires a
nondefault client_min_messages.
But as I don't think sending logs to the client is a unsurmountable
problem (due to the above) I don't really care if we use WARNING or LOG.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> writes:
On 2015-01-16 12:21:13 -0500, Tom Lane wrote:
I think people felt that sending that information to the client wouldn't
be a good idea security-wise.
It won't if issued during the right phase of the authentication:
Good point.
But as I don't think sending logs to the client is a unsurmountable
problem (due to the above) I don't really care if we use WARNING or LOG.
If we're expecting the DBA to need to read it in the postmaster log,
remember that LOG is actually *more* likely to get to the log than
WARNING is. Choosing WARNING because you think it sounds more significant
is a mistake.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/20/14 12:11 PM, Steve Singer wrote:
On 12/19/2014 10:41 AM, Alex Shulgin wrote:
I don't think so. The scenario this patch relies on assumes that the
DBA will remember to look in the log if something goes wrong, and in
your case there would be a message like the following:WARNING: pg_hba.conf not reloaded
So an extra hint about file timestamp is unneeded.
That makes sense to me.
I haven't found any new issues with this patch.
I think it is ready for a committer.
There were later comments in this thread that disagreed with the extra
logging infrastructure, and there were some questions about whether it
should only log on failed authentication attempts. Altogether, still
some open questions about behavior and implementation approach. So I'm
marking this as returned with feedback for now.
Personally, I think this could be a useful feature, but it needs more
fine-tuning.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers