From df1fc6f2d7e2297813388ce0cacd1d953ca4a496 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Sun, 9 Mar 2025 13:49:03 +0100 Subject: [PATCH 01/11] bin: copy: reduce confusion username vs user -> remote_user 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 --- src/bin/copy.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/bin/copy.c b/src/bin/copy.c index 1649c28c..942d58ef 100644 --- a/src/bin/copy.c +++ b/src/bin/copy.c @@ -23,11 +23,11 @@ struct infix_ds { }; struct infix_ds infix_config[] = { - { "startup-config", "startup", SR_DS_STARTUP, 1, "/cfg/startup-config.cfg" }, - { "running-config", "running", SR_DS_RUNNING, 1, NULL }, - { "candidate-config", "candidate", SR_DS_CANDIDATE, 1, NULL }, - { "operational-config", "operational", SR_DS_OPERATIONAL, 1, NULL }, - { "factory-config", "factory-default", SR_DS_FACTORY_DEFAULT, 0, NULL } + { "startup-config", "startup", SR_DS_STARTUP, 1, "/cfg/startup-config.cfg" }, + { "running-config", "running", SR_DS_RUNNING, 1, NULL }, + { "candidate-config", "candidate", SR_DS_CANDIDATE, 1, NULL }, + { "operational-config", "operational", SR_DS_OPERATIONAL, 1, NULL }, + { "factory-config", "factory-default", SR_DS_FACTORY_DEFAULT, 0, NULL } }; static const char *prognm = "copy"; @@ -108,15 +108,15 @@ static const char *infix_ds(const char *text, struct infix_ds **ds) } -static int copy(const char *src, const char *dst, const char *user) +static int copy(const char *src, const char *dst, const char *remote_user) { struct infix_ds *srcds = NULL, *dstds = NULL; char temp_file[20] = "/tmp/copy.XXXXXX"; const char *tmpfn = NULL; sr_session_ctx_t *sess; const char *fn = NULL; - const char *username; sr_conn_ctx_t *conn; + const char *user; char adjust[256]; mode_t oldmask; int rc = 0; @@ -135,7 +135,7 @@ static int copy(const char *src, const char *dst, const char *user) goto err; } - username = getuser(); + user = getuser(); /* 1. Regular ds copy */ if (srcds && dstds) { @@ -157,13 +157,13 @@ static int copy(const char *src, const char *dst, const char *user) if (sr_session_start(conn, dstds->datastore, &sess)) { fprintf(stderr, ERRMSG "unable to open transaction to %s\n", dst); } else { - sr_nacm_set_user(sess, username); + sr_nacm_set_user(sess, user); rc = sr_copy_config(sess, NULL, srcds->datastore, timeout * 1000); if (rc) emsg(sess, ERRMSG "unable to copy configuration, err %d: %s\n", rc, sr_strerror(rc)); else - set_owner(dstds->path, username); + set_owner(dstds->path, user); } rc = sr_disconnect(conn); @@ -187,11 +187,11 @@ static int copy(const char *src, const char *dst, const char *user) if (rc) fprintf(stderr, ERRMSG "failed exporting %s to %s\n", src, fn); else { - rc = systemf("curl %s -LT %s %s", user, fn, dst); + rc = systemf("curl %s -LT %s %s", remote_user, fn, dst); if (rc) fprintf(stderr, ERRMSG "failed uploading %s to %s\n", src, dst); else - set_owner(dst, username); + set_owner(dst, user); } goto err; } @@ -219,7 +219,7 @@ static int copy(const char *src, const char *dst, const char *user) if (rc) fprintf(stderr, ERRMSG "failed copy %s to %s\n", src, fn); else - set_owner(fn, username); + set_owner(fn, user); } else if (dstds) { if (!dstds->sysrepocfg) { fprintf(stderr, ERRMSG "not possible to import to this datastore.\n"); @@ -245,7 +245,7 @@ static int copy(const char *src, const char *dst, const char *user) } if (tmpfn) - rc = systemf("curl %s -Lo %s %s", user, fn, src); + rc = systemf("curl %s -Lo %s %s", remote_user, fn, src); if (rc) { fprintf(stderr, ERRMSG "failed downloading %s", src); } else { @@ -274,7 +274,7 @@ static int copy(const char *src, const char *dst, const char *user) } } - rc = systemf("curl %s -Lo %s %s", user, fn, src); + rc = systemf("curl %s -Lo %s %s", remote_user, fn, src); } else if (strstr(dst, "://")) { fn = cfg_adjust(src, NULL, adjust, sizeof(adjust)); if (!fn) { @@ -286,7 +286,7 @@ static int copy(const char *src, const char *dst, const char *user) if (access(fn, F_OK)) fprintf(stderr, ERRMSG "no such file %s, aborting.", fn); else - rc = systemf("curl %s -LT %s %s", user, fn, dst); + rc = systemf("curl %s -LT %s %s", remote_user, fn, dst); } else { if (!access(dst, F_OK)) { if (!yorn("Overwrite existing file %s", dst)) { From 5021a1da88b243def496d2d208c09a2ff4146b4c Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Sun, 9 Mar 2025 16:05:02 +0100 Subject: [PATCH 02/11] bin: copy: set group owner on (new) files 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 --- src/bin/copy.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 80 insertions(+), 6 deletions(-) diff --git a/src/bin/copy.c b/src/bin/copy.c index 942d58ef..680d0a70 100644 --- a/src/bin/copy.c +++ b/src/bin/copy.c @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -64,6 +65,9 @@ end: } } +/* + * Current system user, same as sysrepo user + */ static char *getuser(void) { const struct passwd *pw; @@ -79,18 +83,87 @@ static char *getuser(void) return pw->pw_name; } +/* + * If UNIX user is in UNIX group of directory containing file, + * return gid, otherwise -1 + * + * 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. + */ +static gid_t in_group(const char *user, const char *fn, gid_t *gid) +{ + char dir[strlen(fn) + 2]; + const struct passwd *pw; + int i, num = 0, rc = 0; + struct stat st; + gid_t *groups; + char *ptr; + + pw = getpwnam(user); + if (!pw) + return 0; + + strlcpy(dir, fn, sizeof(dir)); + ptr = strrchr(dir, '/'); + if (ptr) + *ptr = 0; + else + strlcpy(dir, ".", sizeof(dir)); + + if (stat(dir, &st)) + return 0; + + getgrouplist(user, pw->pw_gid, NULL, &num); + + groups = malloc(num * sizeof(gid_t)); + if (!groups) { + perror("in_group() malloc"); + return 0; + } + + getgrouplist(user, pw->pw_gid, groups, &num); + for (i = 0; i < num; i++) { + if (groups[i] == st.st_gid) { + *gid = st.st_gid; + rc = 1; + break; + } + } + free(groups); + + return rc; +} + +/* + * Set group owner so other users with same directory permissions can + * read/write the file as well. E.g., an 'admin' level user in group + * 'wheel' writing a new file to `/cfg` should be possible to read and + * write to by other administrators. + * + * This function is called only when the file has been successfully + * copied or created in a file system directory. This is why we can + * safely ignore any EPERM errors to chown(), below, because if the file + * already existed, created by another user, we are not allowed to chgrp + * it. The sole purpose of this function is to allow other users in the + * same group to access the file in the future. + */ static void set_owner(const char *fn, const char *user) { - struct passwd *pw; + gid_t gid = 9999; if (!fn) return; /* not an error, e.g., running-config is not a file */ - pw = getpwnam(user); - if (pw && !chmod(fn, 0660) && !chown(fn, pw->pw_uid, pw->pw_gid)) - return; + if (!in_group(user, fn, &gid)) + return; /* user not in parent directory's group */ - fprintf(stderr, ERRMSG "setting owner %s on %s: %s\n", user, fn, strerror(errno)); + if (chown(fn, -1, gid) && errno != EPERM) { + const struct group *gr = getgrgid(gid); + + fprintf(stderr, ERRMSG "setting group owner %s (%d) on %s: %s\n", + gr ? gr->gr_name : "", gid, fn, strerror(errno)); + } } static const char *infix_ds(const char *text, struct infix_ds **ds) @@ -121,7 +194,8 @@ static int copy(const char *src, const char *dst, const char *remote_user) mode_t oldmask; int rc = 0; - oldmask = umask(0660); + /* rw for user and group only */ + oldmask = umask(0006); src = infix_ds(src, &srcds); if (!src) From 82df624ae90945557514c37f339105bd266bb26b Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Sun, 9 Mar 2025 16:41:51 +0100 Subject: [PATCH 03/11] cli: show file extension of copy src and dst on tab completion 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 --- src/bin/files.c | 4 ++++ src/klish-plugin-infix/src/infix.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/bin/files.c b/src/bin/files.c index 85f8066d..e289f55d 100644 --- a/src/bin/files.c +++ b/src/bin/files.c @@ -28,6 +28,10 @@ int files(const char *path, const char *stripext) if (d->d_type != DT_REG || d->d_name[0] == '.') continue; + /* skip startup in /cfg, listed by plugin */ + if (!strcmp(path, "/cfg") && !strcmp(d->d_name, "startup-config.cfg")) + continue; + strlcpy(name, d->d_name, sizeof(name)); if (stripext) { size_t pos = has_ext(name, stripext); diff --git a/src/klish-plugin-infix/src/infix.c b/src/klish-plugin-infix/src/infix.c index ff6c9496..54bae655 100644 --- a/src/klish-plugin-infix/src/infix.c +++ b/src/klish-plugin-infix/src/infix.c @@ -102,7 +102,7 @@ int infix_datastore(kcontext_t *ctx) } done: - return systemf("files /cfg .cfg"); + return systemf("files /cfg"); } int infix_erase(kcontext_t *ctx) From 496d56e97519ada7999d31382bdc4c062bcdb44d Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Sun, 9 Mar 2025 16:43:41 +0100 Subject: [PATCH 04/11] cli: fix dir command's $HOME when not logged in as root Signed-off-by: Joachim Wiberg --- board/common/rootfs/usr/bin/dir | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/board/common/rootfs/usr/bin/dir b/board/common/rootfs/usr/bin/dir index 4d8fc7b7..d28eb47e 100755 --- a/board/common/rootfs/usr/bin/dir +++ b/board/common/rootfs/usr/bin/dir @@ -21,7 +21,11 @@ dir() if [ -d "$1" ]; then dir "$1" else - dir "$HOME" + if [ "$USER" = "root" ]; then + dir "$HOME" + else + dir "/home/$USER" + fi dir "/cfg" dir "/log" fi From 4998e320c510ff326e239a9e92af1d30f83c3108 Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Sun, 9 Mar 2025 16:50:27 +0100 Subject: [PATCH 05/11] doc: update ChangeLog with copy command fix Signed-off-by: Joachim Wiberg --- doc/ChangeLog.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/ChangeLog.md b/doc/ChangeLog.md index 0292b3ee..374f74a8 100644 --- a/doc/ChangeLog.md +++ b/doc/ChangeLog.md @@ -31,6 +31,8 @@ All notable changes to the project are documented in this file. - Fix #956: CLI `copy` command complains it cannot change owner when copying `factory-config` to `running-config`. Bogus error, the latter is not really a file + - Fix #977: "Operation not permitted" when saving `running-config` to + `startup-config` (harmless warning but annoying and concerning) [EVK]: https://www.nxp.com/design/design-center/development-boards-and-designs/8MPLUSLPD4-EVK From 03437fc93657717edcdb30083d8bed60c0a97b29 Mon Sep 17 00:00:00 2001 From: Richard Alpe Date: Mon, 10 Mar 2025 14:52:22 +0100 Subject: [PATCH 06/11] bin: copy: use NGROUPS_MAX when probing user groups 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 --- src/bin/copy.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/bin/copy.c b/src/bin/copy.c index 680d0a70..6e54b079 100644 --- a/src/bin/copy.c +++ b/src/bin/copy.c @@ -114,8 +114,7 @@ static gid_t in_group(const char *user, const char *fn, gid_t *gid) if (stat(dir, &st)) return 0; - getgrouplist(user, pw->pw_gid, NULL, &num); - + num = NGROUPS_MAX; groups = malloc(num * sizeof(gid_t)); if (!groups) { perror("in_group() malloc"); From 5d99c1208905e1c1b42f2491483a37ae99345f4f Mon Sep 17 00:00:00 2001 From: Richard Alpe Date: Mon, 10 Mar 2025 16:16:52 +0100 Subject: [PATCH 07/11] bin: copy: use dirname() to derive parent directory Cosmetic change intended to improve code readability. Signed-off-by: Richard Alpe --- src/bin/copy.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/bin/copy.c b/src/bin/copy.c index 6e54b079..fa20b99b 100644 --- a/src/bin/copy.c +++ b/src/bin/copy.c @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -85,7 +86,7 @@ static char *getuser(void) /* * If UNIX user is in UNIX group of directory containing file, - * return gid, otherwise -1 + * return 1, otherwise 0. * * E.g., writing to /cfg/foo, where /cfg is owned by root:wheel, * should result in the file being owned by $LOGNAME:wheel with @@ -93,23 +94,19 @@ static char *getuser(void) */ static gid_t in_group(const char *user, const char *fn, gid_t *gid) { - char dir[strlen(fn) + 2]; + char path[PATH_MAX]; const struct passwd *pw; int i, num = 0, rc = 0; struct stat st; gid_t *groups; - char *ptr; + char *dir; pw = getpwnam(user); if (!pw) return 0; - strlcpy(dir, fn, sizeof(dir)); - ptr = strrchr(dir, '/'); - if (ptr) - *ptr = 0; - else - strlcpy(dir, ".", sizeof(dir)); + strlcpy(path, fn, sizeof(path)); + dir = dirname(path); if (stat(dir, &st)) return 0; From dd788f5611b1cac232673c0a2655edb0368e5f9a Mon Sep 17 00:00:00 2001 From: Richard Alpe Date: Mon, 10 Mar 2025 16:18:20 +0100 Subject: [PATCH 08/11] bin: copy: save errno in chown error path 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 --- src/bin/copy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bin/copy.c b/src/bin/copy.c index fa20b99b..99e2e804 100644 --- a/src/bin/copy.c +++ b/src/bin/copy.c @@ -155,10 +155,11 @@ static void set_owner(const char *fn, const char *user) return; /* user not in parent directory's group */ if (chown(fn, -1, gid) && errno != EPERM) { + int _errno = errno; const struct group *gr = getgrgid(gid); fprintf(stderr, ERRMSG "setting group owner %s (%d) on %s: %s\n", - gr ? gr->gr_name : "", gid, fn, strerror(errno)); + gr ? gr->gr_name : "", gid, fn, strerror(_errno)); } } From 95931c8c0a8a55c1d47c29fccba504fbe409f7fe Mon Sep 17 00:00:00 2001 From: Richard Alpe Date: Tue, 11 Mar 2025 09:30:19 +0100 Subject: [PATCH 09/11] bin: copy: add newline to same-same error message Signed-off-by: Richard Alpe --- src/bin/copy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/copy.c b/src/bin/copy.c index 99e2e804..80235718 100644 --- a/src/bin/copy.c +++ b/src/bin/copy.c @@ -202,7 +202,7 @@ static int copy(const char *src, const char *dst, const char *remote_user) goto err; if (!strcmp(src, dst)) { - fprintf(stderr, ERRMSG "source and destination are the same, aborting."); + fprintf(stderr, ERRMSG "source and destination are the same, aborting.\n"); goto err; } From b8fc8b7f6207ea02c7ae2afb642e9cbef4426f8b Mon Sep 17 00:00:00 2001 From: Richard Alpe Date: Tue, 11 Mar 2025 09:30:46 +0100 Subject: [PATCH 10/11] bin: copy: fix segfault for one argument 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 --- src/bin/copy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/copy.c b/src/bin/copy.c index 80235718..12e49e60 100644 --- a/src/bin/copy.c +++ b/src/bin/copy.c @@ -418,7 +418,7 @@ int main(int argc, char *argv[]) if (timeout < 0) timeout = 120; - if (optind >= argc) + if (argc - optind != 2) return usage(1); src = argv[optind++]; From 095c9c331e4c048b5ebe375b0265c613b9b3c379 Mon Sep 17 00:00:00 2001 From: Richard Alpe Date: Tue, 11 Mar 2025 13:31:55 +0100 Subject: [PATCH 11/11] bin: copy: fix return type of in_group() Signed-off-by: Richard Alpe --- src/bin/copy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/copy.c b/src/bin/copy.c index 12e49e60..f782f1e7 100644 --- a/src/bin/copy.c +++ b/src/bin/copy.c @@ -92,7 +92,7 @@ static char *getuser(void) * should result in the file being owned by $LOGNAME:wheel with * 0660 perms for other users in same group. */ -static gid_t in_group(const char *user, const char *fn, gid_t *gid) +static int in_group(const char *user, const char *fn, gid_t *gid) { char path[PATH_MAX]; const struct passwd *pw;