Fixes for OpenSSH 9.6 #25
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
martinpitt
added a commit
to martinpitt/cockpit
that referenced
this pull request
Jan 2, 2024
This unbreaks the Shell's SSH support with the latest round of OpenSSH security updates. FIXME: This currently tests allisonkarlitskaya/ferny#25 update the SHA after that PR has landed.
This was referenced Jan 2, 2024
|
cockpit-project/cockpit#19799 works fine, so let's please get that in ASAP? Then we need to do stable release updates in Debian and Ubuntu for cockpit. |
allisonkarlitskaya
previously approved these changes
Jan 2, 2024
test/test_basic.py
Outdated
| @@ -94,8 +94,9 @@ async def test_connection_refused(runtime_dir: pathlib.Path) -> None: | |||
| @pytest.mark.asyncio | |||
| async def test_dns_error(runtime_dir: pathlib.Path) -> None: | |||
| session = ferny.Session() | |||
| with pytest.raises(socket.gaierror): | |||
| with pytest.raises(Exception) as exc_info: | |||
There was a problem hiding this comment.
The docs look like you can pass a tuple of expected exception types here.
https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest.raises
There was a problem hiding this comment.
I actually did look at the docs, but I missed the Union(Tuple) thing. Seems to work!
| @@ -37,3 +37,21 @@ jobs: | |||
|
|
|||
| - name: Run unit tests | |||
| run: PYTHONPATH=src python3 -m pytest --color=yes | |||
|
|
|||
| debian: | |||
There was a problem hiding this comment.
Meta-level: I wonder if we should take this situation as:
- a failure to detect incoming changes (ie: we need better/regular testing in this area)
- an indication that everything actually worked out fine, since we did catch the issue relatively quickly...
There was a problem hiding this comment.
It would have been nice to catch a bit earlier, though. This is more forward-looking.. It's easy and cheap to test on Debian, so let's?
| @@ -2,6 +2,8 @@ name: CI | |||
| on: | |||
| push: | |||
| pull_request: | |||
| schedule: | |||
OpenSSH 0.9.6 "now bans most shell metacharacters from user and hostnames supplied via the command-line" [1], so it rejects the test's invalid host name even before it gets tossed to the DNS resolver. Accept the alternate exception as well. [1] https://www.openssh.com/txt/release-9.6
`-` is not a valid host name, and gets rejected by OpenSSH 0.9.6. Use a valid hostname in has_feature() to unbreak feature detection.
Fedora 39 and Rawhide have quite an old OpenSSH (9.3 at the moment). Debian unstable has the latest 9.6. This reproduces the issues fixed in the last two commits. `test_cancel_askpass` hangs forever in Debian, skip this test for the time being.
This ensures that ferny keeps working on Fedora/Debian with incoming OS updates, and timely alerts us about regressions.
| @@ -94,7 +94,7 @@ async def test_connection_refused(runtime_dir: pathlib.Path) -> None: | |||
| @pytest.mark.asyncio | |||
| async def test_dns_error(runtime_dir: pathlib.Path) -> None: | |||
| session = ferny.Session() | |||
| with pytest.raises(socket.gaierror): | |||
| with pytest.raises((socket.gaierror, ferny.ssh_errors.SshError)): | |||
There was a problem hiding this comment.
follow-ups:
- is it worth capturing and categorizing the new message? I don't like that we blanket-capture SshError here.
- should we try another hostname that is sure to get us a gaierror without hanging in case there's no network access?
There was a problem hiding this comment.
Indeed, sent PR #26 for these
allisonkarlitskaya
approved these changes
Jan 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See cockpit-project/bots#5733. When updating ferny locally to this version, it fixes at least
TestSuperuserDashboard.test. I'll do a test run in a cockpit PR to validate all of this mess: cockpit-project/cockpit#19799