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(a)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(a)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;