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

[Andrea Bolognani]

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?

> The typed parameters exposed in the API are the same as those currently
> supported by the internal virIdentity class.

... although with slightly different names...

> +++ 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?

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.

> +/**
> + * 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