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

Stop adding GitHub SSH keys #171

Merged
merged 1 commit into from Mar 24, 2023
Merged

Stop adding GitHub SSH keys #171

merged 1 commit into from Mar 24, 2023

Conversation

mpdude
Copy link
Member

@mpdude mpdude commented Mar 24, 2023

We need to fix the SSH keys shipped with this action: https://github.blog/2023-03-23-we-updated-our-rsa-ssh-host-key/

But, we have another issue (#106) with regards to host keys: On self-hosted runners which are not ephemeral the known_host file fills up with repeated entries, because every action run adds a new line with the same host keys.

Also, on those machines, the old key will still be in the known_hosts file.

IMHO this action should not be responsible for shipping SSH host keys, that's too much responsibility.

This section in the code is a leftover from early days when GitHub provided runners did not include SSH keys at all. For a long time already, GH takes care of placing their SSH keys in their runner images.

For self-hosted runners, those people setting up the runner should fetch and verify SSH keys themselves and put it into the known_hosts file.

I know this is a breaking change and is going to annoy users. But on the other hand, there is no better opportunity to drop this feature than with an emergency-style key revocation as today.

Closes #106, closes #129, closes #169, closes #170, closes #172.

@mpdude mpdude merged commit d4b9b8f into master Mar 24, 2023
4 of 8 checks passed
@mpdude mpdude deleted the no-host-keys branch March 24, 2023 11:15
@na-jakobs
Copy link

@mpdude This will break alot of workflows. Why not fetch the keys from curl -L https://api.github.com/meta | jq -r '.ssh_keys' ?

@mpdude
Copy link
Member Author

mpdude commented Mar 24, 2023

😭

  • GH hosted runners should be fine, they ship with SSH keys maintained by GitHub
  • Self hosted runners may be a problem, but if they used this action before, they still have the (old!) key in their known_hosts file. Admins will probably have to fix things manually anyway?
  • For new self-hosted runners, people should be responsible for setting up keys themselves.

Am I missing anything?

@mpdude
Copy link
Member Author

mpdude commented Mar 24, 2023

@na-jakobs
Copy link

@mpdude No you are right, this should work. Containerized workflows should just pick up the host keys and accept them. And admins should remove everything from known_hosts on self-hosted. I will update my actions to v0.8.0.

@faahim
Copy link

faahim commented Apr 6, 2023

Sorry if it's a stupid question, but for GitHub-hosted action, do we now need to add GitHub's keys in the know_hosts file? We're currently getting the WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED! in our workflow.

@sebastiankugler
Copy link
Member

@faahim It may be a stupid answer because I'm not familiar with GitHub Actions, but I'd think you'd rather need to remove old and outdated entries from known_hosts than adding new ones (the reason for the warning being that the old entries in known_hosts do not match the current keys). This is how it works locally at least; from what I read above though, editing known_hosts should only be necessary for self-hosted runners.

@tbehling
Copy link

See #174 (comment) for a good way to fetch the GitHub SSH host keys from their API. Fetching over SSH is more secure than trusting ssh-keyscan, for example.

benzado added a commit to benzado/ssh-agent that referenced this pull request Feb 4, 2024
Since webfactory#171 was merged, this action no longer touches `known_hosts`; this line should have been removed from the README at that time.
mpdude pushed a commit that referenced this pull request Feb 5, 2024
Since #171 was merged, this action no longer touches `known_hosts`; this
line should have been removed from the README at that time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants