From 6d7aef100f7aa065984999b6a312f3e9f56dc3ef Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Thu, 16 Mar 2023 04:49:47 +0000 Subject: [PATCH 1/4] feat: use app tickets for web terminal --- coderd/workspaceagents.go | 52 +++--- coderd/workspaceapps/auth.go | 221 +++------------------- coderd/workspaceapps/auth_test.go | 67 ++++--- coderd/workspaceapps/request.go | 264 ++++++++++++++++++++++++++- coderd/workspaceapps/request_test.go | 70 +++++++ coderd/workspaceapps/ticket.go | 7 +- coderd/workspaceapps/ticket_test.go | 140 +++++++++----- 7 files changed, 524 insertions(+), 297 deletions(-) diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 098f83b5358ec..03c02abb23887 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -18,6 +18,7 @@ import ( "sync" "time" + "github.com/go-chi/chi/v5" "github.com/google/uuid" "go.opentelemetry.io/otel/trace" "golang.org/x/mod/semver" @@ -35,6 +36,7 @@ import ( "github.com/coder/coder/coderd/httpmw" "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/coderd/tracing" + "github.com/coder/coder/coderd/workspaceapps" "github.com/coder/coder/codersdk" "github.com/coder/coder/codersdk/agentsdk" "github.com/coder/coder/tailnet" @@ -240,30 +242,36 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) { api.WebsocketWaitMutex.Unlock() defer api.WebsocketWaitGroup.Done() - workspaceAgent := httpmw.WorkspaceAgentParam(r) - workspace := httpmw.WorkspaceParam(r) - if !api.Authorize(r, rbac.ActionCreate, workspace.ExecutionRBAC()) { - httpapi.ResourceNotFound(rw) + ticket, ok := api.WorkspaceAppsProvider.ResolveRequest(rw, r, workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodTerminal, + BasePath: r.URL.Path, + AgentNameOrID: chi.URLParam(r, "workspaceagent"), + }) + if !ok { return } - apiAgent, err := convertWorkspaceAgent( - api.DERPMap, *api.TailnetCoordinator.Load(), workspaceAgent, nil, api.AgentInactiveDisconnectTimeout, - api.DeploymentValues.AgentFallbackTroubleshootingURL.String(), - ) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error reading workspace agent.", - Detail: err.Error(), - }) - return - } - if apiAgent.Status != codersdk.WorkspaceAgentConnected { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("Agent state is %q, it must be in the %q state.", apiAgent.Status, codersdk.WorkspaceAgentConnected), - }) - return - } + // TODO(@deansheather): bring back agent state check in the upstream ticket + // code + /* + apiAgent, err := convertWorkspaceAgent( + api.DERPMap, *api.TailnetCoordinator.Load(), workspaceAgent, nil, api.AgentInactiveDisconnectTimeout, + api.DeploymentValues.AgentFallbackTroubleshootingURL.String(), + ) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error reading workspace agent.", + Detail: err.Error(), + }) + return + } + if apiAgent.Status != codersdk.WorkspaceAgentConnected { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Agent state is %q, it must be in the %q state.", apiAgent.Status, codersdk.WorkspaceAgentConnected), + }) + return + } + */ reconnect, err := uuid.Parse(r.URL.Query().Get("reconnect")) if err != nil { @@ -300,7 +308,7 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) { go httpapi.Heartbeat(ctx, conn) - agentConn, release, err := api.workspaceAgentCache.Acquire(r, workspaceAgent.ID) + agentConn, release, err := api.workspaceAgentCache.Acquire(r, ticket.AgentID) if err != nil { _ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("dial workspace agent: %s", err)) return diff --git a/coderd/workspaceapps/auth.go b/coderd/workspaceapps/auth.go index 6c0b66d011b00..47af6845b722b 100644 --- a/coderd/workspaceapps/auth.go +++ b/coderd/workspaceapps/auth.go @@ -3,13 +3,10 @@ package workspaceapps import ( "context" "database/sql" - "fmt" "net/http" - "strconv" "strings" "time" - "github.com/google/uuid" "golang.org/x/xerrors" "cdr.dev/slog" @@ -73,7 +70,8 @@ func (p *Provider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appRe if err == nil { ticket, err := p.ParseTicket(ticketCookie.Value) if err == nil { - if ticket.MatchesRequest(appReq) { + err := ticket.Request.Validate() + if err == nil && ticket.MatchesRequest(appReq) { // The request has a ticket, which is a valid ticket signed by // us, and matches the app that the user was trying to access. return &ticket, true @@ -85,11 +83,7 @@ func (p *Provider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appRe // session token, validate auth and access to the app, then generate a new // ticket. ticket := Ticket{ - AccessMethod: appReq.AccessMethod, - UsernameOrID: appReq.UsernameOrID, - WorkspaceNameOrID: appReq.WorkspaceNameOrID, - AgentNameOrID: appReq.AgentNameOrID, - AppSlugOrPort: appReq.AppSlugOrPort, + Request: appReq, } // We use the regular API apiKey extraction middleware fn here to avoid any @@ -109,167 +103,26 @@ func (p *Provider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appRe return nil, false } - // Get user. - var ( - user database.User - userErr error - ) - if userID, uuidErr := uuid.Parse(appReq.UsernameOrID); uuidErr == nil { - user, userErr = p.Database.GetUserByID(dangerousSystemCtx, userID) - } else { - user, userErr = p.Database.GetUserByEmailOrUsername(dangerousSystemCtx, database.GetUserByEmailOrUsernameParams{ - Username: appReq.UsernameOrID, - }) - } - if xerrors.Is(userErr, sql.ErrNoRows) { - p.writeWorkspaceApp404(rw, r, &appReq, fmt.Sprintf("user %q not found", appReq.UsernameOrID)) - return nil, false - } else if userErr != nil { - p.writeWorkspaceApp500(rw, r, &appReq, userErr, "get user") - return nil, false - } - ticket.UserID = user.ID - - // Get workspace. - var ( - workspace database.Workspace - workspaceErr error - ) - if workspaceID, uuidErr := uuid.Parse(appReq.WorkspaceNameOrID); uuidErr == nil { - workspace, workspaceErr = p.Database.GetWorkspaceByID(dangerousSystemCtx, workspaceID) - } else { - workspace, workspaceErr = p.Database.GetWorkspaceByOwnerIDAndName(dangerousSystemCtx, database.GetWorkspaceByOwnerIDAndNameParams{ - OwnerID: user.ID, - Name: appReq.WorkspaceNameOrID, - Deleted: false, - }) - } - if xerrors.Is(workspaceErr, sql.ErrNoRows) { - p.writeWorkspaceApp404(rw, r, &appReq, fmt.Sprintf("workspace %q not found", appReq.WorkspaceNameOrID)) - return nil, false - } else if workspaceErr != nil { - p.writeWorkspaceApp500(rw, r, &appReq, workspaceErr, "get workspace") - return nil, false - } - ticket.WorkspaceID = workspace.ID - - // Get agent. - var ( - agent database.WorkspaceAgent - agentErr error - trustAgent = false - ) - if agentID, uuidErr := uuid.Parse(appReq.AgentNameOrID); uuidErr == nil { - agent, agentErr = p.Database.GetWorkspaceAgentByID(dangerousSystemCtx, agentID) - } else { - build, err := p.Database.GetLatestWorkspaceBuildByWorkspaceID(dangerousSystemCtx, workspace.ID) - if err != nil { - p.writeWorkspaceApp500(rw, r, &appReq, err, "get latest workspace build") - return nil, false - } - - // nolint:gocritic // We need to fetch the agent to authenticate the request. This is a system function. - resources, err := p.Database.GetWorkspaceResourcesByJobID(dangerousSystemCtx, build.JobID) - if err != nil { - p.writeWorkspaceApp500(rw, r, &appReq, err, "get workspace resources") - return nil, false - } - resourcesIDs := []uuid.UUID{} - for _, resource := range resources { - resourcesIDs = append(resourcesIDs, resource.ID) - } - - // nolint:gocritic // We need to fetch the agent to authenticate the request. This is a system function. - agents, err := p.Database.GetWorkspaceAgentsByResourceIDs(dangerousSystemCtx, resourcesIDs) - if err != nil { - p.writeWorkspaceApp500(rw, r, &appReq, err, "get workspace agents") - return nil, false - } - - if appReq.AgentNameOrID == "" { - if len(agents) != 1 { - p.writeWorkspaceApp404(rw, r, &appReq, "no agent specified, but multiple exist in workspace") - return nil, false - } - - agent = agents[0] - trustAgent = true - } else { - for _, a := range agents { - if a.Name == appReq.AgentNameOrID { - agent = a - trustAgent = true - break - } - } - } - - if agent.ID == uuid.Nil { - agentErr = sql.ErrNoRows - } - } - if xerrors.Is(agentErr, sql.ErrNoRows) { - p.writeWorkspaceApp404(rw, r, &appReq, fmt.Sprintf("agent %q not found", appReq.AgentNameOrID)) + // Lookup workspace app details from DB. + dbReq, err := appReq.getDatabase(dangerousSystemCtx, p.Database) + if xerrors.Is(err, sql.ErrNoRows) { + p.writeWorkspaceApp404(rw, r, &appReq, err.Error()) return nil, false - } else if agentErr != nil { - p.writeWorkspaceApp500(rw, r, &appReq, agentErr, "get agent") + } else if err != nil { + p.writeWorkspaceApp500(rw, r, &appReq, err, "get app details from database") return nil, false } + ticket.UserID = dbReq.User.ID + ticket.WorkspaceID = dbReq.Workspace.ID + ticket.AgentID = dbReq.Agent.ID + ticket.AppURL = dbReq.AppURL - // Verify the agent belongs to the workspace. - if !trustAgent { - //nolint:gocritic // We need to fetch the agent to authenticate the request. This is a system function. - agentResource, err := p.Database.GetWorkspaceResourceByID(dangerousSystemCtx, agent.ResourceID) - if err != nil { - p.writeWorkspaceApp500(rw, r, &appReq, err, "get agent resource") - return nil, false - } - build, err := p.Database.GetWorkspaceBuildByJobID(dangerousSystemCtx, agentResource.JobID) - if err != nil { - p.writeWorkspaceApp500(rw, r, &appReq, err, "get agent workspace build") - return nil, false - } - if build.WorkspaceID != workspace.ID { - p.writeWorkspaceApp404(rw, r, &appReq, "agent does not belong to workspace") - return nil, false - } - } - ticket.AgentID = agent.ID - - // Get app. - appSharingLevel := database.AppSharingLevelOwner - portUint, portUintErr := strconv.ParseUint(appReq.AppSlugOrPort, 10, 16) - if appReq.AccessMethod == AccessMethodSubdomain && portUintErr == nil { - // If the app slug is a port number, then route to the port as an - // "anonymous app". We only support HTTP for port-based URLs. - // - // This is only supported for subdomain-based applications. - ticket.AppURL = fmt.Sprintf("http://127.0.0.1:%d", portUint) - } else { - app, ok := p.lookupWorkspaceApp(rw, r, agent.ID, appReq.AppSlugOrPort) - if !ok { - return nil, false - } - - if !app.Url.Valid { - site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ - Status: http.StatusBadRequest, - Title: "Bad Request", - Description: fmt.Sprintf("Application %q does not have a URL set.", app.Slug), - RetryEnabled: true, - DashboardURL: p.AccessURL.String(), - }) - return nil, false - } - - if app.SharingLevel != "" { - appSharingLevel = app.SharingLevel - } - ticket.AppURL = app.Url.String - } + // TODO(@deansheather): return an error if the agent is offline or the app + // is not running. // Verify the user has access to the app. - authed, ok := p.fetchWorkspaceApplicationAuth(rw, r, authz, appReq.AccessMethod, workspace, appSharingLevel) + // TODO(@deansheather): this should be a different auth check for terminal reqs + authed, ok := p.fetchWorkspaceApplicationAuth(rw, r, authz, appReq.AccessMethod, dbReq.Workspace, dbReq.AppSharingLevel) if !ok { return nil, false } @@ -282,7 +135,12 @@ func (p *Provider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appRe // Redirect to login as they don't have permission to access the app // and they aren't signed in. - if appReq.AccessMethod == AccessMethodSubdomain { + switch appReq.AccessMethod { + case AccessMethodPath: + httpmw.RedirectToLogin(rw, r, httpmw.SignedOutErrorMessage) + case AccessMethodSubdomain: + // Redirect to the app auth redirect endpoint with a valid redirect + // URI. redirectURI := *r.URL redirectURI.Scheme = p.AccessURL.Scheme redirectURI.Host = httpapi.RequestHost(r) @@ -294,8 +152,9 @@ func (p *Provider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appRe u.RawQuery = q.Encode() http.Redirect(rw, r, u.String(), http.StatusTemporaryRedirect) - } else { - httpmw.RedirectToLogin(rw, r, httpmw.SignedOutErrorMessage) + case AccessMethodTerminal: + // Return an error. + httpapi.ResourceNotFound(rw) } return nil, false } @@ -329,34 +188,6 @@ func (p *Provider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appRe return &ticket, true } -// lookupWorkspaceApp looks up the workspace application by slug in the given -// agent and returns it. If the application is not found or there was a server -// error while looking it up, an HTML error page is returned and false is -// returned so the caller can return early. -func (p *Provider) lookupWorkspaceApp(rw http.ResponseWriter, r *http.Request, agentID uuid.UUID, appSlug string) (database.WorkspaceApp, bool) { - // nolint:gocritic // We need to fetch the workspace app to authorize the request. - app, err := p.Database.GetWorkspaceAppByAgentIDAndSlug(dbauthz.AsSystemRestricted(r.Context()), database.GetWorkspaceAppByAgentIDAndSlugParams{ - AgentID: agentID, - Slug: appSlug, - }) - if xerrors.Is(err, sql.ErrNoRows) { - p.writeWorkspaceApp404(rw, r, nil, "application not found in agent by slug") - return database.WorkspaceApp{}, false - } - if err != nil { - site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ - Status: http.StatusInternalServerError, - Title: "Internal Server Error", - Description: "Could not fetch workspace application: " + err.Error(), - RetryEnabled: true, - DashboardURL: p.AccessURL.String(), - }) - return database.WorkspaceApp{}, false - } - - return app, true -} - func (p *Provider) authorizeWorkspaceApp(ctx context.Context, roles *httpmw.Authorization, accessMethod AccessMethod, sharingLevel database.AppSharingLevel, workspace database.Workspace) (bool, error) { if accessMethod == "" { accessMethod = AccessMethodPath diff --git a/coderd/workspaceapps/auth_test.go b/coderd/workspaceapps/auth_test.go index d4815f72d80d6..a69ae82e71353 100644 --- a/coderd/workspaceapps/auth_test.go +++ b/coderd/workspaceapps/auth_test.go @@ -205,16 +205,12 @@ func Test_ResolveRequest(t *testing.T) { _ = w.Body.Close() require.Equal(t, &workspaceapps.Ticket{ - AccessMethod: req.AccessMethod, - UsernameOrID: req.UsernameOrID, - WorkspaceNameOrID: req.WorkspaceNameOrID, - AgentNameOrID: req.AgentNameOrID, - AppSlugOrPort: req.AppSlugOrPort, - Expiry: ticket.Expiry, // ignored to avoid flakiness - UserID: me.ID, - WorkspaceID: workspace.ID, - AgentID: agentID, - AppURL: appURL, + Request: req, + Expiry: ticket.Expiry, // ignored to avoid flakiness + UserID: me.ID, + WorkspaceID: workspace.ID, + AgentID: agentID, + AppURL: appURL, }, ticket) require.NotZero(t, ticket.Expiry) require.InDelta(t, time.Now().Add(workspaceapps.TicketExpiry).Unix(), ticket.Expiry, time.Minute.Seconds()) @@ -423,17 +419,20 @@ func Test_ResolveRequest(t *testing.T) { t.Parallel() badTicket := workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodPath, - UsernameOrID: me.Username, - WorkspaceNameOrID: workspace.Name, - AgentNameOrID: agentName, - // App name differs - AppSlugOrPort: appNamePublic, - Expiry: time.Now().Add(time.Minute).Unix(), - UserID: me.ID, - WorkspaceID: workspace.ID, - AgentID: agentID, - AppURL: appURL, + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: me.Username, + WorkspaceNameOrID: workspace.Name, + AgentNameOrID: agentName, + // App name differs + AppSlugOrPort: appNamePublic, + }, + Expiry: time.Now().Add(time.Minute).Unix(), + UserID: me.ID, + WorkspaceID: workspace.ID, + AgentID: agentID, + AppURL: appURL, } badTicketStr, err := api.WorkspaceAppsProvider.GenerateTicket(badTicket) require.NoError(t, err) @@ -510,7 +509,7 @@ func Test_ResolveRequest(t *testing.T) { } rw := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/app", nil) + r := httptest.NewRequest("GET", "/", nil) r.Header.Set(codersdk.SessionTokenHeader, client.SessionToken()) ticket, ok := api.WorkspaceAppsProvider.ResolveRequest(rw, r, req) @@ -519,6 +518,30 @@ func Test_ResolveRequest(t *testing.T) { require.Equal(t, "http://127.0.0.1:9090", ticket.AppURL) }) + t.Run("Terminal", func(t *testing.T) { + t.Parallel() + + req := workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodTerminal, + BasePath: "/app", + AgentNameOrID: agentID.String(), + } + + rw := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/app", nil) + r.Header.Set(codersdk.SessionTokenHeader, client.SessionToken()) + + ticket, ok := api.WorkspaceAppsProvider.ResolveRequest(rw, r, req) + require.True(t, ok) + require.Equal(t, req.AccessMethod, ticket.AccessMethod) + require.Equal(t, req.BasePath, ticket.BasePath) + require.Empty(t, ticket.UsernameOrID) + require.Empty(t, ticket.WorkspaceNameOrID) + require.Equal(t, req.AgentNameOrID, ticket.Request.AgentNameOrID) + require.Empty(t, ticket.AppSlugOrPort) + require.Empty(t, ticket.AppURL) + }) + t.Run("InsufficientPermissions", func(t *testing.T) { t.Parallel() diff --git a/coderd/workspaceapps/request.go b/coderd/workspaceapps/request.go index 588a78ed4ace2..e5a2a6cf6d4c0 100644 --- a/coderd/workspaceapps/request.go +++ b/coderd/workspaceapps/request.go @@ -1,10 +1,17 @@ package workspaceapps import ( + "context" + "database/sql" + "fmt" + "strconv" "strings" "golang.org/x/xerrors" + "github.com/google/uuid" + + "github.com/coder/coder/coderd/database" "github.com/coder/coder/codersdk" ) @@ -13,31 +20,55 @@ type AccessMethod string const ( AccessMethodPath AccessMethod = "path" AccessMethodSubdomain AccessMethod = "subdomain" + // Terminal is special since it's not a real app and only applies to the PTY + // endpoint on the API. + AccessMethodTerminal AccessMethod = "terminal" ) type Request struct { - AccessMethod AccessMethod + AccessMethod AccessMethod `json:"access_method"` // BasePath of the app. For path apps, this is the path prefix in the router // for this particular app. For subdomain apps, this should be "/". This is // used for setting the cookie path. - BasePath string + BasePath string `json:"base_path"` - UsernameOrID string + // For the following fields, if the AccessMethod is AccessMethodTerminal, + // then only AgentNameOrID may be set and it must be a UUID. The other + // fields must be left blank. + UsernameOrID string `json:"username_or_id"` // WorkspaceAndAgent xor WorkspaceNameOrID are required. - WorkspaceAndAgent string // "workspace" or "workspace.agent" - WorkspaceNameOrID string + WorkspaceAndAgent string `json:"-"` // "workspace" or "workspace.agent" + WorkspaceNameOrID string `json:"workspace_name_or_id"` // AgentNameOrID is not required if the workspace has only one agent. - AgentNameOrID string - AppSlugOrPort string + AgentNameOrID string `json:"agent_name_or_id"` + AppSlugOrPort string `json:"app_slug_or_port"` } func (r Request) Validate() error { - if r.AccessMethod != AccessMethodPath && r.AccessMethod != AccessMethodSubdomain { + switch r.AccessMethod { + case AccessMethodPath, AccessMethodSubdomain, AccessMethodTerminal: + default: return xerrors.Errorf("invalid access method: %q", r.AccessMethod) } if r.BasePath == "" { return xerrors.New("base path is required") } + + if r.AccessMethod == AccessMethodTerminal { + if r.UsernameOrID != "" || r.WorkspaceAndAgent != "" || r.WorkspaceNameOrID != "" || r.AppSlugOrPort != "" { + return xerrors.New("dev error: cannot specify any fields other than r.AccessMethod, r.BasePath and r.AgentNameOrID for terminal access method") + } + + if r.AgentNameOrID == "" { + return xerrors.New("agent name or ID is required") + } + if _, err := uuid.Parse(r.AgentNameOrID); err != nil { + return xerrors.Errorf("invalid agent name or ID %q, must be a UUID: %w", r.AgentNameOrID, err) + } + + return nil + } + if r.UsernameOrID == "" { return xerrors.New("username or ID is required") } @@ -71,3 +102,220 @@ func (r Request) Validate() error { return nil } + +type databaseRequest struct { + Request + // User is the user that owns the app. + User database.User + // Workspace is the workspace that the app is in. + Workspace database.Workspace + // Agent is the agent that the app is running on. + Agent database.WorkspaceAgent + + // AppURL is the resolved URL to the workspace app. This is only set for non + // terminal requests. + AppURL string + // AppSharingLevel is the sharing level of the app. This is forced to be set + // to AppSharingLevelOwner if the access method is terminal. + AppSharingLevel database.AppSharingLevel +} + +// GetDatabase does queries to get the owner user, workspace and agent +// associated with the app in the request. This will correctly perform the +// queries in the correct order based on the access method and what fields are +// available. +// +// If any of the queries don't return any rows, the error will wrap +// sql.ErrNoRows. All other errors should be considered internal server errors. +func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseRequest, error) { + // If the AccessMethod is AccessMethodTerminal, then we need to get the + // agent first since that's the only info we have. + if r.AccessMethod == AccessMethodTerminal { + agentID, uuidErr := uuid.Parse(r.AgentNameOrID) + if uuidErr != nil { + return nil, xerrors.Errorf("invalid agent name or ID %q, must be a UUID for terminal requests: %w", r.AgentNameOrID, uuidErr) + } + + var err error + agent, err := db.GetWorkspaceAgentByID(ctx, agentID) + if err != nil { + return nil, xerrors.Errorf("get workspace agent %q: %w", agentID, err) + } + + // Get the corresponding resource. + res, err := db.GetWorkspaceResourceByID(ctx, agent.ResourceID) + if err != nil { + return nil, xerrors.Errorf("get workspace agent resource %q: %w", agent.ResourceID, err) + } + + // Get the corresponding workspace build. + build, err := db.GetWorkspaceBuildByJobID(ctx, res.JobID) + if err != nil { + return nil, xerrors.Errorf("get workspace build by job ID %q: %w", res.JobID, err) + } + + // Get the corresponding workspace. + workspace, err := db.GetWorkspaceByID(ctx, build.WorkspaceID) + if err != nil { + return nil, xerrors.Errorf("get workspace %q: %w", build.WorkspaceID, err) + } + + // Get the workspace's owner. + user, err := db.GetUserByID(ctx, workspace.OwnerID) + if err != nil { + return nil, xerrors.Errorf("get user %q: %w", workspace.OwnerID, err) + } + + return &databaseRequest{ + Request: r, + User: user, + Workspace: workspace, + Agent: agent, + AppURL: "", + AppSharingLevel: database.AppSharingLevelOwner, + }, nil + } + + // For non-terminal requests, get the objects in order since we have all + // fields available. + + // Get user. + var ( + user database.User + userErr error + ) + if userID, uuidErr := uuid.Parse(r.UsernameOrID); uuidErr == nil { + user, userErr = db.GetUserByID(ctx, userID) + } else { + user, userErr = db.GetUserByEmailOrUsername(ctx, database.GetUserByEmailOrUsernameParams{ + Username: r.UsernameOrID, + }) + } + if userErr != nil { + return nil, xerrors.Errorf("get user %q: %w", r.UsernameOrID, userErr) + } + + // Get workspace. + var ( + workspace database.Workspace + workspaceErr error + ) + if workspaceID, uuidErr := uuid.Parse(r.WorkspaceNameOrID); uuidErr == nil { + workspace, workspaceErr = db.GetWorkspaceByID(ctx, workspaceID) + } else { + workspace, workspaceErr = db.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{ + OwnerID: user.ID, + Name: r.WorkspaceNameOrID, + Deleted: false, + }) + } + if workspaceErr != nil { + return nil, xerrors.Errorf("get workspace %q: %w", r.WorkspaceNameOrID, workspaceErr) + } + + // Get agent. + var ( + agent database.WorkspaceAgent + ) + if agentID, uuidErr := uuid.Parse(r.AgentNameOrID); uuidErr == nil { + var err error + agent, err = db.GetWorkspaceAgentByID(ctx, agentID) + if err != nil { + return nil, xerrors.Errorf("get workspace agent by ID %q: %w", agentID, err) + } + + // Verify that the agent belongs to the workspace. + + //nolint:gocritic // We need to fetch the agent to authenticate the request. This is a system function. + agentResource, err := db.GetWorkspaceResourceByID(ctx, agent.ResourceID) + if err != nil { + return nil, xerrors.Errorf("get agent workspace resource: %w", err) + } + build, err := db.GetWorkspaceBuildByJobID(ctx, agentResource.JobID) + if err != nil { + return nil, xerrors.Errorf("get workspace build by job ID %q: %w", agentResource.JobID, err) + } + if build.WorkspaceID != workspace.ID { + return nil, xerrors.Errorf("agent %q does not belong to workspace %q: %w", agent.ID, workspace.ID, sql.ErrNoRows) + } + } else { + build, err := db.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspace.ID) + if err != nil { + return nil, xerrors.Errorf("get latest workspace build by workspace ID %q: %w", workspace.ID, err) + } + + // nolint:gocritic // We need to fetch the agent to authenticate the request. This is a system function. + resources, err := db.GetWorkspaceResourcesByJobID(ctx, build.JobID) + if err != nil { + return nil, xerrors.Errorf("get workspace resources by job ID %q: %w", build.JobID, err) + } + resourcesIDs := []uuid.UUID{} + for _, resource := range resources { + resourcesIDs = append(resourcesIDs, resource.ID) + } + + // nolint:gocritic // We need to fetch the agent to authenticate the request. This is a system function. + agents, err := db.GetWorkspaceAgentsByResourceIDs(ctx, resourcesIDs) + if err != nil { + return nil, xerrors.Errorf("get workspace agents by resource IDs %v: %w", resourcesIDs, err) + } + + if r.AgentNameOrID == "" { + if len(agents) != 1 { + return nil, xerrors.Errorf("no agent specified, but multiple exist in workspace") + } + + agent = agents[0] + } else { + for _, a := range agents { + if a.Name == r.AgentNameOrID { + agent = a + break + } + } + } + + if agent.ID == uuid.Nil { + return nil, xerrors.Errorf("no agent found with name %q: %w", r.AgentNameOrID, sql.ErrNoRows) + } + } + + // Get app. + var ( + appSharingLevel = database.AppSharingLevelOwner + appURL string + ) + portUint, portUintErr := strconv.ParseUint(r.AppSlugOrPort, 10, 16) + if r.AccessMethod == AccessMethodSubdomain && portUintErr == nil { + // If the app slug is a port number, then route to the port as an + // "anonymous app". We only support HTTP for port-based URLs. + // + // This is only supported for subdomain-based applications. + appURL = fmt.Sprintf("http://127.0.0.1:%d", portUint) + } else { + app, err := db.GetWorkspaceAppByAgentIDAndSlug(ctx, database.GetWorkspaceAppByAgentIDAndSlugParams{ + AgentID: agent.ID, + Slug: r.AppSlugOrPort, + }) + if err != nil { + return nil, xerrors.Errorf("get workspace app by agent ID %q and slug %q: %w", agent.ID, r.AppSlugOrPort, err) + } + if !app.Url.Valid { + return nil, xerrors.Errorf("app URL is not valid") + } + + if app.SharingLevel != "" { + appSharingLevel = app.SharingLevel + } + appURL = app.Url.String + } + + return &databaseRequest{ + Request: r, + User: user, + Workspace: workspace, + Agent: agent, + AppURL: appURL, + AppSharingLevel: appSharingLevel, + }, nil +} diff --git a/coderd/workspaceapps/request_test.go b/coderd/workspaceapps/request_test.go index f5217bf63b39a..56284f2a2b11a 100644 --- a/coderd/workspaceapps/request_test.go +++ b/coderd/workspaceapps/request_test.go @@ -3,6 +3,7 @@ package workspaceapps_test import ( "testing" + "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/workspaceapps" @@ -39,6 +40,14 @@ func Test_RequestValidate(t *testing.T) { }, { name: "OK3", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodTerminal, + BasePath: "/", + AgentNameOrID: uuid.New().String(), + }, + }, + { + name: "OK4", req: workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, BasePath: "/", @@ -188,6 +197,64 @@ func Test_RequestValidate(t *testing.T) { }, errContains: "app slug or port is required", }, + { + name: "Terminal/OtherFields/UsernameOrID", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodTerminal, + BasePath: "/", + UsernameOrID: "foo", + AgentNameOrID: uuid.New().String(), + }, + errContains: "cannot specify any fields other than", + }, + { + name: "Terminal/OtherFields/WorkspaceAndAgent", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodTerminal, + BasePath: "/", + WorkspaceAndAgent: "bar.baz", + AgentNameOrID: uuid.New().String(), + }, + errContains: "cannot specify any fields other than", + }, + { + name: "Terminal/OtherFields/WorkspaceNameOrID", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodTerminal, + BasePath: "/", + WorkspaceNameOrID: "bar", + AgentNameOrID: uuid.New().String(), + }, + errContains: "cannot specify any fields other than", + }, + { + name: "Terminal/OtherFields/AppSlugOrPort", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodTerminal, + BasePath: "/", + AgentNameOrID: uuid.New().String(), + AppSlugOrPort: "baz", + }, + errContains: "cannot specify any fields other than", + }, + { + name: "Terminal/AgentNameOrID/Empty", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodTerminal, + BasePath: "/", + AgentNameOrID: "", + }, + errContains: "agent name or ID is required", + }, + { + name: "Terminal/AgentNameOrID/NotUUID", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodTerminal, + BasePath: "/", + AgentNameOrID: "baz", + }, + errContains: `invalid agent name or ID "baz", must be a UUID`, + }, } for _, c := range cases { @@ -204,3 +271,6 @@ func Test_RequestValidate(t *testing.T) { }) } } + +// getDatabase is tested heavily in auth_test.go, so we don't have specific +// tests for it here. diff --git a/coderd/workspaceapps/ticket.go b/coderd/workspaceapps/ticket.go index 34026bc9c95ac..3b5d24512ceb4 100644 --- a/coderd/workspaceapps/ticket.go +++ b/coderd/workspaceapps/ticket.go @@ -18,11 +18,7 @@ const ticketSigningAlgorithm = jose.HS512 // The JSON field names are short to reduce the size of the ticket. type Ticket struct { // Request details. - AccessMethod AccessMethod `json:"access_method"` - UsernameOrID string `json:"username_or_id"` - WorkspaceNameOrID string `json:"workspace_name_or_id"` - AgentNameOrID string `json:"agent_name_or_id"` - AppSlugOrPort string `json:"app_slug_or_port"` + Request `json:"r"` // Trusted resolved details. Expiry int64 `json:"expiry"` // set by GenerateTicket if unset @@ -34,6 +30,7 @@ type Ticket struct { func (t Ticket) MatchesRequest(req Request) bool { return t.AccessMethod == req.AccessMethod && + t.BasePath == req.BasePath && t.UsernameOrID == req.UsernameOrID && t.WorkspaceNameOrID == req.WorkspaceNameOrID && t.AgentNameOrID == req.AgentNameOrID && diff --git a/coderd/workspaceapps/ticket_test.go b/coderd/workspaceapps/ticket_test.go index f55ace4e15ef2..0a4da05b3e1cd 100644 --- a/coderd/workspaceapps/ticket_test.go +++ b/coderd/workspaceapps/ticket_test.go @@ -28,17 +28,21 @@ func Test_TicketMatchesRequest(t *testing.T) { name: "OK", req: workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", UsernameOrID: "foo", WorkspaceNameOrID: "bar", AgentNameOrID: "baz", AppSlugOrPort: "qux", }, ticket: workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodPath, - UsernameOrID: "foo", - WorkspaceNameOrID: "bar", - AgentNameOrID: "baz", - AppSlugOrPort: "qux", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "qux", + }, }, want: true, }, @@ -48,7 +52,22 @@ func Test_TicketMatchesRequest(t *testing.T) { AccessMethod: workspaceapps.AccessMethodPath, }, ticket: workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodSubdomain, + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + }, + }, + want: false, + }, + { + name: "DifferentBasePath", + req: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + }, + ticket: workspaceapps.Ticket{ + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + }, }, want: false, }, @@ -56,11 +75,15 @@ func Test_TicketMatchesRequest(t *testing.T) { name: "DifferentUsernameOrID", req: workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", UsernameOrID: "foo", }, ticket: workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodPath, - UsernameOrID: "bar", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: "bar", + }, }, want: false, }, @@ -68,13 +91,17 @@ func Test_TicketMatchesRequest(t *testing.T) { name: "DifferentWorkspaceNameOrID", req: workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", UsernameOrID: "foo", WorkspaceNameOrID: "bar", }, ticket: workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodPath, - UsernameOrID: "foo", - WorkspaceNameOrID: "baz", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: "foo", + WorkspaceNameOrID: "baz", + }, }, want: false, }, @@ -82,15 +109,19 @@ func Test_TicketMatchesRequest(t *testing.T) { name: "DifferentAgentNameOrID", req: workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", UsernameOrID: "foo", WorkspaceNameOrID: "bar", AgentNameOrID: "baz", }, ticket: workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodPath, - UsernameOrID: "foo", - WorkspaceNameOrID: "bar", - AgentNameOrID: "qux", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "qux", + }, }, want: false, }, @@ -98,17 +129,21 @@ func Test_TicketMatchesRequest(t *testing.T) { name: "DifferentAppSlugOrPort", req: workspaceapps.Request{ AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", UsernameOrID: "foo", WorkspaceNameOrID: "bar", AgentNameOrID: "baz", AppSlugOrPort: "qux", }, ticket: workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodPath, - UsernameOrID: "foo", - WorkspaceNameOrID: "bar", - AgentNameOrID: "baz", - AppSlugOrPort: "quux", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "quux", + }, }, want: false, }, @@ -134,11 +169,14 @@ func Test_GenerateTicket(t *testing.T) { t.Parallel() ticketStr, err := provider.GenerateTicket(workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodPath, - UsernameOrID: "foo", - WorkspaceNameOrID: "bar", - AgentNameOrID: "baz", - AppSlugOrPort: "qux", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "qux", + }, Expiry: 0, UserID: uuid.MustParse("b1530ba9-76f3-415e-b597-4ddd7cd466a4"), @@ -163,11 +201,14 @@ func Test_GenerateTicket(t *testing.T) { { name: "OK1", ticket: workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodPath, - UsernameOrID: "foo", - WorkspaceNameOrID: "bar", - AgentNameOrID: "baz", - AppSlugOrPort: "qux", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "qux", + }, Expiry: future, UserID: uuid.MustParse("b1530ba9-76f3-415e-b597-4ddd7cd466a4"), @@ -179,11 +220,14 @@ func Test_GenerateTicket(t *testing.T) { { name: "OK2", ticket: workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodSubdomain, - UsernameOrID: "oof", - WorkspaceNameOrID: "rab", - AgentNameOrID: "zab", - AppSlugOrPort: "xuq", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + BasePath: "/", + UsernameOrID: "oof", + WorkspaceNameOrID: "rab", + AgentNameOrID: "zab", + AppSlugOrPort: "xuq", + }, Expiry: future, UserID: uuid.MustParse("6fa684a3-11aa-49fd-8512-ab527bd9b900"), @@ -195,11 +239,14 @@ func Test_GenerateTicket(t *testing.T) { { name: "Expired", ticket: workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodSubdomain, - UsernameOrID: "foo", - WorkspaceNameOrID: "bar", - AgentNameOrID: "baz", - AppSlugOrPort: "qux", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodSubdomain, + BasePath: "/", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "qux", + }, Expiry: time.Now().Add(-time.Hour).Unix(), UserID: uuid.MustParse("b1530ba9-76f3-415e-b597-4ddd7cd466a4"), @@ -262,11 +309,14 @@ func Test_ParseTicket(t *testing.T) { otherProvider := workspaceapps.New(slogtest.Make(t, nil), nil, nil, nil, nil, nil, otherKey) ticketStr, err := otherProvider.GenerateTicket(workspaceapps.Ticket{ - AccessMethod: workspaceapps.AccessMethodPath, - UsernameOrID: "foo", - WorkspaceNameOrID: "bar", - AgentNameOrID: "baz", - AppSlugOrPort: "qux", + Request: workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: "foo", + WorkspaceNameOrID: "bar", + AgentNameOrID: "baz", + AppSlugOrPort: "qux", + }, Expiry: time.Now().Add(time.Hour).Unix(), UserID: uuid.MustParse("b1530ba9-76f3-415e-b597-4ddd7cd466a4"), From 95de7c86c4e64418b0cb73f84a0df2549900f62f Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Fri, 17 Mar 2023 04:18:59 +0000 Subject: [PATCH 2/4] chore: fix pty authz checks --- coderd/coderd.go | 4 +++- coderd/workspaceapps/auth.go | 45 +++++++++++++++++++++++------------- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index ed53b095751e5..bafd6d5debed4 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -599,6 +599,9 @@ func New(options *Options) *API { r.Post("/report-stats", api.workspaceAgentReportStats) r.Post("/report-lifecycle", api.workspaceAgentReportLifecycle) }) + // No middleware on the PTY endpoint since it uses workspace + // application auth and tickets. + r.Get("/{workspaceagent}/pty", api.workspaceAgentPTY) r.Route("/{workspaceagent}", func(r chi.Router) { r.Use( apiKeyMiddleware, @@ -606,7 +609,6 @@ func New(options *Options) *API { httpmw.ExtractWorkspaceParam(options.Database), ) r.Get("/", api.workspaceAgent) - r.Get("/pty", api.workspaceAgentPTY) r.Get("/listening-ports", api.workspaceAgentListeningPorts) r.Get("/connection", api.workspaceAgentConnection) r.Get("/coordinate", api.workspaceAgentClientCoordinate) diff --git a/coderd/workspaceapps/auth.go b/coderd/workspaceapps/auth.go index 47af6845b722b..92e7dc218af9f 100644 --- a/coderd/workspaceapps/auth.go +++ b/coderd/workspaceapps/auth.go @@ -121,8 +121,7 @@ func (p *Provider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appRe // is not running. // Verify the user has access to the app. - // TODO(@deansheather): this should be a different auth check for terminal reqs - authed, ok := p.fetchWorkspaceApplicationAuth(rw, r, authz, appReq.AccessMethod, dbReq.Workspace, dbReq.AppSharingLevel) + authed, ok := p.verifyAuthz(rw, r, authz, dbReq) if !ok { return nil, false } @@ -188,7 +187,8 @@ func (p *Provider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appRe return &ticket, true } -func (p *Provider) authorizeWorkspaceApp(ctx context.Context, roles *httpmw.Authorization, accessMethod AccessMethod, sharingLevel database.AppSharingLevel, workspace database.Workspace) (bool, error) { +func (p *Provider) authorizeRequest(ctx context.Context, roles *httpmw.Authorization, dbReq *databaseRequest) (bool, error) { + accessMethod := dbReq.AccessMethod if accessMethod == "" { accessMethod = AccessMethodPath } @@ -200,6 +200,7 @@ func (p *Provider) authorizeWorkspaceApp(ctx context.Context, roles *httpmw.Auth // // Site owners are blocked from accessing path-based apps unless the // Dangerous.AllowPathAppSiteOwnerAccess flag is enabled in the check below. + sharingLevel := dbReq.AppSharingLevel if isPathApp && !p.DeploymentValues.Dangerous.AllowPathAppSharing.Value() { sharingLevel = database.AppSharingLevelOwner } @@ -220,18 +221,33 @@ func (p *Provider) authorizeWorkspaceApp(ctx context.Context, roles *httpmw.Auth // workspaces owned by different users. if isPathApp && sharingLevel == database.AppSharingLevelOwner && - workspace.OwnerID.String() != roles.Actor.ID && + dbReq.Workspace.OwnerID.String() != roles.Actor.ID && !p.DeploymentValues.Dangerous.AllowPathAppSiteOwnerAccess.Value() { return false, nil } + // Figure out which RBAC resource to check. For terminals we use execution + // instead of application connect. + var ( + rbacAction rbac.Action = rbac.ActionCreate + rbacResource rbac.Object = dbReq.Workspace.ApplicationConnectRBAC() + // rbacResourceOwned is for the level "authenticated". We still need to + // make sure the API key has permissions to connect to the actor's own + // workspace. Scopes would prevent this. + rbacResourceOwned rbac.Object = rbac.ResourceWorkspaceApplicationConnect.WithOwner(roles.Actor.ID) + ) + if dbReq.AccessMethod == AccessMethodTerminal { + rbacResource = dbReq.Workspace.ExecutionRBAC() + rbacResourceOwned = rbac.ResourceWorkspaceExecution.WithOwner(roles.Actor.ID) + } + // Do a standard RBAC check. This accounts for share level "owner" and any // other RBAC rules that may be in place. // // Regardless of share level or whether it's enabled or not, the owner of // the workspace can always access applications (as long as their API key's // scope allows it). - err := p.Authorizer.Authorize(ctx, roles.Actor, rbac.ActionCreate, workspace.ApplicationConnectRBAC()) + err := p.Authorizer.Authorize(ctx, roles.Actor, rbacAction, rbacResource) if err == nil { return true, nil } @@ -242,19 +258,16 @@ func (p *Provider) authorizeWorkspaceApp(ctx context.Context, roles *httpmw.Auth // Owners can always access their own apps according to RBAC rules, so // they have already been returned from this function. case database.AppSharingLevelAuthenticated: - // The user is authenticated at this point, but we need to make sure - // that they have ApplicationConnect permissions to their own - // workspaces. This ensures that the key's scope has permission to - // connect to workspace apps. - object := rbac.ResourceWorkspaceApplicationConnect.WithOwner(roles.Actor.ID) - err := p.Authorizer.Authorize(ctx, roles.Actor, rbac.ActionCreate, object) + // Check with the owned resource to ensure the API key has permissions + // to connect to the actor's own workspace. This enforces scopes. + err := p.Authorizer.Authorize(ctx, roles.Actor, rbacAction, rbacResourceOwned) if err == nil { return true, nil } case database.AppSharingLevelPublic: // We don't really care about scopes and stuff if it's public anyways. - // Someone with a restricted-scope API key could just not submit the - // API key cookie in the request and access the page. + // Someone with a restricted-scope API key could just not submit the API + // key cookie in the request and access the page. return true, nil } @@ -262,12 +275,12 @@ func (p *Provider) authorizeWorkspaceApp(ctx context.Context, roles *httpmw.Auth return false, nil } -// fetchWorkspaceApplicationAuth authorizes the user using api.Authorizer for a +// verifyAuthz authorizes the user using api.Authorizer for a // given app share level in the given workspace. The user's authorization status // is returned. If a server error occurs, a HTML error page is rendered and // false is returned so the caller can return early. -func (p *Provider) fetchWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Request, authz *httpmw.Authorization, accessMethod AccessMethod, workspace database.Workspace, appSharingLevel database.AppSharingLevel) (authed bool, ok bool) { - ok, err := p.authorizeWorkspaceApp(r.Context(), authz, accessMethod, appSharingLevel, workspace) +func (p *Provider) verifyAuthz(rw http.ResponseWriter, r *http.Request, authz *httpmw.Authorization, dbReq *databaseRequest) (authed bool, ok bool) { + ok, err := p.authorizeRequest(r.Context(), authz, dbReq) if err != nil { p.Logger.Error(r.Context(), "authorize workspace app", slog.Error(err)) site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ From 9398ee4e4b4f103bae6fcc52cb700460fbaf5dfe Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 28 Mar 2023 18:22:20 +0000 Subject: [PATCH 3/4] fix: enforce agent and app health in ticket code --- coderd/coderd.go | 1 + coderd/database/modelmethods.go | 76 +++++++++++++++++++ coderd/workspaceagents.go | 66 ++--------------- coderd/workspaceapps/auth.go | 38 ++++++++++ coderd/workspaceapps/provider.go | 35 +++++---- coderd/workspaceapps/request.go | 111 ++++++++++++++++------------ coderd/workspaceapps/ticket.go | 2 +- coderd/workspaceapps/ticket_test.go | 6 +- codersdk/workspaceagents.go | 1 + 9 files changed, 211 insertions(+), 125 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index f00a06c84a9cb..addf8d6750a65 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -287,6 +287,7 @@ func New(options *Options) *API { options.Database, options.DeploymentValues, oauthConfigs, + options.AgentInactiveDisconnectTimeout, options.AppSigningKey, ), metricsCache: metricsCache, diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index 0d037b7c509f9..376a4c1247d3a 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -3,6 +3,7 @@ package database import ( "sort" "strconv" + "time" "golang.org/x/exp/maps" @@ -36,6 +37,26 @@ func (s WorkspaceStatus) Valid() bool { } } +type WorkspaceAgentStatus string + +// This is also in codersdk/workspaceagents.go and should be kept in sync. +const ( + WorkspaceAgentStatusConnecting WorkspaceAgentStatus = "connecting" + WorkspaceAgentStatusConnected WorkspaceAgentStatus = "connected" + WorkspaceAgentStatusDisconnected WorkspaceAgentStatus = "disconnected" + WorkspaceAgentStatusTimeout WorkspaceAgentStatus = "timeout" +) + +func (s WorkspaceAgentStatus) Valid() bool { + switch s { + case WorkspaceAgentStatusConnecting, WorkspaceAgentStatusConnected, + WorkspaceAgentStatusDisconnected, WorkspaceAgentStatusTimeout: + return true + default: + return false + } +} + type AuditableGroup struct { Group Members []GroupMember `json:"members"` @@ -199,6 +220,61 @@ func (l License) RBACObject() rbac.Object { return rbac.ResourceLicense.WithIDString(strconv.FormatInt(int64(l.ID), 10)) } +type WorkspaceAgentConnectionStatus struct { + Status WorkspaceAgentStatus `json:"status"` + FirstConnectedAt *time.Time `json:"first_connected_at"` + LastConnectedAt *time.Time `json:"last_connected_at"` + DisconnectedAt *time.Time `json:"disconnected_at"` +} + +func (a WorkspaceAgent) Status(inactiveTimeout time.Duration) WorkspaceAgentConnectionStatus { + connectionTimeout := time.Duration(a.ConnectionTimeoutSeconds) * time.Second + + status := WorkspaceAgentConnectionStatus{ + Status: WorkspaceAgentStatusDisconnected, + } + if a.FirstConnectedAt.Valid { + status.FirstConnectedAt = &a.FirstConnectedAt.Time + } + if a.LastConnectedAt.Valid { + status.LastConnectedAt = &a.LastConnectedAt.Time + } + if a.DisconnectedAt.Valid { + status.DisconnectedAt = &a.DisconnectedAt.Time + } + + switch { + case !a.FirstConnectedAt.Valid: + switch { + case connectionTimeout > 0 && Now().Sub(a.CreatedAt) > connectionTimeout: + // If the agent took too long to connect the first time, + // mark it as timed out. + status.Status = WorkspaceAgentStatusTimeout + default: + // If the agent never connected, it's waiting for the compute + // to start up. + status.Status = WorkspaceAgentStatusConnecting + } + // We check before instead of after because last connected at and + // disconnected at can be equal timestamps in tight-timed tests. + case !a.DisconnectedAt.Time.Before(a.LastConnectedAt.Time): + // If we've disconnected after our last connection, we know the + // agent is no longer connected. + status.Status = WorkspaceAgentStatusDisconnected + case Now().Sub(a.LastConnectedAt.Time) > inactiveTimeout: + // The connection died without updating the last connected. + status.Status = WorkspaceAgentStatusDisconnected + // Client code needs an accurate disconnected at if the agent has been inactive. + status.DisconnectedAt = &a.LastConnectedAt.Time + case a.LastConnectedAt.Valid: + // The agent should be assumed connected if it's under inactivity timeouts + // and last connected at has been properly set. + status.Status = WorkspaceAgentStatusConnected + } + + return status +} + func ConvertUserRows(rows []GetUsersRow) []User { users := make([]User, len(rows)) for i, r := range rows { diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 24298e4c218f3..74e2818a6bbe1 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -559,28 +559,6 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) { return } - // TODO(@deansheather): bring back agent state check in the upstream ticket - // code - /* - apiAgent, err := convertWorkspaceAgent( - api.DERPMap, *api.TailnetCoordinator.Load(), workspaceAgent, nil, api.AgentInactiveDisconnectTimeout, - api.DeploymentValues.AgentFallbackTroubleshootingURL.String(), - ) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error reading workspace agent.", - Detail: err.Error(), - }) - return - } - if apiAgent.Status != codersdk.WorkspaceAgentConnected { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("Agent state is %q, it must be in the %q state.", apiAgent.Status, codersdk.WorkspaceAgentConnected), - }) - return - } - */ - reconnect, err := uuid.Parse(r.URL.Query().Get("reconnect")) if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ @@ -1218,45 +1196,11 @@ func convertWorkspaceAgent(derpMap *tailcfg.DERPMap, coordinator tailnet.Coordin } } - if dbAgent.FirstConnectedAt.Valid { - workspaceAgent.FirstConnectedAt = &dbAgent.FirstConnectedAt.Time - } - if dbAgent.LastConnectedAt.Valid { - workspaceAgent.LastConnectedAt = &dbAgent.LastConnectedAt.Time - } - if dbAgent.DisconnectedAt.Valid { - workspaceAgent.DisconnectedAt = &dbAgent.DisconnectedAt.Time - } - - connectionTimeout := time.Duration(dbAgent.ConnectionTimeoutSeconds) * time.Second - switch { - case !dbAgent.FirstConnectedAt.Valid: - switch { - case connectionTimeout > 0 && database.Now().Sub(dbAgent.CreatedAt) > connectionTimeout: - // If the agent took too long to connect the first time, - // mark it as timed out. - workspaceAgent.Status = codersdk.WorkspaceAgentTimeout - default: - // If the agent never connected, it's waiting for the compute - // to start up. - workspaceAgent.Status = codersdk.WorkspaceAgentConnecting - } - // We check before instead of after because last connected at and - // disconnected at can be equal timestamps in tight-timed tests. - case !dbAgent.DisconnectedAt.Time.Before(dbAgent.LastConnectedAt.Time): - // If we've disconnected after our last connection, we know the - // agent is no longer connected. - workspaceAgent.Status = codersdk.WorkspaceAgentDisconnected - case database.Now().Sub(dbAgent.LastConnectedAt.Time) > agentInactiveDisconnectTimeout: - // The connection died without updating the last connected. - workspaceAgent.Status = codersdk.WorkspaceAgentDisconnected - // Client code needs an accurate disconnected at if the agent has been inactive. - workspaceAgent.DisconnectedAt = &dbAgent.LastConnectedAt.Time - case dbAgent.LastConnectedAt.Valid: - // The agent should be assumed connected if it's under inactivity timeouts - // and last connected at has been properly set. - workspaceAgent.Status = codersdk.WorkspaceAgentConnected - } + status := dbAgent.Status(agentInactiveDisconnectTimeout) + workspaceAgent.Status = codersdk.WorkspaceAgentStatus(status.Status) + workspaceAgent.FirstConnectedAt = status.FirstConnectedAt + workspaceAgent.LastConnectedAt = status.LastConnectedAt + workspaceAgent.DisconnectedAt = status.DisconnectedAt return workspaceAgent, nil } diff --git a/coderd/workspaceapps/auth.go b/coderd/workspaceapps/auth.go index 92e7dc218af9f..bcc59fc1fc186 100644 --- a/coderd/workspaceapps/auth.go +++ b/coderd/workspaceapps/auth.go @@ -3,6 +3,7 @@ package workspaceapps import ( "context" "database/sql" + "fmt" "net/http" "strings" "time" @@ -158,6 +159,19 @@ func (p *Provider) ResolveRequest(rw http.ResponseWriter, r *http.Request, appRe return nil, false } + // Check that the agent is online. + agentStatus := dbReq.Agent.Status(p.WorkspaceAgentInactiveTimeout) + if agentStatus.Status != database.WorkspaceAgentStatusConnected { + p.writeWorkspaceAppOffline(rw, r, &appReq, fmt.Sprintf("Agent state is %q, not %q", agentStatus.Status, database.WorkspaceAgentStatusConnected)) + return nil, false + } + + // Check that the app is healthy. + if dbReq.AppHealth != "" && dbReq.AppHealth != database.WorkspaceAppHealthDisabled && dbReq.AppHealth != database.WorkspaceAppHealthHealthy { + p.writeWorkspaceAppOffline(rw, r, &appReq, fmt.Sprintf("App health is %q, not %q", dbReq.AppHealth, database.WorkspaceAppHealthHealthy)) + return nil, false + } + // As a sanity check, ensure the ticket we just made is valid for this // request. if !ticket.MatchesRequest(appReq) { @@ -347,3 +361,27 @@ func (p *Provider) writeWorkspaceApp500(rw http.ResponseWriter, r *http.Request, DashboardURL: p.AccessURL.String(), }) } + +// writeWorkspaceAppOffline writes a HTML 502 error page for a workspace app. If +// appReq is not nil, it will be used to log the request details at debug level. +func (p *Provider) writeWorkspaceAppOffline(rw http.ResponseWriter, r *http.Request, appReq *Request, msg string) { + if appReq != nil { + slog.Helper() + p.Logger.Debug(r.Context(), + "workspace app offline: "+msg, + slog.F("username_or_id", appReq.UsernameOrID), + slog.F("workspace_and_agent", appReq.WorkspaceAndAgent), + slog.F("workspace_name_or_id", appReq.WorkspaceNameOrID), + slog.F("agent_name_or_id", appReq.AgentNameOrID), + slog.F("app_slug_or_port", appReq.AppSlugOrPort), + ) + } + + site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ + Status: http.StatusBadGateway, + Title: "Application Offline", + Description: msg, + RetryEnabled: true, + DashboardURL: p.AccessURL.String(), + }) +} diff --git a/coderd/workspaceapps/provider.go b/coderd/workspaceapps/provider.go index e85e1a893cae2..23e1fb7e3c736 100644 --- a/coderd/workspaceapps/provider.go +++ b/coderd/workspaceapps/provider.go @@ -2,6 +2,7 @@ package workspaceapps import ( "net/url" + "time" "cdr.dev/slog" "github.com/coder/coder/coderd/database" @@ -16,26 +17,32 @@ import ( type Provider struct { Logger slog.Logger - AccessURL *url.URL - Authorizer rbac.Authorizer - Database database.Store - DeploymentValues *codersdk.DeploymentValues - OAuth2Configs *httpmw.OAuth2Configs - TicketSigningKey []byte + AccessURL *url.URL + Authorizer rbac.Authorizer + Database database.Store + DeploymentValues *codersdk.DeploymentValues + OAuth2Configs *httpmw.OAuth2Configs + WorkspaceAgentInactiveTimeout time.Duration + TicketSigningKey []byte } -func New(log slog.Logger, accessURL *url.URL, authz rbac.Authorizer, db database.Store, cfg *codersdk.DeploymentValues, oauth2Cfgs *httpmw.OAuth2Configs, ticketSigningKey []byte) *Provider { +func New(log slog.Logger, accessURL *url.URL, authz rbac.Authorizer, db database.Store, cfg *codersdk.DeploymentValues, oauth2Cfgs *httpmw.OAuth2Configs, workspaceAgentInactiveTimeout time.Duration, ticketSigningKey []byte) *Provider { if len(ticketSigningKey) != 64 { panic("ticket signing key must be 64 bytes") } + if workspaceAgentInactiveTimeout == 0 { + workspaceAgentInactiveTimeout = 1 * time.Minute + } + return &Provider{ - Logger: log, - AccessURL: accessURL, - Authorizer: authz, - Database: db, - DeploymentValues: cfg, - OAuth2Configs: oauth2Cfgs, - TicketSigningKey: ticketSigningKey, + Logger: log, + AccessURL: accessURL, + Authorizer: authz, + Database: db, + DeploymentValues: cfg, + OAuth2Configs: oauth2Cfgs, + WorkspaceAgentInactiveTimeout: workspaceAgentInactiveTimeout, + TicketSigningKey: ticketSigningKey, } } diff --git a/coderd/workspaceapps/request.go b/coderd/workspaceapps/request.go index e5a2a6cf6d4c0..14aa3e706cb83 100644 --- a/coderd/workspaceapps/request.go +++ b/coderd/workspaceapps/request.go @@ -20,8 +20,8 @@ type AccessMethod string const ( AccessMethodPath AccessMethod = "path" AccessMethodSubdomain AccessMethod = "subdomain" - // Terminal is special since it's not a real app and only applies to the PTY - // endpoint on the API. + // AccessMethodTerminal is special since it's not a real app and only + // applies to the PTY endpoint on the API. AccessMethodTerminal AccessMethod = "terminal" ) @@ -115,12 +115,15 @@ type databaseRequest struct { // AppURL is the resolved URL to the workspace app. This is only set for non // terminal requests. AppURL string + // AppHealth is the health of the app. For terminal requests, this is always + // database.WorkspaceAppHealthHealthy. + AppHealth database.WorkspaceAppHealth // AppSharingLevel is the sharing level of the app. This is forced to be set // to AppSharingLevelOwner if the access method is terminal. AppSharingLevel database.AppSharingLevel } -// GetDatabase does queries to get the owner user, workspace and agent +// getDatabase does queries to get the owner user, workspace and agent // associated with the app in the request. This will correctly perform the // queries in the correct order based on the access method and what fields are // available. @@ -131,49 +134,7 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR // If the AccessMethod is AccessMethodTerminal, then we need to get the // agent first since that's the only info we have. if r.AccessMethod == AccessMethodTerminal { - agentID, uuidErr := uuid.Parse(r.AgentNameOrID) - if uuidErr != nil { - return nil, xerrors.Errorf("invalid agent name or ID %q, must be a UUID for terminal requests: %w", r.AgentNameOrID, uuidErr) - } - - var err error - agent, err := db.GetWorkspaceAgentByID(ctx, agentID) - if err != nil { - return nil, xerrors.Errorf("get workspace agent %q: %w", agentID, err) - } - - // Get the corresponding resource. - res, err := db.GetWorkspaceResourceByID(ctx, agent.ResourceID) - if err != nil { - return nil, xerrors.Errorf("get workspace agent resource %q: %w", agent.ResourceID, err) - } - - // Get the corresponding workspace build. - build, err := db.GetWorkspaceBuildByJobID(ctx, res.JobID) - if err != nil { - return nil, xerrors.Errorf("get workspace build by job ID %q: %w", res.JobID, err) - } - - // Get the corresponding workspace. - workspace, err := db.GetWorkspaceByID(ctx, build.WorkspaceID) - if err != nil { - return nil, xerrors.Errorf("get workspace %q: %w", build.WorkspaceID, err) - } - - // Get the workspace's owner. - user, err := db.GetUserByID(ctx, workspace.OwnerID) - if err != nil { - return nil, xerrors.Errorf("get user %q: %w", workspace.OwnerID, err) - } - - return &databaseRequest{ - Request: r, - User: user, - Workspace: workspace, - Agent: agent, - AppURL: "", - AppSharingLevel: database.AppSharingLevelOwner, - }, nil + return r.getDatabaseTerminal(ctx, db) } // For non-terminal requests, get the objects in order since we have all @@ -284,6 +245,7 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR var ( appSharingLevel = database.AppSharingLevelOwner appURL string + appHealth database.WorkspaceAppHealth ) portUint, portUintErr := strconv.ParseUint(r.AppSlugOrPort, 10, 16) if r.AccessMethod == AccessMethodSubdomain && portUintErr == nil { @@ -306,8 +268,11 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR if app.SharingLevel != "" { appSharingLevel = app.SharingLevel + } else { + appSharingLevel = database.AppSharingLevelOwner } appURL = app.Url.String + appHealth = app.Health } return &databaseRequest{ @@ -316,6 +281,60 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR Workspace: workspace, Agent: agent, AppURL: appURL, + AppHealth: appHealth, AppSharingLevel: appSharingLevel, }, nil } + +// getDatabaseTerminal is called by getDatabase for AccessMethodTerminal +// requests. +func (r Request) getDatabaseTerminal(ctx context.Context, db database.Store) (*databaseRequest, error) { + if r.AccessMethod != AccessMethodTerminal { + return nil, xerrors.Errorf("invalid access method %q for terminal request", r.AccessMethod) + } + + agentID, uuidErr := uuid.Parse(r.AgentNameOrID) + if uuidErr != nil { + return nil, xerrors.Errorf("invalid agent name or ID %q, must be a UUID for terminal requests: %w", r.AgentNameOrID, uuidErr) + } + + var err error + agent, err := db.GetWorkspaceAgentByID(ctx, agentID) + if err != nil { + return nil, xerrors.Errorf("get workspace agent %q: %w", agentID, err) + } + + // Get the corresponding resource. + res, err := db.GetWorkspaceResourceByID(ctx, agent.ResourceID) + if err != nil { + return nil, xerrors.Errorf("get workspace agent resource %q: %w", agent.ResourceID, err) + } + + // Get the corresponding workspace build. + build, err := db.GetWorkspaceBuildByJobID(ctx, res.JobID) + if err != nil { + return nil, xerrors.Errorf("get workspace build by job ID %q: %w", res.JobID, err) + } + + // Get the corresponding workspace. + workspace, err := db.GetWorkspaceByID(ctx, build.WorkspaceID) + if err != nil { + return nil, xerrors.Errorf("get workspace %q: %w", build.WorkspaceID, err) + } + + // Get the workspace's owner. + user, err := db.GetUserByID(ctx, workspace.OwnerID) + if err != nil { + return nil, xerrors.Errorf("get user %q: %w", workspace.OwnerID, err) + } + + return &databaseRequest{ + Request: r, + User: user, + Workspace: workspace, + Agent: agent, + AppURL: "", + AppHealth: database.WorkspaceAppHealthHealthy, + AppSharingLevel: database.AppSharingLevelOwner, + }, nil +} diff --git a/coderd/workspaceapps/ticket.go b/coderd/workspaceapps/ticket.go index 3b5d24512ceb4..11fb6465b190a 100644 --- a/coderd/workspaceapps/ticket.go +++ b/coderd/workspaceapps/ticket.go @@ -18,7 +18,7 @@ const ticketSigningAlgorithm = jose.HS512 // The JSON field names are short to reduce the size of the ticket. type Ticket struct { // Request details. - Request `json:"r"` + Request `json:"request"` // Trusted resolved details. Expiry int64 `json:"expiry"` // set by GenerateTicket if unset diff --git a/coderd/workspaceapps/ticket_test.go b/coderd/workspaceapps/ticket_test.go index 0a4da05b3e1cd..c7fd79cff3104 100644 --- a/coderd/workspaceapps/ticket_test.go +++ b/coderd/workspaceapps/ticket_test.go @@ -163,7 +163,7 @@ func Test_TicketMatchesRequest(t *testing.T) { func Test_GenerateTicket(t *testing.T) { t.Parallel() - provider := workspaceapps.New(slogtest.Make(t, nil), nil, nil, nil, nil, nil, coderdtest.AppSigningKey) + provider := workspaceapps.New(slogtest.Make(t, nil), nil, nil, nil, nil, nil, time.Minute, coderdtest.AppSigningKey) t.Run("SetExpiry", func(t *testing.T) { t.Parallel() @@ -286,7 +286,7 @@ func Test_GenerateTicket(t *testing.T) { func Test_ParseTicket(t *testing.T) { t.Parallel() - provider := workspaceapps.New(slogtest.Make(t, nil), nil, nil, nil, nil, nil, coderdtest.AppSigningKey) + provider := workspaceapps.New(slogtest.Make(t, nil), nil, nil, nil, nil, nil, time.Minute, coderdtest.AppSigningKey) t.Run("InvalidJWS", func(t *testing.T) { t.Parallel() @@ -306,7 +306,7 @@ func Test_ParseTicket(t *testing.T) { require.NotEqual(t, coderdtest.AppSigningKey, otherKey) require.Len(t, otherKey, 64) - otherProvider := workspaceapps.New(slogtest.Make(t, nil), nil, nil, nil, nil, nil, otherKey) + otherProvider := workspaceapps.New(slogtest.Make(t, nil), nil, nil, nil, nil, nil, time.Minute, otherKey) ticketStr, err := otherProvider.GenerateTicket(workspaceapps.Ticket{ Request: workspaceapps.Request{ diff --git a/codersdk/workspaceagents.go b/codersdk/workspaceagents.go index 0504c33d2569a..cf2d1687f3dc5 100644 --- a/codersdk/workspaceagents.go +++ b/codersdk/workspaceagents.go @@ -25,6 +25,7 @@ import ( type WorkspaceAgentStatus string +// This is also in database/modelmethods.go and should be kept in sync. const ( WorkspaceAgentConnecting WorkspaceAgentStatus = "connecting" WorkspaceAgentConnected WorkspaceAgentStatus = "connected" From dce5a164d2250e544a98e1593ed7a8601ab88cf6 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 29 Mar 2023 14:18:51 +0000 Subject: [PATCH 4/4] feat: support no agent name when using app slug --- coderd/coderdtest/coderdtest.go | 17 +- coderd/database/dbauthz/querier.go | 9 + coderd/database/dbfake/databasefake.go | 32 ++++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 75 ++++++++ coderd/database/queries/workspaceagents.sql | 20 +++ coderd/workspaceapps/auth.go | 4 +- coderd/workspaceapps/auth_test.go | 187 ++++++++++++++++---- coderd/workspaceapps/request.go | 155 ++++++++-------- coderd/workspaceapps_test.go | 4 +- 10 files changed, 392 insertions(+), 112 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index ff9bf82addc98..a386c264e7fa6 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -638,10 +638,17 @@ func AwaitWorkspaceBuildJob(t *testing.T, client *codersdk.Client, build uuid.UU return workspaceBuild } -// AwaitWorkspaceAgents waits for all resources with agents to be connected. -func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID) []codersdk.WorkspaceResource { +// AwaitWorkspaceAgents waits for all resources with agents to be connected. If +// specific agents are provided, it will wait for those agents to be connected +// but will not fail if other agents are not connected. +func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, workspaceID uuid.UUID, agentNames ...string) []codersdk.WorkspaceResource { t.Helper() + agentNamesMap := make(map[string]struct{}, len(agentNames)) + for _, name := range agentNames { + agentNamesMap[name] = struct{}{} + } + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() @@ -659,6 +666,12 @@ func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, workspaceID uui for _, resource := range workspace.LatestBuild.Resources { for _, agent := range resource.Agents { + if len(agentNames) > 0 { + if _, ok := agentNamesMap[agent.Name]; !ok { + continue + } + } + if agent.Status != codersdk.WorkspaceAgentConnected { t.Logf("agent %s not connected yet", agent.Name) return false diff --git a/coderd/database/dbauthz/querier.go b/coderd/database/dbauthz/querier.go index 2422b5739c546..a55ed6ba95d8f 100644 --- a/coderd/database/dbauthz/querier.go +++ b/coderd/database/dbauthz/querier.go @@ -1292,6 +1292,15 @@ func (q *querier) GetWorkspaceAgentByInstanceID(ctx context.Context, authInstanc return agent, nil } +func (q *querier) GetWorkspaceAgentsInLatestBuildByWorkspaceID(ctx context.Context, workspaceID uuid.UUID) ([]database.WorkspaceAgent, error) { + workspace, err := q.GetWorkspaceByID(ctx, workspaceID) + if err != nil { + return nil, err + } + + return q.db.GetWorkspaceAgentsInLatestBuildByWorkspaceID(ctx, workspace.ID) +} + func (q *querier) UpdateWorkspaceAgentLifecycleStateByID(ctx context.Context, arg database.UpdateWorkspaceAgentLifecycleStateByIDParams) error { agent, err := q.db.GetWorkspaceAgentByID(ctx, arg.ID) if err != nil { diff --git a/coderd/database/dbfake/databasefake.go b/coderd/database/dbfake/databasefake.go index 330c8d0a3d4a1..351707748b3c9 100644 --- a/coderd/database/dbfake/databasefake.go +++ b/coderd/database/dbfake/databasefake.go @@ -314,6 +314,38 @@ func (*fakeQuerier) DeleteOldWorkspaceAgentStats(_ context.Context) error { return nil } +func (q *fakeQuerier) GetWorkspaceAgentsInLatestBuildByWorkspaceID(ctx context.Context, workspaceID uuid.UUID) ([]database.WorkspaceAgent, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + + // Get latest build for workspace. + workspaceBuild, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, workspaceID) + if err != nil { + return nil, xerrors.Errorf("get latest workspace build: %w", err) + } + + // Get resources for build. + resources, err := q.GetWorkspaceResourcesByJobID(ctx, workspaceBuild.JobID) + if err != nil { + return nil, xerrors.Errorf("get workspace resources: %w", err) + } + if len(resources) == 0 { + return []database.WorkspaceAgent{}, nil + } + + resourceIDs := make([]uuid.UUID, len(resources)) + for i, resource := range resources { + resourceIDs[i] = resource.ID + } + + agents, err := q.GetWorkspaceAgentsByResourceIDs(ctx, resourceIDs) + if err != nil { + return nil, xerrors.Errorf("get workspace agents: %w", err) + } + + return agents, nil +} + func (q *fakeQuerier) GetDeploymentWorkspaceAgentStats(_ context.Context, createdAfter time.Time) (database.GetDeploymentWorkspaceAgentStatsRow, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 5ae91154ac46c..4ef49a931fbf4 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -130,6 +130,7 @@ type sqlcQuerier interface { GetWorkspaceAgentStats(ctx context.Context, createdAt time.Time) ([]GetWorkspaceAgentStatsRow, error) GetWorkspaceAgentsByResourceIDs(ctx context.Context, ids []uuid.UUID) ([]WorkspaceAgent, error) GetWorkspaceAgentsCreatedAfter(ctx context.Context, createdAt time.Time) ([]WorkspaceAgent, error) + GetWorkspaceAgentsInLatestBuildByWorkspaceID(ctx context.Context, workspaceID uuid.UUID) ([]WorkspaceAgent, error) GetWorkspaceAppByAgentIDAndSlug(ctx context.Context, arg GetWorkspaceAppByAgentIDAndSlugParams) (WorkspaceApp, error) GetWorkspaceAppsByAgentID(ctx context.Context, agentID uuid.UUID) ([]WorkspaceApp, error) GetWorkspaceAppsByAgentIDs(ctx context.Context, ids []uuid.UUID) ([]WorkspaceApp, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 258ab2927aedc..669c5b7a474d1 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5463,6 +5463,81 @@ func (q *sqlQuerier) GetWorkspaceAgentsCreatedAfter(ctx context.Context, created return items, nil } +const getWorkspaceAgentsInLatestBuildByWorkspaceID = `-- name: GetWorkspaceAgentsInLatestBuildByWorkspaceID :many +SELECT + workspace_agents.id, workspace_agents.created_at, workspace_agents.updated_at, workspace_agents.name, workspace_agents.first_connected_at, workspace_agents.last_connected_at, workspace_agents.disconnected_at, workspace_agents.resource_id, workspace_agents.auth_token, workspace_agents.auth_instance_id, workspace_agents.architecture, workspace_agents.environment_variables, workspace_agents.operating_system, workspace_agents.startup_script, workspace_agents.instance_metadata, workspace_agents.resource_metadata, workspace_agents.directory, workspace_agents.version, workspace_agents.last_connected_replica_id, workspace_agents.connection_timeout_seconds, workspace_agents.troubleshooting_url, workspace_agents.motd_file, workspace_agents.lifecycle_state, workspace_agents.login_before_ready, workspace_agents.startup_script_timeout_seconds, workspace_agents.expanded_directory, workspace_agents.shutdown_script, workspace_agents.shutdown_script_timeout_seconds, workspace_agents.startup_logs_length, workspace_agents.startup_logs_overflowed +FROM + workspace_agents +JOIN + workspace_resources ON workspace_agents.resource_id = workspace_resources.id +JOIN + workspace_builds ON workspace_resources.job_id = workspace_builds.job_id +WHERE + workspace_builds.workspace_id = $1 :: uuid AND + workspace_builds.build_number = ( + SELECT + MAX(build_number) + FROM + workspace_builds AS wb + WHERE + wb.workspace_id = $1 :: uuid + ) +` + +func (q *sqlQuerier) GetWorkspaceAgentsInLatestBuildByWorkspaceID(ctx context.Context, workspaceID uuid.UUID) ([]WorkspaceAgent, error) { + rows, err := q.db.QueryContext(ctx, getWorkspaceAgentsInLatestBuildByWorkspaceID, workspaceID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []WorkspaceAgent + for rows.Next() { + var i WorkspaceAgent + if err := rows.Scan( + &i.ID, + &i.CreatedAt, + &i.UpdatedAt, + &i.Name, + &i.FirstConnectedAt, + &i.LastConnectedAt, + &i.DisconnectedAt, + &i.ResourceID, + &i.AuthToken, + &i.AuthInstanceID, + &i.Architecture, + &i.EnvironmentVariables, + &i.OperatingSystem, + &i.StartupScript, + &i.InstanceMetadata, + &i.ResourceMetadata, + &i.Directory, + &i.Version, + &i.LastConnectedReplicaID, + &i.ConnectionTimeoutSeconds, + &i.TroubleshootingURL, + &i.MOTDFile, + &i.LifecycleState, + &i.LoginBeforeReady, + &i.StartupScriptTimeoutSeconds, + &i.ExpandedDirectory, + &i.ShutdownScript, + &i.ShutdownScriptTimeoutSeconds, + &i.StartupLogsLength, + &i.StartupLogsOverflowed, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const insertWorkspaceAgent = `-- name: InsertWorkspaceAgent :one INSERT INTO workspace_agents ( diff --git a/coderd/database/queries/workspaceagents.sql b/coderd/database/queries/workspaceagents.sql index 3d7438252ffb8..143be63c07267 100644 --- a/coderd/database/queries/workspaceagents.sql +++ b/coderd/database/queries/workspaceagents.sql @@ -132,3 +132,23 @@ INSERT INTO DELETE FROM workspace_agent_startup_logs WHERE agent_id IN (SELECT id FROM workspace_agents WHERE last_connected_at IS NOT NULL AND last_connected_at < NOW() - INTERVAL '7 day'); + +-- name: GetWorkspaceAgentsInLatestBuildByWorkspaceID :many +SELECT + workspace_agents.* +FROM + workspace_agents +JOIN + workspace_resources ON workspace_agents.resource_id = workspace_resources.id +JOIN + workspace_builds ON workspace_resources.job_id = workspace_builds.job_id +WHERE + workspace_builds.workspace_id = @workspace_id :: uuid AND + workspace_builds.build_number = ( + SELECT + MAX(build_number) + FROM + workspace_builds AS wb + WHERE + wb.workspace_id = @workspace_id :: uuid + ); diff --git a/coderd/workspaceapps/auth.go b/coderd/workspaceapps/auth.go index bcc59fc1fc186..e53c6f6ce66d2 100644 --- a/coderd/workspaceapps/auth.go +++ b/coderd/workspaceapps/auth.go @@ -368,7 +368,7 @@ func (p *Provider) writeWorkspaceAppOffline(rw http.ResponseWriter, r *http.Requ if appReq != nil { slog.Helper() p.Logger.Debug(r.Context(), - "workspace app offline: "+msg, + "workspace app unavailable: "+msg, slog.F("username_or_id", appReq.UsernameOrID), slog.F("workspace_and_agent", appReq.WorkspaceAndAgent), slog.F("workspace_name_or_id", appReq.WorkspaceNameOrID), @@ -379,7 +379,7 @@ func (p *Provider) writeWorkspaceAppOffline(rw http.ResponseWriter, r *http.Requ site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ Status: http.StatusBadGateway, - Title: "Application Offline", + Title: "Application Unavailable", Description: msg, RetryEnabled: true, DashboardURL: p.AccessURL.String(), diff --git a/coderd/workspaceapps/auth_test.go b/coderd/workspaceapps/auth_test.go index a69ae82e71353..f1d06ac981938 100644 --- a/coderd/workspaceapps/auth_test.go +++ b/coderd/workspaceapps/auth_test.go @@ -3,11 +3,13 @@ package workspaceapps_test import ( "context" "fmt" + "io" "net" "net/http" "net/http/httptest" "net/http/httputil" "net/url" + "strings" "testing" "time" @@ -36,6 +38,11 @@ func Test_ResolveRequest(t *testing.T) { appNameAuthed = "app-authed" appNamePublic = "app-public" appNameInvalidURL = "app-invalid-url" + appNameUnhealthy = "app-unhealthy" + + // This agent will never connect, so it will never become "connected". + agentNameUnhealthy = "agent-unhealthy" + appNameAgentUnhealthy = "app-agent-unhealthy" // This is not a valid URL we listen on in the test, but it needs to be // set to a value. @@ -43,6 +50,13 @@ func Test_ResolveRequest(t *testing.T) { ) allApps := []string{appNameOwner, appNameAuthed, appNamePublic} + // Start a listener for a server that always responds with 500 for the + // unhealthy app. + unhealthySrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("unhealthy")) + })) + deploymentValues := coderdtest.DeploymentValues(t) deploymentValues.DisablePathApps = false deploymentValues.Dangerous.AllowPathAppSharing = true @@ -86,39 +100,67 @@ func Test_ResolveRequest(t *testing.T) { Resources: []*proto.Resource{{ Name: "example", Type: "aws_instance", - Agents: []*proto.Agent{{ - Id: uuid.NewString(), - Name: agentName, - Auth: &proto.Agent_Token{ - Token: agentAuthToken, - }, - Apps: []*proto.App{ - { - Slug: appNameOwner, - DisplayName: appNameOwner, - SharingLevel: proto.AppSharingLevel_OWNER, - Url: appURL, + Agents: []*proto.Agent{ + { + Id: uuid.NewString(), + Name: agentName, + Auth: &proto.Agent_Token{ + Token: agentAuthToken, }, - { - Slug: appNameAuthed, - DisplayName: appNameAuthed, - SharingLevel: proto.AppSharingLevel_AUTHENTICATED, - Url: appURL, + Apps: []*proto.App{ + { + Slug: appNameOwner, + DisplayName: appNameOwner, + SharingLevel: proto.AppSharingLevel_OWNER, + Url: appURL, + }, + { + Slug: appNameAuthed, + DisplayName: appNameAuthed, + SharingLevel: proto.AppSharingLevel_AUTHENTICATED, + Url: appURL, + }, + { + Slug: appNamePublic, + DisplayName: appNamePublic, + SharingLevel: proto.AppSharingLevel_PUBLIC, + Url: appURL, + }, + { + Slug: appNameInvalidURL, + DisplayName: appNameInvalidURL, + SharingLevel: proto.AppSharingLevel_PUBLIC, + Url: "test:path/to/app", + }, + { + Slug: appNameUnhealthy, + DisplayName: appNameUnhealthy, + SharingLevel: proto.AppSharingLevel_PUBLIC, + Url: appURL, + Healthcheck: &proto.Healthcheck{ + Url: unhealthySrv.URL, + Interval: 1, + Threshold: 1, + }, + }, }, - { - Slug: appNamePublic, - DisplayName: appNamePublic, - SharingLevel: proto.AppSharingLevel_PUBLIC, - Url: appURL, + }, + { + Id: uuid.NewString(), + Name: agentNameUnhealthy, + Auth: &proto.Agent_Token{ + Token: uuid.NewString(), }, - { - Slug: appNameInvalidURL, - DisplayName: appNameInvalidURL, - SharingLevel: proto.AppSharingLevel_PUBLIC, - Url: "test:path/to/app", + Apps: []*proto.App{ + { + Slug: appNameAgentUnhealthy, + DisplayName: appNameAgentUnhealthy, + SharingLevel: proto.AppSharingLevel_PUBLIC, + Url: appURL, + }, }, }, - }}, + }, }}, }, }, @@ -138,7 +180,7 @@ func Test_ResolveRequest(t *testing.T) { t.Cleanup(func() { _ = agentCloser.Close() }) - resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID) + resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID, agentName) agentID := uuid.Nil for _, resource := range resources { @@ -335,7 +377,7 @@ func Test_ResolveRequest(t *testing.T) { ok bool }{ { - name: "WorkspaecOnly", + name: "WorkspaceOnly", workspaceAndAgent: workspace.Name, workspace: workspace.Name, agent: "", @@ -622,4 +664,89 @@ func Test_ResolveRequest(t *testing.T) { require.Equal(t, "app.com", redirectURI.Host) require.Equal(t, "/some-path", redirectURI.Path) }) + + t.Run("UnhealthyAgent", func(t *testing.T) { + t.Parallel() + + req := workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: me.Username, + WorkspaceNameOrID: workspace.Name, + AgentNameOrID: agentNameUnhealthy, + AppSlugOrPort: appNameAgentUnhealthy, + } + + rw := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/app", nil) + r.Header.Set(codersdk.SessionTokenHeader, client.SessionToken()) + + ticket, ok := api.WorkspaceAppsProvider.ResolveRequest(rw, r, req) + require.False(t, ok, "request succeeded even though agent is not connected") + require.Nil(t, ticket) + + w := rw.Result() + defer w.Body.Close() + require.Equal(t, http.StatusBadGateway, w.StatusCode) + + body, err := io.ReadAll(w.Body) + require.NoError(t, err) + bodyStr := string(body) + bodyStr = strings.ReplaceAll(bodyStr, """, `"`) + // It'll either be "connecting" or "disconnected". Both are OK for this + // test. + require.Contains(t, bodyStr, `Agent state is "`) + }) + + t.Run("UnhealthyApp", func(t *testing.T) { + t.Parallel() + + require.Eventually(t, func() bool { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + + agent, err := client.WorkspaceAgent(ctx, agentID) + if err != nil { + t.Log("could not get agent", err) + return false + } + + for _, app := range agent.Apps { + if app.Slug == appNameUnhealthy { + t.Log("app is", app.Health) + return app.Health == codersdk.WorkspaceAppHealthUnhealthy + } + } + + t.Log("could not find app") + return false + }, testutil.WaitLong, testutil.IntervalFast, "wait for app to become unhealthy") + + req := workspaceapps.Request{ + AccessMethod: workspaceapps.AccessMethodPath, + BasePath: "/app", + UsernameOrID: me.Username, + WorkspaceNameOrID: workspace.Name, + AgentNameOrID: agentName, + AppSlugOrPort: appNameUnhealthy, + } + + rw := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/app", nil) + r.Header.Set(codersdk.SessionTokenHeader, client.SessionToken()) + + ticket, ok := api.WorkspaceAppsProvider.ResolveRequest(rw, r, req) + require.False(t, ok, "request succeeded even though app is unhealthy") + require.Nil(t, ticket) + + w := rw.Result() + defer w.Body.Close() + require.Equal(t, http.StatusBadGateway, w.StatusCode) + + body, err := io.ReadAll(w.Body) + require.NoError(t, err) + bodyStr := string(body) + bodyStr = strings.ReplaceAll(bodyStr, """, `"`) + require.Contains(t, bodyStr, `App health is "unhealthy"`) + }) } diff --git a/coderd/workspaceapps/request.go b/coderd/workspaceapps/request.go index 14aa3e706cb83..4cc309848ad4f 100644 --- a/coderd/workspaceapps/request.go +++ b/coderd/workspaceapps/request.go @@ -174,62 +174,97 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR return nil, xerrors.Errorf("get workspace %q: %w", r.WorkspaceNameOrID, workspaceErr) } - // Get agent. + // Get workspace agents. + agents, err := db.GetWorkspaceAgentsInLatestBuildByWorkspaceID(ctx, workspace.ID) + if err != nil { + return nil, xerrors.Errorf("get workspace agents: %w", err) + } + if len(agents) == 0 { + // TODO(@deansheather): return a 404 if there are no agents in the + // workspace, requires a different error type. + return nil, xerrors.New("no agents in workspace") + } + + // Get workspace apps. + agentIDs := make([]uuid.UUID, len(agents)) + for i, agent := range agents { + agentIDs[i] = agent.ID + } + apps, err := db.GetWorkspaceAppsByAgentIDs(ctx, agentIDs) + if err != nil { + return nil, xerrors.Errorf("get workspace apps: %w", err) + } + + // Get the app first, because r.AgentNameOrID is optional depending on + // whether the app is a slug or a port and whether there are multiple agents + // in the workspace or not. var ( - agent database.WorkspaceAgent + agentNameOrID = r.AgentNameOrID + appURL string + appSharingLevel database.AppSharingLevel + appHealth = database.WorkspaceAppHealthDisabled + portUint, portUintErr = strconv.ParseUint(r.AppSlugOrPort, 10, 16) ) - if agentID, uuidErr := uuid.Parse(r.AgentNameOrID); uuidErr == nil { - var err error - agent, err = db.GetWorkspaceAgentByID(ctx, agentID) - if err != nil { - return nil, xerrors.Errorf("get workspace agent by ID %q: %w", agentID, err) + if portUintErr == nil { + if r.AccessMethod != AccessMethodSubdomain { + // TODO(@deansheather): this should return a 400 instead of a 500. + return nil, xerrors.New("port-based URLs are only supported for subdomain-based applications") } - // Verify that the agent belongs to the workspace. - - //nolint:gocritic // We need to fetch the agent to authenticate the request. This is a system function. - agentResource, err := db.GetWorkspaceResourceByID(ctx, agent.ResourceID) - if err != nil { - return nil, xerrors.Errorf("get agent workspace resource: %w", err) - } - build, err := db.GetWorkspaceBuildByJobID(ctx, agentResource.JobID) - if err != nil { - return nil, xerrors.Errorf("get workspace build by job ID %q: %w", agentResource.JobID, err) - } - if build.WorkspaceID != workspace.ID { - return nil, xerrors.Errorf("agent %q does not belong to workspace %q: %w", agent.ID, workspace.ID, sql.ErrNoRows) - } - } else { - build, err := db.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspace.ID) - if err != nil { - return nil, xerrors.Errorf("get latest workspace build by workspace ID %q: %w", workspace.ID, err) + // If the user specified a port, then they must specify the agent if + // there are multiple agents in the workspace. App names are unique per + // workspace. + if agentNameOrID == "" { + if len(agents) != 1 { + return nil, xerrors.New("port specified with no agent, but multiple agents exist in the workspace") + } + agentNameOrID = agents[0].ID.String() } - // nolint:gocritic // We need to fetch the agent to authenticate the request. This is a system function. - resources, err := db.GetWorkspaceResourcesByJobID(ctx, build.JobID) - if err != nil { - return nil, xerrors.Errorf("get workspace resources by job ID %q: %w", build.JobID, err) - } - resourcesIDs := []uuid.UUID{} - for _, resource := range resources { - resourcesIDs = append(resourcesIDs, resource.ID) - } + // If the app slug is a port number, then route to the port as an + // "anonymous app". We only support HTTP for port-based URLs. + // + // This is only supported for subdomain-based applications. + appURL = fmt.Sprintf("http://127.0.0.1:%d", portUint) + appSharingLevel = database.AppSharingLevelOwner + } else { + for _, app := range apps { + if app.Slug == r.AppSlugOrPort { + if !app.Url.Valid { + return nil, xerrors.Errorf("app URL is not valid") + } - // nolint:gocritic // We need to fetch the agent to authenticate the request. This is a system function. - agents, err := db.GetWorkspaceAgentsByResourceIDs(ctx, resourcesIDs) - if err != nil { - return nil, xerrors.Errorf("get workspace agents by resource IDs %v: %w", resourcesIDs, err) + agentNameOrID = app.AgentID.String() + if app.SharingLevel != "" { + appSharingLevel = app.SharingLevel + } else { + appSharingLevel = database.AppSharingLevelOwner + } + appURL = app.Url.String + appHealth = app.Health + break + } } + } + if appURL == "" { + return nil, xerrors.Errorf("no app found with slug %q: %w", r.AppSlugOrPort, sql.ErrNoRows) + } - if r.AgentNameOrID == "" { - if len(agents) != 1 { - return nil, xerrors.Errorf("no agent specified, but multiple exist in workspace") + // Finally, get agent. + var agent database.WorkspaceAgent + if agentID, uuidErr := uuid.Parse(agentNameOrID); uuidErr == nil { + for _, a := range agents { + if a.ID == agentID { + agent = a + break } - + } + } else { + if agentNameOrID == "" && len(agents) == 1 { agent = agents[0] } else { for _, a := range agents { - if a.Name == r.AgentNameOrID { + if a.Name == agentNameOrID { agent = a break } @@ -241,40 +276,6 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR } } - // Get app. - var ( - appSharingLevel = database.AppSharingLevelOwner - appURL string - appHealth database.WorkspaceAppHealth - ) - portUint, portUintErr := strconv.ParseUint(r.AppSlugOrPort, 10, 16) - if r.AccessMethod == AccessMethodSubdomain && portUintErr == nil { - // If the app slug is a port number, then route to the port as an - // "anonymous app". We only support HTTP for port-based URLs. - // - // This is only supported for subdomain-based applications. - appURL = fmt.Sprintf("http://127.0.0.1:%d", portUint) - } else { - app, err := db.GetWorkspaceAppByAgentIDAndSlug(ctx, database.GetWorkspaceAppByAgentIDAndSlugParams{ - AgentID: agent.ID, - Slug: r.AppSlugOrPort, - }) - if err != nil { - return nil, xerrors.Errorf("get workspace app by agent ID %q and slug %q: %w", agent.ID, r.AppSlugOrPort, err) - } - if !app.Url.Valid { - return nil, xerrors.Errorf("app URL is not valid") - } - - if app.SharingLevel != "" { - appSharingLevel = app.SharingLevel - } else { - appSharingLevel = database.AppSharingLevelOwner - } - appURL = app.Url.String - appHealth = app.Health - } - return &databaseRequest{ Request: r, User: user, diff --git a/coderd/workspaceapps_test.go b/coderd/workspaceapps_test.go index 6ae9a1465460a..24e4529a82664 100644 --- a/coderd/workspaceapps_test.go +++ b/coderd/workspaceapps_test.go @@ -513,7 +513,9 @@ func TestWorkspaceAppsProxyPath(t *testing.T) { resp, err := client.Request(ctx, http.MethodGet, fmt.Sprintf("/@%s/%s/apps/%d/", coderdtest.FirstUserParams.Username, workspace.Name, 8080), nil) require.NoError(t, err) defer resp.Body.Close() - require.Equal(t, http.StatusNotFound, resp.StatusCode) + // TODO(@deansheather): This should be 400. There's a todo in the + // resolve request code to fix this. + require.Equal(t, http.StatusInternalServerError, resp.StatusCode) }) }