[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