Ssh Fixes and Improvements (#293)

* feat: parse multiple identity files in ssh

While this does not make it possible to discover multiple identity files
in every case, it does make it possible to parse them individually and
check for user input if it's required for each one.

* chore: remove unnecessary print in updatebus.go

* chore: remove unnecessary print in sshclient.go

* chore: remove old publicKey auth check

With the new callback in place, we no longer need this, so it has been
removed.

* refactor: move logic for wave and config options

The logic for making decisions between details made available from wave
and details made available from ssh_config was spread out. This change
condenses it into one function for gathering those details and one for
picking between them.

It also adds a few new keywords but the logic for those hasn't been
implemented yet.

* feat: allow attempting auth methods in any order

While waveterm does not provide the control over which order to attempt
yet, it is possible to provide that information in the ssh_config. This
change allows that order to take precedence in a case where it is set.

* feat: add batch mode support

BatchMode turns off user input to enter passwords for ssh. Because we
save passwords, we can still attempt these methods but we disable the
user interactive prompts in this case.

* fix: fix auth ordering and identity files

The last few commits introduced a few bugs that are fixed here. The
first is that the auth ordering is parsed as a single string and not a
list. This is fixed by manually splitting the string into a list. The
second is that the copy of identity files was not long enough to copy
the contents of the original. This is now updated to use the length of
the original in its construction.

* deactivate timer while connecting to new ssh

The new ssh setup handles timers differently from the old one due to the
possibility of asking for user input multiple times. This limited the
user input to entirely be done within 15 seconds. This removes that
restriction which will allow those timers to increase. It does not
impact the legacy ssh systems or the local connections on the new
system.

* merge branch 'main' into 'ssh--auth-control'

This was mostly straightforward, but it appears that a previous commit
to main broke the user input modals by deleting a function. This adds
that back in addition to the merge.

* fix: allow 60 second timeouts for ssh inputs

With the previous change, it is now possible to extend the timeout for
manual inputs. 60 seconds should be a reasonable starting point.

* fix: change size of dummy key to 2048

This fixes the CodeQL scan issue for using a weak key.
This commit is contained in:
Sylvie Crowe 2024-02-15 15:58:50 -08:00 committed by GitHub
parent 766b7b90ce
commit 158378a7ad
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 280 additions and 85 deletions

View File

@ -259,8 +259,10 @@ class ViewRemoteConnDetailModal extends React.Component<{}, {}> {
message = "Connected and ready to run commands.";
} else if (remote.status == "connecting") {
message = remote.waitingforpassword ? "Connecting, waiting for user-input..." : "Connecting...";
let connectTimeout = remote.connecttimeout ?? 0;
message = message + " (" + connectTimeout + "s)";
if (remote.countdownactive) {
let connectTimeout = remote.connecttimeout ?? 0;
message = message + " (" + connectTimeout + "s)";
}
} else if (remote.status == "disconnected") {
message = "Disconnected";
} else if (remote.status == "error") {

View File

@ -601,6 +601,9 @@ func (msh *MShellProc) GetRemoteRuntimeState() RemoteRuntimeState {
if state.ConnectTimeout < 0 {
state.ConnectTimeout = 0
}
state.CountdownActive = true
} else {
state.CountdownActive = false
}
}
vars := msh.Remote.StateVars
@ -1318,23 +1321,22 @@ func (NewLauncher) Launch(msh *MShellProc, interactive bool) {
if remoteCopy.ConnectMode != sstore.ConnectModeManual && remoteCopy.SSHOpts.SSHPassword == "" && !interactive {
sshOpts.BatchMode = true
}
makeClientCtx, makeClientCancelFn := context.WithCancel(context.Background())
defer makeClientCancelFn()
msh.WithLock(func() {
msh.Err = nil
msh.ErrNoInitPk = false
msh.Status = StatusConnecting
msh.MakeClientCancelFn = makeClientCancelFn
deadlineTime := time.Now().Add(RemoteConnectTimeout)
msh.MakeClientDeadline = &deadlineTime
go msh.NotifyRemoteUpdate()
})
go msh.watchClientDeadlineTime()
var cmdStr string
var cproc *shexec.ClientProc
var initPk *packet.InitPacketType
if sshOpts.SSHHost == "" && remoteCopy.Local {
cmdStr, err = MakeLocalMShellCommandStr(remoteCopy.IsSudo())
makeClientCtx, makeClientCancelFn := context.WithCancel(context.Background())
defer makeClientCancelFn()
msh.WithLock(func() {
msh.Err = nil
msh.ErrNoInitPk = false
msh.Status = StatusConnecting
msh.MakeClientCancelFn = makeClientCancelFn
deadlineTime := time.Now().Add(RemoteConnectTimeout)
msh.MakeClientDeadline = &deadlineTime
go msh.NotifyRemoteUpdate()
})
go msh.watchClientDeadlineTime()
cmdStr, err := MakeLocalMShellCommandStr(remoteCopy.IsSudo())
if err != nil {
msh.WriteToPtyBuffer("*error, cannot find local mshell binary: %v\n", err)
return
@ -1359,6 +1361,13 @@ func (NewLauncher) Launch(msh *MShellProc, interactive bool) {
}
cproc, initPk, err = shexec.MakeClientProc(makeClientCtx, shexec.CmdWrap{Cmd: ecmd})
} else {
msh.WithLock(func() {
msh.Err = nil
msh.ErrNoInitPk = false
msh.Status = StatusConnecting
msh.MakeClientDeadline = nil
go msh.NotifyRemoteUpdate()
})
var client *ssh.Client
client, err = ConnectToClient(remoteCopy.SSHOpts)
if err != nil {
@ -1375,6 +1384,15 @@ func (NewLauncher) Launch(msh *MShellProc, interactive bool) {
msh.setErrorStatus(statusErr)
return
}
makeClientCtx, makeClientCancelFn := context.WithCancel(context.Background())
defer makeClientCancelFn()
msh.WithLock(func() {
msh.MakeClientCancelFn = makeClientCancelFn
deadlineTime := time.Now().Add(RemoteConnectTimeout)
msh.MakeClientDeadline = &deadlineTime
go msh.NotifyRemoteUpdate()
})
go msh.watchClientDeadlineTime()
cproc, initPk, err = shexec.MakeClientProc(makeClientCtx, shexec.SessionWrap{Session: session, StartCmd: MakeServerRunOnlyCommandStr()})
}
// TODO check if initPk.State is not nil

View File

@ -6,10 +6,11 @@ package remote
import (
"bytes"
"context"
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"encoding/base64"
"fmt"
"log"
"net"
"os"
"os/user"
@ -35,39 +36,94 @@ func (uice UserInputCancelError) Error() string {
return uice.Err.Error()
}
func createPublicKeyAuth(identityFile string, passphrase string) (ssh.Signer, error) {
privateKey, err := os.ReadFile(base.ExpandHomeDir(identityFile))
// This exists to trick the ssh library into continuing to try
// different public keys even when the current key cannot be
// properly parsed
func createDummySigner() ([]ssh.Signer, error) {
dummyKey, err := rsa.GenerateKey(rand.Reader, 2048)
if err != nil {
return nil, fmt.Errorf("failed to read ssh key file. err: %+v", err)
return nil, err
}
signer, err := ssh.ParsePrivateKey(privateKey)
if err == nil {
return signer, err
}
if _, ok := err.(*ssh.PassphraseMissingError); !ok {
return nil, fmt.Errorf("failed to parse private ssh key. err: %+v", err)
dummySigner, err := ssh.NewSignerFromKey(dummyKey)
if err != nil {
return nil, err
}
return []ssh.Signer{dummySigner}, nil
signer, err = ssh.ParsePrivateKeyWithPassphrase(privateKey, []byte(passphrase))
if err == nil {
return signer, err
}
// This is a workaround to only process one identity file at a time,
// even if they have passphrases. It must be combined with retryable
// authentication to work properly
//
// Despite returning an array of signers, we only ever provide one since
// it allows proper user interaction in between attempts
//
// A significant number of errors end up returning dummy values as if
// they were successes. An error in this function prevents any other
// keys from being attempted. But if there's an error because of a dummy
// file, the library can still try again with a new key.
func createPublicKeyCallback(sshKeywords *SshKeywords, passphrase string) func() ([]ssh.Signer, error) {
identityFiles := make([]string, len(sshKeywords.IdentityFile))
copy(identityFiles, sshKeywords.IdentityFile)
identityFilesPtr := &identityFiles
return func() ([]ssh.Signer, error) {
if len(*identityFilesPtr) == 0 {
// skip this key and try with the next
return createDummySigner()
}
identityFile := (*identityFilesPtr)[0]
*identityFilesPtr = (*identityFilesPtr)[1:]
privateKey, err := os.ReadFile(base.ExpandHomeDir(identityFile))
if err != nil {
// skip this key and try with the next
return createDummySigner()
}
signer, err := ssh.ParsePrivateKey(privateKey)
if err == nil {
return []ssh.Signer{signer}, err
}
if _, ok := err.(*ssh.PassphraseMissingError); !ok {
// skip this key and try with the next
return createDummySigner()
}
signer, err = ssh.ParsePrivateKeyWithPassphrase(privateKey, []byte(passphrase))
if err == nil {
return []ssh.Signer{signer}, err
}
if err != x509.IncorrectPasswordError && err.Error() != "bcrypt_pbkdf: empty password" {
// skip this key and try with the next
return createDummySigner()
}
// batch mode deactivates user input
if sshKeywords.BatchMode {
// skip this key and try with the next
return createDummySigner()
}
request := &sstore.UserInputRequestType{
ResponseType: "text",
QueryText: fmt.Sprintf("Enter passphrase for the SSH key: %s", identityFile),
Title: "Publickey Auth + Passphrase",
}
ctx, cancelFn := context.WithTimeout(context.Background(), 60*time.Second)
defer cancelFn()
response, err := sstore.MainBus.GetUserInput(ctx, request)
if err != nil {
// this is an error where we actually do want to stop
// trying keys
return nil, UserInputCancelError{Err: err}
}
signer, err = ssh.ParsePrivateKeyWithPassphrase(privateKey, []byte(response.Text))
if err != nil {
// skip this key and try with the next
return createDummySigner()
}
return []ssh.Signer{signer}, err
}
if err != x509.IncorrectPasswordError && err.Error() != "bcrypt_pbkdf: empty password" {
log.Printf("qwerty: %+v", err)
return nil, fmt.Errorf("failed to parse private ssh key. err: %+v", err)
}
request := &sstore.UserInputRequestType{
ResponseType: "text",
QueryText: fmt.Sprintf("Enter passphrase for the SSH key: %s", identityFile),
Title: "Publickey Auth + Passphrase",
}
ctx, cancelFn := context.WithTimeout(context.Background(), 15*time.Second)
defer cancelFn()
response, err := sstore.MainBus.GetUserInput(ctx, request)
if err != nil {
return nil, UserInputCancelError{Err: err}
}
return ssh.ParsePrivateKeyWithPassphrase(privateKey, []byte(response.Text))
}
func createDefaultPasswordCallbackPrompt(password string) func() (secret string, err error) {
@ -83,7 +139,7 @@ func createInteractivePasswordCallbackPrompt() func() (secret string, err error)
return func() (secret string, err error) {
// limited to 15 seconds for some reason. this should be investigated more
// in the future
ctx, cancelFn := context.WithTimeout(context.Background(), 15*time.Second)
ctx, cancelFn := context.WithTimeout(context.Background(), 60*time.Second)
defer cancelFn()
request := &sstore.UserInputRequestType{
ResponseType: "text",
@ -143,7 +199,7 @@ func createInteractiveKbdInteractiveChallenge() func(name, instruction string, q
func promptChallengeQuestion(question string, echo bool) (answer string, err error) {
// limited to 15 seconds for some reason. this should be investigated more
// in the future
ctx, cancelFn := context.WithTimeout(context.Background(), 15*time.Second)
ctx, cancelFn := context.WithTimeout(context.Background(), 60*time.Second)
defer cancelFn()
request := &sstore.UserInputRequestType{
ResponseType: "text",
@ -410,61 +466,180 @@ func createHostKeyCallback(opts *sstore.SSHOpts) (ssh.HostKeyCallback, error) {
}
func ConnectToClient(opts *sstore.SSHOpts) (*ssh.Client, error) {
ssh_config.ReloadConfigs()
configIdentity, _ := ssh_config.GetStrict(opts.SSHHost, "IdentityFile")
var identityFile string
if opts.SSHIdentity != "" {
identityFile = opts.SSHIdentity
sshConfigKeywords, err := findSshConfigKeywords(opts.SSHHost)
if err != nil {
return nil, err
}
sshKeywords, err := combineSshKeywords(opts, sshConfigKeywords)
if err != nil {
return nil, err
}
publicKeyCallback := ssh.PublicKeysCallback(createPublicKeyCallback(sshKeywords, opts.SSHPassword))
keyboardInteractive := ssh.KeyboardInteractive(createCombinedKbdInteractiveChallenge(opts.SSHPassword))
passwordCallback := ssh.PasswordCallback(createCombinedPasswordCallbackPrompt(opts.SSHPassword))
// batch mode turns off interactive input. this means the number of
// attemtps must drop to 1 with this setup
var attemptsAllowed int
if sshKeywords.BatchMode {
attemptsAllowed = 1
} else {
identityFile = configIdentity
attemptsAllowed = 2
}
// exclude gssapi-with-mic and hostbased until implemented
authMethodMap := map[string]ssh.AuthMethod{
"publickey": ssh.RetryableAuthMethod(publicKeyCallback, len(sshKeywords.IdentityFile)),
"keyboard-interactive": ssh.RetryableAuthMethod(keyboardInteractive, attemptsAllowed),
"password": ssh.RetryableAuthMethod(passwordCallback, attemptsAllowed),
}
authMethodActiveMap := map[string]bool{
"publickey": sshKeywords.PubkeyAuthentication,
"keyboard-interactive": sshKeywords.KbdInteractiveAuthentication,
"password": sshKeywords.PasswordAuthentication,
}
var authMethods []ssh.AuthMethod
for _, authMethodName := range sshKeywords.PreferredAuthentications {
authMethodActive, ok := authMethodActiveMap[authMethodName]
if !ok || !authMethodActive {
continue
}
authMethod, ok := authMethodMap[authMethodName]
if !ok {
continue
}
authMethods = append(authMethods, authMethod)
}
hostKeyCallback, err := createHostKeyCallback(opts)
if err != nil {
return nil, err
}
var authMethods []ssh.AuthMethod
publicKeySigner, err := createPublicKeyAuth(identityFile, opts.SSHPassword)
if err == nil {
authMethods = append(authMethods, ssh.PublicKeys(publicKeySigner))
}
authMethods = append(authMethods, ssh.RetryableAuthMethod(ssh.KeyboardInteractive(createCombinedKbdInteractiveChallenge(opts.SSHPassword)), 2))
authMethods = append(authMethods, ssh.RetryableAuthMethod(ssh.PasswordCallback(createCombinedPasswordCallbackPrompt(opts.SSHPassword)), 2))
configUser, _ := ssh_config.GetStrict(opts.SSHHost, "User")
configHostName, _ := ssh_config.GetStrict(opts.SSHHost, "HostName")
configPort, _ := ssh_config.GetStrict(opts.SSHHost, "Port")
var username string
clientConfig := &ssh.ClientConfig{
User: sshKeywords.User,
Auth: authMethods,
HostKeyCallback: hostKeyCallback,
}
networkAddr := sshKeywords.HostName + ":" + sshKeywords.Port
return ssh.Dial("tcp", networkAddr, clientConfig)
}
type SshKeywords struct {
User string
HostName string
Port string
IdentityFile []string
BatchMode bool
PubkeyAuthentication bool
PasswordAuthentication bool
KbdInteractiveAuthentication bool
PreferredAuthentications []string
}
func combineSshKeywords(opts *sstore.SSHOpts, configKeywords *SshKeywords) (*SshKeywords, error) {
sshKeywords := &SshKeywords{}
if opts.SSHUser != "" {
username = opts.SSHUser
} else if configUser != "" {
username = configUser
sshKeywords.User = opts.SSHUser
} else if configKeywords.User != "" {
sshKeywords.User = configKeywords.User
} else {
user, err := user.Current()
if err != nil {
return nil, fmt.Errorf("failed to get user for ssh: %+v", err)
}
username = user.Username
sshKeywords.User = user.Username
}
var hostName string
if configHostName != "" {
hostName = configHostName
// we have to check the host value because of the weird way
// we store the pattern as the hostname for imported remotes
if configKeywords.HostName != "" {
sshKeywords.HostName = configKeywords.HostName
} else {
hostName = opts.SSHHost
sshKeywords.HostName = opts.SSHHost
}
clientConfig := &ssh.ClientConfig{
User: username,
Auth: authMethods,
HostKeyCallback: hostKeyCallback,
}
var port string
if opts.SSHPort != 0 && opts.SSHPort != 22 {
port = strconv.Itoa(opts.SSHPort)
} else if configPort != "" && configPort != "22" {
port = configPort
sshKeywords.Port = strconv.Itoa(opts.SSHPort)
} else if configKeywords.Port != "" && configKeywords.Port != "22" {
sshKeywords.Port = configKeywords.Port
} else {
port = "22"
sshKeywords.Port = "22"
}
networkAddr := hostName + ":" + port
return ssh.Dial("tcp", networkAddr, clientConfig)
sshKeywords.IdentityFile = []string{opts.SSHIdentity}
sshKeywords.IdentityFile = append(sshKeywords.IdentityFile, configKeywords.IdentityFile...)
// these are not officially supported in the waveterm frontend but can be configured
// in ssh config files
sshKeywords.BatchMode = configKeywords.BatchMode
sshKeywords.PubkeyAuthentication = configKeywords.PubkeyAuthentication
sshKeywords.PasswordAuthentication = configKeywords.PasswordAuthentication
sshKeywords.KbdInteractiveAuthentication = configKeywords.KbdInteractiveAuthentication
sshKeywords.PreferredAuthentications = configKeywords.PreferredAuthentications
return sshKeywords, nil
}
// note that a `var == "yes"` will default to false
// but `var != "no"` will default to true
// when given unexpected strings
func findSshConfigKeywords(hostPattern string) (*SshKeywords, error) {
ssh_config.ReloadConfigs()
sshKeywords := &SshKeywords{}
var err error
sshKeywords.User, err = ssh_config.GetStrict(hostPattern, "User")
if err != nil {
return nil, err
}
sshKeywords.HostName, err = ssh_config.GetStrict(hostPattern, "HostName")
if err != nil {
return nil, err
}
sshKeywords.Port, err = ssh_config.GetStrict(hostPattern, "Port")
if err != nil {
return nil, err
}
sshKeywords.IdentityFile = ssh_config.GetAll(hostPattern, "IdentityFile")
batchModeRaw, err := ssh_config.GetStrict(hostPattern, "BatchMode")
if err != nil {
return nil, err
}
sshKeywords.BatchMode = (strings.ToLower(batchModeRaw) == "yes")
// we currently do not support host-bound or unbound but will use yes when they are selected
pubkeyAuthenticationRaw, err := ssh_config.GetStrict(hostPattern, "PubkeyAuthentication")
if err != nil {
return nil, err
}
sshKeywords.PubkeyAuthentication = (strings.ToLower(pubkeyAuthenticationRaw) != "no")
passwordAuthenticationRaw, err := ssh_config.GetStrict(hostPattern, "PasswordAuthentication")
if err != nil {
return nil, err
}
sshKeywords.PasswordAuthentication = (strings.ToLower(passwordAuthenticationRaw) != "no")
kbdInteractiveAuthenticationRaw, err := ssh_config.GetStrict(hostPattern, "KbdInteractiveAuthentication")
if err != nil {
return nil, err
}
sshKeywords.KbdInteractiveAuthentication = (strings.ToLower(kbdInteractiveAuthenticationRaw) != "no")
// these are parsed as a single string and must be separated
// these are case sensitive in openssh so they are here too
preferredAuthenticationsRaw, err := ssh_config.GetStrict(hostPattern, "PreferredAuthentications")
sshKeywords.PreferredAuthentications = strings.Split(preferredAuthenticationsRaw, ",")
return sshKeywords, nil
}

View File

@ -1004,6 +1004,7 @@ type RemoteRuntimeState struct {
DefaultFeState map[string]string `json:"defaultfestate"`
Status string `json:"status"`
ConnectTimeout int `json:"connecttimeout,omitempty"`
CountdownActive bool `json:"countdownactive"`
ErrorStr string `json:"errorstr,omitempty"`
InstallStatus string `json:"installstatus"`
InstallErrorStr string `json:"installerrorstr,omitempty"`

View File

@ -228,7 +228,6 @@ func (bus *UpdateBus) GetUserInput(ctx context.Context, userInputRequest *UserIn
update := &ModelUpdate{}
AddUpdate(update, *userInputRequest)
bus.SendUpdate(update)
log.Printf("test: %+v", userInputRequest)
var response *scpacket.UserInputResponsePacketType
var err error