[libvirt] [PATCH 0/9] Pass identity information between daemons
This was previously posted as part of the daemon split series: https://www.redhat.com/archives/libvir-list/2019-July/msg01754.html The patches were not merged at the time since they needed more work which is now done by this series. This is all about enabling the polkit fine grain auth checks to work correctly with the split daemons A mgmt app running non-root will connect to virtqemud. virtqemud sees the client identity as the non-root user. virtqemud in turn may connect to virtnetworkd, forwarding API calls that the mgmt app makes. virtnetworkd sees the client identity as the root user. This series allows virtqemud to pass on the non-root identity of the mgmt app to virtnetworkd, so that polkit checks are done against the correct identity. Daniel P. Berrangé (9): api: introduce virConnectSetIdentity for passing uid, gid, selinux info util: change identity class attribute names tests: fix debug messages wrt selinux context when test fails util: make generic identity accessors private util: removed unused virIdentityIsEqual method util: sanitize return values for virIdentity getters util: store identity attrs as virTypedParameter internally util: allow identity to be imported/exported as typed parameters remote: pass identity across to newly opened daemons include/libvirt/libvirt-host.h | 74 +++++ src/access/viraccessdriverpolkit.c | 22 +- src/admin/admin_server.c | 52 +-- src/driver-hypervisor.h | 7 + src/libvirt-host.c | 51 +++ src/libvirt_private.syms | 21 +- src/libvirt_public.syms | 4 + src/libvirt_remote.syms | 1 + src/remote/remote_daemon_dispatch.c | 112 ++++++- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +- src/remote_protocol-structs | 8 + src/rpc/virnetserverclient.c | 20 +- src/rpc/virnetserverclient.h | 2 + src/util/viridentity.c | 488 ++++++++++++++++------------ src/util/viridentity.h | 71 ++-- tests/viridentitytest.c | 115 ++----- tests/virnetserverclienttest.c | 40 +-- 18 files changed, 672 insertions(+), 433 deletions(-) -- 2.21.0
When using the fine grained access control mechanism for APIs, when a client connects to libvirtd, the latter 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 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, with a few small name changes. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/libvirt/libvirt-host.h | 74 ++++++++++++++++++++++++++++++++++ src/driver-hypervisor.h | 7 ++++ src/libvirt-host.c | 51 +++++++++++++++++++++++ src/libvirt_public.syms | 4 ++ 4 files changed, 136 insertions(+) diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h index 7debb5f829..be65b4686b 100644 --- a/include/libvirt/libvirt-host.h +++ b/include/libvirt/libvirt-host.h @@ -579,6 +579,80 @@ virConnectPtr virConnectOpenAuth (const char *name, unsigned int flags); int virConnectRef (virConnectPtr conn); int virConnectClose (virConnectPtr conn); + +/** + * VIR_CONNECT_IDENTITY_USER_NAME: + * + * The operating system user name as VIR_TYPED_PARAM_STRING. + */ +# define VIR_CONNECT_IDENTITY_USER_NAME "user-name" + +/** + * VIR_CONNECT_IDENTITY_UNIX_USER_ID: + * + * The UNIX user ID as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_CONNECT_IDENTITY_UNIX_USER_ID "unix-user-id" + +/** + * VIR_CONNECT_IDENTITY_GROUP_NAME: + * + * The operating system group name as VIR_TYPED_PARAM_STRING. + */ +# define VIR_CONNECT_IDENTITY_GROUP_NAME "group-name" + +/** + * VIR_CONNECT_IDENTITY_UNIX_GROUP_ID: + * + * The UNIX group ID as VIR_TYPED_PARAM_ULLONG. + */ +# define VIR_CONNECT_IDENTITY_UNIX_GROUP_ID "unix-group-id" + +/** + * VIR_CONNECT_IDENTITY_PROCESS_ID: + * + * The operating system process ID as VIR_TYPED_PARAM_LLONG. + */ +# define VIR_CONNECT_IDENTITY_PROCESS_ID "process-id" + +/** + * VIR_CONNECT_IDENTITY_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_PROCESS_TIME "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 58eb731e85..015b2cd01c 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); @@ -1385,6 +1391,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 252f061ba3..40655fbbf5 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -857,4 +857,8 @@ LIBVIRT_5.7.0 { virDomainGetGuestInfo; } LIBVIRT_5.6.0; +LIBVIRT_5.8.0 { + virConnectSetIdentity; +} LIBVIRT_5.7.0; + # .... define new API here using predicted next version number .... -- 2.21.0
Remove the "UNIX" tag from the names for user name, group name, process ID and process time, since these attributes are all usable for non-UNIX platforms like Windows. User ID and group ID are left with a "UNIX" tag, since there's no equivalent on Windows. The closest equivalent concept on Windows, SID, is a struct containing a number of integer fields, which is commonly represented in string format instead. This would require a separate attribute, and is left for a future exercise, since the daemons are not currently built on Windows anyway. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/access/viraccessdriverpolkit.c | 8 ++--- src/admin/admin_server.c | 6 ++-- src/libvirt_private.syms | 16 ++++----- src/rpc/virnetserverclient.c | 8 ++--- src/util/viridentity.c | 56 +++++++++++++++--------------- src/util/viridentity.h | 40 ++++++++++----------- tests/viridentitytest.c | 18 +++++----- tests/virnetserverclienttest.c | 4 +-- 8 files changed, 78 insertions(+), 78 deletions(-) diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index b1473cd0a4..75dbf8a0fa 100644 --- a/src/access/viraccessdriverpolkit.c +++ b/src/access/viraccessdriverpolkit.c @@ -88,14 +88,14 @@ virAccessDriverPolkitGetCaller(const char *actionid, return -1; } - if (virIdentityGetUNIXProcessID(identity, pid) < 0) { + if (virIdentityGetProcessID(identity, pid) < 0) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", - _("No UNIX process ID available")); + _("No process ID available")); goto cleanup; } - if (virIdentityGetUNIXProcessTime(identity, startTime) < 0) { + if (virIdentityGetProcessTime(identity, startTime) < 0) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", - _("No UNIX process start time available")); + _("No process start time available")); goto cleanup; } if (virIdentityGetUNIXUserID(identity, uid) < 0) { diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c index f2a38f6dfa..80e5a36679 100644 --- a/src/admin/admin_server.c +++ b/src/admin/admin_server.c @@ -262,7 +262,7 @@ adminClientGetInfo(virNetServerClientPtr client, VIR_CLIENT_INFO_UNIX_USER_ID, uid) < 0) goto cleanup; - if (virIdentityGetUNIXUserName(identity, &attr) < 0 || + if (virIdentityGetUserName(identity, &attr) < 0 || virTypedParamsAddString(&tmpparams, nparams, &maxparams, VIR_CLIENT_INFO_UNIX_USER_NAME, attr) < 0) @@ -273,13 +273,13 @@ adminClientGetInfo(virNetServerClientPtr client, VIR_CLIENT_INFO_UNIX_GROUP_ID, gid) < 0) goto cleanup; - if (virIdentityGetUNIXGroupName(identity, &attr) < 0 || + if (virIdentityGetGroupName(identity, &attr) < 0 || virTypedParamsAddString(&tmpparams, nparams, &maxparams, VIR_CLIENT_INFO_UNIX_GROUP_NAME, attr) < 0) goto cleanup; - if (virIdentityGetUNIXProcessID(identity, &pid) < 0 || + if (virIdentityGetProcessID(identity, &pid) < 0 || virTypedParamsAddInt(&tmpparams, nparams, &maxparams, VIR_CLIENT_INFO_UNIX_PROCESS_ID, pid) < 0) goto cleanup; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a34d92f5ef..a46d9a9165 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2144,28 +2144,28 @@ virHostGetBootTime; # util/viridentity.h virIdentityGetAttr; virIdentityGetCurrent; +virIdentityGetGroupName; +virIdentityGetProcessID; +virIdentityGetProcessTime; virIdentityGetSASLUserName; virIdentityGetSELinuxContext; virIdentityGetSystem; virIdentityGetUNIXGroupID; -virIdentityGetUNIXGroupName; -virIdentityGetUNIXProcessID; -virIdentityGetUNIXProcessTime; virIdentityGetUNIXUserID; -virIdentityGetUNIXUserName; +virIdentityGetUserName; virIdentityGetX509DName; virIdentityIsEqual; virIdentityNew; virIdentitySetAttr; virIdentitySetCurrent; +virIdentitySetGroupName; +virIdentitySetProcessID; +virIdentitySetProcessTime; virIdentitySetSASLUserName; virIdentitySetSELinuxContext; virIdentitySetUNIXGroupID; -virIdentitySetUNIXGroupName; -virIdentitySetUNIXProcessID; -virIdentitySetUNIXProcessTime; virIdentitySetUNIXUserID; -virIdentitySetUNIXUserName; +virIdentitySetUserName; virIdentitySetX509DName; diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 410de253d2..1b96d0339b 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -779,21 +779,21 @@ virNetServerClientCreateIdentity(virNetServerClientPtr client) if (!(username = virGetUserName(uid))) goto error; - if (virIdentitySetUNIXUserName(ret, username) < 0) + if (virIdentitySetUserName(ret, username) < 0) goto error; if (virIdentitySetUNIXUserID(ret, uid) < 0) goto error; if (!(groupname = virGetGroupName(gid))) goto error; - if (virIdentitySetUNIXGroupName(ret, groupname) < 0) + if (virIdentitySetGroupName(ret, groupname) < 0) goto error; if (virIdentitySetUNIXGroupID(ret, gid) < 0) goto error; - if (virIdentitySetUNIXProcessID(ret, pid) < 0) + if (virIdentitySetProcessID(ret, pid) < 0) goto error; - if (virIdentitySetUNIXProcessTime(ret, timestamp) < 0) + if (virIdentitySetProcessTime(ret, timestamp) < 0) goto error; } diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 4ceff3cb74..d920152c08 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -144,25 +144,25 @@ virIdentityPtr virIdentityGetSystem(void) if (!(ret = virIdentityNew())) goto error; - if (virIdentitySetUNIXProcessID(ret, getpid()) < 0) + if (virIdentitySetProcessID(ret, getpid()) < 0) goto error; if (virProcessGetStartTime(getpid(), &startTime) < 0) goto error; if (startTime != 0 && - virIdentitySetUNIXProcessTime(ret, startTime) < 0) + virIdentitySetProcessTime(ret, startTime) < 0) goto error; if (!(username = virGetUserName(geteuid()))) return ret; - if (virIdentitySetUNIXUserName(ret, username) < 0) + if (virIdentitySetUserName(ret, username) < 0) goto error; if (virIdentitySetUNIXUserID(ret, getuid()) < 0) goto error; if (!(groupname = virGetGroupName(getegid()))) return ret; - if (virIdentitySetUNIXGroupName(ret, groupname) < 0) + if (virIdentitySetGroupName(ret, groupname) < 0) goto error; if (virIdentitySetUNIXGroupID(ret, getgid()) < 0) goto error; @@ -310,11 +310,11 @@ bool virIdentityIsEqual(virIdentityPtr identA, } -int virIdentityGetUNIXUserName(virIdentityPtr ident, - const char **username) +int virIdentityGetUserName(virIdentityPtr ident, + const char **username) { return virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, username); } @@ -342,11 +342,11 @@ int virIdentityGetUNIXUserID(virIdentityPtr ident, return 0; } -int virIdentityGetUNIXGroupName(virIdentityPtr ident, - const char **groupname) +int virIdentityGetGroupName(virIdentityPtr ident, + const char **groupname) { return virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + VIR_IDENTITY_ATTR_GROUP_NAME, groupname); } @@ -375,15 +375,15 @@ int virIdentityGetUNIXGroupID(virIdentityPtr ident, } -int virIdentityGetUNIXProcessID(virIdentityPtr ident, - pid_t *pid) +int virIdentityGetProcessID(virIdentityPtr ident, + pid_t *pid) { unsigned long long val; const char *processid; *pid = 0; if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, + VIR_IDENTITY_ATTR_PROCESS_ID, &processid) < 0) return -1; @@ -399,12 +399,12 @@ int virIdentityGetUNIXProcessID(virIdentityPtr ident, } -int virIdentityGetUNIXProcessTime(virIdentityPtr ident, - unsigned long long *timestamp) +int virIdentityGetProcessTime(virIdentityPtr ident, + unsigned long long *timestamp) { const char *processtime; if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, + VIR_IDENTITY_ATTR_PROCESS_TIME, &processtime) < 0) return -1; @@ -445,11 +445,11 @@ int virIdentityGetSELinuxContext(virIdentityPtr ident, } -int virIdentitySetUNIXUserName(virIdentityPtr ident, - const char *username) +int virIdentitySetUserName(virIdentityPtr ident, + const char *username) { return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, username); } @@ -468,11 +468,11 @@ int virIdentitySetUNIXUserID(virIdentityPtr ident, } -int virIdentitySetUNIXGroupName(virIdentityPtr ident, - const char *groupname) +int virIdentitySetGroupName(virIdentityPtr ident, + const char *groupname) { return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + VIR_IDENTITY_ATTR_GROUP_NAME, groupname); } @@ -491,8 +491,8 @@ int virIdentitySetUNIXGroupID(virIdentityPtr ident, } -int virIdentitySetUNIXProcessID(virIdentityPtr ident, - pid_t pid) +int virIdentitySetProcessID(virIdentityPtr ident, + pid_t pid) { VIR_AUTOFREE(char *) val = NULL; @@ -500,13 +500,13 @@ int virIdentitySetUNIXProcessID(virIdentityPtr ident, return -1; return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, + VIR_IDENTITY_ATTR_PROCESS_ID, val); } -int virIdentitySetUNIXProcessTime(virIdentityPtr ident, - unsigned long long timestamp) +int virIdentitySetProcessTime(virIdentityPtr ident, + unsigned long long timestamp) { VIR_AUTOFREE(char *) val = NULL; @@ -514,7 +514,7 @@ int virIdentitySetUNIXProcessTime(virIdentityPtr ident, return -1; return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, + VIR_IDENTITY_ATTR_PROCESS_TIME, val); } diff --git a/src/util/viridentity.h b/src/util/viridentity.h index 0fde3207ca..e66e60dbf3 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -27,12 +27,12 @@ typedef struct _virIdentity virIdentity; typedef virIdentity *virIdentityPtr; typedef enum { - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, VIR_IDENTITY_ATTR_UNIX_USER_ID, - VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + VIR_IDENTITY_ATTR_GROUP_NAME, VIR_IDENTITY_ATTR_UNIX_GROUP_ID, - VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, - VIR_IDENTITY_ATTR_UNIX_PROCESS_TIME, + VIR_IDENTITY_ATTR_PROCESS_ID, + VIR_IDENTITY_ATTR_PROCESS_TIME, VIR_IDENTITY_ATTR_SASL_USER_NAME, VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, VIR_IDENTITY_ATTR_SELINUX_CONTEXT, @@ -64,18 +64,18 @@ bool virIdentityIsEqual(virIdentityPtr identA, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int virIdentityGetUNIXUserName(virIdentityPtr ident, - const char **username); +int virIdentityGetUserName(virIdentityPtr ident, + const char **username); int virIdentityGetUNIXUserID(virIdentityPtr ident, uid_t *uid); -int virIdentityGetUNIXGroupName(virIdentityPtr ident, - const char **groupname); +int virIdentityGetGroupName(virIdentityPtr ident, + const char **groupname); int virIdentityGetUNIXGroupID(virIdentityPtr ident, gid_t *gid); -int virIdentityGetUNIXProcessID(virIdentityPtr ident, - pid_t *pid); -int virIdentityGetUNIXProcessTime(virIdentityPtr ident, - unsigned long long *timestamp); +int virIdentityGetProcessID(virIdentityPtr ident, + pid_t *pid); +int virIdentityGetProcessTime(virIdentityPtr ident, + unsigned long long *timestamp); int virIdentityGetSASLUserName(virIdentityPtr ident, const char **username); int virIdentityGetX509DName(virIdentityPtr ident, @@ -84,18 +84,18 @@ int virIdentityGetSELinuxContext(virIdentityPtr ident, const char **context); -int virIdentitySetUNIXUserName(virIdentityPtr ident, - const char *username); +int virIdentitySetUserName(virIdentityPtr ident, + const char *username); int virIdentitySetUNIXUserID(virIdentityPtr ident, uid_t uid); -int virIdentitySetUNIXGroupName(virIdentityPtr ident, - const char *groupname); +int virIdentitySetGroupName(virIdentityPtr ident, + const char *groupname); int virIdentitySetUNIXGroupID(virIdentityPtr ident, gid_t gid); -int virIdentitySetUNIXProcessID(virIdentityPtr ident, - pid_t pid); -int virIdentitySetUNIXProcessTime(virIdentityPtr ident, - unsigned long long timestamp); +int virIdentitySetProcessID(virIdentityPtr ident, + pid_t pid); +int virIdentitySetProcessTime(virIdentityPtr ident, + unsigned long long timestamp); int virIdentitySetSASLUserName(virIdentityPtr ident, const char *username); int virIdentitySetX509DName(virIdentityPtr ident, diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c index b60d944d8e..17d6e5f3b3 100644 --- a/tests/viridentitytest.c +++ b/tests/viridentitytest.c @@ -46,12 +46,12 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) goto cleanup; if (virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, "fred") < 0) goto cleanup; if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, &val) < 0) goto cleanup; @@ -61,7 +61,7 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) } if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + VIR_IDENTITY_ATTR_GROUP_NAME, &val) < 0) goto cleanup; @@ -71,14 +71,14 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) } if (virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, "joe") != -1) { VIR_DEBUG("Unexpectedly overwrote attribute"); goto cleanup; } if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, &val) < 0) goto cleanup; @@ -111,7 +111,7 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED) } if (virIdentitySetAttr(identa, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, "fred") < 0) goto cleanup; @@ -121,7 +121,7 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED) } if (virIdentitySetAttr(identb, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, "fred") < 0) goto cleanup; @@ -131,11 +131,11 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED) } if (virIdentitySetAttr(identa, - VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + VIR_IDENTITY_ATTR_GROUP_NAME, "flintstone") < 0) goto cleanup; if (virIdentitySetAttr(identb, - VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + VIR_IDENTITY_ATTR_GROUP_NAME, "flintstone") < 0) goto cleanup; diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c index 4d7c6555b9..5015273e55 100644 --- a/tests/virnetserverclienttest.c +++ b/tests/virnetserverclienttest.c @@ -86,7 +86,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) } if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_NAME, + VIR_IDENTITY_ATTR_USER_NAME, &gotUsername) < 0) { fprintf(stderr, "Missing username in identity\n"); goto cleanup; @@ -110,7 +110,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) } if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + VIR_IDENTITY_ATTR_GROUP_NAME, &gotGroupname) < 0) { fprintf(stderr, "Missing groupname in identity\n"); goto cleanup; -- 2.21.0
Reviewed-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- tests/viridentitytest.c | 3 ++- tests/virnetserverclienttest.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c index 17d6e5f3b3..3c9eb8683a 100644 --- a/tests/viridentitytest.c +++ b/tests/viridentitytest.c @@ -187,7 +187,8 @@ static int testIdentityGetSystem(const void *data) goto cleanup; if (STRNEQ_NULLABLE(val, context)) { - VIR_DEBUG("Unexpected SELinux context attribute"); + VIR_DEBUG("Want SELinux context '%s' got '%s'", + context, val); goto cleanup; } diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c index 5015273e55..aaecfe7987 100644 --- a/tests/virnetserverclienttest.c +++ b/tests/virnetserverclienttest.c @@ -140,8 +140,8 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } if (STRNEQ_NULLABLE("foo_u:bar_r:wizz_t:s0-s0:c0.c1023", gotSELinuxContext)) { - fprintf(stderr, "Want groupname 'foo_u:bar_r:wizz_t:s0-s0:c0.c1023' got '%s'\n", - NULLSTR(gotGroupID)); + fprintf(stderr, "Want SELinux context 'foo_u:bar_r:wizz_t:s0-s0:c0.c1023' got '%s'\n", + NULLSTR(gotSELinuxContext)); goto cleanup; } -- 2.21.0
Only expose the type safe getters/setters to other code in preparation for changing the internal storage of data. Reviewed-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 2 -- src/util/viridentity.c | 28 +++++++++++++++++----- src/util/viridentity.h | 25 ------------------- tests/viridentitytest.c | 44 +++++++++------------------------- tests/virnetserverclienttest.c | 36 ++++++++++------------------ 5 files changed, 46 insertions(+), 89 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a46d9a9165..108a8ef736 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2142,7 +2142,6 @@ virHostGetBootTime; # util/viridentity.h -virIdentityGetAttr; virIdentityGetCurrent; virIdentityGetGroupName; virIdentityGetProcessID; @@ -2156,7 +2155,6 @@ virIdentityGetUserName; virIdentityGetX509DName; virIdentityIsEqual; virIdentityNew; -virIdentitySetAttr; virIdentitySetCurrent; virIdentitySetGroupName; virIdentitySetProcessID; diff --git a/src/util/viridentity.c b/src/util/viridentity.c index d920152c08..abb486c87f 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -41,6 +41,20 @@ VIR_LOG_INIT("util.identity"); +typedef enum { + VIR_IDENTITY_ATTR_USER_NAME, + VIR_IDENTITY_ATTR_UNIX_USER_ID, + VIR_IDENTITY_ATTR_GROUP_NAME, + VIR_IDENTITY_ATTR_UNIX_GROUP_ID, + VIR_IDENTITY_ATTR_PROCESS_ID, + VIR_IDENTITY_ATTR_PROCESS_TIME, + VIR_IDENTITY_ATTR_SASL_USER_NAME, + VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, + VIR_IDENTITY_ATTR_SELINUX_CONTEXT, + + VIR_IDENTITY_ATTR_LAST, +} virIdentityAttrType; + struct _virIdentity { virObject parent; @@ -233,9 +247,10 @@ static void virIdentityDispose(void *object) * * Returns: 0 on success, or -1 on error */ -int virIdentitySetAttr(virIdentityPtr ident, - unsigned int attr, - const char *value) +static int +virIdentitySetAttr(virIdentityPtr ident, + unsigned int attr, + const char *value) { int ret = -1; VIR_DEBUG("ident=%p attribute=%u value=%s", ident, attr, value); @@ -269,9 +284,10 @@ int virIdentitySetAttr(virIdentityPtr ident, * * Returns 0 on success, -1 on error */ -int virIdentityGetAttr(virIdentityPtr ident, - unsigned int attr, - const char **value) +static int +virIdentityGetAttr(virIdentityPtr ident, + unsigned int attr, + const char **value) { VIR_DEBUG("ident=%p attribute=%d value=%p", ident, attr, value); diff --git a/src/util/viridentity.h b/src/util/viridentity.h index e66e60dbf3..e243284cd5 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -26,20 +26,6 @@ typedef struct _virIdentity virIdentity; typedef virIdentity *virIdentityPtr; -typedef enum { - VIR_IDENTITY_ATTR_USER_NAME, - VIR_IDENTITY_ATTR_UNIX_USER_ID, - VIR_IDENTITY_ATTR_GROUP_NAME, - VIR_IDENTITY_ATTR_UNIX_GROUP_ID, - VIR_IDENTITY_ATTR_PROCESS_ID, - VIR_IDENTITY_ATTR_PROCESS_TIME, - VIR_IDENTITY_ATTR_SASL_USER_NAME, - VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, - VIR_IDENTITY_ATTR_SELINUX_CONTEXT, - - VIR_IDENTITY_ATTR_LAST, -} virIdentityAttrType; - virIdentityPtr virIdentityGetCurrent(void); int virIdentitySetCurrent(virIdentityPtr ident); @@ -47,17 +33,6 @@ virIdentityPtr virIdentityGetSystem(void); virIdentityPtr virIdentityNew(void); -int virIdentitySetAttr(virIdentityPtr ident, - unsigned int attr, - const char *value) - ATTRIBUTE_NONNULL(1) - ATTRIBUTE_NONNULL(3); - -int virIdentityGetAttr(virIdentityPtr ident, - unsigned int attr, - const char **value) - ATTRIBUTE_NONNULL(1) - ATTRIBUTE_NONNULL(3); bool virIdentityIsEqual(virIdentityPtr identA, virIdentityPtr identB) diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c index 3c9eb8683a..cdf5325b4c 100644 --- a/tests/viridentitytest.c +++ b/tests/viridentitytest.c @@ -45,14 +45,10 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) if (!(ident = virIdentityNew())) goto cleanup; - if (virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_USER_NAME, - "fred") < 0) + if (virIdentitySetUserName(ident, "fred") < 0) goto cleanup; - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_USER_NAME, - &val) < 0) + if (virIdentityGetUserName(ident, &val) < 0) goto cleanup; if (STRNEQ_NULLABLE(val, "fred")) { @@ -60,9 +56,7 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_GROUP_NAME, - &val) < 0) + if (virIdentityGetGroupName(ident, &val) < 0) goto cleanup; if (val != NULL) { @@ -70,16 +64,12 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_USER_NAME, - "joe") != -1) { + if (virIdentitySetUserName(ident, "joe") >= 0) { VIR_DEBUG("Unexpectedly overwrote attribute"); goto cleanup; } - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_USER_NAME, - &val) < 0) + if (virIdentityGetUserName(ident, &val) < 0) goto cleanup; if (STRNEQ_NULLABLE(val, "fred")) { @@ -110,9 +100,7 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentitySetAttr(identa, - VIR_IDENTITY_ATTR_USER_NAME, - "fred") < 0) + if (virIdentitySetUserName(identa, "fred") < 0) goto cleanup; if (virIdentityIsEqual(identa, identb)) { @@ -120,9 +108,7 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentitySetAttr(identb, - VIR_IDENTITY_ATTR_USER_NAME, - "fred") < 0) + if (virIdentitySetUserName(identb, "fred") < 0) goto cleanup; if (!virIdentityIsEqual(identa, identb)) { @@ -130,13 +116,9 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentitySetAttr(identa, - VIR_IDENTITY_ATTR_GROUP_NAME, - "flintstone") < 0) + if (virIdentitySetGroupName(identa, "flintstone") < 0) goto cleanup; - if (virIdentitySetAttr(identb, - VIR_IDENTITY_ATTR_GROUP_NAME, - "flintstone") < 0) + if (virIdentitySetGroupName(identb, "flintstone") < 0) goto cleanup; if (!virIdentityIsEqual(identa, identb)) { @@ -144,9 +126,7 @@ static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentitySetAttr(identb, - VIR_IDENTITY_ATTR_SASL_USER_NAME, - "fred@FLINTSTONE.COM") < 0) + if (virIdentitySetSASLUserName(identb, "fred@FLINTSTONE.COM") < 0) goto cleanup; if (virIdentityIsEqual(identa, identb)) { @@ -181,9 +161,7 @@ static int testIdentityGetSystem(const void *data) goto cleanup; } - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_SELINUX_CONTEXT, - &val) < 0) + if (virIdentityGetSELinuxContext(ident, &val) < 0) goto cleanup; if (STRNEQ_NULLABLE(val, context)) { diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c index aaecfe7987..3cd76f42ff 100644 --- a/tests/virnetserverclienttest.c +++ b/tests/virnetserverclienttest.c @@ -53,9 +53,9 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) virNetServerClientPtr client = NULL; virIdentityPtr ident = NULL; const char *gotUsername = NULL; - const char *gotUserID = NULL; + uid_t gotUserID; const char *gotGroupname = NULL; - const char *gotGroupID = NULL; + gid_t gotGroupID; const char *gotSELinuxContext = NULL; if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) < 0) { @@ -85,9 +85,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_USER_NAME, - &gotUsername) < 0) { + if (virIdentityGetUserName(ident, &gotUsername) < 0) { fprintf(stderr, "Missing username in identity\n"); goto cleanup; } @@ -97,21 +95,17 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_ID, - &gotUserID) < 0) { + if (virIdentityGetUNIXUserID(ident, &gotUserID) < 0) { fprintf(stderr, "Missing user ID in identity\n"); goto cleanup; } - if (STRNEQ_NULLABLE("666", gotUserID)) { - fprintf(stderr, "Want username '666' got '%s'\n", - NULLSTR(gotUserID)); + if (666 != gotUserID) { + fprintf(stderr, "Want username '666' got '%llu'\n", + (unsigned long long)gotUserID); goto cleanup; } - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_GROUP_NAME, - &gotGroupname) < 0) { + if (virIdentityGetGroupName(ident, &gotGroupname) < 0) { fprintf(stderr, "Missing groupname in identity\n"); goto cleanup; } @@ -121,21 +115,17 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_GROUP_ID, - &gotGroupID) < 0) { + if (virIdentityGetUNIXGroupID(ident, &gotGroupID) < 0) { fprintf(stderr, "Missing group ID in identity\n"); goto cleanup; } - if (STRNEQ_NULLABLE("7337", gotGroupID)) { - fprintf(stderr, "Want groupname '7337' got '%s'\n", - NULLSTR(gotGroupID)); + if (7337 != gotGroupID) { + fprintf(stderr, "Want groupname '7337' got '%llu'\n", + (unsigned long long)gotGroupID); goto cleanup; } - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_SELINUX_CONTEXT, - &gotSELinuxContext) < 0) { + if (virIdentityGetSELinuxContext(ident, &gotSELinuxContext) < 0) { fprintf(stderr, "Missing SELinux context in identity\n"); goto cleanup; } -- 2.21.0
It is simpler to remove this unused method than to rewrite it using typed parameters in the next patch. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 1 - src/util/viridentity.c | 29 -------------------- src/util/viridentity.h | 6 ---- tests/viridentitytest.c | 59 ---------------------------------------- 4 files changed, 95 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 108a8ef736..fec1787497 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2153,7 +2153,6 @@ virIdentityGetUNIXGroupID; virIdentityGetUNIXUserID; virIdentityGetUserName; virIdentityGetX509DName; -virIdentityIsEqual; virIdentityNew; virIdentitySetCurrent; virIdentitySetGroupName; diff --git a/src/util/viridentity.c b/src/util/viridentity.c index abb486c87f..55312fc0a0 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -297,35 +297,6 @@ virIdentityGetAttr(virIdentityPtr ident, } -/** - * virIdentityIsEqual: - * @identA: the first identity - * @identB: the second identity - * - * Compares every attribute in @identA and @identB - * to determine if they refer to the same identity - * - * Returns true if they are equal, false if not equal - */ -bool virIdentityIsEqual(virIdentityPtr identA, - virIdentityPtr identB) -{ - bool ret = false; - size_t i; - VIR_DEBUG("identA=%p identB=%p", identA, identB); - - for (i = 0; i < VIR_IDENTITY_ATTR_LAST; i++) { - if (STRNEQ_NULLABLE(identA->attrs[i], - identB->attrs[i])) - goto cleanup; - } - - ret = true; - cleanup: - return ret; -} - - int virIdentityGetUserName(virIdentityPtr ident, const char **username) { diff --git a/src/util/viridentity.h b/src/util/viridentity.h index e243284cd5..805ad3ea4d 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -33,12 +33,6 @@ virIdentityPtr virIdentityGetSystem(void); virIdentityPtr virIdentityNew(void); - -bool virIdentityIsEqual(virIdentityPtr identA, - virIdentityPtr identB) - ATTRIBUTE_NONNULL(1) - ATTRIBUTE_NONNULL(2); - int virIdentityGetUserName(virIdentityPtr ident, const char **username); int virIdentityGetUNIXUserID(virIdentityPtr ident, diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c index cdf5325b4c..d76c779dd5 100644 --- a/tests/viridentitytest.c +++ b/tests/viridentitytest.c @@ -84,63 +84,6 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) } -static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED) -{ - int ret = -1; - virIdentityPtr identa = NULL; - virIdentityPtr identb = NULL; - - if (!(identa = virIdentityNew())) - goto cleanup; - if (!(identb = virIdentityNew())) - goto cleanup; - - if (!virIdentityIsEqual(identa, identb)) { - VIR_DEBUG("Empty identities were not equal"); - goto cleanup; - } - - if (virIdentitySetUserName(identa, "fred") < 0) - goto cleanup; - - if (virIdentityIsEqual(identa, identb)) { - VIR_DEBUG("Mis-matched identities should not be equal"); - goto cleanup; - } - - if (virIdentitySetUserName(identb, "fred") < 0) - goto cleanup; - - if (!virIdentityIsEqual(identa, identb)) { - VIR_DEBUG("Matched identities were not equal"); - goto cleanup; - } - - if (virIdentitySetGroupName(identa, "flintstone") < 0) - goto cleanup; - if (virIdentitySetGroupName(identb, "flintstone") < 0) - goto cleanup; - - if (!virIdentityIsEqual(identa, identb)) { - VIR_DEBUG("Matched identities were not equal"); - goto cleanup; - } - - if (virIdentitySetSASLUserName(identb, "fred@FLINTSTONE.COM") < 0) - goto cleanup; - - if (virIdentityIsEqual(identa, identb)) { - VIR_DEBUG("Mis-matched identities should not be equal"); - goto cleanup; - } - - ret = 0; - cleanup: - virObjectUnref(identa); - virObjectUnref(identb); - return ret; -} - static int testIdentityGetSystem(const void *data) { const char *context = data; @@ -204,8 +147,6 @@ mymain(void) if (virTestRun("Identity attributes ", testIdentityAttrs, NULL) < 0) ret = -1; - if (virTestRun("Identity equality ", testIdentityEqual, NULL) < 0) - ret = -1; if (virTestRun("Setting fake SELinux context ", testSetFakeSELinuxContext, context) < 0) ret = -1; if (virTestRun("System identity (fake SELinux enabled) ", testIdentityGetSystem, context) < 0) -- 2.21.0
The virIdentity getters are unusual in that they return -1 to indicate "not found" and don't report any error. Change them to return -1 for real errors, 0 for not found, and 1 for success. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/access/viraccessdriverpolkit.c | 18 +++- src/admin/admin_server.c | 52 ++++++---- src/util/viridentity.c | 156 ++++++++++++++++++++--------- tests/viridentitytest.c | 31 ++++-- tests/virnetserverclienttest.c | 10 +- 5 files changed, 180 insertions(+), 87 deletions(-) diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index 75dbf8a0fa..e61ac6fa19 100644 --- a/src/access/viraccessdriverpolkit.c +++ b/src/access/viraccessdriverpolkit.c @@ -80,6 +80,7 @@ virAccessDriverPolkitGetCaller(const char *actionid, { virIdentityPtr identity = virIdentityGetCurrent(); int ret = -1; + int rc; if (!identity) { virAccessError(VIR_ERR_ACCESS_DENIED, @@ -88,17 +89,28 @@ virAccessDriverPolkitGetCaller(const char *actionid, return -1; } - if (virIdentityGetProcessID(identity, pid) < 0) { + if ((rc = virIdentityGetProcessID(identity, pid)) < 0) + goto cleanup; + + if (rc == 0) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", _("No process ID available")); goto cleanup; } - if (virIdentityGetProcessTime(identity, startTime) < 0) { + + if ((rc = virIdentityGetProcessTime(identity, startTime)) < 0) + goto cleanup; + + if (rc == 0) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", _("No process start time available")); goto cleanup; } - if (virIdentityGetUNIXUserID(identity, uid) < 0) { + + if ((rc = virIdentityGetUNIXUserID(identity, uid)) < 0) + goto cleanup; + + if (rc == 0) { virAccessError(VIR_ERR_INTERNAL_ERROR, "%s", _("No UNIX caller UID available")); goto cleanup; diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c index 80e5a36679..248df3f795 100644 --- a/src/admin/admin_server.c +++ b/src/admin/admin_server.c @@ -222,6 +222,7 @@ adminClientGetInfo(virNetServerClientPtr client, const char *attr = NULL; virTypedParameterPtr tmpparams = NULL; virIdentityPtr identity = NULL; + int rc; virCheckFlags(0, -1); @@ -234,11 +235,12 @@ adminClientGetInfo(virNetServerClientPtr client, readonly) < 0) goto cleanup; - if (virIdentityGetSASLUserName(identity, &attr) < 0 || - (attr && - virTypedParamsAddString(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_SASL_USER_NAME, - attr) < 0)) + if ((rc = virIdentityGetSASLUserName(identity, &attr)) < 0) + goto cleanup; + if (rc == 1 && + virTypedParamsAddString(&tmpparams, nparams, &maxparams, + VIR_CLIENT_INFO_SASL_USER_NAME, + attr) < 0) goto cleanup; if (!virNetServerClientIsLocal(client)) { @@ -247,48 +249,60 @@ adminClientGetInfo(virNetServerClientPtr client, sock_addr) < 0) goto cleanup; - if (virIdentityGetX509DName(identity, &attr) < 0 || - (attr && - virTypedParamsAddString(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_X509_DISTINGUISHED_NAME, - attr) < 0)) + if ((rc = virIdentityGetX509DName(identity, &attr)) < 0) + goto cleanup; + if (rc == 1 && + virTypedParamsAddString(&tmpparams, nparams, &maxparams, + VIR_CLIENT_INFO_X509_DISTINGUISHED_NAME, + attr) < 0) goto cleanup; } else { pid_t pid; uid_t uid; gid_t gid; - if (virIdentityGetUNIXUserID(identity, &uid) < 0 || + if ((rc = virIdentityGetUNIXUserID(identity, &uid)) < 0) + goto cleanup; + if (rc == 1 && virTypedParamsAddInt(&tmpparams, nparams, &maxparams, VIR_CLIENT_INFO_UNIX_USER_ID, uid) < 0) goto cleanup; - if (virIdentityGetUserName(identity, &attr) < 0 || + if ((rc = virIdentityGetUserName(identity, &attr)) < 0) + goto cleanup; + if (rc == 1 && virTypedParamsAddString(&tmpparams, nparams, &maxparams, VIR_CLIENT_INFO_UNIX_USER_NAME, attr) < 0) goto cleanup; - if (virIdentityGetUNIXGroupID(identity, &gid) < 0 || + if ((rc = virIdentityGetUNIXGroupID(identity, &gid)) < 0) + goto cleanup; + if (rc == 1 && virTypedParamsAddInt(&tmpparams, nparams, &maxparams, VIR_CLIENT_INFO_UNIX_GROUP_ID, gid) < 0) goto cleanup; - if (virIdentityGetGroupName(identity, &attr) < 0 || + if ((rc = virIdentityGetGroupName(identity, &attr)) < 0) + goto cleanup; + if (rc == 1 && virTypedParamsAddString(&tmpparams, nparams, &maxparams, VIR_CLIENT_INFO_UNIX_GROUP_NAME, attr) < 0) goto cleanup; - if (virIdentityGetProcessID(identity, &pid) < 0 || + if ((rc = virIdentityGetProcessID(identity, &pid)) < 0) + goto cleanup; + if (rc == 1 && virTypedParamsAddInt(&tmpparams, nparams, &maxparams, VIR_CLIENT_INFO_UNIX_PROCESS_ID, pid) < 0) goto cleanup; } - if (virIdentityGetSELinuxContext(identity, &attr) < 0 || - (attr && - virTypedParamsAddString(&tmpparams, nparams, &maxparams, - VIR_CLIENT_INFO_SELINUX_CONTEXT, attr) < 0)) + if ((rc = virIdentityGetSELinuxContext(identity, &attr)) < 0) + goto cleanup; + if (rc == 1 && + virTypedParamsAddString(&tmpparams, nparams, &maxparams, + VIR_CLIENT_INFO_SELINUX_CONTEXT, attr) < 0) goto cleanup; *params = tmpparams; diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 55312fc0a0..964a33d339 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -281,10 +281,8 @@ virIdentitySetAttr(virIdentityPtr ident, * with the identifying attribute @attr in @ident. If * @attr is not set, then it will simply be initialized * to NULL and considered as a successful read - * - * Returns 0 on success, -1 on error */ -static int +static void virIdentityGetAttr(virIdentityPtr ident, unsigned int attr, const char **value) @@ -292,20 +290,29 @@ virIdentityGetAttr(virIdentityPtr ident, VIR_DEBUG("ident=%p attribute=%d value=%p", ident, attr, value); *value = ident->attrs[attr]; - - return 0; } +/* + * Returns: 0 if not present, 1 if present, -1 on error + */ int virIdentityGetUserName(virIdentityPtr ident, const char **username) { - return virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_USER_NAME, - username); + virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_USER_NAME, + username); + + if (!*username) + return 0; + + return 1; } +/* + * Returns: 0 if not present, 1 if present, -1 on error + */ int virIdentityGetUNIXUserID(virIdentityPtr ident, uid_t *uid) { @@ -313,31 +320,44 @@ int virIdentityGetUNIXUserID(virIdentityPtr ident, const char *userid; *uid = -1; - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_ID, - &userid) < 0) - return -1; - + virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_USER_ID, + &userid); if (!userid) - return -1; + return 0; - if (virStrToLong_i(userid, NULL, 10, &val) < 0) + if (virStrToLong_i(userid, NULL, 10, &val) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse user ID '%s'"), userid); return -1; + } *uid = (uid_t)val; - return 0; + return 1; } + +/* + * Returns: 0 if not present, 1 if present, -1 on error + */ int virIdentityGetGroupName(virIdentityPtr ident, const char **groupname) { - return virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_GROUP_NAME, - groupname); + virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_GROUP_NAME, + groupname); + + if (!*groupname) + return 0; + + return 1; } +/* + * Returns: 0 if not present, 1 if present, -1 on error + */ int virIdentityGetUNIXGroupID(virIdentityPtr ident, gid_t *gid) { @@ -345,23 +365,28 @@ int virIdentityGetUNIXGroupID(virIdentityPtr ident, const char *groupid; *gid = -1; - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_GROUP_ID, - &groupid) < 0) - return -1; + virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_GROUP_ID, + &groupid); if (!groupid) - return -1; + return 0; - if (virStrToLong_i(groupid, NULL, 10, &val) < 0) + if (virStrToLong_i(groupid, NULL, 10, &val) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse group ID '%s'"), groupid); return -1; + } *gid = (gid_t)val; - return 0; + return 1; } +/* + * Returns: 0 if not present, 1 if present, -1 on error + */ int virIdentityGetProcessID(virIdentityPtr ident, pid_t *pid) { @@ -369,66 +394,99 @@ int virIdentityGetProcessID(virIdentityPtr ident, const char *processid; *pid = 0; - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_PROCESS_ID, - &processid) < 0) - return -1; + virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_PROCESS_ID, + &processid); if (!processid) - return -1; + return 0; - if (virStrToLong_ull(processid, NULL, 10, &val) < 0) + if (virStrToLong_ull(processid, NULL, 10, &val) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse process ID '%s'"), processid); return -1; + } *pid = (pid_t)val; - return 0; + return 1; } +/* + * Returns: 0 if not present, 1 if present, -1 on error + */ int virIdentityGetProcessTime(virIdentityPtr ident, unsigned long long *timestamp) { const char *processtime; - if (virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_PROCESS_TIME, - &processtime) < 0) - return -1; + + *timestamp = 0; + virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_PROCESS_TIME, + &processtime); if (!processtime) - return -1; + return 0; - if (virStrToLong_ull(processtime, NULL, 10, timestamp) < 0) + if (virStrToLong_ull(processtime, NULL, 10, timestamp) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse process time '%s'"), processtime); return -1; + } - return 0; + return 1; } +/* + * Returns: 0 if not present, 1 if present, -1 on error + */ int virIdentityGetSASLUserName(virIdentityPtr ident, const char **username) { - return virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_SASL_USER_NAME, - username); + virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_SASL_USER_NAME, + username); + + if (!*username) + return 0; + + return 1; } +/* + * Returns: 0 if not present, 1 if present, -1 on error + */ int virIdentityGetX509DName(virIdentityPtr ident, const char **dname) { - return virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, - dname); + virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, + dname); + + if (!*dname) + return 0; + + return 1; } +/* + * Returns: 0 if not present, 1 if present, -1 on error + */ int virIdentityGetSELinuxContext(virIdentityPtr ident, const char **context) { - return virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_SELINUX_CONTEXT, - context); + virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_SELINUX_CONTEXT, + context); + + if (!*context) + return 0; + + return 1; } diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c index d76c779dd5..1eadd6173a 100644 --- a/tests/viridentitytest.c +++ b/tests/viridentitytest.c @@ -41,6 +41,7 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) int ret = -1; virIdentityPtr ident; const char *val; + int rc; if (!(ident = virIdentityNew())) goto cleanup; @@ -48,18 +49,18 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) if (virIdentitySetUserName(ident, "fred") < 0) goto cleanup; - if (virIdentityGetUserName(ident, &val) < 0) + if ((rc = virIdentityGetUserName(ident, &val)) < 0) goto cleanup; - if (STRNEQ_NULLABLE(val, "fred")) { + if (STRNEQ_NULLABLE(val, "fred") || rc != 1) { VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val)); goto cleanup; } - if (virIdentityGetGroupName(ident, &val) < 0) + if ((rc = virIdentityGetGroupName(ident, &val)) < 0) goto cleanup; - if (val != NULL) { + if (val != NULL || rc != 0) { VIR_DEBUG("Unexpected groupname attribute"); goto cleanup; } @@ -69,10 +70,10 @@ static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetUserName(ident, &val) < 0) + if ((rc = virIdentityGetUserName(ident, &val)) < 0) goto cleanup; - if (STRNEQ_NULLABLE(val, "fred")) { + if (STRNEQ_NULLABLE(val, "fred") || rc != 1) { VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val)); goto cleanup; } @@ -90,6 +91,7 @@ static int testIdentityGetSystem(const void *data) int ret = -1; virIdentityPtr ident = NULL; const char *val; + int rc; #if !WITH_SELINUX if (context) { @@ -104,13 +106,20 @@ static int testIdentityGetSystem(const void *data) goto cleanup; } - if (virIdentityGetSELinuxContext(ident, &val) < 0) + if ((rc = virIdentityGetSELinuxContext(ident, &val)) < 0) goto cleanup; - if (STRNEQ_NULLABLE(val, context)) { - VIR_DEBUG("Want SELinux context '%s' got '%s'", - context, val); - goto cleanup; + if (context == NULL) { + if (val != NULL || rc != 0) { + VIR_DEBUG("Unexpected SELinux context %s", NULLSTR(val)); + goto cleanup; + } + } else { + if (STRNEQ_NULLABLE(val, context) || rc != 1) { + VIR_DEBUG("Want SELinux context '%s' got '%s'", + context, val); + goto cleanup; + } } ret = 0; diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c index 3cd76f42ff..d094de9840 100644 --- a/tests/virnetserverclienttest.c +++ b/tests/virnetserverclienttest.c @@ -85,7 +85,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetUserName(ident, &gotUsername) < 0) { + if (virIdentityGetUserName(ident, &gotUsername) <= 0) { fprintf(stderr, "Missing username in identity\n"); goto cleanup; } @@ -95,7 +95,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetUNIXUserID(ident, &gotUserID) < 0) { + if (virIdentityGetUNIXUserID(ident, &gotUserID) <= 0) { fprintf(stderr, "Missing user ID in identity\n"); goto cleanup; } @@ -105,7 +105,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetGroupName(ident, &gotGroupname) < 0) { + if (virIdentityGetGroupName(ident, &gotGroupname) <= 0) { fprintf(stderr, "Missing groupname in identity\n"); goto cleanup; } @@ -115,7 +115,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetUNIXGroupID(ident, &gotGroupID) < 0) { + if (virIdentityGetUNIXGroupID(ident, &gotGroupID) <= 0) { fprintf(stderr, "Missing group ID in identity\n"); goto cleanup; } @@ -125,7 +125,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virIdentityGetSELinuxContext(ident, &gotSELinuxContext) < 0) { + if (virIdentityGetSELinuxContext(ident, &gotSELinuxContext) <= 0) { fprintf(stderr, "Missing SELinux context in identity\n"); goto cleanup; } -- 2.21.0
We'll shortly be exposing the identity as virTypedParameter in the public header, so it simplifies life to use that as the internal representation too. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viridentity.c | 377 ++++++++++++++++++----------------------- 1 file changed, 166 insertions(+), 211 deletions(-) diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 964a33d339..30621ca2a4 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -41,24 +41,12 @@ VIR_LOG_INIT("util.identity"); -typedef enum { - VIR_IDENTITY_ATTR_USER_NAME, - VIR_IDENTITY_ATTR_UNIX_USER_ID, - VIR_IDENTITY_ATTR_GROUP_NAME, - VIR_IDENTITY_ATTR_UNIX_GROUP_ID, - VIR_IDENTITY_ATTR_PROCESS_ID, - VIR_IDENTITY_ATTR_PROCESS_TIME, - VIR_IDENTITY_ATTR_SASL_USER_NAME, - VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, - VIR_IDENTITY_ATTR_SELINUX_CONTEXT, - - VIR_IDENTITY_ATTR_LAST, -} virIdentityAttrType; - struct _virIdentity { virObject parent; - char *attrs[VIR_IDENTITY_ATTR_LAST]; + int nparams; + int maxparams; + virTypedParameterPtr params; }; static virClassPtr virIdentityClass; @@ -229,67 +217,8 @@ virIdentityPtr virIdentityNew(void) static void virIdentityDispose(void *object) { virIdentityPtr ident = object; - size_t i; - - for (i = 0; i < VIR_IDENTITY_ATTR_LAST; i++) - VIR_FREE(ident->attrs[i]); -} - - -/** - * virIdentitySetAttr: - * @ident: the identity to modify - * @attr: the attribute type to set - * @value: the identifying value to associate with @attr - * - * Sets an identifying attribute @attr on @ident. Each - * @attr type can only be set once. - * - * Returns: 0 on success, or -1 on error - */ -static int -virIdentitySetAttr(virIdentityPtr ident, - unsigned int attr, - const char *value) -{ - int ret = -1; - VIR_DEBUG("ident=%p attribute=%u value=%s", ident, attr, value); - - if (ident->attrs[attr]) { - virReportError(VIR_ERR_OPERATION_DENIED, "%s", - _("Identity attribute is already set")); - goto cleanup; - } - - if (VIR_STRDUP(ident->attrs[attr], value) < 0) - goto cleanup; - ret = 0; - - cleanup: - return ret; -} - - -/** - * virIdentityGetAttr: - * @ident: the identity to query - * @attr: the attribute to read - * @value: filled with the attribute value - * - * Fills @value with a pointer to the value associated - * with the identifying attribute @attr in @ident. If - * @attr is not set, then it will simply be initialized - * to NULL and considered as a successful read - */ -static void -virIdentityGetAttr(virIdentityPtr ident, - unsigned int attr, - const char **value) -{ - VIR_DEBUG("ident=%p attribute=%d value=%p", ident, attr, value); - - *value = ident->attrs[attr]; + virTypedParamsFree(ident->params, ident->nparams); } @@ -299,14 +228,11 @@ virIdentityGetAttr(virIdentityPtr ident, int virIdentityGetUserName(virIdentityPtr ident, const char **username) { - virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_USER_NAME, - username); - - if (!*username) - return 0; - - return 1; + *username = NULL; + return virTypedParamsGetString(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_USER_NAME, + username); } @@ -316,21 +242,16 @@ int virIdentityGetUserName(virIdentityPtr ident, int virIdentityGetUNIXUserID(virIdentityPtr ident, uid_t *uid) { - int val; - const char *userid; + unsigned long long val; + int rc; *uid = -1; - virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_ID, - &userid); - if (!userid) - return 0; - - if (virStrToLong_i(userid, NULL, 10, &val) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse user ID '%s'"), userid); - return -1; - } + rc = virTypedParamsGetULLong(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_UNIX_USER_ID, + &val); + if (rc <= 0) + return rc; *uid = (uid_t)val; @@ -344,14 +265,11 @@ int virIdentityGetUNIXUserID(virIdentityPtr ident, int virIdentityGetGroupName(virIdentityPtr ident, const char **groupname) { - virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_GROUP_NAME, - groupname); - - if (!*groupname) - return 0; - - return 1; + *groupname = NULL; + return virTypedParamsGetString(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_GROUP_NAME, + groupname); } @@ -361,22 +279,16 @@ int virIdentityGetGroupName(virIdentityPtr ident, int virIdentityGetUNIXGroupID(virIdentityPtr ident, gid_t *gid) { - int val; - const char *groupid; + unsigned long long val; + int rc; *gid = -1; - virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_GROUP_ID, - &groupid); - - if (!groupid) - return 0; - - if (virStrToLong_i(groupid, NULL, 10, &val) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse group ID '%s'"), groupid); - return -1; - } + rc = virTypedParamsGetULLong(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_UNIX_GROUP_ID, + &val); + if (rc <= 0) + return rc; *gid = (gid_t)val; @@ -390,22 +302,16 @@ int virIdentityGetUNIXGroupID(virIdentityPtr ident, int virIdentityGetProcessID(virIdentityPtr ident, pid_t *pid) { - unsigned long long val; - const char *processid; + long long val; + int rc; *pid = 0; - virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_PROCESS_ID, - &processid); - - if (!processid) - return 0; - - if (virStrToLong_ull(processid, NULL, 10, &val) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse process ID '%s'"), processid); - return -1; - } + rc = virTypedParamsGetLLong(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_PROCESS_ID, + &val); + if (rc <= 0) + return rc; *pid = (pid_t)val; @@ -419,23 +325,11 @@ int virIdentityGetProcessID(virIdentityPtr ident, int virIdentityGetProcessTime(virIdentityPtr ident, unsigned long long *timestamp) { - const char *processtime; - *timestamp = 0; - virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_PROCESS_TIME, - &processtime); - - if (!processtime) - return 0; - - if (virStrToLong_ull(processtime, NULL, 10, timestamp) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse process time '%s'"), processtime); - return -1; - } - - return 1; + return virTypedParamsGetULLong(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_PROCESS_TIME, + timestamp); } @@ -445,14 +339,11 @@ int virIdentityGetProcessTime(virIdentityPtr ident, int virIdentityGetSASLUserName(virIdentityPtr ident, const char **username) { - virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_SASL_USER_NAME, - username); - - if (!*username) - return 0; - - return 1; + *username = NULL; + return virTypedParamsGetString(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_SASL_USER_NAME, + username); } @@ -462,14 +353,11 @@ int virIdentityGetSASLUserName(virIdentityPtr ident, int virIdentityGetX509DName(virIdentityPtr ident, const char **dname) { - virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, - dname); - - if (!*dname) - return 0; - - return 1; + *dname = NULL; + return virTypedParamsGetString(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_X509_DISTINGUISHED_NAME, + dname); } @@ -479,88 +367,125 @@ int virIdentityGetX509DName(virIdentityPtr ident, int virIdentityGetSELinuxContext(virIdentityPtr ident, const char **context) { - virIdentityGetAttr(ident, - VIR_IDENTITY_ATTR_SELINUX_CONTEXT, - context); - - if (!*context) - return 0; - - return 1; + *context = NULL; + return virTypedParamsGetString(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_SELINUX_CONTEXT, + context); } int virIdentitySetUserName(virIdentityPtr ident, const char *username) { - return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_USER_NAME, - username); + if (virTypedParamsGet(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_USER_NAME)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Identity attribute is already set")); + return -1; + } + + return virTypedParamsAddString(&ident->params, + &ident->nparams, + &ident->maxparams, + VIR_CONNECT_IDENTITY_USER_NAME, + username); } int virIdentitySetUNIXUserID(virIdentityPtr ident, uid_t uid) { - VIR_AUTOFREE(char *) val = NULL; - - if (virAsprintf(&val, "%d", (int)uid) < 0) + if (virTypedParamsGet(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_UNIX_USER_ID)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Identity attribute is already set")); return -1; + } - return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_USER_ID, - val); + return virTypedParamsAddULLong(&ident->params, + &ident->nparams, + &ident->maxparams, + VIR_CONNECT_IDENTITY_UNIX_USER_ID, + uid); } int virIdentitySetGroupName(virIdentityPtr ident, const char *groupname) { - return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_GROUP_NAME, - groupname); + if (virTypedParamsGet(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_GROUP_NAME)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Identity attribute is already set")); + return -1; + } + + return virTypedParamsAddString(&ident->params, + &ident->nparams, + &ident->maxparams, + VIR_CONNECT_IDENTITY_GROUP_NAME, + groupname); } int virIdentitySetUNIXGroupID(virIdentityPtr ident, gid_t gid) { - VIR_AUTOFREE(char *) val = NULL; - - if (virAsprintf(&val, "%d", (int)gid) < 0) + if (virTypedParamsGet(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_UNIX_GROUP_ID)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Identity attribute is already set")); return -1; + } - return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_UNIX_GROUP_ID, - val); + return virTypedParamsAddULLong(&ident->params, + &ident->nparams, + &ident->maxparams, + VIR_CONNECT_IDENTITY_UNIX_GROUP_ID, + gid); } int virIdentitySetProcessID(virIdentityPtr ident, pid_t pid) { - VIR_AUTOFREE(char *) val = NULL; - - if (virAsprintf(&val, "%lld", (long long) pid) < 0) + if (virTypedParamsGet(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_PROCESS_ID)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Identity attribute is already set")); return -1; + } - return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_PROCESS_ID, - val); + return virTypedParamsAddLLong(&ident->params, + &ident->nparams, + &ident->maxparams, + VIR_CONNECT_IDENTITY_PROCESS_ID, + pid); } int virIdentitySetProcessTime(virIdentityPtr ident, unsigned long long timestamp) { - VIR_AUTOFREE(char *) val = NULL; - - if (virAsprintf(&val, "%llu", timestamp) < 0) + if (virTypedParamsGet(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_PROCESS_TIME)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Identity attribute is already set")); return -1; + } - return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_PROCESS_TIME, - val); + return virTypedParamsAddULLong(&ident->params, + &ident->nparams, + &ident->maxparams, + VIR_CONNECT_IDENTITY_PROCESS_TIME, + timestamp); } @@ -568,25 +493,55 @@ int virIdentitySetProcessTime(virIdentityPtr ident, int virIdentitySetSASLUserName(virIdentityPtr ident, const char *username) { - return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_SASL_USER_NAME, - username); + if (virTypedParamsGet(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_SASL_USER_NAME)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Identity attribute is already set")); + return -1; + } + + return virTypedParamsAddString(&ident->params, + &ident->nparams, + &ident->maxparams, + VIR_CONNECT_IDENTITY_SASL_USER_NAME, + username); } int virIdentitySetX509DName(virIdentityPtr ident, const char *dname) { - return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME, - dname); + if (virTypedParamsGet(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_X509_DISTINGUISHED_NAME)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Identity attribute is already set")); + return -1; + } + + return virTypedParamsAddString(&ident->params, + &ident->nparams, + &ident->maxparams, + VIR_CONNECT_IDENTITY_X509_DISTINGUISHED_NAME, + dname); } int virIdentitySetSELinuxContext(virIdentityPtr ident, const char *context) { - return virIdentitySetAttr(ident, - VIR_IDENTITY_ATTR_SELINUX_CONTEXT, - context); + if (virTypedParamsGet(ident->params, + ident->nparams, + VIR_CONNECT_IDENTITY_SELINUX_CONTEXT)) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("Identity attribute is already set")); + return -1; + } + + return virTypedParamsAddString(&ident->params, + &ident->nparams, + &ident->maxparams, + VIR_CONNECT_IDENTITY_SELINUX_CONTEXT, + context); } -- 2.21.0
Add ability to import/export all the parameters associated with an identity, so that they can be exposed via the public API. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/viridentity.c | 56 ++++++++++++++++++++++++++++++++++++++++ src/util/viridentity.h | 8 ++++++ 3 files changed, 66 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fec1787497..a406aef0ae 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2144,6 +2144,7 @@ virHostGetBootTime; # util/viridentity.h virIdentityGetCurrent; virIdentityGetGroupName; +virIdentityGetParameters; virIdentityGetProcessID; virIdentityGetProcessTime; virIdentityGetSASLUserName; @@ -2156,6 +2157,7 @@ virIdentityGetX509DName; virIdentityNew; virIdentitySetCurrent; virIdentitySetGroupName; +virIdentitySetParameters; virIdentitySetProcessID; virIdentitySetProcessTime; virIdentitySetSASLUserName; diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 30621ca2a4..22e2644c19 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -36,6 +36,7 @@ #include "virutil.h" #include "virstring.h" #include "virprocess.h" +#include "virtypedparam.h" #define VIR_FROM_THIS VIR_FROM_IDENTITY @@ -545,3 +546,58 @@ int virIdentitySetSELinuxContext(virIdentityPtr ident, VIR_CONNECT_IDENTITY_SELINUX_CONTEXT, context); } + + +int virIdentitySetParameters(virIdentityPtr ident, + virTypedParameterPtr params, + int nparams) +{ + if (virTypedParamsValidate(params, nparams, + VIR_CONNECT_IDENTITY_USER_NAME, + VIR_TYPED_PARAM_STRING, + VIR_CONNECT_IDENTITY_UNIX_USER_ID, + VIR_TYPED_PARAM_ULLONG, + VIR_CONNECT_IDENTITY_GROUP_NAME, + VIR_TYPED_PARAM_STRING, + VIR_CONNECT_IDENTITY_UNIX_GROUP_ID, + VIR_TYPED_PARAM_ULLONG, + VIR_CONNECT_IDENTITY_PROCESS_ID, + VIR_TYPED_PARAM_LLONG, + VIR_CONNECT_IDENTITY_PROCESS_TIME, + VIR_TYPED_PARAM_ULLONG, + VIR_CONNECT_IDENTITY_SASL_USER_NAME, + VIR_TYPED_PARAM_STRING, + VIR_CONNECT_IDENTITY_X509_DISTINGUISHED_NAME, + VIR_TYPED_PARAM_STRING, + VIR_CONNECT_IDENTITY_SELINUX_CONTEXT, + VIR_TYPED_PARAM_STRING, + NULL) < 0) + return -1; + + virTypedParamsFree(ident->params, ident->nparams); + ident->params = NULL; + ident->nparams = 0; + ident->maxparams = 0; + if (virTypedParamsCopy(&ident->params, params, nparams) < 0) + return -1; + ident->nparams = nparams; + ident->maxparams = nparams; + + return 0; +} + + +int virIdentityGetParameters(virIdentityPtr ident, + virTypedParameterPtr *params, + int *nparams) +{ + *params = NULL; + *nparams = 0; + + if (virTypedParamsCopy(params, ident->params, ident->nparams) < 0) + return -1; + + *nparams = ident->nparams; + + return 0; +} diff --git a/src/util/viridentity.h b/src/util/viridentity.h index 805ad3ea4d..861ecca736 100644 --- a/src/util/viridentity.h +++ b/src/util/viridentity.h @@ -71,3 +71,11 @@ int virIdentitySetX509DName(virIdentityPtr ident, const char *dname); int virIdentitySetSELinuxContext(virIdentityPtr ident, const char *context); + +int virIdentitySetParameters(virIdentityPtr ident, + virTypedParameterPtr params, + int nparams); + +int virIdentityGetParameters(virIdentityPtr ident, + virTypedParameterPtr *params, + int *nparams); -- 2.21.0
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 | 112 +++++++++++++++++++++++++--- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +++- src/remote_protocol-structs | 8 ++ src/rpc/virnetserverclient.c | 12 +++ src/rpc/virnetserverclient.h | 2 + 7 files changed, 139 insertions(+), 13 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 ecde959088..dbd2985c38 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,16 @@ 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; + int ret = -1; + + VIR_DEBUG("Getting secondary uri=%s readonly=%d preserveIdent=%d conn=%p", + NULLSTR(uri), readonly, preserveIdentity, conn); + if (*conn) return 0; @@ -1957,16 +1964,43 @@ 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); - return 0; + if (preserveIdentity) { + if (virConnectSetIdentity(*conn, params, nparams, 0) < 0) + goto error; + + VIR_DEBUG("Forwarded current identity to secondary driver"); + } + + ret = 0; + cleanup: + virTypedParamsFree(params, nparams); + return ret; + + error: + if (*conn) { + virConnectClose(*conn); + *conn = NULL; + } + goto cleanup; } @@ -1993,6 +2027,7 @@ remoteGetInterfaceConn(virNetServerClientPtr client) if (remoteOpenConn(priv->interfaceURI, priv->readonly, + true, &priv->interfaceConn) < 0) return NULL; @@ -2008,6 +2043,7 @@ remoteGetNetworkConn(virNetServerClientPtr client) if (remoteOpenConn(priv->networkURI, priv->readonly, + true, &priv->networkConn) < 0) return NULL; @@ -2023,6 +2059,7 @@ remoteGetNodeDevConn(virNetServerClientPtr client) if (remoteOpenConn(priv->nodedevURI, priv->readonly, + true, &priv->nodedevConn) < 0) return NULL; @@ -2038,6 +2075,7 @@ remoteGetNWFilterConn(virNetServerClientPtr client) if (remoteOpenConn(priv->nwfilterURI, priv->readonly, + true, &priv->nwfilterConn) < 0) return NULL; @@ -2053,6 +2091,7 @@ remoteGetSecretConn(virNetServerClientPtr client) if (remoteOpenConn(priv->secretURI, priv->readonly, + true, &priv->secretConn) < 0) return NULL; @@ -2068,6 +2107,7 @@ remoteGetStorageConn(virNetServerClientPtr client) if (remoteOpenConn(priv->storageURI, priv->readonly, + true, &priv->storageConn) < 0) return NULL; @@ -2237,6 +2277,7 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED, #ifdef MODULE_NAME const char *type = NULL; #endif /* !MODULE_NAME */ + bool preserveIdentity = false; VIR_DEBUG("priv=%p conn=%p", priv, priv->conn); virMutexLock(&priv->lock); @@ -2264,16 +2305,16 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED, name = probeduri; } -#endif + + 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); #ifdef MODULE_NAME @@ -2384,6 +2425,53 @@ remoteDispatchConnectClose(virNetServerPtr server ATTRIBUTE_UNUSED, } +static int +remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_set_identity_args *args) +{ + virTypedParameterPtr params = NULL; + int nparams = 0; + int rv = -1; + virConnectPtr conn = remoteGetHypervisorConn(client); + VIR_AUTOUNREF(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); + 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 2b86f55035..8789c5da00 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8507,6 +8507,7 @@ static virHypervisorDriver hypervisor_driver = { .name = "remote", .connectOpen = remoteConnectOpen, /* 0.3.0 */ .connectClose = remoteConnectClose, /* 0.3.0 */ + .connectSetIdentity = remoteConnectSetIdentity, /* 5.8.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 75c2bc69ff..f4e3392212 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; @@ -3736,6 +3739,11 @@ struct remote_domain_get_guest_info_ret { remote_typed_param params<REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX>; }; +struct remote_connect_set_identity_args { + remote_typed_param params<REMOTE_CONNECT_IDENTITY_PARAMS_MAX>; + unsigned int flags; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -6603,5 +6611,11 @@ enum remote_procedure { * @generate: none * @acl: domain:write */ - REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418 + REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418, + + /** + * @generate: client + * @acl: connect:write + */ + REMOTE_PROC_CONNECT_SET_IDENTITY = 419 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 616c3d5d52..71169c4148 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -3115,6 +3115,13 @@ struct remote_domain_get_guest_info_ret { u_int params_len; remote_typed_param * params_val; } params; +} +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, @@ -3535,4 +3542,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_CHECKPOINT_GET_PARENT = 416, REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417, REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418, + REMOTE_PROC_CONNECT_SET_IDENTITY = 419, }; diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 1b96d0339b..171ee636dd 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
On 9/5/19 1:56 PM, Daniel P. Berrangé wrote:
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 | 112 +++++++++++++++++++++++++--- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +++- src/remote_protocol-structs | 8 ++ src/rpc/virnetserverclient.c | 12 +++ src/rpc/virnetserverclient.h | 2 + 7 files changed, 139 insertions(+), 13 deletions(-)
/* Define the program number, protocol version and procedure numbers here. */ @@ -6603,5 +6611,11 @@ enum remote_procedure { * @generate: none * @acl: domain:write */ - REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418 + REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418, + + /** + * @generate: client + * @acl: connect:write + */ + REMOTE_PROC_CONNECT_SET_IDENTITY = 419
IIUC, the only thing that stops a malicious user from switching identity is that they have write access on the opened connection? Because on one hand we want secondary daemons to accept identity switches from the proxy daemon, but at the same time we don't want users to do that. Code-wise, patches are good and I'd ACK them, but this is a bit unclear to me so I'd like to understand it more first.
}; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 616c3d5d52..71169c4148 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -3115,6 +3115,13 @@ struct remote_domain_get_guest_info_ret { u_int params_len; remote_typed_param * params_val; } params; +}
s/}/};/
+struct remote_connect_set_identity_args { + struct { + u_int params_len; + remote_typed_param * params_val; + } params; + u_int flags; };
Michal
On Fri, Sep 13, 2019 at 05:13:09PM +0200, Michal Privoznik wrote:
On 9/5/19 1:56 PM, Daniel P. Berrangé wrote:
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 | 112 +++++++++++++++++++++++++--- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +++- src/remote_protocol-structs | 8 ++ src/rpc/virnetserverclient.c | 12 +++ src/rpc/virnetserverclient.h | 2 + 7 files changed, 139 insertions(+), 13 deletions(-)
/* Define the program number, protocol version and procedure numbers here. */ @@ -6603,5 +6611,11 @@ enum remote_procedure { * @generate: none * @acl: domain:write */ - REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418 + REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418, + + /** + * @generate: client + * @acl: connect:write + */ + REMOTE_PROC_CONNECT_SET_IDENTITY = 419
IIUC, the only thing that stops a malicious user from switching identity is that they have write access on the opened connection?
We consider 'write' access as equivalent to root shell access, but perhaps there is none the less value in having an explicit permission bit for this operation.
Because on one hand we want secondary daemons to accept identity switches from the proxy daemon, but at the same time we don't want users to do that.
In an out of the box config with no ACLs defined in polkit, this will correctly operate. the proxy will get approved by default since it is runnig root & thus polkit grants access regardless. Everything else is denied by default. If polkit is not enabled, then the user can invoke this RPC, but of course without polkit your privileges are again eqiuv to root.
Code-wise, patches are good and I'd ACK them, but this is a bit unclear to me so I'd like to understand it more first.
}; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 616c3d5d52..71169c4148 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -3115,6 +3115,13 @@ struct remote_domain_get_guest_info_ret { u_int params_len; remote_typed_param * params_val; } params; +}
s/}/};/
+struct remote_connect_set_identity_args { + struct { + u_int params_len; + remote_typed_param * params_val; + } params; + u_int flags; };
Michal
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 :|
On 9/13/19 5:23 PM, Daniel P. Berrangé wrote:
On Fri, Sep 13, 2019 at 05:13:09PM +0200, Michal Privoznik wrote:
On 9/5/19 1:56 PM, Daniel P. Berrangé wrote:
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 | 112 +++++++++++++++++++++++++--- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +++- src/remote_protocol-structs | 8 ++ src/rpc/virnetserverclient.c | 12 +++ src/rpc/virnetserverclient.h | 2 + 7 files changed, 139 insertions(+), 13 deletions(-)
/* Define the program number, protocol version and procedure numbers here. */ @@ -6603,5 +6611,11 @@ enum remote_procedure { * @generate: none * @acl: domain:write */ - REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418 + REMOTE_PROC_DOMAIN_GET_GUEST_INFO = 418, + + /** + * @generate: client + * @acl: connect:write + */ + REMOTE_PROC_CONNECT_SET_IDENTITY = 419
IIUC, the only thing that stops a malicious user from switching identity is that they have write access on the opened connection?
We consider 'write' access as equivalent to root shell access, but perhaps there is none the less value in having an explicit permission bit for this operation.
Agreed, you can post it in as a follow up patch.
Because on one hand we want secondary daemons to accept identity switches from the proxy daemon, but at the same time we don't want users to do that.
In an out of the box config with no ACLs defined in polkit, this will correctly operate. the proxy will get approved by default since it is runnig root & thus polkit grants access regardless. Everything else is denied by default.
If polkit is not enabled, then the user can invoke this RPC, but of course without polkit your privileges are again eqiuv to root.
Right, this is exactly what I though. Alright, Reviewed-by: Michal Privoznik <mprivozn@redhat.com> to the whole patchset. Michal
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik