Conversation
70f3854 to
15a1309
Compare
There was a problem hiding this comment.
These changes are based on #2516 currently. Otherwise, testing locally across all targets was not possible given the need for a TLS setup.
Overall the implementation was straight forward. We just need to add the channel binding pieces to the client message steps of the auth dance. The server messages get to remain unchanged.
| export PGX_TEST_MD5_PASSWORD_CONN_STRING="host=127.0.0.1 database=pgx_test user=pgx_md5 password=secret" | ||
| export PGX_TEST_PLAIN_PASSWORD_CONN_STRING="host=127.0.0.1 user=pgx_pw password=secret" | ||
| export PGX_TEST_TLS_CONN_STRING="host=localhost user=pgx_ssl password=secret sslmode=verify-full sslrootcert=`pwd`/.testdb/ca.pem" | ||
| export PGX_TEST_TLS_CONN_STRING="host=localhost user=pgx_ssl password=secret sslmode=verify-full sslrootcert=`pwd`/.testdb/ca.pem channel_binding=disable" |
There was a problem hiding this comment.
Explicitly set channel_binding to disable to preserve the original test cases that utilize this conn string. Left unset would default to prefer and that would automatically upgrade the connection auth mechanism given that pg_hba.conf has pgx_ssl setup to auth with scram. Instead, we copy this conn string to PGX_TEST_SCRAM_PLUS_CONN_STRING above and set the channel binding to require in order to explicitly exercise that path.
| @@ -0,0 +1,400 @@ | |||
| package pgconn | |||
There was a problem hiding this comment.
Interestingly, there were not any tests directly associated with the scram client and it's supporting functions. So, it seemed prudent to add them as the use-case becomes a little more complex when adding channel binding support. And since I was already adding tests around those new items I decided to also add some coverage for those items that previously missing.
15a1309 to
6b5385e
Compare
|
This looks good to me. But I'm not familiar the GS2 header downgrade when getTLSCertificateHash errors on prefer When channel_binding=prefer (the default), we have a TLS connection, but getTLSCertificateHash returns an error (e.g., the Ed25519 case above, or future unknown algorithms): The error is silently ignored (line 58 only checks require) The correct behavior: when on TLS, even if cert hash extraction fails, set channelBindingData to a non-nil sentinel (or add a separate boolean) so clientFirstMessage sends y,,. Or restructure the logic to separate "has TLS" from "has binding data". Proposed fix in scramAuth: And in clientFirstMessage: However, there's a nuance: y,, should only be sent when the client could do channel binding but the server didn't advertise PLUS. If the server did advertise PLUS but we can't compute the hash, it's debatable whether y,, or n,, is correct. libpq in this scenario would fail on require and fall back on prefer -- but it still sends y,, when on TLS. The safer choice is y,,. I don't know enough to know if this is significant, but it seemed worth highlighting. |
Extend the existing `SCRAM-SHA-256` authentication to support `SCRAM-SHA-256-PLUS` with tls-server-end-point channel binding (RFC 5929). `SCRAM-SHA-256` authenticates the client to the server but does not bind the authentication exchange to the underlying TLS connection. `SCRAM-SHA-256-PLUS` closes this gap by incorporating the server's TLS certificate hash into the SCRAM proof, ensuring that the authentication occurred over the same TLS session and protecting against man-in-the-middle attacks. When using SCRAM and the connection is over TLS and the server advertises `SCRAM-SHA-256-PLUS`, the client automatically upgrades. This behavior is controlled by adding support for the libpq `channel_binding` connection parameter. The `channel_binding` connection parameter supports the following values: - `prefer` (default): use `SCRAM-SHA-256-PLUS` when available. - `require`: fail if channel binding cannot be established. - `disable`: always use `SCRAM-SHA-256`.
6b5385e to
5028389
Compare
Yeah, it most definitely is and it's a good catch. I was trying to ensure I followed the libpq on this one and erroneously conflated not having TLS and not being able to determine the server cert hash as being the same thing. 🤦 At any rate, I've updated the client to ensure it is aware of when it is using TLS so that it is able to provide the correct GS2 header flag. 👍 |
Extend the existing
SCRAM-SHA-256authentication to supportSCRAM-SHA-256-PLUSwith tls-server-end-point channel binding (RFC5929).
SCRAM-SHA-256authenticates the client to the server but does not bindthe authentication exchange to the underlying TLS connection.
SCRAM-SHA-256-PLUScloses this gap by incorporating the server's TLScertificate hash into the SCRAM proof, ensuring that the authentication
occurred over the same TLS session and protecting against
man-in-the-middle attacks.
When using SCRAM and the connection is over TLS and the server
advertises
SCRAM-SHA-256-PLUS, the client automatically upgrades. Thisbehavior is controlled by adding support for the libpq
channel_bindingconnection parameter.
The
channel_bindingconnection parameter supports the followingvalues:
prefer(default): useSCRAM-SHA-256-PLUSwhen available.require: fail if channel binding cannot be established.disable: always useSCRAM-SHA-256.Resolves: #2486.