[v25.3.x] [CORE-15822] security/audit: make audit initialization controller-leader independent - #31587
Closed
vbotbuildovich wants to merge 4 commits into
Closed
Conversation
create_internal_topic() unconditionally sent CreateTopics to the controller, so every audit enable/startup depended on controller availability even when the audit topic already existed. In two analyzed production incidents (CORE-15822, CORE-12785) the topic existed and the produce path was viable the whole time, yet the audit client looped on this step forever, which left the audit queues undrained and rejected authentication cluster-wide. Check the local topic table first and skip creation when the topic is already known. RPC sink only: in the Kafka-client sink the create step doubles as the ephemeral-credential bootstrap -- its SASL failure is what triggers inform() to the brokers -- so skipping it there leaves the client unable to authenticate. A stale-negative table read falls back to the old path; a stale-positive read (topic deleted mid-enable) has the same exposure as today's create-then-delete race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 93b416b)
Disable and re-enable auditing on a cluster where the audit topic already exists. RPC sink: assert creation is skipped and events generated after the re-enable still reach the audit log. Kafka-client sink: assert creation is deliberately NOT skipped -- its SASL failure is what triggers ephemeral-credential propagation (inform), so this pins the contract against a future "harmonizing" refactor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit fd294c6)
Give the kafka-client sink the same controller-leader independence the RPC sink got from the exists-skip, so re-initialization (broker restart, audit re-enable) cannot wedge on a leaderless controller: - proactively inform() all brokers of the audit principal's credential right after minting it. Previously propagation rode on the CreateTopics SASL failure via mitigate_error; a broker that minted a fresh credential while its peers were unreachable wedged on connect() even after quorum returned -- peers reject SASL, the client aggregates the per-broker failures into broker_not_available with no node id, and the sasl_authentication_failed mitigation never fires. Only a broker restart healed it. - skip create_acls when both audit-topic bindings are readable locally; the write is a raft-0 command needing an elected leader. - skip CreateTopics when the audit topic exists in the local topic table; without a leader the request cannot even be routed to the controller broker. Safe now that inform-all owns credential propagation instead of the create's SASL failure. - treat topic_authorization_failed like illegal_sasl_state in update_status, so a missing-ACL state (e.g. a stale positive from the ACL exists-skip) degrades loudly through _misconfigured instead of silently dropping batches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 110deaf)
A leaderless controller must not block auditing: both sinks initialize during a leaderless window, an admin event audited during the window is consumed back from the audit topic while the controller is still leaderless, and events keep flowing after quorum returns -- all with no broker restart. Covered: - AuditLogLeaderlessControllerTest (matrix over both transports): 5 brokers, the single audit partition pinned to the first three, stop 3 of 5 so raft group 0 cannot elect a leader while the audit partition keeps 2/3 replicas; restart a survivor, assert initialization completes through the exists-skips with no failure/retry loop, read the in-window admin event back from the audit topic, then the post-recovery event too. - AuditLogTopicRecreateTest (both transports): false-positive guard -- when the audit topic genuinely does not exist the skip must not fire and initialization goes through CreateTopics again. - AuditLogTopicExistsTest: the kafka-client expectation flips from "must keep issuing CreateTopics" to the new contract: skip, ACL skip and the proactive inform-all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 183963b)
Contributor
|
Closing it - we might backport it only if needed. |
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
3 participants
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.
Backport of PR #31411
Conflict details
//src/v/cluster:topic_tabletoimplementation_deps, but v25.3.x has no fine-grained//src/v/cluster:*targets (that split is dev-only) —topic_table.{cc,h}are still part of the monolithic//src/v/clusterlibrary, which this rule already lists indeps. Resolved by taking the target-branch side, leaving the BUILD file unchanged; the newcluster/topic_table.hinclude in client.cc is already satisfied.