Skip to content

Commit

Permalink
lib/ssh: use UserKnownHostFile from configuration in NewClientInterac…
Browse files Browse the repository at this point in the history
…tive

Previously, the ssh Client always use InsecureIgnoreHostKey in
HostKeyCallback.
This may post security issue, like man-in-the-middle attack, since we
did not check the server host key with one of key that known by client
from UserKnownHostFile (for example ~/.ssh/known_hosts).

This changes use the SSH section UserKnownHostFile from configuration
(default to ~/.ssh/known_hosts) to check if the server host key is
valid.
The NewClientInteractive will return an error, "key is unknown", if host
key not exist in UserKnownHostFile or "key is mismatch" if host key
not match with one registered in UserKnownHostFile.

This changes depends on patch of golang.org/x/crypto [1] that has not
reviewed yet, so we need to replace it with one that contains the patch.

[1] https://go-review.googlesource.com/c/crypto/+/523555
  • Loading branch information
shuLhan committed Aug 28, 2023
1 parent 7be4871 commit e9de137
Show file tree
Hide file tree
Showing 6 changed files with 167 additions and 10 deletions.
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ require (
golang.org/x/sys v0.11.0
golang.org/x/term v0.11.0
)

replace golang.org/x/crypto => git.sr.ht/~shulhan/go-x-crypto v0.0.0-20230828162712-e41767625461
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
golang.org/x/crypto v0.12.0 h1:tFM/ta59kqch6LlvYnPa0yx5a83cL2nHflFhYKvv9Yk=
golang.org/x/crypto v0.12.0/go.mod h1:NF0Gs7EO5K4qLn+Ylc+fih8BSTeIjAP05siRnAh98yw=
git.sr.ht/~shulhan/go-x-crypto v0.0.0-20230828162712-e41767625461 h1:jtYbq5NbnQ70R0pipz9quujMv0jXUcRllxUGeCRBIqg=
git.sr.ht/~shulhan/go-x-crypto v0.0.0-20230828162712-e41767625461/go.mod h1:NF0Gs7EO5K4qLn+Ylc+fih8BSTeIjAP05siRnAh98yw=
golang.org/x/net v0.12.0 h1:cfawfvKITfUsFCeJIHJrbSxpeu/E81khclypR0GVT50=
golang.org/x/net v0.12.0/go.mod h1:zEVYFnQC7m/vmpQFELhcD1EWkZlX69l4oqgmer6hfKA=
golang.org/x/sys v0.11.0 h1:eG7RXZHdqOJ1i+0lgLgCpSXAp6M3LYlAo6osgSi0xOM=
Expand Down
106 changes: 98 additions & 8 deletions lib/ssh/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package ssh

import (
"errors"
"fmt"
"io"
"log"
Expand All @@ -14,6 +15,7 @@ import (

"golang.org/x/crypto/ssh"
"golang.org/x/crypto/ssh/agent"
"golang.org/x/crypto/ssh/knownhosts"

libos "github.com/shuLhan/share/lib/os"
"github.com/shuLhan/share/lib/ssh/config"
Expand All @@ -26,11 +28,15 @@ type Client struct {
*ssh.Client
config *ssh.ClientConfig

configHostKeyCallback ssh.HostKeyCallback

cfg *config.Section
stdout io.Writer
stderr io.Writer

remoteAddr string

listKnownHosts []string
}

// NewClientInteractive create a new SSH connection using predefined
Expand All @@ -42,6 +48,15 @@ type Client struct {
//
// If the IdentityFile is encrypted, it will prompt for passphrase in
// terminal.
//
// The following section keys are recognized and implemented by Client,
// - Hostname
// - IdentityAgent
// - IdentityFile
// - Port
// - User
// - UserKnownHostsFile, setting this to "none" will set HostKeyCallback
// to [ssh.InsecureIgnoreHostKey].
func NewClientInteractive(cfg *config.Section) (cl *Client, err error) {
if cfg == nil {
return nil, nil
Expand All @@ -58,15 +73,19 @@ func NewClientInteractive(cfg *config.Section) (cl *Client, err error) {
cl = &Client{
sysEnvs: libos.Environments(),
config: &ssh.ClientConfig{
User: cfg.User(),
HostKeyCallback: ssh.InsecureIgnoreHostKey(),
User: cfg.User(),
},
cfg: cfg,
stdout: os.Stdout,
stderr: os.Stderr,
remoteAddr: fmt.Sprintf(`%s:%s`, cfg.Hostname(), cfg.Port()),
}

err = cl.setConfigHostKeyCallback()
if err != nil {
return nil, fmt.Errorf(`%s: %w`, logp, err)
}

var sshAgentSockPath = cfg.IdentityAgent()
if len(sshAgentSockPath) > 0 {
var sshAgentSock net.Conn
Expand All @@ -83,11 +102,19 @@ func NewClientInteractive(cfg *config.Section) (cl *Client, err error) {
return nil, fmt.Errorf(`%s: %w`, logp, err)
}

signer = cl.dialWithSigners(signers)
signer, err = cl.dialWithSigners(signers)
if signer != nil {
// Client connected with one of the key in agent.
return cl, nil
}

var errKey *knownhosts.KeyError
if errors.As(err, &errKey) {
// Host key is either unknown or mismatch with one
// of known_hosts files, so no need to continue with
// dialWithPrivateKeys.
return nil, fmt.Errorf(`%s: %w`, logp, err)
}
}

if len(cfg.IdentityFile) == 0 {
Expand All @@ -102,24 +129,83 @@ func NewClientInteractive(cfg *config.Section) (cl *Client, err error) {
return cl, nil
}

// setConfigHostKeyCallback set the config.HostKeyCallback based on the
// UserKnownHostsFile in the Section.
// If one of the UserKnownHostsFile set to "none" it will use
// [ssh.InsecureIgnoreHostKey].
func (cl *Client) setConfigHostKeyCallback() (err error) {
var (
logp = `setConfigHostKeyCallback`
userKnownHosts = cl.cfg.UserKnownHostsFile()

knownHosts string
)

for _, knownHosts = range userKnownHosts {
if knownHosts == config.ValueNone {
// If one of the UserKnownHosts set to "none" always
// accept the remote hosts.
cl.config.HostKeyCallback = ssh.InsecureIgnoreHostKey()
return nil
}

knownHosts, err = libos.PathUnfold(knownHosts)
if err != nil {
return fmt.Errorf(`%s: %s: %w`, logp, knownHosts, err)
}

_, err = os.Stat(knownHosts)
if err == nil {
// Add the user known hosts file only if its exist.
cl.listKnownHosts = append(cl.listKnownHosts, knownHosts)
}
}

cl.config.HostKeyCallback, err = knownhosts.New(cl.listKnownHosts...)
if err != nil {
return fmt.Errorf(`%s: %w`, logp, err)
}

return nil
}

// dialError return the error with clear information when the host key is
// missing or mismatch from known_hosts files.
func (cl *Client) dialError(logp string, errDial error) (err error) {
var (
errKey *knownhosts.KeyError
)
if errors.As(errDial, &errKey) {
if len(errKey.Want) == 0 {
err = fmt.Errorf(`%s: %w: server host key is missing from %+v`, logp, errDial, cl.listKnownHosts)
} else {
err = fmt.Errorf(`%s: %w: server host key mismatch in %+v`, logp, errDial, cl.listKnownHosts)
}
} else {
err = fmt.Errorf(`%s: %w`, logp, errDial)
}
return err
}

// dialWithSigners connect to the remote machine using AuthMethod PublicKeys
// using each of signer in the list.
// On success it will return the signer that can connect to remote address.
func (cl *Client) dialWithSigners(signers []ssh.Signer) (signer ssh.Signer) {
func (cl *Client) dialWithSigners(signers []ssh.Signer) (signer ssh.Signer, err error) {
if len(signers) == 0 {
return nil
return nil, nil
}
var err error
var logp = `dialWithSigners`
for _, signer = range signers {
cl.config.Auth = []ssh.AuthMethod{
ssh.PublicKeys(signer),
}
cl.Client, err = ssh.Dial(`tcp`, cl.remoteAddr, cl.config)
if err == nil {
return signer
return signer, nil
}
err = cl.dialError(logp, err)
}
return nil
return nil, err
}

// dialWithPrivateKeys connect to the remote machine using each of the
Expand Down Expand Up @@ -159,6 +245,10 @@ func (cl *Client) dialWithPrivateKeys(sshAgent agent.ExtendedAgent) (err error)
if err == nil {
break
}
err = cl.dialError(logp, err)
}
if err != nil {
return err
}
if cl.Client == nil {
// None of the private key can connect to remote address.
Expand Down
58 changes: 58 additions & 0 deletions lib/ssh/client_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright 2023, Shulhan <[email protected]>. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package ssh

import (
"os"
"path/filepath"
"testing"

"github.com/shuLhan/share/lib/ssh/config"
)

// TestNewClient_KeyError test SSH to server with host key does not exist in
// known_hosts database.
func TestNewClient_KeyError_notExist(t *testing.T) {
t.Skip(`Require active SSH server`)

var (
section = config.NewSection(`localhost`)

wd string
pathFile string
err error
)

wd, err = os.Getwd()
if err != nil {
t.Fatal(err)
}

err = section.Set(config.KeyUser, `ms`)
if err != nil {
t.Fatal(err)
}
err = section.Set(config.KeyHostname, `localhost`)
if err != nil {
t.Fatal(err)
}

pathFile = filepath.Join(wd, `testdata/localhost/known_hosts_empty`)
err = section.Set(config.KeyUserKnownHostsFile, pathFile)
if err != nil {
t.Fatal(err)
}

pathFile = filepath.Join(wd, `testdata/localhost/client.key`)
err = section.Set(config.KeyIdentityFile, pathFile)
if err != nil {
t.Fatal(err)
}

_, err = NewClientInteractive(section)
if err != nil {
t.Fatal(err)
}
}
7 changes: 7 additions & 0 deletions lib/ssh/testdata/localhost/client.key
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
QyNTUxOQAAACA0CnQ54UzXoKW709LIYSxFLiVf+ibVbxsB8sCmGQBDiQAAAJDOW1pGzlta
RgAAAAtzc2gtZWQyNTUxOQAAACA0CnQ54UzXoKW709LIYSxFLiVf+ibVbxsB8sCmGQBDiQ
AAAEDkCGqgWIckW9eebw+fGj6m4cGrzc+qUSPxBjFAsPDHxjQKdDnhTNegpbvT0shhLEUu
JV/6JtVvGwHywKYZAEOJAAAAC2F3d2FuQGxvY2FsAQI=
-----END OPENSSH PRIVATE KEY-----
Empty file.

0 comments on commit e9de137

Please sign in to comment.