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

SSH: Set UserKnownHostsFile to /dev/null #165

Merged
merged 2 commits into from
Aug 20, 2021

Conversation

mook-as
Copy link
Contributor

@mook-as mook-as commented Aug 20, 2021

As we are dealing with temporary SSH host keys, we should not pollute the users's known hosts file with ephemeral data. Additionally, this avoids issues where the user has an invalid known hosts file, as in rancher-sandbox/rancher-desktop#504.

As we are dealing with temporary SSH host keys, we should not pollute the
users's known hosts file with ephermeral data.  Additionally, this avoids
issues where the user has an invalid known hosts file, as in
rancher-sandbox/rancher-desktop#504.

Signed-off-by: Mark Yen <[email protected]>
@@ -155,6 +155,7 @@ func CommonArgs(useDotSSH bool) ([]string, error) {

args = append(args,
"-o", "StrictHostKeyChecking=no",
"-o", "UserKnownHostsFile=/dev/null",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we now want to remove RemoveKnownHostEntries() in L101?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we now want to remove RemoveKnownHostEntries() in L101?

Yes, that function should not be needed anymore if we don't use a known hosts file.

@mook-as can you update the PR to include removal of RemoveKnownHostEntries() (and the call location, of course)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that! Fixed in 08d4bc3

@AkihiroSuda AkihiroSuda added this to the v0.6.1 milestone Aug 20, 2021
@AkihiroSuda AkihiroSuda requested a review from jandubois August 20, 2021 16:56
We no longer attempt to touch the known_hosts file.

Signed-off-by: Mark Yen <[email protected]>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM if CI green

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jandubois jandubois merged commit b499494 into lima-vm:master Aug 20, 2021
@mook-as mook-as deleted the ssh-no-known-hosts branch August 25, 2021 20:55
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