Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

login: Warn before logging into multiple hosts #20861

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+106 −5
Draft

login: Warn before logging into multiple hosts #20861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/static/login.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@
<span id="banner-message" class="pf-v5-c-alert__description"></span>
</div>

<div id="multihost-warning" class="pf-v5-c-alert pf-m-info pf-m-inline dialog-error" aria-label="inline danger alert" hidden="true">
<svg fill="currentColor" viewBox="0 0 448 512" aria-hidden="true">
<path d="M224 512c35.32 0 63.97-28.65 63.97-64H160.03c0 35.35 28.65 64 63.97 64zm215.39-149.71c-19.32-20.76-55.47-51.99-55.47-154.29 0-77.7-54.48-139.9-127.94-155.16V32c0-17.67-14.32-32-31.98-32s-31.98 14.33-31.98 32v20.84C118.56 68.1 64.08 130.3 64.08 208c0 102.3-36.15 133.53-55.47 154.29-6 6.45-8.66 14.16-8.61 21.71.11 16.4 12.98 32 32.1 32h383.8c19.12 0 32-15.6 32.1-32 .05-7.55-2.61-15.27-8.61-21.71z" />
</svg>
<p id="multihost-message" class="pf-v5-c-alert__title"></p>
<button id="multihost-get-me-there" class="pf-v5-c-button">Go there</button>
</div>

<span id="badge"></span>

<div class="container" id="main">
Expand Down
12 changes: 10 additions & 2 deletions pkg/static/login.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,8 +369,16 @@ function debug(...args) {
}
}

if (cur_machine && !environment.page.allow_multihost)
redirect_to_current_machine();
if (cur_machine) {
if (!environment.page.allow_multihost)
redirect_to_current_machine();
else {
id("multihost-message").textContent = format(_("You are already connected to '$0' in this browser session. Connecting to other hosts will allow them to execute arbitrary code on each other. Please be careful."),
cur_machine == "." ? "localhost" : cur_machine);
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test.

id("multihost-get-me-there").addEventListener("click", redirect_to_current_machine);
show('#multihost-warning');
}
}
}

function boot() {
Expand Down
4 changes: 2 additions & 2 deletions pkg/static/login.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,14 +355,14 @@ label.checkbox {
display: none;
}

.login-pf #banner {
.login-pf #banner, .login-pf #multihost-warning {
margin-block: 1rem 0.5rem;
margin-inline: 0;
grid-area: banner;
inline-size: 100%;
}

#banner-message {
#banner-message, #multihost-message {
Comment on lines -358 to +365
Copy link
Member

Choose a reason for hiding this comment

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

We really shouldn't target IDs in the CSS. We already do, I know, but we shouldn't add more. (The selectors should be targeting classes.)

We especially should not have IDs nested under other selectors like this.

Ideally, we'd fix the older parts by changing those too.

Anyway, I'm not expecting you to have to fix this, just pointing out that it's bad form. We're already doing bad stuff anyway in the file, so it's not a huge deal. But it's important for new code and we should (at some point) clean up instances where IDs are used in CSS. It's important everyone on the team understands this, as using IDs leads to other bad things, like CSS not working as expected, resulting in !important and other bad things.

https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/Selectors/Type_Class_and_ID_Selectors#id_selectors

The ID selector has high specificity. This means styles applied based on matching an ID selector will overrule styles applied based on other selector, including class and type selectors. Because an ID can only occur once on a page and because of the high specificity of ID selectors, it is preferable to add a class to an element instead of an ID. If using the ID is the only way to target the element — perhaps because you do not have access to the markup and cannot edit it — consider using the ID within an attribute selector, such as p[id="header"]. Learn specificity.

(Note: We shouldn't use an attribute selector as it suggests, but actually fix the HTML.)

IDs are great and to be used in JavaScript for quick specific element selecting (as there's only one unique instance of an ID allowed on a document), of course. But it's bad for CSS.

Copy link
Member

Choose a reason for hiding this comment

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

(BTW: I'm not blocking on this in this PR or nitpicking, just pointing out something that would have really bad side effects, especially elsewhere in Cockpit.)

white-space: pre-wrap;
max-block-size: 12em;
overflow: auto;
Expand Down
2 changes: 1 addition & 1 deletion test/reference
Open in desktop
Submodule reference updated 5 files
+ − TestLoginMultiHost-testBasic-warning-dark-pixels.png
+ − TestLoginMultiHost-testBasic-warning-medium-pixels.png
+ − TestLoginMultiHost-testBasic-warning-mobile-pixels.png
+ − TestLoginMultiHost-testBasic-warning-pixels.png
+ − TestLoginMultiHost-testBasic-warning-rtl-pixels.png
85 changes: 85 additions & 0 deletions test/verify/check-static-login
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,5 +1111,90 @@ matchrule = <SUBJECT>^DC=LAN,DC=COCKPIT,CN=alice$
check_store_hostkey(real_hostkey)


@testlib.skipBeiboot("host switching disabled in beiboot mode")
class TestLoginMultiHost(testlib.MachineCase):
def testBasic(self):
b = self.browser
m1 = self.machines["machine1"]

ip = "172.27.0.15"

# Disallow multiple sessions initially

if self.multihost_enabled:
m1.write("/etc/cockpit/cockpit.conf",
'[WebService]\nAllowMultiHost=no\n')

m1.start_cockpit()

# Log in via SSH

b.open(f"/={ip}/playground")
b.set_val('#login-user-input', "admin")
b.set_val('#login-password-input', "foobar")
b.click("#login-button")
b.wait_in_text("#hostkey-fingerprint", "SHA256:")
b.click("#login-button")
b.wait_visible('#content')
self.assertEqual(b.eval_js("window.location.pathname"), f"/={ip}/playground")

# Go back to login page. The SSH session from above will still
# be active (for some time until it times out) and the login
# page will immediately redirect us to it. We will end up on
# the Overview (instead of the playground).

b.eval_js("window.location = '/'") # b.go would run into a "navigation canceled error"
b.wait_js_cond(f"window.location.pathname == '/={ip}/system'")
b.wait_visible('#content')
b.logout()

# Enable host switcher for the rest of the test

if self.multihost_enabled:
# clean up AllowMultiHost=no from above
m1.execute("rm /etc/cockpit/cockpit.conf")
self.enable_multihost(m1)
m1.restart_cockpit()

# Log in via SSH

b.open("/")
b.set_val('#login-user-input', "admin")
b.set_val('#login-password-input', "foobar")
b.click("#show-other-login-options")
b.set_val("#server-field", ip)
b.click("#login-button")
b.wait_visible('#content')

# Go back to login page. The SSH session will still be active
# as before, but now the login page will merely warn us about
# it.

b.open("/")
b.wait_in_text("#multihost-warning", f"You are already connected to '{ip}' in this browser session")
b.assert_pixels("body", "warning")

# Clicking on the button should redirect us to that session

b.click("#multihost-warning button")
b.wait_visible('#content')
self.assertEqual(b.eval_js("window.location.pathname"), f"/={ip}/system")

# Go back to login and log in locally anyway.

b.open("/")
b.set_val('#login-user-input', "admin")
b.set_val('#login-password-input', "foobar")
b.click("#login-button")
b.wait_visible('#content')
self.assertEqual(b.eval_js("window.location.pathname"), "/system")

# Go back to first session, it should still be there

b.open(f"/={ip}")
b.wait_visible('#content')
self.assertEqual(b.eval_js("window.location.pathname"), f"/={ip}/system")


if __name__ == '__main__':
testlib.test_main()
Loading