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

add option to enable agent forwarding #187

Merged
merged 3 commits into from
Apr 7, 2024

Conversation

NikitaVasiliev
Copy link

This pull request introduces the forward-ssh-agent option, allowing users to leverage SSH agent forwarding when executing commands on remote hosts. This facilitates secure authentication by forwarding connections from a local authentication agent, such as ssh-agent, to the remote host. It’s particularly useful in environments where direct SSH access needs to be secured and simplified without exposing private key material.

If it isn't needed or doesn't fit the project's direction, feel free to decline this PR.

@umputun
Copy link
Owner

umputun commented Mar 25, 2024

Thank you. Here are a few minor corrections:

  • The linter failed on several trivial warnings.
  • The test code for runSSHAgent does not seem to wait for the completion of both goroutines it created. I believe that sock.Close() will cause them to exit, but it may leave the goroutine running after the test is completed. Perhaps using a simple waitgroup with a wait after the Close will solve this issue.

@NikitaVasiliev NikitaVasiliev force-pushed the add_option_forward_agent branch 2 times, most recently from 550bc61 to cbd03b7 Compare March 25, 2024 17:22
@NikitaVasiliev
Copy link
Author

There was indeed a problem. The simple use of a WaitGroup led to a deadlock because some goroutines were stuck at connections. I had to close those connections as well.

@NikitaVasiliev NikitaVasiliev force-pushed the add_option_forward_agent branch 2 times, most recently from 8f7789a to 3ce497b Compare March 25, 2024 17:54
@coveralls
Copy link

coveralls commented Mar 30, 2024

Pull Request Test Coverage Report for Build 8591076724

Details

  • 9 of 34 (26.47%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 84.001%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/executor/connector.go 6 31 19.35%
Totals Coverage Status
Change from base Build 8565636282: -0.6%
Covered Lines: 2704
Relevant Lines: 3219

💛 - Coveralls

@umputun
Copy link
Owner

umputun commented Mar 30, 2024

A few problems:

  • Test_sshAgentForwarding fails on my mac:
    main_test.go:448: 
        	Error Trace:	/Users/umputun/dev.umputun/spot/cmd/spot/main_test.go:391
        	            				/Users/umputun/dev.umputun/spot/cmd/spot/main_test.go:448
        	Error:      	Received unexpected error:
        	            	listen unix : bind: invalid argument
        	Test:       	Test_sshAgentForwarding
  • The test doesn't check much, and because this is the only test related to the connector change, it would be nice to make sure that the output of the command actually matches the expectation.

@NikitaVasiliev
Copy link
Author

NikitaVasiliev commented Mar 31, 2024

  • The first problem is due to relying on undefined behaviour when provide empty address for the unix network to Listen function in Linux. This should have been fixed new.
  • Now the test checks that the forwarded ssh-agent has the key with an expected fingerprint.

@umputun
Copy link
Owner

umputun commented Apr 7, 2024

LGTM, thx

@umputun umputun merged commit 8b3825a into umputun:master Apr 7, 2024
2 checks passed
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