[SSSD] [PATCH] Process SELinux mapping while offline

Sumit Bose sbose at redhat.com
Tue Jan 8 11:44:54 UTC 2013


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

> 
> [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);
>  }



More information about the sssd-devel mailing list