websockfiy open redirection unit test broken with Python >= 3.10.6 standard lib

[Thomas Goirand]

Lucas Nussbaum reported this Debian bug:

https://bugs.debian.org/1017217

so I started investigating it. It took me a while to understand it was due to a change in the Python 3.10.6 standard http/server.py library.

Running these 2 unit tests against Python 3.10.5 works:

test_websocketproxy.NovaProxyRequestHandlerTestCase.test_reject_open_redirect
console.test_websocketproxy.NovaProxyRequestHandlerTestCase.test_reject_open_redirect_3_slashes

However, under Python 3.10.6, this fails. The reason isn't the interpreter itself, but the standard library, which has additional open redirection protection.

Looking at the changelog here:
https://docs.python.org/3/whatsnew/changelog.html

we see this issue:
https://github.com/python/cpython/issues/87389

which has been addressed by this commit:
https://github.com/python/cpython/commit/defaa2b19a9a01c79c1d5641a8aa179bb10ead3f

If I "fix" the Python 3.10.5 standard library using the 2 lines of code of the first hunk of this patch, then I can reproduce the issue.

I guess that the unit testing should be skipped if using Python >= 3.10.6, probably, or adapted somehow. I leave this to the Nova maintainers: for the Debian package, I'll just skip these 2 unit tests.

Cheers,

Thomas Goirand (zigo)

[Artom Lifshitz (notartom)]

Note: we haven't hit this in the gate because it's still running 3.10.4, but it's coming for us.

[melanie witt (melwitt)]

I will work on this as I have worked on this area of the code before.

[Artom Lifshitz (notartom)]

Setting to Critical because while it's not yet affecting us, I have reproduced locally on Fedora with Python 3.10.6, so it's only a matter of time until this blows up in the gate unless we fix it first.

[sean mooney (sean-k-mooney)]

we can likely adjust the unit test yes

the standard lib is now using lstrip('/')

which will catch the case where you have any number of slass

so '//' and '///' will be converted to /

we can likely revisit how we do the assertion to ensure it works with 3.10.6

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/nova/+/853379

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/853379
Committed: https://opendev.org/openstack/nova/commit/15769b883ed4a86d62b141ea30d3f1590565d8e0
Submitter: "Zuul (22348)"
Branch: master

commit 15769b883ed4a86d62b141ea30d3f1590565d8e0
Author: melanie witt <email address hidden>
Date: Tue Aug 16 06:49:53 2022 +0000

    Adapt websocketproxy tests for SimpleHTTPServer fix

    In response to bug 1927677 we added a workaround to
    NovaProxyRequestHandler to respond with a 400 Bad Request if an open
    redirect is attempted:

      Ie36401c782f023d1d5f2623732619105dc2cfa24
      I95f68be76330ff09e5eabb5ef8dd9a18f5547866

    Recently in python 3.10.6, a fix has landed in cpython to respond with
    a 301 Moved Permanently to a sanitized URL that has had extra leading
    '/' characters removed.

    This breaks our existing unit tests which assume a 400 Bad Request as
    the only expected response.

    This adds handling of a 301 Moved Permanently response and asserts that
    the redirect location is the expected sanitized URL. Doing this instead
    of checking for a given python version will enable the tests to continue
    to work if and when the cpython fix gets backported to older python
    versions.

    While updating the tests, the opportunity was taken to commonize the
    code of two unit tests that were nearly identical.

    Related-Bug: #1927677
    Closes-Bug: #1986545

    Change-Id: I27441d15cc6fa2ff7715ba15aa900961aadbf54a

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova 26.0.0.0rc1 release candidate.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/yoga
Review: https://review.opendev.org/c/openstack/nova/+/866192

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/xena
Review: https://review.opendev.org/c/openstack/nova/+/866193

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/wallaby
Review: https://review.opendev.org/c/openstack/nova/+/866194

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/victoria
Review: https://review.opendev.org/c/openstack/nova/+/866195

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/ussuri
Review: https://review.opendev.org/c/openstack/nova/+/866196

[melanie witt (melwitt)]

The fix for the vulnerability in cpython has been backported to older versions:

https://python-security.readthedocs.io/vuln/http-server-redirection.html

so we will need to fix our unit tests for older branches as well.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/train
Review: https://review.opendev.org/c/openstack/nova/+/866201

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/866192
Committed: https://opendev.org/openstack/nova/commit/4a2b44c7cf55d1d79d5a2dd638bd0def3af0f5af
Submitter: "Zuul (22348)"
Branch: stable/yoga

commit 4a2b44c7cf55d1d79d5a2dd638bd0def3af0f5af
Author: melanie witt <email address hidden>
Date: Tue Aug 16 06:49:53 2022 +0000

    Adapt websocketproxy tests for SimpleHTTPServer fix

    In response to bug 1927677 we added a workaround to
    NovaProxyRequestHandler to respond with a 400 Bad Request if an open
    redirect is attempted:

      Ie36401c782f023d1d5f2623732619105dc2cfa24
      I95f68be76330ff09e5eabb5ef8dd9a18f5547866

    Recently in python 3.10.6, a fix has landed in cpython to respond with
    a 301 Moved Permanently to a sanitized URL that has had extra leading
    '/' characters removed.

    This breaks our existing unit tests which assume a 400 Bad Request as
    the only expected response.

    This adds handling of a 301 Moved Permanently response and asserts that
    the redirect location is the expected sanitized URL. Doing this instead
    of checking for a given python version will enable the tests to continue
    to work if and when the cpython fix gets backported to older python
    versions.

    While updating the tests, the opportunity was taken to commonize the
    code of two unit tests that were nearly identical.

    Related-Bug: #1927677
    Closes-Bug: #1986545

    Change-Id: I27441d15cc6fa2ff7715ba15aa900961aadbf54a
    (cherry picked from commit 15769b883ed4a86d62b141ea30d3f1590565d8e0)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/866193
Committed: https://opendev.org/openstack/nova/commit/0e4a257e8636a979605c614a35e79ba47b74d870
Submitter: "Zuul (22348)"
Branch: stable/xena

commit 0e4a257e8636a979605c614a35e79ba47b74d870
Author: melanie witt <email address hidden>
Date: Tue Aug 16 06:49:53 2022 +0000

    Adapt websocketproxy tests for SimpleHTTPServer fix

    In response to bug 1927677 we added a workaround to
    NovaProxyRequestHandler to respond with a 400 Bad Request if an open
    redirect is attempted:

      Ie36401c782f023d1d5f2623732619105dc2cfa24
      I95f68be76330ff09e5eabb5ef8dd9a18f5547866

    Recently in python 3.10.6, a fix has landed in cpython to respond with
    a 301 Moved Permanently to a sanitized URL that has had extra leading
    '/' characters removed.

    This breaks our existing unit tests which assume a 400 Bad Request as
    the only expected response.

    This adds handling of a 301 Moved Permanently response and asserts that
    the redirect location is the expected sanitized URL. Doing this instead
    of checking for a given python version will enable the tests to continue
    to work if and when the cpython fix gets backported to older python
    versions.

    While updating the tests, the opportunity was taken to commonize the
    code of two unit tests that were nearly identical.

    Related-Bug: #1927677
    Closes-Bug: #1986545

    Change-Id: I27441d15cc6fa2ff7715ba15aa900961aadbf54a
    (cherry picked from commit 15769b883ed4a86d62b141ea30d3f1590565d8e0)
    (cherry picked from commit 4a2b44c7cf55d1d79d5a2dd638bd0def3af0f5af)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/866194
Committed: https://opendev.org/openstack/nova/commit/3023e162e1a415ddaa70b4b8fbe24b1771dbe424
Submitter: "Zuul (22348)"
Branch: stable/wallaby

