Fix SASL to bubble up errors, enable SASL tests in CI, and add inform…

…ative empty SASL password message (#2901) * Enable SASL tests in GitHub actions CI * Add SASL test to ensure that client password is a string * Fix SASL error handling to emit and bubble up errors * Add informative error when SASL password is empty string
brianc · Jan 23, 2023 · bb8745b · bb8745b
1 parent f82f39c
commit bb8745b
Show file tree

Hide file tree

Showing 5 changed files with 94 additions and 7 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -15,6 +15,7 @@ jobs:
           POSTGRES_USER: postgres
           POSTGRES_PASSWORD: postgres
           POSTGRES_DB: ci_db_test
+          POSTGRES_HOST_AUTH_METHOD: 'md5'
         ports:
           - 5432:5432
         options: --health-cmd pg_isready --health-interval 10s --health-timeout 5s --health-retries 5
@@ -23,7 +24,19 @@ jobs:
         node: ['10', '12', '14', '16', '18']
         os: [ubuntu-latest, windows-latest, macos-latest]
     name: Node.js ${{ matrix.node }} (${{ matrix.os }})
+    env:
+        PGUSER: postgres
+        PGHOST: localhost
+        PGPASSWORD: postgres
+        PGDATABASE: ci_db_test
+        PGTESTNOSSL: 'true'
+        SCRAM_TEST_PGUSER: scram_test
+        SCRAM_TEST_PGPASSWORD: test4scram
     steps:
+      - run: |
+          psql \
+            -c "SET password_encryption = 'scram-sha-256'" \
+            -c "CREATE ROLE scram_test LOGIN PASSWORD 'test4scram'"
       - uses: actions/checkout@v3
         with:
           persist-credentials: false
@@ -34,4 +47,4 @@ jobs:
           cache: yarn
       - run: yarn install
       # TODO(bmc): get ssl tests working in ci
-      - run: PGTESTNOSSL=true PGUSER=postgres PGPASSWORD=postgres PGDATABASE=ci_db_test yarn test
+      - run: yarn test
diff --git a/packages/pg/lib/client.js b/packages/pg/lib/client.js
@@ -247,19 +247,31 @@ class Client extends EventEmitter {
 
   _handleAuthSASL(msg) {
     this._checkPgPass(() => {
-      this.saslSession = sasl.startSession(msg.mechanisms)
-      this.connection.sendSASLInitialResponseMessage(this.saslSession.mechanism, this.saslSession.response)
+      try {
+        this.saslSession = sasl.startSession(msg.mechanisms)
+        this.connection.sendSASLInitialResponseMessage(this.saslSession.mechanism, this.saslSession.response)
+      } catch (err) {
+        this.connection.emit('error', err)
+      }
     })
   }
 
   _handleAuthSASLContinue(msg) {
-    sasl.continueSession(this.saslSession, this.password, msg.data)
-    this.connection.sendSCRAMClientFinalMessage(this.saslSession.response)
+    try {
+      sasl.continueSession(this.saslSession, this.password, msg.data)
+      this.connection.sendSCRAMClientFinalMessage(this.saslSession.response)
+    } catch (err) {
+      this.connection.emit('error', err)
+    }
   }
 
   _handleAuthSASLFinal(msg) {
-    sasl.finalizeSession(this.saslSession, msg.data)
-    this.saslSession = null
+    try {
+      sasl.finalizeSession(this.saslSession, msg.data)
+      this.saslSession = null
+    } catch (err) {
+      this.connection.emit('error', err)
+    }
   }
 
   _handleBackendKeyData(msg) {

diff --git a/packages/pg/lib/sasl.js b/packages/pg/lib/sasl.js
@@ -23,6 +23,9 @@ function continueSession(session, password, serverData) {
   if (typeof password !== 'string') {
     throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: client password must be a string')
   }
+  if (password === '') {
+    throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: client password must be a non-empty string')
+  }
   if (typeof serverData !== 'string') {
     throw new Error('SASL: SCRAM-SERVER-FIRST-MESSAGE: serverData must be a string')
   }

diff --git a/packages/pg/test/integration/client/sasl-scram-tests.js b/packages/pg/test/integration/client/sasl-scram-tests.js
@@ -73,3 +73,24 @@ suite.testAsync('sasl/scram fails when password is wrong', async () => {
   )
   assert.ok(usingSasl, 'Should be using SASL for authentication')
 })
+
+suite.testAsync('sasl/scram fails when password is empty', async () => {
+  const client = new pg.Client({
+    ...config,
+    // We use a password function here so the connection defaults do not
+    // override the empty string value with one from process.env.PGPASSWORD
+    password: () =>  '',
+  })
+  let usingSasl = false
+  client.connection.once('authenticationSASL', () => {
+    usingSasl = true
+  })
+  await assert.rejects(
+    () => client.connect(),
+    {
+      message: 'SASL: SCRAM-SERVER-FIRST-MESSAGE: client password must be a non-empty string',
+    },
+    'Error code should be for a password error'
+  )
+  assert.ok(usingSasl, 'Should be using SASL for authentication')
+})
diff --git a/packages/pg/test/unit/client/sasl-scram-tests.js b/packages/pg/test/unit/client/sasl-scram-tests.js
@@ -80,6 +80,44 @@ test('sasl/scram', function () {
       )
     })
 
+    test('fails when client password is not a string', function () {
+      for(const badPasswordValue of [null, undefined, 123, new Date(), {}]) {
+        assert.throws(
+          function () {
+            sasl.continueSession(
+              {
+                message: 'SASLInitialResponse',
+                clientNonce: 'a',
+              },
+              badPasswordValue,
+              'r=1,i=1'
+            )
+          },
+          {
+            message: 'SASL: SCRAM-SERVER-FIRST-MESSAGE: client password must be a string',
+          }
+        )
+      }
+    })
+
+    test('fails when client password is an empty string', function () {
+      assert.throws(
+        function () {
+          sasl.continueSession(
+            {
+              message: 'SASLInitialResponse',
+              clientNonce: 'a',
+            },
+            '',
+            'r=1,i=1'
+          )
+        },
+        {
+          message: 'SASL: SCRAM-SERVER-FIRST-MESSAGE: client password must be a non-empty string',
+        }
+      )
+    })
+
     test('fails when iteration is missing in server message', function () {
       assert.throws(
         function () {