Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pybridge: Improve beiboot error reporting for fatal login failures #19859

Merged
merged 2 commits into from
Mar 21, 2024
Merged

pybridge: Improve beiboot error reporting for fatal login failures #19859

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
781a328
pybridge: Clarify beiboot AuthorizeResponder messages
martinpitt Oct 6, 2023
6bd679a
pybridge: Improve beiboot error reporting for fatal login failures
martinpitt Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/cockpit/beiboot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import logging
import os
import shlex
import sys
from pathlib import Path
from typing import Dict, Iterable, Optional, Sequence

Expand Down Expand Up @@ -309,8 +308,21 @@ async def run(args) -> None:
bridge.write_control(message)
bridge.ssh_peer.thaw_endpoint()
except ferny.InteractionError as exc:
sys.exit(str(exc))
error = ferny.ssh_errors.get_exception_for_ssh_stderr(str(exc))
logger.debug("ferny.InteractionError: %s, interpreted as: %r", exc, error)
if isinstance(error, ferny.SshAuthenticationError):
problem = 'authentication-failed'
elif isinstance(error, ferny.SshHostKeyError):
problem = 'unknown-hostkey'
elif isinstance(error, OSError):
# usually DNS/socket errors
problem = 'unknown-host'
else:
problem = 'internal-error'
bridge.write_control(command='init', problem=problem, message=str(error))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that's enough to get Cockpit Client not to show the stderr output then?

Like, what happens if we leak some stderr and then send a proper init/problem? Which one will get shown? Can we test it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this diff

--- src/cockpit/beiboot.py
+++ src/cockpit/beiboot.py
@@ -308,6 +308,8 @@ async def run(args) -> None:
         bridge.write_control(message)
         bridge.ssh_peer.thaw_endpoint()
     except ferny.InteractionError as exc:
+        import sys
+        sys.stderr.write("Uh, schlimme Dinge!\n")
         error = ferny.ssh_errors.get_exception_for_ssh_stderr(str(exc))
         logger.debug("ferny.InteractionError: %s, interpreted as: %r", exc, error)
         if isinstance(error, ferny.SshAuthenticationError):

... it will still show the "proper" error code and no stderr:

image

But with

--- src/cockpit/beiboot.py
+++ src/cockpit/beiboot.py
@@ -308,6 +308,7 @@ async def run(args) -> None:
         bridge.write_control(message)
         bridge.ssh_peer.thaw_endpoint()
     except ferny.InteractionError as exc:
+        assert False, "Uh, schlimmere Dinge!"
         error = ferny.ssh_errors.get_exception_for_ssh_stderr(str(exc))
         logger.debug("ferny.InteractionError: %s, interpreted as: %r", exc, error)
         if isinstance(error, ferny.SshAuthenticationError):

you get the full glory:

image

That feels right to me?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: fixed with and pre and expander.

return
except CockpitProblem as exc:
logger.debug("CockpitProblem: %s", exc)
bridge.write_control(exc.attrs, command='init')
return

Expand Down
15 changes: 6 additions & 9 deletions test/verify/check-client
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,11 @@ Command = /usr/bin/env python3 -m cockpit.beiboot
b.wait_text("#conversation-prompt", "admin@10.111.113.2's password: ")
b.set_val("#conversation-input", "wrong")
b.click("#login-button")
b.wait_in_text("#login-fatal-message", "admin@10.111.113.2: Permission denied")
b.click("#login-again")
b.wait_text("#brand", "Connect to:")
# resets the host field
b.wait_val("#server-field", "")
b.wait_in_text("#login-error-message", "Authentication failed")
b.wait_val("#server-field", "10.111.113.2")

# connect to most recent host
b.open("/") # reset URL from /metrics and last remote =host
b.click("#recent-hosts-list .host-line button.host-name")
b.wait_text("#conversation-prompt", "admin@10.111.113.2's password: ")
b.set_val("#conversation-input", "foobar")
Expand Down Expand Up @@ -208,17 +206,16 @@ Command = /usr/bin/env python3 -m cockpit.beiboot
# unreachable host
b.set_val("#server-field", "unknownhost")
b.click("#login-button")
b.wait_in_text("#login-fatal-message", "Could not resolve hostname unknownhost")
b.click("#login-again")
b.wait_text("#brand", "Connect to:")
b.wait_in_text("#login-error-message", "Host is unknown")
b.wait_val("#server-field", "unknownhost")
# does not appear in recent hosts
b.wait_in_text("#recent-hosts-list", "10.111.113.2")
self.assertNotIn("unknownhost", b.text("#recent-hosts-list"))

# wrong port
b.set_val("#server-field", "10.111.113.2:222")
b.click("#login-button")
b.wait_in_text("#login-fatal-message", "connect to host 10.111.113.2 port 222: No route to host")
b.wait_in_text("#login-error-message", "Host is unknown")

# unencrypted SSH key login
self.m_client.execute("runuser -u admin -- ssh-keygen -t rsa -N '' -f ~admin/.ssh/id_rsa")
Expand Down