Re: [libvirt] [PATCHv1.5 5/8] security: DAC: Plumb usage of chown callback

2014-07-24 Thread Peter Krempa 
On 07/22/14 17:59, John Ferlan wrote:
 
 
 On 07/22/2014 05:20 AM, Peter Krempa wrote:
 Use the callback to set disk and storage image labels by modifying the
 existing functions and adding wrappers to avoid refactoring a lot of the
 code.
 ---
  src/security/security_dac.c | 89 
 +++--
  1 file changed, 69 insertions(+), 20 deletions(-)

 diff --git a/src/security/security_dac.c b/src/security/security_dac.c
 index 1fb0c86..989329f 100644
 --- a/src/security/security_dac.c
 +++ b/src/security/security_dac.c
 @@ -230,23 +230,54 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
  }

  static int
 -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
 +virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
 +   virStorageSourcePtr src,
 +   const char *path,
 +   uid_t uid,
 +   gid_t gid)
  {
 +int rc;
 +int chown_errno;
 +
  VIR_INFO(Setting DAC user and group on '%s' to '%ld:%ld',
 - path, (long) uid, (long) gid);
 + NULLSTR(src ? src-path : path), (long) uid, (long) gid);
 +
 +if (priv  src  priv-chownCallback) {
 
 This is the 'src' option and 'path' is NULL

Hmm, yes, if control reaches here with path equal to NULL, and ..


 
 +rc = priv-chownCallback(src, uid, gid);

... the chown callback fails to chown the file, then ...

 +
 +/* on -2 returned an error was already reported */
 +if (rc == -2)
 +return -1;

 -if (chown(path, uid, gid)  0) {
 +/* on -1 only errno was set */
 +chown_errno = errno;
 
 Thus rc == -1, path == NULL
 
 +} else {
  struct stat sb;
 -int chown_errno = errno;

 -if (stat(path, sb) = 0) {
 +if (!path) {
 +if (!src || !src-path)
 
 Maybe a if !src  !path return 0 should be before the VIR_INFO...
 Maybe with a VIR_DEBUG that'll help someone some day figure out why they
 aren't getting what they were expecting. Just a thought...
 
 +return 0;
 +
 +if (!virStorageSourceIsLocalStorage(src))
 +return 0;
 +
 +path = src-path;
 
 Reusing a passed parameter is where things get dicey for me.
 
 +}
 +
 +rc = chown(path, uid, gid);
 +chown_errno = errno;
 +
 +if (rc  0 
 +stat(path, sb) = 0) {
  if (sb.st_uid == uid 
  sb.st_gid == gid) {
  /* It's alright, there's nothing to change anyway. */
  return 0;
  }
  }
 +}

 +if (rc  0) {
 
 I think we can get here with path == NULL and rc == -1, resulting in
 subsequent usage of '%s' for path to have 'nil', right?

... yes this will try to *printf a NULL string. So I'll add an
initialization of path to something sane in the branch using the
callback as at that point the path variable will be used only to
report errors.

Our reporting of path in case of remote storage sucks anyways so I'm
planning on adding a function that will format the path of the source
for error messages. At that point I'll revisit this (maybe even today as
the number of broken error messages I've seen in the last month is vast
and I'm not helping it currently)

 
  if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
  VIR_INFO(Setting user and group to '%ld:%ld' on '%s' not 
   supported by filesystem,
 @@ -270,13 +301,31 @@ virSecurityDACSetOwnership(const char *path, uid_t 
 uid, gid_t gid)
  return 0;
  }

 +
  static int
 -virSecurityDACRestoreSecurityFileLabel(const char *path)
 +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
 +{
 +return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
 +}
 +
 +
 +static int
 +virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv,
 +   virStorageSourcePtr src,
 +   const char *path)
  {
 -VIR_INFO(Restoring DAC user and group on '%s', path);
 +VIR_INFO(Restoring DAC user and group on '%s',
 + NULLSTR(src ? src-path : path));

  /* XXX record previous ownership */
 -return virSecurityDACSetOwnership(path, 0, 0);
 +return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);
 
 I know this just follows the previous code, but in the big picture - how
 does this play in with future patches where fs  gluster will
 seemingly go though this path?
 
 
 ACK - in general with the 'path' issue above understood.  For this code,
 I'm mostly curious.

I'll push this patch with path initialized to something sane and
revisit it when tweaking the error messages for functions using
virStorageSource struct.

 
 John
 +}

Peter




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing

[libvirt] [PATCHv1.5 5/8] security: DAC: Plumb usage of chown callback

2014-07-22 Thread Peter Krempa 
Use the callback to set disk and storage image labels by modifying the
existing functions and adding wrappers to avoid refactoring a lot of the
code.
---
 src/security/security_dac.c | 89 +++--
 1 file changed, 69 insertions(+), 20 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 1fb0c86..989329f 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -230,23 +230,54 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
 }

 static int
-virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
+virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
+   virStorageSourcePtr src,
+   const char *path,
+   uid_t uid,
+   gid_t gid)
 {
+int rc;
+int chown_errno;
+
 VIR_INFO(Setting DAC user and group on '%s' to '%ld:%ld',
- path, (long) uid, (long) gid);
+ NULLSTR(src ? src-path : path), (long) uid, (long) gid);
+
+if (priv  src  priv-chownCallback) {
+rc = priv-chownCallback(src, uid, gid);
+
+/* on -2 returned an error was already reported */
+if (rc == -2)
+return -1;

-if (chown(path, uid, gid)  0) {
+/* on -1 only errno was set */
+chown_errno = errno;
+} else {
 struct stat sb;
-int chown_errno = errno;

-if (stat(path, sb) = 0) {
+if (!path) {
+if (!src || !src-path)
+return 0;
+
+if (!virStorageSourceIsLocalStorage(src))
+return 0;
+
+path = src-path;
+}
+
+rc = chown(path, uid, gid);
+chown_errno = errno;
+
+if (rc  0 
+stat(path, sb) = 0) {
 if (sb.st_uid == uid 
 sb.st_gid == gid) {
 /* It's alright, there's nothing to change anyway. */
 return 0;
 }
 }
+}

+if (rc  0) {
 if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
 VIR_INFO(Setting user and group to '%ld:%ld' on '%s' not 
  supported by filesystem,
@@ -270,13 +301,31 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, 
gid_t gid)
 return 0;
 }

+
 static int
-virSecurityDACRestoreSecurityFileLabel(const char *path)
+virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
+{
+return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
+}
+
+
+static int
+virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv,
+   virStorageSourcePtr src,
+   const char *path)
 {
-VIR_INFO(Restoring DAC user and group on '%s', path);
+VIR_INFO(Restoring DAC user and group on '%s',
+ NULLSTR(src ? src-path : path));

 /* XXX record previous ownership */
-return virSecurityDACSetOwnership(path, 0, 0);
+return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);
+}
+
+
+static int
+virSecurityDACRestoreSecurityFileLabel(const char *path)
+{
+return virSecurityDACRestoreSecurityFileLabelInternal(NULL, NULL, path);
 }


@@ -294,10 +343,6 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr 
mgr,
 if (!priv-dynamicOwnership)
 return 0;

-/* XXX: Add support for gluster DAC permissions */
-if (!src-path || !virStorageSourceIsLocalStorage(src))
-return 0;
-
 secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
 if (secdef  !secdef-relabel)
 return 0;
@@ -315,7 +360,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr 
mgr,
 return -1;
 }

-return virSecurityDACSetOwnership(src-path, user, group);
+return virSecurityDACSetOwnershipInternal(priv, src, NULL, user, group);
 }


@@ -349,9 +394,6 @@ 
virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
 if (!priv-dynamicOwnership)
 return 0;

-if (!src-path || !virStorageSourceIsLocalStorage(src))
-return 0;
-
 /* Don't restore labels on readoly/shared disks, because other VMs may
  * still be accessing these. Alternatively we could iterate over all
  * running domains and try to figure out if it is in use, but this would
@@ -373,9 +415,16 @@ 
virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
  * ownership, because that kills access on the destination host which is
  * sub-optimal for the guest VM's I/O attempts :-) */
 if (migrated) {
-int rc = virFileIsSharedFS(src-path);
-if (rc  0)
-return -1;
+int rc = 1;
+
+if (virStorageSourceIsLocalStorage(src)) {
+if (!src-path)
+return 0;
+
+if ((rc = virFileIsSharedFS(src-path))  0)
+return -1;
+}

Re: [libvirt] [PATCHv1.5 5/8] security: DAC: Plumb usage of chown callback

2014-07-22 Thread John Ferlan 


On 07/22/2014 05:20 AM, Peter Krempa wrote:
 Use the callback to set disk and storage image labels by modifying the
 existing functions and adding wrappers to avoid refactoring a lot of the
 code.
 ---
  src/security/security_dac.c | 89 
 +++--
  1 file changed, 69 insertions(+), 20 deletions(-)
 
 diff --git a/src/security/security_dac.c b/src/security/security_dac.c
 index 1fb0c86..989329f 100644
 --- a/src/security/security_dac.c
 +++ b/src/security/security_dac.c
 @@ -230,23 +230,54 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
  }
 
  static int
 -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
 +virSecurityDACSetOwnershipInternal(virSecurityDACDataPtr priv,
 +   virStorageSourcePtr src,
 +   const char *path,
 +   uid_t uid,
 +   gid_t gid)
  {
 +int rc;
 +int chown_errno;
 +
  VIR_INFO(Setting DAC user and group on '%s' to '%ld:%ld',
 - path, (long) uid, (long) gid);
 + NULLSTR(src ? src-path : path), (long) uid, (long) gid);
 +
 +if (priv  src  priv-chownCallback) {

This is the 'src' option and 'path' is NULL

 +rc = priv-chownCallback(src, uid, gid);
 +
 +/* on -2 returned an error was already reported */
 +if (rc == -2)
 +return -1;
 
 -if (chown(path, uid, gid)  0) {
 +/* on -1 only errno was set */
 +chown_errno = errno;

Thus rc == -1, path == NULL

 +} else {
  struct stat sb;
 -int chown_errno = errno;
 
 -if (stat(path, sb) = 0) {
 +if (!path) {
 +if (!src || !src-path)

Maybe a if !src  !path return 0 should be before the VIR_INFO...
Maybe with a VIR_DEBUG that'll help someone some day figure out why they
aren't getting what they were expecting. Just a thought...

 +return 0;
 +
 +if (!virStorageSourceIsLocalStorage(src))
 +return 0;
 +
 +path = src-path;

Reusing a passed parameter is where things get dicey for me.

 +}
 +
 +rc = chown(path, uid, gid);
 +chown_errno = errno;
 +
 +if (rc  0 
 +stat(path, sb) = 0) {
  if (sb.st_uid == uid 
  sb.st_gid == gid) {
  /* It's alright, there's nothing to change anyway. */
  return 0;
  }
  }
 +}
 
 +if (rc  0) {

I think we can get here with path == NULL and rc == -1, resulting in
subsequent usage of '%s' for path to have 'nil', right?

  if (chown_errno == EOPNOTSUPP || chown_errno == EINVAL) {
  VIR_INFO(Setting user and group to '%ld:%ld' on '%s' not 
   supported by filesystem,
 @@ -270,13 +301,31 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, 
 gid_t gid)
  return 0;
  }
 
 +
  static int
 -virSecurityDACRestoreSecurityFileLabel(const char *path)
 +virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
 +{
 +return virSecurityDACSetOwnershipInternal(NULL, NULL, path, uid, gid);
 +}
 +
 +
 +static int
 +virSecurityDACRestoreSecurityFileLabelInternal(virSecurityDACDataPtr priv,
 +   virStorageSourcePtr src,
 +   const char *path)
  {
 -VIR_INFO(Restoring DAC user and group on '%s', path);
 +VIR_INFO(Restoring DAC user and group on '%s',
 + NULLSTR(src ? src-path : path));
 
  /* XXX record previous ownership */
 -return virSecurityDACSetOwnership(path, 0, 0);
 +return virSecurityDACSetOwnershipInternal(priv, src, path, 0, 0);

I know this just follows the previous code, but in the big picture - how
does this play in with future patches where fs  gluster will
seemingly go though this path?


ACK - in general with the 'path' issue above understood.  For this code,
I'm mostly curious.

John
 +}
 +
 +
 +static int
 +virSecurityDACRestoreSecurityFileLabel(const char *path)
 +{
 +return virSecurityDACRestoreSecurityFileLabelInternal(NULL, NULL, path);
  }
 
 
 @@ -294,10 +343,6 @@ 
 virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
  if (!priv-dynamicOwnership)
  return 0;
 
 -/* XXX: Add support for gluster DAC permissions */
 -if (!src-path || !virStorageSourceIsLocalStorage(src))
 -return 0;
 -
  secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
  if (secdef  !secdef-relabel)
  return 0;
 @@ -315,7 +360,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr 
 mgr,
  return -1;
  }
 
 -return virSecurityDACSetOwnership(src-path, user, group);
 +return virSecurityDACSetOwnershipInternal(priv, src, NULL, user, group);
  }
 
 
 @@ -349,9 +394,6 @@ 
 virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
  if