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

x/crypto/ssh: knownhosts does not handle multiple keys with same type #28870

Open
nullron opened this issue Nov 19, 2018 · 1 comment
Open

x/crypto/ssh: knownhosts does not handle multiple keys with same type #28870

nullron opened this issue Nov 19, 2018 · 1 comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@nullron
Copy link

nullron commented Nov 19, 2018

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

go version go1.11 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
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/rom/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/rom/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/h0/fll06vqd6wd6mqndvvtfy75r0000gn/T/go-build120797240=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Open a SSH connection to a host that has multiple keys in my known_hosts file.

There is a load balancer is in front of two SSH servers that maintain different keys. Although I do not agree the setup is best practice, OpenSSH allows for multiple keys+types for the same hostname.

I used a simple test application to validate:

package main

import (
        "flag"
        "fmt"
        "log"
        "net"
        "os"

        "golang.org/x/crypto/ssh"
        "golang.org/x/crypto/ssh/agent"
        "os/user"
        "path/filepath"
        "golang.org/x/crypto/ssh/knownhosts"
)

var (
        USER = flag.String("user", os.Getenv("USER"), "ssh username")
        HOST = flag.String("host", "localhost", "ssh server hostname")
        PORT = flag.Int("port", 22, "ssh server port")
        PASS = flag.String("pass", os.Getenv("SOCKSIE_SSH_PASSWORD"), "ssh password")
        SIZE = flag.Int("s", 1<<15, "set max packet size")
)

func init() {
        flag.Parse()
}

func main() {

        var auths []ssh.AuthMethod
        if aconn, err := net.Dial("unix", os.Getenv("SSH_AUTH_SOCK")); err == nil {
                auths = append(auths, ssh.PublicKeysCallback(agent.NewClient(aconn).Signers))

        }
        if *PASS != "" {
                auths = append(auths, ssh.Password(*PASS))
        }

        callback, err := GetKnownHostsCallback()

        config := ssh.ClientConfig{
                User: *USER,
                Auth: auths,
                HostKeyCallback: callback,
        }
        addr := fmt.Sprintf("%s:%d", *HOST, *PORT)
        conn, err := ssh.Dial("tcp", addr, &config)
        if err != nil {
                log.Fatalf("unable to connect to [%s]: %v", addr, err)
        }

        conn.Close()

}

func GetKnownHostsCallback() (ssh.HostKeyCallback, error) {
        usr, err := user.Current()

        if err != nil {
                return nil, err
        }

        name := filepath.Join(usr.HomeDir, ".ssh", "known_hosts")
        log.Printf("Using known hosts file %s", name)
        f, err := knownhosts.New(name)

        if err != nil {
                return nil, err
        }

        return func(addr string, remote net.Addr, key ssh.PublicKey) error {
                log.Printf("Checking known host %s (%v)", addr, remote)
                return f(addr, remote, key)
        }, nil
}

What did you expect to see?

I expected the host key to be validated in the same manner as OpenSSH.

What did you see instead?

ssh: handshake failed: knownhosts: key mismatch

In my test, I added a fake key of the same type and hostname. If the valid key was first, it worked fine. Anything else would fail.

I noticed this from crypto/ssh/knownhosts/knownhosts.go:

func (db *hostKeyDB) checkAddr(a addr, remoteKey ssh.PublicKey) error {
	// TODO(hanwen): are these the right semantics? What if there
	// is just a key for the IP address, but not for the
	// hostname?

	// Algorithm => key.
	knownKeys := map[string]KnownKey{}
	for _, l := range db.lines {
		if l.match(a) {
			typ := l.knownKey.Key.Type()
			if _, ok := knownKeys[typ]; !ok {
				knownKeys[typ] = l.knownKey
			}
		}
}

Which will only look at the first key of a given type. To work around this, I added another key type for the other server and it worked fine. However, I think this should handle multiple key/type combinations.

@gopherbot gopherbot added this to the Unreleased milestone Nov 19, 2018
@bcmills
Copy link
Contributor

bcmills commented Nov 19, 2018

CC @FiloSottile @hanwen

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 19, 2018
@FiloSottile FiloSottile changed the title x/crypto: knownhosts does not handle multiple keys with same type x/crypto/ssh: knownhosts does not handle multiple keys with same type Nov 23, 2018
evanelias added a commit to skeema/knownhosts that referenced this issue Sep 18, 2023
Currently the behavior of HostKeyAlgorithms never contains duplicates, only by
virtue of golang.org/x/crypto/ssh/knownhosts exposing a maximum of one key per
algorithm in its KeyError.Want slice.

However, that upstream behavior could theoretically change in the future,
especially since golang.org/x/crypto is versioned as a pre-v1 module, and the
one-key-per-type behavior is only documented as a comment (e.g. not part of
any type or function signature).

This commit makes our HostKeyAlgorithms function more robust / future-proof
by ensuring that its result does not contain duplicates, regardless of
upstream behavior.

This means if golang/go#28870 is solved (for example
by golang/crypto#254), there should not be any harm to
our behavior here in github.com/skeema/knownhosts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants