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) +}