GNOME Bugzilla – Bug 651554
implement OpenBSD authentication
Last modified: 2011-12-01 07:59:51 UTC
Created attachment 188942 [details] [review] implement OpenBSD authentication using bsd_auth Hi. OpenBSD does not use PAM for authentication but bsd_auth(3). It has a subset of the similar functionalities (i.e. authenticate against different password backends). This patch adds a bsd_auth authentication scheme so that locking works on OpenBSD. Note that this is a first proposal, because I don't know where gnome-screensaver is heading wrt to other authenticators, so I used what was available in the code already (like the setuid stuff) to make the patch as small as possible (especially the autoconf bits) so it's easier to review. Thoughts?
No one?
Hi. Is there any way I could make people interested in this? Thanks.
Folks seriously, I know no one gives a fuck about !linux in the GNOME community but I find it really rude not even having someone acknowledge and comment this... I'm doing my best trying to support another platform (since most GNOME people usually advertise the fact that if we want our OS supported we have to do the job), but being ignored is really frustrating :-(
Review of attachment 188942 [details] [review]: ::: configure.ac @@ +468,3 @@ +case "$host" in + *-openbsd*) + have_bsdauth=yes Is this really specific to openbsd? Or just since you h @@ +474,3 @@ + NEED_SETUID=yes + fi + AC_SUBST(NEED_SETUID) Weird...it looks like NEED_SETUID at one point was set by gnome-screensaver, then it stopped. Anyways, you should take the AC_SUBST outside if the case statement. @@ +552,2 @@ else + if test "x$have_bsdauth" = "xno" ; then Really we should only check for PAM if we haven't found bsdauth. Or the other way around. ::: src/gs-auth-pam.c @@ +25,3 @@ #include "config.h" +#ifndef HAVE_BSDAUTH If the entire gs-auth-pam.c file isn't needed, rather than ifdef'ing it out, you should make it conditionally compiled in Makefile.am.
(In reply to comment #4) > Review of attachment 188942 [details] [review]: > > ::: configure.ac > @@ +468,3 @@ > +case "$host" in > + *-openbsd*) > + have_bsdauth=yes > > Is this really specific to openbsd? Or just since you h Yes. BSD authentication was also used by BSDi (BSD/OS) in the past but this OS is no more. > + NEED_SETUID=yes > + fi > + AC_SUBST(NEED_SETUID) > > Weird...it looks like NEED_SETUID at one point was set by gnome-screensaver, > then it stopped. Indeed. It was needed for shadow authentication which got removed. It is now needed only for BSD authentication at run time, then it will drop privileges to setgid auth (bsd auth helpers are only accessible by the auth group). <snip> I will take care of the other things you mentioned and update the patch asap, thanks for the review :-)
Created attachment 195187 [details] [review] implement OpenBSD authentication using bsd_auth Hi Colin. Would something like this do?
Hi. I still haven't had any clear answer on that bug report. Now that 3.2 is out, I think it'll be a good time to push this to master so if anyone could have a look, I'd appreciate.
Considering the fact you incorporated Colin's suggestions and that I've been using this too; I for one would be happy to see this being pushed to master.
I only myself look at GNOME/Linux integration issues in general. The official maintainer is in MAINTAINERS file, but I doubt Jon is going to review this. It looks pretty simple and unlikely to regress the PAM case, so I guess I'd tentatively say it's OK. The only other thing I noticed is the type definition in the middle of the function will require C99, but generally in GNOME we stick to C89.
I concur; the patch looks ok to me too. Should just add a { } around the content of the ifdef to address the C99 concern.
Thanks guys. Ok new patch with the brackets and some simplification in the bsdauth handling code. If I hear nothing within the next 24h, I'll commit that, fair enough I hope :)
Created attachment 202432 [details] [review] implement OpenBSD authentication using bsd_auth
Yes, please go ahead
It's in, d7ab99c00a01bea870cf51d855ecf320e0d23bac