public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Security: Fix do not forward login cookie in outgoing requests

Browse Source 
(cherry picked from commit 709657b40ab4c77ce65b7fc1b653acb998570409)
This commit is contained in:
Marcus Efraimsson
2022-10-11 13:04:53 +03:00
committed by Sofia Papagiannaki
parent f80476a7ef
commit b571acc1dc
10 changed files with 52 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/api/datasources.go
+1 -1
View File
@@ -826,7 +826,7 @@ func (hs *HTTPServer) checkDatasourceHealth(c *models.ReqContext, ds *datasource
}
}
proxyutil.ClearCookieHeader(c.Req, ds.AllowedCookies())
proxyutil.ClearCookieHeader(c.Req, ds.AllowedCookies(), []string{hs.Cfg.LoginCookieName})
if cookieStr := c.Req.Header.Get("Cookie"); cookieStr != "" {
req.Headers["Cookie"] = cookieStr
}
pkg/api/metrics_test.go
+3 -2
View File
@@ -15,6 +15,7 @@ import (
"github.com/grafana/grafana/pkg/services/datasources"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/quota/quotatest"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/web/webtest"
"golang.org/x/oauth2"
@@ -68,7 +69,7 @@ func (ts *fakeOAuthTokenService) IsOAuthPassThruEnabled(*datasources.DataSource)
// `/ds/query` endpoint test
func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) {
qds := query.ProvideService(
nil,
setting.NewCfg(),
nil,
nil,
&fakePluginRequestValidator{},
@@ -117,7 +118,7 @@ func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) {
func TestAPIEndpoint_Metrics_PluginDecryptionFailure(t *testing.T) {
qds := query.ProvideService(
nil,
setting.NewCfg(),
nil,
nil,
&fakePluginRequestValidator{},
pkg/api/plugin_resource.go
+1 -10
View File
@@ -15,7 +15,6 @@ import (
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/plugins/backendplugin"
"github.com/grafana/grafana/pkg/services/contexthandler"
"github.com/grafana/grafana/pkg/services/datasources"
"github.com/grafana/grafana/pkg/util/proxyutil"
"github.com/grafana/grafana/pkg/web"
@@ -119,15 +118,7 @@ func (hs *HTTPServer) makePluginResourceRequest(w http.ResponseWriter, req *http
hs.log.Warn("failed to unpack JSONData in datasource instance settings", "err", err)
}
}
list := contexthandler.AuthHTTPHeaderListFromContext(req.Context())
if list != nil {
for _, name := range list.Items {
req.Header.Del(name)
}
}
proxyutil.ClearCookieHeader(req, keepCookieModel.KeepCookies)
proxyutil.ClearCookieHeader(req, keepCookieModel.KeepCookies, []string{hs.Cfg.LoginCookieName})
proxyutil.PrepareProxyRequest(req)
body, err := ioutil.ReadAll(req.Body)
pkg/api/pluginproxy/ds_proxy.go
+1 -1
View File
@@ -223,7 +223,7 @@ func (proxy *DataSourceProxy) director(req *http.Request) {
applyUserHeader(proxy.cfg.SendUserHeader, req, proxy.ctx.SignedInUser)
proxyutil.ClearCookieHeader(req, proxy.ds.AllowedCookies())
proxyutil.ClearCookieHeader(req, proxy.ds.AllowedCookies(), []string{proxy.cfg.LoginCookieName})
req.Header.Set("User-Agent", fmt.Sprintf("Grafana/%s", setting.BuildVersion))
jsonData := make(map[string]interface{})
pkg/infra/httpclient/httpclientprovider/forwarded_cookie_middleware_test.go
+8 -1
View File
@@ -13,6 +13,7 @@ func TestForwardedCookiesMiddleware(t *testing.T) {
tcs := []struct {
desc string
allowedCookies []string
disallowedCookies []string
expectedCookieHeader string
}{
{
@@ -30,6 +31,12 @@ func TestForwardedCookiesMiddleware(t *testing.T) {
allowedCookies: []string{"c1", "c3"},
expectedCookieHeader: "c1=1; c3=3",
},
{
desc: "When provided with allowed and not allowed cookies should populate Cookie header",
allowedCookies: []string{"c1", "c3"},
disallowedCookies: []string{"c1"},
expectedCookieHeader: "c3=3",
},
}
for _, tc := range tcs {
@@ -41,7 +48,7 @@ func TestForwardedCookiesMiddleware(t *testing.T) {
{Name: "c2", Value: "2"},
{Name: "c3", Value: "3"},
}
mw := httpclientprovider.ForwardedCookiesMiddleware(forwarded, tc.allowedCookies)
mw := httpclientprovider.ForwardedCookiesMiddleware(forwarded, tc.allowedCookies, tc.disallowedCookies)
opts := httpclient.Options{}
rt := mw.CreateMiddleware(opts, finalRoundTripper)
require.NotNil(t, rt)
pkg/infra/httpclient/httpclientprovider/forwarded_cookies_middleware.go
+2 -2
View File
@@ -11,13 +11,13 @@ const ForwardedCookiesMiddlewareName = "forwarded-cookies"
// ForwardedCookiesMiddleware middleware that sets Cookie header on the
// outgoing request, if forwarded cookies configured/provided.
func ForwardedCookiesMiddleware(forwardedCookies []*http.Cookie, allowedCookies []string) httpclient.Middleware {
func ForwardedCookiesMiddleware(forwardedCookies []*http.Cookie, allowedCookies []string, disallowedCookies []string) httpclient.Middleware {
return httpclient.NamedMiddlewareFunc(ForwardedCookiesMiddlewareName, func(opts httpclient.Options, next http.RoundTripper) http.RoundTripper {
return httpclient.RoundTripperFunc(func(req *http.Request) (*http.Response, error) {
for _, cookie := range forwardedCookies {
req.AddCookie(cookie)
}
proxyutil.ClearCookieHeader(req, allowedCookies)
proxyutil.ClearCookieHeader(req, allowedCookies, disallowedCookies)
return next.RoundTrip(req)
})
})
pkg/services/query/query.go
+2 -2
View File
@@ -162,7 +162,7 @@ func (s *Service) handleQueryData(ctx context.Context, user *models.SignedInUser
middlewares := []httpclient.Middleware{}
if parsedReq.httpRequest != nil {
middlewares = append(middlewares,
httpclientprovider.ForwardedCookiesMiddleware(parsedReq.httpRequest.Cookies(), ds.AllowedCookies()),
httpclientprovider.ForwardedCookiesMiddleware(parsedReq.httpRequest.Cookies(), ds.AllowedCookies(), []string{s.cfg.LoginCookieName}),
)
}
@@ -179,7 +179,7 @@ func (s *Service) handleQueryData(ctx context.Context, user *models.SignedInUser
}
if parsedReq.httpRequest != nil {
proxyutil.ClearCookieHeader(parsedReq.httpRequest, ds.AllowedCookies())
proxyutil.ClearCookieHeader(parsedReq.httpRequest, ds.AllowedCookies(), []string{s.cfg.LoginCookieName})
if cookieStr := parsedReq.httpRequest.Header.Get("Cookie"); cookieStr != "" {
req.Headers["Cookie"] = cookieStr
}
pkg/services/query/query_test.go
+2 -1
View File
@@ -6,6 +6,7 @@ import (
"testing"
"github.com/grafana/grafana-plugin-sdk-go/backend"
"github.com/grafana/grafana/pkg/setting"
"github.com/stretchr/testify/require"
"golang.org/x/oauth2"
@@ -99,7 +100,7 @@ func setup(t *testing.T) *testContext {
dataSourceCache: dc,
oauthTokenService: tc,
pluginRequestValidator: rv,
queryService: query.ProvideService(nil, dc, nil, rv, ds, pc, tc),
queryService: query.ProvideService(setting.NewCfg(), dc, nil, rv, ds, pc, tc),
}
}
pkg/util/proxyutil/proxyutil.go
+18 -5
View File
@@ -3,6 +3,7 @@ package proxyutil
import (
"net"
"net/http"
"sort"
)
// PrepareProxyRequest prepares a request for being proxied.
@@ -26,19 +27,31 @@ func PrepareProxyRequest(req *http.Request) {
}
}
// ClearCookieHeader clear cookie header, except for cookies specified to be kept.
func ClearCookieHeader(req *http.Request, keepCookiesNames []string) {
var keepCookies []*http.Cookie
// ClearCookieHeader clear cookie header, except for cookies specified to be kept (keepCookiesNames) if not in skipCookiesNames.
func ClearCookieHeader(req *http.Request, keepCookiesNames []string, skipCookiesNames []string) {
keepCookies := map[string]*http.Cookie{}
for _, c := range req.Cookies() {
for _, v := range keepCookiesNames {
if c.Name == v {
keepCookies = append(keepCookies, c)
keepCookies[c.Name] = c
}
}
}
for _, v := range skipCookiesNames {
delete(keepCookies, v)
}
req.Header.Del("Cookie")
for _, c := range keepCookies {
sortedCookies := []string{}
for name := range keepCookies {
sortedCookies = append(sortedCookies, name)
}
sort.Strings(sortedCookies)
for _, name := range sortedCookies {
c := keepCookies[name]
req.AddCookie(c)
}
}
pkg/util/proxyutil/proxyutil_test.go
+14 -2
View File
@@ -49,7 +49,7 @@ func TestClearCookieHeader(t *testing.T) {
require.NoError(t, err)
req.AddCookie(&http.Cookie{Name: "cookie"})
ClearCookieHeader(req, nil)
ClearCookieHeader(req, nil, nil)
require.NotContains(t, req.Header, "Cookie")
})
@@ -60,8 +60,20 @@ func TestClearCookieHeader(t *testing.T) {
req.AddCookie(&http.Cookie{Name: "cookie2"})
req.AddCookie(&http.Cookie{Name: "cookie3"})
ClearCookieHeader(req, []string{"cookie1", "cookie3"})
ClearCookieHeader(req, []string{"cookie1", "cookie3"}, nil)
require.Contains(t, req.Header, "Cookie")
require.Equal(t, "cookie1=; cookie3=", req.Header.Get("Cookie"))
})
t.Run("Clear cookie header with cookies to keep and skip should clear Cookie header and keep cookies", func(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "/", nil)
require.NoError(t, err)
req.AddCookie(&http.Cookie{Name: "cookie1"})
req.AddCookie(&http.Cookie{Name: "cookie2"})
req.AddCookie(&http.Cookie{Name: "cookie3"})
ClearCookieHeader(req, []string{"cookie1", "cookie3"}, []string{"cookie3"})
require.Contains(t, req.Header, "Cookie")
require.Equal(t, "cookie1=", req.Header.Get("Cookie"))
})
}