From d8d97d15ba94223fc94a37cd52c2b1de91a1d530 Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Fri, 12 Aug 2022 09:56:18 -0400 Subject: [PATCH] Alerting: AlertingProxy to elevate permissions for request forwarded to data proxy when RBAC enabled (#53620) --- pkg/services/ngalert/api/api.go | 1 + pkg/services/ngalert/api/util.go | 40 +++++--- pkg/services/ngalert/api/util_test.go | 133 ++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 11 deletions(-) diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index a820bcaa567..49885732eae 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -91,6 +91,7 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) { logger := log.New("ngalert.api") proxy := &AlertingProxy{ DataProxy: api.DataProxy, + ac: api.AccessControl, } // Register endpoints for proxying to Alertmanager-compatible backends. diff --git a/pkg/services/ngalert/api/util.go b/pkg/services/ngalert/api/util.go index 3ca8a648c7e..3f4ff195c4f 100644 --- a/pkg/services/ngalert/api/util.go +++ b/pkg/services/ngalert/api/util.go @@ -17,10 +17,12 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/datasourceproxy" "github.com/grafana/grafana/pkg/services/datasources" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/web" ) @@ -69,19 +71,35 @@ func (w *safeMacaronWrapper) CloseNotify() <-chan bool { return make(chan bool) } -// replacedResponseWriter overwrites the underlying responsewriter used by a *models.ReqContext. -// It's ugly because it needs to replace a value behind a few nested pointers. -func replacedResponseWriter(ctx *models.ReqContext) (*models.ReqContext, *response.NormalResponse) { - resp := response.CreateNormalResponse(make(http.Header), nil, 0) +// createProxyContext creates a new request context that is provided down to the data source proxy. +// The request context +// 1. overwrites the underlying response writer used by a *models.ReqContext because AlertingProxy needs to intercept +// the response from the data source to analyze it and probably change +// 2. elevates the current user permissions to Editor if both conditions are met: RBAC is enabled, user does not have Editor role. +// This is needed to bypass the plugin authorization, which still relies on the legacy roles. +// This elevation can be considered safe because all upstream calls are protected by the RBAC on web request router level. +func (p *AlertingProxy) createProxyContext(ctx *models.ReqContext, request *http.Request, response *response.NormalResponse) *models.ReqContext { cpy := *ctx cpyMCtx := *cpy.Context - cpyMCtx.Resp = web.NewResponseWriter(ctx.Req.Method, &safeMacaronWrapper{resp}) + cpyMCtx.Resp = web.NewResponseWriter(ctx.Req.Method, &safeMacaronWrapper{response}) cpy.Context = &cpyMCtx - return &cpy, resp + cpy.Req = request + + // If RBAC is enabled, the actions are checked upstream and if the user gets here then it is allowed to do an action against a datasource. + // Some data sources require legacy Editor role in order to perform mutating operations. In this case, we elevate permissions for the context that we + // will provide downstream. + // TODO (yuri) remove this after RBAC for plugins is implemented + if !p.ac.IsDisabled() && !ctx.SignedInUser.HasRole(org.RoleEditor) { + newUser := *ctx.SignedInUser + newUser.OrgRole = org.RoleEditor + cpy.SignedInUser = &newUser + } + return &cpy } type AlertingProxy struct { DataProxy *datasourceproxy.DataSourceProxyService + ac accesscontrol.AccessControl } // withReq proxies a different request @@ -100,8 +118,9 @@ func (p *AlertingProxy) withReq( for h, v := range headers { req.Header.Add(h, v) } - newCtx, resp := replacedResponseWriter(ctx) - newCtx.Req = req + // this response will be populated by the response from the datasource + resp := response.CreateNormalResponse(make(http.Header), nil, 0) + proxyContext := p.createProxyContext(ctx, req, resp) datasourceID := web.Params(ctx.Req)[":DatasourceID"] if datasourceID != "" { @@ -109,14 +128,13 @@ func (p *AlertingProxy) withReq( if err != nil { return ErrResp(http.StatusBadRequest, err, "DatasourceID is invalid") } - - p.DataProxy.ProxyDatasourceRequestWithID(newCtx, recipient) + p.DataProxy.ProxyDatasourceRequestWithID(proxyContext, recipient) } else { datasourceUID := web.Params(ctx.Req)[":DatasourceUID"] if datasourceUID == "" { return ErrResp(http.StatusBadRequest, err, "DatasourceUID is empty") } - p.DataProxy.ProxyDatasourceRequestWithUID(newCtx, datasourceUID) + p.DataProxy.ProxyDatasourceRequestWithUID(proxyContext, datasourceUID) } status := resp.Status() diff --git a/pkg/services/ngalert/api/util_test.go b/pkg/services/ngalert/api/util_test.go index 21035f83e0d..22faf4edd6c 100644 --- a/pkg/services/ngalert/api/util_test.go +++ b/pkg/services/ngalert/api/util_test.go @@ -1,9 +1,21 @@ package api import ( + "math/rand" + "net/http" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/api/response" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models" + accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/util" + "github.com/grafana/grafana/pkg/web" ) func TestToMacaronPath(t *testing.T) { @@ -25,3 +37,124 @@ func TestToMacaronPath(t *testing.T) { assert.Equal(t, tc.expectedOutputPath, outputPath) } } + +func TestAlertingProxy_createProxyContext(t *testing.T) { + ctx := &models.ReqContext{ + Context: &web.Context{ + Router: web.NewRouter(), + Req: &http.Request{}, + }, + SignedInUser: &user.SignedInUser{}, + UserToken: &models.UserToken{}, + IsSignedIn: rand.Int63()%2 == 1, + IsRenderCall: rand.Int63()%2 == 1, + AllowAnonymous: rand.Int63()%2 == 1, + SkipCache: rand.Int63()%2 == 1, + Logger: log.New("test"), + RequestNonce: util.GenerateShortUID(), + IsPublicDashboardView: rand.Int63()%2 == 1, + } + + t.Run("should create a copy of request context", func(t *testing.T) { + for _, mock := range []*accesscontrolmock.Mock{ + accesscontrolmock.New(), accesscontrolmock.New().WithDisabled(), + } { + proxy := AlertingProxy{ + DataProxy: nil, + ac: mock, + } + + req := &http.Request{} + resp := &response.NormalResponse{} + + newCtx := proxy.createProxyContext(ctx, req, resp) + + require.NotEqual(t, ctx, newCtx) + require.Equal(t, ctx.UserToken, newCtx.UserToken) + require.Equal(t, ctx.IsSignedIn, newCtx.IsSignedIn) + require.Equal(t, ctx.IsRenderCall, newCtx.IsRenderCall) + require.Equal(t, ctx.AllowAnonymous, newCtx.AllowAnonymous) + require.Equal(t, ctx.SkipCache, newCtx.SkipCache) + require.Equal(t, ctx.Logger, newCtx.Logger) + require.Equal(t, ctx.RequestNonce, newCtx.RequestNonce) + require.Equal(t, ctx.IsPublicDashboardView, newCtx.IsPublicDashboardView) + } + }) + t.Run("should overwrite response writer", func(t *testing.T) { + proxy := AlertingProxy{ + DataProxy: nil, + ac: accesscontrolmock.New(), + } + + req := &http.Request{} + resp := &response.NormalResponse{} + + newCtx := proxy.createProxyContext(ctx, req, resp) + + require.NotEqual(t, ctx.Context.Resp, newCtx.Context.Resp) + require.Equal(t, ctx.Context.Router, newCtx.Context.Router) + require.Equal(t, ctx.Context.Req, newCtx.Context.Req) + + require.NotEqual(t, 123, resp.Status()) + newCtx.Context.Resp.WriteHeader(123) + require.Equal(t, 123, resp.Status()) + }) + t.Run("if access control is enabled", func(t *testing.T) { + t.Run("should elevate permissions to Editor for Viewer", func(t *testing.T) { + proxy := AlertingProxy{ + DataProxy: nil, + ac: accesscontrolmock.New(), + } + + req := &http.Request{} + resp := &response.NormalResponse{} + + viewerCtx := *ctx + viewerCtx.SignedInUser = &user.SignedInUser{ + OrgRole: org.RoleViewer, + } + + newCtx := proxy.createProxyContext(&viewerCtx, req, resp) + require.NotEqual(t, viewerCtx.SignedInUser, newCtx.SignedInUser) + require.Truef(t, newCtx.SignedInUser.HasRole(org.RoleEditor), "user of the proxy request should have at least Editor role but has %s", newCtx.SignedInUser.OrgRole) + }) + t.Run("should not alter user if it is Editor", func(t *testing.T) { + proxy := AlertingProxy{ + DataProxy: nil, + ac: accesscontrolmock.New(), + } + + req := &http.Request{} + resp := &response.NormalResponse{} + + for _, roleType := range []org.RoleType{org.RoleEditor, org.RoleAdmin} { + roleCtx := *ctx + roleCtx.SignedInUser = &user.SignedInUser{ + OrgRole: roleType, + } + newCtx := proxy.createProxyContext(&roleCtx, req, resp) + require.Equalf(t, roleCtx.SignedInUser, newCtx.SignedInUser, "user should not be altered if role is %s", roleType) + } + }) + }) + t.Run("if access control is disabled", func(t *testing.T) { + t.Run("should not alter user", func(t *testing.T) { + proxy := AlertingProxy{ + DataProxy: nil, + ac: accesscontrolmock.New().WithDisabled(), + } + + req := &http.Request{} + resp := &response.NormalResponse{} + + for _, roleType := range []org.RoleType{org.RoleViewer, org.RoleEditor, org.RoleAdmin} { + roleCtx := *ctx + roleCtx.SignedInUser = &user.SignedInUser{ + OrgRole: roleType, + } + newCtx := proxy.createProxyContext(&roleCtx, req, resp) + require.Equalf(t, roleCtx.SignedInUser, newCtx.SignedInUser, "user should not be altered if access control is disabled and role is %s", roleType) + } + }) + }) +}