beiboot: feature parity with cockpit-ssh #19401

martinpitt · 2023-09-27T07:21:26Z

In order to use cockpit-beiboot as cockpit-ssh replacement from the "normal" (not Client mode) login page, it needs to consider the given username and password. It also needs to properly handle "unknown host key" prompts.

This paves the way for completely replacing cockpit-ssh with cockpit.beiboot. See https://issues.redhat.com/browse/COCKPIT-954 and #19441.

Builds on top of PR beiboot preparation fixes #19413
Preferably fix PR pybridge: Avoid BrokenPipeError crash of SubprocessTransport.write() #19414
Builds on top of PR pybridge: Improve beiboot error reporting for fatal login failures #19859
Builds on top of PR Various login/beiboot preparations/cleanups #20967

allisonkarlitskaya

This looks like it's coming together really nicely and I'm starting to get excited about the finished result. 🚀

selinux/HACKING.md

src/cockpit/beiboot.py

allisonkarlitskaya · 2023-09-27T07:57:45Z

src/cockpit/beiboot.py


    async def do_askpass(self, messages: str, prompt: str, hint: str) -> Optional[str]:
+        if self.basic_password and 'password:' in prompt.lower():


yikes...

I guess this will work "for now" but we really need to do something more (airquotes) 'professional' here...

Yep, see my comment above, that's why it's still draft. Of course we could also consider/mark this as # HACK and update ferny as a follow-up. I still do want to do that, but it's much nicer to iterate on something that actually works.

src/cockpit/beiboot.py

test/verify/check-static-login

src/cockpit/beiboot.py

selinux/cockpit.te

martinpitt · 2024-09-17T06:54:30Z

@mvollmer Can you please review this? This PR almost celebrates its first birthday, and I sorted out all the disagreements with @allisonkarlitskaya. This is a prerequisite for fully eliminating cockpit-ssh in PR #19441. That mostly works, but still needs some more things done.

martinpitt · 2024-09-19T03:22:53Z

Just rebasing to current main to get an updated set of tests, as this became stale. No actual changes.

Consistently spell the section "Ssh-Login". This makes it easier to grep. Document the current behaviour that the specified `host` will not have its host key checked. Fix indentation of the `connectToUnknownHosts` paragraph.

We only ever need to get or update the keys for a particular host. So replace the current direct database manipulation with `{get,set}_hostkeys()`. This will come in handy in the next commits when we need to update the host keys from multiple places.

In the bastion host case, create a temporary UserKnownHostsFile for ssh and populate it with data from the authorization (if provided). On successful login, send the updated key data back to the user via the long-disused `x-login-data` mechanism. That'll get it added to the JSON blob that is sent as the reply to the `GET /login` request. On failed login, send the key data as part of the Cockpit protocol error, in a `known-hosts` attribute. Co-Authored-By: Martin Pitt <[email protected]>

Mock enough of the flatpak environment (/.flatpak-info and `flatpak-spawn` wrapper) to make beiboot.py take its `connect_from_flatpak()` code path. This makes the test more realistic.

In bastion mode, we previously used the default 3 password prompts, and sending the same password every time. This just unnecessarily trigges `faillock` and takes too long. Do the same as in "remote channel" bridge mode (`SshPeer` in remote.py). This can be dropped if/when we rework the whole machinery to not restart ssh from scratch with each new authentication attempt, and just follow the conversation.

Using a constant `-` for the authorization challenge is problematic: The login page load always starts with an initial generic auth challenge, which gets cleaned up (in `cockpit_session_reset()`) after some time. When the second challenge (for e.g. an unknown host key) then uses the same `-` challenge, ws fails with "Invalid conversation token: -". Avoid this by using an *actual* nonce: Combine the pid and time to a value that is reasonably unique to avoid collisions. Also add an assertion that the returned challenge is the one that we actually sent.

src/cockpit/beiboot.py

jelly · 2024-09-20T10:34:09Z

src/cockpit/beiboot.py

        self.router = router
+        self.basic_password = basic_password
+        self.have_basic_password = bool(basic_password)


is not None? Although one can argue that "" is also not a password

Just slightly different behaviour

We'd have to change connect_from_bastion_host() to rewrite an empty password as None here:

user, _, basic_password = user_password.partition(':')

I.e. doable, but less robust IMHO. I can do it if you prefer, though.

This would I presume break auth when a user has an empty password, I am not sure if that is supposed to be supported. That is my point, because:

bool("") => False

Ah, I see -- I suppose I could add a test case for an empty password (not sure if that works with ssh), and then do the above condition as "user and password are empty → user_password = None". I don't know if it's supported as we never tested that anywhere.

It works with ssh with

echo "PermitEmptyPasswords yes" > /etc/ssh/sshd_config.d/01-empty-password.conf

And then I can also log into Cockpit with beiboot.

This works because there isn't any actual prompt from ssh for a password, it logs you straight in.

So "" never actually appears in this code path, and thus is not None just works as well. Changed accordingly.

src/cockpit/beiboot.py

jelly · 2024-09-20T12:21:46Z

test/verify/check-static-login

+        b.eval_js(f"""window.localStorage.setItem('known_hosts', '{{"{my_ip}": "{bad_key}"}}')""")
+        try_login("admin", "foobar")
+        b.wait_text("#hostkey-title", f"{my_ip} key changed")
+        b.wait_in_text("#hostkey-fingerprint", "SHA256:")


Do we want to bother testing ED25519 host keys?

We actually do -- our VMs use that by default. But we don't really care about the specific key type anywhere -- login page and localStorage treat that as an opaque value.

Was more about the custom regex parsing. I think we actually don't test IPv6 in this scenario.

https://github.com/cockpit-project/cockpit/pull/19401/files#diff-9066feb4d4e90430fcb12a93072980d3f960f24cb289eda74ac7c31bd4b26d14R175

I see. Doesn't matter much for this particular case, as host names can't have spaces.

Nevertheless, IPv6 tests can't hurt, added. I pulled in another commit from PR #19441 to fix IPv6 address parsing in beiboot.py (that's covered by the full integration tests, but not yet in TestLogin.testLoginSshBeiboot -- note that that test will go away again).

Teach `via_ssh()` to correctly treat IPv6 addresses like `::1`, i.e. don't consider a double :: as a port separator. For IPv6 addresses with a port, the usual syntax is `[A::B:C]:22`, but `ssh` does not recognize the brackets. Strip them off after separating the port. (Both covered by `TestLogin.testServer`)

martinpitt · 2024-09-20T16:22:00Z

@jelly Addressed your comments: interdiff

... and found/tested/fixed another small bug

martinpitt · 2024-09-20T17:49:43Z

Oh-uh, setting an empty password with chpasswd does not work on Debian or arch.

There's a related problem on rhel-9-5 where chpasswd works, but ssh refuses it.

Fixed both by using passwd -d (TIL!)

martinpitt · 2024-09-20T18:41:33Z

Interesting arch bug -- after passwd -d admin, echo 'admin:foobar' | chpasswd succeeds, and /etc/shadow does have a password again -- but it doesn't work. Changing it with passwd admin doesn't work either. Huh?

Update: It's due to restarting sshd with the new PermitEmptyPasswords yes option, that seems to disable password authentication somehow. Even if I add PasswordAuthentication yes to the same file (two others enable it as well). I added a hack.

In order to use cockpit.beiboot as cockpit-ssh replacement from the bastion host (not Client mode) login page, it needs to consider the given username and password. cockpit-ssh sends an initial `authorize` message for that and checks for "Basic" auth. If that fails, it aborts immediately with `authentication-failed`. Implement the same in cockpit.beiboot. Note: The UI does not currently get along with multiple password attempts. Once we drop cockpit-ssh, we should fix the UI and cockpit.beiboot to behave like the flatpak, keep the initial SSH running, and just answer the "try again" prompts. Cover this in a new `TestLogin.testLoginSshBeiboot`. Once we generally replace cockpit-ssh with cockpit.beiboot, this will get absorbed by TestLogin and TestMultiMachine* and can be dropped again.

Stop treating host key prompts as generic conversation messages. We want the UI to handle them properly, with some verbiage/buttons and the UI/recipe for confirming/validating host keys, instead of letting the user type "yes". The login page recognizes these through the presence of the `host-key` authorize field. We can't use ferny's builtin `do_hostkey()` responder for this, as that requires `ferny.Session(handle_host_key=True)`, and that API is not flexible enough to handle our ssh command modifications and the extra beiboot_helper handler. This needs some bigger redesign, see cockpit-project#19668. So just recognize and parse SSH's host key prompts, and rely on our integration tests to spot breakage in future distro releases. While cockpit-ssh gave us the full host key right away during conversation, ssh (and thus ferny/beiboot) don't work that way: During conversation we only get the fingerprint. So for the sending direction, utilize the recently introduced temporary UserKnownHostsFile feature of beiboot, and send the known_hosts entry for the given host as part of the `Authorization:` header. For the receiving direction we need to handle three cases: * For a previously unknown host and a successful login, we only get the full host key as part of the GET /login's `login-data` response. So defer updating our localStorage's `known_hosts` database during the conversation until that happens. * For a failed login (like wrong password) after already confirming the key change, get the host key from the protocol error message. * ssh (and thus ferny/beiboot) don't report changed host keys as part of conversation, but immediatley abort. So remember that state in `ssh_host_key_change_host`, and re-attempt the login without sending known_hosts data. Our usual logic in `do_hostkey_verification()` will notice the change and present the correct dialog. Adjust TestLogin.testLoginSshBeiboot to only expect the host key on the first login attempt. Add testing of changed host key behaviour.

cockpituous · 2024-09-20T19:49:15Z

pkg/static/login.js


