Re: Parsing of pg_hba.conf and authentication inconsistencies

The way pg_hba.conf is set up to be loaded/parsed now, we have:

postmaster: open and tokenize file (load_hba(), tokenize_file()).
backend: parse lines (parse_hba) and check for matches
(check_hba/hba_getauthmethod)

This means that the code in the postmaster is very simple, and it's
shared with the caching of the role and ident files.

It also means that we don't catch any syntax errors in the hba file
until a client connects. For example, if I misspell "local" on the first
line of the file (or just leave a bogus character on a line by mistake),
no client will be able to connect. But I can still do a pg_ctl reload
loading the broken file into the backend, thus making it impossible for
anybody to connect to the database. (when there's a broken line, we
won't continue to look at further lines in the file, obviously)

Is there any actual gain by not doing the parsing in the postmaster,
other than the fact that it's slightly less shared code with the other
two files? If not, then I'd like to move that parsing there for above
reasons.

I've also noticed that authentication methods error out in different
ways when they are not supported. For example, if I try to use Kerberos
without having it compiled in, I get an error when a client tries to
connect (because we compile in stub functions for the authentication
that just throw an error). But if I use pam, I get an "missing or
erroneous pg_hba.conf file" error (because we #ifdef out the entire
option all over the place). I'd like to make these consistent - but
which one of them do people prefer?

//Magnus

"Magnus Hagander" <magnus(at)hagander(dot)net> writes:

> I've also noticed that authentication methods error out in different
> ways when they are not supported. For example, if I try to use Kerberos
> without having it compiled in, I get an error when a client tries to
> connect (because we compile in stub functions for the authentication
> that just throw an error). But if I use pam, I get an "missing or
> erroneous pg_hba.conf file" error (because we #ifdef out the entire
> option all over the place). I'd like to make these consistent - but
> which one of them do people prefer?

Generally I prefer having stub functions which error out because it means
other code doesn't need lots of ifdef's around the call sites.

However it would be nice to throw an error or at least a warning when parsing
the file instead of pretending everything's ok. Perhaps authentication methods
should have a function to check whether the method is supported which is
called when the file is parsed.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's PostGIS support!

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Is there any actual gain by not doing the parsing in the postmaster,

Define "parsing". There's quite a lot of possible errors in pg_hba
that it would be totally unreasonable for the postmaster to detect.
We could catch some simple problems at file load time, perhaps,
but those usually aren't the ones that cause trouble for people.

On the whole, I am against putting any more functionality into the
main postmaster process than absolutely has to be there. Every
new function you put onto it is another potential source of
service-outage-inducing bugs.

> I've also noticed that authentication methods error out in different
> ways when they are not supported.

Yeah, that's something that should be made more consistent.

Idle thought: maybe what would really make sense here is a "lint"
for PG config files, which you'd run as a standalone program and
which would look for not only clear errors but questionable things
to warn about. For instance it might notice multiple pg_hba.conf
entries for the same IP addresses, check whether an LDAP server
can be connected to, check that all user/group/database names
used in the file actually exist, etc. These are things that we'd
certainly not put into any load- or reload-time tests.

regards, tom lane

Tom Lane wrote:

> Idle thought: maybe what would really make sense here is a "lint"
> for PG config files, which you'd run as a standalone program and
> which would look for not only clear errors but questionable things
> to warn about. For instance it might notice multiple pg_hba.conf
> entries for the same IP addresses, check whether an LDAP server
> can be connected to, check that all user/group/database names
> used in the file actually exist, etc. These are things that we'd
> certainly not put into any load- or reload-time tests.

I like this idea.

postgres --check-hba-file /path/to/hba.conf
postgres --check-conf-file /path/to/postgresql.conf

(I think it's better to reuse the same postmaster executable, because
that way it's easier to have the same parsing routines.)

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Magnus,

> However it would be nice to throw an error or at least a warning when parsing
> the file instead of pretending everything's ok. Perhaps authentication methods
> should have a function to check whether the method is supported which is
> called when the file is parsed.
>

The good way to solve this would be to have independant command line
utilities which check pg_hba.conf, pg_ident.conf and postgresql.conf for
errors. Then DBAs could run a check *before* restarting the server.

--Josh

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Is there any actual gain by not doing the parsing in the postmaster,
>
> Define "parsing". There's quite a lot of possible errors in pg_hba
> that it would be totally unreasonable for the postmaster to detect.

Parsing as in turning into a struct with clearly defined parts. Like
what type it is (host/local/hostssl), CIDR mask, auth method and parameters.

> We could catch some simple problems at file load time, perhaps,
> but those usually aren't the ones that cause trouble for people.

It would catch things like typos, invalid CIDR address/mask and
specifying an auth method that doesn't exist. This is the far most
common errors I've seen - which ones are you referring to?

> On the whole, I am against putting any more functionality into the
> main postmaster process than absolutely has to be there. Every
> new function you put onto it is another potential source of
> service-outage-inducing bugs.

True.

But as a counterexample, we have a whole lot of code in there to do the
same for GUC. Which can even call user code (custom variables), no? Are
you also proposing we should look at getting rid of that?

>> I've also noticed that authentication methods error out in different
>> ways when they are not supported.
>
> Yeah, that's something that should be made more consistent.
>
>
> Idle thought: maybe what would really make sense here is a "lint"
> for PG config files, which you'd run as a standalone program and
> which would look for not only clear errors but questionable things
> to warn about. For instance it might notice multiple pg_hba.conf
> entries for the same IP addresses, check whether an LDAP server
> can be connected to, check that all user/group/database names
> used in the file actually exist, etc. These are things that we'd
> certainly not put into any load- or reload-time tests.

That would also be a valuable tool, but IMHO for a slightly different
purpose. To me that sounds more in the line of the tool to "tune/suggest
certain postgresql.conf parameters" that has been discussed earlier.

It would have to be implemented as a SQL callable function or so in
order to make it usable for people doing remote admin, but that could
certainly be done.

It would still leave a fairly large hole open for anybody editing the
config file and just HUPing the postmaster (which a whole lot of people
do, since they're used to doing that to their daemon processes)

//Magnus

Josh Berkus wrote:
> Magnus,
>
>> However it would be nice to throw an error or at least a warning when
>> parsing
>> the file instead of pretending everything's ok. Perhaps authentication
>> methods
>> should have a function to check whether the method is supported which is
>> called when the file is parsed.
>>
>
> The good way to solve this would be to have independant command line
> utilities which check pg_hba.conf, pg_ident.conf and postgresql.conf for
> errors. Then DBAs could run a check *before* restarting the server.

While clearly useful, it'd still leave the fairly large foot-gun that is
editing the hba file and HUPing things which can leave you with a
completely un-connectable database because of a small typo. The
difference in the "could run" vs "must run, thus runs automatically" part...

//Magnus

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> Idle thought: maybe what would really make sense here is a "lint"
>> for PG config files,

> I like this idea.

> postgres --check-hba-file /path/to/hba.conf
> postgres --check-conf-file /path/to/postgresql.conf

> (I think it's better to reuse the same postmaster executable, because
> that way it's easier to have the same parsing routines.)

I'd go for just

postgres --check-config -D $PGDATA

(In a reload scenario, you'd edit the files in-place and then do this
before issuing SIGHUP.)

Reasons:

1. Easier to use: one command gets you all the checks, and you can't
accidentally forget to check the one config file that's gonna give
you problems.

2. An in-place check is the only way to be sure that, for instance,
relative-path 'include' directives are okay.

3. To implement the suggested check on role/database validity, the code
is going to need to pull in the flat-file copies of pg_database etc.
(Or try to contact a live postmaster, but that won't work when you don't
have one.) So it needs access to $PGDATA in any case.

4. There might be usefulness in cross-checking different config files,
so examining a single one out of context just seems like the wrong
mindset.

regards, tom lane

Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> The good way to solve this would be to have independant command line
>> utilities which check pg_hba.conf, pg_ident.conf and postgresql.conf for
>> errors. Then DBAs could run a check *before* restarting the server.

> While clearly useful, it'd still leave the fairly large foot-gun that is
> editing the hba file and HUPing things which can leave you with a
> completely un-connectable database because of a small typo.

That will *always* be possible, just because software is finite and
human foolishness is not ;-).

Now, we could ameliorate it a bit given a "postgres --check-config"
mode by having pg_ctl automatically run that mode before any start,
restart, or reload command, and then refusing to proceed if the check
detects any indubitable errors. On the other hand, that would leave
us with the scenario where the checking code warns about stuff that it
can't be sure is wrong, but then we go ahead and install the borked
config anyway. (Nobody is going to put up with code that refuses
to install config settings that aren't 100% clean, unless the checks
are so weak that they miss a lot of possibly-useful warnings.)

Seems a lot better to me to just train people to run the check-config
code by hand before pulling the trigger to load the settings for real.

regards, tom lane

Alvaro Herrera wrote:
> Tom Lane wrote:
>
>> Idle thought: maybe what would really make sense here is a "lint"
>> for PG config files, which you'd run as a standalone program and
>> which would look for not only clear errors but questionable things
>> to warn about. For instance it might notice multiple pg_hba.conf
>> entries for the same IP addresses, check whether an LDAP server
>> can be connected to, check that all user/group/database names
>> used in the file actually exist, etc. These are things that we'd
>> certainly not put into any load- or reload-time tests.
>
> I like this idea.
>
> postgres --check-hba-file /path/to/hba.conf
> postgres --check-conf-file /path/to/postgresql.conf
>
> (I think it's better to reuse the same postmaster executable, because
> that way it's easier to have the same parsing routines.)

Change that to pg_ctl and you have a deal :)

Joshua D. Drake

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Tom Lane wrote:
>>> Idle thought: maybe what would really make sense here is a "lint"
>>> for PG config files,
>
>> I like this idea.
>
>> postgres --check-hba-file /path/to/hba.conf
>> postgres --check-conf-file /path/to/postgresql.conf
>
>> (I think it's better to reuse the same postmaster executable, because
>> that way it's easier to have the same parsing routines.)
>
> I'd go for just
>
> postgres --check-config -D $PGDATA
>
> (In a reload scenario, you'd edit the files in-place and then do this
> before issuing SIGHUP.)
>
> regards, tom lane

Doesn't it seem reasonable that it should be pg_ctl? You should never
run postgres directly unless it is for DR.

Joshua D. Drake

Tom,

> Seems a lot better to me to just train people to run the check-config
> code by hand before pulling the trigger to load the settings for real.

Or just have pg_ctl --check-first as an option. Cautious DBAs would use
that. In 2-3 versions, we can make --check-first the default, and
require the option --dont-check for unattended restarts.

--Josh

Josh Berkus wrote:
> Tom,
>
>> Seems a lot better to me to just train people to run the check-config
>> code by hand before pulling the trigger to load the settings for real.
>
> Or just have pg_ctl --check-first as an option. Cautious DBAs would use
> that. In 2-3 versions, we can make --check-first the default, and
> require the option --dont-check for unattended restarts.

If the config file is known to be broken, why should you *ever* allow it
to try to restart it?

//Magnus

Joshua D. Drake wrote:
> Tom Lane wrote:

>> I'd go for just
>>
>> postgres --check-config -D $PGDATA
>>
>> (In a reload scenario, you'd edit the files in-place and then do this
>> before issuing SIGHUP.)

Sounds good.

> Doesn't it seem reasonable that it should be pg_ctl? You should never
> run postgres directly unless it is for DR.

What on earth is DR?

The problem with pg_ctl is that it's indirectly calling postgres, and it
doesn't have a lot of a way to know what happened after calling it;
consider the mess we have with pg_ctl -w.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Joshua D. Drake wrote:
>> Tom Lane wrote:

>> Doesn't it seem reasonable that it should be pg_ctl? You should never
>> run postgres directly unless it is for DR.
>
> What on earth is DR?

Disaster Recovery.

>
> The problem with pg_ctl is that it's indirectly calling postgres, and it
> doesn't have a lot of a way to know what happened after calling it;
> consider the mess we have with pg_ctl -w.
>

True enough but perhaps that is a problem in itself. IMO, we should be
encouraging people to never touch the postgres binary. If that means
pg_ctl becomes a lot smarter, then we have to consider that as well.

Comparatively if I do a apachectl configtest it tells me if it is
correct. Now I assume it is actually calling httpd (I haven't checked)
but the point is, I am not calling httpd.

Sincerely,

Joshua D. Drake

Joshua D. Drake wrote:
> Alvaro Herrera wrote:

>> The problem with pg_ctl is that it's indirectly calling postgres, and it
>> doesn't have a lot of a way to know what happened after calling it;
>> consider the mess we have with pg_ctl -w.
>
> True enough but perhaps that is a problem in itself. IMO, we should be
> encouraging people to never touch the postgres binary. If that means
> pg_ctl becomes a lot smarter, then we have to consider that as well.

That's a great idea, but I don't think we should weigh down this idea of
config checking with that responsability. We could discuss solutions to
this problem, but I think it's going to be quite a lot harder than you
seem to think.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Saturday 02 August 2008 13:36:40 Magnus Hagander wrote:
> Josh Berkus wrote:
> > Tom,
> >
> >> Seems a lot better to me to just train people to run the check-config
> >> code by hand before pulling the trigger to load the settings for real.
> >
> > Or just have pg_ctl --check-first as an option. Cautious DBAs would use
> > that. In 2-3 versions, we can make --check-first the default, and
> > require the option --dont-check for unattended restarts.
>
> If the config file is known to be broken, why should you *ever* allow it
> to try to restart it?
>

Certainly there isn't any reason to allow a reload of a file that is just
going to break things when the first connection happens. For that matter,
why would we ever not want to parse it at HUP time rather than connect time?

--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL

"Joshua D. Drake" <jd(at)commandprompt(dot)com> writes:
> True enough but perhaps that is a problem in itself. IMO, we should be
> encouraging people to never touch the postgres binary.

I don't buy that at all. pg_ctl is useful for some people and not so
useful for others; in particular, from the perspective of a system
startup script it tends to get in the way more than it helps.

regards, tom lane

Robert Treat <xzilla(at)users(dot)sourceforge(dot)net> writes:
> Certainly there isn't any reason to allow a reload of a file that is just
> going to break things when the first connection happens. For that matter,
> why would we ever not want to parse it at HUP time rather than connect time?

Two or three reasons why not were already mentioned upthread, but for
the stubborn, here's another one: are you volunteering to write the code
that backs out the config-file reload after the checks have determined
it was bad? Given the amount of pain we suffered trying to make GUC do
something similar, any sane person would run screaming from the
prospect.

regards, tom lane

Tom Lane wrote:
> Robert Treat <xzilla(at)users(dot)sourceforge(dot)net> writes:
>> Certainly there isn't any reason to allow a reload of a file that is just
>> going to break things when the first connection happens. For that matter,
>> why would we ever not want to parse it at HUP time rather than connect time?
>
> Two or three reasons why not were already mentioned upthread, but for
> the stubborn, here's another one: are you volunteering to write the code
> that backs out the config-file reload after the checks have determined
> it was bad? Given the amount of pain we suffered trying to make GUC do
> something similar, any sane person would run screaming from the
> prospect.

For pg_hba.conf, I don't see that as a very big problem, really. It
doesn't (and shouldn't) modify any "external" variables, so it should be
as simple as parsing the new file into a completely separate
list-of-structs and only if it's all correct switch the main pointer
(and free the old struct).

Yes, I still think we should do the "simple parsing" step at HUP time.
That doesn't mean that it wouldn't be a good idea to have one of these
check-config options that can look for conflicting options *as well*, of
course. But I'm getting the feeling I'm on the losing side of the debate
here...

//Magnus

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> The good way to solve this would be to have independant command line
>>> utilities which check pg_hba.conf, pg_ident.conf and postgresql.conf for
>>> errors. Then DBAs could run a check *before* restarting the server.
>
>> While clearly useful, it'd still leave the fairly large foot-gun that is
>> editing the hba file and HUPing things which can leave you with a
>> completely un-connectable database because of a small typo.
>
> That will *always* be possible, just because software is finite and
> human foolishness is not ;-).

Certainly - been bitten by that more than once. But we can make it
harder or easier to make the mistakes..

> Now, we could ameliorate it a bit given a "postgres --check-config"
> mode by having pg_ctl automatically run that mode before any start,
> restart, or reload command, and then refusing to proceed if the check
> detects any indubitable errors. On the other hand, that would leave
> us with the scenario where the checking code warns about stuff that it
> can't be sure is wrong, but then we go ahead and install the borked
> config anyway. (Nobody is going to put up with code that refuses
> to install config settings that aren't 100% clean, unless the checks
> are so weak that they miss a lot of possibly-useful warnings.)
>
> Seems a lot better to me to just train people to run the check-config
> code by hand before pulling the trigger to load the settings for real.

It's certainly easier for us, but that means a whole lot of people are
never going to do it. And initscripts might end up using it anyway,
forcing the issue.

I think it'd be reasonable to refuse starting if the config is *known
broken* (such as containing lines that are unparseable, or that contain
completely invalid tokens), whereas you'd start if they just contain
things that are "probably wrong". But picking from your previous
examples of "more advanced checks", there are lots of cases where
things like overlapping CIDR address ranges are perfectly valid, so I
don't think we could even throw a warning for that - unless there's a
separate flag to enable/disable warnings for such a thing.

//Magnus

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Robert Treat <xzilla(at)users(dot)sourceforge(dot)net> writes:
>> Certainly there isn't any reason to allow a reload of a file that is just
>> going to break things when the first connection happens. For that matter,
>> why would we ever not want to parse it at HUP time rather than connect time?
>
> Two or three reasons why not were already mentioned upthread, but for
> the stubborn, here's another one: are you volunteering to write the code
> that backs out the config-file reload after the checks have determined
> it was bad? Given the amount of pain we suffered trying to make GUC do
> something similar, any sane person would run screaming from the
> prospect.

Wouldn't that be *easier* if we do more parsing in the postmaster instead of
in the backends as Magnus suggested? Then it could build a new set of
structures and if there are any errors just throw them out before replacing
the old ones.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!

* Magnus Hagander (magnus(at)hagander(dot)net) wrote:
> For pg_hba.conf, I don't see that as a very big problem, really. It
> doesn't (and shouldn't) modify any "external" variables, so it should be
> as simple as parsing the new file into a completely separate
> list-of-structs and only if it's all correct switch the main pointer
> (and free the old struct).

I'm in agreement with this approach. Allowing a config which won't
parse properly to completely break access to a running database is
terrible. It just doesn't come across to me as being all that difficult
or complex code for pg_hba.conf.

> Yes, I still think we should do the "simple parsing" step at HUP time.
> That doesn't mean that it wouldn't be a good idea to have one of these
> check-config options that can look for conflicting options *as well*, of
> course. But I'm getting the feeling I'm on the losing side of the debate
> here...

A little extra code in the backend is well worth fixing this foot-gun.
The concerns raised so far have been "who will write it?" and "what if
it has bugs?". Neither of these are particularly compelling arguments
when you've already offered to write and bug-test it (right, Magnus? :).

Thanks,

Stephen

Stephen Frost wrote:
> * Magnus Hagander (magnus(at)hagander(dot)net) wrote:
>> For pg_hba.conf, I don't see that as a very big problem, really. It
>> doesn't (and shouldn't) modify any "external" variables, so it should be
>> as simple as parsing the new file into a completely separate
>> list-of-structs and only if it's all correct switch the main pointer
>> (and free the old struct).
>
> I'm in agreement with this approach. Allowing a config which won't
> parse properly to completely break access to a running database is
> terrible. It just doesn't come across to me as being all that difficult
> or complex code for pg_hba.conf.

That's my thoughts as well, which may be off of course ;-)

>> Yes, I still think we should do the "simple parsing" step at HUP time.
>> That doesn't mean that it wouldn't be a good idea to have one of these
>> check-config options that can look for conflicting options *as well*, of
>> course. But I'm getting the feeling I'm on the losing side of the debate
>> here...
>
> A little extra code in the backend is well worth fixing this foot-gun.
> The concerns raised so far have been "who will write it?" and "what if
> it has bugs?". Neither of these are particularly compelling arguments
> when you've already offered to write and bug-test it (right, Magnus? :).

Toms main argument has been that it would move the code *from* the
backend and into the *postmaster*, which is much more sensitive.

And yes, I've offered to write the code. I take this as an offer from
you to bug-test it :-)

//Magnus

* Magnus Hagander (magnus(at)hagander(dot)net) wrote:
> I think it'd be reasonable to refuse starting if the config is *known
> broken* (such as containing lines that are unparseable, or that contain
> completely invalid tokens), whereas you'd start if they just contain
> things that are "probably wrong". But picking from your previous
> examples of "more advanced checks", there are lots of cases where
> things like overlapping CIDR address ranges are perfectly valid, so I
> don't think we could even throw a warning for that - unless there's a
> separate flag to enable/disable warnings for such a thing.

Agreed. Making sure the config can parse is different from parsable but
non-sensible. It's ridiculously easy to mistakenly add a line w/ a
single character on it or something equally bad when saving a file
that's being modified by hand. That's a simple check that should be
done on re-hup and the broken config shouldn't be put in place.

I certainly agree that we should *also* have a way to just check the
config, so that can be built into init scripts and whatnot. I don't
think having one precludes having the other, and I'm pretty confident we
could find a way to not duplicate the code and have things be clean.

Thanks,

Stephen

* Magnus Hagander (magnus(at)hagander(dot)net) wrote:
> Stephen Frost wrote:
> > A little extra code in the backend is well worth fixing this foot-gun.
> > The concerns raised so far have been "who will write it?" and "what if
> > it has bugs?". Neither of these are particularly compelling arguments
> > when you've already offered to write and bug-test it (right, Magnus? :).
>
> Toms main argument has been that it would move the code *from* the
> backend and into the *postmaster*, which is much more sensitive.

erm, yes, sorry I wasn't being clear. Brain moving faster than fingers
sometimes. :)

