Skip to content

feat: add flag to disable all direct connections instance-wide #7936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jun 21, 2023
Merged
16 changes: 9 additions & 7 deletions agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ func (a *agent) run(ctx context.Context) error {
network := a.network
a.closeMutex.Unlock()
if network == nil {
network, err = a.createTailnet(ctx, manifest.DERPMap)
network, err = a.createTailnet(ctx, manifest.DERPMap, manifest.AllowDirectConnections)
if err != nil {
return xerrors.Errorf("create tailnet: %w", err)
}
Expand All @@ -605,8 +605,9 @@ func (a *agent) run(ctx context.Context) error {

a.startReportingConnectionStats(ctx)
} else {
// Update the DERP map!
// Update the DERP map and allow/disallow direct connections.
network.SetDERPMap(manifest.DERPMap)
network.SetBlockEndpoints(!manifest.AllowDirectConnections)
}

a.logger.Debug(ctx, "running tailnet connection coordinator")
Expand All @@ -631,12 +632,13 @@ func (a *agent) trackConnGoroutine(fn func()) error {
return nil
}

func (a *agent) createTailnet(ctx context.Context, derpMap *tailcfg.DERPMap) (_ *tailnet.Conn, err error) {
func (a *agent) createTailnet(ctx context.Context, derpMap *tailcfg.DERPMap, allowDirectConnections bool) (_ *tailnet.Conn, err error) {
network, err := tailnet.NewConn(&tailnet.Options{
Addresses: []netip.Prefix{netip.PrefixFrom(codersdk.WorkspaceAgentIP, 128)},
DERPMap: derpMap,
Logger: a.logger.Named("tailnet"),
ListenPort: a.tailnetListenPort,
Addresses: []netip.Prefix{netip.PrefixFrom(codersdk.WorkspaceAgentIP, 128)},
DERPMap: derpMap,
Logger: a.logger.Named("tailnet"),
ListenPort: a.tailnetListenPort,
BlockEndpoints: !allowDirectConnections,
})
if err != nil {
return nil, xerrors.Errorf("create tailnet: %w", err)
Expand Down
1 change: 1 addition & 0 deletions cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
derpMap, err := tailnet.NewDERPMap(
ctx, defaultRegion, cfg.DERP.Server.STUNAddresses,
cfg.DERP.Config.URL.String(), cfg.DERP.Config.Path.String(),
cfg.DERP.Config.DisableDirect.Value(),
)
if err != nil {
return xerrors.Errorf("create derp map: %w", err)
Expand Down
8 changes: 8 additions & 0 deletions cli/testdata/coder_server_--help.golden
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,14 @@ backed by Tailscale and WireGuard.
Addresses for STUN servers to establish P2P connections. Use special
value 'disable' to turn off STUN.

--disable-direct bool, $CODER_DISABLE_DIRECT
Disable peer-to-peer (aka. direct) workspace connections. All
workspace connections from the CLI will be proxied through Coder (or
custom configured DERP servers) and will never be peer-to-peer when
enabled. Workspaces may still reach out to STUN servers to get their
address until they are restarted after this change has been made, but
new connections will still be proxied regardless.

Networking / HTTP Options
--disable-password-auth bool, $CODER_DISABLE_PASSWORD_AUTH
Disable password authentication. This is recommended for security
Expand Down
7 changes: 7 additions & 0 deletions cli/testdata/server-config.yaml.golden
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,13 @@ networking:
# for high availability.
# (default: <unset>, type: url)
relayURL:
# Disable peer-to-peer (aka. direct) workspace connections. All workspace
# connections from the CLI will be proxied through Coder (or custom configured
# DERP servers) and will never be peer-to-peer when enabled. Workspaces may still
# reach out to STUN servers to get their address until they are restarted after
# this change has been made, but new connections will still be proxied regardless.
# (default: <unset>, type: bool)
disableDirect: false
# URL to fetch a DERP mapping on startup. See:
# https://tailscale.com/kb/1118/custom-derp-servers/.
# (default: <unset>, type: string)
Expand Down
9 changes: 9 additions & 0 deletions coderd/apidoc/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions coderd/apidoc/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

78 changes: 42 additions & 36 deletions coderd/coderdtest/coderdtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
}

stunAddr, stunCleanup := stuntest.ServeWithPacketListener(t, nettype.Std{})
stunAddr.IP = net.ParseIP("127.0.0.1")
t.Cleanup(stunCleanup)

derpServer := derp.NewServer(key.NewNode(), tailnet.Logger(slogtest.Make(t, nil).Named("derp").Leveled(slog.LevelDebug)))
Expand All @@ -306,6 +307,29 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
require.NoError(t, err)
}

region := &tailcfg.DERPRegion{
EmbeddedRelay: true,
RegionID: int(options.DeploymentValues.DERP.Server.RegionID.Value()),
RegionCode: options.DeploymentValues.DERP.Server.RegionCode.String(),
RegionName: options.DeploymentValues.DERP.Server.RegionName.String(),
Nodes: []*tailcfg.DERPNode{{
Name: fmt.Sprintf("%db", options.DeploymentValues.DERP.Server.RegionID),
RegionID: int(options.DeploymentValues.DERP.Server.RegionID.Value()),
IPv4: "127.0.0.1",
DERPPort: derpPort,
// STUN port is added as a separate node by tailnet.NewDERPMap() if
// direct connections are enabled.
STUNPort: -1,
InsecureForTests: true,
ForceHTTP: options.TLSCertificates == nil,
}},
}
if !options.DeploymentValues.DERP.Server.Enable.Value() {
region = nil
}
derpMap, err := tailnet.NewDERPMap(ctx, region, []string{stunAddr.String()}, "", "", !options.DeploymentValues.DERP.Config.DisableDirect.Value())
require.NoError(t, err)

return func(h http.Handler) {
mutex.Lock()
defer mutex.Unlock()
Expand All @@ -324,42 +348,24 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
Pubsub: options.Pubsub,
GitAuthConfigs: options.GitAuthConfigs,

Auditor: options.Auditor,
AWSCertificates: options.AWSCertificates,
AzureCertificates: options.AzureCertificates,
GithubOAuth2Config: options.GithubOAuth2Config,
RealIPConfig: options.RealIPConfig,
OIDCConfig: options.OIDCConfig,
GoogleTokenValidator: options.GoogleTokenValidator,
SSHKeygenAlgorithm: options.SSHKeygenAlgorithm,
DERPServer: derpServer,
APIRateLimit: options.APIRateLimit,
LoginRateLimit: options.LoginRateLimit,
FilesRateLimit: options.FilesRateLimit,
Authorizer: options.Authorizer,
Telemetry: telemetry.NewNoop(),
TemplateScheduleStore: &templateScheduleStore,
TLSCertificates: options.TLSCertificates,
TrialGenerator: options.TrialGenerator,
DERPMap: &tailcfg.DERPMap{
Regions: map[int]*tailcfg.DERPRegion{
1: {
EmbeddedRelay: true,
RegionID: 1,
RegionCode: "coder",
RegionName: "Coder",
Nodes: []*tailcfg.DERPNode{{
Name: "1a",
RegionID: 1,
IPv4: "127.0.0.1",
DERPPort: derpPort,
STUNPort: stunAddr.Port,
InsecureForTests: true,
ForceHTTP: options.TLSCertificates == nil,
}},
},
},
},
Auditor: options.Auditor,
AWSCertificates: options.AWSCertificates,
AzureCertificates: options.AzureCertificates,
GithubOAuth2Config: options.GithubOAuth2Config,
RealIPConfig: options.RealIPConfig,
OIDCConfig: options.OIDCConfig,
GoogleTokenValidator: options.GoogleTokenValidator,
SSHKeygenAlgorithm: options.SSHKeygenAlgorithm,
DERPServer: derpServer,
APIRateLimit: options.APIRateLimit,
LoginRateLimit: options.LoginRateLimit,
FilesRateLimit: options.FilesRateLimit,
Authorizer: options.Authorizer,
Telemetry: telemetry.NewNoop(),
TemplateScheduleStore: &templateScheduleStore,
TLSCertificates: options.TLSCertificates,
TrialGenerator: options.TrialGenerator,
DERPMap: derpMap,
MetricsCacheRefreshInterval: options.MetricsCacheRefreshInterval,
AgentStatsRefreshInterval: options.AgentStatsRefreshInterval,
DeploymentValues: options.DeploymentValues,
Expand Down
35 changes: 19 additions & 16 deletions coderd/workspaceagents.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,18 +161,19 @@ func (api *API) workspaceAgentManifest(rw http.ResponseWriter, r *http.Request)
}

httpapi.Write(ctx, rw, http.StatusOK, agentsdk.Manifest{
Apps: convertApps(dbApps),
DERPMap: api.DERPMap,
GitAuthConfigs: len(api.GitAuthConfigs),
EnvironmentVariables: apiAgent.EnvironmentVariables,
StartupScript: apiAgent.StartupScript,
Directory: apiAgent.Directory,
VSCodePortProxyURI: vscodeProxyURI,
MOTDFile: workspaceAgent.MOTDFile,
StartupScriptTimeout: time.Duration(apiAgent.StartupScriptTimeoutSeconds) * time.Second,
ShutdownScript: apiAgent.ShutdownScript,
ShutdownScriptTimeout: time.Duration(apiAgent.ShutdownScriptTimeoutSeconds) * time.Second,
Metadata: convertWorkspaceAgentMetadataDesc(metadata),
Apps: convertApps(dbApps),
DERPMap: api.DERPMap,
GitAuthConfigs: len(api.GitAuthConfigs),
EnvironmentVariables: apiAgent.EnvironmentVariables,
StartupScript: apiAgent.StartupScript,
Directory: apiAgent.Directory,
VSCodePortProxyURI: vscodeProxyURI,
MOTDFile: workspaceAgent.MOTDFile,
StartupScriptTimeout: time.Duration(apiAgent.StartupScriptTimeoutSeconds) * time.Second,
ShutdownScript: apiAgent.ShutdownScript,
ShutdownScriptTimeout: time.Duration(apiAgent.ShutdownScriptTimeoutSeconds) * time.Second,
AllowDirectConnections: !api.DeploymentValues.DERP.Config.DisableDirect.Value(),
Metadata: convertWorkspaceAgentMetadataDesc(metadata),
})
}

Expand Down Expand Up @@ -663,9 +664,10 @@ func (api *API) workspaceAgentListeningPorts(rw http.ResponseWriter, r *http.Req
func (api *API) dialWorkspaceAgentTailnet(agentID uuid.UUID) (*codersdk.WorkspaceAgentConn, error) {
clientConn, serverConn := net.Pipe()
conn, err := tailnet.NewConn(&tailnet.Options{
Addresses: []netip.Prefix{netip.PrefixFrom(tailnet.IP(), 128)},
DERPMap: api.DERPMap,
Logger: api.Logger.Named("tailnet"),
Addresses: []netip.Prefix{netip.PrefixFrom(tailnet.IP(), 128)},
DERPMap: api.DERPMap,
Logger: api.Logger.Named("tailnet"),
BlockEndpoints: api.DeploymentValues.DERP.Config.DisableDirect.Value(),
})
if err != nil {
_ = clientConn.Close()
Expand Down Expand Up @@ -733,7 +735,8 @@ func (api *API) workspaceAgentConnection(rw http.ResponseWriter, r *http.Request
ctx := r.Context()

httpapi.Write(ctx, rw, http.StatusOK, codersdk.WorkspaceAgentConnectionInfo{
DERPMap: api.DERPMap,
DERPMap: api.DERPMap,
AllowDirectConnections: !api.DeploymentValues.DERP.Config.DisableDirect.Value(),
})
}

Expand Down
77 changes: 77 additions & 0 deletions coderd/workspaceagents_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package coderd_test

import (
"context"
"encoding/json"
"fmt"
"net"
"net/http"
Expand Down Expand Up @@ -444,6 +445,82 @@ func TestWorkspaceAgentTailnet(t *testing.T) {
require.Equal(t, "test", strings.TrimSpace(string(output)))
}

func TestWorkspaceAgentTailnetDirectDisabled(t *testing.T) {
t.Parallel()

dv := coderdtest.DeploymentValues(t)
err := dv.DERP.Config.DisableDirect.Set("true")
require.NoError(t, err)
require.True(t, dv.DERP.Config.DisableDirect.Value())

client, daemonCloser := coderdtest.NewWithProvisionerCloser(t, &coderdtest.Options{
DeploymentValues: dv,
})
user := coderdtest.CreateFirstUser(t, client)
authToken := uuid.NewString()
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{
Parse: echo.ParseComplete,
ProvisionPlan: echo.ProvisionComplete,
ProvisionApply: echo.ProvisionApplyWithAgent(authToken),
})
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID)
daemonCloser.Close()

ctx := testutil.Context(t, testutil.WaitLong)

// Verify that the manifest has AllowDirectConnections set to false.
agentClient := agentsdk.New(client.URL)
agentClient.SetSessionToken(authToken)
manifest, err := agentClient.Manifest(ctx)
require.NoError(t, err)
require.False(t, manifest.AllowDirectConnections)

agentCloser := agent.New(agent.Options{
Client: agentClient,
Logger: slogtest.Make(t, nil).Named("agent").Leveled(slog.LevelDebug),
})
defer agentCloser.Close()
resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID)
agentID := resources[0].Agents[0].ID

// Verify that the connection data has no STUN ports and
// AllowDirectConnections set to false.
res, err := client.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/workspaceagents/%s/connection", agentID), nil)
require.NoError(t, err)
defer res.Body.Close()
require.Equal(t, http.StatusOK, res.StatusCode)
var connInfo codersdk.WorkspaceAgentConnectionInfo
err = json.NewDecoder(res.Body).Decode(&connInfo)
require.NoError(t, err)
require.False(t, connInfo.AllowDirectConnections)
for _, region := range connInfo.DERPMap.Regions {
t.Logf("region %s (%v)", region.RegionCode, region.EmbeddedRelay)
for _, node := range region.Nodes {
t.Logf(" node %s (stun %d)", node.Name, node.STUNPort)
require.EqualValues(t, -1, node.STUNPort)
// tailnet.NewDERPMap() will create nodes with "stun" in the name,
// but not if direct is disabled.
require.NotContains(t, node.Name, "stun")
require.False(t, node.STUNOnly)
}
}

conn, err := client.DialWorkspaceAgent(ctx, resources[0].Agents[0].ID, &codersdk.DialWorkspaceAgentOptions{
Logger: slogtest.Make(t, nil).Named("client").Leveled(slog.LevelDebug),
})
require.NoError(t, err)
defer conn.Close()
require.True(t, conn.BlockEndpoints())

require.True(t, conn.AwaitReachable(ctx))
_, p2p, _, err := conn.Ping(ctx)
require.NoError(t, err)
require.False(t, p2p)
}

func TestWorkspaceAgentListeningPorts(t *testing.T) {
t.Parallel()

Expand Down
Loading