[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