diff --git a/pkg/runtime/k8s_parity_test.go b/pkg/runtime/k8s_parity_test.go index fd709e252..b98f481e7 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,40 @@ 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_BuildCommandForMatchingUserReturnsRawArgv(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, "'", "'\"'\"'")) + got := buildExecCommandForUser("scion", "scion", cmd) + if len(got) != len(cmd) { + t.Fatalf("expected raw argv %v, got %v", cmd, got) } - 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]) + for i := range cmd { + if got[i] != cmd[i] { + t.Fatalf("expected raw argv %v, got %v", cmd, got) + } } +} - // The -c argument should contain all original args, properly quoted - shellCmd := suCmd[4] +func TestK8sExec_BuildCommandForDifferentUserFallsBackToSu(t *testing.T) { + cmd := []string{"tmux", "attach", "-t", "scion"} + + got := buildExecCommandForUser("root", "scion", cmd) + if len(got) != 5 { + t.Fatalf("expected su argv, got %v", got) + } + 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("shell command %q should contain argument %q", shellCmd, arg) + t.Errorf("su 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,8 +414,82 @@ func TestK8sExec_QuotesSingleQuotesInArgs(t *testing.T) { } } +func TestK8sExecTargetUsername_UsesAnnotationWhenPresent(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "scion", + Name: "agent", + Annotations: map[string]string{"scion.username": "carver"}, + }, + } + if got := execTargetUsername(pod); got != "carver" { + t.Fatalf("got username %q, want carver", got) + } +} + +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 + 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) + } + }) + } +} + func TestK8sAttach_ResolvesUsernameFromAnnotations(t *testing.T) { - // Verify that Attach reads the username from scion.username annotation tests := []struct { name string annotations map[string]string @@ -435,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 1a2c7677b..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,6 +74,96 @@ 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 != "" && validExecUsername.MatchString(u) { + return u + } + } + 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 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)} +} + +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) 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 nil, err + } + + 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 + } + 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 // scion.namespace annotation on the pod itself. Falls back to DefaultNamespace // if the pod is not found or has no annotation. @@ -302,8 +393,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) } } @@ -318,8 +408,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) } } @@ -1719,16 +1808,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: []string{"su", "-", username, "-c", "tmux attach -t scion"}, + Command: execCmd, Stdin: true, Stdout: true, Stderr: true, @@ -1982,17 +2069,14 @@ 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, "'", "'\"'\"'")) + execCmd, err := r.commandForExec(ctx, namespace, podName, cmd) + if err != nil { + return "", err } - suCmd := []string{"su", "-", "scion", "-c", strings.Join(quoted, " ")} option := &corev1.PodExecOptions{ Container: "agent", - Command: suCmd, + Command: execCmd, Stdin: false, Stdout: true, Stderr: true,