Repository navigation
feat(governance): let a policy name the OTLP collector hosts - #23716
iamwhatever wants to merge 1 commit into
Conversation
UX Review (Opus 5.5) — ⏭️ skippedRevision |
Design Review (Opus 5.5) — ✅ PASSDesign-level review of 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 |
First Principles Review (Opus 5.5) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks. The base RFC Notes
What this change shipsInventory (7 items) — 5 justifiedIntent: a fleet admin allows only their own OTLP collector(s) for metrics export. This is an ADDITION.
[FIRST-PRINCIPLES-REVIEWED] c758dfe Description read[DESCRIPTION-READ] c2d68e4cf8d482011797abde217695f97729f57865255001010c601c7a5be8cd That sha256 names the PR description this verdict read. Recompute it with |
Opus 5.5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 6.1 Review — ✅ no blocking findingsGPT 6.1 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
92f89ce to
d442ce4
Compare
|
Fixed in d442ce4: both GPT and Opus BLOCKING findings on |
Sensitive Change ReviewSensitive files changed:
|
bolichen97
left a comment
There was a problem hiding this comment.
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_hostsagainstrequestsURL preparation for\@,%5C@, a tab, a space, a double@, and[::1]\@. In each case the host thatrequestsconnects to is one of the returned readings, or the URL cannot be sent. - An IDNA host is not closed.
straße.examplereturns only the readingstraße.example._match_hostcasefolds it tostrasse.example, so an allow list forstrasse.examplepermits it.requestssends it toxn--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.
d442ce4 to
cae19fa
Compare
Sensitive change reviewHead: cae19fa
|
|
Round 2, fixed in cae19fa.
|
bolichen97
left a comment
There was a problem hiding this comment.
Thanks for the update. Re-reviewed at cae19fa.
Fixed since my last review:
- IDNA:
url_policy_hostsnow 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/v1now yields('straße.example', 'strasse.example', '', 'xn--strae-oqa.example'), so an allow list ofstrasse.examplerefuses it, andrequestswould dialxn--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_governancedocstring 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):
- The new percent-decoded reading decodes the whole netloc, userinfo included.
https://tok%23en@otel.corp.example/v1/metricsreads as('otel.corp.example', 'tok'), so an allow-listed collector is refused, and the WARNING and the SEL item nametok, a piece of the credential. Decode only the host part (netloc.rpartition("@")[2]) before reading it. _policy_ceilingtreats a policy that failed to load as no policy. On a standalone host whose policy file is unreadable,current_context()raisesPlatformCompositionError, the fallback provider isDefaultTelemetryProviderwith nogovernance_ceiling, so it returnsNoneand 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.
cae19fa to
6c34dd6
Compare
bolichen97
left a comment
There was a problem hiding this comment.
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 denyxn--bcher-kva.exampleand deny*.examplerefuse them and allowotel.examplerefuses them;[fe80::1%25eth0]reads asfe80::1; full-form and uppercase IPv6 URLs read in canonical form;otel.example\@evil.exampleis 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.
ca41092 to
bbfb7d0
Compare
Sensitive change reviewHead: bbfb7d0
|
|
@bolichen97 fixed in bbfb7d0, rebased on main (identical
|
bolichen97
left a comment
There was a problem hiding this comment.
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:4318and2001:DB8:0:0:0:0:0:1now 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 denyxn--bcher-kva.example, denyevil.exampleand deny*.examplerefuse them; percent-encoded hosts, an encoded@in userinfo, a root dot andotel.example\@evil.exampleare refused by denyevil.exampleand by allowotel.example;[fe80::1%25eth0]reads asfe80::1;167772165,0x0a000005and the full-form IPv6 URL read in canonical form; allowstrasse.examplestill refusesstraße.example. test/test_governance_list_otlp_hosts.pyandtest/metrics/test_provider_otlp_destinations.pypass 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.
Sensitive change reviewHead: f38760c
|
bbfb7d0 to
f38760c
Compare
bolichen97
left a comment
There was a problem hiding this comment.
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.and2001: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::*and2001: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.pyandtest/metrics/test_provider_otlp_destinations.pypass locally.
Still blocking (inline):
archetypes.py:413: the0:0test is a substring test on the whole glob, so a canonical IPv6 glob whose group ends in 0 before a0group fails to load:fd00:0:1::*,2010:0:1::*,2001:db80:0:1::*. The exact entry2010:0:1::5loads.archetypes.py:419: the leading-zero test runs on every label, so an ordinary DNS glob such as01-collector*.corp.examplefails to load while the exact01-collector1.corp.exampleloads. The GPT lane found the same.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 deny1677721*,http://167772165:4318reads as10.0.0.5and 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.
f38760c to
87e14b4
Compare
Sensitive change reviewHead: 87e14b4
|
|
Fixed the GPT/Opus BLOCK on the previous head ( |
bolichen97
left a comment
There was a problem hiding this comment.
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::*and2001:db8:10:0:*now load, and the first denies[fd00:0:1::5].01-collector*.corp.example,*.0x.org,*.007agent.exampleand*_svc.corp.examplenow load; the first denies01-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.exampleload.test/test_governance_list_otlp_hosts.pypasses locally (88 tests).
Two remaining blockers, both in the new IPv4 branch of _unmatchable_glob:
- A single-label glob of digits and wildcards is refused as an integer IP glob, but
fnmatchlets*cross dots.*,?,10*,1*and*1all 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 refuse10*. - The
0xcheck now runs only on a single-label glob.0x0a.0.0.*,0xa.*and10.0x0.*load again, and as deny entries they refuse nothing:https://0x0a.0.0.5/v1reads as10.0.0.5and 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.
Sensitive change reviewHead: c758dfe
|
87e14b4 to
c758dfe
Compare
Sensitive change reviewHead: c758dfe
|
bolichen97
left a comment
There was a problem hiding this comment.
Re-review at c758dfe (round 12). The two blocking findings from the last round are resolved.
- Single-label globs such as
*and10*were refused at load. Fixed: the numeric shape checks are gone, and*,10*and*:1:0:0:1are now intest_a_matchable_entry_loads. 0:0rule 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 as10.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 "" |
There was a problem hiding this comment.
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.
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.telemetryis an on/off gate with no list (RFC crew-enterprise-edition, appendix row "Diagnostic telemetry target"telemetry.otlp_hosts, markedno; "Optional telemetry" markedpartial). 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
_query_level: with no policy, or a policy without the list, behaviour is unchanged. Enterprise deny-by-default comes from the baseline item.telemetry.enabled) or to the beacon.What changed (motivation → approach → change)
Goal → reuse the existing CapabilityGate inner-ruleset shape (
capabilities.mobile_connectmethods,capabilities.publishdestinations) → one catalog row change plus two call sites.security_posture.py:metrics/provider.pylisted inNON_EGRESS_REDACTION_MODULES(it selects the redaction the SEL row uses and calls none).governance_parts/catalog.py:capabilities.telemetrygainsscope_matchers={"otlp_hosts": "host"}. Queried asotlp_hosts:<host>.metrics/otlp_governance.py:governed_destinationsdrops a destination whose host the policy does not permit before its reader is built, and writes one SELgovernance_decisionrow per destination (tooltelemetry_otlp_export, layerpolicy).gate_exporterwraps each OTLP exporter'sexport, 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.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.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.bootstrap.compose_telemetry_providerwraps the companion provider in_PolicyBoundTelemetry, carrying the companion's ceiling (governance_ceiling) and credential redaction (governance_redact).SecurityEventLog.log_governance_decisiongains an optionalredact, becauseredact_via_contextraises in a process with no context.governance.md(telemetry section) andmetrics.md.Call sites wired:
metrics/provider.py::_otlp_destinations(reader build),metrics/otlp_governance.py::gate_exporterinstalled inmetrics/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.telemetryoff 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 thehostmatcher; 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 gainexport; the gauges test reads the wrapped provider.test/test_sel_bindings.py: signature pin forlog_governance_decisionupdated for the newredactkeyword.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
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement
Contributions are accepted under the project's Apache-2.0 license.