Re: [PATCH v3 1/2] security: also parse user/group names instead of just IDs for DAC labels

[Peter Krempa <pkrempa@xxxxxxxxxx>]

On 10/02/12 19:57, Marcelo Cerri wrote:
> The DAC driver is missing parsing of group and user names for DAC labels
> and currently just parses uid and gid. This patch extends it to support
> names, so the following security label definition is now valid:
> 
>    <seclabel type='static' model='dac' relabel='yes'>
>        <label>qemu:qemu</label>
>        <imagelabel>qemu:qemu</imagelabel>
>    </seclabel>
> 
> When it tries to parse an owner or a group, it first tries to resolve it as
> a name, if it fails or it's an invalid user/group name then it tries to
> parse it as an UID or GID. A leading '+' can also be used for both owner and
> group to force it to be parsed as IDs, so the following example is also
> valid:
> 
>    <seclabel type='static' model='dac' relabel='yes'>
>        <label>+101:+101</label>
>        <imagelabel>+101:+101</imagelabel>
>    </seclabel>
> 
> This ensures that UID 101 and GUI 101 will be used instead of an user or
> group named "101".
> ---
>   src/security/security_dac.c | 92 ++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 70 insertions(+), 22 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 0fa66ce..bb2c6be 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -68,26 +68,79 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
>   static
>   int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
>   {
>   
>   /* returns 1 if label isn't found, 0 on success, -1 on error */
> @@ -103,16 +156,14 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)
>   
>       seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
>       if (seclabel == NULL || seclabel->label == NULL) {
> -        VIR_DEBUG("DAC seclabel for domain '%s' wasn't found", def->name);
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("DAC seclabel for domain '%s' wasn't found"),
> +                       def->name);

This error will be masked so the VIR_DEBUG statement was fine here.

>           return 1;
>       }
>   
> -    if (seclabel->label && parseIds(seclabel->label, &uid, &gid)) {
> -       virReportError(VIR_ERR_INVALID_ARG,
> -                       _("failed to parse DAC seclabel '%s' for domain '%s'"),
> -                       seclabel->label, def->name);
> +    if (parseIds(seclabel->label, &uid, &gid) < 0)
>           return -1;
> -    }
>   
>       if (uidPtr)
>           *uidPtr = uid;
> @@ -168,17 +219,14 @@ int virSecurityDACParseImageIds(virDomainDefPtr def,
>   
>       seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
>       if (seclabel == NULL || seclabel->imagelabel == NULL) {
> -        VIR_DEBUG("DAC imagelabel for domain '%s' wasn't found", def->name);
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("DAC imagelabel for domain '%s' wasn't found"),
> +                       def->name);

And same here.

>           return 1;
>       }
>   
> -    if (seclabel->imagelabel
> -        && parseIds(seclabel->imagelabel, &uid, &gid)) {
> -       virReportError(VIR_ERR_INVALID_ARG,
> -                       _("failed to parse DAC imagelabel '%s' for domain '%s'"),
> -                       seclabel->imagelabel, def->name);
> +    if (parseIds(seclabel->imagelabel, &uid, &gid) < 0)
>           return -1;
> -    }

Otherwise looks good. So I'm going to push this with following changes 
squashed in:

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index bb2c6be..a427e9d 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -156,9 +156,7 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)

     seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
     if (seclabel == NULL || seclabel->label == NULL) {
-        virReportError(VIR_ERR_INVALID_ARG,
-                       _("DAC seclabel for domain '%s' wasn't found"),
-                       def->name);
+        VIR_DEBUG("DAC seclabel for domain '%s' wasn't found", def->name);
         return 1;
     }

@@ -219,9 +217,7 @@ int virSecurityDACParseImageIds(virDomainDefPtr def,

     seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
     if (seclabel == NULL || seclabel->imagelabel == NULL) {
-        virReportError(VIR_ERR_INVALID_ARG,
-                       _("DAC imagelabel for domain '%s' wasn't found"),
-                       def->name);
+        VIR_DEBUG("DAC imagelabel for domain '%s' wasn't found", def->name);
         return 1;
     }

Peter

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list