Implement createGroup/deleteGroup
This commit adds the implementation for CreateGroup and DeleteGroup.
These interfaces give the possibility to create OEMRole and OEMPrivilege
as proposed in the Dynamic Redfish Authz design.
[1] https://github.com/openbmc/docs/blob/master/designs/redfish-authorization.md
Since now secondary groups will change at runtime, this commit made the
|groupsMgr| non-constant. When the service starts up, it will load all
the groups in the system, and recover its |groupsMgr| in memory.
Currently, only groups with certain prefixes are allowed to change
(creation or deletion). The only use case now is Redfish previleges and
roles so the current prefixes only cover that.
Similar to user creation, this commit also added limits and checks to
make sure these interfaces are safe.
Coverage:
lines......: 84.1% (2197 of 2613 lines)
functions..: 94.3% (492 of 522 functions)
branches...: 31.1% (3506 of 11263 branches)
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I245017afda909a0bfa594ef112d7b0d40045f80d
diff --git a/test/user_mgr_test.cpp b/test/user_mgr_test.cpp
index 5db642e..8de2893 100644
--- a/test/user_mgr_test.cpp
+++ b/test/user_mgr_test.cpp
@@ -8,7 +8,9 @@
#include <exception>
#include <filesystem>
#include <fstream>
+#include <vector>
+#include <gmock/gmock.h>
#include <gtest/gtest.h>
namespace phosphor
@@ -278,6 +280,16 @@
ON_CALL(*this, executeUserModifyUserEnable)
.WillByDefault(testing::Return());
+
+ ON_CALL(*this, executeGroupCreation(testing::_))
+ .WillByDefault(testing::Return());
+
+ ON_CALL(*this, executeGroupDeletion(testing::_))
+ .WillByDefault(testing::Return());
+
+ ON_CALL(*this, executeGroupCreation).WillByDefault(testing::Return());
+
+ ON_CALL(*this, executeGroupDeletion).WillByDefault(testing::Return());
}
~UserMgrInTest() override
@@ -304,6 +316,10 @@
MOCK_METHOD(std::vector<std::string>, getFailedAttempt, (const char*),
(override));
+ MOCK_METHOD(void, executeGroupCreation, (const char*), (override));
+
+ MOCK_METHOD(void, executeGroupDeletion, (const char*), (override));
+
protected:
static sdbusplus::bus_t busInTest;
std::string tempPamConfigFile;
@@ -824,5 +840,160 @@
EXPECT_EQ(userLockedForFailedAttempt(username), false);
}
+TEST_F(UserMgrInTest, CheckAndThrowForDisallowedGroupCreationOnSuccess)
+{
+ // Base Redfish Roles
+ EXPECT_NO_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_rfr_Administrator"));
+ EXPECT_NO_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_rfr_Operator"));
+ EXPECT_NO_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_rfr_ReadOnly"));
+ // Base Redfish Privileges
+ EXPECT_NO_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_rfp_Login"));
+ EXPECT_NO_THROW(checkAndThrowForDisallowedGroupCreation(
+ "openbmc_rfp_ConfigureManager"));
+ EXPECT_NO_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_rfp_ConfigureUsers"));
+ EXPECT_NO_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_rfp_ConfigureSelf"));
+ EXPECT_NO_THROW(checkAndThrowForDisallowedGroupCreation(
+ "openbmc_rfp_ConfigureComponents"));
+ // OEM Redfish Roles
+ EXPECT_NO_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_orfr_PowerService"));
+ // OEM Redfish Privileges
+ EXPECT_NO_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_orfp_PowerService"));
+}
+
+TEST_F(UserMgrInTest,
+ CheckAndThrowForDisallowedGroupCreationThrowsIfGroupNameTooLong)
+{
+ std::string groupName(maxSystemGroupNameLength + 1, 'A');
+ EXPECT_THROW(
+ checkAndThrowForDisallowedGroupCreation(groupName),
+ sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
+}
+
+TEST_F(
+ UserMgrInTest,
+ CheckAndThrowForDisallowedGroupCreationThrowsIfGroupNameHasDisallowedCharacters)
+{
+
+ EXPECT_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_rfp_?owerService"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
+ EXPECT_THROW(
+ checkAndThrowForDisallowedGroupCreation("openbmc_rfp_-owerService"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
+}
+
+TEST_F(
+ UserMgrInTest,
+ CheckAndThrowForDisallowedGroupCreationThrowsIfGroupNameHasDisallowedPrefix)
+{
+
+ EXPECT_THROW(
+ checkAndThrowForDisallowedGroupCreation("google_rfp_"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
+ EXPECT_THROW(
+ checkAndThrowForDisallowedGroupCreation("com_rfp_"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
+}
+
+TEST_F(UserMgrInTest, CheckAndThrowForMaxGroupCountOnSuccess)
+{
+ EXPECT_THAT(allGroups().size(), 4);
+ for (size_t i = 0; i < maxSystemGroupCount - 4; ++i)
+ {
+ std::string groupName = "openbmc_rfr_role";
+ groupName += std::to_string(i);
+ EXPECT_NO_THROW(createGroup(groupName));
+ }
+ EXPECT_THROW(
+ createGroup("openbmc_rfr_AnotherRole"),
+ sdbusplus::xyz::openbmc_project::User::Common::Error::NoResource);
+ for (size_t i = 0; i < maxSystemGroupCount - 4; ++i)
+ {
+ std::string groupName = "openbmc_rfr_role";
+ groupName += std::to_string(i);
+ EXPECT_NO_THROW(deleteGroup(groupName));
+ }
+}
+
+TEST_F(UserMgrInTest, CheckAndThrowForGroupExist)
+{
+ std::string groupName = "openbmc_rfr_role";
+ EXPECT_NO_THROW(createGroup(groupName));
+ EXPECT_THROW(
+ createGroup(groupName),
+ sdbusplus::xyz::openbmc_project::User::Common::Error::GroupNameExists);
+ EXPECT_NO_THROW(deleteGroup(groupName));
+}
+
+TEST_F(UserMgrInTest, ByDefaultAllGroupsArePredefinedGroups)
+{
+ EXPECT_THAT(allGroups(),
+ testing::UnorderedElementsAre("web", "redfish", "ipmi", "ssh"));
+}
+
+TEST_F(UserMgrInTest, DeleteGroupThrowsIfGroupIsNotAllowedToChange)
+{
+ EXPECT_THROW(
+ deleteGroup("ipmi"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
+ EXPECT_THROW(
+ deleteGroup("web"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
+ EXPECT_THROW(
+ deleteGroup("redfish"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
+ EXPECT_THROW(
+ deleteGroup("ssh"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
+}
+
+TEST_F(UserMgrInTest,
+ CreateGroupThrowsInternalFailureWhenExecuteGroupCreateFails)
+{
+ EXPECT_CALL(*this, executeGroupCreation)
+ .WillOnce(testing::Throw(
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure()));
+ EXPECT_THROW(
+ createGroup("openbmc_rfr_role1"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure);
+}
+
+TEST_F(UserMgrInTest,
+ DeleteGroupThrowsInternalFailureWhenExecuteGroupDeleteFails)
+{
+ std::string groupName = "openbmc_rfr_role1";
+ EXPECT_NO_THROW(UserMgr::createGroup(groupName));
+ EXPECT_CALL(*this, executeGroupDeletion(testing::StrEq(groupName)))
+ .WillOnce(testing::Throw(
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure()))
+ .WillOnce(testing::DoDefault());
+
+ EXPECT_THROW(
+ deleteGroup(groupName),
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure);
+ EXPECT_NO_THROW(UserMgr::deleteGroup(groupName));
+}
+
+TEST_F(UserMgrInTest, CheckAndThrowForGroupNotExist)
+{
+ EXPECT_THROW(deleteGroup("whatever"),
+ sdbusplus::xyz::openbmc_project::User::Common::Error::
+ GroupNameDoesNotExist);
+}
+
+TEST(ReadAllGroupsOnSystemTest, OnlyReturnsPredefinedGroups)
+{
+ EXPECT_THAT(UserMgr::readAllGroupsOnSystem(),
+ testing::UnorderedElementsAre("web", "redfish", "ipmi", "ssh"));
+}
+
} // namespace user
} // namespace phosphor
diff --git a/user_mgr.cpp b/user_mgr.cpp
index bd63aac..f4a90ec 100644
--- a/user_mgr.cpp
+++ b/user_mgr.cpp
@@ -38,6 +38,7 @@
#include <xyz/openbmc_project/User/Common/error.hpp>
#include <algorithm>
+#include <array>
#include <ctime>
#include <fstream>
#include <numeric>
@@ -99,8 +100,52 @@
sdbusplus::xyz::openbmc_project::User::Common::Error::UserNameGroupFail;
using NoResource =
sdbusplus::xyz::openbmc_project::User::Common::Error::NoResource;
-
using Argument = xyz::openbmc_project::Common::InvalidArgument;
+using GroupNameExists =
+ sdbusplus::xyz::openbmc_project::User::Common::Error::GroupNameExists;
+using GroupNameDoesNotExists =
+ sdbusplus::xyz::openbmc_project::User::Common::Error::GroupNameDoesNotExist;
+
+namespace
+{
+
+// The hardcoded groups in OpenBMC projects
+constexpr std::array<const char*, 4> predefinedGroups = {"web", "redfish",
+ "ipmi", "ssh"};
+
+// These prefixes are for Dynamic Redfish authorization. See
+// https://github.com/openbmc/docs/blob/master/designs/redfish-authorization.md
+
+// Base role and base privileges are added by Redfish implementation (e.g.,
+// BMCWeb) at compile time
+constexpr std::array<const char*, 4> allowedGroupPrefix = {
+ "openbmc_rfr_", // OpenBMC Redfish Base Role
+ "openbmc_rfp_", // OpenBMC Redfish Base Privileges
+ "openbmc_orfr_", // OpenBMC Redfish OEM Role
+ "openbmc_orfp_", // OpenBMC Redfish OEM Privileges
+};
+
+void checkAndThrowsForGroupChangeAllowed(const std::string& groupName)
+{
+ bool allowed = false;
+ for (std::string_view prefix : allowedGroupPrefix)
+ {
+ if (groupName.starts_with(prefix))
+ {
+ allowed = true;
+ break;
+ }
+ }
+ if (!allowed)
+ {
+ log<level::ERR>("Invalid group name; not in the allowed list",
+ entry("GroupName=%s", groupName.c_str()));
+ elog<InvalidArgument>(Argument::ARGUMENT_NAME("Group Name"),
+ Argument::ARGUMENT_VALUE(groupName.c_str()));
+ }
+}
+
+} // namespace
std::string getCSVFromVector(std::span<const std::string> vec)
{
@@ -160,6 +205,21 @@
}
}
+void UserMgr::checkAndThrowForDisallowedGroupCreation(
+ const std::string& groupName)
+{
+ if (groupName.size() > maxSystemGroupNameLength ||
+ !std::regex_match(groupName.c_str(),
+ std::regex("[a-zA-z_][a-zA-Z_0-9]*")))
+ {
+ log<level::ERR>("Invalid group name",
+ entry("GroupName=%s", groupName.c_str()));
+ elog<InvalidArgument>(Argument::ARGUMENT_NAME("Group Name"),
+ Argument::ARGUMENT_VALUE(groupName.c_str()));
+ }
+ checkAndThrowsForGroupChangeAllowed(groupName);
+}
+
void UserMgr::throwForUserExists(const std::string& userName)
{
if (isUserExist(userName))
@@ -255,6 +315,30 @@
}
}
+std::vector<std::string> UserMgr::readAllGroupsOnSystem()
+{
+ std::vector<std::string> allGroups = {predefinedGroups.begin(),
+ predefinedGroups.end()};
+ // rewinds to the beginning of the group database
+ setgrent();
+ struct group* gr = getgrent();
+ while (gr != nullptr)
+ {
+ std::string group(gr->gr_name);
+ for (std::string_view prefix : allowedGroupPrefix)
+ {
+ if (group.starts_with(prefix))
+ {
+ allGroups.push_back(gr->gr_name);
+ }
+ }
+ gr = getgrent();
+ }
+ // close the group database
+ endgrent();
+ return allGroups;
+}
+
void UserMgr::createUser(std::string userName,
std::vector<std::string> groupNames, std::string priv,
bool enabled)
@@ -327,6 +411,73 @@
return;
}
+void UserMgr::checkDeleteGroupConstraints(const std::string& groupName)
+{
+ if (std::find(groupsMgr.begin(), groupsMgr.end(), groupName) ==
+ groupsMgr.end())
+ {
+ log<level::ERR>("Group already exists",
+ entry("GROUP_NAME=%s", groupName.c_str()));
+ elog<GroupNameDoesNotExists>();
+ }
+ checkAndThrowsForGroupChangeAllowed(groupName);
+}
+
+void UserMgr::deleteGroup(std::string groupName)
+{
+ checkDeleteGroupConstraints(groupName);
+ try
+ {
+ executeGroupDeletion(groupName.c_str());
+ }
+ catch (const InternalFailure& e)
+ {
+ log<level::ERR>("Group delete failed",
+ entry("GROUP_NAME=%s", groupName.c_str()));
+ elog<InternalFailure>();
+ }
+
+ groupsMgr.erase(std::find(groupsMgr.begin(), groupsMgr.end(), groupName));
+ UserMgrIface::allGroups(groupsMgr);
+ log<level::INFO>("Group deleted successfully",
+ entry("GROUP_NAME=%s", groupName.c_str()));
+}
+
+void UserMgr::checkCreateGroupConstraints(const std::string& groupName)
+{
+ if (std::find(groupsMgr.begin(), groupsMgr.end(), groupName) !=
+ groupsMgr.end())
+ {
+ log<level::ERR>("Group already exists",
+ entry("GROUP_NAME=%s", groupName.c_str()));
+ elog<GroupNameExists>();
+ }
+ checkAndThrowForDisallowedGroupCreation(groupName);
+ if (groupsMgr.size() >= maxSystemGroupCount)
+ {
+ log<level::ERR>("Group limit reached");
+ elog<NoResource>(xyz::openbmc_project::User::Common::NoResource::REASON(
+ "Group limit reached"));
+ }
+}
+
+void UserMgr::createGroup(std::string groupName)
+{
+ checkCreateGroupConstraints(groupName);
+ try
+ {
+ executeGroupCreation(groupName.c_str());
+ }
+ catch (const InternalFailure& e)
+ {
+ log<level::ERR>("Group create failed",
+ entry("GROUP_NAME=%s", groupName.c_str()));
+ elog<InternalFailure>();
+ }
+ groupsMgr.push_back(groupName);
+ UserMgrIface::allGroups(groupsMgr);
+}
+
void UserMgr::renameUser(std::string userName, std::string newUserName)
{
// All user management lock has to be based on /etc/shadow
@@ -668,7 +819,6 @@
boost::algorithm::split(splitWords, output[t2OutputIndex],
boost::algorithm::is_any_of("\t "),
boost::token_compress_on);
-
uint16_t failAttempts = 0;
try
{
@@ -1056,6 +1206,16 @@
return false;
}
+void UserMgr::executeGroupCreation(const char* groupName)
+{
+ executeCmd("/usr/sbin/groupadd", groupName);
+}
+
+void UserMgr::executeGroupDeletion(const char* groupName)
+{
+ executeCmd("/usr/sbin/groupdel", groupName);
+}
+
UserInfoMap UserMgr::getUserInfo(std::string userName)
{
UserInfoMap userInfo;
@@ -1280,7 +1440,9 @@
if (!userNameList.empty())
{
std::map<std::string, std::vector<std::string>> groupLists;
- for (auto& grp : groupsMgr)
+ // We only track users that are in the |predefinedGroups|
+ // The other groups don't contain real BMC users.
+ for (const char* grp : predefinedGroups)
{
if (grp == grpSsh)
{
@@ -1337,6 +1499,7 @@
pamAuthConfigFile(defaultPamAuthConfigFile)
{
UserMgrIface::allPrivileges(privMgr);
+ groupsMgr = readAllGroupsOnSystem();
std::sort(groupsMgr.begin(), groupsMgr.end());
UserMgrIface::allGroups(groupsMgr);
initializeAccountPolicy();
@@ -1389,17 +1552,5 @@
return executeCmd("/usr/sbin/pam_tally2", "-u", userName);
}
-void UserMgr::createGroup(std::string /*groupName*/)
-{
- log<level::ERR>("Not implemented yet");
- elog<InternalFailure>();
-}
-
-void UserMgr::deleteGroup(std::string /*groupName*/)
-{
- log<level::ERR>("Not implemented yet");
- elog<InternalFailure>();
-}
-
} // namespace user
} // namespace phosphor
diff --git a/user_mgr.hpp b/user_mgr.hpp
index d2dcb62..ec205e4 100644
--- a/user_mgr.hpp
+++ b/user_mgr.hpp
@@ -41,6 +41,8 @@
inline constexpr size_t ipmiMaxUsers = 15;
inline constexpr size_t maxSystemUsers = 30;
inline constexpr uint8_t minPasswdLength = 8;
+inline constexpr size_t maxSystemGroupNameLength = 32;
+inline constexpr size_t maxSystemGroupCount = 64;
using UserMgrIface = sdbusplus::xyz::openbmc_project::User::server::Manager;
using UserSSHLists =
@@ -240,6 +242,12 @@
*/
virtual size_t getIpmiUsersCount(void);
+ void createGroup(std::string groupName) override;
+
+ void deleteGroup(std::string groupName) override;
+
+ static std::vector<std::string> readAllGroupsOnSystem();
+
protected:
/** @brief get pam argument value
* method to get argument value from pam configuration
@@ -322,9 +330,9 @@
virtual void executeUserModifyUserEnable(const char* userName,
bool enabled);
- void createGroup(std::string groupName) override;
+ virtual void executeGroupCreation(const char* groupName);
- void deleteGroup(std::string groupName) override;
+ virtual void executeGroupDeletion(const char* groupName);
virtual std::vector<std::string> getFailedAttempt(const char* userName);
@@ -344,6 +352,22 @@
void initializeAccountPolicy();
+ /** @brief checks if the group creation meets all constraints
+ * @param groupName - group to check
+ */
+ void checkCreateGroupConstraints(const std::string& groupName);
+
+ /** @brief checks if the group deletion meets all constraints
+ * @param groupName - group to check
+ */
+ void checkDeleteGroupConstraints(const std::string& groupName);
+
+ /** @brief checks if the group name is legal and whether it's allowed to
+ * change. The daemon doesn't allow arbitrary group to be created
+ * @param groupName - group to check
+ */
+ void checkAndThrowForDisallowedGroupCreation(const std::string& groupName);
+
private:
/** @brief sdbusplus handler */
sdbusplus::bus_t& bus;
@@ -352,11 +376,11 @@
const std::string path;
/** @brief privilege manager container */
- std::vector<std::string> privMgr = {"priv-admin", "priv-operator",
- "priv-user"};
+ const std::vector<std::string> privMgr = {"priv-admin", "priv-operator",
+ "priv-user"};
/** @brief groups manager container */
- std::vector<std::string> groupsMgr = {"web", "redfish", "ipmi", "ssh"};
+ std::vector<std::string> groupsMgr;
/** @brief map container to hold users object */
using UserName = std::string;