Merge pull request #783 from pjbgf/known_hosts_error

libgit2: improve known_hosts error messages

pkg/git/libgit2/managed/ssh.go

@@ -191,21 +191,8 @@ func (t *sshSmartSubtransport) Action(transportOptionsURL string, action git2go.
 	}
 
 	sshConfig.HostKeyCallback = func(hostname string, remote net.Addr, key ssh.PublicKey) error {
-		marshaledKey := key.Marshal()
+		keyHash := sha256.Sum256(key.Marshal())
-		cert := &git2go.Certificate{
+		return CheckKnownHost(hostname, opts.AuthOpts.KnownHosts, keyHash[:])
-			Kind: git2go.CertificateHostkey,
-			Hostkey: git2go.HostkeyCertificate{
-				Kind:         git2go.HostkeySHA256 | git2go.HostkeyRaw,
-				HashSHA256:   sha256.Sum256(marshaledKey),
-				Hostkey:      marshaledKey,
-				SSHPublicKey: key,
-			},
-		}
-
-		if len(opts.AuthOpts.KnownHosts) > 0 {
-			return KnownHostsCallback(hostname, opts.AuthOpts.KnownHosts)(cert, true, hostname)
-		}
-		return nil
 	}
 
 	if t.connected {

pkg/git/libgit2/managed/ssh_test.go

@@ -17,9 +17,11 @@ limitations under the License.
 package managed
 
 import (
+	"net/url"
 	"os"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/fluxcd/pkg/ssh"
 	"github.com/fluxcd/source-controller/pkg/git"
@@ -97,13 +99,20 @@ func TestSSHManagedTransport_E2E(t *testing.T) {
 	err = server.InitRepo("../../testdata/git/repo", git.DefaultBranch, repoPath)
 	g.Expect(err).ToNot(HaveOccurred())
 
+	u, err := url.Parse(server.SSHAddress())
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(u.Host).ToNot(BeEmpty())
+	knownhosts, err := ssh.ScanHostKey(u.Host, 5*time.Second, git.HostKeyAlgos, false)
+	g.Expect(err).NotTo(HaveOccurred())
+
 	transportOptsURL := "ssh://git@fake-url"
 	sshAddress := server.SSHAddress() + "/" + repoPath
 	AddTransportOptions(transportOptsURL, TransportOptions{
 		TargetURL: sshAddress,
 		AuthOpts: &git.AuthOptions{
-			Username: "user",
+			Username:   "user",
-			Identity: kp.PrivateKey,
+			Identity:   kp.PrivateKey,
+			KnownHosts: knownhosts,
 		},
 	})
 

pkg/git/libgit2/managed/transport.go

@@ -1,6 +1,7 @@
 package managed
 
 import (
+	"encoding/base64"
 	"fmt"
 	"net"
 
@@ -14,11 +15,6 @@ import (
 // git.SSH Transports.
 func KnownHostsCallback(host string, knownHosts []byte) git2go.CertificateCheckCallback {
 	return func(cert *git2go.Certificate, valid bool, hostname string) error {
-		kh, err := pkgkh.ParseKnownHosts(string(knownHosts))
-		if err != nil {
-			return fmt.Errorf("failed to parse known_hosts: %w", err)
-		}
-
 		// First, attempt to split the configured host and port to validate
 		// the port-less hostname given to the callback.
 		hostWithoutPort, _, err := net.SplitHostPort(host)
@@ -47,20 +43,38 @@ func KnownHostsCallback(host string, knownHosts []byte) git2go.CertificateCheckC
 			return fmt.Errorf("invalid host key kind, expected to be of kind SHA256")
 		}
 
-		// We are now certain that the configured host and the hostname
+		return CheckKnownHost(host, knownHosts, fingerprint)
-		// given to the callback match. Use the configured host (that
-		// includes the port), and normalize it, so we can check if there
-		// is an entry for the hostname _and_ port.
-		h := knownhosts.Normalize(host)
-		for _, k := range kh {
-			if k.Matches(h, fingerprint) {
-				return nil
-			}
-		}
-		return fmt.Errorf("hostkey could not be verified")
 	}
 }
 
+// CheckKnownHost checks whether the host being connected to is
+// part of the known_hosts, and if so, it ensures the host
+// fingerprint matches the fingerprint of the known host with
+// the same name.
+func CheckKnownHost(host string, knownHosts []byte, fingerprint []byte) error {
+	kh, err := pkgkh.ParseKnownHosts(string(knownHosts))
+	if err != nil {
+		return fmt.Errorf("failed to parse known_hosts: %w", err)
+	}
+
+	if len(kh) == 0 {
+		return fmt.Errorf("hostkey verification aborted: no known_hosts found")
+	}
+
+	// We are now certain that the configured host and the hostname
+	// given to the callback match. Use the configured host (that
+	// includes the port), and normalize it, so we can check if there
+	// is an entry for the hostname _and_ port.
+	h := knownhosts.Normalize(host)
+	for _, k := range kh {
+		if k.Matches(h, fingerprint) {
+			return nil
+		}
+	}
+	return fmt.Errorf("no entries in known_hosts match host '%s' with fingerprint '%s'",
+		h, base64.RawStdEncoding.EncodeToString(fingerprint))
+}
+
 // RemoteCallbacks constructs git2go.RemoteCallbacks with dummy callbacks.
 func RemoteCallbacks() git2go.RemoteCallbacks {
 	// This may not be fully removed as without some of the callbacks git2go

pkg/git/libgit2/managed/transport_test.go

@@ -16,6 +16,17 @@ github.com ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAA
 github.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl
 `
 
+// To fetch latest knownhosts for source.developers.google.com run:
+// ssh-keyscan -p 2022 source.developers.google.com
+//
+// Expected hash (used in the cases) can get found with:
+// ssh-keyscan -p 2022 source.developers.google.com | ssh-keygen -l -f -
+var knownHostsFixtureWithPort = `[source.developers.google.com]:2022 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBB5Iy4/cq/gt/fPqe3uyMy4jwv1Alc94yVPxmnwNhBzJqEV5gRPiRk5u4/JJMbbu9QUVAguBABxL7sBZa5PH/xY=`
+
+// This is an incorrect known hosts entry, that does not aligned with
+// the normalized format and therefore won't match.
+var knownHostsFixtureUnormalized = `source.developers.google.com:2022 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBB5Iy4/cq/gt/fPqe3uyMy4jwv1Alc94yVPxmnwNhBzJqEV5gRPiRk5u4/JJMbbu9QUVAguBABxL7sBZa5PH/xY=`
+
 func TestKnownHostsCallback(t *testing.T) {
 	tests := []struct {
 		name         string
@@ -25,6 +36,38 @@ func TestKnownHostsCallback(t *testing.T) {
 		hostkey      git2go.HostkeyCertificate
 		want         error
 	}{
+		{
+			name:         "Empty",
+			host:         "source.developers.google.com",
+			knownHosts:   []byte(""),
+			hostkey:      git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("AGvEpqYNMqsRNIviwyk4J4HM0lEylomDBKOWZsBn434")},
+			expectedHost: "source.developers.google.com:2022",
+			want:         fmt.Errorf("hostkey verification aborted: no known_hosts found"),
+		},
+		{
+			name:         "Mismatch incorrect known_hosts",
+			host:         "source.developers.google.com",
+			knownHosts:   []byte(knownHostsFixtureUnormalized),
+			hostkey:      git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("AGvEpqYNMqsRNIviwyk4J4HM0lEylomDBKOWZsBn434")},
+			expectedHost: "source.developers.google.com:2022",
+			want:         fmt.Errorf("no entries in known_hosts match host '[source.developers.google.com]:2022' with fingerprint 'AGvEpqYNMqsRNIviwyk4J4HM0lEylomDBKOWZsBn434'"),
+		},
+		{
+			name:         "Match when host has port",
+			host:         "source.developers.google.com:2022",
+			knownHosts:   []byte(knownHostsFixtureWithPort),
+			hostkey:      git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("AGvEpqYNMqsRNIviwyk4J4HM0lEylomDBKOWZsBn434")},
+			expectedHost: "source.developers.google.com:2022",
+			want:         nil,
+		},
+		{
+			name:         "Match even when host does not have port",
+			host:         "source.developers.google.com",
+			knownHosts:   []byte(knownHostsFixtureWithPort),
+			hostkey:      git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("AGvEpqYNMqsRNIviwyk4J4HM0lEylomDBKOWZsBn434")},
+			expectedHost: "source.developers.google.com:2022",
+			want:         nil,
+		},
 		{
 			name:         "Match",
 			host:         "github.com",
@@ -66,7 +109,7 @@ func TestKnownHostsCallback(t *testing.T) {
 			knownHosts:   []byte(knownHostsFixture),
 			hostkey:      git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("ROQFvPThGrW4RuWLoL9tq9I9zJ42fK4XywyRtbOz/EQ")},
 			expectedHost: "github.com",
-			want:         fmt.Errorf("hostkey could not be verified"),
+			want:         fmt.Errorf("no entries in known_hosts match host 'github.com' with fingerprint 'ROQFvPThGrW4RuWLoL9tq9I9zJ42fK4XywyRtbOz/EQ'"),
 		},
 	}
 	for _, tt := range tests {