UserMgr: Fix the privilege determination
By default, all users in Active Directory have the primary group
`users`. Giving the full access to the BMC to all users from the such
group is a bad idea. And changing the primary group in
Active Directory/LDAP can be inadvisable.
This fix allows to use in the role mapping the group that isn't the
primary group. All members of the such group will get the role,
according with the role mapping.
Tested by:
- Configure LDAP
- Add non primary LDAP group to the role map
- Verify `GetUserInfo` reply for the member of the group used in the
previous step. It should contain corresponding privilege.
- Add primary LDAP group to the role map and verify `GetUserInfo` for
its member. It also should contain corresponding role.
Change-Id: I61a87a21446577c0bf059f50139c7b4c711059c7
Signed-off-by: Alexander Filippov <a.filippov@yadro.com>
diff --git a/test/mock_user_mgr.hpp b/test/mock_user_mgr.hpp
index 9a0ecdd..aa5409b 100644
--- a/test/mock_user_mgr.hpp
+++ b/test/mock_user_mgr.hpp
@@ -15,10 +15,13 @@
MockManager(sdbusplus::bus_t& bus, const char* path) : UserMgr(bus, path)
{}
- MOCK_METHOD1(getLdapGroupName, std::string(const std::string& userName));
MOCK_METHOD0(getPrivilegeMapperObject, DbusUserObj());
MOCK_METHOD1(userLockedForFailedAttempt, bool(const std::string& userName));
MOCK_METHOD1(userPasswordExpired, bool(const std::string& userName));
+ MOCK_CONST_METHOD1(getPrimaryGroup, gid_t(const std::string& userName));
+ MOCK_CONST_METHOD3(isGroupMember,
+ bool(const std::string& userName, gid_t primaryGid,
+ const std::string& groupName));
friend class TestUserMgr;
};
diff --git a/test/user_mgr_test.cpp b/test/user_mgr_test.cpp
index ae8f35d..5db642e 100644
--- a/test/user_mgr_test.cpp
+++ b/test/user_mgr_test.cpp
@@ -17,9 +17,12 @@
{
using ::testing::Return;
+using ::testing::Throw;
using InternalFailure =
sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure;
+using UserNameDoesNotExist =
+ sdbusplus::xyz::openbmc_project::User::Common::Error::UserNameDoesNotExist;
class TestUserMgr : public testing::Test
{
@@ -91,9 +94,10 @@
std::string userName = "user";
UserInfoMap userInfo;
- EXPECT_CALL(mockManager, getLdapGroupName(userName))
- .WillRepeatedly(Return(""));
- EXPECT_THROW(userInfo = mockManager.getUserInfo(userName), InternalFailure);
+ EXPECT_CALL(mockManager, getPrimaryGroup(userName))
+ .WillRepeatedly(Throw(UserNameDoesNotExist()));
+ EXPECT_THROW(userInfo = mockManager.getUserInfo(userName),
+ UserNameDoesNotExist);
}
TEST_F(TestUserMgr, localUser)
@@ -121,13 +125,16 @@
UserInfoMap userInfo;
std::string userName = "ldapUser";
std::string ldapGroup = "ldapGroup";
+ gid_t primaryGid = 1000;
- EXPECT_CALL(mockManager, getLdapGroupName(userName))
- .WillRepeatedly(Return(ldapGroup));
+ EXPECT_CALL(mockManager, getPrimaryGroup(userName))
+ .WillRepeatedly(Return(primaryGid));
// Create privilege mapper dbus object
DbusUserObj object = createPrivilegeMapperDbusObject();
EXPECT_CALL(mockManager, getPrivilegeMapperObject())
.WillRepeatedly(Return(object));
+ EXPECT_CALL(mockManager, isGroupMember(userName, primaryGid, ldapGroup))
+ .WillRepeatedly(Return(true));
userInfo = mockManager.getUserInfo(userName);
EXPECT_EQ(true, std::get<bool>(userInfo["RemoteUser"]));
EXPECT_EQ("priv-admin", std::get<std::string>(userInfo["UserPrivilege"]));
@@ -135,16 +142,20 @@
TEST_F(TestUserMgr, ldapUserWithoutPrivMapper)
{
+ using ::testing::_;
+
UserInfoMap userInfo;
std::string userName = "ldapUser";
std::string ldapGroup = "ldapGroup";
+ gid_t primaryGid = 1000;
- EXPECT_CALL(mockManager, getLdapGroupName(userName))
- .WillRepeatedly(Return(ldapGroup));
+ EXPECT_CALL(mockManager, getPrimaryGroup(userName))
+ .WillRepeatedly(Return(primaryGid));
// Create LDAP config object without privilege mapper
DbusUserObj object = createLdapConfigObjectWithoutPrivilegeMapper();
EXPECT_CALL(mockManager, getPrivilegeMapperObject())
.WillRepeatedly(Return(object));
+ EXPECT_CALL(mockManager, isGroupMember(_, _, _)).Times(0);
userInfo = mockManager.getUserInfo(userName);
EXPECT_EQ(true, std::get<bool>(userInfo["RemoteUser"]));
EXPECT_EQ("", std::get<std::string>(userInfo["UserPrivilege"]));
diff --git a/user_mgr.cpp b/user_mgr.cpp
index aa2fd15..bd63aac 100644
--- a/user_mgr.cpp
+++ b/user_mgr.cpp
@@ -935,53 +935,6 @@
return objects;
}
-std::string UserMgr::getLdapGroupName(const std::string& userName)
-{
- struct passwd pwd
- {};
- struct passwd* pwdPtr = nullptr;
- auto buflen = sysconf(_SC_GETPW_R_SIZE_MAX);
- if (buflen < -1)
- {
- // Use a default size if there is no hard limit suggested by sysconf()
- buflen = 1024;
- }
- std::vector<char> buffer(buflen);
- gid_t gid = 0;
-
- auto status =
- getpwnam_r(userName.c_str(), &pwd, buffer.data(), buflen, &pwdPtr);
- // On success, getpwnam_r() returns zero, and set *pwdPtr to pwd.
- // If no matching password record was found, these functions return 0
- // and store NULL in *pwdPtr
- if (!status && (&pwd == pwdPtr))
- {
- gid = pwd.pw_gid;
- }
- else
- {
- log<level::ERR>("User does not exist",
- entry("USER_NAME=%s", userName.c_str()));
- elog<UserNameDoesNotExist>();
- }
-
- struct group* groups = nullptr;
- std::string ldapGroupName;
-
- while ((groups = getgrent()) != NULL)
- {
- if (groups->gr_gid == gid)
- {
- ldapGroupName = groups->gr_name;
- break;
- }
- }
- // Call endgrent() to close the group database.
- endgrent();
-
- return ldapGroupName;
-}
-
std::string UserMgr::getServiceName(std::string&& path, std::string&& intf)
{
auto mapperCall = bus.new_method_call(objMapperService, objMapperPath,
@@ -1010,6 +963,99 @@
return mapperResponse.begin()->first;
}
+gid_t UserMgr::getPrimaryGroup(const std::string& userName) const
+{
+ static auto buflen = sysconf(_SC_GETPW_R_SIZE_MAX);
+ if (buflen <= 0)
+ {
+ // Use a default size if there is no hard limit suggested by sysconf()
+ buflen = 1024;
+ }
+
+ struct passwd pwd;
+ struct passwd* pwdPtr = nullptr;
+ std::vector<char> buffer(buflen);
+
+ auto status = getpwnam_r(userName.c_str(), &pwd, buffer.data(),
+ buffer.size(), &pwdPtr);
+ // On success, getpwnam_r() returns zero, and set *pwdPtr to pwd.
+ // If no matching password record was found, these functions return 0
+ // and store NULL in *pwdPtr
+ if (!status && (&pwd == pwdPtr))
+ {
+ return pwd.pw_gid;
+ }
+
+ log<level::ERR>("User noes not exist",
+ entry("USER_NAME=%s", userName.c_str()));
+ elog<UserNameDoesNotExist>();
+}
+
+bool UserMgr::isGroupMember(const std::string& userName, gid_t primaryGid,
+ const std::string& groupName) const
+{
+ static auto buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
+ if (buflen <= 0)
+ {
+ // Use a default size if there is no hard limit suggested by sysconf()
+ buflen = 1024;
+ }
+
+ struct group grp;
+ struct group* grpPtr = nullptr;
+ std::vector<char> buffer(buflen);
+
+ auto status = getgrnam_r(groupName.c_str(), &grp, buffer.data(),
+ buffer.size(), &grpPtr);
+
+ // Groups with a lot of members may require a buffer of bigger size than
+ // suggested by _SC_GETGR_R_SIZE_MAX.
+ // 32K should be enough for about 2K members.
+ constexpr auto maxBufferLength = 32 * 1024;
+ while (status == ERANGE && buflen < maxBufferLength)
+ {
+ buflen *= 2;
+ buffer.resize(buflen);
+
+ log<level::DEBUG>("Increase buffer for getgrnam_r()",
+ entry("BUFFER_LENGTH=%zu", buflen));
+
+ status = getgrnam_r(groupName.c_str(), &grp, buffer.data(),
+ buffer.size(), &grpPtr);
+ }
+
+ // On success, getgrnam_r() returns zero, and set *grpPtr to grp.
+ // If no matching group record was found, these functions return 0
+ // and store NULL in *grpPtr
+ if (!status && (&grp == grpPtr))
+ {
+ if (primaryGid == grp.gr_gid)
+ {
+ return true;
+ }
+
+ for (auto i = 0; grp.gr_mem && grp.gr_mem[i]; ++i)
+ {
+ if (userName == grp.gr_mem[i])
+ {
+ return true;
+ }
+ }
+ }
+ else if (status == ERANGE)
+ {
+ log<level::ERR>("Group info requires too much memory",
+ entry("GROUP_NAME=%s", groupName.c_str()));
+ }
+ else
+ {
+ log<level::ERR>("Group does not exist",
+ entry("GROUP_NAME=%s", groupName.c_str()));
+ }
+
+ return false;
+}
+
UserInfoMap UserMgr::getUserInfo(std::string userName)
{
UserInfoMap userInfo;
@@ -1028,13 +1074,7 @@
}
else
{
- std::string ldapGroupName = getLdapGroupName(userName);
- if (ldapGroupName.empty())
- {
- log<level::ERR>("Unable to get group name",
- entry("USER_NAME=%s", userName.c_str()));
- elog<InternalFailure>();
- }
+ auto primaryGid = getPrimaryGroup(userName);
DbusUserObj objects = getPrivilegeMapperObject();
@@ -1043,29 +1083,19 @@
try
{
- for (const auto& obj : objects)
+ for (const auto& [path, interfaces] : objects)
{
- for (const auto& interface : obj.second)
+ auto it = interfaces.find("xyz.openbmc_project.Object.Enable");
+ if (it != interfaces.end())
{
- if ((interface.first ==
- "xyz.openbmc_project.Object.Enable"))
+ auto propIt = it->second.find("Enabled");
+ if (propIt != it->second.end() &&
+ std::get<bool>(propIt->second))
{
- for (const auto& property : interface.second)
- {
- auto value = std::get<bool>(property.second);
- if ((property.first == "Enabled") &&
- (value == true))
- {
- ldapConfigPath = obj.first;
- break;
- }
- }
+ ldapConfigPath = path.str + '/';
+ break;
}
}
- if (!ldapConfigPath.empty())
- {
- break;
- }
}
if (ldapConfigPath.empty())
@@ -1073,36 +1103,38 @@
return userInfo;
}
- for (const auto& obj : objects)
+ for (const auto& [path, interfaces] : objects)
{
- for (const auto& interface : obj.second)
+ if (!path.str.starts_with(ldapConfigPath))
{
- if ((interface.first ==
- "xyz.openbmc_project.User.PrivilegeMapperEntry") &&
- (obj.first.str.find(ldapConfigPath) !=
- std::string::npos))
- {
- std::string privilege;
- std::string groupName;
+ continue;
+ }
- for (const auto& property : interface.second)
+ auto it = interfaces.find(
+ "xyz.openbmc_project.User.PrivilegeMapperEntry");
+ if (it != interfaces.end())
+ {
+ std::string privilege;
+ std::string groupName;
+
+ for (const auto& [propName, propValue] : it->second)
+ {
+ if (propName == "GroupName")
{
- auto value = std::get<std::string>(property.second);
- if (property.first == "GroupName")
- {
- groupName = value;
- }
- else if (property.first == "Privilege")
- {
- privilege = value;
- }
+ groupName = std::get<std::string>(propValue);
}
- if (groupName == ldapGroupName)
+ else if (propName == "Privilege")
{
- userPrivilege = privilege;
- break;
+ privilege = std::get<std::string>(propValue);
}
}
+
+ if (!groupName.empty() && !privilege.empty() &&
+ isGroupMember(userName, primaryGid, groupName))
+ {
+ userPrivilege = privilege;
+ break;
+ }
}
if (!userPrivilege.empty())
{
diff --git a/user_mgr.hpp b/user_mgr.hpp
index 3ba0012..d2dcb62 100644
--- a/user_mgr.hpp
+++ b/user_mgr.hpp
@@ -63,8 +63,7 @@
using DbusUserPropVariant = std::variant<Privilege, ServiceEnabled>;
-using DbusUserObjProperties =
- std::vector<std::pair<PropertyName, DbusUserPropVariant>>;
+using DbusUserObjProperties = std::map<PropertyName, DbusUserPropVariant>;
using Interface = std::string;
@@ -403,15 +402,24 @@
*/
std::string getServiceName(std::string&& path, std::string&& intf);
- protected:
- /** @brief get LDAP group name
- * method to get LDAP group name for the given LDAP user
+ /** @brief get primary group ID of specified user
*
- * @param[in] - userName
- * @return - group name
+ * @param[in] - userName
+ * @return - primary group ID
*/
- virtual std::string getLdapGroupName(const std::string& userName);
+ virtual gid_t getPrimaryGroup(const std::string& userName) const;
+ /** @brief check whether if the user is a member of the group
+ *
+ * @param[in] - userName
+ * @param[in] - ID of the user's primary group
+ * @param[in] - groupName
+ * @return - true if the user is a member of the group
+ */
+ virtual bool isGroupMember(const std::string& userName, gid_t primaryGid,
+ const std::string& groupName) const;
+
+ protected:
/** @brief get privilege mapper object
* method to get dbus privilege mapper object
*