Fix security flaw in username/password handling for libvirt connections

[berrange]

Providing a password via a command line argument is a security flaw since that is exposed all users / services on a host. This removes the ability to provide passwords on the cli in several commands. This is not even needed by the libvirt images built for kubevirt, but for custom setups it is now possible to do this out of band via the libvirt client auth config.

[kubevirt-bot]

Can one of the admins verify this patch?

[stu-gott]

You are correct that the default KubeVirt implementation doesn't use a password as it connects over local Unix socket. You are also correct that supplying passwords on the command line is a terrible idea. However I'm concerned that the assertion that a user can configure authentication using an out of band mechanism via the libvirt client auth config might preclude them from being able to use the virt-handler pod without therefore hard coding said config into a custom docker image. Instead of outright removing the ability to provide a password altogether, what if we switched to using an environment variable? That would still be visible to parties with access to the kubernetes manifest, but I think it addresses the specific concern you've raised.

[rmohr]

That it considers the auth file is good. Having environment variables for username and password makes imho sense too. Like @stu-gott pointed out, it would require docker image changes to bring in the auth file otherwise (e.g. a startup script which creates the config via environment variables).

[fabiand]

Another option is that the libvirt client config is created in the fly as a part of the container start process.
I.e. by the handler itself, by deriving credentials form a Secret or alike.

Actually - If we run the libvirtd container inside the virt-handler pod, then we might not need any authentication at all, if libvirt binds itself to localhost via tcp without authentication.
There should not be a security risk, because connections can only be made within the pod.
Assumption here is that th elibvirtd container uses pod-networking.

[berrange]

Use of environment variables for authentication is just as bad as passing them on the CLI. For UNIX domain sockets connections to libvirt we don't need any authentication at all. The POD running virt-handler will be the only pod in which the UNIX domain socket for libvirtd is visible (via a host mount), no other PODs would be able to access it. Authentication against a remote libvirtd over TCP is a completely different matter - in that case we need TLS certificates, with one-time use only passwords configured dynamically at time the migration is started. So aside from this being insecure, it is also pointless.

[fabiand]

Maybe derailing, but I have a few questions.

The POD running virt-handler will be the only pod in which the UNIX domain socket for libvirtd is visible (via a host mount), no other PODs would be able to access it.

In previous work we ran into problems when the uids between the images of libvirt and the virt-handler differed. Thus I wonder if you see problems with the UNIX socket approach wrt uids and/or user missmatch between client and server.
One solution to this could be to let the virt-handler inherit the libvirt image.

Besides that we probably need the UNIX socket method, because we will use it to determine the right cgroup to use.

Authentication against a remote libvirtd over TCP is a completely different matter - in that case we need TLS certificates, with one-time use only passwords configured dynamically at time the migration is started. So aside from this being insecure, it is also pointless.

I don't understand what is insecure, and why it's pointless.

[berrange]

The UID/GID ownership & permissions of the socket are controllable via the libvirtd.conf, so we can deal with any UID/GID mis-matches if needed, so I don't think there's a need for anything particularly special in this respect.

Providing passwords either in env variables or in CLI args is the security flaw - they are both exposed to other processes on the host - even if other pods can't see them because of PID namespaces, there is still plenty of stuff in the host context that should not see this. They are pointless because we don't need to use username/password auth for our connection to libvirt, and even if we did need it, we can setup the client side auth config file in the virt-handler pod.

[rmohr]

I agree with @berrange that it is better not to use env vars at all at the end. If we need it, we can use the "kubernetes secrets to volume flow" (instead of reading the secret from env) and create a auth file on startup.

After thinking about it, I am for merging this, and if we need auth, we add a script to virt-handler which creates a config auth out of the secrets volume of kubernetes.

Since functional test CI is still broken, tests work for me.

[rmohr]

@fabiand waiting for your opinion

[fabiand]

+1 fom my side as well. The comments below are just to complete that discussion, and should not prevent merging this patch.

The UID/GID ownership & permissions of the socket are controllable via the libvirtd.conf, so we can deal with any UID/GID mis-matches if needed, so I don't think there's a need for anything particularly special in this respect.

Right - The issue is that uid/gids can change between images - this would currently require adjusting the config after each image update of libvirt or the virt-handler (IIUIC). But that's a problem unrelated to this change.

[fabiand]

Providing passwords either in env variables or in CLI args is the security flaw - they are both exposed to other processes on the host - even if other pods can't see them because of PID namespaces, there is still plenty of stuff in the host context that should not see this. They are pointless because we don't need to use username/password auth for our connection to libvirt, and even if we did need it, we can setup the client side auth config file in the virt-handler pod.

Right, I see all the points.

[stu-gott]

They are pointless because we don't need to use username/password auth for our connection to libvirt, and even if we did need it, we can setup the client side auth config file in the virt-handler pod.

I strongly disagree with the notion that username/password are pointless. They don't serve a purpose in the default setup we've created, but this entire discussion has been about users wanting to deploy their own libvirt. It's not for us to decide what security posture makes sense in that case.

Since this PR has now been merged, there no longer exists the ability to use usernames/passwords with libvirt at all. @rmohr's suggestion to map account info from kubernetes secrets is a good one. I suggest we open an issue for that and mark it "new contributors".

[fabiand]

@stu-gott well - it only prevents to use libvirt in our KubeVirt context without a username/password, right?

To me this change was rather about the defaults we are using in our KubeVirt context, and in this context, we want to control how the access to libvirt happens.

But as user can still use the plain libvirt container and use it as he likes. Not?

[stu-gott]

All hooks to use username/password were just removed from KubeVirt. As I understand things, this means the KubeVirt binaries flatly cannot connect to any libvirt instance that's using password based auth because there's no mechanism left with which to supply them.

[berrange]

KubeVirt isn't intended to be used with arbitrarily configured libvirt installs - only with the specific libvirt configuration we're doing for the libvirtd container image. This does not use password auth, so there's no problem in this respect. As mentioned before though, it is still possible to configure a libvirt client auth config file to supply passwords if you trying to test with a non-standard libvirt install.