From 88480e4ef22f5f0158fd0b82dbc516b9c166f53a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Mon, 6 May 2024 13:26:24 +0200 Subject: [PATCH] proxy-backend: require auth by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/empty-spoons-tell.md | 43 +++ .../config/vocabularies/Backstage/accept.txt | 1 + docs/plugins/proxying.md | 23 +- plugins/proxy-backend/config.d.ts | 44 +++ plugins/proxy-backend/package.json | 5 + plugins/proxy-backend/src/alpha.ts | 17 +- .../src/service/router.config.test.ts | 2 +- .../src/service/router.credentials.test.ts | 305 ++++++++++++++++++ plugins/proxy-backend/src/service/router.ts | 94 ++++-- yarn.lock | 5 + 10 files changed, 503 insertions(+), 36 deletions(-) create mode 100644 .changeset/empty-spoons-tell.md create mode 100644 plugins/proxy-backend/src/service/router.credentials.test.ts diff --git a/.changeset/empty-spoons-tell.md b/.changeset/empty-spoons-tell.md new file mode 100644 index 0000000000..8c68cdc721 --- /dev/null +++ b/.changeset/empty-spoons-tell.md @@ -0,0 +1,43 @@ +--- +'@backstage/plugin-proxy-backend': minor +--- + +**BREAKING**: The proxy backend plugin is now protected by Backstage auth, by +default. Unless specifically configured (see below), all proxy endpoints will +reject requests immediately unless a valid Backstage user or service token is +passed along with the request. This aligns the proxy with how other Backstage +backends behave out of the box, and serves to protect your upstreams from +unauthorized access. + +A proxy configuration section can now look as follows: + +```yaml +proxy: + endpoints: + '/pagerduty': + target: https://api.pagerduty.com + credentials: require # NEW! + headers: + Authorization: Token token=${PAGERDUTY_TOKEN} +``` + +There are three possible `credentials` settings at this point: + +- `require`: Callers must provide Backstage user or service credentials with + each request. The credentials are not forwarded to the proxy target. +- `forward`: Callers must provide Backstage user or service credentials with + each request, and those credentials are forwarded to the proxy target. +- `dangerously-allow-unauthenticated`: No Backstage credentials are required to + access this proxy target. The target can still apply its own credentials + checks, but the proxy will not help block non-Backstage-blessed callers. If + you also add `allowedHeaders: ['Authorization']` to an endpoint configuration, + then the Backstage token (if provided) WILL be forwarded. + +The value `dangerously-allow-unauthenticated` was the old default. + +The value `require` is the new default, so requests that were previously +permitted may now start resulting in `401 Unauthorized` responses. + +If you have proxy endpoints that require unauthenticated access still, please +add `credentials: dangerously-allow-unauthenticated` to their declarations in +your app-config. diff --git a/.github/vale/config/vocabularies/Backstage/accept.txt b/.github/vale/config/vocabularies/Backstage/accept.txt index aa11568564..77dd930a73 100644 --- a/.github/vale/config/vocabularies/Backstage/accept.txt +++ b/.github/vale/config/vocabularies/Backstage/accept.txt @@ -447,6 +447,7 @@ unregistering unregistration untracked upsert +upstreams upvote URIs url diff --git a/docs/plugins/proxying.md b/docs/plugins/proxying.md index 40d155e13b..0f37eec982 100644 --- a/docs/plugins/proxying.md +++ b/docs/plugins/proxying.md @@ -18,12 +18,7 @@ The plugin is already added to a default Backstage project. In `packages/backend/src/index.ts`: ```ts -const proxyEnv = useHotMemoize(module, () => createEnv('proxy')); - -const service = createServiceBuilder(module) - .loadConfig(configReader) - /** ... other routers ... */ - .addRouter('/proxy', await proxy(proxyEnv)); +backend.add(import('@backstage/plugin-proxy-backend/alpha')); ``` ## Configuration @@ -40,6 +35,7 @@ proxy: /simple-example: http://simple.example.com:8080 '/larger-example/v1': target: http://larger.example.com:8080/svc.v1 + credentials: require headers: Authorization: ${EXAMPLE_AUTH_HEADER} # ...or interpolating a value into part of a string, @@ -56,6 +52,19 @@ backend requests to `/api/proxy/simple-example/...` and The value inside each route is either a simple URL string, or an object on the format accepted by [http-proxy-middleware](https://www.npmjs.com/package/http-proxy-middleware). +Additionally, it has an optional `credentials` key which can have the following +values: + +- `require`: Callers must provide Backstage user or service credentials with + each request. The credentials are not forwarded to the proxy target. This is + the default. +- `forward`: Callers must provide Backstage user or service credentials with + each request, and those credentials are forwarded to the proxy target. +- `dangerously-allow-unauthenticated`: No Backstage credentials are required to + access this proxy target. The target can still apply its own credentials + checks, but the proxy will not help block non-Backstage-blessed callers. If + you also add `allowedHeaders: ['Authorization']` to an endpoint configuration, + then the Backstage token (if provided) WILL be forwarded. If the value is a string, it is assumed to correspond to: @@ -64,6 +73,7 @@ target: changeOrigin: true pathRewrite: '^/': '/' +credentials: require ``` When the target is an object, it is given verbatim to `http-proxy-middleware` @@ -76,6 +86,7 @@ except with the following caveats for convenience: `'^/api/proxy/larger-example/v1/': '/'` is added. That means that a request to `/api/proxy/larger-example/v1/some/path` will be translated to a request to `http://larger.example.com:8080/svc.v1/some/path`. +- If `credentials` is not specified, it is set to `require`. There are also additional settings: diff --git a/plugins/proxy-backend/config.d.ts b/plugins/proxy-backend/config.d.ts index dd65aa8e75..710ff6d23f 100644 --- a/plugins/proxy-backend/config.d.ts +++ b/plugins/proxy-backend/config.d.ts @@ -71,6 +71,28 @@ export interface Config { * and headers that are set by the proxy will be forwarded. */ allowedHeaders?: string[]; + /** + * The credentials policy to apply. + * + * @remarks + * + * The values are as follows: + * + * - 'require': Callers must provide Backstage user or service + * credentials with each request. The credentials are not + * forwarded to the proxy target. + * - 'forward': Callers must provide Backstage user or service + * credentials with each request, and those credentials are + * forwarded to the proxy target. + * - 'dangerously-allow-unauthenticated': No Backstage credentials + * are required to access this proxy target. The target can still + * apply its own credentials checks, but the proxy will not help + * block non-Backstage-blessed callers. + */ + credentials?: + | 'require' + | 'forward' + | 'dangerously-allow-unauthenticated'; }; }; } & { @@ -121,6 +143,28 @@ export interface Config { * and headers that are set by the proxy will be forwarded. */ allowedHeaders?: string[]; + /** + * The credentials policy to apply. + * + * @remarks + * + * The values are as follows: + * + * - 'require': Callers must provide Backstage user or service + * credentials with each request. The credentials are not + * forwarded to the proxy target. + * - 'forward': Callers must provide Backstage user or service + * credentials with each request, and those credentials are + * forwarded to the proxy target. + * - 'dangerously-allow-unauthenticated': No Backstage credentials + * are required to access this proxy target. The target can still + * apply its own credentials checks, but the proxy will not help + * block non-Backstage-blessed callers. + */ + credentials?: + | 'require' + | 'forward' + | 'dangerously-allow-unauthenticated'; }; }; } diff --git a/plugins/proxy-backend/package.json b/plugins/proxy-backend/package.json index 9430f7120d..aa93b47c5e 100644 --- a/plugins/proxy-backend/package.json +++ b/plugins/proxy-backend/package.json @@ -52,6 +52,7 @@ "@backstage/backend-common": "workspace:^", "@backstage/backend-plugin-api": "workspace:^", "@backstage/config": "workspace:^", + "@backstage/types": "workspace:^", "@types/express": "^4.17.6", "express": "^4.17.1", "express-promise-router": "^4.1.0", @@ -64,14 +65,18 @@ "yup": "^1.0.0" }, "devDependencies": { + "@backstage/backend-defaults": "workspace:^", "@backstage/backend-test-utils": "workspace:^", "@backstage/cli": "workspace:^", "@backstage/config-loader": "workspace:^", + "@backstage/errors": "workspace:^", "@types/http-proxy-middleware": "^1.0.0", "@types/supertest": "^2.0.8", "@types/uuid": "^9.0.0", "@types/yup": "^0.32.0", "msw": "^1.0.0", + "node-fetch": "^2.6.7", + "portfinder": "^1.0.32", "supertest": "^6.1.3" }, "configSchema": "config.d.ts" diff --git a/plugins/proxy-backend/src/alpha.ts b/plugins/proxy-backend/src/alpha.ts index c1d55b84b3..339e42cd94 100644 --- a/plugins/proxy-backend/src/alpha.ts +++ b/plugins/proxy-backend/src/alpha.ts @@ -19,7 +19,7 @@ import { createBackendPlugin, coreServices, } from '@backstage/backend-plugin-api'; -import { createRouter } from './service/router'; +import { createRouterInternal } from './service/router'; /** * The proxy backend plugin. @@ -37,16 +37,11 @@ export default createBackendPlugin({ httpRouter: coreServices.httpRouter, }, async init({ config, discovery, logger, httpRouter }) { - httpRouter.use( - await createRouter({ - config, - discovery, - logger: loggerToWinstonLogger(logger), - }), - ); - httpRouter.addAuthPolicy({ - allow: 'unauthenticated', - path: '/', + await createRouterInternal({ + config, + discovery, + logger: loggerToWinstonLogger(logger), + httpRouterService: httpRouter, }); }, }); diff --git a/plugins/proxy-backend/src/service/router.config.test.ts b/plugins/proxy-backend/src/service/router.config.test.ts index 8643ee67c0..4f8420fc68 100644 --- a/plugins/proxy-backend/src/service/router.config.test.ts +++ b/plugins/proxy-backend/src/service/router.config.test.ts @@ -27,7 +27,7 @@ import request from 'supertest'; import { createRouter } from './router'; // this test is stored in its own file to work around the mocked -// http-proxy-middleware module used in the rest of the tests +// http-proxy-middleware module used in the main test file describe('createRouter reloadable configuration', () => { const server = setupServer( diff --git a/plugins/proxy-backend/src/service/router.credentials.test.ts b/plugins/proxy-backend/src/service/router.credentials.test.ts new file mode 100644 index 0000000000..1496b6ffb6 --- /dev/null +++ b/plugins/proxy-backend/src/service/router.credentials.test.ts @@ -0,0 +1,305 @@ +/* + * Copyright 2024 The Backstage Authors + * + * 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. + */ + +import { createBackend } from '@backstage/backend-defaults'; +import { + mockServices, + setupRequestMockHandlers, +} from '@backstage/backend-test-utils'; +import { ResponseError } from '@backstage/errors'; +import { JsonObject } from '@backstage/types'; +import { rest } from 'msw'; +import { setupServer } from 'msw/node'; +import fetch from 'node-fetch'; +import portFinder from 'portfinder'; + +// this test is stored in its own file to work around the mocked +// http-proxy-middleware module used in the main test file + +describe('credentials', () => { + const worker = setupServer(); + setupRequestMockHandlers(worker); + + it('handles all valid credentials settings', async () => { + const host = 'localhost'; + const port = await portFinder.getPortPromise(); + const baseUrl = `http://${host}:${port}`; + + const config = { + backend: { + baseUrl, + listen: { host, port }, + auth: { + externalAccess: [ + { + type: 'static', + options: { + token: 'static-token', + subject: 'static-subject', + }, + }, + ], + }, + }, + proxy: { + endpoints: { + '/simple': 'http://target.com', + '/default': { + target: 'http://target.com', + }, + '/require': { + target: 'http://target.com', + credentials: 'require', + }, + '/forward': { + target: 'http://target.com', + credentials: 'forward', + }, + '/dangerously-allow-unauthenticated--no-forward': { + target: 'http://target.com', + credentials: 'dangerously-allow-unauthenticated', + }, + '/dangerously-allow-unauthenticated--with-forward': { + target: 'http://target.com', + credentials: 'dangerously-allow-unauthenticated', + allowedHeaders: ['Authorization'], + }, + }, + }, + }; + + worker.use( + rest.all(`${baseUrl}/*`, req => req.passthrough()), + rest.get('http://target.com/*', (req, res, ctx) => { + const auth = req.headers.get('authorization'); + return res( + ctx.status(200), + ctx.json({ payload: { forwardedAuthorization: auth ?? false } }), + ); + }), + ); + + async function call(options: { + endpoint: string; + authorization: string | false; + }): Promise { + const { endpoint, authorization } = options; + return fetch(`${baseUrl}/api/proxy/${endpoint}/just-some-path`, { + headers: authorization ? { Authorization: authorization } : {}, + }).then(async res => { + if (!res.ok) { + throw await ResponseError.fromResponse(res); + } + return res.json(); + }); + } + + // Create an actual backend instead of a test backend, because we want to + // use the real HTTP server that provides the protection middleware etc. A + // bit harder to test, but at least we can use static external access tokens + // for it. + const backend = createBackend(); + backend.add(import('../alpha')); + backend.add(mockServices.rootConfig.factory({ data: config })); + backend.add(mockServices.rootLogger.factory()); + await backend.start(); + + try { + // simple credentials config + await expect( + call({ endpoint: 'simple', authorization: false }), + ).rejects.toMatchObject({ + body: { + error: { + message: 'Missing credentials', + name: 'AuthenticationError', + }, + }, + }); + await expect( + call({ endpoint: 'simple', authorization: 'Bearer static-token' }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: false, + }, + }); + await expect( + call({ endpoint: 'simple', authorization: 'Bearer not-valid' }), + ).rejects.toMatchObject({ + body: { + error: { + message: 'Illegal token', + name: 'AuthenticationError', + }, + }, + }); + + // default credentials config + await expect( + call({ endpoint: 'default', authorization: false }), + ).rejects.toMatchObject({ + body: { + error: { + message: 'Missing credentials', + name: 'AuthenticationError', + }, + }, + }); + await expect( + call({ endpoint: 'default', authorization: 'Bearer static-token' }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: false, + }, + }); + await expect( + call({ endpoint: 'default', authorization: 'Bearer not-valid' }), + ).rejects.toMatchObject({ + body: { + error: { + message: 'Illegal token', + name: 'AuthenticationError', + }, + }, + }); + + // require credentials config + await expect( + call({ endpoint: 'require', authorization: false }), + ).rejects.toMatchObject({ + body: { + error: { + message: 'Missing credentials', + name: 'AuthenticationError', + }, + }, + }); + await expect( + call({ endpoint: 'require', authorization: 'Bearer static-token' }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: false, + }, + }); + await expect( + call({ endpoint: 'require', authorization: 'Bearer not-valid' }), + ).rejects.toMatchObject({ + body: { + error: { + message: 'Illegal token', + name: 'AuthenticationError', + }, + }, + }); + + // forward credentials config + await expect( + call({ endpoint: 'forward', authorization: false }), + ).rejects.toMatchObject({ + body: { + error: { + message: 'Missing credentials', + name: 'AuthenticationError', + }, + }, + }); + await expect( + call({ endpoint: 'forward', authorization: 'Bearer static-token' }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: 'Bearer static-token', + }, + }); + await expect( + call({ endpoint: 'forward', authorization: 'Bearer not-valid' }), + ).rejects.toMatchObject({ + body: { + error: { + message: 'Illegal token', + name: 'AuthenticationError', + }, + }, + }); + + // dangerously-allow-unauthenticated credentials config, no forwarding + await expect( + call({ + endpoint: 'dangerously-allow-unauthenticated--no-forward', + authorization: false, + }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: false, + }, + }); + await expect( + call({ + endpoint: 'dangerously-allow-unauthenticated--no-forward', + authorization: 'Bearer static-token', + }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: false, + }, + }); + await expect( + call({ + endpoint: 'dangerously-allow-unauthenticated--no-forward', + authorization: 'Bearer not-valid', + }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: false, + }, + }); + + // dangerously-allow-unauthenticated credentials config, with forwarding + await expect( + call({ + endpoint: 'dangerously-allow-unauthenticated--with-forward', + authorization: false, + }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: false, + }, + }); + await expect( + call({ + endpoint: 'dangerously-allow-unauthenticated--with-forward', + authorization: 'Bearer static-token', + }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: 'Bearer static-token', + }, + }); + await expect( + call({ + endpoint: 'dangerously-allow-unauthenticated--with-forward', + authorization: 'Bearer not-valid-for-backstage-but-valid-for-target', + }), + ).resolves.toMatchObject({ + payload: { + forwardedAuthorization: + 'Bearer not-valid-for-backstage-but-valid-for-target', + }, + }); + } finally { + await backend.stop(); + } + }); +}); diff --git a/plugins/proxy-backend/src/service/router.ts b/plugins/proxy-backend/src/service/router.ts index 96b9beceee..6a513678f0 100644 --- a/plugins/proxy-backend/src/service/router.ts +++ b/plugins/proxy-backend/src/service/router.ts @@ -26,6 +26,8 @@ import { import { Logger } from 'winston'; import http from 'http'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; +import { JsonObject } from '@backstage/types'; +import { HttpRouterService } from '@backstage/backend-plugin-api'; // A list of headers that are always forwarded to the proxy targets. const safeForwardHeaders = [ @@ -71,9 +73,37 @@ export function buildMiddleware( route: string, config: string | ProxyConfig, reviveConsumedRequestBodies?: boolean, + httpRouterService?: HttpRouterService, ): RequestHandler { - const fullConfig = - typeof config === 'string' ? { target: config } : { ...config }; + let fullConfig: ProxyConfig; + let credentialsPolicy: string; + if (typeof config === 'string') { + fullConfig = { target: config }; + credentialsPolicy = 'require'; + } else { + const { credentials, ...rest } = config as any; + fullConfig = rest; + credentialsPolicy = credentials ?? 'require'; + } + + const credentialsPolicyCandidates = [ + 'require', + 'forward', + 'dangerously-allow-unauthenticated', + ]; + if (!credentialsPolicyCandidates.includes(credentialsPolicy)) { + const valid = credentialsPolicyCandidates.map(c => `'${c}'`).join(', '); + throw new Error( + `Unknown credentials policy '${credentialsPolicy}' for proxy route '${route}'; expected one of ${valid}`, + ); + } + + if (credentialsPolicy === 'dangerously-allow-unauthenticated') { + httpRouterService?.addAuthPolicy({ + path: route, + allow: 'unauthenticated', + }); + } // Validate that target is a valid URL. const targetType = typeof fullConfig.target; @@ -144,6 +174,10 @@ export function buildMiddleware( ].map(h => h.toLocaleLowerCase()), ); + if (credentialsPolicy === 'forward') { + requestHeaderAllowList.add('authorization'); + } + // Use the custom middleware filter to do two things: // 1. Remove any headers not in the allow list to stop them being forwarded // 2. Only permit the allowed HTTP methods if configured @@ -194,13 +228,15 @@ export function buildMiddleware( return createProxyMiddleware(filter, fullConfig); } -function readProxyConfig(config: Config, logger: Logger): unknown { - const endpoints = config.getOptionalConfig('proxy.endpoints')?.get(); +function readProxyConfig(config: Config, logger: Logger): JsonObject { + const endpoints = config + .getOptionalConfig('proxy.endpoints') + ?.get(); if (endpoints) { return endpoints; } - const root = config.getOptionalConfig('proxy')?.get(); + const root = config.getOptionalConfig('proxy')?.get(); if (!root) { return {}; } @@ -220,25 +256,37 @@ function readProxyConfig(config: Config, logger: Logger): unknown { } /** - * Creates a new {@link https://expressjs.com/en/api.html#router | "express router"} that proxy each target configured under the `proxy` key of the config - * @example - * ```ts - * let router = await createRouter({logger, config, discovery}); - * ``` - * @config + * Creates a new + * {@link https://expressjs.com/en/api.html#router | "express router"} that + * proxies each target configured under the `proxy.endpoints` key of the config. + * + * @remarks + * + * Example configuration: + * * ```yaml * proxy: - * simple-example: http://simple.example.com:8080 # Opt 1 Simple URL String - * '/larger-example/v1': # Opt 2 `http-proxy-middleware` compatible object - * target: http://larger.example.com:8080/svc.v1 - * headers: - * Authorization: Bearer ${EXAMPLE_AUTH_TOKEN} - *``` + * endpoints: + * # Option 1: Simple URL String + * simple-example: http://simple.example.com:8080 + * # Option 2: `http-proxy-middleware` compatible object + * '/larger-example/v1': + * target: http://larger.example.com:8080/svc.v1 + * headers: + * Authorization: Bearer ${EXAMPLE_AUTH_TOKEN} + * ``` + * * @see https://backstage.io/docs/plugins/proxying * @public */ export async function createRouter( options: RouterOptions, +): Promise { + return createRouterInternal(options); +} + +export async function createRouterInternal( + options: RouterOptions & { httpRouterService?: HttpRouterService }, ): Promise { const router = Router(); let currentRouter = Router(); @@ -261,7 +309,13 @@ export async function createRouter( const { pathname: pathPrefix } = new URL(externalUrl); const proxyConfig = readProxyConfig(options.config, options.logger); - configureMiddlewares(proxyOptions, currentRouter, pathPrefix, proxyConfig); + configureMiddlewares( + proxyOptions, + currentRouter, + pathPrefix, + proxyConfig, + options.httpRouterService, + ); router.use((...args) => currentRouter(...args)); if (options.config.subscribe) { @@ -279,11 +333,13 @@ export async function createRouter( currentRouter, pathPrefix, newProxyConfig, + options.httpRouterService, ); } }); } + options.httpRouterService?.use(router); return router; } @@ -296,6 +352,7 @@ function configureMiddlewares( router: express.Router, pathPrefix: string, proxyConfig: any, + httpRouterService?: HttpRouterService, ) { Object.entries(proxyConfig).forEach(([route, proxyRouteConfig]) => { try { @@ -307,6 +364,7 @@ function configureMiddlewares( route, proxyRouteConfig, options.reviveConsumedRequestBodies, + httpRouterService, ), ); } catch (e) { diff --git a/yarn.lock b/yarn.lock index dbc233dd64..9eacb239f3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6468,11 +6468,14 @@ __metadata: resolution: "@backstage/plugin-proxy-backend@workspace:plugins/proxy-backend" dependencies: "@backstage/backend-common": "workspace:^" + "@backstage/backend-defaults": "workspace:^" "@backstage/backend-plugin-api": "workspace:^" "@backstage/backend-test-utils": "workspace:^" "@backstage/cli": "workspace:^" "@backstage/config": "workspace:^" "@backstage/config-loader": "workspace:^" + "@backstage/errors": "workspace:^" + "@backstage/types": "workspace:^" "@types/express": ^4.17.6 "@types/http-proxy-middleware": ^1.0.0 "@types/supertest": ^2.0.8 @@ -6483,6 +6486,8 @@ __metadata: http-proxy-middleware: ^2.0.0 morgan: ^1.10.0 msw: ^1.0.0 + node-fetch: ^2.6.7 + portfinder: ^1.0.32 supertest: ^6.1.3 uuid: ^9.0.0 winston: ^3.2.1