4:32 p.m.
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>
---
src/security/security_dac.c | 49 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 40 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index be65d6e..7e11e31 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -66,28 +66,59 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
}
static
+int parseId(const char *str, unsigned int *id)
+{
+ char *endptr = NULL;
+
+ if (str == NULL || id == NULL)
+ return -1;
+
+ if (virStrToLong_ui(str, &endptr, 10, id) || endptr != NULL)
+ return -1;
+
+ return 0;
+}
+
+static
int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
{
+ int rc = -1;
unsigned int theuid;
unsigned int thegid;
- char *endptr = NULL;
+ char *sep = NULL;
+ char *tmp_label = NULL;
if (label == NULL)
- return -1;
+ goto done;
- if (virStrToLong_ui(label, &endptr, 10, &theuid) ||
- endptr == NULL || *endptr != ':') {
- return -1;
- }
+ tmp_label = strdup(label);
+ if (tmp_label == NULL)
+ goto done;
- if (virStrToLong_ui(endptr + 1, NULL, 10, &thegid))
- return -1;
+ sep = strchr(tmp_label, ':');
+ if (sep == NULL)
+ goto done;
+ *sep = '\0';
+
+ if (virGetUserID(tmp_label, &theuid) < 0 &&
+ parseId(tmp_label, &theuid) < 0)
+ goto done;
+
+ if (virGetGroupID(sep + 1, &thegid) < 0 &&
+ parseId(sep + 1, &thegid) < 0)
+ goto done;
if (uidPtr)
*uidPtr = theuid;
if (gidPtr)
*gidPtr = thegid;
- return 0;
+
+ rc = 0;
+
+done:
+ VIR_FREE(tmp_label);
+
+ return rc;
}
/* returns 1 if label isn't found, 0 on success, -1 on error */
--
1.7.12
7:31 p.m.
On 09/19/2012 03:32 PM, 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>
Question for migration - what if qemu is uid/gid 107 on machine A but
108 on machine B. Then it might be nice if an explicit use of
<label>107:107</label> says to preserve uid (but switch effective users)
on migration, while <label>qemu:qemu</label> says to preserve the user
identification, in spite of the change in underlying uid. And depending
on how the shared storage is accessed, such as via NFS with uid mapping,
preserving user name instead of uid might actually be important.
If that's true, then we need to enhance domain_conf.c to track whether
the user passed in ids or names in their XML. For that matter, to avoid
possible ambiguities (since it is legal [although stupid] to have a user
name consisting of all digits and worse having the name differ from the
underlying uid), should we have an optional attribute that gives us a
hint whether the contents are intended as an id number or as a string
name? That is, I wonder if we'd want something like:
<label type='id'>107:107</label>
<imagelabel type='name'>qemu:qemu</label>
except that what happens if I want to mix number and name between uid
and gid?
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8 a.m.
On 09/20/12 02:31, Eric Blake wrote:
On 09/19/2012 03:32 PM, 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>
Question for migration - what if qemu is uid/gid 107 on machine A but
108 on machine B. Then it might be nice if an explicit use of
<label>107:107</label> says to preserve uid (but switch effective users)
on migration, while <label>qemu:qemu</label> says to preserve the user
identification, in spite of the change in underlying uid. And depending
on how the shared storage is accessed, such as via NFS with uid mapping,
preserving user name instead of uid might actually be important.
If that's true, then we need to enhance domain_conf.c to track whether
the user passed in ids or names in their XML. For that matter, to avoid
possible ambiguities (since it is legal [although stupid] to have a user
name consisting of all digits and worse having the name differ from the
underlying uid), should we have an optional attribute that gives us a
hint whether the contents are intended as an id number or as a string
name? That is, I wonder if we'd want something like:
The other option (that I prefer more) would be to document this behavior
and leave it as proposed in this patch. Or maybe just reverse the order
of resolving so that the numerical parsing is done before name parsing.
If we warn the user, that specifying numerical values will result into
using them as UID and GID and strings being translated we filter out the
ambiguity by our design.
I don't think it's worth spending this effort on such a weird corner case.
<label type='id'>107:107</label>
<imagelabel type='name'>qemu:qemu</label>
except that what happens if I want to mix number and name between uid
and gid?
Mixing them will work in my approach presented here.
Peter
8:31 a.m.
On Thu, Sep 20, 2012 at 03:00:17PM +0200, Peter Krempa wrote:
On 09/20/12 02:31, Eric Blake wrote:
>On 09/19/2012 03:32 PM, 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>
>
>Question for migration - what if qemu is uid/gid 107 on machine A but
>108 on machine B. Then it might be nice if an explicit use of
><label>107:107</label> says to preserve uid (but switch effective users)
>on migration, while <label>qemu:qemu</label> says to preserve the user
>identification, in spite of the change in underlying uid. And depending
>on how the shared storage is accessed, such as via NFS with uid mapping,
>preserving user name instead of uid might actually be important.
>
>If that's true, then we need to enhance domain_conf.c to track whether
>the user passed in ids or names in their XML. For that matter, to avoid
>possible ambiguities (since it is legal [although stupid] to have a user
>name consisting of all digits and worse having the name differ from the
>underlying uid), should we have an optional attribute that gives us a
>hint whether the contents are intended as an id number or as a string
>name? That is, I wonder if we'd want something like:
The other option (that I prefer more) would be to document this
behavior and leave it as proposed in this patch. Or maybe just
reverse the order of resolving so that the numerical parsing is done
before name parsing.
If we warn the user, that specifying numerical values will result
into using them as UID and GID and strings being translated we
filter out the ambiguity by our design.
I don't think it's worth spending this effort on such a weird corner case.
I agree with Peter here. These ambiguities will be very rare and a
well-documented behavior should be enough for this scenario. However
reversing the order still can lead to ambiguities, because it's never
possible to be sure if a sequence of digits is a username or not. But
again, we just need to make this point clear in docs.
I'll update the documentation with this in a v2 patch if you're in
agreement.
>
><label type='id'>107:107</label>
><imagelabel type='name'>qemu:qemu</label>
>
>except that what happens if I want to mix number and name between uid
>and gid?
>
Mixing them will work in my approach presented here.
Peter
9:43 a.m.
On 09/20/2012 07:31 AM, Marcelo Cerri wrote:
>> possible ambiguities (since it is legal [although stupid] to
have a user
>> name consisting of all digits and worse having the name differ from the
>> underlying uid),
> The other option (that I prefer more) would be to document this
> behavior and leave it as proposed in this patch. Or maybe just
> reverse the order of resolving so that the numerical parsing is done
> before name parsing.
>
> If we warn the user, that specifying numerical values will result
> into using them as UID and GID and strings being translated we
> filter out the ambiguity by our design.
>
> I don't think it's worth spending this effort on such a weird corner case.
I agree with Peter here. These ambiguities will be very rare and a
well-documented behavior should be enough for this scenario. However
reversing the order still can lead to ambiguities, because it's never
possible to be sure if a sequence of digits is a username or not. But
again, we just need to make this point clear in docs.
The coreutils' solution in chown is that a name parse is attempted
before numeric parse, and that a leading '+' (which is not valid in
names) is the way to force a numeric parse. If anything, we should copy
that approach for consistency.
>> except that what happens if I want to mix number and name
between uid
>> and gid?
>>
>
> Mixing them will work in my approach presented here.
You still didn't answer my bigger question - when migrating, do we care
about the case where the same user name has different uid on the two
machines, and if so, do we make it possible for the user to choose
between migrating with constant uid vs. migrating with constant name?
If we always parse names into uids up front, then we are preventing the
user from migration by name.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:46 a.m.
On Thu, Sep 20, 2012 at 08:43:35AM -0600, Eric Blake wrote:
On 09/20/2012 07:31 AM, Marcelo Cerri wrote:
>>> possible ambiguities (since it is legal [although stupid] to have a user
>>> name consisting of all digits and worse having the name differ from the
>>> underlying uid),
>> The other option (that I prefer more) would be to document this
>> behavior and leave it as proposed in this patch. Or maybe just
>> reverse the order of resolving so that the numerical parsing is done
>> before name parsing.
>>
>> If we warn the user, that specifying numerical values will result
>> into using them as UID and GID and strings being translated we
>> filter out the ambiguity by our design.
>>
>> I don't think it's worth spending this effort on such a weird corner
case.
>
> I agree with Peter here. These ambiguities will be very rare and a
> well-documented behavior should be enough for this scenario. However
> reversing the order still can lead to ambiguities, because it's never
> possible to be sure if a sequence of digits is a username or not. But
> again, we just need to make this point clear in docs.
The coreutils' solution in chown is that a name parse is attempted
before numeric parse, and that a leading '+' (which is not valid in
names) is the way to force a numeric parse. If anything, we should copy
that approach for consistency.
>>> except that what happens if I want to mix number and name between uid
>>> and gid?
>>>
>>
>> Mixing them will work in my approach presented here.
You still didn't answer my bigger question - when migrating, do we care
about the case where the same user name has different uid on the two
machines, and if so, do we make it possible for the user to choose
between migrating with constant uid vs. migrating with constant name?
If we always parse names into uids up front, then we are preventing the
user from migration by name.
You can't migrate between different user IDs, because the target will
not be able to open the disk images - they will be labelled with the
user id of the source host and won't be changed.
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 :|
12:43 p.m.
On 09/20/2012 08:46 AM, Daniel P. Berrange wrote:
> You still didn't answer my bigger question - when migrating,
do we care
> about the case where the same user name has different uid on the two
> machines, and if so, do we make it possible for the user to choose
> between migrating with constant uid vs. migrating with constant name?
> If we always parse names into uids up front, then we are preventing the
> user from migration by name.
You can't migrate between different user IDs, because the target will
not be able to open the disk images - they will be labelled with the
user id of the source host and won't be changed.
Not if the two hosts are both accessing the same storage via NFS, and
NFS id mapping is in play; there, it is the username that is important
(because the name mapping converts the common username, even with
different ids on the source and destination machines, over to the real
uid on the NFS server).
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:34 p.m.
On Thu, Sep 20, 2012 at 08:43:35AM -0600, Eric Blake wrote:
On 09/20/2012 07:31 AM, Marcelo Cerri wrote:
>>> possible ambiguities (since it is legal [although stupid] to have a user
>>> name consisting of all digits and worse having the name differ from the
>>> underlying uid),
>> The other option (that I prefer more) would be to document this
>> behavior and leave it as proposed in this patch. Or maybe just
>> reverse the order of resolving so that the numerical parsing is done
>> before name parsing.
>>
>> If we warn the user, that specifying numerical values will result
>> into using them as UID and GID and strings being translated we
>> filter out the ambiguity by our design.
>>
>> I don't think it's worth spending this effort on such a weird corner
case.
>
> I agree with Peter here. These ambiguities will be very rare and a
> well-documented behavior should be enough for this scenario. However
> reversing the order still can lead to ambiguities, because it's never
> possible to be sure if a sequence of digits is a username or not. But
> again, we just need to make this point clear in docs.
The coreutils' solution in chown is that a name parse is attempted
before numeric parse, and that a leading '+' (which is not valid in
names) is the way to force a numeric parse. If anything, we should copy
that approach for consistency.
I really like this approach.
>>> except that what happens if I want to mix number and name between uid
>>> and gid?
>>>
>>
>> Mixing them will work in my approach presented here.
You still didn't answer my bigger question - when migrating, do we care
about the case where the same user name has different uid on the two
machines, and if so, do we make it possible for the user to choose
between migrating with constant uid vs. migrating with constant name?
If we always parse names into uids up front, then we are preventing the
user from migration by name.
With the current implementation, if names are given then names will be
honored in a migration and if IDs are given then IDs will be honored.
So, if a user has different IDs in different hosts and the seclabel was
defined using names, the owner's uid will be replaced when this guest is
migrated.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:53 a.m.
On 09/19/12 23:32, 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>
---
src/security/security_dac.c | 49 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 40 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index be65d6e..7e11e31 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -66,28 +66,59 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
}
static
+int parseId(const char *str, unsigned int *id)
+{
+ char *endptr = NULL;
+
+ if (str == NULL || id == NULL)
+ return -1;
+
+ if (virStrToLong_ui(str, &endptr, 10, id) || endptr != NULL)
After successful parse "endptr" will point to the '\0' byte at the end
of the string always thiggering this condition. Call virStrToLong_ui
with NULL instead of endptr and the wrapper will do the expected thing.
+ return -1;
+
+ return 0;
+}
+
+static
int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
{
+ int rc = -1;
unsigned int theuid;
unsigned int thegid;
- char *endptr = NULL;
+ char *sep = NULL;
+ char *tmp_label = NULL;
if (label == NULL)
- return -1;
+ goto done;
- if (virStrToLong_ui(label, &endptr, 10, &theuid) ||
- endptr == NULL || *endptr != ':') {
- return -1;
- }
+ tmp_label = strdup(label);
+ if (tmp_label == NULL)
You need to raise an OOM error with virReportOOMError() here even if
it's masked by the upper layer. This error gets recorded to the logs and
might help debugging.
I'm thinking if it's worth reporting other errors in this function as
they get masked. They might be useful for debugging purposes even if
they don't get propagated to the user.
+ goto done;
- if (virStrToLong_ui(endptr + 1, NULL, 10, &thegid))
- return -1;
+ sep = strchr(tmp_label, ':');
+ if (sep == NULL)
+ goto done;
+ *sep = '\0';
+
+ if (virGetUserID(tmp_label, &theuid) < 0 &&
+ parseId(tmp_label, &theuid) < 0)
+ goto done;
+
+ if (virGetGroupID(sep + 1, &thegid) < 0 &&
+ parseId(sep + 1, &thegid) < 0)
+ goto done;
if (uidPtr)
*uidPtr = theuid;
if (gidPtr)
*gidPtr = thegid;
- return 0;
+
+ rc = 0;
+
+done:
I'd rename this label to "cleanup" for consistency with other libvirt code.
+ VIR_FREE(tmp_label);
+
+ return rc;
}
/* returns 1 if label isn't found, 0 on success, -1 on error */
Peter
12:37 p.m.
On Thu, Sep 20, 2012 at 02:53:52PM +0200, Peter Krempa wrote:
On 09/19/12 23:32, 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>
>---
> src/security/security_dac.c | 49 ++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 40 insertions(+), 9 deletions(-)
>
>diff --git a/src/security/security_dac.c b/src/security/security_dac.c
>index be65d6e..7e11e31 100644
>--- a/src/security/security_dac.c
>+++ b/src/security/security_dac.c
>@@ -66,28 +66,59 @@ void virSecurityDACSetDynamicOwnership(virSecurityManagerPtr
mgr,
> }
>
> static
>+int parseId(const char *str, unsigned int *id)
>+{
>+ char *endptr = NULL;
>+
>+ if (str == NULL || id == NULL)
>+ return -1;
>+
>+ if (virStrToLong_ui(str, &endptr, 10, id) || endptr != NULL)
After successful parse "endptr" will point to the '\0' byte at the
end of the string always thiggering this condition. Call
virStrToLong_ui with NULL instead of endptr and the wrapper will do
the expected thing.
You're right.
>+ return -1;
>+
>+ return 0;
>+}
>+
>+static
> int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
> {
>+ int rc = -1;
> unsigned int theuid;
> unsigned int thegid;
>- char *endptr = NULL;
>+ char *sep = NULL;
>+ char *tmp_label = NULL;
>
> if (label == NULL)
>- return -1;
>+ goto done;
>
>- if (virStrToLong_ui(label, &endptr, 10, &theuid) ||
>- endptr == NULL || *endptr != ':') {
>- return -1;
>- }
>+ tmp_label = strdup(label);
>+ if (tmp_label == NULL)
You need to raise an OOM error with virReportOOMError() here even if
it's masked by the upper layer. This error gets recorded to the logs
and might help debugging.
I agree.
I'm thinking if it's worth reporting other errors in this function
as they get masked. They might be useful for debugging purposes even
if they don't get propagated to the user.
Do you think that describing each specific error with VIR_DEBUG should
be enough?
>+ goto done;
>
>- if (virStrToLong_ui(endptr + 1, NULL, 10, &thegid))
>- return -1;
>+ sep = strchr(tmp_label, ':');
>+ if (sep == NULL)
>+ goto done;
>+ *sep = '\0';
>+
>+ if (virGetUserID(tmp_label, &theuid) < 0 &&
>+ parseId(tmp_label, &theuid) < 0)
>+ goto done;
>+
>+ if (virGetGroupID(sep + 1, &thegid) < 0 &&
>+ parseId(sep + 1, &thegid) < 0)
>+ goto done;
>
> if (uidPtr)
> *uidPtr = theuid;
> if (gidPtr)
> *gidPtr = thegid;
>- return 0;
>+
>+ rc = 0;
>+
>+done:
I'd rename this label to "cleanup" for consistency with other libvirt code.
Ok.
>+ VIR_FREE(tmp_label);
>+
>+ return rc;
> }
>
> /* returns 1 if label isn't found, 0 on success, -1 on error */
>
Peter
4483
days inactive
4484
days old
9 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Marcelo Cerri -
Peter Krempa