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.
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.
+static errno_t write_selinux_login_file(const char *username, char
*string)
{
char *path = NULL;
char *tmp_path = NULL;
@@ -379,8 +385,7 @@ static errno_t write_selinux_string(const char
*username, char *string)
return ENOMEM;
}
- path = talloc_asprintf(tmp_ctx, "%s/logins/%s",
selinux_policy_root(),
- username);
+ path = selogin_path(tmp_ctx, username);
if (path == NULL) {
ret = ENOMEM;
goto done;
@@ -448,7 +453,27 @@ done:
return ret;
}
-static errno_t get_selinux_string(struct pam_auth_req *preq)
+static errno_t remove_selinux_login_file(const char *username)
+{
+ char *path;
+ errno_t ret;
+
+ path = selogin_path(NULL, username);
+ if (!path) return ENOMEM;
+
+ errno = 0;
+ ret = unlink(path);
+ if (ret < 0) {
+ ret = errno;
+ DEBUG(SSSDBG_OP_FAILURE, ("Could not remove login file %s [%
d]: %s\n",
+ path, ret, strerror(ret)));
+ }
+
+ talloc_free(path);
+ return ret;
+}
+
+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) {
- 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 ?
}
- } 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:
@@ -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;
}
--
1.7.11.2
--
Simo Sorce * Red Hat, Inc * New York