SCRAM pass-through authentication for postgres_fdw

On Wed, Dec 4, 2024 at 10:45 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

This is achieved by storing the SCRAM ClientKey and ServerKey obtained
during client authentication with the backend. These keys are then
used to complete the SCRAM exchange between the backend and the fdw
server, eliminating the need to derive them from a stored plain-text
password.

What are the assumptions that have to be made for pass-through SCRAM
to succeed? Is it just "the two servers have identical verifiers for
the user," or are there others?

It looks like the patch is using the following property [1]https://datatracker.ietf.org/doc/html/rfc5802#section-9:

If an attacker obtains the authentication information from the
authentication repository and either eavesdrops on one authentication
exchange or impersonates a server, the attacker gains the ability to
impersonate that user to all servers providing SCRAM access using the
same hash function, password, iteration count, and salt. For this
reason, it is important to use randomly generated salt values.

It makes me a little uneasy to give users a reason to copy identical
salts/verifiers around... But for e.g. a loopback connection, it seems
like there'd be no additional risk. Is that the target use case?

I haven't looked at the code very closely yet, but the following hunk
jumped out at me:

-    pg_hmac_ctx *ctx = pg_hmac_create(state->hash_type);
+    pg_hmac_ctx *ctx = pg_hmac_create(PG_SHA256);

Why was that change made?

Thanks,
--Jacob

[1]: https://datatracker.ietf.org/doc/html/rfc5802#section-9

On Wed, 4 Dec 2024 at 23:11, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

It makes me a little uneasy to give users a reason to copy identical
salts/verifiers around... But for e.g. a loopback connection, it seems
like there'd be no additional risk. Is that the target use case?

I don't think that necessarily has to be the usecase,
clustering/sharding setups could benefit from this too. PgBouncer
supports the same functionality[1]. I only see advantages over the
alternative, which is copying the plaintext password around. In case
of compromise of the server, only the salt+verifier has to be rotated,
not the actual user password.

Regarding the actual patch: This definitely needs a bunch of
documentation explaining how to use this and when not to use this.

On Wed, Dec 4, 2024 at 3:05 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

I only see advantages over the
alternative, which is copying the plaintext password around. In case
of compromise of the server, only the salt+verifier has to be rotated,
not the actual user password.

Sure, I'm not saying it's worse than plaintext. But a third
alternative might be actual pass-through SCRAM [1]/messages/by-id/9129a012-0415-947e-a68e-59d423071525@timescale.com, where either you
expect the two servers to share a certificate fingerprint, or
explicitly disable channel bindings on the second authentication pass
in order to allow the MITM. (Or, throwing spaghetti, maybe even have
the primary server communicate the backend cert so you can verify it
and use it in the binding?)

All that is a metric ton more work and analysis, though.

--Jacob

[1]: /messages/by-id/9129a012-0415-947e-a68e-59d423071525@timescale.com

On Wed, Dec 4, 2024 at 3:39 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

actual pass-through SCRAM

