Horton

Thread

  1. Please test peer (socket ident) auth on *BSD

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-05-30T23:57:44Z

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

			regards, tom lane

  2. Re: Please test peer (socket ident) auth on *BSD

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-05-31T02:31:20Z

    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

  3. Re: Please test peer (socket ident) auth on *BSD

    Andrew Dunstan <andrew@dunslane.net> — 2011-05-31T13:43:33Z

    
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

  4. Re: Please test peer (socket ident) auth on *BSD

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-05-31T15:59:11Z

    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

  5. Re: Please test peer (socket ident) auth on *BSD

    Peter Eisentraut <peter_e@gmx.net> — 2011-05-31T19:38:08Z

    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.

  6. Re: Please test peer (socket ident) auth on *BSD

    Greg Stark <gsstark@mit.edu> — 2011-05-31T21:05:20Z

    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

  7. Re: Please test peer (socket ident) auth on *BSD

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-05-31T21:22:30Z

    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

  8. Re: Please test peer (socket ident) auth on *BSD

    Marko Kreen <markokr@gmail.com> — 2011-05-31T21:52:34Z

    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

  9. Re: Please test peer (socket ident) auth on *BSD

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-05-31T22:03:38Z

    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

  10. Re: Please test peer (socket ident) auth on *BSD

    Nicolas Barbier <nicolas.barbier@gmail.com> — 2011-05-31T22:07:00Z

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

Wikipedia doesn't list any other similar projects either:

<URL:http://en.wikipedia.org/wiki/GNU_variants#BSD_variants>

Nicolas

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

  11. Re: Please test peer (socket ident) auth on *BSD

    Marko Kreen <markokr@gmail.com> — 2011-05-31T22:30:56Z

    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

  12. Re: Please test peer (socket ident) auth on *BSD

    Marko Kreen <markokr@gmail.com> — 2011-06-02T14:35:19Z

    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

  13. Re: Please test peer (socket ident) auth on *BSD

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-02T14:49:04Z

    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

  14. Re: Please test peer (socket ident) auth on *BSD

    Marko Kreen <markokr@gmail.com> — 2011-06-02T15:29:08Z

    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

  15. Re: Please test peer (socket ident) auth on *BSD

    Andrew Dunstan <andrew@dunslane.net> — 2011-06-02T15:59:02Z

    
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

  16. Re: Please test peer (socket ident) auth on *BSD

    Marko Kreen <markokr@gmail.com> — 2011-06-02T16:04:53Z

    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

  17. Re: Please test peer (socket ident) auth on *BSD

    Andrew Dunstan <andrew@dunslane.net> — 2011-06-02T16:20:53Z

    
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

  18. Re: Please test peer (socket ident) auth on *BSD

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-02T16:29:44Z

    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

  19. Re: Please test peer (socket ident) auth on *BSD

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-06-02T16:31:57Z

    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

  20. Re: Please test peer (socket ident) auth on *BSD

    Marko Kreen <markokr@gmail.com> — 2011-06-02T16:32:25Z

    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

  21. Re: Please test peer (socket ident) auth on *BSD

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-02T16:44:02Z

    Marko Kreen <markokr@gmail.com> writes:
> On Thu, Jun 2, 2011 at 7:20 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>> 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.

Yeah, but not compiling thirty lines in fe-connect.c is worthwhile.

The auth_peer code in the backend is #ifdef HAVE_UNIX_SOCKETS, and
I see no reason why this chunk in libpq shouldn't be as well.

			regards, tom lane

  22. Re: Please test peer (socket ident) auth on *BSD

    Marko Kreen <markokr@gmail.com> — 2011-06-02T16:45:04Z

    On Thu, Jun 2, 2011 at 7:31 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> 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

Well, there is 2 approaches to portable C code:
1) You #ifdef the main code portable
2) You #ifdef common platform in headers, then main code
is written against common platform, without ifdefs.

I'm from the camp #2.

-- 
marko

  23. Re: Please test peer (socket ident) auth on *BSD

    Marko Kreen <markokr@gmail.com> — 2011-06-02T16:47:44Z

    On Thu, Jun 2, 2011 at 7:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marko Kreen <markokr@gmail.com> writes:
>> On Thu, Jun 2, 2011 at 7:20 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>>> 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.
>
> Yeah, but not compiling thirty lines in fe-connect.c is worthwhile.
>
> The auth_peer code in the backend is #ifdef HAVE_UNIX_SOCKETS, and
> I see no reason why this chunk in libpq shouldn't be as well.

ip.h:

#ifdef  HAVE_UNIX_SOCKETS
#define IS_AF_UNIX(fam) ((fam) == AF_UNIX)
#else
#define IS_AF_UNIX(fam) (0)
#endif

This the #ifdefs-in-headers-only approach to the problem...

-- 
marko

  24. Re: Please test peer (socket ident) auth on *BSD

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-06-02T17:04:06Z

    Excerpts from Marko Kreen's message of jue jun 02 12:45:04 -0400 2011:
> On Thu, Jun 2, 2011 at 7:31 PM, Alvaro Herrera
> <alvherre@commandprompt.com> wrote:
> > 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
> 
> Well, there is 2 approaches to portable C code:
> 1) You #ifdef the main code portable
> 2) You #ifdef common platform in headers, then main code
> is written against common platform, without ifdefs.
> 
> I'm from the camp #2.

I don't disagree, just saying that you seem to have gone out of your way
to produce something that doesn't seem to be necessary.

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

  25. Re: Please test peer (socket ident) auth on *BSD

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-02T17:06:19Z

    Marko Kreen <markokr@gmail.com> writes:
>> -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.

Applied with minor adjustments --- notably, I didn't agree with removing
the special-case error messages for platforms that lack support for
this.

			regards, tom lane

  26. Re: Please test peer (socket ident) auth on *BSD

    Andrew Dunstan <andrew@dunslane.net> — 2011-06-02T17:15:07Z

    
On 06/02/2011 01:04 PM, Alvaro Herrera wrote:
> Excerpts from Marko Kreen's message of jue jun 02 12:45:04 -0400 2011:
>> On Thu, Jun 2, 2011 at 7:31 PM, Alvaro Herrera
>> <alvherre@commandprompt.com>  wrote:
>>> 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
>> Well, there is 2 approaches to portable C code:
>> 1) You #ifdef the main code portable
>> 2) You #ifdef common platform in headers, then main code
>> is written against common platform, without ifdefs.
>>
>> I'm from the camp #2.
> I don't disagree, just saying that you seem to have gone out of your way
> to produce something that doesn't seem to be necessary.

Yeah, I'm from the camp that says "don't compile code that's guaranteed 
to be dead."

cheers

andrew