Please test peer (socket ident) auth on *BSD

I wrote:

I've applied patches to fix Martin Pitt's report of peer auth failing on
FreeBSD-amd64 kernels. I tested it with FreeBSD but do not have the
resources to check every other platform that uses the same code branch
in auth_peer. The buildfarm will soon tell us if the patches fail to
compile anywhere, but since the buildfarm doesn't test that
authentication path, it's not going to be as obvious whether it works.

So, if you have a BSD-ish machine, please try HEAD and see if peer auth
(or "ident" auth in older branches) still works for you. Extra points
if you find out it used to be broken on your machine. (Hey Stefan, did
you ever try that on spoonbill?)

BTW, after looking more closely at the buildfarm configure logs, it
appears that both OpenBSD and NetBSD have getpeereid(), which means
that they don't use this code at all. It is currently looking to me
like the HAVE_STRUCT_FCRED and HAVE_STRUCT_SOCKCRED variants are dead
code. They've been in there since before we had the getpeereid() code
path, and presumably were needed at one time ... but does anyone know
of a platform where they're still needed?

I'm a bit inclined to rip that code out of HEAD, if we can't point to a
platform where it'd be needed, just to reduce the #ifdef spaghetti.

regards, tom lane

On 05/30/2011 07:57 PM, Tom Lane wrote:

I've applied patches to fix Martin Pitt's report of peer auth failing on
FreeBSD-amd64 kernels. I tested it with FreeBSD but do not have the
resources to check every other platform that uses the same code branch
in auth_peer. The buildfarm will soon tell us if the patches fail to
compile anywhere, but since the buildfarm doesn't test that
authentication path, it's not going to be as obvious whether it works.

So, if you have a BSD-ish machine, please try HEAD and see if peer auth
(or "ident" auth in older branches) still works for you. Extra points
if you find out it used to be broken on your machine. (Hey Stefan, did
you ever try that on spoonbill?)

There's actually no reason we couldn't test this in the buildfarm.
Turning it on unconditionally is a one-line change. Making it happen
just on the right platforms would be a few more lines, but nothing much.
It breaks the dblink regression tests, so we'd either have to get around
that or turn it off when checking contrib. I can add this if you think
it's worth it.

But I did try it on my FBSD/x86_64 VM and it passed the tests up till it
got to dblink, so there's another data point for you anyway.

cheers

andrew

I wrote:

BTW, after looking more closely at the buildfarm configure logs, it
appears that both OpenBSD and NetBSD have getpeereid(), which means
that they don't use this code at all. It is currently looking to me
like the HAVE_STRUCT_FCRED and HAVE_STRUCT_SOCKCRED variants are dead
code.

Further research discloses that:

1. FreeBSD has has getpeereid() since 2001; their CVS logs show that it
was implemented "mostly for compatibility with OpenBSD", so OpenBSD has
had it since even further back.

2. NetBSD implemented getpeereid() as of 5.0.

3. Mac OS X has had getpeereid() since 10.3 or thereabouts.

This means that in fact the control-message variant of auth_peer is dead
code on EVERY modern *BSD variant. So far as I can find, the only
platform on which it is still used is Debian/kFreeBSD, that is Linux
userland running on a FreeBSD kernel: glibc naturally lacks getpeereid,
but the kernel doesn't have SO_PEERCRED, so you end up with the control
message stuff. (This doubtless explains why the portability issues in
the control-message code escaped notice for so long.)

However, FreeBSD does have, and Debian/kFreeBSD does expose,
getsockopt(LOCAL_PEERCRED), which turns out to be functionally
equivalent to SO_PEERCRED: in particular, you can just call it and get
the answer without having to fool with getting the far end to send a
message. This is not only a whole lot cleaner than what we have, but
also could be used to implement libpq's requirepeer option, which is
currently unsupported on such platforms.

So what I'm now thinking is we should rip out the control-message
implementation altogether, and instead use LOCAL_PEERCRED. This is
probably not something to back-patch, but it would make things a lot
cleaner going forward.

Comments?

regards, tom lane

On tis, 2011-05-31 at 11:59 -0400, Tom Lane wrote:

However, FreeBSD does have, and Debian/kFreeBSD does expose,
getsockopt(LOCAL_PEERCRED), which turns out to be functionally
equivalent to SO_PEERCRED: in particular, you can just call it and get
the answer without having to fool with getting the far end to send a
message. This is not only a whole lot cleaner than what we have, but
also could be used to implement libpq's requirepeer option, which is
currently unsupported on such platforms.

