RBAC: Cover plugin routes (#80578)

* RBAC: Cover plugin routes * Action instead of ReqAction * Fix test initializations * Fix NewPluginProxy call * Duplicate test to add RBAC checks * Cover legacy access control as well * Fix typo * action -> reqAction * Add example Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com> --------- Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
1 year ago · 6b954165c5
parent 81a49e8016
commit 6b954165c5
6 changed files with 156 additions and 11 deletions
--- a/pkg/api/plugin_proxy.go
+++ b/pkg/api/plugin_proxy.go
@ -51,7 +51,7 @@ func (hs *HTTPServer) ProxyPluginRequest(c *contextmodel.ReqContext) {
 	}

 	proxyPath := getProxyPath(c)
-	p, err := pluginproxy.NewPluginProxy(ps, plugin.Routes, c, proxyPath, hs.Cfg, hs.SecretsService, hs.tracer, pluginProxyTransport, hs.Features)
+	p, err := pluginproxy.NewPluginProxy(ps, plugin.Routes, c, proxyPath, hs.Cfg, hs.SecretsService, hs.tracer, pluginProxyTransport, hs.AccessControl, hs.Features)
 	if err != nil {
 		c.JsonApiErr(http.StatusInternalServerError, "Failed to create plugin proxy", err)
 		return
--- a/pkg/api/pluginproxy/pluginproxy.go
+++ b/pkg/api/pluginproxy/pluginproxy.go
@ -11,6 +11,7 @@ import (

 	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/plugins"
+	ac "github.com/grafana/grafana/pkg/services/accesscontrol"
 	contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/pluginsintegration/pluginsettings"
@ -22,6 +23,7 @@ import (
 )

 type PluginProxy struct {
+	accessControl  ac.AccessControl
 	ps             *pluginsettings.DTO
 	pluginRoutes   []*plugins.Route
 	ctx            *contextmodel.ReqContext
@ -37,8 +39,9 @@ type PluginProxy struct {
 // NewPluginProxy creates a plugin proxy.
 func NewPluginProxy(ps *pluginsettings.DTO, routes []*plugins.Route, ctx *contextmodel.ReqContext,
 	proxyPath string, cfg *setting.Cfg, secretsService secrets.Service, tracer tracing.Tracer,
-	transport *http.Transport, features featuremgmt.FeatureToggles) (*PluginProxy, error) {
+	transport *http.Transport, accessControl ac.AccessControl, features featuremgmt.FeatureToggles) (*PluginProxy, error) {
 	return &PluginProxy{
+		accessControl:  accessControl,
 		ps:             ps,
 		pluginRoutes:   routes,
 		ctx:            ctx,
@ -67,11 +70,9 @@ func (proxy *PluginProxy) HandleRequest() {
 			continue
 		}

-		if route.ReqRole.IsValid() {
-			if !proxy.ctx.HasUserRole(route.ReqRole) {
-				proxy.ctx.JsonApiErr(http.StatusForbidden, "plugin proxy route access denied", nil)
-				return
-			}
+		if !proxy.hasAccessToRoute(route) {
+			proxy.ctx.JsonApiErr(http.StatusForbidden, "plugin proxy route access denied", nil)
+			return
 		}

 		if path, exists := params["*"]; exists {
@ -120,6 +121,21 @@ func (proxy *PluginProxy) HandleRequest() {
 	reverseProxy.ServeHTTP(proxy.ctx.Resp, proxy.ctx.Req)
 }

+func (proxy *PluginProxy) hasAccessToRoute(route *plugins.Route) bool {
+	useRBAC := proxy.features.IsEnabled(proxy.ctx.Req.Context(), featuremgmt.FlagAccessControlOnCall) && route.RequiresRBACAction()
+	if useRBAC {
+		hasAccess := ac.HasAccess(proxy.accessControl, proxy.ctx)(ac.EvalPermission(route.ReqAction))
+		if !hasAccess {
+			proxy.ctx.Logger.Debug("plugin route is covered by RBAC, user doesn't have access", "route", proxy.ctx.Req.URL.Path)
+		}
+		return hasAccess
+	}
+	if route.ReqRole.IsValid() {
+		return proxy.ctx.HasUserRole(route.ReqRole)
+	}
+	return true
+}
+
 func (proxy PluginProxy) director(req *http.Request) {
 	secureJsonData, err := proxy.secretsService.DecryptJsonData(proxy.ctx.Req.Context(), proxy.ps.SecureJSONData)
 	if err != nil {
--- a/pkg/api/pluginproxy/pluginproxy_test.go
+++ b/pkg/api/pluginproxy/pluginproxy_test.go
@ -14,6 +14,7 @@ import (

 	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/plugins"
+	"github.com/grafana/grafana/pkg/services/accesscontrol/acimpl"
 	contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/org"
@ -262,7 +263,8 @@ func TestPluginProxy(t *testing.T) {
 		ps := &pluginsettings.DTO{
 			SecureJSONData: map[string][]byte{},
 		}
-		proxy, err := NewPluginProxy(ps, routes, ctx, "", &setting.Cfg{}, secretsService, tracing.InitializeTracerForTest(), &http.Transport{}, featuremgmt.WithFeatures())
+		cfg := &setting.Cfg{}
+		proxy, err := NewPluginProxy(ps, routes, ctx, "", cfg, secretsService, tracing.InitializeTracerForTest(), &http.Transport{}, acimpl.ProvideAccessControl(cfg), featuremgmt.WithFeatures())
 		require.NoError(t, err)
 		proxy.HandleRequest()

@ -400,7 +402,8 @@ func TestPluginProxyRoutes(t *testing.T) {
 			ps := &pluginsettings.DTO{
 				SecureJSONData: map[string][]byte{},
 			}
-			proxy, err := NewPluginProxy(ps, testRoutes, ctx, tc.proxyPath, &setting.Cfg{}, secretsService, tracing.InitializeTracerForTest(), &http.Transport{}, featuremgmt.WithFeatures())
+			cfg := &setting.Cfg{}
+			proxy, err := NewPluginProxy(ps, testRoutes, ctx, tc.proxyPath, cfg, secretsService, tracing.InitializeTracerForTest(), &http.Transport{}, acimpl.ProvideAccessControl(cfg), featuremgmt.WithFeatures())
 			require.NoError(t, err)
 			proxy.HandleRequest()

@ -421,6 +424,121 @@ func TestPluginProxyRoutes(t *testing.T) {
 	}
 }

+func TestPluginProxyRoutesAccessControl(t *testing.T) {
+	routes := []*plugins.Route{
+		{
+			Path:    "settings",
+			Method:  "GET",
+			URL:     "http://localhost/api/settings",
+			ReqRole: org.RoleAdmin, // Protected by role
+		},
+		{
+			Path:      "projects",
+			Method:    "GET",
+			URL:       "http://localhost/api/projects",
+			ReqAction: "plugin-id.projects:read", // Protected by RBAC action
+		},
+	}
+
+	tcs := []struct {
+		proxyPath       string
+		usrRole         org.RoleType
+		usrPerms        map[string][]string
+		expectedURLPath string
+		expectedStatus  int
+	}{
+		{
+			proxyPath:       "/settings",
+			usrRole:         org.RoleAdmin,
+			expectedURLPath: "/api/settings",
+			expectedStatus:  http.StatusOK,
+		},
+		{
+			proxyPath:       "/settings",
+			usrRole:         org.RoleViewer,
+			expectedURLPath: "/api/settings",
+			expectedStatus:  http.StatusForbidden,
+		},
+		{
+			proxyPath:       "/projects",
+			usrPerms:        map[string][]string{"plugin-id.projects:read": {}},
+			expectedURLPath: "/api/projects",
+			expectedStatus:  http.StatusOK,
+		},
+		{
+			proxyPath:       "/projects",
+			usrPerms:        map[string][]string{},
+			expectedURLPath: "/api/projects",
+			expectedStatus:  http.StatusForbidden,
+		},
+	}
+
+	for _, tc := range tcs {
+		t.Run(fmt.Sprintf("Should enforce RBAC when proxying path %s %s", tc.proxyPath, http.StatusText(tc.expectedStatus)), func(t *testing.T) {
+			secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore())
+			requestHandled := false
+			requestURL := ""
+			backendServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				requestURL = r.URL.RequestURI()
+				w.WriteHeader(200)
+				_, _ = w.Write([]byte("I am the backend"))
+				requestHandled = true
+			}))
+			t.Cleanup(backendServer.Close)
+
+			backendURL, err := url.Parse(backendServer.URL)
+			require.NoError(t, err)
+
+			testRoutes := make([]*plugins.Route, len(routes))
+			for i, r := range routes {
+				u, err := url.Parse(r.URL)
+				require.NoError(t, err)
+				u.Scheme = backendURL.Scheme
+				u.Host = backendURL.Host
+				testRoute := *r
+				testRoute.URL = u.String()
+				testRoutes[i] = &testRoute
+			}
+
+			responseWriter := web.NewResponseWriter("GET", httptest.NewRecorder())
+
+			ctx := &contextmodel.ReqContext{
+				Logger: logger.New("pluginproxy-test"),
+				SignedInUser: &user.SignedInUser{
+					OrgID:       1,
+					OrgRole:     tc.usrRole,
+					Permissions: map[int64]map[string][]string{1: tc.usrPerms},
+				},
+				Context: &web.Context{
+					Req:  httptest.NewRequest("GET", tc.proxyPath, nil),
+					Resp: responseWriter,
+				},
+			}
+			ps := &pluginsettings.DTO{
+				SecureJSONData: map[string][]byte{},
+			}
+			cfg := &setting.Cfg{}
+			proxy, err := NewPluginProxy(ps, testRoutes, ctx, tc.proxyPath, cfg, secretsService, tracing.InitializeTracerForTest(), &http.Transport{}, acimpl.ProvideAccessControl(cfg), featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall))
+			require.NoError(t, err)
+			proxy.HandleRequest()
+
+			for {
+				if requestHandled || ctx.Resp.Written() {
+					break
+				}
+			}
+
+			require.Equal(t, tc.expectedStatus, ctx.Resp.Status())
+
+			if tc.expectedStatus == http.StatusForbidden {
+				return
+			}
+
+			require.Equal(t, tc.expectedURLPath, requestURL)
+		})
+	}
+}
+
 // getPluginProxiedRequest is a helper for easier setup of tests based on global config and ReqContext.
 func getPluginProxiedRequest(t *testing.T, ps *pluginsettings.DTO, secretsService secrets.Service, ctx *contextmodel.ReqContext, cfg *setting.Cfg, route *plugins.Route) *http.Request {
 	// insert dummy route if none is specified
@ -431,7 +549,7 @@ func getPluginProxiedRequest(t *testing.T, ps *pluginsettings.DTO, secretsServic
 			ReqRole: org.RoleEditor,
 		}
 	}
-	proxy, err := NewPluginProxy(ps, []*plugins.Route{}, ctx, "", cfg, secretsService, tracing.InitializeTracerForTest(), &http.Transport{}, featuremgmt.WithFeatures())
+	proxy, err := NewPluginProxy(ps, []*plugins.Route{}, ctx, "", cfg, secretsService, tracing.InitializeTracerForTest(), &http.Transport{}, acimpl.ProvideAccessControl(cfg), featuremgmt.WithFeatures())
 	require.NoError(t, err)

 	req, err := http.NewRequest(http.MethodGet, "/api/plugin-proxy/grafana-simple-app/api/v4/alerts", nil)
--- a/pkg/plugins/plugindef/plugindef.cue
+++ b/pkg/plugins/plugindef/plugindef.cue
@ -364,6 +364,9 @@ schemas: [{
 			reqSignedIn?: bool
 			reqRole?:     string

+			// RBAC action the user must have to access the route. i.e. plugin-id.projects:read
+			reqAction?: string
+
 			// For data source plugins. Route headers adds HTTP headers to the
 			// proxied request.
 			headers?: [...#Header]
--- a/pkg/plugins/plugindef/plugindef_types_gen.go
+++ b/pkg/plugins/plugindef/plugindef_types_gen.go
@ -461,7 +461,10 @@ type Route struct {

 	// For data source plugins. The route path that is replaced by the
 	// route URL field when proxying the call.
-	Path        *string `json:"path,omitempty"`
+	Path *string `json:"path,omitempty"`
+
+	// RBAC action the user must have to access the route. i.e. plugin-id.projects:read
+	ReqAction   *string `json:"reqAction,omitempty"`
 	ReqRole     *string `json:"reqRole,omitempty"`
 	ReqSignedIn *bool   `json:"reqSignedIn,omitempty"`

--- a/pkg/plugins/plugins.go
+++ b/pkg/plugins/plugins.go
@ -194,6 +194,7 @@ type Route struct {
 	Path         string          `json:"path"`
 	Method       string          `json:"method"`
 	ReqRole      org.RoleType    `json:"reqRole"`
+	ReqAction    string          `json:"reqAction"`
 	URL          string          `json:"url"`
 	URLParams    []URLParam      `json:"urlParams"`
 	Headers      []Header        `json:"headers"`
@ -203,6 +204,10 @@ type Route struct {
 	Body         json.RawMessage `json:"body"`
 }

+func (r *Route) RequiresRBACAction() bool {
+	return r.ReqAction != ""
+}
+
 // Header describes an HTTP header that is forwarded with
 // the proxied request for a plugin route
 type Header struct {