1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] storage: use 0711 as the default perms for dirs

Christian Ehrhardt posted 1 patch 8 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1494491469-13768-2-git-send-email-christian.ehrhardt@canonical.com
There is a newer version of this series
docs/formatstorage.html.in | 2 +-
src/storage/storage_util.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
[libvirt] [PATCH] storage: use 0711 as the default perms for dirs
Posted by Christian Ehrhardt 8 years ago 
From: Serge Hallyn <serge.hallyn@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
Posted by John Ferlan 8 years ago 

On 05/11/2017 04:31 AM, Christian Ehrhardt wrote:
> From: Serge Hallyn <serge.hallyn@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@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,
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
Posted by Daniel P. Berrange 8 years ago 
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@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@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
Posted by Martin Kletzander 8 years ago 
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@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@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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
Posted by Christian Ehrhardt 8 years ago 
On Mon, May 15, 2017 at 10:27 AM, Daniel P. Berrange <berrange@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs
Posted by Christian Ehrhardt 8 years ago 
On Fri, May 12, 2017 at 12:36 AM, John Ferlan <jferlan@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] storage: use 0711 as the default perms for dirs
Posted by Christian Ehrhardt 8 years ago 
From: Serge Hallyn <serge.hallyn@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: use 0711 as the default perms for dirs
Posted by Daniel P. Berrange 8 years ago 
On Mon, May 15, 2017 at 01:05:31PM +0200, Christian Ehrhardt wrote:
> From: Serge Hallyn <serge.hallyn@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@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@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] storage: use 0711 as the default perms for dirs
Posted by Christian Ehrhardt 8 years ago 
On Mon, May 15, 2017 at 1:10 PM, Daniel P. Berrange <berrange@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.