Support libpq's channel_binding and require_auth (and enforce them against every authentication request) - #3746
Open
jawj wants to merge 4 commits into
Open
Support libpq's channel_binding and require_auth (and enforce them against every authentication request)#3746jawj wants to merge 4 commits into
channel_binding and require_auth (and enforce them against every authentication request)#3746jawj wants to merge 4 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This builds on my earlier PR, #3356, which added SCRAM-SHA-256-PLUS behind the
enableChannelBindingoption. As I noted then, that option let us enable channel binding but gave no way to require it. Being opt-in and non-standard, I suspect rather few users yet get the benefit of it.This PR moves to using a libpq-style
channel_bindingparameter. It also introduces a libpq-stylerequire_authparameter, and makes sure both are enforced everywhere they need to be.These changes make some connections more secure by default. They also provide users with the option to make connections even more secure, by specifying
channel_binding=requireor specific authentication modes, such asrequire_auth=scram-sha-256, that limit the damage that can be done by an MITM attack. And they bring node-postgres's connection options into closer alignment with libpq.Because
channel_binding=requireauthenticates the server, it also silences the deprecation warning thatsslmode=prefer,sslmode=requireandsslmode=verify-cacurrently emit (this matters to Neon users, for example, who see those warnings even though the weaker guarantees they warn about cannot be exploited when the channel is bound).channel_bindingdisable,preferandrequire, from the client config, a connection string, orPGCHANNELBINDING.prefer: channel binding is used whenever the connection is over SSL and the server offers SCRAM-SHA-256-PLUS, and an unbound exchange is used otherwise. This matches libpq's default.enableChannelBindingis still honored, withtruemeaning"prefer"andfalsemeaning"disable"(it also accepts the three levels), but is now documented as deprecated.channel_binding: 'require'needs SSL, so it is an error to combine it with SSL disabled — unless you are using the native (libpq) client, which negotiates SSL of its own accord and reports for itself if it ends up without.require_authNames the authentication method(s) the server is allowed to ask for, with libpq's semantics: a comma-separated allow-list, or a block-list in which every entry is negated with
!, plusnonefor a connection where the server asks for nothing. It can be set in the config, a connection string, orPGREQUIREAUTH— which an explicitrequire_auth: ''overrides, as in libpq.password,md5,scram-sha-256,gss,sspiandoauth, of which this client implements the first three. A setting that includes only methods we cannot satisfy (e.g.require_auth: 'gss') is an error at construction-time (but e.g.require_auth: 'gss,md5'is fine). The native client passes the setting to libpq and defers to it, so the full set is accepted there.channel_binding: 'require'impliesrequire_auth: 'scram-sha-256', since no other method supports channel binding. Arequire_authvalue that rules out SCRAM contradicts that, so is also an error.Enforcing them
The kind of bug I filed CVE-2025-49146 for against pgjdbc is a risk here — where
channel_binding=requirewas honored inside the SCRAM exchange but a server could simply ask for a plain password instead!So the check lives in one place that every authentication request passes through, modelled on libpq's
check_expected_areq:passwordcallback.AuthenticationOk, and again where the connection becomes usable, so a server cannot evade a requirement by declaring the client authenticated without asking for anything — or by sending no authentication message at all. libpq accepts nothing but an authentication request at that point in its handshake, whereas this client listens for every message from the start, so it needs the second check.connectevent was emitted, and the client was left looking usable. Such a client now reports the failure once and refuses to answer anything further.Connection#end()writes its Terminate before ending the stream, so both re-check that the connection has not been given up on before writing their answer.Two related tightenings in the SCRAM code: the client counts itself channel-bound only once the server's signature has been verified, and only for SCRAM-SHA-256-PLUS, so an unbound exchange cannot satisfy
require; and a server offering SCRAM-SHA-256-PLUS on a connection that is not encrypted is refused.Naming
Both parameters keep libpq's spelling (the same as
client_encodingandapplication_namedo, for example). Since both exist to refuse weak authentication, a camelCasedchannelBindingorrequireAuththrows rather than being quietly ignored, as does a channel binding level that is not one of the supported three. Both options can also be set on aPool, which passes its options to the clients it creates.Changing the default
Changing default behaviour was a concern last time round.
preferonly ever upgrades a connection that is already over SSL, to a server that offers channel binding, and falls back to an unbound exchange otherwise, so there is nothing for a server to be surprised by. The one case where an outcome changes is a server whose certificate uses a signature algorithm the parser doesn't know (Ed448, for instance): that connection now fails rather than silently authenticating unbound. The same thing can happen in libpq. In either case,channel_binding: 'disable'restores the old outcome.Tests
packages/pg/test/unit/client/auth-flow-tests.jsdrives a client through complete exchanges against a scripted SCRAM server using a real certificate, computing the server's replies from what the client actually sent, so it checks the gs2 header and the binding data rather than taking the client's word for either. This helps with @charmander's point on Add support for SCRAM-SHA-256-PLUS i.e. channel binding #3356 that the tests there would have passed with no implementation of channel binding at all. It covers each of the refusals above, including the pipelined login and the post-refusal races.require-auth-tests.jscovers the parsing and the requirement check. TheConnectionParametersandpg-connection-stringtests cover resolution from config, connection strings and the environment, precedence between them, and each error.PGTESTNOSSLis no longer set in CI, so the SSL and channel binding tests actually run there (perhaps there is a good reason for settingPGTESTNOSSLin CI, though?).packages/pg/script/test-server.shstarts the same image CI uses under either podman or docker, for anyone who wants to run them locally.Docs are in
docs/pages/features/ssl.mdxand thepg-connection-stringREADME, with a suggestedCHANGELOGentry too.DefinitelyTyped's
@types/pgwill needchannel_bindingandrequire_authadding. I'll send a PR for that if and when these changes land.