libvirt: Create spice graphics console#195
Conversation
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
|
Just throwing this up for discussion. Any reason to not do this? This makes debugging and connecting to the spawned VM a lot more convenient from virt-manager, combined with setting |
There was a problem hiding this comment.
Code Review
This pull request introduces SPICE graphics console support for libvirt domains, enhancing debugging capabilities with "virt-manager". However, it presents critical issues: the SPICE console is enabled by default, creating an unauthenticated port on the local interface that poses a security risk in multi-user environments. Furthermore, the "XmlWriter" utility is vulnerable to XML injection due to a lack of input escaping for user-supplied data. A significant code issue also arises as the current implementation can lead to invalid domain XML if both VNC and SPICE consoles are configured, as libvirt only allows one "
| // SPICE graphics for virt-manager access | ||
| // Always enable SPICE with auto-port allocation for easy debugging via virt-manager | ||
| writer.start_element("graphics", &[("type", "spice"), ("autoport", "yes")])?; | ||
| writer.write_empty_element("listen", &[("type", "address"), ("address", "127.0.0.1")])?; | ||
| writer.end_element("graphics")?; | ||
| writer.start_element("video", &[])?; | ||
| writer.write_empty_element("model", &[("type", "virtio")])?; | ||
| writer.end_element("video")?; |
There was a problem hiding this comment.
This section enables the SPICE graphics console by default, listening on "127.0.0.1" without authentication. This creates a security vulnerability where local users in multi-user environments could gain unauthorized access to the VM console. It is highly recommended to make this feature optional or implement authentication. Furthermore, this unconditional addition of a SPICE "
| // SPICE graphics for virt-manager access | |
| // Always enable SPICE with auto-port allocation for easy debugging via virt-manager | |
| writer.start_element("graphics", &[("type", "spice"), ("autoport", "yes")])?; | |
| writer.write_empty_element("listen", &[("type", "address"), ("address", "127.0.0.1")])?; | |
| writer.end_element("graphics")?; | |
| writer.start_element("video", &[])?; | |
| writer.write_empty_element("model", &[("type", "virtio")])?; | |
| writer.end_element("video")?; | |
| if self.vnc_port.is_none() { | |
| // SPICE graphics for virt-manager access | |
| // Always enable SPICE with auto-port allocation for easy debugging via virt-manager | |
| writer.start_element("graphics", &[("type", "spice"), ("autoport", "yes")])?; | |
| writer.write_empty_element("listen", &[("type", "address"), ("address", "127.0.0.1")])?; | |
| writer.end_element("graphics")?; | |
| writer.start_element("video", &[])?; | |
| writer.write_empty_element("model", &[("type", "virtio")])?; | |
| writer.end_element("video")?; | |
| } |
|
Changed to draft because (1) missing the |
|
Ultimately - and this would be a big lift, but it would make sense to match a lot of what |
In the immediate term what I think would be a big win and I'd been meaning to do is default to |
Signed-off-by: John Eckersberg jeckersb@redhat.com