commit 3023e162e1a415ddaa70b4b8fbe24b1771dbe424
Author: melanie witt <email address hidden>
Date: Tue Aug 16 06:49:53 2022 +0000

    Adapt websocketproxy tests for SimpleHTTPServer fix

    In response to bug 1927677 we added a workaround to
    NovaProxyRequestHandler to respond with a 400 Bad Request if an open
    redirect is attempted:

      Ie36401c782f023d1d5f2623732619105dc2cfa24
      I95f68be76330ff09e5eabb5ef8dd9a18f5547866

    Recently in python 3.10.6, a fix has landed in cpython to respond with
    a 301 Moved Permanently to a sanitized URL that has had extra leading
    '/' characters removed.

    This breaks our existing unit tests which assume a 400 Bad Request as
    the only expected response.

    This adds handling of a 301 Moved Permanently response and asserts that
    the redirect location is the expected sanitized URL. Doing this instead
    of checking for a given python version will enable the tests to continue
    to work if and when the cpython fix gets backported to older python
    versions.

    While updating the tests, the opportunity was taken to commonize the
    code of two unit tests that were nearly identical.

    Related-Bug: #1927677
    Closes-Bug: #1986545

    Change-Id: I27441d15cc6fa2ff7715ba15aa900961aadbf54a
    (cherry picked from commit 15769b883ed4a86d62b141ea30d3f1590565d8e0)
    (cherry picked from commit 4a2b44c7cf55d1d79d5a2dd638bd0def3af0f5af)
    (cherry picked from commit 0e4a257e8636a979605c614a35e79ba47b74d870)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/866195
Committed: https://opendev.org/openstack/nova/commit/77bc3f004e7fe4077ea035c659630bedef1cfea1
Submitter: "Zuul (22348)"
Branch: stable/victoria

commit 77bc3f004e7fe4077ea035c659630bedef1cfea1
Author: melanie witt <email address hidden>
Date: Tue Aug 16 06:49:53 2022 +0000

    Adapt websocketproxy tests for SimpleHTTPServer fix

    In response to bug 1927677 we added a workaround to
    NovaProxyRequestHandler to respond with a 400 Bad Request if an open
    redirect is attempted:

      Ie36401c782f023d1d5f2623732619105dc2cfa24
      I95f68be76330ff09e5eabb5ef8dd9a18f5547866

    Recently in python 3.10.6, a fix has landed in cpython to respond with
    a 301 Moved Permanently to a sanitized URL that has had extra leading
    '/' characters removed.

    This breaks our existing unit tests which assume a 400 Bad Request as
    the only expected response.

    This adds handling of a 301 Moved Permanently response and asserts that
    the redirect location is the expected sanitized URL. Doing this instead
    of checking for a given python version will enable the tests to continue
    to work if and when the cpython fix gets backported to older python
    versions.

    While updating the tests, the opportunity was taken to commonize the
    code of two unit tests that were nearly identical.

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

    NOTE(melwitt): The conflict is because change
    I58b0382c86d4ef798572edb63d311e0e3e6937bb
    (Refactor and rename test_tcp_rst_no_compute_rpcapi) is not in
    Victoria.

    Related-Bug: #1927677
    Closes-Bug: #1986545

    Change-Id: I27441d15cc6fa2ff7715ba15aa900961aadbf54a
    (cherry picked from commit 15769b883ed4a86d62b141ea30d3f1590565d8e0)
    (cherry picked from commit 4a2b44c7cf55d1d79d5a2dd638bd0def3af0f5af)
    (cherry picked from commit 0e4a257e8636a979605c614a35e79ba47b74d870)
    (cherry picked from commit 3023e162e1a415ddaa70b4b8fbe24b1771dbe424)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/866196
Committed: https://opendev.org/openstack/nova/commit/746d654c23d75f084b6f0c70e6c32b97eebf419c
Submitter: "Zuul (22348)"
Branch: stable/ussuri

