[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid>
From: |
Ian Jackson |
Subject: |
Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility |
Date: |
Mon, 16 Apr 2018 15:06:51 +0100 |
Thanks for the review. Taking your comments out of order slightly:
Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new
-runas <uid>:<gid> facility"):
> [change_process_uid] is the only user of @user_pwd, @user_uid, @user_gid.
>
> Have you considered replacing global @user_pwd by @user_uid, @user_gid
> and @user_name? --runas with numeric uid and gid would leave @user_name
> null.
That would defer the getpwnam from argument parsing to os_setup_post.
I think that's undesriable.
> Ian Jackson <address@hidden> writes:
> > static struct passwd *user_pwd;
> > +static uid_t user_uid = (uid_t)-1;
> > +static gid_t user_gid = (gid_t)-1;
>
> As we'll see below, @user_pwd->pw_uid, @user_pwd_pw_gid take precedence
> over @user_uid, @user_gid. Awkward.
My patch has the right behaviour: each -runas completely overrides the
previous one. -runas that sets user_{uid,gid} always clears user_pwd
on the way. So user_pwd can only be set if the most recent -runas was
a name, and then we should honour the name.
This is rather obscure. I think you are right that this is confusing.
It ought to be clearer.
I will
- add a comment next to these three variables saying they must
all be set at the same time
- explicitly (redundantly) clear user_pwd in os_parse_runas_uid_gid
- explicitly set user_{uid,gid} to -1 when -runas gets a
success from getpwnam
- assert in change_process_uid that the combination is legal
> > + errno = 0;
> > + rc = qemu_strtoul(optarg, &ep, 0, &lv);
...
> > + lv = 0;
>
> Either zero lv before both qemu_strtoul() or neither one.
This is a hangover from the previous version which used raw strtoul.
The assignments to both lv and errno are redundant.
> > - if (!user_pwd) {
> > - fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
> > + if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
> > + error_report("User doesn't exist (and is not <uid>:<gid>)");
>
> The error message no longer includes the offending value. Intentional?
I was in two minds. I will put it back.
> > + if (user_pwd || user_uid != (uid_t)-1) {
> > + gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
> > + uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
> > + if (setgid(intended_gid) < 0) {
> > + fprintf(stderr, "Failed to setgid(%d)\n", intended_gid);
>
> error_report(), please. More of the same below.
I was following the existing code in this function. I'll add a
pre-patch to fix this up here, and maybe a post-patch to fix the rest
of this file.
Thanks,
Ian.