[SSSD] [PATCH] Two enhancements for PAM client

Stephen Gallagher sgallagh at redhat.com
Mon Apr 26 12:13:40 UTC 2010


On 04/26/2010 07:41 AM, Sumit Bose wrote:
> On Thu, Apr 22, 2010 at 09:03:25AM -0400, Stephen Gallagher wrote:
>> On 04/22/2010 06:08 AM, Sumit Bose wrote:
>>> Hi,
>>>
>>> the two patches attached should fix #446 and #417 respectively.
>>>
>>> For #417 a different solution, where the message is generated by SSSD
>>> and send to the client, would be possilbe. But I decided against it,
>>> because with the attached patch it is possilbe to support localized
>>> messages.
>>
>>
>> Patch 0001: Ack.
>>
>> Patch 0002: Nack. You would be introducing a security hole here in the
>> form of a stat-open race condition. Please open the file first and use
>> fstat() to determine its size. It probably wouldn't be a bad idea to
>> verify that the file is owned by root and not writable by anyone but
>> root. I don't think it's important to verify whether the file is a
>> symlink or a regular file.
>
> done
>
>>
>> Even making the above check, it wouldn't be a bad idea to verify that
>> total_len == stat_buf.st_size after the read loop.
>
> done
>
>>
>> The DEBUG message in the read loop is wrong. It should be "read failed",
>> not "open failed".
>
> done
>
>>
>> Also, if you're going to allow custom messages here, we probably need to
>> make this message file be per-domain. Since it may be translatable, I
>> suggest we add a new directory structure under /etc/sssd that looks like
>> this (assuming the domain name is LDAP):
>>
>> /etc/sssd/customize
>> /etc/sssd/customize/LDAP/
>> /etc/sssd/customize/LDAP/root_pw_reset.txt
>> /etc/sssd/customize/LDAP/root_pw_reset.zh_TW
>> ...
>>
>
> done
>
> new version attached.
>
> bye,
> Sumit
>

Nack.

Minor nitpick: s/then/than/ in "read fewer bytes [%d] then expected 
[%d]."  And also, you do not have any variables there to handle the two 
%d substitutions.

Please explain why you would not have a domain_name in 
select_pw_reset_message(). By the time you have received a message from 
PAM that the password needs to be reset, you should absolutely know 
which domain you're in. If domain is NULL here, that's almost certainly 
an error. I don't think it should be possible to specify
/etc/sssd/customize/root_pw_reset.* This is unneeded complexity, since 
we will always have a true default message, and we should never need 
this if, as mentioned above, we're properly detecting the domain.

-- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/



More information about the sssd-devel mailing list