commit 746d654c23d75f084b6f0c70e6c32b97eebf419c
Author: melanie witt <email address hidden>
Date: Tue Aug 16 06:49:53 2022 +0000

    Adapt websocketproxy tests for SimpleHTTPServer fix

    In response to bug 1927677 we added a workaround to
    NovaProxyRequestHandler to respond with a 400 Bad Request if an open
    redirect is attempted:

      Ie36401c782f023d1d5f2623732619105dc2cfa24
      I95f68be76330ff09e5eabb5ef8dd9a18f5547866

    Recently in python 3.10.6, a fix has landed in cpython to respond with
    a 301 Moved Permanently to a sanitized URL that has had extra leading
    '/' characters removed.

    This breaks our existing unit tests which assume a 400 Bad Request as
    the only expected response.

    This adds handling of a 301 Moved Permanently response and asserts that
    the redirect location is the expected sanitized URL. Doing this instead
    of checking for a given python version will enable the tests to continue
    to work if and when the cpython fix gets backported to older python
    versions.

    While updating the tests, the opportunity was taken to commonize the
    code of two unit tests that were nearly identical.

    Related-Bug: #1927677
    Closes-Bug: #1986545

    Change-Id: I27441d15cc6fa2ff7715ba15aa900961aadbf54a
    (cherry picked from commit 15769b883ed4a86d62b141ea30d3f1590565d8e0)
    (cherry picked from commit 4a2b44c7cf55d1d79d5a2dd638bd0def3af0f5af)
    (cherry picked from commit 0e4a257e8636a979605c614a35e79ba47b74d870)
    (cherry picked from commit 3023e162e1a415ddaa70b4b8fbe24b1771dbe424)
    (cherry picked from commit 77bc3f004e7fe4077ea035c659630bedef1cfea1)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/866201
Committed: https://opendev.org/openstack/nova/commit/43af75243b79e95330ecd982552d568571623e39
Submitter: "Zuul (22348)"
Branch: stable/train

commit 43af75243b79e95330ecd982552d568571623e39
Author: melanie witt <email address hidden>
Date: Tue Aug 16 06:49:53 2022 +0000

    Adapt websocketproxy tests for SimpleHTTPServer fix

    In response to bug 1927677 we added a workaround to
    NovaProxyRequestHandler to respond with a 400 Bad Request if an open
    redirect is attempted:

      Ie36401c782f023d1d5f2623732619105dc2cfa24
      I95f68be76330ff09e5eabb5ef8dd9a18f5547866

    Recently in python 3.10.6, a fix has landed in cpython to respond with
    a 301 Moved Permanently to a sanitized URL that has had extra leading
    '/' characters removed.

    This breaks our existing unit tests which assume a 400 Bad Request as
    the only expected response.

    This adds handling of a 301 Moved Permanently response and asserts that
    the redirect location is the expected sanitized URL. Doing this instead
    of checking for a given python version will enable the tests to continue
    to work if and when the cpython fix gets backported to older python
    versions.

    While updating the tests, the opportunity was taken to commonize the
    code of two unit tests that were nearly identical.

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

    NOTE(melwitt): The conflict is because change
    I23ac1cc79482d0fabb359486a4b934463854cae5 (Allow TLS ciphers/protocols
    to be configurable for console proxies) is not in Train. The difference
    from the cherry picked change is because the flake8 version on the
    stable/train branch does not support f-strings [1].

    Related-Bug: #1927677
    Closes-Bug: #1986545

    [1] https://lists.openstack.org/pipermail/openstack-discuss/2019-November/011027.html

    Change-Id: I27441d15cc6fa2ff7715ba15aa900961aadbf54a
    (cherry picked from commit 15769b883ed4a86d62b141ea30d3f1590565d8e0)
    (cherry picked from commit 4a2b44c7cf55d1d79d5a2dd638bd0def3af0f5af)
    (cherry picked from commit 0e4a257e8636a979605c614a35e79ba47b74d870)
    (cherry picked from commit 3023e162e1a415ddaa70b4b8fbe24b1771dbe424)
    (cherry picked from commit 77bc3f004e7fe4077ea035c659630bedef1cfea1)
    (cherry picked from commit 746d654c23d75f084b6f0c70e6c32b97eebf419c)

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova train-eol release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova ussuri-eol release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova victoria-eom release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova wallaby-eom release.