test(swim): cover impersonation, tampering, and key rotation for signed state broadcasts - #99
Merged
nerdalert merged 9 commits intoSep 1, 2026
Conversation
Signed-off-by: Revanth Reddy Airre <revanthreddy@hippocraticai.com> Co-authored-by: Brent Salisbury <bsalisbu@redhat.com>
…Site behavior (praxis-proxy#94) Single-site/combined deployments legitimately run a one-node SWIM mesh with zero peers, so the local GridSite stays Pending with reason AwaitingDiscovery. That is expected and does not block local routing: local InferenceProviders are eligible regardless of GridSite phase; only remote CRDT provider records are phase-gated. Add a 'Single-Site and Combined Deployments' section to the architecture overview and a single-site note to the GridSite lifecycle so single-cluster users do not mistake Pending for a discovery failure or try to add SWIM peers/replicas to fix it. Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
…raxis-proxy#95) determine_phase only returned Active via phase_hint(), which requires at least one Alive SWIM peer. A single-site / combined deployment legitimately has zero peers (peers are other sites, not intra-site gateways/pods), so its GridNetwork was pinned at Initializing forever even though the local control plane was fully operational and already rendering overlays for local providers. Treat a standalone network (no seeds configured) with the SWIM runtime up and TLS trust material as Active. Networks that DO configure seeds still stay Initializing until a peer is observed, so multi-site behavior is unchanged. Peer connectivity remains reported separately via status.connectedSites. Note: the oversized self-site SWIM broadcast is unrelated — it already soft-fails (logged and dropped in swim node + reconcile), so it never blocked the phase. Adds regression tests: determine_phase_standalone_single_site_reaches_active and determine_phase_seeded_but_peerless_stays_initializing. Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
…ion (grid#41) (praxis-proxy#56) * feat(operator): add MCP tools/list probe client with bounded telemetry (grid#41) Adds the live-probe half of the AgentToolProvider reconciler: an rmcp-based Streamable HTTP client that calls tools/list against spec.endpoint, with SSRF protection (blocks loopback/link-local/cloud-metadata targets), TLS material resolution, and bearer-token auth from a Secret. Also wires grid_mcp_probe_total{outcome} and grid_mcp_probe_duration_seconds metrics, following grid#9's bounded-cardinality convention: TlsConfigInvalid's carried reason string is deliberately collapsed to a single label so metric cardinality stays fixed regardless of cluster misconfiguration variety. Signed-off-by: Jordi Gil <jgil@redhat.com> * feat(operator): add AgentToolProvider reconciler (grid#41) Wires the AgentToolProvider CRD to the live MCP probe client added in the previous commit: resolves siteSelector matches against GridNetwork/GridSite, runs the probe, and maps its outcome to status.phase/status.reason/ status.discoveredTools. Mirrors InferenceProvider's reconciliation shape, including its documented cross-resource watch limitation as a follow-up. Data-plane MCP tool-catalog aggregation and cross-cluster tools/call routing (praxis-ai#155, #205, #173) are out of scope here — this is the Grid-side control plane only, per grid#41's stated scope. Signed-off-by: Jordi Gil <jgil@redhat.com> * feat(charts,mock-providers,xtask): AgentToolProvider Helm wiring, mock MCP server, and E2E check (grid#41) - charts/grid-operator: install the AgentToolProvider CRD and grant its RBAC verbs, with helm-unittest coverage for the new ClusterRole rule; wires charts/grid-operator into the CI helm-unittest job (only charts/grid-site ran before). - mock-providers: new --mcp-server mode built on the real rmcp server SDK (Streamable HTTP), so the E2E check below probes actual MCP wire protocol behavior, not a hand-rolled JSON-RPC approximation. Disables rmcp's default DNS-rebinding Host-header allowlist (localhost/127.0.0.1/::1 only), since this mock is reached over its in-cluster Service DNS name or NodePort address, never loopback. - xtask: cargo xtask env verify-agenttoolprovider-convergence deploys the mock as a real in-cluster NodePort service (the operator under test runs out-of-cluster, so it can't resolve in-cluster .svc DNS names) and proves Pending -> Available with discoveredTools populated end-to-end, plus the unreachable-endpoint failure path landing on Unavailable with a reason. Verified twice against a real kind cluster. Signed-off-by: Jordi Gil <jgil@redhat.com> * test(operator): close AgentToolProvider TLS/Secret and GridNetwork test gaps (grid#41) Backfills the coverage gaps found during grid#56's GA readiness audit: - attach_tls_ca/attach_tls_client_identity/read_tls_material now have real unit-test coverage against a mocked kube::Client (tower::service_fn), which the module's own doc comment had falsely claimed already existed. - Along the way, found and fixed a real bug: reqwest::Certificate::from_pem and reqwest::Identity::from_pem don't reject malformed/empty PEM input, unlike the rustls::pki_types-based validation InferenceProvider's build_tls_client_config already uses. attach_tls_ca and attach_tls_client_identity now eagerly validate PEM material the same strict way before handing it to reqwest, so EndpointTlsMaterialInvalid/ EndpointTlsIdentityMismatch are reachable in practice, not just in name. - Also found (but did not fix here, to keep this change scoped) a pre-existing bug in the shared endpoint_tls.rs/secret.rs Secret-read path that also affects InferenceProvider: a key entirely absent from an existing Secret's data is misreported as SecretMissing instead of KeyMissing. Filed as grid#58, with a test that locks in current behavior and points at the issue. - static_config_failure_reason (GridNetworkNotFound/ProviderConfigInvalid) now has direct unit-test coverage via the same kube::Client mocking pattern, proving wrong an existing code comment that claimed this "cannot be unit-tested without a live cluster or a mock Kubernetes server." - Fixed doc drift: overview.md's claim that AgentToolProvider "does not currently run full controllers" (it does, as of grid#56), crds.md's Degraded phase (unreachable by design for this CRD, per phase_and_reason_from_probe's own doc comment) and missing reason/ observedGeneration status fields, and two stale "once PR 2 lands" code comments left over from the original 3-PR plan that landed as one. Signed-off-by: Jordi Gil <jgil@redhat.com> * fix(mock-providers): resolve private-intra-doc-link rustdoc error `router`'s doc comment linked to `FixedToolsServer`, a private struct -- the link only resolved locally because of --document-private-items and failed CI's -D rustdoc::private-intra-doc-links check. Drop the link, keep the type name as plain text with a note that it's module-private. Signed-off-by: Jordi Gil <jgil@redhat.com> * fix(operator): enforce one combined probe timeout, not per-phase budgets Addresses grid#56 review feedback (pull/56#pullrequestreview, praxis-bot): - PROBE_TIMEOUT was documented as a single combined budget for the live MCP probe, but resolve_endpoint_for_probe (DNS), the connect/handshake, and tools/list each independently got up to the full timeout, and TLS Secret material reads via the Kubernetes API had no timeout at all. Worst-case wall-clock time could exceed 3x the documented 10s budget, plus unbounded Kubernetes API latency. Fixed by wrapping the whole probe sequence in a single outer tokio::time::timeout in probe_agent_tool_provider, so all phases now share one real budget. - auth_header_map silently dropped the Authorization header when a bearer token contained characters invalid in an HTTP header value, with no log signal -- the resulting probe failure would look like an auth/response problem with no trace back to the real cause. Added a tracing::warn! on that branch. - validate_probe_url already blocks IPv6 link-local (fe80::/10) and unique-local (fd00::/8) addresses via is_ssrf_sensitive, but had no test coverage proving it (only the IPv4 link-local and IPv6 loopback cases were tested). Added the two missing regression tests. Signed-off-by: Jordi Gil <jgil@redhat.com> * test(operator): cover reconcile() end-to-end against a mocked kube::Client All existing agent_tool_provider tests exercise resolve_phase_and_sites's constituent resolve_*/pure-logic functions in isolation; none drove the public reconcile() entrypoint itself, so nothing proved the resolved (phase, reason, matchingSites, discoveredTools) tuple actually reaches the Kubernetes API as the status PATCH body a real controller sends. Add a PATCH-capturing mock kube::Client and two reconcile()-level tests covering the two code paths that need no live MCP probe: a config-invalid provider (fully short-circuited, no Kubernetes calls at all) and a provider whose gridNetworkRef doesn't resolve (exercises the live GridNetwork GET). Both mutation-tested by hand against the reason strings they assert on to confirm they fail on regression, not just pass by construction. Signed-off-by: Jordi Gil <jgil@redhat.com> * fix(operator): trust only the configured CA for AgentToolProvider probes Addresses grid#56 review feedback (nerdalert): attach_tls_ca used reqwest::ClientBuilder::add_root_certificate, which merges the configured CA into reqwest's platform trust store rather than replacing it -- reqwest's own docs mark this method deprecated in favor of tls_certs_merge()/tls_certs_only() for exactly this ambiguity. EndpointTlsConfig's doc comment promises the opposite: "the scraper trusts only this CA -- system root certificates are not consulted." With add_root_certificate, a publicly trusted certificate could still satisfy a probe explicitly configured to trust only a private CA. Switch to tls_certs_only([ca_cert]), which disables the platform trust store and uses only the supplied CA, matching the documented invariant and mirroring how metrics_scraper::build_tls_client_config already builds its rustls::RootCertStore from empty rather than merging into the platform's. Signed-off-by: Jordi Gil <jgil@redhat.com> * fix(operator): fail AgentToolProvider probe closed on unencodable auth token Addresses grid#56 review feedback (nerdalert): auth_header_map already logged a warning when a resolved spec.auth bearer token contained characters invalid in an HTTP header value (see c2b724e), but still returned an empty header map and let the probe proceed unauthenticated. An MCP endpoint that permits anonymous tools/list could then be marked Available without the configured credential ever being exercised -- the diagnosable warning masked a silent open-fails-open path. auth_header_map now returns Result<_, McpProbeOutcome>, mapping an unencodable token to the new AuthConfigInvalid outcome (status.reason McpAuthTokenInvalid) instead of an empty header map, and run_probe_session propagates that failure instead of sending an unauthenticated request. Regenerated the AgentToolProvider CRD manifests (deploy/, charts/, tests/e2e/) for the new documented reason. Signed-off-by: Jordi Gil <jgil@redhat.com> * fix(operator): bound and normalize discovered tool names before persisting Addresses grid#56 review feedback (nerdalert): Every tool name a probed MCP server returns was copied into status.discoveredTools with no count, name-length, or total-size limit, and in server-returned order. A server advertising an implausibly large or long-named tool catalog could grow the Kubernetes status object without bound; a server merely reordering an unchanged catalog would trigger an unnecessary status patch on every later reconcile. Add bound_and_normalize_discovered_tools: truncates each name to 256 bytes (at a UTF-8 char boundary), deduplicates and sorts (order is not semantically meaningful), then truncates the deduplicated list to 500 entries. Applied to every successful probe's tool list before it reaches McpProbeOutcome::Success. This bounds what this reconciler persists and holds in memory; it does not bound the raw HTTP response rmcp/reqwest buffer before parsing it into a tools/list result -- rmcp's JSON response path (unlike its SSE path, which honors max_sse_event_size) has no such cap in the version this workspace depends on. Tracked as a separate follow-up rather than folded into this fix, since closing it would mean bypassing rmcp's higher-level session API for a manual, byte-capped read. Signed-off-by: Jordi Gil <jgil@redhat.com> --------- Signed-off-by: Jordi Gil <jgil@redhat.com>
…casts (grid#75, partial) (praxis-proxy#97) * feat(swim): verify ECDSA P-256 signatures on pinned state broadcasts Adds sign/verify primitives (swim::signing, backed by ring), a signature field on the StateBroadcast wire extension, and a pinned-identity trust store gate in StateBroadcastHandler::receive_item: an origin with a pinned public key must supply a valid signature or the broadcast is rejected before merge. An origin with no pinned entry passes through unchanged, so pinning can roll out one site at a time instead of requiring a synchronized cutover. Deliberately decoupled from key sourcing. The trust store takes raw public key bytes from any caller, so it works whether grid#75 lands on reusing the site's mTLS certificate or a dedicated signing key -- that question is still open and tracked there, along with the CRD/secret/rotation plumbing needed to populate the trust store from live GridSite state. Signed-off-by: Jordi Gil <jgil@redhat.com> * fix(swim): log rejection reason for pinned-origin signature failures verify_signature_if_pinned() rejected unsigned, unverifiable, and invalid broadcasts with no operational signal at all, unlike the sibling make_room_for() eviction path a few lines below it. Once a trust store is populated, a forged or replayed broadcast for a pinned origin would be silently dropped with no way to tell "nothing is attacking us" from "something is being rejected and nobody can see it" -- exactly the gap nerdalert's grid#75 review asked to close with bounded rejection metrics that never log payload contents. Add a tracing::warn! on each of the three rejection branches, carrying only origin_site and a closed-set reason tag (missing_signature / signable_encode / signature_invalid), never the broadcast body. Signed-off-by: Jordi Gil <jgil@redhat.com> * test(swim): cover Display messages for signing and broadcast errors This codebase has a standing convention of testing thiserror Display output directly (node::tests::state_broadcast_error_formats_correctly asserts on .to_string()). The six error variants introduced for broadcast signing -- InvalidKey, SigningFailed, VerificationError:: Invalid, MissingSignature, SignatureInvalid, SignableEncode -- had no such test, which is exactly why cargo llvm-cov's function-coverage column looked worse than the underlying decision logic actually is. SigningFailed and SignableEncode are constructed directly rather than triggered through the real code path: their real trigger (an RNG failure inside ring's sign, or a bincode encode failure with no writer I/O) has no reachable path through this crate's public API, matching how this same file already treats PoisonError::into_inner recovery as an accepted, untriggerable defensive branch. Signed-off-by: Jordi Gil <jgil@redhat.com> * fix(swim): recover gateway/cert from pre-signature broadcasts during rolling update `BroadcastExtension` gained a third field (`signature`) in the prior commit on this branch. bincode is not self-describing, so decoding a two-field payload from a not-yet-upgraded peer against the current three-field struct fails partway through the missing field rather than falling back to `#[serde(default)]` -- the `?` inside serde's generated `visit_seq` propagates that error first. The existing bare-`String` fallback then silently misdecodes the raw bytes into garbage instead of recovering `gateway_address`/`site_cert_pem`, or erroring. Add a two-field `PreSignatureBroadcastExtension` fallback tier between the current struct decode and the bare-String fallback, so broadcasts from peers still running the pre-signature wire format decode correctly during a rolling update. Extract the extension fallback chain into `decode_extension` to keep `decode` under the line-count lint. Regression test encodes a payload using the exact two-field wire format and asserts both fields, and `signature: None`, survive decode. Signed-off-by: Jordi Gil <jgil@redhat.com> * fix(swim): bound signature replay window and scope signatures to their GridNetwork Addresses grid#97 review feedback (nerdalert) on anti-replay and domain separation, the two findings left partially open after the first pass (key rotation and trust-state migration were already closed). Anti-replay: add StateBroadcast.signed_at_ms (epoch millis), covered by signable_bytes() so a captured signature can't be re-attached to a forged timestamp. verify_signature_if_pinned() now rejects a pinned origin's broadcast whose timestamp is missing or falls outside [now - MAX_BROADCAST_AGE_MS, now + MAX_CLOCK_SKEW_AHEAD_MS] (5 minutes / 30 seconds), grounded in OWASP ASVS 12.3.5's replay-resistant PKI-auth requirement. This bounds the replay window; it does not eliminate replay, since nothing here is persisted across a process restart -- documented explicitly rather than overclaiming full compliance. Domain separation: add StateBroadcast.grid_id (Option<String>), also covered by signable_bytes(), so a signature valid for one GridNetwork cannot be replayed as valid for another sharing the same cluster's SwimHandle. Elevated to a required fix (not optional hardening) once issue praxis-proxy#48 confirmed that multiple GridNetworks per cluster is a supported, documented tenant/trust-domain isolation guarantee. Both new fields reuse the bincode-is-not-self-describing fallback pattern already established for `signature`: BroadcastExtension grows to five fields, with a PreTimestampBroadcastExtension tier preserving interop with peers running the prior three-field wire format. Also corrects two ASVS citations from the first audit pass that didn't hold up against the literal ASVS 5.0.0 control text: trust-state migration's V11.1.1 (about a documented key-management policy, not revalidating merged state) and domain separation's V11.2.1 (about vetted crypto libraries, not payload scoping) are both reframed as general engineering principles rather than dedicated control IDs. New tests: 4 freshness-window boundary checks, 3 receive_item timestamp rejection cases (missing/stale/future), signed_at_ms and grid_id wire round-trips plus a legacy-decode-tier case, 2 new error Display-message tests, and grid_id-specific signable-bytes/cross-grid-signature tests. Fixed node.rs's pin_origin_rotation_update_does_not_purge_already_authenticated_state to attach signed_at_ms, since it already signed a broadcast for a pinned origin. Signed-off-by: Jordi Gil <jgil@redhat.com> * fix(operator): thread GridNetwork identity into published state broadcasts Companion to the swim-crate domain-separation fix on this branch: publish_real_provider_state (grid_network controller) now passes its already-resolved resolve_grid_id(network) value into StateBroadcast::with_grid_id(), so this GridNetwork's signed state cannot be replayed as valid for a different GridNetwork sharing the same cluster's SwimHandle (issue praxis-proxy#48). publish_gateway_address_broadcast intentionally keeps grid_id: None, documented inline, since it can fire before a node has joined any GridNetwork. Signed-off-by: Jordi Gil <jgil@redhat.com> --------- Signed-off-by: Jordi Gil <jgil@redhat.com>
…tate broadcasts (grid#75) receive_item already rejected wrong-key signatures and accepted either key in a rotation window; add the missing tampering case -- a broadcast signed correctly, then edited before it reaches the handler (bumped revision, inflated tenant_spend). Both must fail signature verification and neither tampered payload should merge into the snapshot. These exercise the swim crate's existing sign/verify/TrustStore API only, so they hold regardless of how a real deployment ends up sourcing and pinning per-site signing keys (still open on grid#75/grid#92/grid#93). Signed-off-by: Jordi Gil <jgil@redhat.com>
jordigilh
force-pushed
the
test/swim-signing-impersonation-tampering-rotation
branch
from
August 26, 2026 14:33
eb7b87f to
7f87202
Compare
Two more scenarios for signed state broadcasts: a signature from a key that was dropped at the end of a rotation (not just outnumbered by a new one), and a malformed signature that isn't valid ECDSA at all -- both must be rejected cleanly, the latter without panicking. Signed-off-by: Jordi Gil <jgil@redhat.com>
Signed-off-by: Jordi Gil <jgil@redhat.com>
Member
|
@jordigilh looks good, will merge once you rebase. ty ty. |
…mpersonation-tampering-rotation Signed-off-by: Jordi Gil <jgil@redhat.com> # Conflicts: # .github/workflows/helm.yaml # .github/workflows/operator-image.yaml # Cargo.lock # Cargo.toml # swim/src/state_broadcast.rs # xtask/src/env/llmd_pool_metrics_demo.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the test coverage gap in #75: same-key impersonation, tampering, and key rotation for signed StateBroadcasts.
Impersonation (wrong key signs as the pinned origin) and key rotation (accept either key in a dual-pin window) were already covered by #97. The real gaps were tampering and a couple of edge cases:
Added four tests for that.
Trust-state migration (purging state merged before an origin was pinned, not purging on rotation/unpin) is already covered separately in node.rs's pin_origin/unpin_origin tests, not in this file.