fix(auth): require SSO-principal sponsor for agent registration - #1497
fix(auth): require SSO-principal sponsor for agent registration#1497kjgbot wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6b06378 to
8731e2d
Compare
8731e2d to
9387243
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b0637803d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SPONSOR_PROOF_HASH_METADATA_KEY.to_string(), | ||
| Value::String(sponsor.proof_hash.clone()), |
There was a problem hiding this comment.
When an attacker possesses a workspace key, this local check is bypassable because the request sends only caller-controlled sponsor metadata and a proof hash, not the signed proof that Relaycast could verify. The existing agent-relay mcp-args --register path in crates/broker/src/cli_mcp_args.rs still calls register_agent_token with only the workspace key, and a direct POST /v1/agents does the same, so such a holder can still register or rotate without any SSO grant. Require and verify the signed grant at the Relaycast registration/rotation endpoints rather than only in this client wrapper.
Useful? React with 👍 / 👎.
| agent_type: Option<&str>, | ||
| identity_key: Option<&str>, | ||
| ) -> Result<(String, String, String, Option<String>)> { | ||
| let sponsor = self.require_authenticated_sponsor()?; |
There was a problem hiding this comment.
When startup has no configured workspace key and the sponsor proof is missing or expired, startup_single_session_set_from_sources creates a fresh workspace before execution reaches this check. Registration then fails, leaving an orphan workspace and key on every invocation; supervisor or user retries can accumulate many unused workspaces. Require the authenticated sponsor before create_workspace or any other startup side effect.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/broker/src/relaycast/auth.rs">
<violation number="1" location="crates/broker/src/relaycast/auth.rs:485">
P2: When no workspace key is configured, an invalid or expired sponsor still creates a fresh workspace before registration fails. Validate the sponsor before `create_workspace` so failed authentication does not leak a workspace on every startup.</violation>
<violation number="2" location="crates/broker/src/relaycast/auth.rs:500">
P0: A workspace-key holder can rewrite the sponsor metadata, match its own valid sponsor, and then rotate another agent's token or set a chosen identity hash to reclaim it. Store the sponsor/work-unit binding in immutable server-controlled state, or authorize with an unforgeable server-side attestation instead of editable agent metadata.</violation>
<violation number="3" location="crates/broker/src/relaycast/auth.rs:500">
P2: Upgrading an existing deployment strands every previously-registered agent. Agents created before this change carry no `relayauth_sponsor_id`/`relayauth_sponsor_binding` metadata, so `existing_sponsor`/`existing_binding` are `None`. `rotate_token_no_fallback` then returns a plain non-404 error, so the outer `rotate_token` 404-triggered re-registration fallback never runs; and the reclaim branch in `admit_agent_registration` hard-fails on `!reclaims_same_sponsor`. A broker restart that needs to reclaim its own node agent (whose identity key is unchanged, via `stable_node_identity_key`) or rotate a stale token is now permanently refused with no migration path — the agent must be manually deleted and re-created. Consider allowing a reclaim/rotation when the stored work-unit identity still matches, and re-stamp the fresh sponsor metadata (the operator already holds a valid proof) instead of hard-failing on the missing legacy fields.</violation>
<violation number="4" location="crates/broker/src/relaycast/auth.rs:783">
P1: The new sponsor-proof requirement is enforced only client-side, in this AuthClient wrapper. Any caller that hits the Relaycast registration/rotation endpoints directly (e.g. the `agent-relay mcp-args --register` path in cli_mcp_args.rs, or a raw `POST /v1/agents` with just a workspace key) bypasses this check entirely and can still register or rotate an agent without an SSO sponsor grant. Enforce and verify the signed sponsor proof at the Relaycast server endpoints themselves, not only in this client wrapper.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .metadata | ||
| .get(SPONSOR_BINDING_METADATA_KEY) | ||
| .and_then(Value::as_str); | ||
| if existing_sponsor != Some(sponsor.sponsor_id.as_str()) || existing_binding != Some("oidc") |
There was a problem hiding this comment.
P0: A workspace-key holder can rewrite the sponsor metadata, match its own valid sponsor, and then rotate another agent's token or set a chosen identity hash to reclaim it. Store the sponsor/work-unit binding in immutable server-controlled state, or authorize with an unforgeable server-side attestation instead of editable agent metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/relaycast/auth.rs, line 500:
<comment>A workspace-key holder can rewrite the sponsor metadata, match its own valid sponsor, and then rotate another agent's token or set a chosen identity hash to reclaim it. Store the sponsor/work-unit binding in immutable server-controlled state, or authorize with an unforgeable server-side attestation instead of editable agent metadata.</comment>
<file context>
@@ -457,8 +482,27 @@ impl AuthClient {
+ .metadata
+ .get(SPONSOR_BINDING_METADATA_KEY)
+ .and_then(Value::as_str);
+ if existing_sponsor != Some(sponsor.sponsor_id.as_str()) || existing_binding != Some("oidc")
+ {
+ anyhow::bail!(
</file context>
There was a problem hiding this comment.
Confirmed. Relaycast #324 removes metadata from the trust decision, rejects reserved credential metadata, stores sponsor/OIDC/work-unit ownership in write-once columns, and keeps a durable name claim across deletion. DB triggers recheck the complete binding at the mutation boundary. Forged-metadata rotation/reclaim and delete/recreate transfer have regression tests. This original branch remains superseded, so I am leaving the thread unresolved here.
| .unwrap_or_else(|| format!("agent-{}", Uuid::new_v4().simple())); | ||
|
|
||
| admit_agent_registration(&relay, &name, agent_type, identity_key).await | ||
| admit_agent_registration(&relay, &name, agent_type, identity_key, sponsor).await |
There was a problem hiding this comment.
P1: The new sponsor-proof requirement is enforced only client-side, in this AuthClient wrapper. Any caller that hits the Relaycast registration/rotation endpoints directly (e.g. the agent-relay mcp-args --register path in cli_mcp_args.rs, or a raw POST /v1/agents with just a workspace key) bypasses this check entirely and can still register or rotate an agent without an SSO sponsor grant. Enforce and verify the signed sponsor proof at the Relaycast server endpoints themselves, not only in this client wrapper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/relaycast/auth.rs, line 783:
<comment>The new sponsor-proof requirement is enforced only client-side, in this AuthClient wrapper. Any caller that hits the Relaycast registration/rotation endpoints directly (e.g. the `agent-relay mcp-args --register` path in cli_mcp_args.rs, or a raw `POST /v1/agents` with just a workspace key) bypasses this check entirely and can still register or rotate an agent without an SSO sponsor grant. Enforce and verify the signed sponsor proof at the Relaycast server endpoints themselves, not only in this client wrapper.</comment>
<file context>
@@ -730,12 +774,13 @@ impl AuthClient {
.unwrap_or_else(|| format!("agent-{}", Uuid::new_v4().simple()));
- admit_agent_registration(&relay, &name, agent_type, identity_key).await
+ admit_agent_registration(&relay, &name, agent_type, identity_key, sponsor).await
}
</file context>
There was a problem hiding this comment.
Confirmed. Server enforcement is in Relaycast #324 and hosted wiring in relaycast-cloud #60. Relay #1505 now carries authority on mcp-args, REST, child pre-registration, node-control, fleet, and A2A, but correctness no longer depends on that client behavior. This original branch remains superseded, so I am leaving the thread unresolved here.
| .context("cannot rotate token without agent name")?; | ||
| let api_key = normalize_workspace_key(&cached.api_key) | ||
| .context("cached api_key is not a valid workspace key")?; | ||
| let sponsor = self.require_authenticated_sponsor()?; |
There was a problem hiding this comment.
P2: When no workspace key is configured, an invalid or expired sponsor still creates a fresh workspace before registration fails. Validate the sponsor before create_workspace so failed authentication does not leak a workspace on every startup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/relaycast/auth.rs, line 485:
<comment>When no workspace key is configured, an invalid or expired sponsor still creates a fresh workspace before registration fails. Validate the sponsor before `create_workspace` so failed authentication does not leak a workspace on every startup.</comment>
<file context>
@@ -457,8 +482,27 @@ impl AuthClient {
.context("cannot rotate token without agent name")?;
let api_key = normalize_workspace_key(&cached.api_key)
.context("cached api_key is not a valid workspace key")?;
+ let sponsor = self.require_authenticated_sponsor()?;
let relay = build_relay_client(&api_key, self.base_url.as_deref())?;
</file context>
| .metadata | ||
| .get(SPONSOR_BINDING_METADATA_KEY) | ||
| .and_then(Value::as_str); | ||
| if existing_sponsor != Some(sponsor.sponsor_id.as_str()) || existing_binding != Some("oidc") |
There was a problem hiding this comment.
P2: Upgrading an existing deployment strands every previously-registered agent. Agents created before this change carry no relayauth_sponsor_id/relayauth_sponsor_binding metadata, so existing_sponsor/existing_binding are None. rotate_token_no_fallback then returns a plain non-404 error, so the outer rotate_token 404-triggered re-registration fallback never runs; and the reclaim branch in admit_agent_registration hard-fails on !reclaims_same_sponsor. A broker restart that needs to reclaim its own node agent (whose identity key is unchanged, via stable_node_identity_key) or rotate a stale token is now permanently refused with no migration path — the agent must be manually deleted and re-created. Consider allowing a reclaim/rotation when the stored work-unit identity still matches, and re-stamp the fresh sponsor metadata (the operator already holds a valid proof) instead of hard-failing on the missing legacy fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/relaycast/auth.rs, line 500:
<comment>Upgrading an existing deployment strands every previously-registered agent. Agents created before this change carry no `relayauth_sponsor_id`/`relayauth_sponsor_binding` metadata, so `existing_sponsor`/`existing_binding` are `None`. `rotate_token_no_fallback` then returns a plain non-404 error, so the outer `rotate_token` 404-triggered re-registration fallback never runs; and the reclaim branch in `admit_agent_registration` hard-fails on `!reclaims_same_sponsor`. A broker restart that needs to reclaim its own node agent (whose identity key is unchanged, via `stable_node_identity_key`) or rotate a stale token is now permanently refused with no migration path — the agent must be manually deleted and re-created. Consider allowing a reclaim/rotation when the stored work-unit identity still matches, and re-stamp the fresh sponsor metadata (the operator already holds a valid proof) instead of hard-failing on the missing legacy fields.</comment>
<file context>
@@ -457,8 +482,27 @@ impl AuthClient {
+ .metadata
+ .get(SPONSOR_BINDING_METADATA_KEY)
+ .and_then(Value::as_str);
+ if existing_sponsor != Some(sponsor.sponsor_id.as_str()) || existing_binding != Some("oidc")
+ {
+ anyhow::bail!(
</file context>
There was a problem hiding this comment.
Confirmed. The replacement uses a guarded rollout rather than trusting legacy metadata: deploy Relay #1505 first to pre-stage the exact incumbent agent token, then enable Relaycast #324, then bind once through an agent-token-only endpoint. Workspace keys receive 401 on migration and cannot reclaim legacy rows. A true two-start old-server to enforced-server test covers the flow. Skipped hosts fail closed and need their incumbent token explicitly. This original branch remains superseded, so I am leaving the thread unresolved here.
|
Security follow-up: this PR must not be merged as the SOC2 Hole 1 fix. Its client-only checks remain bypassable by raw Relaycast API and node-control callers, and its metadata-based ownership is caller-editable. The replacement is split across draft PRs:
All original findings have regression coverage in the replacement branches. No merge/deploy has been performed. The original review threads should remain unresolved here because this branch itself still does not close the authority gap. |
|
Closing in favor of #1505, which fixes the same vulnerability class via a different architecture (server-side sponsor authority instead of client-side metadata checks). Guarantee-by-guarantee accounting, so this record survives the close:
G5 and G6 are enforced nowhere today, by either this PR or #1505. They become real only once See #1505 for full detail and current status of that dependency chain. |
3 participants
Summary
Root cause and impact
AuthClient::register_agent_with_workspace_key previously used possession of the shared workspace key as sufficient authority to create an agent, and collision recovery could rotate credentials after only checking a work-unit identity hash. Any workspace-key holder could therefore claim a sponsor identity that was not tied to the SSO-authenticated human operating Chief.
The broker now fails closed unless Chief supplies a valid RelayAuth sponsor grant for the authenticated user. Registration, reclaim, and direct rotation all enforce that binding. The replayable proof is not persisted or published.
Chief integration contract
Chief supplies RELAYAUTH_SPONSOR_ID, RELAYAUTH_SPONSOR_PROOF, RELAYAUTH_SPONSOR_ORG_ID, RELAYAUTH_ISSUER, and RELAYAUTH_SIGNING_KEY_PEM_PUBLIC. The proof must be an unexpired RS256 sponsor_grant for audience relayauth:sponsor-binding and intent identity.create.
Validation