So what I'm now thinking is we should rip out the control-message
implementation altogether, and instead use LOCAL_PEERCRED. This is
probably not something to back-patch, but it would make things a lot
cleaner going forward.

Oh yes, no point in having complicated code that doesn't get exercised.

On Tue, May 31, 2011 at 12:38 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

So what I'm now thinking is we should rip out the control-message
implementation altogether, and instead use LOCAL_PEERCRED.  This is
probably not something to back-patch, but it would make things a lot
cleaner going forward.

Oh yes, no point in having complicated code that doesn't get exercised.

This does amount to desupporting old versions of those OSes in newer
versions of Postgres, at least for this one feature. Since you're
saying you don't want to backport it that doesn't seem like a big deal
to me. Probably something worth mentioning in release notes though.

--
greg

Greg Stark <gsstark@mit.edu> writes:

On Tue, May 31, 2011 at 12:38 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

Oh yes, no point in having complicated code that doesn't get exercised.

This does amount to desupporting old versions of those OSes in newer
versions of Postgres, at least for this one feature. Since you're
saying you don't want to backport it that doesn't seem like a big deal
to me. Probably something worth mentioning in release notes though.

Yeah, possibly. So far as I can tell, both FreeBSD and OpenBSD have had
getpeereid for so long that it couldn't be an issue. I guess there
might still be some people running pre-5.0 versions of NetBSD though.

(BTW, in both FreeBSD and NetBSD, it turns out that getpeereid is just a
thin wrapper around SO_PEERCRED-equivalent getsockopt calls. However,
there doesn't seem to be any point in supporting NetBSD's getsockopt
call directly, because it was added at the same time as the getpeereid
function. Unless maybe there's a kFreeBSD-like project out there with
NetBSD as the kernel?)

regards, tom lane

On Wed, Jun 1, 2011 at 12:22 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Greg Stark <gsstark@mit.edu> writes:

On Tue, May 31, 2011 at 12:38 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

Oh yes, no point in having complicated code that doesn't get exercised.

This does amount to desupporting old versions of those OSes in newer
versions of Postgres, at least for this one feature. Since you're
saying you don't want to backport it that doesn't seem like a big deal
to me. Probably something worth mentioning in release notes though.

Yeah, possibly.  So far as I can tell, both FreeBSD and OpenBSD have had
getpeereid for so long that it couldn't be an issue.  I guess there
might still be some people running pre-5.0 versions of NetBSD though.

(BTW, in both FreeBSD and NetBSD, it turns out that getpeereid is just a
thin wrapper around SO_PEERCRED-equivalent getsockopt calls.  However,
there doesn't seem to be any point in supporting NetBSD's getsockopt
call directly, because it was added at the same time as the getpeereid
function.  Unless maybe there's a kFreeBSD-like project out there with
NetBSD as the kernel?)

My suggestion would be to use getpeereid() everywhere.
And just have compat getpeereid() implementation on non-BSD
platforms. This would minimize ifdeffery in core core.

--
marko

Marko Kreen <markokr@gmail.com> writes:

My suggestion would be to use getpeereid() everywhere.
And just have compat getpeereid() implementation on non-BSD
platforms. This would minimize ifdeffery in core core.

Hm, maybe. I'd be for this if we had more than two call sites, but
as things stand I'm not sure it's worth the trouble to set up a src/port
module for it.

regards, tom lane

2011/5/31, Tom Lane <tgl@sss.pgh.pa.us>:

Unless maybe there's a kFreeBSD-like project out there with NetBSD as
the kernel?)

There used to be an attempt by Debian (called GNU/NetBSD), but that
has since long been abandoned. I don't know of any other similar
projects.

<URL:http://www.debian.org/ports/netbsd/&gt;

Wikipedia doesn't list any other similar projects either:

<URL:http://en.wikipedia.org/wiki/GNU_variants#BSD_variants&gt;

Nicolas

--
A. Because it breaks the logical sequence of discussion.
Q. Why is top posting bad?

On Wed, Jun 1, 2011 at 1:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Marko Kreen <markokr@gmail.com> writes:

My suggestion would be to use getpeereid() everywhere.
And just have compat getpeereid() implementation on non-BSD
platforms.  This would minimize ifdeffery in core core.

Hm, maybe.  I'd be for this if we had more than two call sites, but
as things stand I'm not sure it's worth the trouble to set up a src/port
module for it.

It would remove ~50 lines of low-level code from otherwise
high-level function. So even with one call site it would be improvement.

If the src/port is trouble, how about putting it as 'static inline' into .h? :)

--
marko

On Wed, Jun 1, 2011 at 1:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Marko Kreen <markokr@gmail.com> writes:

My suggestion would be to use getpeereid() everywhere.
And just have compat getpeereid() implementation on non-BSD
platforms.  This would minimize ifdeffery in core core.

Hm, maybe.  I'd be for this if we had more than two call sites, but
as things stand I'm not sure it's worth the trouble to set up a src/port
module for it.

Here's my attempt for it. As conditional port module seems trouble,
I set up an unconditional pgGetpeereid() that is always defined.

The result seems nice. It also fixes broken ifdeffery where
"#error missing implementation" is unreachable, instead
pqGetpwuid() can be reached with undefined uid.

It does drop 2 error messages for HAVE_UNIX_SOCKET but no method
for getting peer id. Now it will give plain ENOSYS in that case.
If really required, the message can be picked based on errno,
but it does not seem worth it.

--
marko

Marko Kreen <markokr@gmail.com> writes:

Here's my attempt for it. As conditional port module seems trouble,
I set up an unconditional pgGetpeereid() that is always defined.

-1 ... why would you think that a conditional substitution is trouble?
We have plenty of others.

regards, tom lane

On Thu, Jun 2, 2011 at 5:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Marko Kreen <markokr@gmail.com> writes:

Here's my attempt for it.  As conditional port module seems trouble,
I set up an unconditional pgGetpeereid() that is always defined.

-1 ... why would you think that a conditional substitution is trouble?
We have plenty of others.

Because it required touching autoconf. ;)

So now I did it. I hope it was that simple.

As there was no going back now, I even touched msvc.pm.

--
marko

On 06/02/2011 11:29 AM, Marko Kreen wrote:

As there was no going back now, I even touched msvc.pm.

Why? Windows doesn't have Unix domain sockets at all.

cheers

andrew

On Thu, Jun 2, 2011 at 6:59 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 06/02/2011 11:29 AM, Marko Kreen wrote:

As there was no going back now, I even touched msvc.pm.

Why? Windows doesn't have Unix domain sockets at all.

Because the function is still referenced in the code.

--
marko

On 06/02/2011 12:04 PM, Marko Kreen wrote:

On Thu, Jun 2, 2011 at 6:59 PM, Andrew Dunstan<andrew@dunslane.net> wrote:

On 06/02/2011 11:29 AM, Marko Kreen wrote:

As there was no going back now, I even touched msvc.pm.

Why? Windows doesn't have Unix domain sockets at all.

Because the function is still referenced in the code.

Then maybe we need to use "#ifndef WIN32" in those places. That's what
we do for similar cases.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

On 06/02/2011 12:04 PM, Marko Kreen wrote:

On Thu, Jun 2, 2011 at 6:59 PM, Andrew Dunstan<andrew@dunslane.net> wrote:

On 06/02/2011 11:29 AM, Marko Kreen wrote:

As there was no going back now, I even touched msvc.pm.

Why? Windows doesn't have Unix domain sockets at all.

Because the function is still referenced in the code.

Then maybe we need to use "#ifndef WIN32" in those places. That's what
we do for similar cases.

Seems reasonable, since the whole code chunk is within IS_AF_UNIX
anyway. Will adjust and apply.

regards, tom lane

Excerpts from Andrew Dunstan's message of jue jun 02 11:59:02 -0400 2011:

On 06/02/2011 11:29 AM, Marko Kreen wrote:

As there was no going back now, I even touched msvc.pm.

Why? Windows doesn't have Unix domain sockets at all.

So much for being thorough :-P

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, Jun 2, 2011 at 7:20 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 06/02/2011 12:04 PM, Marko Kreen wrote:

On Thu, Jun 2, 2011 at 6:59 PM, Andrew Dunstan<andrew@dunslane.net>
 wrote:

On 06/02/2011 11:29 AM, Marko Kreen wrote:

As there was no going back now, I even touched msvc.pm.

Why? Windows doesn't have Unix domain sockets at all.

Because the function is still referenced in the code.

Then maybe we need to use "#ifndef WIN32" in those places. That's what we do
for similar cases.

No, that would be a bad idea - uglifies code for no good reason.

The function is referenced undef IS_AF_UNIX() check, so it would
not be run anyway. Even if it would run somehow, there is only
2 lines to return ENOSYS.

With #ifdef you would need some additional error message under #ifdef WIN32,
just in case, so what exactly would be improved by that?

--
marko