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

known_hosts: Don't update a key to an empty IP address #1332

Closed
wants to merge 1 commit into from

Conversation

@jhillyerd
Copy link

jhillyerd commented Apr 29, 2020

Remove keys that have an empty IP address instead of trying to re-add
them during the update phase.

Should resolve error seen in
nix-community/nixops-vbox#3 (comment)

Remove keys that have an empty IP address instead of trying to re-add
them during the update phase.

Should resolve error seen in
nix-community/nixops-vbox#3 (comment)
@jhillyerd
Copy link
Author

jhillyerd commented Apr 29, 2020

I tested this on nixops-vbox with tests/functional/single_machine_vbox_base.nix as well as a dual machine network of my own with create, deploy, stop (and no stop), destroy, and delete. No issues encountered.

@@ -66,4 +66,5 @@ def update(prev_address: str, new_address: str, public_host_key: str) -> None:
# FIXME: this rewrites known_hosts twice.
if prev_address != new_address:
remove(prev_address, public_host_key)
add(new_address, public_host_key)
if new_address is not None:

This comment has been minimized.

Copy link
@domenkozar

domenkozar May 4, 2020

Member

shouldn't remove also be conditioned to this check?

This comment has been minimized.

Copy link
@jhillyerd

jhillyerd May 4, 2020

Author

Possibly. I don't know a good way to test changes do this code, so I was not comfortable going beyond a break-fix.

My guess is that none of these were meant to be called with empty IP addresses, and that the Virtualbox plugin is broken somewhere.

@grahamc grahamc added this to To do in kanban May 5, 2020
@grahamc grahamc moved this from To do to In progress in kanban May 5, 2020
@grahamc
Copy link
Member

grahamc commented May 5, 2020

I think the better option is to delete the update function, and make users call remove and/or add as appropriate. Note this semi-related issue: #1279 and with that, I'm inclined to close this PR, unless you wanted to do that here. What do y'all think?

@grahamc grahamc moved this from In progress to To do in kanban May 5, 2020
@jhillyerd
Copy link
Author

jhillyerd commented May 5, 2020

Good to close. 👍

@jhillyerd jhillyerd closed this May 5, 2020
kanban automation moved this from To do to Done May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
kanban
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.