Merge commit from fork

* feat: verify rbac on each message and not just during handshake Signed-off-by: pashakostohrys <[email protected]> * feat: verify rbac on each message and not just during handshake Signed-off-by: pashakostohrys <[email protected]> * fix: linter and e2e tests Signed-off-by: pashakostohrys <[email protected]> * fix: linter and e2e tests Signed-off-by: pashakostohrys <[email protected]> --------- Signed-off-by: pashakostohrys <[email protected]>
argoproj · Jul 24, 2024 · e96f32d · e96f32d
1 parent 212d320
commit e96f32d
Show file tree

Hide file tree

Showing 3 changed files with 163 additions and 15 deletions.
diff --git a/server/application/terminal.go b/server/application/terminal.go
@@ -225,7 +225,7 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	fieldLog.Info("terminal session starting")
 
-	session, err := newTerminalSession(w, r, nil, s.sessionManager)
+	session, err := newTerminalSession(ctx, w, r, nil, s.sessionManager, appRBACName, s.enf)
 	if err != nil {
 		http.Error(w, "Failed to start terminal session", http.StatusBadRequest)
 		return

diff --git a/server/application/websocket.go b/server/application/websocket.go
@@ -1,15 +1,20 @@
 package application
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
-	"github.com/argoproj/argo-cd/v2/common"
-	httputil "github.com/argoproj/argo-cd/v2/util/http"
-	util_session "github.com/argoproj/argo-cd/v2/util/session"
 	"net/http"
 	"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"
+	httputil "github.com/argoproj/argo-cd/v2/util/http"
+	util_session "github.com/argoproj/argo-cd/v2/util/session"
+
 	"github.com/gorilla/websocket"
 	log "github.com/sirupsen/logrus"
 	"k8s.io/client-go/tools/remotecommand"
@@ -31,6 +36,7 @@ var upgrader = func() websocket.Upgrader {
 
 // terminalSession implements PtyHandler
 type terminalSession struct {
+	ctx            context.Context
 	wsConn         *websocket.Conn
 	sizeChan       chan remotecommand.TerminalSize
 	doneChan       chan struct{}
@@ -39,6 +45,8 @@ type terminalSession struct {
 	writeLock      sync.Mutex
 	sessionManager *util_session.SessionManager
 	token          *string
+	appRBACName    string
+	enf            *rbac.Enforcer
 }
 
 // getToken get auth token from web socket request
@@ -48,7 +56,7 @@ func getToken(r *http.Request) (string, error) {
 }
 
 // newTerminalSession create terminalSession
-func newTerminalSession(w http.ResponseWriter, r *http.Request, responseHeader http.Header, sessionManager *util_session.SessionManager) (*terminalSession, error) {
+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) {
 	token, err := getToken(r)
 	if err != nil {
 		return nil, err
@@ -59,12 +67,15 @@ func newTerminalSession(w http.ResponseWriter, r *http.Request, responseHeader h
 		return nil, err
 	}
 	session := &terminalSession{
+		ctx:            ctx,
 		wsConn:         conn,
 		tty:            true,
 		sizeChan:       make(chan remotecommand.TerminalSize),
 		doneChan:       make(chan struct{}),
 		sessionManager: sessionManager,
 		token:          &token,
+		appRBACName:    appRBACName,
+		enf:            enf,
 	}
 	return session, nil
 }
@@ -125,6 +136,29 @@ func (t *terminalSession) reconnect() (int, error) {
 	return 0, nil
 }
 
+func (t *terminalSession) validatePermissions(p []byte) (int, error) {
+	permissionDeniedMessage, _ := json.Marshal(TerminalMessage{
+		Operation: "stdout",
+		Data:      "Permission denied",
+	})
+	if err := t.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)
+		}
+		return copy(p, EndOfTransmission), permissionDeniedErr
+	}
+
+	if err := t.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)
+		}
+		return copy(p, EndOfTransmission), permissionDeniedErr
+	}
+	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) {
 	// check if token still valid
@@ -135,6 +169,12 @@ func (t *terminalSession) Read(p []byte) (int, error) {
 		return t.reconnect()
 	}
 
+	// validate permissions
+	code, err := t.validatePermissions(p)
+	if err != nil {
+		return code, err
+	}
+
 	t.readLock.Lock()
 	_, message, err := t.wsConn.ReadMessage()
 	t.readLock.Unlock()

diff --git a/server/application/websocket_test.go b/server/application/websocket_test.go
@@ -1,36 +1,77 @@
 package application
 
 import (
+	"context"
 	"encoding/json"
-	"github.com/gorilla/websocket"
-	"github.com/stretchr/testify/assert"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
+
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/fake"
+
+	"github.com/argoproj/argo-cd/v2/common"
+	"github.com/argoproj/argo-cd/v2/util/assets"
+	"github.com/argoproj/argo-cd/v2/util/rbac"
+
+	"github.com/golang-jwt/jwt/v4"
+	"github.com/gorilla/websocket"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
-func reconnect(w http.ResponseWriter, r *http.Request) {
-	var upgrader = websocket.Upgrader{}
+func newTestTerminalSession(w http.ResponseWriter, r *http.Request) terminalSession {
+	upgrader := websocket.Upgrader{}
 	c, err := upgrader.Upgrade(w, r, nil)
 	if err != nil {
-		return
+		return terminalSession{}
 	}
 
-	ts := terminalSession{wsConn: c}
+	return terminalSession{wsConn: c}
+}
+
+func newEnforcer() *rbac.Enforcer {
+	additionalConfig := make(map[string]string, 0)
+	kubeclientset := fake.NewSimpleClientset(&v1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: testNamespace,
+			Name:      "argocd-cm",
+			Labels: map[string]string{
+				"app.kubernetes.io/part-of": "argocd",
+			},
+		},
+		Data: additionalConfig,
+	}, &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "argocd-secret",
+			Namespace: testNamespace,
+		},
+		Data: map[string][]byte{
+			"admin.password":   []byte("test"),
+			"server.secretkey": []byte("test"),
+		},
+	})
+
+	enforcer := rbac.NewEnforcer(kubeclientset, testNamespace, common.ArgoCDRBACConfigMapName, nil)
+	return enforcer
+}
+
+func reconnect(w http.ResponseWriter, r *http.Request) {
+	ts := newTestTerminalSession(w, r)
 	_, _ = ts.reconnect()
 }
 
 func TestReconnect(t *testing.T) {
-
 	s := httptest.NewServer(http.HandlerFunc(reconnect))
 	defer s.Close()
 
 	u := "ws" + strings.TrimPrefix(s.URL, "http")
 
 	// Connect to the server
 	ws, _, err := websocket.DefaultDialer.Dial(u, nil)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	defer ws.Close()
 
@@ -40,7 +81,74 @@ func TestReconnect(t *testing.T) {
 
 	err = json.Unmarshal(p, &message)
 
-	assert.NoError(t, err)
-	assert.Equal(t, message.Data, ReconnectMessage)
+	require.NoError(t, err)
+	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))
+	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()
+}
+
+func TestValidateWithoutPermissions(t *testing.T) {
+	validate := func(w http.ResponseWriter, r *http.Request) {
+		enf := newEnforcer()
+		_ = enf.SetBuiltinPolicy(assets.BuiltinPolicyCSV)
+		enf.SetDefaultRole("role:test")
+		enf.SetClaimsEnforcerFunc(func(claims jwt.Claims, rvals ...interface{}) bool {
+			return false
+		})
+		ts := newTestTerminalSession(w, r)
+		ts.enf = enf
+		ts.appRBACName = "test"
+		// nolint:staticcheck
+		ts.ctx = context.WithValue(context.Background(), "claims", &jwt.MapClaims{"groups": []string{"test"}})
+		_, err := ts.validatePermissions([]byte{})
+		require.Error(t, err)
+		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)
 }