Skip to content

Commit

Permalink
tunnel server: safer delete
Browse files Browse the repository at this point in the history
do not delete entries from other clients
  • Loading branch information
Roy Razon committed Sep 10, 2023
1 parent 0d983e1 commit ac9a32f
Show file tree
Hide file tree
Showing 8 changed files with 241 additions and 121 deletions.
6 changes: 4 additions & 2 deletions tunnel-server/src/ssh/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export interface BaseSshClient extends EventEmitter {
listener: (
requestId: string,
request: ForwardRequest,
localSocketPath: string,
accept: () => Promise<ClientForward>,
reject: (reason: Error) => void,
) => void
Expand Down Expand Up @@ -179,10 +180,13 @@ export const baseSshServer = (

log.debug('emitting forward: %j', res)

const socketPath = path.join(socketDir, `s_${preevySshClient.clientId}_${randomBytes(16).toString('hex')}`)

preevySshClient.emit(
'forward',
request,
parsed,
socketPath,
() => new Promise<ClientForward>((resolveForward, rejectForward) => {
const socketServer = net.createServer(socket => {
log.debug('socketServer connected')
Expand All @@ -202,8 +206,6 @@ export const baseSshServer = (
)
})

const socketPath = path.join(socketDir, `s_${preevySshClient.clientId}_${randomBytes(16).toString('hex')}`)

const closeSocketServer = () => socketServer.close()

socketServer
Expand Down
57 changes: 30 additions & 27 deletions tunnel-server/src/ssh/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { createPublicKey } from 'crypto'
import { calculateJwkThumbprintUri, exportJWK } from 'jose'
import { inspect } from 'util'
import { Gauge } from 'prom-client'
import { baseSshServer } from './base-server'
import { ActiveTunnelStore, activeTunnelStoreKey } from '../tunnel-store'
import { ClientForward, baseSshServer } from './base-server'
import { ActiveTunnelStore, KeyAlreadyExistsError, TransactionDescriptor, activeTunnelStoreKey } from '../tunnel-store'

export const createSshServer = ({
log,
Expand Down Expand Up @@ -33,43 +33,46 @@ export const createSshServer = ({
const tunnels = new Map<string, string>()
const jwkThumbprint = (async () => await calculateJwkThumbprintUri(await exportJWK(pk)))()
client
.on('forward', async (requestId, { path: tunnelPath, access, meta, inject }, accept, reject) => {
.on('forward', async (requestId, { path: tunnelPath, access, meta, inject }, localSocketPath, accept, reject) => {
const key = activeTunnelStoreKey(clientId, tunnelPath)
if (await activeTunnelStore.has(key)) {
reject(new Error(`duplicate path: ${key}, client map contains path: ${tunnels.has(key)}`))
return
}
const thumbprint = await jwkThumbprint
const forward = await accept()
const closeTunnel = () => {
log.info('deleting tunnel %s', key)
tunnels.delete(requestId)
void activeTunnelStore.delete(key)
tunnelsGauge.dec({ clientId })
}
let isClosed = false
forward.on('close', () => {
isClosed = true
closeTunnel()
})
log.info('creating tunnel %s for localSocket %s', key, forward.localSocketPath)
await activeTunnelStore.set(key, {
log.info('creating tunnel %s for localSocket %s', key, localSocketPath)
const setTx = await activeTunnelStore.set(key, {
tunnelPath,
envId,
target: forward.localSocketPath,
target: localSocketPath,
clientId,
publicKey: pk,
access,
hostname: key,
publicKeyThumbprint: thumbprint,
publicKeyThumbprint: await jwkThumbprint,
meta,
inject,
}).catch(e => {
reject(
e instanceof KeyAlreadyExistsError
? new Error(`duplicate path: ${key}, client map contains path: ${tunnels.has(key)}`)
: new Error(`error setting tunnel ${key}: ${e}`, { cause: e }),
)
})
if (!setTx) {
return undefined
}
const forward = await accept().catch(async e => {
log.warn('error accepting forward %j: %j', requestId, inspect(e))
await activeTunnelStore.delete(key, setTx)
})
if (!forward) {
return undefined
}
tunnels.set(requestId, tunnelUrl(clientId, tunnelPath))
forward.on('close', () => {
log.info('deleting tunnel %s', key)
tunnels.delete(requestId)
void activeTunnelStore.delete(key, setTx)
tunnelsGauge.dec({ clientId })
})
tunnelsGauge.inc({ clientId })
if (isClosed) {
closeTunnel()
}
return undefined
})
.on('error', err => { log.warn('client error %j: %j', clientId, inspect(err)) })
.on('exec', (command, respondWithJson, reject) => {
Expand Down
91 changes: 0 additions & 91 deletions tunnel-server/src/tunnel-store.ts

This file was deleted.

68 changes: 68 additions & 0 deletions tunnel-server/src/tunnel-store/array-map.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { describe, expect, it, beforeEach } from '@jest/globals'
import { ArrayMap, arrayMap } from './array-map'

describe('arrayMap', () => {
let a: ArrayMap<string, { x: number }>
const expectedValues = [{ x: 12 }, { x: 13 }] as const
beforeEach(() => {
a = arrayMap()
a.add('foo', expectedValues[0])
a.add('foo', expectedValues[1])
})

describe('when the key does not exist', () => {
it('returns undefined', () => {
expect(arrayMap().get('bar')).toBeUndefined()
})
})

describe('when the key exists', () => {
let values: { x: number }[] | undefined
beforeEach(() => {
values = a.get('foo')
})
it('returns the values', () => {
expect(values).toBeDefined()
expect(values).toHaveLength(2)
expect(values).toContain(expectedValues[0])
expect(values).toContain(expectedValues[1])
})

describe('when delete is called with a predicate that returns false for everything', () => {
beforeEach(() => {
a.delete('foo', () => false)
values = a.get('foo')
})
it('does not delete the values', () => {
expect(values).toBeDefined()
expect(values).toHaveLength(2)
expect(values).toContain(expectedValues[0])
expect(values).toContain(expectedValues[1])
})
})

describe('when delete is called with a predicate that returns true for everything', () => {
beforeEach(() => {
a.delete('foo', () => true)
values = a.get('foo')
})
it('deletes the values', () => {
expect(values).toBeUndefined()
})
})

describe('when delete is called with a predicate that returns true for a specific value', () => {
beforeEach(() => {
a.delete('foo', ({ x }) => x === expectedValues[0].x)
values = a.get('foo')
})

it('deletes the specific value', () => {
expect(values).toBeDefined()
expect(values).toHaveLength(1)
expect(values).not.toContain(expectedValues[0])
expect(values).toContain(expectedValues[1])
})
})
})
})
33 changes: 33 additions & 0 deletions tunnel-server/src/tunnel-store/array-map.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
export type ArrayMap<K, V> = {
get: (key: K) => readonly V[] | undefined
add: (key: K, value: V) => void
delete: (key: K, pred: (value: V) => boolean) => void
}

export const arrayMap = <K, V>(): ArrayMap<K, V> => {
const map = new Map<K, V[]>()
return {
get: (key: K) => map.get(key),
add: (key: K, value: V) => {
let ar = map.get(key)
if (ar === undefined) {
ar = []
map.set(key, ar)
}
ar.push(value)
},
delete: (key: K, pred: (value: V) => boolean) => {
let ar = map.get(key)
if (ar === undefined) {
return undefined
}
ar = ar.filter(value => !pred(value))
if (ar.length === 0) {
map.delete(key)
} else {
map.set(key, ar)
}
return undefined
},
}
}
78 changes: 78 additions & 0 deletions tunnel-server/src/tunnel-store/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { KeyObject } from 'crypto'
import { Logger } from 'pino'
import { arrayMap } from './array-map'

export { activeTunnelStoreKey } from './key'

export type ScriptInjection = {
pathRegex?: RegExp
src: string
async?: boolean
defer?: boolean
}

export type ActiveTunnel = {
envId: string
clientId: string
tunnelPath: string
target: string
hostname: string
publicKey: KeyObject
publicKeyThumbprint: string
access: 'private' | 'public'
meta: Record<string, unknown>
inject?: ScriptInjection[]
}

export class KeyAlreadyExistsError extends Error {
constructor(readonly key: string) {
super(`key already exists: "${key}"`)
}
}

export type TransactionDescriptor = { txId: number }

export type ActiveTunnelStore = {
get: (key: string) => Promise<ActiveTunnel | undefined>
getByPkThumbprint: (pkThumbprint: string) => Promise<readonly ActiveTunnel[] | undefined>
set: (key: string, value: ActiveTunnel) => Promise<TransactionDescriptor>
delete: (key: string, tx?: TransactionDescriptor) => Promise<void>
}

const txIdGenerator = () => {
let nextTxId = 0
return {
next: () => {
const result = nextTxId
nextTxId += 1
return result
},
}
}

export const inMemoryActiveTunnelStore = ({ log }: { log: Logger }): ActiveTunnelStore => {
const keyToTunnel = new Map<string, ActiveTunnel & { txId: number }>()
const pkThumbprintToTunnel = arrayMap<string, ActiveTunnel>()
const txIdGen = txIdGenerator()
return {
get: async key => keyToTunnel.get(key),
getByPkThumbprint: async pkThumbprint => pkThumbprintToTunnel.get(pkThumbprint),
set: async (key, value) => {
if (keyToTunnel.has(key)) {
throw new KeyAlreadyExistsError(key)
}
const txId = txIdGen.next()
log.debug('setting tunnel %s: %j', key, txId, value)
keyToTunnel.set(key, Object.assign(value, { txId }))
pkThumbprintToTunnel.add(value.publicKeyThumbprint, value)
return { txId }
},
delete: async (key, tx) => {
const tunnel = keyToTunnel.get(key)
if (tunnel && (tx === undefined || tunnel.txId === tx.txId)) {
pkThumbprintToTunnel.delete(tunnel.publicKeyThumbprint, ({ hostname }) => hostname === key)
keyToTunnel.delete(key)
}
},
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable jest/no-standalone-expect */
import { it, describe, expect } from '@jest/globals'
import { createHash } from 'crypto'
import { activeTunnelStoreKey } from './tunnel-store'
import { activeTunnelStoreKey } from './key'

describe('tunnel store key formatting', () => {
it('should create the format {envId}-{clientId}', () => {
Expand Down
Loading

0 comments on commit ac9a32f

Please sign in to comment.