feat: Add user management CLI commands #4053
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
07074c2 to
4c2b878
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
4c2b878 to
c4ad7ec
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
c4ad7ec to
c1b2c4b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
c1b2c4b to
4c2b878
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
4c2b878 to
403fe8f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
403fe8f to
5c8dc01
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
5c8dc01 to
c4ad7ec
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
c4ad7ec to
b17d729
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
b17d729 to
a301178
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
a301178 to
a9647a9
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
a9647a9 to
39d26d7
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Add comprehensive CLI interface for SONiC local user management: - config user add/modify/delete - User lifecycle management - config user add-ssh-key/remove-ssh-key - SSH key management - config user import-existing - Import system users to SONiC - show user [username] - Display user information and status - show user roles - Display available roles and permissions Features: - Role-based access control (administrator/operator) - Secure password handling with confirmation prompts - SSH key format validation and management - CONFIG_DB integration for persistent configuration - Comprehensive input validation and error handling - Complete test suite with unit and integration tests
manoharan-nexthop
force-pushed
the
sonic-net.user-management
branch
from
39d26d7 to
c307f06
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/Semgrep |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive user management CLI commands for SONiC, addressing the User Management HLD requirements. The implementation provides role-based access control (administrator/operator), SSH key management, and security policy configuration with integration to CONFIG_DB for persistent storage.
Key Changes
- User lifecycle management commands (
config user add/modify/delete) with role-based access control - SSH key management with validation and multi-key support
- Security policy configuration for password hardening and login attempt limits
- Display commands (
show user) with permission-based password hash visibility - Import functionality to migrate existing Linux users to SONiC user management
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
config/user.py |
Implements user management configuration commands including add, modify, delete, import-existing, and security policy management |
show/user.py |
Implements display commands for user information, details, and security policies with permission-based filtering |
config/main.py |
Integrates user management commands into main config CLI |
show/main.py |
Integrates user display commands into main show CLI |
tests/config_user_test.py |
Comprehensive test suite covering user lifecycle, SSH keys, security policies, password validation, and error scenarios |
tests/show_user_test.py |
Test suite for show commands including permission checks, SSH key formatting, and various data scenarios |
tests/mock_tables/config_db.json |
Adds test data for LOCAL_USER and LOCAL_ROLE_SECURITY_POLICY tables |
Comments suppressed due to low confidence (1)
config/user.py:193
- 'except' clause does nothing but pass and there is no explanatory comment.
except (ValueError, TypeError):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Check for empty password | ||
| if not password or password.strip() == "": | ||
| click.echo("Error: Password cannot be empty") | ||
| time.sleep(3) # Security delay to prevent rapid brute-force attempts |
There was a problem hiding this comment.
The security delay of 3 seconds after password validation failure could be bypassed by interrupting the command or by timing attacks. Consider implementing rate limiting at a lower level (e.g., in the authentication system) rather than relying on time.sleep() in the CLI, which can be easily bypassed.
| # Remove ssh_keys field if result is empty (all keys were removed) | ||
| del updated_data['ssh_keys'] |
There was a problem hiding this comment.
When SSH keys result is empty (all keys removed), the code attempts to delete the ssh_keys field from updated_data. However, in CONFIG_DB, it's typically better to set empty fields to an empty list [] rather than deleting them, to maintain schema consistency. The deletion could cause issues with YANG validation or other components expecting this field to exist. Consider setting updated_data['ssh_keys'] = [] instead of deleting the field.
| # Remove ssh_keys field if result is empty (all keys were removed) | |
| del updated_data['ssh_keys'] | |
| # Set ssh_keys field to empty list if result is empty (all keys were removed) | |
| updated_data['ssh_keys'] = [] |
| if 'len_min' in policies: | ||
| min_len = policies['len_min'] | ||
| if len(password) < min_len: | ||
| errors.append(f"Password must be at least {min_len} characters long") |
There was a problem hiding this comment.
When len_min field fails to convert to integer (caught at line 193), the code continues and tries to use it in comparison at line 209. This will cause a TypeError when comparing len(password) (int) with a string value. The code should validate that min_len is an integer before using it in the comparison, or raise an error during policy loading if the field has an invalid type.
| current_username = getpass.getuser() | ||
| users = get_user_database() | ||
|
|
||
| user_data = users.get(current_username) | ||
| return user_data and user_data.get('role') == 'administrator' |
There was a problem hiding this comment.
The getpass.getuser() call could raise an exception in certain environments (e.g., when running in containers without proper user mapping). This exception is not caught, which could cause the entire show command to fail. Consider wrapping this in a try-except block and returning False if the username cannot be determined, to ensure the show commands remain functional even in edge cases.
| # Verify command succeeded | ||
| self.assertEqual(result.exit_code, 0) | ||
| # Silent on success - no output expected | ||
|
|
There was a problem hiding this comment.
The comment says "Silent on success - no output expected", but the test doesn't actually verify that no output is produced. Consider adding an assertion like self.assertEqual(result.output.strip(), "") to explicitly verify silent success behavior, which appears to be an intentional design choice for SONiC CLI commands.
| return None, False | ||
| finally: | ||
| # Always clear passwords from memory | ||
| password = None |
There was a problem hiding this comment.
Variable password is not used.
| finally: | ||
| # Always clear passwords from memory | ||
| password = None | ||
| confirm_password = None |
There was a problem hiding this comment.
Variable confirm_password is not used.
| confirm_password = None |
| mock_get_policies.return_value = mock_policies | ||
|
|
||
| # Use valid role name instead of 'test_role' | ||
| result = self.runner.invoke(show_user.show_security_policy, ['administrator'], obj=self.db) |
| result = self.runner.invoke(show_user.show_security_policy, [], obj=self.db) | ||
| # Should handle gracefully | ||
| self.assertEqual(result.exit_code, 0) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| for line in f: | ||
| line = line.strip() | ||
| if line and not line.startswith('#'): | ||
| ssh_keys.append(line) |
There was a problem hiding this comment.
Nested for statement uses loop variable 'line' of enclosing for statement.
| for line in f: | |
| line = line.strip() | |
| if line and not line.startswith('#'): | |
| ssh_keys.append(line) | |
| for ssh_line in f: | |
| ssh_line = ssh_line.strip() | |
| if ssh_line and not ssh_line.startswith('#'): | |
| ssh_keys.append(ssh_line) |
5 participants
What I did
This implementation addresses the User Management HLD requirements for user administration in SONiC. sonic-net/SONiC#2018
Add CLI interface for SONiC local user management:
How I did it
Features:
Commands integrate with userd daemon for system-level user operations.
How to verify it
Validated the CLI and its functionality.
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)