morningman opened a new pull request, #70123: URL: https://github.com/apache/doris/pull/70123
### Background Query audit records every statement an FE executes: the built-in audit log builder writes it to `fe.audit.log` (the `query` module of `audit_log_modules`), and the built-in audit loader stream-loads the same events into `__internal_schema.audit_log` ([audit plugin docs](https://doris.apache.org/docs/admin-manual/audit-plugin)). Global variables (`enable_audit_plugin`, `audit_plugin_*`), the session variable `enable_prepared_stmt_audit_log` and several FE configs control what is recorded and how events are queued, batched and truncated. ADMIN can change most of them at runtime, and any user can change the session variable. ### What problem does this PR solve? Issue Number: close #70122 Related PR: #37894 (introduced the byte truncation whose negative-limit failure is fixed here), #38419 (added enable_prepared_stmt_audit_log), #38310 (added skip_audit_user_list) PR Type: Feature Problem Summary: A deployment that must audit every query cannot rely on query audit today: ADMIN can stop it or hollow it out at runtime, with `SET GLOBAL enable_audit_plugin = false`, `ADMIN SET FRONTEND CONFIG ("audit_event_log_queue_size" = "0")` or `SET GLOBAL audit_plugin_max_sql_length = 0`, and any user keeps their server-side prepared executions out of it. This PR adds the FE config `query_audit_always_on`: while it is on, no parameter that can be changed at runtime can make query audit weaker than its defaults. It also fixes audit records being dropped while `audit_plugin_max_sql_length` is negative. ### Details - **Scope:** With the switch on, the FE that receives a statement always produces its audit event, failed statements and every `COM_STMT_EXECUTE` included, and hands it to both `fe.audit.log` and the audit table. The protected parameters: | Parameter | Kind | With the switch on | |---|---|---| | `enable_audit_plugin` | global variable | stays true | | `enable_prepared_stmt_audit_log` | session variable | stays true | | `audit_plugin_max_sql_length` | global variable | at least 2097152 | | `audit_plugin_max_insert_stmt_length` | global variable | unlimited, its default | | `audit_plugin_load_timeout` | global variable | at least 600 | | `audit_plugin_max_batch_bytes`, `audit_plugin_max_batch_interval_sec` | global variables | fixed at 52428800 and 60 | | `audit_event_log_queue_size` | mutable FE config | at least 250000 | | `query_audit_log_timeout_ms`, `be_report_query_statistics_timeout_ms`, `workload_runtime_status_thread_interval_ms` | mutable FE configs | at most 5000, 60000 and 2000 | | `skip_audit_user_list`, `audit_log_modules` | FE configs read at startup | ignored; always includes `query` | Not covered: code execution inside the FE (`INSTALL PLUGIN`, a JDBC `driver_url`), tampering with written records (`TRUNCATE` on the audit table), reliable delivery (a full queue, a failed load or an FE crash still loses events), `enable_bdbje_debug_mode`, load and slow-query audit, and external audit plugins. - **Design:** One rule: no protected parameter may be weaker than its factory default. `QueryAuditPolicy` (fe-common) defines it once, for three sides. Readers use the effective value, because old values also arrive from the image, the edit log, forwarded sessions and existing sessions. `SET`, `SET_VAR`, `UNSET` and runtime config changes reject a weaker value, so the attempt fails and is audited itself. At startup, conflicting values in fe.conf and fe_custom.conf are replaced with a WARN instead of failing the start, and a master that takes over rewrites persisted non-compliant globals so that `SHOW VARIABLES` agrees. The checks sit on the entries users go through, not in `VariableMgr.setValue`, which replay and image loading also call and whose exceptions they swallow. A prepared execution forwarded to the master is audited only by the FE that received it, as `COM_QUERY` is. Rejected: making only `enable_audit_plugin` read-only, enforcing on read only (the change would appear to succeed), and smaller floors such as 4096 bytes for the SQL length. Proposed in #70122, opened with this PR; not discussed in the community yet. - **Usage:** ``` # fe.conf of every FE, then restart the FE query_audit_always_on = true ``` ```sql SET GLOBAL enable_audit_plugin = false; -- ERROR: Variable 'enable_audit_plugin' can't be set to 'false' because query_audit_always_on is enabled ADMIN SET FRONTEND CONFIG ("audit_event_log_queue_size" = "0"); -- ERROR: Failed to set config 'audit_event_log_queue_size'. err: Config 'audit_event_log_queue_size' can't be -- set to '0' because query_audit_always_on is enabled, the value must be at least 250000 ADMIN SHOW FRONTEND CONFIG LIKE 'query_audit_always_on'; ``` - **Default behavior:** Off. With it off, everything behaves as before except the negative-length fix below. - **Compatibility:** No metadata, image, edit log type, Thrift or Protobuf change; the master's rewrite uses the existing global variable edit log, which older FEs replay, and an older FE ignores the key in fe.conf. Each FE enforces its own setting, so every FE must be configured. With the switch on, a client that sets `enable_prepared_stmt_audit_log = false`, for example in JDBC `sessionVariables`, gets an error; every prepared execution is audited, which raises audit volume for high-QPS point queries; and a cluster that had shortened the recorded statements records up to 2 MB per statement. After the switch is turned off again, the rewritten globals keep their safe values. - **Security and privileges:** Turning query audit off now needs write access to the FE's config files and a restart instead of ADMIN. `threat-model.md` records the switch and adds ยง4.8 property 13 with what it does not cover (M20, which needs maintainer agreement). No privilege is added. - **Behavior change:** With the switch off too, a negative `audit_plugin_max_sql_length` is rejected by `SET` and counts as 0 if already set. Before, it was accepted and dropped the audit record of every statement except `INSERT ... VALUES`: `truncateByBytes` threw and `logAuditLog` swallowed the exception with the event. The same code is on branch-3.1, 4.0, 4.1 and 4.2; the fix is the first commit and can be picked alone. ### Testing - FE unit tests, run locally with `run-fe-ut.sh`: `QueryAuditPolicyTest` (new) for the effective values, checks and startup corrections; `ConfigTest` for runtime config changes and the immutable switch; `VariableMgrTest` for `SET [GLOBAL]`, `= DEFAULT`, `UNSET`, the `SET_VAR` hint and `UNSET ALL` with the switch on and off, and for the master's rewrite (four edit log entries whose replay carries the new values); `MysqlConnectProcessorLongDataTest` for prepared executions (audited with the session switch off, skipped by the master when forwarded, audited when they fail on uploaded data or binding); `AuditLoaderTest` and `AuditLogHelperTest` for the loader and the truncation. The related audit and prepared-statement test classes pass as well. - The negative-length fix: `AuditLogHelperTest#testNegativeMaxSqlLengthStillAudits` and `VariableMgrTest#testNegativeAuditMaxSqlLengthIsRejected` fail with the fix reverted (no event is submitted, and the run logs the swallowed `IndexOutOfBoundsException`) and pass with it. - Docker suite `audit/test_query_audit_always_on` (new): two FEs of this branch with the switch and conflicting values in fe.conf, connected to the follower. It checks the startup corrections, twelve rejected statements and a rejected `SET_VAR` hint, the immutable switch, unchanged globals on the follower, one `ERR` audit record per rejected statement, one record per prepared execution whether or not it is forwarded to the master, and the audit of a user in `skip_audit_user_list`. Run locally against a 4.1.3 BE: it passes, and passes again against the `.out` it generated, which was checked line by line. For that BE only, the local FE build pinned `be_exec_version = 10` and turned off `enable_local_shuffle_planner` and `enable_bucketed_hash_agg`, whose plan nodes the 4.1.3 BE does not know. No PR pipeline runs docker suites. - After rebasing onto the latest master, which changed `MysqlConnectProcessor` in #69805, the unit tests above ran again and pass. ### Notes for Reviewers - Commits: (1) the negative-length fix, which can be picked alone; (2) the config and `QueryAuditPolicy` in fe-common, with the startup corrections; (3) the write and read sides in fe-core and the prepared execution path; (4) the master's rewrite; (5) the docker suite; (6) `threat-model.md`. Start with `QueryAuditPolicy`. - Non-obvious edits: `VariableMgr.getValueToSet` is split out of `setVarInternal` so the check sees the value `= DEFAULT` resolves to. `AuditLogHelper.truncateStmt` replaces three copies of the truncation, one of them in `TableQueryPlanAction`. In `MysqlConnectProcessor.handleExecute`, the audited text starts as the prepared SQL while the switch is on, so an execution that throws is not recorded with an empty statement, and an execution that fails before its executor exists resets the query id, which still belonged to the connection's previous statement. The defaults moved into `QueryAuditPolicy` constants that `GlobalVariable` and `Config` use; `QueryAuditPolicy` names `enable_prepared_stmt_audit_log` as a string because `SessionVariable` is in fe-core, and `VariableMgrTest` checks that every name resolves. - Not in the design, found by the docker suite: a follower copies a forwarded `SET GLOBAL` into its own memory through `setVarForNonMasterFE` even when the master rejected the statement, because `StmtExecutor.forwardToMaster` runs `afterForwardToMaster` unconditionally. Before the fix, `SET GLOBAL enable_audit_plugin = false` through a follower failed for the client but left the follower's global at false. `setVarForNonMasterFE` now applies the same check, which also completes the negative-length fix; `forwardToMaster` itself is unchanged. - Open points: the severity tier of property 13 and M20 itself need maintainer agreement. A rejected `SET_VAR` hint fails with `Can not set session variable ...`, which does not show the reason; that message is unchanged. Docker suites do not run in the PR pipelines. ### Release note Add the FE config `query_audit_always_on` (default false; set it in fe.conf and restart the FE). When it is true, query audit cannot be disabled or weakened at runtime: `enable_audit_plugin` and `enable_prepared_stmt_audit_log` stay on, the other audit variables and the audit queue and timeout configs cannot be set weaker than their defaults, `skip_audit_user_list` is ignored, and every server-side prepared execution is audited. Also fix query audit records being dropped for every statement except `INSERT ... VALUES` while `audit_plugin_max_sql_length` is negative; setting it to a negative value is now rejected. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: Yes (a doris-website page on query_audit_always_on and a note in docs/admin-manual/audit-plugin.md; to be written) - Pre-submission status: Partially verified: the docker suite ran against a 4.1.3 BE, not a BE of this branch, and no PR pipeline runs it ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the code, the tests, the threat-model text, the commit messages and this description - Human review: The author wrote the design and reviewed the key code paths. - Prompts: The author wrote the design and asked to implement it with the develop-with-evidence and doris-pr-description skills, developing directly on the current branch without a new branch or worktree, and to open the PR. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
