Обсуждение: Feature Proposal: Connection Pool Optimization - Change the Connection User

Justin Pryzby <pryzby@telsasoft.com> writes:
> On Sun, Nov 21, 2021 at 03:11:03AM +1100, Todd Hubers wrote:
>> - Google Document with Commenting turned on
>> https://docs.google.com/document/d/1u6mVKEHfKtR80UrMLNYrp5D6cCSW1_arcTaZ9HcAKlw/edit?usp=sharing.

> You proposed a PQ protocol version of SET ROLE/SET SESSION authorization.
> You'd need to make sure that a client connected to the connection pooler cannot
> itself run the PQ "set role".

It's not really apparent to me how this mechanism wouldn't be a giant
security hole.  In particular, I see no meaningful difference between
granting the proposed "impersonate" privilege and granting superuser.
You could restrict it to not allow impersonating superusers, which'd
make it a little better, but what of all the predefined roles we keep
inventing that have privileges we don't want to be accessible to Joe
User?  I think by the time you got to something where "impersonate"
wasn't a security hole, it would be pretty much equivalent to SET ROLE,
ie you could only impersonate users you'd been specifically given the
privlege for.

It also seems like this is transferring the entire responsibility for
user authentication onto the middleware, with the server expected to
just believe that the session should now be allowed to act as user X.
Again, that seems more like a security problem than a good design.

Another issue is that the proposed implementation seems pretty far
short of being an invisible substitute for actually logging in as
the target user.  For starters, what of instantiating ALTER ROLE SET
parameter values that apply to the target user, and getting rid of
such settings that applied to the original user ID?  How would this
interact with the "on login" triggers that people keep asking for?

Also, you'd still have to do DISCARD ALL (at least) when switching
users, or else integrate all that cache-flushing right into the
switching mechanism.  So I'm not entirely convinced about how big
the performance benefits are compared to a fresh session.

One more point is that the proposed business about

* ImpersonateDatabaseUser will either succeed silently (0-RTT), or
  fail. Upon failure, no further commands will be processed until
  ImpersonateDatabaseUser succeeds.

seems to require adding a huge amount of complication on the server side,
and complication in the protocol spec itself, to save a rather minimal
amount of complication in the middleware.  Why can't we just say that
a failed "impersonate" command leaves the session in the same state
as before, and it's up to the pooler to do something about it?  We are
in any case trusting the pooler not to send commands from user A to
a session logged in as user B.

                        regards, tom lane

PS: I wonder how we test such a feature meaningfully without
incorporating a pooler right into the Postgres tree.  I don't
want to do that, for sure.

On Sat, 2021-11-20 at 16:16 -0500, Tom Lane wrote:
> One more point is that the proposed business about
>
> * ImpersonateDatabaseUser will either succeed silently (0-RTT), or
>   fail. Upon failure, no further commands will be processed until
>   ImpersonateDatabaseUser succeeds.
>
> seems to require adding a huge amount of complication on the server side,
> and complication in the protocol spec itself, to save a rather minimal
> amount of complication in the middleware.  Why can't we just say that
> a failed "impersonate" command leaves the session in the same state
> as before, and it's up to the pooler to do something about it?  We are
> in any case trusting the pooler not to send commands from user A to
> a session logged in as user B.

When combined with the 0-RTT goal, I think a silent ignore would just
invite more security problems. Todd is effectively proposing packet
pipelining, so the pipeline has to fail shut.

A more modern approach might be to attach the authentication to the
packet itself (e.g. cryptographically, with a MAC), if the goal is to
enable per-statement authentication anyway. In theory that turns the
middleware into a message passer instead of a confusable deputy. But it
requires more complicated setup between the client and server.

> PS: I wonder how we test such a feature meaningfully without
> incorporating a pooler right into the Postgres tree.  I don't
> want to do that, for sure.

Having protocol-level tests for bytes on the wire would not only help
proposals like this but also get coverage for a huge number of edge
cases. Magnus has added src/test/protocol for the server, written in
Perl, in his PROXY proposal. And I've added a protocol suite for both
the client and server, written in Python/pytest, in my OAuth proof of
concept. I think something is badly needed in this area.

--Jacob

Hi Jacob and Daniel,

Thanks for your feedback.

>@Daniel - I think thats conflating session_user and current_user, SET ROLE is not a login event. This is by design and discussed in the documentation..

Agreed, I am using those terms loosely. I have updated option 4 in the proposal document. I have crossed it out. Option 5 is more suitable "SET SESSION AUTHORIZATION" for further consideration.

>@Daniel - but it's important to remember that we need to cover the functionality in terms of *tests* first, performance benchmarking is another concern.

For implementation absolutely, but not for a basic feasibility prototype. A quick non-secure non-reliable prototype is probably an important first-step to confirming which options work best for the stated goals. Importantly, if the improvement is only 5% (whatever that might mean), then the project is probably not work starting. But I do expect that a benchmark will prove benefits that justify the resources to build the feature(s).

>@Jacob - A more modern approach might be to attach the authentication to the packet itself (e.g. cryptographically, with a MAC), if the goal is to enable per-statement authentication anyway. In theory that turns the middleware into a message passer instead of a confusable deputy. But it requires more complicated setup between the client and server.

I did consider this, but I ruled it out. I have now added it to the proposal document, and included two Issues. Please review and let me know whether I might be mistaken.

>@Jacob - Having protocol-level tests for bytes on the wire would not only help proposals like this but also get coverage for a huge number of edge cases. Magnus has added src/test/protocol for the server, written in Perl, in his PROXY proposal. And I've added a protocol suite for both the client and server, written in Python/pytest, in my OAuth proof of concept. I think something is badly needed in this area.

Thanks for highlighting this emerging work. I have noted this in the proposal in the Next Steps section.

--Todd

Note: Here is the proposal document link again - https://docs.google.com/document/d/1u6mVKEHfKtR80UrMLNYrp5D6cCSW1_arcTaZ9HcAKlw/edit#

On Tue, 23 Nov 2021 at 12:12, Jacob Champion <pchampion@vmware.com> wrote:

On Sat, 2021-11-20 at 16:16 -0500, Tom Lane wrote:
> One more point is that the proposed business about
>
> * ImpersonateDatabaseUser will either succeed silently (0-RTT), or
>   fail. Upon failure, no further commands will be processed until
>   ImpersonateDatabaseUser succeeds.
>
> seems to require adding a huge amount of complication on the server side,
> and complication in the protocol spec itself, to save a rather minimal
> amount of complication in the middleware.  Why can't we just say that
> a failed "impersonate" command leaves the session in the same state
> as before, and it's up to the pooler to do something about it?  We are
> in any case trusting the pooler not to send commands from user A to
> a session logged in as user B.

When combined with the 0-RTT goal, I think a silent ignore would just
invite more security problems. Todd is effectively proposing packet
pipelining, so the pipeline has to fail shut.

A more modern approach might be to attach the authentication to the
packet itself (e.g. cryptographically, with a MAC), if the goal is to
enable per-statement authentication anyway. In theory that turns the
middleware into a message passer instead of a confusable deputy. But it
requires more complicated setup between the client and server.

> PS: I wonder how we test such a feature meaningfully without
> incorporating a pooler right into the Postgres tree.  I don't
> want to do that, for sure.

Having protocol-level tests for bytes on the wire would not only help
proposals like this but also get coverage for a huge number of edge
cases. Magnus has added src/test/protocol for the server, written in
Perl, in his PROXY proposal. And I've added a protocol suite for both
the client and server, written in Python/pytest, in my OAuth proof of
concept. I think something is badly needed in this area.

--Jacob

--

--

Todd Hubers

Hi Everyone,

I have started working on this:

• Benchmarking - increasingly more comprehensive benchmarking
• Prototyping - to simulate the change of users (toggling back and forth)
• Draft Implementation - of OPTION-1 (New Protocol Message)
• (Then: Working with Odyssey and PgBouncer to add support (when the GRANT role privilege is available))

I hope to have a patch ready by the end of March.

Regards,

Todd

On Wed, 24 Nov 2021 at 02:46, Todd Hubers <todd.hubers@gmail.com> wrote:

Hi Jacob and Daniel,

Thanks for your feedback.

>@Daniel - I think thats conflating session_user and current_user, SET ROLE is not a login event. This is by design and discussed in the documentation..

Agreed, I am using those terms loosely. I have updated option 4 in the proposal document. I have crossed it out. Option 5 is more suitable "SET SESSION AUTHORIZATION" for further consideration.

>@Daniel - but it's important to remember that we need to cover the functionality in terms of *tests* first, performance benchmarking is another concern.

For implementation absolutely, but not for a basic feasibility prototype. A quick non-secure non-reliable prototype is probably an important first-step to confirming which options work best for the stated goals. Importantly, if the improvement is only 5% (whatever that might mean), then the project is probably not work starting. But I do expect that a benchmark will prove benefits that justify the resources to build the feature(s).

>@Jacob - A more modern approach might be to attach the authentication to the packet itself (e.g. cryptographically, with a MAC), if the goal is to enable per-statement authentication anyway. In theory that turns the middleware into a message passer instead of a confusable deputy. But it requires more complicated setup between the client and server.

I did consider this, but I ruled it out. I have now added it to the proposal document, and included two Issues. Please review and let me know whether I might be mistaken.

>@Jacob - Having protocol-level tests for bytes on the wire would not only help proposals like this but also get coverage for a huge number of edge cases. Magnus has added src/test/protocol for the server, written in Perl, in his PROXY proposal. And I've added a protocol suite for both the client and server, written in Python/pytest, in my OAuth proof of concept. I think something is badly needed in this area.

Thanks for highlighting this emerging work. I have noted this in the proposal in the Next Steps section.

--Todd

Note: Here is the proposal document link again - https://docs.google.com/document/d/1u6mVKEHfKtR80UrMLNYrp5D6cCSW1_arcTaZ9HcAKlw/edit#

On Tue, 23 Nov 2021 at 12:12, Jacob Champion <pchampion@vmware.com> wrote:

On Sat, 2021-11-20 at 16:16 -0500, Tom Lane wrote:
> One more point is that the proposed business about
>
> * ImpersonateDatabaseUser will either succeed silently (0-RTT), or
>   fail. Upon failure, no further commands will be processed until
>   ImpersonateDatabaseUser succeeds.
>
> seems to require adding a huge amount of complication on the server side,
> and complication in the protocol spec itself, to save a rather minimal
> amount of complication in the middleware.  Why can't we just say that
> a failed "impersonate" command leaves the session in the same state
> as before, and it's up to the pooler to do something about it?  We are
> in any case trusting the pooler not to send commands from user A to
> a session logged in as user B.

When combined with the 0-RTT goal, I think a silent ignore would just
invite more security problems. Todd is effectively proposing packet
pipelining, so the pipeline has to fail shut.

A more modern approach might be to attach the authentication to the
packet itself (e.g. cryptographically, with a MAC), if the goal is to
enable per-statement authentication anyway. In theory that turns the
middleware into a message passer instead of a confusable deputy. But it
requires more complicated setup between the client and server.

> PS: I wonder how we test such a feature meaningfully without
> incorporating a pooler right into the Postgres tree.  I don't
> want to do that, for sure.

Having protocol-level tests for bytes on the wire would not only help
proposals like this but also get coverage for a huge number of edge
cases. Magnus has added src/test/protocol for the server, written in
Perl, in his PROXY proposal. And I've added a protocol suite for both
the client and server, written in Python/pytest, in my OAuth proof of
concept. I think something is badly needed in this area.

--Jacob

--

--

Todd Hubers

--

--

Todd Hubers

Hi Everyone,

Benchmarking work has commenced, and is ongoing.

• OPTIONS 5/6/7 - `SET SESSION AUTHORIZATION` takes double the time of a single separate SimpleQuery. This is to be expected, because double the amount of SimpleQuery messages are being sent, and that requires a full SimpleQuery/Result/Ready cycle. If there is significant latency between a Connection Pooler and the database, this delay is amplified. It would be possible to concatenate text into a single SimpleQuery. In the real world, the performance impact MAY be negligible.
• OPTION 0 - The time to reconnect (start a new connection from scratch with a different username/password) was found to be faster than using `SET SESSION AUTHORIZATION`.
• OPTION 1 - My team is continuing to explore a distinct Impersonate message (Option-1). We are completing a prototype-quality implementation, and then benchmarking it. Given that Option-1 is asynchronous (Request and expect to succeed) and it can even be included within the same TCP packet as the SimpleQuery (at times), we expect the performance will be better than restarting a connection, and not impacted by links of higher latency.
  

I will be recording benchmark results in the document: https://docs.google.com/document/d/1u6mVKEHfKtR80UrMLNYrp5D6cCSW1_arcTaZ9HcAKlw/edit# after completion of the OPTION-1 prototype and benchmarking of that prototype.

