From cbc7966441b625e455d37f9910a14251d5dc4320 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Thu, 7 Jul 2022 15:39:34 -0400 Subject: [PATCH] fix: 'unexpected reserved bits' breaking web terminal (#9605) (#9895) * fix: 'unexpected reserved bits' breaking web terminal (#9605) Signed-off-by: Michael Crenshaw * make things more like they were originally, since the mutex fixes the problem Signed-off-by: Michael Crenshaw * fix typo, don't pass around a pointer when it isn't necessary Signed-off-by: Michael Crenshaw * apply suggestions Signed-off-by: Michael Crenshaw --- docs/operator-manual/argocd-cm.yaml | 3 +++ server/application/terminal.go | 14 +++++++------- server/application/websocket.go | 4 ++++ server/server.go | 2 +- util/settings/settings.go | 11 +++++++++++ 5 files changed, 26 insertions(+), 8 deletions(-) diff --git a/docs/operator-manual/argocd-cm.yaml b/docs/operator-manual/argocd-cm.yaml index 02eec7244f81a..a209d43ddc6de 100644 --- a/docs/operator-manual/argocd-cm.yaml +++ b/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" diff --git a/server/application/terminal.go b/server/application/terminal.go index 6cbbab81b0bd9..94645b53feb6c 100644 --- a/server/application/terminal.go +++ b/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 diff --git a/server/application/websocket.go b/server/application/websocket.go index cf2c4db288910..a5a15b05df9f9 100644 --- a/server/application/websocket.go +++ b/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 diff --git a/server/server.go b/server/server.go index b98c7d62e30a6..83b5187f4cdb3 100644 --- a/server/server.go +++ b/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 { diff --git a/util/settings/settings.go b/util/settings/settings.go index 3f82c402f317a..c0beeffbe93ff 100644 --- a/util/settings/settings.go +++ b/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] }