Thread: 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@hagander.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@hagander.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@hagander.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@commandprompt.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@hagander.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@commandprompt.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@commandprompt.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@users.sourceforge.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@users.sourceforge.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@hagander.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@sss.pgh.pa.us> writes: > Robert Treat <xzilla@users.sourceforge.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@hagander.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@hagander.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@hagander.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@hagander.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@commandprompt.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@hagander.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@hagander.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@hagander.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@hagander.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@hagander.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@hagander.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@hagander.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@hagander.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.comPostgreSQL 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@hagander.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@hagander.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@gmx.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@hagander.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@hagander.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@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@hagander.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@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@hagander.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@hagander.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@hagander.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@momjian.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@momjian.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
Index: src/backend/libpq/auth.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/auth.c,v
retrieving revision 1.167
diff -c -r1.167 auth.c
*** src/backend/libpq/auth.c 1 Aug 2008 11:41:12 -0000 1.167
--- src/backend/libpq/auth.c 16 Aug 2008 15:44:20 -0000
***************
*** 211,217 ****
if (status == STATUS_EOF)
proc_exit(0);
! switch (port->auth_method)
{
case uaReject:
errstr = gettext_noop("authentication failed for user \"%s\": host rejected");
--- 211,217 ----
if (status == STATUS_EOF)
proc_exit(0);
! switch (port->hba->auth_method)
{
case uaReject:
errstr = gettext_noop("authentication failed for user \"%s\": host rejected");
***************
*** 279,285 ****
errmsg("missing or erroneous pg_hba.conf file"),
errhint("See server log for details.")));
! switch (port->auth_method)
{
case uaReject:
--- 279,285 ----
errmsg("missing or erroneous pg_hba.conf file"),
errhint("See server log for details.")));
! switch (port->hba->auth_method)
{
case uaReject:
***************
*** 1761,1767 ****
/*
* Determine the username of the initiator of the connection described
* by "port". Then look in the usermap file under the usermap
! * port->auth_arg and see if that user is equivalent to Postgres user
* port->user.
*
* Return STATUS_OK if yes, STATUS_ERROR if no match (or couldn't get info).
--- 1761,1767 ----
/*
* Determine the username of the initiator of the connection described
* by "port". Then look in the usermap file under the usermap
! * port->hba->usermap and see if that user is equivalent to Postgres user
* port->user.
*
* Return STATUS_OK if yes, STATUS_ERROR if no match (or couldn't get info).
***************
*** 1799,1805 ****
(errmsg("Ident protocol identifies remote user as \"%s\"",
ident_user)));
! if (check_ident_usermap(port->auth_arg, port->user_name, ident_user))
return STATUS_OK;
else
return STATUS_ERROR;
--- 1799,1805 ----
(errmsg("Ident protocol identifies remote user as \"%s\"",
ident_user)));
! if (check_ident_usermap(port->hba->usermap, port->user_name, ident_user))
return STATUS_OK;
else
return STATUS_ERROR;
Index: src/backend/libpq/crypt.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/crypt.c,v
retrieving revision 1.74
diff -c -r1.74 crypt.c
*** src/backend/libpq/crypt.c 1 Jan 2008 19:45:49 -0000 1.74
--- src/backend/libpq/crypt.c 16 Aug 2008 15:44:20 -0000
***************
*** 54,60 ****
return STATUS_ERROR;
/* We can't do crypt with MD5 passwords */
! if (isMD5(shadow_pass) && port->auth_method == uaCrypt)
{
ereport(LOG,
(errmsg("cannot use authentication method \"crypt\" because password is MD5-encrypted")));
--- 54,60 ----
return STATUS_ERROR;
/* We can't do crypt with MD5 passwords */
! if (isMD5(shadow_pass) && port->hba->auth_method == uaCrypt)
{
ereport(LOG,
(errmsg("cannot use authentication method \"crypt\" because password is MD5-encrypted")));
***************
*** 65,71 ****
* Compare with the encrypted or plain password depending on the
* authentication method being used for this connection.
*/
! switch (port->auth_method)
{
case uaMD5:
crypt_pwd = palloc(MD5_PASSWD_LEN + 1);
--- 65,71 ----
* Compare with the encrypted or plain password depending on the
* authentication method being used for this connection.
*/
! switch (port->hba->auth_method)
{
case uaMD5:
crypt_pwd = palloc(MD5_PASSWD_LEN + 1);
***************
*** 155,161 ****
}
}
! if (port->auth_method == uaMD5)
pfree(crypt_pwd);
if (crypt_client_pass != client_pass)
pfree(crypt_client_pass);
--- 155,161 ----
}
}
! if (port->hba->auth_method == uaMD5)
pfree(crypt_pwd);
if (crypt_client_pass != client_pass)
pfree(crypt_client_pass);
Index: src/backend/libpq/hba.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/hba.c,v
retrieving revision 1.166
diff -c -r1.166 hba.c
*** src/backend/libpq/hba.c 1 Aug 2008 09:09:49 -0000 1.166
--- src/backend/libpq/hba.c 16 Aug 2008 15:44:20 -0000
***************
*** 41,48 ****
#define MAX_TOKEN 256
/*
! * These variables hold the pre-parsed contents of the hba and ident
* configuration files, as well as the flat auth file.
* Each is a list of sublists, one sublist for
* each (non-empty, non-comment) line of the file. Each sublist's
--- 41,51 ----
#define MAX_TOKEN 256
+ /* pre-parsed content of HBA config file */
+ static List *parsed_hba_lines = NIL;
+
/*
! * These variables hold the pre-parsed contents of the ident
* configuration files, as well as the flat auth file.
* Each is a list of sublists, one sublist for
* each (non-empty, non-comment) line of the file. Each sublist's
***************
*** 52,61 ****
* one token, since blank lines are not entered in the data structure.
*/
- /* pre-parsed content of HBA config file and corresponding line #s */
- static List *hba_lines = NIL;
- static List *hba_line_nums = NIL;
-
/* pre-parsed content of ident usermap file and corresponding line #s */
static List *ident_lines = NIL;
static List *ident_line_nums = NIL;
--- 55,60 ----
***************
*** 566,696 ****
/*
! * Scan the rest of a host record (after the mask field)
! * and return the interpretation of it as *userauth_p, *auth_arg_p, and
! * *error_p. *line_item points to the next token of the line, and is
! * advanced over successfully-read tokens.
*/
! static void
! parse_hba_auth(ListCell **line_item, UserAuth *userauth_p,
! char **auth_arg_p, bool *error_p)
! {
! char *token;
!
! *auth_arg_p = NULL;
!
! if (!*line_item)
! {
! *error_p = true;
! return;
! }
!
! token = lfirst(*line_item);
! if (strcmp(token, "trust") == 0)
! *userauth_p = uaTrust;
! else if (strcmp(token, "ident") == 0)
! *userauth_p = uaIdent;
! else if (strcmp(token, "password") == 0)
! *userauth_p = uaPassword;
! else if (strcmp(token, "krb5") == 0)
! *userauth_p = uaKrb5;
! else if (strcmp(token, "gss") == 0)
! *userauth_p = uaGSS;
! else if (strcmp(token, "sspi") == 0)
! *userauth_p = uaSSPI;
! else if (strcmp(token, "reject") == 0)
! *userauth_p = uaReject;
! else if (strcmp(token, "md5") == 0)
! *userauth_p = uaMD5;
! else if (strcmp(token, "crypt") == 0)
! *userauth_p = uaCrypt;
! #ifdef USE_PAM
! else if (strcmp(token, "pam") == 0)
! *userauth_p = uaPAM;
! #endif
! #ifdef USE_LDAP
! else if (strcmp(token, "ldap") == 0)
! *userauth_p = uaLDAP;
! #endif
! else
! {
! *error_p = true;
! return;
! }
! *line_item = lnext(*line_item);
!
! /* Get the authentication argument token, if any */
! if (*line_item)
! {
! token = lfirst(*line_item);
! *auth_arg_p = pstrdup(token);
! *line_item = lnext(*line_item);
! /* If there is more on the line, it is an error */
! if (*line_item)
! *error_p = true;
! }
! }
!
!
! /*
! * Process one line from the hba config file.
! *
! * See if it applies to a connection from a host with IP address port->raddr
! * to a database named port->database. If so, return *found_p true
! * and fill in the auth arguments into the appropriate port fields.
! * If not, leave *found_p as it was. If the record has a syntax error,
! * return *error_p true, after issuing a message to the log. If no error,
! * leave *error_p as it was.
! */
! static void
! parse_hba(List *line, int line_num, hbaPort *port,
! bool *found_p, bool *error_p)
{
char *token;
- char *db;
- char *role;
struct addrinfo *gai_result;
struct addrinfo hints;
int ret;
- struct sockaddr_storage addr;
- struct sockaddr_storage mask;
char *cidr_slash;
ListCell *line_item;
line_item = list_head(line);
/* Check the record type. */
token = lfirst(line_item);
if (strcmp(token, "local") == 0)
{
! /* Get the database. */
! line_item = lnext(line_item);
! if (!line_item)
! goto hba_syntax;
! db = lfirst(line_item);
!
! /* Get the role. */
! line_item = lnext(line_item);
! if (!line_item)
! goto hba_syntax;
! role = lfirst(line_item);
!
! line_item = lnext(line_item);
! if (!line_item)
! goto hba_syntax;
!
! /* Read the rest of the line. */
! parse_hba_auth(&line_item, &port->auth_method,
! &port->auth_arg, error_p);
! if (*error_p)
! goto hba_syntax;
!
! /* Disallow auth methods that always need TCP/IP sockets to work */
! if (port->auth_method == uaKrb5)
! goto hba_syntax;
!
! /* Does not match if connection isn't AF_UNIX */
! if (!IS_AF_UNIX(port->raddr.addr.ss_family))
! return;
}
else if (strcmp(token, "host") == 0
|| strcmp(token, "hostssl") == 0
--- 565,593 ----
/*
! * Parse one line in the hba config file and store the result in
! * a HbaLine structure.
*/
! static bool
! parse_hba_line(List *line, int line_num, HbaLine *parsedline)
{
char *token;
struct addrinfo *gai_result;
struct addrinfo hints;
int ret;
char *cidr_slash;
+ char *unsupauth;
ListCell *line_item;
line_item = list_head(line);
+
+ parsedline->linenumber = line_num;
+
/* Check the record type. */
token = lfirst(line_item);
if (strcmp(token, "local") == 0)
{
! parsedline->conntype = ctLocal;
}
else if (strcmp(token, "host") == 0
|| strcmp(token, "hostssl") == 0
***************
*** 700,713 ****
if (token[4] == 's') /* "hostssl" */
{
#ifdef USE_SSL
! /* Record does not match if we are not on an SSL connection */
! if (!port->ssl)
! return;
!
! /* Placeholder to require specific SSL level, perhaps? */
! /* Or a client certificate */
!
! /* Since we were on SSL, proceed as with normal 'host' mode */
#else
/* We don't accept this keyword at all if no SSL support */
goto hba_syntax;
--- 597,603 ----
if (token[4] == 's') /* "hostssl" */
{
#ifdef USE_SSL
! parsedline->conntype = ctHostSSL;
#else
/* We don't accept this keyword at all if no SSL support */
goto hba_syntax;
***************
*** 716,744 ****
#ifdef USE_SSL
else if (token[4] == 'n') /* "hostnossl" */
{
! /* Record does not match if we are on an SSL connection */
! if (port->ssl)
! return;
}
#endif
! /* Get the database. */
! line_item = lnext(line_item);
! if (!line_item)
! goto hba_syntax;
! db = lfirst(line_item);
! /* Get the role. */
! line_item = lnext(line_item);
! if (!line_item)
! goto hba_syntax;
! role = lfirst(line_item);
/* Read the IP address field. (with or without CIDR netmask) */
line_item = lnext(line_item);
if (!line_item)
goto hba_syntax;
! token = lfirst(line_item);
/* Check if it has a CIDR suffix and if so isolate it */
cidr_slash = strchr(token, '/');
--- 606,642 ----
#ifdef USE_SSL
else if (token[4] == 'n') /* "hostnossl" */
{
! parsedline->conntype = ctHostNoSSL;
}
#endif
+ else
+ {
+ /* "host", or "hostnossl" and SSL support not built in */
+ parsedline->conntype = ctHost;
+ }
+ } /* record type */
+ else
+ goto hba_syntax;
! /* Get the database. */
! line_item = lnext(line_item);
! if (!line_item)
! goto hba_syntax;
! parsedline->database = pstrdup(lfirst(line_item));
! /* Get the role. */
! line_item = lnext(line_item);
! if (!line_item)
! goto hba_syntax;
! parsedline->role = pstrdup(lfirst(line_item));
+ if (parsedline->conntype != ctLocal)
+ {
/* Read the IP address field. (with or without CIDR netmask) */
line_item = lnext(line_item);
if (!line_item)
goto hba_syntax;
! token = pstrdup(lfirst(line_item));
/* Check if it has a CIDR suffix and if so isolate it */
cidr_slash = strchr(token, '/');
***************
*** 760,768 ****
{
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("invalid IP address \"%s\" in file \"%s\" line %d: %s",
! token, HbaFileName, line_num,
! gai_strerror(ret))));
if (cidr_slash)
*cidr_slash = '/';
if (gai_result)
--- 658,666 ----
{
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("invalid IP address \"%s\" in file \"%s\" line %d: %s",
! token, HbaFileName, line_num,
! gai_strerror(ret))));
if (cidr_slash)
*cidr_slash = '/';
if (gai_result)
***************
*** 773,786 ****
if (cidr_slash)
*cidr_slash = '/';
! memcpy(&addr, gai_result->ai_addr, gai_result->ai_addrlen);
pg_freeaddrinfo_all(hints.ai_family, gai_result);
/* Get the netmask */
if (cidr_slash)
{
! if (pg_sockaddr_cidr_mask(&mask, cidr_slash + 1,
! addr.ss_family) < 0)
goto hba_syntax;
}
else
--- 671,684 ----
if (cidr_slash)
*cidr_slash = '/';
! memcpy(&parsedline->addr, gai_result->ai_addr, gai_result->ai_addrlen);
pg_freeaddrinfo_all(hints.ai_family, gai_result);
/* Get the netmask */
if (cidr_slash)
{
! if (pg_sockaddr_cidr_mask(&parsedline->mask, cidr_slash + 1,
! parsedline->addr.ss_family) < 0)
goto hba_syntax;
}
else
***************
*** 796,813 ****
{
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("invalid IP mask \"%s\" in file \"%s\" line %d: %s",
! token, HbaFileName, line_num,
! gai_strerror(ret))));
if (gai_result)
pg_freeaddrinfo_all(hints.ai_family, gai_result);
goto hba_other_error;
}
! memcpy(&mask, gai_result->ai_addr, gai_result->ai_addrlen);
pg_freeaddrinfo_all(hints.ai_family, gai_result);
! if (addr.ss_family != mask.ss_family)
{
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
--- 694,711 ----
{
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("invalid IP mask \"%s\" in file \"%s\" line %d: %s",
! token, HbaFileName, line_num,
! gai_strerror(ret))));
if (gai_result)
pg_freeaddrinfo_all(hints.ai_family, gai_result);
goto hba_other_error;
}
! memcpy(&parsedline->mask, gai_result->ai_addr, gai_result->ai_addrlen);
pg_freeaddrinfo_all(hints.ai_family, gai_result);
! if (parsedline->addr.ss_family != parsedline->mask.ss_family)
{
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
***************
*** 816,877 ****
goto hba_other_error;
}
}
! if (addr.ss_family != port->raddr.addr.ss_family)
{
! /*
! * Wrong address family. We allow only one case: if the file has
! * IPv4 and the port is IPv6, promote the file address to IPv6 and
! * try to match that way.
! */
! #ifdef HAVE_IPV6
! if (addr.ss_family == AF_INET &&
! port->raddr.addr.ss_family == AF_INET6)
{
! pg_promote_v4_to_v6_addr(&addr);
! pg_promote_v4_to_v6_mask(&mask);
}
else
- #endif /* HAVE_IPV6 */
{
! /* Line doesn't match client port, so ignore it. */
! return;
}
}
-
- /* Ignore line if client port is not in the matching addr range. */
- if (!pg_range_sockaddr(&port->raddr.addr, &addr, &mask))
- return;
-
- /* Read the rest of the line. */
- line_item = lnext(line_item);
- if (!line_item)
- goto hba_syntax;
- parse_hba_auth(&line_item, &port->auth_method,
- &port->auth_arg, error_p);
- if (*error_p)
- goto hba_syntax;
}
! else
! goto hba_syntax;
!
! /* Does the entry match database and role? */
! if (!check_db(port->database_name, port->user_name, db))
! return;
! if (!check_role(port->user_name, role))
! return;
!
! /* Success */
! *found_p = true;
! return;
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))));
else
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
--- 714,841 ----
goto hba_other_error;
}
}
+ } /* != ctLocal */
+
+ /* Get the authentication method */
+ line_item = lnext(line_item);
+ if (!line_item)
+ goto hba_syntax;
+ token = lfirst(line_item);
+
+ unsupauth = NULL;
+ if (strcmp(token, "trust") == 0)
+ parsedline->auth_method = uaTrust;
+ else if (strcmp(token, "ident") == 0)
+ parsedline->auth_method = uaIdent;
+ else if (strcmp(token, "password") == 0)
+ parsedline->auth_method = uaPassword;
+ else if (strcmp(token, "krb5") == 0)
+ #ifdef KRB5
+ parsedline->auth_method = uaKrb5;
+ #else
+ unsupauth = "krb5";
+ #endif
+ else if (strcmp(token, "gss") == 0)
+ #ifdef ENABLE_GSS
+ parsedline->auth_method = uaGSS;
+ #else
+ unsupauth = "gss";
+ #endif
+ else if (strcmp(token, "sspi") == 0)
+ #ifdef ENABLE_SSPI
+ parsedline->auth_method = uaSSPI;
+ #else
+ unsupauth = "sspi";
+ #endif
+ else if (strcmp(token, "reject") == 0)
+ parsedline->auth_method = uaReject;
+ else if (strcmp(token, "md5") == 0)
+ parsedline->auth_method = uaMD5;
+ else if (strcmp(token, "crypt") == 0)
+ parsedline->auth_method = uaCrypt;
+ else if (strcmp(token, "pam") == 0)
+ #ifdef USE_PAM
+ parsedline->auth_method = uaPAM;
+ #else
+ unsupauth = "pam";
+ #endif
+ else if (strcmp(token, "ldap") == 0)
+ #ifdef USE_LDAP
+ parsedline->auth_method = uaLDAP;
+ #else
+ unsupauth = "ldap";
+ #endif
+ else
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid authentication method \"%s\" in file \"%s\" line %d",
+ token, HbaFileName, line_num)));
+ goto hba_other_error;
+ }
+
+ if (unsupauth)
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("invalid authentication method \"%s\" in file \"%s\" line %d: not supported on this
platform",
+ token, HbaFileName, line_num)));
+ goto hba_other_error;
+ }
+
+ /* Invalid authentication combinations */
+ if (parsedline->conntype == ctLocal &&
+ parsedline->auth_method == uaKrb5)
+ {
+ ereport(LOG,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("krb5 authentication is not supported on local sockets in file \"%s\" line %d",
+ HbaFileName, line_num)));
+ goto hba_other_error;
+ }
+
+ /* Get the authentication argument token, if any */
+ line_item = lnext(line_item);
+ if (line_item)
+ {
+ token = lfirst(line_item);
+ parsedline->auth_arg= pstrdup(token);
+ }
! /*
! * backwards compatible format of ident authentication - support "naked" ident map
! * name, as well as "sameuser"/"samerole"
! */
! if (parsedline->auth_method == uaIdent)
! {
! if (parsedline->auth_arg && strlen(parsedline->auth_arg))
{
! if (strcmp(parsedline->auth_arg, "sameuser\n") == 0 ||
! strcmp(parsedline->auth_arg, "samerole\n") == 0)
{
! /* This is now the default */
! pfree(parsedline->auth_arg);
! parsedline->auth_arg = NULL;
! parsedline->usermap = NULL;
}
else
{
! /* Specific ident map specified */
! parsedline->usermap = parsedline->auth_arg;
! parsedline->auth_arg = NULL;
}
}
}
!
! return true;
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))));
else
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
***************
*** 880,886 ****
/* Come here if suitable message already logged */
hba_other_error:
! *error_p = true;
}
--- 844,850 ----
/* Come here if suitable message already logged */
hba_other_error:
! return false;
}
***************
*** 891,918 ****
static bool
check_hba(hbaPort *port)
{
- bool found_entry = false;
- bool error = false;
ListCell *line;
! ListCell *line_num;
! forboth(line, hba_lines, line_num, hba_line_nums)
{
! parse_hba(lfirst(line), lfirst_int(line_num),
! port, &found_entry, &error);
! if (found_entry || error)
! break;
! }
! if (!error)
! {
! /* If no matching entry was found, synthesize 'reject' entry. */
! if (!found_entry)
! port->auth_method = uaReject;
return true;
}
! else
! return false;
}
--- 855,950 ----
static bool
check_hba(hbaPort *port)
{
ListCell *line;
! HbaLine *hba;
! foreach(line, parsed_hba_lines)
{
! hba = (HbaLine *) lfirst(line);
! /* Check connection type */
! if (hba->conntype == ctLocal)
! {
! if (!IS_AF_UNIX(port->raddr.addr.ss_family))
! continue;
! }
! else
! {
! if (IS_AF_UNIX(port->raddr.addr.ss_family))
! continue;
!
! /* Check SSL state */
! #ifdef USE_SSL
! if (port->ssl)
! {
! /* Connection is SSL, match both "host" and "hostssl" */
! if (hba->conntype == ctHostNoSSL)
! continue;
! }
! else
! {
! /* Connection is not SSL, match both "host" and "hostnossl" */
! if (hba->conntype == ctHostSSL)
! continue;
! }
! #else
! /* No SSL support, so reject "hostssl" lines */
! if (hba->conntype == ctHostSSL)
! continue;
! #endif
!
! /* Check IP address */
! if (port->raddr.addr.ss_family == hba->addr.ss_family)
! {
! if (!pg_range_sockaddr(&port->raddr.addr, &hba->addr, &hba->mask))
! continue;
! }
! #ifdef HAVE_IPV6
! else if (hba->addr.ss_family == AF_INET &&
! port->raddr.addr.ss_family == AF_INET6)
! {
! /*
! * Wrong address family. We allow only one case: if the file has
! * IPv4 and the port is IPv6, promote the file address to IPv6 and
! * try to match that way.
! */
! struct sockaddr_storage addrcopy, maskcopy;
! memcpy(&addrcopy, &hba->addr, sizeof(addrcopy));
! memcpy(&maskcopy, &hba->mask, sizeof(maskcopy));
! pg_promote_v4_to_v6_addr(&addrcopy);
! pg_promote_v4_to_v6_mask(&maskcopy);
!
! if (!pg_range_sockaddr(&port->raddr.addr, &addrcopy, &maskcopy))
! continue;
! }
! #endif /* HAVE_IPV6 */
! else
! /* Wrong address family, no IPV6 */
! continue;
! } /* != ctLocal */
!
! /* Check database and role */
! if (!check_db(port->database_name, port->user_name, hba->database))
! continue;
!
! if (!check_role(port->user_name, hba->role))
! continue;
!
! /* Found a record that matched! */
! port->hba = hba;
return true;
}
!
! /* If no matching entry was found, synthesize 'reject' entry. */
! hba = palloc0(sizeof(HbaLine));
! hba->auth_method = uaReject;
! port->hba = hba;
! return true;
!
! /* XXX:
! * Return false only happens if we have a parsing error, which we can
! * no longer have (parsing now in postmaster). Consider changing API.
! */
}
***************
*** 967,983 ****
}
}
/*
! * Read the config file and create a List of Lists of tokens in the file.
*/
! void
load_hba(void)
{
FILE *file;
!
! if (hba_lines || hba_line_nums)
! free_lines(&hba_lines, &hba_line_nums);
file = AllocateFile(HbaFileName, "r");
/* Failure is fatal since with no HBA entries we can do nothing... */
--- 999,1050 ----
}
}
+ /*
+ * Free the contents of a hba record
+ */
+ static void
+ free_hba_record(HbaLine *record)
+ {
+ if (record->database)
+ pfree(record->database);
+ if (record->role)
+ pfree(record->role);
+ if (record->auth_arg)
+ pfree(record->auth_arg);
+ }
/*
! * Free all records on the parsed HBA list
*/
! static void
! clean_hba_list(List *lines)
! {
! ListCell *line;
!
! foreach(line, lines)
! {
! HbaLine *parsed = (HbaLine *)lfirst(line);
! if (parsed)
! free_hba_record(parsed);
! }
! list_free(lines);
! }
!
! /*
! * Read the config file and create a List of HbaLine records for the contents.
! *
! * The configuration is read into a temporary list, and if any parse error occurs
! * the old list is kept in place and false is returned. Only if the whole file
! * parses Ok is the list replaced, and the function returns true.
! */
! bool
load_hba(void)
{
FILE *file;
! List *hba_lines = NIL;
! List *hba_line_nums = NIL;
! ListCell *line, *line_num;
! List *new_parsed_lines = NIL;
file = AllocateFile(HbaFileName, "r");
/* Failure is fatal since with no HBA entries we can do nothing... */
***************
*** 989,994 ****
--- 1056,1090 ----
tokenize_file(HbaFileName, file, &hba_lines, &hba_line_nums);
FreeFile(file);
+
+ /* Now parse all the lines */
+ forboth(line, hba_lines, line_num, hba_line_nums)
+ {
+ HbaLine *newline;
+
+ newline = palloc0(sizeof(HbaLine));
+
+ if (!parse_hba_line(lfirst(line), lfirst_int(line_num), newline))
+ {
+ /* Parse error in the file, so bail out */
+ free_hba_record(newline);
+ pfree(newline);
+ clean_hba_list(new_parsed_lines);
+ /* Error has already been reported in the parsing function */
+ return false;
+ }
+
+ new_parsed_lines = lappend(new_parsed_lines, newline);
+ }
+
+ /* Loaded new file successfully, replace the one we use */
+ clean_hba_list(parsed_hba_lines);
+ parsed_hba_lines = new_parsed_lines;
+
+ /* Free the temporary lists */
+ free_lines(&hba_lines, &hba_line_nums);
+
+ return true;
}
/*
***************
*** 1100,1106 ****
* See if the user with ident username "ident_user" is allowed to act
* as Postgres user "pgrole" according to usermap "usermap_name".
*
! * Special case: For usermap "samerole", don't look in the usermap
* file. That's an implied map where "pgrole" must be identical to
* "ident_user" in order to be authorized.
*
--- 1196,1203 ----
* See if the user with ident username "ident_user" is allowed to act
* as Postgres user "pgrole" according to usermap "usermap_name".
*
! * Special case: Usermap NULL, equivalent to what was previously called
! * "sameuser" or "samerole", don't look in the usermap
* file. That's an implied map where "pgrole" must be identical to
* "ident_user" in order to be authorized.
*
***************
*** 1116,1129 ****
if (usermap_name == NULL || usermap_name[0] == '\0')
{
- ereport(LOG,
- (errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("cannot use Ident authentication without usermap field")));
- found_entry = false;
- }
- else if (strcmp(usermap_name, "sameuser\n") == 0 ||
- strcmp(usermap_name, "samerole\n") == 0)
- {
if (strcmp(pg_role, ident_user) == 0)
found_entry = true;
else
--- 1213,1218 ----
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.561
diff -c -r1.561 postmaster.c
*** src/backend/postmaster/postmaster.c 26 Jun 2008 02:47:19 -0000 1.561
--- src/backend/postmaster/postmaster.c 16 Aug 2008 15:44:20 -0000
***************
*** 888,894 ****
/*
* Load configuration files for client authentication.
*/
! load_hba();
load_ident();
/*
--- 888,902 ----
/*
* Load configuration files for client authentication.
*/
! if (!load_hba())
! {
! /*
! * It makes no sense continue if we fail to load the HBA file, since
! * there is no way to connect to the database in this case.
! */
! ereport(FATAL,
! (errmsg("could not load pg_hba.conf")));
! }
load_ident();
/*
***************
*** 1926,1932 ****
/* PgStatPID does not currently need SIGHUP */
/* Reload authentication config files too */
! load_hba();
load_ident();
#ifdef EXEC_BACKEND
--- 1934,1943 ----
/* PgStatPID does not currently need SIGHUP */
/* Reload authentication config files too */
! if (!load_hba())
! ereport(WARNING,
! (errmsg("pg_hba.conf not reloaded")));
!
load_ident();
#ifdef EXEC_BACKEND
***************
*** 3080,3086 ****
ALLOCSET_DEFAULT_MAXSIZE);
MemoryContextSwitchTo(PostmasterContext);
! load_hba();
load_ident();
load_role();
#endif
--- 3091,3105 ----
ALLOCSET_DEFAULT_MAXSIZE);
MemoryContextSwitchTo(PostmasterContext);
! if (!load_hba())
! {
! /*
! * It makes no sense continue if we fail to load the HBA file, since
! * there is no way to connect to the database in this case.
! */
! ereport(FATAL,
! (errmsg("could not load pg_hba.conf")));
! }
load_ident();
load_role();
#endif
Index: src/include/libpq/hba.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/libpq/hba.h,v
retrieving revision 1.48
diff -c -r1.48 hba.h
*** src/include/libpq/hba.h 1 Aug 2008 09:09:48 -0000 1.48
--- src/include/libpq/hba.h 16 Aug 2008 15:44:20 -0000
***************
*** 33,42 ****
#endif
} UserAuth;
typedef struct Port hbaPort;
extern List **get_role_line(const char *role);
! extern void load_hba(void);
extern void load_ident(void);
extern void load_role(void);
extern int hba_getauthmethod(hbaPort *port);
--- 33,63 ----
#endif
} UserAuth;
+ typedef enum ConnType
+ {
+ ctLocal,
+ ctHost,
+ ctHostSSL,
+ ctHostNoSSL
+ } ConnType;
+
+ typedef struct
+ {
+ int linenumber;
+ ConnType conntype;
+ char *database;
+ char *role;
+ struct sockaddr_storage addr;
+ struct sockaddr_storage mask;
+ UserAuth auth_method;
+ char *usermap;
+ char *auth_arg;
+ } HbaLine;
+
typedef struct Port hbaPort;
extern List **get_role_line(const char *role);
! extern bool load_hba(void);
extern void load_ident(void);
extern void load_role(void);
extern int hba_getauthmethod(hbaPort *port);
Index: src/include/libpq/libpq-be.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/libpq/libpq-be.h,v
retrieving revision 1.66
diff -c -r1.66 libpq-be.h
*** src/include/libpq/libpq-be.h 26 Apr 2008 22:47:40 -0000 1.66
--- src/include/libpq/libpq-be.h 16 Aug 2008 15:44:20 -0000
***************
*** 121,128 ****
/*
* Information that needs to be held during the authentication cycle.
*/
! UserAuth auth_method;
! char *auth_arg;
char md5Salt[4]; /* Password salt */
char cryptSalt[2]; /* Password salt */
--- 121,127 ----
/*
* Information that needs to be held during the authentication cycle.
*/
! HbaLine *hba;
char md5Salt[4]; /* Password salt */
char cryptSalt[2]; /* Password salt */
Magnus Hagander <magnus@hagander.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@hagander.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@hagander.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@gmail.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@druid.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@gmail.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@hagander.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@gmail.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@gmail.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@hagander.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@gmail.com> writes:
> Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?
+1
regards, tom lane
Tom Lane wrote:
> "Brendan Jurd" <direvus@gmail.com> writes:
>> Or perhaps the DETAIL portion would be more appropriate as a CONTEXT?
>
> +1
Is this the proper syntax for errcontext() messags?
//Magnus
Index: hba.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/hba.c,v
retrieving revision 1.167
diff -c -r1.167 hba.c
*** hba.c 15 Sep 2008 12:32:56 -0000 1.167
--- hba.c 15 Sep 2008 17:38:01 -0000
***************
*** 660,666 ****
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("invalid IP address \"%s\": %s",
token, gai_strerror(ret)),
! errdetail("In file \"%s\", line %d",
HbaFileName, line_num)));
if (cidr_slash)
*cidr_slash = '/';
--- 660,666 ----
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("invalid IP address \"%s\": %s",
token, gai_strerror(ret)),
! errcontext("File \"%s\", line %d",
HbaFileName, line_num)));
if (cidr_slash)
*cidr_slash = '/';
***************
*** 697,703 ****
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("invalid IP mask \"%s\": %s",
token, gai_strerror(ret)),
! errdetail("In file \"%s\", line %d",
HbaFileName, line_num)));
if (gai_result)
pg_freeaddrinfo_all(hints.ai_family, gai_result);
--- 697,703 ----
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("invalid IP mask \"%s\": %s",
token, gai_strerror(ret)),
! errcontext("File \"%s\", line %d",
HbaFileName, line_num)));
if (gai_result)
pg_freeaddrinfo_all(hints.ai_family, gai_result);
***************
*** 773,779 ****
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("invalid authentication method \"%s\"",
token),
! errdetail("In file \"%s\" line %d",
HbaFileName, line_num)));
goto hba_other_error;
}
--- 773,779 ----
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("invalid authentication method \"%s\"",
token),
! errcontext("File \"%s\" line %d",
HbaFileName, line_num)));
goto hba_other_error;
}
***************
*** 784,790 ****
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("invalid authentication method \"%s\": not supported on this platform",
token),
! errdetail("In file \"%s\" line %d",
HbaFileName, line_num)));
goto hba_other_error;
}
--- 784,790 ----
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("invalid authentication method \"%s\": not supported on this platform",
token),
! errcontext("File \"%s\" line %d",
HbaFileName, line_num)));
goto hba_other_error;
}
***************
*** 796,802 ****
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("krb5 authentication is not supported on local sockets"),
! errdetail("In file \"%s\" line %d",
HbaFileName, line_num)));
goto hba_other_error;
}
--- 796,802 ----
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("krb5 authentication is not supported on local sockets"),
! errcontext("File \"%s\" line %d",
HbaFileName, line_num)));
goto hba_other_error;
}
***************
*** 840,852 ****
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))));
else
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("missing field in file \"%s\" at end of line %d",
HbaFileName, line_num)));
/* Come here if suitable message already logged */
--- 840,855 ----
if (line_item)
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("invalid token \"%s\"",
! (char *) lfirst(line_item)),
! errcontext("File %s, line %d",
! HbaFileName, line_num)));
else
ereport(LOG,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
! errmsg("missing field at end of line %d",
! line_num),
! errcontext("File %s, line %d",
HbaFileName, line_num)));
/* Come here if suitable message already logged */
Magnus Hagander <magnus@hagander.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@hagander.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