Re: pg_hba.conf: samehost and samenet

I love using postgresql, and have for a long time. I'm involved with
almost a hundred postgresql installs. But this is the first time I've
gotten into the code.

Renumbering networks happens often, and will happen more frequently as
IPv4 space runs low. The IP based restrictions in pg_hba.conf is one of
the places where renumbering can break running installs. In addition
when postgresql is run in BSD jails, 127.0.0.1 is not available for use
in pg_hba.conf.

It would be great if, in the cidr-address field of pg_hba.conf, we could
specify "samehost" and "samenet". These special values use the local
hosts network interface addresses. "samehost" allows an IP assigned to
the local machine. "samenet" allows any host on the subnets connected to
the local machine.

This is similar to the "sameuser" value that's allowed in the database
field.

A change like this would enable admins like myself to distribute
postgresql with something like this in the default pg_hba.conf file:

host all all samenet md5
hostssl all all 0.0.0.0/0 md5

I've attached an initial patch which implements "samehost" and
"samenet". The patch looks more invasive than it really is, due to
necessary indentation change (ie: a if block), and moving some code into
a separate function.

Thanks for your time. How can I help get a feature like this into
postgresql?

Cheers,

Stef

On Fri, Aug 14, 2009 at 00:50, Stef Walter<stef-list(at)memberwebs(dot)com> wrote:
> I love using postgresql, and have for a long time. I'm involved with
> almost a hundred postgresql installs. But this is the first time I've
> gotten into the code.
>
> Renumbering networks happens often, and will happen more frequently as
> IPv4 space runs low. The IP based restrictions in pg_hba.conf is one of
> the places where renumbering can break running installs. In addition
> when postgresql is run in BSD jails, 127.0.0.1 is not available for use
> in pg_hba.conf.
>
> It would be great if, in the cidr-address field of pg_hba.conf, we could
> specify "samehost" and "samenet". These special values use the local
> hosts network interface addresses. "samehost" allows an IP assigned to
> the local machine. "samenet" allows any host on the subnets connected to
> the local machine.
>
> This is similar to the "sameuser" value that's allowed in the database
> field.
>
> A change like this would enable admins like myself to distribute
> postgresql with something like this in the default pg_hba.conf file:
>
> host      all     all   samenet         md5
> hostssl   all     all   0.0.0.0/0       md5

Seems like a reasonable feature - especially the samehost part.

> I've attached an initial patch which implements "samehost" and
> "samenet". The patch looks more invasive than it really is, due to
> necessary indentation change (ie: a if block), and moving some code into
> a separate function.

A couple of comments on the patch:

* In general, don't include configure in the patch. Just configure.in.
Makes it easier to read, and configure is normally built by the
committer anyway.

* How portable is this? For starters is clearly doesn't do Windows,
which would need to be investigated for similar functionality, but how
many others support getifaddr()? From what I can tell it's not in
POSIX, at least.

* The checks for "not supported" should happen at parsing time, not at runtime.

* It needs to include documentation changes

I haven't looked at the guts of the patch yet, those are just a couple
of first questions.

> Thanks for your time. How can I help get a feature like this into
> postgresql?

Please add it to the open commitfest
(https://commitfest.postgresql.org/action/commitfest_view/open). This
will cause it to be reviewed during the next commitfest, and then you
just need to be around to answer any questions that reviewers come up
with :-)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Fri, Aug 14, 2009 at 00:50, Stef Walter<stef-list(at)memberwebs(dot)com> wrote:
>> It would be great if, in the cidr-address field of pg_hba.conf, we could
>> specify "samehost" and "samenet".

> Seems like a reasonable feature - especially the samehost part.

ISTM people have traditionally used 127.0.0.1 and ::1 for "samehost"
behavior. What's being suggested here is a tad more flexible but
hardly a huge advance. As for "samenet", personally I'd be scared to
death of something like that --- who knows how wide the OS will
think your "net" is? (Think cable modem users on 10.x.x.x ...)
Using samenet in a conf file that's being handed out to random users
seems impossibly dangerous.

