Fix systemd-ssh changes#390565
Conversation
In afeb76d, sshd.service and sshd@.service were switched to Type=notify. This apparently works for sshd.service, but not for sshd@.service. Given that the reason for this working with sshd.service isn't exactly clear, let's revert it for both of them for now, and revisit Type=notify later.
ElvishJerricco
requested review from
NyCodeGHG,
emilazy,
mweinelt and
philiptaron
github-actions
bot
added
6.topic: nixos
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
Definitely want the fix for the notify business if that's not supported by the openssh service.
The test changes to make the test do less and test less because of Hydra and specific present limitations makes me sad. Isn't there a way to mark something as not to be run in Hydra? I'd rather rely on those with beefy machines to test it out rather than just missing the coverage, especially when the test was as clear (in my view) as @NyCodeGHG's test was.
| environment.LD_LIBRARY_PATH = nssModulesPath; | ||
|
|
||
| serviceConfig = { | ||
| Type = "notify"; |
There was a problem hiding this comment.
Pretty sure this was my curiosity leading @NyCodeGHG astray. Sorry!
@philiptaron Even if we did that, we still shouldn't be using an ISO for it. That's a really heavyweight way to start a VM. I don't think we can afford to resolve this right now. If someone wants to write a better test in the future, they may. FWIW, we're not just testing less here. We're also testing more, because we weren't testing the container path before. |
| restartTriggers = [ config.environment.etc."ssh/sshd_config".source ]; | ||
|
|
||
| serviceConfig = { | ||
| Type = "notify"; |
There was a problem hiding this comment.
I'm pretty sure this should work fine, but it's also fine for me to re-introduce this later.
There was a problem hiding this comment.
@NyCodeGHG Yea, I'm pretty sure you're right that non-socket-activated sshd.service now supports sd_notify, which it didn't in the past. But I want to introduce that improvement in a separate PR where we can more seriously test and check it.
3 participants
#372979 Introduced a few major problems.
sshd@.service, because every SSH connection that closed resulted in its corresponding systemd unit entering a failed state. Not fatal to functionality, but makes the system considered degraded. There may have been an avenue for a DoS attack, though we didn't get that far examining it.This no longer tests systemd-ssh's
AF_VSOCKfunctionality. Unfortunately, without nested virtualisation, I'm not sure this is possible. In theory, you could use the host's vsock space, but/dev/vhost-vsockis not available in the sandbox, and you have to hard code a cid for the guest which will clash with others. I think you really do need nested virtualisation, unless qemu has a way to create a unix socket on the host that presents as vsock to the guest.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.