Thread: Please test peer (socket ident) auth on *BSD
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
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/> 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?
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
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 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
Attachment
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
Attachment
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 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
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 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
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
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
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: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
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
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
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