3:31 a.m.
Hi,
while cleaning out patchs that we held for a while on top of libvirt
I found this patch of Serge (thanks!) which I think would make just as
much sense in the upstream project itself.
Or in case the discussion might unveil why it might not make sense,
that would also be a win for us to adapt.
I modified the older patch to match latest master (files owning
the code changed) and added the matching doc update to be complete.
Serge Hallyn (1):
storage: use 0711 as the default perms for dirs
docs/formatstorage.html.in | 2 +-
src/storage/storage_util.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--
2.7.4
3:31 a.m.
New subject: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
From: Serge Hallyn <serge.hallyn(a)ubuntu.com>
There should be no need to make dir based pools world readable.
So use 0711, not 0755, as the default perms for storage dirs.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
---
docs/formatstorage.html.in | 2 +-
src/storage/storage_util.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index 225e190..4946ddf 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -444,7 +444,7 @@
namespace. It provides information about the permissions to use for the
final directory when the pool is built. There are 4 child elements.
The <code>mode</code> element contains the octal permission set.
- The <code>mode</code> defaults to 0755 when not provided.
+ The <code>mode</code> defaults to 0711 when not provided.
The <code>owner</code> element contains the numeric user ID.
The <code>group</code> element contains the numeric group ID.
If <code>owner</code> or <code>group</code> aren't
specified when
diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h
index a05c35d..6f2a1b1 100644
--- a/src/storage/storage_util.h
+++ b/src/storage/storage_util.h
@@ -138,7 +138,7 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb,
ATTRIBUTE_RETURN_CHECK
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755
+# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0711
# define VIR_STORAGE_DEFAULT_VOL_PERM_MODE 0600
int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
--
2.7.4
5:36 p.m.
New subject: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
On 05/11/2017 04:31 AM, Christian Ehrhardt wrote:
From: Serge Hallyn <serge.hallyn(a)ubuntu.com>
There should be no need to make dir based pools world readable.
So use 0711, not 0755, as the default perms for storage dirs.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
---
docs/formatstorage.html.in | 2 +-
src/storage/storage_util.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
Kinda surprised this didn't generate some immediate discussion... I
would also think that if you had a desire to change defaults you'd also
have a libvirt.spec.in adjustment...
Still 0755 or umask(022) seem to be fairly prevalent setting and having
the <mode> for the XML to be able to override a default certainly gives
credence to arguments in either direction whether or not to change the
defaults.
It's been a long while since I considered system/directory/file security
things, but I have this faint recollection of some strange issue when
not having world or group "executable" as a default.
Still for those that desire a higher level of protection and security
there are ways to set more stringent values, but out of the box going
with 755 still seems reasonable. Although I'm sure there's varying
opinions on that depending upon your expectations of a secure system.
Also your commit message notes "world readable", but by going from 755
to 711, you're also changing to "group readable" too ;-)
John
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index 225e190..4946ddf 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -444,7 +444,7 @@
namespace. It provides information about the permissions to use for the
final directory when the pool is built. There are 4 child elements.
The <code>mode</code> element contains the octal permission set.
- The <code>mode</code> defaults to 0755 when not provided.
+ The <code>mode</code> defaults to 0711 when not provided.
The <code>owner</code> element contains the numeric user ID.
The <code>group</code> element contains the numeric group ID.
If <code>owner</code> or <code>group</code> aren't
specified when
diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h
index a05c35d..6f2a1b1 100644
--- a/src/storage/storage_util.h
+++ b/src/storage/storage_util.h
@@ -138,7 +138,7 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb,
ATTRIBUTE_RETURN_CHECK
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755
+# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0711
# define VIR_STORAGE_DEFAULT_VOL_PERM_MODE 0600
int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
3:27 a.m.
New subject: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
On Thu, May 11, 2017 at 06:36:22PM -0400, John Ferlan wrote:
On 05/11/2017 04:31 AM, Christian Ehrhardt wrote:
> From: Serge Hallyn <serge.hallyn(a)ubuntu.com>
>
> There should be no need to make dir based pools world readable.
> So use 0711, not 0755, as the default perms for storage dirs.
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
> ---
> docs/formatstorage.html.in | 2 +-
> src/storage/storage_util.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
Kinda surprised this didn't generate some immediate discussion... I
would also think that if you had a desire to change defaults you'd also
have a libvirt.spec.in adjustment...
Actually no it doesn't - the spec file is already marking
/var/lib/libvirt/images as 0711.
Still 0755 or umask(022) seem to be fairly prevalent setting and
having
the <mode> for the XML to be able to override a default certainly gives
credence to arguments in either direction whether or not to change the
defaults.
It's been a long while since I considered system/directory/file security
things, but I have this faint recollection of some strange issue when
not having world or group "executable" as a default.
The fact that RPM spec ships with 0711 show that it works ok. So I
think this change is reasonable.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
5:55 a.m.
New subject: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
On Mon, May 15, 2017 at 09:27:38AM +0100, Daniel P. Berrange wrote:
On Thu, May 11, 2017 at 06:36:22PM -0400, John Ferlan wrote:
>
>
> On 05/11/2017 04:31 AM, Christian Ehrhardt wrote:
> > From: Serge Hallyn <serge.hallyn(a)ubuntu.com>
> >
> > There should be no need to make dir based pools world readable.
> > So use 0711, not 0755, as the default perms for storage dirs.
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
> > ---
> > docs/formatstorage.html.in | 2 +-
> > src/storage/storage_util.h | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
>
> Kinda surprised this didn't generate some immediate discussion... I
> would also think that if you had a desire to change defaults you'd also
> have a libvirt.spec.in adjustment...
Actually no it doesn't - the spec file is already marking
/var/lib/libvirt/images as 0711.
> Still 0755 or umask(022) seem to be fairly prevalent setting and having
> the <mode> for the XML to be able to override a default certainly gives
> credence to arguments in either direction whether or not to change the
> defaults.
>
> It's been a long while since I considered system/directory/file security
> things, but I have this faint recollection of some strange issue when
> not having world or group "executable" as a default.
The fact that RPM spec ships with 0711 show that it works ok. So I
think this change is reasonable.
Same here. I'm not sure, but I think even SELinux policy defaulted to
that. Anyway, ACK to this one, I'll push this in a while.
While we're on this, is there some global config for the group in all
these permissions? I would love to add a user to one group and make all
libvirt-related readable for that user with that one simple addition.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:58 a.m.
New subject: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
On Mon, May 15, 2017 at 10:27 AM, Daniel P. Berrange <berrange(a)redhat.com>
wrote:
> Kinda surprised this didn't generate some immediate
discussion... I
> would also think that if you had a desire to change defaults you'd also
> have a libvirt.spec.in adjustment...
Actually no it doesn't - the spec file is already marking
/var/lib/libvirt/images as 0711.
As reference that is the current spec content:
libvirt.spec.in:1745:%dir %attr(0711, root, root)
%{_localstatedir}/lib/libvirt/images/
> Still 0755 or umask(022) seem to be fairly prevalent setting and
having
> the <mode> for the XML to be able to override a default certainly gives
> credence to arguments in either direction whether or not to change the
> defaults.
>
> It's been a long while since I considered system/directory/file security
> things, but I have this faint recollection of some strange issue when
> not having world or group "executable" as a default.
The fact that RPM spec ships with 0711 show that it works ok. So I
think this change is reasonable.
Interesting, I didn't check the RPM spec - thanks Daniel to point this out.
It is 711 on Ubuntu as well for quite some time now.
Both together make this even less likely to have hidden drawbacks.
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
6:02 a.m.
New subject: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
On Fri, May 12, 2017 at 12:36 AM, John Ferlan <jferlan(a)redhat.com> wrote:
Also your commit message notes "world readable", but by
going from 755
to 711, you're also changing to "group readable" too ;-)
Good catch John,
the other feedback seems good, so for now I'm just rewording in regard to
this and resubmit to the thread.
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
6:05 a.m.
New subject: [libvirt] [PATCH v2] storage: use 0711 as the default perms for dirs
From: Serge Hallyn <serge.hallyn(a)ubuntu.com>
There should be no need to make dir based pools world/group readable.
So use 0711, not 0755, as the default perms for storage dirs.
Updates in v2:
- adapt commit wording to mention dropping group readable as well
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
---
docs/formatstorage.html.in | 2 +-
src/storage/storage_util.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index 225e190..4946ddf 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -444,7 +444,7 @@
namespace. It provides information about the permissions to use for the
final directory when the pool is built. There are 4 child elements.
The <code>mode</code> element contains the octal permission set.
- The <code>mode</code> defaults to 0755 when not provided.
+ The <code>mode</code> defaults to 0711 when not provided.
The <code>owner</code> element contains the numeric user ID.
The <code>group</code> element contains the numeric group ID.
If <code>owner</code> or <code>group</code> aren't
specified when
diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h
index a05c35d..6f2a1b1 100644
--- a/src/storage/storage_util.h
+++ b/src/storage/storage_util.h
@@ -138,7 +138,7 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb,
ATTRIBUTE_RETURN_CHECK
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755
+# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0711
# define VIR_STORAGE_DEFAULT_VOL_PERM_MODE 0600
int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
--
2.7.4
6:10 a.m.
New subject: [libvirt] [PATCH v2] storage: use 0711 as the default perms for dirs
On Mon, May 15, 2017 at 01:05:31PM +0200, Christian Ehrhardt wrote:
From: Serge Hallyn <serge.hallyn(a)ubuntu.com>
There should be no need to make dir based pools world/group readable.
So use 0711, not 0755, as the default perms for storage dirs.
Updates in v2:
- adapt commit wording to mention dropping group readable as well
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
---
docs/formatstorage.html.in | 2 +-
src/storage/storage_util.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrange <berrange(a)redhat.com>
Will push to git shortly.
BTW, for libvir-list we recommend to send v2/v3/etc followup patches as
top level threads, not in-reply-to the previous versions.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
6:14 a.m.
New subject: [libvirt] [PATCH v2] storage: use 0711 as the default perms for dirs
On Mon, May 15, 2017 at 1:10 PM, Daniel P. Berrange <berrange(a)redhat.com>
wrote:
BTW, for libvir-list we recommend to send v2/v3/etc followup patches
as
top level threads, not in-reply-to the previous versions.
I need a mapper which project prefers what :-), no really - thank you a lot!
Since we are about to submit a bigger pile of apparmor changes that hint
might certainly be handy the next days/weeks.
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
2748
days inactive
2752
days old
9 comments
4 participants
participants (4)
-
Christian Ehrhardt -
Daniel P. Berrange -
John Ferlan -
Martin Kletzander