[libvirt] [PATCH v3 43/48] api: introduce virConnectSetIdentity for pasing uid, gid, selinux info

[Daniel P. Berrangé]

On Tue, Jul 30, 2019 at 08:32:00PM +0200, Andrea Bolognani wrote:
> On Mon, 2019-07-29 at 18:11 +0100, Daniel P. Berrangé wrote:
> > api: introduce virConnectSetIdentity for pasing uid, gid, selinux info
> 
> s/pasing/passing/
> 
> > When using the fine grained access control mechanism for APIs, when a
> > client connects to libvirtd, it will fetch the uid, gid, selinux
> > info of the remote client on the UNIX domain socket. This is then used
> > as the identity when checking ACLs.
> 
> s/it will/the latter will/
> 
> > With the new split daemons things are a bit more complicated. The user
> > can connect to virtproxyd, which in turn connects to virtqemud. When
> > virtqemud requests the identity over the UNIX domain socket, it will
> > get the identity that the virtproxyd is running as, not the identity of
> > the real end user/application.
> 
> s/the virtproxyd/virtproxyd/
> 
> > virproxyd knows what the real identity is, and needs to be able to
> > forward this information to virtqemud. The virConnectSetIdentity API
> > provides a mechanism for doing this. Obviously virtqemud should not
> > accept such identity overrides from any client, it must only honour it
> > from a trusted client, aka one running as the same uid/gid as itself.
> 
> I've been trying to figure out where the very reasonable check you
> describe is performed, either here or later in the series, but I have
> to admit that I haven't been able to find it. Can you please point me
> in the right direction?

In the absence of any policy rules, polkit will only grant access if
the remote peer is running as root.

So if a non-root virtproxyd tried to request identity override to
virtqemud as root, then polkit will reject it.

IOW, we don't need any code in libvirt to protect for this - it just
"does the right thing(tm)"

> > The typed parameters exposed in the API are the same as those currently
> > supported by the internal virIdentity class.
> 
> ... although with slightly different names...

Yes, the internal APIs uses "UNIX", but I didn't consider the attrs to
really be UNIX specific - a username is a portable concept, and so is
process ID.

Windows doesn't have a user ID concept - just a "SID" which
is the string format and so maps to the user name, but we allow any
fields to be optional so user ID can be ignored if we need windows
portability later.

> > +++ b/include/libvirt/libvirt-host.h
> > +/**
> > + * VIR_CONNECT_IDENTITY_OS_USER_NAME:
> > + *
> > + * The operating system user name as VIR_TYPED_PARAM_STRING
> > + */
> > +# define VIR_CONNECT_IDENTITY_OS_USER_NAME "os-user-name"
> 
> The documentation should end with a period. Same for all following
> values.
> 
> > +/**
> > + * VIR_CONNECT_IDENTITY_OS_PROCESS_ID:
> > + *
> > + * The operating system user ID as VIR_TYPED_PARAM_LLONG
> > + */
> > +# define VIR_CONNECT_IDENTITY_OS_PROCESS_ID "os-process-id"
> 
> Welp, looks like you've copy-pasted the same documentation over
> and over again! Please fix that :)
> 
> Anyway, shouldn't this be VIR_TYPED_PARAM_ULLONG as well? Can pids
> be negative?

POSIX says that the pid_t data type is a signed int. It doesn't
specify the size, but ULLONG is the largest we can do so it is
the best fit.

> Looking at the code that you're replacing with patch 46, it uses
> virStrToLong_i() to parse uid and gid and virStrToLong_ull() to
> parse pid, so if anything the VIR_TYPED_PARAM_* should be the other
> way around? But it seems to me like we really want all of these to
> be VIR_TYPED_PARAM_ULLONGs.

POSIX does not explicitly state signed-ness of uid_t/gid_t, but
the docs do require that you explicitly cast any negative values
ie  gid_t foo = (gid_t)-1;

which implies that gid_t is liable to be an unsigned type. Thus
picking ULLONG is best for gid/uid.

> > +/**
> > + * VIR_CONNECT_IDENTITY_SELINUX_CONTEXT:
> > + *
> > + * The application's SELinux context as VIR_TYPED_PARAM_STRING
> > + *
> > + */
> > +# define VIR_CONNECT_IDENTITY_SELINUX_CONTEXT "selinux-context"
> 
> Spurious empty line in the documentation.
> 
> > +++ b/src/libvirt_public.syms
> >  LIBVIRT_5.6.0 {
> >      global:
> > +        virConnectSetIdentity;
> >          virDomainCheckpointCreateXML;
> >          virDomainCheckpointDelete;
> >          virDomainCheckpointFree;
> 
> Yeah, about that...
> 
> 
> Overall the patch looks good.
> 
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|