[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: PAM authentication patch - v2

From: Brian Murphy
Subject: Re: PAM authentication patch - v2
Date: Tue, 15 Apr 2003 10:01:26 +0200
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020623 Debian/1.0.0-0.woody.1

+  [broken-pam-appdata],
+    [--with-broken-pam-appdata],
+    [Use if you have a broken PAM which calls the conversation function
+ with a null application data pointer, Solaris 2.60 is a culprit here]))

This is _not_ the way to use configure unless a condition is totally undetectable. The whole goal behind autoconf and configure is to autodetect conditions that affect your software rather than requiring the builder to know or requiring someone to collect the data per platform and associate it with a table somewhere.

I assume you could write a snippet of code in C to detect if the "application data pointer" is null? Write that and then call AC_TRY_COMPILE, AC_TRY_LINK, or AC_TRY_RUN on the code as necessary. See the Autoconf manual for more on these macros and which will prove most appropriate for you, but avoid AC_TRY_RUN, if possible.

I tried. The problem is that whether PAM calls the conversation function depends on whether the configured module (pam_unix.so below) needs the information it supplies (username / password).
This depends on the system configuration.
You would need (on linux - because this stuff is very system dependant) in /etc/pam.d/$PROG or
/etc/pam.d/other -

auth            required        pam_unix.so
account         required        pam_unix.so

If the module was pam_deny.so then nothing needs to be checked. This is impossible to do reliably
and in a system independant way as far as I can see.

Define better comments in the calls to AC_DEFINE.

-    /* First we see if this user has a password in the CVS-specific
-       password file.  If so, that's enough to authenticate with.  If
-       not, we'll check /etc/passwd. */
+#define PAM_SERVICENAME "cvs"

Hrm. Can we get $PROG (see src/sanity.sh) in here?


-    rc = check_repository_password (username, password, repository,
-                    &host_user);
+struct cvs_pam_userinfo {
+    char *username;
+    char *password;

-    if (rc == 2)
-    return NULL;
+struct cvs_pam_userinfo *global_userinfo;

-    if (rc == 1)
+static int
+cvs_pam_conv(int num_msg,
+        const struct pam_message **msg,
+        struct pam_response **resp,
+        void *appdata_ptr)
+    int i;
+    struct pam_response *response;
+ struct cvs_pam_userinfo *ui = (struct cvs_pam_userinfo *)appdata_ptr;
+    if(!ui)
-    /* host_user already set by reference, so just return. */
-    goto handle_return;
+    ui = global_userinfo;

Is the check for `if (!ui)' really necessary here? In the HAVE_BROKEN_PAM_APPDATA case does PAM really _sometimes_ pass the pointer back?

I don't know. I got this from the apache pam authentication module. I would guess there is a patch from Sun which fixes the problem which some users have applied. Perhaps I should
remove all this stuff until someone runs into this problem on a real system?

...which print error messages and exit on error and render this error handling code and error code return unecessary.

I hear.

Does PAM really ask you to switch on a msg_style flag and not a username or password flag to determine what sort of data to pass? If so, yuck!

Yes and yes.

This change is a noop.  Please remove it.

Another noop.

I had some problems with my editor and your (old) indentation style especially since the tabstop
needed to be 4 spaces. This will be fixed.

Lastly, your indentation appears to be off. Each new block should be indented four spaces, with tab as an acceptable substitute for 8 spaces.

I read the HACKING file in 1.11.5 - this recommends a completely different indentation style...

Thanks for the comments.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]