[libvirt PATCH 3/9] util: generate a persistent system token

[Michal Prívozník]

On 5/4/21 7:43 PM, Daniel P. Berrangé wrote:
> When creating the system identity set the system token. The system
> token is currently stored in a local path
> 
>    /var/run/libvirt/common/system.token
> 
> Obviously with only traditional UNIX DAC in effect, this is largely
> security through obscurity, if the client is running at the same
> privilege level as the daemon. It does, however, reliably distinguish
> an unprivilegd client from the system daemons.
> 
> With a MAC system like SELinux though, or possible use of containers,
> access can be further restricted.
> 
> A possible future improvement for Linux would be to populate the
> kernel keyring with a secret for libvirt daemons to share.
> 
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>  src/util/viridentity.c | 107 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/src/util/viridentity.c b/src/util/viridentity.c
> index 7da4ea12f5..065db06e49 100644
> --- a/src/util/viridentity.c
> +++ b/src/util/viridentity.c
> @@ -22,6 +22,7 @@
>  #include <config.h>
>  
>  #include <unistd.h>
> +#include <fcntl.h>
>  #if WITH_SELINUX
>  # include <selinux/selinux.h>
>  #endif
> @@ -32,11 +33,14 @@
>  #include "viridentity.h"
>  #include "virlog.h"
>  #include "virobject.h"
> +#include "virrandom.h"
>  #include "virthread.h"
>  #include "virutil.h"
>  #include "virstring.h"
>  #include "virprocess.h"
>  #include "virtypedparam.h"
> +#include "virfile.h"
> +#include "configmake.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_IDENTITY
>  
> @@ -54,7 +58,10 @@ struct _virIdentity {
>  
>  G_DEFINE_TYPE(virIdentity, vir_identity, G_TYPE_OBJECT)
>  
> +static char *virIdentityEnsureSystemToken(void);
> +
>  static virThreadLocal virIdentityCurrent;
> +static char *systemToken;
>  
>  static void virIdentityFinalize(GObject *obj);
>  
> @@ -73,6 +80,9 @@ static int virIdentityOnceInit(void)
>          return -1;
>      }
>  
> +    if (!(systemToken = virIdentityEnsureSystemToken()))
> +        return -1;
> +
>      return 0;
>  }
>  
> @@ -144,6 +154,103 @@ int virIdentitySetCurrent(virIdentity *ident)
>  }
>  
>  
> +#define TOKEN_BYTES 16
> +#define TOKEN_STRLEN (TOKEN_BYTES * 2)
> +
> +static char *
> +virIdentityConstructSystemTokenPath(void)
> +{
> +    g_autofree char *commondir = NULL;
> +    if (geteuid() == 0) {
> +        commondir = g_strdup(RUNSTATEDIR "/libvirt/common");
> +    } else {
> +        g_autofree char *rundir = virGetUserRuntimeDirectory();
> +        commondir = g_strdup_printf("%s/common", rundir);
> +    }
> +
> +    if (g_mkdir_with_parents(commondir, 0700) < 0) {
> +        virReportSystemError(errno,
> +                             _("Cannot create daemon common directory '%s'"),
> +                             commondir);
> +        return NULL;
> +    }
> +
> +    return g_strdup_printf("%s/system.token", commondir);
> +}
> +
> +
> +static char *
> +virIdentityEnsureSystemToken(void)
> +{
> +    g_autofree char *tokenfile = virIdentityConstructSystemTokenPath();
> +    g_autofree char *token = NULL;
> +    int fd = -1;

Or VIR_AUTOCLOSE fd = -1 and drop those explicit close calls at the end.

> +    struct stat st;
> +
> +    fd = open(tokenfile, O_RDWR|O_APPEND|O_CREAT, 0600);
> +    if (fd < 0) {
> +        virReportSystemError(errno,
> +                             _("Unable to open system token %s"),
> +                             tokenfile);
> +        goto error;
> +    }
> +
> +    if (virSetCloseExec(fd) < 0) {

I know we have this pattern in many areas, but it's inherently racy.
What's stopping us from passing O_CLOEXEC to open()? IIUC, O_CLOEXEC
will exist on mingw where it's just an alias to O_NOINHERIT.

> +        virReportSystemError(errno,
> +                             _("Failed to set close-on-exec flag '%s'"),
> +                             tokenfile);
> +        goto error;
> +    }

Michal