Skip to content

Fix SCRAM-SHA-256 authentication by cloning the options object #145

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

Conversation

marcbachmann
Copy link
Contributor

This pull request fixes the issue for SCRAM authentication mentioned in #123.
The options object is mutated at two places during the authentication to persist connection information.
To set a nonce:

options.nonce = crypto.randomBytes(18).toString('base64')

And to set the serverSignature:
options.serverSignature = hmac(hmac(saltedPassword, 'Server Key'), auth).toString('base64')

By cloning the object, we can safely mutate it to successfully do the handshake.

@@ -218,7 +218,9 @@ function Postgres(a, b) {

function createConnection(options) {
slots--
const connection = Connection(options)
// The options object gets cloned as the as the authentication in the frontend.js mutates the
// options to persist a nonce and signature, which are unique per connection.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's not a single comment in this file. Should I remove it again? 😁

Copy link
Owner

Choose a reason for hiding this comment

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

I think that's fine considered the non obvious behavior. Maybe we can look at a fix keeping the nonce internal to the connection later to avoid mutating the options.

@marcbachmann marcbachmann force-pushed the fix-scram-authentication-with-parallel-queries branch from e39e04a to 70a659f Compare January 19, 2021 00:13
@marcbachmann
Copy link
Contributor Author

Somehow the issue only occured if there are more than two queries running.
With two queries, I didn't get the error.

@marcbachmann
Copy link
Contributor Author

marcbachmann commented Jan 19, 2021

Load tested using that:

// proof.js
async function start (port) {
  const sql = require('./')(`postgres://postgres:password@localhost:${port}/postgres`, {max: 100})

  try {
    const promises = []
    for (let index = 0; index < 100000; index++) {
      promises.push(sql`SELECT 1`)
    }
    await Promise.all(promises)
  } finally {
    sql.end({timeout: 0})
  }
}

start(5433)
  .catch((err) => console.error('Error from instance with scram authentication: ', err))

@mortifia
Copy link

mortifia commented Jan 19, 2021

load tested with apollo-server and jmeter caling that with no bug

    async coucou() {
        console.log("coucou");

        /*const res = await this.db`
        select *
        from "user".users
        console.log("coucou end");
        return res[0];
        `*/

        const tmp = await Promise.allSettled([
            this.db`select * from "user".users`,
            this.db`select * from "user".users`,
            this.db`select * from "user".users`,
            this.db`select * from "user".users`,
            this.db`select * from "user".users`,
            this.db`select * from "user".users`,
            this.db`select * from "user".users`,
            this.db`select * from "user".users`]);

        console.log("coucou end");

        return res[1].value[0];
    }

12 800 request/s api -> postgresql 13 with no errors

ps tested with more log for checking error

@porsager
Copy link
Owner

Thanks a lot @marcbachmann ! Looks good.

@porsager porsager merged commit 0709db4 into porsager:master Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants