[libvirt] [PATCH 2/2] virsh: Don't attempt polkit processing for non local authn/authz
Michal Privoznik
mprivozn at redhat.com
Thu May 25 06:00:18 UTC 2017
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 at redhat.com>
>>> ---
>>> 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
More information about the libvir-list
mailing list