diff --git a/applicationset/generators/cluster.go b/applicationset/generators/cluster.go index 24ed643a57950..cc291e40bed92 100644 --- 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 index 55cfe5e9efb43..e6d4214c2a17d 100644 --- 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 index 17392783f8e74..e171d4d3c2604 100644 --- 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,19 +60,60 @@ 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 { @@ -74,6 +121,9 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { 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 index 3c8f6f9c9f17f..30d89742baf8a 100644 --- 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) + } + }) + } +}