Skip to content

Commit 921a7c5

Browse files
deansheatherEmyrk
deansheather
and
Emyrk
committed
chore: move app proxying code to workspaceapps pkg
Moves path-app, subdomain-app and reconnecting PTY proxying to the new workspaceapps.WorkspaceAppServer struct. This is in preparation for external workspace proxies. Updates app logout flow to avoid redirecting to coder-logout.${app_host} on logout. Instead, all subdomain app tokens owned by the logging-out user will be deleted every time you logout for simplicity sake. Tests will remain in their original package, pending being moved to an apptest package (or similar). Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
1 parent a96376e commit 921a7c5

23 files changed

+1108
-1268
lines changed

‎cli/server.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ import (
7676
"github.com/coder/coder/coderd/tracing"
7777
"github.com/coder/coder/coderd/updatecheck"
7878
"github.com/coder/coder/coderd/util/slice"
79+
"github.com/coder/coder/coderd/workspaceapps"
7980
"github.com/coder/coder/codersdk"
8081
"github.com/coder/coder/cryptorand"
8182
"github.com/coder/coder/provisioner/echo"
@@ -799,12 +800,9 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
799800
}
800801
}
801802

802-
appSigningKey, err := hex.DecodeString(appSigningKeyStr)
803+
appSigningKey, err := workspaceapps.KeyFromString(appSigningKeyStr)
803804
if err != nil {
804-
return xerrors.Errorf("decode app signing key from database as hex: %w", err)
805-
}
806-
if len(appSigningKey) != 64 {
807-
return xerrors.Errorf("app signing key must be 64 bytes, key in database is %d bytes", len(appSigningKey))
805+
return xerrors.Errorf("decode app signing key from database: %w", err)
808806
}
809807

810808
options.AppSigningKey = appSigningKey

‎cli/server_test.go

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -668,8 +668,7 @@ func TestServer(t *testing.T) {
668668
if c.tlsListener {
669669
accessURLParsed, err := url.Parse(c.requestURL)
670670
require.NoError(t, err)
671-
client := codersdk.New(accessURLParsed)
672-
client.HTTPClient = &http.Client{
671+
client := &http.Client{
673672
CheckRedirect: func(req *http.Request, via []*http.Request) error {
674673
return http.ErrUseLastResponse
675674
},
@@ -682,11 +681,15 @@ func TestServer(t *testing.T) {
682681
},
683682
},
684683
}
685-
defer client.HTTPClient.CloseIdleConnections()
686-
_, err = client.HasFirstUser(ctx)
687-
if err != nil {
688-
require.ErrorContains(t, err, "Invalid application URL")
689-
}
684+
defer client.CloseIdleConnections()
685+
686+
req, err := http.NewRequestWithContext(ctx, http.MethodGet, accessURLParsed.String(), nil)
687+
require.NoError(t, err)
688+
resp, err := client.Do(req)
689+
// We don't care much about the response, just that TLS
690+
// worked.
691+
require.NoError(t, err)
692+
defer resp.Body.Close()
690693
}
691694
})
692695
}

‎coderd/coderd.go

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,8 @@ type Options struct {
124124
SetUserGroups func(ctx context.Context, tx database.Store, userID uuid.UUID, groupNames []string) error
125125
TemplateScheduleStore schedule.TemplateScheduleStore
126126
// AppSigningKey denotes the symmetric key to use for signing temporary app
127-
// tokens. The key must be 64 bytes long.
128-
AppSigningKey []byte
127+
// tokens.
128+
AppSigningKey workspaceapps.AppSigningKey
129129
HealthcheckFunc func(ctx context.Context) (*healthcheck.Report, error)
130130
HealthcheckTimeout time.Duration
131131
HealthcheckRefresh time.Duration
@@ -237,9 +237,6 @@ func New(options *Options) *API {
237237
if options.TemplateScheduleStore == nil {
238238
options.TemplateScheduleStore = schedule.NewAGPLTemplateScheduleStore()
239239
}
240-
if len(options.AppSigningKey) != 64 {
241-
panic("coderd: AppSigningKey must be 64 bytes long")
242-
}
243240
if options.HealthcheckFunc == nil {
244241
options.HealthcheckFunc = func(ctx context.Context) (*healthcheck.Report, error) {
245242
return healthcheck.Run(ctx, &healthcheck.ReportOptions{
@@ -331,6 +328,21 @@ func New(options *Options) *API {
331328
api.workspaceAgentCache = wsconncache.New(api.dialWorkspaceAgentTailnet, 0)
332329
api.TailnetCoordinator.Store(&options.TailnetCoordinator)
333330

331+
workspaceAppServer := workspaceapps.WorkspaceAppServer{
332+
Logger: options.Logger.Named("workspaceapps"),
333+
334+
PrimaryAccessURL: api.AccessURL,
335+
AccessURL: api.AccessURL,
336+
AppHostname: api.AppHostname,
337+
AppHostnameRegex: api.AppHostnameRegex,
338+
DeploymentValues: options.DeploymentValues,
339+
RealIPConfig: options.RealIPConfig,
340+
341+
TokenProvider: api.WorkspaceAppsProvider,
342+
WorkspaceConnCache: api.workspaceAgentCache,
343+
AppSigningKey: options.AppSigningKey,
344+
}
345+
334346
apiKeyMiddleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
335347
DB: options.Database,
336348
OAuth2Configs: oauthConfigs,
@@ -363,11 +375,12 @@ func New(options *Options) *API {
363375
httpmw.ExtractRealIP(api.RealIPConfig),
364376
httpmw.Logger(api.Logger),
365377
httpmw.Prometheus(options.PrometheusRegistry),
366-
// handleSubdomainApplications checks if the first subdomain is a valid
367-
// app URL. If it is, it will serve that application.
378+
// SubdomainAppMW checks if the first subdomain is a valid app URL. If
379+
// it is, it will serve that application.
368380
//
369-
// Workspace apps do their own auth.
370-
api.handleSubdomainApplications(apiRateLimiter),
381+
// Workspace apps do their own auth and must be BEFORE the auth
382+
// middleware.
383+
workspaceAppServer.SubdomainAppMW(apiRateLimiter),
371384
// Build-Version is helpful for debugging.
372385
func(next http.Handler) http.Handler {
373386
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -390,16 +403,9 @@ func New(options *Options) *API {
390403

391404
r.Get("/healthz", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("OK")) })
392405

393-
apps := func(r chi.Router) {
394-
// Workspace apps do their own auth.
395-
r.Use(apiRateLimiter)
396-
r.HandleFunc("/*", api.workspaceAppsProxyPath)
397-
}
398-
// %40 is the encoded character of the @ symbol. VS Code Web does
399-
// not handle character encoding properly, so it's safe to assume
400-
// other applications might not as well.
401-
r.Route("/%40{user}/{workspace_and_agent}/apps/{workspaceapp}", apps)
402-
r.Route("/@{user}/{workspace_and_agent}/apps/{workspaceapp}", apps)
406+
// Attach workspace apps routes.
407+
workspaceAppServer.Attach(r, apiRateLimiter)
408+
403409
r.Route("/derp", func(r chi.Router) {
404410
r.Get("/", derpHandler.ServeHTTP)
405411
// This is used when UDP is blocked, and latency must be checked via HTTP(s).
@@ -641,9 +647,6 @@ func New(options *Options) *API {
641647
r.Post("/report-lifecycle", api.workspaceAgentReportLifecycle)
642648
r.Post("/metadata/{key}", api.workspaceAgentPostMetadata)
643649
})
644-
// No middleware on the PTY endpoint since it uses workspace
645-
// application auth and signed app tokens.
646-
r.Get("/{workspaceagent}/pty", api.workspaceAgentPTY)
647650
r.Route("/{workspaceagent}", func(r chi.Router) {
648651
r.Use(
649652
apiKeyMiddleware,
@@ -652,11 +655,12 @@ func New(options *Options) *API {
652655
)
653656
r.Get("/", api.workspaceAgent)
654657
r.Get("/watch-metadata", api.watchWorkspaceAgentMetadata)
655-
r.Get("/pty", api.workspaceAgentPTY)
656658
r.Get("/startup-logs", api.workspaceAgentStartupLogs)
657659
r.Get("/listening-ports", api.workspaceAgentListeningPorts)
658660
r.Get("/connection", api.workspaceAgentConnection)
659661
r.Get("/coordinate", api.workspaceAgentClientCoordinate)
662+
663+
// PTY is part of workspaceAppServer.
660664
})
661665
})
662666
r.Route("/workspaces", func(r chi.Router) {

‎coderd/coderdtest/coderdtest.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"crypto/x509"
1212
"crypto/x509/pkix"
1313
"encoding/base64"
14-
"encoding/hex"
1514
"encoding/json"
1615
"encoding/pem"
1716
"errors"
@@ -68,6 +67,7 @@ import (
6867
"github.com/coder/coder/coderd/telemetry"
6968
"github.com/coder/coder/coderd/updatecheck"
7069
"github.com/coder/coder/coderd/util/ptr"
70+
"github.com/coder/coder/coderd/workspaceapps"
7171
"github.com/coder/coder/codersdk"
7272
"github.com/coder/coder/codersdk/agentsdk"
7373
"github.com/coder/coder/cryptorand"
@@ -82,7 +82,7 @@ import (
8282

8383
// AppSigningKey is a 64-byte key used to sign JWTs for workspace app tokens in
8484
// tests.
85-
var AppSigningKey = must(hex.DecodeString("64656164626565666465616462656566646561646265656664656164626565666465616462656566646561646265656664656164626565666465616462656566"))
85+
var AppSigningKey = must(workspaceapps.KeyFromString("64656164626565666465616462656566646561646265656664656164626565666465616462656566646561646265656664656164626565666465616462656566"))
8686

8787
type Options struct {
8888
// AccessURL denotes a custom access URL. By default we use the httptest

‎coderd/database/dbauthz/querier.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -994,6 +994,16 @@ func (q *querier) GetTemplateUserRoles(ctx context.Context, id uuid.UUID) ([]dat
994994
return q.db.GetTemplateUserRoles(ctx, id)
995995
}
996996

997+
func (q *querier) DeleteApplicationConnectAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error {
998+
// TODO: This is not 100% correct because it omits apikey IDs.
999+
err := q.authorizeContext(ctx, rbac.ActionDelete,
1000+
rbac.ResourceAPIKey.WithOwner(userID.String()))
1001+
if err != nil {
1002+
return err
1003+
}
1004+
return q.db.DeleteApplicationConnectAPIKeysByUserID(ctx, userID)
1005+
}
1006+
9971007
func (q *querier) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error {
9981008
// TODO: This is not 100% correct because it omits apikey IDs.
9991009
err := q.authorizeContext(ctx, rbac.ActionDelete,

‎coderd/database/dbfake/databasefake.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,19 @@ func (q *fakeQuerier) DeleteAPIKeyByID(_ context.Context, id string) error {
677677
return sql.ErrNoRows
678678
}
679679

680+
func (q *fakeQuerier) DeleteApplicationConnectAPIKeysByUserID(_ context.Context, userID uuid.UUID) error {
681+
q.mutex.Lock()
682+
defer q.mutex.Unlock()
683+
684+
for i := len(q.apiKeys) - 1; i >= 0; i-- {
685+
if q.apiKeys[i].UserID == userID && q.apiKeys[i].Scope == database.APIKeyScopeApplicationConnect {
686+
q.apiKeys = append(q.apiKeys[:i], q.apiKeys[i+1:]...)
687+
}
688+
}
689+
690+
return nil
691+
}
692+
680693
func (q *fakeQuerier) DeleteAPIKeysByUserID(_ context.Context, userID uuid.UUID) error {
681694
q.mutex.Lock()
682695
defer q.mutex.Unlock()

‎coderd/database/querier.go

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 14 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/apikeys.sql

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,18 @@ WHERE
6666
id = $1;
6767

6868
-- name: DeleteAPIKeyByID :exec
69-
DELETE
70-
FROM
69+
DELETE FROM
7170
api_keys
7271
WHERE
7372
id = $1;
7473

74+
-- name: DeleteApplicationConnectAPIKeysByUserID :exec
75+
DELETE FROM
76+
api_keys
77+
WHERE
78+
user_id = $1 AND
79+
scope = 'application_connect'::api_key_scope;
80+
7581
-- name: DeleteAPIKeysByUserID :exec
7682
DELETE FROM
7783
api_keys

‎coderd/userauth.go

Lines changed: 11 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -194,48 +194,17 @@ func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) {
194194
return
195195
}
196196

197-
// Deployments should not host app tokens on the same domain as the
198-
// primary deployment. But in the case they are, we should also delete this
199-
// token.
200-
if appCookie, _ := r.Cookie(codersdk.DevURLSessionTokenCookie); appCookie != nil {
201-
appCookieRemove := &http.Cookie{
202-
// MaxAge < 0 means to delete the cookie now.
203-
MaxAge: -1,
204-
Name: codersdk.DevURLSessionTokenCookie,
205-
Path: "/",
206-
Domain: "." + api.AccessURL.Hostname(),
207-
}
208-
http.SetCookie(rw, appCookieRemove)
209-
210-
id, _, err := httpmw.SplitAPIToken(appCookie.Value)
211-
if err == nil {
212-
err = api.Database.DeleteAPIKeyByID(ctx, id)
213-
if err != nil {
214-
// Don't block logout, just log any errors.
215-
api.Logger.Warn(r.Context(), "failed to delete devurl token on logout",
216-
slog.Error(err),
217-
slog.F("id", id),
218-
)
219-
}
220-
}
221-
}
222-
223-
// This code should be removed after Jan 1 2023.
224-
// This code logs out of the old session cookie before we renamed it
225-
// if it is a valid coder token. Otherwise, this old cookie hangs around
226-
// and we never log out of the user.
227-
oldCookie, err := r.Cookie("session_token")
228-
if err == nil && oldCookie != nil {
229-
_, _, err := httpmw.SplitAPIToken(oldCookie.Value)
230-
if err == nil {
231-
cookie := &http.Cookie{
232-
// MaxAge < 0 means to delete the cookie now.
233-
MaxAge: -1,
234-
Name: "session_token",
235-
Path: "/",
236-
}
237-
http.SetCookie(rw, cookie)
238-
}
197+
// Invalidate all subdomain app tokens. This saves us from having to
198+
// track which app tokens are associated which this browser session and
199+
// doesn't inconvenience the user as they'll just get redirected if they try
200+
// to access the app again.
201+
err = api.Database.DeleteApplicationConnectAPIKeysByUserID(ctx, apiKey.UserID)
202+
if err != nil {
203+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
204+
Message: "Internal error deleting app tokens.",
205+
Detail: err.Error(),
206+
})
207+
return
239208
}
240209

241210
aReq.New = database.APIKey{}

0 commit comments

Comments
 (0)