feat: Add user management daemon #309

manoharan-nexthop · 2025-09-08T08:27:11Z

This implementation addresses the User Management HLD requirements for centralized user administration in SONiC. sonic-net/SONiC#2018

1. User Management Daemon (userd):

Implemented C++ daemon using SWSS framework for CONFIG_DB integration
Added user lifecycle management (create/update/delete/enable/disable)
Implemented role-based group assignment (administrator, operator roles)
Added SSH key management with proper file permissions and ownership
Used posix_spawn() for secure command execution without shell interpretation
Integrated PAM faillock configuration using Jinja2 templates

2. Build System Integration:

Added CMakeLists.txt for C++ compilation with SWSS dependencies
Created debian packaging with proper control files and dependencies
Added systemd service configuration for userd daemon
Integrated Makefile for building and installation

3. Security Features:

Secure password handling using system's native hashing methods
Role-based group assignments with predefined security groups
PAM faillock integration for login attempt limiting

mssonicbld · 2025-09-08T08:27:19Z

/azp run

azure-pipelines · 2025-09-08T08:27:28Z

Azure Pipelines successfully started running 1 pipeline(s).

data/debian/control

aidan-gallagher · 2025-09-25T11:41:24Z

data/debian/rules

 %:
 	dh $@

+override_dh_auto_build:


I believe these changes have no effect?

You override the default behaviour (dh_auto_build) and then invoke the default behaviour (dh_auto_build).

debian/control

mssonicbld · 2025-09-25T13:30:14Z

/azp run

azure-pipelines · 2025-09-25T13:30:24Z

Azure Pipelines successfully started running 1 pipeline(s).

spandan-nexthop · 2025-09-28T02:43:49Z

userd/src/userd.cpp

+        // Add user to role-specific groups
+        for (const std::string& group : it->second) {
+            if (!is_user_in_group(username, group)) {
+                std::vector<std::string> cmd = {"/usr/sbin/usermod", "-a", "-G", group, username};
+                SystemCommand::execute(cmd); // Don't fail if group doesn't exist
+                SWSS_LOG_DEBUG("Added user %s to group %s", username.c_str(), group.c_str());
+            } else {
+                SWSS_LOG_DEBUG("User %s already in group %s", username.c_str(), group.c_str());
+            }
+        }


Would we ever need to remove users from a group? For example if one is downgraded from admin to operator?

spandan-nexthop · 2025-09-28T02:47:02Z

userd/src/userd.cpp

+    bool set_user_password(const std::string& username, const std::string& password_hash) {
+        std::vector<std::string> cmd = {"/usr/sbin/usermod", "-p", password_hash, username};
+
+        if (!SystemCommand::execute(cmd)) {


SystemCommand::execute with log the hash as well. Do we want that?

SONiC needed a centralized user management daemon that can: - Monitor CONFIG_DB for user configuration changes - Manage local system users based on SONiC configuration - Provide role-based access control with predefined groups - Handle SSH key management with proper security - Integrate with PAM for authentication policies - Support efficient change detection to minimize system calls **1. User Management Daemon (userd):** - Implemented C++ daemon using SWSS framework for CONFIG_DB integration - Added comprehensive user lifecycle management (create/update/delete/enable/disable) - Implemented role-based group assignment (administrator, operator roles) - Added SSH key management with proper file permissions and ownership - Used posix_spawn() for secure command execution without shell interpretation - Added efficient change detection using UserInfo comparison to avoid unnecessary system calls - Integrated PAM faillock configuration using Jinja2 templates **2. Build System Integration:** - Added CMakeLists.txt for C++ compilation with SWSS dependencies - Created debian packaging with proper control files and dependencies - Added systemd service configuration for userd daemon - Integrated Makefile for building and installation **3. Security Features:** - Secure password handling using system's native hashing methods - Proper file permissions for SSH keys (600) and directories (700) - Role-based group assignments with predefined security groups - PAM faillock integration for login attempt limiting - Input validation and sanitization for all user operations **4. Testing Framework:** - Added comprehensive unit tests for userd functionality - Integration tests for CONFIG_DB interaction - User lifecycle testing with proper cleanup - SSH key management testing - Role-based access control validation

mssonicbld · 2025-09-29T15:33:17Z

/azp run

azure-pipelines · 2025-09-29T15:33:26Z

Azure Pipelines successfully started running 1 pipeline(s).

rd-foredge · 2025-11-15T03:25:16Z

data/templates/faillock.conf.j2

+# Role-based security policies
+{% for role, policy in security_policies.items() %}
+{% if policy.max_login_attempts %}
+# {{ role }} role settings


does faillock support individual config per group/role/user ?

This is applied per role.

Copilot

Pull request overview

This PR implements a user management daemon (userd) for SONiC that provides centralized user administration capabilities. The daemon integrates with CONFIG_DB to manage local users, roles, SSH keys, and security policies, replacing manual user management with an automated, database-driven approach.

Key Changes

Core daemon implementation (userd.cpp): 1190-line C++ daemon using SWSS framework for CONFIG_DB integration, implementing full user lifecycle management (create/update/delete/enable/disable), role-based access control, SSH key management, and PAM faillock configuration
Build and packaging infrastructure: Complete CMake build system, Debian packaging with systemd service configuration, and Makefile for compilation and installation
Security policy templates: Jinja2 template for rendering PAM faillock configuration based on role-specific security policies

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 25 comments.

Show a summary per file

File	Description
userd/src/userd.cpp	Main daemon implementation with UserManager class for CONFIG_DB-driven user lifecycle management, SSH key handling, PAM configuration, and security policy enforcement
userd/src/CMakeLists.txt	CMake build configuration with C++17, compiler flags, SWSS dependencies, and installation rules
userd/Makefile	Top-level Makefile with build-cpp, install, and clean targets for development and packaging
userd/debian/control	Debian package metadata with dependencies on SWSS, nlohmann-json, and crypt libraries
userd/debian/sonic-host-userd.service	Systemd service unit with dependencies on config-setup and sonic.target
userd/debian/rules	Debian packaging rules integrating with Makefile build system
userd/debian/install	Installation manifest placing userd binary in /usr/local/bin
userd/debian/changelog	Package changelog with initial release information
userd/debian/compat	Debhelper compatibility level 11
userd/debian/copyright	Apache 2.0 license information
data/templates/faillock.conf.j2	Jinja2 template for rendering PAM faillock configuration from security policies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-25T16:48:28Z

userd/src/userd.cpp

+            std::string j2_command = "j2 " + PAM_FAILLOCK_TEMPLATE + " " + temp_json_file;
+            std::string rendered_content;
+            FILE* pipe = popen(j2_command.c_str(), "r");


The popen() function executes the command through a shell, which is a security risk. Since the template path and temp file are constructed from variables, this could potentially be exploited if those paths are manipulated. Consider using posix_spawn() with proper argument parsing (similar to SystemCommand::execute()) or at least validate/sanitize the paths before using them in the shell command.

Copilot · 2025-11-25T16:48:28Z

userd/src/userd.cpp

+            template_data["security_policies"] = policies;
+
+            // Write JSON to temporary file
+            std::string temp_json_file = "/tmp/security_policies.json";


The temporary JSON file /tmp/security_policies.json is created in a predictable location without using mkstemp() or similar secure temporary file creation. This could lead to symlink attacks or race conditions where an attacker could manipulate the file before it's used. Use mkstemp() or a similar secure method to create temporary files with unpredictable names.

Copilot · 2025-11-25T16:48:28Z

userd/src/userd.cpp

+        if (!user_config.password_hash.empty() &&
+            current_info.password_hash != user_config.password_hash) {
+            if (!set_user_password(username, user_config.password_hash)) {
+                return false;
+            }
+        }
+
+        if (current_info.shell != expected_config.shell) {
+            if (!set_user_shell(username, user_config.enabled)) {
+                return false;
+            }
+        }
+
+        if (!user_config.role.empty() && current_info.role != user_config.role) {
+            SWSS_LOG_INFO("Changing user %s role from '%s' to '%s'",
+                         username.c_str(), current_info.role.c_str(), user_config.role.c_str());
+            if (!set_user_groups(username, user_config.role)) {
+                return false;
+            }
+        }
+
+        if (current_info.ssh_keys != user_config.ssh_keys) {
+            if (!setup_ssh_keys(username, user_config.ssh_keys)) {
+                return false;
+            }
+        }
+
+        SWSS_LOG_INFO("Updated user %s", username.c_str());
+        return true;


Inconsistent error handling: when updating the password hash fails (line 863), the function continues checking for other differences but then returns false if password update failed. However, when shell or groups update fails (lines 869-881), the function also returns false but may have already applied partial changes (e.g., password was updated). This could leave users in an inconsistent state. Consider using a transaction-like approach or at least log which updates succeeded before failing.

Suggested change

if (!user_config.password_hash.empty() &&

current_info.password_hash != user_config.password_hash) {

if (!set_user_password(username, user_config.password_hash)) {

return false;

}

}

if (current_info.shell != expected_config.shell) {

if (!set_user_shell(username, user_config.enabled)) {

return false;

}

}

if (!user_config.role.empty() && current_info.role != user_config.role) {

SWSS_LOG_INFO("Changing user %s role from '%s' to '%s'",

username.c_str(), current_info.role.c_str(), user_config.role.c_str());

if (!set_user_groups(username, user_config.role)) {

return false;

}

}

if (current_info.ssh_keys != user_config.ssh_keys) {

if (!setup_ssh_keys(username, user_config.ssh_keys)) {

return false;

}

}

SWSS_LOG_INFO("Updated user %s", username.c_str());

return true;

bool password_updated = true;

bool shell_updated = true;

bool groups_updated = true;

bool ssh_keys_updated = true;

if (!user_config.password_hash.empty() &&

current_info.password_hash != user_config.password_hash) {

password_updated = set_user_password(username, user_config.password_hash);

if (!password_updated) {

SWSS_LOG_ERROR("Failed to update password for user %s", username.c_str());

}

}

if (current_info.shell != expected_config.shell) {

shell_updated = set_user_shell(username, user_config.enabled);

if (!shell_updated) {

SWSS_LOG_ERROR("Failed to update shell for user %s", username.c_str());

}

}

if (!user_config.role.empty() && current_info.role != user_config.role) {

SWSS_LOG_INFO("Changing user %s role from '%s' to '%s'",

username.c_str(), current_info.role.c_str(), user_config.role.c_str());

groups_updated = set_user_groups(username, user_config.role);

if (!groups_updated) {

SWSS_LOG_ERROR("Failed to update groups for user %s", username.c_str());

}

}

if (current_info.ssh_keys != user_config.ssh_keys) {

ssh_keys_updated = setup_ssh_keys(username, user_config.ssh_keys);

if (!ssh_keys_updated) {

SWSS_LOG_ERROR("Failed to update SSH keys for user %s", username.c_str());

}

}

if (password_updated && shell_updated && groups_updated && ssh_keys_updated) {

SWSS_LOG_INFO("Updated user %s", username.c_str());

return true;

} else {

std::ostringstream oss;

oss << "Partial update for user " << username << ": ";

oss << "password=" << (password_updated ? "ok" : "fail") << ", ";

oss << "shell=" << (shell_updated ? "ok" : "fail") << ", ";

oss << "groups=" << (groups_updated ? "ok" : "fail") << ", ";

oss << "ssh_keys=" << (ssh_keys_updated ? "ok" : "fail");

SWSS_LOG_ERROR("%s", oss.str().c_str());

return false;

}

Copilot · 2025-11-25T16:48:29Z

userd/src/userd.cpp

+
+            for (const std::string& key : ssh_keys) {
+                file << key << "\n";
+            }


The file stream is not explicitly checked for write errors. The file << key << "\n" operation could fail silently (e.g., disk full). Check the stream state after writing or use file.flush() with error checking before closing.

Suggested change

}

}

if (!file) {

SWSS_LOG_ERROR("Failed to write SSH keys to authorized_keys file for user %s", username.c_str());

file.close();

return false;

}

Copilot · 2025-11-25T16:48:29Z

userd/src/userd.cpp

+            std::vector<gid_t> groups(ngroups);
+            if (getgrouplist(username.c_str(), pw->pw_gid, groups.data(), &ngroups) != -1) {
+                for (gid_t gid : groups) {
+                    struct group* grp = getgrgid(gid);
+                    if (grp && std::string(grp->gr_name) == groupname) {
+                        return true;
+                    }


The getgrouplist() function is called twice: first with nullptr to get the count, then with the actual buffer. However, the second call could return -1 if the group list changed between calls (race condition) or if the buffer is too small. Check the return value of the second getgrouplist() call and handle the -1 case appropriately.

Suggested change

std::vector<gid_t> groups(ngroups);

if (getgrouplist(username.c_str(), pw->pw_gid, groups.data(), &ngroups) != -1) {

for (gid_t gid : groups) {

struct group* grp = getgrgid(gid);

if (grp && std::string(grp->gr_name) == groupname) {

return true;

}

int max_attempts = 5;

int attempts = 0;

while (attempts < max_attempts) {

std::vector<gid_t> groups(ngroups);

int ret = getgrouplist(username.c_str(), pw->pw_gid, groups.data(), &ngroups);

if (ret != -1) {

for (gid_t gid : groups) {

struct group* grp = getgrgid(gid);

if (grp && std::string(grp->gr_name) == groupname) {

return true;

}

}

break;

} else {

// Buffer was too small, ngroups updated to required size

attempts++;

Copilot · 2025-11-25T16:48:34Z

userd/debian/changelog

+
+  * Initial release
+
+ -- SONiC Maintainers <maintainers@sonic.net>  Mon, 11 Aug 2025 08:00:00 +0000


[nitpick] The changelog indicates a future date (Mon, 11 Aug 2025) which is unusual. Typically changelog dates should reflect the actual release date or use the current date. Consider using the current date or a placeholder that will be updated upon release.

Suggested change

-- SONiC Maintainers <maintainers@sonic.net> Mon, 11 Aug 2025 08:00:00 +0000

-- SONiC Maintainers <maintainers@sonic.net> TODO: update upon release

Copilot · 2025-11-25T16:48:34Z

userd/src/userd.cpp

+        if (!user_config.password_hash.empty() &&
+            current_info.password_hash != user_config.password_hash) {
+            if (!set_user_password(username, user_config.password_hash)) {
+                return false;
+            }
+        }


Password hash comparison is attempted but current_info.password_hash is never populated from system data. The get_existing_users() function doesn't retrieve password hashes from /etc/shadow, so this comparison (line 863) will always show a difference if a password_hash is provided in the config. Either populate the password hash when reading existing users (using getspnam()) or remove this comparison since password hashes can't be reliably compared.

Copilot · 2025-11-25T16:48:34Z

userd/src/userd.cpp

+        UserInfo expected_config = user_config;
+        expected_config.shell = user_config.enabled ? "/bin/bash" : "/usr/sbin/nologin";
+
+        // Compare the configurations
+        if (current_info == expected_config) {
+            SWSS_LOG_DEBUG("User %s configuration is already up to date", username.c_str());
+            return true;
+        }


The comparison current_info == expected_config will always fail because current_info.password_hash is never populated (it's empty), while expected_config.password_hash contains the hash from the configuration. This means users will be unnecessarily updated on every consistency check. Consider excluding password_hash from the comparison or properly populate it in get_existing_users().

Copilot · 2025-11-25T16:48:35Z

userd/src/userd.cpp

+                UserInfo user;
+                user.username = key;


Username validation is missing. Usernames from CONFIG_DB are used directly in system commands without validation. Malicious usernames containing special characters (e.g., semicolons, spaces, shell metacharacters) could potentially cause issues or be used for injection attacks. Add validation to ensure usernames match expected patterns (e.g., ^[a-z_][a-z0-9_-]*[$]?$).

Copilot · 2025-11-25T16:48:35Z

userd/src/userd.cpp

+    std::string role;
+    std::string password_hash;
+    std::vector<std::string> ssh_keys;
+    bool enabled;


The enabled field is not initialized in the default constructor of UserInfo. If a user entry in CONFIG_DB doesn't have an explicit "enabled" field, the value will be uninitialized, leading to undefined behavior. Add a default value initialization in the struct definition (e.g., bool enabled = true;).

Suggested change

bool enabled;

bool enabled = true;

manoharan-nexthop mentioned this pull request Sep 8, 2025

HLD for persistent local user management sonic-net/SONiC#2018

Open

manoharan-nexthop changed the title ~~[userd] Add user management daemon~~ feat: Add user management daemon Sep 8, 2025

aidan-gallagher reviewed Sep 25, 2025

View reviewed changes

debian/control Outdated Show resolved Hide resolved

aidan-gallagher reviewed Sep 25, 2025

View reviewed changes

debian/control Outdated Show resolved Hide resolved

manoharan-nexthop force-pushed the sonic-net.user-management branch from 153ac44 to 2846d1f Compare September 25, 2025 13:30

spandan-nexthop reviewed Sep 28, 2025

View reviewed changes

manoharan-nexthop force-pushed the sonic-net.user-management branch from 2846d1f to 3bbde60 Compare September 29, 2025 15:33

rd-foredge reviewed Nov 15, 2025

View reviewed changes

lguohan requested a review from Copilot November 25, 2025 16:36

Copilot started reviewing on behalf of lguohan November 25, 2025 16:37 View session

Copilot finished reviewing on behalf of lguohan November 25, 2025 16:41

Copilot AI reviewed Nov 25, 2025

View reviewed changes

-            }
+            }
+            if (!file) {
+                SWSS_LOG_ERROR("Failed to write SSH keys to authorized_keys file for user %s", username.c_str());
+                file.close();
+                return false;
+            }


		* Initial release

		-- SONiC Maintainers <maintainers@sonic.net> Mon, 11 Aug 2025 08:00:00 +0000

feat: Add user management daemon #309

Are you sure you want to change the base?

feat: Add user management daemon #309

Uh oh!

Conversation

manoharan-nexthop commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Sep 8, 2025

Uh oh!

azure-pipelines bot commented Sep 8, 2025

Uh oh!

Uh oh!

aidan-gallagher Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Sep 25, 2025

Uh oh!

azure-pipelines bot commented Sep 25, 2025

Uh oh!

spandan-nexthop Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

spandan-nexthop Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Sep 29, 2025

Uh oh!

azure-pipelines bot commented Sep 29, 2025

Uh oh!

rd-foredge Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

manoharan-nexthop Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

manoharan-nexthop commented Sep 8, 2025 •

edited

Loading