Repository navigation
[feature](audit) Add query_audit_always_on to keep query audit from being disabled - #70123
Open
morningman wants to merge 7 commits into
Open
morningman wants to merge 7 commits into
morningman wants to merge 7 commits into
Conversation
…h is negative ### Background Every statement an FE executes produces a query audit event, which goes to `fe.audit.log` and, through the built-in audit loader, to `__internal_schema.audit_log`. Before building the event, `AuditLogHelper` truncates the statement text to the global variable `audit_plugin_max_sql_length` bytes (default 2097152); `INSERT ... VALUES` uses the smaller of it and `audit_plugin_max_insert_stmt_length`. The HTTP query-plan endpoint (`TableQueryPlanAction`) truncates its audited statement the same way. ### What problem does this PR solve? Issue Number: None Related PR: apache#37894 (introduced the byte truncation), apache#51314 (added the clamp on the INSERT path) PR Type: Bugfix Problem Summary: After `SET GLOBAL audit_plugin_max_sql_length = -1`, no statement other than `INSERT ... VALUES` is audited: neither `fe.audit.log` nor the audit table gets a record. The negative limit reaches `truncateByBytes`, `ByteBuffer.wrap(bytes, 0, -1)` throws `IndexOutOfBoundsException`, and `logAuditLog` swallows it together with the event. A negative limit now counts as 0, as it already does on the INSERT path, and `SET` rejects negative values. ### Details - **Frequency:** Every audited statement except `INSERT ... VALUES`, for as long as the variable is negative. Only ADMIN can set it. - **Root cause:** `handleStmt`, `handleCommand` and `TableQueryPlanAction` pass the variable to `truncateByBytes` unchecked. The `INSERT ... VALUES` path wraps it in `Math.max(0, ...)` since apache#51314; the other three never did. - **Why existing tests missed it:** `AuditLogBuilderTest` tries negative limits only on `INSERT ... VALUES` and on an empty statement, neither of which reaches `ByteBuffer.wrap`. No test drove `logAuditLog` with a negative limit and checked that an event was submitted. - **Fix:** The three non-INSERT paths share a new `AuditLogHelper.truncateStmt`, which clamps the limit at 0, so the record keeps the statement's truncation marker. `VariableMgr.setVar`, which `SET`, `SET_VAR` and `UNSET` go through, rejects a negative value, and so does `setVarForNonMasterFE`, through which a follower copies a forwarded `SET GLOBAL` into its own memory: `StmtExecutor.forwardToMaster` runs that copy even after the master rejected the statement. The check is not in `VariableMgr.setValue`, which edit log replay and image loading also call: a rejection there would be swallowed and skip the value on that FE only. A negative value already in the image counts as 0 when read. - **Behavior change:** `SET GLOBAL audit_plugin_max_sql_length` with a negative value now fails with `Variable 'audit_plugin_max_sql_length' can't be set to '-1', the value must not be negative`; it used to be accepted. - **Other paths and branches:** The same code is on branch-3.1, branch-4.0, branch-4.1 and branch-4.2. ### Testing - `AuditLogHelperTest#testNegativeMaxSqlLengthStillAudits` (new): with the limit at -1, `logAuditLog` submits an event whose statement is the truncation marker. With the fix reverted it fails (`Wanted but not invoked`, and the run logs the swallowed `IndexOutOfBoundsException`); with it, it passes. - `VariableMgrTest#testNegativeAuditMaxSqlLengthIsRejected` (new): `SET GLOBAL audit_plugin_max_sql_length = -1` fails and leaves the value unchanged, also on the follower's copy path, while 0 is accepted. It fails with the fix reverted. - Both classes pass in full locally with `run-fe-ut.sh` (AuditLogHelperTest 6, VariableMgrTest 17). ### Release note 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, and a negative value already set counts as 0. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the code, the tests and this message - Human review: not stated Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### Background Query audit records each statement an FE executes in `fe.audit.log` (its `query` module) and, through the built-in audit loader, in `__internal_schema.audit_log`. Global variables (`enable_audit_plugin`, `audit_plugin_*`), a session variable (`enable_prepared_stmt_audit_log`) and FE configs (`audit_event_log_queue_size`, three audit timeouts, `skip_audit_user_list`, `audit_log_modules`) control it, and ADMIN can change most of them at runtime. ### What problem does this PR solve? Issue Number: apache#70122 Related PR: None PR Type: Feature Problem Summary: A deployment that must audit every query cannot rely on query audit today: ADMIN can turn it off or hollow it out at runtime, for example with `SET GLOBAL enable_audit_plugin = false` or `audit_event_log_queue_size = 0`. This commit adds the FE config `query_audit_always_on` and `QueryAuditPolicy`, the one place that defines what the switch allows, and applies it to the config files at startup. The following commits apply it to variables and to the audit paths. ### Details - **Scope:** fe-common: the config; `QueryAuditPolicy` with the effective value of each protected variable, checks for variable and config changes, and startup corrections; the factory defaults as named constants, which `GlobalVariable` and `Config` now use; and a ConfHandler on the four mutable audit configs. fe-core: `DorisFE` applies the startup corrections and logs them once logging is up. - **Design:** With the switch on, no protected parameter may be weaker than its factory default: switches stay on, a limit whose smaller values drop events has its default as the minimum, a wait whose larger values hold events back has it as the maximum, and the audit loader's batching knobs are fixed. Separate, smaller floors were rejected, since nothing says how short a statement may be cut before it is erased. Conflicting values in fe.conf or fe_custom.conf, which loading never checks, are replaced in memory with a WARN instead of failing the start. - **Usage:** Set `query_audit_always_on = true` in fe.conf and restart the FE. `ADMIN SET FRONTEND CONFIG ("audit_event_log_queue_size" = "0")` then fails with `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`. - **Default behavior:** Off; nothing changes while it is off. - **Compatibility:** A new config read at startup only. No metadata, image, edit log, Thrift or Protobuf change, and an older FE ignores the unknown key in fe.conf. ### Testing - `QueryAuditPolicyTest` (new, fe-common): the effective values with the switch off and on, the variable and config checks, the startup corrections and the list they return, and that the registry names exactly the mutable configs that carry its handler. - `ConfigTest#testQueryAuditAlwaysOnRejectsWeakerAuditConfigs` (new): `setMutableConfig`, which `ADMIN SET FRONTEND CONFIG` and `/api/_set_config` both use, rejects a weaker value and leaves the config unchanged, and `query_audit_always_on` itself is not mutable. - Both pass locally with `run-fe-ut.sh`. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: Yes (a doris-website page on query_audit_always_on, written with the PR) - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the code, the tests and this message - Human review: not stated Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… prepared statements ### Background `SET`, the `SET_VAR` hint and `UNSET` change variables through `VariableMgr.setVar`, and `UNSET VARIABLE ALL` through `VariableMgr.setAllVarsToDefaultValue`. The audit loader reads `enable_audit_plugin` and its batching and timeout variables on every event or batch, `AuditLogHelper` truncates statements by the `audit_plugin_max_*` lengths, and a server-side `COM_STMT_EXECUTE` is audited only when the session's `enable_prepared_stmt_audit_log` is on. ### What problem does this PR solve? Issue Number: apache#70122 Related PR: None PR Type: Feature Problem Summary: With `query_audit_always_on` on, the audit variables must not be able to switch query audit off or weaken it, whether through a statement or through values already persisted, forwarded or held by older sessions, and every `COM_STMT_EXECUTE` must be audited once. This commit applies `QueryAuditPolicy` where the variables are set and where they are read, and to the prepared execution path. ### Details - **Scope:** `VariableMgr.setVar` rejects a value the policy does not allow, which covers `SET [GLOBAL]`, `= DEFAULT`, the `SET_VAR` hint and `UNSET`, and so does a follower's copy of a forwarded `SET GLOBAL` (`setVarForNonMasterFE`, which runs even when the master rejected the statement); `UNSET VARIABLE ALL` leaves such a variable as it is instead of failing. The audit loader, the audit stream load and the statement truncation read the effective values. A `COM_STMT_EXECUTE` is audited by the FE that received it whatever its session says; the master does not audit one forwarded to it. An execution that fails before its executor exists (uploaded long data, parameter binding) is audited with the statement it was prepared from, as a failed statement. - **Design:** The write check is on the entries users set variables through, not in `VariableMgr.setValue` or a VarAttr checker: replaying the edit log and loading the image also call `setValue`, which swallows the exception and would skip the value on one FE only, and checkers cannot cover `GlobalVariable` fields. The read side is what guarantees the rules, since old values also arrive from the image, the edit log and forwarded sessions. Only the receiving FE audits a forwarded execution, as for `COM_QUERY`; auditing on both would record a forwarded prepared `INSERT` twice with the same query id. A failed execution no longer carries the query id of the connection's previous statement. - **Usage:** With the switch on, `SET enable_prepared_stmt_audit_log = false` fails with `Variable 'enable_prepared_stmt_audit_log' can't be set to 'false' because query_audit_always_on is enabled`, and every prepared execution is audited. - **Default behavior:** With the switch off, the code paths behave as before. - **Compatibility:** No metadata, image, edit log, Thrift or Protobuf change. Followers apply `SET GLOBAL` from the master without checking it again, so the FE that runs the statement decides. ### Testing - `VariableMgrTest`: with the switch on, `SET [GLOBAL]`, `= DEFAULT`, `UNSET`, the `SET_VAR` hint and the follower's copy path fail for each weaker value and leave the variables unchanged; stronger values are accepted; `UNSET ALL` keeps `enable_prepared_stmt_audit_log` and resets the rest; with it off, all of these succeed; every protected name resolves to a variable. - `MysqlConnectProcessorLongDataTest`: with the switch on, an execution is audited although the session switch is off, the master skips a forwarded one, and executions failing on uploaded data or on binding are audited with the prepared statement and without the previous query id; with it off, the old behavior holds. - `AuditLoaderTest`, `AuditLogHelperTest`: the loader keeps events with `enable_audit_plugin = false`, and statements, `INSERT ... VALUES` included, are cut at the default length at the earliest. - These and the related `AuditLogBuilderTest`, `AuditLogHelperBackendSelectionTest`, `AuditLogWorkloadGroupTest`, `MysqlConnectProcessorCursorFetchTest`, `MysqlConnectProcessorExecuteCleanupTest`, `ConnectProcessorForwardProtocolTest`, `WorkloadRuntimeStatusMgrTest` and `StreamingInsertTaskAuditTest` pass locally with `run-fe-ut.sh`. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: Yes (a doris-website page on query_audit_always_on, written with the PR) - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the code, the tests and this message - Human review: not stated Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ster takes over ### Background Global variables are persisted in the image and the edit log. When a master takes over, `Env.transferToMaster` already rewrites some persisted values, for example the defaults that changed in an upgrade (`VariableMgr.forceUpdateVariables`), and writes each rewrite to the edit log for the followers. ### What problem does this PR solve? Issue Number: apache#70122 Related PR: None PR Type: Feature Problem Summary: A cluster that turns `query_audit_always_on` on may have persisted `enable_audit_plugin=false` or a short `audit_plugin_max_sql_length` earlier. The read side already ignores such values, but `SHOW VARIABLES` would keep showing them. The master now rewrites them to their effective values when it takes over. ### Details - **Scope:** `VariableMgr.applyQueryAuditPolicyToGlobalValues`, called once from `Env.transferToMaster` after the upgrade rewrites. Session values of connections that are already open are not rewritten; the read side covers them. - **Design:** The same lock and the same global variable edit log as `SET GLOBAL` and `refreshDefaultSessionVariables`, so that the followers replay the rewrites and every FE shows the same values; each rewrite is logged at INFO. Only the master writes the edit log, so a follower does not rewrite anything itself. - **Usage:** After turning the switch on and restarting, `SELECT @@global.enable_audit_plugin` returns 1 even if it was set to 0 before. - **Default behavior:** Nothing happens with the switch off. - **Compatibility:** Uses the existing global variable edit log, which older FEs replay. Side effects: after the switch is turned off again, the variables stay at the rewritten values, and the global default of `enable_prepared_stmt_audit_log` becomes true. ### Testing - `VariableMgrTest#testAlwaysOnRewritesPersistedGlobalValues` (new): four non-compliant globals are rewritten, an allowed value stays, exactly four global variable edit logs are written, and what they replay carries the new values; with the switch off nothing changes. Passes locally with `run-fe-ut.sh`. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: Yes (a doris-website page on query_audit_always_on, written with the PR) - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the code, the tests and this message - Human review: not stated Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### Background `query_audit_always_on` is read from fe.conf at startup and cannot be changed at runtime, so a regression suite needs a cluster of its own, which docker suites (`docker(options)`) start with the FE configs they declare. With two FEs, a suite connected to the follower also exercises the paths that forward to the master: `SET GLOBAL` and a prepared `EXECUTE` under `force_forward_all_queries`. ### What problem does this PR solve? Issue Number: apache#70122 Related PR: None PR Type: Test Problem Summary: The unit tests of the previous commits mock the MySQL execution path and the audit pipeline. This suite checks the switch end to end: the FE startup corrections, every supported attempt to weaken query audit through a follower, and what reaches `__internal_schema.audit_log`. ### Details - **Scenarios:** fe.conf sets `query_audit_always_on=true` with conflicting `skip_audit_user_list`, `audit_log_modules` and `audit_event_log_queue_size`, which show up corrected. Twelve weakening statements (`SET [GLOBAL]`, `= DEFAULT`, `UNSET`, `ADMIN SET [ALL] FRONTEND(S) CONFIG`) and a `SET_VAR` hint fail, the switch itself is not mutable, and the globals keep their values on the follower. Each rejected statement is audited once as `ERR`; a prepared execution is audited once whether or not it is forwarded to the master; the user in `skip_audit_user_list` is audited. - **What existing tests miss:** The suites under `regression-test/suites/audit/` run with the switch off. No test covers a follower forwarding these statements, and the unit tests never start an FE from fe.conf. - **Fails when broken:** An earlier run, before the follower's copy of a forwarded `SET GLOBAL` was checked, failed here: the follower reported `enable_audit_plugin` as false and `audit_plugin_max_sql_length` as 100 after the master had rejected both. A duplicate audit record from the master, a missing record, or a lost startup correction changes the `.out` too. ### Testing - Ran locally against two FEs of this branch and a 4.1.3 BE, from a runner container on the Docker VM's host network: passes, and the `.out` was generated by it and checked line by line. For the 4.1.3 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 that BE does not know. No PR pipeline runs docker suites. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Partially verified: run against a 4.1.3 BE, not a BE of this branch; docker suites do not run in CI ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the suite, its `.out` (generated by the run) and this message - Human review: not stated Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### Background `threat-model.md` is the security contract of the repository: what Doris guarantees, what it leaves to operators, and how reports are triaged. It treats a malicious `SUPER` or ADMIN user as out of model (M3) and lists the configuration knobs that move the model (§4.5a), such as `enable_all_http_auth` (M19). ### What problem does this PR solve? Issue Number: apache#70122 Related PR: None PR Type: Build/CI/Chore Problem Summary: `query_audit_always_on` makes "no database account can turn query audit off" a property, which M3 alone would classify as out of model. This commit records the switch as a §4.5a knob, adds it as §4.8 property 13 with what it does not cover, notes the exception on the malicious-ADMIN row of §4.7, and proposes the decision as M20, pending maintainer agreement. ### Details - **Affected:** `threat-model.md` only: the status line, a §4.5a row, §4.7, §4.8 property 13, and a wave-6 table with M20 in §4.14. - **Developer and release impact:** Reviewers and triagers classify reports about disabling query audit by the switch: with it on, a supported entry that still disables or hollows out query audit is a violation; with it off, M3 holds. Code execution inside the FE, tampering with written records and reliable delivery stay out of scope. The severity tier of property 13 is left to the maintainers. - **License and third-party:** None. - **Rollback:** Revert this commit; no code depends on the text. ### Testing - Not applicable: documentation only. The statements in property 13 are covered by the tests of the commits that implement the switch. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the text and this message - Human review: not stated Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
morningman
requested review from
924060929,
CalvinKirs,
dataroaring and
morrySnow
as code owners
November 3, 2026 12:52
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review recovery stopped: Command '['gh', 'api', 'repos/apache/doris/pulls/70123']' returned non-zero exit status 1. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 28196 ms |
Contributor
TPC-DS: Total hot run time: 150749 ms |
Contributor
ClickBench: Total hot run time: 24.37 s |
morningman
added a commit
that referenced
this pull request
Nov 4, 2026
…it messages (#70175) ### Background Every PR against `master` starts from `.github/PULL_REQUEST_TEMPLATE.md`, and the root `AGENTS.md` tells coding agents to write commit messages in the same layout. The current template asks for a problem summary, a release note, and checkboxes for tests, behavior changes, and documentation. Doris merges PRs by squashing, so the description becomes the commit message on `master`. ### What problem does this PR solve? Issue Number: None Related PR: #42999 (introduced the current template), #70123 (a description written in this layout) PR Type: Build/CI/Chore Problem Summary: The current template leaves out what reviewers most often have to ask for: the context of the module, why the change is needed and why this approach, why existing tests missed a bug, and what was verified. Many PRs leave its checkboxes unticked. This PR replaces it with a layout that opens with Background, has required fields for each PR type, asks a diff of more than about 300 changed lines to open Details with a Change map, and adds Testing, optional Notes for Reviewers, a pre-submission status, and an AI disclosure. Commit messages written by `AGENTS.md` rules follow the same layout. ### Details - **Affected:** `.github/PULL_REQUEST_TEMPLATE.md`, which every new PR description starts from, and the commit and worktree rules in the root `AGENTS.md`. No build scripts, CI jobs, or workflows change; `title-checker.yml` accepts the same `[type](scope)` titles as before. - **Developer and release impact:** Authors fill in more sections, with the required fields of each PR type listed in a comment under Details; the reviewer checklist is gone. Coding agents write commit messages in the new layout, and from a worktree they may keep PR handoff files in `${DORIS_REPO}/.git/pr-handoff/`. Releases are unaffected. - **License and third-party:** None. - **Rollback:** Revert this PR; nothing else depends on the template. ### Testing - `check_pr.py`, the checker of the `doris-pr-description` skill (not part of this PR; see Notes for Reviewers), reports the unfilled template as not ready, with 17 errors that include the empty Background, Problem Summary, Testing, and Release note and every blank answer line. Its eleven bundled examples, one or two per PR type and written in this layout, pass it, and so does this description. - The description of #70123, written in this layout, passes the same checker, with one warning that its prose is longer than the limit. ### Notes for Reviewers - Two commits: the template and the `AGENTS.md` rules, then guidance for the Change map and three refined comments. With the HTML comments removed, the second commit changes nothing. - `AGENTS.md` also drops the second copy of the paragraph on which files a commit may contain. - The new `AGENTS.md` text names the `develop-with-evidence` and `doris-pr-description` skills, which are not in this repository yet. They are to be added to [apache/doris-skills](https://github.com/apache/doris-skills), which a later PR includes here as a git submodule. ### Release note None ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the template, the `AGENTS.md` text, the commit messages, and this description - Human review: The author reviewed all of the changes line by line.
Member
|
Reviewed
I also think the policy could be narrower in three places:
I would split out the independently useful negative-SQL-length fix, retain the necessary always-on enforcement, and settle the numerical tuning policy and permanent-normalization requirement separately. The central policy abstraction is reasonable if the broad parameter scope remains; the main simplification opportunity is the scope of the rules. This was a source review; I did not run builds or tests. |
…n identity ### What problem does this PR solve? Issue Number: apache#70122 Related PR: apache#70123 Problem Summary: An oversized audit-plugin timeout caused every audit-table load to fail. Fit authenticated built-in audit loads to the master's Stream Load limits at transaction begin and load planning, where authoritative bounds are available. Also keep the timeout representable by the load sink in signed milliseconds. Forwarded plans preserve the master-selected timeout despite different follower limits. Variable replication and ordinary loads remain unchanged. Prepared executions failing before executor creation retained the previous command's statement ID and SQL hash. Clear the unallocated statement ID and restore the hash captured at PREPARE for every always-on execute. Retaining the effective preparation hash also preserves SQL conversion fallback and subsequent SQL-hash block-rule matching. ### Release note Prevent unsupported timeouts from disabling always-on audit-table loads and preserve the correct statement identity for prepared-execution audit rows. ### Check List (For Author) - Test: Regression tests cover master timeout selection, transaction-validator acceptance, planning and forwarded-plan timeouts, authentication/non-audit exclusions, actual audit events for binding/long-data failures, valid retry, and effective preparation hash reuse. - Behavior changed: Yes - Does this need documentation: No separate page; update the config description and policy comments for master-side Stream Load timeout enforcement.
Member
|
run buildall |
Contributor
TPC-H: Total hot run time: 28852 ms |
Contributor
TPC-H: Total hot run time: 28825 ms |
Contributor
TPC-DS: Total hot run time: 154279 ms |
Contributor
ClickBench: Total hot run time: 24.04 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Query audit records every statement an FE executes: the built-in audit log builder writes it to
fe.audit.log(thequerymodule ofaudit_log_modules), and the built-in audit loader stream-loads the same events into__internal_schema.audit_log(audit plugin docs). Global variables (enable_audit_plugin,audit_plugin_*), the session variableenable_prepared_stmt_audit_logand 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:
Deployments that must audit every query cannot rely on query audit today: ADMIN can switch it off at runtime, for example with
SET GLOBAL enable_audit_plugin = falsefor the audit table, and any user can leave their prepared statements out. This PR addsquery_audit_always_on, an FE config that can only be changed in fe.conf with a restart, so ADMIN cannot turn it off at runtime. Lockingenable_audit_pluginalone is not enough: twelve other parameters can also skip statements, lose events, or cut the recorded text, so while the switch is on none of them can be set weaker than its default, and enforcing that is most of the code. The switch is off by default, and with it off nothing changes after an upgrade except a bundled fix: a negativeaudit_plugin_max_sql_length, which silently dropped audit records, is now rejected.Details
Change map: +1,649 −51 in 20 files. Tests are 1,019 of the added lines (62%): FE unit tests +824 and a docker suite +195, 37 of them a generated
.out.threat-model.mdis +35 −1. The production code, +595 −50, has five parts:QueryAuditPolicy, new, +441, a third of it comments and blank lines;Config+24 −10;GlobalVariable+7 −5): the protected parameters and their safe values, the checks, and the startup corrections. Read it first.VariableMgr, about +40 −11):SET,SET_VAR,UNSET, and a follower's copy of a forwardedSET GLOBAL;ADMIN SET FRONTEND CONFIGgoes through a callback from part 1.AuditLogHelperandAuditLoader), since old values still come from the image, the edit log, forwarded sessions and existing sessions.DorisFE+6,Env+2,VariableMgr+28): apply the startup corrections; rewrite persisted globals.MysqlConnectProcessor+25 −3): audit each one, also when it fails early.Commits: first the negative-length fix (+53 −24, tests +73), which can be picked alone, then the policy, its enforcement, the master's rewrite, the docker suite, and the threat model.
Scope: With the switch on, the FE that receives a statement always produces its audit event, failed statements and every
COM_STMT_EXECUTEincluded, and hands it to bothfe.audit.logand the audit table. The protected parameters:enable_audit_pluginenable_prepared_stmt_audit_logaudit_plugin_max_sql_lengthaudit_plugin_max_insert_stmt_lengthaudit_plugin_load_timeoutaudit_plugin_max_batch_bytes,audit_plugin_max_batch_interval_secaudit_event_log_queue_sizequery_audit_log_timeout_ms,be_report_query_statistics_timeout_ms,workload_runtime_status_thread_interval_msskip_audit_user_list,audit_log_modulesqueryNot covered: code execution inside the FE (
INSTALL PLUGIN, a JDBCdriver_url), tampering with written records (TRUNCATEon 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, enforced wherever a value can come from (parts 2 to 4 of the Change map). A weaker
SETfails, so the attempt is visible and audited itself; a conflicting value in fe.conf or fe_custom.conf is replaced with a WARN instead of failing the start; and the master's rewrite makesSHOW VARIABLESagree with what is enforced. The checks sit on the entries users go through, not inVariableMgr.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, asCOM_QUERYis. Rejected: making onlyenable_audit_pluginread-only, enforcing on read only (the change would appear to succeed), and smaller floors such as 4096 bytes for the SQL length. Proposed in [Feature][Audit] Add query_audit_always_on to keep query audit from being disabled at runtime #70122, opened with this PR; not discussed in the community yet.Usage:
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 JDBCsessionVariables, 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.mdrecords 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_lengthis rejected bySETand counts as 0 if already set. Before, it was accepted and dropped the audit record of every statement exceptINSERT ... VALUES:truncateByBytesthrew andlogAuditLogswallowed 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
run-fe-ut.sh, and again after the rebase onto the latest master, which changedMysqlConnectProcessorin [fix](fe) Preserve non-UTF-8 prepared statement parameters #69805:QueryAuditPolicyTest(new) for the effective values, the checks and the startup corrections;ConfigTestfor runtime config changes and the immutable switch;VariableMgrTestforSET [GLOBAL],= DEFAULT,UNSET, theSET_VARhint andUNSET ALLwith the switch on and off, and for the master's rewrite, whose four edit log entries replay with the new values;MysqlConnectProcessorLongDataTestfor prepared executions, forwarded and failing ones included;AuditLoaderTestandAuditLogHelperTestfor the loader and the truncation. The related audit and prepared-statement test classes pass as well.AuditLogHelperTest#testNegativeMaxSqlLengthStillAuditsandVariableMgrTest#testNegativeAuditMaxSqlLengthIsRejectedfail with the fix reverted (no event is submitted, and the run logs the swallowedIndexOutOfBoundsException) and pass with it.audit/test_query_audit_always_on(new): two FEs of this branch with the switch on and conflicting values in fe.conf, connected to the follower. It checks the startup corrections, twelve rejected statements and a rejectedSET_VARhint, the immutable switch, unchanged globals on the follower, oneERRaudit record per rejected statement, one record per prepared execution whether or not it is forwarded, and the audit of a user inskip_audit_user_list. Run locally against a 4.1.3 BE, for which the local FE build pinnedbe_exec_version = 10and turned offenable_local_shuffle_plannerandenable_bucketed_hash_agg; it passes, and passes again against the.outit generated, which was checked line by line. No PR pipeline runs docker suites.Notes for Reviewers
VariableMgr.getValueToSetis split out ofsetVarInternalso the check sees the value= DEFAULTresolves to.AuditLogHelper.truncateStmtreplaces three copies of the truncation, one of them inTableQueryPlanAction. InMysqlConnectProcessor.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.QueryAuditPolicynamesenable_prepared_stmt_audit_logas a string becauseSessionVariableis in fe-core, andVariableMgrTestchecks that every name resolves.SET GLOBALinto its own memory throughsetVarForNonMasterFEeven when the master rejected the statement, becauseStmtExecutor.forwardToMasterrunsafterForwardToMasterunconditionally. Before the fix,SET GLOBAL enable_audit_plugin = falsethrough a follower failed for the client but left the follower's global at false.setVarForNonMasterFEnow applies the same check, which also completes the negative-length fix;forwardToMasteritself is unchanged.SET_VARhint fails withCan not set session variable ..., which does not show the reason; that message is unchanged.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_pluginandenable_prepared_stmt_audit_logstay on, the other audit variables and the audit queue and timeout configs cannot be set weaker than their defaults,skip_audit_user_listis ignored, and every server-side prepared execution is audited. Also fix query audit records being dropped for every statement exceptINSERT ... VALUESwhileaudit_plugin_max_sql_lengthis negative; setting it to a negative value is now rejected.Check List (For Author)
AI Disclosure