Skip to content

chore: port browser-proxy to pg-gateway Web API #100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion apps/browser-proxy/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
"dependencies": {
"@aws-sdk/client-s3": "^3.645.0",
"debug": "^4.3.7",
"expiry-map": "^2.0.0",
"findhit-proxywrap": "^0.3.13",
"pg-gateway": "0.3.0-alpha.7",
"p-memoize": "^7.1.1",
"pg-gateway": "^0.3.0-alpha.7",
"ws": "^8.18.0"
},
"devDependencies": {
Expand Down
64 changes: 21 additions & 43 deletions apps/browser-proxy/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,43 +1,34 @@
import * as nodeNet from 'node:net'
import * as https from 'node:https'
import { PostgresConnection } from 'pg-gateway'
import { BackendError, PostgresConnection } from 'pg-gateway'
import { fromNodeSocket } from 'pg-gateway/node'
import { WebSocketServer, type WebSocket } from 'ws'
import makeDebug from 'debug'
import * as tls from 'node:tls'
import { extractDatabaseId, isValidServername } from './servername.ts'
import { getTls } from './tls.ts'
import { getTls, setSecureContext } from './tls.ts'
import { createStartupMessage } from './create-message.ts'
import { extractIP } from './extract-ip.ts'

const debug = makeDebug('browser-proxy')

const tcpConnections = new Map<string, nodeNet.Socket>()
const tcpConnections = new Map<string, PostgresConnection>()
const websocketConnections = new Map<string, WebSocket>()

let tlsOptions = await getTls()

// refresh the TLS certificate every week
setInterval(
async () => {
tlsOptions = await getTls()
httpsServer.setSecureContext(tlsOptions)
},
1000 * 60 * 60 * 24 * 7
)

const httpsServer = https.createServer({
...tlsOptions,
SNICallback: (servername, callback) => {
debug('SNICallback', servername)
if (isValidServername(servername)) {
debug('SNICallback', 'valid')
callback(null, tls.createSecureContext(tlsOptions))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the end we don't need that, the default secureContext is used.

callback(null)
} else {
debug('SNICallback', 'invalid')
callback(new Error('invalid SNI'))
}
},
})
await setSecureContext(httpsServer)
// reset the secure context every week to pick up any new TLS certificates
setInterval(() => setSecureContext(httpsServer), 1000 * 60 * 60 * 24 * 7)

const websocketServer = new WebSocketServer({
server: httpsServer,
Expand Down Expand Up @@ -70,8 +61,8 @@ websocketServer.on('connection', (socket, request) => {

socket.on('message', (data: Buffer) => {
debug('websocket message', data.toString('hex'))
const tcpSocket = tcpConnections.get(databaseId)
tcpSocket?.write(data)
const tcpConnection = tcpConnections.get(databaseId)
tcpConnection?.streamWriter?.write(data)
Copy link
Contributor Author

@jgoux jgoux Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is why I needed to add the streamWriter property in pg-gateway, because getting a new writer from duplex here wouldn't work as the stream.writable is locked by processData.

})

socket.on('close', () => {
Expand All @@ -86,50 +77,41 @@ const net = (

const tcpServer = net.createServer()

tcpServer.on('connection', (socket) => {
tcpServer.on('connection', async (socket) => {
let databaseId: string | undefined

const connection = new PostgresConnection(socket, {
tls: tlsOptions,
const connection = await fromNodeSocket(socket, {
tls: getTls,
Comment on lines -93 to +84
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now the TLS certificates should be correctly refreshed after some time. 😅

onTlsUpgrade(state) {
if (!state.tlsInfo?.sniServerName || !isValidServername(state.tlsInfo.sniServerName)) {
// connection.detach()
connection.sendError({
if (!state.tlsInfo?.serverName || !isValidServername(state.tlsInfo.serverName)) {
throw BackendError.create({
code: '08006',
message: 'invalid SNI',
severity: 'FATAL',
})
connection.end()
return
}

const _databaseId = extractDatabaseId(state.tlsInfo.sniServerName!)
const _databaseId = extractDatabaseId(state.tlsInfo.serverName!)

if (!websocketConnections.has(_databaseId!)) {
// connection.detach()
connection.sendError({
throw BackendError.create({
code: 'XX000',
message: 'the browser is not sharing the database',
severity: 'FATAL',
})
connection.end()
return
}

if (tcpConnections.has(_databaseId)) {
// connection.detach()
connection.sendError({
throw BackendError.create({
code: '53300',
message: 'sorry, too many clients already',
severity: 'FATAL',
})
connection.end()
return
}

// only set the databaseId after we've verified the connection
databaseId = _databaseId
tcpConnections.set(databaseId!, connection.socket)
tcpConnections.set(databaseId!, connection)
},
serverVersion() {
return '16.3'
Expand All @@ -138,13 +120,11 @@ tcpServer.on('connection', (socket) => {
const websocket = websocketConnections.get(databaseId!)

if (!websocket) {
connection.sendError({
throw BackendError.create({
code: 'XX000',
message: 'the browser is not sharing the database',
severity: 'FATAL',
})
connection.end()
return
}

const clientIpMessage = createStartupMessage('postgres', 'postgres', {
Expand All @@ -160,13 +140,11 @@ tcpServer.on('connection', (socket) => {
const websocket = websocketConnections.get(databaseId!)

if (!websocket) {
connection.sendError({
throw BackendError.create({
code: 'XX000',
message: 'the browser is not sharing the database',
severity: 'FATAL',
})
connection.end()
return
}

debug('tcp message', { message })
Expand Down
14 changes: 13 additions & 1 deletion apps/browser-proxy/src/tls.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { Buffer } from 'node:buffer'
import { GetObjectCommand, S3Client } from '@aws-sdk/client-s3'
import pMemoize from 'p-memoize'
import ExpiryMap from 'expiry-map'
import type { Server } from 'node:https'

const s3Client = new S3Client({ forcePathStyle: true })

export async function getTls() {
async function _getTls() {
const cert = await s3Client
.send(
new GetObjectCommand({
Expand Down Expand Up @@ -31,3 +34,12 @@ export async function getTls() {
key: Buffer.from(key),
}
}

// cache the TLS certificate for 1 week
const cache = new ExpiryMap(1000 * 60 * 60 * 24 * 7)
Comment on lines +38 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this server restarted 1 day before renewal? I suppose the old cert is still good for another week, so this should be safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check but I think we renew the certificate monthly while the validity is 3 months. So in any cases with 2 week refresh we should be safe.

export const getTls = pMemoize(_getTls, { cache })

export async function setSecureContext(httpsServer: Server) {
const tlsOptions = await getTls()
httpsServer.setSecureContext(tlsOptions)
}
12 changes: 11 additions & 1 deletion apps/postgres-new/components/app-provider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,17 @@ export default function AppProvider({ children }: AppProps) {
if (isStartupMessage(message)) {
const parameters = parseStartupMessage(message)
if ('client_ip' in parameters) {
setConnectedClientIp(parameters.client_ip === '' ? null : parameters.client_ip)
// client disconnected
if (parameters.client_ip === '') {
setConnectedClientIp(null)
// we ensure we're not in a transaction block first
await db.sql`rollback;`.catch()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discard all can't be run in a transaction block and it seems like after a pg_dump we still have a transaction opened somehow. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How bizarre!

// we clean the session state, see: https://www.pgbouncer.org/faq.html#how-to-use-prepared-statements-with-session-pooling
// we do this to avoid having old prepared statements in the session
await db.sql`discard all;`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added after working on making pg_dump works. Running pg_dump twice showed that we kept old prepared statements in the session for the next connected client, which ended up erroring as the next pg_dump creates the same prepare statement.

Copy link
Collaborator

@gregnr gregnr Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we should mention rollback + discard all in the pg-gateway pglite examples too? I imagine this would apply to any pg-gateway + PGlite stack.

} else {
setConnectedClientIp(parameters.client_ip)
}
}
return
}
Expand Down
4 changes: 2 additions & 2 deletions apps/postgres-new/components/live-share-icon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ export const LiveShareIcon = (props: { size?: number; className?: string }) => (
className={cx('lucide lucide-live-share', props.className)}
>
<path
fill-rule="evenodd"
clip-rule="evenodd"
fillRule="evenodd"
clipRule="evenodd"
d="M13.735 1.694L15.178 1l8.029 6.328v1.388l-8.029 6.072-1.443-.694v-2.776h-.59c-4.06-.02-6.71.104-10.61 5.163l-1.534-.493a8.23 8.23 0 0 1 .271-2.255 11.026 11.026 0 0 1 3.92-6.793 11.339 11.339 0 0 1 7.502-2.547h1.04v-2.7zm1.804 7.917v2.776l5.676-4.281-5.648-4.545v2.664h-2.86A9.299 9.299 0 0 0 5.77 8.848a10.444 10.444 0 0 0-2.401 4.122c3.351-3.213 6.19-3.359 9.798-3.359h2.373zm-7.647 5.896a4.31 4.31 0 1 1 4.788 7.166 4.31 4.31 0 0 1-4.788-7.166zm.955 5.728a2.588 2.588 0 1 0 2.878-4.302 2.588 2.588 0 0 0-2.878 4.302z"
/>
</svg>
Expand Down
77 changes: 76 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.