fix(server): authenticate webhook ingress with HMAC + Cedar (ARN-171)#340
fix(server): authenticate webhook ingress with HMAC + Cedar (ARN-171)#340rita-aga wants to merge 1 commit into
Conversation
The inbound webhook route GET|POST /webhooks/{tenant}/{*path} dispatched a
spec action with security_ctx: None — no Cedar authorization and no signature
verification (Class B, ARN-165). This adds one authenticated ingress:
- HMAC-SHA256 over the raw body, constant-time compared (subtle::ConstantTimeEq)
against the signature header; secret resolved from the tenant vault via
{secret:KEY} templates; fail closed on missing/unresolvable secret.
- A restricted webhook:{name} Agent principal authorized through the same
authorize_with_context Cedar path as OData writes (default-deny per tenant);
denials for authenticated callers recorded via record_authz_denial.
Order: lookup(404) -> method(405) -> HMAC(401) -> entity-id(400) -> Cedar(403)
-> dispatch. Revives the previously-dead Webhook.hmac_secret/hmac_header spec
fields. See docs/adrs/0156-class-b-webhook-ingress.md (incl. rollout note).
Red-green exploit tests added (11/11 green): missing/invalid/full-length-wrong
signature -> 401, unresolvable secret -> 401, Cedar-denied -> 403, signed+
authorized -> 200.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Claude-Session: https://claude.ai/code/session_01BM1pdoLhHH7TQgGuiadQiy
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| action, | ||
| resource_type: &entity_type, | ||
| resource_id: &entity_id, | ||
| resource_attrs: serde_json::to_value(&resource_attrs).unwrap_or_default(), |
There was a problem hiding this comment.
unwrap_or_default() silently coerces a serialization failure to serde_json::Value::Null, violating the project's "No silent failures" rule. While BTreeMap<String, serde_json::Value> is infallible to serialize, the silent fallback would pass null as resource_attrs to record_authz_denial, potentially corrupting the denial record. An explicit panic with a comment is the appropriate TigerStyle alternative when the failure is provably unreachable.
| resource_attrs: serde_json::to_value(&resource_attrs).unwrap_or_default(), | |
| resource_attrs: serde_json::to_value(&resource_attrs) | |
| .expect("BTreeMap<String, Value> serializes infallibly"), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-server/src/webhooks/receiver.rs
Line: 167
Comment:
The `unwrap_or_default()` silently coerces a serialization failure to `serde_json::Value::Null`, violating the project's "No silent failures" rule. While `BTreeMap<String, serde_json::Value>` is infallible to serialize, the silent fallback would pass `null` as `resource_attrs` to `record_authz_denial`, potentially corrupting the denial record. An explicit panic with a comment is the appropriate TigerStyle alternative when the failure is provably unreachable.
```suggestion
resource_attrs: serde_json::to_value(&resource_attrs)
.expect("BTreeMap<String, Value> serializes infallibly"),
```
How can I resolve this? If you propose a fix, please make it concise.
| if value.is_empty() || value.contains("{secret:") { | ||
| return None; | ||
| } | ||
| Some(value) |
There was a problem hiding this comment.
{secret:
The check value.contains("{secret:") is designed to detect unresolved templates (where resolve_secret_templates left the pattern verbatim). However, it is evaluated against the resolved value — i.e., what came back from the vault. If the actual signing secret stored in the vault happens to contain the literal substring {secret: (e.g., an unusual token format), the function incorrectly returns None and rejects every request with 401, even when the secret is fully configured. In practice signing secrets are random hex/base64 and this won't occur, but the check is logically applied to the wrong value. The correct approach is to compare the resolved value against the original template: if they are identical the template was unresolved.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-server/src/webhooks/receiver.rs
Line: 282-285
Comment:
**False-positive on resolved secrets containing `{secret:`**
The check `value.contains("{secret:")` is designed to detect unresolved templates (where `resolve_secret_templates` left the pattern verbatim). However, it is evaluated against the *resolved* value — i.e., what came back from the vault. If the actual signing secret stored in the vault happens to contain the literal substring `{secret:` (e.g., an unusual token format), the function incorrectly returns `None` and rejects every request with 401, even when the secret is fully configured. In practice signing secrets are random hex/base64 and this won't occur, but the check is logically applied to the wrong value. The correct approach is to compare the resolved value against the original template: if they are identical the template was unresolved.
How can I resolve this? If you propose a fix, please make it concise.
| tenant: &TenantId, | ||
| webhook: &Webhook, | ||
| headers: &HeaderMap, | ||
| body: &[u8], | ||
| ) -> Result<bool, (StatusCode, String)> { | ||
| let Some(secret_template) = webhook.hmac_secret.as_deref() else { | ||
| return Ok(false); | ||
| }; | ||
|
|
||
| let secret = resolve_webhook_secret(state, tenant, secret_template).ok_or_else(|| { | ||
| ( | ||
| StatusCode::UNAUTHORIZED, | ||
| "Webhook signing secret is not configured".to_string(), | ||
| ) | ||
| })?; | ||
|
|
||
| let header_name = webhook | ||
| .hmac_header | ||
| .as_deref() | ||
| .unwrap_or(DEFAULT_SIGNATURE_HEADER); | ||
| let provided = headers | ||
| .get(header_name) | ||
| .and_then(|value| value.to_str().ok()) | ||
| .map(str::trim) | ||
| .filter(|value| !value.is_empty()) | ||
| .ok_or_else(|| { | ||
| ( | ||
| StatusCode::UNAUTHORIZED, | ||
| format!("Missing webhook signature header '{header_name}'"), | ||
| ) | ||
| })?; | ||
|
|
||
| if signature_matches(&secret, body, provided) { | ||
| Ok(true) | ||
| } else { | ||
| Err(( | ||
| StatusCode::UNAUTHORIZED, | ||
| "Webhook signature verification failed".to_string(), | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
verify_webhook_signature returns three semantically different response bodies under the same 401 status: "Webhook signing secret is not configured", "Missing webhook signature header '...'", and "Webhook signature verification failed". These are all returned verbatim to the caller via return (status, message) in handle_webhook.
An attacker probing the endpoint can distinguish: (a) whether a webhook has HMAC configured at all, and (b) whether they need to supply a header — before they've presented any credential. This reveals internal webhook security posture. A single opaque message ("Webhook authentication failed") would eliminate the signal without degrading legitimate error handling, since the structured detail is already captured in the tracing::warn! log.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-server/src/webhooks/receiver.rs
Line: 225-265
Comment:
**Three distinct 401 bodies leak webhook security configuration**
`verify_webhook_signature` returns three semantically different response bodies under the same 401 status: `"Webhook signing secret is not configured"`, `"Missing webhook signature header '...'"`, and `"Webhook signature verification failed"`. These are all returned verbatim to the caller via `return (status, message)` in `handle_webhook`.
An attacker probing the endpoint can distinguish: (a) whether a webhook has HMAC configured at all, and (b) whether they need to supply a header — before they've presented any credential. This reveals internal webhook security posture. A single opaque message (`"Webhook authentication failed"`) would eliminate the signal without degrading legitimate error handling, since the structured detail is already captured in the `tracing::warn!` log.
How can I resolve this? If you propose a fix, please make it concise.| @@ -0,0 +1,227 @@ | |||
| # ADR-0156: Authenticated webhook ingress (Class B) | |||
|
|
|||
| - Status: Proposed | |||
There was a problem hiding this comment.
Proposed to Accepted before this is merged. A proposed ADR that never transitions to accepted leaves it ambiguous whether the team has ratified the decision.
| - Status: Proposed | |
| - Status: Accepted |
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/adrs/0156-class-b-webhook-ingress.md
Line: 3
Comment:
The ADR status should be updated from `Proposed` to `Accepted` before this is merged. A proposed ADR that never transitions to accepted leaves it ambiguous whether the team has ratified the decision.
```suggestion
- Status: Accepted
```
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!
ARN-165 principle audit — request changesThe HMAC check is useful, but this PR does not yet create an authenticity boundary for ARN-171. Blocking findings
Required direction
Please resolve the ADR and redesign around a signed operation envelope rather than adding another local comparison. |
1 participant
Closes part of ARN-171 (kernel Class B) under epic ARN-165. Draft for review — do not merge.
Problem
GET|POST /webhooks/{tenant}/{*path}dispatched a spec-declared entity action withsecurity_ctx: None— no Cedar authorization and no signature check. Tenant came from the URL andentity_idfrom a client query param (both attacker-chosen), so anyone who could reach the port could drive an arbitrary declared action on an arbitrary entity, unauthenticated. This is the kernel instance of systemic Class B.Fix (
crates/temper-server/src/webhooks/receiver.rs)Two gates before any dispatch — order: lookup(404) → method(405) → HMAC(401) → entity-id(400) → Cedar(403) → dispatch.
subtle::ConstantTimeEq, not==) against the signature header; secret resolved from the tenant vault via the existing{secret:KEY}resolver; optionalsha256=prefix, case-insensitive; fail-closed on missing/unresolvable/wrong secret → 401. Revives the previously-deadWebhook.hmac_secret/hmac_headerspec fields.authorize_with_contextpath as OData writes, using a restrictedwebhook:{name}Agentprincipal (neverSystem), against the entity's resource snapshot → 403 on deny. Denials are recorded (record_authz_denial) only for HMAC-authenticated callers, so the public route can't be used to amplify pending-decision records.Tests (
receiver_test.rs, split out soreceiver.rsstays at 397 lines < 500-line rule)cargo test -p temper-server --lib webhooks::receiver→ 11/11 pass. Exploit tests captured red on unfixed code (missing signature, invalid signature, Cedar-denied). Covers: missing sig, wrong sig, full-length wrong sig (exercises the constant-time equal-length path), unresolvable secret → 401, Cedar-deny with and without a secret, full signed+authorized happy path.The route is now Cedar-authorized always, and a tenant with a policy set is default-deny. So a live webhook needs a permit for its principal:
In-repo enumeration: the only inbound webhook is
test-fixtures/specs/gmail_oauth.ioa.toml(oauth_callback) — its DST test drivessim.step(...), not the HTTP route, so no regression.reference-apps/*/integration.toml[[webhook]]blocks are outbound dispatcher configs, unaffected. Out-of-repo production webhooks (TemperPaw OAuth, Katagami) must ship theirwebhook:*permit in the same deploy — the platform Cedar generator does not auto-emit it today (follow-up).Reviews
DST review PASS (0 findings); code review PASS (re-confirmed on the final delta). Verified
SecurityContext::from_headers(&[])yields the most-restrictive principal (Customer, never Admin/System).Residual
(a) fail-closed for no-secret webhooks depends on the tenant having a policy set loaded (parity with OData; documented in ADR-0156); (b) hex + optional
sha256=prefix only — base64/Stripe-timestamped schemes and replay windows are explicit non-goals/follow-ups; (c) out-of-repo prod webhooks need permit policies landed with the deploy.🤖 Generated with Claude Code
Greptile Summary
This PR closes a Class B kernel vulnerability where
GET|POST /webhooks/{tenant}/{*path}dispatched entity actions with no authentication and no Cedar authorization — tenant and entity ID were both attacker-supplied. The fix inserts two ordered gates: an HMAC-SHA256 authenticity check (constant-time viasubtle::ConstantTimeEq, fail-closed on unresolvable secret) and the same Cedarauthorize_with_contextpath used by the OData write binding, using a restrictedwebhook:{name}Agentprincipal.{secret:KEY}template resolver; rejects with 401 on missing/unresolvable/wrong signature;Ok(false)when no secret is declared (fail-open for OAuth-style callbacks that providers cannot sign, still governed by Gate 2).webhook:{name}principal with anauthenticatedattribute reflecting HMAC result, evaluates against the entity's resource snapshot; denial records are written only for HMAC-authenticated callers to prevent pending-decision amplification via the public route.Confidence Score: 4/5
The two new security gates are correctly implemented and well-tested; the main thing to resolve before merging is the opaque-vs-revealing 401 error bodies and confirming production webhook permits are landed first per the rollout note.
The HMAC computation, constant-time comparison, fail-closed secret resolution, Cedar principal construction, and denial-record suppression for unauthenticated callers are all correct. The 11 exploit-regression tests cover the critical paths. The only concerns are two P2 items: three distinct 401 response bodies that reveal whether a secret is configured, and the ADR status left as Proposed.
The 401 error body differentiation in receiver.rs (verify_webhook_signature) and the ADR status field in docs/adrs/0156-class-b-webhook-ingress.md both warrant a quick fix before merge.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Caller as External Caller participant Handler as handle_webhook participant Registry as SpecRegistry participant Vault as SecretsVault participant HMAC as signature_matches() participant Cedar as authorize_with_context() participant Dispatch as dispatch_tenant_action() Caller->>Handler: "GET|POST /webhooks/{tenant}/{*path}" Handler->>Registry: find_webhook(tenant, path) Registry-->>Handler: Some(entity_type, webhook) or None → 404 Handler->>Handler: validate HTTP method → 405 alt hmac_secret declared Handler->>Vault: resolve_webhook_secret(tenant, template) Vault-->>Handler: resolved secret or None → 401 Handler->>Handler: read hmac_header → 401 if missing Handler->>HMAC: signature_matches(secret, body, provided) HMAC-->>Handler: bool via ct_eq → 401 if false end Handler->>Handler: extract entity_id from query params → 400 if missing Handler->>Cedar: load_authz_resource_snapshot(tenant, entity_type, entity_id) Cedar-->>Handler: resource_attrs or minimal fallback Handler->>Cedar: authorize_with_context(webhook:name principal, action, entity_type, attrs) alt Cedar denies Cedar-->>Handler: Err(denial) alt "authenticated=true" Handler->>Handler: record_authz_denial() Handler-->>Caller: 403 + decision.id else "authenticated=false" Handler-->>Caller: 403 (no record) end else Cedar permits Cedar-->>Handler: Ok(()) Handler->>Dispatch: dispatch_tenant_action(entity_type, entity_id, action, params) Dispatch-->>Handler: Ok(response) or Err → 500 Handler-->>Caller: 200 + JSON response 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 Caller as External Caller participant Handler as handle_webhook participant Registry as SpecRegistry participant Vault as SecretsVault participant HMAC as signature_matches() participant Cedar as authorize_with_context() participant Dispatch as dispatch_tenant_action() Caller->>Handler: "GET|POST /webhooks/{tenant}/{*path}" Handler->>Registry: find_webhook(tenant, path) Registry-->>Handler: Some(entity_type, webhook) or None → 404 Handler->>Handler: validate HTTP method → 405 alt hmac_secret declared Handler->>Vault: resolve_webhook_secret(tenant, template) Vault-->>Handler: resolved secret or None → 401 Handler->>Handler: read hmac_header → 401 if missing Handler->>HMAC: signature_matches(secret, body, provided) HMAC-->>Handler: bool via ct_eq → 401 if false end Handler->>Handler: extract entity_id from query params → 400 if missing Handler->>Cedar: load_authz_resource_snapshot(tenant, entity_type, entity_id) Cedar-->>Handler: resource_attrs or minimal fallback Handler->>Cedar: authorize_with_context(webhook:name principal, action, entity_type, attrs) alt Cedar denies Cedar-->>Handler: Err(denial) alt "authenticated=true" Handler->>Handler: record_authz_denial() Handler-->>Caller: 403 + decision.id else "authenticated=false" Handler-->>Caller: 403 (no record) end else Cedar permits Cedar-->>Handler: Ok(()) Handler->>Dispatch: dispatch_tenant_action(entity_type, entity_id, action, params) Dispatch-->>Handler: Ok(response) or Err → 500 Handler-->>Caller: 200 + JSON response endPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(server): authenticate webhook ingres..." | Re-trigger Greptile