On 5/12/21 3:33 PM, Daniel P. Berrangé wrote: > If you define a secret with private="yes", then libvirt won't let any > client query the secret value after it is set. Only other libvirt > drivers inside the daemon can query it by passing a special internal > only flag to the virSecretGetValue API. The remote driver/daemon > refuses to let this internal flag go over the wire preventing normal > clients from using it > > This doesn't work with the split daemons because the virSecretGetValue > API done by virqemud / virtstoraged has to go over the wire to reach > the virsecretd. > > We need to come up with an alternative way to "prove" that the caller > of virSecretGetValue is a libvirt daemon, as opposed to a general > libvirt client. > > Note with if only traditional POSIX DAC permissions are in effect > then we could consider it pointless trying to restrict access to > clients running the same user/group as the libvirt daemon. We ought > to take into account that the client might be confined by SELinux > though, so the "private secret" concept isn't entirely pointless. > > Thus doing a simple uid of client == uid of daemon check is a bit > too weak. The UID check might also not fly if the modular daemons > are run inside containers with user namespaces, as the container > for virtsecretd and virtqemud might have different user mappings > in theory. > > This series adds a concept of a "token" which is known only to the > libvirt daemons. The first daemon to use it writes a random hex > string to /var/run/libvirt/common/system.token. Other daemons can > read and compare this. Unless a MAC system is present this is still > largely security theatre, but that's not really worse than the > historical behaviour. > > When an API call is made the virIdentity by default reflects the > identity of the UNIX process that initiated it. > > When connecting to virtproxyd, the client apps' identity is forwarded > to the next virtNNNNd daemon. > > When libvirt drivers, however, initiate an API call we never set any > identity. With monolithic libvirtd, they'd inherit the current client > identity automagically since it was all in the same thread local. With > modular daemons the othe driver would see the identity of the other > libvirt daemon which is bad as this gives elevated privileges in the > ACL check. > > Thus we fix the code which drivers use to open a connection to other > daemons, such that it applies the current caller's identity. It does > this using an "elevated" identity though, which means, we have added > in the system token. Thus the virtsecretd daemon getting the call > virSecretGetValue sees the virIdentity reflecting the client > application which originally called the virDomainCreate() API, but > with the system token set. Thus virsecretd can see that the > virSecretGetValue was invoked by another daemon, not a libvirt > client app. > > Changed in v3... > > Properly mock the new APIs in test suite > > Changed in v2... > > We can't set the elevated identity only when opening the virConnect > for the secret driver. This works for modular daemons, as the identity > is passed to the virsecretd at time of opening and thus applies to > the later virSecretGetValue call on that connection. > > For monolithic daemon, the identity present at virConnectOpen is > irrelevant. The virSecretGetValue call will just directly query > the current thread's identity. > > IOW, to work in both deployment scenarios we need to have the > elevated identity set across both virConnectOpen and virSecretGetValue > > Daniel P. Berrangé (10): > util: add virRandomToken API > util: introduce concept of a system token into identities > util: generate a persistent system token > util: set system token for system identity > util: add API for copying identity objects > util: helper to temporary elevate privileges of the current identity > src: add API to determine if current identity is a system identity > src: set identity when opening secondary drivers > src: elevate current identity privilege when fetching secret > secret: rework handling of private secrets > > src/driver-secret.h | 9 +- > src/driver.c | 27 +++ > src/libvirt-secret.c | 2 +- > src/libvirt_private.syms | 8 + > src/libxl/libxl_conf.c | 5 + > src/qemu/qemu_domain.c | 11 +- > src/qemu/qemu_tpm.c | 5 + > src/remote/remote_driver.c | 8 +- > src/secret/secret_driver.c | 34 ++- > src/storage/storage_backend_iscsi.c | 5 + > src/storage/storage_backend_iscsi_direct.c | 5 + > src/storage/storage_backend_rbd.c | 5 + > src/storage/storage_util.c | 5 + > src/util/viridentity.c | 244 ++++++++++++++++++++- > src/util/viridentity.h | 11 + > src/util/viridentitypriv.h | 30 +++ > src/util/virrandom.c | 18 ++ > src/util/virrandom.h | 1 + > src/util/virsecret.c | 3 +- > tests/qemuxml2argvmock.c | 9 + > tests/qemuxml2argvtest.c | 9 +- > tests/viridentitytest.c | 11 +- > 22 files changed, 435 insertions(+), 30 deletions(-) > create mode 100644 src/util/viridentitypriv.h >
Reviewed-by: Michal Privoznik <mpriv...@redhat.com> Michal
