Skip to content

Commit

Permalink
ws: use the failing "init" message as /login result
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@allisonkarlitskaya @martinpitt
allisonkarlitskaya authored and martinpitt committed Sep 1, 2024
1 parent 130e10f commit e617508
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 18 deletions.
17 changes: 14 additions & 3 deletions src/ws/cockpitauth.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down
61 changes: 52 additions & 9 deletions src/ws/test-auth.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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"));
Expand All @@ -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);
Expand All @@ -489,6 +499,7 @@ typedef struct {
const gchar *error_message;
const gchar *warning;
const gchar *path;
const gchar *init_problem;
int error_code;
} ErrorFixture;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -783,6 +812,7 @@ typedef struct {
const gchar *error_message;
const gchar *message;
int error_code;
const gchar *init_problem;
int pause;
} ErrorMultiFixture;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1076,20 +1116,23 @@ 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 = {
.headers = two_steps_wrong,
.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 = {
.headers = two_steps_ssh_wrong,
.prompts = two_prompts,
.error_code = COCKPIT_ERROR_AUTHENTICATION_FAILED,
.error_message = "Authentication failed",
.init_problem = "authentication-failed",
};

static const ErrorMultiFixture fixture_fail_step_timeout = {
Expand Down
6 changes: 3 additions & 3 deletions test/verify/check-static-login
View file
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ matchrule = <SUBJECT>^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", '<h1>Authentication failed</h1>'],
["HTTP/1.1 401 Authentication failed"],
not_expected=["crsf-token"])

# check expired certificate
Expand All @@ -893,7 +893,7 @@ matchrule = <SUBJECT>^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', '<h1>Authentication disabled</h1>'],
['HTTP/1.1 401 Authentication disabled'],
not_expected=["crsf-token"])

# wwithout a CA, alice's cert fails
Expand All @@ -906,7 +906,7 @@ matchrule = <SUBJECT>^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", '<h1>Authentication failed</h1>'],
do_test(alice_cert_key, ["HTTP/1.1 401 Authentication failed"],
not_expected=["crsf-token"])
m.execute("systemctl unmask sssd-ifp")

Expand Down
6 changes: 3 additions & 3 deletions test/verify/check-system-realms
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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", '<h1>Authentication failed</h1>'],
["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', '<h1>Authentication disabled</h1>'],
do_test(alice_user_pass, ['HTTP/1.1 401 Authentication disabled'],
not_expected=["crsf-token"])

# valid user certificate which fails CA validation
Expand Down

0 comments on commit e617508

Please sign in to comment.