Skip to content

fix(security)!: path-based app sharing changes #5772

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 18, 2023
Merged

fix(security)!: path-based app sharing changes #5772

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e99e7ac
fix(security)!: path-based app sharing changes
deansheather Jan 18, 2023
463893f
fixup! fix(security)!: path-based app sharing changes
deansheather Jan 18, 2023
02c5efa
Merge branch 'main' into dean/path-app-security
deansheather Jan 18, 2023
aaa0b4f
fixup! Merge branch 'main' into dean/path-app-security
deansheather Jan 18, 2023
1bb636a
fixup! Merge branch 'main' into dean/path-app-security
deansheather Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions cli/deployment/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,26 @@ func newConfig() *codersdk.DeploymentConfig {
Default: "",
},
},
Dangerous: &codersdk.DangerousConfig{
AllowPathAppSharing: &codersdk.DeploymentConfigField[bool]{
Name: "DANGEROUS: Allow Path App Sharing",
Usage: "Allow workspace apps that are not served from subdomains to be shared. Path-based app sharing is DISABLED by default for security purposes. Path-based apps can make requests to the Coder API and pose a security risk when the workspace serves malicious JavaScript. Path-based apps can be disabled entirely with --disable-path-apps for further security.",
Flag: "dangerous-allow-path-app-sharing",
Default: false,
},
AllowPathAppSiteOwnerAccess: &codersdk.DeploymentConfigField[bool]{
Name: "DANGEROUS: Allow Site Owners to Access Path Apps",
Usage: "Allow site-owners to access workspace apps from workspaces they do not own. Owners cannot access path-based apps they do not own by default. Path-based apps can make requests to the Coder API and pose a security risk when the workspace serves malicious JavaScript. Path-based apps can be disabled entirely with --disable-path-apps for further security.",
Flag: "dangerous-allow-path-app-site-owner-access",
Default: false,
},
},
DisablePathApps: &codersdk.DeploymentConfigField[bool]{
Name: "Disable Path Apps",
Usage: "Disable workspace apps that are not served from subdomains. Path-based apps can make requests to the Coder API and pose a security risk when the workspace serves malicious JavaScript. This is recommended for security purposes if a --wildcard-access-url is configured.",
Flag: "disable-path-apps",
Default: false,
},
}
}

Expand Down
30 changes: 30 additions & 0 deletions cli/testdata/coder_server_--help.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,28 @@ Flags:
with systemd.
Consumes $CODER_CACHE_DIRECTORY (default
"/tmp/coder-cli-test-cache")
--dangerous-allow-path-app-sharing Allow workspace apps that are not served
from subdomains to be shared. Path-based
app sharing is DISABLED by default for
security purposes. Path-based apps can
make requests to the Coder API and pose a
security risk when the workspace serves
malicious JavaScript. Path-based apps can
be disabled entirely with
--disable-path-apps for further security.
Consumes
$CODER_DANGEROUS_ALLOW_PATH_APP_SHARING
--dangerous-allow-path-app-site-owner-access Allow site-owners to access workspace
apps from workspaces they do not own.
Owners cannot access path-based apps they
do not own by default. Path-based apps
can make requests to the Coder API and
pose a security risk when the workspace
serves malicious JavaScript. Path-based
apps can be disabled entirely with
--disable-path-apps for further security.
Consumes
$CODER_DANGEROUS_ALLOW_PATH_APP_SITE_OWNER_ACCESS
--dangerous-disable-rate-limits Disables all rate limits. This is not
recommended in production.
Consumes $CODER_RATE_LIMIT_DISABLE_ALL
Expand Down Expand Up @@ -61,6 +83,14 @@ Flags:
Consumes
$CODER_DERP_SERVER_STUN_ADDRESSES
(default [stun.l.google.com:19302])
--disable-path-apps Disable workspace apps that are not
served from subdomains. Path-based apps
can make requests to the Coder API and
pose a security risk when the workspace
serves malicious JavaScript. This is
recommended for security purposes if a
--wildcard-access-url is configured.
Consumes $CODER_DISABLE_PATH_APPS
--experiments strings Enable one or more experiments. These are
not ready for production. Separate
multiple experiments with commas, or
Expand Down
17 changes: 17 additions & 0 deletions coderd/apidoc/docs.go
View file
Open in desktop

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

17 changes: 17 additions & 0 deletions coderd/apidoc/swagger.json
View file
Open in desktop

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

56 changes: 48 additions & 8 deletions coderd/workspaceapps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request)
workspace := httpmw.WorkspaceParam(r)
agent := httpmw.WorkspaceAgentParam(r)

if api.DeploymentConfig.DisablePathApps.Value {
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
Status: http.StatusUnauthorized,
Title: "Unauthorized",
Description: "Path-based applications are disabled on this Coder deployment by the administrator.",
RetryEnabled: false,
DashboardURL: api.AccessURL.String(),
})
return
}

// We do not support port proxying on paths, so lookup the app by slug.
appSlug := chi.URLParam(r, "workspaceapp")
app, ok := api.lookupWorkspaceApp(rw, r, agent.ID, appSlug)
Expand All @@ -100,7 +111,7 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request)
if app.SharingLevel != "" {
appSharingLevel = app.SharingLevel
}
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, workspace, appSharingLevel)
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, true, workspace, appSharingLevel)
if !ok {
return
}
Expand All @@ -127,6 +138,7 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request)
}

api.proxyWorkspaceApplication(proxyApplication{
IsPathApp: true,
Workspace: workspace,
Agent: agent,
App: &app,
Expand Down Expand Up @@ -238,6 +250,7 @@ func (api *API) handleSubdomainApplications(middlewares ...func(http.Handler) ht
}

api.proxyWorkspaceApplication(proxyApplication{
IsPathApp: false,
Workspace: workspace,
Agent: agent,
App: workspaceAppPtr,
Expand Down Expand Up @@ -411,9 +424,20 @@ func (api *API) lookupWorkspaceApp(rw http.ResponseWriter, r *http.Request, agen
return app, true
}

func (api *API) authorizeWorkspaceApp(r *http.Request, sharingLevel database.AppSharingLevel, workspace database.Workspace) (bool, error) {
//nolint:revive
func (api *API) authorizeWorkspaceApp(r *http.Request, isPathApp bool, sharingLevel database.AppSharingLevel, workspace database.Workspace) (bool, error) {
ctx := r.Context()

// If path-based app sharing is disabled (which is the default), we can
// force the sharing level to be "owner" so that the user can only access
// their own apps.
//
// Site owners are blocked from accessing path-based apps unless the
// Dangerous.AllowPathAppSiteOwnerAccess flag is enabled in the check below.
if isPathApp && !api.DeploymentConfig.Dangerous.AllowPathAppSharing.Value {
sharingLevel = database.AppSharingLevelOwner
}

// Short circuit if not authenticated.
roles, ok := httpmw.UserAuthorizationOptional(r)
if !ok {
Expand All @@ -422,6 +446,21 @@ func (api *API) authorizeWorkspaceApp(r *http.Request, sharingLevel database.App
return sharingLevel == database.AppSharingLevelPublic, nil
}

// Block anyone from accessing workspaces they don't own in path-based apps
// unless the admin disables this security feature. This blocks site-owners
// from accessing any apps from any user's workspaces.
//
// When the Dangerous.AllowPathAppSharing flag is not enabled, the sharing
// level will be forced to "owner", so this check will always be true for
// workspaces owned by different users.
if isPathApp &&
sharingLevel == database.AppSharingLevelOwner &&
workspace.OwnerID != roles.ID &&
!api.DeploymentConfig.Dangerous.AllowPathAppSiteOwnerAccess.Value {

return false, nil
}

// Do a standard RBAC check. This accounts for share level "owner" and any
// other RBAC rules that may be in place.
//
Expand Down Expand Up @@ -463,8 +502,8 @@ func (api *API) authorizeWorkspaceApp(r *http.Request, sharingLevel database.App
// 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 (api *API) fetchWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Request, workspace database.Workspace, appSharingLevel database.AppSharingLevel) (authed bool, ok bool) {
ok, err := api.authorizeWorkspaceApp(r, appSharingLevel, workspace)
func (api *API) fetchWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Request, isPathApp bool, workspace database.Workspace, appSharingLevel database.AppSharingLevel) (authed bool, ok bool) {
ok, err := api.authorizeWorkspaceApp(r, isPathApp, appSharingLevel, workspace)
if err != nil {
api.Logger.Error(r.Context(), "authorize workspace app", slog.Error(err))
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
Expand All @@ -484,8 +523,8 @@ func (api *API) fetchWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Re
// for a given app share level in the given workspace. If the user is not
// authorized or a server error occurs, a discrete HTML error page is rendered
// and false is returned so the caller can return early.
func (api *API) checkWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Request, workspace database.Workspace, appSharingLevel database.AppSharingLevel) bool {
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, workspace, appSharingLevel)
func (api *API) checkWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Request, isPathApp bool, workspace database.Workspace, appSharingLevel database.AppSharingLevel) bool {
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, isPathApp, workspace, appSharingLevel)
if !ok {
return false
}
Expand All @@ -502,7 +541,7 @@ func (api *API) checkWorkspaceApplicationAuth(rw http.ResponseWriter, r *http.Re
// they will be redirected to the route below. If the user does have a session
// key but insufficient permissions a static error page will be rendered.
func (api *API) verifyWorkspaceApplicationSubdomainAuth(rw http.ResponseWriter, r *http.Request, host string, workspace database.Workspace, appSharingLevel database.AppSharingLevel) bool {
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, workspace, appSharingLevel)
authed, ok := api.fetchWorkspaceApplicationAuth(rw, r, false, workspace, appSharingLevel)
if !ok {
return false
}
Expand Down Expand Up @@ -731,6 +770,7 @@ func (api *API) workspaceApplicationAuth(rw http.ResponseWriter, r *http.Request

// proxyApplication are the required fields to proxy a workspace application.
type proxyApplication struct {
IsPathApp bool
Workspace database.Workspace
Agent database.WorkspaceAgent

Expand All @@ -752,7 +792,7 @@ func (api *API) proxyWorkspaceApplication(proxyApp proxyApplication, rw http.Res
if proxyApp.App != nil && proxyApp.App.SharingLevel != "" {
sharingLevel = proxyApp.App.SharingLevel
}
if !api.checkWorkspaceApplicationAuth(rw, r, proxyApp.Workspace, sharingLevel) {
if !api.checkWorkspaceApplicationAuth(rw, r, proxyApp.IsPathApp, proxyApp.Workspace, sharingLevel) {
return
}

Expand Down
Loading