Hi Reyk,
On 19 Jun at 20:38, Reyk Floeter <r...@openbsd.org> wrote:
> Hi,
>
> On Sun, Jun 17, 2018 at 10:35:27PM +0200, obs...@high5.nl wrote:
> > >Synopsis: VMM owner needs to be part of group wheel in order to run vmctl
> > >console|start|stop
>
> the solution is not that easy as it seemed.
>
> 1. Change the umask and let everyone access vmd, restrict the commands
> internally.
>
> While this is a possible solution, I also agree that this allows any
> user (including system/privsep users) to trigger actions and imsgs in
> vmd; even if the result is permission denied as this is checked fairly
> late.
>
> 2. Change the default owner group to root:_vmd.
>
> It would be possible to define a hardcoded group, or use group _vmd,
> but this doesn't feel right.
Why would using _vmd not work?
Wouldn't that be the same for other daemons?
Or wouldn't these be used to assign other users to it?
One which comes to mind is _ladvd, albeit not in base.
> 3. Let the user configure the owner of the control socket.
>
> This allows you to configure your own group, like "devops" (hrhr), or
> fetch the group from YP/LDA, and let them mess with your VMs. I think
> this is much more viable in a multi-user environment. Add the
> following to the top/global section of /etc/vm.conf: "socket owner :devops"
Works for me. And when vm.conf doesn't exist, or the owner would exist it would
fall back to root?
Will check out the patch asap!
Mischa
> privsep/pledge also makes it a bit more complicated for us because I
> don't want to allow the control process to chown the unix socket; so
> it is done by the parent and some messaging.
>
> The attached diff implements 3. ... comments? OK? Should we use 2. instead?
>
> Reyk
>
> Index: usr.sbin/vmd/control.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/control.c,v
> retrieving revision 1.23
> diff -u -p -u -p -r1.23 control.c
> --- usr.sbin/vmd/control.c 13 May 2018 22:48:11 -0000 1.23
> +++ usr.sbin/vmd/control.c 19 Jun 2018 18:27:55 -0000
> @@ -103,6 +103,7 @@ control_dispatch_vmd(int fd, struct priv
> break;
> case IMSG_VMDOP_CONFIG:
> config_getconfig(ps->ps_env, imsg);
> + proc_compose(ps, PROC_PARENT, IMSG_VMDOP_DONE, NULL, 0);
> break;
> case IMSG_CTL_RESET:
> config_getreset(ps->ps_env, imsg);
> @@ -169,6 +170,18 @@ control_init(struct privsep *ps, struct
>
> cs->cs_fd = fd;
> cs->cs_env = ps;
> +
> + proc_compose(ps, PROC_PARENT, IMSG_VMDOP_DONE, NULL, 0);
> +
> + return (0);
> +}
> +
> +int
> +control_reset(struct control_sock *cs)
> +{
> + /* Updating owner of the control socket */
> + if (chown(cs->cs_name, cs->cs_uid, cs->cs_gid) == -1)
> + return (-1);
>
> return (0);
> }
> Index: usr.sbin/vmd/parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/parse.y,v
> retrieving revision 1.35
> diff -u -p -u -p -r1.35 parse.y
> --- usr.sbin/vmd/parse.y 19 Jun 2018 17:12:34 -0000 1.35
> +++ usr.sbin/vmd/parse.y 19 Jun 2018 18:27:55 -0000
> @@ -119,7 +119,8 @@ typedef struct {
>
> %token INCLUDE ERROR
> %token ADD BOOT CDROM DISABLE DISK DOWN ENABLE GROUP INTERFACE LLADDR
> LOCAL
> -%token LOCKED MEMORY NIFS OWNER PATH PREFIX RDOMAIN SIZE SWITCH UP VM
> VMID
> +%token LOCKED MEMORY NIFS OWNER PATH PREFIX RDOMAIN SIZE SOCKET SWITCH
> UP
> +%token VM VMID
> %token <v.number> NUMBER
> %token <v.string> STRING
> %type <v.lladdr> lladdr
> @@ -190,6 +191,10 @@ main : LOCAL PREFIX STRING {
>
> memcpy(&env->vmd_cfg.cfg_localprefix, &h, sizeof(h));
> }
> + | SOCKET OWNER owner_id {
> + env->vmd_ps.ps_csock.cs_uid = $3.uid;
> + env->vmd_ps.ps_csock.cs_gid = $3.gid == -1 ? 0 : $3.gid;
> + }
> ;
>
> switch : SWITCH string {
> @@ -678,6 +683,7 @@ lookup(char *s)
> { "prefix", PREFIX },
> { "rdomain", RDOMAIN },
> { "size", SIZE },
> + { "socket", SOCKET },
> { "switch", SWITCH },
> { "up", UP },
> { "vm", VM }
> Index: usr.sbin/vmd/proc.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/proc.h,v
> retrieving revision 1.12
> diff -u -p -u -p -r1.12 proc.h
> --- usr.sbin/vmd/proc.h 27 Mar 2017 00:28:04 -0000 1.12
> +++ usr.sbin/vmd/proc.h 19 Jun 2018 18:27:55 -0000
> @@ -62,6 +62,8 @@ struct control_sock {
> int cs_fd;
> int cs_restricted;
> void *cs_env;
> + uid_t cs_uid;
> + gid_t cs_gid;
>
> TAILQ_ENTRY(control_sock) cs_entry;
> };
> @@ -192,6 +194,7 @@ int proc_flush_imsg(struct privsep *, e
> /* control.c */
> void control(struct privsep *, struct privsep_proc *);
> int control_init(struct privsep *, struct control_sock *);
> +int control_reset(struct control_sock *);
> int control_listen(struct control_sock *);
> void control_cleanup(struct control_sock *);
>
> Index: usr.sbin/vmd/vm.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vm.conf.5,v
> retrieving revision 1.29
> diff -u -p -u -p -r1.29 vm.conf.5
> --- usr.sbin/vmd/vm.conf.5 18 Jun 2018 06:04:25 -0000 1.29
> +++ usr.sbin/vmd/vm.conf.5 19 Jun 2018 18:27:55 -0000
> @@ -100,6 +100,16 @@ in the
> section below.
> The default is
> .Ar 100.64.0.0/10 .
> +.It Cm socket owner Ar user Ns Op : Ns Ar group
> +Set the control socket owner to the specified user or group.
> +Users with access to the control socket will be allowed to use
> +.Nm vmctl
> +for restricted access to
> +.Nm vmd.
> +The default is
> +.Ar root:wheel .
> +.It Cm socket owner Pf : Ar group
> +Set the control socket owner to the specified group.
> .El
> .Sh VM CONFIGURATION
> Each
> Index: usr.sbin/vmd/vmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
> retrieving revision 1.86
> diff -u -p -u -p -r1.86 vmd.c
> --- usr.sbin/vmd/vmd.c 19 Jun 2018 17:12:34 -0000 1.86
> +++ usr.sbin/vmd/vmd.c 19 Jun 2018 18:27:55 -0000
> @@ -85,6 +85,7 @@ vmd_dispatch_control(int fd, struct priv
> struct vmd_vm *vm = NULL;
> char *str = NULL;
> uint32_t id = 0;
> + struct control_sock *rcs;
>
> switch (imsg->hdr.type) {
> case IMSG_VMDOP_START_VM_REQUEST:
> @@ -274,6 +275,12 @@ vmd_dispatch_control(int fd, struct priv
> IMSG_VMDOP_RECEIVE_VM_END, vm->vm_vmid, imsg->fd,
> NULL, 0);
> }
> + break;
> + case IMSG_VMDOP_DONE:
> + control_reset(&ps->ps_csock);
> + TAILQ_FOREACH(rcs, &ps->ps_rcsocks, cs_entry)
> + control_reset(rcs);
> + cmd = 0;
> break;
> default:
> return (-1);
> Index: usr.sbin/vmd/vmd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmd/vmd.h,v
> retrieving revision 1.68
> diff -u -p -u -p -r1.68 vmd.h
> --- usr.sbin/vmd/vmd.h 27 Apr 2018 12:15:10 -0000 1.68
> +++ usr.sbin/vmd/vmd.h 19 Jun 2018 18:27:55 -0000
> @@ -105,7 +105,8 @@ enum imsg_type {
> IMSG_VMDOP_PRIV_IFRDOMAIN,
> IMSG_VMDOP_VM_SHUTDOWN,
> IMSG_VMDOP_VM_REBOOT,
> - IMSG_VMDOP_CONFIG
> + IMSG_VMDOP_CONFIG,
> + IMSG_VMDOP_DONE
> };
>
> struct vmop_result {
