diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index 578232f87..7cf8dab6a 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -415,6 +415,12 @@ jobs: install-kind: true requires-secret: false + - label: up-provider-kubernetes-restricted-scc + runner: ubuntu-latest + free-disk-space: true + install-kind: true + requires-secret: false + - label: up-provider-podman-rootless-basic runner: ubuntu-latest free-disk-space: false @@ -702,7 +708,7 @@ jobs: uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: path: ${{ runner.temp }}/kind.exe - key: ${{ runner.os }}-${{ runner.arch }}-kind-v0.24.0 + key: ${{ runner.os }}-${{ runner.arch }}-kind-v0.33.0 - name: setup kind (Windows) if: matrix.install-kind == true && runner.os == 'Windows' && (matrix.requires-secret == false || needs.can-read-secret.outputs.secret-set == 'true') @@ -712,8 +718,8 @@ jobs: DOCKER_HOST: npipe:////./pipe/podman-machine-default run: | if [ "${{ steps.kind-cache-windows.outputs.cache-hit }}" != "true" ]; then - curl -Lo "$RUNNER_TEMP/kind.exe" "https://github.com/kubernetes-sigs/kind/releases/download/v0.24.0/kind-windows-amd64" - expected="6f724188289cc79395f45afae0f2b85e0d220c2b84c6ed2f5047d9d0c9a67028" + curl -Lo "$RUNNER_TEMP/kind.exe" "https://github.com/kubernetes-sigs/kind/releases/download/v0.33.0/kind-windows-amd64" + expected="4b22adaa135368c5a465d56bbd8e520cbea87272a06ca00b6078e7b81515c9fc" actual=$(sha256sum "$RUNNER_TEMP/kind.exe" | awk '{print $1}' | sed 's/^[^a-f0-9]*//') if [ "$actual" != "$expected" ]; then echo "SHA256 mismatch for kind.exe! Expected: $expected, Got: $actual" @@ -726,7 +732,7 @@ jobs: echo "$RUNNER_TEMP" >> "$GITHUB_PATH" CLUSTER_NAME=$(python -c "import uuid; print(uuid.uuid4().hex)") - kind create cluster --name "$CLUSTER_NAME" --image kindest/node:v1.34.0@sha256:7416a61b42b1662ca6ca89f02028ac133a309a2a30ba309614e8ec94d976dc5a + kind create cluster --name "$CLUSTER_NAME" --image kindest/node:v1.36.4@sha256:099e049362a1526b2db71494e1947aae99bd16290d7c895f2b7ea312e3cbfaed # NOTE: skevetter/setup-kind does not work on Windows runners - name: setup kind @@ -734,8 +740,8 @@ jobs: uses: skevetter/setup-kind@7febac2ed35df332069b3bb687c6b1fa00fc0883 # v1 with: name: ${{ steps.uuid.outputs.result }} - version: v0.24.0 - image: kindest/node:v1.34.0@sha256:7416a61b42b1662ca6ca89f02028ac133a309a2a30ba309614e8ec94d976dc5a + version: v0.33.0 + image: kindest/node:v1.36.4@sha256:099e049362a1526b2db71494e1947aae99bd16290d7c895f2b7ea312e3cbfaed skipClusterLogsExport: true - name: cache podman installer (Linux) diff --git a/Taskfile.yml b/Taskfile.yml index 6da9bd16d..922bbec0a 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -144,7 +144,45 @@ tasks: cli:test:e2e:kind:setup: desc: setup kind cluster for e2e tests - cmd: kind create cluster --image kindest/node:v1.34.0@sha256:7416a61b42b1662ca6ca89f02028ac133a309a2a30ba309614e8ec94d976dc5a + cmd: | + kind_version="$(kind version -q)" + kind_version="${kind_version#v}" + kind_major="${kind_version%%.*}" + kind_minor_patch="${kind_version#*.}" + kind_minor="${kind_minor_patch%%.*}" + kind_patch="${kind_minor_patch#*.}" + kind_version_valid=true + case "$kind_version" in + *.*.*) ;; + *) kind_version_valid=false ;; + esac + case "$kind_major" in + ''|*[!0-9]*) kind_version_valid=false ;; + esac + case "$kind_minor" in + ''|*[!0-9]*) kind_version_valid=false ;; + esac + case "$kind_patch" in + ''|*[!0-9]*) kind_version_valid=false ;; + esac + if [ "$kind_version_valid" != true ]; then + echo "kind v0.33.0 or newer is required for Kubernetes v1.36.4 (found $kind_version)" >&2 + exit 1 + fi + case "$kind_major" in + 0) + if [ "$kind_minor" -lt 33 ]; then + echo "kind v0.33.0 or newer is required for Kubernetes v1.36.4 (found $kind_version)" >&2 + exit 1 + fi + ;; + [1-9]|[1-9][0-9]*) ;; + *) + echo "kind v0.33.0 or newer is required for Kubernetes v1.36.4 (found $kind_version)" >&2 + exit 1 + ;; + esac + kind create cluster --image kindest/node:v1.36.4@sha256:099e049362a1526b2db71494e1947aae99bd16290d7c895f2b7ea312e3cbfaed cli:test:e2e:kind:teardown: desc: teardown kind cluster for e2e tests diff --git a/cmd/internal/agentcontainer/setup.go b/cmd/internal/agentcontainer/setup.go index d0ca7f8b7..22eb15fbb 100644 --- a/cmd/internal/agentcontainer/setup.go +++ b/cmd/internal/agentcontainer/setup.go @@ -229,6 +229,7 @@ func (cmd *SetupContainerCmd) prepareWorkspace( cleanupFunc := cmd.setupGitCredentials( ctx, state.tunnelClient, + config.GetRemoteUser(state.setupInfo), ) cloneErr := cmd.cloneRepositoryIfNeeded( @@ -520,6 +521,7 @@ func skipSnapshotRestore(target string) bool { func (cmd *SetupContainerCmd) setupGitCredentials( ctx context.Context, tunnelClient tunnel.TunnelClient, + remoteUser string, ) func() { if !cmd.InjectGitCredentials { return nil @@ -531,7 +533,11 @@ func (cmd *SetupContainerCmd) setupGitCredentials( } cancelCtx, cancel := context.WithCancel(ctx) - cleanupFunc, err := configureSystemGitCredentials(cancelCtx, tunnelClient) + cleanupFunc, err := configureSystemGitCredentials( + cancelCtx, + tunnelClient, + remoteUser, + ) if err != nil { cancel() log.Errorf("error configuring git credentials: %v", err) @@ -701,7 +707,6 @@ func (cmd *SetupContainerCmd) installIDE( if newServer, ok := jetbrainsServers[ide.Name]; ok { return newServer(config.GetRemoteUser(setupInfo), ide.Options).Install(setupInfo) } - switch ide.Name { case string(config2.IDENone): return nil @@ -871,11 +876,8 @@ func (cmd *SetupContainerCmd) startBrowserExtensionsInstall( func configureSystemGitCredentials( ctx context.Context, client tunnel.TunnelClient, + remoteUser string, ) (func(), error) { - if !command.Exists("git") { - return nil, errors.New("git not found") - } - serverPort, err := credentials.StartCredentialsServer(ctx, client) if err != nil { return nil, err @@ -894,20 +896,77 @@ func configureSystemGitCredentials( _ = os.Setenv(config2.EnvGitHelperPort, strconv.Itoa(serverPort)) gitConfig := git.At("", git.WithStrictHostKeyChecking(false)).Config() - if err = gitConfig.Add(ctx, "credential.helper", gitCredentials, git.ScopeSystem); err != nil { - return nil, fmt.Errorf("add git credential helper: %w", err) + gitConfig, scope, err := addGitCredentialHelper( + ctx, + gitConfig, + gitCredentials, + remoteUser, + ) + if err != nil { + return nil, err } cleanup := func() { - log.Debug("unset setup system credential helper") - if err = gitConfig.Unset(ctx, "credential.helper", git.ScopeSystem); err != nil { - log.Errorf("unset system credential helper %v", err) + log.Debug("unset setup credential helper") + if err = gitConfig.UnsetValue(ctx, "credential.helper", gitCredentials, scope); err != nil { + log.Errorf("unset credential helper %v", err) } } return cleanup, nil } +// addGitCredentialHelper installs the credential helper system-wide +// (/etc/gitconfig) so it applies regardless of which local user's git +// invocation picks it up. A container's remoteUser can differ from the +// process configuring it. Falls back to remoteUser's global config +// when /etc/gitconfig is not writable (e.g. a non-root OpenShift-style pod +// running as a single fixed UID, where that multi-user concern doesn't +// apply), returning the config and scope actually used so the caller unsets +// the same one. +func addGitCredentialHelper( + ctx context.Context, + gitConfig *git.Config, + value string, + remoteUser string, +) (*git.Config, git.ConfigScope, error) { + err := gitConfig.Add(ctx, "credential.helper", value, git.ScopeSystem) + if err == nil { + return gitConfig, git.ScopeSystem, nil + } + if !isGitPermissionDenied(err) { + return nil, git.ConfigScope{}, fmt.Errorf("add git credential helper: %w", err) + } + + homeDir, err := command.GetHome(remoteUser) + if err != nil { + return nil, git.ConfigScope{}, fmt.Errorf( + "resolve remote user home for git credentials: %w", + err, + ) + } + log.Debugf("system git config is not writable, falling back to %s's global config", remoteUser) + globalGitConfig := git.At( + "", + git.WithStrictHostKeyChecking(false), + git.WithEnv([]string{ + "HOME=" + homeDir, + "XDG_CONFIG_HOME=" + filepath.Join(homeDir, ".config"), + }), + ).Config() + if err := globalGitConfig.Add(ctx, "credential.helper", value, git.ScopeGlobal); err != nil { + return nil, git.ConfigScope{}, fmt.Errorf("add git credential helper: %w", err) + } + return globalGitConfig, git.ScopeGlobal, nil +} + +func isGitPermissionDenied(err error) bool { + var cmdErr *git.CommandError + return errors.As(err, &cmdErr) && + (strings.Contains(cmdErr.Stderr, "Permission denied") || + strings.Contains(cmdErr.Stderr, "Read-only file system")) +} + func streamMount( ctx context.Context, workspaceInfo *provider2.ContainerWorkspaceInfo, diff --git a/cmd/internal/agentcontainer/setup_test.go b/cmd/internal/agentcontainer/setup_test.go index b58763fb1..b53e2fa7f 100644 --- a/cmd/internal/agentcontainer/setup_test.go +++ b/cmd/internal/agentcontainer/setup_test.go @@ -7,10 +7,18 @@ import ( "path/filepath" "testing" + "github.com/devsy-org/devsy/pkg/git" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestIsGitPermissionDenied_ReadOnlyFileSystem(t *testing.T) { + err := &git.CommandError{ + Stderr: "fatal: could not write config: Read-only file system", + } + require.True(t, isGitPermissionDenied(err)) +} + func TestSkipSnapshotRestore_EmptyDir(t *testing.T) { dir := t.TempDir() assert.False(t, skipSnapshotRestore(dir)) diff --git a/cmd/internal/container_tunnel.go b/cmd/internal/container_tunnel.go index 6e63d8dce..97c629e79 100644 --- a/cmd/internal/container_tunnel.go +++ b/cmd/internal/container_tunnel.go @@ -113,11 +113,13 @@ func (cmd *ContainerTunnelCmd) Run(cobraCtx context.Context) error { Stderr: stderr, }) }, - User: cmd.User, - Stdin: os.Stdin, - Stdout: os.Stdout, - Stderr: os.Stderr, - Timeout: workspaceInfo.InjectTimeout, + User: cmd.User, + Stdin: os.Stdin, + Stdout: os.Stdout, + Stderr: os.Stderr, + Timeout: workspaceInfo.InjectTimeout, + RemoteAgentPath: workspaceInfo.Agent.ContainerInstallPath(), + DownloadURL: workspaceInfo.Agent.DownloadURL, }) } @@ -188,7 +190,7 @@ func hasDevContainerResult(ctx context.Context, runner devcontainer.Runner) bool var buf bytes.Buffer err := runner.Command(ctx, devcontainer.CommandParams{ User: containerRootUser, - Command: "cat " + pkgconfig.DevContainerResultPath, + Command: pkgconfig.ReadDevContainerResultCommand(), Stdout: &buf, Stderr: &buf, }) diff --git a/cmd/workspace/ssh.go b/cmd/workspace/ssh.go index e9b07d173..e1a219800 100644 --- a/cmd/workspace/ssh.go +++ b/cmd/workspace/ssh.go @@ -440,7 +440,7 @@ func (cmd *SSHCmd) startTunnel( workdir := resolveWorkdir(cmd.WorkDir, workspaceClient) log.Debugf("run outer container tunnel") - command := cmd.buildSSHServerCommand(workdir) + command := cmd.buildSSHServerCommand(workdir, resolveAgentConfig(workspaceClient)) envVars, err := cmd.retrieveEnVars() if err != nil { @@ -468,6 +468,21 @@ func (cmd *SSHCmd) startTunnel( }) } +// resolveAgentConfig returns the workspace's agent config when workspaceClient +// exposes it, or a zero value otherwise. +func resolveAgentConfig(workspaceClient client2.BaseWorkspaceClient) provider.ProviderAgentConfig { + full, ok := workspaceClient.(client2.WorkspaceClient) + if !ok { + return provider.ProviderAgentConfig{} + } + _, agentInfo, err := full.AgentInfo(provider.CLIOptions{}) + if err != nil { + log.Debugf("resolve agent info: %v", err) + return provider.ProviderAgentConfig{} + } + return agentInfo.Agent +} + // setupTunnelWriter wires up the JSON log pipe and GPG agent tunnel shared by // both tunnel modes. buildSSHServerCommand runs `devsy internal ssh-server`, // which always logs structured JSON on stderr; PipeJSONStream re-emits each @@ -556,9 +571,12 @@ func (cmd *SSHCmd) startTunnelServices( go cmd.startServices(ctx, devsyConfig, containerClient, workspaceClient.WorkspaceConfig(), opts) } -func (cmd *SSHCmd) buildSSHServerCommand(workdir string) string { +func (cmd *SSHCmd) buildSSHServerCommand( + workdir string, + agent provider.ProviderAgentConfig, +) string { commandArgs := []string{ - config.ContainerDevsyHelperLocation, + agent.ContainerInstallPath(), "internal", "ssh-server", names.Flag(names.TrackActivity), @@ -578,7 +596,7 @@ func (cmd *SSHCmd) buildSSHServerCommand(workdir string) string { commandArgs = append(commandArgs, names.Flag(names.Debug)) } command := shellescape.QuoteCommand(commandArgs) - if cmd.User != "" && cmd.User != "root" { + if cmd.User != "" && cmd.User != "root" && !agent.RunsFixedNonRootUser() { command = shellescape.QuoteCommand([]string{"su", "-c", command, cmd.User}) } return command diff --git a/e2e/README.md b/e2e/README.md index 3ade1e89b..3e46153e7 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -14,10 +14,11 @@ BUILDDIR=bin SRCDIR=".." ../hack/build-e2e.sh #### Kubernetes Tests Setup -For tests that require Kubernetes (labeled with `up-kubernetes` or `build`), you need to set up a kind cluster: +For Kubernetes tests, Kind v0.33.0 or newer is required for the Kubernetes v1.36.4 node image. The setup task enforces this prerequisite automatically. +For tests that require Kubernetes (labeled with `up-kubernetes` or `build`), you need to set up a kind cluster: ```bash -kind create cluster --image kindest/node:v1.34.0@sha256:7416a61b42b1662ca6ca89f02028ac133a309a2a30ba309614e8ec94d976dc5a +kind create cluster --image kindest/node:v1.36.4@sha256:099e049362a1526b2db71494e1947aae99bd16290d7c895f2b7ea312e3cbfaed ``` To delete the cluster after testing: diff --git a/e2e/framework/types.go b/e2e/framework/types.go index 973e4b6ce..eab95dcb9 100644 --- a/e2e/framework/types.go +++ b/e2e/framework/types.go @@ -11,8 +11,16 @@ type Pod struct { type PodSpec struct { Containers []PodContainer `json:"containers,omitempty"` + HostUsers *bool `json:"hostUsers,omitempty"` } type PodContainer struct { - Image string `json:"image,omitempty"` + Image string `json:"image,omitempty"` + SecurityContext *SecurityContext `json:"securityContext,omitempty"` +} + +type SecurityContext struct { + RunAsUser *int64 `json:"runAsUser,omitempty"` + RunAsGroup *int64 `json:"runAsGroup,omitempty"` + RunAsNonRoot *bool `json:"runAsNonRoot,omitempty"` } diff --git a/e2e/tests/up/helper.go b/e2e/tests/up/helper.go index d7d073e5f..38b9dc2a6 100644 --- a/e2e/tests/up/helper.go +++ b/e2e/tests/up/helper.go @@ -16,7 +16,11 @@ import ( "github.com/onsi/ginkgo/v2" ) -const secretCmd = "secret" +const ( + secretCmd = "secret" + cmdSSH = "ssh" + flagCommand = "--command" +) // useFileSecretsBackend forces the file backend so tests do not depend on an OS // keyring (unavailable in CI); env is restored on cleanup. @@ -72,7 +76,7 @@ func (btc *baseTestContext) execSSHCapture( ) (string, error) { output, _, err := btc.f.ExecCommandCapture( ctx, - []string{"workspace", "ssh", "--command", command, projectName}, + []string{cmdWorkspace, cmdSSH, flagCommand, command, projectName}, ) return strings.TrimSpace(output), err } diff --git a/e2e/tests/up/provider_kubernetes_restricted.go b/e2e/tests/up/provider_kubernetes_restricted.go new file mode 100644 index 000000000..23fcafe40 --- /dev/null +++ b/e2e/tests/up/provider_kubernetes_restricted.go @@ -0,0 +1,133 @@ +package up + +import ( + "context" + "fmt" + "os" + "os/exec" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +const restrictedNamespace = "devsy-restricted" + +const restrictedSecurityContextYAML = "runAsUser: 1000\n" + + "runAsGroup: 1000\n" + + "runAsNonRoot: true\n" + + "allowPrivilegeEscalation: false\n" + + "seccompProfile:\n" + + " type: RuntimeDefault\n" + + "capabilities:\n" + + " drop: [\"ALL\"]\n" + +const restrictedPodManifestTemplate = "spec:\n hostUsers: true\n" + +func labelNamespaceRestricted(ctx context.Context) error { + createOrUpdate := fmt.Sprintf( + "kubectl create namespace %s --dry-run=client -o yaml | kubectl apply -f -", + restrictedNamespace, + ) + // #nosec G204 -- createOrUpdate is built from the fixed restrictedNamespace const + if err := exec.CommandContext(ctx, "sh", "-c", createOrUpdate).Run(); err != nil { + return err + } + return exec.CommandContext( + ctx, "kubectl", "label", "namespace", restrictedNamespace, + "pod-security.kubernetes.io/enforce=restricted", + "pod-security.kubernetes.io/enforce-version=latest", + "--overwrite", + ).Run() +} + +var _ = ginkgo.Describe( + "testing up command for kubernetes provider under Pod Security Admission restricted", + ginkgo.Label("up-provider-kubernetes-restricted-scc"), + func() { + var initialDir string + + ginkgo.BeforeEach(func(ctx ginkgo.SpecContext) { + var err error + initialDir, err = os.Getwd() + framework.ExpectNoError(err) + + err = labelNamespaceRestricted(ctx) + framework.ExpectNoError(err) + }) + + ginkgo.AfterEach(func(ctx ginkgo.SpecContext) { + _ = exec.CommandContext(ctx, "kubectl", "delete", "namespace", restrictedNamespace, "--ignore-not-found"). + Run() + }) + + ginkgo.It( + "rejects the default root security context and succeeds with AGENT_SECURITY_CONTEXT + STRICT_SECURITY", + func(ctx context.Context) { + f := framework.NewDefaultFramework(initialDir + "/bin") + tempDir, err := framework.CopyToTempDir("tests/up/testdata/kubernetes") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(framework.CleanupTempDir, initialDir, tempDir) + + _ = f.DevsyProviderDelete(ctx, "kubernetes") + err = f.DevsyProviderAdd( + ctx, "kubernetes", + "-o", "KUBERNETES_NAMESPACE="+restrictedNamespace, + "-o", "CREATE_NAMESPACE=false", + ) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + err := f.DevsyProviderDelete(cleanupCtx, "kubernetes") + framework.ExpectNoError(err) + }) + + ginkgo.By("rejecting the default root security context under admission") + err = f.DevsyUp(ctx, tempDir) + gomega.Expect(err).To(gomega.HaveOccurred()) + + ginkgo.By("switching to a restricted-admission security context") + err = f.DevsyProviderUse( + ctx, "kubernetes", + "-o", "STRICT_SECURITY=true", + "-o", "AGENT_SECURITY_CONTEXT="+restrictedSecurityContextYAML, + "-o", "POD_MANIFEST_TEMPLATE="+restrictedPodManifestTemplate, + "-o", "AGENT_INSTALL_PATH=/tmp/devsy", + ) + framework.ExpectNoError(err) + + ginkgo.By("admitting and running a non-root workspace") + err = f.DevsyUp(ctx, tempDir) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(f.DevsyWorkspaceDelete, tempDir) + + list := waitForPodCount(ctx, restrictedNamespace, 1, "Expect 1 pod") + // PSA does not require user namespaces. The explicit template + // override keeps this admission test runnable on Kind nodes where + // the outer container runtime cannot create nested user namespaces. + gomega.Expect(list.Items[0].Spec.HostUsers).ToNot(gomega.BeNil()) + gomega.Expect(*list.Items[0].Spec.HostUsers).To(gomega.BeTrue()) + sc := list.Items[0].Spec.Containers[0].SecurityContext + gomega.Expect(sc).ToNot(gomega.BeNil()) + gomega.Expect(*sc.RunAsUser).To(gomega.Equal(int64(1000))) + gomega.Expect(*sc.RunAsNonRoot).To(gomega.BeTrue()) + + err = f.ExecCommand( + ctx, + true, + true, + "mYtEsTsTrInG", + []string{ + cmdWorkspace, + cmdSSH, + "--agent-forwarding=false", + flagCommand, + "echo 'bVl0RXNUc1RySW5H' | base64 -d", + tempDir, + }, + ) + framework.ExpectNoError(err) + }, + ginkgo.SpecTimeout(framework.TimeoutModerate()), + ) + }, +) diff --git a/e2e/tests/up/up.go b/e2e/tests/up/up.go index 34c43449e..a92ec6834 100644 --- a/e2e/tests/up/up.go +++ b/e2e/tests/up/up.go @@ -177,7 +177,7 @@ var _ = ginkgo.Describe("testing up command", ginkgo.Label("up-workspaces"), fun containerEnvPath, _, err := f.ExecCommandCapture( ctx, - []string{"workspace", "ssh", "--command", "cat " + devcontainerPath, projectName}, + []string{cmdWorkspace, cmdSSH, flagCommand, "cat " + devcontainerPath, projectName}, ) framework.ExpectNoError(err) expectedImageName := language.MapConfig[language.Go].Image diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 3bc20f1d5..1769519b0 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "al.essio.dev/pkg/shellescape" "github.com/devsy-org/devsy/pkg/command" "github.com/devsy-org/devsy/pkg/compress" "github.com/devsy-org/devsy/pkg/config" @@ -423,32 +424,36 @@ type Exec func( ) error type TunnelOptions struct { - Exec Exec - User string - Stdin io.Reader - Stdout io.Writer - Stderr io.Writer - Timeout time.Duration + Exec Exec + User string + Stdin io.Reader + Stdout io.Writer + Stderr io.Writer + Timeout time.Duration + RemoteAgentPath string + DownloadURL string } func Tunnel(ctx context.Context, opts TunnelOptions) error { + remoteAgentPath := opts.RemoteAgentPath + if remoteAgentPath == "" { + remoteAgentPath = config.ContainerDevsyHelperLocation + } + if err := InjectAgent(ctx, &InjectOptions{ Exec: func(ctx context.Context, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { return opts.Exec(ctx, "root", command, stdin, stdout, stderr) }, IsLocal: false, - RemoteAgentPath: config.ContainerDevsyHelperLocation, - DownloadURL: config.DefaultAgentDownloadURL(), + RemoteAgentPath: remoteAgentPath, + DownloadURL: opts.DownloadURL, PreferDownloadFromRemoteUrl: new(false), Timeout: opts.Timeout, }); err != nil { return err } - command := fmt.Sprintf("'%s' internal ssh-server --stdio", config.ContainerDevsyHelperLocation) - if log.DebugEnabled() { - command += " --debug" - } + command := sshServerCommand(remoteAgentPath, log.DebugEnabled()) user := opts.User if user == "" { user = "root" @@ -457,6 +462,16 @@ func Tunnel(ctx context.Context, opts TunnelOptions) error { return opts.Exec(ctx, user, command, opts.Stdin, opts.Stdout, opts.Stderr) } +// sshServerCommand builds the remote command that runs the ssh-server +// subcommand at agentPath. +func sshServerCommand(agentPath string, debug bool) string { + args := []string{agentPath, "internal", "ssh-server", "--stdio"} + if debug { + args = append(args, "--debug") + } + return shellescape.QuoteCommand(args) +} + func applyDockerEnv(cmd *exec.Cmd, envs map[string]string) { if len(envs) == 0 { return diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go new file mode 100644 index 000000000..0df892ab7 --- /dev/null +++ b/pkg/agent/agent_test.go @@ -0,0 +1,43 @@ +package agent + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestSSHServerCommand_EscapesAgentPath(t *testing.T) { + marker := filepath.Join(t.TempDir(), "pwned") + malicious := "/tmp/devsy'; touch " + marker + "; echo 'still-quoted" + + command := sshServerCommand(malicious, false) + + // The built command names a nonexistent binary, so it's expected to + // fail; only the absence of the marker file matters here. + _ = exec.Command("sh", "-c", command).Run() // #nosec G204 -- injection payload under test + + if _, err := os.Stat(marker); err == nil { + t.Fatalf("agent path broke out of shell quoting and ran injected command: %s", command) + } + if !strings.Contains(command, "internal") || !strings.Contains(command, "ssh-server") { + t.Fatalf("command missing expected subcommand: %s", command) + } +} + +func TestSSHServerCommand_AppendsDebugFlag(t *testing.T) { + command := sshServerCommand("/usr/local/bin/devsy", true) + + if !strings.HasSuffix(command, "--debug") { + t.Fatalf("expected trailing --debug flag, got: %s", command) + } +} + +func TestSSHServerCommand_OmitsDebugFlagWhenDisabled(t *testing.T) { + command := sshServerCommand("/usr/local/bin/devsy", false) + + if strings.Contains(command, "--debug") { + t.Fatalf("did not expect --debug flag, got: %s", command) + } +} diff --git a/pkg/agent/binary.go b/pkg/agent/binary.go index ef15f7426..70e1676c6 100644 --- a/pkg/agent/binary.go +++ b/pkg/agent/binary.go @@ -88,6 +88,15 @@ func (m *BinaryManager) AcquireBinary(ctx context.Context, arch string) (io.Read return nil, ErrBinaryNotFound } +// HasLocalOverride returns true when the host can supply a local Linux binary +// for the given arch. +func (m *BinaryManager) HasLocalOverride(arch string) bool { + if strings.TrimSpace(os.Getenv(config.EnvAgentBinary)) != "" { + return true + } + return runtime.GOOS == osLinux && runtime.GOARCH == arch +} + type BinaryCache struct { BaseDir string } @@ -238,15 +247,22 @@ func (s *HTTPDownloadSource) SourceName() string { return "http download" } -func (s *HTTPDownloadSource) buildDownloadURL(arch string) (string, error) { - binaryName := config.BinaryName + "-" + osLinux + "-" + arch - downloadURL, err := url.JoinPath(s.BaseURL, binaryName) +// AgentDownloadURL returns the URL to download the linux agent binary for +// arch from baseURL, matching the naming HTTPDownloadSource resolves for the +// host-side binary manager. +func AgentDownloadURL(baseURL, arch string) (string, error) { + binaryName := strings.Join([]string{config.BinaryName, osLinux, arch}, "-") + downloadURL, err := url.JoinPath(baseURL, binaryName) if err != nil { return "", fmt.Errorf("failed to construct download URL: %w", err) } return downloadURL, nil } +func (s *HTTPDownloadSource) buildDownloadURL(arch string) (string, error) { + return AgentDownloadURL(s.BaseURL, arch) +} + func (s *HTTPDownloadSource) downloadFile( ctx context.Context, downloadURL string, diff --git a/pkg/agent/binary_env_test.go b/pkg/agent/binary_env_test.go index cf6a9a45e..e72b32b36 100644 --- a/pkg/agent/binary_env_test.go +++ b/pkg/agent/binary_env_test.go @@ -5,7 +5,10 @@ import ( "io" "os" "path/filepath" + "runtime" "testing" + + "github.com/devsy-org/devsy/pkg/config" ) func TestEnvPathSourceUsesLocalBinary(t *testing.T) { @@ -35,3 +38,42 @@ func TestEnvPathSourceUnsetFallsThrough(t *testing.T) { t.Fatal("expected an error when the env var is unset so later sources are tried") } } + +func TestHasLocalOverride_EnvOverrideSet(t *testing.T) { + dir := t.TempDir() + binPath := filepath.Join(dir, "devsy-linux-arm64") + if err := os.WriteFile(binPath, []byte("agent-bytes"), 0o755); err != nil { //nolint:gosec + t.Fatal(err) + } + t.Setenv(config.EnvAgentBinary, binPath) + + mgr := &BinaryManager{} + if !mgr.HasLocalOverride("some-other-arch") { + t.Error( + "HasLocalOverride() = false, want true when DEVSY_AGENT_BINARY is set (regardless of arch)", + ) + } +} + +func TestHasLocalOverride_MatchingHostArch(t *testing.T) { + t.Setenv(config.EnvAgentBinary, "") + mgr := &BinaryManager{} + + if runtime.GOOS != osLinux { + t.Skip("this host cannot supply a linux binary; matching-arch case not applicable") + } + if !mgr.HasLocalOverride(runtime.GOARCH) { + t.Error("HasLocalOverride() = false, want true when the host's own arch matches the target") + } +} + +func TestHasLocalOverride_NoOverrideNoArchMatch(t *testing.T) { + t.Setenv(config.EnvAgentBinary, "") + mgr := &BinaryManager{} + + if mgr.HasLocalOverride("definitely-not-a-real-arch") { + t.Error( + "HasLocalOverride() = true, want false with no env override and no matching host arch", + ) + } +} diff --git a/pkg/agent/delivery/delivery.go b/pkg/agent/delivery/delivery.go index 5540aef3e..643105f44 100644 --- a/pkg/agent/delivery/delivery.go +++ b/pkg/agent/delivery/delivery.go @@ -37,10 +37,12 @@ type PreStartOptions struct { } type PostStartOptions struct { - WorkspaceID string - ContainerDetails *config.ContainerDetails - BinarySource BinarySourceFunc - Arch string + WorkspaceID string + ContainerDetails *config.ContainerDetails + BinarySource BinarySourceFunc + Arch string + DownloadURL string + PreferInContainerDownload bool } // Cleaner removes the resources a delivery created for a workspace. Cleanup is diff --git a/pkg/agent/delivery/factory.go b/pkg/agent/delivery/factory.go index da1111716..91a6d6951 100644 --- a/pkg/agent/delivery/factory.go +++ b/pkg/agent/delivery/factory.go @@ -12,15 +12,17 @@ import ( ) type FactoryOptions struct { - WorkspaceConfig *provider.AgentWorkspaceInfo - WorkspaceID string - DockerCommand string - DockerEnv []string - HelperImage string - IsRemoteDocker bool - ContainerID string - ExecFunc inject.ExecFunc //nolint:staticcheck // legacy delivery strategies require this type - PodExec PodExecFunc + IsRemoteDocker bool + WorkspaceID string + DockerCommand string + HelperImage string + KubernetesAgentInstallPath string + ContainerID string + DockerEnv []string + WorkspaceConfig *provider.AgentWorkspaceInfo + DownloadURL string + ExecFunc inject.ExecFunc //nolint:staticcheck // legacy delivery strategies require this type + PodExec PodExecFunc } func NewAgentDelivery(opts FactoryOptions) AgentDelivery { @@ -38,7 +40,7 @@ func NewAgentDelivery(opts FactoryOptions) AgentDelivery { return dockerDelivery(opts) } - return legacyShellDelivery(opts, fmt.Sprintf("driver: %s", driverType)) + return legacyShellDelivery(opts, fmt.Sprintf("driver: %s", driverType), "") } // namedDriverDelivery returns the delivery strategy for driver types that @@ -46,7 +48,7 @@ func NewAgentDelivery(opts FactoryOptions) AgentDelivery { func namedDriverDelivery(driverType string, opts FactoryOptions) AgentDelivery { switch driverType { case provider.CustomDriver: - return legacyShellDelivery(opts, "custom driver") + return legacyShellDelivery(opts, "custom driver", "") case provider.KubernetesDriver: return kubernetesDelivery(opts) case provider.AppleDriver: @@ -63,15 +65,19 @@ func namedDriverDelivery(driverType string, opts FactoryOptions) AgentDelivery { // fallback. func appleDelivery(opts FactoryOptions) AgentDelivery { log.Debugf("using shell-based delivery for apple driver") - return &LegacyShellDelivery{ExecFunc: opts.ExecFunc, DownloadURL: ""} + return &LegacyShellDelivery{ExecFunc: opts.ExecFunc, DownloadURL: opts.DownloadURL} } func kubernetesDelivery(opts FactoryOptions) AgentDelivery { if opts.PodExec == nil { - return legacyShellDelivery(opts, "kubernetes pod exec unavailable") + return legacyShellDelivery( + opts, + "kubernetes pod exec unavailable", + opts.KubernetesAgentInstallPath, + ) } log.Debugf("using kubernetes-native delivery (exec stream)") - return &KubernetesDelivery{Exec: opts.PodExec} + return &KubernetesDelivery{Exec: opts.PodExec, InstallPath: opts.KubernetesAgentInstallPath} } // microsandboxDelivery streams the agent binary over the SDK's guest exec @@ -79,7 +85,7 @@ func kubernetesDelivery(opts FactoryOptions) AgentDelivery { // exposes no argv exec. func microsandboxDelivery(opts FactoryOptions) AgentDelivery { if opts.PodExec == nil { - return legacyShellDelivery(opts, "microsandbox argv exec unavailable") + return legacyShellDelivery(opts, "microsandbox argv exec unavailable", "") } log.Debugf("using stream delivery (exec stream) for microsandbox") return &KubernetesDelivery{Exec: opts.PodExec} @@ -106,14 +112,15 @@ func remoteDockerDelivery(opts FactoryOptions) AgentDelivery { } } -func legacyShellDelivery(opts FactoryOptions, reason string) AgentDelivery { +func legacyShellDelivery(opts FactoryOptions, reason, remoteAgentPath string) AgentDelivery { log.Debugf("using legacy shell delivery for %s", reason) log.Warnf( "legacy shell delivery is deprecated; platform-native delivery will replace this in a future release", ) return &LegacyShellDelivery{ - ExecFunc: opts.ExecFunc, - DownloadURL: "", + ExecFunc: opts.ExecFunc, + DownloadURL: opts.DownloadURL, + RemoteAgentPath: remoteAgentPath, } } diff --git a/pkg/agent/delivery/factory_test.go b/pkg/agent/delivery/factory_test.go index c15c61c42..feeded6ae 100644 --- a/pkg/agent/delivery/factory_test.go +++ b/pkg/agent/delivery/factory_test.go @@ -12,6 +12,8 @@ import ( "github.com/stretchr/testify/require" ) +const testKubernetesInstallPath = "/home/vscode/.local/bin/devsy" + func TestNewAgentDelivery_LocalDocker(t *testing.T) { opts := FactoryOptions{ WorkspaceConfig: &provider.AgentWorkspaceInfo{ @@ -96,6 +98,27 @@ func TestNewAgentDelivery_KubernetesDriver_Native(t *testing.T) { assert.Equal(t, PhasePostStart, d.Phase()) } +func TestNewAgentDelivery_KubernetesDriver_ThreadsInstallPath(t *testing.T) { + podExec := func(_ context.Context, _ []string, _ driver.Streams) error { + return nil + } + + opts := FactoryOptions{ + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.KubernetesDriver, + }, + }, + PodExec: podExec, + KubernetesAgentInstallPath: testKubernetesInstallPath, + } + + d := NewAgentDelivery(opts) + native, ok := d.(*KubernetesDelivery) + require.True(t, ok) + assert.Equal(t, testKubernetesInstallPath, native.InstallPath) +} + func TestNewAgentDelivery_MicrosandboxUsesStreamDelivery(t *testing.T) { podExec := func(_ context.Context, _ []string, _ driver.Streams) error { return nil @@ -127,7 +150,9 @@ func TestNewAgentDelivery_KubernetesDriver_FallsBackWhenNoPodExec(t *testing.T) Driver: provider.KubernetesDriver, }, }, - ExecFunc: execFn, + ExecFunc: execFn, + DownloadURL: "https://artifacts.example.test/devsy", + KubernetesAgentInstallPath: testKubernetesInstallPath, // PodExec intentionally nil → legacy fallback. } @@ -135,6 +160,8 @@ func TestNewAgentDelivery_KubernetesDriver_FallsBackWhenNoPodExec(t *testing.T) legacy, ok := d.(*LegacyShellDelivery) require.True(t, ok) assert.NotNil(t, legacy.ExecFunc) + assert.Equal(t, testKubernetesInstallPath, legacy.RemoteAgentPath) + assert.Equal(t, "https://artifacts.example.test/devsy", legacy.DownloadURL) assert.Equal(t, PhasePostStart, d.Phase()) } diff --git a/pkg/agent/delivery/kubernetes.go b/pkg/agent/delivery/kubernetes.go index b5eb498cf..2d863b0ce 100644 --- a/pkg/agent/delivery/kubernetes.go +++ b/pkg/agent/delivery/kubernetes.go @@ -3,13 +3,22 @@ package delivery import ( "bytes" "context" + "errors" "fmt" + "io" + "net" "strings" + "time" + "al.essio.dev/pkg/shellescape" + "github.com/devsy-org/devsy/pkg/agent" pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/driver" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/version" + "k8s.io/apimachinery/pkg/util/wait" + execerr "k8s.io/client-go/util/exec" + "k8s.io/client-go/util/retry" ) var _ AgentDelivery = (*KubernetesDelivery)(nil) @@ -17,14 +26,26 @@ var _ AgentDelivery = (*KubernetesDelivery)(nil) // PodExecFunc runs argv in the workspace pod's dev container with the given streams. type PodExecFunc func(ctx context.Context, argv []string, streams driver.Streams) error -// KubernetesDelivery streams the agent binary into the pod over the cluster's exec API. +// KubernetesDelivery gets the agent binary into the pod over the cluster's +// exec API. type KubernetesDelivery struct { Exec PodExecFunc // ExpectedVersion defaults to version.GetVersion() when empty. ExpectedVersion string + + // InstallPath overrides where the agent binary is installed inside the + // container. + InstallPath string } +const ( + noDownloadToolExitCode = 127 + downloadTimeoutSeconds = 25 + execStreamAttemptTimeout = 30 * time.Second + execStreamMaxAttempts = 2 +) + func (d *KubernetesDelivery) Phase() DeliveryPhase { return PhasePostStart } @@ -41,7 +62,7 @@ func (d *KubernetesDelivery) DeliverPostStart(ctx context.Context, opts PostStar return fmt.Errorf("exec function is required for kubernetes delivery") } - destPath := pkgconfig.ContainerDevsyHelperLocation + destPath := d.destPath() // Skip delivery when the in-pod binary already matches. expected := d.expectedVersion() @@ -50,26 +71,23 @@ func (d *KubernetesDelivery) DeliverPostStart(ctx context.Context, opts PostStar return nil } - binary, err := opts.BinarySource(ctx, opts.Arch) - if err != nil { - return fmt.Errorf("acquire binary: %w", err) + if opts.PreferInContainerDownload { + if err := d.deliverViaDownload(ctx, destPath, opts.DownloadURL, opts.Arch); err != nil { + log.Debugf( + "in-container download unavailable, falling back to exec-stream delivery: %v", + err, + ) + } else { + log.Debugf("delivered agent binary to pod via in-container download") + return nil + } } - defer func() { _ = binary.Close() }() - - // Write to a temp file and atomically move it into place so a failed stream - // never leaves an executable stub. - script := fmt.Sprintf( - `set -e; d=$(dirname %s); mkdir -p "$d"; `+ - `t=$(mktemp %s.XXXXXX); `+ - `cat > "$t" && chmod 0755 "$t" && mv -f "$t" %s || { rm -f "$t"; exit 1; }`, - destPath, destPath, destPath, - ) - if err := d.Exec(ctx, []string{"sh", "-c", script}, driver.Streams{Stdin: binary}); err != nil { + if err := d.deliverViaExecStream(ctx, destPath, opts); err != nil { return fmt.Errorf("write binary to container: %w", err) } - log.Debugf("delivered agent binary to pod via kubernetes exec") + log.Debugf("delivered agent binary to pod via kubernetes exec-stream") return nil } @@ -77,6 +95,142 @@ func (d *KubernetesDelivery) Cleanup(_ context.Context, _ string) error { return nil } +func (d *KubernetesDelivery) deliverViaDownload( + ctx context.Context, + destPath, downloadURL, arch string, +) error { + if downloadURL == "" { + return fmt.Errorf("no download URL configured") + } + + fetchURL, err := agent.AgentDownloadURL(downloadURL, arch) + if err != nil { + return fmt.Errorf("build download URL: %w", err) + } + + script := downloadScript(destPath, fetchURL) + + var stderr bytes.Buffer + if err := d.Exec( + ctx, + []string{"sh", "-c", script}, + driver.Streams{Stderr: &stderr}, + ); err != nil { + var codeErr execerr.CodeExitError + if errors.As(err, &codeErr) && codeErr.Code == noDownloadToolExitCode { + return fmt.Errorf("no curl or wget in the image: %w", err) + } + return fmt.Errorf("download in container: %w (%s)", err, strings.TrimSpace(stderr.String())) + } + return nil +} + +func downloadScript(destPath, downloadURL string) string { + quotedDest := shellescape.Quote(destPath) + quotedURL := shellescape.Quote(downloadURL) + return fmt.Sprintf( + `set -e +d=$(dirname %s); mkdir -p "$d" +t=$(mktemp %s.XXXXXX) +trap 'rm -f "$t"' EXIT +if command -v curl >/dev/null 2>&1; then + curl -fsSL --max-time %d %s -o "$t" +elif command -v wget >/dev/null 2>&1; then + wget -q -T %d %s -O "$t" +else + exit %d +fi +chmod 0755 "$t" +mv -f "$t" %s +`, + quotedDest, quotedDest, + downloadTimeoutSeconds, quotedURL, + downloadTimeoutSeconds, quotedURL, + noDownloadToolExitCode, + quotedDest, + ) +} + +func (d *KubernetesDelivery) deliverViaExecStream( + ctx context.Context, + destPath string, + opts PostStartOptions, +) error { + attempt := 0 + err := retry.OnError( + wait.Backoff{Steps: execStreamMaxAttempts}, + isTransientDeliveryError, + func() error { + attempt++ + binary, err := opts.BinarySource(ctx, opts.Arch) + if err != nil { + return &permanentDeliveryError{fmt.Errorf("acquire binary: %w", err)} + } + defer func() { _ = binary.Close() }() + + attemptCtx, cancel := context.WithTimeout(ctx, execStreamAttemptTimeout) + defer cancel() + streamErr := d.execStreamOnce(attemptCtx, destPath, binary) + if streamErr != nil && isTransientDeliveryError(streamErr) && + attempt < execStreamMaxAttempts { + log.Warnf( + "exec-stream delivery attempt %d/%d stalled or reset, retrying: %v", + attempt, execStreamMaxAttempts, streamErr, + ) + } + return streamErr + }, + ) + var perm *permanentDeliveryError + if errors.As(err, &perm) { + return perm.err + } + return err +} + +type permanentDeliveryError struct{ err error } + +func (e *permanentDeliveryError) Error() string { return e.err.Error() } + +func (e *permanentDeliveryError) Unwrap() error { return e.err } + +func (d *KubernetesDelivery) execStreamOnce( + ctx context.Context, + destPath string, + binary io.Reader, +) error { + quotedDest := shellescape.Quote(destPath) + script := fmt.Sprintf( + `set -e; d=$(dirname %s); mkdir -p "$d"; `+ + `t=$(mktemp %s.XXXXXX); `+ + `cat > "$t" && chmod 0755 "$t" && mv -f "$t" %s || { rm -f "$t"; exit 1; }`, + quotedDest, quotedDest, quotedDest, + ) + return d.Exec(ctx, []string{"sh", "-c", script}, driver.Streams{Stdin: binary}) +} + +func isTransientDeliveryError(err error) bool { + if err == nil { + return false + } + if _, ok := errors.AsType[*permanentDeliveryError](err); ok { + return false + } + if errors.Is(err, context.DeadlineExceeded) { + return true + } + if _, ok := errors.AsType[net.Error](err); ok { + return true + } + msg := err.Error() + for _, marker := range []string{"i/o timeout", "broken pipe", "connection reset", "unexpected EOF"} { + if strings.Contains(msg, marker) { + return true + } + } + return false +} + func (d *KubernetesDelivery) expectedVersion() string { if d.ExpectedVersion != "" { return d.ExpectedVersion @@ -84,9 +238,17 @@ func (d *KubernetesDelivery) expectedVersion() string { return version.GetVersion() } +func (d *KubernetesDelivery) destPath() string { + if d.InstallPath != "" { + return d.InstallPath + } + return pkgconfig.ContainerDevsyHelperLocation +} + // detectVersion returns the agent version in the pod, or "" if absent or unprobeable. func (d *KubernetesDelivery) detectVersion(ctx context.Context, destPath string) string { - script := fmt.Sprintf(`[ -x "%s" ] && "%s" --version 2>/dev/null || true`, destPath, destPath) + quotedPath := shellescape.Quote(destPath) + script := fmt.Sprintf(`[ -x %s ] && %s --version 2>/dev/null || true`, quotedPath, quotedPath) var stdout bytes.Buffer err := d.Exec(ctx, []string{"sh", "-c", script}, driver.Streams{Stdout: &stdout}) diff --git a/pkg/agent/delivery/kubernetes_test.go b/pkg/agent/delivery/kubernetes_test.go index 8ac355c7e..542e38ca4 100644 --- a/pkg/agent/delivery/kubernetes_test.go +++ b/pkg/agent/delivery/kubernetes_test.go @@ -12,6 +12,7 @@ import ( "github.com/devsy-org/devsy/pkg/driver" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + execerr "k8s.io/client-go/util/exec" ) const testVersion = "v1.2.3" @@ -84,7 +85,6 @@ func TestKubernetesDelivery_DeliverPostStart_RequiresExec(t *testing.T) { func TestKubernetesDelivery_DeliverPostStart_WritesBinary(t *testing.T) { binaryData := "test-binary-content" - // Probe returns nothing → deliver. exec := &recordingExec{stdouts: []string{""}} d := &KubernetesDelivery{Exec: exec.fn, ExpectedVersion: testVersion} @@ -124,7 +124,6 @@ func TestKubernetesDelivery_DeliverPostStart_SkipsWhenVersionMatches(t *testing. } func TestKubernetesDelivery_DeliverPostStart_DeliversWhenProbeErrors(t *testing.T) { - // A failing probe must not abort delivery; the write still succeeds. probeErr := &recordingExec{ stdouts: []string{""}, errs: []error{fmt.Errorf("probe boom"), nil}, @@ -156,3 +155,151 @@ func TestKubernetesDelivery_Cleanup_IsNoOp(t *testing.T) { err := d.Cleanup(context.Background(), "workspace-123") assert.NoError(t, err) } + +func TestKubernetesDelivery_DeliverPostStart_PrefersDownloadOverExecStream(t *testing.T) { + exec := &recordingExec{stdouts: []string{""}} + d := &KubernetesDelivery{Exec: exec.fn, ExpectedVersion: testVersion} + + err := d.DeliverPostStart(context.Background(), PostStartOptions{ + BinarySource: binarySourceFrom("should-not-be-streamed"), + Arch: testArch, + DownloadURL: "https://example.com/releases", + PreferInContainerDownload: true, + }) + require.NoError(t, err) + + require.Len(t, exec.calls, 2, "probe, then in-container download") + downloadScript := strings.Join(exec.calls[1].argv, " ") + assert.Contains(t, downloadScript, "curl") + assert.Contains(t, downloadScript, "example.com/releases") + assert.Empty(t, exec.calls[1].stdin, "download must not receive the binary over stdin") +} + +func TestKubernetesDelivery_DeliverPostStart_FallsBackToExecStreamWhenNoDownloadTool(t *testing.T) { + binaryData := "test-binary-content" + exec := &recordingExec{ + stdouts: []string{""}, + errs: []error{ + nil, + execerr.CodeExitError{ + Code: noDownloadToolExitCode, + Err: fmt.Errorf("command terminated with exit code %d", noDownloadToolExitCode), + }, + }, + } + d := &KubernetesDelivery{Exec: exec.fn, ExpectedVersion: testVersion} + + err := d.DeliverPostStart(context.Background(), PostStartOptions{ + BinarySource: binarySourceFrom(binaryData), + Arch: testArch, + DownloadURL: "https://example.com/releases", + PreferInContainerDownload: true, + }) + require.NoError(t, err) + + require.Len(t, exec.calls, 3, "probe, failed download, then exec-stream fallback") + assert.Equal(t, binaryData, exec.calls[2].stdin) +} + +func TestKubernetesDelivery_DeliverPostStart_SkipsDownloadWhenNoURLConfigured(t *testing.T) { + binaryData := "test-binary-content" + exec := &recordingExec{stdouts: []string{""}} + d := &KubernetesDelivery{Exec: exec.fn, ExpectedVersion: testVersion} + + err := d.DeliverPostStart(context.Background(), PostStartOptions{ + BinarySource: binarySourceFrom(binaryData), + Arch: testArch, + }) + require.NoError(t, err) + + require.Len(t, exec.calls, 2, "probe, then exec-stream (no download attempted)") + assert.Equal(t, binaryData, exec.calls[1].stdin) +} + +func TestKubernetesDelivery_DeliverViaExecStream_RetriesTransientFailureOnce(t *testing.T) { + exec := &recordingExec{ + errs: []error{fmt.Errorf("write binary to container: %w", context.DeadlineExceeded), nil}, + } + d := &KubernetesDelivery{Exec: exec.fn} + + err := d.deliverViaExecStream(context.Background(), "/usr/local/bin/devsy", PostStartOptions{ + BinarySource: binarySourceFrom("data"), + Arch: testArch, + }) + require.NoError(t, err) + assert.Len(t, exec.calls, execStreamMaxAttempts, "one stalled attempt, one successful retry") +} + +func TestKubernetesDelivery_DeliverViaExecStream_DoesNotRetryPermanentFailure(t *testing.T) { + permanentErr := execerr.CodeExitError{Code: 1, Err: fmt.Errorf("no such file or directory")} + exec := &recordingExec{errs: []error{permanentErr}} + d := &KubernetesDelivery{Exec: exec.fn} + + err := d.deliverViaExecStream(context.Background(), "/usr/local/bin/devsy", PostStartOptions{ + BinarySource: binarySourceFrom("data"), + Arch: testArch, + }) + require.Error(t, err) + assert.Len(t, exec.calls, 1, "a permanent failure must not be retried") +} + +func TestIsTransientDeliveryError(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {name: "nil is not transient", err: nil, want: false}, + {name: "our own deadline firing is transient", err: context.DeadlineExceeded, want: true}, + {name: "i/o timeout is transient", err: fmt.Errorf("write tcp: i/o timeout"), want: true}, + {name: "broken pipe is transient", err: fmt.Errorf("write: broken pipe"), want: true}, + { + name: "connection reset is transient", + err: fmt.Errorf("read: connection reset by peer"), + want: true, + }, + { + name: "a real exit code is not transient", + err: execerr.CodeExitError{Code: 1, Err: fmt.Errorf("boom")}, + want: false, + }, + { + name: "missing download tool is not transient", + err: execerr.CodeExitError{ + Code: noDownloadToolExitCode, + Err: fmt.Errorf("command terminated with exit code 127"), + }, + want: false, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.want, isTransientDeliveryError(c.err)) + }) + } +} + +func TestKubernetesDelivery_DeliverPostStart_UsesInstallPathOverride(t *testing.T) { + binaryData := "test-binary-content" + exec := &recordingExec{stdouts: []string{""}} + installPath := testKubernetesInstallPath + d := &KubernetesDelivery{Exec: exec.fn, ExpectedVersion: testVersion, InstallPath: installPath} + + err := d.DeliverPostStart(context.Background(), PostStartOptions{ + BinarySource: binarySourceFrom(binaryData), + Arch: testArch, + }) + require.NoError(t, err) + + require.Len(t, exec.calls, 2) + probeScript := strings.Join(exec.calls[0].argv, " ") + assert.Contains(t, probeScript, installPath) + writeScript := strings.Join(exec.calls[1].argv, " ") + assert.Contains(t, writeScript, installPath) + assert.NotContains(t, writeScript, pkgconfig.ContainerDevsyHelperLocation) +} + +func TestKubernetesDelivery_DestPath_DefaultsWhenInstallPathUnset(t *testing.T) { + d := &KubernetesDelivery{} + assert.Equal(t, pkgconfig.ContainerDevsyHelperLocation, d.destPath()) +} diff --git a/pkg/agent/delivery/legacy_shell.go b/pkg/agent/delivery/legacy_shell.go index 2e2d6d2a9..c3266e6da 100644 --- a/pkg/agent/delivery/legacy_shell.go +++ b/pkg/agent/delivery/legacy_shell.go @@ -13,9 +13,10 @@ import ( ) type LegacyShellDelivery struct { - ExecFunc inject.ExecFunc //nolint:staticcheck - DownloadURL string - Timeout func() time.Duration + ExecFunc inject.ExecFunc //nolint:staticcheck + DownloadURL string + RemoteAgentPath string + Timeout func() time.Duration } func (d *LegacyShellDelivery) Phase() DeliveryPhase { @@ -34,7 +35,7 @@ func (d *LegacyShellDelivery) DeliverPostStart(ctx context.Context, opts PostSta if err := agent.InjectAgent(ctx, &agent.InjectOptions{ Exec: d.ExecFunc, IsLocal: false, - RemoteAgentPath: pkgconfig.ContainerDevsyHelperLocation, + RemoteAgentPath: d.remoteAgentPath(), DownloadURL: d.downloadURL(), PreferDownloadFromRemoteUrl: new(false), Timeout: d.timeout(), @@ -64,6 +65,13 @@ func (d *LegacyShellDelivery) downloadURL() string { return pkgconfig.DefaultAgentDownloadURL() } +func (d *LegacyShellDelivery) remoteAgentPath() string { + if d.RemoteAgentPath != "" { + return d.RemoteAgentPath + } + return pkgconfig.ContainerDevsyHelperLocation +} + func ExecFuncFromDriver( cmdFn func(ctx context.Context, user, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error, user string, diff --git a/pkg/agent/delivery/legacy_shell_test.go b/pkg/agent/delivery/legacy_shell_test.go index e798a1b42..2ad1982c4 100644 --- a/pkg/agent/delivery/legacy_shell_test.go +++ b/pkg/agent/delivery/legacy_shell_test.go @@ -5,6 +5,7 @@ import ( "io" "testing" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -46,6 +47,16 @@ func TestLegacyShellDelivery_DownloadURL_Custom(t *testing.T) { assert.Equal(t, "https://custom.example.com/agent", d.downloadURL()) } +func TestLegacyShellDelivery_RemoteAgentPath_Default(t *testing.T) { + d := &LegacyShellDelivery{} + assert.Equal(t, pkgconfig.ContainerDevsyHelperLocation, d.remoteAgentPath()) +} + +func TestLegacyShellDelivery_RemoteAgentPath_Custom(t *testing.T) { + d := &LegacyShellDelivery{RemoteAgentPath: testKubernetesInstallPath} + assert.Equal(t, testKubernetesInstallPath, d.remoteAgentPath()) +} + func TestExecFuncFromDriver(t *testing.T) { var capturedUser, capturedCmd string cmdFn := func(ctx context.Context, user, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { diff --git a/pkg/config/paths.go b/pkg/config/paths.go index ac158f854..40074189d 100644 --- a/pkg/config/paths.go +++ b/pkg/config/paths.go @@ -1,5 +1,7 @@ package config +import "al.essio.dev/pkg/shellescape" + const ( // IgnoreFileName is the name of the devsy ignore file. IgnoreFileName = "." + BinaryName + "ignore" @@ -14,7 +16,8 @@ const ( DockerCredentialHelperName = "docker-credential-" + BinaryName // DevContainerResultPath is where devcontainer results are written. - DevContainerResultPath = "/var/run/" + BinaryName + "/result.json" + DevContainerResultPath = "/var/run/" + BinaryName + "/result.json" + DevContainerResultSelectorPath = "/var/run/" + BinaryName + "/result.path" // DaemonProcessName is the name used for the fallback background daemon process // PID file and lock file in os.TempDir(). @@ -23,6 +26,16 @@ const ( // ContainerDataDir is the base directory for Devsy data inside containers. ContainerDataDir = "/var/" + BinaryName + // ContainerDataDirFallback is used instead of ContainerDataDir when a + // non-root container (e.g. an OpenShift restricted-SCC pod) cannot create + // /var/devsy. + ContainerDataDirFallback = "/tmp/" + BinaryName + "-data" + + // DevContainerResultFallbackPath mirrors DevContainerResultPath under + // ContainerDataDirFallback. + DevContainerResultFallbackPath = ContainerDataDirFallback + "/result.json" + DevContainerResultFallbackSelectorPath = ContainerDataDirFallback + "/result.path" + // ContainerDevsyHelperLocation is where the Devsy agent binary lives inside containers. ContainerDevsyHelperLocation = "/usr/local/bin/" + BinaryName @@ -35,3 +48,31 @@ const ( // WorkspaceBusyFile is the per-workspace lock file written under the workspace folder. WorkspaceBusyFile = "workspace.lock" ) + +// ReadDevContainerResultCommand returns a command that reads the result selected +// by the setup process. It fails when no valid selector exists. +func ReadDevContainerResultCommand() string { + return readDevContainerResultCommand( + DevContainerResultPath, + DevContainerResultFallbackPath, + DevContainerResultSelectorPath, + DevContainerResultFallbackSelectorPath, + ) +} + +func readDevContainerResultCommand( + primary, fallback, primarySelector, fallbackSelector string, +) string { + primary = shellescape.Quote(primary) + fallback = shellescape.Quote(fallback) + primarySelector = shellescape.Quote(primarySelector) + fallbackSelector = shellescape.Quote(fallbackSelector) + + return "if [ -f " + primarySelector + " ] && [ -f " + primary + + " ] && [ \"$(cat " + primarySelector + ")\" = " + primary + + " ]; then cat " + primary + + "; elif [ -f " + fallbackSelector + " ] && [ -f " + fallback + + " ] && [ \"$(cat " + fallbackSelector + ")\" = " + fallback + + " ]; then cat " + fallback + + "; else echo 'devsy result path selector is missing' >&2; exit 1; fi" +} diff --git a/pkg/config/paths_test.go b/pkg/config/paths_test.go new file mode 100644 index 000000000..1c47fe05f --- /dev/null +++ b/pkg/config/paths_test.go @@ -0,0 +1,155 @@ +package config + +import ( + "os" + "os/exec" + "path/filepath" + "testing" + "time" +) + +const ( + resultCurrent = "current" + resultStale = "stale" +) + +type resultCommandTest struct { + name string + primaryContent, fallbackContent string + primarySelector, fallbackSelector bool + primarySelectorContent, fallbackSelectorContent string + primaryTime, fallbackTime time.Time + want string +} + +func writeResultTestFile(t *testing.T, path, content string) { + t.Helper() + // #nosec G306 -- test files intentionally use result-file permissions. + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +type resultSelectorTest struct { + path, resultPath string + enabled bool + mtime time.Time +} + +func writeResultTestSelector(t *testing.T, test resultSelectorTest) { + t.Helper() + if !test.enabled { + return + } + // #nosec G306 -- test files intentionally use selector permissions. + if err := os.WriteFile(test.path, []byte(test.resultPath), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(test.path, test.mtime, test.mtime); err != nil { + t.Fatal(err) + } +} + +func runResultCommandTest(t *testing.T, test resultCommandTest) ([]byte, error) { + t.Helper() + dir := t.TempDir() + primary := filepath.Join(dir, "primary.json") + fallback := filepath.Join(dir, "fallback.json") + primaryPath := filepath.Join(dir, "primary.path") + fallbackPath := filepath.Join(dir, "fallback.path") + if test.primaryContent != "" { + writeResultTestFile(t, primary, test.primaryContent) + } + if test.fallbackContent != "" { + writeResultTestFile(t, fallback, test.fallbackContent) + } + primarySelectorContent := test.primarySelectorContent + if primarySelectorContent == "" { + primarySelectorContent = primary + } + fallbackSelectorContent := test.fallbackSelectorContent + if fallbackSelectorContent == "" { + fallbackSelectorContent = fallback + } + writeResultTestSelector(t, resultSelectorTest{ + path: primaryPath, + resultPath: primarySelectorContent, + enabled: test.primarySelector, + mtime: test.primaryTime, + }) + writeResultTestSelector(t, resultSelectorTest{ + path: fallbackPath, + resultPath: fallbackSelectorContent, + enabled: test.fallbackSelector, + mtime: test.fallbackTime, + }) + command := readDevContainerResultCommand(primary, fallback, primaryPath, fallbackPath) + if _, err := exec.LookPath("sh"); err != nil { + t.Skipf("skipping result command test: sh unavailable: %v", err) + } + // #nosec G204 -- command is generated from test-owned temporary paths. + return exec.Command("sh", "-c", command).CombinedOutput() +} + +func TestReadDevContainerResultCommandSelectsActiveSelector(t *testing.T) { + tests := []resultCommandTest{ + { + name: "fallback", + primaryContent: resultStale, + fallbackContent: resultCurrent, + fallbackSelector: true, + primaryTime: time.Unix(2, 0), + fallbackTime: time.Unix(2, 0), + want: resultCurrent, + }, + { + name: "equal selector timestamps prefer primary", + primaryContent: resultCurrent, + fallbackContent: resultStale, + primarySelector: true, + fallbackSelector: true, + primaryTime: time.Unix(2, 0), + fallbackTime: time.Unix(2, 0), + want: resultCurrent, + }, + { + name: "primary", + primaryContent: resultCurrent, + fallbackContent: resultStale, + primarySelector: true, + primaryTime: time.Unix(2, 0), + fallbackTime: time.Unix(2, 0), + want: resultCurrent, + }, + { + name: "missing primary", + fallbackContent: resultCurrent, + primarySelector: true, + fallbackSelector: true, + primaryTime: time.Unix(2, 0), + fallbackTime: time.Unix(2, 0), + want: resultCurrent, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + output, err := runResultCommandTest(t, test) + if err != nil { + t.Fatal(err) + } + if string(output) != test.want { + t.Fatalf("result = %q, want %q", output, test.want) + } + }) + } +} + +func TestReadDevContainerResultCommandRequiresSelector(t *testing.T) { + _, err := runResultCommandTest(t, resultCommandTest{ + fallbackContent: resultStale, + }) + if err == nil { + t.Fatal("expected missing-selector error") + } +} diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index 5ef53d490..62c2bb64e 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -144,15 +144,17 @@ func (r *runner) newAgentDelivery() delivery.AgentDelivery { } return delivery.NewAgentDelivery(delivery.FactoryOptions{ - WorkspaceConfig: r.workspaceConfig, - WorkspaceID: r.id, - DockerCommand: dockerCmd, - DockerEnv: dockerEnv, - IsRemoteDocker: docker.RemoteDockerHost(dockerEnv), - HelperImage: r.workspaceConfig.Agent.Docker.HelperImage, - ContainerID: r.id, - ExecFunc: execFn, - PodExec: podExec, + WorkspaceConfig: r.workspaceConfig, + WorkspaceID: r.id, + DockerCommand: dockerCmd, + DockerEnv: dockerEnv, + IsRemoteDocker: docker.RemoteDockerHost(dockerEnv), + HelperImage: r.workspaceConfig.Agent.Docker.HelperImage, + ContainerID: r.id, + DownloadURL: r.resolvedAgentDownloadURL(), + ExecFunc: execFn, + PodExec: podExec, + KubernetesAgentInstallPath: r.workspaceConfig.Agent.Kubernetes.AgentInstallPath, }) } @@ -175,7 +177,7 @@ func (r *runner) deliveryArch(ctx context.Context) (string, error) { } func (r *runner) deliverPostStart(ctx context.Context, strategy delivery.AgentDelivery) error { - binarySource, err := r.newBinarySource() + mgr, err := agent.NewBinaryManager(r.resolvedAgentDownloadURL()) if err != nil { return fmt.Errorf("create binary source: %w", err) } @@ -186,9 +188,11 @@ func (r *runner) deliverPostStart(ctx context.Context, strategy delivery.AgentDe } err = strategy.DeliverPostStart(ctx, delivery.PostStartOptions{ - WorkspaceID: r.id, - BinarySource: binarySource, - Arch: arch, + WorkspaceID: r.id, + BinarySource: mgr.AcquireBinary, + Arch: arch, + DownloadURL: r.resolvedAgentDownloadURL(), + PreferInContainerDownload: !mgr.HasLocalOverride(arch), }) if err != nil { return fmt.Errorf("deliver agent (post-start): %w", err) @@ -214,12 +218,25 @@ func (r *runner) prefetchAgentBinary(ctx context.Context) { _, _ = io.Copy(io.Discard, rc) } -func (r *runner) newBinarySource() (delivery.BinarySourceFunc, error) { - downloadURL := r.agentDownloadURL - if downloadURL == "" { - downloadURL = pkgconfig.DefaultAgentDownloadURL() +// resolvedAgentDownloadURL is the base URL used to fetch the agent binary, +// falling back to the default release URL when the workspace has no override. +func (r *runner) resolvedAgentDownloadURL() string { + if r.agentDownloadURL != "" { + return r.agentDownloadURL } - mgr, err := agent.NewBinaryManager(downloadURL) + return pkgconfig.DefaultAgentDownloadURL() +} + +// agentContainerPath is where the agent binary lives inside the container. +// Defaults to pkgconfig.ContainerDevsyHelperLocation; Kubernetes workspaces +// can override it (AGENT_INSTALL_PATH) to a writable path when running +// non-root, since the default requires root to write. +func (r *runner) agentContainerPath() string { + return r.workspaceConfig.Agent.ContainerInstallPath() +} + +func (r *runner) newBinarySource() (delivery.BinarySourceFunc, error) { + mgr, err := agent.NewBinaryManager(r.resolvedAgentDownloadURL()) if err != nil { return nil, err } @@ -239,8 +256,8 @@ func (r *runner) legacyInject(ctx context.Context, timeout time.Duration) error }) }, IsLocal: false, - RemoteAgentPath: pkgconfig.ContainerDevsyHelperLocation, - DownloadURL: pkgconfig.DefaultAgentDownloadURL(), + RemoteAgentPath: r.agentContainerPath(), + DownloadURL: r.resolvedAgentDownloadURL(), PreferDownloadFromRemoteUrl: new(false), Timeout: timeout, }) @@ -355,7 +372,7 @@ func (r *runner) compressWorkspaceConfig() (string, error) { func (r *runner) buildSetupCommand(compressed, workspaceConfigCompressed string) string { log.Infof("setting up container") args := []string{ - shellescape.Quote(pkgconfig.ContainerDevsyHelperLocation), + shellescape.Quote(r.agentContainerPath()), "internal", "agent", "container", @@ -534,7 +551,7 @@ func (r *runner) executeSetup( func (r *runner) buildSSHTunnelCommand() string { args := []string{ - shellescape.Quote(pkgconfig.ContainerDevsyHelperLocation), + shellescape.Quote(r.agentContainerPath()), "internal", "ssh-server", names.Flag(names.Stdio), } diff --git a/pkg/devcontainer/setup/container_data_dir_symlink_unix_test.go b/pkg/devcontainer/setup/container_data_dir_symlink_unix_test.go new file mode 100644 index 000000000..1b8c12511 --- /dev/null +++ b/pkg/devcontainer/setup/container_data_dir_symlink_unix_test.go @@ -0,0 +1,24 @@ +//go:build darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris + +package setup + +import ( + "os" + "path/filepath" + "testing" +) + +func TestSecuredContainerDataDir_RejectsSymlink(t *testing.T) { + target := filepath.Join(t.TempDir(), "target") + if err := os.Mkdir(target, 0o755); err != nil { // #nosec G301 -- test directory + t.Fatalf("mkdir target: %v", err) + } + link := filepath.Join(t.TempDir(), "devsy-data") + if err := os.Symlink(target, link); err != nil { + t.Fatalf("symlink %s -> %s: %v", link, target, err) + } + + if got := securedContainerDataDir(link); got != "" { + t.Errorf("securedContainerDataDir() = %q, want empty", got) + } +} diff --git a/pkg/devcontainer/setup/container_data_dir_test.go b/pkg/devcontainer/setup/container_data_dir_test.go new file mode 100644 index 000000000..bc31ad9bb --- /dev/null +++ b/pkg/devcontainer/setup/container_data_dir_test.go @@ -0,0 +1,77 @@ +package setup + +import ( + "os" + "path/filepath" + "testing" +) + +func TestSecuredContainerDataDir_CreatesWithExpectedPermissions(t *testing.T) { + dir := filepath.Join(t.TempDir(), "devsy-data") + + got := securedContainerDataDir(dir) + if got != dir { + t.Fatalf("securedContainerDataDir() = %q, want %q", got, dir) + } + + info, err := os.Stat(dir) + if err != nil { + t.Fatalf("stat %s: %v", dir, err) + } + if perm := info.Mode().Perm(); perm != 0o755 { + t.Errorf("mode = %o, want 0755", perm) + } +} + +func TestSecuredContainerDataDir_NarrowsPreExistingLaxPermissions(t *testing.T) { + dir := filepath.Join(t.TempDir(), "devsy-data") + if err := os.Mkdir(dir, 0o777); err != nil { // #nosec G301 -- deliberately lax mode under test + t.Fatalf("mkdir %s: %v", dir, err) + } + if err := os.Chmod(dir, 0o777); err != nil { // #nosec G302 -- deliberately lax mode under test + t.Fatalf("chmod %s: %v", dir, err) + } + info, err := os.Stat(dir) + if err != nil { + t.Fatalf("stat %s: %v", dir, err) + } + if perm := info.Mode().Perm(); perm != 0o777 { + t.Fatalf("mode = %o, want 0777 before narrowing", perm) + } + + got := securedContainerDataDir(dir) + if got != dir { + t.Fatalf("securedContainerDataDir() = %q, want %q", got, dir) + } + + info, err = os.Stat(dir) + if err != nil { + t.Fatalf("stat %s: %v", dir, err) + } + if perm := info.Mode().Perm(); perm != 0o755 { + t.Errorf("mode = %o, want 0755 after narrowing", perm) + } +} + +func TestSecuredContainerDataDir_ReturnsEmptyWhenPathIsAFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "not-a-dir") + if err := os.WriteFile(path, []byte("x"), 0o600); err != nil { + t.Fatalf("write %s: %v", path, err) + } + + if got := securedContainerDataDir(path); got != "" { + t.Errorf("securedContainerDataDir() = %q, want empty", got) + } +} + +func TestDirIsWritable_TrueForOwnedDir(t *testing.T) { + if !dirIsWritable(t.TempDir()) { + t.Error("expected writable temp dir to report writable") + } +} + +func TestDirIsWritable_FalseForNonexistentDir(t *testing.T) { + if dirIsWritable(filepath.Join(t.TempDir(), "does-not-exist")) { + t.Error("expected nonexistent dir to report not writable") + } +} diff --git a/pkg/devcontainer/setup/secure_dir_other.go b/pkg/devcontainer/setup/secure_dir_other.go new file mode 100644 index 000000000..2059ef045 --- /dev/null +++ b/pkg/devcontainer/setup/secure_dir_other.go @@ -0,0 +1,28 @@ +//go:build !darwin && !dragonfly && !freebsd && !linux && !netbsd && !openbsd && !solaris + +package setup + +import ( + "fmt" + "os" +) + +// secureContainerDataDir provides the closest available protection on targets +// without descriptor-based no-follow directory operations. +func secureContainerDataDir(dir string) error { + info, err := os.Lstat(dir) + switch { + case err == nil: + if info.IsDir() { + return os.Chmod(dir, 0o755) + } + return fmt.Errorf("path is not a directory") + case !os.IsNotExist(err): + return err + } + + if err := os.Mkdir(dir, 0o755); err != nil { + return err + } + return nil +} diff --git a/pkg/devcontainer/setup/secure_dir_unix.go b/pkg/devcontainer/setup/secure_dir_unix.go new file mode 100644 index 000000000..5fdcd42db --- /dev/null +++ b/pkg/devcontainer/setup/secure_dir_unix.go @@ -0,0 +1,90 @@ +//go:build darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris + +package setup + +import ( + "errors" + "fmt" + "path/filepath" + + "golang.org/x/sys/unix" +) + +// secureContainerDataDir creates or opens the final directory component without +// following a symlink, then applies the mode through the open descriptor. +func secureContainerDataDir(dir string) error { + parentFD, name, err := openContainerDataParent(dir) + if err != nil { + return err + } + defer func() { _ = unix.Close(parentFD) }() + + if err := ensureContainerDataDir(parentFD, name); err != nil { + return err + } + fd, err := openContainerDataDir(parentFD, name) + if err != nil { + return err + } + defer func() { _ = unix.Close(fd) }() + return secureOpenedContainerDataDir(fd) +} + +func openContainerDataParent(dir string) (int, string, error) { + parent := filepath.Dir(dir) + name := filepath.Base(dir) + // The parent is a fixed system directory (/var or /tmp). macOS exposes + // /tmp as a symlink, so only the final data-directory component is opened + // with O_NOFOLLOW below. + fd, err := unix.Open( + parent, + unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, + 0, + ) + if err != nil { + return 0, "", fmt.Errorf("open parent: %w", err) + } + return fd, name, nil +} + +func ensureContainerDataDir(parentFD int, name string) error { + if err := unix.Mkdirat(parentFD, name, 0o755); err != nil && !errors.Is(err, unix.EEXIST) { + return fmt.Errorf("create directory: %w", err) + } + return nil +} + +func openContainerDataDir(parentFD int, name string) (int, error) { + fd, err := unix.Openat( + parentFD, + name, + unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, + 0, + ) + if err != nil { + return 0, fmt.Errorf("open directory: %w", err) + } + return fd, nil +} + +func secureOpenedContainerDataDir(fd int) error { + var stat unix.Stat_t + if err := unix.Fstat(fd, &stat); err != nil { + return fmt.Errorf("stat directory: %w", err) + } + currentUID := currentUnixUID() + if currentUID != stat.Uid { + return fmt.Errorf("directory is owned by uid %d", stat.Uid) + } + if stat.Mode&unix.S_IFMT != unix.S_IFDIR { + return fmt.Errorf("path is not a directory") + } + if err := unix.Fchmod(fd, 0o755); err != nil { + return fmt.Errorf("chmod directory: %w", err) + } + return nil +} + +func currentUnixUID() uint32 { + return uint32(unix.Geteuid()) //nolint:gosec // euid is nonnegative and Unix UIDs are uint32 +} diff --git a/pkg/devcontainer/setup/setup.go b/pkg/devcontainer/setup/setup.go index b8cfb8647..f8677c3fa 100644 --- a/pkg/devcontainer/setup/setup.go +++ b/pkg/devcontainer/setup/setup.go @@ -13,6 +13,7 @@ import ( "sort" "strconv" "strings" + "sync" "github.com/devsy-org/api/pkg/devsy" "github.com/devsy-org/devsy/pkg/agent/tunnel" @@ -63,7 +64,9 @@ func SetupContainerPreAttach( return DeferredHooks{}, err } - writeResultFile(cfg) + if err := writeResultFile(cfg); err != nil { + return DeferredHooks{}, fmt.Errorf("write container result: %w", err) + } if err := setupWorkspaceOwnership(cfg); err != nil { return DeferredHooks{}, err @@ -204,16 +207,48 @@ func secretMountPath(target string) (string, error) { return filepath.Join(config.SecretsMountDir, target), nil } -func writeResultFile(cfg *ContainerSetupConfig) { +func writeResultFile(cfg *ContainerSetupConfig) error { rawBytes, err := json.Marshal(cfg.SetupInfo) if err != nil { - log.Warnf("error marshal result: %v", err) - return + return fmt.Errorf("marshal result: %w", err) } - if err := writeResultFileTo(pkgconfig.DevContainerResultPath, rawBytes); err != nil { - log.Warnf("error write result to %s: %v", pkgconfig.DevContainerResultPath, err) + activePath := pkgconfig.DevContainerResultPath + if err := writeResultFileTo(activePath, rawBytes); err != nil { + log.Debugf( + "%s is not writable (%v), falling back to %s", + activePath, + err, + pkgconfig.DevContainerResultFallbackPath, + ) + activePath = pkgconfig.DevContainerResultFallbackPath + if err := writeResultFileTo(activePath, rawBytes); err != nil { + return fmt.Errorf("write result to %s: %w", activePath, err) + } + } + if err := writeResultPathSelector(activePath); err != nil { + return fmt.Errorf("select result path %s: %w", activePath, err) + } + return nil +} + +func writeResultPathSelector(activePath string) error { + selectorPath := pkgconfig.DevContainerResultSelectorPath + inactiveSelectorPath := pkgconfig.DevContainerResultFallbackSelectorPath + if activePath == pkgconfig.DevContainerResultFallbackPath { + selectorPath = pkgconfig.DevContainerResultFallbackSelectorPath + inactiveSelectorPath = pkgconfig.DevContainerResultSelectorPath + } + if securedContainerDataDir(filepath.Dir(selectorPath)) == "" { + return fmt.Errorf("create or secure %s", filepath.Dir(selectorPath)) } + if err := sharedfile.WriteFile(selectorPath, []byte(activePath), 0o644); err != nil { + return err + } + if err := os.Remove(inactiveSelectorPath); err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("remove inactive result selector %s: %w", inactiveSelectorPath, err) + } + return nil } // writeResultFileTo writes rawBytes to path at 0644: readable by any @@ -232,8 +267,9 @@ func writeResultFileTo(path string, rawBytes []byte) error { return sharedfile.WidenWithSudoFallback(context.Background(), path, 0o644) } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { // #nosec G301 - return fmt.Errorf("create %s: %w", filepath.Dir(path), err) + dir := filepath.Dir(path) + if securedContainerDataDir(dir) == "" { + return fmt.Errorf("create or secure %s", dir) } return sharedfile.WriteFile(path, rawBytes, 0o644) } @@ -349,8 +385,13 @@ func chownWorkspace(setupInfo *config.Result, recursive bool) error { workspaceRoot := filepath.Dir(workspaceFolder) if workspaceRoot != "/" { log.Infof("chown workspace: user=%s, workspaceRoot=%s", user, workspaceRoot) - if err := copy2.Chown(workspaceRoot, user); err != nil && !copy2.Unsupported(err) { + if err := copy2.Chown(workspaceRoot, user); err != nil && !copy2.DeniedByFilesystem(err) { return fmt.Errorf("chown %s: %w", workspaceRoot, err) + } else if err != nil { + // A non-root container (e.g. an OpenShift restricted-SCC pod) does + // not own workspaceRoot and cannot chown it; the workspace folder + // itself may still get a usable owner below. + log.Debugf("chown workspace: %s kept its owner: %v", workspaceRoot, err) } } @@ -491,7 +532,11 @@ func setupKubeConfig( setupInfo *config.Result, tunnelClient tunnel.TunnelClient, ) error { - if shouldSkipKubeConfig(tunnelClient) { + skip, err := shouldSkipKubeConfig(tunnelClient) + if err != nil { + return err + } + if skip { return nil } @@ -517,12 +562,16 @@ func setupKubeConfig( return nil } -func shouldSkipKubeConfig(tunnelClient tunnel.TunnelClient) bool { +func shouldSkipKubeConfig(tunnelClient tunnel.TunnelClient) (bool, error) { if tunnelClient == nil { - return true + return true, nil } - markerPath := filepath.Join(pkgconfig.ContainerDataDir, "setupKubeConfig.marker") + dir := containerDataDir() + if dir == "" { + return false, fmt.Errorf("container data directory is unavailable") + } + markerPath := filepath.Join(dir, "setupKubeConfig.marker") info, err := os.Stat(markerPath) if err == nil { if info.Mode().Perm()&0o022 != 0 { @@ -531,14 +580,14 @@ func shouldSkipKubeConfig(tunnelClient tunnel.TunnelClient) bool { markerPath, info.Mode().Perm(), ) - return false + return false, nil } - return true + return true, nil } if !errors.Is(err, os.ErrNotExist) { log.Warnf("error checking marker file in shouldSkipKubeConfig: %v", err) } - return false + return false, nil } func writeKubeConfig(setupInfo *config.Result, configData string) error { @@ -606,8 +655,12 @@ func ensureKubeConfigMaps(config *clientcmdapi.Config) *clientcmdapi.Config { // markerExists reports whether the named marker exists with the expected // content; empty markerContent matches any existing marker. It never writes. func markerExists(markerName string, markerContent string) (bool, error) { + dir := containerDataDir() + if dir == "" { + return false, nil + } // #nosec G703 -- markerName is an internal constant - path := filepath.Join(pkgconfig.ContainerDataDir, markerName+".marker") + path := filepath.Join(dir, markerName+".marker") // #nosec G304 -- path is built from internal constants t, err := os.ReadFile(path) if err != nil { @@ -621,12 +674,15 @@ func markerExists(markerName string, markerContent string) (bool, error) { // writeMarker records that the work gated by markerExists has completed. func writeMarker(markerName string, markerContent string) error { - // #nosec G703 -- markerName is an internal constant - path := filepath.Join(pkgconfig.ContainerDataDir, markerName+".marker") - // #nosec G301 -- Standard directory permissions - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return fmt.Errorf("create %s: %w", filepath.Dir(path), err) + dir := containerDataDir() + if dir == "" { + return fmt.Errorf("container data directory is unavailable") } + if securedContainerDataDir(dir) == "" { + return fmt.Errorf("create or secure %s", dir) + } + // #nosec G703 -- markerName is an internal constant + path := filepath.Join(dir, markerName+".marker") // #nosec G703 -- path is built from internal constants if err := os.WriteFile(path, []byte(markerContent), 0o600); err != nil { return fmt.Errorf("write marker: %w", err) @@ -647,6 +703,46 @@ func markerFileExists(markerName string, markerContent string) (bool, error) { return false, nil } +// writableContainerDataDirOnce resolves a writable, user-owned data directory once. +var writableContainerDataDirOnce = sync.OnceValue(func() string { + if dir := securedContainerDataDir(pkgconfig.ContainerDataDir); dir != "" { + return dir + } + fallback := pkgconfig.ContainerDataDirFallback + if dir := securedContainerDataDir(fallback); dir != "" { + return dir + } + log.Warnf("%s could not be created or secured to the current user", fallback) + return "" +}) + +// securedContainerDataDir returns dir only when it is user-owned and writable. +func securedContainerDataDir(dir string) string { + if err := secureContainerDataDir(dir); err != nil { + log.Debugf("%s is not a secure directory: %v", dir, err) + return "" + } + if !dirIsWritable(dir) { + return "" + } + return dir +} + +func dirIsWritable(dir string) bool { + f, err := os.CreateTemp(dir, ".devsy-write-probe-*") + if err != nil { + return false + } + name := f.Name() + _ = f.Close() + _ = os.Remove(name) + return true +} + +func containerDataDir() string { + return writableContainerDataDirOnce() +} + func setupPlatformGitCredentials( ctx context.Context, userName string, diff --git a/pkg/devcontainer/setup_test.go b/pkg/devcontainer/setup_test.go index c6014a6b5..88ca4a56e 100644 --- a/pkg/devcontainer/setup_test.go +++ b/pkg/devcontainer/setup_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/devsy-org/devsy/pkg/agent/delivery" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/docker" provider2 "github.com/devsy-org/devsy/pkg/provider" @@ -237,3 +238,54 @@ func TestBuildResult_DefaultUserEnvProbeEmpty(t *testing.T) { ) } } + +const testAgentInstallPath = "/home/vscode/.local/bin/devsy" + +func TestAgentContainerPath(t *testing.T) { + cases := []struct { + name string + driverName string + agentInstallPath string + want string + }{ + { + name: "non-kubernetes driver always uses the default", + driverName: provider2.DockerDriver, + want: pkgconfig.ContainerDevsyHelperLocation, + }, + { + name: "kubernetes driver with no override uses the default", + driverName: provider2.KubernetesDriver, + want: pkgconfig.ContainerDevsyHelperLocation, + }, + { + name: "kubernetes driver with AGENT_INSTALL_PATH uses the override", + driverName: provider2.KubernetesDriver, + agentInstallPath: testAgentInstallPath, + want: testAgentInstallPath, + }, + { + name: "non-kubernetes driver ignores a stray AgentInstallPath value", + driverName: provider2.DockerDriver, + agentInstallPath: testAgentInstallPath, + want: pkgconfig.ContainerDevsyHelperLocation, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + r := &runner{ + workspaceConfig: &provider2.AgentWorkspaceInfo{ + Agent: provider2.ProviderAgentConfig{ + Driver: c.driverName, + Kubernetes: provider2.ProviderKubernetesDriverConfig{ + AgentInstallPath: c.agentInstallPath, + }, + }, + }, + } + if got := r.agentContainerPath(); got != c.want { + t.Errorf("agentContainerPath() = %q, want %q", got, c.want) + } + }) + } +} diff --git a/pkg/driver/kubernetes/client.go b/pkg/driver/kubernetes/client.go index 64b311fa4..b631520dc 100644 --- a/pkg/driver/kubernetes/client.go +++ b/pkg/driver/kubernetes/client.go @@ -131,20 +131,35 @@ func (c *Client) Exec(ctx context.Context, options *ExecStreamOptions) error { return err } - errChan := make(chan error) - go func() { - errChan <- exec.StreamWithContext(ctx, remotecommand.StreamOptions{ + return waitForStream(ctx, func(streamCtx context.Context) error { + return exec.StreamWithContext(streamCtx, remotecommand.StreamOptions{ Stdin: options.Stdin, Stdout: options.Stdout, Stderr: options.Stderr, }) + }) +} + +// waitForStream waits for the stream to complete or the context to be canceled, +// returning the first error that occurs. +func waitForStream(ctx context.Context, stream func(context.Context) error) error { + errChan := make(chan error, 1) + go func() { + errChan <- stream(ctx) }() select { case <-ctx.Done(): - <-errChan - return nil - case err = <-errChan: + if streamErr := <-errChan; streamErr != nil { + return streamErr + } + return ctx.Err() + case err := <-errChan: + if err == nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return ctxErr + } + } return err } } diff --git a/pkg/driver/kubernetes/client_test.go b/pkg/driver/kubernetes/client_test.go new file mode 100644 index 000000000..15c3d2ad6 --- /dev/null +++ b/pkg/driver/kubernetes/client_test.go @@ -0,0 +1,79 @@ +package kubernetes + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWaitForStream_ReturnsStreamError(t *testing.T) { + wantErr := errors.New("boom") + + err := waitForStream(context.Background(), func(_ context.Context) error { + return wantErr + }) + + require.ErrorIs(t, err, wantErr) +} + +func TestWaitForStream_ReturnsNilOnSuccess(t *testing.T) { + err := waitForStream(context.Background(), func(_ context.Context) error { + return nil + }) + + assert.NoError(t, err) +} + +// TestWaitForStream_CancellationIsNeverReportedAsSuccess is a regression test: +// a deliberately cancelled attempt (e.g. our own attempt deadline firing on a +// stalled exec stream) must be reported as a failure, not silently treated as +// a successful delivery. +func TestWaitForStream_CancellationIsNeverReportedAsSuccess(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + + streamReturned := make(chan struct{}) + err := waitForStream(ctx, func(streamCtx context.Context) error { + <-streamCtx.Done() + close(streamReturned) + return nil + }) + + require.Error(t, err) + assert.ErrorIs(t, err, context.DeadlineExceeded) + <-streamReturned +} + +// TestWaitForStream_NeverReportsSuccessWhenContextAlreadyCancelled is a +// regression test for a race in the original select: ctx.Done() and errChan +// can both be ready at once, and selecting the errChan case with a nil +// stream error must still surface the cancellation rather than success. +func TestWaitForStream_NeverReportsSuccessWhenContextAlreadyCancelled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + for range 200 { + err := waitForStream(ctx, func(_ context.Context) error { + return nil + }) + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) + } +} + +func TestWaitForStream_PropagatesRealStreamErrorEvenAfterCancellation(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + wantErr := errors.New("stream reported a real error while unwinding") + + err := waitForStream(ctx, func(streamCtx context.Context) error { + <-streamCtx.Done() + return wantErr + }) + + require.ErrorIs(t, err, wantErr) +} diff --git a/pkg/driver/kubernetes/helper.go b/pkg/driver/kubernetes/helper.go index d3e5b2777..37f118bfe 100644 --- a/pkg/driver/kubernetes/helper.go +++ b/pkg/driver/kubernetes/helper.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/log" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -132,3 +133,97 @@ func parseResource(resourceName string) (string, resource.Quantity, error) { return splittedResource[0], quantity, nil } + +func parseSecurityContext(raw string) (*corev1.SecurityContext, error) { + if raw == "" { + return nil, nil + } + + sc := &corev1.SecurityContext{} + errInline := yaml.UnmarshalStrict([]byte(raw), sc) + if errInline == nil { + return sc, nil + } + + p, err := filepath.Abs(raw) + if err != nil { + return nil, fmt.Errorf( + "parsing security context failed: %w (inline) or %w (file)", + errInline, + err, + ) + } + // #nosec G304 -- path comes from the operator-controlled AGENT_SECURITY_CONTEXT provider option + body, err := os.ReadFile(p) + if err != nil { + return nil, fmt.Errorf( + "parsing security context failed: %w (inline) or %w (file)", + errInline, + err, + ) + } + if err = yaml.UnmarshalStrict(body, sc); err == nil { + return sc, nil + } + + return nil, fmt.Errorf( + "parsing security context failed: %w (inline) or %w (file)", + errInline, + err, + ) +} + +func applyBaseFallback(override, base *corev1.SecurityContext) { + if base == nil { + return + } + if override.Capabilities == nil { + override.Capabilities = base.Capabilities + } + if override.Privileged == nil { + override.Privileged = base.Privileged + } +} + +func resolveContainerSecurityContext( + strictSecurity, agentSecurityContext string, + base *corev1.SecurityContext, +) (*corev1.SecurityContext, error) { + override, err := parseSecurityContext(agentSecurityContext) + if err != nil { + return nil, fmt.Errorf("AGENT_SECURITY_CONTEXT: %w", err) + } + if override != nil { + applyBaseFallback(override, base) + return override, nil + } + + if strictSecurity != pkgconfig.BoolTrue { + return base, nil + } + if base == nil { + return nil, nil + } + return &corev1.SecurityContext{ + Capabilities: base.Capabilities, + Privileged: base.Privileged, + }, nil +} + +type securityContextOptions struct { + Capabilities *corev1.Capabilities + Privileged *bool + StrictSecurity string + AgentSecurityContext string +} + +func (o securityContextOptions) resolve() (*corev1.SecurityContext, error) { + base := &corev1.SecurityContext{ + Capabilities: o.Capabilities, + Privileged: o.Privileged, + RunAsUser: new(int64), + RunAsGroup: new(int64), + RunAsNonRoot: new(bool), + } + return resolveContainerSecurityContext(o.StrictSecurity, o.AgentSecurityContext, base) +} diff --git a/pkg/driver/kubernetes/init_container.go b/pkg/driver/kubernetes/init_container.go index a8f42b6f9..61eeb4c16 100644 --- a/pkg/driver/kubernetes/init_container.go +++ b/pkg/driver/kubernetes/init_container.go @@ -4,7 +4,6 @@ import ( "fmt" "strings" - pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/driver" corev1 "k8s.io/api/core/v1" ) @@ -13,28 +12,24 @@ func (k *KubernetesDriver) getInitContainers( options *driver.RunOptions, pod *corev1.Pod, initialize bool, -) []corev1.Container { +) ([]corev1.Container, error) { if !initialize { - // don't build init container and clean up existing one if defined - return filterOutInitContainer(pod.Spec.InitContainers) + return filterOutInitContainer(pod.Spec.InitContainers), nil } volumeMounts, commands := buildVolumeCopyCommands(options) retContainers, existingInitContainer := splitInitContainers(pod.Spec.InitContainers) - - // check if there is at least one mount if len(volumeMounts) == 0 { - return retContainers + return retContainers, nil } - securityContext := &corev1.SecurityContext{ - RunAsUser: &[]int64{0}[0], - RunAsGroup: &[]int64{0}[0], - RunAsNonRoot: &[]bool{false}[0], - } - if k.options.StrictSecurity == pkgconfig.BoolTrue { - securityContext = nil + securityContext, err := (securityContextOptions{ + StrictSecurity: k.options.StrictSecurity, + AgentSecurityContext: k.options.AgentSecurityContext, + }).resolve() + if err != nil { + return nil, err } resources := corev1.ResourceRequirements{} @@ -55,7 +50,7 @@ func (k *KubernetesDriver) getInitContainers( mergeContainer(&initContainer, existingInitContainer) retContainers = append(retContainers, initContainer) - return retContainers + return retContainers, nil } func filterOutInitContainer(containers []corev1.Container) []corev1.Container { @@ -111,6 +106,42 @@ func splitInitContainers(containers []corev1.Container) ([]corev1.Container, *co return retContainers, existingInitContainer } +func overrideIfSet[T any](dst **T, src *T) { + if src != nil { + *dst = src + } +} + +func mergeSecurityContext(dst, src *corev1.SecurityContext) *corev1.SecurityContext { + if src == nil { + return dst + } + merged := corev1.SecurityContext{} + if dst != nil { + merged = *dst + } + if src.RunAsUser == nil && + src.RunAsNonRoot != nil && *src.RunAsNonRoot && + merged.RunAsUser != nil && *merged.RunAsUser == 0 { + // A template that sets runAsNonRoot=true without runAsUser must + // remove the generated container's implicit root UID. + merged.RunAsUser = nil + } + overrideIfSet(&merged.Capabilities, src.Capabilities) + overrideIfSet(&merged.Privileged, src.Privileged) + overrideIfSet(&merged.SELinuxOptions, src.SELinuxOptions) + overrideIfSet(&merged.WindowsOptions, src.WindowsOptions) + overrideIfSet(&merged.RunAsUser, src.RunAsUser) + overrideIfSet(&merged.RunAsGroup, src.RunAsGroup) + overrideIfSet(&merged.RunAsNonRoot, src.RunAsNonRoot) + overrideIfSet(&merged.ReadOnlyRootFilesystem, src.ReadOnlyRootFilesystem) + overrideIfSet(&merged.AllowPrivilegeEscalation, src.AllowPrivilegeEscalation) + overrideIfSet(&merged.ProcMount, src.ProcMount) + overrideIfSet(&merged.SeccompProfile, src.SeccompProfile) + overrideIfSet(&merged.AppArmorProfile, src.AppArmorProfile) + return &merged +} + func mergeContainer(dst, src *corev1.Container) { if src == nil { return @@ -124,7 +155,5 @@ func mergeContainer(dst, src *corev1.Container) { dst.VolumeMounts...) dst.ImagePullPolicy = src.ImagePullPolicy - if dst.SecurityContext == nil && src.SecurityContext != nil { - dst.SecurityContext = src.SecurityContext - } + dst.SecurityContext = mergeSecurityContext(dst.SecurityContext, src.SecurityContext) } diff --git a/pkg/driver/kubernetes/run.go b/pkg/driver/kubernetes/run.go index bc6fc2e49..da61507ee 100644 --- a/pkg/driver/kubernetes/run.go +++ b/pkg/driver/kubernetes/run.go @@ -143,11 +143,15 @@ func (k *KubernetesDriver) buildPod( return nil, err } - initContainers := k.getInitContainers(options, pod, initialize) + initContainers, err := k.getInitContainers(options, pod, initialize) + if err != nil { + return nil, err + } volumeMounts, tmpfsVolumes := buildVolumeMounts(mount, options) capabilities := buildCapabilities(options.CapAdd) envVars, daemonConfig := splitEnvVars(options.Env) + envVars = withAgentInstallPathEnv(envVars, k.options.AgentInstallPath) serviceAccount, err := k.ensureServiceAccount(ctx, id) if err != nil { @@ -169,7 +173,7 @@ func (k *KubernetesDriver) buildPod( return nil, err } - k.assemblePodSpec(pod, id, &podSpecInputs{ + if err := k.assemblePodSpec(pod, id, &podSpecInputs{ options: options, meta: meta, initContainers: initContainers, @@ -180,7 +184,9 @@ func (k *KubernetesDriver) buildPod( serviceAccount: serviceAccount, daemonConfigSecretName: daemonConfigSecretName, pullSecretsCreated: pullSecretsCreated, - }) + }); err != nil { + return nil, err + } return pod, nil } @@ -198,30 +204,39 @@ type podSpecInputs struct { pullSecretsCreated bool } -func (k *KubernetesDriver) assemblePodSpec(pod *corev1.Pod, id string, in *podSpecInputs) { +func (k *KubernetesDriver) assemblePodSpec(pod *corev1.Pod, id string, in *podSpecInputs) error { pod.Name = id pod.Labels = in.meta.labels pod.Spec.ServiceAccountName = in.serviceAccount pod.Spec.NodeSelector = in.meta.nodeSelector pod.Spec.InitContainers = in.initContainers - pod.Spec.Containers = getContainers( - pod, - in.options.Image, - in.options.Entrypoint, - in.options.Cmd, - in.envVars, - in.volumeMounts, - in.capabilities, - in.meta.resources, - in.options.Privileged, - k.options.StrictSecurity, - in.daemonConfigSecretName, - ) + + containers, err := getContainers(pod, devsyContainerInputs{ + ImageName: in.options.Image, + Entrypoint: in.options.Entrypoint, + Args: in.options.Cmd, + EnvVars: in.envVars, + VolumeMounts: in.volumeMounts, + Resources: in.meta.resources, + Security: securityContextOptions{ + Capabilities: in.capabilities, + Privileged: in.options.Privileged, + StrictSecurity: k.options.StrictSecurity, + AgentSecurityContext: k.options.AgentSecurityContext, + }, + DaemonConfigSecretName: in.daemonConfigSecretName, + }) + if err != nil { + return err + } + pod.Spec.Containers = containers + pod.Spec.Volumes = append( getVolumes(pod, id, in.daemonConfigSecretName), in.tmpfsVolumes...) k.finalizePodSpec(pod, id, in.pullSecretsCreated) + return nil } func (k *KubernetesDriver) resolveWorkspaceMount( @@ -341,6 +356,21 @@ func splitEnvVars(env map[string]string) ([]corev1.EnvVar, string) { return envVars, daemonConfig } +// withAgentInstallPathEnv adds the AGENT_INSTALL_PATH env var to envVars if installPath is non-empty, +// replacing any existing value. +func withAgentInstallPathEnv(envVars []corev1.EnvVar, installPath string) []corev1.EnvVar { + if installPath == "" { + return envVars + } + for i := range envVars { + if envVars[i].Name == pkgconfig.EnvAgentPath { + envVars[i].Value = installPath + return envVars + } + } + return append(envVars, corev1.EnvVar{Name: pkgconfig.EnvAgentPath, Value: installPath}) +} + func (k *KubernetesDriver) ensureServiceAccount( ctx context.Context, id string, @@ -429,6 +459,11 @@ func (k *KubernetesDriver) finalizePodSpec(pod *corev1.Pod, id string, pullSecre FSGroupChangePolicy: ptr.To(corev1.FSGroupChangeOnRootMismatch), } } + if (k.options.KubernetesUserNamespaces == pkgconfig.BoolTrue || + k.options.StrictSecurity == pkgconfig.BoolTrue || + k.options.AgentSecurityContext != "") && pod.Spec.HostUsers == nil { + pod.Spec.HostUsers = new(bool) + } if k.options.KubernetesPullSecretsEnabled == pkgconfig.BoolTrue && pullSecretsCreated { pod.Spec.ImagePullSecrets = []corev1.LocalObjectReference{{Name: getPullSecretsName(id)}} } @@ -509,47 +544,42 @@ func (k *KubernetesDriver) runPod(ctx context.Context, id string, pod *corev1.Po return nil } -func getContainers( - pod *corev1.Pod, - imageName, - entrypoint string, - args []string, - envVars []corev1.EnvVar, - volumeMounts []corev1.VolumeMount, - capabilities *corev1.Capabilities, - resources corev1.ResourceRequirements, - privileged *bool, - strictSecurity string, - daemonConfigSecretName string, -) []corev1.Container { +type devsyContainerInputs struct { + ImageName string + Entrypoint string + Args []string + EnvVars []corev1.EnvVar + VolumeMounts []corev1.VolumeMount + Resources corev1.ResourceRequirements + Security securityContextOptions + DaemonConfigSecretName string +} + +func getContainers(pod *corev1.Pod, in devsyContainerInputs) ([]corev1.Container, error) { + daemonConfigSecretName := in.DaemonConfigSecretName + volumeMounts := in.VolumeMounts if daemonConfigSecretName != "" { volumeMounts = append(volumeMounts, corev1.VolumeMount{ Name: DevContainerName + "-daemon-config", MountPath: "/var/run/secrets/" + DevContainerName, }) } - devsyContainer := corev1.Container{ - Name: DevContainerName, - Image: imageName, - Command: []string{entrypoint}, - Args: args, - Env: envVars, - Resources: resources, - VolumeMounts: volumeMounts, - SecurityContext: &corev1.SecurityContext{ - Capabilities: capabilities, - Privileged: privileged, - RunAsUser: &[]int64{0}[0], - RunAsGroup: &[]int64{0}[0], - RunAsNonRoot: &[]bool{false}[0], - }, - } - if strictSecurity == pkgconfig.BoolTrue { - devsyContainer.SecurityContext = nil + securityContext, err := in.Security.resolve() + if err != nil { + return nil, err } - // merge with existing container if it exists + devsyContainer := corev1.Container{ + Name: DevContainerName, + Image: in.ImageName, + Command: []string{in.Entrypoint}, + Args: in.Args, + Env: in.EnvVars, + Resources: in.Resources, + VolumeMounts: volumeMounts, + SecurityContext: securityContext, + } var existingDevsyContainer *corev1.Container retContainers := []corev1.Container{} if pod != nil { @@ -565,7 +595,7 @@ func getContainers( mergeContainer(&devsyContainer, existingDevsyContainer) retContainers = append(retContainers, devsyContainer) - return retContainers + return retContainers, nil } func getVolumes(pod *corev1.Pod, id string, daemonConfigSecretName string) []corev1.Volume { diff --git a/pkg/driver/kubernetes/run_test.go b/pkg/driver/kubernetes/run_test.go new file mode 100644 index 000000000..ecc7fc5f5 --- /dev/null +++ b/pkg/driver/kubernetes/run_test.go @@ -0,0 +1,420 @@ +package kubernetes + +import ( + "testing" + + pkgconfig "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" + provider2 "github.com/devsy-org/devsy/pkg/provider" + corev1 "k8s.io/api/core/v1" +) + +const ( + testImageName = "image" + testEntrypoint = "entrypoint" + testEnvVarName = "FOO" + testEnvVarValue = "bar" +) + +func TestGetContainersDefaultRunsAsRoot(t *testing.T) { + containers, err := getContainers(nil, devsyContainerInputs{ + ImageName: testImageName, Entrypoint: testEntrypoint, + Resources: corev1.ResourceRequirements{}, Security: securityContextOptions{}, + }) + if err != nil { + t.Fatalf("getContainers: %v", err) + } + sc := containers[0].SecurityContext + if sc == nil || sc.RunAsUser == nil || *sc.RunAsUser != 0 { + t.Errorf("default SecurityContext = %+v, want RunAsUser=0", sc) + } +} + +func TestWithAgentInstallPathEnv_AppendsWhenSet(t *testing.T) { + envVars := withAgentInstallPathEnv( + []corev1.EnvVar{{Name: testEnvVarName, Value: testEnvVarValue}}, + "/home/vscode/.local/bin/devsy", + ) + + if len(envVars) != 2 { + t.Fatalf("envVars = %+v, want 2 entries", envVars) + } + got := envVars[1] + want := corev1.EnvVar{Name: pkgconfig.EnvAgentPath, Value: "/home/vscode/.local/bin/devsy"} + if got != want { + t.Errorf("envVars[1] = %+v, want %+v", got, want) + } +} + +func TestWithAgentInstallPathEnv_LeavesUnchangedWhenUnset(t *testing.T) { + original := []corev1.EnvVar{{Name: testEnvVarName, Value: testEnvVarValue}} + + got := withAgentInstallPathEnv(original, "") + + if len(got) != 1 || got[0] != original[0] { + t.Errorf("envVars = %+v, want unchanged %+v", got, original) + } +} + +func TestWithAgentInstallPathEnv_ReplacesExistingEntry(t *testing.T) { + envVars := withAgentInstallPathEnv( + []corev1.EnvVar{ + {Name: testEnvVarName, Value: testEnvVarValue}, + {Name: pkgconfig.EnvAgentPath, Value: "/old/path"}, + }, + "/new/path", + ) + + if len(envVars) != 2 { + t.Fatalf("envVars = %+v, want 2 entries", envVars) + } + want := corev1.EnvVar{Name: pkgconfig.EnvAgentPath, Value: "/new/path"} + if envVars[1] != want { + t.Errorf("envVars[1] = %+v, want %+v", envVars[1], want) + } +} + +func TestGetContainersStrictSecurityClearsRunAs(t *testing.T) { + containers, err := getContainers(nil, devsyContainerInputs{ + ImageName: testImageName, + Entrypoint: testEntrypoint, + Resources: corev1.ResourceRequirements{}, + Security: securityContextOptions{StrictSecurity: pkgconfig.BoolTrue}, + }) + if err != nil { + t.Fatalf("getContainers: %v", err) + } + sc := containers[0].SecurityContext + if sc.RunAsUser != nil { + t.Errorf("RunAsUser = %v, want nil under STRICT_SECURITY", sc.RunAsUser) + } +} + +func TestGetContainersAgentSecurityContextOverride(t *testing.T) { + containers, err := getContainers(nil, devsyContainerInputs{ + ImageName: testImageName, + Entrypoint: testEntrypoint, + Resources: corev1.ResourceRequirements{}, + Security: securityContextOptions{AgentSecurityContext: "runAsUser: 1002010000\n"}, + }) + if err != nil { + t.Fatalf("getContainers: %v", err) + } + sc := containers[0].SecurityContext + if sc.RunAsUser == nil || *sc.RunAsUser != 1002010000 { + t.Errorf("RunAsUser = %v, want 1002010000", sc.RunAsUser) + } +} + +func TestGetContainersInvalidAgentSecurityContextErrors(t *testing.T) { + _, err := getContainers(nil, devsyContainerInputs{ + ImageName: testImageName, + Entrypoint: testEntrypoint, + Resources: corev1.ResourceRequirements{}, + Security: securityContextOptions{AgentSecurityContext: "not: valid: yaml: at: all:"}, + }) + if err == nil { + t.Fatal("expected error for invalid AGENT_SECURITY_CONTEXT") + } +} + +func TestFinalizePodSpecSetsHostUsersFalseWhenStrictSecurityEnabled(t *testing.T) { + k := &KubernetesDriver{ + options: &provider2.ProviderKubernetesDriverConfig{ + StrictSecurity: pkgconfig.BoolTrue, + }, + } + pod := &corev1.Pod{} + + k.finalizePodSpec(pod, "devsy-ws-1", false) + + if pod.Spec.HostUsers == nil || *pod.Spec.HostUsers { + t.Errorf("HostUsers = %v, want false", pod.Spec.HostUsers) + } +} + +func TestFinalizePodSpecSetsHostUsersFalseWhenUserNamespacesEnabled(t *testing.T) { + k := &KubernetesDriver{ + options: &provider2.ProviderKubernetesDriverConfig{ + KubernetesUserNamespaces: pkgconfig.BoolTrue, + }, + } + pod := &corev1.Pod{} + + k.finalizePodSpec(pod, "devsy-ws-1", false) + + if pod.Spec.HostUsers == nil || *pod.Spec.HostUsers { + t.Errorf("HostUsers = %v, want false", pod.Spec.HostUsers) + } +} + +func TestFinalizePodSpecSetsHostUsersFalseWhenAgentSecurityContextSet(t *testing.T) { + k := &KubernetesDriver{ + options: &provider2.ProviderKubernetesDriverConfig{ + AgentSecurityContext: "runAsUser: 1000\n", + }, + } + pod := &corev1.Pod{} + + k.finalizePodSpec(pod, "devsy-ws-1", false) + + if pod.Spec.HostUsers == nil || *pod.Spec.HostUsers { + t.Errorf("HostUsers = %v, want false", pod.Spec.HostUsers) + } +} + +func TestFinalizePodSpecLeavesHostUsersUnsetByDefault(t *testing.T) { + k := &KubernetesDriver{options: &provider2.ProviderKubernetesDriverConfig{}} + pod := &corev1.Pod{} + + k.finalizePodSpec(pod, "devsy-ws-1", false) + + if pod.Spec.HostUsers != nil { + t.Errorf( + "HostUsers = %v, want nil (untouched) by default", + pod.Spec.HostUsers, + ) + } +} + +func TestFinalizePodSpecRespectsTemplateHostUsers(t *testing.T) { + k := &KubernetesDriver{ + options: &provider2.ProviderKubernetesDriverConfig{ + KubernetesUserNamespaces: pkgconfig.BoolTrue, + }, + } + pod := &corev1.Pod{Spec: corev1.PodSpec{HostUsers: new(true)}} + + k.finalizePodSpec(pod, "devsy-ws-1", false) + + if pod.Spec.HostUsers == nil || !*pod.Spec.HostUsers { + t.Errorf("HostUsers = %v, want true (template value preserved)", pod.Spec.HostUsers) + } +} + +func findContainerByName(pod *corev1.Pod, name string) *corev1.Container { + for i := range pod.Spec.Containers { + if pod.Spec.Containers[i].Name == name { + return &pod.Spec.Containers[i] + } + } + return nil +} + +func assertRunAsUserAndNonRoot(t *testing.T, sc *corev1.SecurityContext, wantUID int64) { + t.Helper() + if sc == nil { + t.Fatalf("SecurityContext = nil, want RunAsUser=%d", wantUID) + } + if sc.RunAsUser == nil || *sc.RunAsUser != wantUID { + t.Errorf("RunAsUser = %v, want %d", sc, wantUID) + } + if sc.RunAsNonRoot == nil || !*sc.RunAsNonRoot { + t.Errorf("RunAsNonRoot = %v, want true", sc.RunAsNonRoot) + } +} + +func TestMergeSecurityContextTemplateFieldsWinPerField(t *testing.T) { + dst := &corev1.SecurityContext{ + RunAsUser: new(int64(1000)), + RunAsNonRoot: new(true), + } + src := &corev1.SecurityContext{ + RunAsUser: new(int64(2000)), + SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault}, + } + + merged := mergeSecurityContext(dst, src) + + if merged.RunAsUser == nil || *merged.RunAsUser != 2000 { + t.Errorf("RunAsUser = %v, want 2000 (template field wins)", merged.RunAsUser) + } + if merged.RunAsNonRoot == nil || !*merged.RunAsNonRoot { + t.Errorf( + "RunAsNonRoot = %v, want true (kept from dst, template didn't set it)", + merged.RunAsNonRoot, + ) + } + wantRuntimeDefault := merged.SeccompProfile != nil && + merged.SeccompProfile.Type == corev1.SeccompProfileTypeRuntimeDefault + if !wantRuntimeDefault { + t.Errorf( + "SeccompProfile = %v, want RuntimeDefault (template-only field applied)", + merged.SeccompProfile, + ) + } +} + +func TestMergeSecurityContextNilSrcKeepsDst(t *testing.T) { + dst := &corev1.SecurityContext{RunAsUser: new(int64(1000))} + if got := mergeSecurityContext(dst, nil); got != dst { + t.Errorf("mergeSecurityContext(dst, nil) = %v, want dst unchanged", got) + } +} + +func TestGetContainersTemplateSecurityContextWinsUnderStrict(t *testing.T) { + pod := &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: DevContainerName, + SecurityContext: &corev1.SecurityContext{ + RunAsUser: new(int64(5000)), + RunAsNonRoot: new(true), + }, + }}, + }, + } + + containers, err := getContainers(pod, devsyContainerInputs{ + ImageName: testImageName, + Entrypoint: testEntrypoint, + Resources: corev1.ResourceRequirements{}, + Security: securityContextOptions{StrictSecurity: pkgconfig.BoolTrue}, + }) + if err != nil { + t.Fatalf("getContainers: %v", err) + } + sc := containers[0].SecurityContext + if sc.RunAsUser == nil || *sc.RunAsUser != 5000 { + t.Errorf( + "RunAsUser = %v, want 5000 (POD_MANIFEST_TEMPLATE wins over STRICT_SECURITY)", + sc.RunAsUser, + ) + } + if sc.RunAsNonRoot == nil || !*sc.RunAsNonRoot { + t.Errorf( + "RunAsNonRoot = %v, want true (POD_MANIFEST_TEMPLATE wins over STRICT_SECURITY)", + sc.RunAsNonRoot, + ) + } +} + +func TestGetContainersTemplateSecurityContextWinsOverAgentSecurityContext(t *testing.T) { + pod := &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: DevContainerName, + SecurityContext: &corev1.SecurityContext{RunAsUser: new(int64(5000))}, + }}, + }, + } + + containers, err := getContainers(pod, devsyContainerInputs{ + ImageName: testImageName, + Entrypoint: testEntrypoint, + Resources: corev1.ResourceRequirements{}, + Security: securityContextOptions{ + AgentSecurityContext: "runAsUser: 1000\nrunAsNonRoot: true\n", + }, + }) + if err != nil { + t.Fatalf("getContainers: %v", err) + } + sc := containers[0].SecurityContext + if sc.RunAsUser == nil || *sc.RunAsUser != 5000 { + t.Errorf( + "RunAsUser = %v, want 5000 (POD_MANIFEST_TEMPLATE wins over AGENT_SECURITY_CONTEXT)", + sc.RunAsUser, + ) + } + if sc.RunAsNonRoot == nil || !*sc.RunAsNonRoot { + t.Errorf( + "RunAsNonRoot = %v, want true (AGENT_SECURITY_CONTEXT fills the field the template left unset)", + sc.RunAsNonRoot, + ) + } +} + +func TestGetContainersDefaultModeTemplateSecurityContextWins(t *testing.T) { + pod := &corev1.Pod{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: DevContainerName, + SecurityContext: &corev1.SecurityContext{RunAsNonRoot: new(true)}, + }}, + }, + } + + containers, err := getContainers(pod, devsyContainerInputs{ + ImageName: testImageName, Entrypoint: testEntrypoint, + Resources: corev1.ResourceRequirements{}, Security: securityContextOptions{}, + }) + if err != nil { + t.Fatalf("getContainers: %v", err) + } + sc := containers[0].SecurityContext + if sc.RunAsNonRoot == nil || !*sc.RunAsNonRoot { + t.Errorf( + "RunAsNonRoot = %v, want true (POD_MANIFEST_TEMPLATE wins over the hardcoded default in default mode)", + sc.RunAsNonRoot, + ) + } + if sc.RunAsUser != nil { + t.Errorf( + "RunAsUser = %v, want nil (runAsNonRoot=true clears the implicit root default)", + sc.RunAsUser, + ) + } +} + +func TestGetInitContainersTemplateSecurityContextWins(t *testing.T) { + k := &KubernetesDriver{ + options: &provider2.ProviderKubernetesDriverConfig{StrictSecurity: pkgconfig.BoolTrue}, + } + options := &driver.RunOptions{ + Mounts: []*config.Mount{{Type: pkgconfig.ResourceVolume, Target: "/workspace"}}, + } + pod := &corev1.Pod{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{{ + Name: InitContainerName, + SecurityContext: &corev1.SecurityContext{RunAsUser: new(int64(7000))}, + }}, + }, + } + + containers, err := k.getInitContainers(options, pod, true) + if err != nil { + t.Fatalf("getInitContainers: %v", err) + } + if len(containers) != 1 { + t.Fatalf("got %d init containers, want 1", len(containers)) + } + sc := containers[0].SecurityContext + if sc.RunAsUser == nil || *sc.RunAsUser != 7000 { + t.Errorf( + "RunAsUser = %v, want 7000 (POD_MANIFEST_TEMPLATE wins over STRICT_SECURITY for the init container)", + sc.RunAsUser, + ) + } +} + +func TestAssemblePodSpecOpenShiftScenario(t *testing.T) { + k := &KubernetesDriver{ + options: &provider2.ProviderKubernetesDriverConfig{ + StrictSecurity: pkgconfig.BoolTrue, + AgentSecurityContext: "runAsUser: 1002010000\nrunAsGroup: 1002010000\nrunAsNonRoot: true\n", + KubernetesUserNamespaces: pkgconfig.BoolTrue, + }, + } + pod := &corev1.Pod{} + + err := k.assemblePodSpec(pod, "devsy-ws-openshift", &podSpecInputs{ + options: &driver.RunOptions{Image: testImageName, Entrypoint: "devsy"}, + meta: &podMetadata{labels: map[string]string{}, nodeSelector: map[string]string{}}, + }) + if err != nil { + t.Fatalf("assemblePodSpec: %v", err) + } + + if pod.Spec.HostUsers == nil || *pod.Spec.HostUsers { + t.Errorf("HostUsers = %v, want false", pod.Spec.HostUsers) + } + + devsyContainer := findContainerByName(pod, DevContainerName) + if devsyContainer == nil { + t.Fatal("devsy container not found") + } + assertRunAsUserAndNonRoot(t, devsyContainer.SecurityContext, 1002010000) +} diff --git a/pkg/driver/kubernetes/security_context_test.go b/pkg/driver/kubernetes/security_context_test.go new file mode 100644 index 000000000..a64023852 --- /dev/null +++ b/pkg/driver/kubernetes/security_context_test.go @@ -0,0 +1,237 @@ +package kubernetes + +import ( + "os" + "path/filepath" + "strings" + "testing" + + pkgconfig "github.com/devsy-org/devsy/pkg/config" + corev1 "k8s.io/api/core/v1" +) + +func TestParseSecurityContextEmpty(t *testing.T) { + sc, err := parseSecurityContext("") + if err != nil { + t.Fatalf("parseSecurityContext: %v", err) + } + if sc != nil { + t.Errorf("got %+v, want nil", sc) + } +} + +func TestParseSecurityContextInlineYAML(t *testing.T) { + sc, err := parseSecurityContext( + "runAsUser: 1002010000\nrunAsGroup: 1002010000\nrunAsNonRoot: true\n", + ) + if err != nil { + t.Fatalf("parseSecurityContext: %v", err) + } + if sc == nil || sc.RunAsUser == nil || *sc.RunAsUser != 1002010000 { + t.Fatalf("got %+v, want RunAsUser=1002010000", sc) + } + if sc.RunAsNonRoot == nil || !*sc.RunAsNonRoot { + t.Fatalf("got %+v, want RunAsNonRoot=true", sc) + } +} + +func TestParseSecurityContextInvalid(t *testing.T) { + if _, err := parseSecurityContext("not: valid: yaml: at: all:"); err == nil { + t.Fatal("expected error for invalid inline yaml and nonexistent file path") + } +} + +// TestParseSecurityContextInvalidFileReturnsRealParseError is a regression +// test: a file that exists but holds invalid YAML must surface the actual +// unmarshal error, not a formatted-nil placeholder from a shadowed variable. +func TestParseSecurityContextInvalidFileReturnsRealParseError(t *testing.T) { + path := filepath.Join(t.TempDir(), "security-context.yaml") + if err := os.WriteFile(path, []byte("not: [valid"), 0o600); err != nil { + t.Fatalf("write temp file: %v", err) + } + + _, err := parseSecurityContext(path) + if err == nil { + t.Fatal("expected error for invalid YAML file") + } + if strings.Contains(err.Error(), "%!w()") { + t.Fatalf("error lost the real parse failure behind a shadowed nil: %v", err) + } +} + +func rootBase() *corev1.SecurityContext { + return &corev1.SecurityContext{ + Capabilities: &corev1.Capabilities{Add: []corev1.Capability{"NET_ADMIN"}}, + Privileged: new(true), + RunAsUser: new(int64(0)), + RunAsGroup: new(int64(0)), + RunAsNonRoot: new(bool), + } +} + +func TestResolveContainerSecurityContextDefault(t *testing.T) { + sc, err := resolveContainerSecurityContext("", "", rootBase()) + if err != nil { + t.Fatalf("resolveContainerSecurityContext: %v", err) + } + if sc.RunAsUser == nil || *sc.RunAsUser != 0 { + t.Errorf("RunAsUser = %v, want 0", sc.RunAsUser) + } + if sc.Capabilities == nil || len(sc.Capabilities.Add) != 1 { + t.Errorf("Capabilities not preserved: %+v", sc.Capabilities) + } + if sc.Privileged == nil || !*sc.Privileged { + t.Errorf("Privileged not preserved: %v", sc.Privileged) + } +} + +func assertCapabilitiesUnchanged(t *testing.T, sc *corev1.SecurityContext) { + t.Helper() + if sc.Capabilities == nil || len(sc.Capabilities.Add) != 1 { + t.Errorf("Capabilities not preserved: %+v", sc.Capabilities) + } +} + +func assertPrivilegedUnchanged(t *testing.T, sc *corev1.SecurityContext) { + t.Helper() + if sc.Privileged == nil || !*sc.Privileged { + t.Errorf("Privileged not preserved: %v", sc.Privileged) + } +} + +func TestResolveContainerSecurityContextStrict(t *testing.T) { + sc, err := resolveContainerSecurityContext(pkgconfig.BoolTrue, "", rootBase()) + if err != nil { + t.Fatalf("resolveContainerSecurityContext: %v", err) + } + if sc.RunAsUser != nil { + t.Errorf("RunAsUser = %v, want nil", sc.RunAsUser) + } + if sc.RunAsGroup != nil { + t.Errorf("RunAsGroup = %v, want nil", sc.RunAsGroup) + } + if sc.RunAsNonRoot != nil { + t.Errorf("RunAsNonRoot = %v, want nil", sc.RunAsNonRoot) + } + assertCapabilitiesUnchanged(t, sc) + assertPrivilegedUnchanged(t, sc) +} + +func TestResolveContainerSecurityContextStrictNilBase(t *testing.T) { + sc, err := resolveContainerSecurityContext(pkgconfig.BoolTrue, "", nil) + if err != nil { + t.Fatalf("resolveContainerSecurityContext: %v", err) + } + if sc != nil { + t.Errorf("got %+v, want nil", sc) + } +} + +func TestResolveContainerSecurityContextOverride(t *testing.T) { + sc, err := resolveContainerSecurityContext( + "", + "runAsUser: 1002010000\nrunAsNonRoot: true\n", + rootBase(), + ) + if err != nil { + t.Fatalf("resolveContainerSecurityContext: %v", err) + } + if sc.RunAsUser == nil || *sc.RunAsUser != 1002010000 { + t.Errorf("RunAsUser = %v, want 1002010000", sc.RunAsUser) + } + if sc.RunAsNonRoot == nil || !*sc.RunAsNonRoot { + t.Errorf("RunAsNonRoot = %v, want true", sc.RunAsNonRoot) + } + assertCapabilitiesUnchanged(t, sc) + assertPrivilegedUnchanged(t, sc) +} + +func assertCapabilitiesDropAll(t *testing.T, sc *corev1.SecurityContext) { + t.Helper() + if sc.Capabilities == nil || len(sc.Capabilities.Add) != 0 || len(sc.Capabilities.Drop) != 1 || + sc.Capabilities.Drop[0] != "ALL" { + t.Errorf( + "Capabilities = %+v, want override's drop=[ALL] with no inherited Add", + sc.Capabilities, + ) + } +} + +func TestResolveContainerSecurityContextOverrideOwnCapabilitiesWin(t *testing.T) { + sc, err := resolveContainerSecurityContext( + "", + "runAsUser: 1000\nallowPrivilegeEscalation: false\ncapabilities:\n drop: [\"ALL\"]\n", + rootBase(), + ) + if err != nil { + t.Fatalf("resolveContainerSecurityContext: %v", err) + } + assertCapabilitiesDropAll(t, sc) + assertPrivilegedUnchanged(t, sc) + if sc.AllowPrivilegeEscalation == nil || *sc.AllowPrivilegeEscalation { + t.Errorf("AllowPrivilegeEscalation = %v, want false", sc.AllowPrivilegeEscalation) + } +} + +func TestResolveContainerSecurityContextOverrideWinsOverStrict(t *testing.T) { + sc, err := resolveContainerSecurityContext(pkgconfig.BoolTrue, "runAsUser: 5000\n", rootBase()) + if err != nil { + t.Fatalf("resolveContainerSecurityContext: %v", err) + } + if sc.RunAsUser == nil || *sc.RunAsUser != 5000 { + t.Errorf("RunAsUser = %v, want 5000 (override wins over strict)", sc.RunAsUser) + } +} + +func TestResolveContainerSecurityContextInvalidOverride(t *testing.T) { + if _, err := resolveContainerSecurityContext( + "", + "not: valid: yaml: at: all:", + rootBase(), + ); err == nil { + t.Fatal("expected error for invalid AGENT_SECURITY_CONTEXT") + } +} + +func TestSecurityContextOptionsResolveDefault(t *testing.T) { + sc, err := (securityContextOptions{}).resolve() + if err != nil { + t.Fatalf("resolve: %v", err) + } + if sc.RunAsUser == nil || *sc.RunAsUser != 0 { + t.Errorf("RunAsUser = %v, want 0", sc.RunAsUser) + } +} + +func TestSecurityContextOptionsResolveStrictNoCapabilities(t *testing.T) { + sc, err := (securityContextOptions{StrictSecurity: pkgconfig.BoolTrue}).resolve() + if err != nil { + t.Fatalf("resolve: %v", err) + } + if sc == nil { + t.Fatal("got nil, want a non-nil SecurityContext with cleared run-as fields") + } + if sc.RunAsUser != nil { + t.Errorf("RunAsUser = %v, want nil", sc.RunAsUser) + } +} + +func TestSecurityContextOptionsResolvePropagatesCapabilities(t *testing.T) { + caps := &corev1.Capabilities{Add: []corev1.Capability{"NET_ADMIN"}} + sc, err := (securityContextOptions{Capabilities: caps, Privileged: new(true)}).resolve() + if err != nil { + t.Fatalf("resolve: %v", err) + } + if sc.Capabilities != caps { + t.Errorf("Capabilities = %v, want %v", sc.Capabilities, caps) + } + if sc.Privileged == nil || !*sc.Privileged { + t.Errorf("Privileged = %v, want true", sc.Privileged) + } +} + +func TestSecurityContextOptionsResolveInvalidOverride(t *testing.T) { + if _, err := (securityContextOptions{AgentSecurityContext: "not: valid: yaml: at: all:"}).resolve(); err == nil { + t.Fatal("expected error for invalid AGENT_SECURITY_CONTEXT") + } +} diff --git a/pkg/git/config.go b/pkg/git/config.go index b40b61fb0..ecc01ed26 100644 --- a/pkg/git/config.go +++ b/pkg/git/config.go @@ -94,6 +94,21 @@ func (c *Config) Unset(ctx context.Context, key string, scope ConfigScope) error return nil } +// UnsetValue removes all exact matching values from a config key. +func (c *Config) UnsetValue(ctx context.Context, key, value string, scope ConfigScope) error { + args := append([]string{subConfig}, scope.args()...) + args = append(args, "--fixed-value", "--unset-all", key, value) + if _, err := c.repo.run(ctx, args...); err != nil { + // Exit code 5 means the key does not exist or no value matched. + var cmdErr *CommandError + if errors.As(err, &cmdErr) && cmdErr.ExitCode == 5 { + return nil + } + return fmt.Errorf("unset git config %q: %w", key, err) + } + return nil +} + // UnsetAll removes all values of a multi-valued config key in the given scope. An absent key is not an error. func (c *Config) UnsetAll(ctx context.Context, key string, scope ConfigScope) error { args := append([]string{subConfig}, scope.args()...) diff --git a/pkg/git/config_test.go b/pkg/git/config_test.go index 2dac332fb..7682616c9 100644 --- a/pkg/git/config_test.go +++ b/pkg/git/config_test.go @@ -81,6 +81,46 @@ func TestConfigUnsetSystemScope(t *testing.T) { fake.lastArgs()) } +func TestConfigUnsetValueScopesToExactValue(t *testing.T) { + fake := &fakeRunner{} + config := At("", WithRunner(fake)).Config() + + err := config.UnsetValue(context.Background(), "credential.helper", "!my-helper", ScopeSystem) + assert.NilError(t, err) + assert.DeepEqual( + t, + []string{ + subConfig, + flagSystem, + "--fixed-value", + "--unset-all", + "credential.helper", + "!my-helper", + }, + fake.lastArgs(), + ) +} + +func TestConfigUnsetValueNoMatchIsNotError(t *testing.T) { + fake := &fakeRunner{err: &CommandError{ExitCode: 5}} + config := At("", WithRunner(fake)).Config() + + err := config.UnsetValue(context.Background(), "credential.helper", "!my-helper", ScopeSystem) + assert.NilError(t, err) +} + +func TestConfigUnsetValueRealFailurePropagates(t *testing.T) { + fake := &fakeRunner{err: &CommandError{ExitCode: 128, Stderr: "fatal: bad config"}} + config := At("", WithRunner(fake)).Config() + + err := config.UnsetValue(context.Background(), "credential.helper", "!my-helper", ScopeSystem) + assert.Assert(t, err != nil) + + var cmdErr *CommandError + assert.Assert(t, errors.As(err, &cmdErr)) + assert.Equal(t, 128, cmdErr.ExitCode) +} + func TestConfigGetAbsentKeyIsNotError(t *testing.T) { // `git config --get` exits 1 with no output when the key is absent. fake := &fakeRunner{err: &CommandError{ExitCode: 1}} diff --git a/pkg/options/resolve.go b/pkg/options/resolve.go index 98fafbbe8..913dda176 100644 --- a/pkg/options/resolve.go +++ b/pkg/options/resolve.go @@ -344,6 +344,7 @@ func resolveAgentKubernetesConfig( k8s.PodManifestTemplate = resolver.ResolveDefaultValue(k8s.PodManifestTemplate, options) k8s.Labels = resolver.ResolveDefaultValue(k8s.Labels, options) k8s.StrictSecurity = resolver.ResolveDefaultValue(k8s.StrictSecurity, options) + k8s.AgentSecurityContext = resolver.ResolveDefaultValue(k8s.AgentSecurityContext, options) k8s.CreateNamespace = resolver.ResolveDefaultValue(k8s.CreateNamespace, options) k8s.ClusterRole = resolver.ResolveDefaultValue(k8s.ClusterRole, options) k8s.ServiceAccount = resolver.ResolveDefaultValue(k8s.ServiceAccount, options) @@ -353,6 +354,11 @@ func resolveAgentKubernetesConfig( options, ) k8s.DiskSize = resolver.ResolveDefaultValue(k8s.DiskSize, options) + k8s.AgentInstallPath = resolver.ResolveDefaultValue(k8s.AgentInstallPath, options) + k8s.KubernetesUserNamespaces = resolver.ResolveDefaultValue( + k8s.KubernetesUserNamespaces, + options, + ) } func resolveAgentAppleConfig( diff --git a/pkg/options/resolve_test.go b/pkg/options/resolve_test.go index e9bb5d0ff..9bb5fa185 100644 --- a/pkg/options/resolve_test.go +++ b/pkg/options/resolve_test.go @@ -883,3 +883,31 @@ func TestResolveAgentDownloadURL(t *testing.T) { }) } } + +func TestResolveAgentKubernetesConfigAgentSecurityContext(t *testing.T) { + agentConfig := &provider.ProviderAgentConfig{} + options := map[string]string{ + "AGENT_SECURITY_CONTEXT": "runAsUser: 1000", + } + agentConfig.Kubernetes.AgentSecurityContext = "${AGENT_SECURITY_CONTEXT}" + + resolveAgentKubernetesConfig(agentConfig, options) + + if got := agentConfig.Kubernetes.AgentSecurityContext; got != "runAsUser: 1000" { + t.Errorf("AgentSecurityContext = %q, want %q", got, "runAsUser: 1000") + } +} + +func TestResolveAgentKubernetesConfigAgentInstallPath(t *testing.T) { + agentConfig := &provider.ProviderAgentConfig{} + options := map[string]string{ + "AGENT_INSTALL_PATH": "/home/vscode/.local/bin/devsy", + } + agentConfig.Kubernetes.AgentInstallPath = "${AGENT_INSTALL_PATH}" + + resolveAgentKubernetesConfig(agentConfig, options) + + if got := agentConfig.Kubernetes.AgentInstallPath; got != "/home/vscode/.local/bin/devsy" { + t.Errorf("AgentInstallPath = %q, want %q", got, "/home/vscode/.local/bin/devsy") + } +} diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 41fe948ed..5f200111f 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -1,7 +1,12 @@ package provider import ( + "os" + "path/filepath" + + "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/types" + "sigs.k8s.io/yaml" ) const ( @@ -153,6 +158,158 @@ func (a ProviderAgentConfig) IsDockerDriver() bool { return a.Driver == "" || a.Driver == DockerDriver } +// ContainerInstallPath is where the agent binary lives inside the container +// for this driver: config.ContainerDevsyHelperLocation by default, or the +// Kubernetes driver's AgentInstallPath override when set (a writable path +// for non-root containers, e.g. an OpenShift restricted-SCC pod). +func (a ProviderAgentConfig) ContainerInstallPath() string { + if a.Driver == KubernetesDriver && a.Kubernetes.AgentInstallPath != "" { + return a.Kubernetes.AgentInstallPath + } + return config.ContainerDevsyHelperLocation +} + +// runAsFields is the subset of corev1.SecurityContext this package needs to +// judge effective non-root execution, without depending on k8s.io/api. +type runAsFields struct { + RunAsUser *int64 `json:"runAsUser,omitempty"` + RunAsNonRoot *bool `json:"runAsNonRoot,omitempty"` +} + +// unmarshalInlineOrFile parses raw as inline YAML into out, falling back to +// treating raw as a file path on failure. It mirrors the Kubernetes driver's +// own dual-mode parsing of AGENT_SECURITY_CONTEXT and POD_MANIFEST_TEMPLATE +// (pkg/driver/kubernetes: parseSecurityContext, getPodTemplate). +func unmarshalInlineOrFile(raw string, out any) error { + if err := yaml.Unmarshal([]byte(raw), out); err == nil { + return nil + } + p, err := filepath.Abs(raw) + if err != nil { + return err + } + body, err := os.ReadFile( + p, + ) // #nosec G304 -- path comes from the operator-controlled provider config, not untrusted input + if err != nil { + return err + } + return yaml.Unmarshal(body, out) +} + +// minimalPodManifest is the subset of corev1.Pod this package needs to +// resolve a podManifestTemplate's pod- and per-container run-as overrides, +// without depending on k8s.io/api. +type minimalPodManifest struct { + Spec minimalPodSpec `json:"spec"` +} + +type minimalPodSpec struct { + SecurityContext *runAsFields `json:"securityContext,omitempty"` + Containers []minimalContainer `json:"containers"` +} + +type minimalContainer struct { + Name string `json:"name"` + SecurityContext *runAsFields `json:"securityContext,omitempty"` +} + +// podManifestRunAsFields returns the pod-level and named "devsy" container +// run-as fields from a podManifestTemplate, or nil fields if the template is +// empty, unparsable, or contains no corresponding values. +func podManifestRunAsFields(podManifestTemplate string) (pod, container *runAsFields) { + if podManifestTemplate == "" { + return nil, nil + } + var manifest minimalPodManifest + if err := unmarshalInlineOrFile(podManifestTemplate, &manifest); err != nil { + return nil, nil + } + for _, c := range manifest.Spec.Containers { + if c.Name == config.BinaryName { + return manifest.Spec.SecurityContext, c.SecurityContext + } + } + return manifest.Spec.SecurityContext, nil +} + +// effectiveKubernetesRunAsFields resolves the run-as fields Devsy's +// Kubernetes driver actually applies to the "devsy" container. Container-level +// fields from podManifestTemplate override AGENT_SECURITY_CONTEXT fields, +// which override pod-level fields (pkg/driver/kubernetes: resolveContainer- +// SecurityContext, mergeContainer). The generated default container security +// context explicitly runs as root, so pod-level fields are effective only when +// strict security or an agent security context removes those defaults. +func mergeRunAsFields(dst *runAsFields, haveAny *bool, fields *runAsFields) { + if fields == nil { + return + } + if fields.RunAsUser != nil { + dst.RunAsUser = fields.RunAsUser + *haveAny = true + } + if fields.RunAsNonRoot != nil { + dst.RunAsNonRoot = fields.RunAsNonRoot + *haveAny = true + } +} + +func normalizeRunAsFields(dst, override *runAsFields) { + if override == nil || + override.RunAsUser != nil || + override.RunAsNonRoot == nil || !*override.RunAsNonRoot || + dst.RunAsUser == nil || *dst.RunAsUser != 0 { + return + } + dst.RunAsUser = nil +} + +func effectiveKubernetesRunAsFields(k ProviderKubernetesDriverConfig) *runAsFields { + var sc runAsFields + haveAny := false + + podFields, containerFields := podManifestRunAsFields(k.PodManifestTemplate) + switch { + case k.AgentSecurityContext != "": + var agentFields runAsFields + if err := unmarshalInlineOrFile(k.AgentSecurityContext, &agentFields); err == nil { + mergeRunAsFields(&sc, &haveAny, podFields) + mergeRunAsFields(&sc, &haveAny, &agentFields) + } + case k.StrictSecurity == config.BoolTrue: + mergeRunAsFields(&sc, &haveAny, podFields) + default: + rootUID := int64(0) + root := false + sc.RunAsUser = &rootUID + sc.RunAsNonRoot = &root + haveAny = true + } + mergeRunAsFields(&sc, &haveAny, containerFields) + normalizeRunAsFields(&sc, containerFields) + if !haveAny { + return nil + } + return &sc +} + +// RunsFixedNonRootUser reports whether the effective Kubernetes container +// security context explicitly guarantees the container runs as a fixed +// non-root UID. STRICT_SECURITY can expose pod-level run-as fields because it +// removes the generated root defaults; an agent context can also omit those +// fields and inherit the pod-level values. +func (a ProviderAgentConfig) RunsFixedNonRootUser() bool { + if a.Driver != KubernetesDriver { + return false + } + sc := effectiveKubernetesRunAsFields(a.Kubernetes) + if sc == nil { + return false + } + return (sc.RunAsNonRoot != nil && *sc.RunAsNonRoot) || + (sc.RunAsUser != nil && *sc.RunAsUser != 0) +} + const ( DockerDriver = "docker" KubernetesDriver = "kubernetes" @@ -281,7 +438,26 @@ type ProviderKubernetesDriverConfig struct { PodManifestTemplate string `json:"podManifestTemplate,omitempty"` Labels string `json:"labels,omitempty"` - StrictSecurity string `json:"strictSecurity,omitempty"` + StrictSecurity string `json:"strictSecurity,omitempty"` + AgentSecurityContext string `json:"agentSecurityContext,omitempty"` + + // AgentInstallPath overrides where the agent binary is installed inside + // the devsy/devsy-init containers. Defaults to /usr/local/bin/devsy, + // which requires root to write; set this (to a path under a writable + // mount, e.g. the workspace volume) when running non-root so delivery + // and the container's own entrypoint agree on a writable location. + AgentInstallPath string `json:"agentInstallPath,omitempty"` + + // KubernetesUserNamespaces opts into setting spec.hostUsers to false + // (unless a pod template already sets it), so the kubelet maps the + // container's UIDs into a Linux user namespace instead of the host's. + // Defaults unset: STRICT_SECURITY and AGENT_SECURITY_CONTEXT also set + // HostUsers false for OpenShift restricted-v3 compatibility. The field's + // mere presence requires the cluster's UserNamespacesSupport feature gate + // (on by default only from Kubernetes 1.33) and node-level user-namespace + // support (Linux kernel 6.3+, containerd 2.0+/CRI-O 1.25+); use a pod + // template to override HostUsers on clusters without that support. + KubernetesUserNamespaces string `json:"kubernetesUserNamespaces,omitempty"` } type ProviderAgentConfigExec struct { diff --git a/pkg/provider/security_context_test.go b/pkg/provider/security_context_test.go new file mode 100644 index 000000000..398d91c0a --- /dev/null +++ b/pkg/provider/security_context_test.go @@ -0,0 +1,190 @@ +package provider + +import ( + "testing" + + "github.com/devsy-org/devsy/pkg/config" +) + +type runsFixedNonRootUserCase struct { + name string + config ProviderAgentConfig + want bool +} + +var basicRunsFixedNonRootUserCases = []runsFixedNonRootUserCase{ + { + name: "non-kubernetes driver ignores security context", + config: ProviderAgentConfig{ + Driver: DockerDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + AgentSecurityContext: "runAsNonRoot: true", + }, + }, + want: false, + }, + { + name: "strict security alone is not a guarantee", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{StrictSecurity: "true"}, + }, + want: false, + }, + { + name: "capabilities-only security context is not a guarantee", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + AgentSecurityContext: "capabilities:\n add: [\"SYS_PTRACE\"]", + }, + }, + want: false, + }, + { + name: "explicit runAsNonRoot true is a guarantee", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + AgentSecurityContext: "runAsNonRoot: true", + }, + }, + want: true, + }, + { + name: "explicit nonzero runAsUser is a guarantee", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{AgentSecurityContext: "runAsUser: 1000"}, + }, + want: true, + }, + { + name: "runAsUser zero is not a guarantee", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{AgentSecurityContext: "runAsUser: 0"}, + }, + want: false, + }, + { + name: "unparseable security context is not a guarantee", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + AgentSecurityContext: "runAsUser: [", + }, + }, + want: false, + }, +} + +func TestRunsFixedNonRootUser(t *testing.T) { + for _, tc := range basicRunsFixedNonRootUserCases { + t.Run(tc.name, func(t *testing.T) { + if got := tc.config.RunsFixedNonRootUser(); got != tc.want { + t.Errorf("RunsFixedNonRootUser() = %v, want %v", got, tc.want) + } + }) + } +} + +var podManifestTemplateRunsFixedNonRootUserCases = []runsFixedNonRootUserCase{ + { + name: "strict security exposes pod-level runAsUser", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + StrictSecurity: "true", + PodManifestTemplate: "spec:\n" + + " securityContext:\n runAsUser: 1000\n", + }, + }, + want: true, + }, + { + name: "default root container masks pod-level runAsUser", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + PodManifestTemplate: "spec:\n" + + " securityContext:\n runAsUser: 1000\n", + }, + }, + want: false, + }, + { + name: "agent context inherits pod-level runAsUser when unset", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + AgentSecurityContext: "runAsNonRoot: false", + PodManifestTemplate: "spec:\n" + + " securityContext:\n runAsUser: 1000\n", + }, + }, + want: true, + }, + { + name: "agent security context overrides pod-level runAsUser", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + AgentSecurityContext: "runAsUser: 0", + PodManifestTemplate: "spec:\n" + + " securityContext:\n runAsUser: 1000\n", + }, + }, + want: false, + }, + { + name: "podManifestTemplate devsy container overrides agentSecurityContext to root", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + AgentSecurityContext: "runAsUser: 1000\nrunAsNonRoot: true", + PodManifestTemplate: "spec:\n" + + " securityContext:\n runAsUser: 2000\n" + + " containers:\n" + + " - name: " + config.BinaryName + "\n" + + " securityContext:\n runAsUser: 0\n runAsNonRoot: false\n", + }, + }, + want: false, + }, + { + name: "podManifestTemplate devsy container alone guarantees non-root", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + PodManifestTemplate: "spec:\n containers:\n" + + " - name: " + config.BinaryName + "\n" + + " securityContext:\n runAsUser: 5000\n", + }, + }, + want: true, + }, + { + name: "podManifestTemplate for a different container is ignored", + config: ProviderAgentConfig{ + Driver: KubernetesDriver, + Kubernetes: ProviderKubernetesDriverConfig{ + AgentSecurityContext: "runAsUser: 1000", + PodManifestTemplate: "spec:\n containers:\n" + + " - name: sidecar\n" + + " securityContext:\n runAsUser: 0\n", + }, + }, + want: true, + }, +} + +func TestRunsFixedNonRootUser_PodManifestTemplatePrecedence(t *testing.T) { + for _, tc := range podManifestTemplateRunsFixedNonRootUserCases { + t.Run(tc.name, func(t *testing.T) { + if got := tc.config.RunsFixedNonRootUser(); got != tc.want { + t.Errorf("RunsFixedNonRootUser() = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/pkg/tunnel/services.go b/pkg/tunnel/services.go index b85052a0b..7c6223a57 100644 --- a/pkg/tunnel/services.go +++ b/pkg/tunnel/services.go @@ -309,7 +309,7 @@ func getContainerResult(ctx context.Context, p portForwardParams) (*config2.Resu stderr := &bytes.Buffer{} err := devssh.Run(ctx, devssh.RunOptions{ Client: p.containerClient, - Command: "cat " + config.DevContainerResultPath, + Command: config.ReadDevContainerResultCommand(), Stdout: stdout, Stderr: stderr, }) @@ -327,7 +327,7 @@ func getContainerResult(ctx context.Context, p portForwardParams) (*config2.Resu if err != nil { return nil, fmt.Errorf("error parsing container result %s: %w", stdout.String(), err) } - log.Debugf("parsed container result from %s", config.DevContainerResultPath) + log.Debugf("parsed container result") return result, nil } diff --git a/providers/kubernetes/provider.yaml b/providers/kubernetes/provider.yaml index 33c29a37f..30b2397d7 100644 --- a/providers/kubernetes/provider.yaml +++ b/providers/kubernetes/provider.yaml @@ -28,6 +28,9 @@ optionGroups: - LABELS - DOCKERLESS_DISABLED - DOCKERLESS_IMAGE + - AGENT_SECURITY_CONTEXT + - AGENT_INSTALL_PATH + - KUBERNETES_USER_NAMESPACES name: "Advanced Options" options: DISK_SIZE: @@ -92,7 +95,20 @@ options: global: true default: "false" STRICT_SECURITY: - description: "EXPERIMENTAL! Use at your own risk. Removes the default security context and merges the one from POD_MANIFEST_TEMPLATE if specified." + description: Clears the injected containers' RunAsUser/RunAsGroup/RunAsNonRoot (letting the cluster assign a UID, e.g. an OpenShift SCC) unless POD_MANIFEST_TEMPLATE or AGENT_SECURITY_CONTEXT already set these fields. It also sets spec.hostUsers to false unless POD_MANIFEST_TEMPLATE explicitly sets it, as required by OpenShift restricted-v3. Capabilities and Privileged (from CapAdd/--privileged) are always kept. + global: true + type: boolean + default: "false" + AGENT_SECURITY_CONTEXT: + description: Inline YAML (or a file path) for a Kubernetes SecurityContext merged field by field onto the injected devsy and devsy-init containers. It can set run-as, capabilities, privilege escalation, seccomp, and other supported SecurityContext fields, e.g. to satisfy an OpenShift SCC's allocated UID range. Takes precedence over STRICT_SECURITY and the built-in root default. It sets spec.hostUsers to false unless POD_MANIFEST_TEMPLATE explicitly sets it. A container matching the devsy/devsy-init name in POD_MANIFEST_TEMPLATE still takes precedence over this option. + global: true + type: multiline + AGENT_INSTALL_PATH: + description: Overrides where the agent binary is installed inside the devsy/devsy-init containers, e.g. a path under a writable mount such as WORKSPACE_VOLUME_MOUNT. The default (/usr/local/bin/devsy) requires root to write; set this when running non-root (e.g. with AGENT_SECURITY_CONTEXT or STRICT_SECURITY on an OpenShift restricted SCC) so delivery and the container's own entrypoint agree on a writable location. + global: true + type: string + KUBERNETES_USER_NAMESPACES: + description: Sets spec.hostUsers to false (unless POD_MANIFEST_TEMPLATE already set it), so the kubelet maps the workspace pod's UIDs into a Linux user namespace. STRICT_SECURITY and AGENT_SECURITY_CONTEXT also enable this OpenShift restricted-v3 compatibility behavior. Requires the cluster's UserNamespacesSupport feature gate through Kubernetes 1.35; the feature is GA and the gate is locked on from Kubernetes 1.36. Node-level support is also required (Linux kernel 6.3+, containerd 2.0+/CRI-O 1.25+); enabling this on a cluster without that support can get the pod rejected or mishandled. type: boolean default: false WORKSPACE_VOLUME_MOUNT: @@ -129,6 +145,9 @@ agent: podManifestTemplate: ${POD_MANIFEST_TEMPLATE} labels: ${LABELS} strictSecurity: ${STRICT_SECURITY} + agentSecurityContext: ${AGENT_SECURITY_CONTEXT} + agentInstallPath: ${AGENT_INSTALL_PATH} + kubernetesUserNamespaces: ${KUBERNETES_USER_NAMESPACES} exec: command: |- "${DEVSY}" internal sh -c "${COMMAND}" diff --git a/sites/docs-devsy-sh/content/docs/developing-providers/driver.mdx b/sites/docs-devsy-sh/content/docs/developing-providers/driver.mdx index c2411b49a..cac039cef 100644 --- a/sites/docs-devsy-sh/content/docs/developing-providers/driver.mdx +++ b/sites/docs-devsy-sh/content/docs/developing-providers/driver.mdx @@ -75,7 +75,19 @@ The allowed options for the Kubernetes driver are: - **workspaceVolumeMount**: overrides the path where the workspace volume is mounted. Defaults to the root of your workspace source code. - **podManifestTemplate**: a pod manifest template (inline YAML or a file path) used as the base to build the Devsy pod - **labels**: labels to add to the workspace pod, e.g. `devsy.sh/example=value,devsy.sh/example2=value2` -- **strictSecurity**: *Experimental.* Removes the default security context and merges the one from `podManifestTemplate` if specified. +- **strictSecurity**: Clears the hardcoded `runAsUser`/`runAsGroup`/`runAsNonRoot` fields (retaining capabilities and `privileged`), letting the cluster assign the container's UID/GID instead of forcing root. It sets `hostUsers: false` unless `podManifestTemplate` explicitly supplies `hostUsers`; this satisfies OpenShift `restricted-v3`. +- **agentSecurityContext**: Inline YAML or a file path for a `corev1.SecurityContext` merged field by field onto the workspace and init containers. It can configure run-as, capabilities, privilege escalation, seccomp, and other supported security-context fields, overriding Devsy's defaults. It sets `hostUsers: false` unless `podManifestTemplate` explicitly supplies `hostUsers`. A matching named container in `podManifestTemplate` remains the highest-precedence override. +- **agentInstallPath**: overrides where the agent binary is installed inside the devsy/devsy-init containers. Defaults to `/usr/local/bin/devsy`, which requires root to write; set this to a path under a writable mount (e.g. the workspace volume) when running non-root. +- **kubernetesUserNamespaces**: Sets `hostUsers: false` (unless `podManifestTemplate` already set it), mapping the pod's UIDs into a Linux user namespace. `strictSecurity` and `agentSecurityContext` also opt in for OpenShift `restricted-v3`; provide `hostUsers` explicitly through `podManifestTemplate` on clusters without user-namespace support. UserNamespacesSupport is disabled by default in Kubernetes 1.30–1.32, enabled by default in 1.33–1.35, and becomes GA with the gate locked on from 1.36. Node-level support is also required (Linux kernel 6.3+, containerd 2.0+/CRI-O 1.25+). + + +On OpenShift, the default container security context (fixed `runAsUser`/`runAsGroup`) is +rejected by the `restricted-v2`/`restricted-v3` SCCs, which assign UIDs/GIDs from a +per-namespace range. Set `strictSecurity: "true"` and/or `agentSecurityContext` (or override +the container's `securityContext` via a named container in `podManifestTemplate`) to satisfy +those SCCs. Also set `agentInstallPath` to a path under a writable mount, because a non-root +container cannot write the default `/usr/local/bin/devsy`. + Devsy also supports building images inside Kubernetes without Docker, via a