r22390: Patchset sent to samba-technical to address the winbind

loop when allocating a new id for a SID:

auth_util.patch		Revert create_local_token() to
			the 3.0.24 codebase

idmap_type.patch	Have the caller fillin the
			id_map.xid.type field when
			resolving a SID so that if we allocate
			a new id, we know what type to use

winbindd_api.patch	Remove the WINBINDD_SIDS_TO_XIDS calls
			from the public winbindd interface
			for the 3.0.25 release

idmap_rid.patch		Cleanup the idmap_rid backend to not
			call back into winbindd to resolve
			the SID in order to verify it's type.

8 files changed, 81 insertions, 203 deletions

diff --git a/source/auth/auth_util.c b/source/auth/auth_util.c
index 0de3bf23252..336daa906d0 100644
--- a/source/auth/auth_util.c
+++ b/source/auth/auth_util.c
@@ -637,9 +637,7 @@ static NTSTATUS log_nt_token(TALLOC_CTX *tmp_ctx, NT_USER_TOKEN *token)
 NTSTATUS create_local_token(auth_serversupplied_info *server_info)
 {
 	TALLOC_CTX *mem_ctx;
-	struct id_map *ids;
 	NTSTATUS status;
-	BOOL wb = True;
 	size_t i;
 	
 
@@ -686,46 +684,20 @@ NTSTATUS create_local_token(auth_serversupplied_info *server_info)
 	server_info->groups = NULL;
 
 	/* Start at index 1, where the groups start. */
-	ids = talloc_zero_array(mem_ctx, struct id_map, server_info->ptok->num_sids);
-	for (i = 0; i < server_info->ptok->num_sids-1; i++) {
-		ids[i].sid = &server_info->ptok->user_sids[i + 1]; /* store the sids */
-	}
-
-	if (!winbind_sids_to_unixids(ids, server_info->ptok->num_sids-1)) {
-		DEBUG(2, ("Query to map secondary SIDs failed!\n"));
-		if (!winbind_ping()) {
-			DEBUG(2, ("Winbindd is not running, will try to map SIDs one by one with legacy code\n"));
-			wb = False;
-		}
-	}
 
-	for (i = 0; i < server_info->ptok->num_sids-1; i++) {
-		gid_t agid;
+	for (i=1; i<server_info->ptok->num_sids; i++) {
+		gid_t gid;
+		DOM_SID *sid = &server_info->ptok->user_sids[i];
 
-		if (wb) {
-			if (ids[i].status != ID_MAPPED) {
-				DEBUG(10, ("Could not convert SID %s to gid, "
-					   "ignoring it\n", sid_string_static(ids[i].sid)));
-				continue;
-			}
-			if (ids[i].xid.type == ID_TYPE_UID) {
-				DEBUG(10, ("SID %s is a User ID (%u) not a Group ID, "
-					   "ignoring it\n", sid_string_static(ids[i].sid), ids[i].xid.id));
-				continue;
-			}
-			agid = (gid_t)ids[i].xid.id;
-		} else {
-			if (! sid_to_gid(ids[i].sid, &agid)) {
-				continue;
-			}
-		}
-		if (!add_gid_to_array_unique(server_info, agid, &server_info->groups,
-					&server_info->n_groups)) {
-			TALLOC_FREE(mem_ctx);
-			return NT_STATUS_NO_MEMORY;
+		if (!sid_to_gid(sid, &gid)) {
+			DEBUG(10, ("Could not convert SID %s to gid, "
+				   "ignoring it\n", sid_string_static(sid)));
+			continue;
 		}
+		add_gid_to_array_unique(server_info, gid, &server_info->groups,
+					&server_info->n_groups);
 	}
-
+	
 	debug_nt_user_token(DBGC_AUTH, 10, server_info->ptok);
 
 	status = log_nt_token(mem_ctx, server_info->ptok);
diff --git a/source/include/idmap.h b/source/include/idmap.h
index 472358a2305..f4926f1e5c4 100644
--- a/source/include/idmap.h
+++ b/source/include/idmap.h
@@ -52,8 +52,14 @@ struct idmap_methods {
 	/* Called when backend is first loaded */
 	NTSTATUS (*init)(struct idmap_domain *dom);
 
+	/* Map an array of uids/gids to SIDs.  The caller specifies
+	   the uid/gid and type. Gets back the SID. */
 	NTSTATUS (*unixids_to_sids)(struct idmap_domain *dom, struct id_map **ids);
+
+	/* Map an arry of SIDs to uids/gids.  The caller sets the SID
+	   and type and gets back a uid or gid. */
 	NTSTATUS (*sids_to_unixids)(struct idmap_domain *dom, struct id_map **ids);
+
 	NTSTATUS (*set_mapping)(struct idmap_domain *dom, const struct id_map *map);
 	NTSTATUS (*remove_mapping)(struct idmap_domain *dom, const struct id_map *map);
 
diff --git a/source/include/smb.h b/source/include/smb.h
index f85201ed303..6c44db5a348 100644
--- a/source/include/smb.h
+++ b/source/include/smb.h
@@ -276,13 +276,14 @@ typedef struct dom_sid {
 #define dom_sid28 dom_sid
 
 enum id_mapping {
-	ID_UNKNOWN,
+	ID_UNKNOWN = 0,
 	ID_MAPPED,
 	ID_UNMAPPED,
 	ID_EXPIRED
 };
 
 enum id_type {
+	ID_TYPE_NOT_SPECIFIED = 0,
 	ID_TYPE_UID,
 	ID_TYPE_GID
 };
diff --git a/source/nsswitch/idmap.c b/source/nsswitch/idmap.c
index 82b8f3d592e..530e03089d0 100644
--- a/source/nsswitch/idmap.c
+++ b/source/nsswitch/idmap.c
@@ -91,24 +91,6 @@ static struct idmap_alloc_methods *get_alloc_methods(struct idmap_alloc_backend
 	return NULL;
 }
 
-/* part of a quick hack to avoid loops, need to be sorted out correctly later on */
-static BOOL idmap_in_own_child;
-
-static BOOL idmap_is_in_own_child(void)
-{
-	return idmap_in_own_child;
-}
-
-void reset_idmap_in_own_child(void)
-{
-	idmap_in_own_child = False;
-}
-
-void set_idmap_in_own_child(void)
-{
-	idmap_in_own_child = True;
-}
-
 BOOL idmap_is_offline(void)
 {
 	return ( lp_winbind_offline_logon() &&
@@ -855,9 +837,6 @@ static NTSTATUS idmap_new_mapping(TALLOC_CTX *ctx, struct id_map *map)
 {
 	NTSTATUS ret;
 	struct idmap_domain *dom;
-	char *domname, *name;
-	enum lsa_SidType sid_type;
-	BOOL wbret;
 
 	/* If we are offline we cannot lookup SIDs, deny mapping */
 	if (idmap_is_offline())	{
@@ -869,70 +848,46 @@ static NTSTATUS idmap_new_mapping(TALLOC_CTX *ctx, struct id_map *map)
 		return NT_STATUS_NONE_MAPPED;
 	}
 
-	/* quick hack to make things work, will need proper fix later on */	
-	if (idmap_is_in_own_child()) {
-		/* by default calls to winbindd are disabled
-		   the following call will not recurse so this is safe */
-		winbind_on();
-		wbret = winbind_lookup_sid(ctx, map->sid,
-						(const char **)&domname,
-						(const char **)&name,
-						&sid_type);
-		winbind_off();
-	} else {
-		wbret = winbindd_lookup_name_by_sid(ctx, map->sid,
-							&domname,
-							&name,
-							&sid_type);
-	}
-
 	/* check if this is a valid SID and then map it */
-	if (wbret) {
-		switch (sid_type) {
-		case SID_NAME_USER:
-			ret = idmap_allocate_uid(&map->xid);
-			if ( ! NT_STATUS_IS_OK(ret)) {
-				/* can't allocate id, let's just leave it unmapped */
-				DEBUG(2, ("uid allocation failed! Can't create mapping\n"));
-				return NT_STATUS_NONE_MAPPED;
-			}
-			break;
-		case SID_NAME_DOM_GRP:
-		case SID_NAME_ALIAS:
-		case SID_NAME_WKN_GRP:
-			ret = idmap_allocate_gid(&map->xid);
-			if ( ! NT_STATUS_IS_OK(ret)) {
-				/* can't allocate id, let's just leave it unmapped */
-				DEBUG(2, ("gid allocation failed! Can't create mapping\n"));
-				return NT_STATUS_NONE_MAPPED;
-			}
-			break;
-		default:
-			/* invalid sid, let's just leave it unmapped */
-			DEBUG(10, ("SID %s is UNKNOWN, skip mapping\n", sid_string_static(map->sid)));
+	switch (map->xid.type) {
+	case ID_TYPE_UID:
+		ret = idmap_allocate_uid(&map->xid);
+		if ( ! NT_STATUS_IS_OK(ret)) {
+			/* can't allocate id, let's just leave it unmapped */
+			DEBUG(2, ("uid allocation failed! Can't create mapping\n"));
 			return NT_STATUS_NONE_MAPPED;
 		}
+		break;
+	case ID_TYPE_GID:
+		ret = idmap_allocate_gid(&map->xid);
+		if ( ! NT_STATUS_IS_OK(ret)) {
+			/* can't allocate id, let's just leave it unmapped */
+			DEBUG(2, ("gid allocation failed! Can't create mapping\n"));
+			return NT_STATUS_NONE_MAPPED;
+		}
+		break;
+	default:
+		/* invalid sid, let's just leave it unmapped */
+		DEBUG(3,("idmap_new_mapping: Refusing to create a "
+			 "mapping for an unspecified ID type.\n"));		
+		return NT_STATUS_NONE_MAPPED;
+	}
 
-		/* ok, got a new id, let's set a mapping */
-		map->status = ID_MAPPED;
+	/* ok, got a new id, let's set a mapping */
+	map->status = ID_MAPPED;
 
-		DEBUG(10, ("Setting mapping: %s <-> %s %lu\n",
-			   sid_string_static(map->sid),
-			   (map->xid.type == ID_TYPE_UID) ? "UID" : "GID",
-			   (unsigned long)map->xid.id));
-		ret = dom->methods->set_mapping(dom, map);
+	DEBUG(10, ("Setting mapping: %s <-> %s %lu\n",
+		   sid_string_static(map->sid),
+		   (map->xid.type == ID_TYPE_UID) ? "UID" : "GID",
+		   (unsigned long)map->xid.id));
+	ret = dom->methods->set_mapping(dom, map);
 
-		if ( ! NT_STATUS_IS_OK(ret)) {
-			/* something wrong here :-( */
-			DEBUG(2, ("Failed to commit mapping\n!"));
+	if ( ! NT_STATUS_IS_OK(ret)) {
+		/* something wrong here :-( */
+		DEBUG(2, ("Failed to commit mapping\n!"));
 
-			/* TODO: would it make sense to have an "unalloc_id function?" */
+		/* TODO: would it make sense to have an "unalloc_id function?" */
 
-			return NT_STATUS_NONE_MAPPED;
-		}
-	} else {
-		DEBUG(2,("Invalid SID, not mapping %s (type %d)\n",
-				sid_string_static(map->sid), sid_type));
 		return NT_STATUS_NONE_MAPPED;
 	}
 
@@ -1439,6 +1394,8 @@ void idmap_dump_maps(char *logfile)
 				(unsigned long)maps[i].xid.id,
 				sid_string_static(maps[i].sid));
 			break;
+		case ID_TYPE_NOT_SPECIFIED:
+			break;
 		}
 	}
 
diff --git a/source/nsswitch/idmap_rid.c b/source/nsswitch/idmap_rid.c
index 298d6fed355..8e016879b8c 100644
--- a/source/nsswitch/idmap_rid.c
+++ b/source/nsswitch/idmap_rid.c
@@ -37,7 +37,7 @@ struct idmap_rid_context {
   we support multiple domains in the new idmap
  *****************************************************************************/
 
-static NTSTATUS idmap_rid_initialize(struct idmap_domain *dom, const char *compat_params)
+static NTSTATUS idmap_rid_initialize(struct idmap_domain *dom)
 {
 	NTSTATUS ret;
 	struct idmap_rid_context *ctx;
@@ -86,9 +86,6 @@ failed:
 
 static NTSTATUS idmap_rid_id_to_sid(TALLOC_CTX *memctx, struct idmap_rid_context *ctx, struct id_map *map)
 {
-	const char *domname, *name;
-	enum lsa_SidType sid_type;
-	BOOL ret;
 	struct winbindd_domain *domain;	
 
 	/* apply filters before checking */
@@ -104,45 +101,9 @@ static NTSTATUS idmap_rid_id_to_sid(TALLOC_CTX *memctx, struct idmap_rid_context
 	
 	sid_compose(map->sid, &domain->sid, map->xid.id - ctx->low_id + ctx->base_rid);
 
-	/* by default calls to winbindd are disabled
-	   the following call will not recurse so this is safe */
-	winbind_on();
-	ret = winbind_lookup_sid(memctx, map->sid, &domname, &name, &sid_type);
-	winbind_off();
-
-	if (ret) {
-		switch (sid_type) {
-		case SID_NAME_USER:
-			if (map->xid.type != ID_TYPE_UID) {
-				/* wrong type */
-				map->status = ID_UNMAPPED;
-				DEBUG(5, ("Resulting SID is of wrong ID type\n"));
-				return NT_STATUS_NONE_MAPPED;
-			}
-			break;
-		case SID_NAME_DOM_GRP:
-		case SID_NAME_ALIAS:
-		case SID_NAME_WKN_GRP:
-			if (map->xid.type != ID_TYPE_GID) {
-				/* wrong type */
-				map->status = ID_UNMAPPED;
-				DEBUG(5, ("Resulting SID is of wrong ID type\n"));
-				return NT_STATUS_NONE_MAPPED;
-			}
-			break;
-		default:
-			/* invalid sid?? */
-			map->status = ID_UNKNOWN;
-			DEBUG(10, ("SID %s is UNKNOWN, skip mapping\n", sid_string_static(map->sid)));
-			return NT_STATUS_NONE_MAPPED;
-		}
-	} else {
-		/* TODO: how do we known if the lookup was negative
-		 * or something just failed? */
-		map->status = ID_UNMAPPED;
-		DEBUG(2, ("Failed: to resolve SID\n"));
-		return NT_STATUS_UNSUCCESSFUL;
-	}
+	/* We **really** should have some way of validating 
+	   the SID exists and is the correct type here.  But 
+	   that is a deficiency in the idmap_rid design. */
 
 	map->status = ID_MAPPED;
 
@@ -155,46 +116,13 @@ static NTSTATUS idmap_rid_id_to_sid(TALLOC_CTX *memctx, struct idmap_rid_context
 
 static NTSTATUS idmap_rid_sid_to_id(TALLOC_CTX *memctx, struct idmap_rid_context *ctx, struct id_map *map)
 {
-	const char *domname, *name;
-	enum lsa_SidType sid_type;
 	uint32_t rid;
-	BOOL ret;
 
 	sid_peek_rid(map->sid, &rid);
 	map->xid.id = rid - ctx->base_rid + ctx->low_id;
 
-	/* by default calls to winbindd are disabled
-	   the following call will not recurse so this is safe */
-	winbind_on();
-	/* check if this is a valid SID and set the type */
-	ret = winbind_lookup_sid(memctx, map->sid, &domname, &name, &sid_type);
-	winbind_off();
-
-	if (ret) {
-		switch (sid_type) {
-		case SID_NAME_USER:
-			map->xid.type = ID_TYPE_UID;
-			break;
-		case SID_NAME_DOM_GRP:
-		case SID_NAME_ALIAS:
-		case SID_NAME_WKN_GRP:
-			map->xid.type = ID_TYPE_GID;
-			break;
-		default:
-			/* invalid sid, let's just leave it unmapped */
-			DEBUG(10, ("SID %s is UNKNOWN, skip mapping\n", sid_string_static(map->sid)));
-			map->status = ID_UNKNOWN;
-			return NT_STATUS_NONE_MAPPED;
-		}
-	} else {
-		/* TODO: how do we known if the lookup was negative
-		 * or something just failed? */
-		map->status = ID_UNMAPPED;
-		DEBUG(2, ("Failed: to resolve SID\n"));
-		return NT_STATUS_UNSUCCESSFUL;
-	}
-
 	/* apply filters before returning result */
+
 	if ((map->xid.id < ctx->low_id) || (map->xid.id > ctx->high_id)) {
 		DEBUG(5, ("Requested id (%u) out of range (%u - %u). Filtered!\n",
 				map->xid.id, ctx->low_id, ctx->high_id));
@@ -202,6 +130,10 @@ static NTSTATUS idmap_rid_sid_to_id(TALLOC_CTX *memctx, struct idmap_rid_context
 		return NT_STATUS_NONE_MAPPED;
 	}
 
+	/* We **really** should have some way of validating 
+	   the SID exists and is the correct type here.  But 
+	   that is a deficiency in the idmap_rid design. */
+
 	map->status = ID_MAPPED;
 
 	return NT_STATUS_OK;
diff --git a/source/nsswitch/idmap_util.c b/source/nsswitch/idmap_util.c
index 540dafaa734..40a5fb854be 100644
--- a/source/nsswitch/idmap_util.c
+++ b/source/nsswitch/idmap_util.c
@@ -105,18 +105,24 @@ NTSTATUS idmap_sid_to_uid(DOM_SID *sid, uid_t *uid)
 	DEBUG(10,("idmap_sid_to_uid: sid = [%s]\n", sid_string_static(sid)));
 
 	map.sid = sid;
+	map.xid.type = ID_TYPE_UID;	
 	
 	maps[0] = &map;
 	maps[1] = NULL;
 	
 	ret = idmap_sids_to_unixids(maps);
 	if ( ! NT_STATUS_IS_OK(ret)) {
-		DEBUG(10, ("error mapping sid [%s] to uid\n", sid_string_static(sid)));
+		DEBUG(10, ("error mapping sid [%s] to uid\n", 
+			   sid_string_static(sid)));
 		return ret;
 	}
 
 	if ((map.status != ID_MAPPED) || (map.xid.type != ID_TYPE_UID)) {
-		DEBUG(10, ("sid [%s] not mapped to an uid [%u,%u,%u]\n", sid_string_static(sid), map.status, map.xid.type, map.xid.id));
+		DEBUG(10, ("sid [%s] not mapped to an uid [%u,%u,%u]\n", 
+			   sid_string_static(sid), 
+			   map.status, 
+			   map.xid.type, 
+			   map.xid.id));
 		return NT_STATUS_NONE_MAPPED;
 	}
 
@@ -139,18 +145,24 @@ NTSTATUS idmap_sid_to_gid(DOM_SID *sid, gid_t *gid)
 	DEBUG(10,("idmap_sid_to_gid: sid = [%s]\n", sid_string_static(sid)));
 
 	map.sid = sid;
+	map.xid.type = ID_TYPE_GID;
 	
 	maps[0] = &map;
 	maps[1] = NULL;
 	
 	ret = idmap_sids_to_unixids(maps);
 	if ( ! NT_STATUS_IS_OK(ret)) {
-		DEBUG(10, ("error mapping sid [%s] to gid\n", sid_string_static(sid)));
+		DEBUG(10, ("error mapping sid [%s] to gid\n", 
+			   sid_string_static(sid)));
 		return ret;
 	}
 
 	if ((map.status != ID_MAPPED) || (map.xid.type != ID_TYPE_GID)) {
-		DEBUG(10, ("sid [%s] not mapped to an gid [%u,%u,%u]\n", sid_string_static(sid), map.status, map.xid.type, map.xid.id));
+		DEBUG(10, ("sid [%s] not mapped to an gid [%u,%u,%u]\n", 
+			   sid_string_static(sid), 
+			   map.status, 
+			   map.xid.type, 
+			   map.xid.id));
 		return NT_STATUS_NONE_MAPPED;
 	}
 
diff --git a/source/nsswitch/winbindd.c b/source/nsswitch/winbindd.c
index b17172f4f27..fcedaebb273 100644
--- a/source/nsswitch/winbindd.c
+++ b/source/nsswitch/winbindd.c
@@ -248,7 +248,9 @@ static struct winbindd_dispatch_table {
 	{ WINBINDD_SID_TO_GID, winbindd_sid_to_gid, "SID_TO_GID" },
 	{ WINBINDD_UID_TO_SID, winbindd_uid_to_sid, "UID_TO_SID" },
 	{ WINBINDD_GID_TO_SID, winbindd_gid_to_sid, "GID_TO_SID" },
+#if 0   /* DISABLED until we fix the interface in Samba 3.0.26 --jerry */
 	{ WINBINDD_SIDS_TO_XIDS, winbindd_sids_to_unixids, "SIDS_TO_XIDS" },
+#endif  /* end DISABLED */
 	{ WINBINDD_ALLOCATE_UID, winbindd_allocate_uid, "ALLOCATE_UID" },
 	{ WINBINDD_ALLOCATE_GID, winbindd_allocate_gid, "ALLOCATE_GID" },
 	{ WINBINDD_SET_MAPPING, winbindd_set_mapping, "SET_MAPPING" },
@@ -1010,9 +1012,6 @@ int main(int argc, char **argv, char **envp)
 
 	namecache_enable();
 
-	/* quick hack to avoid a loop in idmap, proper fix later */
-	reset_idmap_in_own_child();
-
 	/* Winbind daemon initialisation */
 
 	if ( ! NT_STATUS_IS_OK(idmap_init_cache()) ) {
diff --git a/source/nsswitch/winbindd_dual.c b/source/nsswitch/winbindd_dual.c
index 20cf55b51d0..8d475e6c9f9 100644
--- a/source/nsswitch/winbindd_dual.c
+++ b/source/nsswitch/winbindd_dual.c
@@ -364,7 +364,9 @@ static struct winbindd_child_dispatch_table child_dispatch_table[] = {
 	{ WINBINDD_CHECK_MACHACC,        winbindd_dual_check_machine_acct,    "CHECK_MACHACC" },
 	{ WINBINDD_DUAL_SID2UID,         winbindd_dual_sid2uid,               "DUAL_SID2UID" },
 	{ WINBINDD_DUAL_SID2GID,         winbindd_dual_sid2gid,               "DUAL_SID2GID" },
+#if 0   /* DISABLED until we fix the interface in Samba 3.0.26 --jerry */
 	{ WINBINDD_DUAL_SIDS2XIDS,       winbindd_dual_sids2xids,             "DUAL_SIDS2XIDS" },
+#endif  /* end DISABLED */
 	{ WINBINDD_DUAL_UID2SID,         winbindd_dual_uid2sid,               "DUAL_UID2SID" },
 	{ WINBINDD_DUAL_GID2SID,         winbindd_dual_gid2sid,               "DUAL_GID2SID" },
 	{ WINBINDD_DUAL_UID2NAME,        winbindd_dual_uid2name,              "DUAL_UID2NAME" },
@@ -921,9 +923,6 @@ static BOOL fork_domain_child(struct winbindd_child *child)
 			child);
 	}
 
-	/* quick hack to avoid a loop in idmap, proper fix later */
-	set_idmap_in_own_child();
-
 	while (1) {
 
 		int ret;