[SSSD] Only create the SELinux login file if there are mappings on the server

Pavel Březina pbrezina at redhat.com
Tue Aug 14 09:11:01 UTC 2012


On 08/07/2012 10:01 AM, Jakub Hrozek wrote:
> On Mon, Aug 06, 2012 at 09:31:07AM -0400, Simo Sorce wrote:
>> On Mon, 2012-08-06 at 10:51 +0200, Jakub Hrozek wrote:
>>> [PATCH 1/2] Do not try to remove the temp login file if already
>>> renamed
>>>
>>> write_selinux_string() would try to unlink the temporary file even
>>> after
>>> it was renamed. Failure to unlink the file would not be fatal, but
>>> would
>>> produce a confusing error message.
>>>
>>> Also don't use "0" for the default fd number, that's reserved for
>>> stdin.
>>> Using -1 is safer.
>>>
>>> [PATCH 2/2] Only create the SELinux login file if there are mappings
>>> on the server
>>>
>>> https://fedorahosted.org/sssd/ticket/1455
>>>
>>> In case there are no rules on the IPA server, we must simply avoid
>>> generating
>>> the login file. That would make us fall back to the system-wide
>>> default
>>> defined in /etc/selinux/targeted/seusers.
>>>
>>> The IPA default must be only used if there *are* rules on the server,
>>> but none matches.
>>
>> Minor nitpicks, but I agree with the patches purposes, so auto-ACK when
>> nitpicks are addressed.
>>
>>
>>>  From 87ec8c6d2bfeed404672ddca114cc8d0e1497346 Mon Sep 17 00:00:00 2001
>>> From: Jakub Hrozek <jhrozek at redhat.com>
>>> Date: Sun, 5 Aug 2012 22:03:11 +0200
>>> Subject: [PATCH 1/2] Do not try to remove the temp login file if
>>> already
>>>   renamed
>>>
>>>   done:
>>> -    if (fd > 0) {
>>> +    if (fd > -1) {
>>
>> use fd != -1 here, it makes it clear only -1 is a special value.
>
> Done.
>
>>
>>
>>>  From 90bd88e09e185dbcdaec17b9a8a619a0fdc9589a Mon Sep 17 00:00:00 2001
>>> From: Jakub Hrozek <jhrozek at redhat.com>
>>> Date: Sun, 5 Aug 2012 22:37:09 +0200
>>> Subject: [PATCH 2/2] Only create the SELinux login file if there are
>>> mappings
>>>   on the server
>>
>>
>>> -static errno_t write_selinux_string(const char *username, char
>>> *string)
>>> +static char *selogin_path(TALLOC_CTX *mem_ctx, const char *username)
>>> +{
>>> +    return talloc_asprintf(mem_ctx, "%s/logins/%s",
>>> selinux_policy_root(),
>>> +                           username);
>>> +}
>>
>> I would use a macro here, a whole function for a oneliner seem
>> excessive.
>
> OK, I assumed the function would be inlined by the compiler but wanted to
> keep how the login file path is constructed at one place, so that in case
> it changed in the future, there's only one place to change.
>
>>> +static errno_t process_selinux_mappings(struct pam_auth_req *preq)
>>>   {
>>>       struct sysdb_ctx *sysdb;
>>>       TALLOC_CTX *tmp_ctx;
>>> @@ -549,13 +574,16 @@ static errno_t get_selinux_string(struct
>>> pam_auth_req *preq)
>>>       }
>>>
>>>       if (ret == ENOENT) {
>
> I also moved this ENOENT handler together with handling of other errors
> to get rid of one level of indentation. I think the function is more
> compact and readable now.
>
>>> -        DEBUG(SSSDBG_TRACE_FUNC, ("No user maps found, using
>>> default!"));
>>> +        DEBUG(SSSDBG_TRACE_FUNC, ("No maps defined on the server
>>> \n"));
>>> +    } else {
>>> +        /* If no maps match, we'll use the default SELinux user from
>>> the
>>> +           config */
>>>           file_content = talloc_strdup(tmp_ctx, default_user);
>>>           if (file_content == NULL) {
>>>               ret = ENOMEM;
>>>               goto done;
>>
>> This error condition will leave the existing selinux users file in
>> place, shouldn't you remove it in the error path instead of later in the
>> body of the function ?
>
> Probably yes, I didn't do it earlier because removing the file triggers
> an allocation, which is likely to fail, too, if you're handling an
> ENOMEM situation.
>
>
>>
>>>           }
>>> -    } else {
>>> +
>>>           /* Iterate through the order array and try to find SELinux
>>> users
>>>            * in fetched maps. The order array contains all SELinux
>>> users
>>>            * allowed in the domain in the same order they should appear
>>> @@ -589,7 +617,9 @@ static errno_t get_selinux_string(struct
>>> pam_auth_req *preq)
>>>       }
>>>
>>>       if (file_content) {
>>> -        ret = write_selinux_string(pd->user, file_content);
>>> +        ret = write_selinux_login_file(pd->user, file_content);
>>> +    } else {
>>> +        ret = remove_selinux_login_file(pd->user);
>>>       }
>>
>> See above comment, shoudln;t this be done after the 'done' label ?
>
> Done in 'done' now :-)
>
>>
>>>   done:
>>> @@ -796,7 +826,7 @@ static void pam_reply(struct pam_auth_req *preq)
>>>           pd->pam_status == PAM_SUCCESS) {
>>>           /* Try to fetch data from sysdb
>>>            * (auth already passed -> we should have them) */
>>> -        ret = get_selinux_string(preq);
>>> +        ret = process_selinux_mappings(preq);
>>>           if (ret != EOK) {
>>>               pd->pam_status = PAM_SYSTEM_ERR;
>>>           }
>
> New patches are attached.

Nack.

User cannot login when the loginfile does not exist because you return 
!EOK here:

     if (!file_content) {
         err = remove_selinux_login_file(pd->user);
         /* Don't overwrite original error condition if there was one */
         if (ret == EOK) ret = err;
     }
     talloc_free(tmp_ctx);
     return ret;



More information about the sssd-devel mailing list