Обсуждение: 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@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
Вложения
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 # (changerequires 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@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
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. +
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
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
Вложения
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@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. -- 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@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 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@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
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 wecomplete the * authentication handshake. This is required both for security * reasons and because many clientscan'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@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
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.