Skip to content

ws: use the failing "init" message as /login result#20954

Merged
martinpitt merged 6 commits intocockpit-project:mainfrom
allisonkarlitskaya:login-fail-json
Sep 1, 2024
Merged

ws: use the failing "init" message as /login result#20954
martinpitt merged 6 commits intocockpit-project:mainfrom
allisonkarlitskaya:login-fail-json

Conversation

@allisonkarlitskaya
Copy link
Copy Markdown
Member

@allisonkarlitskaya allisonkarlitskaya commented Aug 29, 2024 •
edited by martinpitt
Loading

If we fail a login attempt in response to receiving a

{ "command": "init", "problem", "..." }

control message from the session program, then return the message in full as the result text for the /login page.

This makes use of an until-now dead code path in cockpithandlers that pretty-prints and sets JSON data as the body of the GET /cockpit/login result if it's returned from cockpit_auth_login_finish(). Previously we were returning a fancy fail.html-formatted page which the XHR request in login.js was simply ignoring.

Going forward, this will allow us to forward more interesting errors to the login page.

Adjust the auth tests to expect the response blob in the cases where it's relevant instead of always expecting NULL on the error path.

@martinpitt martinpitt added the blocked Don't land until something else happens first (see task list) label Aug 29, 2024
@martinpitt
Copy link
Copy Markdown
Member

martinpitt commented Aug 30, 2024

I tried this PR in my local changes for PR #19401, and I am now also getting these bogus "401 Authentication required" failures without a proper error code (beiboot.py sends invalid-hostkey). This seems to come from src/ws/cockpithandlers.c on_login_complete() , and is the very reason for this change. So apparently this is missing some corresponding login.js changes to evaluate the JSON content of the reply and parse the actual error ?

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Aug 30, 2024
@martinpitt
beiboot: Handle ssh host key prompts
10acfad 
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.

We want to keep using/storing full host keys

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 reently 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" side we need to
jump through a few hoops:

 * For a previously unknown host, we only get the full host key after a
   successful login, as part of the GET /login's `login-data` response.
   So defer updating our localStorage's `known_hosts` database until
   then.

 * 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.

 * For a changed host key where the login fails due to wrong
   credentials, the login page never receives the new host key whose
   fingerprint the user just confirmed. So we can't remember that
   confirmation, and the next time the user has to re-confirm the
   changed key. cockpit-project#20954 provides some required plumbing for that. In the
   meantime, this seems acceptable as long as cockpit-beiboot isn't the
   default bastion host implementation yet.

Adjust TestLogin.testLoginSshBeiboot to only expect the host key on the
first login attempt. Add testing of changed host key behaviour.
@allisonkarlitskaya
Copy link
Copy Markdown
Member Author

allisonkarlitskaya commented Aug 30, 2024

I tried this PR in my local changes for PR #19401, and I am now also getting these bogus "401 Authentication required" failures without a proper error code (beiboot.py sends invalid-hostkey). This seems to come from src/ws/cockpithandlers.c on_login_complete() , and is the very reason for this change. So apparently this is missing some corresponding login.js changes to evaluate the JSON content of the reply and parse the actual error ?

login.js looks at the response status text, not the body. The issue is that when we have a response_data != NULL we hit a code path that hardcodes "401 Authentication required" as the status instead of running the more nuanced cockpit_web_response_gerror(). I'm working on that now.

This was referenced Aug 30, 2024
Merged
Merged
@allisonkarlitskaya
ws: give cockpit_auth_login_finish() the g_autoptr treatment
9e9e8b1 
Move most local variables to their point of initialization and change
everything to be g_autoptr() or g_autofree.  Tuck a g_info() message
into the block that would trigger its printing (instead of basing it on
inspecting a variable which gets set later).
@allisonkarlitskaya
ws: g_autoptr()-ify on_login_complete()
c32c2e5 
The usual fixes...
@allisonkarlitskaya
common: add cockpit_web_response_error_with_body()
de50217 
Add a variant of this function which accepts a body to return to the
client instead of the usual error page.  We take this approach instead
of adding a parameter to the existing function in order to avoid needing
to modify the (very) many call sites.
@allisonkarlitskaya
common: add body to cockpit_web_response_gerror()
4c538e5 
Just as with the last commit, wire through a "body" parameter on this
function.  This time we do it without adding a new variant, though, as
there aren't so many callers of this API.
@martinpitt
Copy link
Copy Markdown
Member