> And yes, I've offered to write the code. I take this as an offer from
> you to bug-test it :-)

Indeed, as long as I'm bug-testing the Kerberos mappings at the same
time... ;) Seriously though, I'd be happy to review and test the code.

Thanks,

Stephen

"Joshua D. Drake" <jd(at)commandprompt(dot)com> writes:
> Alvaro Herrera wrote:
>> (I think it's better to reuse the same postmaster executable, because
>> that way it's easier to have the same parsing routines.)

> Change that to pg_ctl and you have a deal :)

Did you not understand Alvaro's point? Putting this functionality into
pg_ctl will result in huge code bloat, because it will have to duplicate
a lot of code that already exists inside the postgres executable.

I don't object to having a pg_ctl option to call "postgres --check",
but I do object to maintaining two copies of the same code.

regards, tom lane

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> Seems a lot better to me to just train people to run the check-config
>> code by hand before pulling the trigger to load the settings for real.

> I think it'd be reasonable to refuse starting if the config is *known
> broken* (such as containing lines that are unparseable, or that contain
> completely invalid tokens), whereas you'd start if they just contain
> things that are "probably wrong". But picking from your previous
> examples of "more advanced checks", there are lots of cases where
> things like overlapping CIDR address ranges are perfectly valid, so I
> don't think we could even throw a warning for that - unless there's a
> separate flag to enable/disable warnings for such a thing.

There are cases that are sane, and there are cases that are not.
You've got three possibilities:

* two lines referencing the exact same address range (and other
selectors such as user/database). Definitely a mistake, because
the second one is unreachable.

* two lines where the second's address range is a subset of the
first (and other stuff is the same). Likewise a mistake.

* two lines where the first's address range is a subset of the
second's. This one is the only sane one.

(The nature of CIDR notation is that there are no partial overlaps,
so it must be one of these three cases.)

We have in fact seen complaints from people who apparently missed
the fact that pg_hba.conf entries are order-sensitive, so I think
a test like this would be worth making. But it shouldn't be done
by the postmaster.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Tom Lane wrote:
>>> Seems a lot better to me to just train people to run the check-config
>>> code by hand before pulling the trigger to load the settings for real.
>
>> I think it'd be reasonable to refuse starting if the config is *known
>> broken* (such as containing lines that are unparseable, or that contain
>> completely invalid tokens), whereas you'd start if they just contain
>> things that are "probably wrong". But picking from your previous
>> examples of "more advanced checks", there are lots of cases where
>> things like overlapping CIDR address ranges are perfectly valid, so I
>> don't think we could even throw a warning for that - unless there's a
>> separate flag to enable/disable warnings for such a thing.
>
> There are cases that are sane, and there are cases that are not.
> You've got three possibilities:
>
> * two lines referencing the exact same address range (and other
> selectors such as user/database). Definitely a mistake, because
> the second one is unreachable.
>
> * two lines where the second's address range is a subset of the
> first (and other stuff is the same). Likewise a mistake.
>
> * two lines where the first's address range is a subset of the
> second's. This one is the only sane one.

Yeah, certainly. But a very common one at that.

> (The nature of CIDR notation is that there are no partial overlaps,
> so it must be one of these three cases.)

Right.

> We have in fact seen complaints from people who apparently missed
> the fact that pg_hba.conf entries are order-sensitive, so I think
> a test like this would be worth making. But it shouldn't be done
> by the postmaster.

Agreed. Postmaster should verify things only to the point that it's a
valid CIDR mask (say that the IP is actually numeric and not
1.2.foo.3/32). Any further context analysis does not belong there.

Should I read this as you warming up slightly to the idea of having the
postmaster do that? ;-)

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Should I read this as you warming up slightly to the idea of having the
> postmaster do that? ;-)

No ;-). I still think that a "postgres --check-config" feature would be
far more complete and useful, as well as less likely to cause bugs in
critical code paths.

A point that I don't think has been made so far in the thread: the
only place the postmaster could complain in event of problems is the
postmaster log, which we know too well isn't watched by inexperienced
DBAs. I guarantee you that we will get bug reports along the lines of
"I updated pg_hba.conf and did pg_ctl reload, but nothing changed!
Postgres sucks!" if we implement checking at load time. I think one of
the main advantages of a --check-config approach is that whatever it had
to say would come out on the user's terminal.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Should I read this as you warming up slightly to the idea of having the
>> postmaster do that? ;-)
>
> No ;-).

Bummer. Worth a shot though :-)

> I still think that a "postgres --check-config" feature would be
> far more complete and useful, as well as less likely to cause bugs in
> critical code paths.

I still think we should have both :-)

> A point that I don't think has been made so far in the thread: the
> only place the postmaster could complain in event of problems is the
> postmaster log, which we know too well isn't watched by inexperienced
> DBAs. I guarantee you that we will get bug reports along the lines of
> "I updated pg_hba.conf and did pg_ctl reload, but nothing changed!
> Postgres sucks!" if we implement checking at load time. I think one of
> the main advantages of a --check-config approach is that whatever it had
> to say would come out on the user's terminal.

How is this different from how we deal with postgresql.conf today? That
one can only log errors there as well, no? (And has a lot more complex
code to get there)

Which would also be helped byu a --check-config approach, of course -
I'm not saying we shouldn't have that, just that I want us to have both :-)

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> A point that I don't think has been made so far in the thread: the
>> only place the postmaster could complain in event of problems is the
>> postmaster log, which we know too well isn't watched by inexperienced
>> DBAs. I guarantee you that we will get bug reports along the lines of
>> "I updated pg_hba.conf and did pg_ctl reload, but nothing changed!
>> Postgres sucks!" if we implement checking at load time.

> How is this different from how we deal with postgresql.conf today?

It isn't, and I seem to recall we've had that scenario play out a couple
times already for postgresql.conf changes. But pg_hba.conf is far more
complex than "variable = value" ...

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Tom Lane wrote:
>>> A point that I don't think has been made so far in the thread: the
>>> only place the postmaster could complain in event of problems is the
>>> postmaster log, which we know too well isn't watched by inexperienced
>>> DBAs. I guarantee you that we will get bug reports along the lines of
>>> "I updated pg_hba.conf and did pg_ctl reload, but nothing changed!
>>> Postgres sucks!" if we implement checking at load time.
>
>> How is this different from how we deal with postgresql.conf today?
>
> It isn't, and I seem to recall we've had that scenario play out a couple
> times already for postgresql.conf changes. But pg_hba.conf is far more
> complex than "variable = value" ...

Ok, then I didn't misunderstand that part at least :-)

Ah, well. I know that if others don't pipe in on my side of it, I'm
implicitly out-voted ;), since I've stated my case by now... Thus, I
won't put any time into working on it unless someone does.

//Magnus

* Magnus Hagander (magnus(at)hagander(dot)net) wrote:
> Tom Lane wrote:
> > It isn't, and I seem to recall we've had that scenario play out a couple
> > times already for postgresql.conf changes. But pg_hba.conf is far more
> > complex than "variable = value" ...
>
> Ok, then I didn't misunderstand that part at least :-)
>
> Ah, well. I know that if others don't pipe in on my side of it, I'm
> implicitly out-voted ;), since I've stated my case by now... Thus, I
> won't put any time into working on it unless someone does.

Having one doesn't imply we don't have the other. I believe we should
definitely have both the --check-config (to address Tom's concern, and
to improve the user experience when doing an /etc/init.d/postgresql
reload or similar) and the check done in the postmaster and have it only
update the running config if the config file parsed correctly.

Thanks,

Stephen

On Sun, 2008-08-03 at 10:36 +0200, Magnus Hagander wrote:
> Tom Lane wrote:
> > Magnus Hagander <magnus(at)hagander(dot)net> writes:
> >>> The good way to solve this would be to have independant command line
> >>> utilities which check pg_hba.conf, pg_ident.conf and postgresql.conf for
> >>> errors. Then DBAs could run a check *before* restarting the server.
> >
> >> While clearly useful, it'd still leave the fairly large foot-gun that is
> >> editing the hba file and HUPing things which can leave you with a
> >> completely un-connectable database because of a small typo.
> >
> > That will *always* be possible, just because software is finite and
> > human foolishness is not ;-).
>
> Certainly - been bitten by that more than once. But we can make it
> harder or easier to make the mistakes..

Yeah. I'm sure we've all done it.

Would it be possible to have two config files? An old and a new?

That way we could specify new file, but if an error is found we revert
to the last known-good file?

That would encourage the best practice of take-a-copy-then-edit.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Aug 5, 2008, at 4:07 PM, Simon Riggs wrote:

>
> On Sun, 2008-08-03 at 10:36 +0200, Magnus Hagander wrote:
>> Tom Lane wrote:
>>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>>> The good way to solve this would be to have independant command
>>>>> line
>>>>> utilities which check pg_hba.conf, pg_ident.conf and
>>>>> postgresql.conf for
>>>>> errors. Then DBAs could run a check *before* restarting the
>>>>> server.
>>>
>>>> While clearly useful, it'd still leave the fairly large foot-gun
>>>> that is
>>>> editing the hba file and HUPing things which can leave you with a
>>>> completely un-connectable database because of a small typo.
>>>
>>> That will *always* be possible, just because software is finite and
>>> human foolishness is not ;-).
>>
>> Certainly - been bitten by that more than once. But we can make it
>> harder or easier to make the mistakes..
>
> Yeah. I'm sure we've all done it.
>
> Would it be possible to have two config files? An old and a new?
>
> That way we could specify new file, but if an error is found we revert
> to the last known-good file?
>
> That would encourage the best practice of take-a-copy-then-edit.

Perhaps the --check-config option should take an (optional) file name?
That would allow you to validate a config file without having to copy
it into place first.

postgres --check-config=myFilenameGoesHere -D $PGDATA

-- Korry

korry wrote:
>
> On Aug 5, 2008, at 4:07 PM, Simon Riggs wrote:
>
>>
>> On Sun, 2008-08-03 at 10:36 +0200, Magnus Hagander wrote:
>>> Tom Lane wrote:
>>>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>>>> The good way to solve this would be to have independant command line
>>>>>> utilities which check pg_hba.conf, pg_ident.conf and
>>>>>> postgresql.conf for
>>>>>> errors. Then DBAs could run a check *before* restarting the server.
>>>>
>>>>> While clearly useful, it'd still leave the fairly large foot-gun
>>>>> that is
>>>>> editing the hba file and HUPing things which can leave you with a
>>>>> completely un-connectable database because of a small typo.
>>>>
>>>> That will *always* be possible, just because software is finite and
>>>> human foolishness is not ;-).
>>>
>>> Certainly - been bitten by that more than once. But we can make it
>>> harder or easier to make the mistakes..
>>
>> Yeah. I'm sure we've all done it.
>>
>> Would it be possible to have two config files? An old and a new?
>>
>> That way we could specify new file, but if an error is found we revert
>> to the last known-good file?
>>
>> That would encourage the best practice of take-a-copy-then-edit.
>
> Perhaps the --check-config option should take an (optional) file name?
> That would allow you to validate a config file without having to copy it
> into place first.
>
> postgres --check-config=myFilenameGoesHere -D $PGDATA

If you're doing it that way, you need one for each type of file again.
And you're still not helping the vast majority who will not bother with
more than one file. They'll edit one file, and trust the system not to
load a known broken file. That's kind of like every other daemon on the
system works, so that's what people will be expecting.

//Magnus

Am Tuesday, 5. August 2008 schrieb korry:
> Perhaps the --check-config option should take an (optional) file name?
> That would allow you to validate a config file without having to copy
> it into place first.
>
> postgres --check-config=myFilenameGoesHere -D $PGDATA

There is already an elaborate system to tell where configuration files are
located. So just doing postgres --check-config -D myFilenameGoesHere should
do the job.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Am Tuesday, 5. August 2008 schrieb korry:
>> Perhaps the --check-config option should take an (optional) file name?
>> That would allow you to validate a config file without having to copy
>> it into place first.
>>
>> postgres --check-config=myFilenameGoesHere -D $PGDATA

> There is already an elaborate system to tell where configuration files are
> located. So just doing postgres --check-config -D myFilenameGoesHere should
> do the job.

Even more to the point: in the presence of include directives with
relative file paths, the idea of checking a file that's not in its
intended place is broken to begin with. Both pg_hba.conf and
postgresql.conf have include-type syntax, though the former doesn't
call it that.

regards, tom lane

Stephen Frost wrote:
> * Magnus Hagander (magnus(at)hagander(dot)net) wrote:
>> Tom Lane wrote:
>>> It isn't, and I seem to recall we've had that scenario play out a couple
>>> times already for postgresql.conf changes. But pg_hba.conf is far more
>>> complex than "variable = value" ...
>> Ok, then I didn't misunderstand that part at least :-)
>>
>> Ah, well. I know that if others don't pipe in on my side of it, I'm
>> implicitly out-voted ;), since I've stated my case by now... Thus, I
>> won't put any time into working on it unless someone does.
>
> Having one doesn't imply we don't have the other. I believe we should
> definitely have both the --check-config (to address Tom's concern, and
> to improve the user experience when doing an /etc/init.d/postgresql
> reload or similar) and the check done in the postmaster and have it only
> update the running config if the config file parsed correctly.

I thought of another issue with this. My "grand plan" includes being
able to do username mapping (per pg_ident.conf) for other authentication
methods than ident. Specifically this would be interesting for all
external methods, like gssapi/sspi/kerberos/ldap. I was originally
planning to allow each row in pg_hba to specify it's own pg_ident.conf
if necessary (so you could map LDAP and GSSAPI differently, for example,
or map two different kerberos realms differently). To be able to load
these, the postmaster would need to know about them, which means it'd
have to parse that data out anyway.

The other way to do that is to simply say that all external mapping will
use pg_ident.conf, and the only thing you can specify on a per-row basis
is "use map: yes or no". This decreases the flexibility, but would not
require the postmaster to do the parsing.

What do people think about these? I know Stephen for example really want
that feature so - would that restriction make it a lot less useful for you?

//Magnus

Magnus,

* Magnus Hagander (magnus(at)hagander(dot)net) wrote:
> I thought of another issue with this. My "grand plan" includes being
> able to do username mapping (per pg_ident.conf) for other authentication
> methods than ident. Specifically this would be interesting for all
> external methods, like gssapi/sspi/kerberos/ldap. I was originally
> planning to allow each row in pg_hba to specify it's own pg_ident.conf
> if necessary (so you could map LDAP and GSSAPI differently, for example,
> or map two different kerberos realms differently). To be able to load
> these, the postmaster would need to know about them, which means it'd
> have to parse that data out anyway.

I certainly like the concept of having them be in seperate files.

> The other way to do that is to simply say that all external mapping will
> use pg_ident.conf, and the only thing you can specify on a per-row basis
> is "use map: yes or no". This decreases the flexibility, but would not
> require the postmaster to do the parsing.

I don't think it makes sense to have multiple different auth types using
the same mappings... For Kerberos, as an example, we should support
user(at)REALM as an option for the mapping, but that might not make sense
for LDAP, which might have cn=User,ou=people,dc=example,dc=com, and
neither of those really make sense for ident. Mashing all of those
together would make each auth type supporting the mapping have to search
through the list trying to make sense of some mappings and throwing away
others, just ugly all around..

> What do people think about these? I know Stephen for example really want
> that feature so - would that restriction make it a lot less useful for you?

If we really wanted to keep it to a single *file*, then I think there
should be a way to key rows in the pg_hba.conf to sets-of-rows in the
mapping file. eg: have an option of 'mapkey=xyz' in pg_hba, and then
'xyz' as the first column of the mapping file, with it being repeated
across rows to form that mapping table.

It wouldn't be very easy/clean to do that w/o breaking the existing
structure of pg_ident though, which makes me feel like using seperate
files is probably the way to go.

Thanks,

Stephen

Stephen Frost wrote:
> Magnus,
>
> * Magnus Hagander (magnus(at)hagander(dot)net) wrote:
>> I thought of another issue with this. My "grand plan" includes being
>> able to do username mapping (per pg_ident.conf) for other authentication
>> methods than ident. Specifically this would be interesting for all
>> external methods, like gssapi/sspi/kerberos/ldap. I was originally
>> planning to allow each row in pg_hba to specify it's own pg_ident.conf
>> if necessary (so you could map LDAP and GSSAPI differently, for example,
>> or map two different kerberos realms differently). To be able to load
>> these, the postmaster would need to know about them, which means it'd
>> have to parse that data out anyway.
>
> I certainly like the concept of having them be in seperate files.
>
>> The other way to do that is to simply say that all external mapping will
>> use pg_ident.conf, and the only thing you can specify on a per-row basis
>> is "use map: yes or no". This decreases the flexibility, but would not
>> require the postmaster to do the parsing.
>
> I don't think it makes sense to have multiple different auth types using
> the same mappings... For Kerberos, as an example, we should support
> user(at)REALM as an option for the mapping, but that might not make sense
> for LDAP, which might have cn=User,ou=people,dc=example,dc=com, and
> neither of those really make sense for ident. Mashing all of those
> together would make each auth type supporting the mapping have to search
> through the list trying to make sense of some mappings and throwing away
> others, just ugly all around..

Yeah. I think the question there is just - how likely is it that the
same installation actually uses >1 authentication method. Personally, I
think it's not very uncommon at all, but fact remains that as long as
you only use one of them at a time, using a shared file doesn't matter.

>> What do people think about these? I know Stephen for example really want
>> that feature so - would that restriction make it a lot less useful for you?
>
> If we really wanted to keep it to a single *file*, then I think there
> should be a way to key rows in the pg_hba.conf to sets-of-rows in the
> mapping file. eg: have an option of 'mapkey=xyz' in pg_hba, and then
> 'xyz' as the first column of the mapping file, with it being repeated
> across rows to form that mapping table.

Yuck. Honestly, that seems even uglier :-)

> It wouldn't be very easy/clean to do that w/o breaking the existing
> structure of pg_ident though, which makes me feel like using seperate
> files is probably the way to go.

Yeah, thats my feeling as well. Now, can someone figure out a way to do
that without parsing the file in the postmaster? (And if we do parse it,
there's no point in not storing the parsed version, IMHO). And if not,
the question it comes down to is which is most important - keeping the
parsing away, or being able to do this ;-)

//Magnus

Magnus,

* Magnus Hagander (magnus(at)hagander(dot)net) wrote:
> Yeah. I think the question there is just - how likely is it that the
> same installation actually uses >1 authentication method. Personally, I
> think it's not very uncommon at all, but fact remains that as long as
> you only use one of them at a time, using a shared file doesn't matter.

We use multiple authentication types *alot*.. ident, md5, kerberos, and
gssapi are all currently in use on our systems. ident for local unix
logins, md5 for 'role' accounts and software the doesn't understand
kerberos, kerberos/gssapi depending on the age of the client library
connecting. Oh, and we use pam too.. We use some mappings now with
ident, which I'd expect to continue to do, and I've got definite plans
for mappings under Kerberos/GSSAPI once it's supported..

> > It wouldn't be very easy/clean to do that w/o breaking the existing
> > structure of pg_ident though, which makes me feel like using seperate
> > files is probably the way to go.
>
> Yeah, thats my feeling as well. Now, can someone figure out a way to do
> that without parsing the file in the postmaster? (And if we do parse it,
> there's no point in not storing the parsed version, IMHO). And if not,
> the question it comes down to is which is most important - keeping the
> parsing away, or being able to do this ;-)

I don't have an answer wrt the parsing issue, but I definitely want to
be able to do this. :)

Thanks,

Stephen

Stephen Frost wrote:
> Magnus,
>
> * Magnus Hagander (magnus(at)hagander(dot)net) wrote:
>> Yeah. I think the question there is just - how likely is it that the
>> same installation actually uses >1 authentication method. Personally, I
>> think it's not very uncommon at all, but fact remains that as long as
>> you only use one of them at a time, using a shared file doesn't matter.
>
> We use multiple authentication types *alot*.. ident, md5, kerberos, and
> gssapi are all currently in use on our systems. ident for local unix
> logins, md5 for 'role' accounts and software the doesn't understand
> kerberos, kerberos/gssapi depending on the age of the client library
> connecting. Oh, and we use pam too.. We use some mappings now with
> ident, which I'd expect to continue to do, and I've got definite plans
> for mappings under Kerberos/GSSAPI once it's supported..

Ok. Good to know - if you want to use it, there are bound to be a number
of others who would like it as well :)

>>> It wouldn't be very easy/clean to do that w/o breaking the existing
>>> structure of pg_ident though, which makes me feel like using seperate
>>> files is probably the way to go.
>> Yeah, thats my feeling as well. Now, can someone figure out a way to do
>> that without parsing the file in the postmaster? (And if we do parse it,
>> there's no point in not storing the parsed version, IMHO). And if not,
>> the question it comes down to is which is most important - keeping the
>> parsing away, or being able to do this ;-)
>
> I don't have an answer wrt the parsing issue, but I definitely want to
> be able to do this. :)

Right.

I guess one option would be to load the map file at runtime in the
backend, and not pre-load/cache it from the postmaster. But that seems
rahter sub-optimal to me. Other thoughts?

//Magnus

Magnus Hagander wrote:

[about the ability to use different maps for ident auth, gss and krb
auth for example]

>>>> It wouldn't be very easy/clean to do that w/o breaking the existing
>>>> structure of pg_ident though, which makes me feel like using seperate
>>>> files is probably the way to go.

Actually, I may have to take that back. We already have support for
multiple maps in the ident file, I'm not really sure anymore of the case
where this wouldn't be enough :-)

That said, I still think we want to parse pg_hba in the postmaster,
because it allows us to not load known broken files, and show errors
when you actually change the file etc. ;-)

I did code up a POC patch for it, and it's not particularly hard to do.
Mostly it's just moving the codepath from the backend to the postmaster.
I'll clean it up a but and post it, just so ppl can see what it looks
like...

//Magnus

Hello,

On Sat, 02 Aug 2008 18:37:25 +0200 Magnus Hagander wrote:

> Tom Lane wrote:
> > Magnus Hagander <magnus(at)hagander(dot)net> writes:
>
> > We could catch some simple problems at file load time, perhaps,
> > but those usually aren't the ones that cause trouble for people.
>
> It would catch things like typos, invalid CIDR address/mask and
> specifying an auth method that doesn't exist. This is the far most
> common errors I've seen - which ones are you referring to?

it may not be the far most common error but of course it's a big
problem.

For the DBA: if the configfile is in a version control system you
first have to edit the file again, search the error, submit the file and
then restart the DB - if you got the syntax error during a database
restart you are cursing all the time because the database is offline
right now.

For an newbie: as mentioned before, this guy doesn't even know where to
look for an error, but the database is offline. "Stupid Postgres, i
want something else which is working."

Of course a syntax check before or on startup cannot check for all
errors, especially not for logic errors but if we can exclude any
syntax error that would be a big help. For myself i don't care which
tool is doing the check as long as it's possible to check the config at
all.

Kind regards

--
Andreas 'ads' Scherbaum
German PostgreSQL User Group

Magnus Hagander wrote:
> Magnus Hagander wrote:
>
> [about the ability to use different maps for ident auth, gss and krb
> auth for example]
>
> >>>> It wouldn't be very easy/clean to do that w/o breaking the existing
> >>>> structure of pg_ident though, which makes me feel like using seperate
> >>>> files is probably the way to go.
>
> Actually, I may have to take that back. We already have support for
> multiple maps in the ident file, I'm not really sure anymore of the case
> where this wouldn't be enough :-)
>
> That said, I still think we want to parse pg_hba in the postmaster,
> because it allows us to not load known broken files, and show errors
> when you actually change the file etc. ;-)
>
> I did code up a POC patch for it, and it's not particularly hard to do.
> Mostly it's just moving the codepath from the backend to the postmaster.
> I'll clean it up a but and post it, just so ppl can see what it looks
> like...

To address Magnus' specific question, right now we store the pg_hba.conf
tokens as strings in the postmaster. I am fine with storing them in a
more native format and throwing errors for values that don't convert.
What would concern me is calling lots of 3rd party libraries from the
postmaster to validate items.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Magnus Hagander wrote:
>> Magnus Hagander wrote:
>>
>> [about the ability to use different maps for ident auth, gss and krb
>> auth for example]
>>
>>>>>> It wouldn't be very easy/clean to do that w/o breaking the existing
>>>>>> structure of pg_ident though, which makes me feel like using seperate
>>>>>> files is probably the way to go.
>> Actually, I may have to take that back. We already have support for
>> multiple maps in the ident file, I'm not really sure anymore of the case
>> where this wouldn't be enough :-)
>>
>> That said, I still think we want to parse pg_hba in the postmaster,
>> because it allows us to not load known broken files, and show errors
>> when you actually change the file etc. ;-)
>>
>> I did code up a POC patch for it, and it's not particularly hard to do.
>> Mostly it's just moving the codepath from the backend to the postmaster.
>> I'll clean it up a but and post it, just so ppl can see what it looks
>> like...
>
> To address Magnus' specific question, right now we store the pg_hba.conf
> tokens as strings in the postmaster. I am fine with storing them in a
> more native format and throwing errors for values that don't convert.
> What would concern me is calling lots of 3rd party libraries from the
> postmaster to validate items.

If I was unclear about that, that part was never part of what I
proposed. I'm only talking aobut parsing the syntax. The only external
calls in the code there now is the getaddrinfo calls to convert the IPs,
IIRC.

//Magnus

Magnus Hagander wrote:
> > To address Magnus' specific question, right now we store the pg_hba.conf
> > tokens as strings in the postmaster. I am fine with storing them in a
> > more native format and throwing errors for values that don't convert.
> > What would concern me is calling lots of 3rd party libraries from the
> > postmaster to validate items.
>
> If I was unclear about that, that part was never part of what I
> proposed. I'm only talking aobut parsing the syntax. The only external
> calls in the code there now is the getaddrinfo calls to convert the IPs,
> IIRC.

That seems safe to me. The use of strings for the pg_hba.conf content
was only for convenience; I can see the advantage of using a more
natural format.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Magnus Hagander wrote:
>>> To address Magnus' specific question, right now we store the pg_hba.conf
>>> tokens as strings in the postmaster. I am fine with storing them in a
>>> more native format and throwing errors for values that don't convert.
>>> What would concern me is calling lots of 3rd party libraries from the
>>> postmaster to validate items.
>> If I was unclear about that, that part was never part of what I
>> proposed. I'm only talking aobut parsing the syntax. The only external
>> calls in the code there now is the getaddrinfo calls to convert the IPs,
>> IIRC.
>
> That seems safe to me. The use of strings for the pg_hba.conf content
> was only for convenience; I can see the advantage of using a more
> natural format.

Attached is the patch I have so far. The only "extra" it adds over today
is that it allows the use of "ident" authentication without explicitly
specifying "sameuser" when you want that.

Other than that, it moves code around to do the parsing in the
postmaster and the maching in the backend. This means that now if there
is a syntax error in the file on a reload, we just keep the old file
around still letting people log into the database. If there is a syntax
error on server startup, it's FATAL of course, since we can't run
without any kind of pg_hba.

It also changes a couple of error cases to explicitly state that support
for a certain auth method isn't compiled in, rather than just call it a
syntax error.

Comments?

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Other than that, it moves code around to do the parsing in the
> postmaster and the maching in the backend.

How does that work in EXEC_BACKEND environments?

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Other than that, it moves code around to do the parsing in the
>> postmaster and the maching in the backend.
>
> How does that work in EXEC_BACKEND environments?

(Not tested yet, still on my TODO, but still)

It will parse the file in the postmaster *and* in the backend. Thus, the
safety that it won't reload on a broken file actually won't work since
backends reload the configuration everytime they start, but you will
still get the error/warning on reload.

//Magnus

Hi Magnus,

Josh has assigned your patch to me for an initial review.

First up I'd like to say that this is a really nice upgrade.
Shielding a running server from reloading a bogus conf file makes a
whole lot of sense.

On Sun, Aug 17, 2008 at 1:47 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> Attached is the patch I have so far. The only "extra" it adds over today
> is that it allows the use of "ident" authentication without explicitly
> specifying "sameuser" when you want that.
>

Nice. I'd expect that custom ident maps are pretty rare, so this
seems like a good move.

> Other than that, it moves code around to do the parsing in the
> postmaster and the maching in the backend. This means that now if there
> is a syntax error in the file on a reload, we just keep the old file
> around still letting people log into the database. If there is a syntax
> error on server startup, it's FATAL of course, since we can't run
> without any kind of pg_hba.
>
> It also changes a couple of error cases to explicitly state that support
> for a certain auth method isn't compiled in, rather than just call it a
> syntax error.
>

The patch applied cleanly to HEAD, compiled fine on amd64 gentoo and
the features appeared to work as advertised. I tried putting various
kinds of junk into my pg_hba.conf file and reloading/restarting the
postmaster to see how it would react. As expected, on a reload I got
a parse error and a WARNING letting me know that the new configuration
was not loaded. On a restart, I got the same parse error, and a FATAL
indicating that the postmaster couldn't start.

I also checked that it was safe to omit "sameuser" in an "ident"
record, and again that worked as expected.

The patch doesn't include any updates to documentation. I humbly
suggest that the change to the ident map (making "sameuser" the
default) should be mentioned both in the Manual itself, and in the
sample conf file comments. Here are a few sites that may be in want
of an update:

* src/backend/libpq/pg_ident.conf.sample:33 - "Instead, use the
special map name "sameuser" in pg_hba.conf"

* doc/src/sgml/client-auth.sgml:512 has a sample "ident sameuser" record

* doc/src/sgml/client-auth.sgml:931 - "There is a predefined ident
map <literal>sameuser</literal>"

The section on Ident Authentication might need some broader rewording
to indicate that the map argument is now optional.

The new error messages are good, but I wonder if they could be
improved by using DETAIL segments. The guidelines on error message
style say that the primary message should be terse and make a
reasonable effort to fit on one line. For example, this:

LOG: invalid IP address "a.b.c.d" in file
"/home/direvus/src/postgres/inst/data/pg_hba.conf" line 77: Name or
service not known

Could be rewritten as something like this:

LOG: invalid IP address "a.b.c.d" in auth config: Name or service not known
DETAIL: In file "/home/direvus/src/postgres/inst/data/pg_hba.conf", line 77

That's all the feedback I have for the moment. I hope you find my
comments helpful.

Cheers,
BJ

Brendan Jurd wrote:
> Hi Magnus,
>
> Josh has assigned your patch to me for an initial review.
>
> First up I'd like to say that this is a really nice upgrade.
> Shielding a running server from reloading a bogus conf file makes a
> whole lot of sense.

Hi!

Thanks for your review.

> The patch doesn't include any updates to documentation. I humbly
> suggest that the change to the ident map (making "sameuser" the
> default) should be mentioned both in the Manual itself, and in the
> sample conf file comments. Here are a few sites that may be in want
> of an update:

Yes, absolutely. Thanks for the couple of pointers, that'll help me not
to miss them :-)

> The new error messages are good, but I wonder if they could be
> improved by using DETAIL segments. The guidelines on error message
> style say that the primary message should be terse and make a
> reasonable effort to fit on one line. For example, this:
>
> LOG: invalid IP address "a.b.c.d" in file
> "/home/direvus/src/postgres/inst/data/pg_hba.conf" line 77: Name or
> service not known
>
> Could be rewritten as something like this:
>
> LOG: invalid IP address "a.b.c.d" in auth config: Name or service not known
> DETAIL: In file "/home/direvus/src/postgres/inst/data/pg_hba.conf", line 77

Makes a lot of sense, I'll go about making that change.

//Magnus

On Fri, 12 Sep 2008 06:53:55 +1000
"Brendan Jurd" <direvus(at)gmail(dot)com> wrote:
> Josh has assigned your patch to me for an initial review.

And me.

> First up I'd like to say that this is a really nice upgrade.
> Shielding a running server from reloading a bogus conf file makes a
> whole lot of sense.

Yes.

> The patch applied cleanly to HEAD, compiled fine on amd64 gentoo and

I had a small problem compiling. I'm not sure why it would be
different for me. I run NetBSD -current. Here is the error:

../../../src/include/libpq/hba.h:51: error: field 'addr' has incomplete
type

I was able to fix this by adding the following line to hba.h:

#include "libpq/pqcomm.h" /* needed for struct sockaddr_storage */

--
D'Arcy J.M. Cain <darcy(at)druid(dot)net> | Democracy is three wolves
http://www.druid.net/darcy/ | and a sheep voting on
+1 416 425 1212 (DoD#0082) (eNTP) | what's for dinner.

D'Arcy J.M. Cain wrote:
> On Fri, 12 Sep 2008 06:53:55 +1000
> "Brendan Jurd" <direvus(at)gmail(dot)com> wrote:
>> Josh has assigned your patch to me for an initial review.
>
> And me.

Thanks for the reviews, guys. I've adjusted the patch per your comments
(I think), and applied it.

//Magnus

On Mon, Sep 15, 2008 at 10:53 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> Thanks for the reviews, guys. I've adjusted the patch per your comments
> (I think), and applied it.
>

Cool. I had a look at the commitdiff and I think you missed one of
the error messages (it's still all on one line). It's at
src/backend/libpq/hba.c line 841. I've included a patch to split it
up below.

Cheers,
BJ

--- src/backend/libpq/hba.c
+++ src/backend/libpq/hba.c
@@ -840,9 +840,10 @@ hba_syntax:
if (line_item)
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("invalid entry in file \"%s\" at line %d, token \"%s\"",
- HbaFileName, line_num,
- (char *) lfirst(line_item))));
+ errmsg("invalid entry for token \"%s\"",
+ (char *) lfirst(line_item)),
+ errdetail("In file \"%s\", line %d",
+ HbaFileName, line_num)));
else
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> Cool. I had a look at the commitdiff and I think you missed one of
> the error messages (it's still all on one line). It's at
> src/backend/libpq/hba.c line 841. I've included a patch to split it
> up below.

Hm, that doesn't seem like a great improvement --- in particular, it
violates the style guideline that detail messages should be complete
sentences.

I think the error text is all right as-is, really.

regards, tom lane

Tom Lane wrote:
> "Brendan Jurd" <direvus(at)gmail(dot)com> writes:
>> Cool. I had a look at the commitdiff and I think you missed one of
>> the error messages (it's still all on one line). It's at
>> src/backend/libpq/hba.c line 841. I've included a patch to split it
>> up below.
>
> Hm, that doesn't seem like a great improvement --- in particular, it
> violates the style guideline that detail messages should be complete
> sentences.
>
> I think the error text is all right as-is, really.

I think I need to hit myself over the head with those guidelines
repeatedly, because I think the *changed* messages are in violation of
this, and need to be changed again...

//Magnus

On Mon, Sep 15, 2008 at 11:46 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Tom Lane wrote:
>>
>> Hm, that doesn't seem like a great improvement --- in particular, it
>> violates the style guideline that detail messages should be complete
>> sentences.
>>
>> I think the error text is all right as-is, really.
>
> I think I need to hit myself over the head with those guidelines
> repeatedly, because I think the *changed* messages are in violation of
> this, and need to be changed again...
>

Right, I was just applying the same DETAIL structure as was used
elsewhere in the patch. Tom's got a point about the complete
sentences though.

I still feel that the primary message is too long once the arguments
have been substituted in. How about just tweaking the DETAIL portion
so that it is a complete sentence?

Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?

Cheers,
BJ

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?

+1

regards, tom lane

Tom Lane wrote:
> "Brendan Jurd" <direvus(at)gmail(dot)com> writes:
>> Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?
>
> +1

Is this the proper syntax for errcontext() messags?

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?
>>
>> +1

> Is this the proper syntax for errcontext() messags?

Hmm, I think I'd suggest following the wording already in use in
ts_locale.c:

errcontext("line %d of configuration file \"%s\"",
stp->lineno,
stp->filename);

if only to save translation effort.

Also, in the last example, don't include the line number in the
errmsg. Otherwise it looks ok to me.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>> Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?
>>> +1
>
>> Is this the proper syntax for errcontext() messags?
>
> Hmm, I think I'd suggest following the wording already in use in
> ts_locale.c:
>
> errcontext("line %d of configuration file \"%s\"",
> stp->lineno,
> stp->filename);
>
> if only to save translation effort.
>
> Also, in the last example, don't include the line number in the
> errmsg. Otherwise it looks ok to me.

Updated and applied.

//Magnus