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

pyinfra/connectors: Fix overwriting of users known_hosts file (#1209) #1248

Closed
wants to merge 5 commits into from

Conversation

evoldstad
Copy link
Contributor

This fixes issue #1209, making it so that we append new keys to the users known_hosts file instead of overwriting it.

I also added a testcase that should discover this breaking in the future.

Additionally:

  • Broke out the append functionality into a "append_hostkey" function, making it so we don't needlessly reuse code for the AskPolicy and AcceptNewPolicy.

Previous behaviour when adding a new key:

  • Create a paramiko.HostKeys object
  • Read the users known_hosts file
  • Add the new key to the object
  • Save the object, overwriting the users host_keys file.

New behaviour:

  • Create a paramiko.HostKeyEntry object using the new hostname and corresponding key.
  • Append this key to the existing known_hosts file.

Instead of relying on paramiko to correctly parse the full
hostfile, this fix makes it so that we only append new keys to
the hostfile rather than overwriting it completely.

The old behaviour results in unwanted alterations to the hostfile,
such as hostfile entries not parseable by paramiko disappearing
without users consent, and comments being removed.

I also split out the logic for appending hostkeys into a new function
instead of having the code be unnecessarily repeated in multiple places.
This makes the MissingHostKeyPolicy children a bit more readable IMO.
@evoldstad
Copy link
Contributor Author

Forgot to do linting and squashing commits, will redo this.

@evoldstad evoldstad closed this Nov 27, 2024
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.

1 participant