Add support for VNC console for shim VMs #3822

europaul · 2024-03-15T13:35:48Z

We enable access to shim VMs via VNC console through the following steps:

add another virtual console in the QEMU configuration that's gonna be used by VNC
call agetty on that virtual console during VMs boot to enable login

When connecting to the QEMU instance via VNC the user will first be presented with a view of the currently running process (container's entry point). To switch to the shell of the shim VM the user will have to press 'Ctrl+Alt+2' and then 'Enter' to get the login prompt. Due to inability of some VNC clients to change the resolution of the display, the client might crash. However upon the client restart the new virtual console will appear as expected. The user can then switch back to the previous virtual console by pressing 'Ctrl+Alt+1'.

codecov · 2024-03-15T13:50:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.51%. Comparing base (d864826) to head (7d949cd).
Report is 19 commits behind head on master.

❗ Current head 7d949cd differs from pull request most recent head e7b84e7. Consider uploading reports for the commit e7b84e7 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3822   +/-   ##
=======================================
  Coverage   17.51%   17.51%           
=======================================
  Files           3        3           
  Lines         805      805           
=======================================
  Hits          141      141           
  Misses        629      629           
  Partials       35       35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/xen-tools/initrd/init-initrd

rouming · 2024-03-18T11:29:43Z

Can you please add to the commit message and PR description that this will work only for the standalone vnc client, not the "Remote Console" guacamole plugin. Also would be great if you specify the reasons why it does not work there.

I do not have any objections on this PR, this is what we discussed with you. The main thing is left to be done here is to enable this feature by a separate flag, like the regular vnc is enabled: https://github.com/lf-edge/eve-api/blob/6180247abbc1c04422e093458809e8da59a25eff/proto/config/vm.proto#L52

Why? There are huge security concerns that we can't let everyone who has a vnc access to the Container let freely access the Shim Vm (if you wish we can discuss that once again with you and @shjala).

So my advice is in order to complete this PR is to add a new field to the VmConfig structure (vm.proto). Then we can raise the whole security discussion how to enable this safely while the VNC for shim VM stays disabled unless cloud explicitly enables this (which can be done later).

europaul · 2024-03-22T14:30:58Z

@rouming I added the flag EnableVncShimVm to pillar. It needs to be added to edgeview as well, but since it depends on pillar I will have to create another PR for that after this one is merged.

pkg/pillar/hypervisor/kvm.go

eriknordmark · 2024-03-22T20:16:39Z

Can you please add to the commit message and PR description that this will work only for the standalone vnc client, not the "Remote Console" guacamole plugin. Also would be great if you specify the reasons why it does not work there.

I'm also curious why it wouldn't work since guacamole is built using the vnc access in kvm/qemu.

Don't we have some markdown file which documents the current use of the console/vnc where this information can be added? If not we need to add it.

pkg/pillar/hypervisor/xen.go

rouming · 2024-04-02T08:47:34Z

@europaul There are a few changes requested (documentation, branching shim-vnc enabling). Also please fix yetus. Seems it wants "VM" name (all capital).

rouming · 2024-04-02T08:55:58Z

Can you please add to the commit message and PR description that this will work only for the standalone vnc client, not the "Remote Console" guacamole plugin. Also would be great if you specify the reasons why it does not work there.

I'm also curious why it wouldn't work since guacamole is built using the vnc access in kvm/qemu.

Here we talk about this https://www.qemu.org/docs/master/system/keys.html, so you can switch between consoles from the VNC viewport. But guacamole browser client (I assume this is guacamole) does not let you send those keys combinations (or this is the browser? I did not invesitate that, might be Paul knows). So the switch between VNC consoles (vm and shim-vm) will work from application client (tightvnc, etc), but not guacamole.

Don't we have some markdown file which documents the current use of the console/vnc where this information can be added? If not we need to add it.

Seems indeed we don't have any documentation VNC related.

europaul · 2024-04-02T20:02:18Z

@eriknordmark @europaul I'm still looking into how to switch between consoles using a key combination in guacamole. According to the guacamole's FAQ this might be possible. If I find a solution it will be part of another PR.

pkg/pillar/cmd/zedagent/parseconfig.go

europaul · 2024-04-05T10:02:07Z

@eriknordmark @rouming I think I addressed all comments, feel free to have another look!

rouming

Ack from me.

europaul · 2024-04-05T14:35:45Z

@rouming @eriknordmark I actually just confirmed that switching between virtual consoles works via the keyboard shortcut in guacamole in both Firefox in Chromium on my machine. So I removed that line about the limitation from the docs.

rouming · 2024-04-05T15:12:24Z

That's cool. Thanks for update.

christoph-zededa · 2024-04-05T15:26:02Z

pkg/pillar/hypervisor/kvm.go

@@ -173,6 +173,16 @@ const qemuConfTemplate = `# This file is automatically generated by domainmgr
  driver = "virtconsole"
  chardev = "charserial1"
  name = "org.lfedge.eve.console.prime"
+
+{{- if .DomainConfig.EnableVncShimVM}}


What about adding a test to check that this is actually activated like the other tests we already have for this template?

eriknordmark

In the docs should you also refer to app.allow.vnc in CONFIG PROPETIES.md? Without that one can't use a VNC client to connect

pkg/pillar/types/domainmgrtypes.go

Signed-off-by: Paul Gaiduk <paulg@zededa.com>

We enable access to shim VMs via VNC console through the following steps: - add another virtual console in the QEMU configuration that's gonna be used by VNC - call agetty on that virtual console during VMs boot to enable login When connecting to the QEMU instance via VNC the user will first be presented with a view of the currently running process (container's entry point). To switch to the shell of the shim VM the user will have to press 'Ctrl+Alt+2' and then 'Enter' to get the login prompt. Due to inability of some VNC clients to change the resolution of the display, the client might crash. However upon the client restart the new virtual console will appear as expected. The user can then switch back to the previous virtual console by pressing 'Ctrl+Alt+1'. Signed-off-by: Paul Gaiduk <paulg@zededa.com>

The fields - VirtualizationMode - EnableVnc - VncDisplay - VncPassword are already part of VmConfig, which is embedded in DomainStatus, so there is no need to duplicate them in DomainStatus. Signed-off-by: Paul Gaiduk <paulg@zededa.com>

Signed-off-by: Paul Gaiduk <paulg@zededa.com>

We differentiate between the access to the edge application and the shim VM: - `enableVNC` is used to enable VNC for the edge application - `enableVncShimVm` is used to enable VNC for the shim VM This differentiation is need, because currently shim VM doesn't support any user authentication, so anybody able to access the VNC port can access the shim VM. This way VNC access to the shim VM stays disabled by default unless explicitly enabled by the controller. VNC is enabled in QEMU if either of the flags is set, but the virtconsole is only enabled if the `enableVNCshimVm` flag is set. Signed-off-by: Paul Gaiduk <paulg@zededa.com>

Signed-off-by: Paul Gaiduk <paulg@zededa.com>

Add a test to verify that the QEMU configuration for VNC in contianer is generated correctly Signed-off-by: Paul Gaiduk <paulg@zededa.com>

christoph-zededa · 2024-04-12T14:22:08Z

pkg/pillar/hypervisor/kvm_test.go

+	if err := kvmIntel.CreateDomConfig("test", config, types.DomainStatus{}, diskStatuses, &aa, conf); err != nil {
+		t.Errorf("CreateDomConfig failed %v", err)
+	}
+	defer os.Truncate(conf.Name(), 0)


Suggested change

defer os.Truncate(conf.Name(), 0)

why is it needed for if you remove the file anyways?

eriknordmark

LGTM (but I think there is a comment or two from @christoph-zededa to look at)

europaul requested a review from rouming as a code owner March 15, 2024 13:35

eriknordmark requested a review from rucoder March 15, 2024 14:16

rouming reviewed Mar 18, 2024

View reviewed changes

pkg/xen-tools/initrd/init-initrd Show resolved Hide resolved

rouming requested review from eriknordmark and shjala March 18, 2024 10:50

europaul force-pushed the enable-vnc-for-shim-vm branch from 868c4ce to 0424695 Compare March 18, 2024 11:01

europaul requested a review from rene as a code owner March 18, 2024 11:01

github-actions bot requested review from OhmSpectator and rouming March 18, 2024 11:02

europaul requested review from milan-zededa and deitch as code owners March 22, 2024 14:28

github-actions bot requested review from christoph-zededa, jsfakian, naiming-zededa and uncleDecart March 22, 2024 14:29

europaul force-pushed the enable-vnc-for-shim-vm branch from 53d3579 to c01cde6 Compare March 22, 2024 16:04

eriknordmark reviewed Mar 22, 2024

View reviewed changes

pkg/pillar/hypervisor/kvm.go Outdated Show resolved Hide resolved

christoph-zededa reviewed Mar 25, 2024

View reviewed changes

pkg/pillar/hypervisor/xen.go Outdated Show resolved Hide resolved

europaul force-pushed the enable-vnc-for-shim-vm branch from c01cde6 to c594d29 Compare April 4, 2024 15:46

github-actions bot requested review from christoph-zededa and eriknordmark April 4, 2024 15:47

christoph-zededa reviewed Apr 4, 2024

View reviewed changes

pkg/pillar/cmd/zedagent/parseconfig.go Show resolved Hide resolved

rouming approved these changes Apr 5, 2024

View reviewed changes

europaul force-pushed the enable-vnc-for-shim-vm branch from c594d29 to 7d949cd Compare April 5, 2024 14:33

github-actions bot requested review from christoph-zededa and rouming April 5, 2024 14:34

christoph-zededa reviewed Apr 5, 2024

View reviewed changes

eriknordmark requested changes Apr 7, 2024

View reviewed changes

pkg/pillar/types/domainmgrtypes.go Outdated Show resolved Hide resolved

europaul added 7 commits April 12, 2024 15:21

Fix typo in chronyd start message

5fe2f45

Signed-off-by: Paul Gaiduk <paulg@zededa.com>

Remove duplicate fields from DomainStatus

98ca68b

The fields - VirtualizationMode - EnableVnc - VncDisplay - VncPassword are already part of VmConfig, which is embedded in DomainStatus, so there is no need to duplicate them in DomainStatus. Signed-off-by: Paul Gaiduk <paulg@zededa.com>

pkg: make bump-eve-api result

229bc54

Signed-off-by: Paul Gaiduk <paulg@zededa.com>

Add documentation for VNC support in ECOs

802b23e

Signed-off-by: Paul Gaiduk <paulg@zededa.com>

Add TestCreateDomConfigContainerVNC

e7b84e7

Add a test to verify that the QEMU configuration for VNC in contianer is generated correctly Signed-off-by: Paul Gaiduk <paulg@zededa.com>

europaul force-pushed the enable-vnc-for-shim-vm branch from 7d949cd to e7b84e7 Compare April 12, 2024 13:28

github-actions bot requested review from christoph-zededa and eriknordmark April 12, 2024 13:28

christoph-zededa reviewed Apr 12, 2024

View reviewed changes

christoph-zededa approved these changes Apr 12, 2024

View reviewed changes

eriknordmark approved these changes Apr 12, 2024

View reviewed changes

eriknordmark merged commit 79dd190 into lf-edge:master Apr 13, 2024
38 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for VNC console for shim VMs #3822

Add support for VNC console for shim VMs #3822

europaul commented Mar 15, 2024

codecov bot commented Mar 15, 2024 •

edited

Loading

rouming commented Mar 18, 2024 •

edited

Loading

europaul commented Mar 22, 2024

eriknordmark commented Mar 22, 2024

rouming commented Apr 2, 2024 •

edited

Loading

rouming commented Apr 2, 2024

europaul commented Apr 2, 2024

europaul commented Apr 5, 2024

rouming left a comment

europaul commented Apr 5, 2024

rouming commented Apr 5, 2024

christoph-zededa Apr 5, 2024

europaul Apr 12, 2024

eriknordmark left a comment

christoph-zededa Apr 12, 2024

eriknordmark left a comment

Add support for VNC console for shim VMs #3822

Add support for VNC console for shim VMs #3822

Conversation

europaul commented Mar 15, 2024

codecov bot commented Mar 15, 2024 • edited Loading

Codecov Report

rouming commented Mar 18, 2024 • edited Loading

europaul commented Mar 22, 2024

eriknordmark commented Mar 22, 2024

rouming commented Apr 2, 2024 • edited Loading

rouming commented Apr 2, 2024

europaul commented Apr 2, 2024

europaul commented Apr 5, 2024

rouming left a comment

Choose a reason for hiding this comment

europaul commented Apr 5, 2024

rouming commented Apr 5, 2024

christoph-zededa Apr 5, 2024

Choose a reason for hiding this comment

europaul Apr 12, 2024

Choose a reason for hiding this comment

eriknordmark left a comment

Choose a reason for hiding this comment

christoph-zededa Apr 12, 2024

Choose a reason for hiding this comment

eriknordmark left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 15, 2024 •

edited

Loading

rouming commented Mar 18, 2024 •

edited

Loading

rouming commented Apr 2, 2024 •

edited

Loading