login: Prevent multiple logins in a single browser session #20860
Conversation
|
This is the first half of #20834. I think we should get it in, before we figure out the warning texts. |
mvollmer
force-pushed
the
login-no-multihost
branch
from
020cc27
to
4a5557e
Compare
|
Nice, TestEmbed.testBasic got broken, so we do test this! |
mvollmer
force-pushed
the
login-no-multihost
branch
2 times, most recently
from
6e0d509
to
38a681e
Compare
Changing the Shell= setting back without restarting cockpit has no effect.
mvollmer
force-pushed
the
login-no-multihost
branch
from
38a681e
to
51e0c02
Compare
|
Nicely testing this requires Browser tab support in testlib.py. Let's not block on that. The check-embed test covers this a bit, fortunately. |
unless allowed by cockpit.conf. If the login page is loaded and a valid session cookie is already available, then we are about to log into a second host from the same browser session, and both logins will have access to each others cookies. This should only be allowed when AllowMultiHost is true. If it is not true, the login page immediately redirects to the session for the existing cookie. Information about session cookies is not available to login.js, so cockpit-ws helps out by exposing which ones are present, without exposing the cookies themselves.
mvollmer
force-pushed
the
login-no-multihost
branch
from
51e0c02
to
1dd4eca
Compare
| // from two machines into the same browser origin. This needs | ||
| // to be allowed explicitly via a configuration setting. | ||
|
|
||
| const logged_into = environment.logged_into || []; |
There was a problem hiding this comment.
This added line is not executed by any test.
| if (cur_machine === ".") { | ||
| login_reload("/"); | ||
| } else { | ||
| login_reload("/=" + cur_machine); |
There was a problem hiding this comment.
This added line is not executed by any test.
There was a problem hiding this comment.
Thanks! This wasn't on my radar (no review request). This by and large looks right, and it would indeed be a really nice fix for tomorrow's release. I found a few style issues, but they are not critical and could become follow-ups. Are you otherwise happy with this?
|
|
||
| if (cookie_value) | ||
| { | ||
| decoded = base64_decode_string (cookie_value); |
There was a problem hiding this comment.
You can use g_autofree here, drop the declaration in line 1248 and the g_free below.
update: Oh, this is moved code. OK to keep if you prefer not to touch it.
| // to be allowed explicitly via a configuration setting. | ||
|
|
||
| const logged_into = environment.logged_into || []; | ||
| const cur_machine = logged_into.length > 0 ? logged_into[0] : null; |
There was a problem hiding this comment.
const cur_machine = environment.logged_into?.[0]
(but if you prefer this, it's fine)
There was a problem hiding this comment.
Looks too much like Perl for my taste... :-) but let's do it.
| gboolean cockpit_auth_is_valid_cookie_value (CockpitAuth *self, | ||
| const gchar *cookie_value); | ||
|
|
||
| gchar * cockpit_auth_parse_application (const gchar *path, |
| gchar *h = g_hash_table_lookup (request_headers, "Cookie"); | ||
| while (h && *h) { | ||
| const gchar *start = h; | ||
| while (*h && *h != '=') | ||
| h++; | ||
| const gchar *equal = h; | ||
| while (*h && *h != ';') | ||
| h++; | ||
| const gchar *end = h; | ||
| if (*h) | ||
| h++; | ||
| while (*h && *h == ' ') | ||
| h++; | ||
|
|
||
| if (*equal != '=') | ||
| continue; | ||
|
|
||
| g_autofree gchar *value = g_strndup (equal + 1, end - equal - 1); | ||
|
|
||
| if (!cockpit_auth_is_valid_cookie_value (auth, value)) | ||
| continue; | ||
|
|
||
| g_autofree gchar *name = g_strndup (start, equal - start); |
There was a problem hiding this comment.
Hmm, do you want sscanf() here?
There was a problem hiding this comment.
Oh, that would be the second time in my life that I use sscanf... hmm.
|
I tested this with the flatpak, and it behaves the same way: Opening a second window goes straight into the running session. We should be able to improve that by isolating the webkit instances from one another somehow, but that can be a follow-up. |
unless allowed by cockpit.conf.
If the login page is loaded and a valid session cookie is already available, then we are about to log into a second host from the same browser session, and both logins will have access to each others cookies. This should only be allowed when AllowMultiHost is true. If it is not true, the login page immediately redirects to the session for the existing cookie.
Information about session cookies is not available to login.js, so cockpit-ws helps out by exposing which ones are present, without exposing the cookies themselves.