kaxil commented on code in PR #74444:
URL: https://github.com/apache/airflow/pull/74444#discussion_r4218337197
##########
providers/common/ai/docs/agent_security.rst:
##########
@@ -112,7 +112,11 @@ No single layer is sufficient on its own. They work
together.
(``DESCRIBE``/``SHOW``) statements pass; INSERT, UPDATE, DELETE, DROP,
and writes hidden behind ``EXPLAIN`` are rejected.
- Does not prevent the agent from reading sensitive data that the
- database user has SELECT access to.
+ database user has SELECT access to. Without ``allowed_tables`` the
check is
Review Comment:
This reads as if setting `allowed_tables` closes the gap, but the function
check only rejects calls sqlglot parses as `Anonymous`. Typed sequence syntax
still passes with `allowed_tables` set: T-SQL `SELECT NEXT VALUE FOR seq`
parses as `NextValueFor`, and Snowflake `SELECT seq.nextval` parses as a
column. Maybe reuse the `sql.rst` wording here ("rejects only functions sqlglot
cannot type")?
##########
providers/common/ai/docs/agent_security.rst:
##########
@@ -112,7 +112,11 @@ No single layer is sufficient on its own. They work
together.
(``DESCRIBE``/``SHOW``) statements pass; INSERT, UPDATE, DELETE, DROP,
and writes hidden behind ``EXPLAIN`` are rejected.
- Does not prevent the agent from reading sensitive data that the
- database user has SELECT access to.
+ database user has SELECT access to. Without ``allowed_tables`` the
check is
+ statement-level only: a side-effecting function inside a SELECT
+ (``nextval``, ``pg_terminate_backend``, ``dblink_exec``) is not
blocked. Use a
+ least-privilege role, or a read-only transaction where the database
supports
Review Comment:
I think this should be "and" rather than "or", which is also what `sql.rst`
says. A read-only session does stop `nextval`, but on Postgres 17
`pg_terminate_backend` still succeeds with `default_transaction_read_only =
on`, and `dblink_exec` inside `BEGIN READ ONLY` still ran a `DELETE`, because
dblink opens its own remote session. So a reader who picks the read-only option
is still open to two of the three functions named in the sentence above.
Also, "transaction" isn't something the user can set up here.
`SQLToolset._query` goes through `hook.run()`, which opens a fresh connection
and commits on every call, so only a role or session default reaches the
agent's queries, e.g. `ALTER ROLE <role> SET default_transaction_read_only =
on`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
