-
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
pybridge: Add initial authorize request to cockpit-beiboot, and handle host key prompts #19401
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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 looks like it's coming together really nicely and I'm starting to get excited about the finished result. 🚀
src/cockpit/beiboot.py
Outdated
|
||
async def do_askpass(self, messages: str, prompt: str, hint: str) -> Optional[str]: | ||
if self.basic_password and 'password:' in prompt.lower(): |
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.
yikes...
I guess this will work "for now" but we really need to do something more (airquotes) 'professional' here...
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.
Yep, see my comment above, that's why it's still draft. Of course we could also consider/mark this as # HACK
and update ferny as a follow-up. I still do want to do that, but it's much nicer to iterate on something that actually works.
774f2ee
to
60c6729
Compare
edae05b
to
ee489a1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
fede982
to
1220a19
Compare
This comment was marked as outdated.
This comment was marked as outdated.
d7f3fa3
to
a10e8a8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a10e8a8
to
f3ee0e7
Compare
aeae62e
to
f21866b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
f21866b
to
fb73cc7
Compare
03fb291
to
90e585a
Compare
I'm a bit tired of this PR being blocked for so long, and due to $reasons I don't see #19668 land in the next 4 months either. That string parsing hack isn't so bad IMHO, it's effectively the same what ferny does (just admittedly less elegant), but both are covered by integration tests. So I rebased this again, added a proper pointer to #19668, and ask for review again. Thanks! |
@allisonkarlitskaya gentle review ping? |
Let's talk about this today in 1:1. |
@@ -181,9 +196,20 @@ def __init__(self, router: Router, destination: str, args: argparse.Namespace): | |||
super().__init__(router) | |||
|
|||
async def do_connect_transport(self) -> None: | |||
# do we have user/password (Basic auth) from the login page? | |||
auth = await self.router.request_authorization("*") |
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.
@allisonkarlitskaya IIRC you were worried that this PR changes the Client. It's not supposed to, as that will just "fail" (i.e. give empty-ish reply ":"
) on the client, as the login page doesn't have user and password fields. This only applies to using cockpit-beiboot as cockpit-ssh replacement on the "normal" login page.
|
||
if self.have_basic_password and 'password:' in prompt.lower(): | ||
# the UI currently expects us to fail on a wrong password, so only send it once | ||
if self.basic_password is not None: |
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 confused the heck out of me until I realized that you were setting self.basic_password = None
below.
As far as I can tell, the code is correct as it's written, but the have_basic_password
should definitely be renamed to something like had_
or got_
to remove the suggestion that it applies to the current state.
I might also just drop that and do something like
if self.basic_password and 'password:' in prompt.lower():
if self.password_attempted:
raise ...
self.password_attempted = True
return self.basic_password
It has the disadvantage of not "freeing" the password, but you're not really scrubbing it anyway...
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.
Wrt. the renaming: it may be the current state, just that have_basic_password stays (to remember in which "mode" we are), and basic_password gets consumed, as it doesn't make sense to try more than once. got_
is certainly fine, but if you think password_attempted
is cleaner to read, that's fine for me.
Scrubbing in Python is indeed difficult, as str
is immutable AFAIK. I.e. the only thing I can hope for is that the memory gets GCed and eventually overwritten. This may be possible with StringIO
or similar, but I figure all of this is kind of hopeless in Python, and we rather rely on ptrace protection and such..
user, basic_password = base64.b64decode(auth.split(' ', 1)[1]).decode().split(':', 1) | ||
if user: # this can be empty, i.e. auth is just ":" | ||
logger.debug("got username %s and password from Basic auth", user) | ||
ssh_opts = ['-l', user] |
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.
What happens if you give a username and connect to a destination like admin@c
. Who wins? Should we have a 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.
The user name wins: ssh -l admin root@c
logs in as "admin". I did that in commit 38fa110 for the shell, happy to do a similar test case for the login page.
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.
Correction: We already do have a test, right in this PR:
# colliding usernames; user names in "Connect to:" are *not* supported,
# but pin down the behaviour
try_login("admin", "foobar", server=f"other@{my_ip}")
check_session()
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.
Cool! Didn't see that, thanks!
src/cockpit/beiboot.py
Outdated
fp_match = re.search(r'\n(\w+) key fingerprint is ([^.]+)\.', prompt) | ||
args = {} | ||
if fp_match: | ||
args['host-key'] = f'{fp_match.group(2)} {fp_match.group(1)}' |
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 guess this is still unresolved — I worry about existing users who might be concerned that their key no longer matches, on upgrade. Do we have a test for that?
src/cockpit/beiboot.py
Outdated
fp_match = re.search(r'\n(\w+) key fingerprint is ([^.]+)\.', prompt) | ||
args = {} | ||
if fp_match: | ||
args['host-key'] = f'{fp_match.group(2)} {fp_match.group(1)}' |
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.
(don't get me wrong — I think this change is good and ultimately necessary if we're to abandon our dependency on hostkey commands... but I feel like we need to try a bit harder on the migration front...)
90e585a
to
7da876c
Compare
7da876c
to
6a9a3e0
Compare
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.
A thought: in the browser we know what key type we already know for the host in question. Could we somehow proactively send that to the -ws in a way that gets passed on to cockpit.beiboot
to either let it (a) do some more intelligent comparison, or (b) hint ssh about which kind of host key we want to see?
Proper review coming tomorrow :)
@@ -1,5 +1,8 @@ | |||
import "./login.scss"; | |||
|
|||
import sha256 from "js-sha256"; | |||
import { base64_decode, base64_encode } from "_internal/base64"; |
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.
lol that didn't take long...
Wrt. the active key type request: This is a problem which we've had forever, in main: The login page only receives (and stores) a single key in its database, whichever key type ssh decides to use and report. Aside from the fact that we want to get rid of cockpit-ssh, What could possibly work is to (1) get rid of cockpit-ssh and replace it with cockpit-beiboot/ssh (for which this PR is the first stepping stone, but there's still a lot of further work to do), (2) let cockpit-beiboot call ssh(1) in a way that it writes the host keys into a temporary TBH this is the very last thing I want to work on now... |
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'm still quite worried about the migration.
And: once we do this, there's no going back. If we want to imagine a future scenario where we put the key from the browser into a temporary known-hosts file and run ssh
with that file and send the result (with added host keys) back to the browser, we can't do that anymore (since we only have fingerprints now, and hashing is one way).
I also know you want to get this done and out of the way so you can work on more interesting things...
Maybe I'm too worried about localstorage. This is browsers after all. People clear their cookies and lose their phones and stuff all the time, don't they?
if (db_key) { | ||
// exact same key → good | ||
if (key_db[key_key] == key) { | ||
converse(data.id, 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.
We can also reply with the body of the key, right? Is that what default already is?
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.
Yes, it is. It is really poorly named, but I don't want to change protocol here between three moving parts (login.js, cockpit-ssh, and cockpit-beiboot). Reworking this gets easier when at least cockpit-ssh is gone.
pkg/static/login.js
Outdated
* "myhost ecdsa-sha2-nistp256 AAAA[..]ToM=\n"; ssh(1) does not show them, only fingerprints, so | ||
* cockpit-beiboot only responds with fingerprints as well; they look like "myhost ED25519 SHA256:JVG[..]xs" | ||
* check if we can match the given fingerprint to an existing entry */ | ||
const [db_key_type, db_key_value] = db_key.split(" ").slice(1); |
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 guess this could very easily return the wrong number of arguments...
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.
Yes it could, hence the code below needs to be careful to get along with an undefined
db_key_value. That's why it checks for its truthiness first.
pkg/static/login.js
Outdated
* check if we can match the given fingerprint to an existing entry */ | ||
const [db_key_type, db_key_value] = db_key.split(" ").slice(1); | ||
if (key_value.startsWith('SHA256:') && db_key_value && !db_key_value.includes(':')) { | ||
// cockpit-ssh and ssh use different notation: ED25519 vs. ssh-ed25519 |
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.
⬢[lis@toolbox openssh-portable]$ git grep '\.name =\|\.shortname ='
...
ssh-dss.c: /* .name = */ "ssh-dss",
ssh-dss.c: /* .shortname = */ "DSA",
ssh-dss.c: /* .name = */ "ssh-dss-cert-v01@openssh.com",
ssh-dss.c: /* .shortname = */ "DSA-CERT",
ssh-ecdsa-sk.c: /* .name = */ "sk-ecdsa-sha2-nistp256@openssh.com",
ssh-ecdsa-sk.c: /* .shortname = */ "ECDSA-SK",
ssh-ecdsa-sk.c: /* .name = */ "sk-ecdsa-sha2-nistp256-cert-v01@openssh.com",
ssh-ecdsa-sk.c: /* .shortname = */ "ECDSA-SK-CERT",
ssh-ecdsa-sk.c: /* .name = */ "webauthn-sk-ecdsa-sha2-nistp256@openssh.com",
ssh-ecdsa-sk.c: /* .shortname = */ "ECDSA-SK",
ssh-ecdsa.c: /* .name = */ "ecdsa-sha2-nistp256",
ssh-ecdsa.c: /* .shortname = */ "ECDSA",
ssh-ecdsa.c: /* .name = */ "ecdsa-sha2-nistp256-cert-v01@openssh.com",
ssh-ecdsa.c: /* .shortname = */ "ECDSA-CERT",
ssh-ecdsa.c: /* .name = */ "ecdsa-sha2-nistp384",
ssh-ecdsa.c: /* .shortname = */ "ECDSA",
ssh-ecdsa.c: /* .name = */ "ecdsa-sha2-nistp384-cert-v01@openssh.com",
ssh-ecdsa.c: /* .shortname = */ "ECDSA-CERT",
ssh-ecdsa.c: /* .name = */ "ecdsa-sha2-nistp521",
ssh-ecdsa.c: /* .shortname = */ "ECDSA",
ssh-ecdsa.c: /* .name = */ "ecdsa-sha2-nistp521-cert-v01@openssh.com",
ssh-ecdsa.c: /* .shortname = */ "ECDSA-CERT",
ssh-ed25519-sk.c: /* .name = */ "sk-ssh-ed25519@openssh.com",
ssh-ed25519-sk.c: /* .shortname = */ "ED25519-SK",
ssh-ed25519-sk.c: /* .name = */ "sk-ssh-ed25519-cert-v01@openssh.com",
ssh-ed25519-sk.c: /* .shortname = */ "ED25519-SK-CERT",
ssh-ed25519.c: /* .name = */ "ssh-ed25519",
ssh-ed25519.c: /* .shortname = */ "ED25519",
ssh-ed25519.c: /* .name = */ "ssh-ed25519-cert-v01@openssh.com",
ssh-ed25519.c: /* .shortname = */ "ED25519-CERT",
ssh-rsa.c: /* .name = */ "ssh-rsa",
ssh-rsa.c: /* .shortname = */ "RSA",
ssh-rsa.c: /* .name = */ "ssh-rsa-cert-v01@openssh.com",
ssh-rsa.c: /* .shortname = */ "RSA-CERT",
ssh-rsa.c: /* .name = */ "rsa-sha2-256",
ssh-rsa.c: /* .shortname = */ "RSA",
ssh-rsa.c: /* .name = */ "rsa-sha2-512",
ssh-rsa.c: /* .shortname = */ "RSA",
ssh-rsa.c: /* .name = */ "rsa-sha2-256-cert-v01@openssh.com",
ssh-rsa.c: /* .shortname = */ "RSA-CERT",
ssh-rsa.c: /* .name = */ "rsa-sha2-512-cert-v01@openssh.com",
ssh-rsa.c: /* .shortname = */ "RSA-CERT",
ssh-xmss.c: /* .name = */ "ssh-xmss@openssh.com",
ssh-xmss.c: /* .shortname = */ "XMSS",
ssh-xmss.c: /* .name = */ "ssh-xmss-cert-v01@openssh.com",
ssh-xmss.c: /* .shortname = */ "XMSS-CERT",
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.
Meh.. But I just realized that we don't really actually have to do this comparison. If we assume collision resistance (SHA256 's only job), then we can either be much more lenient about this and do a lower-case substring match, or even just drop it completely -- the hash of a different key type is going to be different. If not, the world has much bigger problems 😰
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 comparison is gone now.
pkg/static/login.js
Outdated
if (key_value.startsWith('SHA256:') && db_key_value && !db_key_value.includes(':')) { | ||
// cockpit-ssh and ssh use different notation: ED25519 vs. ssh-ed25519 | ||
if (key_type.replace('ssh-', '').toLowerCase() == db_key_type.replace('ssh-', '').toLowerCase()) { | ||
// cockpit-ssh and ssh agree no which key type to show to the user; |
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.
agree *on
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 comment is gone now.
pkg/static/login.js
Outdated
if (key_type.replace('ssh-', '').toLowerCase() == db_key_type.replace('ssh-', '').toLowerCase()) { | ||
// cockpit-ssh and ssh agree no which key type to show to the user; | ||
// compute SHA256 fingerprint of reference db key and compare it | ||
const fp_raw = 'SHA256:' + base64_encode(sha256.digest(base64_decode(db_key_value))); |
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 would never have thought that this was so straightforward...
pkg/static/login.js
Outdated
// cockpit-ssh and ssh disagree on which key type to show to the user | ||
console.warn("host key verification: host sent us key type", key_type, | ||
"but our localStore database has type", db_key_type, "; treating as new host"); | ||
db_key = null; |
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 mentioned on IRC why I think this is a bad decision.
If someone wants to attack a user, and they know that this user uses Cockpit and recently went through this upgrade step then they can tune their attack by MITM-ing to a server which presents a key of a different type than the one that the user has in their DB in order that they get a more-innocent looking "new host" dialog instead of the "you are being attacked!!" one. ie: in this scenario it's not a case of cockpit-ssh and ssh disagreeing, it's a case of the attacker having manipulated the scenario.
If we further assume that our attackers will always take advantage of this possibility to elicit a less-scary dialog, then it means that we'll actually only see the super-scary dialog in the case where we're not under attack (ie: key change for innocent reasons).
So ya: I don't like that we give the attacker the trivial ability to downgrade the type of warning.
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 think I misunderstood you. This situation can happen all the time, even with ssh
itself. Over time and different server or libssh versions, the presented key type probably changed a lot. This PR doesn't fundamentally change that. But reading your full comment here I think that you merely want to show the "host key changed" dialog in this situation, which is fair (and I agree).
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.
Changed. This makes the patch much simpler actually, and it's more consistent. Sorry for the misunderstanding!
# we don't have acces to the full key, but the fingerprint is good enough | ||
args['host-key'] = f'{host_match.group(1)} {fp_match.group(1)} {fp_match.group(2)}' | ||
# very oddly named, login.js do_hostkey_verification() expects the fingerprint here | ||
args['default'] = fp_match.group(2) |
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.
It seems a bit weird that we make this the default. Is that something that the current dialog expects/requires?
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.
Yes, see the comment above it. With cockpit-ssh, default
is the fingerprint while host-key
is the full thing.
There is no principal reason for stopping us here. We can do the inverse of what this PR does and check if we already know the fingerprint of the host, before calling ssh with a temporary known_hosts file. (Note that you can't write files from login.js, this needs a ton of work in cockpit-ws to start supporting that -- I don't see us do that anytime soon). |
In order to use cockpit.beiboot as cockpit-ssh replacement from the "normal" (not Client mode) login page, it needs to consider the given username and password. cockpit-ssh sends an initial `authorize` message for that and checks for "Basic" auth. If that fails, it aborts immediately with `authentication-failed`. Implement the same in cockpit.beiboot. Note: The UI does not currently get along with multiple password attempts. Once we drop cockpit-ssh, we should fix the UI and cockpit.beiboot to behave like the flatpak, keep the initial SSH running, and just answer the "try again" prompts. Cover this in a new `TestLogin.testLoginSshBeiboot`. Once we generally replace cockpit-ssh with cockpit.beiboot, this will get absorbed by TestLogin and TestMultiMachine* and can be dropped again.
Stop treating host key prompts as generic conversation messages. We want the UI to handle them properly, with some verbiage/buttons and the recipe for validating host keys, instead of letting the user type "yes". The login page recognizes these through the presence of the `host-key` authorize field (and irritatingly, an extra `default` field with the actual value). We can't use ferny's builtin `do_hostkey()` responder for this, as that requires `ferny.Session(handle_host_key=True)`, and that API is not flexible enough to handle our ssh command modifications and the extra beiboot_helper handler. This needs some bigger redesign, see cockpit-project#19668. So just recognize and parse SSH's host key prompts, and rely on our integration tests to spot breakage in future distro releases. This enables the login page's host key localstorage mechanism, so adjust TestLogin.testLoginSshBeiboot to only expect the host key on the first login attempt.
6a9a3e0
to
ef11fe8
Compare
cockpit-ssh sends us full host keys, which the login page puts into localStore. ferny/cockpit-beiboot can't do that, as there is no portable way to tell ssh(1) to show full host keys. Make this check compatible: If the reference key in the db is a full host key (not including a `:`, which is invalid base64), but the key received from the auth bridge (cockpit-beiboot) is a fingerprint (starting with "SHA256:"), then compute the fingerprint of the reference key and compare it to the received fingerprint.
ef11fe8
to
fc5af4a
Compare
|
ps: I realize that this essentially represents a radical rethinking of the way we handle host keys in the browser localstorage world, and I know you didn't sign up for this, and really want to work on getting the -ws into the container... ...but if it worked, wouldn't it be nice? |
I'm still not a fan. You can't get the full keys into a known_hosts file without actually ack'ing the fingerprint and doing the connection to a machine, so this is would be a two-step process: first attempt tries to connect, fails, presents the FP to the user. Second connection attempt then sends the acked FP to beiboot/ssh, connects, and writes the known_hosts file, which then needs to be sent to the UI after the connection succeeded, and needs to be correlated to the first attempt, and stored. login.js then needs to special-case this for cockpit-beiboot as cockpit-ssh cannot do that. I want to kill cockpit-ssh first, then we have more leeway to change the protocol. Doing another Authorize after success is indeed a big change in the protocol, and I don't have enough of it in my head to say how difficult it will be. But in the end I consider it not worth spending that time and effort on. The only thing that this PR changes its storing full keys → their SHA256 fingerprint. This is cryptographically sane enough. I don't want to block this for yet another year and turn it into another #19668.. (Plus, this PR doesn't actually change anything in the product, it's just the prerequisite) |
This is a very good point and I hadn't considered it. |
In order to use cockpit-beiboot as cockpit-ssh replacement from the "normal" (not Client mode) login page, it needs to consider the given username and password. It also needs to properly handle "unknown host key" prompts.
This paves the way for completely replacing cockpit-ssh with cockpit.beiboot. See https://issues.redhat.com/browse/COCKPIT-954 and #19441.