Skip to content

feat(governance): let a policy name the OTLP collector hosts - #23716

Open
iamwhatever wants to merge 1 commit into
mainfrom
feat/ee-list-otlp-hosts
Open

iamwhatever wants to merge 1 commit into
mainfrom
feat/ee-list-otlp-hosts

Conversation

@iamwhatever

@iamwhatever iamwhatever commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Problem / Motivation

Goal: A security policy can name the exact OTLP collector hosts metrics may be exported to, and export to any other host stops.

capabilities.telemetry is an on/off gate with no list (RFC crew-enterprise-edition, appendix row "Diagnostic telemetry target" telemetry.otlp_hosts, marked no; "Optional telemetry" marked partial). A fleet can turn the beacon off but cannot say "metrics may go to our collector and nowhere else".

Why it matters

An enterprise fleet needs to allow its own collector while refusing every other one. Without a list, the only lever is all-or-nothing, and the deny-by-default baseline (a separate item) has nothing to narrow to.

Not a goal

  • No enterprise-only branch and no change to _query_level: with no policy, or a policy without the list, behaviour is unchanged. Enterprise deny-by-default comes from the baseline item.
  • No change to local metrics collection (telemetry.enabled) or to the beacon.
  • The other three partial controls (feature content hosts, share sites, decision lists) are separate PRs.

What changed (motivation → approach → change)

Goal → reuse the existing CapabilityGate inner-ruleset shape (capabilities.mobile_connect methods, capabilities.publish destinations) → one catalog row change plus two call sites.

  • security_posture.py: metrics/provider.py listed in NON_EGRESS_REDACTION_MODULES (it selects the redaction the SEL row uses and calls none).
  • governance_parts/catalog.py: capabilities.telemetry gains scope_matchers={"otlp_hosts": "host"}. Queried as otlp_hosts:<host>.
  • New metrics/otlp_governance.py:
    • governed_destinations drops a destination whose host the policy does not permit before its reader is built, and writes one SEL governance_decision row per destination (tool telemetry_otlp_export, layer policy).
    • gate_exporter wraps each OTLP exporter's export, so in a process with a platform context a policy installed at runtime (policy_distribution.apply_ceiling) stops the next export, the shutdown flush included, without a rebuild. A change of answer is audited once, not each interval. The MCP gateway daemon gets no runtime install and keeps its composed companion's ceiling until it restarts.
    • Every host a client could connect to is asked (governance_parts.matching.url_policy_hosts: the standard library plain and host-percent-decoded, urllib3, yarl; non-ASCII in IDNA form), and a destination proceeds only if every reading is permitted. The same helper is added identically in feat(governance): let a policy name the feature content hosts #23717 and feat(governance): let a policy name decision models, points and endpoints #23719.
    • A process with no context whose provider carries no ceiling refuses every host (otlp_governance.UNEVALUABLE).
  • metrics/provider.py: the build path (_otlp_destinations) applies the filter. otlp_egress_active (the Privacy panel and the local-only enable gate) deliberately keeps the unfiltered set: a host refused today may be permitted later, so it still counts as egress.
  • Policy layer only, as the beacon probe: the build is process-wide with no session.
  • MCP gateway daemon (no platform context): bootstrap.compose_telemetry_provider wraps the companion provider in _PolicyBoundTelemetry, carrying the companion's ceiling (governance_ceiling) and credential redaction (governance_redact). SecurityEventLog.log_governance_decision gains an optional redact, because redact_via_context raises in a process with no context.
  • Docs: governance.md (telemetry section) and metrics.md.

Call sites wired: metrics/provider.py::_otlp_destinations (reader build), metrics/otlp_governance.py::gate_exporter installed in metrics/provider.py::_build_otlp_reader (every export).

Backwards compatibility

Compatible: with no policy, or a policy that does not name otlp_hosts, every destination is kept and every export proceeds, as on main.

One deliberate tightening: an item query checks the gate first, so a policy that already pins capabilities.telemetry off now also stops OTLP metrics export, not only the beacon. That is the RFC's "Optional telemetry" control.

Reader: src/kiro_crew/metrics/provider.py:otlp_egress_active -- chat -- unchanged unfiltered set; test_the_enable_gate_still_reports_egress_for_a_refused_host
Reader: src/kiro_crew/mcp_gateway/gatewayd.py:_compose_and_apply -- app -- wrapper delegates every provider attribute; test_compose_telemetry_provider_applies_the_boot_security_floor

Tests

  • test/test_governance_list_otlp_hosts.py: the key parses with the host matcher; a list allowing X permits X and denies Y; gate off denies every host; no policy or no list permits; a host glob binds subdomains. Call sites: only listed hosts get a reader and each decision is audited; gate off builds no reader; no policy keeps every destination; the enable gate still reports egress for a refused host; a context-less process uses the companion ceiling and still writes its SEL row; a running exporter stops when a new policy denies its host.
  • test/metrics/*: stubs gain export; the gauges test reads the wrapped provider.
  • test/test_sel_bindings.py: signature pin for log_governance_decision updated for the new redact keyword.

Proof: not provable -- other: new feature; the tests assert behaviour that main does not have

Manual verification

N/A — unit coverage drives the real evaluator, the provider build path and the exporter wrapper.

Related Issues

Refs #23206. RFC: #23208.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

Contributions are accepted under the project's Apache-2.0 license.

@iamwhatever
iamwhatever requested review from a team and KiroCrewCaptain as code owners October 24, 2026 09:31
@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

UX Review (Opus 5.5) — ⏭️ skipped

Revision c758dfef7f757cda8ae7d733cb8cea3048f3b909 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Does not block merge.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Oct 24, 2026
@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Design Review (Opus 5.5) — ✅ PASS

Design-level review of c758dfef7f757cda8ae7d733cb8cea3048f3b909 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

Design-Verdict: PASS

Nothing blocks: the change reuses the existing CapabilityGate inner-ruleset shape, and the one tightening (gate off now also stops OTLP export) is listed and accepted.

Notes

[DESIGN-REVIEWED] c758dfe

Description read

[DESCRIPTION-READ] 7930f692cf994ae1b76e31d45fad4fa5a4bb1dac1f08fe019c12c2819d423616

That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

First Principles Review (Opus 5.5) — ✅ PASS

Premise-level review of c758dfef7f757cda8ae7d733cb8cea3048f3b909 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

First-Principles-Verdict: PASS

Nothing blocks. The base RFC crew-enterprise-edition is accepted and covers this list. The URL host-reading helper and the policy-load validator go further than an exporter with a single client needs.

Notes

  • Item 4 — oversized: the exporter connects through requests/urllib3, but url_policy_hosts also asks yarl and the backslash-to-path reading (_add(lambda: yarl.URL(text).raw_host)). It is also a second way of getting a host next to matching._url_host, which network.egress already uses. Subtraction: shrink it to the standard-library and urllib3 readings, and drop the yarl and backslash branches.
  • Item 5 — undeclared, one consumer, generalized: _refuse_unmatchable_host_entries runs for "a capability's host lists". Grepping scope_matchers=.*"host" in catalog.py gives 1 match, otlp_hosts. It refuses the whole policy at load, and the description never mentions it. Subtraction: delete _refuse_unmatchable_host_entries and _unmatchable_glob, and leave the documented rule that an unmatched allow entry refuses.

What this change ships

Inventory (7 items) — 5 justified

Intent: a fleet admin allows only their own OTLP collector(s) for metrics export. This is an ADDITION.

  1. A policy can list the OTLP hosts metrics may go to (capabilities.telemetry.otlp_hosts) — justified
  2. A policy with telemetry pinned off now also stops OTLP export — justified
  3. A policy installed at runtime stops the next export of a reader already running — justified
  4. Each endpoint's host is checked under every client's reading of the URL — oversized
  5. A policy whose capability host entry can never match fails to load — undeclared, one consumer, generalized
  6. The MCP gateway daemon enforces the companion's ceiling and writes its SEL rows with the companion's redaction — justified
  7. SEL audit rows record each build decision and each change in a running exporter's answer — justified

[FIRST-PRINCIPLES-REVIEWED] c758dfe

Description read

[DESCRIPTION-READ] c2d68e4cf8d482011797abde217695f97729f57865255001010c601c7a5be8cd

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 24, 2026
@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Opus 5.5 Review — ✅ no blocking findings

Reviewed c758dfef7f757cda8ae7d733cb8cea3048f3b909 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] c758dfe

Verdict parsed from the review's SHA-scoped output markers for commit c758dfef7f757cda8ae7d733cb8cea3048f3b909.

False positive or not applicable? A repository writer can comment:
/ai-review override fable c758dfef7f757cda8ae7d733cb8cea3048f3b909 false-positive: <reason, 20 to 500 characters>

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

GPT 6.1 Review — ✅ no blocking findings

GPT 6.1 completed its review of c758dfef7f757cda8ae7d733cb8cea3048f3b909 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c758dfe

False positive or not applicable? A repository writer can comment:
/ai-review override gpt c758dfef7f757cda8ae7d733cb8cea3048f3b909 false-positive: <reason, 20 to 500 characters>

@iamwhatever
iamwhatever force-pushed the feat/ee-list-otlp-hosts branch from 92f89ce to d442ce4 Compare October 24, 2026 09:58
@iamwhatever

Copy link
Copy Markdown
Contributor Author

Fixed in d442ce4: both GPT and Opus BLOCKING findings on otlp_governance.py host parsing (backslash in the authority read differently by urlsplit and urllib3). The class is "the host a policy is asked about differs from the host the transport connects to". Every reading is now asked (governance_parts.matching.url_policy_hosts: the urlsplit reading, the reading with \\ as a path separator, NFKC-folded, root dot removed), and a destination is kept, or an export proceeds, only if every reading is permitted. Tests: TestEndpointReadings in test/test_governance_list_otlp_hosts.py. The same class is fixed in #23717 and #23719. Static Ratchet: metrics/provider.py is listed in NON_EGRESS_REDACTION_MODULES with its reason (it selects a redaction for the SEL row and calls none).

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Sensitive Change Review

Sensitive files changed:

  • src/kiro_crew/security_posture.py

  • @iamwhatever posted a complete reasoning template for c758dfe.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 24, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ready. The backslash fix holds, but one host reading is still missing.

Head reviewed: d442ce4. Bots read: Design PASS, First Principles PASS, UX skipped, Opus BLOCK, GPT BLOCK, readiness checking.

What I verified on this head:

  • The backslash authority case is closed. I ran url_policy_hosts against requests URL preparation for \@, %5C@, a tab, a space, a double @, and [::1]\@. In each case the host that requests connects to is one of the returned readings, or the URL cannot be sent.
  • An IDNA host is not closed. straße.example returns only the reading straße.example. _match_host casefolds it to strasse.example, so an allow list for strasse.example permits it. requests sends it to xn--strae-oqa.example. This confirms the GPT lane finding. See the inline comment.

🔴 1 must-fix · 🟡 1 should-fix.

Not repeated: the Sensitive Change Review template for security_posture.py is the author's step and the check names it.

This change adds a governance control under an RFC that is still a draft. After the fixes, it needs a design sign-off from a maintainer before merge.

Comment thread src/kiro_crew/platform/governance_parts/matching.py Outdated
Comment thread src/kiro_crew/platform/bootstrap.py
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 24, 2026
@iamwhatever
iamwhatever force-pushed the feat/ee-list-otlp-hosts branch from d442ce4 to cae19fa Compare October 24, 2026 10:41
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 24, 2026
@iamwhatever

Copy link
Copy Markdown
Contributor Author

Sensitive change review

Head: cae19fa

  • What rule changed: one entry added to NON_EGRESS_REDACTION_MODULES in src/kiro_crew/security_posture.py: metrics/provider.py, with its reason. No redactor, mask, pattern or sink row changed.
  • Before -> after: before, test_every_redactor_call_site_is_a_registered_sink_or_allowlisted failed because the new _policy_redactor() name matches the call-site regex; after, the module is classified as non-egress. It calls no redactor: it returns the companion's credentials.redact so the SEL governance_decision row (the registered sink) uses it in a process with no platform context.
  • Worst case if wrong: if metrics/provider.py later gains a real egress redaction call, the drift guard would not flag it, because the module is already allowlisted. The OTLP payload itself is not redacted by this module either before or after this PR.
  • Evidence checked: read every redact match in metrics/provider.py on this head: _policy_redactor (returns a callable, calls none) and the two redact=_policy_redactor() keyword arguments. No sandbox run applies; this change touches no sandbox or command rule.
  • No regression:
    • Tools still work: no tool, command or path rule is touched; test/test_security_posture.py passes locally on this head (74 passed with the OTLP list tests).
    • Backward compatible: the allowlist only grows by one module; every existing sink row and allowlist entry is unchanged.
    • Existing tests: test/test_security_posture.py, test/test_governance_list_otlp_hosts.py, test/metrics/, test/test_sel*.py passed locally on this head.
  • How to undo: remove the metrics/provider.py entry and its comment from NON_EGRESS_REDACTION_MODULES (the drift guard then fails until the module is classified another way).

@iamwhatever

Copy link
Copy Markdown
Contributor Author

Round 2, fixed in cae19fa.

  1. Opus BLOCKING (bootstrap.py:145, the MCP gateway daemon keeps its composed ceiling): the daemon installs no platform context and receives no runtime policy install, so the per-export gate there can only hold the ceiling its companion was composed with. I narrowed the claim instead of building a daemon-side policy refresh, which is new scope. otlp_governance.py and governance.md now say that a runtime install stops a running reader in a process with a platform context, and that the daemon keeps its composed ceiling until it restarts.
  2. GPT BLOCKING (matching.py:298, IDNA straße → xn--strae-oqa): this is the second round of the same class, "the host the policy is asked about is not the host the transport connects to". So I changed the approach rather than add a third spelling. url_policy_hosts now asks each client's own parser (the standard library, plain and percent-decoded, urllib3.parse_url and yarl.URL.raw_host), adds each reading IDNA-encoded, and adds "" (which an allow list refuses) for any reading that is still not a plain ASCII host. A request proceeds only if every reading is permitted. Test: test_a_host_the_transport_reads_differently_is_refused.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 24, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. Re-reviewed at cae19fa.

Fixed since my last review:

  • IDNA: url_policy_hosts now adds the IDNA-encoded reading, the urllib3 and yarl readings, and "" for any reading that is not a plain ASCII host. I reproduced it at this head: https://straße.example/v1 now yields ('straße.example', 'strasse.example', '', 'xn--strae-oqa.example'), so an allow list of strasse.example refuses it, and requests would dial xn--strae-oqa.example. Full-width letters, U+3002, percent-encoded hosts and the backslash case are all refused too.
  • The governance spec and the otlp_governance docstring now say the MCP gateway daemon keeps the ceiling it was composed with until restart. The PR description still says a runtime install stops the next export without that caveat; worth a one-line edit.

Still blocking, both confirmed locally at this head (they match the Opus and GPT lane findings):

  1. The new percent-decoded reading decodes the whole netloc, userinfo included. https://tok%23en@otel.corp.example/v1/metrics reads as ('otel.corp.example', 'tok'), so an allow-listed collector is refused, and the WARNING and the SEL item name tok, a piece of the credential. Decode only the host part (netloc.rpartition("@")[2]) before reading it.
  2. _policy_ceiling treats a policy that failed to load as no policy. On a standalone host whose policy file is unreadable, current_context() raises PlatformCompositionError, the fallback provider is DefaultTelemetryProvider with no governance_ceiling, so it returns None and every OTLP destination is kept. A failed load should deny OTLP egress rather than permit it.

The same url_policy_hosts hunk is in #23717 and #23719: they already carry the IDNA fix, but they also carry the userinfo decode in finding 1, so they need the same change.

Tests run at this head: test/test_governance_list_otlp_hosts.py, test/metrics/test_provider_otlp_destinations.py, test/metrics/test_mcp_gateway_gauges.py, test/test_sel_bindings.py: 159 passed. Backend test shards were still pending in CI when I looked.

This is a feat for a tier T4 issue and touches security_posture.py, so the design call stays with a maintainer whatever the code verdict.

Comment thread src/kiro_crew/platform/governance_parts/matching.py Outdated
Comment thread src/kiro_crew/metrics/provider.py Outdated
@iamwhatever
iamwhatever force-pushed the feat/ee-list-otlp-hosts branch from cae19fa to 6c34dd6 Compare October 24, 2026 11:37
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 24, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix in ca41092. I re-ran the earlier findings against policies built with parse_policy and decided through _decide(ceiling, hosts_of(url)) on this head.

Fixed (verified both allow and deny direction):

  • Round 7: a Unicode deny entry (bücher.example, bü*.example, uppercase, NFD, fullwidth) and a full-form or alternate IP entry (2001:db8:0:0:0:0:0:1, ::ffff:10.0.0.5, 0x0a000005, 167772165, 127.1, 010.0.0.5) and a root-dot entry now fail the policy load with a message naming the form to write.
  • Earlier rounds still hold: Unicode, NFD, fullwidth, ideographic full stop, percent-encoded and userinfo URLs all read as the xn-- / ASCII host, so deny xn--bcher-kva.example and deny *.example refuse them and allow otel.example refuses them; [fe80::1%25eth0] reads as fe80::1; full-form and uppercase IPv6 URLs read in canonical form; otel.example\@evil.example is refused by both lists.
  • The new file passes locally (46 passed).

One remaining finding of the same class, inline. The validator still lets through entries that no reading can produce, so the docs line "any other entry fails the policy load" is not true yet. This is a feature on a tier:T4 issue that touches security_posture.py, so once the code is clean the design call stays with a maintainer.

Comment thread src/kiro_crew/platform/governance_parts/archetypes.py Outdated
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 24, 2026
@iamwhatever
iamwhatever force-pushed the feat/ee-list-otlp-hosts branch from ca41092 to bbfb7d0 Compare October 24, 2026 22:14
@iamwhatever

Copy link
Copy Markdown
Contributor Author

Sensitive change review

Head: bbfb7d0

  • What rule changed: one entry added to NON_EGRESS_REDACTION_MODULES in src/kiro_crew/security_posture.py: metrics/provider.py, with its reason. No redactor, mask, pattern or sink row changed.
  • Before -> after: before, test_every_redactor_call_site_is_a_registered_sink_or_allowlisted failed because the new _policy_redactor() name matches the call-site regex; after, the module is classified as non-egress. It calls no redactor: it returns the companion's credentials.redact so the SEL governance_decision row (the registered sink) uses it in a process with no platform context.
  • Worst case if wrong: if metrics/provider.py later gains a real egress redaction call, the drift guard would not flag it, because the module is already allowlisted. The OTLP payload itself is not redacted by this module either before or after this PR.
  • Evidence checked: read every redact match in metrics/provider.py on this head: _policy_redactor (returns a callable, calls none) and the two redact=_policy_redactor() keyword arguments. No sandbox run applies; this change touches no sandbox or command rule.
  • No regression:
    • Tools still work: no tool, command or path rule is touched; test/test_security_posture.py passes locally on this head (1448 passed with the OTLP list and metrics tests).
    • Backward compatible: the allowlist only grows by one module; every existing sink row and allowlist entry is unchanged.
    • Existing tests: test/test_security_posture.py, test/test_governance_list_otlp_hosts.py, test/metrics/, test/test_sel*.py passed locally on this head.
  • How to undo: remove the metrics/provider.py entry and its comment from NON_EGRESS_REDACTION_MODULES (the drift guard then fails until the module is classified another way).

@iamwhatever

Copy link
Copy Markdown
Contributor Author

@bolichen97 fixed in bbfb7d0, rebased on main (identical archetypes.py in #23717 37c5351 and #23719 8bd4a89). I took your suggestion: a non-glob entry must be exactly a form url_policy_hosts asks, or the policy fails to load.

  • An IP entry must equal its canonical, zone-free form, so fe80::1%eth0 is refused and fe80::1 is the form to write.
  • A bracketed IPv6 entry ([2001:db8::1]) is refused. A bracket is otherwise read as an fnmatch class.
  • A name with a port, %, / or @ (ev%69l.example, host:4318) is refused.
  • Non-ASCII, root-dot and non-canonical IP entries stay refused as before.
    test_the_policy_fails_to_load covers all of these in allow and in deny mode.
    Two more notes:
  • Your inline note on bootstrap.py:145 (the daemon keeps its startup ceiling): otlp_governance.py and governance.md say the daemon binds at its next start. I took the narrowing option rather than add a daemon-side refresh.
  • The rebase is for main's feat: ignore local enables with no governance key in enterprise builds #23703 _edition_locked_view, which answers () for a None config. The tests now pass a real config object (_CFG) instead of None.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Oct 24, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bbfb7d0. It applies the round 8 suggestion, and I re-ran every earlier finding against policies built with parse_policy and decided through _decide(ceiling, hosts_of(url)) on this head.

Fixed (verified in both allow and deny direction):

  • Round 8: deny fe80::1%eth0, [2001:db8::1], ev%69l.example, otel.corp.example:4318 and 2001:DB8:0:0:0:0:0:1 now fail the policy load.
  • Earlier rounds still hold: Unicode, uppercase Unicode, NFD, fullwidth and ideographic full stop entries fail the load; the same spellings in a URL read as the xn-- or ASCII host, so deny xn--bcher-kva.example, deny evil.example and deny *.example refuse them; percent-encoded hosts, an encoded @ in userinfo, a root dot and otel.example\@evil.example are refused by deny evil.example and by allow otel.example; [fe80::1%25eth0] reads as fe80::1; 167772165, 0x0a000005 and the full-form IPv6 URL read in canonical form; allow strasse.example still refuses straße.example.
  • test/test_governance_list_otlp_hosts.py and test/metrics/test_provider_otlp_destinations.py pass locally (73 passed). CI on this head is green except one backend shard still running.

One remaining finding of the same class, inline: glob entries skip every form check, so a deny glob in a spelling no reading produces still loads and refuses nothing. The docs line "any other entry fails the policy load" is therefore not true yet.

Suggested rule, so each new spelling cannot reopen the hole: canonicalize every policy entry at load (globs included, literal parts canonicalized) and refuse any entry whose canonical form differs from what was written.

This is a feature on a tier:T4 issue that touches security_posture.py, so once the code is clean the design call stays with a maintainer.

Comment thread src/kiro_crew/platform/governance_parts/archetypes.py Outdated
@iamwhatever

iamwhatever commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Sensitive change review

Head: f38760c

  • What rule changed: one entry added to NON_EGRESS_REDACTION_MODULES in src/kiro_crew/security_posture.py: metrics/provider.py, with its reason. No redactor, mask, pattern or sink row changed.
  • Before -> after: before, test_every_redactor_call_site_is_a_registered_sink_or_allowlisted failed because the new _policy_redactor() name matches the call-site regex; after, the module is classified as non-egress. It calls no redactor: it returns the companion's credentials.redact so the SEL governance_decision row (the registered sink) uses it in a process with no platform context.
  • Worst case if wrong: if metrics/provider.py later gains a real egress redaction call, the drift guard would not flag it, because the module is already allowlisted. The OTLP payload itself is not redacted by this module either before or after this PR.
  • Evidence checked: read every redact match in metrics/provider.py on this head: _policy_redactor (returns a callable, calls none) and the two redact=_policy_redactor() keyword arguments. No sandbox run applies; this change touches no sandbox or command rule.
  • No regression:
    • Tools still work: no tool, command or path rule is touched; test/test_security_posture.py passes locally on this head (1448 passed with the OTLP list and metrics tests).
    • Backward compatible: the allowlist only grows by one module; every existing sink row and allowlist entry is unchanged.
    • Existing tests: test/test_security_posture.py, test/test_governance_list_otlp_hosts.py, test/metrics/, test/test_sel*.py passed locally on this head.
  • How to undo: remove the metrics/provider.py entry and its comment from NON_EGRESS_REDACTION_MODULES (the drift guard then fails until the module is classified another way).

@iamwhatever
iamwhatever force-pushed the feat/ee-list-otlp-hosts branch from bbfb7d0 to f38760c Compare October 24, 2026 23:54
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 24, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the glob load check. Re-checked on f38760c with policies built by parse_policy and decided through _decide(hosts_of(url)).

Fixed and verified, both directions:

  • The deny globs from the last round are refused at load: ev%69l*, *.ev%69l.example, 010.0.0.*, 0x0a00000?, evil.example:*, fe80::1%eth*, *@evil.example, evil.example/*, 2001:db8:0:0:*, [ab]vil.example, plus *.evil.example. and 2001:0db8::*.
  • Every earlier red still holds: percent-encoded, userinfo, IDNA, zoned, bracketed, integer and hex readings are asked in canonical form, so evil.example, *.evil.example, xn--bcher-kva*, 10.0.0.*, fe80::* and 2001:db8::* deny what they name, and the matching allow entries admit the canonical host and refuse the userinfo and backslash tricks.
  • 95 tests in test/test_governance_list_otlp_hosts.py and test/metrics/test_provider_otlp_destinations.py pass locally.

Still blocking (inline):

  1. archetypes.py:413: the 0:0 test is a substring test on the whole glob, so a canonical IPv6 glob whose group ends in 0 before a 0 group fails to load: fd00:0:1::*, 2010:0:1::*, 2001:db80:0:1::*. The exact entry 2010:0:1::5 loads.
  2. archetypes.py:419: the leading-zero test runs on every label, so an ordinary DNS glob such as 01-collector*.corp.example fails to load while the exact 01-collector1.corp.example loads. The GPT lane found the same.
  3. archetypes.py:448: glob forms of two spellings the exact check already refuses still load and refuse nothing: integer IPv4 (1677721*, 16777216?) and IPv4-mapped IPv6 (::ffff:*, ::ffff:a00:*). With deny 1677721*, http://167772165:4318 reads as 10.0.0.5 and is permitted.

Items 1 and 2 make a valid policy abort boot with no glob that expresses the same intent. Item 3 is the same class as the last round: a deny that loads and silently refuses nothing. A rule tied to the canonical form closes all three: decide numeric checks per label only when the glob is IP shaped, test zero runs per group, and refuse an all-digit glob with no dot and any glob that starts with ::ffff:.

The linked issue is tier T4, so the design call on this capability stays with a maintainer either way.

Comment thread src/kiro_crew/platform/governance_parts/archetypes.py Outdated
Comment thread src/kiro_crew/platform/governance_parts/archetypes.py Outdated
Comment thread src/kiro_crew/platform/governance_parts/archetypes.py
@iamwhatever
iamwhatever force-pushed the feat/ee-list-otlp-hosts branch from f38760c to 87e14b4 Compare October 25, 2026 00:54
@iamwhatever

Copy link
Copy Markdown
Contributor Author

Sensitive change review

Head: 87e14b4

  • What rule changed: one entry added to NON_EGRESS_REDACTION_MODULES in src/kiro_crew/security_posture.py: metrics/provider.py, with its reason. No redactor, mask, pattern or sink row changed.
  • Before -> after: before, test_every_redactor_call_site_is_a_registered_sink_or_allowlisted failed because the new _policy_redactor() name matches the call-site regex; after, the module is classified as non-egress. It calls no redactor: it returns the companion's credentials.redact so the SEL governance_decision row (the registered sink) uses it in a process with no platform context.
  • Worst case if wrong: if metrics/provider.py later gains a real egress redaction call, the drift guard would not flag it, because the module is already allowlisted. The OTLP payload itself is not redacted by this module either before or after this PR.
  • Evidence checked: read every redact match in metrics/provider.py on this head: _policy_redactor (returns a callable, calls none) and the two redact=_policy_redactor() keyword arguments. No sandbox run applies; this change touches no sandbox or command rule.
  • No regression:
    • Tools still work: no tool, command or path rule is touched; test/test_security_posture.py passes locally on this head (1448 passed with the OTLP list and metrics tests).
    • Backward compatible: the allowlist only grows by one module; every existing sink row and allowlist entry is unchanged.
    • Existing tests: test/test_security_posture.py, test/test_governance_list_otlp_hosts.py, test/metrics/, test/test_sel*.py passed locally on this head.
  • How to undo: remove the metrics/provider.py entry and its comment from NON_EGRESS_REDACTION_MODULES (the drift guard then fails until the module is classified another way).

@iamwhatever

Copy link
Copy Markdown
Contributor Author

Fixed the GPT/Opus BLOCK on the previous head (_unmatchable_glob refused valid DNS globs with numbered labels, canonical IPv6 globs such as 2001:db8:10:0:*, and _). Numbers are now checked only in IP-shaped globs, and the zero-run test compares groups. Shared archetypes.py: #23716 87e14b4, #23717 05bd4d7, #23719 937a1ef.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 11 review of 87e14b4, delta from f38760c.

Fixed since the last round, reproduced with parse_policy and the real resolver (url_policy_hosts plus the export gate decision):

  • fd00:0:1::* and 2001:db8:10:0:* now load, and the first denies [fd00:0:1::5].
  • 01-collector*.corp.example, *.0x.org, *.007agent.example and *_svc.corp.example now load; the first denies 01-collector-a.corp.example.
  • 1677721*, ::ffff:* and ::ffff:a00:* now fail to load in both allow and deny.
  • Every earlier red still holds in both directions: Unicode and Unicode glob, root dot, full-form IPv6, octal IPv4 exact and glob, single-label hex glob, zoned exact and glob, bracketed, percent-encoded exact and glob, port, userinfo, path and [ ] class entries all fail to load. IDNA, userinfo percent-decode and zoned readings behave as fixed in earlier rounds.
  • Canonical 10.0.0.5, 10.0.0.*, 2001:db8::1, fe80::*, otel.corp.example, *.corp.example load. test/test_governance_list_otlp_hosts.py passes locally (88 tests).

Two remaining blockers, both in the new IPv4 branch of _unmatchable_glob:

  1. A single-label glob of digits and wildcards is refused as an integer IP glob, but fnmatch lets * cross dots. *, ?, 10*, 1* and *1 all match real readings, and each now makes the whole policy fail to load. The AI lanes flagged bare *; guarding only on "has a digit" would still refuse 10*.
  2. The 0x check now runs only on a single-label glob. 0x0a.0.0.*, 0xa.* and 10.0x0.* load again, and as deny entries they refuse nothing: https://0x0a.0.0.5/v1 reads as 10.0.0.5 and is permitted. f38760c refused all three.

Two smaller IPv6 notes are inline. The linked issue is tier:T4, so after the code is clean the design call stays with a maintainer.

Comment thread src/kiro_crew/platform/governance_parts/archetypes.py Outdated
Comment thread src/kiro_crew/platform/governance_parts/archetypes.py Outdated
Comment thread src/kiro_crew/platform/governance_parts/archetypes.py Outdated
Comment thread src/kiro_crew/platform/governance_parts/archetypes.py Outdated
@iamwhatever

iamwhatever commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Sensitive change review

Head: c758dfe

  • What rule changed: one entry added to NON_EGRESS_REDACTION_MODULES in src/kiro_crew/security_posture.py: metrics/provider.py, with its reason. No redactor, mask, pattern or sink row changed.
  • Before -> after: before, test_every_redactor_call_site_is_a_registered_sink_or_allowlisted failed because the new _policy_redactor() name matches the call-site regex; after, the module is classified as non-egress. It calls no redactor: it returns the companion's credentials.redact so the SEL governance_decision row (the registered sink) uses it in a process with no platform context.
  • Worst case if wrong: if metrics/provider.py later gains a real egress redaction call, the drift guard would not flag it, because the module is already allowlisted. The OTLP payload itself is not redacted by this module either before or after this PR.
  • Evidence checked: read every redact match in metrics/provider.py on this head: _policy_redactor (returns a callable, calls none) and the two redact=_policy_redactor() keyword arguments. No sandbox run applies; this change touches no sandbox or command rule.
  • No regression:
    • Tools still work: no tool, command or path rule is touched; test/test_security_posture.py passes locally on this head (1448 passed with the OTLP list and metrics tests).
    • Backward compatible: the allowlist only grows by one module; every existing sink row and allowlist entry is unchanged.
    • Existing tests: test/test_security_posture.py, test/test_governance_list_otlp_hosts.py, test/metrics/, test/test_sel*.py passed locally on this head.
  • How to undo: remove the metrics/provider.py entry and its comment from NON_EGRESS_REDACTION_MODULES (the drift guard then fails until the module is classified another way).

@iamwhatever
iamwhatever force-pushed the feat/ee-list-otlp-hosts branch from 87e14b4 to c758dfe Compare October 25, 2026 01:32
@iamwhatever

iamwhatever commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Fixed the GPT/Opus BLOCK on 87e14b4 (*, ?, ** were refused as integer IP globs) by narrowing: glob entries get a character check only, and the numeric-spelling rules for globs are removed (gap recorded in #23734). Shared archetypes.py: #23716 c758dfe, #23717 d798145, #23719 d0b65c0.

@iamwhatever

Copy link
Copy Markdown
Contributor Author

Sensitive change review

Head: c758dfe

  • What rule changed: one entry added to NON_EGRESS_REDACTION_MODULES in src/kiro_crew/security_posture.py: metrics/provider.py, with its reason. No redactor, mask, pattern or sink row changed.
  • Before -> after: before, test_every_redactor_call_site_is_a_registered_sink_or_allowlisted failed because the new _policy_redactor() name matches the call-site regex; after, the module is classified as non-egress. It calls no redactor: it returns the companion's credentials.redact so the SEL governance_decision row (the registered sink) uses it in a process with no platform context.
  • Worst case if wrong: if metrics/provider.py later gains a real egress redaction call, the drift guard would not flag it, because the module is already allowlisted. The OTLP payload itself is not redacted by this module either before or after this PR.
  • Evidence checked: read every redact match in metrics/provider.py on this head: _policy_redactor (returns a callable, calls none) and the two redact=_policy_redactor() keyword arguments. No sandbox run applies; this change touches no sandbox or command rule.
  • No regression:
    • Tools still work: no tool, command or path rule is touched; test/test_security_posture.py passes locally on this head (1448 passed with the OTLP list and metrics tests).
    • Backward compatible: the allowlist only grows by one module; every existing sink row and allowlist entry is unchanged.
    • Existing tests: test/test_security_posture.py, test/test_governance_list_otlp_hosts.py, test/metrics/, test/test_sel*.py passed locally on this head.
  • How to undo: remove the metrics/provider.py entry and its comment from NON_EGRESS_REDACTION_MODULES (the drift guard then fails until the module is classified another way).

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at c758dfe (round 12). The two blocking findings from the last round are resolved.

  • Single-label globs such as * and 10* were refused at load. Fixed: the numeric shape checks are gone, and *, 10* and *:1:0:0:1 are now in test_a_matchable_entry_loads.
  • 0:0 rule refused *:1:0:0:1. Fixed by the same removal.
  • Hex and octal IP globs (0x0a.0.0.*, 010.0.0.*) load and match nothing. Not fixed in code, but now stated in the spec and tracked in #23734. An allow list stays fail closed (the dead entry admits nothing); only a deny list loses those entries, and the spec already says a deny list is not the hard bound. I accept this as a deferral.
  • *:ffff:* loads. Same deferral. Note that ::ffff:* and ::ffff:a00:*, refused last round, now load too: readings unmap IPv4-mapped addresses ([::ffff:10.0.0.1] is asked as 10.0.0.1), so these globs can never match.

Verified locally at this head: test/test_governance_list_otlp_hosts.py passes (79 tests), and I probed _unmatchable_glob and url_policy_hosts directly for the spellings above. Exact entries already get the canonicalize and compare check at load, so the remaining gap is globs only. One non-blocking suggestion inline.

This is a tier T4 change, so it still needs the human design sign-off; I am not approving.

return "is not an IPv6 glob in canonical form (hex groups only, no zone or port)"
if not re.fullmatch(r"[a-z0-9._:-]*", body):
return "has a character no host is asked with ('%', '@', '/' or similar)"
return ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: a narrow check for non-canonical IP globs, and a docstring that now overstates the guarantee.

Exact entries are canonicalized at load and refused when the canonical form differs from what was written. Globs have no single canonical form, which is why the shape rules kept refusing valid entries. A cheap middle ground that avoids those false positives: build a few sample instances of the glob (replace every * and ? with "", "0", "1"), and refuse only when at least one sample parses as an IP and every sample that parses is not in canonical form (compare against the same canonicalizer url_policy_hosts uses, so ::ffff:1 counts as 0.0.0.1).

    return ""

On the cases from this thread: 0x0a.0.0.*, 010.0.0.* and ::ffff:* are refused; *, 10*, *0.0.0.1, 0*.0.0.1 and *:1:0:0:1 still load. 1677721* and 2001:db8:0:0:* would still slip through, so #23734 stays open either way. Fine to leave for that issue.

Separately, the _refuse_unmatchable_host_entries docstring says an entry in another spelling "fails to load instead". That now holds for exact entries only; a line saying globs get a character check only would keep it accurate.

Rule: an entry check at load should not promise more than it enforces.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 25, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants