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
This commit is contained in:
Zach
2026-04-17 16:55:24 -06:00
committed by GitHub
parent 8e2343f59c
commit 72f35e1cd3
9 changed files with 685 additions and 4 deletions
+113 -2
View File
@@ -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.
+192 -2
View File
@@ -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{
+16
View File
@@ -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)
+185
View File
@@ -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)
})
}
+27
View File
@@ -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
}
+65
View File
@@ -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.
+19
View File
@@ -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"`
+32
View File
@@ -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
}
+36
View File
@@ -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)
}