It only redundantly reflects whether pkagent != NULL. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- tools/virsh.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b9f3f851d3ec..da35c5c2b9c1 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -119,7 +119,6 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) bool keepalive_forced = false; virPolkitAgent *pkagent = NULL; int authfail = 0; - bool agentCreated = false; if (ctl->keepalive_interval >= 0) { interval = ctl->keepalive_interval; @@ -141,12 +140,11 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) goto cleanup; err = virGetLastError(); - if (!agentCreated && + if (!pkagent && err && err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_UNAVAILABLE) { - if (!pkagent && !(pkagent = virPolkitAgentCreate())) + if (!(pkagent = virPolkitAgentCreate())) goto cleanup; - agentCreated = true; } else if (err && err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_FAILED) { authfail++; -- 2.34.0

Automatic "Ptr " -> " *" also wreaked havoc in comments. Fix it and while at it reword the sentence so it is clear that the object is newly allocated. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/util/virpolkit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index e90b3b871d15..86255a96760f 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -168,7 +168,7 @@ virPolkitAgentDestroy(virPolkitAgent *agent) * * Allocate and setup a polkit agent * - * Returns a virCommand *on success and NULL on failure + * Returns newly allocated virPolkitAgent * on success and NULL on failure */ virPolkitAgent * virPolkitAgentCreate(void) -- 2.34.0

With this function we can decide whether to try running the polkit text agent only if it is available, removing a potential needless error saying that the agent binary does not exist, which is useful especially when running the agent before knowing whether it is going to be needed. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/util/virpolkit.c | 44 ++++++++++++++++++++++++++++++++++++++++ src/util/virpolkit.h | 1 + 3 files changed, 46 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7bc50a4d16d..c11be4eafa19 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3078,6 +3078,7 @@ virPidFileWritePath; # util/virpolkit.h +virPolkitAgentAvailable; virPolkitAgentCreate; virPolkitAgentDestroy; virPolkitCheckAuth; diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 86255a96760f..3b333547d70b 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -20,6 +20,7 @@ */ #include <config.h> +#include <fcntl.h> #include <unistd.h> #include "virpolkit.h" @@ -217,6 +218,42 @@ virPolkitAgentCreate(void) } +/* + * virPolkitAgentAvailable + * + * This function does some preliminary checking that the pkttyagent does not + * fail starting so that it can be started without waiting for first failed + * connection with VIR_ERR_AUTH_UNAVAILABLE. + */ +bool +virPolkitAgentAvailable(void) +{ + const char *termid = ctermid(NULL); + VIR_AUTOCLOSE fd = -1; + + if (!virFileExists(PKTTYAGENT)) + return false; + + if (!termid) + return false; + + /* + *The pkttyagent needs to open the controlling terminal. + * + * Just in case we are running without a ctty make sure this open() does not + * change that. + * + * We could check if our session has a controlling terminal available + * instead, but it would require parsing `/proc/self/stat` on Linux, which + * is not portable and moreover requires way more work than just open(). + */ + fd = open(termid, O_RDWR | O_NOCTTY); + if (fd < 0) + return false; + + return true; +} + #else /* ! WITH_POLKIT */ int virPolkitCheckAuth(const char *actionid G_GNUC_UNUSED, @@ -247,4 +284,11 @@ virPolkitAgentCreate(void) _("polkit text authentication agent unavailable")); return NULL; } + +bool +virPolkitAgentAvailable(void) +{ + return false; +} + #endif /* WITH_POLKIT */ diff --git a/src/util/virpolkit.h b/src/util/virpolkit.h index a577d59452ba..7bcd040e5e06 100644 --- a/src/util/virpolkit.h +++ b/src/util/virpolkit.h @@ -37,3 +37,4 @@ typedef struct _virPolkitAgent virPolkitAgent; void virPolkitAgentDestroy(virPolkitAgent *cmd); virPolkitAgent *virPolkitAgentCreate(void); +bool virPolkitAgentAvailable(void); -- 2.34.0

On Fri, Dec 10, 2021 at 05:48:03PM -0700, Jim Fehlig wrote: With a polkit access driver, without a polkit agent already running, and without a polkit rule allowing access without authentication? Well, pkttyagent does not use stdin at all. It checks the controlling terminal and then opens that to be used for both stdout and stdin. That's why I tried to make it work. The thing is that I got our test suite failing because we had the opposite problem. Meson is running tests in a new session without a controlling terminal, however with stdin normally accessible. Virsh tries to run it and it fails every time. I guess there is no harm in also checking if stdin is a tty, I'll send a patch for that. Thanks for letting me know!

My idea was that running pkttyagent unconditionally, modulo checks that pkttyagent itself does to make sure it does not fail, is not going to be an issue turned out to be wrong. Adding back the original check for stdin being a tty helps in some testing scenarios as reported by Jim Fehlig and does not really cause any issues. I originally wanted it in because it also made pkttyagent auth work with redirected input into virsh (with a connection that requires polkit authentication and without a session-wide polkit tty agent, basically making pkttyagent necessary to succeed). But anyone running virsh like that is asking for problems already anyway =) Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/util/virpolkit.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 8970de192fe1..bb68b5a59614 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -237,6 +237,9 @@ virPolkitAgentAvailable(void) const char *termid = ctermid(NULL); VIR_AUTOCLOSE fd = -1; + if (!isatty(STDIN_FILENO)) + return false; + if (!virFileIsExecutable(PKTTYAGENT)) return false; -- 2.34.1

The reason for this is twofold: - the polkit build option is documented for UNIX socket access checks - there is no server-side change or dbus call done when enabling this as it only starts a polkit agent on the client-side (actually only in virsh) and does not need any requirements (starting is skipped if pkttyagent is not installed) Also move the conditional implementation to the bottom of the file so that it does not look like the whole file is build conditionally and the common functions are at the top. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/util/virpolkit.c | 240 ++++++++++++++++++++----------------------- 1 file changed, 109 insertions(+), 131 deletions(-) diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 7156adc10c0a..b51104100796 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -21,6 +21,7 @@ #include <config.h> #include <fcntl.h> +#include <poll.h> #include <unistd.h> #include "virpolkit.h" @@ -37,119 +38,10 @@ VIR_LOG_INIT("util.polkit"); -#if WITH_POLKIT -# include <poll.h> - struct _virPolkitAgent { virCommand *cmd; }; -/* - * virPolkitCheckAuth: - * @actionid: permission to check - * @pid: client process ID - * @startTime: process start time, or 0 - * @uid: client process user ID - * @details: NULL terminated (key, value) pair list - * @allowInteraction: true if auth prompts are allowed - * - * Check if a client is authenticated with polkit - * - * Returns 0 on success, -1 on failure, -2 on auth denied - */ -int virPolkitCheckAuth(const char *actionid, - pid_t pid, - unsigned long long startTime, - uid_t uid, - const char **details, - bool allowInteraction) -{ - GDBusConnection *sysbus; - GVariantBuilder builder; - GVariant *gprocess = NULL; - GVariant *gdetails = NULL; - g_autoptr(GVariant) message = NULL; - g_autoptr(GVariant) reply = NULL; - g_autoptr(GVariantIter) iter = NULL; - char *retkey; - char *retval; - gboolean is_authorized; - gboolean is_challenge; - bool is_dismissed = false; - const char **next; - - if (!(sysbus = virGDBusGetSystemBus())) - return -1; - - VIR_INFO("Checking PID %lld running as %d", - (long long) pid, uid); - - g_variant_builder_init(&builder, G_VARIANT_TYPE("a{sv}")); - g_variant_builder_add(&builder, "{sv}", "pid", g_variant_new_uint32(pid)); - g_variant_builder_add(&builder, "{sv}", "start-time", g_variant_new_uint64(startTime)); - g_variant_builder_add(&builder, "{sv}", "uid", g_variant_new_int32(uid)); - gprocess = g_variant_builder_end(&builder); - - g_variant_builder_init(&builder, G_VARIANT_TYPE("a{ss}")); - - if (details) { - for (next = details; *next; next++) { - const char *detail1 = *(next++); - const char *detail2 = *next; - g_variant_builder_add(&builder, "{ss}", detail1, detail2); - } - } - - gdetails = g_variant_builder_end(&builder); - - message = g_variant_new("((s@a{sv})s@a{ss}us)", - "unix-process", - gprocess, - actionid, - gdetails, - allowInteraction, - "" /* cancellation ID */); - - if (virGDBusCallMethod(sysbus, - &reply, - G_VARIANT_TYPE("((bba{ss}))"), - NULL, - "org.freedesktop.PolicyKit1", - "/org/freedesktop/PolicyKit1/Authority", - "org.freedesktop.PolicyKit1.Authority", - "CheckAuthorization", - message) < 0) - return -1; - - g_variant_get(reply, "((bba{ss}))", &is_authorized, &is_challenge, &iter); - - while (g_variant_iter_loop(iter, "{ss}", &retkey, &retval)) { - if (STREQ(retkey, "polkit.dismissed") && STREQ(retval, "true")) - is_dismissed = true; - } - - VIR_DEBUG("is auth %d is challenge %d", - is_authorized, is_challenge); - - if (is_authorized) - return 0; - - if (is_dismissed) { - virReportError(VIR_ERR_AUTH_CANCELLED, "%s", - _("user cancelled authentication process")); - } else if (is_challenge) { - virReportError(VIR_ERR_AUTH_UNAVAILABLE, - _("no polkit agent available to authenticate action '%s'"), - actionid); - } else { - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("access denied by policy")); - } - - return -2; -} - - /* virPolkitAgentDestroy: * @cmd: Pointer to the virCommand * created during virPolkitAgentCreate * @@ -260,6 +152,114 @@ virPolkitAgentAvailable(void) return true; } + +#if WITH_POLKIT + +/* + * virPolkitCheckAuth: + * @actionid: permission to check + * @pid: client process ID + * @startTime: process start time, or 0 + * @uid: client process user ID + * @details: NULL terminated (key, value) pair list + * @allowInteraction: true if auth prompts are allowed + * + * Check if a client is authenticated with polkit + * + * Returns 0 on success, -1 on failure, -2 on auth denied + */ +int virPolkitCheckAuth(const char *actionid, + pid_t pid, + unsigned long long startTime, + uid_t uid, + const char **details, + bool allowInteraction) +{ + GDBusConnection *sysbus; + GVariantBuilder builder; + GVariant *gprocess = NULL; + GVariant *gdetails = NULL; + g_autoptr(GVariant) message = NULL; + g_autoptr(GVariant) reply = NULL; + g_autoptr(GVariantIter) iter = NULL; + char *retkey; + char *retval; + gboolean is_authorized; + gboolean is_challenge; + bool is_dismissed = false; + const char **next; + + if (!(sysbus = virGDBusGetSystemBus())) + return -1; + + VIR_INFO("Checking PID %lld running as %d", + (long long) pid, uid); + + g_variant_builder_init(&builder, G_VARIANT_TYPE("a{sv}")); + g_variant_builder_add(&builder, "{sv}", "pid", g_variant_new_uint32(pid)); + g_variant_builder_add(&builder, "{sv}", "start-time", g_variant_new_uint64(startTime)); + g_variant_builder_add(&builder, "{sv}", "uid", g_variant_new_int32(uid)); + gprocess = g_variant_builder_end(&builder); + + g_variant_builder_init(&builder, G_VARIANT_TYPE("a{ss}")); + + if (details) { + for (next = details; *next; next++) { + const char *detail1 = *(next++); + const char *detail2 = *next; + g_variant_builder_add(&builder, "{ss}", detail1, detail2); + } + } + + gdetails = g_variant_builder_end(&builder); + + message = g_variant_new("((s@a{sv})s@a{ss}us)", + "unix-process", + gprocess, + actionid, + gdetails, + allowInteraction, + "" /* cancellation ID */); + + if (virGDBusCallMethod(sysbus, + &reply, + G_VARIANT_TYPE("((bba{ss}))"), + NULL, + "org.freedesktop.PolicyKit1", + "/org/freedesktop/PolicyKit1/Authority", + "org.freedesktop.PolicyKit1.Authority", + "CheckAuthorization", + message) < 0) + return -1; + + g_variant_get(reply, "((bba{ss}))", &is_authorized, &is_challenge, &iter); + + while (g_variant_iter_loop(iter, "{ss}", &retkey, &retval)) { + if (STREQ(retkey, "polkit.dismissed") && STREQ(retval, "true")) + is_dismissed = true; + } + + VIR_DEBUG("is auth %d is challenge %d", + is_authorized, is_challenge); + + if (is_authorized) + return 0; + + if (is_dismissed) { + virReportError(VIR_ERR_AUTH_CANCELLED, "%s", + _("user cancelled authentication process")); + } else if (is_challenge) { + virReportError(VIR_ERR_AUTH_UNAVAILABLE, + _("no polkit agent available to authenticate action '%s'"), + actionid); + } else { + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("access denied by policy")); + } + + return -2; +} + #else /* ! WITH_POLKIT */ int virPolkitCheckAuth(const char *actionid G_GNUC_UNUSED, @@ -275,26 +275,4 @@ int virPolkitCheckAuth(const char *actionid G_GNUC_UNUSED, return -1; } - -void -virPolkitAgentDestroy(virPolkitAgent *agent G_GNUC_UNUSED) -{ - return; /* do nothing */ -} - - -virPolkitAgent * -virPolkitAgentCreate(void) -{ - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("polkit text authentication agent unavailable")); - return NULL; -} - -bool -virPolkitAgentAvailable(void) -{ - return false; -} - #endif /* WITH_POLKIT */ -- 2.34.0