(This was probably not a helpful way to describe what I'm talking
about; the method here in the thread could be considered "actual
pass-through SCRAM" as well. Proxied SCRAM, maybe?)

--Jacob

Thanks for the comments!

On 04/12/24 19:11, Jacob Champion wrote:

This is achieved by storing the SCRAM ClientKey and ServerKey obtained
during client authentication with the backend. These keys are then
used to complete the SCRAM exchange between the backend and the fdw
server, eliminating the need to derive them from a stored plain-text
password.

What are the assumptions that have to be made for pass-through SCRAM
to succeed? Is it just "the two servers have identical verifiers for
the user," or are there others?

Yes, from the tests that I've performed I would say that this is the
only requirement.

It looks like the patch is using the following property [1]:

If an attacker obtains the authentication information from the
authentication repository and either eavesdrops on one authentication
exchange or impersonates a server, the attacker gains the ability to
impersonate that user to all servers providing SCRAM access using the
same hash function, password, iteration count, and salt. For this
reason, it is important to use randomly generated salt values.

It makes me a little uneasy to give users a reason to copy identical
salts/verifiers around... But for e.g. a loopback connection, it seems
like there'd be no additional risk. Is that the target use case?

I think that both can be use cases. In case of using with different
servers it can be another option over the plain-text password approach.

I'm attaching a v2 patch with a TAP test that validate the both use
cases. For connections with different servers an ALTER ROLE <role>
PASSWORD <encrypted_password> is required, so that both servers have
identical verifiers.

-    pg_hmac_ctx *ctx = pg_hmac_create(state->hash_type);
+    pg_hmac_ctx *ctx = pg_hmac_create(PG_SHA256);

Why was that change made?

Not needed, sorry. Fixed on v2

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

On 04/12/24 20:05, Jelte Fennema-Nio wrote:

On Wed, 4 Dec 2024 at 23:11, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:

It makes me a little uneasy to give users a reason to copy identical
salts/verifiers around... But for e.g. a loopback connection, it seems
like there'd be no additional risk. Is that the target use case?

I don't think that necessarily has to be the usecase,
clustering/sharding setups could benefit from this too. PgBouncer
supports the same functionality[1]. I only see advantages over the
alternative, which is copying the plaintext password around. In case
of compromise of the server, only the salt+verifier has to be rotated,
not the actual user password.

The patch is very similar with what was implemented on PgBoucer[1]https://github.com/pgbouncer/pgbouncer/commit/ba1abfe#diff-128a3f9ffa6a6f3863e843089ede6d07010215acf49c66b2d1f1d9baba2f49e7R1001

Regarding the actual patch: This definitely needs a bunch of
documentation explaining how to use this and when not to use this.

I'm attaching a patch with a initial documentation, so that we can get
initial thoughts (not sure if I should put the documentation on the
same patch of code changes).

Thanks!

[1]: https://github.com/pgbouncer/pgbouncer/commit/ba1abfe#diff-128a3f9ffa6a6f3863e843089ede6d07010215acf49c66b2d1f1d9baba2f49e7R1001
https://github.com/pgbouncer/pgbouncer/commit/ba1abfe#diff-128a3f9ffa6a6f3863e843089ede6d07010215acf49c66b2d1f1d9baba2f49e7R1001

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

Em qua., 4 de dez. de 2024 às 20:39, Jacob Champion
<jacob.champion@enterprisedb.com> escreveu:

On Wed, Dec 4, 2024 at 3:05 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

I only see advantages over the
alternative, which is copying the plaintext password around. In case
of compromise of the server, only the salt+verifier has to be rotated,
not the actual user password.

Sure, I'm not saying it's worse than plaintext. But a third
alternative might be actual pass-through SCRAM [1], where either you
expect the two servers to share a certificate fingerprint, or
explicitly disable channel bindings on the second authentication pass
in order to allow the MITM. (Or, throwing spaghetti, maybe even have
the primary server communicate the backend cert so you can verify it
and use it in the binding?)

I'm not understanding how these options would work for this scenario.
I understand your concern about making the users copying the SCRAM
verifiers around but I don't understand how this third alternative fix
this issue. Would it be something similar to what you've implemented on
[1]: /messages/by-id/9129a012-0415-947e-a68e-59d423071525@timescale.com

The approach on this patch is that when the backend open the
connection with the foreign server, it will use the ClientKey stored
from the first client connection with the backend to build the final
client message that will be sent to the foreign server, which was
created/validated based on verifiers of the first backend. I can't see
how the foreign server can validate the client proof without having
the identical verifiers with the first backend.

I tested a scenario where the client open a connection with the
backend using channel binding and the backend open a connection with
the foreign server also using channel binding and everything seems to
works fine. I don't know if I missing something here, but here is how
I've tested this:

- Configure build system to use openssl
meson setup build -Dssl=openssl ...
- Start two Postgresql servers
- Configure to use ssl on both servers
ssl = on
ssl_cert_file = '/path/to/server.crt'
ssl_key_file = '/path/to/server.key'
- Changed pg_hba to use ssl on both servers
hostssl all all 127.0.0.1/32 scram-sha-256
hostssl all all ::1/128 scram-sha-256
- Performed all foreign server setup (CREATE SERVER ...)
- Connect into the backend using channel_binding=require
psql "host=127.0.0.1 dbname=local sslmode=require channel_binding=require"
- Execute a query on fdw server

I've also put a debug message before strcmp(channel_binding, b64_message)
[2]: src/backend/libpq/auth-scram.c#read_client_final_message
got the log message on both servers logs.

Sorry if I misunderstood your message, I probably missed something here.

[1]: /messages/by-id/9129a012-0415-947e-a68e-59d423071525@timescale.com
[2]: src/backend/libpq/auth-scram.c#read_client_final_message

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

On Wed, Dec 11, 2024 at 11:04 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:

Em qua., 4 de dez. de 2024 às 20:39, Jacob Champion
<jacob.champion@enterprisedb.com> escreveu:

Sure, I'm not saying it's worse than plaintext. But a third
alternative might be actual pass-through SCRAM [1], where either you
expect the two servers to share a certificate fingerprint, or
explicitly disable channel bindings on the second authentication pass
in order to allow the MITM. (Or, throwing spaghetti, maybe even have
the primary server communicate the backend cert so you can verify it
and use it in the binding?)

I'm not understanding how these options would work for this scenario.
I understand your concern about making the users copying the SCRAM
verifiers around but I don't understand how this third alternative fix
this issue. Would it be something similar to what you've implemented on
[1]?

Yeah, I was speaking in reference to my LDAP/SCRAM patchset from a
while back. (I'm just going to call that approach "proxied SCRAM" for
now.)

The approach on this patch is that when the backend open the
connection with the foreign server, it will use the ClientKey stored
from the first client connection with the backend to build the final
client message that will be sent to the foreign server, which was
created/validated based on verifiers of the first backend. I can't see
how the foreign server can validate the client proof without having
the identical verifiers with the first backend.

Correct. The only way this strategy will work is if the verifiers are
the same. (Proxied SCRAM allows for different verifiers -- with
different salts and/or iterations -- with the same password.)

I do like that the action "copy the verifier" is a pretty clear signal
that you want the servers to be able to MITM each other. It's less
attack surface than having the two servers share a certificate, for
sure, and less work than communicating a new binding. Only users that
have opted into that are "vulnerable".

I tested a scenario where the client open a connection with the
backend using channel binding and the backend open a connection with
the foreign server also using channel binding and everything seems to
works fine. I don't know if I missing something here, but here is how
I've tested this:

All that looks good. Sorry, I hadn't intended to derail your
particular proposal with that -- the channel binding problem only
shows up with my proxied SCRAM, because the client has to decide which
tls-server-end-point to trust and put into the binding.

(It's important that your patchset works with channel bindings too, of
course, but I don't expect that to be a problem. It shouldn't matter
to this approach.)

--Jacob

This patch is surprisingly compact and straightforward for providing
such complex functionality.

I have one major code comment that needs addressing:

In src/interfaces/libpq/fe-auth-scram.c, there is:

+ memcpy(ClientKey, state->conn->scram_client_key_binary,
SCRAM_MAX_KEY_LEN);

Here you are assuming that scram_client_key_binary has a fixed length,
but the allocation is

+       len = pg_b64_dec_len(strlen(conn->scram_client_key));
+       conn->scram_client_key_len = len;
+       conn->scram_client_key_binary = malloc(len);

And scram_client_key is passed by the client. There needs to be some
verification that what is passed in is of the right length.

At the moment, we only support one variant of SCRAM, so all the keys
etc. are of a fixed length. But we should make sure that this wouldn't
break in confusing ways in the future if that is no longer the case.

Attached are a few minor fix-up patches for your patches. I have marked
a couple of places where more documentation could be added.

In the future, you can squash all of this (code plus documentation) into
one patch.

postgres_fdw has this error message:

ereport(ERROR,
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
errmsg("password or GSSAPI delegated credentials required"),
errdetail("Non-superusers must delegate GSSAPI credentials
or provide a password in the user mapping.")));

Maybe the option of having SCRAM pass-through should be mentioned here?
It seems sort of analogous to the delegate GSSAPI credentials case.

Finally, if you have time, maybe look into whether this could also be
implemented in dblink.

Thanks for the review!

Em qua., 8 de jan. de 2025 às 07:49, Peter Eisentraut
<peter@eisentraut.org> escreveu:

This patch is surprisingly compact and straightforward for providing
such complex functionality.

I have one major code comment that needs addressing:

In src/interfaces/libpq/fe-auth-scram.c, there is:

+ memcpy(ClientKey, state->conn->scram_client_key_binary,
SCRAM_MAX_KEY_LEN);

Here you are assuming that scram_client_key_binary has a fixed length,
but the allocation is

+       len = pg_b64_dec_len(strlen(conn->scram_client_key));
+       conn->scram_client_key_len = len;
+       conn->scram_client_key_binary = malloc(len);

And scram_client_key is passed by the client. There needs to be some
verification that what is passed in is of the right length.

At the moment, we only support one variant of SCRAM, so all the keys
etc. are of a fixed length. But we should make sure that this wouldn't
break in confusing ways in the future if that is no longer the case.

Fixed

postgres_fdw has this error message:

ereport(ERROR,
(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
errmsg("password or GSSAPI delegated credentials required"),
errdetail("Non-superusers must delegate GSSAPI credentials
or provide a password in the user mapping.")));

Maybe the option of having SCRAM pass-through should be mentioned here?
It seems sort of analogous to the delegate GSSAPI credentials case.

Yeah, I also think that makes sense.

I've made all changes on the attached v2.

--
Matheus Alcantara

On 09.01.25 16:22, Matheus Alcantara wrote:

Yeah, I also think that makes sense.

I've made all changes on the attached v2.

(This should probably have been v3, since you had already sent a v2
earlier.)

This all looks good to me.

Attached is a fixup patch where I have tried to expand the documentation
a bit in an attempt to clarify how to use this. Maybe check that what I
wrote is correct.

Em ter., 14 de jan. de 2025 às 06:21, Peter Eisentraut
<peter@eisentraut.org> escreveu:

On 09.01.25 16:22, Matheus Alcantara wrote:

Yeah, I also think that makes sense.

I've made all changes on the attached v2.

(This should probably have been v3, since you had already sent a v2
earlier.)

Oops, sorry about that.

This all looks good to me.

Attached is a fixup patch where I have tried to expand the documentation
a bit in an attempt to clarify how to use this. Maybe check that what I
wrote is correct.

It looks good to me, it's much clearer now. Thanks.

v4 attached with these fixes and also rebased with master.

--
Matheus Alcantara

On 14.01.25 15:14, Matheus Alcantara wrote:

Attached is a fixup patch where I have tried to expand the documentation
a bit in an attempt to clarify how to use this. Maybe check that what I
wrote is correct.

It looks good to me, it's much clearer now. Thanks.

v4 attached with these fixes and also rebased with master.

Committed, after pgindent and pgperltidy.

Em qua., 15 de jan. de 2025 às 14:03, Peter Eisentraut
<peter@eisentraut.org> escreveu:

On 14.01.25 15:14, Matheus Alcantara wrote:

Attached is a fixup patch where I have tried to expand the documentation
a bit in an attempt to clarify how to use this. Maybe check that what I
wrote is correct.

It looks good to me, it's much clearer now. Thanks.

v4 attached with these fixes and also rebased with master.

Committed, after pgindent and pgperltidy.

Thanks!

--
Matheus Alcantara

Matheus Alcantara писал(а) 2025-01-16 16:07:

Em qua., 15 de jan. de 2025 às 14:03, Peter Eisentraut
<peter@eisentraut.org> escreveu:

On 14.01.25 15:14, Matheus Alcantara wrote:

Attached is a fixup patch where I have tried to expand the documentation
a bit in an attempt to clarify how to use this. Maybe check that what I
wrote is correct.

It looks good to me, it's much clearer now. Thanks.

v4 attached with these fixes and also rebased with master.

Committed, after pgindent and pgperltidy.

Thanks!

Hi.
I've started to look at this feature and found an issue - MyProcPort can
be not set if connection is initiated
by some bgworker. (Internally we use one for statistics collection.) In
other places (for example, in be_gssapi_get_delegation())
there are checks that port is not NULL. Likely postgres_fdw and dblink
should do something similar.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

Hi, thanks for testing and reporting the issue!

On 25/06/25 11:37, Alexander Pyhalov wrote:

Hi.
I've started to look at this feature and found an issue - MyProcPort
can be not set if connection is initiated
by some bgworker. (Internally we use one for statistics collection.)
In other places (for example, in be_gssapi_get_delegation())
there are checks that port is not NULL. Likely postgres_fdw and dblink
should do something similar.

In this case the bgworker is used to collect statistics for the fdw
tables? If that's the case, since we don't have the MyProcPort and the
scram keys, will it use the user and password configured on user mapping
properties? If that's also the case I think that we may have a problem
because the goal of this feature is to avoid storing the password on
user mapping.

Do you have steps to reproduce the issue?

--
Matheus Alcantara

Matheus Alcantara писал(а) 2025-06-25 14:36:

Hi, thanks for testing and reporting the issue!

On 25/06/25 11:37, Alexander Pyhalov wrote:

Hi.
I've started to look at this feature and found an issue - MyProcPort
can be not set if connection is initiated
by some bgworker. (Internally we use one for statistics collection.)
In other places (for example, in be_gssapi_get_delegation())
there are checks that port is not NULL. Likely postgres_fdw and dblink
should do something similar.

In this case the bgworker is used to collect statistics for the fdw
tables? If that's the case, since we don't have the MyProcPort and the
scram keys, will it use the user and password configured on user
mapping
properties? If that's also the case I think that we may have a problem
because the goal of this feature is to avoid storing the password on
user mapping.

Do you have steps to reproduce the issue?

Hi. I've created a simple extension to reproduce an issue. Just put
attached files to contrib and run make check.
You'll see bgworker crash.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

On 25.06.25 20:07, Alexander Pyhalov wrote:

Matheus Alcantara писал(а) 2025-06-25 14:36:

Hi, thanks for testing and reporting the issue!

On 25/06/25 11:37, Alexander Pyhalov wrote:

Hi.
I've started to look at this feature and found an issue - MyProcPort
can be not set if connection is initiated
by some bgworker. (Internally we use one for statistics collection.)
In other places (for example, in be_gssapi_get_delegation())
there are checks that port is not NULL. Likely postgres_fdw and dblink
should do something similar.

In this case the bgworker is used to collect statistics for the fdw
tables? If that's the case, since we don't have the MyProcPort and the
scram keys, will it use the user and password configured on user mapping
properties? If that's also the case I think that we may have a problem
because the goal of this feature is to avoid storing the password on
user mapping.

Do you have steps to reproduce the issue?

Hi. I've created a simple extension to reproduce an issue. Just put
attached files to contrib and run make check.
You'll see bgworker crash.

Thank you for this. I think your patch to fix this makes sense.

On Wed Jun 25, 2025 at 3:07 PM -03, Alexander Pyhalov wrote:

Matheus Alcantara писал(а) 2025-06-25 14:36:

Hi, thanks for testing and reporting the issue!

On 25/06/25 11:37, Alexander Pyhalov wrote:

Hi.
I've started to look at this feature and found an issue - MyProcPort
can be not set if connection is initiated
by some bgworker. (Internally we use one for statistics collection.)
In other places (for example, in be_gssapi_get_delegation())
there are checks that port is not NULL. Likely postgres_fdw and dblink
should do something similar.

In this case the bgworker is used to collect statistics for the fdw
tables? If that's the case, since we don't have the MyProcPort and the
scram keys, will it use the user and password configured on user
mapping
properties? If that's also the case I think that we may have a problem
because the goal of this feature is to avoid storing the password on
user mapping.

Do you have steps to reproduce the issue?

Hi. I've created a simple extension to reproduce an issue. Just put
attached files to contrib and run make check.
You'll see bgworker crash.

Thanks! I was able to reproduce the issue.

I've also made some other tests and your patch looks good, so +1.

I've also made some tests by using the use_scram_passthrough option on
foreign server and if a bgworker try to use a foreign table that has
this option associated with the foreign server the connection will fail
because we don't have the MyProcPort and the password. To make it work
the password is required on USER MAPPING options. I think that this
limitation should be documented, see patch attached.

--
Matheus Alcantara