[SSSD] [PATCH] Process SELinux mapping while offline

Sumit Bose sbose at redhat.com
Tue Jan 8 12:37:50 UTC 2013


On Tue, Jan 08, 2013 at 12:44:54PM +0100, Sumit Bose wrote:
> On Mon, Jan 07, 2013 at 12:29:14AM +0100, Jakub Hrozek wrote:
> > This patchset fixes the SELinux processing so that it works also offline
> > for cases described in #1626 for example.
> > 
> > The code was architected in an extremely strange way where every request
> > would store a per-request/per-user score attribute in the selinux mapping
> > objects themselves and the responder would just pick the highest score.
> > 
> > At the same time, the scores were only calculated when the mappings were
> > downloaded and only for rules that matched, which pretty much broke offline
> > support. I intend to fix the architecture by only making the provider only
> > download and store the rules and let the responder pick the context. But I'm
> > not sure if I can do that right during the current time constraints. I filed
> > #1743 to track that effort and I'll be sending some patches for master so
> > far, but I'd like to include this patchset to fix the functionality at last.
> > 
> > I also noticed that the rules are downloaded on *every login*, without
> > any timeout in between, so I filed #1744 to either reuse
> > IPA_HBAC_REFRESH or introduce a similar timeout for performance reasons.
> > 
> > [PATCH 1/4] SYSDB: Remove duplicate selinux defines
> > Some constants were defined in both sysdb.h and sysdb_selinux.h. This
> > patch removes the duplicates.
> 
> ACK

sorry, ACK was too early, this patch breaks make check. Following patch
fixes it:

diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h
index 01c32f4..66ca8ba 100644
--- a/src/providers/ipa/ipa_opts.h
+++ b/src/providers/ipa/ipa_opts.h
@@ -28,6 +28,7 @@
 #include "db/sysdb_sudo.h"
 #include "db/sysdb_autofs.h"
 #include "db/sysdb_services.h"
