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: make sshnpd execute ssh command via Process.start #278

Merged
merged 22 commits into from Jul 25, 2023

Conversation

gkc
Copy link
Contributor

@gkc gkc commented Jul 23, 2023

- What I did

  • feat: For perf reasons, by default we will have SSHNPDImpl directly execute the ssh command on the host rather than using SSHClient
    • writes privatekey which it received earlier from sshnp into a tmp file which is later deleted
    • added a bunch of comments explaining how the command is constructed and why
  • refactor: remove _sshPublicKey instance variable from SSHNPDImpl and make it a local variable in the case 'sshpublickey' in the notification handler
  • feat: Direct AtSignLogger log messages to stdErr for sshnpd, sshnp and sshrvd via AtSignLogger.defaultLoggingHandler = AtSignLogger.stdErrLoggingHandler;
  • feat: Added command-line option --ssh-client which can currently have values '/usr/bin/ssh' (the default) or 'pure-dart'. If set to pure-dart then we'll do it using SSHClient as we used to.
  • feat: added more command-line options to the ssh command which sshnp generates and prints to stdout
    • -o StrictHostKeyChecking=accept-new to silently accept new, but still check existing
    • -o IdentitiesOnly=yes to use only the identity in the specified identity file

gkc added 2 commits July 23, 2023 15:01
…d sshrvd via `AtSignLogger.defaultLoggingHandler = AtSignLogger.stdErrLoggingHandler;`
…and on the host rather than using SSHClient

- writes privatekey which it received earlier from sshnp into a tmp file which is later deleted
- added a bunch of comments explaining how the command is constructed
refactor: remove _sshPublicKey instance variable from SSHNPDImpl and make it a local variable in the `case 'sshpublickey'` in the notification handler
@gkc gkc requested a review from cconstab July 23, 2023 15:03
@gkc
Copy link
Contributor Author

gkc commented Jul 23, 2023

@cconstab My local manual tests worked but the e2e tests are failing with 'Host key verification failed', because of something I'm getting wrong. Please take a look at how I'm constructing the ssh command - a fresh pair of eyes might see what the problem is straight away.

@gkc
Copy link
Contributor Author

gkc commented Jul 23, 2023

I believe the tests are failing because SSHClient does not do host key verification, but ssh command by default does

TerminalStudio/dartssh2#51

Fix would be to add this to the ssh command args

-o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null

@gkc
Copy link
Contributor Author

gkc commented Jul 23, 2023

Applied the fix mentioned in the previous comment, and now only one of the e2e tests is failing, but it's failing in a different way.

130 sshnpd  | SHOUT|2023-07-23 18:19:51.546987| sshnpd |197de568-697a-4851-a789-9e82a722e5cb | Non-zero exit code from /usr/bin/ssh atsign@10.1.0.38 -p 40749 -t -t -i /tmp/.pem-436bdbbb-5052-4cb8-bd16-80ab0f86c376 -R 34925:localhost:22 -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null sleep 15 
131 sshnpd  | SHOUT|2023-07-23 18:19:51.548265| sshnpd |197de568-697a-4851-a789-9e82a722e5cb | stdout   :  
132 sshnpd  | SHOUT|2023-07-23 18:19:51.548293| sshnpd |197de568-697a-4851-a789-9e82a722e5cb | stderr   : Warning: Permanently added '[10.1.0.38]:40749' (ED25519) to the list of known hosts.
133 sshnpd  | Load key "/tmp/.pem-436bdbbb-5052-4cb8-bd16-80ab0f86c376": error in libcrypto
134 sshnpd  | atsign@10.1.0.38: Permission denied (publickey).

I think the repeat failure of the local-to-local test is an artefact of the test rig

All of the other tests which are running sshnpd from the branch (local) are passing (i.e. trunk to local, latest release to local, 3.2.0 to local are all passing)

In this run, only the first test (local to local) failed: https://github.com/atsign-foundation/sshnoports/actions/runs/5637947000 with the Permission denied (publickey) problem

