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

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

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

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

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

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

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

* apply suggestions

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

docs/operator-manual/argocd-cm.yaml

@@ -233,6 +233,14 @@ data:
   # If omitted, Argo CD injects the app name into the label: 'app.kubernetes.io/instance'
   application.instanceLabelKey: mycompany.com/appname
 
+  # You can change the resource tracking method Argo CD uses by changing the
+  # setting application.resourceTrackingMethod to the desired method.
+  # The following methods are available:
+  # - label            : Uses the application.instanceLabelKey label for tracking
+  # - annotation       : Uses an annotation with additional metadata for tracking instead of the label
+  # - annotation+label : Also uses an annotation for tracking, but additionally labels the resource with the application name
+  application.resourceTrackingMethod: annotation
+
   # disables admin user. Admin is enabled by default
   admin.enabled: "false"
   # add an additional local user with apiKey and login capabilities
@@ -291,6 +299,9 @@ 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"
+
   # oidc.tls.insecure.skip.verify determines whether certificate verification is skipped when verifying tokens with the
   # configured OIDC provider (either external or the bundled Dex instance). Setting this to "true" will cause JWT
   # token verification to pass despite the OIDC provider having an invalid certificate. Only set to "true" if you

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

@@ -808,7 +808,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"`
 	// OIDCTLSInsecureSkipVerify determines whether certificate verification is skipped when verifying tokens with the
 	// configured OIDC provider (either external or the bundled Dex instance). Setting this to `true` will cause JWT
 	// token verification to pass despite the OIDC provider having an invalid certificate. Only set to `true` if you
@@ -413,6 +415,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"
 	// oidcTLSInsecureSkipVerifyKey is the key to configure whether TLS cert verification is skipped for OIDC connections
 	oidcTLSInsecureSkipVerifyKey = "oidc.tls.insecure.skip.verify"
 )
@@ -1280,6 +1284,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.OIDCTLSInsecureSkipVerify = argoCDCM.Data[oidcTLSInsecureSkipVerifyKey] == "true"
 	settings.TrackingMethod = argoCDCM.Data[settingsResourceTrackingMethodKey]
 }