On 2013-08-20 03:23, John Thiltges wrote:
These are two AllowGroups snags we've run into:
Unlike nscd, sssd doesn't allow enumeration by default (and is
case-sensitive). We add this to /etc/sssd/sssd.conf on the slurmctld
node:
enumerate = True
case_sensitive = False
Looking at src/slurmctld/groups.c:get_group_members it seems to be a
case of using both belts and suspenders in order to work around broken
configurations (multiple groups with the same GID, or user primary
groups not being listed in the group data entry), resulting in something
that turns out to not work in case enumeration is disabled. As many
systems such as sssd and winbind disable enumeration by default (for
performance, and maybe security-by-obscurity), IMHO it would be better
to avoid relying on such a feature. getgrnam_r() already returns all the
members of the group, there's no need to iterate over both the entire
user and group databases to see if entries match.
The attached patch simplifies the code to just use the group member list
returned by the getgrnam_r() call, without enumerating all users or groups.
If you have large groups, you might run into the buffer size limit,
which is 65k characters. (PW_BUF_SIZE in uid.h)
The patch also fixes this, by calling getgrnam_r() in a loop, increasing
the buffer size if it was too small.
--
Janne Blomqvist, D.Sc. (Tech.), Scientific Computing Specialist
Aalto University School of Science, PHYS & BECS
+358503841576 || janne.blomqv...@aalto.fi
diff --git a/src/slurmctld/groups.c b/src/slurmctld/groups.c
index 7728484..fd7d758 100644
--- a/src/slurmctld/groups.c
+++ b/src/slurmctld/groups.c
@@ -83,21 +83,13 @@ struct group_cache_rec {
* NOTE: User root has implicitly access to every group
* NOTE: The caller must xfree non-NULL return values
*/
-extern uid_t *get_group_members(char *group_name)
+extern uid_t *get_group_members(const char *group_name)
{
- char grp_buffer[PW_BUF_SIZE];
+ char *grp_buffer;
+ long buflen;
struct group grp, *grp_result = NULL;
- struct passwd *pwd_result = NULL;
- uid_t *group_uids = NULL, my_uid;
- gid_t my_gid;
- int i, j, uid_cnt;
-#ifdef HAVE_AIX
- FILE *fp = NULL;
-#elif defined (__APPLE__) || defined (__CYGWIN__)
-#else
- char pw_buffer[PW_BUF_SIZE];
- struct passwd pw;
-#endif
+ uid_t *group_uids = NULL;
+ size_t i, j, uid_cnt;
group_uids = _get_group_cache(group_name);
if (group_uids) { /* We found in cache */
@@ -105,84 +97,46 @@ extern uid_t *get_group_members(char *group_name)
return group_uids;
}
- /* We need to check for !grp_result, since it appears some
- * versions of this function do not return an error on failure.
- */
- if (getgrnam_r(group_name, &grp, grp_buffer, PW_BUF_SIZE,
- &grp_result) || (grp_result == NULL)) {
+ buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
+ if (buflen <= 0)
+ buflen = 1024;
+ grp_buffer = xmalloc(buflen);
+
+ /* Call getgrnam_r in a loop, increasing the buffer size if it
+ * turned out it was too small. */
+ while (1) {
+ int res = getgrnam_r(group_name, &grp, grp_buffer, buflen,
+ &grp_result);
+ if (res) {
+ switch(errno) {
+ case ERANGE:
+ buflen *= 2;
+ xrealloc(grp_buffer, buflen);
+ break;
+ default:
+ error("getgrnam_r failed");
+ xfree(grp_buffer);
+ return NULL;
+ }
+ } else
+ break;
+ }
+ if (grp_result == NULL) {
error("Could not find configured group %s", group_name);
+ xfree(grp_buffer);
return NULL;
}
- my_gid = grp_result->gr_gid;
-
- j = 0;
uid_cnt = 0;
-#ifdef HAVE_AIX
- setgrent_r(&fp);
- while (!getgrent_r(&grp, grp_buffer, PW_BUF_SIZE, &fp)) {
- grp_result = &grp;
-#elif defined (__APPLE__) || defined (__CYGWIN__)
- setgrent();
- while ((grp_result = getgrent()) != NULL) {
-#else
- setgrent();
- while (getgrent_r(&grp, grp_buffer, PW_BUF_SIZE,
- &grp_result) == 0 && grp_result != NULL) {
-#endif
- if (grp_result->gr_gid == my_gid) {
- if (strcmp(grp_result->gr_name, group_name)) {
- debug("including members of group '%s' as it "
- "corresponds to the same gid as group"
- " '%s'",grp_result->gr_name,group_name);
- }
-
- for (i=0; grp_result->gr_mem[i]; i++) {
- if (uid_from_string(grp_result->gr_mem[i],
- &my_uid) < 0) {
- /* Group member without valid login */
- continue;
- }
- if (my_uid == 0)
- continue;
- if (j+1 >= uid_cnt) {
- uid_cnt += 100;
- xrealloc(group_uids,
- (sizeof(uid_t) * uid_cnt));
- }
- group_uids[j++] = my_uid;
- }
- }
- }
-#ifdef HAVE_AIX
- endgrent_r(&fp);
- setpwent_r(&fp);
- while (!getpwent_r(&pw, pw_buffer, PW_BUF_SIZE, &fp)) {
- pwd_result = &pw;
-#else
- endgrent();
- setpwent();
-#if defined (__sun)
- while ((pwd_result = getpwent_r(&pw, pw_buffer, PW_BUF_SIZE)) != NULL) {
-#elif defined (__APPLE__) || defined (__CYGWIN__)
- while ((pwd_result = getpwent()) != NULL) {
-#else
- while (!getpwent_r(&pw, pw_buffer, PW_BUF_SIZE, &pwd_result)) {
-#endif
-#endif
- if (pwd_result->pw_gid != my_gid)
- continue;
- if (j+1 >= uid_cnt) {
- uid_cnt += 100;
- xrealloc(group_uids, (sizeof(uid_t) * uid_cnt));
- }
- group_uids[j++] = pwd_result->pw_uid;
+ for (i = 0; grp.gr_mem[i]; i++)
+ uid_cnt++;
+ group_uids = xmalloc(sizeof(uid_t) * uid_cnt);
+ j = 0;
+ for (i = 0; grp.gr_mem[i]; i++) {
+ uid_t u;
+ if (!uid_from_string(grp.gr_mem[i], &u))
+ group_uids[j++] = u;
}
-#ifdef HAVE_AIX
- endpwent_r(&fp);
-#else
- endpwent();
-#endif
-
+ xfree(grp_buffer);
_put_group_cache(group_name, group_uids, j);
_log_group_members(group_name, group_uids);
return group_uids;
