diff --git a/packages/agent/src/routes/security/client-ip.ts b/packages/agent/src/routes/security/client-ip.ts new file mode 100644 index 0000000000..b13ccc8b9c --- /dev/null +++ b/packages/agent/src/routes/security/client-ip.ts @@ -0,0 +1,36 @@ +import { BlockList, isIPv6 } from 'net'; + +const TRUSTED_PROXY_SUBNETS: [ip: string, prefix: number, type: 'ipv4' | 'ipv6'][] = [ + ['127.0.0.0', 8, 'ipv4'], + ['10.0.0.0', 8, 'ipv4'], + ['172.16.0.0', 12, 'ipv4'], + ['192.168.0.0', 16, 'ipv4'], + ['169.254.0.0', 16, 'ipv4'], + ['::1', 128, 'ipv6'], + ['fc00::', 7, 'ipv6'], + ['fe80::', 10, 'ipv6'], +]; + +const trustedProxies = new BlockList(); +TRUSTED_PROXY_SUBNETS.forEach(([ip, prefix, type]) => trustedProxies.addSubnet(ip, prefix, type)); + +export const ipVersion = (ip: string): 'ipv4' | 'ipv6' => (isIPv6(ip) ? 'ipv6' : 'ipv4'); + +const isTrustedProxy = (ip: string): boolean => trustedProxies.check(ip.trim(), ipVersion(ip)); + +const resolveClientIp = (socketIp: string, forwardedFor?: string | string[]): string => { + const header = `${forwardedFor ?? ''}`; + const hops = header === '' ? [] : header.split(',').map(hop => hop.trim()); + + hops.push(socketIp.trim()); + + let index = hops.length - 1; + + while (index > 0 && isTrustedProxy(hops[index])) { + index -= 1; + } + + return hops[index]; +}; + +export default resolveClientIp; diff --git a/packages/agent/src/routes/security/ip-whitelist.ts b/packages/agent/src/routes/security/ip-whitelist.ts index 0b8fb94755..2ba53c7055 100644 --- a/packages/agent/src/routes/security/ip-whitelist.ts +++ b/packages/agent/src/routes/security/ip-whitelist.ts @@ -2,17 +2,16 @@ import type { IpWhitelistConfiguration } from '@forestadmin/forestadmin-client'; import type Router from '@koa/router'; import type { Context, Next } from 'koa'; -import { BlockList, isIPv6 } from 'net'; +import { BlockList } from 'net'; import { HttpCode, RouteType } from '../../types'; import BaseRoute from '../base-route'; +import resolveClientIp, { ipVersion } from './client-ip'; type IpRule = IpWhitelistConfiguration['ipRules'][number]; const LOOPBACK_IPS = ['127.0.0.1', '::1', '::ffff:127.0.0.1']; -const ipVersion = (ip: string): 'ipv4' | 'ipv6' => (isIPv6(ip) ? 'ipv6' : 'ipv4'); - export default class IpWhitelist extends BaseRoute { type = RouteType.Authentication; @@ -58,7 +57,10 @@ export default class IpWhitelist extends BaseRoute { return next(); } - const currentIp = `${context.request.headers['x-forwarded-for'] ?? context.request.ip}`; + const currentIp = resolveClientIp( + context.req?.socket?.remoteAddress ?? '', + context.request.headers['x-forwarded-for'], + ); if (!this.allowedIps.check(currentIp, ipVersion(currentIp))) { return context.throw(HttpCode.Forbidden, `IP address rejected (${currentIp})`); diff --git a/packages/agent/test/routes/security/client-ip.test.ts b/packages/agent/test/routes/security/client-ip.test.ts new file mode 100644 index 0000000000..e4823b84d1 --- /dev/null +++ b/packages/agent/test/routes/security/client-ip.test.ts @@ -0,0 +1,90 @@ +import resolveClientIp from '../../../src/routes/security/client-ip'; + +describe('resolveClientIp', () => { + describe('when the socket peer is a public address', () => { + test.each(['9.9.9.9', '', '9.9.9.9, 10.20.15.10', '127.0.0.1'])( + 'resolves to the socket peer with header %s', + forwardedFor => { + expect(resolveClientIp('203.0.113.77', forwardedFor)).toBe('203.0.113.77'); + }, + ); + + test('resolves to the socket peer when there is no header', () => { + expect(resolveClientIp('203.0.113.77')).toBe('203.0.113.77'); + }); + + test('resolves to an ipv6 socket peer', () => { + expect(resolveClientIp('2001:db8::1', '9.9.9.9')).toBe('2001:db8::1'); + }); + + test('resolves to an ipv4-mapped public socket peer', () => { + expect(resolveClientIp('::ffff:203.0.113.5', '9.9.9.9')).toBe('::ffff:203.0.113.5'); + }); + }); + + describe('when the socket peer is a trusted proxy', () => { + test('resolves to the single hop behind a private proxy', () => { + expect(resolveClientIp('10.0.0.2', '9.9.9.9')).toBe('9.9.9.9'); + }); + + test('resolves to the rightmost untrusted hop of a multi-hop header', () => { + expect(resolveClientIp('10.0.0.2', '9.9.9.9, 203.0.113.77')).toBe('203.0.113.77'); + }); + + test('walks a chain of trusted proxies from the right', () => { + expect(resolveClientIp('127.0.0.1', '9.9.9.9, 10.0.0.3')).toBe('9.9.9.9'); + }); + + test('returns the leftmost hop when the whole chain is trusted', () => { + expect(resolveClientIp('127.0.0.1', '10.0.0.3, 192.168.1.1')).toBe('10.0.0.3'); + }); + + test.each([ + ['10.0.0.2', '172.16.0.5'], + ['10.0.0.2', '192.168.1.1'], + ['10.0.0.2', '127.0.0.99'], + ['10.0.0.2', '169.254.1.1'], + ['10.0.0.2', '::1'], + ['fd00::1', '2001:db8::1'], + ['fe80::1', '2001:db8::1'], + ['10.0.0.2', 'fd12::1'], + ['::ffff:10.0.0.2', '203.0.113.77'], + ])('resolves to the hop behind trusted proxy socket %s', (socketIp, expected) => { + expect(resolveClientIp(socketIp, expected)).toBe(expected); + }); + + test.each(['::ffff:10.0.0.3', '::ffff:a00:3', '::FFFF:10.0.0.3'])( + 'skips the ipv4-mapped trusted proxy hop %s while walking', + mappedProxy => { + expect(resolveClientIp('10.0.0.2', `203.0.113.77, ${mappedProxy}`)).toBe('203.0.113.77'); + }, + ); + + test('trims the hops whatever the spacing', () => { + expect(resolveClientIp(' 10.0.0.2 ', ' 203.0.113.77 , 10.0.0.3 ')).toBe('203.0.113.77'); + }); + + test('joins a repeated header and walks it from the right', () => { + expect(resolveClientIp('10.0.0.2', ['203.0.113.77', '10.0.0.3'])).toBe('203.0.113.77'); + }); + }); + + describe('when a hop is not a valid address', () => { + test('returns a malformed hop met during the walk instead of skipping it', () => { + expect(resolveClientIp('10.0.0.2', '9.9.9.9, not-an-ip')).toBe('not-an-ip'); + }); + + test('returns a hop carrying a port instead of skipping it', () => { + expect(resolveClientIp('10.0.0.2', '203.0.113.77:1234')).toBe('203.0.113.77:1234'); + }); + + test('returns an empty slot met during the walk', () => { + expect(resolveClientIp('10.0.0.2', ', 10.0.0.3')).toBe(''); + }); + + test('returns an empty string when the socket cannot be read and no header exists', () => { + expect(resolveClientIp('')).toBe(''); + expect(resolveClientIp('', undefined)).toBe(''); + }); + }); +}); diff --git a/packages/agent/test/routes/security/ip-whitelist.test.ts b/packages/agent/test/routes/security/ip-whitelist.test.ts index 330515c2fd..5b58e5577a 100644 --- a/packages/agent/test/routes/security/ip-whitelist.test.ts +++ b/packages/agent/test/routes/security/ip-whitelist.test.ts @@ -74,6 +74,16 @@ describe('IpWhitelist', () => { return ipWhitelistService; }; + const peerContext = (opts: { socketIp: string; xff?: string | null }): Context => + ({ + req: { socket: { remoteAddress: opts.socketIp } }, + request: { + ip: opts.socketIp, + headers: { 'x-forwarded-for': opts.xff ?? null }, + }, + throw: jest.fn(), + } as unknown as Context); + test('should call the next callback', async () => { const ipWhitelistService = await setupIpWhitelistService({ isFeatureEnabled: false, @@ -99,11 +109,7 @@ describe('IpWhitelist', () => { ipRules: [rule], }); - // The ip property of the koa context is not supposed to be changed - // Thus, forging a manual context is the only way of testing this function - const context = { - request: { ip: '10.20.15.10', headers: { 'x-forwarded-for': null } }, - } as unknown as Context; + const context = peerContext({ socketIp: '10.20.15.10' }); const next = jest.fn() as Next; await ipWhitelistService.checkIp(context, next); @@ -125,7 +131,7 @@ describe('IpWhitelist', () => { const notAllowedIp = '10.20.15.1'; - const context = createMockContext({ headers: { 'x-forwarded-for': notAllowedIp } }); + const context = peerContext({ socketIp: '10.0.0.2', xff: notAllowedIp }); const next = jest.fn() as Next; await ipWhitelistService.checkIp(context, next); @@ -146,7 +152,7 @@ describe('IpWhitelist', () => { }); const allowedIp = '10.20.15.10'; - const context = createMockContext({ headers: { 'x-forwarded-for': allowedIp } }); + const context = peerContext({ socketIp: '10.0.0.2', xff: allowedIp }); const next = jest.fn() as Next; await ipWhitelistService.checkIp(context, next); @@ -166,7 +172,7 @@ describe('IpWhitelist', () => { ipRules: [rule], }); - const context = createMockContext({ headers: { 'x-forwarded-for': ip } }); + const context = peerContext({ socketIp: '10.0.0.2', xff: ip }); const next = jest.fn() as Next; await ipWhitelistService.checkIp(context, next); @@ -181,7 +187,7 @@ describe('IpWhitelist', () => { ipRules: [{ type: 2, range: '2001:db8::/32' }], }); - const context = createMockContext({ headers: { 'x-forwarded-for': '2001:db9::1' } }); + const context = peerContext({ socketIp: '10.0.0.2', xff: '2001:db9::1' }); const next = jest.fn() as Next; await ipWhitelistService.checkIp(context, next); @@ -201,9 +207,7 @@ describe('IpWhitelist', () => { ipRules: [{ type: 2, range: '10.20.15.0/24' }], }); - const context = createMockContext({ - headers: { 'x-forwarded-for': '::ffff:10.20.15.10' }, - }); + const context = peerContext({ socketIp: '10.0.0.2', xff: '::ffff:10.20.15.10' }); const next = jest.fn() as Next; await ipWhitelistService.checkIp(context, next); @@ -227,7 +231,7 @@ describe('IpWhitelist', () => { ipRules: [rule], }); - const context = createMockContext({ headers: { 'x-forwarded-for': '10.20.15.10' } }); + const context = peerContext({ socketIp: '10.0.0.2', xff: '10.20.15.10' }); const next = jest.fn() as Next; await ipWhitelistService.checkIp(context, next); @@ -263,7 +267,7 @@ describe('IpWhitelist', () => { 'IP whitelist: ignoring invalid rule {"type":0,"ip":"not-an-ip"}', ); - const context = createMockContext({ headers: { 'x-forwarded-for': '10.20.15.10' } }); + const context = peerContext({ socketIp: '10.0.0.2', xff: '10.20.15.10' }); const next = jest.fn() as Next; await ipWhitelistService.checkIp(context, next); @@ -273,25 +277,67 @@ describe('IpWhitelist', () => { }); describe('when the ip cannot be read', () => { - test.each(['', 'not-an-ip', '10.20.15.10, 10.20.15.11'])( - 'should reject the caller sending %s', - async forwardedFor => { + test.each([ + { socketIp: '203.0.113.5', forwardedFor: '', judged: '203.0.113.5' }, + { socketIp: '10.0.0.2', forwardedFor: 'not-an-ip', judged: 'not-an-ip' }, + ])( + 'should reject the caller judged on $judged', + async ({ socketIp, forwardedFor, judged }) => { const ipWhitelistService = await setupIpWhitelistService({ isFeatureEnabled: true, ipRules: [{ type: 0, ip: '10.20.15.10' }], }); - const context = createMockContext({ - headers: { 'x-forwarded-for': forwardedFor }, - }); + const context = peerContext({ socketIp, xff: forwardedFor }); const next = jest.fn() as Next; await ipWhitelistService.checkIp(context, next); expect(next).not.toHaveBeenCalled(); - expect(context.throw).toHaveBeenCalledWith(HttpCode.Forbidden, expect.any(String)); + expect(context.throw).toHaveBeenCalledWith( + HttpCode.Forbidden, + `IP address rejected (${judged})`, + ); }, ); + + test('should use the rightmost untrusted hop behind a trusted proxy', async () => { + const ipWhitelistService = await setupIpWhitelistService({ + isFeatureEnabled: true, + ipRules: [{ type: 0, ip: '9.9.9.9' }], + }); + + const context = peerContext({ socketIp: '10.0.0.2', xff: '9.9.9.9, 203.0.113.77' }); + const next = jest.fn() as Next; + + await ipWhitelistService.checkIp(context, next); + + expect(next).not.toHaveBeenCalled(); + expect(context.throw).toHaveBeenCalledWith( + HttpCode.Forbidden, + 'IP address rejected (203.0.113.77)', + ); + }); + }); + + describe('when the socket peer is a public address', () => { + test('checks the socket address', async () => { + const ipWhitelistService = await setupIpWhitelistService({ + isFeatureEnabled: true, + ipRules: [{ type: 0, ip: '9.9.9.9' }], + }); + + const context = peerContext({ socketIp: '203.0.113.77', xff: '9.9.9.9' }); + const next = jest.fn() as Next; + + await ipWhitelistService.checkIp(context, next); + + expect(next).not.toHaveBeenCalled(); + expect(context.throw).toHaveBeenCalledWith( + HttpCode.Forbidden, + 'IP address rejected (203.0.113.77)', + ); + }); }); }); @@ -301,22 +347,11 @@ describe('IpWhitelist', () => { ipRules: [{ type: 0, ip: '10.20.15.10' }], }; - // socketIp = the true socket peer (req.socket.remoteAddress); xff is the proxy hop marker. - const forgeContext = (opts: { socketIp: string; xff?: string | null }): Context => - ({ - req: { socket: { remoteAddress: opts.socketIp } }, - request: { - ip: opts.socketIp, - headers: { 'x-forwarded-for': opts.xff ?? null }, - }, - throw: jest.fn(), - } as unknown as Context); - test.each(['127.0.0.1', '::1', '::ffff:127.0.0.1'])( 'exempts a direct loopback caller with no proxy hop from socket %s', async socketIp => { const service = await setupIpWhitelistService(restrictiveRules); - const context = forgeContext({ socketIp }); + const context = peerContext({ socketIp }); const next = jest.fn() as Next; await service.checkIp(context, next); @@ -328,7 +363,7 @@ describe('IpWhitelist', () => { test('does NOT exempt a non-loopback socket', async () => { const service = await setupIpWhitelistService(restrictiveRules); - const context = forgeContext({ socketIp: '203.0.113.5' }); + const context = peerContext({ socketIp: '203.0.113.5' }); const next = jest.fn() as Next; await service.checkIp(context, next); @@ -341,7 +376,7 @@ describe('IpWhitelist', () => { // x-forwarded-for → the exemption must NOT fire, so the real IP is checked normally. test('does NOT exempt a loopback socket that carries x-forwarded-for (proxy hop)', async () => { const service = await setupIpWhitelistService(restrictiveRules); - const context = forgeContext({ socketIp: '127.0.0.1', xff: '203.0.113.5' }); + const context = peerContext({ socketIp: '127.0.0.1', xff: '203.0.113.5' }); const next = jest.fn() as Next; await service.checkIp(context, next); @@ -353,11 +388,9 @@ describe('IpWhitelist', () => { ); }); - // Anti-spoof: an external socket carrying x-forwarded-for: 127.0.0.1 must NOT be exempted - // (exemption reads the socket, not the header) and the header IP is what gets rejected. - test('ignores a spoofed x-forwarded-for loopback (socket is external)', async () => { + test('does not exempt a loopback hop when the socket is external', async () => { const service = await setupIpWhitelistService(restrictiveRules); - const context = forgeContext({ socketIp: '203.0.113.5', xff: '127.0.0.1' }); + const context = peerContext({ socketIp: '203.0.113.5', xff: '127.0.0.1' }); const next = jest.fn() as Next; await service.checkIp(context, next); @@ -365,13 +398,13 @@ describe('IpWhitelist', () => { expect(next).not.toHaveBeenCalled(); expect(context.throw).toHaveBeenCalledWith( HttpCode.Forbidden, - `IP address rejected (127.0.0.1)`, + `IP address rejected (203.0.113.5)`, ); }); test('a proxied user from a whitelisted IP still passes the normal check', async () => { const service = await setupIpWhitelistService(restrictiveRules); - const context = forgeContext({ socketIp: '127.0.0.1', xff: '10.20.15.10' }); + const context = peerContext({ socketIp: '127.0.0.1', xff: '10.20.15.10' }); const next = jest.fn() as Next; await service.checkIp(context, next);