fix(inference): authenticate pin-route revocation with its policy - #7889
Conversation
Revoking a superseded HTTPS Pin Runtime route has to authenticate the running adapter first, and the control-plane challenge proof binds the route-source policy that adapter was started with. The revoke path never passed one, so it proved against the `127.0.0.1/32` default while every sandbox-facing adapter runs on the OpenShell bridge range. The probe could not match, the live adapter was reported unauthenticatable, and `inference set --endpoint-url` committed the new route while leaving the superseded one registered with its raw upstream credentials resident, telling the operator to uninstall NemoClaw to purge them. Each endpoint update on a DNS-backed HTTPS compatible-endpoint leaked another one. Record the policy in host recovery state when the adapter is started or reused, and read it back at revocation. Re-deriving it there would look equivalent but is not: discovery returns whatever the bridge looks like now, so a recreated or renumbered network yields a value the running adapter never used, and the same leak returns. When no policy is recorded the revoke path fails closed rather than proving against a value it made up; the next route registration records one and heals it. Fixes #7878 Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe HTTPS Pin Runtime adapter now persists its actual allowed-source CIDR policy and uses it during route revocation authentication. Revocation fails closed when policy data is missing or mismatched. Tests and endpoint security documentation cover the updated behavior. ChangesHTTPS Pin Runtime revocation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant EndpointUpdate
participant revokeRouteLocked
participant AdapterState
participant ControlHealthProbe
participant RouteRegistry
EndpointUpdate->>revokeRouteLocked: revoke superseded route
revokeRouteLocked->>AdapterState: read recorded source CIDRs
revokeRouteLocked->>ControlHealthProbe: authenticate with token and CIDR policy
ControlHealthProbe-->>revokeRouteLocked: authenticated adapter proof
revokeRouteLocked->>RouteRegistry: delete authenticated route
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 1d5ebb7 in the TypeScript / code-coverage/cliThe overall coverage in commit 1d5ebb7 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7889.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
cjagwani
left a comment
There was a problem hiding this comment.
Approved exact head ac46b7dff6f49d5a239077652fea621f6d5f3842.
Security review: PASS across credential handling, authentication/authorization, injection, file/state handling, network policy/SSRF boundaries, TOCTOU behavior, cryptography, dependencies, and fail-closed errors. The adapter-wide source-CIDR policy is recorded only after authenticated spawn/reuse, canonicalized with the existing matcher on read, and bound into the revocation challenge. Missing, malformed, or mismatched provenance never deletes the credential-bearing route; an absent adapter still clears stale state.
All 54 exact-head checks are green, including CodeQL, self-hosted sandbox/gateway coverage, image builds, macOS E2E, and the protected E2E gate. DCO and the Verified commit pass; no unresolved major/critical findings remain. Stale-base-only failure is waived because GitHub reports MERGEABLE/conflict-free.
Treat a persisted control token as evidence that the adapter may still hold credential-bearing routes when PID metadata is missing. Keep the route record until revocation can authenticate the adapter. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Describe the complete non-secret recovery metadata and state that upstream URLs, pinned addresses, and credentials are not stored. Clarify that a persisted token means the adapter may still hold a credential. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical dated `v0.0.101` changelog entry that was missing when the release tag was cut. This post-release recovery records the shipped behavior on current `main` without changing or replacing the existing tag. ## Changes - Add `docs/changelog/2026-08-03.mdx` with the exact `## v0.0.101` heading, release summary, detailed behavior changes, support boundaries, and links to durable documentation. - [#7317](#7317) -> `docs/changelog/2026-08-03.mdx`: Records experimental OpenClaw Google Chat support and its restricted credential and webhook boundary. - [#7715](#7715) -> `docs/changelog/2026-08-03.mdx`: Records strict onboarding recovery state and authoritative resume identity. - [#7749](#7749) -> `docs/changelog/2026-08-03.mdx`: Records the provider-neutral policy seam and unchanged runtime support boundary. - [#7817](#7817) -> `docs/changelog/2026-08-03.mdx`: Records preserved Hermes home-channel assignments across rebuilds. - [#7820](#7820) -> `docs/changelog/2026-08-03.mdx`: Records the SSH-session status field correction. - [#7847](#7847) -> `docs/changelog/2026-08-03.mdx`: Records fail-closed credential filtering for migration and rebuild backups. - [#7870](#7870) -> `docs/changelog/2026-08-03.mdx`: Records sandbox-qualified in-sandbox host command hints. - [#7875](#7875) -> `docs/changelog/2026-08-03.mdx`: Records Microsoft Teams stop and start E2E coverage. - [#7885](#7885) -> `docs/changelog/2026-08-03.mdx`: Records Hermes managed gateway detection in status. - [#7889](#7889) -> `docs/changelog/2026-08-03.mdx`: Records policy-authenticated HTTPS Pin Runtime route revocation. - [#7891](#7891) -> `docs/changelog/2026-08-03.mdx`: Records default fallback for negative timeout and polling overrides. - [#7993](#7993) -> `docs/changelog/2026-08-03.mdx`: Records correct sibling detection during uninstall. - [#7995](#7995) -> `docs/changelog/2026-08-03.mdx`: Records absent configuration-hash handling before shields lock. - [#8001](#8001) -> `docs/changelog/2026-08-03.mdx`: Records the dormant atomic managed workload replacement foundation. - [#8029](#8029) -> `docs/changelog/2026-08-03.mdx`: Records repository terminology review in PR Review Advisor. - [#8031](#8031) -> `docs/changelog/2026-08-03.mdx`: Records provider-neutral managed snapshot authority. - [#8032](#8032) -> `docs/changelog/2026-08-03.mdx`: Records immutable managed clone handoff contracts. - [#8034](#8034) -> `docs/changelog/2026-08-03.mdx`: Records the dormant provider-owned clone transaction surface. - [#8035](#8035) -> `docs/changelog/2026-08-03.mdx`: Records the dormant Hermes managed clone broker boundary. - [#8036](#8036) -> `docs/changelog/2026-08-03.mdx`: Records the dormant transactional managed bootstrap boundary. - [#8037](#8037) -> `docs/changelog/2026-08-03.mdx`: Records dormant Docker bootstrap primitives and the unchanged provider support boundary. - [#8070](#8070) -> `docs/changelog/2026-08-03.mdx`: Records consolidated sandbox resource-limit E2E coverage. - [#8071](#8071) -> `docs/changelog/2026-08-03.mdx`: Records escaped and bounded CLI validation diagnostics. - [#8081](#8081) -> `docs/changelog/2026-08-03.mdx`: Records bounded linear snapshot Base64 validation. - [#8085](#8085) -> `docs/changelog/2026-08-03.mdx`: Records commit-bound workflow approval for eligible same-repository maintainers. - [#8088](#8088) -> `docs/changelog/2026-08-03.mdx`: Records Hermes managed-policy E2E selection. - [#8090](#8090) -> `docs/changelog/2026-08-03.mdx`: Records pinned CI search-tool provisioning. - [#8106](#8106) -> `docs/changelog/2026-08-03.mdx`: Records fallback from failed managed OpenShell gateway startup. - [#8107](#8107) -> `docs/changelog/2026-08-03.mdx`: Records Hermes adapter lifecycle E2E selection. - [#8128](#8128) -> `docs/changelog/2026-08-03.mdx`: Records the dormant transactional Docker bootstrap adapter and rollback authority. - [#8140](#8140) -> `docs/changelog/2026-08-03.mdx`: Records Slack conflict scope across independent OpenShell gateways. - [#8147](#8147) -> `docs/changelog/2026-08-03.mdx`: Records completion of durable v0.0.100 documentation audit follow-ups. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This documentation-only recovery does not change executable behavior. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Independently reviewed `docs/changelog/2026-08-03.mdx` at commit `0bebe1f568e3dc85cf410aac1dfb8f8830070b85`. Its blob is `82887920f9720eafd75db6b2271c35f7477edb9b`. The entry follows the writing guide, controlled terminology, changelog structure, MDX SPDX format, literal CLI-name rule, and root-absolute route requirements. It accurately records the `v0.0.100...v0.0.101` release range, Announcement #8162, accepted scope boundaries, and shipped security behavior. There are no code samples. Focused changelog tests and the documentation build pass for this commit. - Agent: Codex Desktop independent documentation writer <!-- docs-review-head-sha: 0bebe1f --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## Security Review - Result: `PASS` - Reviewed commit: `0bebe1f568e3dc85cf410aac1dfb8f8830070b85` - Base commit: `643a4ab8b5f583d8555192a37927268b26022c51` - Findings: None. - Secrets and credentials: `PASS`. No credential values or secret files are present. - Input validation and data sanitization: `PASS`. No executable input path changes. - Authentication and authorization: `PASS`. No identity or permission logic changes. - Dependencies and third-party libraries: `PASS`. No dependency changes. - Error handling and logging: `PASS`. No runtime path changes; diagnostic-security claims are precise. - Cryptography and data protection: `PASS`. No implementation changes. - Configuration and security controls: `PASS`. No configuration, container, port, or HTTP changes. - Security testing: `PASS`. No coverage is removed; the entry records shipped test and security behavior. - System security: `PASS`. No runtime control changes; dormant and non-activation boundaries are explicit. - Agent: Codex Desktop independent security reviewer ## Verification - [ ] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub — verification is pending after commit `0bebe1f568e3dc85cf410aac1dfb8f8830070b85` is pushed. - [ ] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable — commit hooks passed; pre-push is pending. - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — tests are not applicable to this documentation-only recovery. - [x] Applicable broad gate passed — not applicable to this documentation-only recovery. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, credentials, or private keys are added by this diff. - [ ] `npm run docs` builds without warnings (doc changes only) — GitHub documentation checks are pending. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) — independent documentation review passed. - [x] New doc pages include SPDX header and frontmatter (new pages only) — the native changelog entry uses the required parser-safe MDX SPDX comment and intentionally has no frontmatter. GitHub CI is authoritative. Focused changelog tests and `npm run docs` passed after the merge refresh. --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added experimental Google Chat support. * Improved runtime and session status visibility. * Added onboarding recovery and persistence safeguards. * Added snapshot validation and dormant managed-workload support. * **Bug Fixes** * Improved backup sanitization, route handling, and gateway reliability. * **Documentation** * Added the v0.0.101 changelog and related updates. * **Tests** * Expanded end-to-end coverage and strengthened trusted CI validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com>
Summary
Revoking a superseded HTTPS Pin Runtime route has to authenticate the running adapter, and
the control-plane challenge proof binds the route-source policy that adapter was started
with. The revoke path never supplied one, so it proved against the loopback default while
every sandbox-facing adapter runs on the OpenShell bridge range. The superseded route stayed
registered with its raw upstream credentials resident. This PR records the policy when the
adapter is started or reused and reads it back at revocation.
Closes #7878.
Reproduction
Run on our DGX Spark aarch64 test host (GB10 GPU) — the reporter's platform is aarch64, so
the repro was not moved to x86_64:
Environment
main4dcb89ea1d89c0e46fc24a880b9f7cfdd8241efcnvidia-prod, switched tocompatible-endpointObserved on
main(before fix)The update exits 0 and only warns:
Host recovery state shows the superseded route is still registered:
The adapter's own log records both registrations and no revocation — the probe never
reached the control plane:
Probing the live adapter directly isolates the cause — same token, only the policy differs:
Observed on
fix/...(after fix)The adapter log now shows the revocation reaching the control plane, which never appeared
before:
The upgrade path was exercised on the same host: this adapter had been started by the
unfixed build (
allowedSourceCidrs: None). The firstinference seton the fixed buildrecorded the policy, and the next switch revoked cleanly — no adapter restart and no
NemoClaw uninstall.
Analysis
probeAdapterControlHealthbuilds the challenge proof over the route-source policy:Three callers reach it.
waitForAdapterHealthtakes the policy as a required parameter andfindReusableAdapterControlTokenreceives it from its caller — both are on the registrationpath, which is why registering routes always worked.
revokeRouteLockedpassed neither, soit fell back to
127.0.0.1/32whileensureHttpsPinRuntimeAdapterRoutestarts every adapterwith the discovered
openshell-dockerIPAM subnets. The digests differ, the proof does notmatch,
authenticatedLiveAdapteris false, and because the PID is an adapter process thepath throws
Cannot authenticate the live HTTPS Pin Runtime adapter for revocation.inference-set.tscatches that, warns, and continues — the new route is committed, the oldroute stays in adapter memory with its upstream credential, and the only remedy offered is
uninstalling NemoClaw.
destroy.tscalls the same revoke helper and prints the same class ofwarning, so the failure was not specific to the update path.
Fix
Record the route-source policy the adapter is actually running under in host recovery state,
and use that recorded value at revocation.
ensureAdapterProcessLockedwrites it alongside the pid at spawn — that is the policy thechild was launched with.
persistRouteStatewrites it on every successful registration, which also covers theadapter-reuse path where no spawn happens but
findReusableAdapterControlTokenhas justproven the live adapter answers under that policy.
removeRouteStatecarries it forward; dropping one route does not change the policy theadapter is running under.
revokeRouteLockedreads it back and passes it to the probe.Why not re-derive it at revocation. Calling
discoverOpenShellBridgeSourceCidrs()in therevoke path is a smaller diff and looks equivalent, but it re-derives the value it then uses
to authenticate: discovery returns whatever the bridge looks like now, so a recreated or
renumbered
openshell-dockernetwork produces a policy the running adapter never used andthe same leak returns. That leaves a subset of the reported class unfixed. The recorded value
is the fact; discovery is an inference about it.
Failure path. When no policy is recorded — an adapter started before this change — the
revoke path fails closed: it does not probe, does not delete the route, and does not drop the
route record, so the caller keeps warning and nothing is silently lost. Falling back to the
default or to re-discovery would authenticate the adapter against a value this process just
made up. The error names the recovery step, and the next route registration records the
policy, so an existing adapter heals without a restart (verified above).
Whole-class review. All three
probeAdapterControlHealthcall sites now carry a realpolicy. Both consumers of
revokeHttpsPinRuntimeAdapterRoute—inference-set.ts(endpointupdate) and
sandbox/destroy.ts(last-reference teardown) — are fixed by the change insiderevokeRouteLocked; neither needed its own edit. Nothing widens what the adapter accepts:the proof still binds the policy digest, the token check is unchanged, and the adapter's own
source-CIDR enforcement is untouched.
Tests (
https-pin-runtime-revoke-source-policy.test.ts, split out so the existing suitestays inside the test-file size budget): revocation probes with the recorded policy and
deletes the route; a live adapter with no recorded policy fails closed without probing,
deleting, or dropping state; an absent adapter with no recorded policy still clears its stale
record (regression lock on today's behaviour); a recorded policy that no longer matches still
reports the original authentication failure; and five malformed-record shapes (absent, empty,
non-array, non-string entries, blank entries) are all rejected rather than partially trusted.
Changes
src/lib/inference/https-pin-runtime-adapter.ts: record the route-source policy at spawn and on registration, carry it across route removal, and authenticate revocation with itsrc/lib/inference/https-pin-runtime-revoke-source-policy.test.ts: new focused suite for the revocation policy contractsrc/lib/inference/https-pin-runtime-adapter.test.ts: thread the new dependency through the existing revoke casesdocs/inference/custom-endpoint-security.mdx: document the recorded policy, why it is not re-derived, and the recovery step for an adapter started before it existedType of Change
Verification
npx prek run --all-filespassesnpm testpasses (touched files at minimum)make docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation