beiboot preparation fixes #19413
beiboot preparation fixes #19413
Conversation
|
My take: this could land now. There are some very serious issues to fix for the handling of cockpit-askpass in the flatpak vs. outside of it, but the code here is clearly working (and we understand why). I'm happy to clean this up further in the context of #19264, and in any case, I'd like to clear the way for you to continue working on the container build... |
…n text Also fix an eslint complaint. We don't run eslint on that directory, but vim does.
martinpitt
added
the
no-test
martinpitt
force-pushed
the
beiboot-prep
branch
from
8b3e6ce
to
345785d
Compare
|
I added an ssh flatpak integration test, there were too many "accidents" involved in dodging that bullet. 😅 I tested it locally, now giving it a spin on GH while I test my reworked beiboot against #19401 locally. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Exercise the beiboot → SSH code path with actual interaction (user and
password).
Only run the test when giving `$COCKPIT_TEST_SSH_{HOST,PASS}`. That way,
developers can run the test locally against our usual test VM:
COCKPIT_TEST_SSH_HOST=admin@127.0.0.2:2201 COCKPIT_TEST_SSH_PASS=foobar
Create a user in the GitHub VM.
This is quite literally what it is defined to do. This races with the CDP driver finishing the command, so sometimes it would fail the test on throwing that RuntimeError.
Knowing what to change in the policy is often not quite obvious, and takes several attempts. Show how to do this much more quickly than repeated rpm/image builds.
This is a prerequisite for switching the login page from cockpit-ssh to the cockpit-beiboot implementation, which runs ssh(1) directly.
On Fedora 39 we get this rejection:
avc: denied { read } for comm="cockpit-askpass" name="possible" dev="sysfs"
scontext=system_u:system_r:cockpit_ws_t:s0 tcontext=system_u:object_r:sysfs_t:s0
This also happens for "cockpit-beiboot". Our code doesn't directly
access sysfs (at least not /devices/system/cpu/possible), but glibc
does, and Python somehow inherits this.
There is no harm in letting it read sysfs, and it may even be necessary
for hardware based authentication schemas, so allow it.
Commit ba93304 dropped the executable wrapper around beiboot.py, which broke `COCKPIT_DEBUG=cockpit.beiboot`. Hardcode the name to bring it back.
martinpitt
force-pushed
the
beiboot-prep
branch
from
345785d
to
e897c2c
Compare
martinpitt
removed
the
no-test
We need and want ensure_ferny_askpass() *only* for the flatpak case. Add some comments about that, as we already forgot it once. When running it from distro packages or the cockpit/ws container, we always want to use the packaged cockpit-askpass helper. Re-use `patch_libexecdir()` for that, as it already has all the necessary magic. Turn that into a proper global function. This fixes running cockpit-beiboot as cockpit-wsinstance system user, as there is no existing/writable home directory, and trying to mkdir ~/.cache EPERMs.
martinpitt
force-pushed
the
beiboot-prep
branch
from
e897c2c
to
33154dd
Compare
martinpitt
added
the
.github-changes
|
Lol - fedora-39 BrokenPipeError that's what #19414 fixes. Retrying. |
| else: | ||
| # outside of the flatpak we expect cockpit-ws and thus an installed helper | ||
| askpass = patch_libexecdir('${libexecdir}/cockpit-askpass') | ||
| assert isinstance(askpass, str) |
There was a problem hiding this comment.
This assert shouldn't be necessary.
Take a look at the definition of the J typevar:
J = TypeVar('J', JsonObject, JsonDocument)I feel like if you fully-expanded JsonDocument you'd get the correct result. So like
J = TypeVar('J', str, float, JsonObject, JsonList, ...etc...)
...but probably not JsonDocument.
and if that works then probably remove the comment above it above it being a HACK. I have a bit of a better understanding of what's going on here than I did when I first wrote this.
There was a problem hiding this comment.
If the above fails, then just adding str to the front of the list ought to suffice, but then leave the comment (or adjust it). :)
There was a problem hiding this comment.
I tried with
J = TypeVar('J', str, float, bool, JsonObject, JsonList, None)and
J = TypeVar('J', JsonLiteral, JsonObject, JsonList)and some more variations, but it's never happy about that. I also tried your second suggestion
J = TypeVar('J', str, JsonObject, JsonDocument)and while that makes this assert go away, it breaks this in merge_patch():
return result # E: Incompatible return value type (got "dict[str, JsonObject | JsonList | str | float | bool | None]", expected "str") [return-value]I need to leave in 10 mins for the long weekend -- if you have a better idea, feel free to force-push. Thanks!
There was a problem hiding this comment.
This seems to be a limitation in mypy. Let's leave well enough alone for now.
allisonkarlitskaya
merged commit 214cdb2
into
cockpit-project:main
These are the relatively harmless parts from #19401, split out to get them out of the way.