search

skeema / knownhosts

Go SSH known_hosts wrapper with host key lookup
Apache License 2.0
32 stars 4 forks source link

ipv6: missing port in address #1

Closed lonnywong closed 1 year ago

lonnywong commented 1 year ago

Using WriteKnownHost will get an entry with ipv6:

mbp.local,[fe80::abc:abcd:abcd:abcd%en0] ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyN...

The next login fails:

knownhosts: /Users/lonny/.ssh/known_hosts:26: address [fe80::abc:abcd:abcd:abcd%en0]: missing port in address
evanelias commented 1 year ago

Thank you for the issue report! After looking into this a bit, I suspect the root cause is actually https://github.com/golang/go/issues/53463: in golang.org/x/crypto/ssh/knownhosts, the Normalize function doesn't correctly handle ipv6 addresses that use the standard port 22.

The WriteKnownHost function here in https://github.com/skeema/knownhosts currently relies on the value returned by that golang.org/x/crypto/ssh/knownhosts Normalize function as-is, but we could adjust this to work around https://github.com/golang/go/issues/53463. I think it would just require a few lines of code to check for leading + trailing brackets, and stripping them if both are found, meaning it's an ipv6 address without a port specified.

I can try that on my end in a few days, or happy to accept a PR as long as it includes test coverage for that situation. Either way is fine. Thanks again for spotting this and reporting it!

lonnywong commented 1 year ago

@evanelias Thanks for providing the detailed information. I will try to submit a PR tomorrow.

evanelias commented 1 year ago

Thanks, sounds great!

I just realized that WriteKnownHost doesn't have its own unit test yet, so that might make test coverage for the change tricky. For now, one possible path could be to just create a new exported Normalize function, which wraps golang.org/x/crypto/ssh/knownhosts's Normalize but fixes its handling of ipv6. Then you could just add a unit test which covers Normalize, rather than having to write a unit test for all of WriteKnownHost.

Separate from the write path, there's also the question of how to handle existing ~/.ssh/known_hosts files which already have ipv6 entries with brackets but no port. We probably just need to require users to edit them by hand :(

In theory we could do something in New to handle this "missing port in address" error automatically (rewrite the file to a temp location with the problem fixed, and re-parse?) but that feels messy...

lonnywong commented 1 year ago

In theory we could do something in New to handle this "missing port in address" error automatically (rewrite the file to a temp location with the problem fixed, and re-parse?) but that feels messy...

If there is an invalid entry in the known hosts, everyone removes them on error.

It is probably not a good idea to modify the known hosts automatically without explicit authorization from the user.

I think adding a Fix function would be better than modifying the New function if you want to do it one day.

evanelias commented 1 year ago

Yeah definitely agree that New shouldn't modify the known_hosts file automatically in-place. To clarify what I meant -- upon getting a "missing port in address" error, in theory New could read the full known_hosts file, attempt to write a "fixed" version of it to a temporary file (obtained from os.CreateTemp), pass that temporary file path to x/crypto/ssh/knownhosts, and then delete the temporary file afterwards.

This feels messy though, and also means any KeyError.Want.Filename values will be wrong (referring to the temp file instead of the real file). So I agree your suggestion of a separate Fix function may be a cleaner approach, if someone wants to automate this in the future.