Add "view, edit, and add Video devices" to detailspage

[Shotaro-Kawaguchi]

Added a feature to display video devices in virtual machine information. With this commit, it will be possible to add, edit, and delete video devices. This feature was implemented by a use case that sets a VNC password.

Screenshot viewing video device.

Click the "Add video device" button to display the following screen.

Click the "Edit" button to display the following screen.

Click the "..." button to display the following screen.

Click the "Remove" button to display the following screen.

[martinpitt]

@Shotaro-Kawaguchi : Thanks! Some initial notes:

• spice is deprecated in RHEL 8 and unsupported in RHEL 9 and 10. A few months ago we went through some great efforts to move users away from SPICE. So adding an option to add spice graphics is contrary to that.

• Does it make sense to add more than one VNC device? I don't know exactly, but I remember something like "you can only add one of each type".

• So perhaps it is enough to start with the "Edit VNC" functionality to set a password?

• This needs integration tests for all code paths, which in its current form is a big combinatorial explosion (add/remove multiple types/devices/password/varying OS support). Unfortunately it will take our team very long to help with that as we don't have a current machines maintainer. So it would be good if you could work on them yourself? (Happy to guide, of course)

• needsShutdownVideo() and its conditional code should be removed, as it's a constant False. It removes a code path.

I suggest to start with a single feature addition (like set/change VNC password), cover it with proper tests, and then add more things as follow-up PRs if there's any use case.

[garrett]

Thanks for this PR! We have had a design for this for a few years already, as @jelly pointed out.

Hopefully this PR is enough to push the feature forward. If that's the case, perhaps I can find some time to revisit the design and adjust it. It's been in a "holding pattern", waiting for someone to start working on it, so I can allocate time to it again.

Having fewer options (in other words: removing SPICE) should help simplify the UI further too.

Are there any use cases for having more than one VNC display per VM? All I can think about are:

1. Graphics vs. text mode... but you'd likely want to use SSH for text instead.
2. Multi-user setups, where people are using VNC or RDP to log into a host remotely. (But I'm not sure Cockpit is the best way to maintain a setup like that?) Even still: Having multiple VNC displays would probably be best by selecting the # and having ports allocated, right?
3. Having one app per VNC port might make some kind of sense, maybe? But I think this is a very specific use case and not really realistic?

We shouldn't add something because you can do it, but because it's actually useful to do. I don't think any of the above are really typical uses of running VMs on a server using Cockpit. (Please prove me wrong though, and/or provide other realistic cases.)

We had an issue suggesting adding VNC like in this PR before, but it doesn't make sense, as we have a dedicated place for graphics and remote graphics is quite different from other devices for several reasons. (It's integrated in-page, it really probably should be just 1 connection, it has graphical and text modes, it's not really a standard virtualize device provided to the VM like disks and network cards, and we already have a place for graphics at the top so we shouldn't have two different places for it.)

Also, I need to point out again that this feature is mostly useless as-is standalone, as it requires having access to the ports, which means either poking holes in the firewall or turning off a firewall. To really make this useful, we'd want to add in firewall integration. This doesn't mean we shouldn't make it better (we definitely should), but that we shouldn't stop just here. In other words, we need to make sure this works on a remote server (without jumping through hoops), not just from localhost.

Again, thank you for "getting the ball rolling again" with this PR! It's a good first step to getting remote graphics support improved overall!

[Shotaro-Kawaguchi]

@jelly Thank you for your response.

Is adding more devices a hard requirement or is just changing the default port/host/password good enough? The hard requirement for this use case is to be able to change the VNC password.

[Shotaro-Kawaguchi]

@martinpitt Thank you for your response.

spice is deprecated in RHEL 8 and unsupported in RHEL 9 and 10. A few months ago we went through some great efforts to move users away from SPICE. So adding an option to add spice graphics is contrary to that.

I was not aware that the SPICE type is deprecated. Thank you for pointing it out. I will remove the option to select the SPICE device type.

Does it make sense to add more than one VNC device? I don't know exactly, but I remember something like "you can only add one of each type".

I was not aware of the number of devices that can be added. Thank you for pointing it out. I will fix it so that only one VNC device can be added.

This needs integration tests for all code paths, which in its current form is a big combinatorial explosion (add/remove multiple types/devices/password/varying OS support). Unfortunately it will take our team very long to help with that as we don't have a current machines maintainer. So it would be good if you could work on them yourself? (Happy to guide, of course)

I will try to create test code.

needsShutdownVideo() and its conditional code should be removed, as it's a constant False. It removes a code path. Got it. I will remove the needsShutdownVideo() function.

In this pull request, I will make it possible to view, delete, and change the password of VNC devices.

[Shotaro-Kawaguchi]

We had an issue suggesting adding VNC like in this PR before, but it doesn't make sense, as we have a dedicated place for graphics and remote graphics is quite different from other devices for several reasons. (It's integrated in-page, it really probably should be just 1 connection, it has graphical and text modes, it's not really a standard virtualize device provided to the VM like disks and network cards, and we already have a place for graphics at the top so we shouldn't have two different places for it.)

Would it be good to move the button as shown in the attached image?

[garrett]

@Shotaro-Kawaguchi: Can you reply to my comment above please? Have you looked at the mockups? (The current mockups are @ https://github.com/cockpit-project/cockpit-machines/issues/553#issuecomment-1061581973.)

I'll have to adapt the mockups for the SPICE removal (and I've started doing that).

But I won't merge this PR if it's about adding VNC in a different place. We need to have the VNC console be an integrated experience, not scattered across the page.

Since you're interested, I did dust off the old designs and started to update them and would be happy to work with you to get the features integrated... as long as the design of it is considered.

Thanks!

(I think we always have VNC in all VMs Cockpit creates. In VMs where it's not enabled, we could have edit implicitly create it. In the edit dialog, we could also have a remove action. I could try to illustrate this in a mockup tomorrow, as it's getting late here today.)

[Shotaro-Kawaguchi]

@garrett

Can you reply to my comment above please? Have you looked at the mockups? (The current mockups are @ #553 (comment).)

I apologize for the delayed reply. I have looked at the current mockups. The image I posted above was not good. In the following comments, similar things are said, but in the current mockups, how will the VNC display be added if it does not exist in the current mockups? https://github.com/cockpit-project/cockpit-machines/issues/553#issuecomment-1878490509

Are there any use cases for having more than one VNC display per VM?

I was not aware that assigning multiple VNC displays to a VM would prevent the VM from starting due to the following error. So, I think one VNC display is sufficient.

error: Failed to start domain 'vm1'
error: unsupported configuration: only 1 graphics device of each type (sdl, vnc, spice, headless, dbus) is supported

[garrett]

In the following comments, similar things are said, but in the current mockups, how will the VNC display be added if it does not exist in the current mockups?

I've spent a little bit of time to start to update the mockups a bit and even designed an "empty state" pattern that would make it easy to add VNC if it hadn't been added to a machine already.

By default, machines with VNC and serial text consoles would look like this:

Here's what clicking on "Text" would look like:

If a machine does not have VNC support, then it should probably default to text, and if the VNC tab is active, then we could display a message saying that the support has not been enabled and that it can be added. That would look like this, which would have a button that would add VNC support with defaults set up in the same way VNC support is added to VMs created by Cockpit:

[Shotaro-Kawaguchi]

Thank you for sending the mockups. I think these mockups are very good.

[jelly]

In the following comments, similar things are said, but in the current mockups, how will the VNC display be added if it does not exist in the current mockups?

I've spent a little bit of time to start to update the mockups a bit and even designed an "empty state" pattern that would make it easy to add VNC if it hadn't been added to a machine already.

By default, machines with VNC and serial text consoles would look like this:

Here's what clicking on "Text" would look like:

If a machine does not have VNC support, then it should probably default to text, and if the VNC tab is active, then we could display a message saying that the support has not been enabled and that it can be added. That would look like this, which would have a button that would add VNC support with defaults set up in the same way VNC support is added to VMs created by Cockpit:

We still support SPICE on Fedora as only RHEL dropped it but Fedora and other distros still have it around. So it should be possible add or remove it.

[Shotaro-Kawaguchi]

I plan to proceed with the implementation according to the mockup created by Garrett. First, how about implementing the VNC device and considering SPICE later if necessary? @garrett Do you already have the source code that can operate like the mockup? If so, could you share it with us?

[garrett]

@Shotaro-Kawaguchi There's no "source code" for the mockup, as it's a mockup (and not a prototype). I do have a Penpot file of the current state:

• VM console options.penpot.zip

(You'd have to extract the zip file, then import the penpot file in Penpot.)

I've already exported the interesting parts as PNGs and attached them above.

[Shotaro-Kawaguchi]

@garrett Thank you for sharing the mockup. I will proceed with the implementation based on this.

[Shotaro-Kawaguchi]

There are some differences from the mockup, but I have implemented the functional parts of addition and editing.

[Shotaro-Kawaguchi]

@garrett @martinpitt I have revised the implementation based on the mockup, so please review it.

[mvollmer]

Thanks! We sure make you implement a lot of stuff just to be able to change the VNC password, no? :-)

I'll be your host for the next couple of rounds of review, and I hope we can get this in soon.

Let's start with how it looks:

• There needs to be some margin between the "Disconnect" button and the console, just like there currently is on "main".
• The toggle buttons are right aligned while the mockups have them on the left. (Not sure how important that is.)

• The width of the VncConsole doesn't seem to be well controlled. It overflows my browser window.

• A not-running machine still let's me select which console to view, but doesn't let me expand the Console card. I think that is inconsistent. I would prefer to be able to expand the Console card also for shut-off machines. The expanded Console allows me to start and stop a machine, so it makes perfect sense (to me) to have expanded consoles for not-running machines.

• If we keep the toggle buttons for non-running machines, I think we should change the text to say "Please start the virtual machine to access this console." In the case of a shut-off machine without VNC, this text would be replaced by something about graphical support not being available and the "Add VNC" button. The current state is confusin, IMO, since it both tells the user to start the machine and has "Add VNC" button. Both running and shut-off machines can look exactly the same as in the mockup when "VNC" is selected.

[mvollmer]

Big things:

• The expanded console is broken now.
• The VNC console size is out of control and freely extends out of its allocated space. The serial console doesn't do this.
• There might be more than one serial console, and the current code does seem to handle this. (I think, haven't actually tested this.)

This is all due to replacing the AccessConsoles component with custom toggle buttons. @garrett, how much do you want the toggle buttons to be part of this PR? I can commit to implementing that part independently from this PR.

[mvollmer]

Ok, my proposal is to reduce this work to the bare minimum. See #1973 for what I think that entails.

[mvollmer]

Ok, my proposal is to reduce this work to the bare minimum. See #1973 for what I think that entails.

@Shotaro-Kawaguchi, please check #1973. Do you think we should continue there? I have reserved some time to work on this myself. #1973 has your dialogs, plus some of the improvements. I will also start working on implementing the mockups from https://github.com/cockpit-project/cockpit-machines/pull/1795#issuecomment-2361343040.

Also see https://issues.redhat.com/browse/COCKPIT-868, which has many more items that I want to work on eventually.

[Shotaro-Kawaguchi]

@mvollmer Sorry, I didn't check #1973. I checked #1973. I appreciate your work.

[garrett]

@Shotaro-Kawaguchi: Hi! Thanks again for sharing your work here.

So we talked about this PR for a while in our face to face meeting last week.

We want to make sure we're solving tasks you and your team need, while also avoiding any unintended side effects, like breaking features for others or setting incorrect expectations for other people using Cockpit Machines.

I feel like there's a bunch of things we would like to know more about, to be able to step back and look at the bigger picture... basically, to better understand what you're trying to do and why. And then we can use this information to help with the redesign and implementation that solves your needs as well as others.

So, here are a few questions that we were wondering about that would help us to understand where you're coming from and how to best address your needs:

1. What is the main goal you're trying to achieve? (I’m asking about the task you’re trying to do, which is independent of how things currently work. Basically: What you're trying to do and how you would like to ideally do that.)

2. What are you currently using in production? How are you using it? (This isn't necessarily even Cockpit related.)

3. How are you currently using Cockpit? And are you using Cockpit Machines specifically, or do you need a fix to be able to use it in production? If you are using Cockpit Machines already, what configuration did you need to set up?

4. Are there any current bugs or limitations preventing you from using the in-page VNC connection instead of using a remote viewer app? (For example: Is the lack of a larger size the ultimate reason why you're using a native app? Or are there other reasons?)

5. Do you know that the current implementation of Cockpit Machines remote viewing is mostly broken by default in most cases and needs special configuration to make it work? (It currently only works when running the viewer on the same machine as Cockpit and also requires the firewall to be changed or turned off.)

6. Are you aware that the implementation is unencrypted? The password you send is in cleartext over the network, as is all mouse and keyboard interaction, including sudo passwords and any sensitive information. This is generally a pretty bad idea in most network environments, even behind a corporate firewall. (There may be some exceptions with special isolated and restricted networks. But, in general, even on "secure" networks, it's still better to make sure this kind of connection is encrypted, especially where passwords and sensitive information are concerned.)

Thanks in advance for helping us out in answering these questions!

For what it's worth, we did talk about solving some of the issues with something other than ports (which are unencrypted and require firewall configuration as well as a UI to set) and are thinking about SSH tunneling instead, which can be done like virt-viewer --connect qemu+ssh://user@host/system your-vm-name-here — and Cockpit could probably construct something like this without having to ask for any configuration. It's currently unknown if we could put this in a .vv file (it didn't seem to be the case when I looked at this a few years back — we would need to investigate this again) or if we would have to show something like this with copy/paste... either with the command line and/or even just the URI. However, virt-viewer and most other remote apps support "bookmarking" or a "recently used" history, so once you'd copy/paste from Cockpit into your viewer app, you wouldn't have to a second time (as you can go directly to your viewer app's history or favorites/bookmarking system).

But answers to the above questions would help us to know if that would work for you or what else we'd need to consider.

Thanks again!

[Shotaro-Kawaguchi]

@garrett I apologize for the late reply. Thank you for the confirmation. I will consider and reply.

[Shotaro-Kawaguchi]

@garrett : Thank you for considering this PR.

1. What is the main goal you're trying to achieve? (I’m asking about the task you’re trying to do, which is independent of how things currently work. Basically: What you're trying to do and how you would like to ideally do that.)

The goal is to securely manage VNC passwords for our virtual machines, enabling add, edit, and set passwords directly within the Cockpit interface.

2. What are you currently using in production? How are you using it? (This isn't necessarily even Cockpit related.)

We are currently using virt-manager in production.

3. How are you currently using Cockpit? And are you using Cockpit Machines specifically, or do you need a fix to be able to use it in production? If you are using Cockpit Machines already, what configuration did you need to set up?

We are not currently using Cockpit Machines in production. One of the reasons why we cannot adopt Cockpit Machines is that we cannot set the VNC password.

4. Are there any current bugs or limitations preventing you from using the in-page VNC connection instead of using a remote viewer app? (For example: Is the lack of a larger size the ultimate reason why you're using a native app? Or are there other reasons?)

Since we are not using the in-page VNC connection, we haven't encountered specific bugs or limitations. However, the security concerns raised in your subsequent questions (points 5 & 6) are major factors influencing our decision to use a separate, more secure remote viewer.

5. Do you know that the current implementation of Cockpit Machines remote viewing is mostly broken by default in most cases and needs special configuration to make it work? (It currently only works when running the viewer on the same machine as Cockpit and also requires the firewall to be changed or turned off.)

Thanks for pointing that out. I was unaware of these limitations.

6.Are you aware that the implementation is unencrypted? The password you send is in cleartext over the network, as is all mouse and keyboard interaction, including sudo passwords and any sensitive information. This is generally a pretty bad idea in most network environments, even behind a corporate firewall. (There may be some exceptions with special isolated and restricted networks. But, in general, even on "secure" networks, it's still better to make sure this kind of connection is encrypted, especially where passwords and sensitive information are concerned.)

I appreciate the security warning. I was unaware of that. Considering the security aspect, the SSH tunneling you are considering seems to be good.

Thank you.

[mvollmer]

2. What are you currently using in production? How are you using it? (This isn't necessarily even Cockpit related.)

We are currently using virt-manager in production.

How do you connect to libvirt with virt-manager? Do you use the default "QEMU/KVM" connection, or do you use the "Add connection" feature to access the KVM host over SSH?

6.Are you aware that the implementation is unencrypted? The password you send is in cleartext over the network, as is all mouse and keyboard interaction, including sudo passwords and any sensitive information.

(The VNC password is not actually send in cleartext, but everything else is, so this technicality does not change the fact that not using encryption is a bad idea.)

So for #1973 this means that we drop all configurability and just keep the "Add VNC" functionality?

[garrett]

@Shotaro-Kawaguchi: Thanks for your replies to all the questions! It really helps clarify a lot of points for us!

The goal is to securely manage VNC passwords for our virtual machines, enabling add, edit, and set passwords directly within the Cockpit interface.

I think it's probably obvious enough from the rest of your replies and also from @mvollmer's reply, but I'll state it plainly here to be absolutely clear: That "securely" means the code and UI in this PR is (sadly) not suitable as-is.

From my understanding, this means we need follow up with extra work to:

1. Prioritize in-page VNC, as that is (AFAIK? we should definitely double-check) securely transmitted over SSL via the browser.

   • It's currently limited in multiple ways (can't go fullscreen, has too much Cockpit UI around it, some keyboard shortcuts can't work), but we can at least solve the screen real estate problems. I have a simple prototype for how this might work already (it still needs a little more work).
   • The keyboard shortcut issues can't fully be worked around unless someone is using Cockpit Client or a native remote viewer like virt-viewer. In many cases, the keyboard shortcut issues won't matter too much.
   • For many people, for many tasks, this could be "good enough", even if it isn't completely "good enough" for every single case.
   • (Someone else on the team might already know for a fact that this is secure. When I say that we should double check, I mean, I, as a designer, am not sure if it is secure or not, but am assuming it is, and assuming it is transferred via the browser over SSL from the server directly. It would be nice if someone else on the team who knows the implementation could chime in to confirm this.)
   • But this would be the least amount of hassle for everyone, if it is indeed 100% sure to be secure and does not require any additional steps or configuration.

2. Figure out a way to get remote viewing via an app working securely, in a hassle-free manner.

   • For example: This might be via TLS and a port (and poking a hole in the firewall to make it actually available) or it might be over SSH tunneling (which would be much simpler from a UI/UX standpoint, as it'd "just work" as long as SSH access is allowed).
   • This requires more work. I think @mvollmer was already looking into this, and how we can get either working.
   • I know the virt-viewer command line supports ssh tunneling. It's still unknown if the .vv format has support for this yet (it didn't appear so years ago when I looked into this) and/or TLS.
   • It's also still unknown if SSH tunneling works on non-Linux platforms, like Windows and macOS. TLS probably does, but we'd need to check the viability of both still, on all OSes.
   • And it would be extra nice if we could also find a solution that doesn't lock us to virt-viewer (although that's cross platform).

3. If anything is determined to be insecure, we need to either remove it outright or at the very least have a HUGE OBVIOUS DANGER WARNING that it's not secure.

   • But I think having something that isn't secure that really needs to be secure means we really should remove it if we can't get it working securely.
   • (An insecure connection to a VM is basically "a toy" that people rightfully shouldn't use in production. Anything that is inherently insecure, especially without a notice about danger, is misleading and a glaring security issue, IMO.)

The results of looking into this will need to inform the UX which will inform the UI which will then need to be coded in such a way where it works well to solve the secure connection issue.

Considering the above, to make a TODO list of what's needed, I think we'll want the following, which can be split into separate steps (some of which is already being worked on, and not all are completely interrelated):

• Better in-page connection (larger, etc.)
• A secure way to connect to remotes via an app (virt-viewer, and possibly others, like using an RDP connection, which is already built into or at least available for most OSes, IIRC), using SSH and/or TLS
• Some UI to set a password that's integrated into the overall design of the console on the page
• Some way to retroactively add support for remote viewing (VNC, Spice, RDP, etc.)
• Revised UI that takes this all into consideration

[martinpitt]

@garrett:

That "securely" means the code and UI in this PR is (sadly) not suitable as-is.

I don't see anything insecure here. It's not at all obvious to me, can you please explain? Adding a localhost-only VNC to a VM is no different than what we do by default for newly created VMs.

[martinpitt]

Of course my comment was aimed at #1973 (which is the "good" version of this PR). It's rather confusing to have two, can we please close this and continue/restrict the discussion to #1973?

[mvollmer]

1. Prioritize in-page VNC, as that is (AFAIK? we should definitely double-check) securely transmitted over SSL via the browser.

The in-page VNC uses the same type of connection that Cockpit uses to reach the KVM host. This is typically https and might also involve SSH along the way. So we can assume it is secure enough.

[mvollmer]

It's still unknown if the .vv format has support for [SSH tunneling].

We know it does not support it, unfortunately. I have made a JIRA issue for this, that we can prioritize appropriately. @jelly has already contacted the libvirt team, and there seems to be consensus that it is a good idea.

[mvollmer]

It's also still unknown if SSH tunneling works on non-Linux platforms, like Windows and macOS.

The "qemu+ssh://" connection scheme (aka SSH tunneling) is not supported on Windows, but it is on MacOS. (I assume the sentence "Once started, you can use virsh as you would on Linux." implies that.)