Re: [PATCH] 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

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

* Andres Freund (andres(at)2ndquadrant(dot)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

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.
>
>
>
>

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.

On Thu, Nov 6, 2014 at 5:46 PM, Peter Eisentraut <peter_e(at)gmx(dot)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

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Thu, Nov 27, 2014 at 8:49 AM, Bruce Momjian <bruce(at)momjian(dot)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

Peter Eisentraut <peter_e(at)gmx(dot)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

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?

Steve Singer <steve(at)ssinger(dot)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. -- 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?

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

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

Craig Ringer <craig(at)2ndquadrant(dot)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 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.

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

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
>
>

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

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

Andres Freund <andres(at)2ndquadrant(dot)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

On 2015-01-16 12:21:13 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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

Andres Freund <andres(at)2ndquadrant(dot)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

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.