Skip to content

Commit a1db825

Browse files
EmyrkEmyrk
authored
chore: Dynamic CSP connect-src to support terminals connecting to workspace proxies (#7352)
* chore: Expose proxy hostnames to csp header
1 parent 465fe86 commit a1db825

File tree

6 files changed

+220
-117
lines changed

6 files changed

+220
-117
lines changed

‎coderd/coderd.go

+16-2
Original file line numberDiff line numberDiff line change
@@ -793,7 +793,16 @@ func New(options *Options) *API {
793793
r.Get("/swagger/*", globalHTTPSwaggerHandler)
794794
}
795795

796-
r.NotFound(compressHandler(http.HandlerFunc(api.siteHandler.ServeHTTP)).ServeHTTP)
796+
// Add CSP headers to all static assets and pages. CSP headers only affect
797+
// browsers, so these don't make sense on api routes.
798+
cspMW := httpmw.CSPHeaders(func() []string {
799+
if f := api.WorkspaceProxyHostsFn.Load(); f != nil {
800+
return (*f)()
801+
}
802+
// By default we do not add extra websocket connections to the CSP
803+
return []string{}
804+
})
805+
r.NotFound(cspMW(compressHandler(http.HandlerFunc(api.siteHandler.ServeHTTP))).ServeHTTP)
797806
return api
798807
}
799808

@@ -813,7 +822,12 @@ type API struct {
813822
WorkspaceClientCoordinateOverride atomic.Pointer[func(rw http.ResponseWriter) bool]
814823
TailnetCoordinator atomic.Pointer[tailnet.Coordinator]
815824
QuotaCommitter atomic.Pointer[proto.QuotaCommitter]
816-
TemplateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore]
825+
// WorkspaceProxyHostsFn returns the hosts of healthy workspace proxies
826+
// for header reasons.
827+
WorkspaceProxyHostsFn atomic.Pointer[func() []string]
828+
// TemplateScheduleStore is a pointer to an atomic pointer because this is
829+
// passed to another struct, and we want them all to be the same reference.
830+
TemplateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore]
817831

818832
HTTPAuth *HTTPAuthorizer
819833

‎coderd/httpmw/csp.go

