On 09/21/2012 05:41 AM, Jakub Hrozek wrote:
> On Thu, Sep 20, 2012 at 09:34:37AM -0400, Stephen Gallagher wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>
>> * [PATCH 1/2] AD: Detect domain controller compatibility version
>> This patch allows us to read the domain controller version from
>> the RootDSE, if it exists. This will be used by the next patch
>> to determine if the SSSD can use tokenGroups for lookups.
>>
>> * [PATCH 2/2] AD: Optimize initgroups lookups with tokenGroups
>> If we are doing ID-mapped lookups, we can take advantage of the
>> AD-specific option "tokenGroups", which allows us to retrieve a
>> pre-flattened list of all the group SIDs for which this user is a
>> member. From this, we can convert them to GIDs and conclude the
>> initgroups() lookup with only two LDAP queries (the search for
>> the user and the base search for the user's tokengroups; the
>> latter must be performed as a base search due to restrictions
>> imposed on the tokenGroups attribute by AD).
>>
>> When we save these groups to the sysdb, if the group is being
>> seen for the first time it will be temporarily given the name of
>> the SID representing it. Later, when we look it up by name or
>> GID, this group will be replaced with the real one.
>>
>
> I only have one question:
>
> + /* Get the list of group SIDs */ + ret =
> sysdb_attrs_get_el_ext(users[0], AD_TOKENGROUPS_ATTR, + false,
> &el); + if (ret != EOK) { + if (ret == ENOENT) { +
> DEBUG(SSSDBG_TRACE_LIBS, + ("No tokenGroups
> entries for [%s]\n", + state->username));
>
> Should we just return success here? Does a missing togenGroup
> imply a user that does not belong to any group?
>
> + } + DEBUG(SSSDBG_MINOR_FAILURE, + ("Could not read
> tokenGroups attribute: [%s]\n", + strerror(ret))); + goto
> done; + }
>
You're right, I meant to do more with that IF block and forgot about
it (though in reality, that's a branch we should never take since all
users are members of at least the built-in "Users" group that we later
filter out).
Just returning success here isn't sufficient, though. We need to make
sure that the sysdb matches. The attached patch creates a new
ldb_message_element with zero members that will short-circuit the
group-processing loop below and advance on to where we correct the
group memberships.
Additionally, while fixing this I realized that I had forgotten to use
the _recv() function in sdap_async_initgroups.c. It had been working
by lucky coincidence because the _recv() function it was calling
instead happened to have the exact same implementation (checking only
for errors, not requiring a specific state variable). This is now also
corrected.
Thanks for the review!
>> Fixes https://fedorahosted.org/sssd/ticket/1355
>
> I'm going to move the ticket to 1.9.0. This enhancement is not
> really intrusive and provides performance gain so big that I don't
> think we should wait.
> _______________________________________________ sssd-devel mailing
> list sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
>From 73dc74725686292e54470313d082ef783f96ccc5 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Tue, 18 Sep 2012 14:24:38 -0400
Subject: [PATCH 1/2] AD: Detect domain controller compatibility version
---
src/providers/ldap/sdap.c | 30 ++++++++++++++++++++++++++++++
src/providers/ldap/sdap.h | 13 +++++++++++++
src/providers/ldap/sdap_async.c | 1 +
3 files changed, 44 insertions(+)
diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
index 65fbc8c1e79802bd5355a083434e5c88100b8c5a..65824fc11796fae2f4ffa73e0bb46634f5d634d7 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -897,6 +897,7 @@ int sdap_get_server_opts_from_rootdse(TALLOC_CTX *memctx,
char *endptr = NULL;
int ret;
int i;
+ uint32_t dc_level;
so = talloc_zero(memctx, struct sdap_server_opts);
if (!so) {
@@ -968,6 +969,35 @@ int sdap_get_server_opts_from_rootdse(TALLOC_CTX *memctx,
}
}
}
+
+ /* Detect Active Directory version if available */
+ ret = sysdb_attrs_get_uint32_t(rootdse,
+ SDAP_ROOTDSE_ATTR_AD_VERSION,
+ &dc_level);
+ if (ret == EOK) {
+ /* Validate that the DC level matches an expected value */
+ switch(dc_level) {
+ case DS_BEHAVIOR_WIN2000:
+ case DS_BEHAVIOR_WIN2003:
+ case DS_BEHAVIOR_WIN2008:
+ case DS_BEHAVIOR_WIN2008R2:
+ case DS_BEHAVIOR_WIN2012:
+ opts->dc_functional_level = dc_level;
+ DEBUG(SSSDBG_CONF_SETTINGS,
+ ("Setting AD compatibility level to [%d]\n",
+ opts->dc_functional_level));
+ break;
+ default:
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Received invalid value for AD compatibility level. "
+ "Continuing without AD performance enhancements\n"));
+ }
+ } else if (ret != ENOENT) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Error detecting Active Directory compatibility level "
+ "(%s). Continuing without AD performance enhancements\n",
+ strerror(ret)));
+ }
}
if (!last_usn_name) {
diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
index 01c33e42117281dbebebfdd8240d468d657ae522..d844ad6369dc19c82ea761afae746f9d4bd0ce82 100644
--- a/src/providers/ldap/sdap.h
+++ b/src/providers/ldap/sdap.h
@@ -129,6 +129,7 @@ struct sdap_ppolicy_data {
#define SDAP_ROOTDSE_ATTR_NAMING_CONTEXTS "namingContexts"
#define SDAP_ROOTDSE_ATTR_DEFAULT_NAMING_CONTEXT "defaultNamingContext"
+#define SDAP_ROOTDSE_ATTR_AD_VERSION "domainControllerFunctionality"
#define SDAP_IPA_USN "entryUSN"
#define SDAP_IPA_LAST_USN "lastUSN"
@@ -364,6 +365,17 @@ struct sdap_search_base {
const char *filter;
};
+/* Values from
+ * http://msdn.microsoft.com/en-us/library/cc223272%28v=prot.13%29.aspx
+ */
+enum dc_functional_level {
+ DS_BEHAVIOR_WIN2000 = 0,
+ DS_BEHAVIOR_WIN2003 = 2,
+ DS_BEHAVIOR_WIN2008 = 3,
+ DS_BEHAVIOR_WIN2008R2 = 4,
+ DS_BEHAVIOR_WIN2012 = 5
+};
+
struct sdap_options {
struct dp_option *basic;
struct sdap_attr_map *gen_map;
@@ -397,6 +409,7 @@ struct sdap_options {
struct sdap_search_base **autofs_search_bases;
bool support_matching_rule;
+ enum dc_functional_level dc_functional_level;
};
struct sdap_server_opts {
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 42c8dd6805f4321b131ab799384bb388f2eadfb2..e0440625d279fbbaa1cc2e6343b73f5a247371f7 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -855,6 +855,7 @@ struct tevent_req *sdap_get_rootdse_send(TALLOC_CTX *memctx,
"supportedFeatures",
"supportedLDAPVersion",
"supportedSASLMechanisms",
+ SDAP_ROOTDSE_ATTR_AD_VERSION,
SDAP_ROOTDSE_ATTR_DEFAULT_NAMING_CONTEXT,
SDAP_IPA_LAST_USN, SDAP_AD_LAST_USN,
NULL
--
1.7.12
>From b2cf36d61b4cc01be32aa9204a787945a51eca7c Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Tue, 18 Sep 2012 16:51:15 -0400
Subject: [PATCH 2/2] AD: Optimize initgroups lookups with tokenGroups
https://fedorahosted.org/sssd/ticket/1355
---
src/providers/ldap/sdap_async.h | 16 ++
src/providers/ldap/sdap_async_initgroups.c | 24 ++-
src/providers/ldap/sdap_async_initgroups_ad.c | 277 ++++++++++++++++++++++++++
3 files changed, 313 insertions(+), 4 deletions(-)
diff --git a/src/providers/ldap/sdap_async.h b/src/providers/ldap/sdap_async.h
index d024e30a92893a84d9e8280c1469bb40ecaff045..8c16d94e6486336e92b6112cd8b5a2dff4c97957 100644
--- a/src/providers/ldap/sdap_async.h
+++ b/src/providers/ldap/sdap_async.h
@@ -31,6 +31,8 @@
#include "providers/ldap/sdap_id_op.h"
#include "providers/fail_over.h"
+#define AD_TOKENGROUPS_ATTR "tokenGroups"
+
struct tevent_req *sdap_connect_send(TALLOC_CTX *memctx,
struct tevent_context *ev,
struct sdap_options *opts,
@@ -275,4 +277,18 @@ sdap_get_ad_match_rule_initgroups_send(TALLOC_CTX *mem_ctx,
errno_t
sdap_get_ad_match_rule_initgroups_recv(struct tevent_req *req);
+
+struct tevent_req *
+sdap_get_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct sdap_options *opts,
+ struct sysdb_ctx *sysdb,
+ struct sdap_handle *sh,
+ const char *name,
+ const char *orig_dn,
+ int timeout);
+
+errno_t
+sdap_get_ad_tokengroups_initgroups_recv(struct tevent_req *req);
+
#endif /* _SDAP_ASYNC_H_ */
diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c
index d55f661ff4d83de82fc9c920f17adc68b9594fea..71b4536b359c4cc0183e2733ae8d8074ca30c0e3 100644
--- a/src/providers/ldap/sdap_async_initgroups.c
+++ b/src/providers/ldap/sdap_async_initgroups.c
@@ -2642,6 +2642,8 @@ static void sdap_get_initgr_user(struct tevent_req *subreq)
const char *orig_dn;
const char *cname;
bool in_transaction = false;
+ bool use_id_mapping =
+ dp_opt_get_bool(state->opts->basic, SDAP_ID_MAPPING);
DEBUG(9, ("Receiving info for the user\n"));
@@ -2731,9 +2733,17 @@ static void sdap_get_initgr_user(struct tevent_req *subreq)
return;
}
- if (state->opts->support_matching_rule
- && dp_opt_get_bool(state->opts->basic,
- SDAP_AD_MATCHING_RULE_INITGROUPS)) {
+ if (use_id_mapping
+ && state->opts->dc_functional_level >= DS_BEHAVIOR_WIN2008) {
+ /* Take advantage of AD's tokenGroups mechanism to look up all
+ * parent groups in a single request.
+ */
+ subreq = sdap_get_ad_tokengroups_initgroups_send(
+ state, state->ev, state->opts, state->sysdb,
+ state->sh, cname, orig_dn, state->timeout);
+ } else if (state->opts->support_matching_rule
+ && dp_opt_get_bool(state->opts->basic,
+ SDAP_AD_MATCHING_RULE_INITGROUPS)) {
/* Take advantage of AD's extensibleMatch filter to look up
* all parent groups in a single request.
*/
@@ -2815,7 +2825,13 @@ static void sdap_get_initgr_done(struct tevent_req *subreq)
case SDAP_SCHEMA_RFC2307BIS:
case SDAP_SCHEMA_AD:
- if (dp_opt_get_bool(state->opts->basic, SDAP_AD_MATCHING_RULE_INITGROUPS)) {
+ if (use_id_mapping
+ && state->opts->dc_functional_level >= DS_BEHAVIOR_WIN2008) {
+ ret = sdap_get_ad_tokengroups_initgroups_recv(subreq);
+ }
+ else if (state->opts->support_matching_rule
+ && dp_opt_get_bool(state->opts->basic,
+ SDAP_AD_MATCHING_RULE_INITGROUPS)) {
ret = sdap_get_ad_match_rule_initgroups_recv(subreq);
} else {
ret = sdap_initgr_rfc2307bis_recv(subreq);
diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
index f74729f47c5bf31f326aba1d91765ca4d4846f40..1333e3080949dc138330b12c8b991a9ea3561b2b 100644
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -24,6 +24,8 @@
#include "providers/ldap/sdap_async.h"
#include "providers/ldap/ldap_common.h"
#include "providers/ldap/sdap_async_private.h"
+#include "providers/ldap/sdap_idmap.h"
+#include "lib/idmap/sss_idmap.h"
struct sdap_ad_match_rule_initgr_state {
struct tevent_context *ev;
@@ -290,3 +292,278 @@ sdap_get_ad_match_rule_initgroups_recv(struct tevent_req *req)
TEVENT_REQ_RETURN_ON_ERROR(req);
return EOK;
}
+
+struct sdap_ad_tokengroups_initgr_state {
+ struct tevent_context *ev;
+ struct sdap_options *opts;
+ struct sysdb_ctx *sysdb;
+ struct sdap_handle *sh;
+ const char *username;
+};
+
+static void
+sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req *req);
+
+struct tevent_req *
+sdap_get_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
+ struct tevent_context *ev,
+ struct sdap_options *opts,
+ struct sysdb_ctx *sysdb,
+ struct sdap_handle *sh,
+ const char *name,
+ const char *orig_dn,
+ int timeout)
+{
+ struct tevent_req *req;
+ struct tevent_req *subreq;
+ struct sdap_ad_tokengroups_initgr_state *state;
+ const char *attrs[] = {AD_TOKENGROUPS_ATTR, NULL};
+
+ req = tevent_req_create(mem_ctx, &state,
+ struct sdap_ad_tokengroups_initgr_state);
+ if (!req) return NULL;
+
+ state->ev = ev;
+ state->opts = opts;
+ state->sysdb = sysdb;
+ state->sh = sh;
+ state->username = name;
+
+ subreq = sdap_get_generic_send(
+ state, state->ev, state->opts, state->sh,
+ orig_dn, LDAP_SCOPE_BASE, NULL, attrs,
+ NULL, 0, timeout, false);
+ if (!subreq) {
+ tevent_req_error(req, ENOMEM);
+ tevent_req_post(req, ev);
+ return req;
+ }
+
+ tevent_req_set_callback(subreq,
+ sdap_get_ad_tokengroups_initgroups_lookup_done,
+ req);
+ return req;
+}
+
+static void
+sdap_get_ad_tokengroups_initgroups_lookup_done(struct tevent_req *subreq)
+{
+ errno_t ret, sret;
+ enum idmap_error_code err;
+ size_t user_count, group_count, i;
+ TALLOC_CTX *tmp_ctx;
+ bool in_transaction = false;
+ char *sid_str;
+ gid_t gid;
+ time_t now;
+ struct sysdb_attrs **users;
+ struct ldb_message_element *el;
+ struct ldb_message *msg;
+ char **ldap_grouplist;
+ char **sysdb_grouplist;
+ char **add_groups;
+ char **del_groups;
+ const char *attrs[] = { SYSDB_NAME, NULL };
+ const char *group_name;
+ struct tevent_req *req =
+ tevent_req_callback_data(subreq, struct tevent_req);
+ struct sdap_ad_tokengroups_initgr_state *state =
+ tevent_req_data(req, struct sdap_ad_tokengroups_initgr_state);
+
+ tmp_ctx = talloc_new(NULL);
+ if (!tmp_ctx) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ ret = sdap_get_generic_recv(subreq, tmp_ctx, &user_count, &users);
+ talloc_zfree(subreq);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("LDAP search failed: [%s]\n", strerror(ret)));
+ goto done;
+ }
+
+ if (user_count != 1) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("More than one result on a base search!\n"));
+ ret = EINVAL;
+ goto done;
+ }
+
+ /* Get the list of group SIDs */
+ ret = sysdb_attrs_get_el_ext(users[0], AD_TOKENGROUPS_ATTR,
+ false, &el);
+ if (ret != EOK) {
+ if (ret == ENOENT) {
+ DEBUG(SSSDBG_TRACE_LIBS,
+ ("No tokenGroups entries for [%s]\n",
+ state->username));
+ /* No groups in LDAP. We need to ensure that the
+ * sysdb matches.
+ */
+ el = talloc_zero(tmp_ctx, struct ldb_message_element);
+ if (!el) {
+ ret = ENOMEM;
+ goto done;
+ }
+ el->num_values = 0;
+
+ /* This will skip the group-processing loop below
+ * and proceed to removing any sysdb groups.
+ */
+ } else {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not read tokenGroups attribute: [%s]\n",
+ strerror(ret)));
+ goto done;
+ }
+ }
+
+ /* Process the groups */
+ now = time(NULL);
+
+ ret = sysdb_transaction_start(state->sysdb);
+ if (ret != EOK) goto done;
+ in_transaction = true;
+
+ ldap_grouplist = talloc_array(tmp_ctx, char *, el->num_values + 1);
+ if (!ldap_grouplist) {
+ ret = ENOMEM;
+ goto done;
+ }
+ group_count = 0;
+
+ for (i = 0; i < el->num_values; i++) {
+ /* Get the SID and convert it to a GID */
+
+ err = sss_idmap_bin_sid_to_sid(state->opts->idmap_ctx->map,
+ el->values[i].data,
+ el->values[i].length,
+ &sid_str);
+ if (err != IDMAP_SUCCESS) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not convert binary SID to string: [%s]. Skipping\n",
+ idmap_error_string(err)));
+ continue;
+ }
+ DEBUG(SSSDBG_TRACE_LIBS,
+ ("Processing membership SID [%s]\n",
+ sid_str));
+ ret = sdap_idmap_sid_to_unix(state->opts->idmap_ctx, sid_str,
+ &gid);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not convert SID to GID: [%s]. Skipping\n",
+ strerror(ret)));
+ continue;
+ }
+
+ DEBUG(SSSDBG_TRACE_LIBS,
+ ("Processing membership GID [%lu]\n",
+ gid));
+
+ /* Check whether this GID already exists in the sysdb */
+ ret = sysdb_search_group_by_gid(tmp_ctx, state->sysdb,
+ gid, attrs, &msg);
+ if (ret == EOK) {
+ group_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
+ if (!group_name) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not retrieve group name from sysdb\n"));
+ ret = EINVAL;
+ goto done;
+ }
+ } else if (ret == ENOENT) {
+ /* This is a new group. For now, we will store it
+ * under the name of its SID. When a direct lookup of
+ * the group or its GID occurs, it will replace this
+ * temporary entry.
+ */
+ group_name = sid_str;
+ ret = sysdb_add_incomplete_group(state->sysdb, group_name,
+ gid, NULL, false, now);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not create incomplete group: [%s]\n",
+ strerror(ret)));
+ goto done;
+ }
+ }
+
+ ldap_grouplist[group_count] =
+ talloc_strdup(ldap_grouplist, group_name);
+ if (!ldap_grouplist[group_count]) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ group_count++;
+ }
+ ldap_grouplist[group_count] = NULL;
+
+ /* Get the current sysdb group list for this user
+ * so we can update it.
+ */
+ ret = get_sysdb_grouplist(state, state->sysdb, state->username,
+ &sysdb_grouplist);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Could not get the list of groups for [%s] in the sysdb: "
+ "[%s]\n",
+ state->username, strerror(ret)));
+ goto done;
+ }
+
+ /* Find the differences between the sysdb and LDAP lists
+ * Groups in the sysdb only must be removed.
+ */
+ ret = diff_string_lists(tmp_ctx, ldap_grouplist, sysdb_grouplist,
+ &add_groups, &del_groups, NULL);
+ if (ret != EOK) goto done;
+
+ DEBUG(SSSDBG_TRACE_LIBS,
+ ("Updating memberships for [%s]\n", state->username));
+ ret = sysdb_update_members(state->sysdb, state->username,
+ SYSDB_MEMBER_USER,
+ (const char *const *) add_groups,
+ (const char *const *) del_groups);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("Membership update failed [%d]: %s\n",
+ ret, strerror(ret)));
+ goto done;
+ }
+
+ ret = sysdb_transaction_commit(state->sysdb);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Could not commit transaction! [%s]\n",
+ strerror(ret)));
+ goto done;
+ }
+ in_transaction = false;
+
+done:
+ if (in_transaction) {
+ sret = sysdb_transaction_cancel(state->sysdb);
+ DEBUG(SSSDBG_FATAL_FAILURE,
+ ("Could not cancel transaction! [%s]\n",
+ sret));
+ }
+
+ if (ret == EOK) {
+ tevent_req_done(req);
+ } else {
+ tevent_req_error(req, ret);
+ }
+ talloc_free(tmp_ctx);
+ return;
+}
+
+errno_t
+sdap_get_ad_tokengroups_initgroups_recv(struct tevent_req *req)
+{
+ TEVENT_REQ_RETURN_ON_ERROR(req);
+ return EOK;
+}
--
1.7.12
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
