pg_hba.conf: samehost and samenet

On Fri, Aug 14, 2009 at 00:50, Stef Walter<stef-list@memberwebs.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@hagander.net> writes:

On Fri, Aug 14, 2009 at 00:50, Stef Walter<stef-list@memberwebs.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@hagander.net> writes:

On Fri, Aug 14, 2009 at 00:50, Stef Walter<stef-list@memberwebs.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@memberwebs.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@memberwebs.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@sss.pgh.pa.us> wrote:

Stef Walter <stef-list@memberwebs.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@memberwebs.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@memberwebs.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@memberwebs.com> wrote:

Magnus Hagander wrote:

On Wed, Aug 19, 2009 at 03:58, Stef Walter<stef-list@memberwebs.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/

Stef Walter wrote:

Magnus Hagander wrote:

and not just use SIOCGIFCONF for all Unixen?

I do know that using SIOCGIFCONF on AIX comes with strange wrinkles and
variable length data structures etc... getifaddrs() on AIX is a far more
maintainable interface.

Clearly the getifaddrs code looks nicer. How can we distinguish the
SIOCGIFCONF implementations that have wrinkles from those that don't?
Is there some autoconf macro?

Something to keep in mind -- my getifaddrs(3) manpage says that on BSD
it can return addresses that have ifa_addr set to NULL, which your code
doesn't seem to check.

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

Something is very wrong here -- this message does not have a
message-id!

Stef Walter wrote:

Magnus Hagander wrote:

On Wed, Aug 19, 2009 at 15:02, Stef Walter<stef-list@memberwebs.com> wrote:

Magnus Hagander wrote:

On Wed, Aug 19, 2009 at 03:58, Stef Walter<stef-list@memberwebs.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,

It is in my opinion, it is the most modern and maintainable of the
methods for obtaining network interface address information. Various
unixes have added support for getifaddr() over the years, and (again my
opinion) would probably continue to do so.

and not
just use SIOCGIFCONF for all Unixen?

I do know that using SIOCGIFCONF on AIX comes with strange wrinkles and
variable length data structures etc... getifaddrs() on AIX is a far more
maintainable interface.

That said, I'll adjust the patch as you feel is best for the long term
inclusion in the postgresql source.

Cheers,

Stef

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2009/8/25 Alvaro Herrera <alvherre@commandprompt.com>:

Stef Walter wrote:

Magnus Hagander wrote:

and not just use SIOCGIFCONF for all Unixen?

I do know that using SIOCGIFCONF on AIX comes with strange wrinkles and
variable length data structures etc... getifaddrs() on AIX is a far more
maintainable interface.

Clearly the getifaddrs code looks nicer.  How can we distinguish the
SIOCGIFCONF implementations that have wrinkles from those that don't?
Is there some autoconf macro?

If there are some that do have it, and these platforms support
getifaddrs(), that certainly seems like a worthwhile reason. I was
just looking for the case when the SIOGIFCONF code would be identical
on all platforms - in which case we'd jus tbe maintaining some
unnecessary code. So it seems fine here.

Something to keep in mind -- my getifaddrs(3) manpage says that on BSD
it can return addresses that have ifa_addr set to NULL, which your code
doesn't seem to check.

Eek. This is not defined by any standard, is it? I wonder how many
different behaviours we can find there :(

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

[Thanks for the heads up about the MessageID missing when posting this
previously. Was doing some mail filter development, and accidentally
left it in place... ]

Magnus Hagander wrote:

2009/8/25 Alvaro Herrera <alvherre@commandprompt.com>:

Something to keep in mind -- my getifaddrs(3) manpage says that on BSD
it can return addresses that have ifa_addr set to NULL, which your code
doesn't seem to check.

Thanks for catching that. I've added a check, and attached a new patch.

Eek. This is not defined by any standard, is it? I wonder how many
different behaviours we can find there :(

I've checked AIX, Linux, BSD and Mac OS and NULL ifa_addr's are
documented in all of them.

Cheers,

Stef

(This is my review of the latest version of Stef Walter's samehost/net
patch, posted on 2009-09-17. See
http://archives.postgresql.org/message-id/4AB28043.3050109@memberwebs.com
for the original message.)

The patch applies and builds cleanly, and the samehost/samenet keywords
in pg_hba.conf work as advertised. I have no particular opinion on the
desirability of the feature, but I can see it would be useful in some
circumstances, as Stef described.

I think the patch is more or less ready, but I have a few minor
comments:

First, it needs to be reformatted to not use a space before the opening
parentheses in (some) function calls and definitions.

*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
[...]

+       <para>Instead of an <replaceable>CIDR-address</replaceable>, you can specify 
+        the values <literal>samehost</literal> or <literal>samenet</literal>. To 
+        match any address on the subnets connected to the local machine, specify 
+        <literal>samenet</literal>. By specifying <literal>samehost</literal>, any 
+        addresses present on the network interfaces of local machine will match.
+       </para>
+ 

I'd suggest something like the following instead:

<para>Instead of a <replaceable>CIDR-address</replaceable>, you can
specify <literal>samehost</literal> to match any of the server's own
IP addresses, or <literal>samenet</literal> to match any address in
a subnet that the server belongs to.

*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
[...]

+ 	else if (addr->sa_family == AF_INET &&
+ 			 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.
+ 		 */

How about this instead:

If we're listening on IPv6 but the file specifies an IPv4 address to
match against, we promote the latter also to an IPv6 address before
trying to match the client's address.

(The comment is repeated elsewhere.)

+ int
+ pg_foreach_ifaddr(PgIfAddrCallback callback, void * cb_data)
+ {
+ #ifdef WIN32
+ 	return foreach_ifaddr_win32(callback, cb_data);
+ #else /* !WIN32 */
+ #ifdef HAVE_GETIFADDRS
+ 	return foreach_ifaddr_getifaddrs(callback, cb_data);
+ #else /* !HAVE_GETIFADDRS */
+ 	return foreach_ifaddr_ifconf(callback, cb_data);
+ #endif
+ #endif /* !WIN32 */
+ }

First, writing it this way is less noisy:

#ifdef WIN32
return foreach_ifaddr_win32(callback, cb_data);
#elif defined(HAVE_GETIFADDRS)
return foreach_ifaddr_getifaddrs(callback, cb_data);
#else
return foreach_ifaddr_ifconf(callback, cb_data);
#endif

Second, I can't see that it makes very much difference, but why do it
this way at all? You could just have each of the three #ifdef blocks
define a function named pg_foreach_ifaddr() and be done with it. No
need for a fourth function.

*** a/src/backend/libpq/pg_hba.conf.sample
--- b/src/backend/libpq/pg_hba.conf.sample
[...]

+ # You can also specify "samehost" to limit connections to those from addresses
+ # of the local machine. Or you can specify "samenet" to limit connections
+ # to addresses on the subnets of the local network.

This should be reworded to match the documentation change suggested
above.

-- ams

On Sun, Sep 20, 2009 at 05:59, Abhijit Menon-Sen <ams@toroid.org> wrote:

I think the patch is more or less ready, but I have a few minor
comments:

First, it needs to be reformatted to not use a space before the opening
parentheses in (some) function calls and definitions.

*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
[...]

+       <para>Instead of an <replaceable>CIDR-address</replaceable>, you can specify
+        the values <literal>samehost</literal> or <literal>samenet</literal>. To
+        match any address on the subnets connected to the local machine, specify
+        <literal>samenet</literal>. By specifying <literal>samehost</literal>, any
+        addresses present on the network interfaces of local machine will match.
+       </para>
+

I'd suggest something like the following instead:

   <para>Instead of a <replaceable>CIDR-address</replaceable>, you can
   specify <literal>samehost</literal> to match any of the server's own
   IP addresses, or <literal>samenet</literal> to match any address in
   a subnet that the server belongs to.

*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
[...]

+     else if (addr->sa_family == AF_INET &&
+                      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.
+              */

How about this instead:

   If we're listening on IPv6 but the file specifies an IPv4 address to
   match against, we promote the latter also to an IPv6 address before
   trying to match the client's address.

(The comment is repeated elsewhere.)

That's actually a copy/paste from the code that's in hba.c now. That's
not reason not to fix it of course :-)

+ int
+ pg_foreach_ifaddr(PgIfAddrCallback callback, void * cb_data)
+ {
+ #ifdef WIN32
+     return foreach_ifaddr_win32(callback, cb_data);
+ #else /* !WIN32 */
+ #ifdef HAVE_GETIFADDRS
+     return foreach_ifaddr_getifaddrs(callback, cb_data);
+ #else /* !HAVE_GETIFADDRS */
+     return foreach_ifaddr_ifconf(callback, cb_data);
+ #endif
+ #endif /* !WIN32 */
+ }

First, writing it this way is less noisy:

   #ifdef WIN32
       return foreach_ifaddr_win32(callback, cb_data);
   #elif defined(HAVE_GETIFADDRS)
       return foreach_ifaddr_getifaddrs(callback, cb_data);
   #else
       return foreach_ifaddr_ifconf(callback, cb_data);
   #endif

Second, I can't see that it makes very much difference, but why do it
this way at all? You could just have each of the three #ifdef blocks
define a function named pg_foreach_ifaddr() and be done with it. No
need for a fourth function.

That was my thought as well when I looked at the patch. It'd be
clearer and I think more in line with what we do at other places.

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

Thanks for your review!

Abhijit Menon-Sen wrote:

First, it needs to be reformatted to not use a space before the opening
parentheses in (some) function calls and definitions.

Fixed in the attached patch.

*** a/doc/src/sgml/client-auth.sgml
--- b/doc/src/sgml/client-auth.sgml
[...]

I'd suggest something like the following instead:

<para>Instead of a <replaceable>CIDR-address</replaceable>, you can
specify <literal>samehost</literal> to match any of the server's own
IP addresses, or <literal>samenet</literal> to match any address in
a subnet that the server belongs to.

Updated in attached patch.

*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
[...]

+ 	else if (addr->sa_family == AF_INET &&
+ 			 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.
+ 		 */

How about this instead:

If we're listening on IPv6 but the file specifies an IPv4 address to
match against, we promote the latter also to an IPv6 address before
trying to match the client's address.

As Magnus noted, this is a comment already present in the postgresql
code. I simply moved it into a function. However, I've attached a second
patch which fixes this issue, and can be committed at your discretion.

You could just have each of the three #ifdef blocks
define a function named pg_foreach_ifaddr() and be done with it. No
need for a fourth function.

Done.

*** a/src/backend/libpq/pg_hba.conf.sample
--- b/src/backend/libpq/pg_hba.conf.sample
[...]

+ # You can also specify "samehost" to limit connections to those from addresses
+ # of the local machine. Or you can specify "samenet" to limit connections
+ # to addresses on the subnets of the local network.

This should be reworded to match the documentation change suggested
above.

Done.

Cheers,

Stef

On Mon, Sep 21, 2009 at 20:12, Stef Walter <stef-list@memberwebs.com> wrote:

<snip>

Updated in attached patch.

This patch does not build on Windows, the error is:
ip.obj : error LNK2019: unresolved external symbol __imp__WSAIoctl@36 referenced
in function _pg_foreach_ifaddr
ip.obj : error LNK2019: unresolved external symbol __imp__WSASocketA@24 referenc
ed in function _pg_foreach_ifaddr
.\Release\libpq\libpq.dll : fatal error LNK1120: 2 unresolved externals

I don't have time to investigate this further right now, so if
somebody else want to dig into why that is happening that would be
helpful :)

Also, one thought - with samenet we currently from what I can tell
enumerate all interfaces. Not just those we bind to based on
listen_addresses. Is that intentional, or should we restrict us to
subnets reachable through the interfaces we're actually listening on?

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