[libvirt PATCH 0/6] polkit: Make it possible to write safer rules
See patch 5/6 for details. Andrea Bolognani (6): docs: The Polkit access driver is disabled by default docs: Document org.libvirt.unix.* actions rpc: Introduce virNetServerHasGranularPolkit() remote: Set granularPolkit if applicable remote: Expose granularPolkit attribute to rules docs: Document granularPolkit attribute docs/aclpolkit.rst | 33 +++++++++++++++++++++++++++++ src/libvirt_remote.syms | 1 + src/locking/lock_daemon.c | 6 ++++-- src/logging/log_daemon.c | 6 ++++-- src/lxc/lxc_controller.c | 3 ++- src/remote/remote_daemon.c | 24 +++++++++++++++++---- src/remote/remote_daemon_dispatch.c | 6 +++++- src/rpc/virnetserver.c | 16 ++++++++++++-- src/rpc/virnetserver.h | 4 +++- tests/virnetdaemontest.c | 3 ++- 10 files changed, 88 insertions(+), 14 deletions(-) -- 2.42.0
This might not be immediately obvious to someone who ended up on the page without passing through acl.html first. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/aclpolkit.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/aclpolkit.rst b/docs/aclpolkit.rst index 07f4735001..a9e2a305a5 100644 --- a/docs/aclpolkit.rst +++ b/docs/aclpolkit.rst @@ -23,6 +23,13 @@ all APIs, or just read-only access. The polkit access control driver in libvirt builds on this capability to allow for fine grained control over the operations a user may perform on an object. +Enabling the polkit driver +-------------------------- + +The polkit driver is disabled by default. The `access control +<acl.html#access-control-drivers>`__ documentation includes +information on how to enable it. + Permission names ---------------- -- 2.42.0
Before any of the API can be executed, the client needs to be authenticated by allowing one of these special actions. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/aclpolkit.rst | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/aclpolkit.rst b/docs/aclpolkit.rst index a9e2a305a5..9b0a374c53 100644 --- a/docs/aclpolkit.rst +++ b/docs/aclpolkit.rst @@ -53,6 +53,23 @@ The default policy for any permission which corresponds to a "read only" operation, is to allow access. All other permissions default to deny access. +Special actions +--------------- + +In addition to the various ``org.libvirt.api.*`` actions mentioned +above, each of which corresponds to a specific API call, there are +two more actions that can be allowed or rejected via Polkit rules: + + * ``org.libvirt.unix.monitor`` for read-only access to the API; + * ``org.libvirt.unix.manage`` for read/write access. + +When a user connects to the daemon locally (or through the ssh +transport), the appropriate ``org.libvirt.unix.*`` action will need +to be approved by Polkit before any further APIs can be called. + +Read-only access is granted to all local users by default, but +read/write access needs to be explicitly allowed. + Object identity attributes -------------------------- -- 2.42.0
It's always set to false for now. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/libvirt_remote.syms | 1 + src/locking/lock_daemon.c | 6 ++++-- src/logging/log_daemon.c | 6 ++++-- src/lxc/lxc_controller.c | 3 ++- src/remote/remote_daemon.c | 6 ++++-- src/rpc/virnetserver.c | 16 ++++++++++++++-- src/rpc/virnetserver.h | 4 +++- tests/virnetdaemontest.c | 3 ++- 8 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index f0f90815cf..16a8adcdcc 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -136,6 +136,7 @@ virNetServerGetMaxUnauthClients; virNetServerGetName; virNetServerGetThreadPoolParameters; virNetServerHasClients; +virNetServerHasGranularPolkit; virNetServerNeedsAuth; virNetServerNew; virNetServerNewPostExecRestart; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index ba52ce7d77..ac44c80927 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -129,7 +129,8 @@ virLockDaemonNew(virLockDaemonConfig *config, bool privileged) virLockDaemonClientNew, virLockDaemonClientPreExecRestart, virLockDaemonClientFree, - (void*)(intptr_t)(privileged ? 0x1 : 0x0)))) + (void*)(intptr_t)(privileged ? 0x1 : 0x0), + false))) goto error; if (virNetDaemonAddServer(lockd->dmn, srv) < 0) @@ -142,7 +143,8 @@ virLockDaemonNew(virLockDaemonConfig *config, bool privileged) remoteAdmClientNew, remoteAdmClientPreExecRestart, remoteAdmClientFree, - lockd->dmn))) + lockd->dmn, + false))) goto error; if (virNetDaemonAddServer(lockd->dmn, srv) < 0) diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c index daf7ef4b2f..752f4bd7b6 100644 --- a/src/logging/log_daemon.c +++ b/src/logging/log_daemon.c @@ -124,7 +124,8 @@ virLogDaemonNew(virLogDaemonConfig *config, bool privileged) virLogDaemonClientNew, virLogDaemonClientPreExecRestart, virLogDaemonClientFree, - (void*)(intptr_t)(privileged ? 0x1 : 0x0)))) + (void*)(intptr_t)(privileged ? 0x1 : 0x0), + false))) goto error; if (virNetDaemonAddServer(logd->dmn, srv) < 0) @@ -137,7 +138,8 @@ virLogDaemonNew(virLogDaemonConfig *config, bool privileged) remoteAdmClientNew, remoteAdmClientPreExecRestart, remoteAdmClientFree, - logd->dmn))) + logd->dmn, + false))) goto error; if (virNetDaemonAddServer(logd->dmn, srv) < 0) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 48f5c73fce..54409d6961 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -928,7 +928,8 @@ static int virLXCControllerSetupServer(virLXCController *ctrl) virLXCControllerClientPrivateNew, NULL, virLXCControllerClientPrivateFree, - ctrl))) + ctrl, + false))) goto error; if (virSecurityManagerSetSocketLabel(ctrl->securityManager, ctrl->def) < 0) diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 657c053f6f..59170373cb 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -1035,7 +1035,8 @@ int main(int argc, char **argv) { remoteClientNew, NULL, remoteClientFree, - NULL))) { + NULL, + false))) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } @@ -1102,7 +1103,8 @@ int main(int argc, char **argv) { remoteAdmClientNew, NULL, remoteAdmClientFree, - dmn))) { + dmn, + false))) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 770476c1a6..d8c91172d0 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -71,6 +71,8 @@ struct _virNetServer { virNetServerClientPrivPreExecRestart clientPrivPreExecRestart; virFreeCallback clientPrivFree; void *clientPrivOpaque; + + bool granularPolkit; }; @@ -365,7 +367,8 @@ virNetServerNew(const char *name, virNetServerClientPrivNew clientPrivNew, virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, virFreeCallback clientPrivFree, - void *clientPrivOpaque) + void *clientPrivOpaque, + bool granularPolkit) { g_autoptr(virNetServer) srv = NULL; g_autofree char *jobName = g_strdup_printf("rpc-%s", name); @@ -402,6 +405,8 @@ virNetServerNew(const char *name, srv->clientPrivFree = clientPrivFree; srv->clientPrivOpaque = clientPrivOpaque; + srv->granularPolkit = granularPolkit; + return g_steal_pointer(&srv); } @@ -486,7 +491,7 @@ virNetServerNewPostExecRestart(virJSONValue *object, max_anonymous_clients, keepaliveInterval, keepaliveCount, clientPrivNew, clientPrivPreExecRestart, - clientPrivFree, clientPrivOpaque))) + clientPrivFree, clientPrivOpaque, false))) return NULL; if (!(services = virJSONValueObjectGet(object, "services"))) { @@ -988,6 +993,13 @@ virNetServerGetName(virNetServer *srv) } +bool +virNetServerHasGranularPolkit(virNetServer *srv) +{ + return srv->granularPolkit; +} + + int virNetServerGetThreadPoolParameters(virNetServer *srv, size_t *minWorkers, diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 9f0cf3a3fc..efdfab03b8 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -41,7 +41,8 @@ virNetServer *virNetServerNew(const char *name, virNetServerClientPrivNew clientPrivNew, virNetServerClientPrivPreExecRestart clientPrivPreExecRestart, virFreeCallback clientPrivFree, - void *clientPrivOpaque) + void *clientPrivOpaque, + bool granularPolkit) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(10) ATTRIBUTE_NONNULL(12); virNetServer *virNetServerNewPostExecRestart(virJSONValue *object, @@ -100,6 +101,7 @@ void virNetServerSetClientAuthenticated(virNetServer *srv, virNetServerClient *c void virNetServerUpdateServices(virNetServer *srv, bool enabled); const char *virNetServerGetName(virNetServer *srv); +bool virNetServerHasGranularPolkit(virNetServer *srv); int virNetServerGetThreadPoolParameters(virNetServer *srv, size_t *minWorkers, diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c index 110ec748f8..a52f427d89 100644 --- a/tests/virnetdaemontest.c +++ b/tests/virnetdaemontest.c @@ -104,7 +104,8 @@ testCreateServer(const char *server_name, const char *host, int family) testClientNew, testClientPreExec, testClientFree, - NULL))) + NULL, + false))) goto error; if (!(svc1 = virNetServerServiceNewTCP(host, -- 2.42.0
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/remote/remote_daemon.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 59170373cb..fc5e543470 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -411,16 +411,29 @@ daemonSetupNetDevOpenvswitch(struct daemonConfig *config) static int -daemonSetupAccessManager(struct daemonConfig *config) +daemonSetupAccessManager(struct daemonConfig *config, + bool *granularPolkit) { virAccessManager *mgr; const char *none[] = { "none", NULL }; const char **drv = (const char **)config->access_drivers; + const char **iter; if (!drv || !drv[0]) drv = none; + /* We only declare support for granular Polkit access when Polkit + * is the only configured access driver, to avoid scenarios in + * which the Polkit policy would deny access to a certain action + * but another (possibly misconfigured) driver would allow it */ + *granularPolkit = false; + iter = drv; + while (*iter) { + *granularPolkit = STREQ(*iter, "polkit"); + iter++; + } + if (!(mgr = virAccessManagerNewStack(drv))) return -1; @@ -805,6 +818,7 @@ int main(int argc, char **argv) { bool implicit_conf = false; char *run_dir = NULL; mode_t old_umask; + bool granularPolkit = false; struct option opts[] = { { "verbose", no_argument, &verbose, 'v' }, @@ -946,7 +960,7 @@ int main(int argc, char **argv) { daemonSetupNetDevOpenvswitch(config); - if (daemonSetupAccessManager(config) < 0) { + if (daemonSetupAccessManager(config, &granularPolkit) < 0) { VIR_ERROR(_("Can't initialize access manager")); exit(EXIT_FAILURE); } @@ -1036,7 +1050,7 @@ int main(int argc, char **argv) { NULL, remoteClientFree, NULL, - false))) { + granularPolkit))) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } -- 2.42.0
This makes it possible to write Polkit rules that won't accidentally grant undesired privileges to users. To understand why this is necessary, suppose we wanted to grant user "fred" full access to the QEMU domain "demo". A JavaScript rule along the lines of polkit.addRule(function(action, subject) { // user "fred" if (subject.user == "fred") { // can authenticate in read/write mode if (action.id == "org.libvirt.unix.manage") { return polkit.Result.YES; } // and manage the QEMU domain "demo" if (action.id.indexOf("org.libvirt.api.domain.") == 0 && action.lookup("connect_driver") == "QEMU" && action.lookup("domain_name") == "demo") { return polkit.Result.YES; } } }); would do the trick. However, suppose that at some point after creating this rule we disabled the Polkit access control driver and forgot to delete the file. All of a sudden, allowing "org.libvirt.unix.manage" is no longer a trivial matter: since the Polkit access driver doesn't broker access to subsequent API calls anymore, user "fred" now has full administrative access to all drivers. Rewriting the check seen above as if (action.id == "org.libvirt.unix.manage" && action.lookup("granular") == "true") { return polkit.Result.YES; } ensures that this undesired scenario will not happen, by only allowing "org.libvirt.unix.manage" when the Polkit access driver is enabled. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/remote/remote_daemon_dispatch.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 7daf503b51..2a9ee19cc3 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3975,6 +3975,10 @@ remoteDispatchAuthPolkit(virNetServer *server, uid_t callerUid = -1; unsigned long long timestamp; const char *action; + const char *attrs[] = { + "granular", virNetServerHasGranularPolkit(server) ? "true" : "false", + NULL, + }; char *ident = NULL; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -4009,7 +4013,7 @@ remoteDispatchAuthPolkit(virNetServer *server, callerPid, timestamp, callerUid, - NULL, + attrs, true); if (rv == -1) goto authfail; -- 2.42.0
On Mon, Nov 20, 2023 at 03:49:56PM +0100, Andrea Bolognani wrote:
This makes it possible to write Polkit rules that won't accidentally grant undesired privileges to users.
To understand why this is necessary, suppose we wanted to grant user "fred" full access to the QEMU domain "demo".
A JavaScript rule along the lines of
polkit.addRule(function(action, subject) {
// user "fred" if (subject.user == "fred") {
// can authenticate in read/write mode if (action.id == "org.libvirt.unix.manage") { return polkit.Result.YES; }
// and manage the QEMU domain "demo" if (action.id.indexOf("org.libvirt.api.domain.") == 0 && action.lookup("connect_driver") == "QEMU" && action.lookup("domain_name") == "demo") { return polkit.Result.YES; } } });
would do the trick.
However, suppose that at some point after creating this rule we disabled the Polkit access control driver and forgot to delete the file.
All of a sudden, allowing "org.libvirt.unix.manage" is no longer a trivial matter: since the Polkit access driver doesn't broker access to subsequent API calls anymore, user "fred" now has full administrative access to all drivers.
Rewriting the check seen above as
if (action.id == "org.libvirt.unix.manage" && action.lookup("granular") == "true") { return polkit.Result.YES; }
ensures that this undesired scenario will not happen, by only allowing "org.libvirt.unix.manage" when the Polkit access driver is enabled.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/remote/remote_daemon_dispatch.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 7daf503b51..2a9ee19cc3 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3975,6 +3975,10 @@ remoteDispatchAuthPolkit(virNetServer *server, uid_t callerUid = -1; unsigned long long timestamp; const char *action; + const char *attrs[] = { + "granular", virNetServerHasGranularPolkit(server) ? "true" : "false", + NULL,
We potentially have a list of access drivers available, and I think we should expose each of their names as an attr 'access_driver.$NAME' IOW, if access_drivers = [], then don't expose any attr, but if access_drivers = ["polkit", "selinux"] we would set "access_driver.polkit=true" "access_driver.selinux=true" With 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 :|
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/aclpolkit.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/aclpolkit.rst b/docs/aclpolkit.rst index 9b0a374c53..fe825c504b 100644 --- a/docs/aclpolkit.rst +++ b/docs/aclpolkit.rst @@ -70,6 +70,15 @@ to be approved by Polkit before any further APIs can be called. Read-only access is granted to all local users by default, but read/write access needs to be explicitly allowed. +:since:`Since 9.10.0`, these requests will come with the ``granular`` +attribute (see below) set to either ``"true"``, if the Polkit access +driver is enabled, or ``"false"`` otherwise. A policy designed to +work with the Polkit access driver should only allow the +``org.libvirt.unix.manage`` action if the ``granular`` attribute is +set to ``"true"``: failing to do so might result in accidentally +granting full administrative access to libvirt to more users than +intended if the Polkit access driver is later disabled. + Object identity attributes -------------------------- -- 2.42.0
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé