On 02/12/2016 04:47 AM, Daniel P. Berrange wrote:
> On Thu, Feb 11, 2016 at 06:38:12PM -0500, John Ferlan wrote:
>> When there isn't a ssh -X type session running and a user has not
>> been added to the libvirt group, attempts to run 'virsh -c qemu:///system'
>> commands from an otherwise unprivileged user will fail with rather
>> generic or opaque error message:
>>
>> "error: authentication failed: no agent is available to authenticate"
>>
>> This patch will adjust the error message to help reflect not only
>> that it was a polkit agent, but which 'action-id' was not found. It
>> does so carefully, by adding to the text rather than adjusting it
>> since virpolkittest is looking for a specific string. The result on
>> a failure then becomes:
>>
>> "error: authentication failed: no agent is available to authenticate -
>> no polkit agent for action 'org.libvirt.unix.manage'"
>>
>> A bit more history on this - at one time a failure generated the
>> following type message when running the 'pkcheck' as a subprocess:
>>
>> "error: authentication failed: polkit\56retains_authorization_after_challenge=1
>> Authorization requires authentication but no agent is available."
>>
>> but, a patch was generated to adjust the error message to help provide
>> more details about what failed. This was pushed as commit id '96a108c99'.
>> That patch prepended a "polkit: " to the output. It really didn't solve
>> the problem, but gave a hint.
>>
>> After some time it was deemed using DBus API calls directly was a
>> better way to go (since pkcheck calls them anyway). So, commit id
>> '1b854c76' (more or less) copied the code from remoteDispatchAuthPolkit
>> and adjusted it. Then commit id 'c7542573' adjusted the remote.c
>> code to call the new API (virPolkitCheckAuth). Finally, commit id
>> '308c0c5a' altered the code to call DBus APIs directly. In doing
>> so, it reverted the failing error message to the generic message
>> that would have been received from DBus anyway.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/util/virpolkit.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
>> index 8da91f2..d837a14 100644
>> --- a/src/util/virpolkit.c
>> +++ b/src/util/virpolkit.c
>> @@ -1,7 +1,7 @@
>> /*
>> * virpolkit.c: helpers for using polkit APIs
>> *
>> - * Copyright (C) 2013, 2014 Red Hat, Inc.
>> + * Copyright (C) 2013, 2014, 2016 Red Hat, Inc.
>> *
>> * This library is free software; you can redistribute it and/or
>> * modify it under the terms of the GNU Lesser General Public
>> @@ -121,8 +121,10 @@ int virPolkitCheckAuth(const char *actionid,
>> virReportError(VIR_ERR_AUTH_CANCELLED, "%s",
>> _("user cancelled authentication process"));
>> else if (is_challenge)
>> - virReportError(VIR_ERR_AUTH_FAILED, "%s",
>> - _("no agent is available to authenticate"));
>> + virReportError(VIR_ERR_AUTH_FAILED,
>> + _("no agent is available to authenticate - "
>> + "no polkit agent for action '%s'"),
>
> This is a littled repetitive still. ACK if you make it say
>
> "No polkit agent available to authenticate action '%s'"
>
OK - that will require a change to virpolkittest.c too (adding the
domain and code checks as well):
index 1ef7635..56f98b2 100644
--- a/tests/virpolkittest.c
+++ b/tests/virpolkittest.c
@@ -220,8 +220,9 @@ static int testPolkitAuthChallenge(const void
*opaque ATTRIBUTE_UNUSED)
}
err = virGetLastError();
- if (!err || !strstr(err->message,
- _("no agent is available to authenticate"))) {
+ if (!err || err->domain != VIR_FROM_POLKIT ||
+ err->code != VIR_ERR_AUTH_FAILED || !strstr(err->message,
+ _("no polkit agent available to authenticate"))) {
fprintf(stderr, "Incorrect error response\n");
goto cleanup;
}
Although my thoughts in reply to the patch3 review may need to tweak
this more...
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list