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

feat: Add support for pure dart interactive shell #604

Merged
merged 19 commits into from
Dec 4, 2023

Conversation

gkc
Copy link
Contributor

@gkc gkc commented Dec 3, 2023

- What we did
feat: Add support for pure dart interactive shell

- How I did it
Feature

  • added startUserSession method to InitialTunnelHandler
  • renamed *InitialTunnelHandler to *SshSessionHandler
  • added userKeyPairIdentifier to SshnpCore constructor
  • substantial refactoring of SshnpDartSshSessionHandler to enable code reuse when implementing startUserSession
  • added bool get canRunShell and Future<SshnpRemoteProcess> runShell() to Sshnp and all subclasses and implemented for the SshnpDartPureImpl
  • make sshnoports/bin/sshnp.dart main call runShell if canRunShell is true, and hook up stdio to the remote shell

Other

  • fixed a few typos
  • improved logging a bit
  • ran dart format

- How to verify it
Manually test - e.g.

dart bin/sshnp.dart -f @client_at_sign -t @daemon_at_sign -d mbp -h @rv_eu \
  -i ~/.ssh/noports -s ~/.ssh/noports.pub \
  -u gary --ssh-client dart -v 

gkc and others added 8 commits December 3, 2023 19:17
- added startUserSession method to InitialTunnelHandler
- renamed *InitialTunnelHandler to *SshSessionHandler
- added userKeyPairIdentifier to SshnpCore constructor
- substantial refactoring of SshnpDartSshSessionHandler to enable
  code reuse when implementing startUserSession
- added `bool get canRunShell` and `Future<SshnpRemoteProcess> runShell()` to Sshnp and all subclasses

TODOS:
- Implement startUserSession in SshnpDartSshSessionHandler
- Hook something up to stdin, stdout and stderr of the SshnpRemoteProcess
- Test test test test test
- Refactor until everything is tidy

Other
- fixed a few typos
- ran dart format
' to localhost:${params.remoteSshdPort} on remote side');

await helper.startForwarding(
fLocalPort: localPort,
Copy link
Member

Choose a reason for hiding this comment

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

I think we will have to rethink how we do this later, ideally we can control whether we forward the sshd port to a physical socket, or forward it via Dart Streams to the user session.

The first is preferred on Desktop, but not possible on mobile, so we will need the option of falling back to the second somehow

Copy link
Member

Choose a reason for hiding this comment

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

I think the current behaviour is fine for the current release though, it shouldn't cause any breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's an implementation detail within the PureDartImpl so should be fine to change; my objective for 4.0.0 is to get the pure dart CLI working in such a way that we can follow with 4.1.0 which works in mobile

XavierChanth and others added 10 commits December 3, 2023 17:48
…-interactive-ssh

feat: pure dart interactive ssh -> gkc
- Added identityKeyPair as mandatory constructor parameter for SshnpDartPureImpl in order to fix bug where public key wasn't being sent to remote daemon because identityKeyPair wasn't set
- Added `done` future to SshnpRemoteProcess so program can listen on it
- Made various logging changes, objective being to ensure that with --verbose, there is a reasonably complete log of what is happening. Previously, sshnpdchannel and sshrvdchannel were not honouring the --verbose
@gkc gkc marked this pull request as ready for review December 4, 2023 14:06
@gkc
Copy link
Contributor Author

gkc commented Dec 4, 2023

@XavierChanth I think we need to make -i mandatory for the openssh implementation as well; when it's not supplied we get ssh -p 52777 gary@localhost as output which means almost certain auth failure if you've got lots of identity files, vs ssh -p 53257 -o StrictHostKeyChecking=accept-new -o IdentitiesOnly=yes gary@localhost -i /Users/gary/.ssh/noports when -i is supplied

wdyt?

@XavierChanth
Copy link
Member

XavierChanth commented Dec 4, 2023

@XavierChanth I think we need to make -i mandatory for the openssh implementation as well; when it's not supplied we get ssh -p 52777 gary@localhost as output which means almost certain auth failure if you've got lots of identity files, vs ssh -p 53257 -o StrictHostKeyChecking=accept-new -o IdentitiesOnly=yes gary@localhost -i /Users/gary/.ssh/noports when -i is supplied

wdyt?

openssh has a list of fallback keys, hence why I didn't enforce it:

from the man pages:

-i identity_file
             Selects a file from which the identity (private key) for public key authentication is read.  You can also
             specify a public key file to use the corresponding private key that is loaded in ssh-agent(1) when the
             private key file is not present locally.  The default is ~/.ssh/id_rsa, ~/.ssh/id_ecdsa,
             ~/.ssh/id_ecdsa_sk, ~/.ssh/id_ed25519, ~/.ssh/id_ed25519_sk and ~/.ssh/id_dsa.  Identity files may also
             be specified on a per-host basis in the configuration file.  It is possible to have multiple -i options
             (and multiple identities specified in configuration files).  If no certificates have been explicitly
             specified by the CertificateFile directive, ssh will also try to load certificate information from the
             filename obtained by appending -cert.pub to identity filenames.
             ```
             
"The default is ~/.ssh/id_rsa, ~/.ssh/id_ecdsa, ~/.ssh/id_ecdsa_sk, ~/.ssh/id_ed25519, ~/.ssh/id_ed25519_sk and ~/.ssh/id_dsa."

@gkc
Copy link
Contributor Author

gkc commented Dec 4, 2023

Ack 👍

@gkc gkc merged commit 043b4d3 into trunk Dec 4, 2023
11 checks passed
@gkc gkc deleted the gkc/pure-dart-interactive-ssh branch December 4, 2023 16:07
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