Review NACM and UNIX permissions #1354
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The sysrepocfg tool does not set up nacm based on $USER, and we now have the shell 'copy' tool which does. So drop 'cfg' and while we're at it we also drop the 'edit' alias. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
- The new SocketGroup setting allows for configurable /run/klishd.sock - Also a bug fix for async commands, needed to fix regressions in the klish-plugin-sysrepo commands 'change passwordk' and 'text-editor' after the latest upgrade to sysrepo + libyang v4 Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
We could go with the old default sha512crypt, but since the default has changed to yescrypt, as used by the 'change password' command. We use that for consistency. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
This patch enables umask and nacm defalts in the Buildoot sysrepo package and a patch to sysrepo to set the sysrepo group also on the event pipes in /etc/sysrepo/, without which it would be hard to inject any type of new event as non-root user now since the umask now prevent all 'other' users from interacting with sysrepo. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Previously the sys-cli group was for interactive shell access, but with ever changing requirements this split has become necessary. This commit introduces the 'sysrepo' group for low-level access to all sysrepo commands, i.e., bootstrap only. For user-level shell access a 'klish' group is added which allows users to connect to the CLI. This is now the only group users, including the default 'admin', are members of, effectively making the new 'copy' tool the norm. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Prevent non-admin level users from getting UNIX wheel group assignment. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Like err.h but without the leading "argv[0]: " prefix. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Since sysrepocfg does not do any NACM based on the UNIX user, we expand the scope of the copy tool slightly to allow outputting JSON to stdout. This allows us to replace all sysrepocfg commands in the CLI used to show runnining/startup/factory. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Show operational details about nacm and user//group mappings. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
- relocate src/show.py to src/bin/show/ - Refactor container() to use run_sysrepo() instead of rolling its own - Replace sysrepocfg with copy which supports nacm - Rename run_sysrepocfg() -> get_json() Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Otherwise the file will not be updated by confd on datastore copy: Jan 12 14:26:37 foo confd[3410]: Overwrite existing file /cfg/startup-config.cfg (y/N)? <FF> Jan 12 14:26:37 foo confd[3410]: Error: OK, aborting.:Inappropriate ioctl for device Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Fixes #1344 Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Switch RESTCONF from PUT to PATCH for configuration updates. This gives RESTCONF the same merge behavior as NETCONF edit-config, allowing NACM to correctly enforce path-specific permissions instead of requiring write access to the entire datastore. Fixes #617 Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Used by the new NACM basic test to verify ACL rules on explicit XPaths or modules. For this we cannot rely on put_config_dict() since it now uses the candidate datastore for all changes. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Need to verify that client hostnames have been set before starting the client, otherwise the test will fail due to clients getting pool lease, which is intended. In a real-world scenario this is not a problem. Here we've booby trapped the server to try to trip up errors. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
Slight refactor of WARNING and NOTE admonintions to improve readability. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
wkz
requested changes
Jan 20, 2026
Contributor
wkz
left a comment
wkz
left a comment
There was a problem hiding this comment.
What a lift from the current situation! Great work! 🔥
patches/sysrepo/4.2.10/0009-sr_mkfifo-set-sysrepo-group-if-available.patch
Show resolved
Hide resolved
Comment on lines
+448
to
+449
| else if (!dst) | ||
| err = systemf("cat %s", srcpath); |
Contributor
There was a problem hiding this comment.
I would really like to avoid any calls to system(), especially as we're running as root. Since srcpath is user provided, I think there might be Bobby;DROP TABLE issues hiding here.
I think you can use the subprocess() to implement cat() much like cp() above.
|
|
||
| *path = strdup((*ds)->path); | ||
| } else if (!*dst) { | ||
| return 0; |
Contributor
There was a problem hiding this comment.
If we move this check up to before the call to infix_ds(), then we won't have to have the NULL check bleed into that implementation as well.
Comment on lines
-630
to
+640
| if (argc - optind != 2) | ||
| if (argc - optind < 1) |
Contributor
There was a problem hiding this comment.
2nd thought: Could we not set dst to /dev/stdout when it is missing, and avoid having any special cases at all in the rest of the code?
mattiaswal
approved these changes
Jan 21, 2026
Contributor
mattiaswal
left a comment
mattiaswal
left a comment
There was a problem hiding this comment.
Awseome additions. I really like the restconf PATCH addition, may speed up tests slightly.
Contributor
Author
Thanks! Have a few fixes left to address from Tobias' review, but soon we'll have PATCH support available for all in Infamy 😎 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces two new user levels in Infix:
guestandoperator. Each can only read parts of the configuration and the latter is allowed limited management as well. The NACM rules and default user levels are documented in the user guide.Additionally, the default UNIX permissions for accessing sysrepo now require
rootaccess, which is limited to administrator level users. All access to the configuration, asadmin, is now done exclusively using thecopytool which respects NACM rules based on the user name. It is now a multicall binary with added XPath and RPC support, the latter under the guise of anrpccommand. See the usage text for each of the commands for details.Checklist
Tick relevant boxes, this PR is-a or has-a: