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@redhat.com>
Michal