[libvirt] [PATCH] Allow root users to have their own configuration file
Currently, we have two configuration file paths, one global (where "global" means root-only and we're probably not changing this in near future) and one per-user. Unfortunately root user cannot use the second option because until now we were choosing the file path depending only on whether the user is root or not. This patch modifies the mentioned behavior for root only, allowing him to set his own configuration files without changing anything in system-wide configuration folders. This also makes the virsh-uriprecedence test pass its first test case when ran as root. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- Notes: I'm playing along previously mentioned "proper behavior" in this patch. However, IMNSHO, our "global" or "system-wide" configuration file (defaulting to '/etc/libvirt/libvirt.conf') should be accessible for all users since this has no security impact (security information may be in files 'libvirtd.conf' or 'qemu.conf'). This file should be also read and used for all users. After that, settings in user configuration file (defaulting to '~/.config/libvirt/libvirt.conf') may override some of these settings for that user. This is how all sensible configurations are loaded and that's also what I'd prefer. Unfortunately some developers feels this should be done in completely different way. src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 20a2d4c..bfc466b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -957,28 +957,34 @@ error: return -1; } -static char * -virConnectGetConfigFilePath(void) +/* + * Return code 0 means no error, but doesn't guarantee path != NULL. + */ +static int +virConnectGetConfigFilePath(char **path, bool global) { - char *path; - if (geteuid() == 0) { - if (virAsprintf(&path, "%s/libvirt/libvirt.conf", + char *userdir = NULL; + int ret = -1; + *path = NULL; + + /* Don't provide the global configuration file to non-root users */ + if (geteuid() != 0 && global) + return 0; + + if (global) { + if (virAsprintf(path, "%s/libvirt/libvirt.conf", SYSCONFDIR) < 0) - return NULL; + goto cleanup; } else { - char *userdir = virGetUserConfigDirectory(); - if (!userdir) - return NULL; - - if (virAsprintf(&path, "%s/libvirt.conf", - userdir) < 0) { - VIR_FREE(userdir); - return NULL; - } - VIR_FREE(userdir); + if (!(userdir = virGetUserConfigDirectory()) || + virAsprintf(path, "%s/libvirt.conf", userdir) < 0) + goto cleanup; } - return path; + ret = 0; + cleanup: + VIR_FREE(userdir); + return ret; } static int @@ -989,12 +995,22 @@ virConnectGetConfigFile(virConfPtr *conf) *conf = NULL; - if (!(filename = virConnectGetConfigFilePath())) + /* Try reading user configuration file unconditionally */ + if (virConnectGetConfigFilePath(&filename, false) < 0) goto cleanup; if (!virFileExists(filename)) { - ret = 0; - goto cleanup; + /* and in case there is none, try the global one. */ + + VIR_FREE(filename); + if (virConnectGetConfigFilePath(&filename, true) < 0) + goto cleanup; + + if (!filename || + !virFileExists(filename)) { + ret = 0; + goto cleanup; + } } VIR_DEBUG("Loading config file '%s'", filename); -- 1.8.3.2
On Thu, Sep 12, 2013 at 11:53:32AM +0200, Martin Kletzander wrote:
Currently, we have two configuration file paths, one global (where "global" means root-only and we're probably not changing this in near future) and one per-user. Unfortunately root user cannot use the second option because until now we were choosing the file path depending only on whether the user is root or not.
This patch modifies the mentioned behavior for root only, allowing him to set his own configuration files without changing anything in system-wide configuration folders.
This also makes the virsh-uriprecedence test pass its first test case when ran as root.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I'm playing along previously mentioned "proper behavior" in this patch. However, IMNSHO, our "global" or "system-wide" configuration file (defaulting to '/etc/libvirt/libvirt.conf') should be accessible for all users since this has no security impact (security information may be in files 'libvirtd.conf' or 'qemu.conf'). This file should be also read and used for all users. After that, settings in user configuration file (defaulting to '~/.config/libvirt/libvirt.conf') may override some of these settings for that user.
This is how all sensible configurations are loaded and that's also what I'd prefer. Unfortunately some developers feels this should be done in completely different way.
src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 20 deletions(-)
NACK to this. The root user already has their own dedicated configuration file, /etc/libvirt/libvirt.conf. The /etc/libvirt directory permissions prevent *any* file there being read by non-root, so the /etc/libvirt/libvirt.conf file could not be used by non-root. IMHO the only flaw here is the test suite, not the config file path handling. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/12/2013 12:00 PM, Daniel P. Berrange wrote:
On Thu, Sep 12, 2013 at 11:53:32AM +0200, Martin Kletzander wrote:
Currently, we have two configuration file paths, one global (where "global" means root-only and we're probably not changing this in near future) and one per-user. Unfortunately root user cannot use the second option because until now we were choosing the file path depending only on whether the user is root or not.
This patch modifies the mentioned behavior for root only, allowing him to set his own configuration files without changing anything in system-wide configuration folders.
This also makes the virsh-uriprecedence test pass its first test case when ran as root.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I'm playing along previously mentioned "proper behavior" in this patch. However, IMNSHO, our "global" or "system-wide" configuration file (defaulting to '/etc/libvirt/libvirt.conf') should be accessible for all users since this has no security impact (security information may be in files 'libvirtd.conf' or 'qemu.conf'). This file should be also read and used for all users. After that, settings in user configuration file (defaulting to '~/.config/libvirt/libvirt.conf') may override some of these settings for that user.
This is how all sensible configurations are loaded and that's also what I'd prefer. Unfortunately some developers feels this should be done in completely different way.
src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 20 deletions(-)
NACK to this. The root user already has their own dedicated configuration file, /etc/libvirt/libvirt.conf. The /etc/libvirt directory permissions prevent *any* file there being read by non-root, so the /etc/libvirt/libvirt.conf file could not be used by non-root.
As mentioned in the commit message, this patch doesn't change the behavior for non-root users, that's only a nag in the notes. The only thing that changes after applying this patch is that *iff* ran as root, we'll *also* check "${XDG_CONFIG_HOME}/libvirt/libvirt.conf". Martin
On Thu, Sep 12, 2013 at 12:07:54PM +0200, Martin Kletzander wrote:
On 09/12/2013 12:00 PM, Daniel P. Berrange wrote:
On Thu, Sep 12, 2013 at 11:53:32AM +0200, Martin Kletzander wrote:
Currently, we have two configuration file paths, one global (where "global" means root-only and we're probably not changing this in near future) and one per-user. Unfortunately root user cannot use the second option because until now we were choosing the file path depending only on whether the user is root or not.
This patch modifies the mentioned behavior for root only, allowing him to set his own configuration files without changing anything in system-wide configuration folders.
This also makes the virsh-uriprecedence test pass its first test case when ran as root.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I'm playing along previously mentioned "proper behavior" in this patch. However, IMNSHO, our "global" or "system-wide" configuration file (defaulting to '/etc/libvirt/libvirt.conf') should be accessible for all users since this has no security impact (security information may be in files 'libvirtd.conf' or 'qemu.conf'). This file should be also read and used for all users. After that, settings in user configuration file (defaulting to '~/.config/libvirt/libvirt.conf') may override some of these settings for that user.
This is how all sensible configurations are loaded and that's also what I'd prefer. Unfortunately some developers feels this should be done in completely different way.
src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 20 deletions(-)
NACK to this. The root user already has their own dedicated configuration file, /etc/libvirt/libvirt.conf. The /etc/libvirt directory permissions prevent *any* file there being read by non-root, so the /etc/libvirt/libvirt.conf file could not be used by non-root.
As mentioned in the commit message, this patch doesn't change the behavior for non-root users, that's only a nag in the notes.
The only thing that changes after applying this patch is that *iff* ran as root, we'll *also* check "${XDG_CONFIG_HOME}/libvirt/libvirt.conf".
I don't see any point in doing that at all. Root already has a config file exclusively for their own use. They don't need 2. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu 12 Sep 2013 12:12:22 PM CEST, Daniel P. Berrange wrote:
On Thu, Sep 12, 2013 at 12:07:54PM +0200, Martin Kletzander wrote:
On 09/12/2013 12:00 PM, Daniel P. Berrange wrote:
On Thu, Sep 12, 2013 at 11:53:32AM +0200, Martin Kletzander wrote:
Currently, we have two configuration file paths, one global (where "global" means root-only and we're probably not changing this in near future) and one per-user. Unfortunately root user cannot use the second option because until now we were choosing the file path depending only on whether the user is root or not.
This patch modifies the mentioned behavior for root only, allowing him to set his own configuration files without changing anything in system-wide configuration folders.
This also makes the virsh-uriprecedence test pass its first test case when ran as root.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I'm playing along previously mentioned "proper behavior" in this patch. However, IMNSHO, our "global" or "system-wide" configuration file (defaulting to '/etc/libvirt/libvirt.conf') should be accessible for all users since this has no security impact (security information may be in files 'libvirtd.conf' or 'qemu.conf'). This file should be also read and used for all users. After that, settings in user configuration file (defaulting to '~/.config/libvirt/libvirt.conf') may override some of these settings for that user.
This is how all sensible configurations are loaded and that's also what I'd prefer. Unfortunately some developers feels this should be done in completely different way.
src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 20 deletions(-)
NACK to this. The root user already has their own dedicated configuration file, /etc/libvirt/libvirt.conf. The /etc/libvirt directory permissions prevent *any* file there being read by non-root, so the /etc/libvirt/libvirt.conf file could not be used by non-root.
As mentioned in the commit message, this patch doesn't change the behavior for non-root users, that's only a nag in the notes.
The only thing that changes after applying this patch is that *iff* ran as root, we'll *also* check "${XDG_CONFIG_HOME}/libvirt/libvirt.conf".
I don't see any point in doing that at all. Root already has a config file exclusively for their own use. They don't need 2.
I wasn't discussing that, but it might be possible to differentiate that configs and made it possible to for them to share /etc configs between hosts and make his own Don't get me wrong, I'm not arguing nor I need this to go in, it just (still) makes way more sense to me. Take a look at git for example (or virtually anything) and his /etc/gitconfig => ~/.gitconfig; $XDG_CONFIG_HOME/git/config => $repository/.git/config and so on... I've sent a v2 [1] fixing the test. Martin [1] http://www.redhat.com/archives/libvir-list/2013-September/msg00701.html
On Thu, Sep 12, 2013 at 6:30 AM, Martin Kletzander <mkletzan@redhat.com> wrote:
On Thu 12 Sep 2013 12:12:22 PM CEST, Daniel P. Berrange wrote:
On Thu, Sep 12, 2013 at 12:07:54PM +0200, Martin Kletzander wrote:
On 09/12/2013 12:00 PM, Daniel P. Berrange wrote:
On Thu, Sep 12, 2013 at 11:53:32AM +0200, Martin Kletzander wrote:
Currently, we have two configuration file paths, one global (where "global" means root-only and we're probably not changing this in near future) and one per-user. Unfortunately root user cannot use the second option because until now we were choosing the file path depending only on whether the user is root or not.
This patch modifies the mentioned behavior for root only, allowing him to set his own configuration files without changing anything in system-wide configuration folders.
This also makes the virsh-uriprecedence test pass its first test case when ran as root.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I'm playing along previously mentioned "proper behavior" in this patch. However, IMNSHO, our "global" or "system-wide" configuration file (defaulting to '/etc/libvirt/libvirt.conf') should be accessible for all users since this has no security impact (security information may be in files 'libvirtd.conf' or 'qemu.conf'). This file should be also read and used for all users. After that, settings in user configuration file (defaulting to '~/.config/libvirt/libvirt.conf') may override some of these settings for that user.
This is how all sensible configurations are loaded and that's also what I'd prefer. Unfortunately some developers feels this should be done in completely different way.
src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 20 deletions(-)
NACK to this. The root user already has their own dedicated configuration file, /etc/libvirt/libvirt.conf. The /etc/libvirt directory permissions prevent *any* file there being read by non-root, so the /etc/libvirt/libvirt.conf file could not be used by non-root.
As mentioned in the commit message, this patch doesn't change the behavior for non-root users, that's only a nag in the notes.
The only thing that changes after applying this patch is that *iff* ran as root, we'll *also* check "${XDG_CONFIG_HOME}/libvirt/libvirt.conf".
I don't see any point in doing that at all. Root already has a config file exclusively for their own use. They don't need 2.
I wasn't discussing that, but it might be possible to differentiate that configs and made it possible to for them to share /etc configs between hosts and make his own
Don't get me wrong, I'm not arguing nor I need this to go in, it just (still) makes way more sense to me. Take a look at git for example (or virtually anything) and his /etc/gitconfig => ~/.gitconfig; $XDG_CONFIG_HOME/git/config => $repository/.git/config and so on...
I've sent a v2 [1] fixing the test.
Martin
[1] http://www.redhat.com/archives/libvir-list/2013-September/msg00701.html
If we're going to move forward with this. We potentially need to do some other things such as allowing different certificates for a root user using qemu:///session vs qemu:///system. Otherwise there's a bit of inconsistency with the rest of the system. -- Doug Goldstein
On 09/12/2013 11:53 AM, Martin Kletzander wrote:
Currently, we have two configuration file paths, one global (where "global" means root-only and we're probably not changing this in near future) and one per-user. Unfortunately root user cannot use the second option because until now we were choosing the file path depending only on whether the user is root or not.
This patch modifies the mentioned behavior for root only, allowing him to set his own configuration files without changing anything in system-wide configuration folders.
This also makes the virsh-uriprecedence test pass its first test case when ran as root.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> ---
Notes: I'm playing along previously mentioned "proper behavior" in this patch. However, IMNSHO, our "global" or "system-wide" configuration file (defaulting to '/etc/libvirt/libvirt.conf') should be accessible for all users since this has no security impact (security information may be in files 'libvirtd.conf' or 'qemu.conf'). This file should be also read and used for all users. After that, settings in user configuration file (defaulting to '~/.config/libvirt/libvirt.conf') may override some of these settings for that user.
This is how all sensible configurations are loaded and that's also what I'd prefer. Unfortunately some developers feels this should be done in completely different way.
src/libvirt.c | 56 ++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 20 deletions(-)
If someone doesn't like the 'bool global' parameter, I'm fine with splitting the functions. If mentioned with an ACK, I'll squash this in: diff --git a/src/libvirt.c b/src/libvirt.c index bfc466b..927868f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -961,25 +961,31 @@ error: * Return code 0 means no error, but doesn't guarantee path != NULL. */ static int -virConnectGetConfigFilePath(char **path, bool global) +virConnectGetGlobalConfigFilePath(char **path) { - char *userdir = NULL; - int ret = -1; *path = NULL; /* Don't provide the global configuration file to non-root users */ - if (geteuid() != 0 && global) + if (geteuid() != 0) return 0; - if (global) { - if (virAsprintf(path, "%s/libvirt/libvirt.conf", - SYSCONFDIR) < 0) - goto cleanup; - } else { - if (!(userdir = virGetUserConfigDirectory()) || - virAsprintf(path, "%s/libvirt.conf", userdir) < 0) - goto cleanup; - } + if (virAsprintf(path, "%s/libvirt/libvirt.conf", + SYSCONFDIR) < 0) + return -1; + + return 0; +} + +static int +virConnectGetUserConfigFilePath(char **path) +{ + char *userdir = NULL; + int ret = -1; + *path = NULL; + + if (!(userdir = virGetUserConfigDirectory()) || + virAsprintf(path, "%s/libvirt.conf", userdir) < 0) + goto cleanup; ret = 0; cleanup: @@ -996,14 +1002,14 @@ virConnectGetConfigFile(virConfPtr *conf) *conf = NULL; /* Try reading user configuration file unconditionally */ - if (virConnectGetConfigFilePath(&filename, false) < 0) + if (virConnectGetUserConfigFilePath(&filename) < 0) goto cleanup; if (!virFileExists(filename)) { /* and in case there is none, try the global one. */ VIR_FREE(filename); - if (virConnectGetConfigFilePath(&filename, true) < 0) + if (virConnectGetGlobalConfigFilePath(&filename) < 0) goto cleanup; if (!filename || -- Martin
participants (3)
-
Daniel P. Berrange -
Doug Goldstein -
Martin Kletzander