Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn about security risks of the recommended libvirt install configuration #3638

Merged
merged 3 commits into from Jun 1, 2020

Conversation

berrange
Copy link
Contributor

The libvirt installer documentation is recommending a highly insecure configuration of libvirtd, with no auth or encryption, listening on all IP addresses, which gives clients prives equiv to a root shell. It does give some sense of this being an insecure config, but doesn't spell out the full implications to users following the doc. So this adds a much more explicit warning about the risks to fully inform users.

A few other minor improvements were made too.

@openshift-ci-robot
Copy link
Contributor

Hi @berrange. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 21, 2020
@berrange
Copy link
Contributor Author

@cfergeau @zeenix who were looking at options for enabling a more secure encryption & auth scheme for the libvirt installer.

Copy link
Contributor

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from one minor question/suggestion, this looks good to me and very much needed.

docs/dev/libvirt/README.md Outdated Show resolved Hide resolved
The "libvirt" RPM is a meta package which depends on every single other
libvirt RPM. It is undesirable to install this because it pulls in a
huge chain of dependencies, which are irrelevant for accomplishing the
steps described in this document. The main interesting thing it was
likely needed for is the "virsh" client, and can thus be replaced by
the "libvirt-client" RPM

The "libvirt-daemon-kvm" RPM pulls in everything needed for a typical
libvirt installation that will be used for running KVM guests, and is
the recommended option for scenarios that don't need to go to extreme
to minimize features installed.

The "qemu-kvm" RPM does not need to be listed explicitly, since it is
already a dependancy of "libvirt-daemon-kvm".

Further information to help understand the libvirt RPM choices is
present at https://libvirt.org/kbase/rpm-deployment.html

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
A connection to libvirtd gives the client application privileges that
are equivalent to those of a root shell. IOW, disabling authentication
and encryption in libvirtd is akin to running a telnet server with no
root password. This implication is not obvious to users following the
guide, so should be spelt out explicitly, so they understand it is
critical to correctly apply the firewall rules listed later in the
install guide.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In newer libvirtd that ships the "libvirt-tcp.socket" unit files for
socket activation, the --listen argument to libvirtd should not be
used. Enabling both socket activation and the --listen argument will
cause libvirtd to exit with an error about mutually exclusive
configuration options.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
@cfergeau
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 22, 2020
Copy link
Contributor

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I can get pull request merged in this project, but ack from me.

@cfergeau
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2020
@cfergeau
Copy link
Contributor

@zeenix can you take a look/merge this?

@praveenkumar
Copy link
Contributor

/ok-to-test

@praveenkumar
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, praveenkumar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2020
@openshift-merge-robot openshift-merge-robot merged commit 268cfcd into openshift:master Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants