So far, in the lock state we are storing only the file
descriptors of the files we've locked. Therefore, when unlocking
them and something does wrong the only thing we can report is FD
number, which is not user friendly at all. But if we store paths
among with FDs we can do better error reporting.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The S_ISSOCK macro is not available on Windows platforms.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently, when security driver is not available users are informed that
it wasn't found which can be confusing.
1. Update error message
2. Add comment to domain doc
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by Sebastian Mitterle <smitterl@redhat.com>
Use G_GNUC_UNUSED from GLib instead of ATTRIBUTE_UNUSED.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This function returns the name of the secdriver. Since the name
is invariant we don't really need to lock the manager - it won't
change.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
This function is in fact returning the name of the virtualization
driver that registered the security manager/driver.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
In upcoming commits, virSecurityManagerSetAllLabel() will perform
rollback in case of failure by calling
virSecurityManagerRestoreAllLabel(). But in order to do that, the
former needs to have @migrated argument so that it can be passed
to the latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
The usleep function was missing on older mingw versions, but we can rely
on it existing everywhere these days. It may only support times upto 1
second in duration though, so we'll prefer to use g_usleep instead.
The commandhelper program is not changed since that can't link to glib.
Fortunately it doesn't need to build on Windows platforms either.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
If there are two paths on the list that are the same we need to
lock it only once. Because when we try to lock it the second time
then open() fails. And if it didn't, locking it the second time
would fail for sure. After all, it is sufficient to lock all
paths just once satisfy the caller.
Reported-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Document why we need to sort paths while it's still fresh in my
memory.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
The purpose of this API is to allow caller move XATTRs (or remove
them) from one file to another. This will be needed when moving
top level of disk chain (either by introducing new HEAD or
removing it).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This reverts commit a5e1602090.
Getting rid of unistd.h from our headers will require more work than
just fixing the broken mingw build. Revert it until I have a more
complete proposal.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
util/virutil.h bogously included unistd.h. Drop it and replace it by
including it directly where needed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Now that we have replacement in the form of the image labeling function
we can drop the unnecessary functions by replacing all callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Security labeling of disks consists of labeling of the disk image
itself and it's backing chain. Modify
virSecurityManager[Set|Restore]ImageLabel to take a boolean flag that
will label the full chain rather than the top image itself.
This allows to delete/unify some parts of the code and will also
simplify callers in some cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.
In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.
With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to find the
author of a particular bit of code.
This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.
The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Trying to use virlockd to lock metadata turns out to be too big
gun. Since we will always spawn a separate process for relabeling
we are safe to use thread unsafe POSIX locks and take out
virtlockd completely out of the picture.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
When metadata locking is enabled that means the security commit
processing will be run in a fork similar to how namespaces use fork()'s
for processing. This is done to ensure libvirt can properly and
synchronously modify the metadata to store the original owner data.
Since fork()'s (e.g. virFork) have been seen as a performance bottleneck
being able to disable them allows the admin to choose whether the
performance 'hit' is worth the extra 'security' of being able to
remember the original owner of a lock.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Two new APIs are added so that security driver can lock and
unlock paths it wishes to touch. These APIs are not for other
drivers to call but security drivers (DAC and SELinux). That is
the reason these APIs are not exposed through our
libvirt_private.syms file.
Three interesting things happen in this commit. The first is the
global @lockManagerMutex. Unfortunately, this has to exist so that
there is only one thread talking to virtlockd at a time. If there
were more threads and one of them closed the connection
prematurely, it would cause virtlockd killing libvirtd. Instead
of complicated code that would handle that, let's have a mutex
and keep the code simple.
The second interesting thing is keeping connection open between
lock and unlock API calls. This is achieved by duplicating client
FD and keeping it open until unlock is called. This trick is used
by regular disk content locking code when the FD is leaked to
qemu.
Finally, the third thing is polling implemented at client side.
Since virtlockd has only one thread that handles locking
requests, all it can do is either acquire lock or error out.
Therefore, the polling has to be implemented in client. The
polling is capped at 60 second timeout, which should be plenty
since the metadata lock is held only for a fraction of a second.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Now that we know what metadata lock manager user wishes to use we
can load it when initializing security driver. This is achieved
by adding new argument to virSecurityManagerNewDriver() and
subsequently to all functions that end up calling it.
The cfg.mk change is needed in order to allow lock_manager.h
inclusion in security driver without 'syntax-check' complaining.
This is safe thing to do as locking APIs will always exist (it's
only backend implementation that changes). However, instead of
allowing the include for all other drivers (like cpu, network,
and so on) allow it only for security driver. This will still
trigger the error if including from other drivers.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
It will be desirable to run transactions more often than we
currently do. Even if the domain we're relabeling the paths for
does not run in a namespace. If that's the case, there is no need
to fork() as we are already running in the right namespace. To
differentiate whether transaction code should fork() or not the
@pid argument now accepts -1 (which means do not fork).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The virSecurityStackAddNested() can fail in which case
virSecurityManagerNewStack() should fail too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Nothing is setting that flag now so it can be removed. Note that
removing 'mgr' from 'load_profile' in the apparmor driver would create a
lot of churn.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In this patch we label the swtpm process with SELinux labels. We give it the
same label as the QEMU process has. We label its state directory and files
as well. We restore the old security labels once the swtpm has terminated.
The file and process labels now look as follows:
Directory: /var/lib/libvirt/swtpm
[root@localhost swtpm]# ls -lZ
total 4
rwx------. 2 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 4096 Apr 5 16:46 testvm
[root@localhost testvm]# ls -lZ
total 8
-rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 3648 Apr 5 16:46 tpm-00.permall
The log in /var/log/swtpm/libvirt/qemu is labeled as follows:
-rw-r--r--. 1 tss tss system_u:object_r:svirt_image_t:s0:c254,c932 2237 Apr 5 16:46 vtpm.log
[root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep swtpm | grep ctrl | grep -v grep
system_u:system_r:svirt_t:s0:c254,c932 tss 25664 0.0 0.0 28172 3892 ? Ss 16:57 0:00 /usr/bin/swtpm socket --daemon --ctrl type=unixio,path=/var/run/libvirt/qemu/swtpm/testvm-swtpm.sock,mode=0660 --tpmstate dir=/var/lib/libvirt/swtpm/testvm/tpm1.2 --log file=/var/log/swtpm/libvirt/qemu/testvm-swtpm.log
[root@localhost 485d0004-a48f-436a-8457-8a3b73e28567]# ps auxZ | grep qemu | grep tpm | grep -v grep
system_u:system_r:svirt_t:s0:c254,c932 qemu 25669 99.0 0.0 3096704 48500 ? Sl 16:57 3:28 /bin/qemu-system-x86_64 [..]
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
So far we are repeating the following lines over and over:
if (!(virSomeObjectClass = virClassNew(virClassForObject(),
"virSomeObject",
sizeof(virSomeObject),
virSomeObjectDispose)))
return -1;
While this works, it is impossible to do some checking. Firstly,
the class name (the 2nd argument) doesn't match the name in the
code in all cases (the 3rd argument). Secondly, the current style
is needlessly verbose. This commit turns example into following:
if (!(VIR_CLASS_NEW(virSomeObject,
virClassForObject)))
return -1;
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
virSecurityManagerDomainSetPathLabel is used to make a path known
to the security modules, but today is used interchangably for
- paths to files/dirs to be accessed directly
- paths to a dir, but the access will actually be to files therein
Depending on the security module it is important to know which of
these types it will be.
The argument allowSubtree augments the call to the implementations of
DomainSetPathLabel that can - per security module - decide if extra
actions shall be taken.
For now dac/selinux handle this as before, but apparmor will make
use of it to add a wildcard to the path that was passed.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
SELinux and DAC drivers already have both functions but they were not
exported as public API of security manager.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Seeing a log message saying 'flags=93' is ambiguous & confusing unless
you happen to know that libvirt always prints flags as hex. Change our
debug messages so that they always add a '0x' prefix when printing flags,
and '0' prefix when printing mode. A few other misc places gain a '0x'
prefix in error messages too.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The VIR_SECURITY_MANAGER_MOUNT_NAMESPACE flag informs the DAC driver
if mount namespaces are in use for the VM. Will be used for future
changes.
Wire it up in the qemu driver
When security drivers are active but confinement is not enabled,
there is no need to autogenerate <seclabel> elements when starting
a domain def that contains no <seclabel> elements. In fact,
autogenerating the elements can result in needless save/restore and
migration failures when the security driver is not active on the
restore/migration target.
This patch changes the virSecurityManagerGenLabel function in
src/security_manager.c to only autogenerate a <seclabel> element
if none is already defined for the domain *and* default
confinement is enabled. Otherwise the needless <seclabel>
autogeneration is skipped.
Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017
In the case that virtlogd is used as stdio handler we pass to QEMU
only FD to a PIPE connected to virtlogd instead of the file itself.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
With our new qemu namespace code in place, the relabelling of
devices is done not as good is it could: a child process is
spawned, it enters the mount namespace of the qemu process and
then runs desired API of the security driver.
Problem with this approach is that internal state transition of
the security driver done in the child process is not reflected in
the parent process. While currently it wouldn't matter that much,
it is fairly easy to forget about that. We should take the extra
step now while this limitation is still fresh in our minds.
Three new APIs are introduced here:
virSecurityManagerTransactionStart()
virSecurityManagerTransactionCommit()
virSecurityManagerTransactionAbort()
The Start() is going to be used to let security driver know that
we are starting a new transaction. During a transaction no
security labels are actually touched, but rather recorded and
only at Commit() phase they are actually updated. Should
something go wrong Abort() aborts the transaction freeing up all
memory allocated by transaction.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Since its introduction in 2012 this internal API did nothing.
Moreover we have the same API that does exactly the same:
virSecurityManagerDomainSetPathLabel.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
We have macros for both positive and negative string matching.
Therefore there is no need to use !STREQ or !STRNEQ. At the same
time as we are dropping this, new syntax-check rule is
introduced to make sure we won't introduce it again.
Signed-off-by: Ishmanpreet Kaur Khera <khera.ishman@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
So imagine you want to crate new security manager:
if (!(mgr = virSecurityManagerNew("selinux", "QEMU", false, true, false, true)));
Hard to parse, right? What about this:
if (!(mgr = virSecurityManagerNew("selinux", "QEMU",
VIR_SECURITY_MANAGER_DEFAULT_CONFINED |
VIR_SECURITY_MANAGER_PRIVILEGED)));
Now that's better! This is what the commit does.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
We may want to do some decisions in drivers based on fact if we
are running as privileged user or not. Propagate this info there.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
if (mgr == NULL || mgr->drv == NULL)
return ret;
This check isn't really necessary, security manager cannot be a NULL
pointer as it is either selinux (by default) or 'none', if no other driver is
set in the config. Even with no config file driver name yields 'none'.
The other hunk checks for domain's security model validity, but we should
also check devices' security model as well, therefore this hunk is moved into
a separate function which is called by virSecurityManagerCheckAllLabel that
checks both the domain's security model and devices' security model.
https://bugzilla.redhat.com/show_bug.cgi?id=1165485
Signed-off-by: Ján Tomko <jtomko@redhat.com>
We do have a check for valid per-domain security model, however we still
do permit an invalid security model for a domain's device (those which
are specified with <source> element).
This patch introduces a new function virSecurityManagerCheckAllLabel
which compares user specified security model against currently
registered security drivers. That being said, it also permits 'none'
being specified as a device security model.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165485
Signed-off-by: Ján Tomko <jtomko@redhat.com>
To integrate the security driver with the storage driver we need to
pass a callback for a function that will chown storage volumes.
Introduce and document the callback prototype.
This negation in names of boolean variables is driving me insane. The
code is much more readable if we drop the 'no-' prefix. Well, at least
for me.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
