When opening a connection to a second driver inside the daemon, we must
ensure the identity of the current user is passed across. This allows
the second daemon to perform access control checks against the real end
users, instead of against the libvirt daemon that's proxying across the
API calls.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon_dispatch.c | 110 +++++++++++++++++++++++++---
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 18 ++++-
src/remote_protocol-structs | 8 ++
src/rpc/virnetserverclient.c | 12 +++
src/rpc/virnetserverclient.h | 2 +
7 files changed, 140 insertions(+), 12 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 3307d74324..0493467f46 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -178,6 +178,7 @@ virNetServerClientSetAuthLocked;
virNetServerClientSetAuthPendingLocked;
virNetServerClientSetCloseHook;
virNetServerClientSetDispatcher;
+virNetServerClientSetIdentity;
virNetServerClientSetQuietEOF;
virNetServerClientSetReadonly;
virNetServerClientStartKeepAlive;
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index 9ef76daa55..f828b75f3b 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -51,6 +51,7 @@
#include "virpolkit.h"
#include "virthreadjob.h"
#include "configmake.h"
+#include "access/viraccessapicheck.h"
#define VIR_FROM_THIS VIR_FROM_RPC
@@ -1945,10 +1946,15 @@ static void remoteClientCloseFunc(virNetServerClientPtr client)
static int
remoteOpenConn(const char *uri,
bool readonly,
+ bool preserveIdentity,
virConnectPtr *conn)
{
- VIR_DEBUG("Getting secondary uri=%s readonly=%d conn=%p",
- NULLSTR(uri), readonly, conn);
+ virTypedParameterPtr params = NULL;
+ int nparams = 0;
+
+ VIR_DEBUG("Getting secondary uri=%s readonly=%d preserveIdent=%d conn=%p",
+ NULLSTR(uri), readonly, preserveIdentity, conn);
+
if (*conn)
return 0;
@@ -1957,15 +1963,42 @@ remoteOpenConn(const char *uri,
return -1;
}
+ if (preserveIdentity) {
+ VIR_AUTOUNREF(virIdentityPtr) ident = NULL;
+
+ if (!(ident = virIdentityGetCurrent()))
+ return -1;
+
+ if (virIdentityGetParameters(ident, ¶ms, &nparams) < 0)
+ goto error;
+ }
+
VIR_DEBUG("Opening driver %s", uri);
if (readonly)
*conn = virConnectOpenReadOnly(uri);
else
*conn = virConnectOpen(uri);
if (!*conn)
- return -1;
+ goto error;
VIR_DEBUG("Opened driver %p", *conn);
+
+ if (preserveIdentity) {
+ if (virConnectSetIdentity(*conn, params, nparams, 0) < 0)
+ goto error;
+
+ virTypedParamsFree(params, nparams);
+ VIR_DEBUG("Forwarded current identity to secondary driver");
+ }
+
return 0;
+
+ error:
+ virTypedParamsFree(params, nparams);
+ if (*conn) {
+ virConnectClose(*conn);
+ *conn = NULL;
+ }
+ return -1;
}
@@ -1992,6 +2025,7 @@ remoteGetInterfaceConn(virNetServerClientPtr client)
if (remoteOpenConn(priv->interfaceURI,
priv->readonly,
+ true,
&priv->interfaceConn) < 0)
return NULL;
@@ -2007,6 +2041,7 @@ remoteGetNetworkConn(virNetServerClientPtr client)
if (remoteOpenConn(priv->networkURI,
priv->readonly,
+ true,
&priv->networkConn) < 0)
return NULL;
@@ -2022,6 +2057,7 @@ remoteGetNodeDevConn(virNetServerClientPtr client)
if (remoteOpenConn(priv->nodedevURI,
priv->readonly,
+ true,
&priv->nodedevConn) < 0)
return NULL;
@@ -2037,6 +2073,7 @@ remoteGetNWFilterConn(virNetServerClientPtr client)
if (remoteOpenConn(priv->nwfilterURI,
priv->readonly,
+ true,
&priv->nwfilterConn) < 0)
return NULL;
@@ -2052,6 +2089,7 @@ remoteGetSecretConn(virNetServerClientPtr client)
if (remoteOpenConn(priv->secretURI,
priv->readonly,
+ true,
&priv->secretConn) < 0)
return NULL;
@@ -2067,6 +2105,7 @@ remoteGetStorageConn(virNetServerClientPtr client)
if (remoteOpenConn(priv->storageURI,
priv->readonly,
+ true,
&priv->storageConn) < 0)
return NULL;
@@ -2235,6 +2274,7 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
#ifndef LIBVIRTD
const char *type = NULL;
#endif
+ bool preserveIdentity = false;
VIR_DEBUG("priv=%p conn=%p", priv, priv->conn);
virMutexLock(&priv->lock);
@@ -2264,14 +2304,16 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
}
#endif
+#ifdef VIRTPROXYD
+ preserveIdentity = true;
+#endif /* VIRTPROXYD */
+
VIR_DEBUG("Opening driver %s", name);
- if (priv->readonly) {
- if (!(priv->conn = virConnectOpenReadOnly(name)))
- goto cleanup;
- } else {
- if (!(priv->conn = virConnectOpen(name)))
- goto cleanup;
- }
+ if (remoteOpenConn(name,
+ priv->readonly,
+ preserveIdentity,
+ &priv->conn) < 0)
+ goto cleanup;
VIR_DEBUG("Opened %p", priv->conn);
#ifndef LIBVIRTD
@@ -2375,6 +2417,54 @@ remoteDispatchConnectClose(virNetServerPtr server ATTRIBUTE_UNUSED,
}
+static int
+remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
+ remote_connect_set_identity_args *args)
+{
+ virTypedParameterPtr params = NULL;
+ int nparams = 0;
+ int rv = -1;
+ virConnectPtr conn = remoteGetHypervisorConn(client);
+ virIdentityPtr ident = NULL;
+ if (!conn)
+ goto cleanup;
+
+ VIR_DEBUG("Received forwarded identity");
+ if (virTypedParamsDeserialize((virTypedParameterRemotePtr) args->params.params_val,
+ args->params.params_len,
+ REMOTE_CONNECT_IDENTITY_PARAMS_MAX,
+ ¶ms,
+ &nparams) < 0)
+ goto cleanup;
+
+ VIR_TYPED_PARAMS_DEBUG(params, nparams);
+
+ if (virConnectSetIdentityEnsureACL(conn) < 0)
+ goto cleanup;
+
+ if (!(ident = virIdentityNew()))
+ goto cleanup;
+
+ if (virIdentitySetParameters(ident, params, nparams) < 0)
+ goto cleanup;
+
+ virNetServerClientSetIdentity(client, ident);
+
+ rv = 0;
+
+ cleanup:
+ virTypedParamsFree(params, nparams);
+ virObjectUnref(ident);
+ if (rv < 0)
+ virNetMessageSaveError(rerr);
+ return rv;
+}
+
+
+
static int
remoteDispatchDomainGetSchedulerType(virNetServerPtr server ATTRIBUTE_UNUSED,
virNetServerClientPtr client,
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 889d62ba4f..e2d2dc66be 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8510,6 +8510,7 @@ static virHypervisorDriver hypervisor_driver = {
.name = "remote",
.connectOpen = remoteConnectOpen, /* 0.3.0 */
.connectClose = remoteConnectClose, /* 0.3.0 */
+ .connectSetIdentity = remoteConnectSetIdentity, /* 5.6.0 */
.connectSupportsFeature = remoteConnectSupportsFeature, /* 0.3.0 */
.connectGetType = remoteConnectGetType, /* 0.3.0 */
.connectGetVersion = remoteConnectGetVersion, /* 0.3.0 */
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 2f91bd1921..42e61ba20f 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -53,6 +53,9 @@ typedef string remote_nonnull_string<REMOTE_STRING_MAX>;
/* A long string, which may be NULL. */
typedef remote_nonnull_string *remote_string;
+/* Upper limit on identity parameters */
+const REMOTE_CONNECT_IDENTITY_PARAMS_MAX = 20;
+
/* Upper limit on lists of domains. */
const REMOTE_DOMAIN_LIST_MAX = 16384;
@@ -3723,6 +3726,11 @@ struct remote_domain_checkpoint_delete_args {
unsigned int flags;
};
+struct remote_connect_set_identity_args {
+ remote_typed_param params<REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX>;
+ unsigned int flags;
+};
+
/*----- Protocol. -----*/
/* Define the program number, protocol version and procedure numbers here. */
@@ -6538,7 +6546,7 @@ enum remote_procedure {
*/
REMOTE_PROC_NETWORK_PORT_DELETE = 410,
- /**
+ /**
* @generate: both
* @acl: domain:checkpoint
* @acl: domain:fs_freeze:VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE
@@ -6584,5 +6592,11 @@ enum remote_procedure {
* @generate: both
* @acl: domain:checkpoint
*/
- REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417
+ REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417,
+
+ /**
+ * @generate: client
+ * @acl: connect:write
+ */
+ REMOTE_PROC_CONNECT_SET_IDENTITY = 418
};
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index a42b4a9671..05229f00c5 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -3105,6 +3105,13 @@ struct remote_domain_checkpoint_delete_args {
remote_nonnull_domain_checkpoint checkpoint;
u_int flags;
};
+struct remote_connect_set_identity_args {
+ struct {
+ u_int params_len;
+ remote_typed_param * params_val;
+ } params;
+ u_int flags;
+};
enum remote_procedure {
REMOTE_PROC_CONNECT_OPEN = 1,
REMOTE_PROC_CONNECT_CLOSE = 2,
@@ -3523,4 +3530,5 @@ enum remote_procedure {
REMOTE_PROC_DOMAIN_CHECKPOINT_LOOKUP_BY_NAME = 415,
REMOTE_PROC_DOMAIN_CHECKPOINT_GET_PARENT = 416,
REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417,
+ REMOTE_PROC_CONNECT_SET_IDENTITY = 418,
};
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 1f020a5a04..2a278171f5 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -844,6 +844,18 @@ virIdentityPtr virNetServerClientGetIdentity(virNetServerClientPtr client)
}
+void virNetServerClientSetIdentity(virNetServerClientPtr client,
+ virIdentityPtr identity)
+{
+ virObjectLock(client);
+ virObjectUnref(client->identity);
+ client->identity = identity;
+ if (client->identity)
+ virObjectRef(client->identity);
+ virObjectUnlock(client);
+}
+
+
int virNetServerClientGetSELinuxContext(virNetServerClientPtr client,
char **context)
{
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 1b01bedbcb..1c520fef6b 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -123,6 +123,8 @@ int virNetServerClientGetSELinuxContext(virNetServerClientPtr client,
char **context);
virIdentityPtr virNetServerClientGetIdentity(virNetServerClientPtr client);
+void virNetServerClientSetIdentity(virNetServerClientPtr client,
+ virIdentityPtr identity);
void *virNetServerClientGetPrivateData(virNetServerClientPtr client);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel P. Berrangé writes:
> When opening a connection to a second driver inside the daemon, we must
> ensure the identity of the current user is passed across. This allows
> the second daemon to perform access control checks against the real end
> users, instead of against the libvirt daemon that's proxying across the
> API calls.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> src/libvirt_remote.syms | 1 +
> src/remote/remote_daemon_dispatch.c | 110 +++++++++++++++++++++++++---
> src/remote/remote_driver.c | 1 +
> src/remote/remote_protocol.x | 18 ++++-
> src/remote_protocol-structs | 8 ++
> src/rpc/virnetserverclient.c | 12 +++
> src/rpc/virnetserverclient.h | 2 +
> 7 files changed, 140 insertions(+), 12 deletions(-)
>
> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
> index 3307d74324..0493467f46 100644
> --- a/src/libvirt_remote.syms
> +++ b/src/libvirt_remote.syms
> @@ -178,6 +178,7 @@ virNetServerClientSetAuthLocked;
> virNetServerClientSetAuthPendingLocked;
> virNetServerClientSetCloseHook;
> virNetServerClientSetDispatcher;
> +virNetServerClientSetIdentity;
> virNetServerClientSetQuietEOF;
> virNetServerClientSetReadonly;
> virNetServerClientStartKeepAlive;
> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index 9ef76daa55..f828b75f3b 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -51,6 +51,7 @@
> #include "virpolkit.h"
> #include "virthreadjob.h"
> #include "configmake.h"
> +#include "access/viraccessapicheck.h"
>
> #define VIR_FROM_THIS VIR_FROM_RPC
>
> @@ -1945,10 +1946,15 @@ static void remoteClientCloseFunc(virNetServerClientPtr client)
> static int
> remoteOpenConn(const char *uri,
> bool readonly,
> + bool preserveIdentity,
> virConnectPtr *conn)
> {
> - VIR_DEBUG("Getting secondary uri=%s readonly=%d conn=%p",
> - NULLSTR(uri), readonly, conn);
> + virTypedParameterPtr params = NULL;
> + int nparams = 0;
> +
> + VIR_DEBUG("Getting secondary uri=%s readonly=%d preserveIdent=%d conn=%p",
> + NULLSTR(uri), readonly, preserveIdentity, conn);
> +
> if (*conn)
> return 0;
>
> @@ -1957,15 +1963,42 @@ remoteOpenConn(const char *uri,
> return -1;
> }
>
> + if (preserveIdentity) {
> + VIR_AUTOUNREF(virIdentityPtr) ident = NULL;
> +
> + if (!(ident = virIdentityGetCurrent()))
> + return -1;
> +
> + if (virIdentityGetParameters(ident, ¶ms, &nparams) < 0)
> + goto error;
> + }
> +
> VIR_DEBUG("Opening driver %s", uri);
> if (readonly)
> *conn = virConnectOpenReadOnly(uri);
> else
> *conn = virConnectOpen(uri);
> if (!*conn)
> - return -1;
> + goto error;
> VIR_DEBUG("Opened driver %p", *conn);
> +
> + if (preserveIdentity) {
> + if (virConnectSetIdentity(*conn, params, nparams, 0) < 0)
> + goto error;
> +
> + virTypedParamsFree(params, nparams);
> + VIR_DEBUG("Forwarded current identity to secondary driver");
> + }
> +
> return 0;
> +
> + error:
> + virTypedParamsFree(params, nparams);
> + if (*conn) {
> + virConnectClose(*conn);
> + *conn = NULL;
> + }
> + return -1;
> }
>
>
> @@ -1992,6 +2025,7 @@ remoteGetInterfaceConn(virNetServerClientPtr client)
>
> if (remoteOpenConn(priv->interfaceURI,
> priv->readonly,
> + true,
> &priv->interfaceConn) < 0)
Consider adding a variable "preserveIdentity = true" and passing that
around to make it easier to read what that "true" is about?
> return NULL;
>
> @@ -2007,6 +2041,7 @@ remoteGetNetworkConn(virNetServerClientPtr client)
>
> if (remoteOpenConn(priv->networkURI,
> priv->readonly,
> + true,
> &priv->networkConn) < 0)
> return NULL;
>
> @@ -2022,6 +2057,7 @@ remoteGetNodeDevConn(virNetServerClientPtr client)
>
> if (remoteOpenConn(priv->nodedevURI,
> priv->readonly,
> + true,
> &priv->nodedevConn) < 0)
> return NULL;
>
> @@ -2037,6 +2073,7 @@ remoteGetNWFilterConn(virNetServerClientPtr client)
>
> if (remoteOpenConn(priv->nwfilterURI,
> priv->readonly,
> + true,
> &priv->nwfilterConn) < 0)
> return NULL;
>
> @@ -2052,6 +2089,7 @@ remoteGetSecretConn(virNetServerClientPtr client)
>
> if (remoteOpenConn(priv->secretURI,
> priv->readonly,
> + true,
> &priv->secretConn) < 0)
> return NULL;
>
> @@ -2067,6 +2105,7 @@ remoteGetStorageConn(virNetServerClientPtr client)
>
> if (remoteOpenConn(priv->storageURI,
> priv->readonly,
> + true,
> &priv->storageConn) < 0)
> return NULL;
>
> @@ -2235,6 +2274,7 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
> #ifndef LIBVIRTD
> const char *type = NULL;
> #endif
> + bool preserveIdentity = false;
>
> VIR_DEBUG("priv=%p conn=%p", priv, priv->conn);
> virMutexLock(&priv->lock);
> @@ -2264,14 +2304,16 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
> }
> #endif
>
> +#ifdef VIRTPROXYD
> + preserveIdentity = true;
> +#endif /* VIRTPROXYD */
> +
> VIR_DEBUG("Opening driver %s", name);
> - if (priv->readonly) {
> - if (!(priv->conn = virConnectOpenReadOnly(name)))
> - goto cleanup;
> - } else {
> - if (!(priv->conn = virConnectOpen(name)))
> - goto cleanup;
> - }
> + if (remoteOpenConn(name,
> + priv->readonly,
> + preserveIdentity,
> + &priv->conn) < 0)
> + goto cleanup;
> VIR_DEBUG("Opened %p", priv->conn);
>
> #ifndef LIBVIRTD
> @@ -2375,6 +2417,54 @@ remoteDispatchConnectClose(virNetServerPtr server ATTRIBUTE_UNUSED,
> }
>
>
> +static int
> +remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client,
> + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
Why ATTRIBUTE_UNUSED? Seems used in the cleanup?
> + remote_connect_set_identity_args *args)
> +{
> + virTypedParameterPtr params = NULL;
> + int nparams = 0;
> + int rv = -1;
> + virConnectPtr conn = remoteGetHypervisorConn(client);
> + virIdentityPtr ident = NULL;
(Trying to learn about coding style and conventions)
Why is this not autounref here? Is there a convention that if you
have explicit cleanup, you don't autounref?
> + if (!conn)
> + goto cleanup;
> +
> + VIR_DEBUG("Received forwarded identity");
> + if (virTypedParamsDeserialize((virTypedParameterRemotePtr) args->params.params_val,
> + args->params.params_len,
> + REMOTE_CONNECT_IDENTITY_PARAMS_MAX,
> + ¶ms,
> + &nparams) < 0)
> + goto cleanup;
Would it be useful to change the value rv over these cases,
and if rv < 0, add a VIR_DEBUG with its value? Or is there
sufficient debugging info from the individual calls already?
> +
> + VIR_TYPED_PARAMS_DEBUG(params, nparams);
> +
> + if (virConnectSetIdentityEnsureACL(conn) < 0)
> + goto cleanup;
> +
> + if (!(ident = virIdentityNew()))
> + goto cleanup;
> +
> + if (virIdentitySetParameters(ident, params, nparams) < 0)
> + goto cleanup;
> +
> + virNetServerClientSetIdentity(client, ident);
> +
> + rv = 0;
> +
> + cleanup:
> + virTypedParamsFree(params, nparams);
> + virObjectUnref(ident);
> + if (rv < 0)
> + virNetMessageSaveError(rerr);
> + return rv;
> +}
> +
> +
> +
> static int
> remoteDispatchDomainGetSchedulerType(virNetServerPtr server ATTRIBUTE_UNUSED,
> virNetServerClientPtr client,
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 889d62ba4f..e2d2dc66be 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -8510,6 +8510,7 @@ static virHypervisorDriver hypervisor_driver = {
> .name = "remote",
> .connectOpen = remoteConnectOpen, /* 0.3.0 */
> .connectClose = remoteConnectClose, /* 0.3.0 */
> + .connectSetIdentity = remoteConnectSetIdentity, /* 5.6.0 */
> .connectSupportsFeature = remoteConnectSupportsFeature, /* 0.3.0 */
> .connectGetType = remoteConnectGetType, /* 0.3.0 */
> .connectGetVersion = remoteConnectGetVersion, /* 0.3.0 */
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 2f91bd1921..42e61ba20f 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -53,6 +53,9 @@ typedef string remote_nonnull_string<REMOTE_STRING_MAX>;
> /* A long string, which may be NULL. */
> typedef remote_nonnull_string *remote_string;
>
> +/* Upper limit on identity parameters */
> +const REMOTE_CONNECT_IDENTITY_PARAMS_MAX = 20;
> +
> /* Upper limit on lists of domains. */
> const REMOTE_DOMAIN_LIST_MAX = 16384;
>
> @@ -3723,6 +3726,11 @@ struct remote_domain_checkpoint_delete_args {
> unsigned int flags;
> };
>
> +struct remote_connect_set_identity_args {
> + remote_typed_param params<REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX>;
> + unsigned int flags;
> +};
> +
> /*----- Protocol. -----*/
>
> /* Define the program number, protocol version and procedure numbers here. */
> @@ -6538,7 +6546,7 @@ enum remote_procedure {
> */
> REMOTE_PROC_NETWORK_PORT_DELETE = 410,
>
> - /**
> + /**
> * @generate: both
> * @acl: domain:checkpoint
> * @acl: domain:fs_freeze:VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE
> @@ -6584,5 +6592,11 @@ enum remote_procedure {
> * @generate: both
> * @acl: domain:checkpoint
> */
> - REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417
> + REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417,
> +
> + /**
> + * @generate: client
> + * @acl: connect:write
> + */
> + REMOTE_PROC_CONNECT_SET_IDENTITY = 418
> };
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index a42b4a9671..05229f00c5 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -3105,6 +3105,13 @@ struct remote_domain_checkpoint_delete_args {
> remote_nonnull_domain_checkpoint checkpoint;
> u_int flags;
> };
> +struct remote_connect_set_identity_args {
> + struct {
> + u_int params_len;
> + remote_typed_param * params_val;
Indent by 8 spaces and try to align variables in the same file?
Nothing good could come out of it ;-)
> + } params;
> + u_int flags;
> +};
> enum remote_procedure {
> REMOTE_PROC_CONNECT_OPEN = 1,
> REMOTE_PROC_CONNECT_CLOSE = 2,
> @@ -3523,4 +3530,5 @@ enum remote_procedure {
> REMOTE_PROC_DOMAIN_CHECKPOINT_LOOKUP_BY_NAME = 415,
> REMOTE_PROC_DOMAIN_CHECKPOINT_GET_PARENT = 416,
> REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417,
> + REMOTE_PROC_CONNECT_SET_IDENTITY = 418,
> };
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 1f020a5a04..2a278171f5 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -844,6 +844,18 @@ virIdentityPtr virNetServerClientGetIdentity(virNetServerClientPtr client)
> }
>
>
> +void virNetServerClientSetIdentity(virNetServerClientPtr client,
> + virIdentityPtr identity)
> +{
> + virObjectLock(client);
> + virObjectUnref(client->identity);
> + client->identity = identity;
> + if (client->identity)
> + virObjectRef(client->identity);
> + virObjectUnlock(client);
> +}
> +
> +
> int virNetServerClientGetSELinuxContext(virNetServerClientPtr client,
> char **context)
> {
> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> index 1b01bedbcb..1c520fef6b 100644
> --- a/src/rpc/virnetserverclient.h
> +++ b/src/rpc/virnetserverclient.h
> @@ -123,6 +123,8 @@ int virNetServerClientGetSELinuxContext(virNetServerClientPtr client,
> char **context);
>
> virIdentityPtr virNetServerClientGetIdentity(virNetServerClientPtr client);
> +void virNetServerClientSetIdentity(virNetServerClientPtr client,
> + virIdentityPtr identity);
>
> void *virNetServerClientGetPrivateData(virNetServerClientPtr client);
>
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jul 30, 2019 at 11:32:22AM +0200, Christophe de Dinechin wrote:
>
> Daniel P. Berrangé writes:
>
> > When opening a connection to a second driver inside the daemon, we must
> > ensure the identity of the current user is passed across. This allows
> > the second daemon to perform access control checks against the real end
> > users, instead of against the libvirt daemon that's proxying across the
> > API calls.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > src/libvirt_remote.syms | 1 +
> > src/remote/remote_daemon_dispatch.c | 110 +++++++++++++++++++++++++---
> > src/remote/remote_driver.c | 1 +
> > src/remote/remote_protocol.x | 18 ++++-
> > src/remote_protocol-structs | 8 ++
> > src/rpc/virnetserverclient.c | 12 +++
> > src/rpc/virnetserverclient.h | 2 +
> > 7 files changed, 140 insertions(+), 12 deletions(-)
> >
> > +static int
> > +remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED,
> > + virNetServerClientPtr client,
> > + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> > + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
>
> Why ATTRIBUTE_UNUSED? Seems used in the cleanup?
copy+paste mistake
> > + remote_connect_set_identity_args *args)
> > +{
> > + virTypedParameterPtr params = NULL;
> > + int nparams = 0;
> > + int rv = -1;
> > + virConnectPtr conn = remoteGetHypervisorConn(client);
> > + virIdentityPtr ident = NULL;
>
> (Trying to learn about coding style and conventions)
> Why is this not autounref here? Is there a convention that if you
> have explicit cleanup, you don't autounref?
autounref is our preferred modern style. I'm just not in the habit
well enough, especially when copying existnig code.
>
> > + if (!conn)
> > + goto cleanup;
> > +
> > + VIR_DEBUG("Received forwarded identity");
> > + if (virTypedParamsDeserialize((virTypedParameterRemotePtr) args->params.params_val,
> > + args->params.params_len,
> > + REMOTE_CONNECT_IDENTITY_PARAMS_MAX,
> > + ¶ms,
> > + &nparams) < 0)
> > + goto cleanup;
>
> Would it be useful to change the value rv over these cases,
> and if rv < 0, add a VIR_DEBUG with its value? Or is there
> sufficient debugging info from the individual calls already?
By convention in libvirt the return values are usually only ever
-1 or 0. We have only a few places with return '-errno' in the
code. So we don't need to report the return value most of the
time.
>
> > +
> > + VIR_TYPED_PARAMS_DEBUG(params, nparams);
> > +
> > + if (virConnectSetIdentityEnsureACL(conn) < 0)
> > + goto cleanup;
> > +
> > + if (!(ident = virIdentityNew()))
> > + goto cleanup;
> > +
> > + if (virIdentitySetParameters(ident, params, nparams) < 0)
> > + goto cleanup;
> > +
> > + virNetServerClientSetIdentity(client, ident);
> > +
> > + rv = 0;
> > +
> > + cleanup:
> > + virTypedParamsFree(params, nparams);
> > + virObjectUnref(ident);
> > + if (rv < 0)
> > + virNetMessageSaveError(rerr);
> > + return rv;
> > +}
> > +
> > +
> > +
> > static int
> > remoteDispatchDomainGetSchedulerType(virNetServerPtr server ATTRIBUTE_UNUSED,
> > virNetServerClientPtr client,
> > diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> > index a42b4a9671..05229f00c5 100644
> > --- a/src/remote_protocol-structs
> > +++ b/src/remote_protocol-structs
> > @@ -3105,6 +3105,13 @@ struct remote_domain_checkpoint_delete_args {
> > remote_nonnull_domain_checkpoint checkpoint;
> > u_int flags;
> > };
> > +struct remote_connect_set_identity_args {
> > + struct {
> > + u_int params_len;
> > + remote_typed_param * params_val;
>
> Indent by 8 spaces and try to align variables in the same file?
> Nothing good could come out of it ;-)
This particular file content has to match the auto-generated
output from the 'pdwtags' command. It is basically a sanity
check to catch people who mistakenly change something in the
remote protocol which would break ABI.
>
> > + } params;
> > + u_int flags;
> > +};
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
On Mon, 2019-07-29 at 18:11 +0100, Daniel P. Berrangé wrote:
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1945,10 +1946,15 @@ static void remoteClientCloseFunc(virNetServerClientPtr client)
> static int
> remoteOpenConn(const char *uri,
> bool readonly,
> + bool preserveIdentity,
> virConnectPtr *conn)
> {
[...]
> if (!*conn)
> - return -1;
> + goto error;
> VIR_DEBUG("Opened driver %p", *conn);
> +
> + if (preserveIdentity) {
> + if (virConnectSetIdentity(*conn, params, nparams, 0) < 0)
> + goto error;
> +
> + virTypedParamsFree(params, nparams);
> + VIR_DEBUG("Forwarded current identity to secondary driver");
> + }
> +
> return 0;
> +
> + error:
> + virTypedParamsFree(params, nparams);
> + if (*conn) {
> + virConnectClose(*conn);
> + *conn = NULL;
> + }
> + return -1;
Here I would go for the tried and true
virTypedParameterPtr params = NULL;
int nparams = 0;
int ret = -1;
if (operationFailed)
goto error;
ret = 0;
cleanup:
virTypedParamsFree(params, nparams);
return ret;
error:
if (*conn) {
virConnectClose(*conn);
*conn = NULL;
}
goto cleanup;
Less repetition, and more difficult to get wrong even as other people
come in and make changes :)
[...]
> @@ -1992,6 +2025,7 @@ remoteGetInterfaceConn(virNetServerClientPtr client)
>
> if (remoteOpenConn(priv->interfaceURI,
> priv->readonly,
> + true,
> &priv->interfaceConn) < 0)
So when opening secondary drivers, we'll always attempt to preserve
the user identity...
[...]
> @@ -2264,14 +2304,16 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
> }
> #endif
>
> +#ifdef VIRTPROXYD
> + preserveIdentity = true;
> +#endif /* VIRTPROXYD */
... and we'll do the same when forwarding a connection to hypervisor
daemons through virtproxyd.
Makes sense to me, I'm just not seeing the check ensuring we're
running as the same user that you hinted to in patch 43...
Is it enough to consider virtproxyd trusted for the hypervisor
drivers, and in turn the hypervisor drivers trusted for the secondary
drivers, as we seem to be doing here?
> +static int
> +remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED,
> + virNetServerClientPtr client,
> + virNetMessagePtr msg ATTRIBUTE_UNUSED,
> + virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
> + remote_connect_set_identity_args *args)
As Christophe already pointed out, rerr is actually used.
> +{
> + virTypedParameterPtr params = NULL;
> + int nparams = 0;
> + int rv = -1;
Usually we call this 'ret'.
> + virConnectPtr conn = remoteGetHypervisorConn(client);
> + virIdentityPtr ident = NULL;
Again as pointed out by Christophe, this can be VIR_AUTOUNREF()d.
> +++ b/src/remote/remote_protocol.x
> +struct remote_connect_set_identity_args {
> + remote_typed_param params<REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX>;
Pretty sure you want to use REMOTE_CONNECT_IDENTITY_PARAMS_MAX
here.
> @@ -6538,7 +6546,7 @@ enum remote_procedure {
> */
> REMOTE_PROC_NETWORK_PORT_DELETE = 410,
>
> - /**
> + /**
Unrelated indentation change, but one that's probably not deserving
of its own patch...
Anyway, while the changes overall look good, there are still a few
open questions that I hope you'll address. It also doesn't work at
all for me, at least in legacy mode on a Fedora 30 machine where I
installed the result of 'make rpm' on top of the distro packages:
$ sudo systemctl start libvirtd
Job for libvirtd.service failed because the control process exited with error code.
See "systemctl status libvirtd.service" and "journalctl -xe" for details.
$ sudo journalctl -b 0 -u libvirtd
...
Jul 30 20:06:10 kinshicho systemd[1]: Failed to start Virtualization daemon.
Jul 30 23:24:39 kinshicho systemd[1]: Starting Virtualization daemon...
Jul 30 23:24:40 kinshicho systemd[1]: libvirtd.service: Main process exited, code=exited, status=6/NOTCONFIGURED
Jul 30 23:24:40 kinshicho systemd[1]: libvirtd.service: Failed with result 'exit-code'.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2019-07-30 at 23:30 +0200, Andrea Bolognani wrote:
> On Mon, 2019-07-29 at 18:11 +0100, Daniel P. Berrangé wrote:
> > +++ b/src/remote/remote_protocol.x
> > @@ -6538,7 +6546,7 @@ enum remote_procedure {
> > */
> > REMOTE_PROC_NETWORK_PORT_DELETE = 410,
> >
> > - /**
> > + /**
>
> Unrelated indentation change, but one that's probably not deserving
> of its own patch...
Nevermind :)
commit b0ecc0a04cfcfc706e252d3960f7f10db45c9186
Author: Eric Blake <eblake@redhat.com>
Date: Tue Jul 30 16:46:55 2019 -0500
backup: remote: Trivial whitespace fix
I messed up formatting during conflict resolution across rebasing
while preparing my checkpoint patches :)
Signed-off-by: Eric Blake <eblake@redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.