-
Notifications
You must be signed in to change notification settings - Fork 301
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
Fix SCRAM-SHA-256 authentication by cloning the options object #145
Conversation
9b85786
to
e39e04a
Compare
@@ -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. |
There was a problem hiding this comment.
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? 😁
There was a problem hiding this comment.
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.
…is used to persist a nonce per connection
e39e04a
to
70a659f
Compare
Somehow the issue only occured if there are more than two queries running. |
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)) |
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 |
Thanks a lot @marcbachmann ! Looks good. |
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
:postgres/lib/frontend.js
Line 97 in 4d5fdc1
And to set the
serverSignature
:postgres/lib/frontend.js
Line 122 in 4d5fdc1
By cloning the object, we can safely mutate it to successfully do the handshake.