[SSSD] [PATCH] SELinux: Always use the default if it exists on the server

Jakub Hrozek jhrozek at redhat.com
Thu Sep 13 14:48:28 UTC 2012


On Thu, Sep 13, 2012 at 03:24:05PM +0200, Pavel Březina wrote:
> On 09/13/2012 12:24 PM, Jakub Hrozek wrote:
> >In order to test these patches properly, you need to be running the
> >latest patches that are currently on review on freeipa-devel. Ping me
> >off-line for access to such IPA client.
> >
> >[PATCH 1/2] DB: Always write the SELinux object to sysdb
> >This is a fallout of the transaction processing refactoring.
> >
> >There's no point in checking if the object already exists because we always
> >wipe the whole sysdb subtree. We were also immediatelly cancelling the
> >transaction because we'd jump to goto, even though it was with EOK.
> 
> Ack.
> 
> >[PATCH 2/2] SELinux: Always use the default if it exists on the server
> >
> >https://fedorahosted.org/sssd/ticket/1513
> >
> >This is a counterpart of the FreeIPA ticket https://fedorahosted.org/freeipa/ticket/3045
> >
> >During an e-mail discussion, it was decided that
> >
> >     * if the default is set in the IPA config object, the SSSD would use
> >       that default no matter what
> >     * if the default is not set (aka empty or missing), the SSSD would just
> >       use the system default and skip creating the login file altogether
> 
> Nack.
> 
> >@@ -136,11 +136,9 @@ static void ipa_selinux_handler_done(struct tevent_req *req)
> >         goto fail;
> >     }
> >
> >-    if (default_user != NULL && map_order != NULL) {
> >-        ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
> >-        if (ret != EOK) {
> >-            goto fail;
> >-        }
> >+    ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
> >+    if (ret != EOK) {
> >+        goto fail;
> >     }
> 
> Are you sure that it is safe to remove the NULL checks? You are
> testing default_user inside sysdb_store_selinux_config(), but not
> map_order. There is a code path where map_order may be NULL.

Right, I should have kept the map_order check. I moved it into the save
function itself.
-------------- next part --------------
>From 1383854d46d5997b25998d58b7e150c4989937c1 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 13 Sep 2012 05:44:12 -0400
Subject: [PATCH 1/2] DB: Always write the SELinux object to sysdb

There's no point in checking if the object already exists because we always
wipe the whole sysdb subtree. We were also immediatelly cancelling the
transaction because we'd jump to goto, even though it was with EOK.
---
 src/db/sysdb_selinux.c | 78 ++++++--------------------------------------------
 1 file changed, 8 insertions(+), 70 deletions(-)

diff --git a/src/db/sysdb_selinux.c b/src/db/sysdb_selinux.c
index 565b7612fe1618cbe951317227b9001af549ca0e..bc067225b7ea2f921f0c21c2482285da62f600c0 100644
--- a/src/db/sysdb_selinux.c
+++ b/src/db/sysdb_selinux.c
@@ -24,48 +24,6 @@
 #include "db/sysdb_private.h"
 
 /* Some generic routines */
-static errno_t get_rm_msg(TALLOC_CTX *mem_ctx,
-                          struct ldb_message *old_msg,
-                          struct sysdb_attrs *new_attrs,
-                          struct ldb_message **_msg)
-{
-    struct ldb_message *rm_msg;
-    const char *tmp_str;
-    errno_t ret;
-    int i;
-
-    rm_msg = ldb_msg_new(mem_ctx);
-    if (rm_msg == NULL) {
-        ret = ENOMEM;
-        goto done;
-    }
-
-    rm_msg->dn = old_msg->dn;
-    rm_msg->elements = talloc_zero_array(rm_msg, struct ldb_message_element,
-                                         old_msg->num_elements);
-    rm_msg->num_elements = 0;
-
-    for (i = 0; i < old_msg->num_elements; i++) {
-        ret = sysdb_attrs_get_string(new_attrs, old_msg->elements[i].name, &tmp_str);
-        if (ret != ENOENT) {
-            continue;
-        }
-
-        rm_msg->elements[rm_msg->num_elements] = old_msg->elements[i];
-        rm_msg->elements[rm_msg->num_elements].flags = LDB_FLAG_MOD_DELETE;
-        rm_msg->num_elements++;
-    }
-
-    ret = EOK;
-done:
-    if (ret != EOK) {
-        talloc_free(rm_msg);
-    } else {
-        *_msg = rm_msg;
-    }
-
-    return ret;
-}
 
 static errno_t
 sysdb_add_selinux_entity(struct sysdb_ctx *sysdb,
@@ -123,8 +81,6 @@ static errno_t sysdb_store_selinux_entity(struct sysdb_ctx *sysdb,
                                           enum selinux_entity_type type)
 {
     TALLOC_CTX *tmp_ctx;
-    struct ldb_message *msg;
-    struct ldb_message *rm_msg;
     bool in_transaction = false;
     const char *objectclass = NULL;
     const char *name;
@@ -185,18 +141,8 @@ static errno_t sysdb_store_selinux_entity(struct sysdb_ctx *sysdb,
     ret = sysdb_attrs_add_time_t(attrs, SYSDB_LAST_UPDATE, now);
     if (ret) goto done;
 
-    if (type == SELINUX_CONFIG) {
-        ret = sysdb_search_selinux_config(tmp_ctx, sysdb, NULL, &msg);
-    } else if (type == SELINUX_USER_MAP) {
-        ret = sysdb_search_selinux_usermap_by_mapname(tmp_ctx, sysdb, name,
-                                                      NULL, &msg);
-    }
-
-    if (ret && ret != ENOENT) {
-        goto done;
-    }
-    if (ret == ENOENT) {
-        ret = sysdb_add_selinux_entity(sysdb, dn, objectclass, attrs, now);
+    ret = sysdb_add_selinux_entity(sysdb, dn, objectclass, attrs, now);
+    if (ret != EOK) {
         goto done;
     }
 
@@ -205,16 +151,6 @@ static errno_t sysdb_store_selinux_entity(struct sysdb_ctx *sysdb,
         goto done;
     }
 
-    /* Now delete attributes which are no longer present */
-    ret = get_rm_msg(tmp_ctx, msg, attrs, &rm_msg);
-    if (ret != EOK) {
-        goto done;
-    }
-
-    if (rm_msg->num_elements > 0) {
-        ret = ldb_modify(sysdb->ldb, rm_msg);
-    }
-
     ret = sysdb_transaction_commit(sysdb);
     if (ret != EOK) {
         DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to commit transaction\n"));
@@ -255,10 +191,12 @@ errno_t sysdb_store_selinux_config(struct sysdb_ctx *sysdb,
         return ENOMEM;
     }
 
-    ret = sysdb_attrs_add_string(attrs, SYSDB_SELINUX_DEFAULT_USER,
-                                 default_user);
-    if (ret != EOK) {
-        goto done;
+    if (default_user) {
+        ret = sysdb_attrs_add_string(attrs, SYSDB_SELINUX_DEFAULT_USER,
+                                    default_user);
+        if (ret != EOK) {
+            goto done;
+        }
     }
 
     ret = sysdb_attrs_add_string(attrs, SYSDB_SELINUX_DEFAULT_ORDER,
-- 
1.7.11.4

-------------- next part --------------
>From e0afdeabffbd4cfa76c68454814ac7db5a243d2e Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 13 Sep 2012 10:07:29 +0200
Subject: [PATCH 2/2] SELinux: Always use the default if it exists on the
 server

https://fedorahosted.org/sssd/ticket/1513

This is a counterpart of the FreeIPA ticket https://fedorahosted.org/freeipa/ticket/3045

During an e-mail discussion, it was decided that

    * if the default is set in the IPA config object, the SSSD would use
      that default no matter what
    * if the default is not set (aka empty or missing), the SSSD
      would just use the system default and skip creating the login
      file altogether
---
 src/db/sysdb_selinux.c          | 11 +++++++++--
 src/providers/ipa/ipa_selinux.c | 18 ++++++++---------
 src/responder/pam/pamsrv_cmd.c  | 43 ++++++++++++++++++++---------------------
 3 files changed, 39 insertions(+), 33 deletions(-)

diff --git a/src/db/sysdb_selinux.c b/src/db/sysdb_selinux.c
index bc067225b7ea2f921f0c21c2482285da62f600c0..857b17d956babc07670901d06f7219ce8a6f72a5 100644
--- a/src/db/sysdb_selinux.c
+++ b/src/db/sysdb_selinux.c
@@ -191,6 +191,11 @@ errno_t sysdb_store_selinux_config(struct sysdb_ctx *sysdb,
         return ENOMEM;
     }
 
+    if (!order) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("The SELinux order is missing\n"));
+        return EINVAL;
+    }
+
     if (default_user) {
         ret = sysdb_attrs_add_string(attrs, SYSDB_SELINUX_DEFAULT_USER,
                                     default_user);
@@ -205,7 +210,7 @@ errno_t sysdb_store_selinux_config(struct sysdb_ctx *sysdb,
         goto done;
     }
 
-    ret =  sysdb_store_selinux_entity(sysdb, attrs, SELINUX_CONFIG);
+    ret = sysdb_store_selinux_entity(sysdb, attrs, SELINUX_CONFIG);
 done:
     talloc_free(attrs);
     return ret;
@@ -344,7 +349,9 @@ errno_t sysdb_search_selinux_usermap_by_username(TALLOC_CTX *mem_ctx,
 
     ret = sysdb_search_entry(tmp_ctx, sysdb, basedn, LDB_SCOPE_SUBTREE, filter,
                              attrs, &msgs_count, &msgs);
-    if (ret) {
+    if (ret == ENOENT) {
+        msgs_count = 0;
+    } else if (ret) {
         goto done;
     }
 
diff --git a/src/providers/ipa/ipa_selinux.c b/src/providers/ipa/ipa_selinux.c
index 36a2bfb4a43a03afc17da5faa5f564dcba5b8367..0adc0fd02e110a4d4bb3168e34c99c217ead6f74 100644
--- a/src/providers/ipa/ipa_selinux.c
+++ b/src/providers/ipa/ipa_selinux.c
@@ -136,11 +136,9 @@ static void ipa_selinux_handler_done(struct tevent_req *req)
         goto fail;
     }
 
-    if (default_user != NULL && map_order != NULL) {
-        ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
-        if (ret != EOK) {
-            goto fail;
-        }
+    ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
+    if (ret != EOK) {
+        goto fail;
     }
 
     if (map_count > 0 && maps != NULL) {
@@ -668,13 +666,15 @@ ipa_get_selinux_recv(struct tevent_req *req,
     if (state->defaults != NULL) {
         ret = sysdb_attrs_get_string(state->defaults, IPA_CONFIG_SELINUX_DEFAULT_MAP,
                                      &tmp_str);
-        if (ret != EOK) {
+        if (ret != EOK && ret != ENOENT) {
             return ret;
         }
 
-        *default_user = talloc_strdup(mem_ctx, tmp_str);
-        if (*default_user == NULL) {
-            return ENOMEM;
+        if (ret == EOK) {
+            *default_user = talloc_strdup(mem_ctx, tmp_str);
+            if (*default_user == NULL) {
+                return ENOMEM;
+            }
         }
 
         ret = sysdb_attrs_get_string(state->defaults, IPA_CONFIG_SELINUX_MAP_ORDER,
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 4c0356832df29b849229f424e1f393d6b3ffa687..07fa96ab83a46e82a4f501e2d73cf0ef167c3cba 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -519,30 +519,33 @@ static errno_t process_selinux_mappings(struct pam_auth_req *preq)
         goto done;
     }
 
-    /* We need two values from the config object:
-     * - default SELinux user in case no other is available
-     * - the order for fetched usermaps
-     */
-    for (i = 0; i < config->num_elements; i++) {
-        if (strcasecmp(config->elements[i].name, SYSDB_SELINUX_DEFAULT_USER) == 0) {
-            default_user = (const char *)config->elements[i].values[0].data;
-        } else if (strcasecmp(config->elements[i].name, SYSDB_SELINUX_DEFAULT_ORDER) == 0) {
-            tmp_str = (char *)config->elements[i].values[0].data;
-            len = config->elements[i].values[0].length;
-            order = talloc_strdup(tmp_ctx, tmp_str);
-            if (order == NULL) {
-                goto done;
-            }
-        }
+    default_user = ldb_msg_find_attr_as_string(config,
+                                               SYSDB_SELINUX_DEFAULT_USER,
+                                               NULL);
+    if (!default_user || default_user[0] == '\0') {
+        /* Skip creating the maps altogether if there is no default
+         * or empty default
+         */
+        ret = EOK;
+        goto done;
     }
 
-    if (default_user == NULL || order == NULL) {
-        DEBUG(SSSDBG_OP_FAILURE, ("No default SELinux user "
-                                  "or map order given!\n"));
+    tmp_str = ldb_msg_find_attr_as_string(config,
+                                          SYSDB_SELINUX_DEFAULT_ORDER,
+                                          NULL);
+    if (tmp_str == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("No map order given!\n"));
         ret = EINVAL;
         goto done;
     }
 
+    order = talloc_strdup(tmp_ctx, tmp_str);
+    if (order == NULL) {
+        ret = ENOMEM;
+        goto done;
+    }
+    len = strlen(order);
+
     /* The "order" string contains one or more SELinux user records
      * separated by $. Now we need to create an array of string from
      * this one string. First find out how many elements in the array
@@ -577,10 +580,6 @@ static errno_t process_selinux_mappings(struct pam_auth_req *preq)
                                                    &usermaps);
     if (ret != EOK && ret != ENOENT) {
         goto done;
-    } else if (ret == ENOENT) {
-        DEBUG(SSSDBG_TRACE_FUNC, ("No maps defined on the server\n"));
-        ret = EOK;
-        goto done;
     }
 
     /* If no maps match, we'll use the default SELinux user from the
-- 
1.7.11.4



More information about the sssd-devel mailing list