[libvirt PATCH 0/9] make internal only secrets work with split daemons

Daniel P. Berrangé posted 9 patches 1 week, 6 days ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210504174350.488942-1-berrange@redhat.com
src/driver-secret.h        |   9 +-
src/driver.c               |  27 +++++
src/libvirt-secret.c       |   2 +-
src/libvirt_private.syms   |   5 +
src/remote/remote_driver.c |   8 +-
src/secret/secret_driver.c |  34 ++++--
src/util/viridentity.c     | 230 +++++++++++++++++++++++++++++++++++++
src/util/viridentity.h     |   7 ++
src/util/virrandom.c       |  18 +++
src/util/virrandom.h       |   1 +
src/util/virsecret.c       |   3 +-
tests/qemuxml2argvtest.c   |   3 +-
12 files changed, 320 insertions(+), 27 deletions(-)

[libvirt PATCH 0/9] make internal only secrets work with split daemons

Posted by Daniel P. Berrangé 1 week, 6 days ago
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.

Daniel P. Berrangé (9):
  util: add virRandomToken API
  util: introduce concept of a system token into identities
  util: generate a persistent system token
  src: set system token for system identity
  util: add API for copying identity objects
  util: add method for getting the current identity with system token
  src: add API to determine if current identity is a system identity
  secret: rework handling of private secrets
  src: set identity when opening secondary drivers

 src/driver-secret.h        |   9 +-
 src/driver.c               |  27 +++++
 src/libvirt-secret.c       |   2 +-
 src/libvirt_private.syms   |   5 +
 src/remote/remote_driver.c |   8 +-
 src/secret/secret_driver.c |  34 ++++--
 src/util/viridentity.c     | 230 +++++++++++++++++++++++++++++++++++++
 src/util/viridentity.h     |   7 ++
 src/util/virrandom.c       |  18 +++
 src/util/virrandom.h       |   1 +
 src/util/virsecret.c       |   3 +-
 tests/qemuxml2argvtest.c   |   3 +-
 12 files changed, 320 insertions(+), 27 deletions(-)

-- 
2.31.1


Re: [libvirt PATCH 0/9] make internal only secrets work with split daemons

Posted by Michal Prívozník 1 week, 5 days ago
On 5/4/21 7:43 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.
> 
> Daniel P. Berrangé (9):
>   util: add virRandomToken API
>   util: introduce concept of a system token into identities
>   util: generate a persistent system token
>   src: set system token for system identity
>   util: add API for copying identity objects
>   util: add method for getting the current identity with system token
>   src: add API to determine if current identity is a system identity
>   secret: rework handling of private secrets
>   src: set identity when opening secondary drivers
> 
>  src/driver-secret.h        |   9 +-
>  src/driver.c               |  27 +++++
>  src/libvirt-secret.c       |   2 +-
>  src/libvirt_private.syms   |   5 +
>  src/remote/remote_driver.c |   8 +-
>  src/secret/secret_driver.c |  34 ++++--
>  src/util/viridentity.c     | 230 +++++++++++++++++++++++++++++++++++++
>  src/util/viridentity.h     |   7 ++
>  src/util/virrandom.c       |  18 +++
>  src/util/virrandom.h       |   1 +
>  src/util/virsecret.c       |   3 +-
>  tests/qemuxml2argvtest.c   |   3 +-
>  12 files changed, 320 insertions(+), 27 deletions(-)
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal