vnc: Better error messages

When a second remote viewer starts, our in-page viewer will be cleanly
disconnected.

But when we fail to establish a connection because of protocol
incompatabilities or authentication failures, we should make that
clear in the UI by showing the error and saying "Retry" instead of
"Connect".

Unfortunately, there is not enough information coming from NoVNC that
would allow us to detect that the connection failed because the VNC
server wants encryption. But we can guess by looking into qemu.conf.

Author: mvollmer

src/components/vm/consoles/vnc.tsx

@@ -56,12 +56,31 @@ const _ = cockpit.gettext;
 export type VncSizeMode = "none" | "local" | "remote";
 
 export class VncState extends ConsoleState {
-    sizeMode: VncSizeMode = "none";
+    sizeMode: VncSizeMode;
+    failure_reason: string | null;
+
+    constructor() {
+        super();
+        this.sizeMode = "none";
+        this.failure_reason = null;
+    }
 
     setSizeMode(val: VncSizeMode) {
         this.sizeMode = val;
         this.update();
     }
+
+    setConnected(val: boolean) {
+        this.connected = val;
+        this.failure_reason = null;
+        this.update();
+    }
+
+    setDisconnected(reason: string | null) {
+        this.connected = false;
+        this.failure_reason = reason;
+        this.update();
+    }
 }
 
 const VncEditModal = ({
@@ -470,9 +489,25 @@ export class VncActive extends React.Component<VncActiveProps, VncActiveState> {
             this.observer.disconnect();
     }
 
-    onDisconnected(clean: boolean) { // server disconnected
+    async onDisconnected(clean: boolean) { // server disconnected
         console.info('Connection lost: ', clean ? "clean" : "unclean");
-        this.props.state.setConnected(false);
+        let reason;
+        if (clean)
+            reason = null;
+        else if (this.props.state.failure_reason) {
+            // The reason might have been set by onSecurityFailure
+            // already, so we keep it.
+            reason = this.props.state.failure_reason;
+        } else {
+            // This might be TLS.
+            const qemu_conf = await readQemuConf();
+            if (qemu_conf.vnc_tls) {
+                reason = _("VNC with TLS is not supported by the in-page viewer");
+            } else {
+                reason = _("Failed to connect");
+            }
+        }
+        this.props.state.setDisconnected(reason);
     }
 
     onInitFailed(detail: unknown) {
@@ -481,6 +516,7 @@ export class VncActive extends React.Component<VncActiveProps, VncActiveState> {
 
     onSecurityFailure(reason: string | undefined) {
         console.info('Security failure:', reason || "unknown reason");
+        this.props.state.setDisconnected(reason || _("Security failure"));
     }
 
     render() {
@@ -548,10 +584,14 @@ export class VncActive extends React.Component<VncActiveProps, VncActiveState> {
                     />
                     : <div className="vm-console-vnc">
                         <EmptyState>
-                            <EmptyStateBody>{_("Disconnected")}</EmptyStateBody>
+                            <EmptyStateBody>
+                                { state.failure_reason || _("Disconnected") }
+                            </EmptyStateBody>
                             <EmptyStateFooter>
-                                <Button variant="primary" onClick={() => state.setConnected(true)}>
-                                    {_("Connect")}
+                                <Button
+                                    variant="primary"
+                                    onClick={() => state.setConnected(true)}>
+                                    { state.failure_reason ? _("Retry") : _("Connect") }
                                 </Button>
                             </EmptyStateFooter>
                         </EmptyState>

src/helpers.ts

@@ -997,20 +997,27 @@ export function vmHasVFIOHostDevs(vm: VM): boolean {
 }
 
 export interface QemuConf {
+    vnc_tls: boolean;
     vnc_password: string | null;
 }
 
+const VNC_TLS_RX = /^[ \t]*vnc_tls[ \t]*=[ \t]*([0-9]*)/m;
 const VNC_PASSWORD_RX = /^[ \t]*vnc_password[ \t]*=[ \t]*"([^"\\]*(?:\\.[^"\\]*)*)"/m;
 
 export async function readQemuConf(): Promise<QemuConf> {
     const conf: QemuConf = {
+        vnc_tls: false,
         vnc_password: null,
     };
 
     const text = await cockpit.file("/etc/libvirt/qemu.conf", { superuser: "try" }).read();
     if (!text)
         return conf;
 
+    const tls_match = text.match(VNC_TLS_RX);
+    if (tls_match)
+        conf.vnc_tls = Number(tls_match[1]) != 0;
+
     const password_match = text.match(VNC_PASSWORD_RX);
     if (password_match)
         conf.vnc_password = password_match[1].replaceAll('\\"', '"');

test/check-machines-consoles

@@ -744,6 +744,126 @@ fullscreen=0
         b.wait_visible(".vm-console-vnc canvas")
         self.assertIn("password=on", m.execute(f"grep ^-vnc /var/log/libvirt/qemu/{name}.log"))
 
+        if not m.ws_container:
+            # We consistently get a 1006 error in the "ws-container"
+            # scenario instead of the expected "Authentication
+            # failed", for unknown reasons.
+            #
+            # TODO: Debug this properly.
+
+            # Change global password, but without restarting the VM. This
+            # means that Cockpit will use the new password when
+            # connecting, but the VM still expects the old.
+            m.execute("sed -i -e 's/vnc_password = \"barfoo\"/vnc_password = \"foobar\"/' /etc/libvirt/qemu.conf")
+
+            # We are still connected, but reconnecting will fail.
+
+            b.click(".consoles-card button:contains(Disconnect)")
+            b.wait_in_text(".consoles-card", "Disconnected")
+
+            # Quite often the reconnection here fails with code 1006
+            # (Abnormal Closure), but retrying will give us the expected
+            # error message, usually on the second try.
+
+            b.click(".consoles-card button:contains(Connect)")
+            for _ in range(5):
+                try:
+                    b.wait_in_text(".consoles-card", "Authentication failed")
+                    break
+                except testlib.Error:
+                    if "Failed to connect" in b.text(".consoles-card"):
+                        print("Unexpected error message. Retrying...")
+                        b.click(".consoles-card button:contains(Retry)")
+                    else:
+                        raise
+
+            b.wait_in_text(".consoles-card", "Authentication failed")
+
+    @testlib.skipImage("cockpit-ws too old",
+                       "rhel-9-*", "centos-9-*", "ubuntu-*", "debian-trixie", "opensuse-tumbleweed")
+    def testNonSharedExternal(self):
+        b = self.browser
+        m = self.machine
+
+        name = "subVmTest1"
+        self.createVm(name, graphics="vnc")
+
+        self.login_and_go("/machines")
+        self.waitPageInit()
+        self.waitVmRow(name)
+        self.goToVmPage(name)
+
+        # VNC viewer should be connected
+        b.wait_visible(".vm-console-vnc canvas")
+
+        # Start external non-shared viewer.  We use Cockpit itself, in
+        # "Remote resizing" mode.
+        b2 = self.new_browser()
+        cookie = b.cookie("cockpit")
+        b2.open(f"/cockpit/@localhost/machines/index.html#/vm/vnc?name={name}&connection=system", cookie=cookie)
+
+        # Allow extra time for the page to initialize itself. It's a
+        # whole instance of cockpit-machines that has to list all VMs
+        # and all node devices...
+        #
+        # See https://issues.redhat.com/browse/COCKPIT-1302
+        with b2.wait_timeout(60):
+            b2.select_PF("#vm-console-vnc-scaling", "Remote resizing")
+
+        # Our viewer should disconnect cleanly
+        b.wait_in_text(".consoles-card", "Disconnected")
+
+        def count_vnc_connections():
+            # Two open ports on localhost per connection
+            return int(m.execute("ss -tn | grep 5900 | wc -l")) / 2
+
+        # There should be a single VNC connection (for the detached viewer)
+        testlib.wait(lambda: count_vnc_connections() == 1)
+
+        # Trying to reconnect should fail while the external viewer is running
+        b.click('.consoles-card button:contains("Connect")')
+        b.wait_in_text(".consoles-card", "Failed to connect")
+
+        # Stopping the external viewer should allow us to connect again
+        b2.go("#/")
+        testlib.wait(lambda: count_vnc_connections() == 0)
+
+        b.click('.consoles-card button:contains("Retry")')
+        b.wait_visible(".vm-console-vnc canvas")
+        testlib.wait(lambda: count_vnc_connections() == 1)
+
+    @testlib.skipImage("No sscg", "arch", "opensuse-tumbleweed")
+    def testVNCTLS(self):
+        b = self.browser
+        m = self.machine
+
+        # Make some random certs
+        m.execute("""
+          mkdir -p /etc/pki/libvirt
+          cd /etc/pki/libvirt/
+          sscg --force --ca-file=ca-cert.pem --cert-file=server-cert.pem --cert-key-file=server-key.pem
+          chmod a+r /etc/pki/libvirt/*
+        """)
+
+        # Configure VNC to use TLS
+        self.write_file("/etc/libvirt/qemu.conf", """
+vnc_tls = 1
+vnc_tls_x509_cert_dir = "/etc/pki/libvirt"
+        """, append=True)
+
+        m.execute(f"systemctl restart {self.getLibvirtServiceName()}")
+
+        name = "subVmTest1"
+        self.createVm(name, graphics="vnc")
+
+        self.login_and_go("/machines")
+        self.waitPageInit()
+        self.waitVmRow(name)
+        self.goToVmPage(name)
+
+        # VNC viewer should not be connected, with a helpful message
+        b.wait_in_text(".consoles-card", "VNC with TLS is not supported")
+
 
 if __name__ == '__main__':
     testlib.test_main()