fix(server,authz): ingress auth edge — never trust a client-asserted principal (ARN-170)#343
Open
rita-aga wants to merge 1 commit into
Open
fix(server,authz): ingress auth edge — never trust a client-asserted principal (ARN-170)#343rita-aga wants to merge 1 commit into
rita-aga wants to merge 1 commit into
Conversation
…auth edge (ARN-170) Systemic Class A fix. Strip all inbound x-temper-* identity headers at the ingress edge (except an observability allowlist); derive the principal only from a resolved credential, carried as an unforgeable EdgeAuthenticatedPrincipal request extension and materialized into trusted headers after the strip. from_headers now defaults to Customer, honors Admin only alongside the trusted in-process marker, and never mints System from headers. bearer_auth fails closed when a key is configured but no credential is presented. Closes the header-spoofable admin bypass and fail-open auth (ARN-170); root- causes ARN-187 (OTS) and is mirrored by ARN-167 (TemperPaw). Residual cleanup tracked in ARN-202. ADR-0157. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+127
to
+134
| } else { | ||
| tracing::warn!( | ||
| principal_id = %principal.id, | ||
| "resolved principal id is not a valid header value; dropping to anonymous" | ||
| ); | ||
| } | ||
| } | ||
| next.run(req).await |
There was a problem hiding this comment.
When HeaderValue::from_str(&principal.id) fails, the authenticated principal is silently dropped and the request continues as Customer:anonymous — the caller receives a 200 (or whatever the anonymous path returns) instead of an error. Today operator() always uses the ASCII-valid "api-key-holder", so this branch is unreachable in production. But any future EdgeAuthenticatedPrincipal with a non-ASCII or control-character ID would silently downgrade a fully-authenticated request, which could be very difficult to diagnose. CLAUDE.md's TigerStyle explicitly requires "no silent failures." Returning an internal-server-error response here keeps the fail-closed property while also surfacing the problem to the caller.
Suggested change
| } else { | |
| tracing::warn!( | |
| principal_id = %principal.id, | |
| "resolved principal id is not a valid header value; dropping to anonymous" | |
| ); | |
| } | |
| } | |
| next.run(req).await | |
| } else { | |
| tracing::error!( | |
| principal_id = %principal.id, | |
| "resolved principal id is not a valid header value; cannot materialize principal" | |
| ); | |
| return axum::response::IntoResponse::into_response( | |
| axum::http::StatusCode::INTERNAL_SERVER_ERROR, | |
| ); | |
| } | |
| } | |
| next.run(req).await |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-server/src/authz/edge.rs
Line: 127-134
Comment:
**Silent identity drop on invalid principal ID violates TigerStyle "no silent failures"**
When `HeaderValue::from_str(&principal.id)` fails, the authenticated principal is silently dropped and the request continues as `Customer:anonymous` — the caller receives a `200` (or whatever the anonymous path returns) instead of an error. Today `operator()` always uses the ASCII-valid `"api-key-holder"`, so this branch is unreachable in production. But any future `EdgeAuthenticatedPrincipal` with a non-ASCII or control-character ID would silently downgrade a fully-authenticated request, which could be very difficult to diagnose. CLAUDE.md's TigerStyle explicitly requires "no silent failures." Returning an internal-server-error response here keeps the fail-closed property while also surfacing the problem to the caller.
```suggestion
} else {
tracing::error!(
principal_id = %principal.id,
"resolved principal id is not a valid header value; cannot materialize principal"
);
return axum::response::IntoResponse::into_response(
axum::http::StatusCode::INTERNAL_SERVER_ERROR,
);
}
}
next.run(req).await
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
91
to
101
| kind = match value.as_str() { | ||
| "customer" => PrincipalKind::Customer, | ||
| "agent" => PrincipalKind::Agent, | ||
| "admin" => PrincipalKind::Admin, | ||
| // "system" is NOT accepted from headers to prevent | ||
| // privilege escalation via header spoofing. Use | ||
| // "admin" is honored only alongside the trusted marker | ||
| // the edge strips from client requests. "system" is | ||
| // never accepted from headers — use | ||
| // SecurityContext::system() for trusted internal paths. | ||
| // Everything else (including untrusted "admin" and | ||
| // "system") falls back to the most restrictive kind. | ||
| "admin" if trusted_principal => PrincipalKind::Admin, | ||
| _ => PrincipalKind::Customer, | ||
| }; |
There was a problem hiding this comment.
"customer" arm makes the privilege-sensitive match harder to audit
The previous code had "customer" => PrincipalKind::Customer as an explicit arm. Dropping it to the wildcard _ is functionally equivalent today, but in security-sensitive code the convention of being explicit about every recognized value matters: a future maintainer adding a new kind (e.g. "service") or renaming "customer" would not get a compile-time warning that the old string now silently falls to Customer. Keeping the explicit arm preserves the audit trail and makes the exhaustive-match intent clearer.
Suggested change
| kind = match value.as_str() { | |
| "customer" => PrincipalKind::Customer, | |
| "agent" => PrincipalKind::Agent, | |
| "admin" => PrincipalKind::Admin, | |
| // "system" is NOT accepted from headers to prevent | |
| // privilege escalation via header spoofing. Use | |
| // "admin" is honored only alongside the trusted marker | |
| // the edge strips from client requests. "system" is | |
| // never accepted from headers — use | |
| // SecurityContext::system() for trusted internal paths. | |
| // Everything else (including untrusted "admin" and | |
| // "system") falls back to the most restrictive kind. | |
| "admin" if trusted_principal => PrincipalKind::Admin, | |
| _ => PrincipalKind::Customer, | |
| }; | |
| kind = match value.as_str() { | |
| "customer" => PrincipalKind::Customer, | |
| "agent" => PrincipalKind::Agent, | |
| // "admin" is honored only alongside the trusted marker | |
| // the edge strips from client requests. "system" is | |
| // never accepted from headers — use | |
| // SecurityContext::system() for trusted internal paths. | |
| // Everything else (including untrusted "admin" and | |
| // "system") falls back to the most restrictive kind. | |
| "admin" if trusted_principal => PrincipalKind::Admin, | |
| _ => PrincipalKind::Customer, | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-authz/src/context.rs
Line: 91-101
Comment:
**Removing the explicit `"customer"` arm makes the privilege-sensitive match harder to audit**
The previous code had `"customer" => PrincipalKind::Customer` as an explicit arm. Dropping it to the wildcard `_` is functionally equivalent today, but in security-sensitive code the convention of being explicit about every recognized value matters: a future maintainer adding a new kind (e.g. `"service"`) or renaming `"customer"` would not get a compile-time warning that the old string now silently falls to `Customer`. Keeping the explicit arm preserves the audit trail and makes the exhaustive-match intent clearer.
```suggestion
kind = match value.as_str() {
"customer" => PrincipalKind::Customer,
"agent" => PrincipalKind::Agent,
// "admin" is honored only alongside the trusted marker
// the edge strips from client requests. "system" is
// never accepted from headers — use
// SecurityContext::system() for trusted internal paths.
// Everything else (including untrusted "admin" and
// "system") falls back to the most restrictive kind.
"admin" if trusted_principal => PrincipalKind::Admin,
_ => PrincipalKind::Customer,
};
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Collaborator
Author
ARN-165 principle audit — reject this trust design and redesignThis branch removes one raw-header path but replaces it with an ambient global-admin credential and creates a confused deputy. Blocking findings
Required architecture
The current branch is |
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
1 participant
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.
Closes ARN-170 (kernel Class A) under epic ARN-165 — the linchpin auth fix. Draft for review — do not merge.
Problem (systemic Class A)
Identity was derived from client-asserted
x-temper-*headers.SecurityContext::from_headersmintedAdminfromx-temper-principal-kind: admin, andbearer_authfailed open when noTEMPER_API_KEYwas set. So any caller could self-assert admin and bypass every Cedar gate (require_policy_auth,require_observe_auth,require_repl_auth). This is the root cause behind ARN-187 (OTS) and is the reason ARN-166's Layer-A gate is only fully effective once this lands.Fix — one edge, not per-site (ADR-0157)
x-temper-*identity header at the ingress edge (authz/edge.rs, outermost middleware), except a small observability allowlist. A client can no longer assert its own principal.EdgeAuthenticatedPrincipalrequest extension (headers can't forge or survive-strip it), materialized into trusted headers after the strip.from_headersnow defaults toCustomer; honorsAdminonly alongside the internal trusted marker; never mintsSystemfrom headers.bearer_authfails closed when a key is configured but no credential is presented.Verification
adminheader →Customer:anonymous; no-credential on a privileged surface → 401; smuggled trusted-marker → stripped; operator key →Admin:api-key-holder. (edge 5/5, authz context 16/16.)Notes
context.rsgrew to 527 lines (>500 soft-split guideline) — folds into ARN-184's file-split scope, not split here to keep this PR scoped.edge.rs; GT500 +1).blockedBythis — its/api/replLayer-A guarantee completes when this edge lands, so merge this with or before fix(sandbox,server): isolate REPL host ops and gate /api/repl behind Cedar (ARN-166) #342.🤖 Generated with Claude Code
Greptile Summary
This PR closes a Class A privilege-escalation vulnerability (ARN-170):
SecurityContext::from_headerspreviously mintedAdminfrom a client-suppliedx-temper-principal-kind: adminheader, andbearer_auth_checkfailed open when no API key was configured, meaning any caller could self-assert admin and bypass every Cedar gate. The fix is a three-layer ingress edge applied atbuild_platform_routerthat enforces "identity is derived from a resolved credential, never from a client header."strip_inbound_identity_headers, outermost): removes allx-temper-*authority headers before credential resolution; observability prefixes (x-temper-observe-*,x-temper-workflow-*) are preserved.from_headershardening:Adminis honored only when the internalTRUSTED_PRINCIPAL_HEADERmarker is present (set exclusively by trusted in-process code after the edge);Systemremains non-derivable from headers; everything else defaults toCustomer.materialize_authenticated_principal, innermost): converts a resolvedEdgeAuthenticatedPrincipalextension (set bybearer_auth_checkon a verified global-key match) into trusted headers with the marker; agent credentials flow through the existing resolved-identity extension path unchanged.Confidence Score: 4/5
The core auth fix is correct and thoroughly tested; the three-layer edge ordering is verified against Axum middleware semantics; exploit tests cover all four attack scenarios. The only issues found are quality/style concerns.
The auth architecture is sound, layer ordering is correct, and exploit test coverage is comprehensive. The four findings are a stale doc comment, a theoretically unreachable TigerStyle-violating silent-drop, a missing explicit match arm, and a deferred file-split. None affect the security property of this PR.
crates/temper-server/src/authz/edge.rs(silent-drop path inmaterialize_authenticated_principal) andcrates/temper-platform/src/bearer_auth.rs(stale resolution-order doc comment) deserve a second look before ARN-167 reuses this pattern.Important Files Changed
strip_inbound_identity_headersremoves allx-temper-*authority headers (preserving observability prefixes), andmaterialize_authenticated_principalconverts a resolvedEdgeAuthenticatedPrincipalextension into trusted post-strip headers. Core logic is correct; silent-drop-to-anonymous on invalid principal ID violates TigerStyle no-silent-failures.TRUSTED_PRINCIPAL_HEADERconstant and gatesAdminderivation infrom_headerson the trusted marker being present;Systemremains non-derivable from headers. The fix is correct. The explicit"customer"arm was silently dropped to_(functionally equivalent but less auditable in security-sensitive code). File is now 527 lines, crossing the >500-line split rule.EdgeAuthenticatedPrincipal::operator()extension) before attempting agent credential resolution, reversing the old precedence. The old guest-override impersonation path is removed. Module doc comment still lists the old ordering (agent → global key) and needs updating.strip_inbound_identity_headersas the outermost layer andmaterialize_authenticated_principalas the innermost layer inbuild_platform_router. Layer ordering (last.layer()= outermost) is correct per Axum semantics.TRUSTED_PRINCIPAL_HEADERinto in-process local-TData dispatch headers, enablingfrom_headersto honor an Admin kind inherited from the invokingSecurityContext. This is intentional: the invoking context is trusted in-process code, not an external request.tokio::spawn(async move { ... })into named future variables before spawn, allowing// determinism-okDST annotations to be placed on the spawn lines. No behavioral change.X-Temper-Principal-Kind: adminas a header shortcut are migrated toAuthorization: Bearer test-operator-key. The test state now setsapi_tokento enforce the real credential path.TRUSTED_PRINCIPAL_HEADERpublicly so internal crates (local_tdata_host, tests) can set the trusted marker without encoding the string literal themselves.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Client participant Strip as strip_inbound_identity_headers participant Auth as bearer_auth_check participant Mat as materialize_authenticated_principal participant Handler Client->>Strip: Request + x-temper-principal-kind: admin Note over Strip: Removes ALL x-temper-* authority headers Strip->>Auth: Request (identity headers gone) alt No API key configured Auth->>Mat: passthrough, no extension Mat->>Handler: Customer:anonymous else Global API key match Auth->>Auth: Insert EdgeAuthenticatedPrincipal::operator() Auth->>Mat: Request + extension Note over Mat: Writes kind=admin, id=api-key-holder, trusted-marker=1 Mat->>Handler: Admin:api-key-holder else Agent credential match Auth->>Auth: Insert ResolvedIdentity extension Auth->>Mat: Request + agent extension Mat->>Handler: Agent identity else No credential Auth-->>Client: 401 Unauthorized end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Client participant Strip as strip_inbound_identity_headers participant Auth as bearer_auth_check participant Mat as materialize_authenticated_principal participant Handler Client->>Strip: Request + x-temper-principal-kind: admin Note over Strip: Removes ALL x-temper-* authority headers Strip->>Auth: Request (identity headers gone) alt No API key configured Auth->>Mat: passthrough, no extension Mat->>Handler: Customer:anonymous else Global API key match Auth->>Auth: Insert EdgeAuthenticatedPrincipal::operator() Auth->>Mat: Request + extension Note over Mat: Writes kind=admin, id=api-key-holder, trusted-marker=1 Mat->>Handler: Admin:api-key-holder else Agent credential match Auth->>Auth: Insert ResolvedIdentity extension Auth->>Mat: Request + agent extension Mat->>Handler: Agent identity else No credential Auth-->>Client: 401 Unauthorized endComments Outside Diff (5)
docs/adrs/0157-class-a-auth-edge.md, line 1508-1512 (link)The ADR states: "A WASM TData host call that inherited an Admin principal now inherits it as Customer." However, the code does the opposite.
local_tdata_host::header_mapunconditionally insertsTRUSTED_PRINCIPAL_HEADER: "1", andsecurity_context_headersmapsPrincipalKind::Adminto"admin".from_headerstherefore seesadmin+ the trusted marker and yieldsAdmin— not Customer. The inline comment inlocal_tdata_host.rsand the updatedlocal_tdata_headers_inherit_invoking_security_contexttest both confirm that Admin IS preserved in local TData dispatch. Future readers relying on the Negative consequences section for the security model will be misled.Prompt To Fix With AI
crates/temper-platform/src/bearer_auth.rs, line 57-62 (link)PreAuthenticatedRequestbypass is unreachable in the production stackThis check requires both the
PreAuthenticatedRequestextension and thex-temper-principal-kind/x-temper-principal-idheaders to be present. In the production layer order (strip_inbound_identity_headersoutermost thenbearer_auth_check), the strip layer removes those principal headers before this code runs, and no middleware in the current production stack setsPreAuthenticatedRequestbetween the two layers. The testpre_authenticated_request_bypasses_bearer_requirementexercises a stack without the strip layer, so it cannot verify the production path. The bypass is dead code under the full edge and would silently remain so for any future embedder that applies the strip.Prompt To Fix With AI
crates/temper-authz/src/context.rs, line 1 (link)context.rsis now 527 lines, past the "Files > 500 lines must be split into directory modules" rule in CLAUDE.md/AGENTS.md. The PR description notes this is deferred to ARN-184, so flagging here to ensure it is tracked and not forgotten.Context Used: CLAUDE.md (source)
Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
crates/temper-platform/src/bearer_auth.rs, line 23-29 (link)The module doc comment still lists the old resolution order — agent credentials (step 3) before global API key (step 4) — but the implementation now checks the global API key first (the new
if matches_global_api_keyblock at line 88, labeled "Step 1", precedes the agent-credential check labeled "Step 2"). A reviewer or future maintainer reading the doc will expect the opposite order and may introduce a regression when modifying this flow.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
crates/temper-authz/src/context.rs, line 1 (link)context.rsexceeds the 500-line module-split thresholdThe file is now 527 lines, which crosses the "Files > 500 lines must be split into directory modules" rule in CLAUDE.md. The PR description acknowledges this and defers the split to ARN-184, and the readability baseline has been bumped to reflect it. This is a tracked known violation, but it is worth calling out as a concrete follow-up gate before ARN-184 closes.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(server,authz): never trust a client-..." | Re-trigger Greptile
Context used: