Re: PAM authentication patch - v2

>
>> +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