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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
