Re: pg_hba.conf: samehost and samenet [REVIEW]

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

2009/8/25 Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(at)toroid(dot)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(at)memberwebs(dot)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(at)36 referenced
in function _pg_foreach_ifaddr
ip.obj : error LNK2019: unresolved external symbol __imp__WSASocketA(at)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/

Magnus Hagander wrote:
> On Mon, Sep 21, 2009 at 20:12, Stef Walter <stef-list(at)memberwebs(dot)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(at)36 referenced
> in function _pg_foreach_ifaddr
> ip.obj : error LNK2019: unresolved external symbol __imp__WSASocketA(at)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 :)

My windows VM is giving me problems, but I'll try look into it unless
someone else beats me to do it.

> 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?

This would change the scope of the patch significantly. It seems that
adding that limitation is unnecessary. In my opinion, if stricter hba
security is required, and limiting to specific subnets are desired,
those subnets should be entered directly into the pg_hba.conf file.

Currently people are adding 0.0.0.0 to a default pg_hba.conf file in
order to allow access from nearby machines, without running into the
maintenance problems of hard coding IP addresses. However using 0.0.0.0
is clearly suboptimal from a security perspective.

I've seen the samenet feature as a way to avoid the use of 0.0.0.0 in
these cases.

Obviously people who would like stricter postgres security can configure
subnets manually, and would probably not be comfortable with 'automatic'
decisions being made about the subnets allowed.

Cheers,

Stef

On Wed, Sep 23, 2009 at 18:41, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
> Magnus Hagander wrote:
>> On Mon, Sep 21, 2009 at 20:12, Stef Walter <stef-list(at)memberwebs(dot)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(at)36 referenced
>>  in function _pg_foreach_ifaddr
>> ip.obj : error LNK2019: unresolved external symbol __imp__WSASocketA(at)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 :)
>
> My windows VM is giving me problems, but I'll try look into it unless
> someone else beats me to do it.

If you want a VM that works, look at:
http://blog.hagander.net/archives/151-Testing-PostgreSQL-patches-on-Windows-using-Amazon-EC2.html

If it's just the VM... :-)

>> 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?
>
> This would change the scope of the patch significantly. It seems that
> adding that limitation is unnecessary. In my opinion, if stricter hba
> security is required, and limiting to specific subnets are desired,
> those subnets should be entered directly into the pg_hba.conf file.
>
> Currently people are adding 0.0.0.0 to a default pg_hba.conf file in
> order to allow access from nearby machines, without running into the
> maintenance problems of hard coding IP addresses. However using 0.0.0.0
> is clearly suboptimal from a security perspective.
>
> I've seen the samenet feature as a way to avoid the use of 0.0.0.0 in
> these cases.
>
> Obviously people who would like stricter postgres security can configure
> subnets manually, and would probably not be comfortable with 'automatic'
> decisions being made about the subnets allowed.

Agreed. In that case, I think we just need to make that clearer in the
docs, so people don't make the mistake of thinking it means somehting
other than what it does.

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

On Wed, Sep 23, 2009 at 12:41 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
> Currently people are adding 0.0.0.0 to a default pg_hba.conf file in
> order to allow access from nearby machines, without running into the
> maintenance problems of hard coding IP addresses. However using 0.0.0.0
> is clearly suboptimal from a security perspective.

If people aren't willing to take the time (5 minutes?) to create an
hba.conf file that implements a reasonable security policy, I'm not
sure anything we can do - and certainly not this - is going to help
very much. I haven't really looked at this patch, but how confident
are we that this is actually portable? It would be a shame to spend a
lot of time and energy troubleshooting portability problems with a
feature that - IMO - has a fairly marginal use case to begin with.
...Robert

Robert Haas wrote:
> On Wed, Sep 23, 2009 at 12:41 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
>> Currently people are adding 0.0.0.0 to a default pg_hba.conf file in
>> order to allow access from nearby machines, without running into the
>> maintenance problems of hard coding IP addresses. However using 0.0.0.0
>> is clearly suboptimal from a security perspective.
>
> If people aren't willing to take the time (5 minutes?) to create an
> hba.conf file that implements a reasonable security policy, I'm not
> sure anything we can do - and certainly not this - is going to help
> very much. I haven't really looked at this patch, but how confident
> are we that this is actually portable? It would be a shame to spend a
> lot of time and energy troubleshooting portability problems with a
> feature that - IMO - has a fairly marginal use case to begin with.

Obviously this isn't the an authentication method. If you're using
'trust' authentication with anything but unix sockets you're pretty
screwed anyway. This is used in conjuction with an authentication method.

The core problem is with renumbering. Due to IPv4 addresses becoming
more and more scarce, ISPs are regularly foisting renumbering on their
customers. For example, it's in all the new contracts.

Often renumbering takes place on networks where the original developers
are long gone.

Postgresql has always been very fragile when renumbering due to hard
coded IP addresses in the pg_hba.conf file. This patch solves that
problem for most of the cases, where hosts nearby on the network can
talk to postgresql hosts without putting fragile rules into pg_hba.conf.

Allowing host names in pg_hba.conf would also solve this problem,
although the last person who tried to implement this it was a topic of
contention. I asked if I should focus on reverse DNS host names in
pg_hba.conf or portability for this samenet patch, and it was indicated
that I should do the latter.

If there is clear direction within the community to work on DNS based
stuff in pg_hba.conf I'd be willing to contribute effort there.

Cheers,

Stef

On Wed, Sep 23, 2009 at 3:53 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
> Robert Haas wrote:
>> On Wed, Sep 23, 2009 at 12:41 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
>>> Currently people are adding 0.0.0.0 to a default pg_hba.conf file in
>>> order to allow access from nearby machines, without running into the
>>> maintenance problems of hard coding IP addresses. However using 0.0.0.0
>>> is clearly suboptimal from a security perspective.
>>
>> If people aren't willing to take the time (5 minutes?) to create an
>> hba.conf file that implements a reasonable security policy, I'm not
>> sure anything we can do - and certainly not this - is going to help
>> very much.  I haven't really looked at this patch, but how confident
>> are we that this is actually portable?  It would be a shame to spend a
>> lot of time and energy troubleshooting portability problems with a
>> feature that - IMO - has a fairly marginal use case to begin with.
>
> Obviously this isn't the an authentication method. If you're using
> 'trust' authentication with anything but unix sockets you're pretty
> screwed anyway. This is used in conjuction with an authentication method.
>
> The core problem is with renumbering. Due to IPv4 addresses becoming
> more and more scarce, ISPs are regularly foisting renumbering on their
> customers. For example, it's in all the new contracts.
>
> Often renumbering takes place on networks where the original developers
> are long gone.
>
> Postgresql has always been very fragile when renumbering due to hard
> coded IP addresses in the pg_hba.conf file. This patch solves that
> problem for most of the cases, where hosts nearby on the network can
> talk to postgresql hosts without putting fragile rules into pg_hba.conf.
>
> Allowing host names in pg_hba.conf would also solve this problem,
> although the last person who tried to implement this it was a topic of
> contention. I asked if I should focus on reverse DNS host names in
> pg_hba.conf or portability for this samenet patch, and it was indicated
> that I should do the latter.
>
> If there is clear direction within the community to work on DNS based
> stuff in pg_hba.conf I'd be willing to contribute effort there.

Personally, I can't imagine using any of these for anything that I
cared very much about. IP renumberings are a pain, but I'd rather
take a little extra time to make sure it gets done right. I have
other things that would need to be fixed too, besides PostgreSQL: for
example, IP tables rules.

That having been said, I don't think it's my place to harangue someone
else about their feature because it doesn't fit my use case. But if
it's going to make PostgreSQL not compile/not work the same way on
platforms that we otherwise support, then I think it's a bad idea.
Otherwise I have no objection.

...Robert

Stef Walter <stef-list(at)memberwebs(dot)com> writes:
> Allowing host names in pg_hba.conf would also solve this problem,
> although the last person who tried to implement this it was a topic of
> contention. I asked if I should focus on reverse DNS host names in
> pg_hba.conf or portability for this samenet patch, and it was indicated
> that I should do the latter.

Agreed, a DNS-based solution would be a huge pain in the rear to do
correctly. However, I think what Robert wanted to know was just how
portable you believe this solution is. If it doesn't work, and work
pretty much the same, on all our supported platforms then I'm afraid
we can't use it. There's nothing worse than a security-critical
feature that works differently than you expect it to.

In this case what particularly scares me is the idea that 'samenet'
might be interpreted to let in a larger subnet than the user expected,
eg 10/8 instead of 10.0.0/24. You'd likely not notice the problem until
after you'd been broken into ...

regards, tom lane

If looking for representation -

I consider the default pg_hba.conf to be problematic. Newbies start with
"trust" access, and then do silly things to open it up.

I would use samehost, and if samenet worked the same way it does for
Postfix, I would probably use samenet. This information can be pulled
from the operating system, and the requirement for it to be hard-coded
in pg_hba.conf is inconvenient at best, and problematic at worst. Yes,
renumbering requires some thought - but I prefer applications that do
the majority of this thought for me over applications that require me to
do mundane activities.

I would also use DNS in pg_hba.conf if it were available. I can see some
of the issues with this (should it be mapped to IP right away, or should
it be re-evaluated every time?), but ultimately the feature would be
useful, and would be widely used. Especially once we get to IPv6,
specification of the addresses will become a horrible chore, and
solutions which require the IPv6 address to be spelled out will be
painful to use.

Both of these are generally one time costs for me. They are a pain, but
most of us suck it up and swallow. It hasn't been on my list of itches
that I just have to scratch. Remember, though, that the majority of
PostgreSQL users are not represented on this list, and my pain here
might be acceptable, but a newbie will probably either turn away or do
something wrong. Better to give them a sensible configuration from the
start from, and allow the experts to specify IP addresses if that is
what they want to do.

Cheers,
mark

--
Mark Mielke<mark(at)mielke(dot)cc>

Tom Lane wrote:
> In this case what particularly scares me is the idea that 'samenet'
> might be interpreted to let in a larger subnet than the user expected,
> eg 10/8 instead of 10.0.0/24. You'd likely not notice the problem until
> after you'd been broken into ...
>
>

I haven't looked at this "feature" at all, but I'd be inclined, on the
grounds you quite reasonably cite, to require a netmask with "samenet",
rather than just ask the interface for its netmask.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> In this case what particularly scares me is the idea that 'samenet'
>> might be interpreted to let in a larger subnet than the user expected,
>> eg 10/8 instead of 10.0.0/24. You'd likely not notice the problem until
>> after you'd been broken into ...

> I haven't looked at this "feature" at all, but I'd be inclined, on the
> grounds you quite reasonably cite, to require a netmask with "samenet",
> rather than just ask the interface for its netmask.

I was just thinking the same thing. Could we then unify samehost and
samenet into one thing? sameaddr/24 or something like that, with
samehost just being the limiting case of all bits used. I am not
sure though if this works nicely for IPv6 as well as IPv4.

regards, tom lane

On 09/23/2009 05:37 PM, Andrew Dunstan wrote:
> Tom Lane wrote:
>> In this case what particularly scares me is the idea that 'samenet'
>> might be interpreted to let in a larger subnet than the user expected,
>> eg 10/8 instead of 10.0.0/24. You'd likely not notice the problem until
>> after you'd been broken into ...
>>
>
> I haven't looked at this "feature" at all, but I'd be inclined, on the
> grounds you quite reasonably cite, to require a netmask with
> "samenet", rather than just ask the interface for its netmask.

I think requiring a netmask defeats some of the value of samenet. When
being assigned a new address can change subnet as well. For example,
when we moved one of our machines from one room to another it went from
/24 to /26.

I think it should be understood that the network will not work properly
if the user has the wrong network configuration. If they accidentally
use /8 instead of /24 on their interface - it's more likely that some or
all of their network will become inaccessible, than somebody breaking
into their machine. And, anything is better than 0.0.0.0.

There are two questions here I think - one is whether or not samenet is
valid and would provide value, which I think it is and it does. A second
question is whether it should be enabled in the default pg_hba.conf - I
think not.

Postfix has this capability and it works fine. I use it to allow relay
email from machines I "trust", because they are on my network. I think
many people would use it, and it would be the right solution for many
problems. Worrying about how some person somewhere might screw up, when
they have the same opportunity to screw up if things are left unchanged
(0.0.0.0) is not a practical way of looking at things.

How many Postfix servers have you heard of being open relays as a result
of "samenet"? I haven't heard of it ever happening. I suppose it doesn't
mean it hasn't happened - but I think getting the network interface
configured properly being a necessity for the machine working properly
is a very good encouragement for it to work.

Cheers,
mark

--
Mark Mielke<mark(at)mielke(dot)cc>

Mark Mielke <mark(at)mark(dot)mielke(dot)cc> writes:
> Postfix has this capability and it works fine.

Hmm, have we looked at the Postfix code to see exactly how they do it?
I'd be a *lot* more comfortable adopting logic that's been proven in the
field than something written from scratch.

regards, tom lane

On 09/23/2009 05:40 PM, Tom Lane wrote:
>> I haven't looked at this "feature" at all, but I'd be inclined, on the
>> grounds you quite reasonably cite, to require a netmask with "samenet",
>> rather than just ask the interface for its netmask.
>>
> I was just thinking the same thing. Could we then unify samehost and
> samenet into one thing? sameaddr/24 or something like that, with
> samehost just being the limiting case of all bits used. I am not
> sure though if this works nicely for IPv6 as well as IPv4.

I could see some people wanting this as well - but it's not a
replacement for samenet, it would be an additional feature. For example,
at my company, I have a cluster of machines on a /26 subnet, but for
some accesses, I would prefer to "open it up" to /8, since our company
has a /8, and I may want to allow anybody in the company to connect,
regardless of how things are routed.

I may still want samenet in the same configuration, to grant additional
access if the person happens to be on my switch compared to "anywhere in
the company". For my switch, having to hard code the subnet is back to
being a pain. If we enlarge our subnet to /25, it's one more thing that
I would have to remember to change unnecessarily.

Cheers,
mark

--
Mark Mielke<mark(at)mielke(dot)cc>

Tom Lane wrote:
> Mark Mielke <mark(at)mark(dot)mielke(dot)cc> writes:
>> Postfix has this capability and it works fine.
>
> Hmm, have we looked at the Postfix code to see exactly how they do it?
> I'd be a *lot* more comfortable adopting logic that's been proven in the
> field than something written from scratch.

Good idea.

As far as I know postfix doesn't support win32. They use a similar
approach with using ioctls on some systems, getifaddrs on others.

I can take a look at the postfix code (src/util/inet_addr_local.c),
check out licenses, add win32 support and adapt it to postgres uses.

Cheers,

Stef

Tom Lane wrote:
> Stef Walter <stef-list(at)memberwebs(dot)com> writes:
>> Allowing host names in pg_hba.conf would also solve this problem,
>> although the last person who tried to implement this it was a topic of
>> contention. I asked if I should focus on reverse DNS host names in
>> pg_hba.conf or portability for this samenet patch, and it was indicated
>> that I should do the latter.
>
> Agreed, a DNS-based solution would be a huge pain in the rear to do
> correctly. However, I think what Robert wanted to know was just how
> portable you believe this solution is. If it doesn't work, and work
> pretty much the same, on all our supported platforms then I'm afraid
> we can't use it.

It does work the same on the platforms noted earlier. After work today,
I'll put time into making sure that the winsock build problem noted
earlier is sorted out.

> In this case what particularly scares me is the idea that 'samenet'
> might be interpreted to let in a larger subnet than the user expected,
> eg 10/8 instead of 10.0.0/24. You'd likely not notice the problem until
> after you'd been broken into ...

As Mark noted in another email, ones networking wouldn't work at all
with such a misconfiguration.

But if you like I can add additional defensive checks in the code to
ignore those obviously invalid netmasks like /0. Basically the OS would
be giving postgres bad information. Does postgres generally try to guard
against this? I'll follow the convention of the project.

Cheers,

Stef

Stef Walter <stef-list(at)memberwebs(dot)com> writes:
> But if you like I can add additional defensive checks in the code to
> ignore those obviously invalid netmasks like /0. Basically the OS would
> be giving postgres bad information. Does postgres generally try to guard
> against this? I'll follow the convention of the project.

I think we'd only bother with that if it was a known failure mode
due to platform bugs or common misconfigurations.

regards, tom lane

On Wed, Sep 23, 2009 at 7:56 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
> Tom Lane wrote:
>> Stef Walter <stef-list(at)memberwebs(dot)com> writes:
>>> Allowing host names in pg_hba.conf would also solve this problem,
>>> although the last person who tried to implement this it was a topic of
>>> contention. I asked if I should focus on reverse DNS host names in
>>> pg_hba.conf or portability for this samenet patch, and it was indicated
>>> that I should do the latter.
>>
>> Agreed, a DNS-based solution would be a huge pain in the rear to do
>> correctly.  However, I think what Robert wanted to know was just how
>> portable you believe this solution is.  If it doesn't work, and work
>> pretty much the same, on all our supported platforms then I'm afraid
>> we can't use it.
>
> It does work the same on the platforms noted earlier. After work today,
> I'll put time into making sure that the winsock build problem noted
> earlier is sorted out.

Rereading the thread, it seems that the main question is whether there
are any platforms that we support that have neither getifaddrs or
SIOCGIFCONF, or where they don't work properly.

By the way, in foreach_ifaddr_ifconf, what happens if the number of
addresses is too large to fit in the arbitrary-size buffer you've
chosen here?

...Robert

Magnus Hagander wrote:
> On Mon, Sep 21, 2009 at 20:12, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
> This patch does not build on Windows, the error is:
> ip.obj : error LNK2019: unresolved external symbol __imp__WSAIoctl(at)36 referenced
> in function _pg_foreach_ifaddr
> ip.obj : error LNK2019: unresolved external symbol __imp__WSASocketA(at)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 :)

Seems there are two windows build systems. Once I discovered the MSVC
one, and got it working, I added the required ws2 library (already used
by other components of postgresql).

Attached patch contains a fix.

Cheers,

Stef

On Thu, Sep 24, 2009 at 8:32 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
> Magnus Hagander wrote:
>> On Mon, Sep 21, 2009 at 20:12, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
>> This patch does not build on Windows, the error is:
>> ip.obj : error LNK2019: unresolved external symbol __imp__WSAIoctl(at)36 referenced
>>  in function _pg_foreach_ifaddr
>> ip.obj : error LNK2019: unresolved external symbol __imp__WSASocketA(at)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 :)
>
> Seems there are two windows build systems. Once I discovered the MSVC
> one, and got it working, I added the required ws2 library (already used
> by other components of postgresql).
>
> Attached patch contains a fix.

So is this one Ready for Committer?

...Robert

Robert Haas wrote:
>> Attached patch contains a fix.
>
> So is this one Ready for Committer?

Not yet. Two more things to do. Will work on them early next week:

* On Solaris the ioctl used only returns IPv4 addresses.
* Don't use hard coded buffers on win32 and ioctl.

Cheers,

Stef

Robert Haas wrote:
> So is this one Ready for Committer?

Here we go, I think this one is ready. In addition to previous patches,
it does:

* Use some techniques from postfix for getting interface addresses.
Couldn't use code outright, due to license incompatibilities.
* Tested on Solaris, FreeBSD, Linux and Windows. As far as I can tell
this should also work on Mac OS, HPUX and AIX, and probably others.
* Added src/tools/ifaddrs/test_ifaddrs tool for testing interface
address code.

Cheers,

Stef

Whoops I missed this email...

Robert Haas wrote:
> Rereading the thread, it seems that the main question is whether there
> are any platforms that we support that have neither getifaddrs or
> SIOCGIFCONF, or where they don't work properly.

As far as I can tell, there are no non-ancient mainstream platforms that
we're missing here. As Tom suggested, I've looked over postfix, bind and
pcap and merged what I've learned into the (attached) samenet patch. I
believe we're hitting all the majors here:

* Win32 using win_wsa2.dll
* Modern versions of: Linux, BSD, Mac OS X, AIX using getifaddrs
* Modern Solaris and HPUX using ioctl/SIOCGLIFCONF
* Older unixes (BSD, Linux, Solaris, AIX) using ioctl/SIOCGIFCONF

SIOCGIFCONF doesn't return IPv6 information on certain platforms (such
as modern Solaris, or older Linux).

I believe we're covering every single Unix in use out there. I have
however only verified this assertion on open source OS's. I've also
verified that the SIOCGIFCONF method on Linux, BSD and Solaris, even
though they use more modern methods by default.

If a problem occurs with this code the src/tools/ifaddrs tool can be
used to diagnose the problem, and send in debugging feedback.

> By the way, in foreach_ifaddr_ifconf, what happens if the number of
> addresses is too large to fit in the arbitrary-size buffer you've
> chosen here?

The old approach was not a security vulnerability, and I find it hard to
believe that anyone would have had more than 10K of addresses. However
for the sake of completeness attached is a patch with dynamically sized
buffers. This adds some code complexity, but maybe someone out there
would have run into this (extremely) edge case.

I believe this patch to be complete, and am looking forward to review.

Cheers,

Stef

On Mon, Sep 28, 2009 at 10:10 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:

>  * Win32 using win_wsa2.dll

I assume you mean ws2_32.dll?

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Dave Page wrote:
> On Mon, Sep 28, 2009 at 10:10 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
>
>> * Win32 using win_wsa2.dll
>
> I assume you mean ws2_32.dll?

Yes. I get dyslexic around windows DLLs. :)

Stef

On Mon, Sep 28, 2009 at 4:04 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
> Robert Haas wrote:
>> So is this one Ready for Committer?
>
> Here we go, I think this one is ready. In addition to previous patches,
> it does:
>
>  * Use some techniques from postfix for getting interface addresses.
>   Couldn't use code outright, due to license incompatibilities.
>  * Tested on Solaris, FreeBSD, Linux and Windows. As far as I can tell
>   this should also work on Mac OS, HPUX and AIX, and probably others.
>  * Added src/tools/ifaddrs/test_ifaddrs tool for testing interface
>   address code.

Abhijit -

This look ready to you, too? If so, please mark it as such.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Sep 28, 2009 at 4:04 PM, Stef Walter <stef-list(at)memberwebs(dot)com> wrote:
>> * Tested on Solaris, FreeBSD, Linux and Windows. As far as I can tell
>> this should also work on Mac OS, HPUX and AIX, and probably others.

> This look ready to you, too? If so, please mark it as such.

I was just poking at this. It seems to need rather a lot of
editorialization (eg to fix the lack of consistency about whether
nonstandard headers have configure tests, or bother to make use of the
tests that did get added). However, it does actually compile and appear
to work on HPUX 10.20, which is my personal benchmark for hopeless
obsolescence ;-). So modulo the issue about how much we trust the
system-reported netmasks, it seems we could adopt this.

regards, tom lane

Tom Lane wrote:
> I was just poking at this.

Thanks for trying it out.

It seems to need rather a lot of
> editorialization (eg to fix the lack of consistency about whether
> nonstandard headers have configure tests, or bother to make use of the
> tests that did get added).

I've now added tests for sys/ioctl.h and net/if.h even though these
headers seemed to be common to all the unixes investigated.

The test for ifaddrs.h is to allow the test for getifaddrs() later in
configure.in to work. This is how other open source projects have
handled this situation, but if you'd like me to do it differently for
postgres I can.

> However, it does actually compile and appear
> to work on HPUX 10.20, which is my personal benchmark for hopeless
> obsolescence ;-).

Good news.

So modulo the issue about how much we trust the
> system-reported netmasks, it seems we could adopt this.

FWIW, there are checks for various bad netmasks. I incorporated these
techniques after seeing them in the corresponding postfix code.

BTW, there's also fallback code. If none of the methods work on a given
OS, then the ifaddrs code just lists 127.0.0.1/8 and ::1/128.

Cheers,

Stef

At 2009-09-30 11:16:57 -0500, stef-list(at)memberwebs(dot)com wrote:
>
> I've now added tests for sys/ioctl.h and net/if.h even though these
> headers seemed to be common to all the unixes investigated.

Thanks. I've marked this ready for committer now.

> FWIW, there are checks for various bad netmasks. I incorporated these
> techniques after seeing them in the corresponding postfix code. [...]
>
> BTW, there's also fallback code. If none of the methods work on a
> given OS, then the ifaddrs code just lists 127.0.0.1/8 and ::1/128.

Both of these look good.

> + /*
> + * 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.
> + */

I still think this comment should be fixed in _both_ places it now
occurs, but oh well.

(Note: I have not tested under Windows, and I've not tested
test_ifaddrs.c. Also, there are a few lines which introduce
trailing whitespace.)

-- ams

Stef Walter <stef-list(at)memberwebs(dot)com> writes:
> [ postgres-hba-samenet-8.patch ]

Applied with some mostly-cosmetic editorialization.

regards, tom lane