From 2d90f0f93a409dcb251560f572b3ccee0b8eb2b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 15:54:36 +0000 Subject: [PATCH 1/2] Make minimum UID/GID configurable via multiuser.minuid/mingid The UID/GID thresholds below which a mapped username is rejected as a system account were hard-coded to 500 (g_minimum_uid / g_minimum_gid). Sites that mount shared file systems (e.g. Lustre) may have legitimate groups with IDs below 500, which were being denied access. Convert the thresholds to static members of UserSentry (single definition with external linkage, mirroring m_is_cmsd) so a value set at config time is seen across all translation units, and parse two new optional config directives, multiuser.minuid and multiuser.mingid, in MultiuserFileSystem::Config(). Both default to 500, preserving existing behavior. The effective thresholds are logged at startup. Closes #68 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UFwX4qfurnnEvbPtzL46ZL --- README.md | 17 ++++++++++++ configs/60-osg-multiuser.cfg | 7 +++++ src/MultiuserFileSystem.cc | 52 ++++++++++++++++++++++++++++++++++++ src/UserSentry.hh | 24 ++++++++++++----- src/multiuser.cpp | 5 ++++ 5 files changed, 99 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 42f897e..9a471ca 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,23 @@ To enable the checksum (only on XRootD 5.2+): ofs.ckslib * libXrdMultiuser.so ``` +The following optional directives can also be set in the Xrootd configuration file: + +| Directive | Default | Description | +| --- | --- | --- | +| `multiuser.umask ` | (unset) | Apply this umask to files and directories created through the plugin. | +| `multiuser.checksumonwrite ` | `off` | Compute checksums while a file is being written. | +| `multiuser.minuid ` | `500` | Minimum UID a mapped username may resolve to; usernames mapping to a lower UID are treated as system accounts and denied. | +| `multiuser.mingid ` | `500` | Minimum GID a mapped username may resolve to; usernames mapping to a lower GID are treated as system accounts and denied. | + +For example, to allow users and groups with IDs as low as 100 (e.g., groups +imported from a Lustre file system): + +``` +multiuser.minuid 100 +multiuser.mingid 100 +``` + Startup ------- diff --git a/configs/60-osg-multiuser.cfg b/configs/60-osg-multiuser.cfg index 050c646..69ea8b9 100644 --- a/configs/60-osg-multiuser.cfg +++ b/configs/60-osg-multiuser.cfg @@ -17,4 +17,11 @@ if defined ?~XC_ENABLE_MULTIUSER # checksum while it is writing a file. To turn this on, uncomment the # following line: # multiuser.checksumonwrite on + + # Usernames that map to a UID or GID below these thresholds are treated as + # system accounts and are denied access. Both default to 500. Lower them + # if your site has legitimate users/groups with smaller IDs (for example, + # groups imported from a Lustre file system): + # multiuser.minuid 500 + # multiuser.mingid 500 fi diff --git a/src/MultiuserFileSystem.cc b/src/MultiuserFileSystem.cc index 714a99f..7de7445 100644 --- a/src/MultiuserFileSystem.cc +++ b/src/MultiuserFileSystem.cc @@ -68,6 +68,31 @@ MultiuserFileSystem::Config(XrdSysLogger *lp, const char *configfn) } Config.Attach(cfgFD); const char *val; + + // Parse a single non-negative integer argument for the given directive. + // On success stores the value in `out` and returns true; on any error it + // logs an appropriate message and returns false. + auto parse_min_id = [&](const char *directive, int &out) -> bool { + val = Config.GetWord(); + if (!val || !val[0]) { + m_log.Emsg("Config", directive, "must specify a value"); + return false; + } + char *endptr = NULL; + errno = 0; + long int id_val = strtol(val, &endptr, 10); + if (errno || (endptr && *endptr != '\0')) { + m_log.Emsg("Config", directive, "must specify a valid integer value"); + return false; + } + if (id_val < 0) { + m_log.Emsg("Config", directive, "must not be negative"); + return false; + } + out = static_cast(id_val); + return true; + }; + while ((val = Config.GetMyFirstWord())) { if (!strcmp("multiuser.umask", val)) { val = Config.GetWord(); @@ -92,6 +117,26 @@ MultiuserFileSystem::Config(XrdSysLogger *lp, const char *configfn) m_umask_mode = umask_val; } + // Minimum UID a mapped username may resolve to. + if (!strcmp("multiuser.minuid", val)) { + int min_uid = 0; + if (!parse_min_id("multiuser.minuid", min_uid)) { + Config.Close(); + return false; + } + UserSentry::SetMinimumUid(min_uid); + } + + // Minimum GID a mapped username may resolve to. + if (!strcmp("multiuser.mingid", val)) { + int min_gid = 0; + if (!parse_min_id("multiuser.mingid", min_gid)) { + Config.Close(); + return false; + } + UserSentry::SetMinimumGid(min_gid); + } + // Checksum on write if (!strcmp("multiuser.checksumonwrite", val)) { val = Config.GetWord(); @@ -159,6 +204,13 @@ MultiuserFileSystem::Config(XrdSysLogger *lp, const char *configfn) umask(m_umask_mode); } + { + std::stringstream ss; + ss << "Requiring mapped users to have a minimum UID of " << UserSentry::GetMinimumUid() + << " and a minimum GID of " << UserSentry::GetMinimumGid(); + m_log.Emsg("Config", ss.str().c_str()); + } + return true; } diff --git a/src/UserSentry.hh b/src/UserSentry.hh index fb2c9f5..ad47e46 100644 --- a/src/UserSentry.hh +++ b/src/UserSentry.hh @@ -23,10 +23,6 @@ #include #include -// TODO: set this via library parameters. -static const int g_minimum_uid = 500; -static const int g_minimum_gid = 500; - /** * Note: originally, we tried to use CAP_DAC_READ_SEARCH as that provides exactly what we need - @@ -110,6 +106,14 @@ public: static bool IsCmsd() {return m_is_cmsd;} + // Configure the minimum UID/GID a mapped username may resolve to. Any + // username resolving to an ID below these thresholds is treated as a + // system account and denied access. Defaults to 500 (see multiuser.cpp). + static void SetMinimumUid(int uid) {m_min_uid = uid;} + static void SetMinimumGid(int gid) {m_min_gid = gid;} + static int GetMinimumUid() {return m_min_uid;} + static int GetMinimumGid() {return m_min_gid;} + static bool IsGsiUserMapped(const XrdSecEntity *client) { // If VOMS was used to map client, return true if (client->vorg) { return true; } @@ -164,11 +168,14 @@ public: } return; } - if (pwd.pw_uid < g_minimum_uid) { + // Cast to a signed type for the comparison: m_min_uid/m_min_gid are + // mutable ints, so comparing them directly against the unsigned + // pw_uid/pw_gid would trip -Wsign-compare (and -Werror). + if (static_cast(pwd.pw_uid) < m_min_uid) { m_log.Emsg("UserSentry", "Multiuser denying access: Username", username.c_str(), "maps to a system UID; rejecting lookup"); return; } - if (pwd.pw_gid < g_minimum_gid) { + if (static_cast(pwd.pw_gid) < m_min_gid) { m_log.Emsg("UserSentry", "Multiuser denying access: Username", username.c_str(), "maps to a system GID; rejecting lookup"); return; } @@ -229,6 +236,11 @@ private: static bool m_is_cmsd; + // Minimum UID/GID thresholds; configurable via multiuser.minuid / + // multiuser.mingid. Definitions (and defaults) live in multiuser.cpp. + static int m_min_uid; + static int m_min_gid; + XrdSysError &m_log; }; diff --git a/src/multiuser.cpp b/src/multiuser.cpp index 522cff3..91b98c4 100644 --- a/src/multiuser.cpp +++ b/src/multiuser.cpp @@ -26,6 +26,11 @@ ChecksumManager* g_checksum_manager = nullptr; bool UserSentry::m_is_cmsd = false; +// Default minimum UID/GID. Usernames mapping to an ID below these values are +// rejected as system accounts. Override via multiuser.minuid / multiuser.mingid. +int UserSentry::m_min_uid = 500; +int UserSentry::m_min_gid = 500; + XrdVERSIONINFO(XrdOssGetFileSystem, Multiuser); // The status-quo to retrieve the default object is to copy/paste the From 5f98de6bc6e5f723608c6e9e78a42b67a14df211 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 21:24:14 +0000 Subject: [PATCH 2/2] Fix review feedback for min UID/GID handling --- src/MultiuserFileSystem.cc | 24 ++++++++++++++++++------ src/UserSentry.hh | 19 ++++++++----------- src/multiuser.cpp | 4 ++-- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/MultiuserFileSystem.cc b/src/MultiuserFileSystem.cc index 7de7445..9f99df6 100644 --- a/src/MultiuserFileSystem.cc +++ b/src/MultiuserFileSystem.cc @@ -18,11 +18,13 @@ #include "MultiuserFile.hh" #include +#include #include #include #include #include #include +#include #include #include @@ -72,7 +74,7 @@ MultiuserFileSystem::Config(XrdSysLogger *lp, const char *configfn) // Parse a single non-negative integer argument for the given directive. // On success stores the value in `out` and returns true; on any error it // logs an appropriate message and returns false. - auto parse_min_id = [&](const char *directive, int &out) -> bool { + auto parse_min_id = [&](const char *directive, long int &out) -> bool { val = Config.GetWord(); if (!val || !val[0]) { m_log.Emsg("Config", directive, "must specify a value"); @@ -89,7 +91,7 @@ MultiuserFileSystem::Config(XrdSysLogger *lp, const char *configfn) m_log.Emsg("Config", directive, "must not be negative"); return false; } - out = static_cast(id_val); + out = id_val; return true; }; @@ -119,22 +121,32 @@ MultiuserFileSystem::Config(XrdSysLogger *lp, const char *configfn) // Minimum UID a mapped username may resolve to. if (!strcmp("multiuser.minuid", val)) { - int min_uid = 0; + long int min_uid = 0; if (!parse_min_id("multiuser.minuid", min_uid)) { Config.Close(); return false; } - UserSentry::SetMinimumUid(min_uid); + if (static_cast(min_uid) > static_cast(std::numeric_limits::max())) { + m_log.Emsg("Config", "multiuser.minuid exceeds the maximum supported UID value"); + Config.Close(); + return false; + } + UserSentry::SetMinimumUid(static_cast(min_uid)); } // Minimum GID a mapped username may resolve to. if (!strcmp("multiuser.mingid", val)) { - int min_gid = 0; + long int min_gid = 0; if (!parse_min_id("multiuser.mingid", min_gid)) { Config.Close(); return false; } - UserSentry::SetMinimumGid(min_gid); + if (static_cast(min_gid) > static_cast(std::numeric_limits::max())) { + m_log.Emsg("Config", "multiuser.mingid exceeds the maximum supported GID value"); + Config.Close(); + return false; + } + UserSentry::SetMinimumGid(static_cast(min_gid)); } // Checksum on write diff --git a/src/UserSentry.hh b/src/UserSentry.hh index ad47e46..c83c605 100644 --- a/src/UserSentry.hh +++ b/src/UserSentry.hh @@ -109,10 +109,10 @@ public: // Configure the minimum UID/GID a mapped username may resolve to. Any // username resolving to an ID below these thresholds is treated as a // system account and denied access. Defaults to 500 (see multiuser.cpp). - static void SetMinimumUid(int uid) {m_min_uid = uid;} - static void SetMinimumGid(int gid) {m_min_gid = gid;} - static int GetMinimumUid() {return m_min_uid;} - static int GetMinimumGid() {return m_min_gid;} + static void SetMinimumUid(uid_t uid) {m_min_uid = uid;} + static void SetMinimumGid(gid_t gid) {m_min_gid = gid;} + static uid_t GetMinimumUid() {return m_min_uid;} + static gid_t GetMinimumGid() {return m_min_gid;} static bool IsGsiUserMapped(const XrdSecEntity *client) { // If VOMS was used to map client, return true @@ -168,14 +168,11 @@ public: } return; } - // Cast to a signed type for the comparison: m_min_uid/m_min_gid are - // mutable ints, so comparing them directly against the unsigned - // pw_uid/pw_gid would trip -Wsign-compare (and -Werror). - if (static_cast(pwd.pw_uid) < m_min_uid) { + if (pwd.pw_uid < m_min_uid) { m_log.Emsg("UserSentry", "Multiuser denying access: Username", username.c_str(), "maps to a system UID; rejecting lookup"); return; } - if (static_cast(pwd.pw_gid) < m_min_gid) { + if (pwd.pw_gid < m_min_gid) { m_log.Emsg("UserSentry", "Multiuser denying access: Username", username.c_str(), "maps to a system GID; rejecting lookup"); return; } @@ -238,8 +235,8 @@ private: // Minimum UID/GID thresholds; configurable via multiuser.minuid / // multiuser.mingid. Definitions (and defaults) live in multiuser.cpp. - static int m_min_uid; - static int m_min_gid; + static uid_t m_min_uid; + static gid_t m_min_gid; XrdSysError &m_log; }; diff --git a/src/multiuser.cpp b/src/multiuser.cpp index 91b98c4..6dc0437 100644 --- a/src/multiuser.cpp +++ b/src/multiuser.cpp @@ -28,8 +28,8 @@ bool UserSentry::m_is_cmsd = false; // Default minimum UID/GID. Usernames mapping to an ID below these values are // rejected as system accounts. Override via multiuser.minuid / multiuser.mingid. -int UserSentry::m_min_uid = 500; -int UserSentry::m_min_gid = 500; +uid_t UserSentry::m_min_uid = 500; +gid_t UserSentry::m_min_gid = 500; XrdVERSIONINFO(XrdOssGetFileSystem, Multiuser);