WIP: implement workaround for wildcards and non-standard port

*** This is a work-in-progress commit, which will be amended/rewritten ***

README.md

@@ -18,6 +18,9 @@ This repo ([github.com/skeema/knownhosts](https://github.com/skeema/knownhosts))
 
 * Look up known_hosts public keys for any given host
 * Auto-populate ssh.ClientConfig.HostKeyAlgorithms easily based on known_hosts, providing a solution for [golang/go#29286](https://github.com/golang/go/issues/29286)
+  * As of v1.3.0, this also properly handles CAs when [using the NewDB constructor](#enhancements-requiring-extra-parsing)
+* Properly match wildcard hostname known_hosts entries regardless of port number, providing a solution for [golang/go#52056](https://github.com/golang/go/issues/52056)
+  * Added in v1.3.0; requires [using the NewDB constructor](#enhancements-requiring-extra-parsing)
 * Write new known_hosts entries to an io.Writer
 * Properly format/normalize new known_hosts entries containing ipv6 addresses, providing a solution for [golang/go#53463](https://github.com/golang/go/issues/53463)
 * Determine if an ssh.HostKeyCallback's error corresponds to a host whose key has changed (indicating potential MitM attack) vs a host that just isn't known yet
@@ -57,6 +60,19 @@ func sshConfigForHost(hostWithPort string) (*ssh.ClientConfig, error) {
 }
 ```
 
+## Enhancements requiring extra parsing
+
+Originally, this package did not re-read/parse the known_hosts files at all, relying entirely on [golang.org/x/crypto/ssh/knownhosts](https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts) for all known_hosts file reading and processing. This package only offered a constructor called `New`, returning a host key callback, identical to the call pattern of [golang.org/x/crypto/ssh/knownhosts](https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts). This package just added extra methods on top of its returned callback type.
+
+However, a couple bugs in [golang.org/x/crypto/ssh/knownhosts](https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts) cannot possibly be solved without re-reading the known_hosts file. Therefore, as of v1.3.0 of this package, we now offer an alternative constructor `NewDB`, which returns a struct type `HostKeyDB`. When using `NewDB`, this package does an additional read of the known_hosts file (after the one from [golang.org/x/crypto/ssh/knownhosts](https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts)), in order to detect:
+
+* @cert-authority lines, which require returning different host key algorithm values than regular host keys
+* host pattern wildcards, which [golang.org/x/crypto/ssh/knownhosts](https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts) doesn't properly match against all port numbers, like OpenSSH does
+
+Aside from detecting these special cases, this package otherwise still directly uses [golang.org/x/crypto/ssh/knownhosts](https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts) for host lookups and all other known_hosts file processing. We do not fork or re-implement these core behaviors of [golang.org/x/crypto/ssh/knownhosts](https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts).
+
+The performance impact of this extra read should be minimal, as the file should typically be in the filesystem cache already from the original read by [golang.org/x/crypto/ssh/knownhosts](https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts). That said, users who wish to avoid the extra read can stay with the `New` constructor, which retains its pre-v1.3.0 behavior as-is. However, the extra fixes for @cert-authority and host pattern wildcards will not be present in that case.
+
 ## Writing new known_hosts entries
 
 If you wish to mimic the behavior of OpenSSH's `StrictHostKeyChecking=no` or `StrictHostKeyChecking=ask`, this package provides a few functions to simplify this task. For example:

example_test.go

@@ -48,6 +48,45 @@ func ExampleNewDB() {
 	defer client.Close()
 }
 
+func ExampleHostKeyCallback_ToDB() {
+	khFile := "/home/myuser/.ssh/known_hosts"
+	var kh *knownhosts.HostKeyDB
+	var err error
+
+	// Example of using conditional logic to determine whether or not to perform
+	// extra parsing pass on the known_hosts file in order to enable enhanced
+	// behaviors
+	if os.Getenv("SKIP_KNOWNHOSTS_ENHANCEMENTS") != "" {
+		// Create a HostKeyDB using New + ToDB: this will skip the extra known_hosts
+		// processing
+		var cb knownhosts.HostKeyCallback
+		if cb, err = knownhosts.New(khFile); err == nil {
+			kh = cb.ToDB()
+		}
+	} else {
+		// Create a HostKeyDB using NewDB: this will perform extra known_hosts
+		// processing, allowing proper support for CAs, as well as OpenSSH-like
+		// wildcard matching on non-standard ports
+		kh, err = knownhosts.NewDB(khFile)
+	}
+	if err != nil {
+		log.Fatal("Failed to read known_hosts: ", err)
+	}
+
+	sshHost := "yourserver.com:22"
+	config := &ssh.ClientConfig{
+		User:              "myuser",
+		Auth:              []ssh.AuthMethod{ /* ... */ },
+		HostKeyCallback:   kh.HostKeyCallback(),
+		HostKeyAlgorithms: kh.HostKeyAlgorithms(sshHost),
+	}
+	client, err := ssh.Dial("tcp", sshHost, config)
+	if err != nil {
+		log.Fatal("Failed to dial: ", err)
+	}
+	defer client.Close()
+}
+
 func ExampleWriteKnownHost() {
 	sshHost := "yourserver.com:22"
 	khPath := "/home/myuser/.ssh/known_hosts"

knownhosts.go

@@ -22,22 +22,31 @@ import (
 // behaviors, such as the ability to perform host key/algorithm lookups from
 // known_hosts entries.
 type HostKeyDB struct {
-	callback ssh.HostKeyCallback
-	isCert   map[string]bool // keyed by "filename:line"
+	callback   ssh.HostKeyCallback
+	isCert     map[string]bool // keyed by "filename:line"
+	isWildcard map[string]bool // keyed by "filename:line"
 }
 
 // NewDB creates a HostKeyDB from the given OpenSSH known_hosts file(s). It
 // reads and parses the provided files one additional time (beyond logic in
-// golang.org/x/crypto/ssh/knownhosts) in order to handle CA lines properly.
+// golang.org/x/crypto/ssh/knownhosts) in order to:
+//
+//   - Handle CA lines properly and return ssh.CertAlgo* values when calling the
+//     HostKeyAlgorithms method, for use in ssh.ClientConfig.HostKeyAlgorithms
+//   - Allow * wildcards in hostnames to match on non-standard ports, providing
+//     a workaround for https://github.com/golang/go/issues/52056 in order to
+//     align with OpenSSH's wildcard behavior
+//
 // When supplying multiple files, their order does not matter.
 func NewDB(files ...string) (*HostKeyDB, error) {
 	cb, err := xknownhosts.New(files...)
 	if err != nil {
 		return nil, err
 	}
 	hkdb := &HostKeyDB{
-		callback: cb,
-		isCert:   make(map[string]bool),
+		callback:   cb,
+		isCert:     make(map[string]bool),
+		isWildcard: make(map[string]bool),
 	}
 
 	// Re-read each file a single time, looking for @cert-authority lines. The
@@ -59,6 +68,16 @@ func NewDB(files ...string) (*HostKeyDB, error) {
 			if len(line) > 15 && bytes.HasPrefix(line, []byte("@cert-authority")) && (line[15] == ' ' || line[15] == '\t') {
 				mapKey := fmt.Sprintf("%s:%d", filename, lineNum)
 				hkdb.isCert[mapKey] = true
+				line = bytes.TrimSpace(line[16:])
+			}
+			// truncate line to just the host pattern field
+			if i := bytes.IndexAny(line, "\t "); i >= 0 {
+				line = line[:i]
+			}
+			// Does the host pattern contain a * wildcard and no specific port?
+			if i := bytes.IndexRune(line, '*'); i >= 0 && !bytes.Contains(line[i:], []byte("]:")) {
+				mapKey := fmt.Sprintf("%s:%d", filename, lineNum)
+				hkdb.isWildcard[mapKey] = true
 			}
 		}
 		if err := scanner.Err(); err != nil {
@@ -73,11 +92,56 @@ func NewDB(files ...string) (*HostKeyDB, error) {
 // Alternatively, you can wrap it with an outer callback to potentially handle
 // appending a new entry to the known_hosts file; see example in WriteKnownHost.
 func (hkdb *HostKeyDB) HostKeyCallback() ssh.HostKeyCallback {
-	return hkdb.callback
+	// Either NewDB found no wildcard host patterns, or hkdb was created from
+	// HostKeyCallback.ToDB in which case we didn't scan known_hosts for them:
+	// return the callback (which came from x/crypto/ssh/knownhosts) as-is
+	if len(hkdb.isWildcard) == 0 {
+		return hkdb.callback
+	}
+
+	// If we scanned for wildcards and found at least one, return a wrapped
+	// callback with extra behavior: if the host lookup found no matches, and the
+	// host arg had a non-standard port, re-do the lookup on standard port 22. If
+	// that second call returns a *xknownhosts.KeyError, filter down any resulting
+	// Want keys to known wildcard entries.
+	f := func(hostname string, remote net.Addr, key ssh.PublicKey) error {
+		callbackErr := hkdb.callback(hostname, remote, key)
+		if callbackErr == nil || IsHostKeyChanged(callbackErr) { // hostname has known_host entries as-is
+			return callbackErr
+		}
+		justHost, port, splitErr := net.SplitHostPort(hostname)
+		if splitErr != nil || port == "" || port == "22" { // hostname already using standard port
+			return callbackErr
+		}
+		// If we reach here, the port was non-standard and no known_host entries
+		// were found for the non-standard port. Try again with standard port.
+		if tcpAddr, ok := remote.(*net.TCPAddr); ok && tcpAddr.Port != 22 {
+			remote = &net.TCPAddr{
+				IP:   tcpAddr.IP,
+				Port: 22,
+				Zone: tcpAddr.Zone,
+			}
+		}
+		callbackErr = hkdb.callback(justHost+":22", remote, key)
+		var keyErr *xknownhosts.KeyError
+		if errors.As(callbackErr, &keyErr) && len(keyErr.Want) > 0 {
+			wildcardKeys := make([]xknownhosts.KnownKey, 0, len(keyErr.Want))
+			for _, wantKey := range keyErr.Want {
+				if hkdb.isWildcard[fmt.Sprintf("%s:%d", wantKey.Filename, wantKey.Line)] {
+					wildcardKeys = append(wildcardKeys, wantKey)
+				}
+			}
+			callbackErr = &xknownhosts.KeyError{
+				Want: wildcardKeys,
+			}
+		}
+		return callbackErr
+	}
+	return ssh.HostKeyCallback(f)
 }
 
 // PublicKey wraps ssh.PublicKey with an additional field, to identify
-// whether they key corresponds to a certificate authority.
+// whether the key corresponds to a certificate authority.
 type PublicKey struct {
 	ssh.PublicKey
 	Cert bool
@@ -96,7 +160,8 @@ func (hkdb *HostKeyDB) HostKeys(hostWithPort string) (keys []PublicKey) {
 	placeholderAddr := &net.TCPAddr{IP: []byte{0, 0, 0, 0}}
 	placeholderPubKey := &fakePublicKey{}
 	var kkeys []xknownhosts.KnownKey
-	if hkcbErr := hkdb.callback(hostWithPort, placeholderAddr, placeholderPubKey); errors.As(hkcbErr, &keyErr) {
+	callback := hkdb.HostKeyCallback()
+	if hkcbErr := callback(hostWithPort, placeholderAddr, placeholderPubKey); errors.As(hkcbErr, &keyErr) {
 		kkeys = append(kkeys, keyErr.Want...)
 		knownKeyLess := func(i, j int) bool {
 			if kkeys[i].Filename < kkeys[j].Filename {
@@ -190,14 +255,16 @@ func keyTypeToCertAlgo(keyType string) string {
 // otherwise identical to ssh.HostKeyCallback, and does not introduce any file-
 // parsing behavior beyond what is in golang.org/x/crypto/ssh/knownhosts.
 //
+// In most situations, use HostKeyDB and its constructor NewDB instead of using
+// the HostKeyCallback type. The HostKeyCallback type is only provided for
+// backwards compatibility with older versions of this package, as well as for
+// very strict situations where any extra known_hosts file-parsing is
+// undesirable.
+//
 // Methods of HostKeyCallback do not provide any special treatment for
 // @cert-authority lines, which will (incorrectly) look like normal non-CA host
-// keys. HostKeyCallback should generally only be used in situations in which
-// @cert-authority lines won't appear, and/or in very strict situations where
-// any extra known_hosts file-parsing is undesirable.
-//
-// In most situations, use HostKeyDB and its constructor NewDB instead of using
-// the HostKeyCallback type.
+// keys. Additionally, HostKeyCallback lacks the fix for applying * wildcard
+// known_host entries to all ports, like OpenSSH's behavior.
 type HostKeyCallback ssh.HostKeyCallback
 
 // New creates a HostKeyCallback from the given OpenSSH known_hosts file(s). The
@@ -207,9 +274,9 @@ type HostKeyCallback ssh.HostKeyCallback
 // When supplying multiple files, their order does not matter.
 //
 // In most situations, you should avoid this function, as the returned value
-// does not handle @cert-authority lines correctly. See doc comment for
-// HostKeyCallback for more information. Instead, use NewDB to create a
-// HostKeyDB with proper CA support.
+// lacks several enhanced behaviors. See doc comment for HostKeyCallback for
+// more information. Instead, most callers should use NewDB to create a
+// HostKeyDB, which includes these enhancements.
 func New(files ...string) (HostKeyCallback, error) {
 	cb, err := xknownhosts.New(files...)
 	return HostKeyCallback(cb), err
@@ -222,16 +289,20 @@ func (hkcb HostKeyCallback) HostKeyCallback() ssh.HostKeyCallback {
 }
 
 // ToDB converts the receiver into a HostKeyDB. However, the returned HostKeyDB
-// lacks proper CA support. It is usually preferable to create a CA-supporting
-// HostKeyDB instead, by using NewDB.
-// This method is provided for situations in which the calling code needs to
-// make CA support optional / user-configurable. This way, calling code can
-// conditionally create a non-CA-supporting HostKeyDB by calling New(...).ToDB()
-// or a CA-supporting HostKeyDB by calling NewDB(...).
+// lacks the enhanced behaviors described in the doc comment for NewDB: proper
+// CA support, and wildcard matching on nonstandard ports.
+//
+// It is generally preferable to create a HostKeyDB by using NewDB. The ToDB
+// method is only provided for situations in which the calling code needs to
+// make the extra NewDB behaviors optional / user-configurable, perhaps for
+// reasons of performance or code trust (since NewDB reads the known_host file
+// an extra time, which may be undesirable in some strict situations). This way,
+// callers can conditionally create a non-enhanced HostKeyDB by using New and
+// ToDB. See code example.
 func (hkcb HostKeyCallback) ToDB() *HostKeyDB {
-	// This intentionally leaves the isCert map field as nil, as there is no way
-	// to retroactively populate it from just a HostKeyCallback. Methods of
-	// HostKeyDB will skip any CA-related behaviors accordingly.
+	// This intentionally leaves the isCert and isWildcard map fields as nil, as
+	// there is no way to retroactively populate them from just a HostKeyCallback.
+	// Methods of HostKeyDB will skip any related enhanced behaviors accordingly.
 	return &HostKeyDB{callback: ssh.HostKeyCallback(hkcb)}
 }
 

knownhosts_test.go

@@ -191,7 +191,13 @@ func TestWithCertLines(t *testing.T) {
 			expectedAlgos:    []string{ssh.KeyAlgoRSASHA512, ssh.KeyAlgoRSASHA256, ssh.KeyAlgoRSA, ssh.CertAlgoECDSA256v01},
 		},
 		{
-			host:             "whatever.lol.test:22", // only matches the * entry
+			host:             "whatever.test:22", // only matches the * entry
+			expectedKeyTypes: []string{ssh.KeyAlgoECDSA256},
+			expectedIsCert:   []bool{true},
+			expectedAlgos:    []string{ssh.CertAlgoECDSA256v01},
+		},
+		{
+			host:             "whatever.test:22022", // only matches the * entry
 			expectedKeyTypes: []string{ssh.KeyAlgoECDSA256},
 			expectedIsCert:   []bool{true},
 			expectedAlgos:    []string{ssh.CertAlgoECDSA256v01},
@@ -202,6 +208,12 @@ func TestWithCertLines(t *testing.T) {
 			expectedIsCert:   []bool{true, true, true},
 			expectedAlgos:    []string{ssh.CertAlgoRSASHA512v01, ssh.CertAlgoRSASHA256v01, ssh.CertAlgoRSAv01, ssh.CertAlgoECDSA256v01, ssh.CertAlgoED25519v01},
 		},
+		{
+			host:             "oddport.certy.test:2345",
+			expectedKeyTypes: []string{ssh.KeyAlgoRSA, ssh.KeyAlgoECDSA256, ssh.KeyAlgoED25519},
+			expectedIsCert:   []bool{true, true, true},
+			expectedAlgos:    []string{ssh.CertAlgoRSASHA512v01, ssh.CertAlgoRSASHA256v01, ssh.CertAlgoRSAv01, ssh.CertAlgoECDSA256v01, ssh.CertAlgoED25519v01},
+		},
 	}
 	for _, tc := range testCases {
 		annotatedKeys := kh.HostKeys(tc.host)