From 3a8f040abdb237e6b296b28126419ac0f6292007 Mon Sep 17 00:00:00 2001 From: Antoine Arlaud Date: Mon, 9 Dec 2024 17:14:56 +0100 Subject: [PATCH 1/4] feat: add tighter connection auth validation --- accept-server.local.json | 18 ++ lib/client/auth/brokerServerConnection.ts | 50 ++++++ lib/client/auth/oauth.ts | 1 - lib/client/socket.ts | 41 ++++- lib/hybrid-sdk/clientRequestHelpers.ts | 1 + lib/server/auth/authHelpers.ts | 45 +++++ lib/server/index.ts | 5 + lib/server/routesHandlers/authHandlers.ts | 59 +++++++ .../routesHandlers/connectionStatusHandler.ts | 2 +- .../routesHandlers/httpRequestHandler.ts | 10 +- lib/server/socket.ts | 156 ++++++++++++------ lib/server/socketHandlers/closeHandler.ts | 2 +- lib/server/socketHandlers/identifyHandler.ts | 24 ++- package.json | 6 +- 14 files changed, 356 insertions(+), 64 deletions(-) create mode 100644 accept-server.local.json create mode 100644 lib/client/auth/brokerServerConnection.ts create mode 100644 lib/server/auth/authHelpers.ts create mode 100644 lib/server/routesHandlers/authHandlers.ts diff --git a/accept-server.local.json b/accept-server.local.json new file mode 100644 index 000000000..07b8e3f24 --- /dev/null +++ b/accept-server.local.json @@ -0,0 +1,18 @@ +{ + "//": "private refers to what's internal to snyk, i.e. the snyk.io server", + "private": [ + { + "//": "send any type of request to our connected clients", + "method": "any", + "path": "/*" + } + ], + "public": [ + { + "//": "send any type of request to our connected clients", + "method": "any", + "path": "/*" + } + ] +} + diff --git a/lib/client/auth/brokerServerConnection.ts b/lib/client/auth/brokerServerConnection.ts new file mode 100644 index 000000000..e870a112b --- /dev/null +++ b/lib/client/auth/brokerServerConnection.ts @@ -0,0 +1,50 @@ +import { getConfig } from '../../common/config/config'; +import { PostFilterPreparedRequest } from '../../common/relay/prepareRequest'; +import version from '../../common/utils/version'; +import { + HttpResponse, + makeRequestToDownstream, +} from '../../hybrid-sdk/http/request'; +import { Role } from '../types/client'; + +export interface BrokerServerConnectionParams { + connectionIdentifier: string; + brokerClientId: string; + authorization: string; + role: Role; + serverId: number; +} +export const renewBrokerServerConnection = async ( + brokerServerConnectionParams: BrokerServerConnectionParams, +): Promise => { + const clientConfig = getConfig(); + const apiHostname = clientConfig.API_BASE_URL; + const body = { + data: { + type: 'broker_connection', + attributes: { + broker_client_id: brokerServerConnectionParams.brokerClientId, + }, + }, + }; + const url = new URL( + `${apiHostname}/hidden/brokers/connections/${brokerServerConnectionParams.connectionIdentifier}/auth/refresh`, + ); + url.searchParams.append('role', brokerServerConnectionParams.role); + if (brokerServerConnectionParams.serverId) { + url.searchParams.append( + 'serverId', + `${brokerServerConnectionParams.serverId}`, + ); + } + const req: PostFilterPreparedRequest = { + url: url.toString(), + headers: { + authorization: brokerServerConnectionParams.authorization, + 'user-agent': `Snyk Broker Client ${version}`, + }, + method: 'POST', + body: JSON.stringify(body), + }; + return await makeRequestToDownstream(req); +}; diff --git a/lib/client/auth/oauth.ts b/lib/client/auth/oauth.ts index 0a9cbd36f..ad6422381 100644 --- a/lib/client/auth/oauth.ts +++ b/lib/client/auth/oauth.ts @@ -38,7 +38,6 @@ export async function fetchJwt( const jwt = accessToken.access_token; const type = accessToken.token_type; const expiresIn = accessToken.expires_in; - return { expiresIn: expiresIn, authHeader: `${type} ${jwt}` }; } catch (err) { logger.error({ err }, 'Unable to retrieve JWT'); diff --git a/lib/client/socket.ts b/lib/client/socket.ts index cd8f5c942..c6b96582f 100644 --- a/lib/client/socket.ts +++ b/lib/client/socket.ts @@ -22,6 +22,7 @@ import { fetchJwt } from './auth/oauth'; import { getServerId } from './dispatcher'; import { determineFilterType } from './utils/filterSelection'; import { notificationHandler } from './socketHandlers/notificationHandler'; +import { renewBrokerServerConnection } from './auth/brokerServerConnection'; export const createWebSocketConnectionPairs = async ( websocketConnections: WebSocketConnection[], @@ -66,7 +67,6 @@ export const createWebSocketConnectionPairs = async ( } else { logger.info( { - connection: socketIdentifyingMetadata.friendlyName, serverId: serverId, }, 'received server id', @@ -97,6 +97,11 @@ export const createWebSocket = ( const localClientOps = Object.assign({}, clientOpts); identifyingMetadata.identifier = identifyingMetadata.identifier ?? localClientOps.config.brokerToken; + if (!identifyingMetadata.identifier) { + throw new Error( + `Invalid Broker Identifier/Token in websocket tunnel creation step.`, + ); + } const Socket = Primus.createSocket({ transformer: 'engine.io', parser: 'EJSON', @@ -175,8 +180,18 @@ export const createWebSocket = ( websocket.transport.extraHeaders['Authorization'] = clientOpts.accessToken!.authHeader; - // websocket.end(); - // websocket.open(); + if (clientOpts.config.WS_TUNNEL_BOUNCE_ON_AUTH_REFRESH) { + websocket.end(); + websocket.open(); + } else { + await renewBrokerServerConnection({ + connectionIdentifier: identifyingMetadata.identifier!, + brokerClientId: identifyingMetadata.clientId, + authorization: clientOpts.accessToken!.authHeader, + role: identifyingMetadata.role, + serverId: serverId, + }); + } timeoutHandlerId = setTimeout( timeoutHandler, (clientOpts.accessToken!.expiresIn - 60) * 1000, @@ -235,6 +250,26 @@ export const createWebSocket = ( openHandler(websocket, localClientOps, identifyingMetadata), ); + websocket.on('service', (msg) => { + logger.info({ msg }, 'service message received'); + }); + // websocket.on('outgoing::open', function () { + // type OnErrorHandler = (type: string, code: number) => void; + + // const originalErrorHandler: OnErrorHandler = + // websocket.socket.transport.onError; + + // websocket.socket.transport.onError = (...args: [string, number]) => { + // const [type, code] = args; // Destructure for clarity + // if (code === 401) { + // logger.error({ type, code }, `Connection denied: unauthorized.`); + // } else { + // logger.error({ type, code }, `Transport error during polling.`); + // } + // originalErrorHandler.apply(websocket.socket?.transport, args); + // }; + // }); + websocket.on('close', () => closeHandler(localClientOps, identifyingMetadata), ); diff --git a/lib/hybrid-sdk/clientRequestHelpers.ts b/lib/hybrid-sdk/clientRequestHelpers.ts index e608a1f88..8189c5827 100644 --- a/lib/hybrid-sdk/clientRequestHelpers.ts +++ b/lib/hybrid-sdk/clientRequestHelpers.ts @@ -82,6 +82,7 @@ export class HybridClientRequestHandler { response: this.res, streamBuffer, streamSize: 0, + brokerAppClientId: this.res.locals.brokerAppClientId, }); streamBuffer.pipe(this.res); const simplifiedContextWithStreamingID = this.simplifiedContext; diff --git a/lib/server/auth/authHelpers.ts b/lib/server/auth/authHelpers.ts new file mode 100644 index 000000000..6791e540b --- /dev/null +++ b/lib/server/auth/authHelpers.ts @@ -0,0 +1,45 @@ +import { PostFilterPreparedRequest } from '../../common/relay/prepareRequest'; +import { makeSingleRawRequestToDownstream } from '../../hybrid-sdk/http/request'; +import { log as logger } from '../../logs/logger'; + +export const validateBrokerClientCredentials = async ( + authHeaderValue: string, + brokerAppClientId: string, + brokerConnectionIdentifier: string, +) => { + if ( + !process.env.HPS_BACKEND_URL_WITH_BASE_PATH || + !process.env.HPS_BACKEND_VERSION + ) { + logger.error({}, `HPS Backend not configured correctly.`); + throw new Error(`HPS Backend not configured correctly.`); + } + const body = { + data: { + type: 'broker_connection', + attributes: { + broker_app_client_id: brokerAppClientId, + }, + }, + }; + const req: PostFilterPreparedRequest = { + url: `${process.env.HPS_BACKEND_URL_WITH_BASE_PATH}/${brokerConnectionIdentifier}/auth/validate?version=${process.env.HPS_BACKEND_VERSION}`, + headers: { + authorization: authHeaderValue, + 'Content-type': 'application/vnd.api+json', + }, + method: 'POST', + body: JSON.stringify(body), + }; + + const response = await makeSingleRawRequestToDownstream(req); + if (response.statusCode === 201) { + return true; + } else { + logger.debug( + { statusCode: response.statusCode, message: response.statusText }, + `Broker ${brokerConnectionIdentifier} app client ID ${brokerAppClientId} failed validation.`, + ); + return false; + } +}; diff --git a/lib/server/index.ts b/lib/server/index.ts index e6bb05b09..43aacfdcb 100644 --- a/lib/server/index.ts +++ b/lib/server/index.ts @@ -13,6 +13,7 @@ import { getForwardHttpRequestHandler } from './socketHandlers/initHandlers'; import { loadAllFilters } from '../common/filter/filtersAsync'; import { FiltersType } from '../common/types/filter'; import filterRulesLoader from '../common/filter/filter-rules-loading'; +import { authRefreshHandler } from './routesHandlers/authHandlers'; export const main = async (serverOpts: ServerOpts) => { logger.info({ version }, 'Broker starting in server mode'); @@ -57,6 +58,10 @@ export const main = async (serverOpts: ServerOpts) => { app.use(applyPrometheusMiddleware()); } app.get('/connection-status/:token', connectionStatusHandler); + app.post( + '/hidden/brokers/connections/:identifier/auth/refresh', + authRefreshHandler, + ); app.all( '/broker/:token/*', overloadHttpRequestWithConnectionDetailsMiddleware, diff --git a/lib/server/routesHandlers/authHandlers.ts b/lib/server/routesHandlers/authHandlers.ts new file mode 100644 index 000000000..a57b0f6db --- /dev/null +++ b/lib/server/routesHandlers/authHandlers.ts @@ -0,0 +1,59 @@ +import { Request, Response } from 'express'; +import { validateBrokerClientCredentials } from '../auth/authHelpers'; +import { log as logger } from '../../logs/logger'; +import { validate } from 'uuid'; +import { getSocketConnectionByIdentifier } from '../socket'; +interface BrokerConnectionAuthRequest { + data: { + attributes: { + broker_client_id: string; + }; + id: string; + type: 'broker_connection'; + }; +} +export const authRefreshHandler = async (req: Request, res: Response) => { + const credentials = req.headers['authorization']; + const brokerAppClientId = + req.headers[`${process.env.SNYK_INTERNAL_AUTH_CLIENT_ID_HEADER}`]; + const identifier = req.params.identifier; + const body = JSON.parse(req.body.toString()) as BrokerConnectionAuthRequest; + const brokerClientId = body.data.attributes.broker_client_id; + if ( + !validate(identifier) || + !validate(brokerClientId) || + !validate(brokerAppClientId) + ) { + logger.warn( + { identifier, brokerClientId, brokerAppClientId }, + 'Invalid credentials', + ); + return res.status(401).send('Invalid parameters or credentials.'); + } + const connection = getSocketConnectionByIdentifier(identifier); + const currentClient = connection + ? connection.find((x) => x.metadata.clientId === brokerClientId) + : null; + logger.debug({ identifier, brokerClientId }, 'Validating credentials'); + if ( + credentials === undefined || + brokerAppClientId === undefined || + credentials?.split('.').length != 3 || + !connection || + !currentClient + ) { + return res.status(401).send('Invalid credentials.'); + } else { + const credsCheckResponse = await validateBrokerClientCredentials( + credentials, + brokerAppClientId as string, + identifier, + ); + if (credsCheckResponse) { + return res.status(200).send('OK'); + } else { + currentClient.socket!.end(); + return res.status(401).send('Invalid credentials.'); + } + } +}; diff --git a/lib/server/routesHandlers/connectionStatusHandler.ts b/lib/server/routesHandlers/connectionStatusHandler.ts index 50529bffa..c6bb181e6 100644 --- a/lib/server/routesHandlers/connectionStatusHandler.ts +++ b/lib/server/routesHandlers/connectionStatusHandler.ts @@ -11,7 +11,7 @@ export const connectionStatusHandler = async (req: Request, res: Response) => { const desensitizedToken = getDesensitizedToken(token); const connections = getSocketConnections(); if (connections.has(token)) { - const clientsMetadata = connections.get(req.params.token).map((conn) => ({ + const clientsMetadata = connections.get(req.params.token)!.map((conn) => ({ version: conn.metadata && conn.metadata.version, filters: conn.metadata && conn.metadata.filters, })); diff --git a/lib/server/routesHandlers/httpRequestHandler.ts b/lib/server/routesHandlers/httpRequestHandler.ts index 699d53dc0..307e8ea13 100644 --- a/lib/server/routesHandlers/httpRequestHandler.ts +++ b/lib/server/routesHandlers/httpRequestHandler.ts @@ -66,12 +66,14 @@ export const overloadHttpRequestWithConnectionDetailsMiddleware = async ( // Grab a first (newest) client from the pool // This is really silly... - res.locals.websocket = connections.get(token)[0].socket; - res.locals.socketVersion = connections.get(token)[0].socketVersion; - res.locals.capabilities = connections.get(token)[0].metadata.capabilities; + res.locals.websocket = connections.get(token)![0].socket; + res.locals.socketVersion = connections.get(token)![0].socketVersion; + res.locals.capabilities = connections.get(token)![0].metadata.capabilities; + res.locals.brokerAppClientId = + connections.get(token)![0].metadata.brokerAppClientId; req['locals'] = {}; req['locals']['capabilities'] = - connections.get(token)[0].metadata.capabilities; + connections.get(token)![0].metadata.capabilities; // strip the leading url req.url = req.url.slice(`/broker/${token}`.length); if (req.url.includes('connection_role')) { diff --git a/lib/server/socket.ts b/lib/server/socket.ts index 91c45c814..0e304da78 100644 --- a/lib/server/socket.ts +++ b/lib/server/socket.ts @@ -5,15 +5,32 @@ import { SocketHandler } from './types/socket'; import { handleIoError } from './socketHandlers/errorHandler'; import { handleSocketConnection } from './socketHandlers/connectionHandler'; import { initConnectionHandler } from './socketHandlers/initHandlers'; -// import { maskToken } from '../common/utils/token'; -// import { log as logger } from '../logs/logger'; +import { log as logger } from '../logs/logger'; +import { maskToken } from '../common/utils/token'; +import { validateBrokerClientCredentials } from './auth/authHelpers'; +import { Role } from '../client/types/client'; +import { decode } from 'jsonwebtoken'; -const socketConnections = new Map(); +export interface ClientSocket { + socket?: { end() }; + socketType: 'server'; + socketVersion: number; + brokerClientId: string; + brokerAppClientId: string; + role: Role; + jwt: any; + metadata?: any; +} +const socketConnections = new Map(); export const getSocketConnections = () => { return socketConnections; }; +export const getSocketConnectionByIdentifier = (identifier: string) => { + return socketConnections.get(identifier); +}; + const socket = ({ server, loadedServerOpts }): SocketHandler => { const ioConfig = { transformer: 'engine.io', @@ -30,54 +47,95 @@ const socket = ({ server, loadedServerOpts }): SocketHandler => { }; const websocket = new Primus(server, ioConfig); - // websocket.authorize(async (req, done) => { - // const maskedToken = maskToken( - // req.uri.pathname.replaceAll(/^\/primus\/([^/]+)\//g, '$1').toLowerCase(), - // ); - // const authHeader = req.headers['authorization']; - - // if ( - // (!authHeader || !authHeader.startsWith('Bearer')) && - // loadedServerOpts.config.BROKER_SERVER_MANDATORY_AUTH_ENABLED - // ) { - // logger.error({ maskedToken }, 'request missing Authorization header'); - // done({ - // statusCode: 401, - // authenticate: 'Bearer', - // message: 'missing required authorization header', - // }); - // return; - // } + if ( + process.env.HPS_BACKEND_URL_WITH_BASE_PATH && + process.env.HPS_BACKEND_VERSION + ) { + websocket.authorize(async (req, done) => { + const connectionIdentifier = req.uri.pathname + .replaceAll(/^\/primus\/([^/]+)\//g, '$1') + .toLowerCase(); + const maskedToken = maskToken(connectionIdentifier); + const authHeader = req.headers['authorization']; + const brokerClientId = req.headers['x-snyk-broker-client-id'] ?? null; + const role = req.headers['x-snyk-broker-client-role'] ?? null; + if ( + (!authHeader || + !authHeader.toLowerCase().startsWith('bearer') || + !brokerClientId) && + loadedServerOpts.config.BROKER_SERVER_MANDATORY_AUTH_ENABLED + ) { + logger.debug({ maskedToken }, 'request missing Authorization header'); + done({ + statusCode: 401, + authenticate: 'Bearer', + message: 'missing required authorization header', + }); + return; + } - // const jwt = authHeader - // ? authHeader.substring(authHeader.indexOf(' ') + 1) - // : ''; - // if (!jwt) logger.debug({}, `TODO: Validate jwt`); - // done(); - // // let oauthResponse = await axiosInstance.request({ - // // url: 'http://localhost:8080/oauth2/introspect', - // // method: 'POST', - // // headers: { - // // 'Content-Type': 'application/x-www-form-urlencoded', - // // }, - // // auth: { - // // username: 'broker-connection-a', - // // password: 'secret', - // // }, - // // data: `token=${token}`, - // // }); + const jwt = authHeader + ? authHeader.substring(authHeader.indexOf(' ') + 1) + : ''; + if ( + !jwt && + loadedServerOpts.config.BROKER_SERVER_MANDATORY_AUTH_ENABLED + ) { + done({ + statusCode: 401, + authenticate: 'Bearer', + message: 'Invalid JWT', + }); + return; + } else { + logger.debug( + { maskedToken: maskToken(connectionIdentifier), brokerClientId }, + `Validating auth for connection ${connectionIdentifier} client Id ${brokerClientId}, role ${role}`, + ); + const decodedJwt = decode(jwt, { complete: true }); + const brokerAppClientId = decodedJwt?.payload['azp'] ?? ''; + const credsCheckResponse = await validateBrokerClientCredentials( + jwt, + brokerAppClientId, + connectionIdentifier, + ); + if (!credsCheckResponse) { + done({ + statusCode: 401, + authenticate: 'Bearer', + message: 'Invalid credentials.', + }); + } - // // if (!oauthResponse.data.active) { - // // logger.error({maskedToken}, 'JWT is not active (could be expired, malformed, not issued by us, etc)'); - // // done({ - // // statusCode: 403, - // // message: 'token not active', - // // }); - // // } else { - // // req.oauth_data = oauthResponse.data; - // // done(); - // // } - // }); + const currentClient: ClientSocket = { + socketType: 'server', + socketVersion: 1, + brokerClientId: brokerClientId, + brokerAppClientId: brokerAppClientId, + role: role ?? Role.primary, + jwt, + }; + const connections = getSocketConnections(); + const clientPool = + (connections.get(connectionIdentifier) as Array) || []; + const currentClientIndex = clientPool.findIndex( + (x) => + x.brokerClientId === currentClient.brokerClientId && + x.role === currentClient.role, + ); + if (currentClientIndex < 0) { + clientPool.unshift(currentClient); + } else { + clientPool[currentClientIndex] = { + ...clientPool[currentClientIndex], + ...currentClient, + }; + } + connections.set(connectionIdentifier, clientPool); + } + done(); + }); + } websocket.socketType = 'server'; websocket.socketVersion = 1; websocket.plugin('emitter', Emitter); diff --git a/lib/server/socketHandlers/closeHandler.ts b/lib/server/socketHandlers/closeHandler.ts index 1859006b5..da0a2dba4 100644 --- a/lib/server/socketHandlers/closeHandler.ts +++ b/lib/server/socketHandlers/closeHandler.ts @@ -19,7 +19,7 @@ export const handleConnectionCloseOnSocket = ( .get(token) ?.filter((_) => _.socket !== socket); const filteredClientPool = - clientPool?.filter((_) => _.socket !== socket) || ''; + clientPool?.filter((_) => _.socket !== socket) || []; logger.info( { closeReason, diff --git a/lib/server/socketHandlers/identifyHandler.ts b/lib/server/socketHandlers/identifyHandler.ts index 214a674e0..e3c4ae929 100644 --- a/lib/server/socketHandlers/identifyHandler.ts +++ b/lib/server/socketHandlers/identifyHandler.ts @@ -73,14 +73,30 @@ export const handleIdentifyOnSocket = (clientData, socket, token): boolean => { }, 'new client connection identified', ); - const connections = getSocketConnections(); - const clientPool = connections.get(token) || []; - clientPool.unshift({ + const currentClient = { socket, socketType: 'server', socketVersion: 1, metadata: clientData.metadata, - }); + brokerClientId: clientData.metadata.clientId, + }; + const connections = getSocketConnections(); + const clientPool = (connections.get(token) as Array) || []; + const currentClientIndex = clientPool.findIndex( + (x) => + clientData.metadata.clientId && + x.brokerClientId === clientData.metadata.clientId && + clientData.metadata.role && + x.role === clientData.metadata.role, + ); + if (currentClientIndex < 0) { + clientPool.unshift(currentClient); + } else { + clientPool[currentClientIndex] = { + ...clientPool[currentClientIndex], + ...currentClient, + }; + } connections.set(token, clientPool); socket.on('chunk', streamingResponse(token)); diff --git a/package.json b/package.json index a4fd304ff..c115789f2 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,11 @@ "prepare": "npm run build && rm -rf ./dist/client-templates && cp -Rf ./client-templates ./dist", "start": "node .", "dev": "tsc-watch --project tsconfig.json --onSuccess 'node .' | ./node_modules/.bin/bunyan", - "dev:client": "tsc-watch --project tsconfig.json --onSuccess 'node dist/cli/index.js client' | ./node_modules/.bin/bunyan", + "dev:client": "LOG_LEVEL=debug PORT=9001 NODE_ENV=development tsc-watch --project tsconfig.json --onSuccess 'node dist/cli/index.js client' | ./node_modules/.bin/bunyan", + "dev:client-to-local-server": "USE_LOCAL_BACKEND='true' LOG_LEVEL=debug PORT=9001 NODE_ENV=development tsc-watch --project tsconfig.json --onSuccess 'node dist/cli/index.js client' | ./node_modules/.bin/bunyan", + "dev:client2-to-local-server": "USE_LOCAL_BACKEND='true' LOG_LEVEL=debug PORT=9002 NODE_ENV=development tsc-watch --project tsconfig.json --onSuccess 'node dist/cli/index.js client' | ./node_modules/.bin/bunyan", + "dev:server": "LOG_LEVEL=debug PORT=9000 ACCEPT=accept-server.local.json NODE_ENV=development tsc-watch --project tsconfig.json --onSuccess 'node dist/cli/index.js server' | ./node_modules/.bin/bunyan", + "dev:server-with-auth": "BROKER_SERVER_MANDATORY_AUTH_ENABLED=true npm run dev:server", "test": "npm run test:unit && npm run test:functional", "test:unit": "jest unit --detectOpenHandles", "test:functional": "jest functional --detectOpenHandles --runInBand", From de6fde5b0bc59e10c679beb9377860a1a4f1ef50 Mon Sep 17 00:00:00 2001 From: Antoine Arlaud Date: Mon, 9 Dec 2024 18:29:15 +0100 Subject: [PATCH 2/4] feat: add tighter connection auth validation on data response --- .../http/server-post-stream-handler.ts | 11 +++++-- .../routesHandlers/httpRequestHandler.ts | 2 +- .../routesHandlers/postResponseHandler.ts | 31 +++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/hybrid-sdk/http/server-post-stream-handler.ts b/lib/hybrid-sdk/http/server-post-stream-handler.ts index 397c86ee9..95323dbef 100644 --- a/lib/hybrid-sdk/http/server-post-stream-handler.ts +++ b/lib/hybrid-sdk/http/server-post-stream-handler.ts @@ -15,6 +15,7 @@ export interface StreamResponse { streamBuffer: stream.PassThrough; response: Response; streamSize?: number; + brokerAppClientId: string; } export class StreamResponseHandler { @@ -35,12 +36,18 @@ export class StreamResponseHandler { streamingID, streamResponse.streamBuffer, streamResponse.response, + streamResponse.brokerAppClientId, ); } - constructor(streamingID, streamBuffer, response) { + constructor(streamingID, streamBuffer, response, brokerAppClientId) { this.streamingID = streamingID; - this.streamResponse = { streamBuffer, response, streamSize: 0 }; + this.streamResponse = { + streamBuffer, + response, + streamSize: 0, + brokerAppClientId, + }; } writeStatusAndHeaders = (statusAndHeaders) => { diff --git a/lib/server/routesHandlers/httpRequestHandler.ts b/lib/server/routesHandlers/httpRequestHandler.ts index 307e8ea13..257a8d20d 100644 --- a/lib/server/routesHandlers/httpRequestHandler.ts +++ b/lib/server/routesHandlers/httpRequestHandler.ts @@ -70,7 +70,7 @@ export const overloadHttpRequestWithConnectionDetailsMiddleware = async ( res.locals.socketVersion = connections.get(token)![0].socketVersion; res.locals.capabilities = connections.get(token)![0].metadata.capabilities; res.locals.brokerAppClientId = - connections.get(token)![0].metadata.brokerAppClientId; + connections.get(token)![0].brokerAppClientId ?? ''; req['locals'] = {}; req['locals']['capabilities'] = connections.get(token)![0].metadata.capabilities; diff --git a/lib/server/routesHandlers/postResponseHandler.ts b/lib/server/routesHandlers/postResponseHandler.ts index b6cc9684e..c4f69a657 100644 --- a/lib/server/routesHandlers/postResponseHandler.ts +++ b/lib/server/routesHandlers/postResponseHandler.ts @@ -4,6 +4,8 @@ import { log as logger } from '../../logs/logger'; import { getDesensitizedToken } from '../utils/token'; import { incrementHttpRequestsTotal } from '../../common/utils/metrics'; import { StreamResponseHandler } from '../../hybrid-sdk/http/server-post-stream-handler'; +import { getConfig } from '../../common/config/config'; +import { decode } from 'jsonwebtoken'; export const handlePostResponse = (req: Request, res: Response) => { incrementHttpRequestsTotal(false, 'data-response'); @@ -32,6 +34,35 @@ export const handlePostResponse = (req: Request, res: Response) => { .json({ message: 'unable to find request matching streaming id' }); return; } + const credentials = req.headers.authorization; + if (getConfig().BROKER_SERVER_MANDATORY_AUTH_ENABLED && !credentials) { + logger.error( + logContext, + 'Invalid Broker Client credentials on response data', + ); + res.status(401).json({ message: 'Invalid Broker Client credentials' }); + return; + } + const decodedJwt = credentials + ? decode(credentials!.replace(/bearer /i, ''), { + complete: true, + }) + : null; + + const brokerAppClientId = decodedJwt ? decodedJwt?.payload['azp'] : ''; + + if ( + getConfig().BROKER_SERVER_MANDATORY_AUTH_ENABLED && + (!brokerAppClientId || + brokerAppClientId != streamHandler.streamResponse.brokerAppClientId) + ) { + logger.error( + logContext, + 'Invalid Broker Client credentials for stream on response data', + ); + res.status(401).json({ message: 'Invalid Broker Client credentials' }); + return; + } let statusAndHeaders = ''; let statusAndHeadersSize = -1; From d710ac7d706122080d2a439234413198a868c5e5 Mon Sep 17 00:00:00 2001 From: Antoine Arlaud Date: Tue, 10 Dec 2024 15:38:18 +0100 Subject: [PATCH 3/4] fix: direct all auth flows to the api endpoints --- lib/client/index.ts | 1 + lib/client/socket.ts | 6 ++++-- lib/server/auth/authHelpers.ts | 19 ++++++++----------- lib/server/routesHandlers/authHandlers.ts | 3 ++- .../routesHandlers/postResponseHandler.ts | 1 - lib/server/socket.ts | 13 ++++--------- 6 files changed, 19 insertions(+), 24 deletions(-) diff --git a/lib/client/index.ts b/lib/client/index.ts index 66f7aed7f..293425023 100644 --- a/lib/client/index.ts +++ b/lib/client/index.ts @@ -89,6 +89,7 @@ export const main = async (clientOpts: ClientOpts) => { clientOpts.config.brokerClientId, ); } + await retrieveAndLoadFilters(clientOpts); if (process.env.NODE_ENV != 'test') { setInterval(async () => { diff --git a/lib/client/socket.ts b/lib/client/socket.ts index c6b96582f..295aac777 100644 --- a/lib/client/socket.ts +++ b/lib/client/socket.ts @@ -194,13 +194,15 @@ export const createWebSocket = ( } timeoutHandlerId = setTimeout( timeoutHandler, - (clientOpts.accessToken!.expiresIn - 60) * 1000, + // (clientOpts.accessToken!.expiresIn - 60) * 1000, + 30000, ); }; timeoutHandlerId = setTimeout( timeoutHandler, - (clientOpts.accessToken!.expiresIn - 60) * 1000, + // (clientOpts.accessToken!.expiresIn - 60) * 1000, + 10000, ); } diff --git a/lib/server/auth/authHelpers.ts b/lib/server/auth/authHelpers.ts index 6791e540b..23bf603d3 100644 --- a/lib/server/auth/authHelpers.ts +++ b/lib/server/auth/authHelpers.ts @@ -1,29 +1,26 @@ +import { getConfig } from '../../common/config/config'; import { PostFilterPreparedRequest } from '../../common/relay/prepareRequest'; import { makeSingleRawRequestToDownstream } from '../../hybrid-sdk/http/request'; import { log as logger } from '../../logs/logger'; export const validateBrokerClientCredentials = async ( authHeaderValue: string, - brokerAppClientId: string, + brokerClientId: string, brokerConnectionIdentifier: string, ) => { - if ( - !process.env.HPS_BACKEND_URL_WITH_BASE_PATH || - !process.env.HPS_BACKEND_VERSION - ) { - logger.error({}, `HPS Backend not configured correctly.`); - throw new Error(`HPS Backend not configured correctly.`); - } const body = { data: { type: 'broker_connection', attributes: { - broker_app_client_id: brokerAppClientId, + broker_client_id: brokerClientId, }, }, }; + const req: PostFilterPreparedRequest = { - url: `${process.env.HPS_BACKEND_URL_WITH_BASE_PATH}/${brokerConnectionIdentifier}/auth/validate?version=${process.env.HPS_BACKEND_VERSION}`, + url: `${ + getConfig().apiHostname + }/hidden/brokers/connections/${brokerConnectionIdentifier}/auth/validate?version=2024-02-08~experimental`, headers: { authorization: authHeaderValue, 'Content-type': 'application/vnd.api+json', @@ -38,7 +35,7 @@ export const validateBrokerClientCredentials = async ( } else { logger.debug( { statusCode: response.statusCode, message: response.statusText }, - `Broker ${brokerConnectionIdentifier} app client ID ${brokerAppClientId} failed validation.`, + `Broker ${brokerConnectionIdentifier} client ID ${brokerClientId} failed validation.`, ); return false; } diff --git a/lib/server/routesHandlers/authHandlers.ts b/lib/server/routesHandlers/authHandlers.ts index a57b0f6db..08529dd18 100644 --- a/lib/server/routesHandlers/authHandlers.ts +++ b/lib/server/routesHandlers/authHandlers.ts @@ -30,6 +30,7 @@ export const authRefreshHandler = async (req: Request, res: Response) => { ); return res.status(401).send('Invalid parameters or credentials.'); } + const connection = getSocketConnectionByIdentifier(identifier); const currentClient = connection ? connection.find((x) => x.metadata.clientId === brokerClientId) @@ -46,7 +47,7 @@ export const authRefreshHandler = async (req: Request, res: Response) => { } else { const credsCheckResponse = await validateBrokerClientCredentials( credentials, - brokerAppClientId as string, + brokerClientId as string, identifier, ); if (credsCheckResponse) { diff --git a/lib/server/routesHandlers/postResponseHandler.ts b/lib/server/routesHandlers/postResponseHandler.ts index c4f69a657..c5d7bd1b6 100644 --- a/lib/server/routesHandlers/postResponseHandler.ts +++ b/lib/server/routesHandlers/postResponseHandler.ts @@ -50,7 +50,6 @@ export const handlePostResponse = (req: Request, res: Response) => { : null; const brokerAppClientId = decodedJwt ? decodedJwt?.payload['azp'] : ''; - if ( getConfig().BROKER_SERVER_MANDATORY_AUTH_ENABLED && (!brokerAppClientId || diff --git a/lib/server/socket.ts b/lib/server/socket.ts index 0e304da78..5523b2447 100644 --- a/lib/server/socket.ts +++ b/lib/server/socket.ts @@ -18,7 +18,6 @@ export interface ClientSocket { brokerClientId: string; brokerAppClientId: string; role: Role; - jwt: any; metadata?: any; } const socketConnections = new Map(); @@ -47,10 +46,7 @@ const socket = ({ server, loadedServerOpts }): SocketHandler => { }; const websocket = new Primus(server, ioConfig); - if ( - process.env.HPS_BACKEND_URL_WITH_BASE_PATH && - process.env.HPS_BACKEND_VERSION - ) { + if (loadedServerOpts.config.BROKER_SERVER_MANDATORY_AUTH_ENABLED) { websocket.authorize(async (req, done) => { const connectionIdentifier = req.uri.pathname .replaceAll(/^\/primus\/([^/]+)\//g, '$1') @@ -92,11 +88,9 @@ const socket = ({ server, loadedServerOpts }): SocketHandler => { { maskedToken: maskToken(connectionIdentifier), brokerClientId }, `Validating auth for connection ${connectionIdentifier} client Id ${brokerClientId}, role ${role}`, ); - const decodedJwt = decode(jwt, { complete: true }); - const brokerAppClientId = decodedJwt?.payload['azp'] ?? ''; const credsCheckResponse = await validateBrokerClientCredentials( jwt, - brokerAppClientId, + brokerClientId, connectionIdentifier, ); if (!credsCheckResponse) { @@ -107,13 +101,14 @@ const socket = ({ server, loadedServerOpts }): SocketHandler => { }); } + const decodedJwt = decode(jwt, { complete: true }); + const brokerAppClientId = decodedJwt?.payload['azp'] ?? ''; const currentClient: ClientSocket = { socketType: 'server', socketVersion: 1, brokerClientId: brokerClientId, brokerAppClientId: brokerAppClientId, role: role ?? Role.primary, - jwt, }; const connections = getSocketConnections(); const clientPool = From d302d12d59f763c96d5279533b46228cb7566ed7 Mon Sep 17 00:00:00 2001 From: Antoine Arlaud Date: Tue, 10 Dec 2024 18:04:50 +0100 Subject: [PATCH 4/4] fix: direct data streaming to the api endpoint --- lib/client/socket.ts | 6 ++---- .../http/downstream-post-stream-to-server.ts | 5 ++++- lib/server/index.ts | 12 +++++++++++- .../server-client-universal-bearer-auth.test.ts | 4 ++-- ...erver-client-universal-pooled-credentials.test.ts | 4 ++-- test/functional/server-client-universal.test.ts | 3 ++- 6 files changed, 23 insertions(+), 11 deletions(-) diff --git a/lib/client/socket.ts b/lib/client/socket.ts index 295aac777..c6b96582f 100644 --- a/lib/client/socket.ts +++ b/lib/client/socket.ts @@ -194,15 +194,13 @@ export const createWebSocket = ( } timeoutHandlerId = setTimeout( timeoutHandler, - // (clientOpts.accessToken!.expiresIn - 60) * 1000, - 30000, + (clientOpts.accessToken!.expiresIn - 60) * 1000, ); }; timeoutHandlerId = setTimeout( timeoutHandler, - // (clientOpts.accessToken!.expiresIn - 60) * 1000, - 10000, + (clientOpts.accessToken!.expiresIn - 60) * 1000, ); } diff --git a/lib/hybrid-sdk/http/downstream-post-stream-to-server.ts b/lib/hybrid-sdk/http/downstream-post-stream-to-server.ts index 8384e6775..dd01a58df 100644 --- a/lib/hybrid-sdk/http/downstream-post-stream-to-server.ts +++ b/lib/hybrid-sdk/http/downstream-post-stream-to-server.ts @@ -64,8 +64,11 @@ class BrokerServerPostResponseHandler { async #initHttpClientRequest() { try { + const backendHostname = this.#config.universalBrokerEnabled + ? `${this.#config.API_BASE_URL}/hidden/broker` + : this.#config.brokerServerUrl; const url = new URL( - `${this.#config.brokerServerUrl}/response-data/${this.#brokerToken}/${ + `${backendHostname}/response-data/${this.#brokerToken}/${ this.#streamingId }`, ); diff --git a/lib/server/index.ts b/lib/server/index.ts index 43aacfdcb..76a25e5ce 100644 --- a/lib/server/index.ts +++ b/lib/server/index.ts @@ -69,7 +69,17 @@ export const main = async (serverOpts: ServerOpts) => { getForwardHttpRequestHandler(), ); - app.post('/response-data/:brokerToken/:streamingId', handlePostResponse); + if ( + loadedServerOpts.config.BROKER_SERVER_MANDATORY_AUTH_ENABLED || + loadedServerOpts.config.RESPONSE_DATA_HIDDEN_ENABLED + ) { + app.post( + '/hidden/broker/response-data/:brokerToken/:streamingId', + handlePostResponse, + ); + } else { + app.post('/response-data/:brokerToken/:streamingId', handlePostResponse); + } app.get('/', (req, res) => res.status(200).json({ ok: true, version })); diff --git a/test/functional/server-client-universal-bearer-auth.test.ts b/test/functional/server-client-universal-bearer-auth.test.ts index 1217aa8e6..a1b6182b9 100644 --- a/test/functional/server-client-universal-bearer-auth.test.ts +++ b/test/functional/server-client-universal-bearer-auth.test.ts @@ -28,9 +28,9 @@ describe('proxy requests originating from behind the broker server', () => { beforeAll(async () => { const PORT = 9999; tws = await createTestWebServer(); - + process.env.RESPONSE_DATA_HIDDEN_ENABLED = 'true'; bs = await createBrokerServer({ filters: serverAccept, port: PORT }); - + process.env.API_BASE_URL = `http://localhost:${bs.port}`; process.env.SNYK_BROKER_SERVER_UNIVERSAL_CONFIG_ENABLED = 'true'; process.env.UNIVERSAL_BROKER_ENABLED = 'true'; process.env.SERVICE_ENV = 'universaltest7'; diff --git a/test/functional/server-client-universal-pooled-credentials.test.ts b/test/functional/server-client-universal-pooled-credentials.test.ts index 2b2b07cbb..2e87f29d8 100644 --- a/test/functional/server-client-universal-pooled-credentials.test.ts +++ b/test/functional/server-client-universal-pooled-credentials.test.ts @@ -33,9 +33,9 @@ describe('proxy requests originating from behind the broker server with pooled c const PORT = 9999; tws = await createTestWebServer(); - + process.env.RESPONSE_DATA_HIDDEN_ENABLED = 'true'; bs = await createBrokerServer({ port: PORT, filters: serverAccept }); - + process.env.API_BASE_URL = `http://localhost:${bs.port}`; process.env.SNYK_BROKER_SERVER_UNIVERSAL_CONFIG_ENABLED = 'true'; process.env.UNIVERSAL_BROKER_ENABLED = 'true'; process.env.SERVICE_ENV = 'universaltestpool'; diff --git a/test/functional/server-client-universal.test.ts b/test/functional/server-client-universal.test.ts index 6da64569a..a75f90468 100644 --- a/test/functional/server-client-universal.test.ts +++ b/test/functional/server-client-universal.test.ts @@ -28,10 +28,11 @@ describe('proxy requests originating from behind the broker server', () => { beforeAll(async () => { const PORT = 9999; tws = await createTestWebServer(); - + process.env.RESPONSE_DATA_HIDDEN_ENABLED = 'true'; bs = await createBrokerServer({ filters: serverAccept, port: PORT }); process.env.SNYK_BROKER_SERVER_UNIVERSAL_CONFIG_ENABLED = 'true'; + process.env.API_BASE_URL = `http://localhost:${bs.port}`; process.env.UNIVERSAL_BROKER_ENABLED = 'true'; process.env.SERVICE_ENV = 'universaltest'; process.env.BROKER_TOKEN_1 = 'brokertoken1';