From 9226c2aaa015dd72f1714879f1199b0b3de0330b Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Thu, 8 Oct 2020 14:12:23 +0200 Subject: [PATCH] feat(proxy-backend): limit the forwarded http headers to a safe set --- .changeset/2804.md | 15 ++ docs/plugins/proxying.md | 9 + plugins/proxy-backend/package.json | 2 +- plugins/proxy-backend/src/index.ts | 2 +- plugins/proxy-backend/src/service/index.ts | 17 ++ .../proxy-backend/src/service/router.test.ts | 167 +++++++++++++++++- plugins/proxy-backend/src/service/router.ts | 48 ++++- 7 files changed, 256 insertions(+), 4 deletions(-) create mode 100644 .changeset/2804.md create mode 100644 plugins/proxy-backend/src/service/index.ts diff --git a/.changeset/2804.md b/.changeset/2804.md new file mode 100644 index 0000000000..5223a24791 --- /dev/null +++ b/.changeset/2804.md @@ -0,0 +1,15 @@ +--- +'@backstage/plugin-proxy-backend': minor +--- + +Limit the http headers that are forwarded from the request to a safe set of defaults. +A user can configure additional headers that should be forwarded if the specific applications needs that. + +```yaml +proxy: + '/my-api': + target: 'https://my-api.com/get' + allowedHeaders: + # We need to forward the Authorization header that was provided by the caller + - Authorization +``` diff --git a/docs/plugins/proxying.md b/docs/plugins/proxying.md index 3d9aa90447..f55b4d9229 100644 --- a/docs/plugins/proxying.md +++ b/docs/plugins/proxying.md @@ -57,6 +57,15 @@ is also possible to limit the forwarded HTTP methods with the configuration `allowedMethods`, for example `allowedMethods: ['GET']` to enforce read-only access. +By default, the proxy will only forward safe HTTP request headers to the target. +Those are based on the headers that are considered safe for CORS and includes +headers like `content-type` or `last-modified`, as well as all headers that are +set by the proxy. If the proxy should forward other headers like +`authorization`, this must be enabled by the `allowedHeaders` config, for +example `allowedHeaders: ['Authorization']`. This should help to not +accidentally forward confidential headers (`cookie`, `X-Auth-Request-User`) to +third-parties. + If the value is a string, it is assumed to correspond to: ```yaml diff --git a/plugins/proxy-backend/package.json b/plugins/proxy-backend/package.json index 723c3e395e..11f18287f0 100644 --- a/plugins/proxy-backend/package.json +++ b/plugins/proxy-backend/package.json @@ -22,7 +22,6 @@ "@backstage/backend-common": "^0.1.1-alpha.24", "@backstage/config": "^0.1.1-alpha.24", "@types/express": "^4.17.6", - "@types/http-proxy-middleware": "^0.19.3", "express": "^4.17.1", "express-promise-router": "^3.0.3", "http-proxy-middleware": "^0.19.1", @@ -36,6 +35,7 @@ }, "devDependencies": { "@backstage/cli": "^0.1.1-alpha.24", + "@types/http-proxy-middleware": "^0.19.3", "@types/node-fetch": "^2.5.7", "@types/supertest": "^2.0.8", "@types/uuid": "^8.0.0", diff --git a/plugins/proxy-backend/src/index.ts b/plugins/proxy-backend/src/index.ts index 7612c392a2..de2be19b6d 100644 --- a/plugins/proxy-backend/src/index.ts +++ b/plugins/proxy-backend/src/index.ts @@ -14,4 +14,4 @@ * limitations under the License. */ -export * from './service/router'; +export * from './service'; diff --git a/plugins/proxy-backend/src/service/index.ts b/plugins/proxy-backend/src/service/index.ts new file mode 100644 index 0000000000..38fbb697c4 --- /dev/null +++ b/plugins/proxy-backend/src/service/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { createRouter } from './router'; diff --git a/plugins/proxy-backend/src/service/router.test.ts b/plugins/proxy-backend/src/service/router.test.ts index 07737e9ea8..012adaed93 100644 --- a/plugins/proxy-backend/src/service/router.test.ts +++ b/plugins/proxy-backend/src/service/router.test.ts @@ -14,13 +14,30 @@ * limitations under the License. */ -import { createRouter } from './router'; +import { buildMiddleware, createRouter } from './router'; import * as winston from 'winston'; import { ConfigReader } from '@backstage/config'; import { loadBackendConfig, SingleHostDiscovery, } from '@backstage/backend-common'; +import createProxyMiddleware, { + Config as ProxyMiddlewareConfig, + Proxy, +} from 'http-proxy-middleware'; +import * as http from 'http'; + +jest.mock('http-proxy-middleware', () => { + return jest.fn().mockImplementation( + (): Proxy => { + return () => undefined; + }, + ); +}); + +const mockCreateProxyMiddleware = createProxyMiddleware as jest.MockedFunction< + typeof createProxyMiddleware +>; describe('createRouter', () => { it('works', async () => { @@ -35,3 +52,151 @@ describe('createRouter', () => { expect(router).toBeDefined(); }); }); + +describe('buildMiddleware', () => { + const logger = winston.createLogger(); + + beforeEach(() => { + mockCreateProxyMiddleware.mockClear(); + }); + + it('accepts strings', async () => { + buildMiddleware('/api/', logger, 'test', 'http://mocked'); + + expect(createProxyMiddleware).toHaveBeenCalledTimes(1); + + const [filter, fullConfig] = mockCreateProxyMiddleware.mock.calls[0] as [ + (pathname: string, req: Partial) => boolean, + ProxyMiddlewareConfig, + ]; + expect(filter('', { method: 'GET' })).toBe(true); + expect(filter('', { method: 'POST' })).toBe(true); + expect(filter('', { method: 'PUT' })).toBe(true); + expect(filter('', { method: 'PATCH' })).toBe(true); + expect(filter('', { method: 'DELETE' })).toBe(true); + + expect(fullConfig.pathRewrite).toEqual({ '^/api/test/': '/' }); + expect(fullConfig.changeOrigin).toBe(true); + expect(fullConfig.logProvider!(logger)).toBe(logger); + }); + + it('limits allowedMethods', async () => { + buildMiddleware('/api/', logger, 'test', { + target: 'http://mocked', + allowedMethods: ['GET', 'DELETE'], + }); + + expect(createProxyMiddleware).toHaveBeenCalledTimes(1); + + const [filter, fullConfig] = mockCreateProxyMiddleware.mock.calls[0] as [ + (pathname: string, req: Partial) => boolean, + ProxyMiddlewareConfig, + ]; + expect(filter('', { method: 'GET' })).toBe(true); + expect(filter('', { method: 'POST' })).toBe(false); + expect(filter('', { method: 'PUT' })).toBe(false); + expect(filter('', { method: 'PATCH' })).toBe(false); + expect(filter('', { method: 'DELETE' })).toBe(true); + + expect(fullConfig.pathRewrite).toEqual({ '^/api/test/': '/' }); + expect(fullConfig.changeOrigin).toBe(true); + expect(fullConfig.logProvider!(logger)).toBe(logger); + }); + + it('permits default headers', async () => { + buildMiddleware('/api/', logger, 'test', { + target: 'http://mocked', + }); + + expect(createProxyMiddleware).toHaveBeenCalledTimes(1); + + const config = mockCreateProxyMiddleware.mock + .calls[0][1] as ProxyMiddlewareConfig; + + const testClientRequest = { + getHeaderNames: () => [ + 'cache-control', + 'content-language', + 'content-length', + 'content-type', + 'expires', + 'last-modified', + 'pragma', + 'host', + 'accept', + 'accept-language', + 'user-agent', + 'cookie', + ], + removeHeader: jest.fn(), + } as Partial; + + expect(config).toBeDefined(); + expect(config.onProxyReq).toBeDefined(); + + config.onProxyReq!( + testClientRequest as http.ClientRequest, + {} as http.IncomingMessage, + {} as http.ServerResponse, + ); + + expect(testClientRequest.removeHeader).toHaveBeenCalledTimes(1); + expect(testClientRequest.removeHeader).toHaveBeenCalledWith('cookie'); + }); + + it('permits default and configured headers', async () => { + buildMiddleware('/api/', logger, 'test', { + target: 'http://mocked', + headers: { + Authorization: 'my-token', + }, + }); + + expect(createProxyMiddleware).toHaveBeenCalledTimes(1); + + const config = mockCreateProxyMiddleware.mock + .calls[0][1] as ProxyMiddlewareConfig; + + const testClientRequest = { + getHeaderNames: () => ['authorization', 'Cookie'], + removeHeader: jest.fn(), + } as Partial; + + config.onProxyReq!( + testClientRequest as http.ClientRequest, + {} as http.IncomingMessage, + {} as http.ServerResponse, + ); + + expect(testClientRequest.removeHeader).toHaveBeenCalledTimes(1); + expect(testClientRequest.removeHeader).toHaveBeenCalledWith('Cookie'); + }); + + it('permits configured headers', async () => { + buildMiddleware('/api/', logger, 'test', { + target: 'http://mocked', + allowedHeaders: ['authorization', 'cookie'], + }); + + expect(createProxyMiddleware).toHaveBeenCalledTimes(1); + + const config = mockCreateProxyMiddleware.mock + .calls[0][1] as ProxyMiddlewareConfig; + + const testClientRequest = { + getHeaderNames: () => ['authorization', 'Cookie', 'X-Auth-Request-User'], + removeHeader: jest.fn(), + } as Partial; + + config.onProxyReq!( + testClientRequest as http.ClientRequest, + {} as http.IncomingMessage, + {} as http.ServerResponse, + ); + + expect(testClientRequest.removeHeader).toHaveBeenCalledTimes(1); + expect(testClientRequest.removeHeader).toHaveBeenCalledWith( + 'X-Auth-Request-User', + ); + }); +}); diff --git a/plugins/proxy-backend/src/service/router.ts b/plugins/proxy-backend/src/service/router.ts index 8cec31719c..75eab029b5 100644 --- a/plugins/proxy-backend/src/service/router.ts +++ b/plugins/proxy-backend/src/service/router.ts @@ -25,6 +25,27 @@ import { Logger } from 'winston'; import http from 'http'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; +// A list of headers that are always forwarded to the proxy targets. +const safeForwardHeaders = [ + // https://fetch.spec.whatwg.org/#cors-safelisted-request-header + 'cache-control', + 'content-language', + 'content-length', + 'content-type', + 'expires', + 'last-modified', + 'pragma', + + // host is overridden by default. if changeOrigin is configured to false, + // we assume this is a intentional and should also be forwarded. + 'host', + + // other headers that we assume to be ok + 'accept', + 'accept-language', + 'user-agent', +]; + export interface RouterOptions { logger: Logger; config: Config; @@ -33,11 +54,12 @@ export interface RouterOptions { export interface ProxyConfig extends ProxyMiddlewareConfig { allowedMethods?: string[]; + allowedHeaders?: string[]; } // Creates a proxy middleware, possibly with defaults added on top of the // given config. -function buildMiddleware( +export function buildMiddleware( pathPrefix: string, logger: Logger, route: string, @@ -68,6 +90,30 @@ function buildMiddleware( return fullConfig?.allowedMethods?.includes(req.method!) ?? true; }; + // Only forward the allowed HTTP headers to not forward unwanted secret headers + const headerAllowList = new Set( + [ + // allow all safe headers + ...safeForwardHeaders, + + // allow all headers that are set by the proxy + ...((fullConfig.headers && Object.keys(fullConfig.headers)) || []), + + // allow all configured headers + ...(fullConfig.allowedHeaders || []), + ].map(h => h.toLocaleLowerCase()), + ); + + fullConfig.onProxyReq = (proxyReq: http.ClientRequest) => { + const headerNames = proxyReq.getHeaderNames(); + + headerNames.forEach(h => { + if (!headerAllowList.has(h.toLocaleLowerCase())) { + proxyReq.removeHeader(h); + } + }); + }; + return createProxyMiddleware(filter, fullConfig); }