x/crypto/ssh/knownhosts: Should "*" match ports other than 22?

[jgold-stripe]

What version of Go are you using (go version)?

go version go1.16 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jgold/Library/Caches/go-build"
GOENV="/Users/jgold/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/jgold/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/jgold/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.16/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.16/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/jgold/redact/crypto/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/11/02nsw83j2_3gcdzdxs193t300000gn/T/go-build3880178866=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I tried to use knownhosts.New along with a file using @cert-authority wildcards that are intended to match a server listening on a port other than 22. In short, I believe the following unit test should pass (it does not in current version):

// in ssh/knownhosts/knownhosts_test.go
func TestWildcardNon22Port(t *testing.T) {
	str := fmt.Sprintf("* %s", edKeyStr)
	db := testDB(t, str)

	want := &KeyError{
		Want: []KnownKey{{
			Filename: "testdb",
			Line:     1,
			Key:      edKey,
		}},
	}

	got := db.check("server.domain:2222", &net.TCPAddr{}, ecKey)
	if !reflect.DeepEqual(got, want) {
		t.Errorf("got %s, want %s", got, want)
	}
}

What did you expect to see?

I was expecting this to pass. It appears that the formulation is allowed by OpenSSH at least (I tried to find that in the BSD source code but don't read C well enough to navigate that codebase).

What did you see instead?

Host key matching in ssh/knownhosts/knownhosts.go currently fails for such a wildcard. I believe this is because newHostnameMatcher, when provided the input pattern *, gets an error (as expected) in the call to SplitHostPort, which leads it to supply an explicit expectation of port 22 in the generated matcher. That subsequently will fail the port check in the p.addr.port == a.port expression used to test matches.

I'm happy to propose changes, but wonder if perhaps I'm misunderstanding OpenSSH (it being the reference implementation, IIUC).

[cherrymui]

Thanks for the issue. The code above currently using internal functions like testDB. Could you explain what exported functions this is affecting? Could you add an example using exported functions? Thanks.

[jgold-stripe]

That code is a unit test for you in the x/crypto/ssh/knownhosts package itself, so it certainly uses internal functions :) The issue isn't about code using exported APIs. The exported APIs are fine. The issue is that the wildcard parser internal to the package does not appear to handle the same wildcards in a known-hosts file that OpenSSH does. The unit test above shows exactly the kind of line that OpenSSH will match, but that the Go library currently doesn't (the test currently fails, to demonstrate this).

[cherrymui]

Sorry, I'm not sure I understand. Is there anything unexpected that can be observed from exported API? Or this is just an implementation detail that users of this package cannot observe? Thanks.

[jgold-stripe]

Yes -- A user trying to use the host key returned by knownhosts.New will find that in fact the callback does not appear to conform to the behavior of OpenSSH, as the docs state it should:

New creates a host key callback from the given OpenSSH host key files.

For example (and codified in the unit test I've provided), if the known-hosts file contains an entry like @cert-authority * ssh-rsa AAAAkeymaterial...., the Go library fails to match it, whereas OpenSSH will.

I clarified the test case a bit and tried to rename it more precisely (original version didn't have the actual @cert-authority string, so maybe that's why it is confusing?)

func TestWildcardHostAuthorityNon22Port(t *testing.T) {
	str := fmt.Sprintf("@cert-authority * %s", edKeyStr)
	db := testDB(t, str)

	want := &KeyError{
		Want: []KnownKey{{
			Filename: "testdb",
			Line:     1,
			Key:      edKey,
		}},
	}

	got := db.check("server.domain:2222", &net.TCPAddr{}, ecKey)
	if !reflect.DeepEqual(got, want) {
		t.Errorf("got %s, want %s", got, want)
	}
}

[cherrymui]

Thanks.

cc @FiloSottile @rolandshoemaker

[evanelias]

We're encountering the same problem in skeema/knownhosts#9 ... it appears that wildcards are only applied to the hostname portion, and not the port portion, due to the logic at https://cs.opensource.google/go/x/crypto/+/refs/tags/v0.24.0:ssh/knownhosts/knownhosts.go;l=110

func (p *hostPattern) match(a addr) bool {
	return wildcardMatch([]byte(p.addr.host), []byte(a.host)) && p.addr.port == a.port
}

The OpenSSH doc doesn't specify whether wildcards should apply to ports, but indeed the actual behavior of OpenSSH differs from that of x/crypto/ssh/knownhosts here.

In order to match a non-standard port with x/crypto/ssh/knownhosts, currently the known_hosts line needs to specify the exact port, e.g. @cert-authority [*]:2222 ssh-rsa AAAAkeymaterial... which seems weird/incorrect.