Re: [libvirt] [PATCH 2/2] virsh: Don't attempt polkit processing for non local authn/authz

On 05/24/2017 10:38 AM, Michal Privoznik wrote: > On 05/11/2017 05:04 PM, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1374126 >> >> Due to how the processing for authentication using polkit works, the >> virshConnect code must first "attempt" an virConnectOpenAuth and then >> check for a "special" return error code VIR_ERR_AUTH_UNAVAILABLE in >> order to attempt to "retry" the authentication after performing a creation >> of a pkttyagent to handle the challenge/response for the client. >> >> However, attempting to use a remote connection, (such as perhaps >> "qemu+ssh://someuser@localhost/system"), will cause a never ending >> loop since attempting to generate a pkttyagent would fail for the >> network client connection resulting in a never ending loop since the >> return code is always VIR_ERR_AUTH_UNAVAILABLE from virPolkitCheckAuth. >> The only way out of the loop is a forced quit (e.g. ctrl-c) as the >> @authfail wouldn't be incremented as a result of a failed authn from >> pkttyagent. >> >> So rather than take any extra step for which the only result will be >> a failure, let's check if there is a URI and if it's not using ":///", >> then just fail. >> >> This resolves the never ending loop and will generate an error: >> >> error: failed to connect to the hypervisor >> error: authentication unavailable: no polkit agent available to authenticate action 'org.libvirt.unix.manage' >> >> NB: If the authentication was for a sufficiently privileged client, such as >> qemu+ssh://root@localhost/system, then the remoteDispatchAuthList "allows" >> the authentication to use libvirt since @callerUid would be 0. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> tools/virsh.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/tools/virsh.c b/tools/virsh.c >> index 1f5c2b1..be368ba 100644 >> --- a/tools/virsh.c >> +++ b/tools/virsh.c >> @@ -166,6 +166,11 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) >> if (readonly) >> goto cleanup; >> >> + /* No URI or indication of a requesting a remote connection, then >> + * polkit will not work for the authentication/authorization */ >> + if (!uri || !(strstr(uri, ":///"))) >> + goto cleanup; > > But we can get here even without polkit, right? Therefore it'd be much > more safer to check err && err->domain == VIR_FROM_POLKIT. > True - that's simple enough to adjust... So instead of: err = virGetLastError(); if (err && err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_UNAVAILABLE) { if (!pkagent && !(pkagent = virPolkitAgentCreate())) goto cleanup; } else if (err && err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_FAILED) { authfail++; } else { goto cleanup; } I could have if (err && err->domain == VIR_FROM_POLKIT) { /* No URI or indication of a requesting a remote connection, then * polkit will not work for the authentication/authorization */ if (!uri || !(strstr(uri, ":///"))) goto cleanup; if (err->code == VIR_ERR_AUTH_UNAVAILABLE) { if (!pkagent && !(pkagent = virPolkitAgentCreate())) goto cleanup; } else if (err->code == VIR_ERR_AUTH_FAILED) { authfail++; } else { goto cleanup; } } else { goto cleanup; } So is there a preferred approach? See the cover letter...