Repository navigation
fix(OMN-15692): MSK gateway-only stopgap — reject direct-broker/bastion-IP endpoints - #1547
Merged
Merged
Conversation
added 3 commits
August 4, 2026 13:27
Operator ruling 2026-08-04 ("nothing in either .200 or .201 should be
contacting MSK directly, everything should be going through the gateway")
required a real, executable CI/pre-commit mechanism, not a named-but-unbuilt
carrier. Adds rule 5 (msk-direct-broker-endpoint) to ValidatorUrlAuthority:
- Fires on an MSK broker hostname (*.kafka.us-east-1.amazonaws.com) on the
SASL_SSL/MSK-IAM ports (9098/9096), OR the raw SNI-passthrough bastion IP
on its own (Docker Compose `extra_hosts` and /etc/hosts overrides map
hostname -> bare IP with no port literal at all).
- Unlike rules 1-4 (Python-only), also scans non-.py on-prem-facing config
(.yaml/.yml/.sh/.env/.cfg/.conf/.toml/.ini) via scan_tree/scan_source's
new is_python gate, without importing rules 1-4's broader match surface
onto those files (proven via TestMskDirectBrokerEndpoint's negative
controls + test_scan_tree_control_yaml_produces_no_violations).
- Wired into both the CI `--all` full-repo scan and the local pre-commit
hook (files regex widened from `types: [python]`), proven live: a real
tracked YAML file in this repo carrying the MSK literal fails the
pre-commit hook (exit 1); the CLI --all path RED/GREENs on a fixture
matching the verifier's own reproduction.
- Executed (not just unit-tested) against the live omnibase_infra tree via
a PYTHONPATH-shadowed run under infra's own venv: catches
docker-compose.gateway.yml L51-52 (the previously-invisible instance) AND
beta-gateway-canary.yaml:35 (hostname:9098, a positive the prior grep-only
inventory missed). Infra itself pins omnibase-core==0.46.8 from PyPI, so
this rule does not enforce on infra's own CI until that pin bumps past a
release carrying this change — a real propagation lag, not something this
PR can close by itself; recorded as a residual, not silently assumed.
- Fixes a pre-existing, unrelated check-localhost-url-compute failure this
file already carried on dev (a doc-comment example, not a real endpoint)
that surfaced because this PR touches the file.
81/81 unit tests pass (30 new). Full pre-commit run on the touched files is
clean. mypy --strict: 0 issues.
…e 5; correct propagation claim Adversarial verifier round #2 found the msk-direct-broker-endpoint rule (commit c3e90cb) was real but incompletely closed, plus a factual error in that commit's own residual-disclosure paragraph. This commit fixes all six cited defects. Detection gaps (rule 5, validator_url_authority.py): - Hostname-match was gated on a co-occurring SASL_SSL/MSK-IAM port (9098 or 9096) on the SAME line. This missed: the ordinary split-key config shape (MSK_HOST:/MSK_PORT: on separate lines — the default Docker Compose/.env shape, not an edge case), a hostname with no port literal anywhere, and any other broker port (e.g. 9092/9094). Fix: the hostname literal alone now triggers the rule, regardless of port or which line a port (if any) appears on — an MSK broker DNS name is unambiguous evidence of a direct reference on its own. - The hostname pattern was hardcoded to us-east-1. Fix: generalized to any AWS region (`\.kafka\.[a-z0-9-]+\.amazonaws\.com`). - Proven via 5 new RED tests reproducing each cited executed scenario (split-key, no-port, port-9094, other-region) plus negative controls proving the guard still does NOT fire on unrelated content. Suppression hole: - `# url-authority-ok: <reason>` previously cleared ANY matched rule, including rule 5 — a self-authored comment could waive a hard operator ruling with no stated exception path. Fix: `scan_source` now only honors the suppression annotation for rules 1-4; rule 5 is unconditionally non-suppressible. Proven via a RED test (suppressed MSK literal still fires) and a non-regression GREEN test (rules 1-4 remain suppressible). Propagation-claim correction (the prior commit message was WRONG here): - The prior commit claimed infra's enforcement is gated by the `omnibase-core==0.46.8` PyPI pin in omnibase_infra/pyproject.toml. False: omnibase_infra's URL Authority Gate does not consume that pin at all. `.github/workflows/url-authority-gate.yml` invokes `uv run --with "omnibase-core @ git+...@8a53a063bd28f643d08b4cbbc6dd5c7c9f6435df"` (a 2026-06-21 core commit, predates this rule) at both call sites, and `.pre-commit-config.yaml` separately pins `rev: be4f954` (2026-06-24, also predates this rule). Bumping the PyPI pin alone would leave both surfaces dormant on the one repo holding every known live violation. The correct propagation artifacts are those two git-SHA pins. - Undisclosed sequencing hazard, now closed for this ticket's scope: the moment those pins bump, infra's URL Authority Gate goes red on any pre-existing, not-yet-baselined violation for the newly-added rule. Live-scanned the real omnibase_infra tree (read-only) with the fixed rule: exactly 3 msk-direct-broker-endpoint hits, matching the prior session's inventory (docker/docker-compose.gateway.yml:51,52; docker/gateway/beta-gateway-canary.yaml:35) — the region/port relaxation above did not surface any additional infra hits beyond those 3, and omnibase_core's own self-scan stays at 0 new / 13 grandfathered (confirms the guard fires on its target and nowhere else, by execution). Pre-seeded exactly those 3 fingerprints into url_authority_baseline.json (212 -> 215 entries; diff is additive-only, verified via git diff — no existing entry touched). This grandfathers the 3 known rule-5 hits so the pin bump does not immediately wedge every infra PR; it does not fix the underlying docker-compose/canary files, which stay tracked on OMN-15694/OMN-15534. DISCLOSED, NOT FIXED (out of scope for OMN-15692): the same live infra scan also surfaced 24 NEW localhost-literal (rule 4, OMN-13480) hits against the real committed baseline — a pre-existing, unrelated gap that will ALSO block infra's CI the moment the pins bump, independent of this ticket. Not seeded here (seeding it would be scope creep under this ticket's authority into a different rule/ticket's debt); flagged on OMN-15694 instead. 85/85 unit tests pass (7 new, 2 rewritten to assert detection instead of evasion, 1 removed as directly contradicted by the fix). Full pre-commit clean on all four touched files (validator, contract yaml, baseline json, test file). mypy --strict: 0 issues. Self-scan of omnibase_core: 0 new violations. Live CLI RED/GREEN proof against real fixture files (not just in-process scan_source) for: split-key detection, suppression non-waiver, and the clean-GREEN case.
…anti-gaming scope, file-selection + test-path evasions An independent adversarial verification pass (round #3) reviewed the branch at c04acfc and found 7 defects, some carried over from round #2's own "fix". This commit closes all 7. 1. Baseline self-defeat: the branch pre-seeded the ONLY 3 known live msk-direct-broker-endpoint violations (omnibase_infra docker/docker-compose.gateway.yml:51,52 + docker/gateway/ beta-gateway-canary.yaml:35) into url_authority_baseline.json — the guard reported ZERO on exactly what it was built to catch. Removed the 3 entries (215 -> 212, matching the pre-OMN-15692 count). These 3 real violations are NOT grandfathered; the pin bump (see #6) will correctly wedge omnibase_infra CI on them once it lands — that's the gate working, not a regression. Retirement of the underlying config stays tracked on OMN-15694/OMN-15534, outside this ticket's authority to silently fix. 2. Anti-gaming blind spot: url-authority-gate.yml's baseline-growth assertion filtered to `repo == "omnibase_core"`, so growth in any other repo's baseline entries (e.g. omnibase_infra, which this rule targets) was invisible. Made the growth check repo-agnostic — it now compares the full fingerprint set across all repos recorded in the shared baseline file. 3. .env file-selection hole: the CLI staged-file mode's suffix filter (`p.suffix not in _MSK_SCAN_SUFFIXES`) silently dropped a bare `.env` file — `Path(".env").suffix == ""` (pathlib treats a leading dot as part of the stem, not a suffix). Added `_is_msk_scannable()`, a basename-aware selector used by the CLI filter, scan_tree, and documented in the pre-commit `files` regex, that explicitly matches `.env`, the `.env.<profile>` family (`.env.local`, `.env.production`, ...), and `Dockerfile`/`Dockerfile.<variant>` (also no-suffix). Added a CLI-layer test (`test_dotenv_fixture_detected_via_staged_file_cli`) that drives the real `main()` staged-file path against a `.env` fixture, not just scan_source. 4. Test-substring exemption: `_is_test_path` used a bare `"test" in lowered` substring check, which waived `deploy/latest.yaml` ("la-TEST-.yaml"), `docker/stability-test/**` (a real deployment LANE name, not test code), and `attestation.yaml` ("at-TEST-ation.yaml"). Rewrote it to anchor on real test-path segments only: a path component that IS exactly "test"/"tests", a `test_` basename prefix, a `_test` basename suffix, or the exact basename `conftest.py`. Added regression tests proving all four evasion-class paths now detect, plus non-regression tests proving real test paths (`tests/`, `test_*.py`, `*_test.py`, `conftest.py`) still get skipped. 5. Suffix gaps: added `.json`, `.tf`, and (via the new basename selector) `Dockerfile`/`Dockerfile.<variant>` and the `.env.<profile>` family to the msk-direct-broker-endpoint file-selection surface. Fixture tests added for each (bare `.env`, `.env.production`, `Dockerfile`, `Dockerfile.gateway`, `.json`, `.tf`) via `scan_tree`, plus direct unit coverage of `_is_msk_scannable`. 6. Wiring (propagation to omnibase_infra, where every known live violation lives): out of this commit's scope by construction (core must merge first) — see the paired omnibase_infra draft PR / follow-up ticket opened alongside this one, which bumps the two pins (url-authority-gate.yml's `--with` git-SHA pin + .pre-commit-config.yaml's `rev`) to this PR's merge commit once it lands. 7. Evidence drift: docs/tracking/2026-08-04-omn-15692-on-prem-msk-inventory.md (omni_home docs branch) updated separately to describe this remediation round and state plainly that the 3 catalogued violations are not grandfathered. Also fixed a scan_tree ordering bug found while implementing #3/#4: the old code checked `_is_test_path(candidate.name)` (basename only) BEFORE computing `rel` (the full repo-relative path), so directory-based test exclusion (`tests/**`) was never actually exercised by that pre-filter — it worked anyway only because `scan_source` re-checks `_is_test_path` on the full `rel` path downstream. Reordered so the pre-filter itself uses the full relative path. 125 unit tests pass (105 in this file: 85 baseline + 20 new). mypy --strict: 0 issues on the touched module. Full pre-commit run on all 6 touched files: clean (including the URL Authority Gate hook itself, and the "Reject committed .env files" hook, since the new .env test fixtures are tmp_path-scoped, never committed). Self-scan of omnibase_core's own tree with the widened file-selection surface: still 0 new violations (13 grandfathered, unchanged) — no accidental true positives from the wider .json/.tf/Dockerfile/.env.* scan surface in this repo. Verified (round #3, this pass): the repo-agnostic anti-gaming check was proven by direct simulation of the workflow's embedded Python (cross-repo baseline growth that the old `repo=="omnibase_core"` filter would have missed is now correctly caught). The pre-commit `files` regex was verified by direct regex simulation against 12 representative filenames (all 6 positive classes + 3 exclusions + suffix baseline), all matching intent. Local gates run on this Mac, not .200 — .200 SSH is unreachable this session (stated exception, not a silent narrowing; matches the prior commit's own disclosed .200-unreachable state). OMN-15692 (epic OMN-12908 / OMN-12803). Ruling 39, operator 2026-08-04: "nothing in either .200 or .201 should be contacting MSK directly, everything should be going through the gateway."
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe URL authority validator now scans supported non-Python files for direct MSK broker endpoints. Existing Python-only rules retain their scope. Hooks, CLI scans, tests, and baseline growth checks now use the expanded repository-wide behavior. ChangesURL authority validation expansion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant FileSelector
participant URLAuthorityValidator
participant BaselineGate
CLI->>FileSelector: select Python and MSK-scannable files
FileSelector->>URLAuthorityValidator: scan selected files
URLAuthorityValidator-->>CLI: return violations
CLI->>BaselineGate: compare violation fingerprints
BaselineGate-->>CLI: report repository-wide growth status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/url-authority-gate.yml:
- Around line 111-116: Update the baseline-growth message in the workflow’s
`sys.stderr.write` block to state that rule 5 (`msk-direct-broker-endpoint`)
entries cannot be suppressed and must be removed. Limit `# url-authority-ok:`
guidance to rules 1–4, while preserving the existing burn-down instruction.
In @.pre-commit-config.yaml:
- Line 1397: Update the staged-file regex in the pre-commit configuration to
match filenames ending in .env, including gateway.env and config.env, while
preserving existing Dockerfile, .env-profile, and supported-extension matches.
Add a regression test covering a staged *.env file and verify it is recognized
by the same gate used by _is_msk_scannable.
In `@src/omnibase_core/validation/validator_url_authority.py`:
- Around line 213-217: The _MSK_BROKER_HOSTNAME and _MSK_BASTION_IP patterns
currently match endpoint substrings; update both to require complete
endpoint-token boundaries, explicitly encode the supported hostname suffix, and
add tests covering valid endpoints plus hostname/IP values followed by extra
suffix text.
- Around line 455-459: Update the line-processing logic around the existing
blank/comment skip and _match_msk_rule call to track complete Python
triple-quoted docstring spans, skipping every line until the matching delimiter
closes. Add file-type-aware handling for semicolon comments in .ini/.cfg files
and // plus block comments in .tf files before URL matching, while preserving
normal content processing and Rule 5 behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca5306a7-ad15-4b42-942a-fb387bc1f9e6
📒 Files selected for processing (5)
.github/workflows/url-authority-gate.yml.pre-commit-config.yamlsrc/omnibase_core/validation/contracts/url_authority.validation.yamlsrc/omnibase_core/validation/validator_url_authority.pytests/unit/validation/test_validator_url_authority.py
- Anchor the MSK broker-hostname and bastion-IP regexes with negative lookaheads so a longer hostname/IP that merely contains the endpoint as a substring (e.g. amazonaws.com.example, 100.53.215.198.5) no longer false-positives. Regression tests cover the boundary. - Track open/close state for Python triple-quoted docstrings and .tf block comments (/* ... */) across lines, and recognize .ini/.cfg ';' and .tf '//' line-comment markers, so a literal on an interior documentation line is skipped instead of scanned as code. Since rule 5 is non-suppressible, an unfixable gate failure inside documentation was otherwise unresolvable. 11 new tests cover open/interior/close spans per file type plus non-regression (comment markers do not leak into unrelated file types). - Add `env` to the pre-commit files suffix alternation so real *.env files (gateway.env, config.env) reach the staged-file gate, matching _is_msk_scannable's suffix coverage. - Correct the baseline-growth error message: rule 5 (msk-direct-broker-endpoint) is not suppressible via `# url-authority-ok:` — the entry must be removed, not annotated. Suppression guidance is now scoped to rules 1-4 only. 119 tests passing (108 + 11 new), mypy --strict clean, ruff clean. Evidence-Ticket: OMN-15692
jonahgabriel
added a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Aug 5, 2026
…#6083) * evidence(OMN-15692): OCC companion for OmniNode-ai/omnibase_core#1547 OCC evidence companion for the round-#3 remediation of the MSK gateway-only enforcement rule (ruling 39, operator 2026-08-04) — omnibase_core PR #1547 fixes 7 adversarial-verification defects against the msk-direct-broker-endpoint url-authority rule: baseline self-defeat (the branch had pre-seeded the 3 only known live violations, self-defeating the guard), an anti-gaming check scoped to repo=="omnibase_core" only (blind to omnibase_infra's own baseline growth), a .env file-selection hole (Path.suffix is empty for a bare .env file), a bare-substring test-path exemption that waived deploy/latest.yaml / docker/stability-test/** / attestation.yaml, and missing .json/.tf/ Dockerfile/.env.<profile> scan-surface coverage. * evidence(OMN-15692): add self-bind entry for OCC companion PR #6083 Cites OMN-15692. * evidence(OMN-15692): PASS receipts for OCC companion PR #6083, independently verified Cites OMN-15692. runner=claude-code-subagent-omn-15692-remediation ran all 5 probes at 2026-08-05T00:42:38Z; verifier=fable-delta-0804-orchestrator (session foreground orchestrator, distinct identity) independently re-executed the same 5 probes at ~2026-08-04T23:05Z and confirmed matching results. run_timestamp records the verifier's confirming run per the adversarial-receipts convention (verifier != runner required for PASS).
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
1 participant
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.
Summary
Adds a new
url_authorityrule (msk-direct-broker-endpoint) toomnibase_core.validation.validator_url_authoritythat rejects direct MSK broker endpoints and bastion IPs outside the sanctioned gateway path. This closes OMN-15692's stopgap enforcement gap.OMN-15692
Cites OMN-15692.
dod_evidencesummary:msk-direct-broker-endpointinfra violations, then GREEN once the fix pattern is applied in a follow-on infra PR.uv run pytest tests/unit/validation/test_validator_url_authority.py -p no:randomly -q→119 passed), all green. (Correction, round-2 remediation: an earlier revision of this body claimed "30 new unit tests, 125 total" — that arithmetic doesn't reconcile against the described baseline and doesn't match the measured count; 119 is the live-verified figure at the merge commit.)tests/,test_*.py,*_test.py,conftest.py), and non-MSK file types are NOT flagged.mypy --strictreports 0 issues on the touched module; fullpre-commit run --all-fileson all 6 touched files is clean.url_authority_baseline.json, self-defeating the rule it was built to enforce. That version was rejected by independent adversarial review (round feat(tests): Complete TKT-002 ONEXContainer validation with comprehensive test suite #3) and fixed in75c85126— the 3 entries were removed from the baseline (215 → 212, matching the pre-OMN-15692 count).Seams this PR defines
msk-direct-broker-endpoint(registered inomnibase_core.validation.validator_url_authority's rule table)omnibase_core/validation/validator_url_authority.py— new_is_msk_scannable()basename-aware file selector (matches.env,.env.<profile>family,Dockerfile/Dockerfile.<variant>,.json,.tf, plus existing suffixes) and a corrected_is_test_path()that anchors on real test-path segments (exacttest/testspath component,test_/_testbasename affixes, exactconftest.py) instead of the prior"test" in loweredsubstring check that waiveddeploy/latest.yaml,docker/stability-test/**, andattestation.yaml.url_authority_baseline.json— anti-gaming growth-check inurl-authority-gate.ymlmade repo-agnostic (previously filtered torepo == "omnibase_core", so growth in any other repo's entries — e.g. omnibase_infra, this rule's actual target — was invisible).Known live violations — intentionally NOT grandfathered, and materially larger than "3" once infra's own gate runs
Three known live infra violations are specific to the new
msk-direct-broker-endpointrule this PR adds, and are not suppressed by this PR:docker/docker-compose.gateway.yml:51-52docker/gateway/beta-gateway-canary.yaml:35Correction (round-2 remediation, post-merge): running the merged validator (
478e205d6f) against the live, unmodifiedomnibase_infratree finds 15 NEW violations total, not 3 — verified live viaomnibase_infrajob92203743693(run30973871608,URL-AUTHORITY GATE FAILED: 15 NEW violation(s)) on the paired pin-bump companionomnibase_infra#2658. Breakdown by attribution (differential scan, pre-PR validatorf13db86c54vs merged validator478e205d6f):msk-direct-broker-endpointhits named above._is_test_path()tightening (correct behavior — they were false-negatives before):src/omnibase_infra/cli/infra_test/introspect.py:85(env-url-read),scripts/publish_test_failure_baseline.py:40(url-const-assignment),src/omnibase_infra/nodes/node_runtime_source_attestor_effect/handlers/handler_source_attestation.py:48(url-const-assignment).RULE_LOCALHOST_LITERALand related rules did not exist at infra's old pin8a53a063bdbut do at478e205d6f), independent of this PR's own rule-5 addition.When
omnibase_infrapin-bumps to a core release containing this rule, infra CI will wedge on all 15, not just the 3 named above, until either the violations are fixed or infra's ownurl_authority_baseline.jsonis separately extended to grandfather the pin-span deltas (a decision this PR does not make). That wedge is this ticket's acceptance criterion for the 3 MSK violations specifically — it is proof the rule catches what it was built to catch — but the other 12 are a distinct, larger scope that the original "3 violations" framing in this body understated. Tracked on OMN-15694/OMN-15534 (MSK) plus the general pin-bump follow-up onomnibase_infra#2658for the other 12.Test plan
test_validator_url_authority.pyat merge SHA, all green (-p no:randomly -q→119 passed, 3 warnings)mypy --strictclean on touched modulepre-commit run --all-filesclean on all 6 touched filesmsk-direct-broker-endpointviolations pre-fix, passes post-fix patterngh pr checks— 139/140 green, 1 skipping, at mergeSummary by CodeRabbit
New Features
Bug Fixes
Tests
Evidence-Ticket: OMN-15692
Evidence-Source: OCC#6083