-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Handle SASL SCRAM server error responses #3521
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
Add proper error handling for SCRAM-SERVER-FINAL-MESSAGE error attribute. The SCRAM specification allows servers to return error messages via the 'e' attribute in the server final message. Currently, these errors are ignored and authentication fails later during signature verification. Postgres typically doesn't return this error (see [here](https://github.com/postgres/postgres/blob/2047ad068139f0b8c6da73d0b845ca9ba30fb33d/src/backend/libpq/auth-scram.c#L423) on why), but poolers, or other applications using the postgres protocol might, and it's part of the SCRAM spec, so it probably makes sense for node-postgres to handle it. Aligns behaviour with psql, postgrex, and somewhat with pgJDBC (pgJDBC in particular is stricter with scram errors). For reference: - libpq handling it: https://github.com/postgres/postgres/blob/2047ad068139f0b8c6da73d0b845ca9ba30fb33d/src/interfaces/libpq/fe-auth-scram.c#L708
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.
Is it possible to include an integration test for this? One that hits the server? I want to make sure throwing here doesn't crash the client w/ an uncaught exception.
Hi, Brian. I'd be happy to, but I'm not sure about how, since Postgres itself doesn't return this error. Do you suggest some approach to do it?
For the record, I discovered this because Supavisor returns scram errors. I sent a PR aligning it's behaviour with Postgres, but I decided to open this one too to align node-postgres scram implementation with the others.
oh i see - what returns this error? or how can you get this error to trigger?
what was worrying to me about this is throwing errors, depending on where they're thrown, might not be caught by the client and then emitted as an error event & instead result in uncaughtExceptions - I audited the code and it looks like all this SASL code is wrapped in try/catches & uses throwing to abort in the event of bad, malformed, unexpected responses and the like, so this is actually okay...and if its impossible to write an integration test for I'm okay to merge it like this. I don't love merging code that doesn't actually have an integration test around it with a real backed, but if there's no way to force/trick postgres into returning an e attribute, we can do it.
Uh oh!
There was an error while loading. Please reload this page.
Add proper error handling for SCRAM-SERVER-FINAL-MESSAGE error attribute. The SCRAM specification allows servers to return error messages via the 'e' attribute in the server final message. Currently, these errors are ignored and authentication fails later during signature verification.
Postgres typically doesn't return this error (see here on why), but poolers, or other applications using the postgres protocol might, and it's part of the SCRAM spec, so it probably makes sense for node-postgres to handle it.
Aligns behaviour with psql, postgrex, and somewhat with pgJDBC (pgJDBC is stricter with scram errors).
For reference: