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 <mpriv...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
