Re: [libvirt] [PATCH RFC 1/3] security_dac: Remember owner prior chown() and restore on relabel
On 28.02.2013 02:22, Eric Blake wrote:
On 02/27/2013 02:25 AM, Michal Privoznik wrote:
>> Are you really planning on storing a string uid:gid? Wouldn't it be
>> simpler to store a uid_t and gid_t as read from struct stat, as long as
>> the data is only in memory? And when storing the data to disk in XML to
>> survive libvirtd restarts, it seems like storing two attributes
>> user='nnn' group='nnn' is nicer than storing one attribute
>> owner='+nnn:+nnn' that requires further parsing back into user and
group.
>
> My idea is; store userName:groupName whenever possible; When one of them
> is not accessible, use +NNN instead. The rationale is - whenever a user
> or a gropu changes its ID, we will follow it. For instance, a file X has
> owner A:B (1:1) but libvirt chowns to C:D. Meanwhile A's ID is changed
> from 1 to 2. So when relabeling we should chown to 2:1 instead of 1:1 as
> A's ID is 2 not 1. That means I have to do parsing and all the
> virAsprintf magic. However, maybe this is not what we want and I should
> really remember just numeric values of IDs which has nice tradeoff -
> much simpler code.
Migration and shared storage makes this problem so much tougher - the
uid on shared storage is common, but the name is not necessarily common.
I'd go for uid only; leave name lookups to each local machine
connecting to shared storage, but don't store the name ourselves.
Besides, admins tend not to change the name associated with a uid all
that frequently (it's generally one-time setup).
Dan has a point that you really need to involve the lock manager or use
some persistent storage (extended attribute or additional file in the
storage pool) alongside the file whose attributes we want to remember -
if a file is on shared storage, then it is the responsibility of the
last machine using the image to restore permissions, even if it was not
the machine that first did a chmod(). Merely storing a hash in just a
single libvirtd instance won't scale. Does our virlockd interface
support attaching attributes to a file as part of locking it?
Okay. You two has a point. I have not thought about that initially.
Anyway, if I go with xattrs, would it be safe? I mean, I'd need to
atomically get extended attribute for refCount. If there's none, get
current owner of the file and store it - again - as an extended
attribute among with refCount = 1. Any later libvirtd instance would
then just atomically increment/decrement refCount. And here's where I
see the trouble. In case of files protected by sanlock/virlockd we are
safe - the relabeling is done after we obtain the lock. But what about
files that are not protected by lock, e.g. <shared/> and <readonly/>?
Unless we lock them for the time we do the xattr magic, we cannot
guarantee atomicity, right? But this will extend need of usage of
sanlock/virlockd to nearly all cases, which is something we might not
want. Even if you have a couple of domains within a single host and
you're sharing an ISO image you'd need the lock daemon.
Would it suffice if my first draft does not deal with the explicit
locking of files we don't lock now? We can assume some luck, right? :)
Michal