martinpitt commented Aug 30, 2024

Thanks, this looks promising! This failure (two tests, but common code path) seems fine to just adjust to the "new" error message. This is a regression though, that screenshot is just weird. I suppose that'll be a slight change to login.js?

@allisonkarlitskaya
ws: on_login_complete: clean up JSON error path
7763dee 
Instead of hardcoding the error message for the case where we receive a
JSON blob from cockpit_auth_login_finish(), make use of the "usual"
cockpit_web_response_gerror() machinery.

Currently this is only used by authorize challenges, in which case
login.js doesn't care about the specific error message.  In the next
commit we'll also start returning JSON for login failures caused by the
session program returning an error, and we want to ensure that this
change doesn't result in the hardcoded error message, but rather
continues to use the existing logic.
@allisonkarlitskaya allisonkarlitskaya force-pushed the login-fail-json branch 2 times, most recently from ffd500a to d2ae4ef Compare August 30, 2024 11:28
martinpitt added a commit to martinpitt/cockpit that referenced this pull request Aug 30, 2024
@martinpitt
beiboot: Handle ssh host key prompts
93654e4 
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.

We want to keep using/storing full host keys

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 reently 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" side we need to
jump through a few hoops:

 * For a previously unknown host, we only get the full host key after a
   successful login, as part of the GET /login's `login-data` response.
   So defer updating our localStorage's `known_hosts` database until
   then.

 * 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.

 * For a changed host key where the login fails due to wrong
   credentials, the login page never receives the new host key whose
   fingerprint the user just confirmed. So we can't remember that
   confirmation, and the next time the user has to re-confirm the
   changed key. cockpit-project#20954 provides some required plumbing for that. In the
   meantime, this seems acceptable as long as cockpit-beiboot isn't the
   default bastion host implementation yet.

Adjust TestLogin.testLoginSshBeiboot to only expect the host key on the
first login attempt. Add testing of changed host key behaviour.
@allisonkarlitskaya
ws: use the failing "init" message as /login result
2e1aa4f 
If we fail a login attempt in response to receiving a

  { "command": "init", "problem", "..." }

control message from the session program, then return the message in
full as the result text for the /login page.

This makes use of an until-now dead code path in cockpithandlers that
pretty-prints and sets JSON data as the body of the GET /cockpit/login
result if it's returned from cockpit_auth_login_finish().  Previously we
were returning a fancy fail.html-formatted page which the XHR request in
login.js was simply ignoring.

Going forward, this will allow us to forward more interesting errors to
the login page.

Adjust the auth unit tests to expect the response blob in the cases where
it's relevant instead of always expecting NULL on the error path.

We also have to adjust two integration tests that manually inspected
that the result of /login contained HTML: it no longer does, because
it's JSON.  Just inspect the request status reason, like login.js does.

Finally, there is a couple of cases where our "more intelligent" message
handling results in a more accurate error being reported: fix those in
the tests.
martinpitt
martinpitt approved these changes Aug 30, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! Assuming green, this looks good to me (I already reviewed while you were still working on it). Please note the "blocked" though, I'm happy to shepherd this in next week.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Aug 30, 2024
@martinpitt
beiboot: Handle ssh host key prompts
8a5f558 
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.

We want to keep using/storing full host keys

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 reently 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" side we need to
jump through a few hoops:

 * For a previously unknown host, we only get the full host key after a
   successful login, as part of the GET /login's `login-data` response.
   So defer updating our localStorage's `known_hosts` database until
   then.

 * 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.

 * For a changed host key where the login fails due to wrong
   credentials, the login page never receives the new host key whose
   fingerprint the user just confirmed. So we can't remember that
   confirmation, and the next time the user has to re-confirm the
   changed key. cockpit-project#20954 provides some required plumbing for that. In the
   meantime, this seems acceptable as long as cockpit-beiboot isn't the
   default bastion host implementation yet.

Adjust TestLogin.testLoginSshBeiboot to only expect the host key on the
first login attempt. Add testing of changed host key behaviour.
martinpitt added a commit to martinpitt/cockpit that referenced this pull request Sep 1, 2024
@martinpitt
beiboot: Handle ssh host key prompts
6ec8e6d 
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.

We want to keep using/storing full host keys

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 reently 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" side we need to
jump through a few hoops:

 * For a previously unknown host, we only get the full host key after a
   successful login, as part of the GET /login's `login-data` response.
   So defer updating our localStorage's `known_hosts` database until
   then.

 * 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.

 * For a changed host key where the login fails due to wrong
   credentials, the login page never receives the new host key whose
   fingerprint the user just confirmed. So we can't remember that
   confirmation, and the next time the user has to re-confirm the
   changed key. cockpit-project#20954 provides some required plumbing for that. In the
   meantime, this seems acceptable as long as cockpit-beiboot isn't the
   default bastion host implementation yet.

Adjust TestLogin.testLoginSshBeiboot to only expect the host key on the
first login attempt. Add testing of changed host key behaviour.
martinpitt added a commit to martinpitt/cockpit that referenced this pull request Sep 1, 2024
@martinpitt
beiboot: Handle ssh host key prompts
50ef6df 
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.

We want to keep using/storing full host keys

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 reently 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" side we need to
jump through a few hoops:

 * For a previously unknown host, we only get the full host key after a
   successful login, as part of the GET /login's `login-data` response.
   So defer updating our localStorage's `known_hosts` database until
   then.

 * 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.

 * For a changed host key where the login fails due to wrong
   credentials, the login page never receives the new host key whose
   fingerprint the user just confirmed. So we can't remember that
   confirmation, and the next time the user has to re-confirm the
   changed key. cockpit-project#20954 provides some required plumbing for that. In the
   meantime, this seems acceptable as long as cockpit-beiboot isn't the
   default bastion host implementation yet.

Adjust TestLogin.testLoginSshBeiboot to only expect the host key on the
first login attempt. Add testing of changed host key behaviour.
martinpitt added a commit to martinpitt/cockpit that referenced this pull request Sep 1, 2024
@martinpitt
beiboot: Handle ssh host key prompts
d7a5835 
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.

We want to keep using/storing full host keys

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 reently 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" side we need to
jump through a few hoops:

 * For a previously unknown host, we only get the full host key after a
   successful login, as part of the GET /login's `login-data` response.
   So defer updating our localStorage's `known_hosts` database until
   then.

 * 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.

 * For a changed host key where the login fails due to wrong
   credentials, the login page never receives the new host key whose
   fingerprint the user just confirmed. So we can't remember that
   confirmation, and the next time the user has to re-confirm the
   changed key. cockpit-project#20954 provides some required plumbing for that. In the
   meantime, this seems acceptable as long as cockpit-beiboot isn't the
   default bastion host implementation yet.

Adjust TestLogin.testLoginSshBeiboot to only expect the host key on the
first login attempt. Add testing of changed host key behaviour.
@martinpitt martinpitt removed the blocked Don't land until something else happens first (see task list) label Sep 1, 2024
@martinpitt
Copy link
Copy Markdown
Member

martinpitt commented Sep 1, 2024

Time out freezing for translation updates -- when they come in, I'll do the release not from main, but from a temporary branch (latest safe commit + cherry-pick)

@martinpitt martinpitt merged commit e617508 into cockpit-project:main Sep 1, 2024
@martinpitt martinpitt deleted the login-fail-json branch September 1, 2024 23:17
martinpitt added a commit to martinpitt/cockpit that referenced this pull request Sep 2, 2024
@martinpitt
beiboot: Handle ssh host key prompts
daf7670 
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.

We want to keep using/storing full host keys

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 reently 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" side we need to
jump through a few hoops:

 * For a previously unknown host, we only get the full host key after a
   successful login, as part of the GET /login's `login-data` response.
   So defer updating our localStorage's `known_hosts` database until
   then.

 * 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.

 * For a changed host key where the login fails due to wrong
   credentials, the login page never receives the new host key whose
   fingerprint the user just confirmed. So we can't remember that
   confirmation, and the next time the user has to re-confirm the
   changed key. cockpit-project#20954 provides some required plumbing for that. In the
   meantime, this seems acceptable as long as cockpit-beiboot isn't the
   default bastion host implementation yet.

Adjust TestLogin.testLoginSshBeiboot to only expect the host key on the
first login attempt. Add testing of changed host key behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martinpitt martinpitt martinpitt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@allisonkarlitskaya @martinpitt