fix(api): add an authenticated access policy to the websocket endpoint (#1979)

* fix(api): add an authenticated access policy to the websocket endpoint

* refactor(api): centralize EndpointAccess validation

* feat(api): validate id query parameter for the /websocket/exec endpoint

api/http/handler/endpointproxy/handler.go

@@ -11,16 +11,16 @@ import (
 // Handler is the HTTP handler used to proxy requests to external APIs.
 type Handler struct {
 	*mux.Router
-	EndpointService       portainer.EndpointService
+	requestBouncer  *security.RequestBouncer
-	EndpointGroupService  portainer.EndpointGroupService
+	EndpointService portainer.EndpointService
-	TeamMembershipService portainer.TeamMembershipService
+	ProxyManager    *proxy.Manager
-	ProxyManager          *proxy.Manager
 }
 
 // NewHandler creates a handler to proxy requests to external APIs.
 func NewHandler(bouncer *security.RequestBouncer) *Handler {
 	h := &Handler{
-		Router: mux.NewRouter(),
+		Router:         mux.NewRouter(),
+		requestBouncer: bouncer,
 	}
 	h.PathPrefix("/{id}/azure").Handler(
 		bouncer.AuthenticatedAccess(httperror.LoggerHandler(h.proxyRequestsToAzureAPI)))
@@ -30,21 +30,3 @@ func NewHandler(bouncer *security.RequestBouncer) *Handler {
 		bouncer.AuthenticatedAccess(httperror.LoggerHandler(h.proxyRequestsToStoridgeAPI)))
 	return h
 }
-
-func (handler *Handler) checkEndpointAccess(endpoint *portainer.Endpoint, userID portainer.UserID) error {
-	memberships, err := handler.TeamMembershipService.TeamMembershipsByUserID(userID)
-	if err != nil {
-		return err
-	}
-
-	group, err := handler.EndpointGroupService.EndpointGroup(endpoint.GroupID)
-	if err != nil {
-		return err
-	}
-
-	if !security.AuthorizedEndpointAccess(endpoint, group, userID, memberships) {
-		return portainer.ErrEndpointAccessDenied
-	}
-
-	return nil
-}

api/http/handler/endpointproxy/proxy_azure.go

@@ -6,7 +6,6 @@ import (
 	"github.com/portainer/portainer"
 	httperror "github.com/portainer/portainer/http/error"
 	"github.com/portainer/portainer/http/request"
-	"github.com/portainer/portainer/http/security"
 
 	"net/http"
 )
@@ -24,18 +23,9 @@ func (handler *Handler) proxyRequestsToAzureAPI(w http.ResponseWriter, r *http.R
 		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to find an endpoint with the specified identifier inside the database", err}
 	}
 
-	tokenData, err := security.RetrieveTokenData(r)
+	err = handler.requestBouncer.EndpointAccess(r, endpoint)
 	if err != nil {
-		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to retrieve user authentication token", err}
+		return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-	}
-
-	if tokenData.Role != portainer.AdministratorRole {
-		err = handler.checkEndpointAccess(endpoint, tokenData.ID)
-		if err != nil && err == portainer.ErrEndpointAccessDenied {
-			return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-		} else if err != nil {
-			return &httperror.HandlerError{http.StatusInternalServerError, "Unable to verify permission to access endpoint", err}
-		}
 	}
 
 	var proxy http.Handler

api/http/handler/endpointproxy/proxy_docker.go

@@ -6,7 +6,6 @@ import (
 	"github.com/portainer/portainer"
 	httperror "github.com/portainer/portainer/http/error"
 	"github.com/portainer/portainer/http/request"
-	"github.com/portainer/portainer/http/security"
 
 	"net/http"
 )
@@ -24,18 +23,9 @@ func (handler *Handler) proxyRequestsToDockerAPI(w http.ResponseWriter, r *http.
 		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to find an endpoint with the specified identifier inside the database", err}
 	}
 
-	tokenData, err := security.RetrieveTokenData(r)
+	err = handler.requestBouncer.EndpointAccess(r, endpoint)
 	if err != nil {
-		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to retrieve user authentication token", err}
+		return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-	}
-
-	if tokenData.Role != portainer.AdministratorRole {
-		err = handler.checkEndpointAccess(endpoint, tokenData.ID)
-		if err != nil && err == portainer.ErrEndpointAccessDenied {
-			return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-		} else if err != nil {
-			return &httperror.HandlerError{http.StatusInternalServerError, "Unable to verify permission to access endpoint", err}
-		}
 	}
 
 	var proxy http.Handler

api/http/handler/endpointproxy/proxy_storidge.go

@@ -6,7 +6,6 @@ import (
 	"github.com/portainer/portainer"
 	httperror "github.com/portainer/portainer/http/error"
 	"github.com/portainer/portainer/http/request"
-	"github.com/portainer/portainer/http/security"
 
 	"net/http"
 )
@@ -24,18 +23,9 @@ func (handler *Handler) proxyRequestsToStoridgeAPI(w http.ResponseWriter, r *htt
 		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to find an endpoint with the specified identifier inside the database", err}
 	}
 
-	tokenData, err := security.RetrieveTokenData(r)
+	err = handler.requestBouncer.EndpointAccess(r, endpoint)
 	if err != nil {
-		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to retrieve user authentication token", err}
+		return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-	}
-
-	if tokenData.Role != portainer.AdministratorRole {
-		err = handler.checkEndpointAccess(endpoint, tokenData.ID)
-		if err != nil && err == portainer.ErrEndpointAccessDenied {
-			return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-		} else if err != nil {
-			return &httperror.HandlerError{http.StatusInternalServerError, "Unable to verify permission to access endpoint", err}
-		}
 	}
 
 	var storidgeExtension *portainer.EndpointExtension

api/http/handler/stacks/handler.go

@@ -14,13 +14,12 @@ import (
 type Handler struct {
 	stackCreationMutex *sync.Mutex
 	stackDeletionMutex *sync.Mutex
+	requestBouncer     *security.RequestBouncer
 	*mux.Router
 	FileService            portainer.FileService
 	GitService             portainer.GitService
 	StackService           portainer.StackService
 	EndpointService        portainer.EndpointService
-	EndpointGroupService   portainer.EndpointGroupService
-	TeamMembershipService  portainer.TeamMembershipService
 	ResourceControlService portainer.ResourceControlService
 	RegistryService        portainer.RegistryService
 	DockerHubService       portainer.DockerHubService
@@ -34,6 +33,7 @@ func NewHandler(bouncer *security.RequestBouncer) *Handler {
 		Router:             mux.NewRouter(),
 		stackCreationMutex: &sync.Mutex{},
 		stackDeletionMutex: &sync.Mutex{},
+		requestBouncer:     bouncer,
 	}
 	h.Handle("/stacks",
 		bouncer.RestrictedAccess(httperror.LoggerHandler(h.stackCreate))).Methods(http.MethodPost)
@@ -49,21 +49,3 @@ func NewHandler(bouncer *security.RequestBouncer) *Handler {
 		bouncer.RestrictedAccess(httperror.LoggerHandler(h.stackFile))).Methods(http.MethodGet)
 	return h
 }
-
-func (handler *Handler) checkEndpointAccess(endpoint *portainer.Endpoint, userID portainer.UserID) error {
-	memberships, err := handler.TeamMembershipService.TeamMembershipsByUserID(userID)
-	if err != nil {
-		return err
-	}
-
-	group, err := handler.EndpointGroupService.EndpointGroup(endpoint.GroupID)
-	if err != nil {
-		return err
-	}
-
-	if !security.AuthorizedEndpointAccess(endpoint, group, userID, memberships) {
-		return portainer.ErrEndpointAccessDenied
-	}
-
-	return nil
-}

api/http/handler/stacks/stack_create.go

@@ -7,7 +7,6 @@ import (
 	"github.com/portainer/portainer"
 	httperror "github.com/portainer/portainer/http/error"
 	"github.com/portainer/portainer/http/request"
-	"github.com/portainer/portainer/http/security"
 )
 
 func (handler *Handler) cleanUp(stack *portainer.Stack, doCleanUp *bool) error {
@@ -47,18 +46,9 @@ func (handler *Handler) stackCreate(w http.ResponseWriter, r *http.Request) *htt
 		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to find an endpoint with the specified identifier inside the database", err}
 	}
 
-	tokenData, err := security.RetrieveTokenData(r)
+	err = handler.requestBouncer.EndpointAccess(r, endpoint)
 	if err != nil {
-		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to retrieve user authentication token", err}
+		return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-	}
-
-	if tokenData.Role != portainer.AdministratorRole {
-		err = handler.checkEndpointAccess(endpoint, tokenData.ID)
-		if err != nil && err == portainer.ErrEndpointAccessDenied {
-			return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-		} else if err != nil {
-			return &httperror.HandlerError{http.StatusInternalServerError, "Unable to verify permission to access endpoint", err}
-		}
 	}
 
 	switch portainer.StackType(stackType) {

api/http/handler/stacks/stack_delete.go

@@ -105,18 +105,9 @@ func (handler *Handler) deleteExternalStack(r *http.Request, w http.ResponseWrit
 		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to find the endpoint associated to the stack inside the database", err}
 	}
 
-	tokenData, err := security.RetrieveTokenData(r)
+	err = handler.requestBouncer.EndpointAccess(r, endpoint)
 	if err != nil {
-		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to retrieve user authentication token", err}
+		return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-	}
-
-	if tokenData.Role != portainer.AdministratorRole {
-		err = handler.checkEndpointAccess(endpoint, tokenData.ID)
-		if err != nil && err == portainer.ErrEndpointAccessDenied {
-			return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
-		} else if err != nil {
-			return &httperror.HandlerError{http.StatusInternalServerError, "Unable to verify permission to access endpoint", err}
-		}
 	}
 
 	stack = &portainer.Stack{

api/http/handler/websocket/handler.go

@@ -1,12 +1,11 @@
 package websocket
 
 import (
-	"net/http"
-
 	"github.com/gorilla/mux"
 	"github.com/gorilla/websocket"
 	"github.com/portainer/portainer"
 	httperror "github.com/portainer/portainer/http/error"
+	"github.com/portainer/portainer/http/security"
 )
 
 // Handler is the HTTP handler used to handle websocket operations.
@@ -14,15 +13,18 @@ type Handler struct {
 	*mux.Router
 	EndpointService    portainer.EndpointService
 	SignatureService   portainer.DigitalSignatureService
+	requestBouncer     *security.RequestBouncer
 	connectionUpgrader websocket.Upgrader
 }
 
 // NewHandler creates a handler to manage websocket operations.
-func NewHandler() *Handler {
+func NewHandler(bouncer *security.RequestBouncer) *Handler {
 	h := &Handler{
 		Router:             mux.NewRouter(),
 		connectionUpgrader: websocket.Upgrader{},
+		requestBouncer:     bouncer,
 	}
-	h.Handle("/websocket/exec", httperror.LoggerHandler(h.websocketExec)).Methods(http.MethodGet)
+	h.PathPrefix("/websocket/exec").Handler(
+		bouncer.AuthenticatedAccess(httperror.LoggerHandler(h.websocketExec)))
 	return h
 }

api/http/handler/websocket/websocket_exec.go

@@ -12,6 +12,7 @@ import (
 	"net/url"
 	"time"
 
+	"github.com/asaskevich/govalidator"
 	"github.com/gorilla/websocket"
 	"github.com/koding/websocketproxy"
 	"github.com/portainer/portainer"
@@ -31,15 +32,19 @@ type execStartOperationPayload struct {
 	Detach bool
 }
 
-// websocketExec handles GET requests on /websocket/exec?id=<execID>&endpointId=<endpointID>&nodeName=<nodeName>
+// websocketExec handles GET requests on /websocket/exec?id=<execID>&endpointId=<endpointID>&nodeName=<nodeName>&token=<token>
 // If the nodeName query parameter is present, the request will be proxied to the underlying agent endpoint.
 // If the nodeName query parameter is not specified, the request will be upgraded to the websocket protocol and
 // an ExecStart operation HTTP request will be created and hijacked.
+// Authentication and access is controled via the mandatory token query parameter.
 func (handler *Handler) websocketExec(w http.ResponseWriter, r *http.Request) *httperror.HandlerError {
 	execID, err := request.RetrieveQueryParameter(r, "id", false)
 	if err != nil {
 		return &httperror.HandlerError{http.StatusBadRequest, "Invalid query parameter: id", err}
 	}
+	if !govalidator.IsHexadecimal(execID) {
+		return &httperror.HandlerError{http.StatusBadRequest, "Invalid query parameter: id (must be hexadecimal identifier)", err}
+	}
 
 	endpointID, err := request.RetrieveNumericQueryParameter(r, "endpointId", false)
 	if err != nil {
@@ -53,6 +58,11 @@ func (handler *Handler) websocketExec(w http.ResponseWriter, r *http.Request) *h
 		return &httperror.HandlerError{http.StatusInternalServerError, "Unable to find the endpoint associated to the stack inside the database", err}
 	}
 
+	err = handler.requestBouncer.EndpointAccess(r, endpoint)
+	if err != nil {
+		return &httperror.HandlerError{http.StatusForbidden, "Permission denied to access endpoint", portainer.ErrEndpointAccessDenied}
+	}
+
 	params := &webSocketExecRequestParams{
 		endpoint: endpoint,
 		execID:   execID,

api/http/security/authorization.go

@@ -166,10 +166,10 @@ func AuthorizedUserManagement(userID portainer.UserID, context *RestrictedReques
 	return false
 }
 
-// AuthorizedEndpointAccess ensure that the user can access the specified endpoint.
+// authorizedEndpointAccess ensure that the user can access the specified endpoint.
 // It will check if the user is part of the authorized users or part of a team that is
 // listed in the authorized teams of the endpoint and the associated group.
-func AuthorizedEndpointAccess(endpoint *portainer.Endpoint, endpointGroup *portainer.EndpointGroup, userID portainer.UserID, memberships []portainer.TeamMembership) bool {
+func authorizedEndpointAccess(endpoint *portainer.Endpoint, endpointGroup *portainer.EndpointGroup, userID portainer.UserID, memberships []portainer.TeamMembership) bool {
 	groupAccess := authorizedAccess(userID, memberships, endpointGroup.AuthorizedUsers, endpointGroup.AuthorizedTeams)
 	if !groupAccess {
 		return authorizedAccess(userID, memberships, endpoint.AuthorizedUsers, endpoint.AuthorizedTeams)

api/http/security/bouncer.go

@@ -14,9 +14,19 @@ type (
 		jwtService            portainer.JWTService
 		userService           portainer.UserService
 		teamMembershipService portainer.TeamMembershipService
+		endpointGroupService  portainer.EndpointGroupService
 		authDisabled          bool
 	}
 
+	// RequestBouncerParams represents the required parameters to create a new RequestBouncer instance.
+	RequestBouncerParams struct {
+		JWTService            portainer.JWTService
+		UserService           portainer.UserService
+		TeamMembershipService portainer.TeamMembershipService
+		EndpointGroupService  portainer.EndpointGroupService
+		AuthDisabled          bool
+	}
+
 	// RestrictedRequestContext is a data structure containing information
 	// used in RestrictedAccess
 	RestrictedRequestContext struct {
@@ -28,12 +38,13 @@ type (
 )
 
 // NewRequestBouncer initializes a new RequestBouncer
-func NewRequestBouncer(jwtService portainer.JWTService, userService portainer.UserService, teamMembershipService portainer.TeamMembershipService, authDisabled bool) *RequestBouncer {
+func NewRequestBouncer(parameters *RequestBouncerParams) *RequestBouncer {
 	return &RequestBouncer{
-		jwtService:            jwtService,
+		jwtService:            parameters.JWTService,
-		userService:           userService,
+		userService:           parameters.UserService,
-		teamMembershipService: teamMembershipService,
+		teamMembershipService: parameters.TeamMembershipService,
-		authDisabled:          authDisabled,
+		endpointGroupService:  parameters.EndpointGroupService,
+		authDisabled:          parameters.AuthDisabled,
 	}
 }
 
@@ -70,6 +81,36 @@ func (bouncer *RequestBouncer) AdministratorAccess(h http.Handler) http.Handler
 	return h
 }
 
+// EndpointAccess retrieves the JWT token from the request context and verifies
+// that the user can access the specified endpoint.
+// An error is returned when access is denied.
+func (bouncer *RequestBouncer) EndpointAccess(r *http.Request, endpoint *portainer.Endpoint) error {
+	tokenData, err := RetrieveTokenData(r)
+	if err != nil {
+		return err
+	}
+
+	if tokenData.Role == portainer.AdministratorRole {
+		return nil
+	}
+
+	memberships, err := bouncer.teamMembershipService.TeamMembershipsByUserID(tokenData.ID)
+	if err != nil {
+		return err
+	}
+
+	group, err := bouncer.endpointGroupService.EndpointGroup(endpoint.GroupID)
+	if err != nil {
+		return err
+	}
+
+	if !authorizedEndpointAccess(endpoint, group, tokenData.ID, memberships) {
+		return portainer.ErrEndpointAccessDenied
+	}
+
+	return nil
+}
+
 // mwSecureHeaders provides secure headers middleware for handlers.
 func mwSecureHeaders(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -120,6 +161,10 @@ func (bouncer *RequestBouncer) mwCheckAuthentication(next http.Handler) http.Han
 		if !bouncer.authDisabled {
 			var token string
 
+			// Optionally, token might be set via the "token" query parameter.
+			// For example, in websocket requests
+			token = r.URL.Query().Get("token")
+
 			// Get token from the Authorization header
 			tokens, ok := r.Header["Authorization"]
 			if ok && len(tokens) >= 1 {

api/http/security/filter.go

@@ -88,7 +88,7 @@ func FilterEndpoints(endpoints []portainer.Endpoint, groups []portainer.Endpoint
 		for _, endpoint := range endpoints {
 			endpointGroup := getAssociatedGroup(&endpoint, groups)
 
-			if AuthorizedEndpointAccess(&endpoint, endpointGroup, context.UserID, context.UserMemberships) {
+			if authorizedEndpointAccess(&endpoint, endpointGroup, context.UserID, context.UserMemberships) {
 				filteredEndpoints = append(filteredEndpoints, endpoint)
 			}
 		}

api/http/server.go

@@ -64,7 +64,14 @@ type Server struct {
 
 // Start starts the HTTP server
 func (server *Server) Start() error {
-	requestBouncer := security.NewRequestBouncer(server.JWTService, server.UserService, server.TeamMembershipService, server.AuthDisabled)
+	requestBouncerParameters := &security.RequestBouncerParams{
+		JWTService:            server.JWTService,
+		UserService:           server.UserService,
+		TeamMembershipService: server.TeamMembershipService,
+		EndpointGroupService:  server.EndpointGroupService,
+		AuthDisabled:          server.AuthDisabled,
+	}
+	requestBouncer := security.NewRequestBouncer(requestBouncerParameters)
 	proxyManagerParameters := &proxy.ManagerParams{
 		ResourceControlService: server.ResourceControlService,
 		TeamMembershipService:  server.TeamMembershipService,
@@ -98,8 +105,6 @@ func (server *Server) Start() error {
 
 	var endpointProxyHandler = endpointproxy.NewHandler(requestBouncer)
 	endpointProxyHandler.EndpointService = server.EndpointService
-	endpointProxyHandler.EndpointGroupService = server.EndpointGroupService
-	endpointProxyHandler.TeamMembershipService = server.TeamMembershipService
 	endpointProxyHandler.ProxyManager = proxyManager
 
 	var fileHandler = file.NewHandler(filepath.Join(server.AssetsPath, "public"))
@@ -119,8 +124,6 @@ func (server *Server) Start() error {
 	stackHandler.FileService = server.FileService
 	stackHandler.StackService = server.StackService
 	stackHandler.EndpointService = server.EndpointService
-	stackHandler.EndpointGroupService = server.EndpointGroupService
-	stackHandler.TeamMembershipService = server.TeamMembershipService
 	stackHandler.ResourceControlService = server.ResourceControlService
 	stackHandler.SwarmStackManager = server.SwarmStackManager
 	stackHandler.ComposeStackManager = server.ComposeStackManager
@@ -153,7 +156,7 @@ func (server *Server) Start() error {
 	userHandler.ResourceControlService = server.ResourceControlService
 	userHandler.SettingsService = server.SettingsService
 
-	var websocketHandler = websocket.NewHandler()
+	var websocketHandler = websocket.NewHandler(requestBouncer)
 	websocketHandler.EndpointService = server.EndpointService
 	websocketHandler.SignatureService = server.SignatureService
 

app/docker/views/containers/console/containerConsoleController.js

@@ -1,6 +1,6 @@
 angular.module('portainer.docker')
-.controller('ContainerConsoleController', ['$scope', '$transition$', 'ContainerService', 'ImageService', 'EndpointProvider', 'Notifications', 'ContainerHelper', 'ExecService', 'HttpRequestHelper',
+.controller('ContainerConsoleController', ['$scope', '$transition$', 'ContainerService', 'ImageService', 'EndpointProvider', 'Notifications', 'ContainerHelper', 'ExecService', 'HttpRequestHelper', 'LocalStorage',
-function ($scope, $transition$, ContainerService, ImageService, EndpointProvider, Notifications, ContainerHelper, ExecService, HttpRequestHelper) {
+function ($scope, $transition$, ContainerService, ImageService, EndpointProvider, Notifications, ContainerHelper, ExecService, HttpRequestHelper, LocalStorage) {
   var socket, term;
 
   $scope.state = {
@@ -36,7 +36,8 @@ function ($scope, $transition$, ContainerService, ImageService, EndpointProvider
     ContainerService.createExec(execConfig)
     .then(function success(data) {
       execId = data.Id;
-      var url = window.location.href.split('#')[0] + 'api/websocket/exec?id=' + execId + '&endpointId=' + EndpointProvider.endpointID();
+      var jwtToken = LocalStorage.getJWT();
+      var url = window.location.href.split('#')[0] + 'api/websocket/exec?id=' + execId + '&endpointId=' + EndpointProvider.endpointID() + '&token=' + jwtToken;
       if ($transition$.params().nodeName) {
         url += '&nodeName=' + $transition$.params().nodeName;
       }