+#include "db/sysdb_selinux.h"
 
 struct dp_option ipa_basic_opts[] = {
     { "ipa_domain", DP_OPT_STRING, NULL_STRING, NULL_STRING },



bye,
Sumit

> 
> > 
> > [PATCH 2/4] SYSDB: Split a function to read all SELinux maps
> > This function will be reused in the DP to read maps from cache when
> > offline.
> 
> ACK
> 
> > 
> > [PATCH 3/4] SELINUX: Process maps even when offline
> > The patch is quite big, but I couldn't split it in a meaningful way,
> > sorry. The patch changes the ipa_get_selinux{send,recv} to only provide
> > data independently on the offline stat and moves all the processing to
> > the ipa_selinux_handler. The scores are calculated
> 
> see comments below
> > 
> > [PATCH 4/4] IPA: Rename IPA_CONFIG_SELINUX_DEFAULT_MAP
> > The option IPA_CONFIG_SELINUX_DEFAULT_MAP was misnamed. It doesn't
> > describe any map, but the default context the user should obtain when
> > nothing else matches.
> 
> ACK
> 
> I'll run some test now. Is there anything I should test explicitly?
> 
> bye,
> Sumit
> 
> > @@ -90,23 +90,93 @@ void ipa_selinux_handler(struct be_req *be_req)
> >                               be_req->be_ctx->bet_info[BET_SELINUX].pvt_bet_data,
> >                               struct ipa_selinux_ctx);
> >  
> > +    hostname = dp_opt_get_string(selinux_ctx->id_ctx->ipa_options->basic,
> > +                                 IPA_HOSTNAME);
> > +    if (!hostname) {
> > +        DEBUG(SSSDBG_OP_FAILURE, ("Cannot determine this machine's host name\n"));
> > +        goto fail;
> > +    }
> >  
> > -    req = ipa_get_selinux_send(be_req, pd, selinux_ctx);
> > +    op_ctx = ipa_selinux_create_op_ctx(be_req, be_req->sysdb, pd->user, hostname);
> > +    if (op_ctx == NULL) {
> > +        DEBUG(SSSDBG_OP_FAILURE, ("Cannot create op context\n"));
> > +        goto fail;
> > +    }
> > +    op_ctx->be_req = be_req;
> 
> minor: why not set op_ctx->be_req in ipa_selinux_create_op_ctx as well?
> Then we would be sure that op_ctx is set up properly after
> ipa_selinux_create_op_ctx().
> 
> > +
> > +    req = ipa_get_selinux_send(be_req, op_ctx->user, op_ctx->host, selinux_ctx);
> >      if (req == NULL) {
> > +        DEBUG(SSSDBG_OP_FAILURE, ("Cannot initiate the search\n"));
> >          goto fail;
> >      }
> >  
> > -    tevent_req_set_callback(req, ipa_selinux_handler_done, be_req);
> > -
> > +    tevent_req_set_callback(req, ipa_selinux_handler_done, op_ctx);
> >      return;
> >  
> >  fail:
> >      be_req->fn(be_req, DP_ERR_FATAL, PAM_SYSTEM_ERR, NULL);
> >  }
> >  
> > +static struct ipa_selinux_op_ctx *
> > +ipa_selinux_create_op_ctx(TALLOC_CTX *mem_ctx, struct sysdb_ctx *sysdb,
> > +                          const char *username, const char *hostname)
> > +{
> > +    struct ipa_selinux_op_ctx *op_ctx;
> > +    struct ldb_dn *host_dn;
> > +    const char *attrs[] = { SYSDB_ORIG_DN,
> > +                            SYSDB_ORIG_MEMBEROF,
> > +                            NULL };
> > +    size_t count;
> > +    struct ldb_message **msgs;
> > +    struct sysdb_attrs **hosts;
> > +    errno_t ret;
> > +
> > +    op_ctx = talloc_zero(mem_ctx, struct ipa_selinux_op_ctx);
> > +    if (op_ctx == NULL) {
> > +        return NULL;
> > +    }
> > +
> > +    ret = sss_selinux_extract_user(op_ctx, sysdb, username, &op_ctx->user);
> > +    if (ret != EOK) {
> > +        goto fail;
> > +    }
> > +
> > +    host_dn = sysdb_custom_dn(sysdb, op_ctx, hostname, HBAC_HOSTS_SUBDIR);
> > +    if (host_dn == NULL) {
> > +        goto fail;
> > +    }
> > +
> > +    /* Look up the host to get its originalMemberOf entries */
> > +    ret = sysdb_search_entry(op_ctx, sysdb, host_dn,
> > +                             LDB_SCOPE_BASE, NULL,
> > +                             attrs, &count, &msgs);
> > +    if (ret == ENOENT || count == 0) {
> > +        op_ctx->host = NULL;
> 
> I think a return op_ctx; is missing here
> 
> > +    } else if (ret != EOK) {
> > +        goto fail;
> > +    } else if (count > 1) {
> > +        DEBUG(SSSDBG_OP_FAILURE, ("More than one result for a BASE search!\n"));
> > +        goto fail;
> > +    }
> > +
> > +    ret = sysdb_msg2attrs(op_ctx, count, msgs, &hosts);
> > +    talloc_free(msgs);
> > +    if (ret != EOK) {
> > +        goto fail;
> > +    }
> > +
> > +    op_ctx->host = hosts[0];
> > +    return op_ctx;
> > +
> > +fail:
> > +    talloc_free(op_ctx);
> > +    return NULL;
> > +}
> > +
> >  static void ipa_selinux_handler_done(struct tevent_req *req)
> >  {
> > -    struct be_req *breq = tevent_req_callback_data(req, struct be_req);
> > +    struct ipa_selinux_op_ctx *op_ctx = tevent_req_callback_data(req, struct ipa_selinux_op_ctx);
> > +    struct be_req *breq = op_ctx->be_req;
> >      struct sysdb_ctx *sysdb = breq->be_ctx->sysdb;
> >      errno_t ret, sret;
> >      size_t map_count = 0;
> > @@ -115,13 +185,23 @@ static void ipa_selinux_handler_done(struct tevent_req *req)
> >      char *default_user = NULL;
> >      struct pam_data *pd = talloc_get_type(breq->req_data, struct pam_data);
> >      char *map_order = NULL;
> > +    size_t hbac_count;
> > +    struct sysdb_attrs **hbac_rules;
> 
> Please initialize hbac_count and hbac_rules to avoid compiler warnings.
> 
> >  
> >      ret = ipa_get_selinux_recv(req, breq, &map_count, &maps,
> > +                               &hbac_count, &hbac_rules,
> >                                 &default_user, &map_order);
> >      if (ret != EOK) {
> >          goto fail;
> >      }
> >  
> > +    ret = ipa_selinux_process_maps(op_ctx->user, op_ctx->host,
> > +                                   maps, map_count,
> > +                                   hbac_rules, hbac_count);
> > +    if (ret != EOK) {
> > +        goto fail;
> > +    }
> > +
> >      ret = sysdb_transaction_start(sysdb);
> >      if (ret != EOK) {
> >          DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to start transaction\n"));
> > @@ -153,11 +233,8 @@ static void ipa_selinux_handler_done(struct tevent_req *req)
> >          DEBUG(SSSDBG_OP_FAILURE, ("Could not commit transaction\n"));
> >          goto fail;
> >      }
> > -
> > -    /* Just in case more code will follow after this place in the future */
> >      in_transaction = false;
> >  
> > -
> >      pd->pam_status = PAM_SUCCESS;
> >      breq->fn(breq, DP_ERR_OK, EOK, "Success");
> >      return;
> > @@ -176,9 +253,195 @@ fail:
> >      }
> >  }
> >  
> > -static struct tevent_req *ipa_get_selinux_send(struct be_req *breq,
> > -                                               struct pam_data *pd,
> > -                                               struct ipa_selinux_ctx *selinux_ctx)
> > +static errno_t
> > +ipa_selinux_process_seealso_maps(struct sysdb_attrs *user,
> > +                                 struct sysdb_attrs *host,
> > +                                 struct sysdb_attrs **seealso_rules,
> > +                                 size_t seealso_rules_count,
> > +                                 struct sysdb_attrs **hbac_rules,
> > +                                 size_t hbac_rule_count);
> > +static errno_t
> > +ipa_selinux_process_maps(struct sysdb_attrs *user,
> > +                         struct sysdb_attrs *host,
> > +                         struct sysdb_attrs **selinux_maps,
> > +                         size_t selinux_map_count,
> > +                         struct sysdb_attrs **hbac_rules,
> > +                         size_t hbac_rule_count)
> > +{
> > +    TALLOC_CTX *tmp_ctx;
> > +    int i;
> > +    errno_t ret;
> > +    uint32_t priority = 0;
> > +    struct sysdb_attrs **seealso_rules;
> > +    size_t num_seealso_rules;
> > +    const char *seealso_str;
> > +
> > +    tmp_ctx = talloc_new(NULL);
> > +    if (!tmp_ctx) {
> > +        return ENOMEM;
> > +    }
> > +
> > +    seealso_rules = talloc_zero_array(tmp_ctx, struct sysdb_attrs *,
> > +                                      selinux_map_count + 1);
> > +    if (seealso_rules == NULL) {
> > +        ret = ENOMEM;
> > +        goto done;
> > +    }
> > +    num_seealso_rules = 0;
> > +
> > +    for (i = 0; i < selinux_map_count; i++) {
> > +        if (sss_selinux_match(selinux_maps[i], user,
> > +                              host, &priority)) {
> > +            priority &= ~(SELINUX_PRIORITY_USER_NAME |
> > +                          SELINUX_PRIORITY_USER_GROUP |
> > +                          SELINUX_PRIORITY_USER_CAT);
> > +            ret = sysdb_attrs_add_uint32(selinux_maps[i],
> > +                                         SYSDB_SELINUX_HOST_PRIORITY,
> > +                                         priority);
> > +            if (ret != EOK) {
> > +                goto done;
> > +            }
> > +            continue;
> > +        }
> > +
> > +        ret = sysdb_attrs_get_string(selinux_maps[i],
> > +                                     SYSDB_SELINUX_SEEALSO, &seealso_str);
> > +        if (ret == ENOENT) {
> > +            continue;
> > +        } else if (ret != EOK) {
> > +            goto done;
> > +        }
> > +
> > +        seealso_rules[num_seealso_rules] = selinux_maps[i];
> > +        num_seealso_rules++;
> > +    }
> > +
> > +    ret = ipa_selinux_process_seealso_maps(user, host,
> > +                                           seealso_rules, num_seealso_rules,
> > +                                           hbac_rules, hbac_rule_count);
> > +    if (ret != EOK) {
> > +        goto done;
> > +    }
> > +
> > +    ret = EOK;
> > +done:
> > +    talloc_free(tmp_ctx);
> > +    return ret;
> > +}
> > +
> > +static errno_t
> > +ipa_selinux_process_seealso_maps(struct sysdb_attrs *user,
> > +                                 struct sysdb_attrs *host,
> > +                                 struct sysdb_attrs **seealso_rules,
> > +                                 size_t seealso_rules_count,
> > +                                 struct sysdb_attrs **hbac_rules,
> > +                                 size_t hbac_rule_count)
> > +{
> > +    int i, j;
> > +    errno_t ret;
> > +    struct ldb_message_element *el;
> > +    uint32_t priority = 0;
> > +    struct sysdb_attrs *usermap;
> > +    const char *seealso_dn;
> > +    const char *hbac_dn;
> > +
> > +    for (i = 0; i < hbac_rule_count; i++) {
> > +        ret = sysdb_attrs_get_string(hbac_rules[i], SYSDB_ORIG_DN, &hbac_dn);
> > +        if (ret != EOK) {
> > +            return ret;
> > +        }
> > +
> > +        /* We need to do this translation for further processing. We have to
> > +         * do it manually because no map was used to retrieve HBAC rules.
> > +         */
> > +        ret = sysdb_attrs_get_el(hbac_rules[i], IPA_MEMBER_HOST, &el);
> > +        if (ret != EOK) return ret;
> > +        el->name = SYSDB_ORIG_MEMBER_HOST;
> > +
> > +        ret = sysdb_attrs_get_el(hbac_rules[i], IPA_MEMBER_USER, &el);
> > +        if (ret != EOK) return ret;
> > +        el->name = SYSDB_ORIG_MEMBER_USER;
> > +
> > +        DEBUG(SSSDBG_TRACE_ALL,
> > +              ("Matching HBAC rule %s with SELinux mappings\n", hbac_dn));
> > +
> > +        if (!sss_selinux_match(hbac_rules[i], user, host, &priority)) {
> > +            DEBUG(SSSDBG_TRACE_ALL, ("Rule did not match\n"));
> > +            continue;
> > +        }
> > +
> > +        /* HBAC rule matched, find if it is in the "possible" list */
> > +        for (j = 0; j < seealso_rules_count; j++) {
> > +            usermap = seealso_rules[j];
> > +            if (usermap == NULL) {
> > +                continue;
> > +            }
> > +
> > +            ret = sysdb_attrs_get_string(usermap, SYSDB_SELINUX_SEEALSO, &seealso_dn);
> > +            if (ret != EOK) {
> > +                return ret;
> > +            }
> > +
> > +            if (strcasecmp(hbac_dn, seealso_dn) == 0) {
> > +                DEBUG(SSSDBG_TRACE_FUNC, ("HBAC rule [%s] matched, copying its"
> > +                                          "attributes to SELinux user map [%s]\n",
> > +                                          hbac_dn, seealso_dn));
> > +                priority &= ~(SELINUX_PRIORITY_USER_NAME |
> > +                              SELINUX_PRIORITY_USER_GROUP |
> > +                              SELINUX_PRIORITY_USER_CAT);
> > +                ret = sysdb_attrs_add_uint32(usermap,
> > +                                             SYSDB_SELINUX_HOST_PRIORITY,
> > +                                             priority);
> > +                if (ret != EOK) {
> > +                    return ret;
> > +                }
> > +
> > +                ret = sysdb_attrs_copy_values(hbac_rules[i], usermap, SYSDB_ORIG_MEMBER_USER);
> > +                if (ret != EOK) {
> > +                    return ret;
> > +                }
> > +
> > +                ret = sysdb_attrs_copy_values(hbac_rules[i], usermap, SYSDB_USER_CATEGORY);
> > +                if (ret != EOK) {
> > +                    return ret;
> > +                }
> > +
> > +                /* Speed up the next iteration */
> > +                seealso_rules[j] = NULL;
> > +            }
> > +        }
> > +    }
> > +
> > +    return EOK;
> > +}
> > +
> > +/* A more generic request to gather all SELinux and HBAC rules. Updates
> > + * cache if necessary
> > + */
> > +struct ipa_get_selinux_state {
> > +    struct be_req *be_req;
> > +    struct ipa_selinux_ctx *selinux_ctx;
> > +    struct sdap_id_op *op;
> > +
> > +    struct sysdb_attrs *host;
> > +    struct sysdb_attrs *user;
> > +
> > +    struct sysdb_attrs *defaults;
> > +    struct sysdb_attrs **selinuxmaps;
> > +    size_t nmaps;
> > +
> > +    struct sysdb_attrs **hbac_rules;
> > +    size_t hbac_rule_count;
> > +};
> > +
> > +static errno_t
> > +ipa_get_selinux_maps_offline(struct tevent_req *req);
> > +
> > +static struct tevent_req *
> > +ipa_get_selinux_send(struct be_req *breq,
> > +                     struct sysdb_attrs *user,
> > +                     struct sysdb_attrs *host,
> > +                     struct ipa_selinux_ctx *selinux_ctx)
> >  {
> >      struct tevent_req *req;
> >      struct tevent_req *subreq;
> > @@ -194,8 +457,9 @@ static struct tevent_req *ipa_get_selinux_send(struct be_req *breq,
> >      }
> >  
> >      state->be_req = breq;
> > -    state->pd = pd;
> >      state->selinux_ctx = selinux_ctx;
> > +    state->user = user;
> > +    state->host = host;
> >  
> >      offline = be_is_offline(bctx);
> >      DEBUG(SSSDBG_TRACE_INTERNAL, ("Connection status is [%s].\n",
> > @@ -219,7 +483,7 @@ static struct tevent_req *ipa_get_selinux_send(struct be_req *breq,
> >  
> >          tevent_req_set_callback(subreq, ipa_get_selinux_connect_done, req);
> >      } else {
> > -        ret = EAGAIN;
> > +        ret = ipa_get_selinux_maps_offline(req);
> >          goto immediate;
> >      }
> >  
> > @@ -248,67 +512,41 @@ static void ipa_get_selinux_connect_done(struct tevent_req *subreq)
> >  
> >      const char *access_name;
> >      const char *selinux_name;
> > -    struct ldb_dn *host_dn;
> > -    const char *attrs[] = { SYSDB_ORIG_DN,
> > -                            SYSDB_ORIG_MEMBEROF,
> > -                            NULL };
> > -    size_t count;
> > -    struct ldb_message **msgs;
> > -    struct sysdb_attrs **hosts;
> > +    const char *hostname;
> >  
> >      ret = sdap_id_op_connect_recv(subreq, &dp_error);
> >      talloc_zfree(subreq);
> >  
> >      if (dp_error == DP_ERR_OFFLINE) {
> >          talloc_zfree(state->op);
> > -        ret = EAGAIN;
> > +        ret = ipa_get_selinux_maps_offline(req);
> > +        goto fail;
> 
> Is goto fail; rigth here? If ipa_get_selinux_maps_offline() is
> successful tevent_req_error() is called with ret==EOK, which might lead
> to confusion at other places?
> 
> >      }
> >  
> >      if (ret != EOK) {
> >          goto fail;
> >      }
> >  
> > -    state->hostname = dp_opt_get_string(state->selinux_ctx->id_ctx->ipa_options->basic,
> > -                                        IPA_HOSTNAME);
> > -
> >      access_name = state->be_req->be_ctx->bet_info[BET_ACCESS].mod_name;
> >      selinux_name = state->be_req->be_ctx->bet_info[BET_SELINUX].mod_name;
> > -    if (strcasecmp(access_name, selinux_name) == 0) {
> > -        host_dn = sysdb_custom_dn(bctx->sysdb, state, state->hostname, HBAC_HOSTS_SUBDIR);
> > -        if (host_dn == NULL) {
> > -            ret = ENOMEM;
> > -            goto fail;
> > -        }
> > -
> > -        /* Look up the host to get its originalMemberOf entries */
> > -        ret = sysdb_search_entry(state, bctx->sysdb, host_dn,
> > -                                 LDB_SCOPE_BASE, NULL,
> > -                                 attrs, &count, &msgs);
> > -        if (ret == ENOENT || count == 0) {
> > -            /* We need to query the server */
> > -            goto server;
> > -        } else if (ret != EOK) {
> > -            goto fail;
> > -        } else if (count > 1) {
> > -            DEBUG(SSSDBG_OP_FAILURE, ("More than one result for a BASE search!\n"));
> > -            ret = EIO;
> > -            goto fail;
> > -        }
> > -
> > -        ret = sysdb_msg2attrs(state, count, msgs, &hosts);
> > -        if (ret != EOK) {
> > -            goto fail;
> > -        }
> > -
> > -        state->host = hosts[0];
> > +    if (strcasecmp(access_name, selinux_name) == 0 && state->host != NULL) {
> > +        /* If the access control module is the same as the selinux module
> > +         * and the access control had already discovered the host
> > +         */
> >          return ipa_get_config_step(req);
> >      }
> >  
> > -server:
> > +    hostname = dp_opt_get_string(state->selinux_ctx->id_ctx->ipa_options->basic,
> > +                                        IPA_HOSTNAME);
> > +    if (hostname == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot determine the host name\n"));
> > +        goto fail;
> > +    }
> > +
> >      subreq = ipa_host_info_send(state, bctx->ev, bctx->sysdb,
> >                                  sdap_id_op_handle(state->op),
> >                                  id_ctx->sdap_id_ctx->opts,
> > -                                state->hostname,
> > +                                hostname,
> >                                  id_ctx->ipa_options->host_map,
> >                                  NULL,
> >                                  state->selinux_ctx->host_search_bases);
> > @@ -318,13 +556,92 @@ server:
> >      }
> >  
> >      tevent_req_set_callback(subreq, ipa_get_selinux_hosts_done, req);
> > -
> >      return;
> >  
> >  fail:
> >      tevent_req_error(req, ret);
> >  }
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list