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

Improve handling of ssh-agent process #87

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Sn0wCrack
Copy link

@Sn0wCrack Sn0wCrack commented Sep 19, 2024

Fixes #22

Currently I have the following added to all of my workflow files using the deployerphp action for our self hosted runners:

- name: Clean Up SSH Agent
  if: always()
  run: |
    killall ssh-agent
    rm ~/.ssh/config

This works fine, however recently we had started having multiple runner instances on each of our runner servers and conflicts began to arise with my solution due to the usage of killall

While I could have improved killing off the ssh-agent, I've decided to take a crack at implement a way of handling the ssh-agent process a little more nicely within the action iteself and have ended up with this.

There is probably a slightly better way of doing this via a proper setting, however I was aiming for backwards compatibility where possible.


What this PR does:

  1. Spawns the SSH agent without specify a socket path, ensuring a unique socket path for each runner.
  2. Adds a new option to toggle if the action should disable strict host checking if no known hosts are provided.
  3. Adds a cleanup task that ensures all SSH keys are removed from the SSH Agent.
  4. Ensures the SSH is shutdown on the runner server.

Why make these changes:

  1. Improves supports for self hosted runners while also adding a bit of extra security for those using GitHub hosted runners.
  2. Allows for the action to still handle the SSH related tasks for the end user without actually touching anything SSH related if necessary.
  3. Probably doing things "the right way", so to speak, in that we're cleaning up after ourselves as part of the action.

essentially attempt to spawn ssh agent independently and kill it when the action is over
doesn't seem like eval persists the environment variables
chore: add comment explaining why we regex output
@Sn0wCrack Sn0wCrack changed the title Improve hanlding of ssh-agent process Improve handling of ssh-agent process Sep 19, 2024
@antonmedv
Copy link
Member

Thanks for PR. Let's merge it but with little bit changes.

@Sn0wCrack
Copy link
Author

Thanks for PR. Let's merge it but with little bit changes.

Thanks for the consideration, what changes did you have in mind here?

@antonmedv
Copy link
Member

Different naming and default. Not running ssh=agent instead of killing.

@Sn0wCrack
Copy link
Author

Different naming and default.

Do you have any suggestions for these?

Not running ssh=agent instead of killing.

The main reason I'm still running ssh-agent in my scenario is because deployerphp itself cannot read in a private key from a string and requires it to be a file on the system.
Instead of attempting to create the key file and have to clean that up I found it easier to just continue to use the ssh-agent process and remove all the keys from the ssh-agent process and kill it.

I could put the "cleanup" step behind an additional flag so it only kills off the ssh-agent process if you pass through another option telling it to.

I should note killing the ssh-agent process does cleanup the socket file as well.

@Sn0wCrack
Copy link
Author

I'm just circling back to this PR after a bit, is this still something you'd be interesting in merging @antonmedv?

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.

Error: Command failed with exit code 1: ssh-agent
2 participants