Re: [libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication
On 02/12/2016 06:57 AM, Daniel P. Berrange wrote:
On Fri, Feb 12, 2016 at 06:49:22AM -0500, John Ferlan wrote:
> [...]
>
>>> + err = virGetLastError();
>>> + if (err && strstr(err->message,
>>> + _("no agent is available to
authenticate"))) {
>>
>>> + if (!pkagent) {
>>> + if (!(pkagent = virPolkitAgentCreate()))
>>> + goto cleanup;
>>> + }
>>> + agentstart++;
>>> + } else if (err && strstr(err->message,
_("authentication failed:"))) {
>>
>> String matching is pretty unpleasant. I think we can match on
>> err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_FAILED
>> for this.
>>
>
> Using VIR_ERR_AUTH_FAILED I cannot distinguish between the failure of
> available agent or access denied by policy from virPolkitCheckAuth.
> Adjusting what virPolkitCheckAuth returns means more code modification
> since the assumption is -2 has 3 possible issues of which 2 currently
> are tested by a err->message comparison.
My point is that you don't actually need to distinguish those two
cases directly. You can do this:
if (err && err->code == VIR_FROM_POLKIT && err->code ==
VIR_ER_AUTH_FAILED) {
if (!virDBusIsServiceRegistered(...polkit...)) {
Including "virdbus.h" to get virDBusIsServiceRegistered from virsh.c
sends me down the build system rabbit hole again:
In file included from virsh.c:59:0:
../src/util/virdbus.h:27:25: fatal error: dbus/dbus.h: No such file or
directory
compilation terminated.
Adding "$(DBUS_CFLAGS)" to the virsh_CLFAGS in Makefile.am still leaves
me with:
virsh-virsh.o: In function `virshConnect':
/home/jferlan/git/libvirt.work/tools/virsh.c:183: undefined reference to
`virDBusIsServiceRegistered'
collect2: error: ld returned 1 exit status
...
Even if/once figured out, wouldn't that a dependency to virsh?
John
....start agent...
}
....retry auth...
}
> I would think in this case, I wouldn't want to create a text agent if
> access is denied by policy. So should I bite the bullet and adjust the
> return value checking? Or should I add a new error code
> "VIR_ERR_AUTH_DENY" and likewise adjust the code/tests to use that
> rather than the current string comparisons.
It is actually generally bad security practice to tell users /why/ auth
failed - that we return different error messages for these two cases
is probably something we should in fact fix.