fix: Exec with disable auth not logged in #17114 (#19529)

* initial with tests Signed-off-by: reggie-k <[email protected]> * refactor Signed-off-by: reggie-k <[email protected]> * cleanup Signed-off-by: reggie-k <[email protected]> * made TerminalOpts public and lint Signed-off-by: reggie-k <[email protected]> * pass reference Signed-off-by: reggie-k <[email protected]> --------- Signed-off-by: reggie-k <[email protected]> Co-authored-by: pasha-codefresh <[email protected]>
2026-10-07 08:44:50 +00:00 · 2024-08-15 10:48:45 +00:00
parent 8468a519a9
commit 4ac612bed3
4 changed files with 86 additions and 59 deletions
@@ -33,29 +33,32 @@ import (
 type terminalHandler struct {
 	appLister         applisters.ApplicationLister
 	db                db.ArgoDB
-	enf               *rbac.Enforcer
 	cache             *servercache.Cache
 	appResourceTreeFn func(ctx context.Context, app *appv1.Application) (*appv1.ApplicationTree, error)
 	allowedShells     []string
 	namespace         string
 	enabledNamespaces []string
 	sessionManager    *util_session.SessionManager
+	terminalOptions   *TerminalOptions
+}
+
+type TerminalOptions struct {
+	DisableAuth bool
+	Enf         *rbac.Enforcer
 }

 // NewHandler returns a new terminal handler.
-func NewHandler(appLister applisters.ApplicationLister, namespace string, enabledNamespaces []string, db db.ArgoDB, enf *rbac.Enforcer, cache *servercache.Cache,
-	appResourceTree AppResourceTreeFn, allowedShells []string, sessionManager *util_session.SessionManager,
-) *terminalHandler {
+func NewHandler(appLister applisters.ApplicationLister, namespace string, enabledNamespaces []string, db db.ArgoDB, cache *servercache.Cache, appResourceTree AppResourceTreeFn, allowedShells []string, sessionManager *sessionmgr.SessionManager, terminalOptions *TerminalOptions) *terminalHandler {
 	return &terminalHandler{
 		appLister:         appLister,
 		db:                db,
-		enf:               enf,
 		cache:             cache,
 		appResourceTreeFn: appResourceTree,
 		allowedShells:     allowedShells,
 		namespace:         namespace,
 		enabledNamespaces: enabledNamespaces,
 		sessionManager:    sessionManager,
+		terminalOptions:   terminalOptions,
 	}
 }

@@ -146,12 +149,12 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()

 	appRBACName := security.RBACName(s.namespace, project, appNamespace, app)
-	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, appRBACName); err != nil {
+	if err := s.terminalOptions.Enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, appRBACName); err != nil {
 		http.Error(w, err.Error(), http.StatusUnauthorized)
 		return
 	}

-	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceExec, rbacpolicy.ActionCreate, appRBACName); err != nil {
+	if err := s.terminalOptions.Enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceExec, rbacpolicy.ActionCreate, appRBACName); err != nil {
 		http.Error(w, err.Error(), http.StatusUnauthorized)
 		return
 	}
@@ -229,7 +232,7 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {

 	fieldLog.Info("terminal session starting")

-	session, err := newTerminalSession(ctx, w, r, nil, s.sessionManager, appRBACName, s.enf)
+	session, err := newTerminalSession(ctx, w, r, nil, s.sessionManager, appRBACName, s.terminalOptions)
 	if err != nil {
 		http.Error(w, "Failed to start terminal session", http.StatusBadRequest)
 		return
@@ -8,10 +8,8 @@ import (
 	"sync"
 	"time"

-	"github.com/argoproj/argo-cd/v2/server/rbacpolicy"
-	"github.com/argoproj/argo-cd/v2/util/rbac"
-
 	"github.com/argoproj/argo-cd/v2/common"
+	"github.com/argoproj/argo-cd/v2/server/rbacpolicy"
 	httputil "github.com/argoproj/argo-cd/v2/util/http"
 	util_session "github.com/argoproj/argo-cd/v2/util/session"

@@ -46,7 +44,7 @@ type terminalSession struct {
 	sessionManager *util_session.SessionManager
 	token          *string
 	appRBACName    string
-	enf            *rbac.Enforcer
+	terminalOpts   *TerminalOptions
 }

 // getToken get auth token from web socket request
@@ -56,7 +54,7 @@ func getToken(r *http.Request) (string, error) {
 }

 // newTerminalSession create terminalSession
-func newTerminalSession(ctx context.Context, w http.ResponseWriter, r *http.Request, responseHeader http.Header, sessionManager *util_session.SessionManager, appRBACName string, enf *rbac.Enforcer) (*terminalSession, error) {
+func newTerminalSession(ctx context.Context, w http.ResponseWriter, r *http.Request, responseHeader http.Header, sessionManager *util_session.SessionManager, appRBACName string, terminalOpts *TerminalOptions) (*terminalSession, error) {
 	token, err := getToken(r)
 	if err != nil {
 		return nil, err
@@ -75,7 +73,7 @@ func newTerminalSession(ctx context.Context, w http.ResponseWriter, r *http.Requ
 		sessionManager: sessionManager,
 		token:          &token,
 		appRBACName:    appRBACName,
-		enf:            enf,
+		terminalOpts:   terminalOpts,
 	}
 	return session, nil
 }
@@ -141,7 +139,7 @@ func (t *terminalSession) validatePermissions(p []byte) (int, error) {
 		Operation: "stdout",
 		Data:      "Permission denied",
 	})
-	if err := t.enf.EnforceErr(t.ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, t.appRBACName); err != nil {
+	if err := t.terminalOpts.Enf.EnforceErr(t.ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, t.appRBACName); err != nil {
 		err = t.wsConn.WriteMessage(websocket.TextMessage, permissionDeniedMessage)
 		if err != nil {
 			log.Errorf("permission denied message err: %v", err)
@@ -149,7 +147,7 @@ func (t *terminalSession) validatePermissions(p []byte) (int, error) {
 		return copy(p, EndOfTransmission), permissionDeniedErr
 	}

-	if err := t.enf.EnforceErr(t.ctx.Value("claims"), rbacpolicy.ResourceExec, rbacpolicy.ActionCreate, t.appRBACName); err != nil {
+	if err := t.terminalOpts.Enf.EnforceErr(t.ctx.Value("claims"), rbacpolicy.ResourceExec, rbacpolicy.ActionCreate, t.appRBACName); err != nil {
 		err = t.wsConn.WriteMessage(websocket.TextMessage, permissionDeniedMessage)
 		if err != nil {
 			log.Errorf("permission denied message err: %v", err)
@@ -159,8 +157,12 @@ func (t *terminalSession) validatePermissions(p []byte) (int, error) {
 	return 0, nil
 }

-// Read called in a loop from remotecommand as long as the process is running
-func (t *terminalSession) Read(p []byte) (int, error) {
+func (t *terminalSession) performValidationsAndReconnect(p []byte) (int, error) {
+	// In disable auth mode, no point verifying the token or validating permissions
+	if t.terminalOpts.DisableAuth {
+		return 0, nil
+	}
+
 	// check if token still valid
 	_, newToken, err := t.sessionManager.VerifyToken(*t.token)
 	// err in case if token is revoked, newToken in case if refresh happened
@@ -168,13 +170,21 @@ func (t *terminalSession) Read(p []byte) (int, error) {
 		// need to send reconnect code in case if token was refreshed
 		return t.reconnect()
 	}
-
-	// validate permissions
 	code, err := t.validatePermissions(p)
 	if err != nil {
 		return code, err
 	}

+	return 0, nil
+}
+
+// Read called in a loop from remotecommand as long as the process is running
+func (t *terminalSession) Read(p []byte) (int, error) {
+	code, err := t.performValidationsAndReconnect(p)
+	if err != nil {
+		return code, err
+	}
+
 	t.readLock.Lock()
 	_, message, err := t.wsConn.ReadMessage()
 	t.readLock.Unlock()
@@ -85,24 +85,8 @@ func TestReconnect(t *testing.T) {
 	assert.Equal(t, ReconnectMessage, message.Data)
 }

-func TestValidateWithAdminPermissions(t *testing.T) {
-	validate := func(w http.ResponseWriter, r *http.Request) {
-		enf := newEnforcer()
-		_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
-		enf.SetDefaultRole("role:admin")
-		enf.SetClaimsEnforcerFunc(func(claims jwt.Claims, rvals ...interface{}) bool {
-			return true
-		})
-		ts := newTestTerminalSession(w, r)
-		ts.enf = enf
-		ts.appRBACName = "test"
-		// nolint:staticcheck
-		ts.ctx = context.WithValue(context.Background(), "claims", &jwt.MapClaims{"groups": []string{"admin"}})
-		_, err := ts.validatePermissions([]byte{})
-		require.NoError(t, err)
-	}
-
-	s := httptest.NewServer(http.HandlerFunc(validate))
+func testServerConnection(t *testing.T, testFunc func(w http.ResponseWriter, r *http.Request), expectPermissionDenied bool) {
+	s := httptest.NewServer(http.HandlerFunc(testFunc))
 	defer s.Close()

 	u := "ws" + strings.TrimPrefix(s.URL, "http")
@@ -112,6 +96,52 @@ func TestValidateWithAdminPermissions(t *testing.T) {
 	require.NoError(t, err)

 	defer ws.Close()
+	if expectPermissionDenied {
+		_, p, _ := ws.ReadMessage()
+
+		var message TerminalMessage
+
+		err = json.Unmarshal(p, &message)
+
+		require.NoError(t, err)
+		assert.Equal(t, "Permission denied", message.Data)
+	}
+}
+
+func TestVerifyAndReconnectDisableAuthTrue(t *testing.T) {
+	validate := func(w http.ResponseWriter, r *http.Request) {
+		ts := newTestTerminalSession(w, r)
+		// Currently testing only the usecase of disableAuth: true since the disableAuth: false case
+		// requires a valid token to be passed in the request.
+		// Note that running with disableAuth: false will surprisingly succeed as well, because
+		// the underlying token nil pointer dereference is swallowed in a location I didn't find,
+		// or even swallowed by the test framework.
+		ts.terminalOpts = &TerminalOptions{DisableAuth: true}
+		code, err := ts.performValidationsAndReconnect([]byte{})
+		assert.Equal(t, 0, code)
+		require.NoError(t, err)
+	}
+	testServerConnection(t, validate, false)
+}
+
+func TestValidateWithAdminPermissions(t *testing.T) {
+	validate := func(w http.ResponseWriter, r *http.Request) {
+		enf := newEnforcer()
+		_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
+		enf.SetDefaultRole("role:admin")
+		enf.SetClaimsEnforcerFunc(func(claims jwt.Claims, rvals ...interface{}) bool {
+			return true
+		})
+		ts := newTestTerminalSession(w, r)
+		ts.terminalOpts = &TerminalOptions{Enf: enf}
+		ts.appRBACName = "test"
+		// nolint:staticcheck
+		ts.ctx = context.WithValue(context.Background(), "claims", &jwt.MapClaims{"groups": []string{"admin"}})
+		_, err := ts.validatePermissions([]byte{})
+		require.NoError(t, err)
+	}
+
+	testServerConnection(t, validate, false)
 }

 func TestValidateWithoutPermissions(t *testing.T) {
@@ -123,7 +153,7 @@ func TestValidateWithoutPermissions(t *testing.T) {
 			return false
 		})
 		ts := newTestTerminalSession(w, r)
-		ts.enf = enf
+		ts.terminalOpts = &TerminalOptions{Enf: enf}
 		ts.appRBACName = "test"
 		// nolint:staticcheck
 		ts.ctx = context.WithValue(context.Background(), "claims", &jwt.MapClaims{"groups": []string{"test"}})
@@ -132,23 +162,5 @@ func TestValidateWithoutPermissions(t *testing.T) {
 		assert.Equal(t, permissionDeniedErr.Error(), err.Error())
 	}

-	s := httptest.NewServer(http.HandlerFunc(validate))
-	defer s.Close()
-
-	u := "ws" + strings.TrimPrefix(s.URL, "http")
-
-	// Connect to the server
-	ws, _, err := websocket.DefaultDialer.Dial(u, nil)
-	require.NoError(t, err)
-
-	defer ws.Close()
-
-	_, p, _ := ws.ReadMessage()
-
-	var message TerminalMessage
-
-	err = json.Unmarshal(p, &message)
-
-	require.NoError(t, err)
-	assert.Equal(t, "Permission denied", message.Data)
+	testServerConnection(t, validate, true)
 }
@@ -1041,7 +1041,9 @@ func (a *ArgoCDServer) newHTTPServer(ctx context.Context, port int, grpcWebHandl
 	}
 	mux.Handle("/api/", handler)

-	terminal := application.NewHandler(a.appLister, a.Namespace, a.ApplicationNamespaces, a.db, a.enf, a.Cache, appResourceTreeFn, a.settings.ExecShells, a.sessionMgr).
+	terminalOpts := application.TerminalOptions{DisableAuth: a.ArgoCDServerOpts.DisableAuth, Enf: a.enf}
+
+	terminal := application.NewHandler(a.appLister, a.Namespace, a.ApplicationNamespaces, a.db, a.Cache, appResourceTreeFn, a.settings.ExecShells, a.sessionMgr, &terminalOpts).
 		WithFeatureFlagMiddleware(a.settingsMgr.GetSettings)
 	th := util_session.WithAuthMiddleware(a.DisableAuth, a.sessionMgr, terminal)
 	mux.Handle("/terminal", th)