Skip to content

Commit b571acc

Browse files
marefrpapagian
authored andcommitted
Security: Fix do not forward login cookie in outgoing requests
(cherry picked from commit 709657b40ab4c77ce65b7fc1b653acb998570409)
1 parent f80476a commit b571acc

File tree

10 files changed

+52
-27
lines changed

10 files changed

+52
-27
lines changed

Diff for: pkg/api/datasources.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -826,7 +826,7 @@ func (hs *HTTPServer) checkDatasourceHealth(c *models.ReqContext, ds *datasource
826826
}
827827
}
828828

829-
proxyutil.ClearCookieHeader(c.Req, ds.AllowedCookies())
829+
proxyutil.ClearCookieHeader(c.Req, ds.AllowedCookies(), []string{hs.Cfg.LoginCookieName})
830830
if cookieStr := c.Req.Header.Get("Cookie"); cookieStr != "" {
831831
req.Headers["Cookie"] = cookieStr
832832
}

Diff for: pkg/api/metrics_test.go

+3-2
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"github.com/grafana/grafana/pkg/services/datasources"
1616
"github.com/grafana/grafana/pkg/services/featuremgmt"
1717
"github.com/grafana/grafana/pkg/services/quota/quotatest"
18+
"github.com/grafana/grafana/pkg/setting"
1819
"github.com/grafana/grafana/pkg/web/webtest"
1920

2021
"golang.org/x/oauth2"
@@ -68,7 +69,7 @@ func (ts *fakeOAuthTokenService) IsOAuthPassThruEnabled(*datasources.DataSource)
6869
// `/ds/query` endpoint test
6970
func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) {
7071
qds := query.ProvideService(
71-
nil,
72+
setting.NewCfg(),
7273
nil,
7374
nil,
7475
&fakePluginRequestValidator{},
@@ -117,7 +118,7 @@ func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) {
117118

118119
func TestAPIEndpoint_Metrics_PluginDecryptionFailure(t *testing.T) {
119120
qds := query.ProvideService(
120-
nil,
121+
setting.NewCfg(),
121122
nil,
122123
nil,
123124
&fakePluginRequestValidator{},

Diff for: pkg/api/plugin_resource.go

+1-10
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515

1616
"github.com/grafana/grafana/pkg/models"
1717
"github.com/grafana/grafana/pkg/plugins/backendplugin"
18-
"github.com/grafana/grafana/pkg/services/contexthandler"
1918
"github.com/grafana/grafana/pkg/services/datasources"
2019
"github.com/grafana/grafana/pkg/util/proxyutil"
2120
"github.com/grafana/grafana/pkg/web"
@@ -119,15 +118,7 @@ func (hs *HTTPServer) makePluginResourceRequest(w http.ResponseWriter, req *http
119118
hs.log.Warn("failed to unpack JSONData in datasource instance settings", "err", err)
120119
}
121120
}
122-
123-
list := contexthandler.AuthHTTPHeaderListFromContext(req.Context())
124-
if list != nil {
125-
for _, name := range list.Items {
126-
req.Header.Del(name)
127-
}
128-
}
129-
130-
proxyutil.ClearCookieHeader(req, keepCookieModel.KeepCookies)
121+
proxyutil.ClearCookieHeader(req, keepCookieModel.KeepCookies, []string{hs.Cfg.LoginCookieName})
131122
proxyutil.PrepareProxyRequest(req)
132123

133124
body, err := ioutil.ReadAll(req.Body)

Diff for: pkg/api/pluginproxy/ds_proxy.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ func (proxy *DataSourceProxy) director(req *http.Request) {
223223

224224
applyUserHeader(proxy.cfg.SendUserHeader, req, proxy.ctx.SignedInUser)
225225

226-
proxyutil.ClearCookieHeader(req, proxy.ds.AllowedCookies())
226+
proxyutil.ClearCookieHeader(req, proxy.ds.AllowedCookies(), []string{proxy.cfg.LoginCookieName})
227227
req.Header.Set("User-Agent", fmt.Sprintf("Grafana/%s", setting.BuildVersion))
228228

229229
jsonData := make(map[string]interface{})

Diff for: pkg/infra/httpclient/httpclientprovider/forwarded_cookie_middleware_test.go

+8-1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ func TestForwardedCookiesMiddleware(t *testing.T) {
1313
tcs := []struct {
1414
desc string
1515
allowedCookies []string
16+
disallowedCookies []string
1617
expectedCookieHeader string
1718
}{
1819
{
@@ -30,6 +31,12 @@ func TestForwardedCookiesMiddleware(t *testing.T) {
3031
allowedCookies: []string{"c1", "c3"},
3132
expectedCookieHeader: "c1=1; c3=3",
3233
},
34+
{
35+
desc: "When provided with allowed and not allowed cookies should populate Cookie header",
36+
allowedCookies: []string{"c1", "c3"},
37+
disallowedCookies: []string{"c1"},
38+
expectedCookieHeader: "c3=3",
39+
},
3340
}
3441

3542
for _, tc := range tcs {
@@ -41,7 +48,7 @@ func TestForwardedCookiesMiddleware(t *testing.T) {
4148
{Name: "c2", Value: "2"},
4249
{Name: "c3", Value: "3"},
4350
}
44-
mw := httpclientprovider.ForwardedCookiesMiddleware(forwarded, tc.allowedCookies)
51+
mw := httpclientprovider.ForwardedCookiesMiddleware(forwarded, tc.allowedCookies, tc.disallowedCookies)
4552
opts := httpclient.Options{}
4653
rt := mw.CreateMiddleware(opts, finalRoundTripper)
4754
require.NotNil(t, rt)

Diff for: pkg/infra/httpclient/httpclientprovider/forwarded_cookies_middleware.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,13 @@ const ForwardedCookiesMiddlewareName = "forwarded-cookies"
1111

1212
// ForwardedCookiesMiddleware middleware that sets Cookie header on the
1313
// outgoing request, if forwarded cookies configured/provided.
14-
func ForwardedCookiesMiddleware(forwardedCookies []*http.Cookie, allowedCookies []string) httpclient.Middleware {
14+
func ForwardedCookiesMiddleware(forwardedCookies []*http.Cookie, allowedCookies []string, disallowedCookies []string) httpclient.Middleware {
1515
return httpclient.NamedMiddlewareFunc(ForwardedCookiesMiddlewareName, func(opts httpclient.Options, next http.RoundTripper) http.RoundTripper {
1616
return httpclient.RoundTripperFunc(func(req *http.Request) (*http.Response, error) {
1717
for _, cookie := range forwardedCookies {
1818
req.AddCookie(cookie)
1919
}
20-
proxyutil.ClearCookieHeader(req, allowedCookies)
20+
proxyutil.ClearCookieHeader(req, allowedCookies, disallowedCookies)
2121
return next.RoundTrip(req)
2222
})
2323
})

Diff for: pkg/services/query/query.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ func (s *Service) handleQueryData(ctx context.Context, user *models.SignedInUser
162162
middlewares := []httpclient.Middleware{}
163163
if parsedReq.httpRequest != nil {
164164
middlewares = append(middlewares,
165-
httpclientprovider.ForwardedCookiesMiddleware(parsedReq.httpRequest.Cookies(), ds.AllowedCookies()),
165+
httpclientprovider.ForwardedCookiesMiddleware(parsedReq.httpRequest.Cookies(), ds.AllowedCookies(), []string{s.cfg.LoginCookieName}),
166166
)
167167
}
168168

@@ -179,7 +179,7 @@ func (s *Service) handleQueryData(ctx context.Context, user *models.SignedInUser
179179
}
180180

181181
if parsedReq.httpRequest != nil {
182-
proxyutil.ClearCookieHeader(parsedReq.httpRequest, ds.AllowedCookies())
182+
proxyutil.ClearCookieHeader(parsedReq.httpRequest, ds.AllowedCookies(), []string{s.cfg.LoginCookieName})
183183
if cookieStr := parsedReq.httpRequest.Header.Get("Cookie"); cookieStr != "" {
184184
req.Headers["Cookie"] = cookieStr
185185
}

Diff for: pkg/services/query/query_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77

88
"github.com/grafana/grafana-plugin-sdk-go/backend"
9+
"github.com/grafana/grafana/pkg/setting"
910
"github.com/stretchr/testify/require"
1011
"golang.org/x/oauth2"
1112

@@ -99,7 +100,7 @@ func setup(t *testing.T) *testContext {
99100
dataSourceCache: dc,
100101
oauthTokenService: tc,
101102
pluginRequestValidator: rv,
102-
queryService: query.ProvideService(nil, dc, nil, rv, ds, pc, tc),
103+
queryService: query.ProvideService(setting.NewCfg(), dc, nil, rv, ds, pc, tc),
103104
}
104105
}
105106

Diff for: pkg/util/proxyutil/proxyutil.go

+18-5
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package proxyutil
33
import (
44
"net"
55
"net/http"
6+
"sort"
67
)
78

89
// PrepareProxyRequest prepares a request for being proxied.
@@ -26,19 +27,31 @@ func PrepareProxyRequest(req *http.Request) {
2627
}
2728
}
2829

29-
// ClearCookieHeader clear cookie header, except for cookies specified to be kept.
30-
func ClearCookieHeader(req *http.Request, keepCookiesNames []string) {
31-
var keepCookies []*http.Cookie
30+
// ClearCookieHeader clear cookie header, except for cookies specified to be kept (keepCookiesNames) if not in skipCookiesNames.
31+
func ClearCookieHeader(req *http.Request, keepCookiesNames []string, skipCookiesNames []string) {
32+
keepCookies := map[string]*http.Cookie{}
3233
for _, c := range req.Cookies() {
3334
for _, v := range keepCookiesNames {
3435
if c.Name == v {
35-
keepCookies = append(keepCookies, c)
36+
keepCookies[c.Name] = c
3637
}
3738
}
3839
}
3940

41+
for _, v := range skipCookiesNames {
42+
delete(keepCookies, v)
43+
}
44+
4045
req.Header.Del("Cookie")
41-
for _, c := range keepCookies {
46+
47+
sortedCookies := []string{}
48+
for name := range keepCookies {
49+
sortedCookies = append(sortedCookies, name)
50+
}
51+
sort.Strings(sortedCookies)
52+
53+
for _, name := range sortedCookies {
54+
c := keepCookies[name]
4255
req.AddCookie(c)
4356
}
4457
}

Diff for: pkg/util/proxyutil/proxyutil_test.go

+14-2
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ func TestClearCookieHeader(t *testing.T) {
4949
require.NoError(t, err)
5050
req.AddCookie(&http.Cookie{Name: "cookie"})
5151

52-
ClearCookieHeader(req, nil)
52+
ClearCookieHeader(req, nil, nil)
5353
require.NotContains(t, req.Header, "Cookie")
5454
})
5555

@@ -60,8 +60,20 @@ func TestClearCookieHeader(t *testing.T) {
6060
req.AddCookie(&http.Cookie{Name: "cookie2"})
6161
req.AddCookie(&http.Cookie{Name: "cookie3"})
6262

63-
ClearCookieHeader(req, []string{"cookie1", "cookie3"})
63+
ClearCookieHeader(req, []string{"cookie1", "cookie3"}, nil)
6464
require.Contains(t, req.Header, "Cookie")
6565
require.Equal(t, "cookie1=; cookie3=", req.Header.Get("Cookie"))
6666
})
67+
68+
t.Run("Clear cookie header with cookies to keep and skip should clear Cookie header and keep cookies", func(t *testing.T) {
69+
req, err := http.NewRequest(http.MethodGet, "/", nil)
70+
require.NoError(t, err)
71+
req.AddCookie(&http.Cookie{Name: "cookie1"})
72+
req.AddCookie(&http.Cookie{Name: "cookie2"})
73+
req.AddCookie(&http.Cookie{Name: "cookie3"})
74+
75+
ClearCookieHeader(req, []string{"cookie1", "cookie3"}, []string{"cookie3"})
76+
require.Contains(t, req.Header, "Cookie")
77+
require.Equal(t, "cookie1=", req.Header.Get("Cookie"))
78+
})
6779
}

0 commit comments

Comments
 (0)