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

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(a)redhat.com&gt; > --- > 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