On 05/24/2017 05:45 PM, John Ferlan wrote: > > > 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@xxxxxxxxxx> >>> --- >>> 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... Aha. So these two patches are mutually exclusive? I haven't read it until now O:-) Well, I like the first one better because it's more simple. It fixes just what is broken and is not introducing any bug. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
