Omit the IPv6 zone ID if it contains spaces #4

lonnywong · 2023-09-16T01:10:22Z

Closes #3

evanelias · 2023-09-16T05:35:10Z

Thank you for the issue report + fix PR!

I must confess I'm not too familiar with IPv6 zone IDs. Does the rest of golang.org/x/crypto/ssh/knownhosts work properly with them?

In particular: if we strip the zone ID (if it contained a space) when writing the knownhosts entry, will that knownhosts line properly match for future host lookups on the same ipv6 address that have the zone ID present?

From quick scan of golang.org/x/crypto/ssh/knownhosts I'm not sure. Some of the logic uses net.SplitHostPort, which keeps the zone ID in the host.

I haven't tested or looked in depth though. To fully test I think there would need to be no hostname present on the lookup attempt, only an ipv6 address with zone id. (If a hostname is supplied on the lookup, it is used preferentially instead of the ip address, if I understand correctly.)

evanelias · 2023-09-16T06:02:36Z

one other question -- in common ssh implementations such as OpenSSH, what gets written to the knownhosts file in this situation?

lonnywong · 2023-09-16T07:23:33Z

My test with openssh:

platform	command	knownhosts / error
Windows	`ssh hostname.local`	knownhosts: `hostname.local ssh-rsa AAAA...`
Windows	`ssh fe80::20c:29ff:fe18:e2b5`	knownhosts: `fe80::20c:29ff:fe18:e2b5 ssh-rsa AAAA...`
Windows	`ssh "fe80::20c:29ff:fe18:e2b5%Ethernet0"`	login error: `ssh: Could not resolve hostname fe80::20c:29ff:fe18:e2b5%Ethernet0: No such host is known.`
Windows	`ssh "fe80::20c:29ff:fe18:e2b5%Ethernet 1"`	login error: `ssh: Could not resolve hostname fe80::20c:29ff:fe18:e2b5%Ethernet 1: No such host is known.`
MacOS	`ssh hostname.local`	knownhosts: `hostname.local ssh-rsa AAAA...`
MacOS	`ssh fe80::20c:29ff:fe18:e2b5`	login error: `ssh: connect to host fe80::20c:29ff:fe18:e2b5 port 22: No route to host`
MacOS	`ssh fe80::20c:29ff:fe18:e2b5%en0`	knownhosts: `fe80::20c:29ff:fe18:e2b5%en0 ssh-rsa AAAA...`
MacOS	`ssh "fe80::20c:29ff:fe18:e2b5%en 1"`	don't know how to make the zone id with space on MacOS

My test with trzsz-ssh written in go:

platform	command	knownhosts / error
Windows	`tssh hostname.local`	knownhosts damaged: `hostname.local,fe80::20c:29ff:fe18:e2b5%Ethernet 1 ssh-rsa AAAA...`
Windows	`tssh fe80::20c:29ff:fe18:e2b5`	knownhosts: `fe80::20c:29ff:fe18:e2b5 ssh-rsa AAAA...`
Windows	`tssh "fe80::20c:29ff:fe18:e2b5%Ethernet0"`	knownhosts: `fe80::20c:29ff:fe18:e2b5%Ethernet0 ssh-rsa AAAA...`
Windows	`tssh "fe80::20c:29ff:fe18:e2b5%Ethernet 1"`	knownhosts damaged: `fe80::20c:29ff:fe18:e2b5%Ethernet 1 ssh-rsa AAAA...`
MacOS	`tssh hostname.local`	knownhosts: `hostname.local,fe80::20c:29ff:fe18:e2b5%en0 ssh-rsa AAAA...`
MacOS	`tssh fe80::20c:29ff:fe18:e2b5`	login error: `dial tcp [fe80::20c:29ff:fe18:e2b5]:22: connect: no route to host`
MacOS	`tssh fe80::20c:29ff:fe18:e2b5%en0`	knownhosts: `fe80::20c:29ff:fe18:e2b5%en0 ssh-rsa AAAA...`
MacOS	`tssh "fe80::20c:29ff:fe18:e2b5%en 1"`	don't know how to make the zone id with space on MacOS

lonnywong · 2023-09-16T07:33:02Z

I want to fix the case tssh hostname.local only, make sure it's not writing some extra spaces to knownhosts.

evanelias · 2023-09-16T16:01:04Z

Thanks, that helps clarify the situation.

I definitely agree we must avoid writing the known_hosts line with the extra space. But I want to be fully confident that stripping the zone ID entirely is safe, and won't cause any other problems down the line in other use-cases.

Here's a situation I'm concerned about, regarding whether the knownhosts lookup path will work correctly after the zone is stripped:

User SSH to "fe80::20c:29ff:fe18:e2b5%Ethernet 1" for the first time.
Let's say we write the known_hosts line with the zone stripped, e.g. fe80::20c:29ff:fe18:e2b5 ssh-rsa AAAA....
Later on, user SSH to "fe80::20c:29ff:fe18:e2b5%Ethernet 1" again. Does the line that we wrote to known_hosts above (which had the zone stripped) successfully match on this IP (with the zone present)?
If it doesn't match this line, and we're implementing behavior like StrictHostKeyChecking=Ask, we can end up prompting the user and writing a duplicate known_hosts line each time they SSH to this address. That seems wrong.
- To be clear though I haven't tested the matching logic so I'm not sure. But from scanning the code I suspect it won't match. And apparently some other parts of Go standard lib have issues with zones...

Instead of stripping the zone, could this approach work?

If the ipv6 zone contains a space, but we also have a hostname (i.e. remoteStrNormalized != Normalize(hostname)), then just write the known_hosts line with only the hostname and omit the ipv6 address entirely. For example, when user SSH to hostname.local, known_hosts line would get written simply as hostname.local ssh-rsa AAAA... without including the ipv6 address at all.
If the ipv6 zone contains a space, and we do NOT have a hostname (remoteStrNormalized == Normalize(hostname)) then perhaps WriteKnownHost should just return an error?

lonnywong · 2023-09-16T16:19:23Z

72c9964 works like this:

If the zone ID contains spaces, ssh hostname.local writes hostname.local,fe80::20c:29ff:fe18:e2b5 ssh-rsa AAAA..., it works with ssh fe80::20c:29ff:fe18:e2b5 later ( on Windows only ).
If no spaces in the zone ID, ssh hostname.local writes hostname.local,fe80::20c:29ff:fe18:e2b5%en0 ssh-rsa AAAA..., it works same as before.
ssh fe80::20c:29ff:fe18:e2b5%en0 writes fe80::20c:29ff:fe18:e2b5%en0 ssh-rsa AAAA..., it works same as before.
ssh "fe80::20c:29ff:fe18:e2b5%Ethernet 1" writes fe80::20c:29ff:fe18:e2b5%Ethernet 1 ssh-rsa AAAA..., it breaks knownhosts as before. In this case, maybe we should return an error:
```
if strings.ContainsAny(hostname, "\t ") {
    return fmt.Errorf("knownhosts: hostname '%s' contains spaces", hostname)
}
```

evanelias · 2023-09-16T16:56:01Z

I may be misremembering how ssh.HostKeyCallback args work when the hostname is an IP address, but I thought in that case we get a hostname string equal to the IP address, such that Normalize(hostname) == Normalize(remote.String())

So if I'm reading 72c9964 correctly, I would expect that case 4 would actually write fe80::20c:29ff:fe18:e2b5%Ethernet 1,fe80::20c:29ff:fe18:e2b5 ssh-rsa AAAA... since the zone is stripped from remoteStr but not from hostname, which means remoteStrNormalized will not equal Normalize(hostname) anymore and then we include both values when writing the known_hosts line. (That's still bad, to be clear... just nitpicking to ensure I am reading the code correctly)

Anyway yes I think it would be better to just return an error in that specific situation of case 4 (remote contains a zone ID with a space, and we don't have a separate hostname).

We disagree on case 1 though (remote contains a zone ID with a space, but we also have a separate hostname that isn't just an IP address). I think it might be better to just write hostname.local ssh-rsa AAAA... in that situation, since that may be aligned with what openssh would write in that situation. But it's not a hard opinion, I am ok with merging this as-is in terms of the case 1 behavior.

lonnywong · 2023-09-16T17:09:09Z

I would expect that case 4 would actually write fe80::20c:29ff:fe18:e2b5%Ethernet 1,fe80::20c:29ff:fe18:e2b5 ssh-rsa AAAA...

You are right. Let's keep it simple, we return an error if the hostname contains spaces, and we align it with openssh if the remote address contains spaces.

evanelias · 2023-09-16T17:33:58Z

That approach sounds good to me, thanks!

evanelias

This looks great! Thanks again for finding and fixing the issue, and also for your patience in the discussion. I'll merge momentarily, and will plan to tag a new version on Monday.

evanelias · 2023-09-16T19:16:58Z

knownhosts.go

+	if strings.ContainsAny(hostnameNormalized, "\t ") {
+		return fmt.Errorf("knownhosts: hostname '%s' contains spaces", hostnameNormalized)
+	}
+	addresses := []string{hostnameNormalized}


Just noting for background: previously, addresses had the non-normalized versions of the strings, because Line already also calls Normalize on these. But I suppose it's harmless to change here, because both functions need to normalize the input (since they're exported functions and Line may be used independently) and there's no harm in calling it redundantly.

lonnywong · 2023-09-16T20:58:33Z

@evanelias Thanks for your help.

Avoid writing extra spaces in WriteKnownHost skeema#4

4d5bb77

lonnywong force-pushed the main branch from 72c9964 to 4d5bb77 Compare September 16, 2023 17:35

evanelias approved these changes Sep 16, 2023

View reviewed changes

evanelias merged commit 09454b7 into skeema:main Sep 16, 2023
1 check passed

Porkepix mentioned this pull request Sep 27, 2023

skeema 1.11.0 Homebrew/homebrew-core#146244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omit the IPv6 zone ID if it contains spaces #4

Omit the IPv6 zone ID if it contains spaces #4

lonnywong commented Sep 16, 2023

evanelias commented Sep 16, 2023

evanelias commented Sep 16, 2023

lonnywong commented Sep 16, 2023

lonnywong commented Sep 16, 2023

evanelias commented Sep 16, 2023

lonnywong commented Sep 16, 2023 •

edited

Loading

evanelias commented Sep 16, 2023

lonnywong commented Sep 16, 2023

evanelias commented Sep 16, 2023

evanelias left a comment

evanelias Sep 16, 2023

lonnywong commented Sep 16, 2023

Omit the IPv6 zone ID if it contains spaces #4

Omit the IPv6 zone ID if it contains spaces #4

Conversation

lonnywong commented Sep 16, 2023

evanelias commented Sep 16, 2023

evanelias commented Sep 16, 2023

lonnywong commented Sep 16, 2023

lonnywong commented Sep 16, 2023

evanelias commented Sep 16, 2023

lonnywong commented Sep 16, 2023 • edited Loading

evanelias commented Sep 16, 2023

lonnywong commented Sep 16, 2023

evanelias commented Sep 16, 2023

evanelias left a comment

Choose a reason for hiding this comment

evanelias Sep 16, 2023

Choose a reason for hiding this comment

lonnywong commented Sep 16, 2023

lonnywong commented Sep 16, 2023 •

edited

Loading