From 72f35e1cd3563a0298b6a38c474abfcc7c0db0c8 Mon Sep 17 00:00:00 2001 From: Zach <3724288+zedkipp@users.noreply.github.com> Date: Fri, 17 Apr 2026 16:55:24 -0600 Subject: [PATCH] feat: runtime user secrets injection into workspaces (#24313) Injects user secrets into workspace agents at runtime via the agent manifest. Secrets with an environment variable name are set as environment variables in every agent session and startup script. Secrets with a file path are written to disk before startup scripts run. - Fetch user secrets in GetManifest and convert to proto - Defensively strip secrets from manifests received by the agent to avoid accidental leakage - Add WorkspaceSecret type and proto conversion helpers to agentsdk - Write secret files eagerly on manifest fetch (0600 perms, 0700 dirs) - Inject secret env vars per-session in updateCommandEnv - Expand ~/paths using caller-resolved home directory - Log file write errors without blocking workspace startup --- agent/agent.go | 115 +++++++++++++++++- agent/agent_test.go | 194 +++++++++++++++++++++++++++++- agent/agenttest/client.go | 16 +++ agent/write_secret_files_test.go | 185 ++++++++++++++++++++++++++++ coderd/agentapi/manifest.go | 27 +++++ coderd/agentapi/manifest_test.go | 65 ++++++++++ codersdk/agentsdk/agentsdk.go | 19 +++ codersdk/agentsdk/convert.go | 32 +++++ codersdk/agentsdk/convert_test.go | 36 ++++++ 9 files changed, 685 insertions(+), 4 deletions(-) create mode 100644 agent/write_secret_files_test.go diff --git a/agent/agent.go b/agent/agent.go index f60531399b..683070ab94 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -30,6 +30,7 @@ import ( "go.uber.org/atomic" "golang.org/x/sync/errgroup" "golang.org/x/xerrors" + googleproto "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/timestamppb" "tailscale.com/net/speedtest" "tailscale.com/tailcfg" @@ -280,7 +281,11 @@ type agent struct { environmentVariables map[string]string - manifest atomic.Pointer[agentsdk.Manifest] // manifest is atomic because values can change after reconnection. + manifest atomic.Pointer[agentsdk.Manifest] // manifest is atomic because values can change after reconnection. + // secrets are held separately from the manifest so that code paths that + // only need manifest data cannot accidentally access or leak secret + // values. Callers that need secrets must explicitly load this. + secrets atomic.Pointer[[]agentsdk.WorkspaceSecret] reportMetadataInterval time.Duration scriptRunner *agentscripts.Runner announcementBanners atomic.Pointer[[]codersdk.BannerConfig] // announcementBanners is atomic because it is periodically updated. @@ -1233,11 +1238,20 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, manifestOK.complete(err) } }() - mp, err := aAPI.GetManifest(ctx, &proto.GetManifestRequest{}) + mpRaw, err := aAPI.GetManifest(ctx, &proto.GetManifestRequest{}) if err != nil { return xerrors.Errorf("fetch metadata: %w", err) } a.logger.Info(ctx, "fetched manifest") + + // Strip secrets from the proto manifest immediately to avoid accidental leakage. + secrets := agentsdk.SecretsFromProto(mpRaw.Secrets) + mpRaw.Secrets = nil + mp, ok := googleproto.Clone(mpRaw).(*proto.Manifest) + if !ok { + return xerrors.Errorf("clone manifest: type mismatch") + } + manifest, err := agentsdk.ManifestFromProto(mp) if err != nil { a.logger.Critical(ctx, "failed to convert manifest", slog.F("manifest", mp), slog.Error(err)) @@ -1285,10 +1299,26 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, return xerrors.Errorf("update workspace agent startup: %w", err) } + a.secrets.Store(&secrets) oldManifest := a.manifest.Swap(&manifest) manifestOK.complete(nil) sentResult = true + // Write secret files after signaling manifest readiness so that network + // initialization (which depends on manifestOK) starts as soon as + // possible. This creates a theoretical race where an SSH session that + // connects and reads a secret file before writes finish would see stale + // or missing content, but in practice SSH requires network init + + // coordination before any connection arrives, which should take far + // longer than file writes. Startup scripts still wait because they run + // sequentially below. Env var injection is unaffected because it + // happens lazily per-command in updateCommandEnv. + homeDir, err := os.UserHomeDir() + if err != nil { + a.logger.Warn(ctx, "failed to resolve home directory for secret files", slog.Error(err)) + } + writeSecretFiles(ctx, a.logger, a.filesystem, homeDir, secrets) + // The startup script should only execute on the first run! if oldManifest == nil { a.setLifecycle(codersdk.WorkspaceAgentLifecycleStarting) @@ -1495,6 +1525,7 @@ func (a *agent) createOrUpdateNetwork(manifestOK, networkOK *checkpoint) func(co // - Predefined workspace environment variables // - Environment variables currently set (overriding predefined) // - Environment variables passed via the agent manifest (overriding predefined and current) +// - User secret variables passed via the agent manifest (overriding predefined, current, and manifest env vars) // - Agent-level environment variables (overriding all) func (a *agent) updateCommandEnv(current []string) (updated []string, err error) { manifest := a.manifest.Load() @@ -1556,6 +1587,19 @@ func (a *agent) updateCommandEnv(current []string) (updated []string, err error) envs[k] = os.ExpandEnv(v) } + // User secrets override manifest env vars so that secrets + // take precedence over template-defined values, but are + // still overridden by agent-level bootstrap vars below. + // Values are assigned raw without os.ExpandEnv because + // secret values may contain dollar signs (e.g. passwords) + // that must not be interpreted as variable references. + if secretsPtr := a.secrets.Load(); secretsPtr != nil { + for _, secret := range *secretsPtr { + if secret.EnvName != "" { + envs[secret.EnvName] = string(secret.Value) + } + } + } // Agent-level environment variables should take over all. This is // used for setting agent-specific variables like CODER_AGENT_TOKEN // and GIT_ASKPASS. @@ -1576,6 +1620,73 @@ func (a *agent) updateCommandEnv(current []string) (updated []string, err error) return updated, nil } +// writeSecretFiles writes user secrets with file_path set to disk. +// Errors are logged but do not block workspace startup. +func writeSecretFiles(ctx context.Context, logger slog.Logger, fs afero.Fs, homeDir string, secrets []agentsdk.WorkspaceSecret) { + // Track resolved paths to detect collisions after ~/ expansion. + // Two secrets with different file_path values can resolve to + // the same absolute path (e.g. ~/x and /home/coder/x). The API + // layer prevents duplicates on the raw file_path but cannot see + // post-resolution collisions. We still write both, with the + // later one winning, but log a warning so the conflict is + // visible. + seen := make(map[string]string, len(secrets)) + + for _, secret := range secrets { + if secret.FilePath == "" { + continue + } + + filePath := secret.FilePath + if strings.HasPrefix(filePath, "~/") { + if homeDir == "" { + logger.Warn(ctx, "skipping secret file with ~/ path: home directory unknown", + slog.F("file_path", filePath), + ) + continue + } + filePath = filepath.Join(homeDir, filePath[2:]) + } + filePath = filepath.Clean(filePath) + + if original, ok := seen[filePath]; ok { + // Known shortcoming: the winning secret is determined by the order + // of secrets in the manifest, which is currently alphabetical by + // secret name from ListUserSecretsWithValues. This ordering is not + // user-controllable and has no semantic meaning; users should avoid + // path collisions rather than rely on which secret wins. + logger.Warn(ctx, "multiple secrets resolve to the same file path; later secret in manifest order will win (not user-controllable)", + slog.F("resolved_path", filePath), + slog.F("first_file_path", original), + slog.F("conflicting_file_path", secret.FilePath), + ) + } + seen[filePath] = secret.FilePath + + dir := filepath.Dir(filePath) + if err := fs.MkdirAll(dir, 0o700); err != nil { + logger.Warn(ctx, "failed to create directory for secret file", + slog.F("file_path", filePath), + slog.Error(err), + ) + continue + } + + // The 0o600 perm only applies when the file is created. + // If the file already exists, its permissions are + // preserved. We only update the content. + if err := afero.WriteFile(fs, filePath, secret.Value, 0o600); err != nil { + logger.Warn(ctx, "failed to write secret file", + slog.F("file_path", filePath), + slog.Error(err), + ) + continue + } + + logger.Debug(ctx, "wrote secret file", slog.F("file_path", filePath)) + } +} + func (*agent) wireguardAddresses(agentID uuid.UUID) []netip.Prefix { return []netip.Prefix{ // This is the IP that should be used primarily. diff --git a/agent/agent_test.go b/agent/agent_test.go index ba0949f000..72d27fda08 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -483,6 +483,155 @@ func TestAgent_Session_EnvironmentVariables(t *testing.T) { } } +func TestAgent_Session_SecretInjection(t *testing.T) { + t.Parallel() + + manifest := agentsdk.Manifest{ + EnvironmentVariables: map[string]string{ + "SHOULD_BE_OVERRIDDEN": "manifest-value", + }, + } + secrets := []agentsdk.WorkspaceSecret{ + {EnvName: "MY_SECRET_ENV", Value: []byte("env-secret-value")}, + {FilePath: "/tmp/secret-file", Value: []byte("file-secret-content")}, + {EnvName: "BOTH_ENV", FilePath: "/tmp/both-file", Value: []byte("both-value")}, + {EnvName: "SHOULD_BE_OVERRIDDEN", Value: []byte("secret-wins")}, + } + + ctx := testutil.Context(t, testutil.WaitLong) + //nolint:dogsled + conn, _, _, fs, _ := setupAgentWithSecrets(t, manifest, secrets, 0) + + // Verify file injection via the agent's filesystem. + content, err := afero.ReadFile(fs, "/tmp/secret-file") + require.NoError(t, err) + require.Equal(t, "file-secret-content", string(content)) + + content, err = afero.ReadFile(fs, "/tmp/both-file") + require.NoError(t, err) + require.Equal(t, "both-value", string(content)) + + // Verify env var injection via an SSH session. + sshClient, err := conn.SSHClient(ctx) + require.NoError(t, err) + t.Cleanup(func() { _ = sshClient.Close() }) + + session, err := sshClient.NewSession() + require.NoError(t, err) + t.Cleanup(func() { _ = session.Close() }) + + command := "sh" + if runtime.GOOS == "windows" { + command = "cmd.exe" + } + + stdin, err := session.StdinPipe() + require.NoError(t, err) + defer stdin.Close() + stdout, err := session.StdoutPipe() + require.NoError(t, err) + + err = session.Start(command) + require.NoError(t, err) + + go func() { + <-ctx.Done() + _ = session.Close() + }() + + s := bufio.NewScanner(stdout) + + echoEnv := func(t *testing.T, w io.Writer, env string) { + t.Helper() + if runtime.GOOS == "windows" { + _, err := fmt.Fprintf(w, "echo %%%s%%\r\n", env) + require.NoError(t, err) + } else { + _, err := fmt.Fprintf(w, "echo $%s\n", env) + require.NoError(t, err) + } + } + + for k, partialV := range map[string]string{ + "MY_SECRET_ENV": "env-secret-value", + "BOTH_ENV": "both-value", + "SHOULD_BE_OVERRIDDEN": "secret-wins", + } { + echoEnv(t, stdin, k) + found := false + for s.Scan() { + got := strings.TrimSpace(s.Text()) + t.Logf("%s=%s", k, got) + if strings.Contains(got, partialV) { + found = true + break + } + } + require.True(t, found, "env %s not found in output", k) + if err := s.Err(); !errors.Is(err, io.EOF) { + require.NoError(t, err) + } + } +} + +func TestAgent_StartupScript_SecretInjection(t *testing.T) { + t.Parallel() + + if runtime.GOOS == "windows" { + t.Skip("startup script test uses sh syntax") + } + + tmpDir := t.TempDir() + secretFilePath := filepath.Join(tmpDir, "secret-file") + envProofPath := filepath.Join(tmpDir, "env-proof") + fileProofPath := filepath.Join(tmpDir, "file-proof") + + // The startup script reads the secret env var and the secret file, + // writing both to proof files so we can verify they were available + // at script execution time. + script := fmt.Sprintf( + "echo \"$MY_STARTUP_SECRET\" > %s && cat %s > %s", + envProofPath, secretFilePath, fileProofPath, + ) + + manifest := agentsdk.Manifest{ + Scripts: []codersdk.WorkspaceAgentScript{{ + Script: script, + Timeout: 30 * time.Second, + RunOnStart: true, + }}, + } + secrets := []agentsdk.WorkspaceSecret{ + {EnvName: "MY_STARTUP_SECRET", Value: []byte("startup-env-value")}, + {FilePath: secretFilePath, Value: []byte("startup-file-content")}, + } + + // Use the real OS filesystem so that both writeSecretFiles and + // the startup script operate on the same filesystem. + //nolint:dogsled + _, client, _, _, _ := setupAgentWithSecrets(t, manifest, secrets, 0, func(_ *agenttest.Client, opts *agent.Options) { + opts.Filesystem = afero.NewOsFs() + }) + + // Wait for the startup script to complete. + var got []codersdk.WorkspaceAgentLifecycle + assert.Eventually(t, func() bool { + got = client.GetLifecycleStates() + return len(got) > 0 && got[len(got)-1] == codersdk.WorkspaceAgentLifecycleReady + }, testutil.WaitLong, testutil.IntervalMedium) + require.Contains(t, got, codersdk.WorkspaceAgentLifecycleReady, "agent never reached ready") + + // Verify the startup script could read the secret env var. + envProof, err := os.ReadFile(envProofPath) + require.NoError(t, err) + require.Equal(t, "startup-env-value", strings.TrimSpace(string(envProof))) + + // Verify the startup script could read the secret file. + fileProof, err := os.ReadFile(fileProofPath) + require.NoError(t, err) + require.Equal(t, "startup-file-content", string(fileProof)) +} + func TestAgent_GitSSH(t *testing.T) { t.Parallel() session := setupSSHSession(t, agentsdk.Manifest{}, codersdk.ServiceBannerConfig{}, nil) @@ -3305,8 +3454,10 @@ func TestAgent_DebugServer(t *testing.T) { require.NoError(t, os.WriteFile(logPath, []byte(randLogStr), 0o600)) derpMap, _ := tailnettest.RunDERPAndSTUN(t) //nolint:dogsled - conn, _, _, _, agnt := setupAgent(t, agentsdk.Manifest{ + conn, _, _, _, agnt := setupAgentWithSecrets(t, agentsdk.Manifest{ DERPMap: derpMap, + }, []agentsdk.WorkspaceSecret{ + {EnvName: "DEBUG_SECRET", Value: []byte("super-secret-value-12345")}, }, 0, func(c *agenttest.Client, o *agent.Options) { o.LogDir = logDir }) @@ -3408,6 +3559,31 @@ func TestAgent_DebugServer(t *testing.T) { require.NotNil(t, v) }) + t.Run("ManifestSecretsStripped", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL+"/debug/manifest", nil) + require.NoError(t, err) + + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, http.StatusOK, res.StatusCode) + + body, err := io.ReadAll(res.Body) + require.NoError(t, err) + + // The response must not contain the secret value. + require.NotContains(t, string(body), "super-secret-value-12345") + + // Confirm we can decode as a Manifest. The SDK type + // intentionally has no Secrets field, so there is nothing + // to leak through JSON encoding. + var v agentsdk.Manifest + require.NoError(t, json.Unmarshal(body, &v)) + }) + t.Run("Logs", func(t *testing.T) { t.Parallel() @@ -3559,6 +3735,20 @@ func setupAgent(t testing.TB, metadata agentsdk.Manifest, ptyTimeout time.Durati <-chan *proto.Stats, afero.Fs, agent.Agent, +) { + return setupAgentWithSecrets(t, metadata, nil, ptyTimeout, opts...) +} + +// setupAgentWithSecrets is like setupAgent but also injects user +// secrets into the agent's proto manifest. Separate from setupAgent +// because agentsdk.Manifest intentionally does not carry secrets; see +// the Manifest doc comment in codersdk/agentsdk. +func setupAgentWithSecrets(t testing.TB, metadata agentsdk.Manifest, secrets []agentsdk.WorkspaceSecret, ptyTimeout time.Duration, opts ...func(*agenttest.Client, *agent.Options)) ( + workspacesdk.AgentConn, + *agenttest.Client, + <-chan *proto.Stats, + afero.Fs, + agent.Agent, ) { logger := slogtest.Make(t, &slogtest.Options{ // Agent can drop errors when shutting down, and some, like the @@ -3589,7 +3779,7 @@ func setupAgent(t testing.TB, metadata agentsdk.Manifest, ptyTimeout time.Durati }) statsCh := make(chan *proto.Stats, 50) fs := afero.NewMemMapFs() - c := agenttest.NewClient(t, logger.Named("agenttest"), metadata.AgentID, metadata, statsCh, coordinator) + c := agenttest.NewClientWithSecrets(t, logger.Named("agenttest"), metadata.AgentID, metadata, secrets, statsCh, coordinator) t.Cleanup(c.Close) options := agent.Options{ diff --git a/agent/agenttest/client.go b/agent/agenttest/client.go index f61bf21c3e..b367252520 100644 --- a/agent/agenttest/client.go +++ b/agent/agenttest/client.go @@ -40,6 +40,21 @@ func NewClient(t testing.TB, manifest agentsdk.Manifest, statsChan chan *agentproto.Stats, coordinator tailnet.Coordinator, +) *Client { + return NewClientWithSecrets(t, logger, agentID, manifest, nil, statsChan, coordinator) +} + +// NewClientWithSecrets is like NewClient but also injects user +// secrets into the agent's proto manifest. Separate from NewClient +// because agentsdk.Manifest intentionally does not carry secrets; +// see the Manifest doc comment in codersdk/agentsdk. +func NewClientWithSecrets(t testing.TB, + logger slog.Logger, + agentID uuid.UUID, + manifest agentsdk.Manifest, + secrets []agentsdk.WorkspaceSecret, + statsChan chan *agentproto.Stats, + coordinator tailnet.Coordinator, ) *Client { if manifest.AgentID == uuid.Nil { manifest.AgentID = agentID @@ -58,6 +73,7 @@ func NewClient(t testing.TB, require.NoError(t, err) mp, err := agentsdk.ProtoFromManifest(manifest) require.NoError(t, err) + mp.Secrets = agentsdk.ProtoFromSecrets(secrets) fakeAAPI := NewFakeAgentAPI(t, logger, mp, statsChan) err = agentproto.DRPCRegisterAgent(mux, fakeAAPI) require.NoError(t, err) diff --git a/agent/write_secret_files_test.go b/agent/write_secret_files_test.go new file mode 100644 index 0000000000..935d1ba736 --- /dev/null +++ b/agent/write_secret_files_test.go @@ -0,0 +1,185 @@ +package agent //nolint:testpackage // Exercises internal agent secrets handling. + +import ( + "testing" + + "github.com/spf13/afero" + "github.com/stretchr/testify/require" + + "cdr.dev/slog/v3/sloggers/slogtest" + "github.com/coder/coder/v2/codersdk/agentsdk" + "github.com/coder/coder/v2/testutil" +) + +func TestWriteSecretFiles(t *testing.T) { + t.Parallel() + + t.Run("AbsolutePath", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil) + + writeSecretFiles(ctx, logger, fs, "/home/coder", []agentsdk.WorkspaceSecret{ + {FilePath: "/etc/myapp/config.json", Value: []byte(`{"key":"val"}`)}, + }) + + content, err := afero.ReadFile(fs, "/etc/myapp/config.json") + require.NoError(t, err) + require.Equal(t, `{"key":"val"}`, string(content)) + + fi, err := fs.Stat("/etc/myapp/config.json") + require.NoError(t, err) + require.Equal(t, 0o600, int(fi.Mode().Perm())) + + di, err := fs.Stat("/etc/myapp") + require.NoError(t, err) + require.Equal(t, 0o700, int(di.Mode().Perm())) + }) + + t.Run("TildePath", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil) + + writeSecretFiles(ctx, logger, fs, "/home/coder", []agentsdk.WorkspaceSecret{ + {FilePath: "~/.ssh/id_rsa", Value: []byte("private-key")}, + }) + + content, err := afero.ReadFile(fs, "/home/coder/.ssh/id_rsa") + require.NoError(t, err) + require.Equal(t, "private-key", string(content)) + + fi, err := fs.Stat("/home/coder/.ssh/id_rsa") + require.NoError(t, err) + require.Equal(t, 0o600, int(fi.Mode().Perm())) + + di, err := fs.Stat("/home/coder/.ssh") + require.NoError(t, err) + require.Equal(t, 0o700, int(di.Mode().Perm())) + }) + + t.Run("TildePathNoHomeDir", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil) + + writeSecretFiles(ctx, logger, fs, "", []agentsdk.WorkspaceSecret{ + {FilePath: "~/.config/token", Value: []byte("token")}, + }) + + empty, err := afero.IsEmpty(fs, "/") + require.NoError(t, err) + require.True(t, empty, "no file should be written when home dir is unknown") + }) + + t.Run("EmptyFilePathSkipped", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil) + + writeSecretFiles(ctx, logger, fs, "/home/coder", []agentsdk.WorkspaceSecret{ + {EnvName: "MY_TOKEN", Value: []byte("token")}, + }) + + // Nothing should be written. + empty, err := afero.IsEmpty(fs, "/") + require.NoError(t, err) + require.True(t, empty) + }) + + t.Run("MultipleSecrets", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil) + + writeSecretFiles(ctx, logger, fs, "/home/coder", []agentsdk.WorkspaceSecret{ + {FilePath: "/etc/secret-a", Value: []byte("aaa")}, + {FilePath: "~/.secret-b", Value: []byte("bbb")}, + {EnvName: "SKIP_ME", Value: []byte("env-only")}, + }) + + a, err := afero.ReadFile(fs, "/etc/secret-a") + require.NoError(t, err) + require.Equal(t, "aaa", string(a)) + + b, err := afero.ReadFile(fs, "/home/coder/.secret-b") + require.NoError(t, err) + require.Equal(t, "bbb", string(b)) + }) + + t.Run("OverwritesExisting", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil) + + require.NoError(t, afero.WriteFile(fs, "/secret", []byte("old"), 0o644)) + + writeSecretFiles(ctx, logger, fs, "", []agentsdk.WorkspaceSecret{ + {FilePath: "/secret", Value: []byte("new")}, + }) + + content, err := afero.ReadFile(fs, "/secret") + require.NoError(t, err) + require.Equal(t, "new", string(content)) + + // Pre-existing file permissions are intentionally preserved. + // The file may not have been created by us (e.g. a template + // provisioned it), so we should not alter its permissions. + fi, err := fs.Stat("/secret") + require.NoError(t, err) + require.Equal(t, 0o644, int(fi.Mode().Perm())) + }) + + t.Run("PathCollisionAfterTildeResolution", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil) + + // "~/collide" and "/home/coder/collide" resolve to the same + // absolute path. The later secret should win. + writeSecretFiles(ctx, logger, fs, "/home/coder", []agentsdk.WorkspaceSecret{ + {FilePath: "~/collide", Value: []byte("first")}, + {FilePath: "/home/coder/collide", Value: []byte("second")}, + }) + + content, err := afero.ReadFile(fs, "/home/coder/collide") + require.NoError(t, err) + require.Equal(t, "second", string(content)) + }) + + t.Run("EmptySlice", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil) + + writeSecretFiles(ctx, logger, fs, "/home/coder", nil) + + empty, err := afero.IsEmpty(fs, "/") + require.NoError(t, err) + require.True(t, empty) + }) + + t.Run("BinaryContent", func(t *testing.T) { + t.Parallel() + fs := afero.NewMemMapFs() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil) + + binaryData := []byte{0x00, 0x01, 0x02, 0xFF, 0xFE, 0xFD} + writeSecretFiles(ctx, logger, fs, "", []agentsdk.WorkspaceSecret{ + {FilePath: "/cert.der", Value: binaryData}, + }) + + content, err := afero.ReadFile(fs, "/cert.der") + require.NoError(t, err) + require.Equal(t, binaryData, content) + }) +} diff --git a/coderd/agentapi/manifest.go b/coderd/agentapi/manifest.go index 77834bc36c..c123913a12 100644 --- a/coderd/agentapi/manifest.go +++ b/coderd/agentapi/manifest.go @@ -90,6 +90,14 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest return nil, xerrors.Errorf("fetching workspace agent data: %w", err) } + // Fetch user secrets for injection into the agent manifest. + // This runs after the errgroup because it needs workspace.OwnerID. + //nolint:gocritic // System context needed to read secrets for the workspace owner. + userSecrets, err := a.Database.ListUserSecretsWithValues(dbauthz.AsSystemRestricted(ctx), workspace.OwnerID) + if err != nil { + return nil, xerrors.Errorf("getting user secrets: %w", err) + } + appSlug := appurl.ApplicationURL{ AppSlugOrPort: "{{port}}", AgentName: workspaceAgent.Name, @@ -141,6 +149,7 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest Apps: apps, Metadata: dbAgentMetadataToProtoDescription(metadata), Devcontainers: dbAgentDevcontainersToProto(devcontainers), + Secrets: dbUserSecretsToProto(userSecrets), }, nil } @@ -265,3 +274,21 @@ func dbAgentDevcontainersToProto(devcontainers []database.WorkspaceAgentDevconta } return ret } + +func dbUserSecretsToProto(secrets []database.UserSecret) []*agentproto.WorkspaceSecret { + ret := make([]*agentproto.WorkspaceSecret, 0, len(secrets)) + for _, s := range secrets { + // Only include secrets that have an environment variable + // name or file path set. Secrets with neither are not + // injected at runtime. + if s.EnvName == "" && s.FilePath == "" { + continue + } + ret = append(ret, &agentproto.WorkspaceSecret{ + EnvName: s.EnvName, + FilePath: s.FilePath, + Value: []byte(s.Value), + }) + } + return ret +} diff --git a/coderd/agentapi/manifest_test.go b/coderd/agentapi/manifest_test.go index a885ea1bd9..bbcef00f83 100644 --- a/coderd/agentapi/manifest_test.go +++ b/coderd/agentapi/manifest_test.go @@ -336,6 +336,7 @@ func TestGetManifest(t *testing.T) { }).Return(metadata, nil) mDB.EXPECT().GetWorkspaceAgentDevcontainersByAgentID(gomock.Any(), agent.ID).Return(devcontainers, nil) mDB.EXPECT().GetWorkspaceByID(gomock.Any(), workspace.ID).Return(workspace, nil) + mDB.EXPECT().ListUserSecretsWithValues(gomock.Any(), workspace.OwnerID).Return(nil, nil) got, err := api.GetManifest(context.Background(), &agentproto.GetManifestRequest{}) require.NoError(t, err) @@ -362,6 +363,7 @@ func TestGetManifest(t *testing.T) { Apps: protoApps, Metadata: protoMetadata, Devcontainers: protoDevcontainers, + Secrets: []*agentproto.WorkspaceSecret{}, } // Log got and expected with spew. @@ -401,6 +403,7 @@ func TestGetManifest(t *testing.T) { }).Return([]database.WorkspaceAgentMetadatum{}, nil) mDB.EXPECT().GetWorkspaceAgentDevcontainersByAgentID(gomock.Any(), childAgent.ID).Return([]database.WorkspaceAgentDevcontainer{}, nil) mDB.EXPECT().GetWorkspaceByID(gomock.Any(), workspace.ID).Return(workspace, nil) + mDB.EXPECT().ListUserSecretsWithValues(gomock.Any(), workspace.OwnerID).Return(nil, nil) got, err := api.GetManifest(context.Background(), &agentproto.GetManifestRequest{}) require.NoError(t, err) @@ -427,11 +430,71 @@ func TestGetManifest(t *testing.T) { Apps: []*agentproto.WorkspaceApp{}, Metadata: []*agentproto.WorkspaceAgentMetadata_Description{}, Devcontainers: []*agentproto.WorkspaceAgentDevcontainer{}, + Secrets: []*agentproto.WorkspaceSecret{}, } require.Equal(t, expected, got) }) + t.Run("SecretsFiltering", func(t *testing.T) { + t.Parallel() + + mDB := dbmock.NewMockStore(gomock.NewController(t)) + + api := &agentapi.ManifestAPI{ + AccessURL: &url.URL{Scheme: "https", Host: "example.com"}, + AppHostname: "*--apps.example.com", + ExternalAuthConfigs: []*externalauth.Config{ + {Type: string(codersdk.EnhancedExternalAuthProviderGitHub)}, + {Type: "some-provider"}, + {Type: string(codersdk.EnhancedExternalAuthProviderGitLab)}, + }, + DisableDirectConnections: true, + DerpForceWebSockets: true, + + AgentFn: func(ctx context.Context) (database.WorkspaceAgent, error) { return childAgent, nil }, + WorkspaceID: workspace.ID, + Database: mDB, + DerpMapFn: derpMapFn, + } + + mDB.EXPECT().GetWorkspaceAppsByAgentID(gomock.Any(), childAgent.ID).Return([]database.WorkspaceApp{}, nil) + mDB.EXPECT().GetWorkspaceAgentScriptsByAgentIDs(gomock.Any(), []uuid.UUID{childAgent.ID}).Return([]database.WorkspaceAgentScript{}, nil) + mDB.EXPECT().GetWorkspaceAgentMetadata(gomock.Any(), database.GetWorkspaceAgentMetadataParams{ + WorkspaceAgentID: childAgent.ID, + Keys: nil, + }).Return([]database.WorkspaceAgentMetadatum{}, nil) + mDB.EXPECT().GetWorkspaceAgentDevcontainersByAgentID(gomock.Any(), childAgent.ID).Return([]database.WorkspaceAgentDevcontainer{}, nil) + mDB.EXPECT().GetWorkspaceByID(gomock.Any(), workspace.ID).Return(workspace, nil) + + // Return a mix of secrets: env-only, file-only, both, and + // one with neither set. The last should be filtered out. + mDB.EXPECT().ListUserSecretsWithValues(gomock.Any(), workspace.OwnerID).Return([]database.UserSecret{ + {EnvName: "GITHUB_TOKEN", FilePath: "", Value: "ghp_xxxx"}, + {EnvName: "", FilePath: "~/.ssh/id_rsa", Value: "private-key"}, + {EnvName: "BOTH_ENV", FilePath: "/etc/both", Value: "both-val"}, + {EnvName: "", FilePath: "", Value: "stored-only"}, + }, nil) + + got, err := api.GetManifest(context.Background(), &agentproto.GetManifestRequest{}) + require.NoError(t, err) + + // The secret with neither env_name nor file_path should + // be filtered out, leaving exactly 3. + require.Len(t, got.Secrets, 3) + require.Equal(t, "GITHUB_TOKEN", got.Secrets[0].EnvName) + require.Equal(t, "", got.Secrets[0].FilePath) + require.Equal(t, []byte("ghp_xxxx"), got.Secrets[0].Value) + + require.Equal(t, "", got.Secrets[1].EnvName) + require.Equal(t, "~/.ssh/id_rsa", got.Secrets[1].FilePath) + require.Equal(t, []byte("private-key"), got.Secrets[1].Value) + + require.Equal(t, "BOTH_ENV", got.Secrets[2].EnvName) + require.Equal(t, "/etc/both", got.Secrets[2].FilePath) + require.Equal(t, []byte("both-val"), got.Secrets[2].Value) + }) + t.Run("NoAppHostname", func(t *testing.T) { t.Parallel() @@ -522,6 +585,7 @@ func TestGetManifest(t *testing.T) { }).Return(metadata, nil) mDB.EXPECT().GetWorkspaceAgentDevcontainersByAgentID(gomock.Any(), agent.ID).Return(devcontainers, nil) mDB.EXPECT().GetWorkspaceByID(gomock.Any(), workspace.ID).Return(workspace, nil) + mDB.EXPECT().ListUserSecretsWithValues(gomock.Any(), workspace.OwnerID).Return(nil, nil) got, err := api.GetManifest(context.Background(), &agentproto.GetManifestRequest{}) require.NoError(t, err) @@ -547,6 +611,7 @@ func TestGetManifest(t *testing.T) { Apps: protoApps, Metadata: protoMetadata, Devcontainers: protoDevcontainers, + Secrets: []*agentproto.WorkspaceSecret{}, } // Log got and expected with spew. diff --git a/codersdk/agentsdk/agentsdk.go b/codersdk/agentsdk/agentsdk.go index 8b008aea01..b660db0f87 100644 --- a/codersdk/agentsdk/agentsdk.go +++ b/codersdk/agentsdk/agentsdk.go @@ -101,6 +101,15 @@ type PostMetadataRequest struct { // performance. type PostMetadataRequestDeprecated = codersdk.WorkspaceAgentMetadataResult +// Manifest is the workspace agent's view of its own configuration. +// +// Secrets are intentionally not a field on this struct. The manifest +// may be serialized (JSON, %+v, logger fields, debug endpoints) in +// many places that do not and should not carry secret values. +// Keeping Secrets off of the struct makes leaking them impossible +// via any code path that only holds a *Manifest. Callers that need +// secrets must load them explicitly via SecretsFromProto on the raw +// proto. type Manifest struct { ParentID uuid.UUID `json:"parent_id"` AgentID uuid.UUID `json:"agent_id"` @@ -128,6 +137,16 @@ type Manifest struct { Devcontainers []codersdk.WorkspaceAgentDevcontainer `json:"devcontainers"` } +// WorkspaceSecret is a user secret for injection into a workspace. +// +// Value carries decrypted secret material and is omitted from JSON +// serialization to protect against future leaking of the secret. +type WorkspaceSecret struct { + EnvName string + FilePath string + Value []byte `json:"-"` +} + type LogSource struct { ID uuid.UUID `json:"id"` DisplayName string `json:"display_name"` diff --git a/codersdk/agentsdk/convert.go b/codersdk/agentsdk/convert.go index ca36a6eba1..46cecff8de 100644 --- a/codersdk/agentsdk/convert.go +++ b/codersdk/agentsdk/convert.go @@ -14,6 +14,11 @@ import ( "github.com/coder/coder/v2/tailnet" ) +// ManifestFromProto converts the proto manifest to the SDK Manifest. +// Secrets are intentionally NOT included on the returned Manifest: +// keeping them off of the SDK type makes it impossible for any code +// path that only holds a *Manifest to leak secret values via +// logging, JSON encoding, fmt verbs, or debug endpoints. func ManifestFromProto(manifest *proto.Manifest) (Manifest, error) { parentID := uuid.Nil if pid := manifest.GetParentId(); pid != nil { @@ -65,6 +70,9 @@ func ManifestFromProto(manifest *proto.Manifest) (Manifest, error) { }, nil } +// ProtoFromManifest converts the SDK Manifest to the proto manifest. +// It does not populate the proto's Secrets field because the SDK +// Manifest intentionally does not carry secrets (see ManifestFromProto). func ProtoFromManifest(manifest Manifest) (*proto.Manifest, error) { apps, err := ProtoFromApps(manifest.Apps) if err != nil { @@ -477,3 +485,27 @@ func ProtoFromPatchAppStatus(pas PatchAppStatus) (*proto.UpdateAppStatusRequest, Uri: pas.URI, }, nil } + +func SecretsFromProto(protoSecrets []*proto.WorkspaceSecret) []WorkspaceSecret { + ret := make([]WorkspaceSecret, len(protoSecrets)) + for i, s := range protoSecrets { + ret[i] = WorkspaceSecret{ + EnvName: s.EnvName, + FilePath: s.FilePath, + Value: s.Value, + } + } + return ret +} + +func ProtoFromSecrets(secrets []WorkspaceSecret) []*proto.WorkspaceSecret { + ret := make([]*proto.WorkspaceSecret, len(secrets)) + for i, s := range secrets { + ret[i] = &proto.WorkspaceSecret{ + EnvName: s.EnvName, + FilePath: s.FilePath, + Value: s.Value, + } + } + return ret +} diff --git a/codersdk/agentsdk/convert_test.go b/codersdk/agentsdk/convert_test.go index 15f063f4e4..4d97481f92 100644 --- a/codersdk/agentsdk/convert_test.go +++ b/codersdk/agentsdk/convert_test.go @@ -233,3 +233,39 @@ func TestMetadataFromProto(t *testing.T) { require.Equal(t, "lemons", smd.Value) require.Equal(t, "rats", smd.Error) } + +func TestSecretsRoundTrip(t *testing.T) { + t.Parallel() + secrets := []agentsdk.WorkspaceSecret{ + { + EnvName: "GITHUB_TOKEN", + FilePath: "", + Value: []byte("ghp_xxxx"), + }, + { + EnvName: "", + FilePath: "~/.aws/credentials", + Value: []byte("[default]\naws_access_key_id=AKIA..."), + }, + { + EnvName: "BOTH_ENV", + FilePath: "/etc/both", + Value: []byte("both-value"), + }, + } + + protoSecrets := agentsdk.ProtoFromSecrets(secrets) + require.Len(t, protoSecrets, 3) + require.Equal(t, "GITHUB_TOKEN", protoSecrets[0].EnvName) + require.Equal(t, "", protoSecrets[0].FilePath) + require.Equal(t, []byte("ghp_xxxx"), protoSecrets[0].Value) + require.Equal(t, "", protoSecrets[1].EnvName) + require.Equal(t, "~/.aws/credentials", protoSecrets[1].FilePath) + require.Equal(t, []byte("[default]\naws_access_key_id=AKIA..."), protoSecrets[1].Value) + require.Equal(t, "BOTH_ENV", protoSecrets[2].EnvName) + require.Equal(t, "/etc/both", protoSecrets[2].FilePath) + require.Equal(t, []byte("both-value"), protoSecrets[2].Value) + + roundTripped := agentsdk.SecretsFromProto(protoSecrets) + require.Equal(t, secrets, roundTripped) +}