-        if (key_db[key_host] == key) {
+        // code path for old C cockpit-ssh, which doesn't set a known_hosts file in advance (like beiboot)
+        if (db_keys == key) {


This added line is not executed by any test.

cockpituous · 2024-09-20T19:49:15Z

pkg/static/login.js

+                        } catch (ex) {
+                            console.error("Failed to parse response text as JSON:", xhr.responseText, ":", JSON.stringify(ex));


These 2 added lines are not executed by any test.

cockpituous · 2024-09-20T19:49:15Z

pkg/static/login.js

+                    console.debug("setup_localstorage(): updating known_hosts database for deferred host key for", login_data_host, ":", hostkey);
+                    set_hostkeys(login_data_host, hostkey);
+                } else {
+                    console.error("login.js internal error: setup_localstorage() received a pending login-data host, but login-data does not contain known-hosts");


This added line is not executed by any test.

martinpitt · 2024-09-23T07:50:15Z

Yay -- this PR missed its first birthday cake by 4 days! 🎂

martinpitt marked this pull request as draft September 27, 2023 07:22

martinpitt requested a review from allisonkarlitskaya September 27, 2023 07:22

This comment was marked as outdated.

Sign in to view

allisonkarlitskaya requested changes Sep 27, 2023

View reviewed changes

martinpitt force-pushed the beiboot-authorize branch 2 times, most recently from 774f2ee to 60c6729 Compare September 27, 2023 09:06

martinpitt commented Sep 27, 2023

View reviewed changes

selinux/cockpit.te Outdated Show resolved Hide resolved

martinpitt force-pushed the beiboot-authorize branch 2 times, most recently from edae05b to ee489a1 Compare September 27, 2023 10:32

This comment was marked as resolved.

Sign in to view

martinpitt force-pushed the beiboot-authorize branch 2 times, most recently from fede982 to 1220a19 Compare September 28, 2023 05:39

This comment was marked as outdated.

Sign in to view

martinpitt requested a review from allisonkarlitskaya September 28, 2023 05:43

martinpitt force-pushed the beiboot-authorize branch 2 times, most recently from d7f3fa3 to a10e8a8 Compare September 28, 2023 08:17

This comment was marked as outdated.

Sign in to view

martinpitt force-pushed the beiboot-authorize branch from a10e8a8 to f3ee0e7 Compare September 28, 2023 08:56

martinpitt mentioned this pull request Sep 28, 2023

Beiboot is broken on ubuntu-2204 #19412

Closed

martinpitt force-pushed the beiboot-authorize branch 2 times, most recently from aeae62e to f21866b Compare September 28, 2023 15:12

martinpitt mentioned this pull request Sep 28, 2023

beiboot preparation fixes #19413

Merged

This comment was marked as resolved.

Sign in to view

martinpitt added the blocked Don't land until something else happens first (see task list) label Sep 29, 2023

martinpitt force-pushed the beiboot-authorize branch from f21866b to fb73cc7 Compare September 29, 2023 16:58

martinpitt removed the blocked Don't land until something else happens first (see task list) label Sep 29, 2023

martinpitt changed the title ~~cockpit-beiboot improvements/fixes~~ pybridge: Add initial authorize request to cockpit-beiboot Sep 29, 2023

martinpitt marked this pull request as ready for review September 30, 2023 04:30

martinpitt requested review from allisonkarlitskaya and removed request for allisonkarlitskaya September 30, 2023 04:30

martinpitt requested a review from allisonkarlitskaya September 17, 2024 06:40

martinpitt force-pushed the beiboot-authorize branch from f7c3ba6 to 1cd9453 Compare September 19, 2024 03:22

martinpitt removed the request for review from allisonkarlitskaya September 19, 2024 03:23

martinpitt and others added 6 commits September 19, 2024 14:32

doc: Clean up Ssh-Login in authentication.md

48afe00

Consistently spell the section "Ssh-Login". This makes it easier to grep. Document the current behaviour that the specified `host` will not have its host key checked. Fix indentation of the `connectToUnknownHosts` paragraph.

test: Make TestClient use beiboot's "flatpak" code path

2c15eb8

Mock enough of the flatpak environment (/.flatpak-info and `flatpak-spawn` wrapper) to make beiboot.py take its `connect_from_flatpak()` code path. This makes the test more realistic.

martinpitt force-pushed the beiboot-authorize branch from 1cd9453 to afce95b Compare September 19, 2024 12:33

martinpitt requested a review from jelly September 20, 2024 04:26

jelly reviewed Sep 20, 2024

View reviewed changes

martinpitt force-pushed the beiboot-authorize branch from afce95b to a33575d Compare September 20, 2024 16:21

martinpitt requested a review from jelly September 20, 2024 16:22

martinpitt force-pushed the beiboot-authorize branch from a33575d to 6699c33 Compare September 20, 2024 16:41

martinpitt force-pushed the beiboot-authorize branch from 6699c33 to 242e2d3 Compare September 20, 2024 17:49

martinpitt added 2 commits September 20, 2024 20:51

martinpitt force-pushed the beiboot-authorize branch from 242e2d3 to db8d88c Compare September 20, 2024 18:52

cockpituous reviewed Sep 20, 2024

View reviewed changes

jelly approved these changes Sep 23, 2024

View reviewed changes

martinpitt merged commit 5a73281 into cockpit-project:main Sep 23, 2024
84 checks passed

martinpitt deleted the beiboot-authorize branch September 23, 2024 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

beiboot: feature parity with cockpit-ssh #19401

beiboot: feature parity with cockpit-ssh #19401

martinpitt commented Sep 27, 2023 •

edited

Loading

This comment was marked as outdated.

allisonkarlitskaya left a comment

allisonkarlitskaya Sep 27, 2023

martinpitt Sep 27, 2023

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

martinpitt commented Sep 17, 2024

martinpitt commented Sep 19, 2024

jelly Sep 20, 2024

jelly Sep 20, 2024

martinpitt Sep 20, 2024

jelly Sep 20, 2024

martinpitt Sep 20, 2024

martinpitt Sep 20, 2024

jelly Sep 20, 2024

martinpitt Sep 20, 2024

jelly Sep 20, 2024 •

edited

Loading

martinpitt Sep 20, 2024

martinpitt commented Sep 20, 2024 •

edited

Loading

martinpitt commented Sep 20, 2024

martinpitt commented Sep 20, 2024 •

edited

Loading

cockpituous Sep 20, 2024

cockpituous Sep 20, 2024

cockpituous Sep 20, 2024

martinpitt commented Sep 23, 2024


		async def do_askpass(self, messages: str, prompt: str, hint: str) -> Optional[str]:
		if self.basic_password and 'password:' in prompt.lower():

		} catch (ex) {
		console.error("Failed to parse response text as JSON:", xhr.responseText, ":", JSON.stringify(ex));

beiboot: feature parity with cockpit-ssh #19401

beiboot: feature parity with cockpit-ssh #19401

Conversation

martinpitt commented Sep 27, 2023 • edited Loading

This comment was marked as outdated.

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

martinpitt commented Sep 17, 2024

martinpitt commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Sep 20, 2024 • edited Loading

martinpitt commented Sep 20, 2024

martinpitt commented Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt commented Sep 23, 2024

martinpitt commented Sep 27, 2023 •

edited

Loading

jelly Sep 20, 2024 •

edited

Loading

martinpitt commented Sep 20, 2024 •

edited

Loading

martinpitt commented Sep 20, 2024 •

edited

Loading