But in this run, both the local to local test failed, AND the v3.2.0 to local test failed for the same (Permission denied (publickey)) reason https://github.com/atsign-foundation/sshnoports/actions/runs/5637996683/attempts/1
But then it passed in the re-run https://github.com/atsign-foundation/sshnoports/actions/runs/5637996683

The local-to-local test will not pass no matter how many times I re-run it

@JeremyTubongbanua Can you think of anything particularly special about the local-local test with regards to timings, setup, etc?

@JeremyTubongbanua
Copy link
Member

Applied the fix mentioned in the previous comment, and now only one of the e2e tests is failing, but it's failing in a different way.

130 sshnpd  | SHOUT|2023-07-23 18:19:51.546987| sshnpd |197de568-697a-4851-a789-9e82a722e5cb | Non-zero exit code from /usr/bin/ssh atsign@10.1.0.38 -p 40749 -t -t -i /tmp/.pem-436bdbbb-5052-4cb8-bd16-80ab0f86c376 -R 34925:localhost:22 -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null sleep 15 
131 sshnpd  | SHOUT|2023-07-23 18:19:51.548265| sshnpd |197de568-697a-4851-a789-9e82a722e5cb | stdout   :  
132 sshnpd  | SHOUT|2023-07-23 18:19:51.548293| sshnpd |197de568-697a-4851-a789-9e82a722e5cb | stderr   : Warning: Permanently added '[10.1.0.38]:40749' (ED25519) to the list of known hosts.
133 sshnpd  | Load key "/tmp/.pem-436bdbbb-5052-4cb8-bd16-80ab0f86c376": error in libcrypto
134 sshnpd  | atsign@10.1.0.38: Permission denied (publickey).

I think the repeat failure of the local-to-local test is an artefact of the test rig

All of the other tests which are running sshnpd from the branch (local) are passing (i.e. trunk to local, latest release to local, 3.2.0 to local are all passing)

In this run, only the first test (local to local) failed: https://github.com/atsign-foundation/sshnoports/actions/runs/5637947000 with the Permission denied (publickey) problem

But in this run, both the local to local test failed, AND the v3.2.0 to local test failed for the same (Permission denied (publickey)) reason https://github.com/atsign-foundation/sshnoports/actions/runs/5637996683/attempts/1 But then it passed in the re-run https://github.com/atsign-foundation/sshnoports/actions/runs/5637996683

The local-to-local test will not pass no matter how many times I re-run it

@JeremyTubongbanua Can you think of anything particularly special about the local-local test with regards to timings, setup, etc?

Yes, @XavierChanth recently refactored the tests and I believe the bug is addressed in a comment in this ticket - #276

@JeremyTubongbanua
Copy link
Member

Fix should be in #280

@gkc gkc marked this pull request as ready for review July 24, 2023 06:42
@gkc
Copy link
Contributor Author

gkc commented Jul 24, 2023

@JeremyTubongbanua thanks - the problem I was encountering was actually a different problem (Permission denied (publickey)) which @cconstab diagnosed here

@JeremyTubongbanua
Copy link
Member

@JeremyTubongbanua thanks - the problem I was encountering was actually a different problem (Permission denied (publickey)) which @cconstab diagnosed here

Awesome :)

@gkc gkc changed the title feat: make sshnpd execute ssh command via Process.run feat: make sshnpd execute ssh command via Process.start Jul 25, 2023
gkc added 2 commits July 25, 2023 14:17
…Impl classes, and their factories / constructors

feat: implemented feature for new command-line flag --ssh-client so that it will use either the host's ssh command OR the pure dart SSHClient from dartssh2 package
build: re-added dartssh2 and ssh-key dependencies
refactor: made the creation of the args easier to read and not possible to break by accidentally omitting a comma
style: ran dart format
@gkc
Copy link
Contributor Author

gkc commented Jul 25, 2023

@cconstab Ready for final review

@gkc gkc merged commit 6731cc4 into trunk Jul 25, 2023
13 checks passed
@gkc gkc deleted the gkc-exec-ssh-on-host branch July 25, 2023 21:02
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.

None yet

3 participants