Skip to content
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

Disable prepared statements for PostgreSQL scaler #5138

Closed
ArtemijRodionov opened this issue Oct 30, 2023 · 9 comments
Closed

Disable prepared statements for PostgreSQL scaler #5138

ArtemijRodionov opened this issue Oct 30, 2023 · 9 comments
Labels
feature-request All issues for new features that have not been committed to needs-discussion

Comments

@ArtemijRodionov
Copy link

ArtemijRodionov commented Oct 30, 2023

Proposal

I suggest to use QueryExecModeExec from pgx for PostgreSQL scaler. This will disable prepared statements and allow KEDA work with PostgreSQL, when using proxies like PgBouncer. Even though prepared statements are good for safety and speed, the scaler doesn't really need them for its tasks, because of low QPS and absence of user's input

Use-Case

I want to use KEDA with PostgreSQL behind a proxy. But there's a problem: the proxy doesn't support prepared statements, and I can't change query mode with URI's query parameters because I don't have access to the component that sets up the connection string

Is this a feature you are interested in implementing yourself?

Yes

Anything else?

From my perspective, the KEDA PostgreSQL scaler doesn't initially require prepared statements. Therefore, it appears as a good idea to address this issue from this end.

Thanks in advance!

@ArtemijRodionov ArtemijRodionov added feature-request All issues for new features that have not been committed to needs-discussion labels Oct 30, 2023
@JorTurFer
Copy link
Member

I think that this can be a good improvement, but maybe as an optional parameter because there is a user input with the query. An attacker could get access to the database creating a user thanks to scaler query if we don't prepare the statement.

I'm not 100% sure about how to protect this (in KEDA side, inside database, KEDA user must have just read permissions), what do you think @zroubalik ?

@hacst
Copy link

hacst commented Nov 8, 2023

For us this is actually a regression/bug. We just stumbled over this when trying to update from 2.10.1 to the current KEDA version as our setup relies on the KEDA PostgreSQL being able to work through pgbouncer in transaction pooling mode. From the changelog pgx was introduced in 2.11.0 so presumably the previous option did not use prepared statements.

The error we see is:

2023-11-08T13:02:06Z    ERROR   postgresql_scaler       could not query postgreSQL: ERROR: prepared statement "stmtcache_22225" does not exist (SQLSTATE 26000)     {"type": "ScaledJob", "namespace": "xxx", "name": "xxx", "error": "ERROR: prepared statement \"stmtcache_22225\" does not exist (SQLSTATE 26000)"}

Not sure why there would be a security difference though. Postgresql client libraries usually safely handle arguments passed to them for a query regardless of whether prepared statements are used. From the documentation linked above pgx also behaves that way.

@ArtemijRodionov
Copy link
Author

Hi! @zroubalik @JorTurFer

Could you please take a look when you have a chance? If it's a bug, I would like to fix it

@zroubalik
Copy link
Member

Hi, adding this as an optional parameter makes total sense to me. @ArtemijRodionov I will go ahead and assign this issue to you.

Copy link

stale bot commented Jan 14, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Jan 14, 2024
@hacst
Copy link

hacst commented Jan 14, 2024

As far as I am aware this is not resolved yet. Any blockers on this @ArtemijRodionov?

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Jan 14, 2024
@ArtemijRodionov
Copy link
Author

ArtemijRodionov commented Jan 15, 2024

@hacst There are no blockers. I found the way to set a pgx's query parameter for a connection string default_query_exec_mode=exec to disable preprared statements, so it's fixed for my case. I will step back from this issue

@ArtemijRodionov ArtemijRodionov removed their assignment Jan 15, 2024
@JorTurFer
Copy link
Member

so, can this be closed?

@hacst
Copy link

hacst commented Feb 8, 2024

We used the connection string workaround successfully. That makes the connection string pgx specific. From my POV this can be closed by it definitely remains a trap people with pgbouncer in their setup will keep stumbling over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request All issues for new features that have not been committed to needs-discussion
Projects
Archived in project
Development

No branches or pull requests

4 participants