From ef431583f224cc449a2bbf5c569be991f2223c61 Mon Sep 17 00:00:00 2001 From: Michael Freeman Date: Thu, 9 Apr 2026 19:06:37 -0500 Subject: [PATCH 1/3] Handle non-root Kubernetes exec user context --- pkg/runtime/k8s_parity_test.go | 57 +++++++++++++++++++++------------- pkg/runtime/k8s_runtime.go | 51 ++++++++++++++++++++++++------ 2 files changed, 77 insertions(+), 31 deletions(-) diff --git a/pkg/runtime/k8s_parity_test.go b/pkg/runtime/k8s_parity_test.go index fd709e252..e646a0af6 100644 --- a/pkg/runtime/k8s_parity_test.go +++ b/pkg/runtime/k8s_parity_test.go @@ -17,7 +17,6 @@ package runtime import ( "context" "embed" - "fmt" "os" "path/filepath" "strings" @@ -25,6 +24,7 @@ import ( "github.com/GoogleCloudPlatform/scion/pkg/api" "github.com/GoogleCloudPlatform/scion/pkg/k8s" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8sruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/dynamic/fake" @@ -373,39 +373,33 @@ func TestCreateAuthFileSecret(t *testing.T) { // --- K8s exec user context parity (matches Docker/Podman --user scion) --- -func TestK8sExec_WrapsCommandWithSu(t *testing.T) { - // Verify that Exec wraps commands with su to run as the scion user, - // matching the --user scion flag used by Docker/Podman runtimes. +func TestK8sExec_WrapsCommandWithUserAwareShell(t *testing.T) { cmd := []string{"tmux", "send-keys", "-t", "scion:0", "hello world", "Enter"} - // Simulate the wrapping logic from Exec - quoted := make([]string, len(cmd)) - for i, arg := range cmd { - quoted[i] = fmt.Sprintf("'%s'", strings.ReplaceAll(arg, "'", "'\"'\"'")) + wrapped := wrapExecCommandForUser("scion", cmd) + if len(wrapped) != 3 { + t.Fatalf("expected 3-part shell command, got %v", wrapped) } - suCmd := []string{"su", "-", "scion", "-c", strings.Join(quoted, " ")} - - if suCmd[0] != "su" || suCmd[1] != "-" || suCmd[2] != "scion" || suCmd[3] != "-c" { - t.Fatalf("expected su - scion -c prefix, got: %v", suCmd[:4]) + if wrapped[0] != "/bin/sh" || wrapped[1] != "-lc" { + t.Fatalf("expected /bin/sh -lc wrapper, got %v", wrapped[:2]) } - // The -c argument should contain all original args, properly quoted - shellCmd := suCmd[4] + shellCmd := wrapped[2] + if !strings.Contains(shellCmd, `id -un`) { + t.Fatalf("expected current-user check in %q", shellCmd) + } + if !strings.Contains(shellCmd, `exec su - 'scion' -c`) { + t.Fatalf("expected su fallback in %q", shellCmd) + } for _, arg := range cmd { if !strings.Contains(shellCmd, arg) { - t.Errorf("shell command %q should contain argument %q", shellCmd, arg) + t.Errorf("wrapped shell command %q should contain argument %q", shellCmd, arg) } } } func TestK8sExec_QuotesSingleQuotesInArgs(t *testing.T) { - cmd := []string{"echo", "it's a test"} - - quoted := make([]string, len(cmd)) - for i, arg := range cmd { - quoted[i] = fmt.Sprintf("'%s'", strings.ReplaceAll(arg, "'", "'\"'\"'")) - } - shellCmd := strings.Join(quoted, " ") + shellCmd := shellJoin([]string{"echo", "it's a test"}) // The single quote in "it's" should be escaped if !strings.Contains(shellCmd, "'\"'\"'") { @@ -413,6 +407,25 @@ func TestK8sExec_QuotesSingleQuotesInArgs(t *testing.T) { } } +func TestK8sExecUsername_UsesAnnotationWhenPresent(t *testing.T) { + clientset := k8sfake.NewSimpleClientset() + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "scion", + Name: "agent", + Annotations: map[string]string{"scion.username": "carver"}, + }, + } + if _, err := clientset.CoreV1().Pods("scion").Create(context.Background(), pod, metav1.CreateOptions{}); err != nil { + t.Fatalf("failed to create pod: %v", err) + } + + rt := &KubernetesRuntime{Client: &k8s.Client{Clientset: clientset}} + if got := rt.execUsername(context.Background(), "scion", "agent"); got != "carver" { + t.Fatalf("got username %q, want carver", got) + } +} + func TestK8sAttach_ResolvesUsernameFromAnnotations(t *testing.T) { // Verify that Attach reads the username from scion.username annotation tests := []struct { diff --git a/pkg/runtime/k8s_runtime.go b/pkg/runtime/k8s_runtime.go index 1a2c7677b..71bad8930 100644 --- a/pkg/runtime/k8s_runtime.go +++ b/pkg/runtime/k8s_runtime.go @@ -73,6 +73,45 @@ func (r *KubernetesRuntime) ExecUser() string { return "scion" } +func shellQuote(arg string) string { + return fmt.Sprintf("'%s'", strings.ReplaceAll(arg, "'", "'\"'\"'")) +} + +func shellJoin(args []string) string { + quoted := make([]string, len(args)) + for i, arg := range args { + quoted[i] = shellQuote(arg) + } + return strings.Join(quoted, " ") +} + +func wrapExecCommandForUser(username string, cmd []string) []string { + if username == "" { + username = "scion" + } + + shellCmd := shellJoin(cmd) + wrapper := fmt.Sprintf( + `if [ "$(id -un)" = %s ]; then exec %s; else exec su - %s -c %s; fi`, + shellQuote(username), + shellCmd, + shellQuote(username), + shellQuote(shellCmd), + ) + return []string{"/bin/sh", "-lc", wrapper} +} + +func (r *KubernetesRuntime) execUsername(ctx context.Context, namespace, podName string) string { + pod, err := r.Client.Clientset.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) + if err != nil { + return "scion" + } + if u := strings.TrimSpace(pod.Annotations["scion.username"]); u != "" { + return u + } + return "scion" +} + // resolveNamespace determines the namespace for a pod by looking up the // scion.namespace annotation on the pod itself. Falls back to DefaultNamespace // if the pod is not found or has no annotation. @@ -1728,7 +1767,7 @@ func (r *KubernetesRuntime) Attach(ctx context.Context, id string) error { option := &corev1.PodExecOptions{ Container: "agent", - Command: []string{"su", "-", username, "-c", "tmux attach -t scion"}, + Command: wrapExecCommandForUser(username, []string{"tmux", "attach", "-t", "scion"}), Stdin: true, Stdout: true, Stderr: true, @@ -1982,17 +2021,11 @@ func (r *KubernetesRuntime) Exec(ctx context.Context, id string, cmd []string) ( Namespace(namespace). SubResource("exec") - // Wrap command with su to run as the scion user (K8s exec has no --user flag). - // Shell-quote each argument to handle spaces and special characters. - quoted := make([]string, len(cmd)) - for i, arg := range cmd { - quoted[i] = fmt.Sprintf("'%s'", strings.ReplaceAll(arg, "'", "'\"'\"'")) - } - suCmd := []string{"su", "-", "scion", "-c", strings.Join(quoted, " ")} + username := r.execUsername(ctx, namespace, podName) option := &corev1.PodExecOptions{ Container: "agent", - Command: suCmd, + Command: wrapExecCommandForUser(username, cmd), Stdin: false, Stdout: true, Stderr: true, From f304bd332f35d3b6fc564e7806b37c5e6cbe92a5 Mon Sep 17 00:00:00 2001 From: Michael Freeman Date: Thu, 9 Apr 2026 19:23:18 -0500 Subject: [PATCH 2/3] Remove Kubernetes exec shell wrappers --- pkg/runtime/k8s_parity_test.go | 88 ++++++++++++++++++++++------ pkg/runtime/k8s_runtime.go | 104 ++++++++++++++++++++++++--------- 2 files changed, 144 insertions(+), 48 deletions(-) diff --git a/pkg/runtime/k8s_parity_test.go b/pkg/runtime/k8s_parity_test.go index e646a0af6..6905e4441 100644 --- a/pkg/runtime/k8s_parity_test.go +++ b/pkg/runtime/k8s_parity_test.go @@ -373,27 +373,34 @@ func TestCreateAuthFileSecret(t *testing.T) { // --- K8s exec user context parity (matches Docker/Podman --user scion) --- -func TestK8sExec_WrapsCommandWithUserAwareShell(t *testing.T) { +func TestK8sExec_BuildCommandForMatchingUserReturnsRawArgv(t *testing.T) { cmd := []string{"tmux", "send-keys", "-t", "scion:0", "hello world", "Enter"} - wrapped := wrapExecCommandForUser("scion", cmd) - if len(wrapped) != 3 { - t.Fatalf("expected 3-part shell command, got %v", wrapped) + got := buildExecCommandForUser("scion", "scion", cmd) + if len(got) != len(cmd) { + t.Fatalf("expected raw argv %v, got %v", cmd, got) } - if wrapped[0] != "/bin/sh" || wrapped[1] != "-lc" { - t.Fatalf("expected /bin/sh -lc wrapper, got %v", wrapped[:2]) + for i := range cmd { + if got[i] != cmd[i] { + t.Fatalf("expected raw argv %v, got %v", cmd, got) + } } +} + +func TestK8sExec_BuildCommandForDifferentUserFallsBackToSu(t *testing.T) { + cmd := []string{"tmux", "attach", "-t", "scion"} - shellCmd := wrapped[2] - if !strings.Contains(shellCmd, `id -un`) { - t.Fatalf("expected current-user check in %q", shellCmd) + got := buildExecCommandForUser("root", "scion", cmd) + if len(got) != 5 { + t.Fatalf("expected su argv, got %v", got) } - if !strings.Contains(shellCmd, `exec su - 'scion' -c`) { - t.Fatalf("expected su fallback in %q", shellCmd) + if got[0] != "su" || got[1] != "-" || got[2] != "scion" || got[3] != "-c" { + t.Fatalf("expected su - scion -c prefix, got %v", got[:4]) } + shellCmd := got[4] for _, arg := range cmd { if !strings.Contains(shellCmd, arg) { - t.Errorf("wrapped shell command %q should contain argument %q", shellCmd, arg) + t.Errorf("su shell command %q should contain argument %q", shellCmd, arg) } } } @@ -407,8 +414,7 @@ func TestK8sExec_QuotesSingleQuotesInArgs(t *testing.T) { } } -func TestK8sExecUsername_UsesAnnotationWhenPresent(t *testing.T) { - clientset := k8sfake.NewSimpleClientset() +func TestK8sExecTargetUsername_UsesAnnotationWhenPresent(t *testing.T) { pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ Namespace: "scion", @@ -416,13 +422,57 @@ func TestK8sExecUsername_UsesAnnotationWhenPresent(t *testing.T) { Annotations: map[string]string{"scion.username": "carver"}, }, } - if _, err := clientset.CoreV1().Pods("scion").Create(context.Background(), pod, metav1.CreateOptions{}); err != nil { - t.Fatalf("failed to create pod: %v", err) + if got := execTargetUsername(pod); got != "carver" { + t.Fatalf("got username %q, want carver", got) } +} - rt := &KubernetesRuntime{Client: &k8s.Client{Clientset: clientset}} - if got := rt.execUsername(context.Background(), "scion", "agent"); got != "carver" { - t.Fatalf("got username %q, want carver", got) +func TestK8sPodRunsAsNonRoot(t *testing.T) { + runAsUser := int64(1000) + runAsNonRoot := true + tests := []struct { + name string + pod *corev1.Pod + want bool + }{ + { + name: "pod security context runAsUser", + pod: &corev1.Pod{ + Spec: corev1.PodSpec{ + SecurityContext: &corev1.PodSecurityContext{RunAsUser: &runAsUser}, + }, + }, + want: true, + }, + { + name: "container security context runAsNonRoot", + pod: &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "agent", + SecurityContext: &corev1.SecurityContext{RunAsNonRoot: &runAsNonRoot}, + }}, + }, + }, + want: true, + }, + { + name: "no security context", + pod: &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "agent"}}, + }, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := podRunsAsNonRoot(tt.pod, "agent"); got != tt.want { + t.Fatalf("got %v, want %v", got, tt.want) + } + }) } } diff --git a/pkg/runtime/k8s_runtime.go b/pkg/runtime/k8s_runtime.go index 71bad8930..5f7de09f1 100644 --- a/pkg/runtime/k8s_runtime.go +++ b/pkg/runtime/k8s_runtime.go @@ -73,6 +73,15 @@ func (r *KubernetesRuntime) ExecUser() string { return "scion" } +func execTargetUsername(pod *corev1.Pod) string { + if pod != nil { + if u := strings.TrimSpace(pod.Annotations["scion.username"]); u != "" { + return u + } + } + return "scion" +} + func shellQuote(arg string) string { return fmt.Sprintf("'%s'", strings.ReplaceAll(arg, "'", "'\"'\"'")) } @@ -85,31 +94,69 @@ func shellJoin(args []string) string { return strings.Join(quoted, " ") } -func wrapExecCommandForUser(username string, cmd []string) []string { - if username == "" { - username = "scion" +func buildExecCommandForUser(currentUser, targetUser string, cmd []string) []string { + if targetUser == "" { + targetUser = "scion" } + if currentUser == "" || currentUser == targetUser || targetUser == "root" { + return append([]string(nil), cmd...) + } + return []string{"su", "-", targetUser, "-c", shellJoin(cmd)} +} - shellCmd := shellJoin(cmd) - wrapper := fmt.Sprintf( - `if [ "$(id -un)" = %s ]; then exec %s; else exec su - %s -c %s; fi`, - shellQuote(username), - shellCmd, - shellQuote(username), - shellQuote(shellCmd), - ) - return []string{"/bin/sh", "-lc", wrapper} +func podRunsAsNonRoot(pod *corev1.Pod, containerName string) bool { + if pod == nil { + return false + } + if pod.Spec.SecurityContext != nil { + if pod.Spec.SecurityContext.RunAsUser != nil { + return *pod.Spec.SecurityContext.RunAsUser != 0 + } + if pod.Spec.SecurityContext.RunAsNonRoot != nil && *pod.Spec.SecurityContext.RunAsNonRoot { + return true + } + } + for _, container := range pod.Spec.Containers { + if container.Name != containerName { + continue + } + if container.SecurityContext == nil { + return false + } + if container.SecurityContext.RunAsUser != nil { + return *container.SecurityContext.RunAsUser != 0 + } + if container.SecurityContext.RunAsNonRoot != nil && *container.SecurityContext.RunAsNonRoot { + return true + } + return false + } + return false +} + +func (r *KubernetesRuntime) currentExecUser(ctx context.Context, namespace, podName string) (string, error) { + out, err := r.execInPod(ctx, namespace, podName, []string{"id", "-un"}) + if err != nil { + return "", err + } + return strings.TrimSpace(out), nil } -func (r *KubernetesRuntime) execUsername(ctx context.Context, namespace, podName string) string { +func (r *KubernetesRuntime) commandForExec(ctx context.Context, namespace, podName string, cmd []string) ([]string, error) { pod, err := r.Client.Clientset.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) if err != nil { - return "scion" + return nil, err } - if u := strings.TrimSpace(pod.Annotations["scion.username"]); u != "" { - return u + + targetUser := execTargetUsername(pod) + currentUser, err := r.currentExecUser(ctx, namespace, podName) + if err == nil && currentUser != "" { + return buildExecCommandForUser(currentUser, targetUser, cmd), nil } - return "scion" + if podRunsAsNonRoot(pod, "agent") { + return append([]string(nil), cmd...), nil + } + return buildExecCommandForUser("root", targetUser, cmd), nil } // resolveNamespace determines the namespace for a pod by looking up the @@ -341,8 +388,7 @@ func (r *KubernetesRuntime) Run(ctx context.Context, config RunConfig) (string, // Fix ownership: tar extraction runs as root via K8s exec, so synced // files are owned by root. chown them to the scion user so the // privilege-dropped harness process can access its home directory. - chownCmd := fmt.Sprintf("chown -R %s:%s %s", config.UnixUsername, config.UnixUsername, destHome) - if _, err := r.execInPod(ctx, namespace, createdPod.Name, []string{"sh", "-c", chownCmd}); err != nil { + if _, err := r.execInPod(ctx, namespace, createdPod.Name, []string{"chown", "-R", fmt.Sprintf("%s:%s", config.UnixUsername, config.UnixUsername), destHome}); err != nil { runtimeLog.Debug("Failed to chown home directory (non-fatal)", "error", err) } } @@ -357,8 +403,7 @@ func (r *KubernetesRuntime) Run(ctx context.Context, config RunConfig) (string, return createdPod.Name, fmt.Errorf("failed to sync workspace: %w", err) } // Fix workspace ownership for the scion user - chownCmd := fmt.Sprintf("chown -R %s:%s /workspace", config.UnixUsername, config.UnixUsername) - if _, err := r.execInPod(ctx, namespace, createdPod.Name, []string{"sh", "-c", chownCmd}); err != nil { + if _, err := r.execInPod(ctx, namespace, createdPod.Name, []string{"chown", "-R", fmt.Sprintf("%s:%s", config.UnixUsername, config.UnixUsername), "/workspace"}); err != nil { runtimeLog.Debug("Failed to chown workspace (non-fatal)", "error", err) } } @@ -1758,16 +1803,14 @@ func (r *KubernetesRuntime) Attach(ctx context.Context, id string) error { Namespace(namespace). SubResource("exec") - // Determine the container username so we attach as the correct user - // (K8s exec has no --user flag; we use su to switch from root). - username := "scion" - if u, ok := agent.Annotations["scion.username"]; ok && u != "" { - username = u + execCmd, err := r.commandForExec(ctx, namespace, podName, []string{"tmux", "attach", "-t", "scion"}) + if err != nil { + return err } option := &corev1.PodExecOptions{ Container: "agent", - Command: wrapExecCommandForUser(username, []string{"tmux", "attach", "-t", "scion"}), + Command: execCmd, Stdin: true, Stdout: true, Stderr: true, @@ -2021,11 +2064,14 @@ func (r *KubernetesRuntime) Exec(ctx context.Context, id string, cmd []string) ( Namespace(namespace). SubResource("exec") - username := r.execUsername(ctx, namespace, podName) + execCmd, err := r.commandForExec(ctx, namespace, podName, cmd) + if err != nil { + return "", err + } option := &corev1.PodExecOptions{ Container: "agent", - Command: wrapExecCommandForUser(username, cmd), + Command: execCmd, Stdin: false, Stdout: true, Stderr: true, From 77a0be04f73e030f2024ab60382b1f295c237761 Mon Sep 17 00:00:00 2001 From: Michael Freeman Date: Sat, 11 Apr 2026 18:46:42 -0500 Subject: [PATCH 3/3] Validate Kubernetes exec username annotation --- pkg/runtime/k8s_parity_test.go | 29 +++++++++++++++++++++-------- pkg/runtime/k8s_runtime.go | 7 ++++++- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/pkg/runtime/k8s_parity_test.go b/pkg/runtime/k8s_parity_test.go index 6905e4441..b98f481e7 100644 --- a/pkg/runtime/k8s_parity_test.go +++ b/pkg/runtime/k8s_parity_test.go @@ -427,6 +427,19 @@ func TestK8sExecTargetUsername_UsesAnnotationWhenPresent(t *testing.T) { } } +func TestK8sExecTargetUsername_FallsBackWhenAnnotationInvalid(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "scion", + Name: "agent", + Annotations: map[string]string{"scion.username": "--help"}, + }, + } + if got := execTargetUsername(pod); got != "scion" { + t.Fatalf("got username %q, want scion", got) + } +} + func TestK8sPodRunsAsNonRoot(t *testing.T) { runAsUser := int64(1000) runAsNonRoot := true @@ -477,7 +490,6 @@ func TestK8sPodRunsAsNonRoot(t *testing.T) { } func TestK8sAttach_ResolvesUsernameFromAnnotations(t *testing.T) { - // Verify that Attach reads the username from scion.username annotation tests := []struct { name string annotations map[string]string @@ -498,17 +510,18 @@ func TestK8sAttach_ResolvesUsernameFromAnnotations(t *testing.T) { annotations: map[string]string{"scion.username": ""}, wantUser: "scion", }, + { + name: "defaults to scion when annotation invalid", + annotations: map[string]string{"scion.username": "bad user"}, + wantUser: "scion", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Simulate the username resolution logic from Attach - username := "scion" - if u, ok := tt.annotations["scion.username"]; ok && u != "" { - username = u - } - if username != tt.wantUser { - t.Errorf("got username %q, want %q", username, tt.wantUser) + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: tt.annotations}} + if got := execTargetUsername(pod); got != tt.wantUser { + t.Errorf("got username %q, want %q", got, tt.wantUser) } }) } diff --git a/pkg/runtime/k8s_runtime.go b/pkg/runtime/k8s_runtime.go index 5f7de09f1..91ad491bf 100644 --- a/pkg/runtime/k8s_runtime.go +++ b/pkg/runtime/k8s_runtime.go @@ -26,6 +26,7 @@ import ( "os/exec" "os/signal" "path/filepath" + "regexp" "strings" "syscall" "time" @@ -73,9 +74,11 @@ func (r *KubernetesRuntime) ExecUser() string { return "scion" } +var validExecUsername = regexp.MustCompile(`^[a-z_][a-z0-9_-]{0,31}$`) + func execTargetUsername(pod *corev1.Pod) string { if pod != nil { - if u := strings.TrimSpace(pod.Annotations["scion.username"]); u != "" { + if u := strings.TrimSpace(pod.Annotations["scion.username"]); u != "" && validExecUsername.MatchString(u) { return u } } @@ -149,6 +152,8 @@ func (r *KubernetesRuntime) commandForExec(ctx context.Context, namespace, podNa } targetUser := execTargetUsername(pod) + // Probe the live pod user at exec time instead of caching it because the + // effective exec user is a property of the running container state. currentUser, err := r.currentExecUser(ctx, namespace, podName) if err == nil && currentUser != "" { return buildExecCommandForUser(currentUser, targetUser, cmd), nil