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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
