feat(hermes): use invoke JWT for inference - #6885
Conversation
Salvaged from #4627 after the contributor branch became inaccessible to maintainer pushes. Signed-off-by: Shannon Sands <shannon.sands.1979@gmail.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughHermes OAuth onboarding and tool-gateway refresh flows now support Nous invoke JWT credentials alongside legacy agent keys, with JWT inspection, path-specific expiration metadata, updated provider registration, refresh handling, and corresponding test coverage. ChangesHermes inference credential flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HermesToolGatewayBroker
participant OAuthDeviceCode
participant HermesProviderAuth
participant OpenShellInferenceProvider
HermesToolGatewayBroker->>OAuthDeviceCode: refresh OAuth access token
OAuthDeviceCode-->>HermesToolGatewayBroker: return invoke JWT or opaque token
HermesToolGatewayBroker->>HermesProviderAuth: select inference credential path
alt invoke JWT is usable
HermesProviderAuth->>OpenShellInferenceProvider: register JWT credential
else legacy fallback
HermesProviderAuth->>OpenShellInferenceProvider: register minted agent key
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 7170ee5 in the TypeScript / code-coverage/cliThe overall coverage in commit 7170ee5 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: Blockers
|
wscurran
added
area: onboarding
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/hermes-tool-gateway-broker.test.ts (1)
342-365: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd broker-level fallback and rejection coverage.
Add a separate case where refresh returns an opaque, expired, or wrong-scope token; assert exactly one agent-key request,
legacy_agent_keystate, the trusted base URL, and no credential leakage in output. The current test only proves the positive invoke path.As per path instructions, agent-boundary changes require negative-path tests proving fallback cannot bypass guards or expose credentials.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/hermes-tool-gateway-broker.test.ts` around lines 342 - 365, Add a separate broker-level test case alongside the existing positive invoke-path test, configuring refresh to return an opaque, expired, or wrong-scope token and exercising the fallback path. Assert exactly one agent-key request, persisted legacy_agent_key state, the trusted base URL, and that broker output contains no credentials; preserve the existing successful-path assertions.Source: Path instructions
🧹 Nitpick comments (1)
src/lib/oauth-device-code.test.ts (1)
77-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the credential classifier’s rejection boundaries.
Add behavior tests for an expired token, a token inside the 120-second skew, a wrong/missing scope, and the supported
scpformats. The opaque-token assertion alone will not catch regressions that route unusable JWTs intoinvoke_jwt.As per path instructions, tests should provide behavioral confidence through observable credential-selection outcomes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/oauth-device-code.test.ts` around lines 77 - 86, Add behavioral tests in the isNousInvokeAccessToken suite covering expired tokens, tokens within the 120-second expiry skew, wrong or missing scopes, and each supported scp representation. Assert credential-selection outcomes rather than only boolean classification, while preserving the existing valid JWT and opaque-token cases.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/hermes/host/tool-gateway-broker.ts`:
- Around line 251-306: Replace the payload-only logic in decodeJwtClaims,
jwtScopes, jwtExpiresAtIso, and isUsableInvokeJwt with one authoritative JWT
verification/classification contract that validates signature, issuer, audience,
required scope, and expiry before selecting invoke_jwt. In
src/lib/oauth-device-code.ts lines 151-201, expose and reuse this same verified
classification contract rather than maintaining separate JWT semantics; update
both sites consistently, with no direct change required elsewhere.
In `@src/lib/hermes-provider-auth.ts`:
- Around line 168-189: Validate the effective inference endpoint before calling
registerHermesInferenceProvider or persisting credentials, applying the broker’s
existing explicit allowlist and fail-closed policy. Cover
tokens.inference_base_url, minted.inference_base_url, and caller-supplied
baseUrl, and reject any untrusted or attacker-controlled URL before it can be
assigned to OPENAI_BASE_URL.
In `@src/lib/oauth-device-code.ts`:
- Around line 184-190: Guard the Date conversion in jwtExpiresAt in
src/lib/oauth-device-code.ts at lines 184-190 by validating the constructed
date’s time value before calling toISOString(), returning null when invalid.
Apply the same invalid-Date guard in agents/hermes/host/tool-gateway-broker.ts
at lines 291-296 so malformed expiration data cannot abort broker rotation.
---
Outside diff comments:
In `@test/hermes-tool-gateway-broker.test.ts`:
- Around line 342-365: Add a separate broker-level test case alongside the
existing positive invoke-path test, configuring refresh to return an opaque,
expired, or wrong-scope token and exercising the fallback path. Assert exactly
one agent-key request, persisted legacy_agent_key state, the trusted base URL,
and that broker output contains no credentials; preserve the existing
successful-path assertions.
---
Nitpick comments:
In `@src/lib/oauth-device-code.test.ts`:
- Around line 77-86: Add behavioral tests in the isNousInvokeAccessToken suite
covering expired tokens, tokens within the 120-second expiry skew, wrong or
missing scopes, and each supported scp representation. Assert
credential-selection outcomes rather than only boolean classification, while
preserving the existing valid JWT and opaque-token cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 05c5c6d9-34f8-4b33-9e8c-7a385d51b8ac
📒 Files selected for processing (6)
agents/hermes/host/tool-gateway-broker.tssrc/lib/hermes-provider-auth.test.tssrc/lib/hermes-provider-auth.tssrc/lib/oauth-device-code.test.tssrc/lib/oauth-device-code.tstest/hermes-tool-gateway-broker.test.ts
| function decodeBase64UrlJson(value) { | ||
| try { | ||
| const base64 = String(value || "") | ||
| .replace(/-/g, "+") | ||
| .replace(/_/g, "/"); | ||
| const parsed = JSON.parse(Buffer.from(base64, "base64").toString("utf8")); | ||
| return parsed && typeof parsed === "object" && !Array.isArray(parsed) ? parsed : null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function decodeJwtClaims(token) { | ||
| if (typeof token !== "string") return null; | ||
| const parts = token.split("."); | ||
| if (parts.length < 2) return null; | ||
| return decodeBase64UrlJson(parts[1]); | ||
| } | ||
|
|
||
| function jwtScopes(claims) { | ||
| const scopes = new Set(); | ||
| const scope = claims?.scope; | ||
| if (typeof scope === "string") { | ||
| for (const value of scope.split(/\s+/)) { | ||
| if (value) scopes.add(value); | ||
| } | ||
| } | ||
| const scp = claims?.scp; | ||
| if (typeof scp === "string") { | ||
| for (const value of scp.split(/\s+/)) { | ||
| if (value) scopes.add(value); | ||
| } | ||
| } else if (Array.isArray(scp)) { | ||
| for (const value of scp) { | ||
| if (typeof value === "string" && value) scopes.add(value); | ||
| } | ||
| } | ||
| return scopes; | ||
| } | ||
|
|
||
| function jwtExpiresAtIso(token) { | ||
| const exp = decodeJwtClaims(token)?.exp; | ||
| const seconds = | ||
| typeof exp === "number" ? exp : typeof exp === "string" && exp.trim() ? Number(exp) : NaN; | ||
| if (!Number.isFinite(seconds)) return null; | ||
| return new Date(seconds * 1000).toISOString(); | ||
| } | ||
|
|
||
| function isUsableInvokeJwt(token) { | ||
| const claims = decodeJwtClaims(token); | ||
| if (!claims || !jwtScopes(claims).has(NOUS_INFERENCE_INVOKE_SCOPE)) return false; | ||
| const expiresAt = jwtExpiresAtIso(token); | ||
| return Boolean( | ||
| expiresAt && !timestampExpiresSoon(expiresAt, INFERENCE_CREDENTIAL_REFRESH_SKEW_MS), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Replace payload-only JWT classification with one authoritative verifier. Both implementations trust unsigned claims to select the direct credential path.
agents/hermes/host/tool-gateway-broker.ts#L251-L306: verify signature, issuer, audience, scope, and expiry before selectinginvoke_jwt.src/lib/oauth-device-code.ts#L151-L201: expose and reuse the same verified classification contract rather than maintaining separate semantics.
📍 Affects 2 files
agents/hermes/host/tool-gateway-broker.ts#L251-L306(this comment)src/lib/oauth-device-code.ts#L151-L201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/hermes/host/tool-gateway-broker.ts` around lines 251 - 306, Replace
the payload-only logic in decodeJwtClaims, jwtScopes, jwtExpiresAtIso, and
isUsableInvokeJwt with one authoritative JWT verification/classification
contract that validates signature, issuer, audience, required scope, and expiry
before selecting invoke_jwt. In src/lib/oauth-device-code.ts lines 151-201,
expose and reuse this same verified classification contract rather than
maintaining separate JWT semantics; update both sites consistently, with no
direct change required elsewhere.
Source: Path instructions
| const inferenceBaseUrl = tokens.inference_base_url || baseUrl; | ||
| const useInvokeJwt = oauth.isNousInvokeAccessToken(tokens.access_token); | ||
| const minted = useInvokeJwt | ||
| ? null | ||
| : await oauth.mintAgentKeyWithAccessToken(tokens.access_token, { | ||
| fetch, | ||
| minTtlSeconds: AGENT_KEY_MIN_TTL_SECONDS, | ||
| }); | ||
| const credential = useInvokeJwt ? tokens.access_token : minted?.api_key; | ||
| if (!credential) { | ||
| throw new Error("Hermes Provider credential is empty"); | ||
| } | ||
| const credentialExpiresAt = useInvokeJwt | ||
| ? oauth.jwtExpiresAt(tokens.access_token) | ||
| : minted | ||
| ? agentKeyExpiresAt(minted) | ||
| : null; | ||
| registerHermesInferenceProvider( | ||
| minted.api_key, | ||
| credential, | ||
| runOpenshell, | ||
| HERMES_INFERENCE_CREDENTIAL_ENV, | ||
| inferenceBaseUrl, | ||
| minted?.inference_base_url || inferenceBaseUrl, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Validate the inference endpoint before registering the credential.
tokens.inference_base_url, minted.inference_base_url, and caller-supplied baseUrl flow directly into OPENAI_BASE_URL. An untrusted value can make OpenShell send the invoke JWT or agent key to an attacker-controlled host. Apply the same explicit allowlist/fail-closed policy used by the broker before registration and persistence.
This matches the PR’s listed untrusted inference-base-URL credential-boundary blocker.
Also applies to: 206-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/hermes-provider-auth.ts` around lines 168 - 189, Validate the
effective inference endpoint before calling registerHermesInferenceProvider or
persisting credentials, applying the broker’s existing explicit allowlist and
fail-closed policy. Cover tokens.inference_base_url, minted.inference_base_url,
and caller-supplied baseUrl, and reject any untrusted or attacker-controlled URL
before it can be assigned to OPENAI_BASE_URL.
| export function jwtExpiresAt(token: unknown): string | null { | ||
| const claims = decodeJwtClaims(token); | ||
| const exp = claims?.exp; | ||
| const seconds = | ||
| typeof exp === "number" ? exp : typeof exp === "string" && exp.trim() ? Number(exp) : NaN; | ||
| if (!Number.isFinite(seconds)) return null; | ||
| return new Date(seconds * 1000).toISOString(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard JWT expiration conversion against invalid Date ranges.
src/lib/oauth-device-code.ts#L184-L190: returnnullwhen the constructed date has an invalid time value.agents/hermes/host/tool-gateway-broker.ts#L291-L296: apply the same guard so malformed OAuth data cannot abort broker rotation.
📍 Affects 2 files
src/lib/oauth-device-code.ts#L184-L190(this comment)agents/hermes/host/tool-gateway-broker.ts#L291-L296
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/oauth-device-code.ts` around lines 184 - 190, Guard the Date
conversion in jwtExpiresAt in src/lib/oauth-device-code.ts at lines 184-190 by
validating the constructed date’s time value before calling toISOString(),
returning null when invalid. Apply the same invalid-Date guard in
agents/hermes/host/tool-gateway-broker.ts at lines 291-296 so malformed
expiration data cannot abort broker rotation.
|
Salvage follow-up: normal CI and the selected live E2E lanes now pass on the unchanged head after one bounded rerun of transient live-runner failures. This PR remains draft. The CodeRabbit findings confirm the existing approval blockers:
No security-design changes were made during salvage. Documentation review found no update needed for the test-only salvage follow-up. |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Exact-head security review for 0b9098db3eb01d9b3e3261d63d82ac7fca65c22b: changes requested. Please keep this draft blocked until the authentication and credential-lifecycle design is defined and implemented.
The current invoke_jwt selection decodes claims without verifying the JWT signature, algorithm, issuer, audience, key ID/rotation, or nbf; the tests even use alg: none. Separately, inference_base_url returned by the token or agent-key response can override the caller's validated endpoint before the credential is registered, which can redirect a credential to an attacker-controlled endpoint. A direct invoke JWT also has no refresh owner when managed tools are absent, so the configured provider becomes unrecoverable after expiry. Malformed but finite exp values can additionally throw during toISOString().
Before implementation proceeds, please define one authoritative verification contract: trusted issuer/audience/algorithms/JWKS and outage behavior; endpoint allowlist and precedence for every URL source; refresh ownership without managed tools; least-privilege scope; fail-closed versus legacy fallback behavior; and state migration/rollback. Then reuse that contract in onboarding and the broker, and add negative tests for signature/claims/key rotation, endpoint injection, malformed expiry, no-tools refresh, fallback, concurrency, crash recovery, migration, and redaction.
CI being green does not cover these trust-boundary failures; three related automated-review threads are also still unresolved.
|
v0.0.88 disposition: deferred while this remains a draft and the authentication design is incomplete. Approval requires an authoritative JWT verification and lifecycle contract covering issuer, audience, JWKS trust, endpoint validation, expiration/clock behavior, and refresh ownership. This is a security-design dependency, not a missing-test disposition. |
Co-authored-by: Shannon Sands <shannon.sands.1979@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Shannon Sands <shannon.sands.1979@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
cv
added
v0.0.102
and removed
needs: design
|
@ericksoa, could you review this draft and provide the product-scope and authentication decisions needed before implementation continues? The current reviews block direct
The unresolved security findings are the payload-only JWT classification and untrusted inference endpoint registration. We are holding code changes until an accepted decision defines this supported credential surface. |
|
I approve direct This should follow the OpenShell Providers v2 model used for refresh-backed providers such as Microsoft Graph/Entra:
One compatibility question must be resolved before implementation: OpenShell's generic Please revise this PR around that provider boundary. Live validation should cover inference-only use across multiple token lifetimes, managed-tools use, gateway/host restart, refresh-token rotation and concurrency, endpoint injection rejection, terminal reauthentication, rollback, and confirmation that no access or refresh token appears in logs or persisted NemoClaw state. |
9 participants
Summary
Salvages the contributor work from #4627 onto an NVIDIA-owned branch after maintainer pushes to the contributor fork were denied. Hermes onboarding and the managed tool-gateway broker prefer Nous
inference:invokeaccess JWTs forinference.local, with legacy agent-key minting retained as a fallback.This remains a draft. Product-scope approval and the credential-boundary findings recorded on #4627 must be resolved before approval.
Changes
main.inference:invokeand legacy agent-key scopes during Hermes OAuth device-code onboarding.hermes-provider, while retaining legacy agent-key minting for opaque, expired, or non-invoke tokens.Type of Change
Quality Gates
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run src/lib/oauth-device-code.test.ts src/lib/hermes-provider-auth.test.ts test/hermes-tool-gateway-broker.test.ts --maxWorkers=1 --testTimeout=60000(14 tests passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Shannon Sands shannon.sands.1979@gmail.com
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit