Skip to content

Conversation

@jeckersb
Copy link
Collaborator

Signed-off-by: John Eckersberg jeckersb@redhat.com

Signed-off-by: John Eckersberg <jeckersb@redhat.com>
@jeckersb
Copy link
Collaborator Author

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 systemd.debug_shell=tty1 in the kargs so it dumps into a root shell.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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 "

Comment on lines +462 to +469
// 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")?;

Choose a reason for hiding this comment

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

security-medium medium

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 "

Suggested change
// 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")?;
}

@jeckersb jeckersb marked this pull request as draft January 28, 2026 19:27
@jeckersb
Copy link
Collaborator Author

Changed to draft because (1) missing the Assisted-by annotation, (2) I don't think this matches how spice is typically setup (thanks gemini). But the overall point still stands.

@cgwalters
Copy link
Collaborator

Ultimately - and this would be a big lift, but it would make sense to match a lot of what virt-install offers/exposes.

@cgwalters
Copy link
Collaborator

combined with setting systemd.debug_shell=tty1 in the kargs so it dumps into a root shell.

In the immediate term what I think would be a big win and I'd been meaning to do is default to console=hvc0 in the base image; it pairs with 84a8153

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants