Nested 'path' query param in console URL breaks serialproxy

[Jason Anderson]

Description
===========

Change I2ddf0f4d768b698e980594dd67206464a9cea37b changed all console URLs to have the token attached as a nested query parameter inside an outer "path" query parameter, e.g. "?path=?token=***".

While this was necessary for NoVNC support, it appears to have broken Ironic serial consoles, which use the nova-serialproxy service, which apparently is not aware that it needs to parse the token in this manner. It uses websockify.

To test, I enabled debug mode and added some extra logging in the nova-serialproxy to prove that "token" was empty in this function: https://github.com/openstack/nova/blob/stable/rocky/nova/objects/console_auth_token.py#L143

Steps to reproduce
==================

1. Have Ironic set up to allow web/serial consoles (https://docs.openstack.org/ironic/pike/admin/console.html). I believe this also requires having nova-serialproxy deployed.
2. Launch an Ironic instance and attempt to access the console via Horizon.

Expected result
===============

The serial console loads in the web interface; "Status: Opened" is displayed in the bottom. Console is interactive assuming the node has booted properly.

Actual result
=============

The serial console loads, but is blank; "Status: Closed" is displayed in the bottom. nova-serialproxy logs indicate the token was expired or invalid. The console never becomes interactive, but does not indicate there is an error in Horizon (at least on my deployment.)

Environment
===========

OpenStack Rocky release, deployed with Kolla-Ansible.

[Matt Riedemann (mriedem)]

I know tempest has a novnc console test, I wonder if the same is possible for ironic serial consoles in ironic CI testing so we could avoid these types of regressions in the future?

[melanie witt (melwitt)]

OK... I looked into this and I think I know what's going on. As Jason observed, the token nested in the 'path' parameter isn't working for the serial console. And I think I found in the code why this is, the 'path' parameter is specific to noVNC and I didn't realize this before. :(

Here is the code where 'path' is parsed out of the query string and is then appended to the url which websockify will receive [1]:

        const path = readQueryVariable('path', 'websockify');

        // | | | | | |
        // | | | Connect | | |
        // v v v v v v
        status("Connecting");
        // Build the websocket URL used to connect
        let url;
        if (window.location.protocol === "https:") {
            url = 'wss';
        } else {
            url = 'ws';
        }
        url += '://' + host;
        if(port) {
            url += ':' + port;
        }
        url += '/' + path;

So, given a url for noVNC that looks like this:

  https://foo.com/vnc_lite.html?path=%3Ftoken%3Dfoo

because of the processing in vnc_lite.html, the url the websockify server receives looks something like this:

  ...?token=foo

without any nesting. And so the parsing works fine.

But, if you're running the serial proxy and there's nothing processing the 'path' variable, the websockify server receives the original url with the token embedded in the 'path' variable and will not parse it correctly.

There are two ways we could fix this, I think.

(1) Revert much of change I2ddf0f4d768b698e980594dd67206464a9cea37b and add the 'path' variable back *only* for noVNC console access url generation

or

(2) Add additional parsing logic to nova/console/websocketproxy.py to also look for a 'token' query parameter nested in a 'path' query parameter

It seems like the right thing to do is (1) but I will ask what others think before going forward with that approach.

[1] https://github.com/novnc/noVNC/blob/v1.1.0/vnc_lite.html#L155-L174

[Matt Riedemann (mriedem)]

Without knowing the full details or history behind why noVNC changed, it seems like nesting the token within the path is a hack and I agree that reducing the scope of that to only noVNC (option 1) seems ideal rather than build a hack in nova (option 2) to deal with the original hack. I don't know how simple option 1 is though.

[melanie witt (melwitt)]

Option 1 is simple and is how the original change should have been made. I'll put a patch together. Might just do a full revert of the original change and a redo on top of it. Because most of the original patch was changing every console type's url and now we're only going to change on (noVNC).

[melanie witt (melwitt)]

Also, for context, the details and history of why noVNC changed is that previously, in the vnc_lite.html file used to handle a 'token' query parameter and stash it in a cookie, which we would process on the nova console proxy side:

https://github.com/novnc/noVNC/commit/51f9f0098d306bbc67cc8e02ae547921b6f6585c#diff-1d6838e3812778e95699b90d530543a1L173

https://github.com/openstack/nova/blob/207d2c22538ddec4d82fafbc01e756c9d25f6e36/nova/console/websocketproxy.py#L168

But after that noVNC commit (v1.1.0) they stopped handling the 'token' query parameter and passing it in a cookie. So they directed us to use the 'path' query parameter instead:

https://github.com/novnc/noVNC/pull/1220

It's clear now that this is something specific to noVNC but somehow I confused it because noVNC and websockify are/used to be so closely connected.

I think the right thing to do is leave the noVNC specific handling to get_vnc_console only.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/685194

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/stein
Review: https://review.opendev.org/686063

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/train
Review: https://review.opendev.org/686066

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/rocky
Review: https://review.opendev.org/686067

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/685194
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=54125a75fb056dbea115408610f90e7d6eee5139
Submitter: Zuul
Branch: master

commit 54125a75fb056dbea115408610f90e7d6eee5139
Author: melanie witt <email address hidden>
Date: Fri Sep 27 02:10:22 2019 +0000

    Reduce scope of 'path' query parameter to noVNC consoles

    This is a partial revert of commit
    9606c80402f6db20d62b689c58aa8f024183628a which added the 'path' query
    parameter to work with noVNC v1.1.0. This broke all other console types
    using websockify server (serial, spice) because the websockify server
    itself doesn't know how to handle the 'path' query parameter. It is the
    noVNC vnc_lite.html file which parses the 'path' variable and uses it
    as the url to the websockify server. So, all other console types should
    *not* be generating a console access url with a 'path' query parameter,
    only noVNC.

    Closes-Bug: #1845243

    TODO(melwitt): Figure out how to test serial and/or spice console in
    the gate

    Change-Id: I9521f21a685edc44121d75bdf534c201fa87c2d7

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/686066
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=e736babdfea277720634ac00fd9dc119042837b9
Submitter: Zuul
Branch: stable/train

commit e736babdfea277720634ac00fd9dc119042837b9
Author: melanie witt <email address hidden>
Date: Fri Sep 27 02:10:22 2019 +0000

    Reduce scope of 'path' query parameter to noVNC consoles

    This is a partial revert of commit
    9606c80402f6db20d62b689c58aa8f024183628a which added the 'path' query
    parameter to work with noVNC v1.1.0. This broke all other console types
    using websockify server (serial, spice) because the websockify server
    itself doesn't know how to handle the 'path' query parameter. It is the
    noVNC vnc_lite.html file which parses the 'path' variable and uses it
    as the url to the websockify server. So, all other console types should
    *not* be generating a console access url with a 'path' query parameter,
    only noVNC.

    Closes-Bug: #1845243

    TODO(melwitt): Figure out how to test serial and/or spice console in
    the gate

    Change-Id: I9521f21a685edc44121d75bdf534c201fa87c2d7
    (cherry picked from commit 54125a75fb056dbea115408610f90e7d6eee5139)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/686063
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=ff29f7018fdd4fc12b6b029ebef25904f9455aa9
Submitter: Zuul
Branch: stable/stein

commit ff29f7018fdd4fc12b6b029ebef25904f9455aa9
Author: melanie witt <email address hidden>
Date: Fri Sep 27 02:10:22 2019 +0000

    Reduce scope of 'path' query parameter to noVNC consoles

    This is a partial revert of commit
    9606c80402f6db20d62b689c58aa8f024183628a which added the 'path' query
    parameter to work with noVNC v1.1.0. This broke all other console types
    using websockify server (serial, spice) because the websockify server
    itself doesn't know how to handle the 'path' query parameter. It is the
    noVNC vnc_lite.html file which parses the 'path' variable and uses it
    as the url to the websockify server. So, all other console types should
    *not* be generating a console access url with a 'path' query parameter,
    only noVNC.

    Closes-Bug: #1845243

    TODO(melwitt): Figure out how to test serial and/or spice console in
    the gate

    NOTE(melwitt): The difference in nova/tests/unit/console/test_websocketproxy.py
    from the Train change is because change
    I0498599fd636aa9e30df932f0d893db5efa23260 is not in Stein.

    Change-Id: I9521f21a685edc44121d75bdf534c201fa87c2d7
    (cherry picked from commit 54125a75fb056dbea115408610f90e7d6eee5139)
    (cherry picked from commit e736babdfea277720634ac00fd9dc119042837b9)

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova 19.0.3 release.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/686067
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=f3eac7232cabe139ad34945b3be021af1f0d1599
Submitter: Zuul
Branch: stable/rocky

commit f3eac7232cabe139ad34945b3be021af1f0d1599
Author: melanie witt <email address hidden>
Date: Fri Sep 27 02:10:22 2019 +0000

    Reduce scope of 'path' query parameter to noVNC consoles

    This is a partial revert of commit
    9606c80402f6db20d62b689c58aa8f024183628a which added the 'path' query
    parameter to work with noVNC v1.1.0. This broke all other console types
    using websockify server (serial, spice) because the websockify server
    itself doesn't know how to handle the 'path' query parameter. It is the
    noVNC vnc_lite.html file which parses the 'path' variable and uses it
    as the url to the websockify server. So, all other console types should
    *not* be generating a console access url with a 'path' query parameter,
    only noVNC.

    Closes-Bug: #1845243

    TODO(melwitt): Figure out how to test serial and/or spice console in
    the gate

     Conflicts:
     nova/tests/unit/console/test_websocketproxy.py

    NOTE(melwitt): The conflict is because change
    I7f5f08691ca3f73073c66c29dddb996fb2c2b266 is not in Rocky.

    Change-Id: I9521f21a685edc44121d75bdf534c201fa87c2d7
    (cherry picked from commit 54125a75fb056dbea115408610f90e7d6eee5139)
    (cherry picked from commit e736babdfea277720634ac00fd9dc119042837b9)
    (cherry picked from commit ff29f7018fdd4fc12b6b029ebef25904f9455aa9)

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova 20.0.0.0rc2 release candidate.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova 18.2.3 release.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/queens
Review: https://review.opendev.org/692581

[OpenStack Infra (hudson-openstack)]

Change abandoned by Lee Yarwood (<email address hidden>) on branch: stable/queens
Review: https://review.opendev.org/692581