However, I wouldn't object too much if it weren't for this:

> * How portable is this? For starters is clearly doesn't do Windows,
> which would need to be investigated for similar functionality, but how
> many others support getifaddr()? From what I can tell it's not in
> POSIX, at least.

I don't see it on HPUX, for one. Unless a portable solution can be
found I don't think we can consider this. We're not in the habit
of exposing significant functionality that's only available on some
platforms.

regards, tom lane

Magnus Hagander wrote:
>
> A couple of comments on the patch:

Thanks I'll keep these in mind, as things progress and for future patches.

> * In general, don't include configure in the patch. Just configure.in.
> Makes it easier to read, and configure is normally built by the
> committer anyway.
>
> * How portable is this? For starters is clearly doesn't do Windows,
> which would need to be investigated for similar functionality, but how
> many others support getifaddr()? From what I can tell it's not in
> POSIX, at least.

I'll look further into this, and about compat shims. getifaddrs() is on
the BSDs, Linux and Mac OS.

> Please add it to the open commitfest
> (https://commitfest.postgresql.org/action/commitfest_view/open). This
> will cause it to be reviewed during the next commitfest, and then you
> just need to be around to answer any questions that reviewers come up
> with :-)

Cool, I'll do that once we've worked out the kinks here. Is the right
way to go about it?

Cheers,

Stef

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Fri, Aug 14, 2009 at 00:50, Stef Walter<stef-list(at)memberwebs(dot)com> wrote:
>>> It would be great if, in the cidr-address field of pg_hba.conf, we could
>>> specify "samehost" and "samenet".
>
>> Seems like a reasonable feature - especially the samehost part.
>
> ISTM people have traditionally used 127.0.0.1 and ::1 for "samehost"
> behavior.

Yes for sure. As noted in the original email 127.0.0.1 doesn't work as
you would expect in BSD jails. As it currently stands, you have to put
the local IP address to achieve similar access control. This causes
major pains when renumbering or dealing with postgresql hosted in large
amounts of jails.

Another way we could sort of get around most of these renumbering
problems, is by the ability to include host names in pg_hba.conf, rather
than IP addresses. I first set out to implement this, but as advised in
"How to Contribute" looked around the mailing lists for previous
discussion on the topic and found this:

http://archives.postgresql.org/pgsql-hackers/2008-06/msg00569.php

There seems to be no consensus in the postgresql community about this
feature, and its implementation. The last guy who tried to work on it
got scared away, and so I decided to try an approach that might be more
palatable.

I'm willing to put in the work on either approach, and I could revive
discussion about host names in pg_hba.conf if that's more desirable.

What's being suggested here is a tad more flexible but
> hardly a huge advance. As for "samenet", personally I'd be scared to
> death of something like that --- who knows how wide the OS will
> think your "net" is? (Think cable modem users on 10.x.x.x ...)
> Using samenet in a conf file that's being handed out to random users
> seems impossibly dangerous.

I understand what you're saying. In this case it would be handed out to
hosted clients and those sorts of users. ie: a controlled environment.
Obviously this wouldn't go into the default postgresql pg_hba.conf.

> However, I wouldn't object too much if it weren't for this:
>
>> * How portable is this? For starters is clearly doesn't do Windows,
>> which would need to be investigated for similar functionality, but how
>> many others support getifaddr()? From what I can tell it's not in
>> POSIX, at least.
>
> I don't see it on HPUX, for one. Unless a portable solution can be
> found I don't think we can consider this. We're not in the habit
> of exposing significant functionality that's only available on some
> platforms.

True. I could build compatibility getifaddrs for various systems, if the
community thought this patch was worth it, and would otherwise accept
the patch.

Cheers,

Stef

Stef Walter <stef-list(at)memberwebs(dot)com> writes:
> True. I could build compatibility getifaddrs for various systems, if the
> community thought this patch was worth it, and would otherwise accept
> the patch.

If you can do that I think it'd remove the major objection.

regards, tom lane

Attached is a new patch, which I hope addresses all the concerns raised.

Magnus Hagander wrote:
>> I've attached an initial patch which implements "samehost" and
>> "samenet". The patch looks more invasive than it really is, due to
>> necessary indentation change (ie: a if block), and moving some code into
>> a separate function.
>
> A couple of comments on the patch:
>
> * In general, don't include configure in the patch. Just configure.in.
> Makes it easier to read, and configure is normally built by the
> committer anyway.

Removed configure and pg_config.h.in from the patch.

> * How portable is this? For starters is clearly doesn't do Windows,
> which would need to be investigated for similar functionality, but how
> many others support getifaddr()? From what I can tell it's not in
> POSIX, at least.

getifaddr() is at least supported on *BSD, Linux and AIX.

In the new patch, I've added support for other unixes like Solaris,
HPUX, IRIC, SCO Unix (using the SIOCGIFCONF ioctl). Also included is
Win32 support (using winsock's SIO_GET_INTERFACE_LIST).

Obviously I don't have all of the above proprietary unixes to test on,
but I've studied documentation, and I believe that the code in the patch
will run on those systems.

> * It needs to include documentation changes

Done.

> Please add it to the open commitfest
> (https://commitfest.postgresql.org/action/commitfest_view/open). This
> will cause it to be reviewed during the next commitfest, and then you
> just need to be around to answer any questions that reviewers come up
> with :-)

I need some sort of a login to add a patch to the commit fest. Is that
something I can get? Or is there someone I should ask to add my patch to
the commit fest? I hope I'm not being dense and missing something
obvious. :)

Cheers,

Stef

Stef Walter <stef-list(at)memberwebs(dot)com> writes:
> Magnus Hagander wrote:
>> Please add it to the open commitfest
>> (https://commitfest.postgresql.org/action/commitfest_view/open). This
>> will cause it to be reviewed during the next commitfest, and then you
>> just need to be around to answer any questions that reviewers come up
>> with :-)

> I need some sort of a login to add a patch to the commit fest. Is that
> something I can get?

Go here:
http://www.postgresql.org/community/signup

The login page for the commitfest app really ought to provide a link
to that ... Robert?

regards, tom lane

On Tue, Aug 18, 2009 at 10:11 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Stef Walter <stef-list(at)memberwebs(dot)com> writes:
>> Magnus Hagander wrote:
>>> Please add it to the open commitfest
>>> (https://commitfest.postgresql.org/action/commitfest_view/open). This
>>> will cause it to be reviewed during the next commitfest, and then you
>>> just need to be around to answer any questions that reviewers come up
>>> with :-)
>
>> I need some sort of a login to add a patch to the commit fest. Is that
>> something I can get?
>
> Go here:
> http://www.postgresql.org/community/signup
>
> The login page for the commitfest app really ought to provide a link
> to that ... Robert?

Done.

...Robert

On Wed, Aug 19, 2009 at 03:58, Stef Walter<stef-list(at)memberwebs(dot)com> wrote:
> Attached is a new patch, which I hope addresses all the concerns raised.

I think you forgot to actually attach the patch....

(others have taken care of the question about login already I see)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander wrote:
> On Wed, Aug 19, 2009 at 03:58, Stef Walter<stef-list(at)memberwebs(dot)com> wrote:
>> Attached is a new patch, which I hope addresses all the concerns raised.
>
> I think you forgot to actually attach the patch....

Whoops. Here it is.

Stef

On Wed, Aug 19, 2009 at 15:02, Stef Walter<stef-list(at)memberwebs(dot)com> wrote:
> Magnus Hagander wrote:
>> On Wed, Aug 19, 2009 at 03:58, Stef Walter<stef-list(at)memberwebs(dot)com> wrote:
>>> Attached is a new patch, which I hope addresses all the concerns raised.
>>
>> I think you forgot to actually attach the patch....
>
> Whoops. Here it is.

Is there any actual advantage to using getifaddr() on Linux, and not
just use SIOCGIFCONF for all Unixen?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/