feat: support pod exec terminal logging (argoproj#9385)

* feat: support pod exec terminal logging Signed-off-by: smcavallo <[email protected]> * enhanced validation and logging when resource not found Signed-off-by: smcavallo <[email protected]> * fix lint Signed-off-by: smcavallo <[email protected]> * log warning when pod or container not found Signed-off-by: smcavallo <[email protected]> * go/log-injection fixes Signed-off-by: smcavallo <[email protected]> * log levels and lowercase message Signed-off-by: smcavallo <[email protected]>
pre · May 17, 2022 · 23d9cf2 · 23d9cf2
1 parent 8cd7d47
commit 23d9cf2
Show file tree

Hide file tree

Showing 4 changed files with 159 additions and 7 deletions.
diff --git a/applicationset/generators/cluster.go b/applicationset/generators/cluster.go
@@ -166,7 +166,7 @@ func (g *ClusterGenerator) getSecretsByClusterName(appSetGenerator *argoappsetv1
 
 }
 
-// santize the name in accordance with the below rules
+// sanitize the name in accordance with the below rules
 // 1. contain no more than 253 characters
 // 2. contain only lowercase alphanumeric characters, '-' or '.'
 // 3. start and end with an alphanumeric character

diff --git a/server/application/application.go b/server/application/application.go
@@ -70,7 +70,7 @@ var (
 	watchAPIBufferSize = env.ParseNumFromEnv(argocommon.EnvWatchAPIBufferSize, 1000, 0, math.MaxInt32)
 )
 
-// Server provides a Application service
+// Server provides an Application service
 type Server struct {
 	ns             string
 	kubeclientset  kubernetes.Interface

diff --git a/server/application/terminal.go b/server/application/terminal.go
@@ -4,10 +4,15 @@ import (
 	"context"
 	"io"
 	"net/http"
+	"net/url"
+	"strings"
 
 	"github.com/argoproj/gitops-engine/pkg/utils/kube"
+	log "github.com/sirupsen/logrus"
 	v1 "k8s.io/api/core/v1"
+	apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
@@ -21,6 +26,7 @@ import (
 	"github.com/argoproj/argo-cd/v2/util/argo"
 	"github.com/argoproj/argo-cd/v2/util/db"
 	"github.com/argoproj/argo-cd/v2/util/rbac"
+	sessionmgr "github.com/argoproj/argo-cd/v2/util/session"
 )
 
 type terminalHandler struct {
@@ -54,26 +60,70 @@ func (s *terminalHandler) getApplicationClusterRawConfig(ctx context.Context, a
 	return clst.RawRestConfig(), nil
 }
 
+// isValidPodName checks that a podName is valid
+func isValidPodName(name string) bool {
+	// https://github.com/kubernetes/kubernetes/blob/976a940f4a4e84fe814583848f97b9aafcdb083f/pkg/apis/core/validation/validation.go#L241
+	isValid := apimachineryvalidation.NameIsDNSSubdomain(name, false)
+	return len(isValid) == 0
+}
+
+// isValidNamespaceName checks that a namespace name is valid
+func isValidNamespaceName(name string) bool {
+	// https://github.com/kubernetes/kubernetes/blob/976a940f4a4e84fe814583848f97b9aafcdb083f/pkg/apis/core/validation/validation.go#L262
+	isValid := apimachineryvalidation.ValidateNamespaceName(name, false)
+	return len(isValid) == 0
+}
+
+// isValidContainerName checks that a containerName is valid
+func isValidContainerName(name string) bool {
+	// quick check to ensure we aren't passing along anything unsafe
+	isQualified := validation.IsQualifiedName(name)
+	return len(isQualified) == 0
+}
+
+// GetQueryValue returns a value for a given url key
+// and strips newline to prevent go/log-injection
+func GetQueryValue(q url.Values, key string) string {
+	return strings.Replace(q.Get(key), "\n", "", -1)
+}
+
 func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	q := r.URL.Query()
-	podName := q.Get("pod")
-	container := q.Get("container")
-	app := q.Get("appName")
-	namespace := q.Get("namespace")
-	shell := q.Get("shell")
+	podName := GetQueryValue(q, "pod")
+	container := GetQueryValue(q, "container")
+	app := GetQueryValue(q, "appName")
+	namespace := GetQueryValue(q, "namespace")
+	shell := GetQueryValue(q, "shell")
 
 	if podName == "" || container == "" || app == "" || namespace == "" {
 		http.Error(w, "Missing required parameters", http.StatusBadRequest)
 		return
 	}
 
+	// validate query string parameters to prevent unsafe usage
+	if !isValidNamespaceName(namespace) {
+		http.Error(w, "Namespace name is not valid", http.StatusBadRequest)
+		return
+	}
+	if !isValidPodName(podName) {
+		http.Error(w, "Pod name is not valid", http.StatusBadRequest)
+		return
+	}
+	if !isValidContainerName(container) {
+		http.Error(w, "Container name is not valid", http.StatusBadRequest)
+		return
+	}
+
 	ctx := r.Context()
 	a, err := s.appLister.Get(app)
 	if err != nil {
 		http.Error(w, "Cannot get app", http.StatusBadRequest)
 		return
 	}
 
+	fieldLog := log.WithFields(log.Fields{"userName": sessionmgr.Username(ctx), "container": container,
+		"podName": podName, "namespace": namespace, "cluster": a.Spec.Destination.Name})
+
 	appRBACName := apputil.AppRBACName(*a)
 	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, appRBACName); err != nil {
 		http.Error(w, err.Error(), http.StatusUnauthorized)
@@ -111,6 +161,7 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	pod, err := kubeClientset.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{})
 	if err != nil {
+		fieldLog.Errorf("error retrieving pod: %s", err)
 		http.Error(w, "Cannot find pod", http.StatusBadRequest)
 		return
 	}
@@ -128,10 +179,13 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 	if !findContainer {
+		fieldLog.Warn("terminal container not found")
 		http.Error(w, "Cannot find container", http.StatusBadRequest)
 		return
 	}
 
+	fieldLog.Info("terminal session starting")
+
 	session, err := newTerminalSession(w, r, nil)
 	if err != nil {
 		http.Error(w, "Failed to start terminal session", http.StatusBadRequest)

diff --git a/server/application/terminal_test.go b/server/application/terminal_test.go
@@ -74,3 +74,101 @@ func TestPodExists(t *testing.T) {
 		})
 	}
 }
+
+func TestIsValidPodName(t *testing.T) {
+	for _, tcase := range []struct {
+		name           string
+		resourceName   string
+		expectedResult bool
+	}{
+		{
+			name:           "valid pod name",
+			resourceName:   "argocd-server-794644486d-r8v9d",
+			expectedResult: true,
+		},
+		{
+			name:           "not valid contains spaces",
+			resourceName:   "kubectl delete pods",
+			expectedResult: false,
+		},
+		{
+			name:           "not valid",
+			resourceName:   "kubectl -n kube-system delete pods --all",
+			expectedResult: false,
+		},
+		{
+			name:           "not valid contains special characters",
+			resourceName:   "delete+*+from+etcd%3b",
+			expectedResult: false,
+		},
+	} {
+		t.Run(tcase.name, func(t *testing.T) {
+			result := isValidPodName(tcase.resourceName)
+			if result != tcase.expectedResult {
+				t.Errorf("Expected result %v, but got %v", tcase.expectedResult, result)
+			}
+		})
+	}
+}
+
+func TestIsValidNamespaceName(t *testing.T) {
+	for _, tcase := range []struct {
+		name           string
+		resourceName   string
+		expectedResult bool
+	}{
+		{
+			name:           "valid pod namespace name",
+			resourceName:   "argocd",
+			expectedResult: true,
+		},
+		{
+			name:           "not valid contains spaces",
+			resourceName:   "kubectl delete ns argocd",
+			expectedResult: false,
+		},
+		{
+			name:           "not valid contains special characters",
+			resourceName:   "delete+*+from+etcd%3b",
+			expectedResult: false,
+		},
+	} {
+		t.Run(tcase.name, func(t *testing.T) {
+			result := isValidNamespaceName(tcase.resourceName)
+			if result != tcase.expectedResult {
+				t.Errorf("Expected result %v, but got %v", tcase.expectedResult, result)
+			}
+		})
+	}
+}
+
+func TestIsValidContainerNameName(t *testing.T) {
+	for _, tcase := range []struct {
+		name           string
+		resourceName   string
+		expectedResult bool
+	}{
+		{
+			name:           "valid container name",
+			resourceName:   "argocd-server",
+			expectedResult: true,
+		},
+		{
+			name:           "not valid contains spaces",
+			resourceName:   "kubectl delete pods",
+			expectedResult: false,
+		},
+		{
+			name:           "not valid contains special characters",
+			resourceName:   "delete+*+from+etcd%3b",
+			expectedResult: false,
+		},
+	} {
+		t.Run(tcase.name, func(t *testing.T) {
+			result := isValidContainerName(tcase.resourceName)
+			if result != tcase.expectedResult {
+				t.Errorf("Expected result %v, but got %v", tcase.expectedResult, result)
+			}
+		})
+	}
+}