Note: In order to accommodate something like OPTION-8, an Impersonation message might have a flag (valid for 1x SimpleQuery only, then automatically restore back to the last user).

Regards,

Todd

On Fri, 7 Jan 2022 at 10:55, Todd Hubers <todd.hubers@gmail.com> wrote:

Hi Everyone,

I have started working on this:

• Benchmarking - increasingly more comprehensive benchmarking
• Prototyping - to simulate the change of users (toggling back and forth)
• Draft Implementation - of OPTION-1 (New Protocol Message)
• (Then: Working with Odyssey and PgBouncer to add support (when the GRANT role privilege is available))

I hope to have a patch ready by the end of March.

Regards,

Todd

On Wed, 24 Nov 2021 at 02:46, Todd Hubers <todd.hubers@gmail.com> wrote:

Hi Jacob and Daniel,

Thanks for your feedback.

>@Daniel - I think thats conflating session_user and current_user, SET ROLE is not a login event. This is by design and discussed in the documentation..

Agreed, I am using those terms loosely. I have updated option 4 in the proposal document. I have crossed it out. Option 5 is more suitable "SET SESSION AUTHORIZATION" for further consideration.

>@Daniel - but it's important to remember that we need to cover the functionality in terms of *tests* first, performance benchmarking is another concern.

For implementation absolutely, but not for a basic feasibility prototype. A quick non-secure non-reliable prototype is probably an important first-step to confirming which options work best for the stated goals. Importantly, if the improvement is only 5% (whatever that might mean), then the project is probably not work starting. But I do expect that a benchmark will prove benefits that justify the resources to build the feature(s).

>@Jacob - A more modern approach might be to attach the authentication to the packet itself (e.g. cryptographically, with a MAC), if the goal is to enable per-statement authentication anyway. In theory that turns the middleware into a message passer instead of a confusable deputy. But it requires more complicated setup between the client and server.

I did consider this, but I ruled it out. I have now added it to the proposal document, and included two Issues. Please review and let me know whether I might be mistaken.

>@Jacob - Having protocol-level tests for bytes on the wire would not only help proposals like this but also get coverage for a huge number of edge cases. Magnus has added src/test/protocol for the server, written in Perl, in his PROXY proposal. And I've added a protocol suite for both the client and server, written in Python/pytest, in my OAuth proof of concept. I think something is badly needed in this area.

Thanks for highlighting this emerging work. I have noted this in the proposal in the Next Steps section.

--Todd

Note: Here is the proposal document link again - https://docs.google.com/document/d/1u6mVKEHfKtR80UrMLNYrp5D6cCSW1_arcTaZ9HcAKlw/edit#

On Tue, 23 Nov 2021 at 12:12, Jacob Champion <pchampion@vmware.com> wrote:

On Sat, 2021-11-20 at 16:16 -0500, Tom Lane wrote:
> One more point is that the proposed business about
>
> * ImpersonateDatabaseUser will either succeed silently (0-RTT), or
>   fail. Upon failure, no further commands will be processed until
>   ImpersonateDatabaseUser succeeds.
>
> seems to require adding a huge amount of complication on the server side,
> and complication in the protocol spec itself, to save a rather minimal
> amount of complication in the middleware.  Why can't we just say that
> a failed "impersonate" command leaves the session in the same state
> as before, and it's up to the pooler to do something about it?  We are
> in any case trusting the pooler not to send commands from user A to
> a session logged in as user B.

When combined with the 0-RTT goal, I think a silent ignore would just
invite more security problems. Todd is effectively proposing packet
pipelining, so the pipeline has to fail shut.

A more modern approach might be to attach the authentication to the
packet itself (e.g. cryptographically, with a MAC), if the goal is to
enable per-statement authentication anyway. In theory that turns the
middleware into a message passer instead of a confusable deputy. But it
requires more complicated setup between the client and server.

> PS: I wonder how we test such a feature meaningfully without
> incorporating a pooler right into the Postgres tree.  I don't
> want to do that, for sure.

Having protocol-level tests for bytes on the wire would not only help
proposals like this but also get coverage for a huge number of edge
cases. Magnus has added src/test/protocol for the server, written in
Perl, in his PROXY proposal. And I've added a protocol suite for both
the client and server, written in Python/pytest, in my OAuth proof of
concept. I think something is badly needed in this area.

--Jacob

--

--

Todd Hubers

--

--

Todd Hubers

--

--

Todd Hubers