On Thu, Aug 22, 2019 at 13:15:33 +0200, Michal Privoznik wrote:
It may happen that we leave some XATTRs behind. For instance, on
a sudden power loss, the host just shuts down without calling
restore on domain paths. This creates a problem, because when the
host starts up again, the XATTRs are there but they don't reflect
the true state and this may result in libvirt denying start of a
domain.
To solve this, save a unique timestamp (host boot time) among
with our XATTRs.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/security/security_util.c | 196 +++++++++++++++++++++++++++++++-
tests/qemusecuritymock.c | 12 ++
tools/libvirt_recover_xattrs.sh | 2 +-
3 files changed, 208 insertions(+), 2 deletions(-)
diff --git a/src/security/security_util.c b/src/security/security_util.c
index 365b2dd2d6..c65e27e6d4 100644
--- a/src/security/security_util.c
+++ b/src/security/security_util.c
@@ -22,11 +22,16 @@
#include "virfile.h"
#include "virstring.h"
#include "virerror.h"
+#include "virlog.h"
+#include "viruuid.h"
+#include "virhostuptime.h"
#include "security_util.h"
#define VIR_FROM_THIS VIR_FROM_SECURITY
+VIR_LOG_INIT("security.security_util");
+
/* There are four namespaces available on Linux (xattr(7)):
*
* user - can be modified by anybody,
@@ -83,6 +88,151 @@ virSecurityGetRefCountAttrName(const char *name ATTRIBUTE_UNUSED)
}
+#ifdef XATTR_NAMESPACE
+static char *
+virSecurityGetTimestampAttrName(const char *name)
+{
+ char *ret = NULL;
+ ignore_value(virAsprintf(&ret,
XATTR_NAMESPACE".libvirt.security.timestamp_%s", name));
I'd put a space between XATTR_NAMESPACE and ".
+ return ret;
+}
+#else /* !XATTR_NAMESPACE */
+static char *
+virSecurityGetTimestampAttrName(const char *name ATTRIBUTE_UNUSED)
+{
+ errno = ENOSYS;
+ virReportSystemError(errno, "%s",
+ _("Extended attributes are not supported on this
system"));
+ return NULL;
+}
+#endif /* !XATTR_NAMESPACE */
+
+
+static char *
+virSecurityGetTimestamp(void)
+{
+ unsigned long long boottime = 0;
+ char *ret = NULL;
+
+ if (virHostGetBootTime(&boottime) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to get host boot time"));
+ return NULL;
+ }
+
+ ignore_value(virAsprintf(&ret, "%llu", boottime));
+ return ret;
+}
+
+
+/**
+ * virSecurityValidateTimestamp:
+ * @name: security driver name
+ * @path: file name
+ *
+ * Check if remembered label on @path for security driver @name
+ * is valid, i.e. the label has been set since the last boot. If
+ * the label was set in previous runs, all XATTRs related to
+ * @name are removed so that clean slate is restored.
+ *
+ * This is done having extra attribute timestamp_$SECDRIVER which
+ * contains the host boot time. Its value is then compared to
+ * actual host boot time. If these two values don't match then
+ * XATTRs are considered as stale and thus invalid.
+ *
+ * In ideal world, where there network file systems have XATTRs
+ * using plain host boot time is not enough as it may lead to a
+ * situation where a freshly started host sees XATTRs, sees the
+ * timestamp put there by some longer running host and considers
+ * the XATTRs invalid. Well, there is not an easy way out. We
+ * would need to somehow check if the longer running host is
+ * still there and is the @path (how?).
Looks like there is a missing word in the sentence above.
+ * Fortunately, there is only one network file system which
+ * supports XATTRs currently (GlusterFS via FUSE) and it is used
+ * so rarely that it's almost a corner case.
+ * The worst thing that happens there is that we remove XATTRs
+ * and thus return @path to the default label for $SECDRIVER.
+ *
+ * Returns: 0 if remembered label is valid,
+ * 1 if remembered label was not valid,
+ * -1 otherwise.
+ */
+static int
+virSecurityValidateTimestamp(const char *name,
+ const char *path)
+{
+ VIR_AUTOFREE(char *) expected_timestamp = NULL;
+ VIR_AUTOFREE(char *) timestamp_name = NULL;
+ VIR_AUTOFREE(char *) value = NULL;
+
+ if (!(expected_timestamp = virSecurityGetTimestamp()) ||
+ !(timestamp_name = virSecurityGetTimestampAttrName(name)))
+ return -1;
+
+ errno = 0;
+ if (virFileGetXAttrQuiet(path, timestamp_name, &value) < 0) {
+ if (errno == ENOSYS || errno == ENOTSUP) {
+ return -1;
This is the only path which returns -1 without setting an error. I know
it's because the caller will check errno and do something else then with
other paths that return -1, but it's weird. I think it would be much
better to just return -2 here and let the caller use only the return
value without looking at errno.
+ } else if (errno != ENODATA) {
+ virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ timestamp_name,
+ path);
+ return -1;
+ }
+
+ /* Timestamp is missing. We can continue and claim a valid timestamp.
s/can/could/
+ * But then we would never remove stale XATTRs. Therefore,
claim it
+ * invalid and have the code below remove all XATTRs. This of course
+ * means that we will not restore the original owner, but the plus side
+ * is that we reset refcounter which will represent the true state.
+ */
+ }
+
+ if (STREQ_NULLABLE(value, expected_timestamp)) {
+ VIR_DEBUG("XATTRs on %s secdriver=%s are valid", path, name);
+ return 0;
+ }
+
+ VIR_WARN("Invalid XATTR timestamp detected on %s secdriver=%s", path,
name);
+
+ if (virSecurityMoveRememberedLabel(name, path, NULL) < 0)
+ return -1;
+
+ return 1;
+}
+
+
+static int
+virSecurityAddTimestamp(const char *name,
+ const char *path)
+{
+ VIR_AUTOFREE(char *) timestamp_name = NULL;
+ VIR_AUTOFREE(char *) timestamp_value = NULL;
+
+ if (!(timestamp_name = virSecurityGetTimestampAttrName(name)))
Missing timestamp_value = virSecurityGetTimestamp()
+ return -1;
+
+ return virFileSetXAttr(path, timestamp_name, timestamp_value);
+}
+
+
+static int
+virSecurityRemoveTimestamp(const char *name,
+ const char *path)
+{
+ VIR_AUTOFREE(char *) timestamp_name = NULL;
+
+ if (!(timestamp_name = virSecurityGetTimestampAttrName(name)))
+ return -1;
+
+ if (virFileRemoveXAttr(path, timestamp_name) < 0 && errno != ENOENT)
+ return -1;
+
+ return 0;
+}
+
+
/**
* virSecurityGetRememberedLabel:
* @name: security driver name
@@ -120,6 +270,12 @@ virSecurityGetRememberedLabel(const char *name,
*label = NULL;
+ if (virSecurityValidateTimestamp(name, path) < 0) {
Here you could just return what virSecurityValidateTimestamp returned if
you follow what I suggested above.
+ if (errno == ENOSYS || errno == ENOTSUP)
+ return -2;
+ return -1;
+ }
+
if (!(ref_name = virSecurityGetRefCountAttrName(name)))
return -1;
@@ -163,6 +319,9 @@ virSecurityGetRememberedLabel(const char *name,
if (virFileRemoveXAttr(path, attr_name) < 0)
return -1;
+
+ if (virSecurityRemoveTimestamp(name, path) < 0)
+ return -1;
}
return 0;
@@ -199,6 +358,12 @@ virSecuritySetRememberedLabel(const char *name,
VIR_AUTOFREE(char *) value = NULL;
unsigned int refcount = 0;
+ if (virSecurityValidateTimestamp(name, path) < 0) {
Here you could just return what virSecurityValidateTimestamp returned if
you follow what I suggested above.
+ if (errno == ENOSYS || errno == ENOTSUP)
+ return -2;
+ return -1;
+ }
+
if (!(ref_name = virSecurityGetRefCountAttrName(name)))
return -1;
...
Jirka