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