Fix annoying "permission denied" when saving running-config #979
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
For future readers, the user paramenter to the copy() function is actually the remote user in any curl command. Also, realign columns in infix_config[]. Signed-off-by: Joachim Wiberg <[email protected]>
When copying a file to a directory, the file should be accessible (read + write) by all members of the group that are allowed write access. E.g., an 'admin' level user creating a new file /cfg/foo.cfg should result in the file being owned by $LOGNAME:wheel with 0660 perms, because /cfg is root:wheel. Writing to already existing file, e.g., created by 'root' at first boot, say /cfg/startup-config.cfg should be possible by all members of the 'wheel' group. In this case thef file already exists as root:wheel and any user trying to chgrp it will fail, this is fine. Fixes #977 Signed-off-by: Joachim Wiberg <[email protected]>
As per earlier decsision, the .cfg suffix is no longer implied when a user copy to a regular file, so we should allow tab completion in the copy command to show foo.cfg, otherwise the copy command will fail. Skip /cfg/startup-config.cfg since it's treated as a special case and added to the list by the infix_datastore() plugin function. Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
Signed-off-by: Joachim Wiberg <[email protected]>
17 tasks
wkz
reviewed
Mar 9, 2025
Comment on lines
+90
to
+92
| * E.g., writing to /cfg/foo, where /cfg is owned by root:wheel, | ||
| * should result in the file being owned by $LOGNAME:wheel with | ||
| * 0660 perms for other users in same group. |
There was a problem hiding this comment.
If we made sure that we create startup with g+s, could we rely on the kernel to handle this logic for us?
There was a problem hiding this comment.
If we made sure that we create
startupwithg+s, could we rely on the kernel to handle this logic for us?
You mean on /cfg, I assume. Yes, but I was worrying about bind + overlay mounts, as well as external media with vfat/exfat file systems.
rical
reviewed
Mar 10, 2025
| return 0; | ||
| } | ||
|
|
||
| getgrouplist(user, pw->pw_gid, groups, &num); |
There was a problem hiding this comment.
Isn't there's a theoretical race here, if the user is added to a group after the last call to getgrouplist()? If so, perhaps it's worth wrapping this in an if (..... != -1).
There was a problem hiding this comment.
There is, wrapping it with a realloc() is probably a good idea to be safer.
src/bin/copy.c
Outdated
| return 0; | ||
|
|
||
| strlcpy(dir, fn, sizeof(dir)); | ||
| ptr = strrchr(dir, '/'); |
There was a problem hiding this comment.
Can this block use dirname() to make it easier to understand?
|
CCB: assigning to @rical to fix the comments. Thanks! |
Avoid a theoretical race between getting the number of groups a user belongs to and actually processing them. We should not need to check the return value of getgrouplist() as we know the number of groups fit inside the buffer. Signed-off-by: Richard Alpe <[email protected]>
Cosmetic change intended to improve code readability. Signed-off-by: Richard Alpe <[email protected]>
Prior to this patch the errno value was overwritten by getgrgid() making the printouts invalid: Error: setting group owner wheel (10) on /cfg/startup-config.cfg: Success Signed-off-by: Richard Alpe <[email protected]>
Signed-off-by: Richard Alpe <[email protected]>
The code assumes both SRC and DST is passed, here we ensure this early. Prior to this patch: root@host:~$ copy foo Segmentation fault (core dumped) Signed-off-by: Richard Alpe <[email protected]>
Signed-off-by: Richard Alpe <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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
Fixes #977
Checklist
Tick relevant boxes, this PR is-a or has-a: