fix: allow connections via IP address as host (#641)

* fix: allow connections via IP address as host

Currently, we're passing in a root CA for all requests as SSL option. However, when passing in an IP address, this will always fail. As we are starting to transition storage over to pgbouncer, internal connections require connecting via an IPv6 address. To support this, we need to not include the root CA options, in case we're connecting against an IP.

* unify

* decode uri component

Author: kevcodez

.github/workflows/ci.yml

@@ -19,15 +19,15 @@ jobs:
     runs-on: ${{ matrix.platform }}
 
     steps:
-      - uses: actions/checkout@v2
-      - uses: actions/cache@v1
+      - uses: actions/checkout@v4
+      - uses: actions/cache@v4
         with:
           path: ~/.npm
           key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
           restore-keys: |
             ${{ runner.os }}-node-
       - name: Set up Node
-        uses: actions/setup-node@v1
+        uses: actions/setup-node@v4
         with:
           node-version: ${{ matrix.node }}
 

.github/workflows/cli.yaml

@@ -24,7 +24,7 @@ jobs:
           git config --global user.email '${{ env.github_user_email }}'
 
       - name: Checkout remote repository
-        uses: actions/checkout@v2
+        uses: actions/checkout@v4
         with:
           repository: '${{ env.cli_repo_owner }}/${{ env.cli_repo }}'
           token: ${{ secrets.CLI_PR_USER_ACCESS_TOKEN }}

.github/workflows/docs.yml

@@ -18,15 +18,15 @@ jobs:
     runs-on: ${{ matrix.platform }}
 
     steps:
-      - uses: actions/checkout@v2
-      - uses: actions/cache@v1
+      - uses: actions/checkout@v4
+      - uses: actions/cache@v4
         with:
           path: ~/.npm
           key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }}
           restore-keys: |
             ${{ runner.os }}-node-
       - name: Set up Node
-        uses: actions/setup-node@v1
+        uses: actions/setup-node@v4
         with:
           node-version: ${{ matrix.node }}
 

.github/workflows/release.yml

@@ -13,10 +13,10 @@ jobs:
       published: ${{ steps.semantic.outputs.new_release_published }}
       version: ${{ steps.semantic.outputs.new_release_version }}
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - name: Set up Node
-        uses: actions/setup-node@v1
+        uses: actions/setup-node@v4
         with:
           node-version: 14 # https://github.com/cycjimmy/semantic-release-action/issues/159
 

src/internal/database/connection.ts

@@ -8,6 +8,7 @@ import { DbActiveConnection, DbActivePool } from '../monitoring/metrics'
 import KnexTimeoutError = knex.KnexTimeoutError
 import { logger, logSchema } from '../monitoring'
 import { ERRORS } from '@internal/errors'
+import { getSslSettings } from './util'
 
 // https://github.com/knex/knex/issues/387#issuecomment-51554522
 pg.types.setTypeParser(20, 'text', parseInt)
@@ -95,6 +96,8 @@ export class TenantConnection {
 
     const isExternalPool = Boolean(options.isExternalPool)
 
+    const sslSettings = getSslSettings({ connectionString, databaseSSLRootCert })
+
     knexPool = knex({
       client: 'pg',
       version: dbPostgresVersion,
@@ -113,7 +116,7 @@ export class TenantConnection {
       connection: {
         connectionString: connectionString,
         connectionTimeoutMillis: databaseConnectionTimeout,
-        ...this.sslSettings(),
+        ssl: sslSettings ? { ...sslSettings } : undefined,
       },
       acquireConnectionTimeout: databaseConnectionTimeout,
     })
@@ -143,13 +146,6 @@ export class TenantConnection {
     return new this(knexPool, options)
   }
 
-  protected static sslSettings() {
-    if (databaseSSLRootCert) {
-      return { ssl: { ca: databaseSSLRootCert } }
-    }
-    return {}
-  }
-
   async dispose() {
     if (this.options.isExternalPool) {
       await this.pool.destroy()

src/internal/database/migrations/migrate.ts

@@ -13,6 +13,7 @@ import { ProgressiveMigrations } from './progressive'
 import { RunMigrationsOnTenants, ResetMigrationsOnTenant } from '@storage/events'
 import { ERRORS } from '@internal/errors'
 import { DBMigration } from './types'
+import { getSslSettings } from '../util'
 
 const {
   multitenantDatabaseUrl,
@@ -312,16 +313,10 @@ export async function runMigrationsOnTenant(
   tenantId?: string,
   waitForLock = true
 ): Promise<void> {
-  let ssl: ClientConfig['ssl'] | undefined = undefined
-
-  if (databaseSSLRootCert) {
-    ssl = { ca: databaseSSLRootCert }
-  }
-
   await connectAndMigrate({
     databaseUrl,
     migrationsDirectory: './migrations/tenant',
-    ssl,
+    ssl: getSslSettings({ connectionString: databaseUrl, databaseSSLRootCert }),
     shouldCreateStorageSchema: true,
     tenantId,
     waitForLock,

src/internal/database/util.ts

@@ -0,0 +1,35 @@
+import { logger } from '@internal/monitoring'
+
+export function getSslSettings({
+  connectionString,
+  databaseSSLRootCert,
+}: {
+  connectionString: string
+  databaseSSLRootCert: string | undefined
+}): { ca: string } | undefined {
+  if (!databaseSSLRootCert) return undefined
+
+  try {
+    // When connecting through PGBouncer, we connect through an IPv6 address rather than a hostname
+    // When passing in the root CA for SSL, this will always fail, so we need to skip passing in the SSL root cert
+    // in case the hostname is an IP address
+    const url = new URL(connectionString)
+    if (url.hostname && isIpAddress(url.hostname)) {
+      return undefined
+    }
+  } catch (err) {
+    // ignore to ensure this never breaks the connection in case of an invalid URL
+    logger.warn(err, 'Failed to parse connection string')
+  }
+
+  return { ca: databaseSSLRootCert }
+}
+
+export function isIpAddress(ip: string) {
+  const ipv4Pattern = /^(\d{1,3}\.){3}\d{1,3}$/
+  const ipv6Pattern = /^([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}$/
+
+  // IP might be URL-encoded and won't match the regex unless we decode first
+  const decodedIp = decodeURIComponent(ip)
+  return ipv4Pattern.test(decodedIp) || ipv6Pattern.test(decodedIp)
+}

src/test/database/util.test.ts

@@ -0,0 +1,53 @@
+import { getSslSettings, isIpAddress } from '@internal/database/util'
+
+describe('database utils', () => {
+  test.each([
+    ['', false],
+    ['test', false],
+    ['db.foobar.supabase.red', false],
+    ['5.5.5.a', false],
+    ['5.5.5.5', true],
+    ['121.212.187.123', true],
+    ['121.212.187.5', true],
+    ['2001:db8:3333:4444:5555:6666:7777:8888', true],
+    ['2001:db8:3333:4444:CCCC:DDDD:EEEE:FFFF', true],
+    ['2406%3Ada18%3A4fd%3A9b09%3A2c76%3A5d38%3Ade30%3A7904', true],
+  ])('is %s ip address, expected %s', (text: string, expected: boolean) => {
+    expect(isIpAddress(text)).toBe(expected)
+  })
+
+  describe('getSslSettings', () => {
+    test('should return no SSL settings if database root CA not set', () => {
+      expect(
+        getSslSettings({ connectionString: 'foobar', databaseSSLRootCert: undefined })
+      ).toBeUndefined()
+    })
+
+    test('should return no SSL settings if hostname is an IP address', () => {
+      expect(
+        getSslSettings({
+          connectionString: 'postgres://foo:[email protected]:5432/postgres',
+          databaseSSLRootCert: '<cert>',
+        })
+      ).toBeUndefined()
+    })
+
+    test('should return SSL settings if hostname is not an IP address', () => {
+      expect(
+        getSslSettings({
+          connectionString: 'postgres://foo:[email protected]:5432/postgres',
+          databaseSSLRootCert: '<cert>',
+        })
+      ).toStrictEqual({ ca: '<cert>' })
+    })
+
+    test('should return SSL settings if hostname is not parseable', () => {
+      expect(
+        getSslSettings({
+          connectionString: 'postgres://foo:[email protected]."red:5432/postgres',
+          databaseSSLRootCert: '<cert>',
+        })
+      ).toStrictEqual({ ca: '<cert>' })
+    })
+  })
+})