Re: PAM authentication patch - v2

Brian Murphy wrote:

> Mark D. Baushke wrote:
>
>> The patch applies cleanly and auto-detects PAM support. I have fully
>> tested it on FreeBSD (where is passes all of the regression tests in
>> sanity.sh), and test-compiled it on Redhat 7.3 GNU/Linux and Solaris 7.
>>  
>>
> Cool. Nice to see. 


Thanks for testing, Mark.

>
>> I suspect that the AC_HELP_STRING for --enable-pam should really
>> indicate that it will be enabled if it is detected.  
>>
> Can do if necessary. I would prefer to wait with another spin until all
> the remaining issues are resolved and then submit one final version (if
> required)
>
>>
>>> +    [Use to enable system authentication with PAM instead of using 
>>> the +    simple getpwnam interface.  This allows authentication (in 
>>> theory) +    with any PAM module, e.g. on systems with shadow 
>>> passwords or via LDAP]), ,   
>>
>>
>> The text probably needs to be rewritten as something like this:
>>
>>    [Use to enable system authentication with PAM instead of using the
>>    simple getpwnam interface (default). This allows authentication (in
>>    theory) with any PAM module (e.g., on systems with shadow passwords
>>    or via LDAP). Use --disable-pam to disable this feature.]), ,
>
> When consensus is reached I will rewrite (or the person who applies 
> the patch can
> do it). 


If PAM is enabled when detected, the AC_HELP_STRING should indicate 
that, but since the voting developers are effectively tied on this 
issue, I will concede Mark's suggestion to have PAM disabled by default 
and wait for feedback before switching the default to enabled.

>
>> To be honest, I am not in favor of this being the default (this is
>> probably not a big surprise to you). I'd rather that the HAVE_PAM logic
>> be separate from USE_PAM logic.
>> It may be that I am only stating the minority opinion, but I think it is
>> worth raising this point for consideration.
>>  
>>
> This is fine. I would like to hear other people opinions on this and I 
> will not
> object if the consensus is to disable PAM detection per default 
> without the extra
> option to configure.
>
>> BTW: You will probably want to send the contrib/pam directory contents
>> separately or diff them each against /dev/null or something... I suppose
>> that Derek could create the pam directory in contrib to make life easier
>> for you.
>>  
>>
> They were with the previous patch version. I just forgot to give -N to 
> diff.
> Just give me the word and I will send a new complete patch but you can 
> just edit the
> previous patch to extract the necessary. I think I have wasted enough 
> bandwith
> already ;-).
>
> /Brian


I will try to test and review the patch myself in the next few days.

Derek

-- 
                *8^)

Email: [email protected]

Get CVS support at <http://ximbiot.com>!
-- 
Do not walk behind me, for I may not lead.  Do not walk ahead of me, for
I may not follow.  Do not walk beside me, either.  Just leave me alone.