-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Various login/beiboot preparations/cleanups #20967
Conversation
Having a random "cookie" here brings absolutely no security — it just makes it more difficult to test. Use predictable sequence numbers instead.
This lets you get access to the underlying response object, in case you want to get extra fields out of it.
These have diverged enough that splitting them out from each other substantially improves readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
pkg/static/login.js
Outdated
@@ -1,5 +1,10 @@ | |||
import "./login.scss"; | |||
|
|||
function debug(...args) { | |||
if (window.debugging == 'all' || window.debugging?.includes('login')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
===
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this debugging?.includes()
business is black magic :) I'd have expected .?()
to be needed there as well... I guess that has to do with Javascript's strange this
binding logic... TIL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do ===
, that was mostly copy pasta from all the other places.
I don't understand your .?()
comment though -- either window.debugging
is set, then it has to be a string and has an includes()
method. Or it's undefined, then debugging?
short-circuits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that I read this parsed like so:
( (window . debugging) ?. includes ) ()
and assuming that window.debugging
is undefined, then I'd also expect (window.debugging)?.includes
to be indefined. You can't call undefined()
, which is why I'd have expected a ?.()
to be required. But your code works, I tested it. I just don't understand why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a dozen copy of that code everywhere. Indeed ?.
is a bit magic for function calls, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining.
I.e. foo?.method(...)
is undefined
(and nothing gets called) iff foo
is undefined or null.
@@ -770,15 +770,15 @@ import "./login.scss"; | |||
function do_hostkey_verification(data) { | |||
const key_db = get_known_hosts_db(); | |||
const key = data["host-key"]; | |||
const key_key = key.split(" ")[0]; | |||
const key_host = key.split(" ")[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3. Srsly. This confused the heck out of me too.
...tests are angry, though :( |
"beiboot: support for temporary UserKnownHostsFile" causes the TestClient regressions. I made the code backwards compatible when login.js does not send the known host keys after |
.. but I'll look into this some more, we also wanted to change the scope of the TemporaryFile |
Nah, this is stupid. Let's keep that commit in #19401. |
We don't use this anywhere right now, but soon will.
The first component of the `host-key` protocol field is the host name/IP, which is used to index the known_hosts key database. It's confusing to call this `key_key`, as it's really not the key (nor fingerprint) material.
In Fedora 41, systemd ships a new file /etc/ssh/ssh_config.d/20-systemd-ssh-proxy.conf which is owned by systemd_conf_t. Allow cockpit-ws to read that, otherwise the whole `ssh` command fails with a SELinux denial as it can't read that config file.
This factorizes the current value and pins it down to a single query at the time when clicking the "Login" button. That avoids a small race condition where the user may change the field while the asynchronous login process is running.
bfcec78
to
971b727
Compare
if (window.debugging === 'all' || window.debugging?.includes('login')) | ||
console.debug('login:', ...args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 added lines are not executed by any test.
@@ -552,8 +557,7 @@ import "./login.scss"; | |||
} | |||
|
|||
function host_failure(msg) { | |||
const host = id("server-field").value; | |||
if (!host) { | |||
if (!login_machine) { |
There was a problem hiding this comment.
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.
const user = trim(id("login-user-input").value); | ||
if (user === "" && !environment.is_cockpit_client) { | ||
login_failure(_("User name cannot be empty")); | ||
} else if (need_host() && id("server-field").value === "") { | ||
} else if (need_host() && login_machine === "") { |
There was a problem hiding this comment.
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.
if (key_db[key_host] == key) { | ||
debug("do_hostkey_verification: received key matches known_hosts database, auto-accepting fingerprint", data.default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 added lines are not executed by any test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the tempfile stuff moving to the other PR anyway: as we've seen, it needs some changes.
Broken out from #19401.