From 3ba636d6c9e247882798714e3233930441e0a64e Mon Sep 17 00:00:00 2001 From: Ayman Bagabas Date: Mon, 9 Jan 2023 17:34:24 +0300 Subject: [PATCH] fix(ssh): unable to pass a custom HostKeyCallback func Don't overwrite HostKeyCallback if one is provided. Fixes: c35b8082c863 ("plumbing: transport/ssh, auto-populate ClientConfig.HostKeyAlgorithms. Fixes #411") Fixes: https://github.com/go-git/go-git/issues/654 Signed-off-by: Ayman Bagabas --- plumbing/transport/ssh/auth_method.go | 28 ++++---- plumbing/transport/ssh/common.go | 33 ++++----- plumbing/transport/ssh/common_test.go | 79 +++++++++++++++++++--- plumbing/transport/ssh/upload_pack_test.go | 4 ++ 4 files changed, 101 insertions(+), 43 deletions(-) diff --git a/plumbing/transport/ssh/auth_method.go b/plumbing/transport/ssh/auth_method.go index 9d3bcd359..e89ce4ba3 100644 --- a/plumbing/transport/ssh/auth_method.go +++ b/plumbing/transport/ssh/auth_method.go @@ -43,6 +43,7 @@ const ( type KeyboardInteractive struct { User string Challenge ssh.KeyboardInteractiveChallenge + HostKeyCallbackHelper } func (a *KeyboardInteractive) Name() string { @@ -54,18 +55,19 @@ func (a *KeyboardInteractive) String() string { } func (a *KeyboardInteractive) ClientConfig() (*ssh.ClientConfig, error) { - return &ssh.ClientConfig{ + return a.SetHostKeyCallback(&ssh.ClientConfig{ User: a.User, Auth: []ssh.AuthMethod{ a.Challenge, }, - }, nil + }) } // Password implements AuthMethod by using the given password. type Password struct { User string Password string + HostKeyCallbackHelper } func (a *Password) Name() string { @@ -77,10 +79,10 @@ func (a *Password) String() string { } func (a *Password) ClientConfig() (*ssh.ClientConfig, error) { - return &ssh.ClientConfig{ + return a.SetHostKeyCallback(&ssh.ClientConfig{ User: a.User, Auth: []ssh.AuthMethod{ssh.Password(a.Password)}, - }, nil + }) } // PasswordCallback implements AuthMethod by using a callback @@ -88,6 +90,7 @@ func (a *Password) ClientConfig() (*ssh.ClientConfig, error) { type PasswordCallback struct { User string Callback func() (pass string, err error) + HostKeyCallbackHelper } func (a *PasswordCallback) Name() string { @@ -99,16 +102,17 @@ func (a *PasswordCallback) String() string { } func (a *PasswordCallback) ClientConfig() (*ssh.ClientConfig, error) { - return &ssh.ClientConfig{ + return a.SetHostKeyCallback(&ssh.ClientConfig{ User: a.User, Auth: []ssh.AuthMethod{ssh.PasswordCallback(a.Callback)}, - }, nil + }) } // PublicKeys implements AuthMethod by using the given key pairs. type PublicKeys struct { User string Signer ssh.Signer + HostKeyCallbackHelper } // NewPublicKeys returns a PublicKeys from a PEM encoded private key. An @@ -147,10 +151,10 @@ func (a *PublicKeys) String() string { } func (a *PublicKeys) ClientConfig() (*ssh.ClientConfig, error) { - return &ssh.ClientConfig{ + return a.SetHostKeyCallback(&ssh.ClientConfig{ User: a.User, Auth: []ssh.AuthMethod{ssh.PublicKeys(a.Signer)}, - }, nil + }) } func username() (string, error) { @@ -173,6 +177,7 @@ func username() (string, error) { type PublicKeysCallback struct { User string Callback func() (signers []ssh.Signer, err error) + HostKeyCallbackHelper } // NewSSHAgentAuth returns a PublicKeysCallback based on a SSH agent, it opens @@ -207,10 +212,10 @@ func (a *PublicKeysCallback) String() string { } func (a *PublicKeysCallback) ClientConfig() (*ssh.ClientConfig, error) { - return &ssh.ClientConfig{ + return a.SetHostKeyCallback(&ssh.ClientConfig{ User: a.User, Auth: []ssh.AuthMethod{ssh.PublicKeysCallback(a.Callback)}, - }, nil + }) } // NewKnownHostsCallback returns ssh.HostKeyCallback based on a file based on a @@ -286,9 +291,6 @@ func filterKnownHostsFiles(files ...string) ([]string, error) { // HostKeyCallbackHelper is a helper that provides common functionality to // configure HostKeyCallback into a ssh.ClientConfig. -// Deprecated in favor of SetConfigHostKeyFields (see common.go) which provides -// a mechanism for also setting ClientConfig.HostKeyAlgorithms for a specific -// host. type HostKeyCallbackHelper struct { // HostKeyCallback is the function type used for verifying server keys. // If nil default callback will be create using NewKnownHostsCallback diff --git a/plumbing/transport/ssh/common.go b/plumbing/transport/ssh/common.go index 4b9ac0797..e06958a3b 100644 --- a/plumbing/transport/ssh/common.go +++ b/plumbing/transport/ssh/common.go @@ -10,6 +10,7 @@ import ( "github.com/go-git/go-git/v5/plumbing/transport" "github.com/go-git/go-git/v5/plumbing/transport/internal/common" + "github.com/skeema/knownhosts" "github.com/kevinburke/ssh_config" "golang.org/x/crypto/ssh" @@ -122,9 +123,18 @@ func (c *command) connect() error { return err } hostWithPort := c.getHostWithPort() - config, err = SetConfigHostKeyFields(config, hostWithPort) - if err != nil { - return err + if config.HostKeyCallback == nil { + kh, err := newKnownHosts() + if err != nil { + return err + } + config.HostKeyCallback = kh.HostKeyCallback() + config.HostKeyAlgorithms = kh.HostKeyAlgorithms(hostWithPort) + } else if len(config.HostKeyAlgorithms) == 0 { + // Set the HostKeyAlgorithms based on HostKeyCallback. + // For background see https://github.com/go-git/go-git/issues/411 as well as + // https://github.com/golang/go/issues/29286 for root cause. + config.HostKeyAlgorithms = knownhosts.HostKeyAlgorithms(config.HostKeyCallback, hostWithPort) } overrideConfig(c.config, config) @@ -167,23 +177,6 @@ func dial(network, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { return ssh.NewClient(c, chans, reqs), nil } -// SetConfigHostKeyFields sets cfg.HostKeyCallback and cfg.HostKeyAlgorithms -// based on OpenSSH known_hosts. cfg is modified in-place. hostWithPort must be -// supplied, since the algorithms will be set based on the known host keys for -// that specific host. Otherwise, golang.org/x/crypto/ssh can return an error -// upon connecting to a host whose *first* key is not known, even though other -// keys (of different types) are known and match properly. -// For background see https://github.com/go-git/go-git/issues/411 as well as -// https://github.com/golang/go/issues/29286 for root cause. -func SetConfigHostKeyFields(cfg *ssh.ClientConfig, hostWithPort string) (*ssh.ClientConfig, error) { - kh, err := newKnownHosts() - if err == nil { - cfg.HostKeyCallback = kh.HostKeyCallback() - cfg.HostKeyAlgorithms = kh.HostKeyAlgorithms(hostWithPort) - } - return cfg, err -} - func (c *command) getHostWithPort() string { if addr, found := c.doGetHostWithPortFromSSHConfig(); found { return addr diff --git a/plumbing/transport/ssh/common_test.go b/plumbing/transport/ssh/common_test.go index 6d634d532..496e82d17 100644 --- a/plumbing/transport/ssh/common_test.go +++ b/plumbing/transport/ssh/common_test.go @@ -5,23 +5,25 @@ import ( "github.com/go-git/go-git/v5/plumbing/transport" + "github.com/gliderlabs/ssh" "github.com/kevinburke/ssh_config" - "golang.org/x/crypto/ssh" + stdssh "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/testdata" . "gopkg.in/check.v1" ) func Test(t *testing.T) { TestingT(t) } func (s *SuiteCommon) TestOverrideConfig(c *C) { - config := &ssh.ClientConfig{ + config := &stdssh.ClientConfig{ User: "foo", - Auth: []ssh.AuthMethod{ - ssh.Password("yourpassword"), + Auth: []stdssh.AuthMethod{ + stdssh.Password("yourpassword"), }, - HostKeyCallback: ssh.FixedHostKey(nil), + HostKeyCallback: stdssh.FixedHostKey(nil), } - target := &ssh.ClientConfig{} + target := &stdssh.ClientConfig{} overrideConfig(config, target) c.Assert(target.User, Equals, "foo") @@ -30,11 +32,11 @@ func (s *SuiteCommon) TestOverrideConfig(c *C) { } func (s *SuiteCommon) TestOverrideConfigKeep(c *C) { - config := &ssh.ClientConfig{ + config := &stdssh.ClientConfig{ User: "foo", } - target := &ssh.ClientConfig{ + target := &stdssh.ClientConfig{ User: "bar", } @@ -93,12 +95,69 @@ func (s *SuiteCommon) TestDefaultSSHConfigWildcard(c *C) { c.Assert(cmd.getHostWithPort(), Equals, "github.com:22") } +func (s *SuiteCommon) TestIgnoreHostKeyCallback(c *C) { + uploadPack := &UploadPackSuite{ + opts: []ssh.Option{ + ssh.HostKeyPEM(testdata.PEMBytes["ed25519"]), + }, + } + uploadPack.SetUpSuite(c) + // Use the default client, which does not have a host key callback + uploadPack.Client = DefaultClient + auth, err := NewPublicKeys("foo", testdata.PEMBytes["rsa"], "") + c.Assert(err, IsNil) + c.Assert(auth, NotNil) + auth.HostKeyCallback = stdssh.InsecureIgnoreHostKey() + ep := uploadPack.newEndpoint(c, "bar.git") + ps, err := uploadPack.Client.NewUploadPackSession(ep, auth) + c.Assert(err, IsNil) + c.Assert(ps, NotNil) +} + +func (s *SuiteCommon) TestFixedHostKeyCallback(c *C) { + hostKey, err := stdssh.ParsePrivateKey(testdata.PEMBytes["ed25519"]) + c.Assert(err, IsNil) + uploadPack := &UploadPackSuite{ + opts: []ssh.Option{ + ssh.HostKeyPEM(testdata.PEMBytes["ed25519"]), + }, + } + uploadPack.SetUpSuite(c) + // Use the default client, which does not have a host key callback + uploadPack.Client = DefaultClient + auth, err := NewPublicKeys("foo", testdata.PEMBytes["rsa"], "") + c.Assert(err, IsNil) + c.Assert(auth, NotNil) + auth.HostKeyCallback = stdssh.FixedHostKey(hostKey.PublicKey()) + ep := uploadPack.newEndpoint(c, "bar.git") + ps, err := uploadPack.Client.NewUploadPackSession(ep, auth) + c.Assert(err, IsNil) + c.Assert(ps, NotNil) +} + +func (s *SuiteCommon) TestFailHostKeyCallback(c *C) { + uploadPack := &UploadPackSuite{ + opts: []ssh.Option{ + ssh.HostKeyPEM(testdata.PEMBytes["ed25519"]), + }, + } + uploadPack.SetUpSuite(c) + // Use the default client, which does not have a host key callback + uploadPack.Client = DefaultClient + auth, err := NewPublicKeys("foo", testdata.PEMBytes["rsa"], "") + c.Assert(err, IsNil) + c.Assert(auth, NotNil) + ep := uploadPack.newEndpoint(c, "bar.git") + _, err = uploadPack.Client.NewUploadPackSession(ep, auth) + c.Assert(err, NotNil) +} + func (s *SuiteCommon) TestIssue70(c *C) { uploadPack := &UploadPackSuite{} uploadPack.SetUpSuite(c) - config := &ssh.ClientConfig{ - HostKeyCallback: ssh.InsecureIgnoreHostKey(), + config := &stdssh.ClientConfig{ + HostKeyCallback: stdssh.InsecureIgnoreHostKey(), } r := &runner{ config: config, diff --git a/plumbing/transport/ssh/upload_pack_test.go b/plumbing/transport/ssh/upload_pack_test.go index e65e04a7a..f172feeda 100644 --- a/plumbing/transport/ssh/upload_pack_test.go +++ b/plumbing/transport/ssh/upload_pack_test.go @@ -25,6 +25,7 @@ import ( type UploadPackSuite struct { test.UploadPackSuite fixtures.Suite + opts []ssh.Option port int base string @@ -57,6 +58,9 @@ func (s *UploadPackSuite) SetUpSuite(c *C) { s.UploadPackSuite.NonExistentEndpoint = s.newEndpoint(c, "non-existent.git") server := &ssh.Server{Handler: handlerSSH} + for _, opt := range s.opts { + opt(server) + } go func() { log.Fatal(server.Serve(l)) }()