mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
feat: filter blocked user secret file targets (#28519)
When `DisableUserSecretFilePath` is set, coderd strips file targets while building the agent manifest: env-only secrets are unchanged, dual-target secrets keep the env var with an empty file path, and file-only secrets are omitted so their plaintext never crosses DRPC. Stored paths stay in the database and reappear on the next manifest fetch once the option is cleared. Agents converge on reconnect; no protobuf, provisioner, or agent-side changes are needed, and files already written to workspace disks are left alone. Also exposes the positive capability as the `user-secret-file-path-enabled` embedded metadata tag, which the next PR in the stack consumes in the dashboard. Builds on #28518. Refs #27488. Reviewed and updated by Coder Agents on behalf of @dylanhuff-at-coder.
This commit is contained in:
+11
-9
@@ -105,6 +105,7 @@ type Options struct {
|
||||
AgentStatsRefreshInterval time.Duration
|
||||
DisableDirectConnections bool
|
||||
DerpForceWebSockets bool
|
||||
DisableUserSecretFilePath bool
|
||||
DerpMapUpdateFrequency time.Duration
|
||||
ExternalAuthConfigs []*externalauth.Config
|
||||
Experiments codersdk.Experiments
|
||||
@@ -123,15 +124,16 @@ func New(opts Options, workspace database.Workspace, agent database.WorkspaceAge
|
||||
}
|
||||
|
||||
api.ManifestAPI = &ManifestAPI{
|
||||
AccessURL: opts.AccessURL,
|
||||
AppHostname: opts.AppHostname,
|
||||
ExternalAuthConfigs: opts.ExternalAuthConfigs,
|
||||
DisableDirectConnections: opts.DisableDirectConnections,
|
||||
DerpForceWebSockets: opts.DerpForceWebSockets,
|
||||
AgentFn: api.agent,
|
||||
Database: opts.Database,
|
||||
DerpMapFn: opts.DerpMapFn,
|
||||
WorkspaceID: opts.WorkspaceID,
|
||||
AccessURL: opts.AccessURL,
|
||||
AppHostname: opts.AppHostname,
|
||||
ExternalAuthConfigs: opts.ExternalAuthConfigs,
|
||||
DisableDirectConnections: opts.DisableDirectConnections,
|
||||
DerpForceWebSockets: opts.DerpForceWebSockets,
|
||||
DisableUserSecretFilePath: opts.DisableUserSecretFilePath,
|
||||
AgentFn: api.agent,
|
||||
Database: opts.Database,
|
||||
DerpMapFn: opts.DerpMapFn,
|
||||
WorkspaceID: opts.WorkspaceID,
|
||||
}
|
||||
|
||||
// Don't cache details for prebuilds, though the cached fields will eventually be updated
|
||||
|
||||
@@ -25,12 +25,13 @@ import (
|
||||
)
|
||||
|
||||
type ManifestAPI struct {
|
||||
AccessURL *url.URL
|
||||
AppHostname string
|
||||
ExternalAuthConfigs []*externalauth.Config
|
||||
DisableDirectConnections bool
|
||||
DerpForceWebSockets bool
|
||||
WorkspaceID uuid.UUID
|
||||
AccessURL *url.URL
|
||||
AppHostname string
|
||||
ExternalAuthConfigs []*externalauth.Config
|
||||
DisableDirectConnections bool
|
||||
DerpForceWebSockets bool
|
||||
DisableUserSecretFilePath bool
|
||||
WorkspaceID uuid.UUID
|
||||
|
||||
AgentFn func(ctx context.Context) (database.WorkspaceAgent, error)
|
||||
Database database.Store
|
||||
@@ -129,6 +130,11 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest
|
||||
parentID = workspaceAgent.ParentID.UUID[:]
|
||||
}
|
||||
|
||||
secretFilePathPolicy := userSecretFilePathAllowed
|
||||
if a.DisableUserSecretFilePath {
|
||||
secretFilePathPolicy = userSecretFilePathBlocked
|
||||
}
|
||||
|
||||
return &agentproto.Manifest{
|
||||
AgentId: workspaceAgent.ID[:],
|
||||
AgentName: workspaceAgent.Name,
|
||||
@@ -149,7 +155,7 @@ func (a *ManifestAPI) GetManifest(ctx context.Context, _ *agentproto.GetManifest
|
||||
Apps: apps,
|
||||
Metadata: dbAgentMetadataToProtoDescription(metadata),
|
||||
Devcontainers: dbAgentDevcontainersToProto(devcontainers),
|
||||
Secrets: dbUserSecretsToProto(userSecrets),
|
||||
Secrets: dbUserSecretsToProto(userSecrets, secretFilePathPolicy),
|
||||
}, nil
|
||||
}
|
||||
|
||||
@@ -275,7 +281,16 @@ func dbAgentDevcontainersToProto(devcontainers []database.WorkspaceAgentDevconta
|
||||
return ret
|
||||
}
|
||||
|
||||
func dbUserSecretsToProto(secrets []database.UserSecret) []*agentproto.WorkspaceSecret {
|
||||
// userSecretFilePathPolicy is a named type rather than a bool parameter to
|
||||
// satisfy revive's flag-parameter rule.
|
||||
type userSecretFilePathPolicy int
|
||||
|
||||
const (
|
||||
userSecretFilePathAllowed userSecretFilePathPolicy = iota
|
||||
userSecretFilePathBlocked
|
||||
)
|
||||
|
||||
func dbUserSecretsToProto(secrets []database.UserSecret, policy userSecretFilePathPolicy) []*agentproto.WorkspaceSecret {
|
||||
ret := make([]*agentproto.WorkspaceSecret, 0, len(secrets))
|
||||
for _, s := range secrets {
|
||||
// Skip disabled secrets so they are not injected as env vars or
|
||||
@@ -285,9 +300,16 @@ func dbUserSecretsToProto(secrets []database.UserSecret) []*agentproto.Workspace
|
||||
if !s.Enabled {
|
||||
continue
|
||||
}
|
||||
filePath := s.FilePath
|
||||
if policy == userSecretFilePathBlocked {
|
||||
if s.EnvName == "" {
|
||||
continue
|
||||
}
|
||||
filePath = ""
|
||||
}
|
||||
ret = append(ret, &agentproto.WorkspaceSecret{
|
||||
EnvName: s.EnvName,
|
||||
FilePath: s.FilePath,
|
||||
FilePath: filePath,
|
||||
Value: []byte(s.Value),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -7,9 +7,60 @@ import (
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
agentproto "github.com/coder/coder/v2/agent/proto"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
|
||||
)
|
||||
|
||||
func Test_dbUserSecretsToProto(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
secrets := []database.UserSecret{
|
||||
{Name: "env-only", EnvName: "ENV_ONLY", Value: "env-val", Enabled: true},
|
||||
{Name: "file-only", FilePath: "~/.ssh/id_rsa", Value: "file-val", Enabled: true},
|
||||
{Name: "dual", EnvName: "DUAL_ENV", FilePath: "/etc/dual", Value: "dual-val", Enabled: true},
|
||||
{Name: "disabled", EnvName: "DISABLED_ENV", FilePath: "/etc/disabled", Value: "disabled-val"},
|
||||
}
|
||||
|
||||
cases := []struct {
|
||||
name string
|
||||
policy userSecretFilePathPolicy
|
||||
expected []*agentproto.WorkspaceSecret
|
||||
}{
|
||||
{
|
||||
name: "FilePathAllowed",
|
||||
policy: userSecretFilePathAllowed,
|
||||
expected: []*agentproto.WorkspaceSecret{
|
||||
{EnvName: "ENV_ONLY", Value: []byte("env-val")},
|
||||
{FilePath: "~/.ssh/id_rsa", Value: []byte("file-val")},
|
||||
{EnvName: "DUAL_ENV", FilePath: "/etc/dual", Value: []byte("dual-val")},
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "FilePathBlocked",
|
||||
policy: userSecretFilePathBlocked,
|
||||
expected: []*agentproto.WorkspaceSecret{
|
||||
{EnvName: "ENV_ONLY", Value: []byte("env-val")},
|
||||
{EnvName: "DUAL_ENV", Value: []byte("dual-val")},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
for _, c := range cases {
|
||||
t.Run(c.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
got := dbUserSecretsToProto(secrets, c.policy)
|
||||
require.Len(t, got, len(c.expected))
|
||||
for i, want := range c.expected {
|
||||
require.Equal(t, want.EnvName, got[i].EnvName, "secret %d env_name", i)
|
||||
require.Equal(t, want.FilePath, got[i].FilePath, "secret %d file_path", i)
|
||||
require.Equal(t, want.Value, got[i].Value, "secret %d value", i)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func Test_vscodeProxyURI(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
+13
-12
@@ -776,18 +776,19 @@ func New(options *Options) *API {
|
||||
Telemetry: api.Telemetry.Enabled(),
|
||||
}
|
||||
api.SiteHandler, err = site.New(&site.Options{
|
||||
CacheDir: siteCacheDir,
|
||||
Database: options.Database,
|
||||
Authorizer: options.Authorizer,
|
||||
SiteFS: site.FS(),
|
||||
OAuth2Configs: oauthConfigs,
|
||||
DocsURL: options.DeploymentValues.DocsURL.String(),
|
||||
AppearanceFetcher: &api.AppearanceFetcher,
|
||||
BuildInfo: buildInfo,
|
||||
Entitlements: options.Entitlements,
|
||||
Telemetry: options.Telemetry,
|
||||
Logger: options.Logger.Named("site"),
|
||||
AIGatewayEnabled: options.DeploymentValues.AI.BridgeConfig.Enabled.Value(),
|
||||
CacheDir: siteCacheDir,
|
||||
Database: options.Database,
|
||||
Authorizer: options.Authorizer,
|
||||
SiteFS: site.FS(),
|
||||
OAuth2Configs: oauthConfigs,
|
||||
DocsURL: options.DeploymentValues.DocsURL.String(),
|
||||
AppearanceFetcher: &api.AppearanceFetcher,
|
||||
BuildInfo: buildInfo,
|
||||
Entitlements: options.Entitlements,
|
||||
Telemetry: options.Telemetry,
|
||||
Logger: options.Logger.Named("site"),
|
||||
AIGatewayEnabled: options.DeploymentValues.AI.BridgeConfig.Enabled.Value(),
|
||||
UserSecretFilePathEnabled: !options.DeploymentValues.DisableUserSecretFilePath.Value(),
|
||||
})
|
||||
if err != nil {
|
||||
options.Logger.Fatal(ctx, "failed to initialize site handler", slog.Error(err))
|
||||
|
||||
@@ -183,6 +183,7 @@ func (api *API) workspaceAgentRPC(rw http.ResponseWriter, r *http.Request) {
|
||||
AgentStatsRefreshInterval: api.AgentStatsRefreshInterval,
|
||||
DisableDirectConnections: api.DeploymentValues.DERP.Config.BlockDirect.Value(),
|
||||
DerpForceWebSockets: api.DeploymentValues.DERP.Config.ForceWebSockets.Value(),
|
||||
DisableUserSecretFilePath: api.DeploymentValues.DisableUserSecretFilePath.Value(),
|
||||
DerpMapUpdateFrequency: api.DERPMapUpdateFrequency,
|
||||
ExternalAuthConfigs: api.ExternalAuthConfigs,
|
||||
Experiments: api.Experiments,
|
||||
|
||||
@@ -13,8 +13,11 @@ import (
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||
"github.com/coder/coder/v2/coderd/database/dbfake"
|
||||
"github.com/coder/coder/v2/coderd/database/dbgen"
|
||||
"github.com/coder/coder/v2/coderd/database/dbtestutil"
|
||||
"github.com/coder/coder/v2/coderd/database/dbtime"
|
||||
"github.com/coder/coder/v2/coderd/rbac"
|
||||
"github.com/coder/coder/v2/codersdk"
|
||||
"github.com/coder/coder/v2/codersdk/agentsdk"
|
||||
"github.com/coder/coder/v2/provisionersdk/proto"
|
||||
"github.com/coder/coder/v2/testutil"
|
||||
@@ -170,6 +173,59 @@ func TestAgentAPI_LargeManifest(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// End-to-end wiring: the deployment option must reach the manifest the
|
||||
// agent actually receives over RPC.
|
||||
func TestWorkspaceAgentRPCUserSecretFilePathDisabled(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
dv := coderdtest.DeploymentValues(t, func(dv *codersdk.DeploymentValues) {
|
||||
dv.DisableUserSecretFilePath = true
|
||||
})
|
||||
db, ps := dbtestutil.NewDB(t)
|
||||
client := coderdtest.New(t, &coderdtest.Options{
|
||||
Database: db, Pubsub: ps, DeploymentValues: dv,
|
||||
})
|
||||
owner := coderdtest.CreateFirstUser(t, client)
|
||||
|
||||
// Seed rows the way a deployment that allowed file paths would have
|
||||
// stored them; the API rejects new paths while the policy is on.
|
||||
for _, seed := range []database.UserSecret{
|
||||
{Name: "env-only", Value: "env-value", EnvName: "ENV_ONLY"},
|
||||
{Name: "file-only", Value: "file-value", FilePath: "~/.file-only"},
|
||||
{Name: "dual-target", Value: "dual-value", EnvName: "DUAL_TARGET", FilePath: "~/.dual-target"},
|
||||
} {
|
||||
dbgen.UserSecret(t, db, database.UserSecret{UserID: owner.UserID, Name: seed.Name},
|
||||
func(p *database.CreateUserSecretParams) {
|
||||
p.Value, p.EnvName, p.FilePath, p.Enabled = seed.Value, seed.EnvName, seed.FilePath, true
|
||||
})
|
||||
}
|
||||
|
||||
workspace := dbfake.WorkspaceBuild(t, db, database.WorkspaceTable{
|
||||
OrganizationID: owner.OrganizationID,
|
||||
OwnerID: owner.UserID,
|
||||
}).WithAgent().Do()
|
||||
|
||||
agentClient := agentsdk.New(client.URL, agentsdk.WithFixedToken(workspace.AgentToken))
|
||||
conn, err := agentClient.ConnectRPC(ctx)
|
||||
require.NoError(t, err)
|
||||
defer conn.Close()
|
||||
|
||||
manifest, err := agentproto.NewDRPCAgentClient(conn).GetManifest(ctx, &agentproto.GetManifestRequest{})
|
||||
require.NoError(t, err)
|
||||
|
||||
require.Len(t, manifest.Secrets, 2)
|
||||
byEnv := make(map[string][]byte, len(manifest.Secrets))
|
||||
for _, secret := range manifest.Secrets {
|
||||
require.Empty(t, secret.FilePath)
|
||||
byEnv[secret.EnvName] = secret.Value
|
||||
}
|
||||
require.Equal(t, map[string][]byte{
|
||||
"ENV_ONLY": []byte("env-value"),
|
||||
"DUAL_TARGET": []byte("dual-value"),
|
||||
}, byEnv)
|
||||
}
|
||||
|
||||
func TestWorkspaceAgentRPCRole(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
@@ -29,6 +29,10 @@
|
||||
<meta property="docs-url" content="{{ .DocsURL }}" />
|
||||
<meta property="logo-url" content="{{ .LogoURL }}" />
|
||||
<meta property="ai-gateway-enabled" content="{{ .AIGatewayEnabled }}" />
|
||||
<meta
|
||||
property="user-secret-file-path-enabled"
|
||||
content="{{ .UserSecretFilePathEnabled }}"
|
||||
/>
|
||||
<meta property="permissions" content="{{ .Permissions }}" />
|
||||
<meta property="organizations" content="{{ .Organizations }}" />
|
||||
<link
|
||||
|
||||
+23
-15
@@ -71,18 +71,19 @@ func init() {
|
||||
}
|
||||
|
||||
type Options struct {
|
||||
CacheDir string
|
||||
Database database.Store
|
||||
Authorizer rbac.Authorizer
|
||||
SiteFS fs.FS
|
||||
OAuth2Configs *httpmw.OAuth2Configs
|
||||
DocsURL string
|
||||
BuildInfo codersdk.BuildInfoResponse
|
||||
AppearanceFetcher *atomic.Pointer[appearance.Fetcher]
|
||||
Entitlements *entitlements.Set
|
||||
Telemetry telemetry.Reporter
|
||||
Logger slog.Logger
|
||||
AIGatewayEnabled bool
|
||||
CacheDir string
|
||||
Database database.Store
|
||||
Authorizer rbac.Authorizer
|
||||
SiteFS fs.FS
|
||||
OAuth2Configs *httpmw.OAuth2Configs
|
||||
DocsURL string
|
||||
BuildInfo codersdk.BuildInfoResponse
|
||||
AppearanceFetcher *atomic.Pointer[appearance.Fetcher]
|
||||
Entitlements *entitlements.Set
|
||||
Telemetry telemetry.Reporter
|
||||
Logger slog.Logger
|
||||
AIGatewayEnabled bool
|
||||
UserSecretFilePathEnabled bool
|
||||
}
|
||||
|
||||
func New(opts *Options) (*Handler, error) {
|
||||
@@ -266,9 +267,10 @@ type htmlState struct {
|
||||
Regions string
|
||||
DocsURL string
|
||||
|
||||
AIGatewayEnabled string
|
||||
Permissions string
|
||||
Organizations string
|
||||
AIGatewayEnabled string
|
||||
UserSecretFilePathEnabled string
|
||||
Permissions string
|
||||
Organizations string
|
||||
}
|
||||
|
||||
type csrfState struct {
|
||||
@@ -520,6 +522,12 @@ func (h *Handler) populateHTMLState(
|
||||
state.AIGatewayEnabled = html.EscapeString(string(data))
|
||||
}
|
||||
})
|
||||
wg.Go(func() {
|
||||
data, err := json.Marshal(h.opts.UserSecretFilePathEnabled)
|
||||
if err == nil {
|
||||
state.UserSecretFilePathEnabled = html.EscapeString(string(data))
|
||||
}
|
||||
})
|
||||
wg.Go(func() {
|
||||
sdkOrgs := slice.List(userOrgs, db2sdk.Organization)
|
||||
data, err := json.Marshal(sdkOrgs)
|
||||
|
||||
@@ -470,6 +470,57 @@ func TestOrganizationsMetadata(t *testing.T) {
|
||||
assert.NotContains(t, memberOrgIDs, deletedOrg.ID)
|
||||
}
|
||||
|
||||
func TestUserSecretFilePathEnabledMetadata(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// The dashboard reads this as a positive capability, so the
|
||||
// metadata is the negation of the deployment's disable option.
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
enabled bool
|
||||
expected string
|
||||
}{
|
||||
{name: "Enabled", enabled: true, expected: "true"},
|
||||
{name: "Disabled", enabled: false, expected: "false"},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// GIVEN: a site handler configured with the capability and a
|
||||
// template that renders only that metadata value.
|
||||
siteFS := fstest.MapFS{
|
||||
"index.html": &fstest.MapFile{
|
||||
Data: []byte("{{ .UserSecretFilePathEnabled }}"),
|
||||
},
|
||||
}
|
||||
db, _ := dbtestutil.NewDB(t)
|
||||
handler, err := site.New(&site.Options{
|
||||
Telemetry: telemetry.NewNoop(),
|
||||
Database: db,
|
||||
SiteFS: siteFS,
|
||||
UserSecretFilePathEnabled: tc.enabled,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
user := dbgen.User(t, db, database.User{})
|
||||
_, token := dbgen.APIKey(t, db, database.APIKey{
|
||||
UserID: user.ID,
|
||||
ExpiresAt: time.Now().Add(time.Hour),
|
||||
})
|
||||
|
||||
// WHEN: an authenticated user loads the page.
|
||||
r := httptest.NewRequest("GET", "/", nil)
|
||||
r.Header.Set(codersdk.SessionTokenHeader, token)
|
||||
rw := httptest.NewRecorder()
|
||||
handler.ServeHTTP(rw, r)
|
||||
|
||||
// THEN: the metadata renders as a JSON boolean.
|
||||
require.Equal(t, http.StatusOK, rw.Code)
|
||||
require.Equal(t, tc.expected, strings.TrimSpace(rw.Body.String()))
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestCaching(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user