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

Knownhosts tilde aware path expansion - fixes #1107 #1115

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

memetb
Copy link
Contributor

@memetb memetb commented Oct 23, 2024

This directly addresses issue #1107 but is also a small refactor effort to move some utility code into the util package with associated unit tests.

The specific changes are to move the file path expansion code to a dedicated utility function in the util package and add an associated unit test.

As a note, issue #1107 is legacy to v0.8.x and existed before here.

if doVerify {
		cb, err := knownhosts.New(os.ExpandEnv(knownHostsPath)) // <- this does not expand tilde
		if err != nil {
			return nil, fmt.Errorf("failed to read ssh known hosts: %w", err)
		}
		hostKeyCallback = cb
	}

previously this code would hand back the path as specified by the caller if it
failed (a path which can be a user specified string). However, this would mean
that the returned entity would be a string or a filepath depending on the
codepath. This behaviour previously existed in the codebase, however it is not
clear if it caused any issues anywhere in production. This PR, including the
associated unit test shows that under at least some circumstances (github ci)
the outcomes are unexpected.
@dmacvicar
Copy link
Owner

I won't call this monkey patching. It is just a drop-in replacement function.

The part I think we can improve is the naming. We are replacing os.ExpandEnv, which expands generic string, for something that is specialized into paths, so why not just ExpandPath ?

Even its argument is called path.

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.

2 participants