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

Daniel P. Berrangé posted 48 patches 6 years, 6 months ago
There is a newer version of this series
[libvirt] [PATCH v3 43/48] api: introduce virConnectSetIdentity for pasing uid, gid, selinux info
Posted by Daniel P. Berrangé 6 years, 6 months ago
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.

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.

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.

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

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/libvirt/libvirt-host.h | 75 ++++++++++++++++++++++++++++++++++
 src/driver-hypervisor.h        |  7 ++++
 src/libvirt-host.c             | 51 +++++++++++++++++++++++
 src/libvirt_public.syms        |  1 +
 4 files changed, 134 insertions(+)

diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h
index 7debb5f829..8ea3531750 100644
--- a/include/libvirt/libvirt-host.h
+++ b/include/libvirt/libvirt-host.h
@@ -579,6 +579,81 @@ virConnectPtr           virConnectOpenAuth      (const char *name,
                                                  unsigned int flags);
 int                     virConnectRef           (virConnectPtr conn);
 int                     virConnectClose         (virConnectPtr conn);
+
+/**
+ * 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"
+
+/**
+ * VIR_CONNECT_IDENTITY_OS_USER_ID:
+ *
+ * The operating system user ID as VIR_TYPED_PARAM_ULLONG
+ */
+# define VIR_CONNECT_IDENTITY_OS_USER_ID "os-user-id"
+
+/**
+ * VIR_CONNECT_IDENTITY_OS_GROUP_NAME:
+ *
+ * The operating system user ID as VIR_TYPED_PARAM_STRING
+ */
+# define VIR_CONNECT_IDENTITY_OS_GROUP_NAME "os-group-name"
+
+/**
+ * VIR_CONNECT_IDENTITY_OS_GROUP_ID:
+ *
+ * The operating system user ID as VIR_TYPED_PARAM_ULLONG
+ */
+# define VIR_CONNECT_IDENTITY_OS_GROUP_ID "os-group-id"
+
+/**
+ * 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"
+
+/**
+ * VIR_CONNECT_IDENTITY_OS_PROCESS_TIME:
+ *
+ * The operating system process start time as VIR_TYPED_PARAM_ULLONG
+ *
+ * The units the time is measured in vary according to the
+ * host operating system. On Linux this is usually clock
+ * ticks (as reported in /proc/$PID/stat field 22).
+ */
+# define VIR_CONNECT_IDENTITY_OS_PROCESS_TIME "os-process-time"
+
+/**
+ * VIR_CONNECT_IDENTITY_SASL_USER_NAME:
+ *
+ * The SASL authenticated username as VIR_TYPED_PARAM_STRING
+ */
+# define VIR_CONNECT_IDENTITY_SASL_USER_NAME "sasl-user-name"
+
+/**
+ * VIR_CONNECT_IDENTITY_X509_DISTINGUISHED_NAME:
+ *
+ * The TLS x509 certificate distinguished named as VIR_TYPED_PARAM_STRING
+ */
+# define VIR_CONNECT_IDENTITY_X509_DISTINGUISHED_NAME "x509-distinguished-name"
+
+/**
+ * VIR_CONNECT_IDENTITY_SELINUX_CONTEXT:
+ *
+ * The application's SELinux context as VIR_TYPED_PARAM_STRING
+ *
+ */
+# define VIR_CONNECT_IDENTITY_SELINUX_CONTEXT "selinux-context"
+
+
+int                     virConnectSetIdentity   (virConnectPtr conn,
+                                                 virTypedParameterPtr params,
+                                                 int nparams,
+                                                 unsigned int flags);
+
 const char *            virConnectGetType       (virConnectPtr conn);
 int                     virConnectGetVersion    (virConnectPtr conn,
                                                  unsigned long *hvVer);
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index c1632ae4c6..f1bc355b65 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -36,6 +36,12 @@ typedef virDrvOpenStatus
 typedef int
 (*virDrvConnectClose)(virConnectPtr conn);
 
+typedef int
+(*virDrvConnectSetIdentity)(virConnectPtr conn,
+                            virTypedParameterPtr params,
+                            int nparams,
+                            unsigned int flags);
+
 typedef int
 (*virDrvConnectSupportsFeature)(virConnectPtr conn,
                                 int feature);
@@ -1378,6 +1384,7 @@ struct _virHypervisorDriver {
     virDrvConnectURIProbe connectURIProbe;
     virDrvConnectOpen connectOpen;
     virDrvConnectClose connectClose;
+    virDrvConnectSetIdentity connectSetIdentity;
     virDrvConnectSupportsFeature connectSupportsFeature;
     virDrvConnectGetType connectGetType;
     virDrvConnectGetVersion connectGetVersion;
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index e5c4e5f72a..d7b1b82277 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -61,6 +61,57 @@ virConnectRef(virConnectPtr conn)
 }
 
 
+/**
+ * virConnectSetIdentity:
+ * @conn: pointer to the hypervisor connection
+ * @params: parameters containing the identity attributes
+ * @nparams: size of @params array
+ * @flags: currently unused, pass 0
+ *
+ * Override the default identity information associated with
+ * the connection. When connecting to a stateful driver over
+ * a UNIX socket, the daemon will interrogate the remote end
+ * of the UNIX socket to acquire the application's identity.
+ * This identity is used for the fine grained access control
+ * checks on API calls.
+ *
+ * There may be times when application is operating on behalf
+ * of a variety of users, and thus the identity that the
+ * application runs as is not appropriate for access control
+ * checks. In this case, if the application is considered
+ * trustworthy, it can supply alternative identity information.
+ *
+ * The driver may reject the request to change the identity
+ * on a connection if the application is not trustworthy.
+ *
+ * Returns: 0 if the identity change was accepted, -1 on error
+ */
+int
+virConnectSetIdentity(virConnectPtr conn,
+                      virTypedParameterPtr params,
+                      int nparams,
+                      unsigned int flags)
+{
+    VIR_DEBUG("conn=%p params=%p nparams=%d flags=0x%x", conn, params, nparams, flags);
+    VIR_TYPED_PARAMS_DEBUG(params, nparams);
+
+    virResetLastError();
+
+    if (conn->driver->connectSetIdentity) {
+        int ret = conn->driver->connectSetIdentity(conn, params, nparams, flags);
+        if (ret < 0)
+            goto error;
+        return ret;
+    }
+
+    virReportUnsupportedError();
+
+ error:
+    virDispatchError(conn);
+    return -1;
+}
+
+
 /*
  * Not for public use.  This function is part of the internal
  * implementation of driver features in the remote case.
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 54256b6317..580b14b6a0 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -838,6 +838,7 @@ LIBVIRT_5.5.0 {
 
 LIBVIRT_5.6.0 {
     global:
+        virConnectSetIdentity;
         virDomainCheckpointCreateXML;
         virDomainCheckpointDelete;
         virDomainCheckpointFree;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 43/48] api: introduce virConnectSetIdentity for pasing uid, gid, selinux info
Posted by Andrea Bolognani 6 years, 6 months ago
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 43/48] api: introduce virConnectSetIdentity for pasing uid, gid, selinux info
Posted by Daniel P. Berrangé 6 years, 5 months ago
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list