fix: 'unexpected reserved bits' breaking web terminal (#9605) (#9895)

* fix: 'unexpected reserved bits' breaking web terminal (#9605)

Signed-off-by: Michael Crenshaw <[email protected]>

* make things more like they were originally, since the mutex fixes the problem

Signed-off-by: Michael Crenshaw <[email protected]>

* fix typo, don't pass around a pointer when it isn't necessary

Signed-off-by: Michael Crenshaw <[email protected]>

* apply suggestions

Signed-off-by: Michael Crenshaw <[email protected]>

docs/operator-manual/argocd-cm.yaml

@@ -298,3 +298,6 @@ data:
 
   # exec.enabled indicates whether the UI exec feature is enabled. It is disabled by default.
   exec.enabled: "false"
+
+  # exec.shells restricts which shells are allowed for `exec`, and in which order they are attempted
+  exec.shells: "bash,sh,powershell,cmd"

server/application/terminal.go

@@ -33,17 +33,19 @@ type terminalHandler struct {
 	enf               *rbac.Enforcer
 	cache             *servercache.Cache
 	appResourceTreeFn func(ctx context.Context, app *appv1.Application) (*appv1.ApplicationTree, error)
+	allowedShells     []string
 }
 
 // NewHandler returns a new terminal handler.
 func NewHandler(appLister applisters.ApplicationNamespaceLister, db db.ArgoDB, enf *rbac.Enforcer, cache *servercache.Cache,
-	appResourceTree AppResourceTreeFn) *terminalHandler {
+	appResourceTree AppResourceTreeFn, allowedShells []string) *terminalHandler {
 	return &terminalHandler{
 		appLister:         appLister,
 		db:                db,
 		enf:               enf,
 		cache:             cache,
 		appResourceTreeFn: appResourceTree,
+		allowedShells:     allowedShells,
 	}
 }
 
@@ -123,7 +125,7 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "Namespace name is not valid", http.StatusBadRequest)
 		return
 	}
-	shell := q.Get("shell") // No need to validate. Will only buse used if it's in the allow-list.
+	shell := q.Get("shell") // No need to validate. Will only be used if it's in the allow-list.
 
 	ctx := r.Context()
 
@@ -216,14 +218,12 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 	defer session.Done()
 
-	validShells := []string{"bash", "sh", "powershell", "cmd"}
-	if isValidShell(validShells, shell) {
+	if isValidShell(s.allowedShells, shell) {
 		cmd := []string{shell}
 		err = startProcess(kubeClientset, config, namespace, podName, container, cmd, session)
 	} else {
-		// No shell given or it was not valid: try some shells until one succeeds or all fail
-		// FIXME: if the first shell fails then the first keyboard event is lost
-		for _, testShell := range validShells {
+		// No shell given or the given shell was not allowed: try the configured shells until one succeeds or all fail.
+		for _, testShell := range s.allowedShells {
 			cmd := []string{testShell}
 			if err = startProcess(kubeClientset, config, namespace, podName, container, cmd, session); err == nil {
 				break

server/application/websocket.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"sync"
 	"time"
 
 	"github.com/gorilla/websocket"
@@ -26,6 +27,7 @@ type terminalSession struct {
 	sizeChan chan remotecommand.TerminalSize
 	doneChan chan struct{}
 	tty      bool
+	readLock sync.Mutex
 }
 
 // newTerminalSession create terminalSession
@@ -60,7 +62,9 @@ func (t *terminalSession) Next() *remotecommand.TerminalSize {
 
 // Read called in a loop from remotecommand as long as the process is running
 func (t *terminalSession) Read(p []byte) (int, error) {
+	t.readLock.Lock()
 	_, message, err := t.wsConn.ReadMessage()
+	t.readLock.Unlock()
 	if err != nil {
 		log.Errorf("read message err: %v", err)
 		return copy(p, EndOfTransmission), err

server/server.go

@@ -801,7 +801,7 @@ func (a *ArgoCDServer) newHTTPServer(ctx context.Context, port int, grpcWebHandl
 	}
 	mux.Handle("/api/", handler)
 
-	terminalHandler := application.NewHandler(a.appLister, a.db, a.enf, a.Cache, appResourceTreeFn)
+	terminalHandler := application.NewHandler(a.appLister, a.db, a.enf, a.Cache, appResourceTreeFn, a.settings.ExecShells)
 	mux.HandleFunc("/terminal", func(writer http.ResponseWriter, request *http.Request) {
 		argocdSettings, err := a.settingsMgr.GetSettings()
 		if err != nil {

util/settings/settings.go

@@ -99,6 +99,8 @@ type ArgoCDSettings struct {
 	ServerRBACLogEnforceEnable bool `json:"serverRBACLogEnforceEnable"`
 	// ExecEnabled indicates whether the UI exec feature is enabled
 	ExecEnabled bool `json:"execEnabled"`
+	// ExecShells restricts which shells are allowed for `exec` and in which order they are tried
+	ExecShells []string `json:"execShells"`
 	// TrackingMethod defines the resource tracking method to be used
 	TrackingMethod string `json:"application.resourceTrackingMethod,omitempty"`
 }
@@ -408,6 +410,8 @@ const (
 	helmValuesFileSchemesKey = "helm.valuesFileSchemes"
 	// execEnabledKey is the key to configure whether the UI exec feature is enabled
 	execEnabledKey = "exec.enabled"
+	// execShellsKey is the key to configure which shells are allowed for `exec` and in what order they are tried
+	execShellsKey = "exec.shells"
 )
 
 var (
@@ -1273,6 +1277,13 @@ func updateSettingsFromConfigMap(settings *ArgoCDSettings, argoCDCM *apiv1.Confi
 	}
 	settings.InClusterEnabled = argoCDCM.Data[inClusterEnabledKey] != "false"
 	settings.ExecEnabled = argoCDCM.Data[execEnabledKey] == "true"
+	execShells := argoCDCM.Data[execShellsKey]
+	if execShells != "" {
+		settings.ExecShells = strings.Split(execShells, ",")
+	} else {
+		// Fall back to default. If you change this list, also change docs/operator-manual/argocd-cm.yaml.
+		settings.ExecShells = []string{"bash", "sh", "powershell", "cmd"}
+	}
 	settings.TrackingMethod = argoCDCM.Data[settingsResourceTrackingMethodKey]
 }