Re: PAM support lacks pam_setcred() call

[Steve McIntyre]

On Tue, Oct 21, 2003 at 03:29:32PM -0400, Derek Robert Price wrote:
>
>Thanks!  This particular patch looks pretty good.  I'd like to see it
>against the trunk since that should merge more easily with the changes
>Brian already made.  I've made a few other notes below.
>
>|diff -ur cvs-1.12.1/config.h.in cvs-1.12.1.new/config.h.in
>Not needed - automatically generated.
>
>|diff -ur cvs-1.12.1/configure cvs-1.12.1.new/configure
>Ditto.
>
>|diff -ur cvs-1.12.1/configure.in cvs-1.12.1.new/configure.in
>|--- cvs-1.12.1/configure.in    2003-01-16 20:16:56.000000000 +0000
>|+++ cvs-1.12.1.new/configure.in    2003-01-30 01:32:00.000000000 +0000
>|@@ -258,6 +258,12 @@
>| AC_SEARCH_LIBS(getspnam, sec gen, AC_DEFINE(HAVE_GETSPNAM, 1,
>I like this in general, but for now, while this is an experimental
>feature, I prefer the user-enabled version already commited based on
>Brian's patch.  If Brian's isn't also checking that PAM is present, it
>might not hurt to merge the two and issue a warning when the user
>specifies that PAM should be used and the library couldn't be found.

Yes, fine. This is just a dump of the Debian patch that I've made. I
try to work with clean upstream source, which is why the patches to
the automake-generated files. Plus I know that the Debian CVS will
always be configured with PAM, so I can cut corners here.

>|diff -ur cvs-1.12.1/src/cvs.h cvs-1.12.1.new/src/cvs.h
>
>This is nice.  Perhaps an #else that prints some message such as "PAM
>not enabled in this executable" would be appropriate.

Yes, that's easy enough to add.

>|diff -ur cvs-1.12.1/src/server.c cvs-1.12.1.new/src/server.c
>|--- cvs-1.12.1/src/server.c    2003-06-01 23:41:37.000000000 +0100
>|+++ cvs-1.12.1.new/src/server..c    2003-10-13 00:39:24.000000000 +0100
>|@@ -16,6 +16,13 @@
>| #include "getline.h"
>| #include "buffer.h"
>|
>|+#ifdef HAVE_PAM
>|+#include <security/pam_misc.h>
>|+#include <security/pam_appl.h>
>|+static char *default_pam_username = NULL;
>|+int pam_auth = 1;
>|+#endif
>|+
>| #if defined(SERVER_SUPPORT) || defined(CLIENT_SUPPORT)
>|
>| # ifdef HAVE_GSSAPI
>|@@ -117,7 +117,7 @@
>|
>| /* Should we check for system usernames/passwords?  Can be changed by
>|    CVSROOT/config.  */
>|-int system_auth = 1;
>|+int system_auth = 0;
>
>I think I would prefer defaults of pam_auth=0 and system_auth=1, at
>least for now.  It's never nice to change defaults on users unless we
>can't help it.  If PAM ever ceases to be an experimental feature, then
>we can consider it, but system_auth should certainly never default to 0
>when !HAVE_PAM.

Of course. Again, this is the defaults I'm using for Debian. What do
you think of adding the PamAuth option in general? I like having the
option of using the normal code as a run-time option set in
CVSROOT/config, and some people may prefer it.

Marc, what did you have to add for the pam_setcred() call? And how are
you trying to use pam_group?

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
Is there anybody out there?

pgptpvvB1ZVfF.pgp
Description: PGP signature