Repository navigation
feat(OMN-18012): flip the dev-lane broker to SASL and give all 15 clients the credential - #3276
Conversation
✅ Hostile Reviewer — REVIEWEDCritical findings: 2
Semantics (OMN-17492 — the model finds, thread resolution gates)
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review, qwen3-review-b, glm-review (OMN-8468/OMN-8524/OMN-17492) |
3200e33 to
83cfda5
Compare
#8555) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#3276 * evidence: OCC companion self-bind for #8555 * evidence(OMN-18012): rebind OCC#8555 receipt to the union contract after OCC#8544 and OCC#8550 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai> Co-authored-by: jonahgabriel <jonah@omninode.ai>
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 2
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[CRITICAL] hostile-reviewer (glm-review)
Flip one-shot replaces the entire superusers list |
rpk cluster config set superusers "['$USER']"is a full-value assignment on a list-typed cluster property. Any pre-existing superusers on this cluster are silently removed, and any future operator-added superuser will be removed the next time the idempotent re-run executes (the comment explicitly advertises re-running as safe). Idempotence here is destructive idempotence: the script re-asserts a singleton list, not membership. | Evidence: /usr/bin/rpk clResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
No retry around the unauthenticated-refusal check after a dynamic config set |
enable_saslwas confirmed needs_restart=False, but a dynamically set cluster property still propagates through the controller log to each broker. The script flips enable_sasl and immediately probes the Kafka listener with an unauthenticated client. If propagation has not completed, the probe succeeds, the one-shot exits 1, and the deploy fails spuriously. The readback for enable_sasl reads the controller's view, not the listeneResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Credential passed on the rpk command line | The AUTH string expands the password into argv for every authenticated rpk invocation. Inside a single-purpose container the exposure is limited, but the password is readable from /proc//cmdline for the process lifetime and may surface in any future exec/logging instrumentation. The username and password are already exported in the service environment; argv is strictly worse. | Evidence: AUTH="-X sasl.mechanism=SCRAM-SHA-256 -X user=$$DEV_KAFKA_SASL_USERNAME
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Substring grep on the superusers readback can false-positive |
grep -q "$DEV_KAFKA_SASL_USERNAME"matches if the username appears as a substring of any element of the serialized list (e.g. principalsvc_kafkamatches a list containingsvc_kafka_v2). The assertion that the grant landed is therefore weaker than the comment claims. | Evidence: /usr/bin/rpk cluster config get superusers $$ADMIN | grep -q "$$DEV_KAFKA_SASL_USERNAME" | Fix: Match with delimiters appropriate to rpk's output format (grep -E "Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
enable_sasl readback asserts on an unspecified output format |
grep -qx trueassumesrpk cluster config get enable_saslprints the bare tokentrue. rpk's default YAML-flavored config output may quote or format boolean values; a format change in the pinned image line or a future image bump breaks the readback as a false failure, and the assertion's coupling to presentation is undocumented. | Evidence: /usr/bin/rpk cluster config get enable_sasl $$ADMIN | grep -qx true | Fix: Use `rpk cluster config getResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (glm-review)
redpanda-sasl-enable is absent from the deploy-runtime restart path | The change adds DEV_LANE_EXTRA_BROKER_CLIENTS to deploy-runtime.sh and adds the flip to refresh_dev_lane.sh, but the flip one-shot appears in neither deploy-runtime restart array. The ticket's central invariant (the refusal exists and is verified) is asserted only by a service that compose runs on
up. If the broker volume is rebuilt through the sanctioned deploy path, or the one-shot's prior run predates a controller-log change, the depResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Branch-scoping test depends on incidental file ordering | test_the_extra_array_is_appended_only_on_the_dev_lane_branch slices the script with text.index("lane_only_services=(") and text.index("docker-compose.stability-test.yml)"). This assumes the dev branch is the first lane_only_services assignment and precedes the stability-test case label. Reordering cases or adding a lane above the dev branch silently turns the slice into the wrong region, and the test then asserts against an irrelevant span (and can p
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
…ents the credential Phase B of OMN-18012, and the half that makes the refusal exist. Phase A (#3258) created the lane's SCRAM principal and deliberately changed nothing else: the listener still accepted PLAINTEXT, so nothing was proven yet. Escape 1 of 2026-09-06: omnimarket's credential_publisher and generation_publisher built their event bus without apply_environment_overrides, produced a PLAINTEXT client against the IAM-only MSK listener, and every BYOK registration 503'd in front of a customer. Nothing local caught it because every local proof lane runs a no-auth broker, and a no-auth broker accepts that client. WHAT LANDS - redpanda-sasl-enable, a dev-lane one-shot that grants the principal superuser, sets enable_sasl=true, and then asserts the result: an unauthenticated rpk against the Kafka listener must FAIL and an authenticated one must SUCCEED. The second half is the positive control -- a broker that is simply down refuses the anonymous client too. - x-dev-lane-broker-auth-env, binding the four names ModelKafkaEventBusConfig.apply_environment_overrides() reads, on all 15 Kafka-speaking dev-lane services. MEASURED, NOT ASSUMED enable_sasl needs_restart=False and superusers needs_restart=False, read live from this lane's own admin API cluster_config/schema. That is what makes a flip on a running lane safe rather than a broker recreate. kafka_enable_authorization is null, which in Redpanda means "follow enable_sasl", so the superuser grant has to precede the flip -- reversed, every client authenticates and is then authorized for nothing, which reads identically in a client log. A cluster-config one-shot and not --set on the broker command: --set is a node-startup flag, this lane's redpanda_data volume already exists (cluster config version 2), so a --set would render, deploy, log nothing and leave the listener open. It is also the only safe form -- the redpanda service is declared in the BASE file that four other lanes merge. TWO DEFECTS THE NEW GATES CAUGHT, BOTH FIXED HERE 1. context-audit-consumer is a dev-lane Kafka client that NO deploy path restarts: absent from RUNTIME_SERVICES and from DEV_LANE_ONLY_RUNTIME_SERVICES. It had been up 2 weeks across every intervening deploy. Binding a credential on a container nobody recreates is inert, so it would have kept speaking PLAINTEXT to a listener that now refuses it -- escape 1's shape reintroduced by the change meant to prevent it. deploy-runtime.sh gains DEV_LANE_EXTRA_BROKER_CLIENTS, expanded in the dev branch only. 2. redpanda-sasl-enable was outside refresh_dev_lane.sh's REFRESH_BUILD_SERVICES, so a governed refresh would never re-assert the flip. The four observability consumers looked like an exception and are not: they resolve their bootstrap ADDRESS from a service-prefixed variable and their AUTH from the unprefixed names, through the same apply_environment_overrides(). Read in each consumer module, not inferred from the prefix -- that asymmetry is why this was not a search-and-replace on one variable. RED/GREEN: the 10 new assertions fail on the parent (origin/dev a868bbd) and pass here. The restart-coverage gate isolates the defect exactly -- with the compose half applied and the deploy half not, the uncovered set is precisely ['context-audit-consumer']. BLAST RADIUS, by real compose render: dev goes 29 -> 30 services, 0 -> 15 authenticated clients, 1 -> 2 one-shots. prod (12 services) and stability-test (19 services) render rc=0 with zero authenticated clients and zero one-shots, before and after. Zero occurrences of any of the five needles in the base file or in the prod, stability-test, judge and lakshman overlays; positive control, 28 occurrences in the dev-lane overlay. Credentials are fail-closed compose references resolved from the operator env file, never literals. NOT a credential rotation (Operating Rule 22): this binds the synthetic principal phase A provisioned; nothing is rotated, re-issued or revoked. RESIDUAL: this proves PLAINTEXT-vs-authenticated, not IAM. Staging and prod are AWS_MSK_IAM over SASL_SSL; the mechanism differs, the pinned failure mode -- a client built with no credentials at all -- does not. The IAM leg stays a staging concern. Ticket: OMN-18012
83cfda5 to
e6a658b
Compare
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: qwen3-review, qwen3-review-b, glm-review
Models failed: none
New finding threads: 3
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 3
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[CRITICAL] hostile-reviewer (qwen3-review)
Insecure credential handling in rpk command arguments | The
redpanda-sasl-enableservice passes SASL credentials directly as command-line arguments viarpk cluster config set. In Linux environments, command-line arguments are visible to all users viaps auxor/proc/<pid>/cmdline. This exposes the SCRAM-SHA-256 password in plaintext to any local user or process with read access to the process table. While the container is ephemeral, this violates standard security practices for credential handling.Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[CRITICAL] hostile-reviewer (qwen3-review-b)
Credentials exposed via container inspection and logs | The
redpanda-sasl-enableservice runs in the same Docker network as the broker and other clients. It receivesDEV_KAFKA_SASL_USERNAMEandDEV_KAFKA_SASL_PASSWORDas environment variables. Docker containers expose their environment variables viadocker inspectand/proc/self/environ. Any compromised container or user withdocker inspectaccess can retrieve these plaintext credentials. Furthermore, the script usesechostatements that may lResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Unauthenticated admin API can silently undo the SASL flip | The flip's entire purpose is to make the refusal exist, yet the same one-shot acknowledges the admin API stays open with admin_api_require_auth=false. Any client that can reach redpanda:9644 on the omnibase-infra-network can issue
cluster config set enable_sasl falseor rewritesuperusers, silently reverting the gate. Nothing in the flip service, the refresh script, or the test suite detects this reversion; the readback runs once at flip time aResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
SCRAM password exposed in process arguments and container metadata | The password is interpolated into
rpk -X pass=$$DEV_KAFKA_SASL_PASSWORDas a command-line argument, readable viapsinside the container and via process auditing. It is additionally stored in plaintext in the flip container's environment, visible to anyone withdocker inspecton the host. The compose comments treat reference-vs-literal as the credential boundary, but that boundary only covers the YAML file; the runtime exposure is unResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Superuser readback grep is a substring match and treats the username as a regex |
grep -q "$DEV_KAFKA_SASL_USERNAME"against the superusers list will match if the username appears as a substring of any other principal, and treats regex metacharacters in the username as patterns. A username such assvcwould falsely confirm whensvc-adminis a superuser and the intended principal's grant silently failed. The flip would then enable SASL with no authorized principal, producing exactly the lockout the ordResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Unquoted shell expansions break on spaces or special characters in credentials |
$$ADMIN,$$BROKER, and$$AUTHare expanded unquoted, and AUTH embeds the username and password directly. Any whitespace or glob character in the operator-supplied password splits into multiple argv entries, producing an authentication failure whose diagnostics will not indicate the cause.set -udoes not catch this class. | Evidence: AUTH="-X sasl.mechanism=SCRAM-SHA-256 -X user=$$DEV_KAFKA_SASL_USERNAME -X pass=$$DEV_KResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
SASL_PLAINTEXT with SCRAM leaves the lane vulnerable to MITM credential harvesting | SCRAM-SHA-256 over a plaintext listener protects the raw password but is subject to man-in-the-middle downgrade and offline dictionary attack on the SCRAM exchange. The comments acknowledge the mechanism differs from staging (SASL_SSL) and argue the failure mode transfers, but the credential-transfer property does not: a network observer on the dev lane captures auth material staging never exposes. The tradeoff may be accep
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Flip ordering relative to client container recreation is unverified | The flip one-shot runs during compose up, while the deploy script restarts runtime services from its arrays. If any client container starts (or is recreated) before the flip completes, it attempts a plaintext connection against a listener that may already require SASL, producing a transient failure window. Nothing in depends_on or the deploy script sequences runtime restarts after redpanda-sasl-enable exits successfully; the change relies
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MINOR] hostile-reviewer (glm-review)
Grant-precedes-flip test asserts on comment text, not executable script |
script.index("cluster config set superusers")searches the full command text including the extensive comment block. If a future comment mentions the string earlier than the actual command, the ordering assertion passes while the lockout window it guards is open. The same brittleness applies to the refusal/positive-control string assertions, which test echo literals rather than rpk invocations. | Evidence: grant_at = script.index("clResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Bash array regex is fragile against inline closing parens |
_bash_arraymatches^readonly NAME=\((.*?)^\)non-greedily to the first line starting with). Any array whose body contains a line beginning with)(for example a wrapped one-element array or a comment terminating in a paren placed at line start) truncates the parse. The positive control checks counts >= 15, which mitigates but does not eliminate this: a truncation that still leaves 15+ entries yields a silently wrong set and a vacuous coveResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Test derives client set from the overlay only, missing base-declared clients on the dev lane |
_authenticated_dev_lane_clientsenumerates services declared in docker-compose.dev-lane.yml. Any Kafka client declared solely in the base infra file and inheriting credentials through a shared anchor (or added to the dev lane later via x-common-env paths) escapes the guard. The test's docstring claims it 'derives the client set from the compose overlay' as a virtue, but the merge of base and overlay is the actuaResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
…SCRAM credential (#3392) OMN-18012 flipped the dev lane's broker to SASL and, in its own words, gave "all 15 clients the credential" (#3276). The gateway forwarder was not one of the fifteen. It lives in a different compose project (omninode-gateway) and configures its bus legs per-leg in a mounted YAML rather than through the shared x-dev-lane-broker-auth-env anchor, so the enumeration that credentialed the lane did not reach it. Both of its dev-lane legs -- local_bus and lane_mirror_buses.dev -- kept declaring PLAINTEXT against a listener that now closes a plaintext connection at handshake. Every republish failed at connect, and because the deployed build predates the delivery-confirmation fix it logged a delivery anyway. The stability->dev hook mirror has moved zero records since. Measured on .201 2026-09-10, read-only, each zero with a positive control: rpk --config <empty> -X brokers=<dev external> cluster metadata -> "SASL is required but not provided" rpk --config <empty> -X brokers=<stability external> cluster metadata -> CLUSTER / BROKERS ... all four dev hook topics' high watermarks static across 91s (tool-executed 599687, session-started 1251873, prompt-submitted 8660, session-ended 9759) while the forwarder logged 110 "Lane mirror delivered" lines in three minutes control: onex.dlq.omnibase-infra.quarantine.v1 11475729 -> 11475736 The credential is a REFERENCE resolved from an operator-supplied mounted map, never a literal and never an environment variable. beta-gateway-canary.yaml is tracked in this repo AND mounted into the container verbatim, so a value spelled there is a credential in git; an env var would be readable from `docker inspect` for the life of the container, and this repo's check-env-reads gate refuses new os.environ reads outside the overlay surfaces. A leg declares sasl_credential_ref and _materialize_lane_broker_credentials resolves it against the map passed as --lane-credential-map -- the same not-in-git, operator-mounted mechanism the cloud leg's endpoint already rides via --broker-ref-map. A single ambient KAFKA_SASL_* is deliberately not the mechanism: this process holds three legs on three brokers with three postures (cloud MSK SASL_SSL / AWS_MSK_IAM, the stability source leg still PLAINTEXT, the dev legs SASL_PLAINTEXT / SCRAM-SHA-256). One value cannot be right for all three and would break the leg that works. test_the_source_leg_stays_plaintext pins that asymmetry so a later "make them consistent" edit cannot undo it. Fails closed in every direction: no map, an absent file, a non-mapping file, a missing entry or an empty field all refuse to start naming the ref; a ref on a PLAINTEXT leg refuses rather than silently collecting a credential it would never send; a ref alongside a literal refuses as ambiguous. A bare literal is left alone on purpose so the loader can re-read its own model_dump() output -- the same trap _materialize_contract_https_ingest records for its null round trip; that the SHIPPED file carries no literal is asserted statically over the tracked YAML instead. NOT A ROTATION (Operating Rule 22). This binds the principal OMN-18012 phase A provisioned. Nothing is rotated, re-issued or revoked, and no value appears in the diff, the tests, the error messages or the logs. Red at parent bcbbf90: 15 failed / 1 passed, the one pass being the premise that the stability source leg stays plaintext. Green at head: 284 passed across the forwarder node, the runtime loader, the compose-credential and deploy-gateway suites.
OMN-18012 phase B — the dev lane's broker now refuses a plaintext client
Phase A (#3258) created the lane's SCRAM principal and deliberately changed
nothing else: the listener still accepted PLAINTEXT. This is the half that makes
the refusal exist.
Escape 1 of 2026-09-06: omnimarket's
credential_publisherandgeneration_publisherbuilt their event bus withoutapply_environment_overrides, produced a PLAINTEXT client against the IAM-onlyMSK listener, and every BYOK registration 503'd in front of a customer. No local
check could catch it, because every local proof lane runs a no-auth broker and a
no-auth broker accepts that client.
What lands
redpanda-sasl-enableenable_sasl=true, then asserts an unauthenticated rpk fails and an authenticated one succeedsx-dev-lane-broker-auth-envModelKafkaEventBusConfig.apply_environment_overrides()reads, on all 15 Kafka-speaking dev-lane servicesDEV_LANE_EXTRA_BROKER_CLIENTSdeploy-runtime.shnow recreatescontext-audit-consumeron the dev lanerefresh_dev_lane.shredpanda-sasl-enableadded toREFRESH_BUILD_SERVICESso a governed refresh re-asserts the flipMeasured, not assumed
Read live from this lane's own admin API
cluster_config/schema:enable_saslneeds_restart=False,superusersneeds_restart=False. That is what makes aflip on a running lane safe rather than a broker recreate.
kafka_enable_authorizationis null — in Redpanda that means "followenable_sasl" — so the superuser grant must precede the flip. Reversed, every
client authenticates and is then authorized for nothing, which reads identically
in a client log.
A cluster-config one-shot and not
--seton the broker command:--setis anode-startup flag, this lane's
redpanda_datavolume already exists (clusterconfig version 2), so a
--setwould render, deploy, log nothing and leave thelistener open. It is also the only safe form —
redpandais declared in the BASEfile that four other lanes merge.
Two defects the new gates caught, both fixed here rather than deferred
context-audit-consumeris a dev-lane Kafka client that no deploy pathrestarts — absent from
RUNTIME_SERVICESand fromDEV_LANE_ONLY_RUNTIME_SERVICES. Up 2 weeks across every intervening deploy.Binding a credential on a container nobody recreates is inert, so it would
have kept speaking PLAINTEXT to a listener that now refuses it — escape 1's
own shape, reintroduced by the change meant to prevent it.
redpanda-sasl-enablewas outsiderefresh_dev_lane.sh's build scope, soa governed refresh would never re-assert the flip.
The four observability consumers looked like an exception and are not: they
resolve their bootstrap address from a service-prefixed variable and their
auth from the unprefixed names, through the same
apply_environment_overrides(). Read in each consumer module, not inferred fromthe prefix — that asymmetry is why this was not a search-and-replace.
RED / GREEN
The 10 new assertions fail on the parent (
origin/deva868bbd91) and passhere. The restart-coverage gate isolates the defect exactly: with the compose
half applied and the deploy half not, the uncovered set is precisely
['context-audit-consumer'].Blast radius, by real
docker compose configrenderZero occurrences of any of the five needles in the base file or in the prod,
stability-test, judge and lakshman overlays. Positive control: 28 occurrences in
the dev-lane overlay. The stability-test / judge / lakshman render errors seen
during this work reproduce identically at the parent commit and are unrelated to
this change.
Credentials are fail-closed compose references resolved from the operator env
file, never literals. Not a credential rotation (Operating Rule 22): this
binds the synthetic principal phase A provisioned; nothing is rotated, re-issued
or revoked, and none is authorized.
Residual
This proves PLAINTEXT-vs-authenticated, not IAM. Staging and prod are
AWS_MSK_IAMoverSASL_SSL; the mechanism differs, the pinned failure mode — aclient built with no credentials at all — does not. The IAM leg stays a staging
concern.
Ticket: OMN-18012
Evidence-Ticket: OMN-18012
Evidence-Source: OCC#8555