From 47692530d7288d751434e314a95a5353b616eb18 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Wed, 8 Jun 2022 13:31:19 +0530 Subject: [PATCH] libgit2: fix ssh host key verification regression Earlier, host key verification could potentially fail if there were multiple entries in the known_hosts file and if the intended encryption algorithm wasn't the first entry. This happened because we used the same hasher object to compute the sum of all the public keys present in the known_hosts file, which led to invalid hashes, resulting in a mismatch when compared with the hash of the advertised public key. This is fixed, by not creating the hasher ourselves and instead delegating that to the function actually doing the matching, ensuring that a new hasher is used for each comparison. Regression introduced in v0.25.0 and reported in https://github.com/fluxcd/image-automation-controller/issues/378 Signed-off-by: Sanskar Jaiswal --- go.mod | 2 +- go.sum | 4 ++-- pkg/git/libgit2/managed/transport.go | 6 +---- pkg/git/libgit2/managed/transport_test.go | 27 +++++++++++++---------- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/go.mod b/go.mod index b7342fd5b..29af8a5ea 100644 --- a/go.mod +++ b/go.mod @@ -28,7 +28,7 @@ require ( github.com/fluxcd/pkg/helmtestserver v0.7.3 github.com/fluxcd/pkg/lockedfile v0.1.0 github.com/fluxcd/pkg/runtime v0.16.1 - github.com/fluxcd/pkg/ssh v0.4.1 + github.com/fluxcd/pkg/ssh v0.5.0 github.com/fluxcd/pkg/testserver v0.2.0 github.com/fluxcd/pkg/untar v0.1.0 github.com/fluxcd/pkg/version v0.1.0 diff --git a/go.sum b/go.sum index 1623a4462..9c62cd357 100644 --- a/go.sum +++ b/go.sum @@ -282,8 +282,8 @@ github.com/fluxcd/pkg/lockedfile v0.1.0 h1:YsYFAkd6wawMCcD74ikadAKXA4s2sukdxrn7w github.com/fluxcd/pkg/lockedfile v0.1.0/go.mod h1:EJLan8t9MiOcgTs8+puDjbE6I/KAfHbdvIy9VUgIjm8= github.com/fluxcd/pkg/runtime v0.16.1 h1:WU1vNZz4TAzmATQ/tl2zB/FX6GIUTgYeBn/G5RuTA2c= github.com/fluxcd/pkg/runtime v0.16.1/go.mod h1:cgVJkOXCg9OmrIUGklf/0UtV28MNzkuoBJhaEQICT6E= -github.com/fluxcd/pkg/ssh v0.4.1 h1:O5FCjb5NIZ9PeRjdF2iL9jaPNM+RL+IjrMBZPkqF9W4= -github.com/fluxcd/pkg/ssh v0.4.1/go.mod h1:KGgOUOy1uI6RC6+qxIBLvP1AeOOs/nLB25Ca6TZMIXE= +github.com/fluxcd/pkg/ssh v0.5.0 h1:jE9F2XvUXC2mgseeXMATvO014fLqdB30/VzlPLKsk20= +github.com/fluxcd/pkg/ssh v0.5.0/go.mod h1:KGgOUOy1uI6RC6+qxIBLvP1AeOOs/nLB25Ca6TZMIXE= github.com/fluxcd/pkg/testserver v0.2.0 h1:Mj0TapmKaywI6Fi5wvt1LAZpakUHmtzWQpJNKQ0Krt4= github.com/fluxcd/pkg/testserver v0.2.0/go.mod h1:bgjjydkXsZTeFzjz9Cr4heGANr41uTB1Aj1Q5qzuYVk= github.com/fluxcd/pkg/untar v0.1.0 h1:k97V/xV5hFrAkIkVPuv5AVhyxh1ZzzAKba/lbDfGo6o= diff --git a/pkg/git/libgit2/managed/transport.go b/pkg/git/libgit2/managed/transport.go index 502c82f62..5f6202366 100644 --- a/pkg/git/libgit2/managed/transport.go +++ b/pkg/git/libgit2/managed/transport.go @@ -1,9 +1,7 @@ package managed import ( - "crypto/sha256" "fmt" - "hash" "net" pkgkh "github.com/fluxcd/pkg/ssh/knownhosts" @@ -42,11 +40,9 @@ func KnownHostsCallback(host string, knownHosts []byte) git2go.CertificateCheckC } var fingerprint []byte - var hasher hash.Hash switch { case cert.Hostkey.Kind&git2go.HostkeySHA256 > 0: fingerprint = cert.Hostkey.HashSHA256[:] - hasher = sha256.New() default: return fmt.Errorf("invalid host key kind, expected to be of kind SHA256") } @@ -57,7 +53,7 @@ func KnownHostsCallback(host string, knownHosts []byte) git2go.CertificateCheckC // is an entry for the hostname _and_ port. h := knownhosts.Normalize(host) for _, k := range kh { - if k.Matches(h, fingerprint, hasher) { + if k.Matches(h, fingerprint) { return nil } } diff --git a/pkg/git/libgit2/managed/transport_test.go b/pkg/git/libgit2/managed/transport_test.go index fc847ba66..59dfe3bd6 100644 --- a/pkg/git/libgit2/managed/transport_test.go +++ b/pkg/git/libgit2/managed/transport_test.go @@ -1,10 +1,7 @@ package managed import ( - "crypto/x509" "encoding/base64" - "encoding/pem" - "errors" "fmt" "testing" @@ -14,7 +11,10 @@ import ( // knownHostsFixture is known_hosts fixture in the expected // format. -var knownHostsFixture = `github.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ==` +var knownHostsFixture = `github.com ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ== +github.com ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBEmKSENjQEezOmxkZMy7opKgwFB9nkt5YRrYMjNuG5N87uRgg6CLrbo5wAdT/y6v0mKV0U2w0WZ2YB/++Tpockg= +github.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl +` func TestKnownHostsCallback(t *testing.T) { tests := []struct { @@ -41,6 +41,17 @@ func TestKnownHostsCallback(t *testing.T) { expectedHost: "github.com:22", want: nil, }, + { + // Test case to specifically detect a regression introduced in v0.25.0 + // Ref: https://github.com/fluxcd/image-automation-controller/issues/378 + name: "Match regardless of order of known_hosts", + host: "github.com", + knownHosts: []byte(knownHostsFixture), + // Use ecdsa-sha2-nistp256 instead of ssh-rsa + hostkey: git2go.HostkeyCertificate{Kind: git2go.HostkeySHA256, HashSHA256: sha256Fingerprint("p2QAMXNIC1TJYWeIOttrVc98/R1BUFWu3/LiyKgUfQM")}, + expectedHost: "github.com:22", + want: nil, + }, { name: "Hostname mismatch", host: "github.com", @@ -83,11 +94,3 @@ func sha256Fingerprint(in string) [32]byte { copy(out[:], d) return out } - -func certificateFromPEM(pemBytes string) (*x509.Certificate, error) { - block, _ := pem.Decode([]byte(pemBytes)) - if block == nil { - return nil, errors.New("failed to decode PEM") - } - return x509.ParseCertificate(block.Bytes) -}