Re: PAM authentication patch - v2

[Brian Murphy]

> > +AC_ARG_WITH(
> > +  [broken-pam-appdata],
> > +  AC_HELP_STRING(
> > +    [--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?   

Yes.

> > -    rc = check_repository_password (username, password, repository,
> > -                    &host_user);
> > +struct cvs_pam_userinfo {
> > +    char *username;
> > +    char *password;
> > +};
> >
> > -    if (rc == 2)
> > -    return NULL;
> > +#ifdef HAVE_BROKEN_PAM_APPDATA
> > +struct cvs_pam_userinfo *global_userinfo;
> > +#endif
> >
> > -    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;
> > +
> > +#ifdef HAVE_BROKEN_PAM_APPDATA
> > +    if(!ui)
> >     {
> > -    /* host_user already set by reference, so just return. */
> > -    goto handle_return;
> > +    ui = global_userinfo;
> >     }
> > +#endif
>
> 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.

/Brian