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

Allow running multiple SSH daemons #83

Merged
merged 2 commits into from
Jul 5, 2022
Merged

Allow running multiple SSH daemons #83

merged 2 commits into from
Jul 5, 2022

Conversation

SteffenDE
Copy link
Contributor

Hello there,

while creating nerves_ssh_shell to connect to regular shells on nerves devices, I found the need to start multiple ssh daemons (as ssh subsystems do not allow the same set of features than running a separate daemon). As I also did not want to reimplement what nerves_ssh is already doing very well, I thought that adding the possibility to run multiple daemons seems like something that might benefit others too.

This is not complete yet, for example I only changed the tests to pass for now and did not add new tests for starting multiple daemons, as I first of all wanted some feedback what you all think about this. I also added a configuration option for the used ssh_cli setting, but this might not be needed as there is already the daemon_option_overrides.

@fhunleth
Copy link
Member

Hi @SteffenDE!

Thanks for writing about this feature. I haven't looked through your changes yet, but let me give you some quick feedback since I'm not sure when I'll get back to you on details.

  1. I think that supporting Unix shells can be useful and is totally worth supporting.
  2. Being able to run multiple ssh daemons is something that I've wanted, but never gotten the chance to implement, so thanks for getting that started.
  3. Supporting a custom CLI hook seems useful too.
  4. The Unix shell support that you did doesn't look like all that much code, right? What do you think about working towards getting that integrated into :nerves_ssh so that it's available by default to Nerves users? I'm thinking that our instructions would be to copy/paste this configuration fragment and you'd get a basic sh shell.

In any case, I want to be supportive of this direction even if I don't have time this week to really put a lot of thought into it. @jjcarstens - do you have other feelings or concerns?

@SteffenDE
Copy link
Contributor Author

Yes, I think that supporting Unix shells in nerves_ssh would be very cool and I'd be very happy to work on this being available by default.

For this pull request, I think the focus should still be making running multiple daemons possible first, as this is the basis for supporting other shells running next to IEx when not using subsystems (that have some limitations as documented in the :nerves_ssh_shell README).

I think it probably makes sense to continue the discussion around unix shell support, so I've opened #84 for that.

Copy link
Member

@jjcarstens jjcarstens left a comment

Choose a reason for hiding this comment

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

Just a couple comments, but otherwise seems good to me

lib/nerves_ssh/options.ex Outdated Show resolved Hide resolved
lib/nerves_ssh.ex Outdated Show resolved Hide resolved
@SteffenDE
Copy link
Contributor Author

So, I just added one test for actually starting two daemons and found an issue with NervesSSH.UserPasswords and NervesSSH.Keys needing access to the daemon name. I changed the code accordingly and passed the registered name to the NervesSSH.Options struct.

lib/nerves_ssh.ex Outdated Show resolved Hide resolved
lib/nerves_ssh.ex Outdated Show resolved Hide resolved
@SteffenDE SteffenDE marked this pull request as ready for review June 29, 2022 16:52
Copy link
Member

@fhunleth fhunleth left a comment

Choose a reason for hiding this comment

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

LGTM!

@jjcarstens jjcarstens merged commit cce8d41 into nerves-project:main Jul 5, 2022
@SteffenDE SteffenDE deleted the cli branch July 9, 2022 12:39
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.

3 participants