From e617508dfce40348c830dea5ac5755425bbdd8f6 Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Thu, 29 Aug 2024 13:39:32 +0200 Subject: [PATCH] ws: use the failing "init" message as /login result 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. --- src/ws/cockpitauth.c | 17 +++++++-- src/ws/test-auth.c | 61 ++++++++++++++++++++++++++++----- test/verify/check-static-login | 6 ++-- test/verify/check-system-realms | 6 ++-- 4 files changed, 72 insertions(+), 18 deletions(-) diff --git a/src/ws/cockpitauth.c b/src/ws/cockpitauth.c index 8f27ab14c2e..f603a9bdc1a 100644 --- a/src/ws/cockpitauth.c +++ b/src/ws/cockpitauth.c @@ -125,6 +125,9 @@ typedef struct { /* An authorize challenge from session */ JsonObject *authorize; + /* The failing "init" message from the session */ + JsonObject *init_failure; + /* The conversation in progress */ gchar *conversation; } CockpitSession; @@ -156,6 +159,8 @@ cockpit_session_reset (gpointer data) session->cookie = NULL; self = session->auth; + g_clear_pointer (&session->init_failure, json_object_unref); + /* No accessing session after this point */ if (cookie) @@ -750,6 +755,8 @@ on_transport_control (CockpitTransport *transport, if (!cockpit_json_get_string (options, "message", NULL, &message)) message = NULL; propagate_problem_to_error (session, options, problem, message, &error); + g_clear_pointer (&session->init_failure, json_object_unref); + session->init_failure = json_object_ref (options); if (session->login_task == NULL) { g_message ("ignoring failure from session process: %s", error->message); @@ -1535,13 +1542,17 @@ cockpit_auth_login_finish (CockpitAuth *self, g_return_val_if_fail (g_task_is_valid (result, self), NULL); + CockpitSession *session = g_task_get_task_data (G_TASK (result)); + if (!g_task_propagate_boolean (G_TASK (result), error)) - goto out; + { + if (session != NULL) + body = g_steal_pointer (&session->init_failure); + goto out; + } - CockpitSession *session = g_task_get_task_data (G_TASK (result)); g_return_val_if_fail (session != NULL, NULL); g_return_val_if_fail (session->login_task == NULL, NULL); - cockpit_session_reset (session); if (session->authorize) diff --git a/src/ws/test-auth.c b/src/ws/test-auth.c index dc5d9541db6..c1c428f711b 100644 --- a/src/ws/test-auth.c +++ b/src/ws/test-auth.c @@ -260,7 +260,10 @@ test_userpass_bad (Test *test, response = cockpit_auth_login_finish (test->auth, result, NULL, headers, &error); g_object_unref (result); - g_assert (response == NULL); + g_assert (response != NULL); + g_assert_cmpstr (json_object_get_string_member (response, "problem"), ==, "authentication-failed"); + g_clear_pointer (&response, json_object_unref); + g_assert_error (error, COCKPIT_ERROR, COCKPIT_ERROR_AUTHENTICATION_FAILED); g_clear_error (&error); @@ -289,7 +292,10 @@ test_userpass_emptypass (Test *test, response = cockpit_auth_login_finish (test->auth, result, NULL, headers, &error); g_object_unref (result); - g_assert (response == NULL); + g_assert (response != NULL); + g_assert_cmpstr (json_object_get_string_member (response, "problem"), ==, "authentication-failed"); + g_clear_pointer (&response, json_object_unref); + g_assert_error (error, COCKPIT_ERROR, COCKPIT_ERROR_AUTHENTICATION_FAILED); g_clear_error (&error); @@ -461,8 +467,10 @@ test_max_startups (Test *test, g_main_context_iteration (NULL, TRUE); response = cockpit_auth_login_finish (test->auth, result1, NULL, NULL, &error1); g_object_unref (result1); - g_assert (response == NULL); + g_assert (response != NULL); + g_assert_cmpstr (json_object_get_string_member (response, "problem"), ==, "authentication-failed"); g_assert_cmpstr ("Authentication failed", ==, error1->message); + g_clear_pointer (&response, json_object_unref); /* Now that first is finished we can successfully run another one */ g_hash_table_insert (headers_fail, g_strdup ("Authorization"), g_strdup ("testscheme fail")); @@ -473,7 +481,9 @@ test_max_startups (Test *test, g_main_context_iteration (NULL, TRUE); response = cockpit_auth_login_finish (test->auth, result3, NULL, NULL, &error3); g_object_unref (result3); - g_assert (response == NULL); + g_assert (response != NULL); + g_assert_cmpstr (json_object_get_string_member (response, "problem"), ==, "authentication-failed"); + g_clear_pointer (&response, json_object_unref); g_assert_cmpstr ("Authentication failed", ==, error3->message); g_clear_error (&error1); @@ -489,6 +499,7 @@ typedef struct { const gchar *error_message; const gchar *warning; const gchar *path; + const gchar *init_problem; int error_code; } ErrorFixture; @@ -530,7 +541,17 @@ test_custom_fail (Test *test, response = cockpit_auth_login_finish (test->auth, result, NULL, headers, &error); g_object_unref (result); - g_assert (response == NULL); + if (fix->init_problem) + { + g_assert (response != NULL); + g_assert_cmpstr (json_object_get_string_member (response, "problem"), ==, fix->init_problem); + g_clear_pointer (&response, json_object_unref); + } + else + { + g_assert (response == NULL); + } + if (fix->error_code) g_assert_error (error, COCKPIT_ERROR, fix->error_code); else @@ -679,36 +700,41 @@ static const SuccessFixture fixture_ssh_alt = { static const ErrorFixture fixture_bad_conversation = { .header = "X-Conversation conversation-id xxx", - .error_message = "Invalid conversation token" + .error_message = "Invalid conversation token", }; static const ErrorFixture fixture_ssh_basic_failed = { .error_message = "Authentication failed", - .header = "Basic dXNlcjp0aGlzIGlzIHRoZSBwYXNzd29yZA==" + .header = "Basic dXNlcjp0aGlzIGlzIHRoZSBwYXNzd29yZA==", + .init_problem = "authentication-failed", }; static const ErrorFixture fixture_ssh_remote_basic_failed = { .error_message = "Authentication failed", .header = "Basic d3Jvbmc6dGhpcyBpcyB0aGUgbWFjaGluZSBwYXNzd29yZA==", - .path = "/cockpit+=machine" + .path = "/cockpit+=machine", + .init_problem = "authentication-failed", }; static const ErrorFixture fixture_ssh_not_supported = { .error_code = COCKPIT_ERROR_AUTHENTICATION_FAILED, .error_message = "Authentication failed: authentication-not-supported", .header = "testsshscheme not-supported", + .init_problem = "authentication-not-supported", }; static const ErrorFixture fixture_ssh_auth_failed = { .error_code = COCKPIT_ERROR_AUTHENTICATION_FAILED, .error_message = "Authentication failed", .header = "testsshscheme ssh-fail", + .init_problem = "authentication-failed", }; static const ErrorFixture fixture_ssh_auth_with_error = { .error_code = COCKPIT_ERROR_FAILED, .error_message = "Authentication failed: unknown: detail for error", .header = "testsshscheme with-error", + .init_problem = "unknown", }; static const SuccessFixture fixture_no_cookie = { @@ -739,18 +765,21 @@ static const ErrorFixture fixture_auth_failed = { .error_code = COCKPIT_ERROR_AUTHENTICATION_FAILED, .error_message = "Authentication failed", .header = "testscheme fail", + .init_problem = "authentication-failed", }; static const ErrorFixture fixture_auth_denied = { .error_code = COCKPIT_ERROR_PERMISSION_DENIED, .error_message = "Permission denied", .header = "testscheme denied", + .init_problem = "access-denied", }; static const ErrorFixture fixture_auth_with_error = { .error_code = COCKPIT_ERROR_FAILED, .error_message = "Authentication failed: unknown: detail for error", .header = "testscheme with-error", + .init_problem = "unknown", }; static const ErrorFixture fixture_auth_none = { @@ -783,6 +812,7 @@ typedef struct { const gchar *error_message; const gchar *message; int error_code; + const gchar *init_problem; int pause; } ErrorMultiFixture; @@ -1030,7 +1060,17 @@ test_multi_step_fail (Test *test, } else { - g_assert (response == NULL); + if (fix->init_problem) + { + g_assert (response != NULL); + g_assert_cmpstr (json_object_get_string_member (response, "problem"), ==, fix->init_problem); + g_clear_pointer (&response, json_object_unref); + } + else + { + g_assert (response == NULL); + } + if (fix->error_code) g_assert_error (error, COCKPIT_ERROR, fix->error_code); else @@ -1076,6 +1116,7 @@ static const ErrorMultiFixture fixture_fail_three_steps = { .prompts = three_prompts, .error_code = COCKPIT_ERROR_AUTHENTICATION_FAILED, .error_message = "Authentication failed", + .init_problem = "authentication-failed", }; static const ErrorMultiFixture fixture_fail_two_steps = { @@ -1083,6 +1124,7 @@ static const ErrorMultiFixture fixture_fail_two_steps = { .prompts = two_prompts, .error_code = COCKPIT_ERROR_AUTHENTICATION_FAILED, .error_message = "Authentication failed", + .init_problem = "authentication-failed", }; static const ErrorMultiFixture fixture_fail_ssh_two_steps = { @@ -1090,6 +1132,7 @@ static const ErrorMultiFixture fixture_fail_ssh_two_steps = { .prompts = two_prompts, .error_code = COCKPIT_ERROR_AUTHENTICATION_FAILED, .error_message = "Authentication failed", + .init_problem = "authentication-failed", }; static const ErrorMultiFixture fixture_fail_step_timeout = { diff --git a/test/verify/check-static-login b/test/verify/check-static-login index 752ee1ba4ed..a83552d6bc4 100755 --- a/test/verify/check-static-login +++ b/test/verify/check-static-login @@ -875,7 +875,7 @@ matchrule = ^DC=LAN,DC=COCKPIT,CN=alice$ # another certificate gets rejected self.allow_journal_messages("cockpit-session: No matching user for certificate") do_test(["--cert", self.vm_tmpdir + "/bob.pem", "--key", self.vm_tmpdir + "/bob.key"], - ["HTTP/1.1 401 Authentication failed", '

Authentication failed

'], + ["HTTP/1.1 401 Authentication failed"], not_expected=["crsf-token"]) # check expired certificate @@ -893,7 +893,7 @@ matchrule = ^DC=LAN,DC=COCKPIT,CN=alice$ m.write("/etc/cockpit/cockpit.conf", "[Basic]\naction = none\n", append=True) do_test(alice_cert_key, ['HTTP/1.1 200 OK', '"csrf-token"']) do_test(['-u', 'alice:foobar123'], - ['HTTP/1.1 401 Authentication disabled', '

Authentication disabled

'], + ['HTTP/1.1 401 Authentication disabled'], not_expected=["crsf-token"]) # wwithout a CA, alice's cert fails @@ -906,7 +906,7 @@ matchrule = ^DC=LAN,DC=COCKPIT,CN=alice$ self.allow_journal_messages("cockpit-session: Failed to map .* Could not activate remote peer.*") self.allow_journal_messages("cockpit-session: Failed to map .* Unit sssd-ifp.service is masked.") m.execute("systemctl mask sssd-ifp; systemctl stop sssd-ifp") - do_test(alice_cert_key, ["HTTP/1.1 401 Authentication failed", '

Authentication failed

'], + do_test(alice_cert_key, ["HTTP/1.1 401 Authentication failed"], not_expected=["crsf-token"]) m.execute("systemctl unmask sssd-ifp") diff --git a/test/verify/check-system-realms b/test/verify/check-system-realms index b905d3b9f11..92609aec54b 100755 --- a/test/verify/check-system-realms +++ b/test/verify/check-system-realms @@ -432,7 +432,7 @@ class CommonTests: self.allow_journal_messages("cockpit-session: user account access failed: 4 alice: System error") # cert auth should not be enabled by default - do_test(alice_cert_key, ["HTTP/1.1 401 Authentication required", '"authorize"']) + do_test(alice_cert_key, ["HTTP/1.1 401 Authentication failed", '"authorize"']) # password auth should work (but might need to be retried) do_test(alice_user_pass, ['HTTP/1.1 200 OK', '"csrf-token"'], session_leader='cockpit-session', retry=True) @@ -458,14 +458,14 @@ class CommonTests: self.allow_journal_messages("cockpit-session: No matching user for certificate") m.upload(["bob.pem", "bob.key"], "/var/tmp", relative_dir="src/tls/ca/") do_test(['--cert', "/var/tmp/bob.pem", '--key', "/var/tmp/bob.key"], - ["HTTP/1.1 401 Authentication failed", '

Authentication failed

'], + ["HTTP/1.1 401 Authentication failed"], not_expected=["crsf-token"]) self.allow_journal_messages("cockpit-session: Failed to map certificate to user: .* Invalid certificate provided") # disallow password auth m.write("/etc/cockpit/cockpit.conf", "[Basic]\naction = none\n", append=True) do_test(alice_cert_key, ['HTTP/1.1 200 OK', '"csrf-token"']) - do_test(alice_user_pass, ['HTTP/1.1 401 Authentication disabled', '

Authentication disabled

'], + do_test(alice_user_pass, ['HTTP/1.1 401 Authentication disabled'], not_expected=["crsf-token"]) # valid user certificate which fails CA validation