Hi
On Tue, Sep 24, 2019 at 2:08 PM Ján Tomko <jtomko@xxxxxxxxxx> wrote:
>
> On Tue, Sep 24, 2019 at 10:12:19AM +0400, Marc-André Lureau wrote:
> >Hi
> >
> >On Mon, Sep 23, 2019 at 4:35 PM Ján Tomko <jtomko@xxxxxxxxxx> wrote:
> >>
> >> On Mon, Sep 23, 2019 at 02:44:25PM +0400, marcandre.lureau@xxxxxxxxxx wrote:
> >> >From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >> >
> >> >Whether a directory is missing or is not readable doesn't make much
> >> >difference when populating the available configs.
> >> >
> >> >This removes errors when firmare or vhost-user config is looked up
> >> >from a user session libvirt if /etc/libvirt is not readable for ex.
> >> >
> >> >Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >> >---
> >> > src/qemu/qemu_interop_config.c | 8 +++++++-
> >> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >> >
> >> >diff --git a/src/qemu/qemu_interop_config.c b/src/qemu/qemu_interop_config.c
> >> >index 1f39d4b576..f3c5d2e083 100644
> >> >--- a/src/qemu/qemu_interop_config.c
> >> >+++ b/src/qemu/qemu_interop_config.c
> >> >@@ -41,8 +41,14 @@ qemuBuildFileList(virHashTablePtr files, const char *dir)
> >> > int rc;
> >> > int ret = -1;
> >> >
> >> >- if ((rc = virDirOpenIfExists(&dirp, dir)) < 0)
> >> >+ if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) {
> >> >+ /* silently ignore unreadable directories */
> >>
> >> This will not be silent - virDirOpenIfExists already logged an error.
> >
> >Right, let's drop the comment.
> >
> >>
> >> You can call virFileExists upfront - which is just a wrapper to access
> >> and then take virDirOpenIfExists errors seriously - if someone changes
> >> the permissions in the meantime, they deserve the error.
> >
> >That won't work the desired way, since virFileExists() is true even
> >when you don't have permissions to read it.
> >
> >>
> >> >+ if (virLastErrorIsSystemErrno(EACCES)) {
> >> >+ virResetLastError();
> >> >+ return 0;
> >> >+ }
> >> > return -1;
> >> >+ }
> >> >
> >>
> >> With that fixed:
> >> Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>
> >
> >I see several ways forward, if any of the directory doesn't have
> >readable permissions :
> >
> >1. keep behaviour before this patch: fail entirely. This is
> >unnecessarily strict imho
> >2. use this patch (remove the comment): log an error, but browse other
> >directories
> >3. change the code further so that no error is logged
> >
> >To me, 2 is a good compromise.
> >
> >Does your r-b tag holds with the comment removed only?
> >
>
> Logging errors that are not treated as such pollutes logs and misleads
> users.
>
It's treated as an error: it logs an error, but it's not fatal.
> I assume the only possible path here is QEMU_ETC_LOCATION - the user
> should have access to $HOME and preventing access to /usr/share does
> not sound like a reasonable system.
>
> What would be the benefits of making /etc/qemu private? If it's
> a misconfiguration, we don't have to be lenient here and can go with #1
It could be a misconfiguration or an administrator policy. In both
cases, it's not a good idea to prevent the user from starting the VM
with the user configs.
>
> Otherwise, making
> virFileAccessibleAs(etcLocation, R_OK | X_OK, geteuid(), getegid())
> a condition for
> if (qemuBuildFileList(files, etcLocation) < 0)
> return -1;
> seems to be the least effort to silence unaccessible /etc/qemu while still
> erroring out on the required cases.
Sounds okay to me, but the user would have no clear clue that
something unusual happened (that system config are not readable).
>
> Jano
>
>
> >thanks
> >
> >--
> >libvir-list mailing list
> >libvir-list@xxxxxxxxxx
> >https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