+119
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
package httpmw
2+
3+
import (
4+
"fmt"
5+
"net/http"
6+
"strings"
7+
)
8+
9+
// cspDirectives is a map of all csp fetch directives to their values.
10+
// Each directive is a set of values that is joined by a space (' ').
11+
// All directives are semi-colon separated as a single string for the csp header.
12+
type cspDirectives map[CSPFetchDirective][]string
13+
14+
func (s cspDirectives) Append(d CSPFetchDirective, values ...string) {
15+
if _, ok := s[d]; !ok {
16+
s[d] = make([]string, 0)
17+
}
18+
s[d] = append(s[d], values...)
19+
}
20+
21+
// CSPFetchDirective is the list of all constant fetch directives that
22+
// can be used/appended to.
23+
type CSPFetchDirective string
24+
25+
const (
26+
cspDirectiveDefaultSrc = "default-src"
27+
cspDirectiveConnectSrc = "connect-src"
28+
cspDirectiveChildSrc = "child-src"
29+
cspDirectiveScriptSrc = "script-src"
30+
cspDirectiveFontSrc = "font-src"
31+
cspDirectiveStyleSrc = "style-src"
32+
cspDirectiveObjectSrc = "object-src"
33+
cspDirectiveManifestSrc = "manifest-src"
34+
cspDirectiveFrameSrc = "frame-src"
35+
cspDirectiveImgSrc = "img-src"
36+
cspDirectiveReportURI = "report-uri"
37+
cspDirectiveFormAction = "form-action"
38+
cspDirectiveMediaSrc = "media-src"
39+
cspFrameAncestors = "frame-ancestors"
40+
cspDirectiveWorkerSrc = "worker-src"
41+
)
42+
43+
// CSPHeaders returns a middleware that sets the Content-Security-Policy header
44+
// for coderd. It takes a function that allows adding supported external websocket
45+
// hosts. This is primarily to support the terminal connecting to a workspace proxy.
46+
func CSPHeaders(websocketHosts func() []string) func(next http.Handler) http.Handler {
47+
return func(next http.Handler) http.Handler {
48+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
49+
// Content-Security-Policy disables loading certain content types and can prevent XSS injections.
50+
// This site helps eval your policy for syntax and other common issues: https://csp-evaluator.withgoogle.com/
51+
// If we ever want to render something like a PDF, we need to adjust "object-src"
52+
//
53+
// The list of CSP options: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/default-src
54+
cspSrcs := cspDirectives{
55+
// All omitted fetch csp srcs default to this.
56+
cspDirectiveDefaultSrc: {"'self'"},
57+
cspDirectiveConnectSrc: {"'self'"},
58+
cspDirectiveChildSrc: {"'self'"},
59+
// https://github.com/suren-atoyan/monaco-react/issues/168
60+
cspDirectiveScriptSrc: {"'self'"},
61+
cspDirectiveStyleSrc: {"'self' 'unsafe-inline'"},
62+
// data: is used by monaco editor on FE for Syntax Highlight
63+
cspDirectiveFontSrc: {"'self' data:"},
64+
cspDirectiveWorkerSrc: {"'self' blob:"},
65+
// object-src is needed to support code-server
66+
cspDirectiveObjectSrc: {"'self'"},
67+
// blob: for loading the pwa manifest for code-server
68+
cspDirectiveManifestSrc: {"'self' blob:"},
69+
cspDirectiveFrameSrc: {"'self'"},
70+
// data: for loading base64 encoded icons for generic applications.
71+
// https: allows loading images from external sources. This is not ideal
72+
// but is required for the templates page that renders readmes.
73+
// We should find a better solution in the future.
74+
cspDirectiveImgSrc: {"'self' https: data:"},
75+
cspDirectiveFormAction: {"'self'"},
76+
cspDirectiveMediaSrc: {"'self'"},
77+
// Report all violations back to the server to log
78+
cspDirectiveReportURI: {"/api/v2/csp/reports"},
79+
cspFrameAncestors: {"'none'"},
80+
81+
// Only scripts can manipulate the dom. This prevents someone from
82+
// naming themselves something like '<svg onload="alert(/cross-site-scripting/)" />'.
83+
// "require-trusted-types-for" : []string{"'script'"},
84+
}
85+
86+
// This extra connect-src addition is required to support old webkit
87+
// based browsers (Safari).
88+
// See issue: https://github.com/w3c/webappsec-csp/issues/7
89+
// Once webkit browsers support 'self' on connect-src, we can remove this.
90+
// When we remove this, the csp header can be static, as opposed to being
91+
// dynamically generated for each request.
92+
host := r.Host
93+
// It is important r.Host is not an empty string.
94+
if host != "" {
95+
// We can add both ws:// and wss:// as browsers do not let https
96+
// pages to connect to non-tls websocket connections. So this
97+
// supports both http & https webpages.
98+
cspSrcs.Append(cspDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", host))
99+
}
100+
101+
// The terminal requires a websocket connection to the workspace proxy.
102+
// Make sure we allow this connection to healthy proxies.
103+
extraConnect := websocketHosts()
104+
if len(extraConnect) > 0 {
105+
for _, extraHost := range extraConnect {
106+
cspSrcs.Append(cspDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", extraHost))
107+
}
108+
}
109+
110+
var csp strings.Builder
111+
for src, vals := range cspSrcs {
112+
_, _ = fmt.Fprintf(&csp, "%s %s; ", src, strings.Join(vals, " "))
113+
}
114+
115+
w.Header().Set("Content-Security-Policy", csp.String())
116+
next.ServeHTTP(w, r)
117+
})
118+
}
119+
}

‎coderd/httpmw/csp_test.go

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package httpmw_test
2+
3+
import (
4+
"fmt"
5+
"net/http"
6+
"net/http/httptest"
7+
"testing"
8+
9+
"github.com/stretchr/testify/require"
10+
11+
"github.com/coder/coder/coderd/httpmw"
12+
)
13+
14+
func TestCSPConnect(t *testing.T) {
15+
t.Parallel()
16+
17+
expected := []string{"example.com", "coder.com"}
18+
19+
r := httptest.NewRequest(http.MethodGet, "/", nil)
20+
rw := httptest.NewRecorder()
21+
22+
httpmw.CSPHeaders(func() []string {
23+
return expected
24+
})(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
25+
rw.WriteHeader(http.StatusOK)
26+
})).ServeHTTP(rw, r)
27+
28+
require.NotEmpty(t, rw.Header().Get("Content-Security-Policy"), "Content-Security-Policy header should not be empty")
29+
for _, e := range expected {
30+
require.Containsf(t, rw.Header().Get("Content-Security-Policy"), fmt.Sprintf("ws://%s", e), "Content-Security-Policy header should contain ws://%s", e)
31+
require.Containsf(t, rw.Header().Get("Content-Security-Policy"), fmt.Sprintf("wss://%s", e), "Content-Security-Policy header should contain wss://%s", e)
32+
}
33+
}

‎enterprise/coderd/coderd.go

+4
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,10 @@ func New(ctx context.Context, options *Options) (*API, error) {
250250
// Force the initial loading of the cache. Do this in a go routine in case
251251
// the calls to the workspace proxies hang and this takes some time.
252252
go api.forceWorkspaceProxyHealthUpdate(ctx)
253+
254+
// Use proxy health to return the healthy workspace proxy hostnames.
255+
f := api.ProxyHealth.ProxyHosts
256+
api.AGPL.WorkspaceProxyHostsFn.Store(&f)
253257
}
254258

255259
err = api.updateEntitlements(ctx)

‎enterprise/coderd/proxyhealth/proxyhealth.go

+47-6
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"encoding/json"
66
"fmt"
77
"net/http"
8+
"net/url"
89
"strings"
910
"sync"
1011
"sync/atomic"
@@ -59,7 +60,9 @@ type ProxyHealth struct {
5960
logger slog.Logger
6061
client *http.Client
6162

62-
cache *atomic.Pointer[map[uuid.UUID]ProxyStatus]
63+
// Cached values for quick access to the health of proxies.
64+
cache *atomic.Pointer[map[uuid.UUID]ProxyStatus]
65+
proxyHosts *atomic.Pointer[[]string]
6366

6467
// PromMetrics
6568
healthCheckDuration prometheus.Histogram
@@ -112,6 +115,7 @@ func New(opts *Options) (*ProxyHealth, error) {
112115
logger: opts.Logger,
113116
client: client,
114117
cache: &atomic.Pointer[map[uuid.UUID]ProxyStatus]{},
118+
proxyHosts: &atomic.Pointer[[]string]{},
115119
healthCheckDuration: healthCheckDuration,
116120
healthCheckResults: healthCheckResults,
117121
}, nil
@@ -133,12 +137,24 @@ func (p *ProxyHealth) Run(ctx context.Context) {
133137
p.logger.Error(ctx, "proxy health check failed", slog.Error(err))
134138
continue
135139
}
136-
// Store the statuses in the cache.
137-
p.cache.Store(&statuses)
140+
p.storeProxyHealth(statuses)
138141
}
139142
}
140143
}
141144

145+
func (p *ProxyHealth) storeProxyHealth(statuses map[uuid.UUID]ProxyStatus) {
146+
var proxyHosts []string
147+
for _, s := range statuses {
148+
if s.ProxyHost != "" {
149+
proxyHosts = append(proxyHosts, s.ProxyHost)
150+
}
151+
}
152+
153+
// Store the statuses in the cache before any other quick values.
154+
p.cache.Store(&statuses)
155+
p.proxyHosts.Store(&proxyHosts)
156+
}
157+
142158
// ForceUpdate runs a single health check and updates the cache. If the health
143159
// check fails, the cache is not updated and an error is returned. This is useful
144160
// to trigger an update when a proxy is created or deleted.
@@ -148,8 +164,7 @@ func (p *ProxyHealth) ForceUpdate(ctx context.Context) error {
148164
return err
149165
}
150166

151-
// Store the statuses in the cache.
152-
p.cache.Store(&statuses)
167+
p.storeProxyHealth(statuses)
153168
return nil
154169
}
155170

@@ -168,12 +183,28 @@ type ProxyStatus struct {
168183
// useful to know as it helps determine if the proxy checked has different values
169184
// then the proxy in hand. AKA if the proxy was updated, and the status was for
170185
// an older proxy.
171-
Proxy database.WorkspaceProxy
186+
Proxy database.WorkspaceProxy
187+
// ProxyHost is the host:port of the proxy url. This is included in the status
188+
// to make sure the proxy url is a valid URL. It also makes it easier to
189+
// escalate errors if the url.Parse errors (should never happen).
190+
ProxyHost string
172191
Status Status
173192
Report codersdk.ProxyHealthReport
174193
CheckedAt time.Time
175194
}
176195

196+
// ProxyHosts returns the host:port of all healthy proxies.
197+
// This can be computed from HealthStatus, but is cached to avoid the
198+
// caller needing to loop over all proxies to compute this on all
199+
// static web requests.
200+
func (p *ProxyHealth) ProxyHosts() []string {
201+
ptr := p.proxyHosts.Load()
202+
if ptr == nil {
203+
return []string{}
204+
}
205+
return *ptr
206+
}
207+
177208
// runOnce runs the health check for all workspace proxies. If there is an
178209
// unexpected error, an error is returned. Expected errors will mark a proxy as
179210
// unreachable.
@@ -248,6 +279,7 @@ func (p *ProxyHealth) runOnce(ctx context.Context, now time.Time) (map[uuid.UUID
248279
status.Status = Unhealthy
249280
break
250281
}
282+
251283
status.Status = Healthy
252284
case err == nil && resp.StatusCode != http.StatusOK:
253285
// Unhealthy as we did reach the proxy but it got an unexpected response.
@@ -262,6 +294,15 @@ func (p *ProxyHealth) runOnce(ctx context.Context, now time.Time) (map[uuid.UUID
262294
status.Status = Unknown
263295
}
264296

297+
u, err := url.Parse(proxy.Url)
298+
if err != nil {
299+
// This should never happen. This would mean the proxy sent
300+
// us an invalid url?
301+
status.Report.Errors = append(status.Report.Errors, fmt.Sprintf("failed to parse proxy url: %s", err.Error()))
302+
status.Status = Unhealthy
303+
}
304+
status.ProxyHost = u.Host
305+
265306
// Set the prometheus metric correctly.
266307
switch status.Status {
267308
case Healthy:

0 commit comments

Comments
 (0)