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.

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

openssh: init module #1172

Merged
merged 1 commit into from
Jan 11, 2025
Merged

openssh: init module #1172

merged 1 commit into from
Jan 11, 2025

Conversation

Enzime
Copy link
Collaborator

@Enzime Enzime commented Nov 16, 2024

No description provided.

@Enzime Enzime force-pushed the push-yxtrnyuxzmny branch 2 times, most recently from 2333041 to 7826fe3 Compare November 23, 2024 02:03
@Enzime Enzime requested a review from emilazy November 24, 2024 03:53
Comment on lines 18 to 17
services.openssh.useAppleServer = lib.mkOption {
type = lib.types.bool;
default = true;
example = false;
description = ''
Whether to use Apple's built-in OpenSSH server.
'';
};
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly I think we can just drop this if it defaults to true, since nobody is ever going to set it to false until/unless it’s implemented, and I guess time has mellowed me into thinking it might be too annoying to force people to explicitly toggle it to use this :)

Comment on lines +36 to +32
# We don't use `systemsetup -setremotelogin` as it requires Full Disk Access
system.activationScripts.launchd.text = lib.mkIf (cfg.enable != null) (if cfg.enable then ''
if [[ "$(systemsetup -getremotelogin | sed 's/Remote Login: //')" == "Off" ]]; then
launchctl enable system/com.openssh.sshd
launchctl bootstrap system /System/Library/LaunchDaemons/ssh.plist
fi
'' else ''
if [[ "$(systemsetup -getremotelogin | sed 's/Remote Login: //')" == "On" ]]; then
launchctl bootout system/com.openssh.sshd
launchctl disable system/com.openssh.sshd
fi
'');
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

The logic for when cfg.enable is false worries me a little, because it seems like you might think your SSH server is off and you’re totally protected, but there’s actually a race condition between boot and the activation scripts running where the server can be accessed?

Do we know if there are any meaningful differences between what this does and what systemsetup -setremotelogin does? It may be better to just require FDA for this module, as it would certainly solve the race condition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently we're adding the text to the launchd activation script, which is not one of the activation scripts that runs on boot (etcChecks, etc and keyboard) (Unrelated tangent: we'll probably want users.users.launchd.agents to allow us to do user activation when they log in as it might have interactive prompts)

When you bootout and disable the SSH server, it won't get reenabled on reboot, I've tested this on a VM by manually going into the Sharing menu, enabling Remote Login, doing a darwin-rebuild switch with services.openssh.enable = false;, rebooting and checking that Remote Login is still disabled

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, so it actually toggles the visible preference in System Settings when you do this? That’s good enough for me then. Strange that they have a back door like this, though; I wouldn’t be surprised if we have to move to the proper command in future.

@Enzime Enzime force-pushed the push-yxtrnyuxzmny branch from 7826fe3 to be4c1b8 Compare January 9, 2025 23:24
Comment on lines +36 to +32
# We don't use `systemsetup -setremotelogin` as it requires Full Disk Access
system.activationScripts.launchd.text = lib.mkIf (cfg.enable != null) (if cfg.enable then ''
if [[ "$(systemsetup -getremotelogin | sed 's/Remote Login: //')" == "Off" ]]; then
launchctl enable system/com.openssh.sshd
launchctl bootstrap system /System/Library/LaunchDaemons/ssh.plist
fi
'' else ''
if [[ "$(systemsetup -getremotelogin | sed 's/Remote Login: //')" == "On" ]]; then
launchctl bootout system/com.openssh.sshd
launchctl disable system/com.openssh.sshd
fi
'');
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, so it actually toggles the visible preference in System Settings when you do this? That’s good enough for me then. Strange that they have a back door like this, though; I wouldn’t be surprised if we have to move to the proper command in future.

@Enzime Enzime merged commit 6ace2f2 into LnL7:master Jan 11, 2025
6 checks passed
@Enzime Enzime deleted the push-yxtrnyuxzmny branch January 11, 2025 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants