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 <mprivozn(a)redhat.com>
Michal