[libvirt] [PATCH] security: also parse user/group names instead of just IDs for DAC labels
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
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 :|
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Marcelo Cerri -
Peter Krempa