Skip to content

feat(filter): add identity header guard filter - #709

Open
yossiovadia wants to merge 6 commits into
praxis-proxy:mainfrom
yossiovadia:feat/identity-header-guard
Open

feat(filter): add identity header guard filter#709
yossiovadia wants to merge 6 commits into
praxis-proxy:mainfrom
yossiovadia:feat/identity-header-guard

Conversation

@yossiovadia

Copy link
Copy Markdown
Contributor

Summary

Fixes #698

Motivation

The external_metering filter reads tenant identity from request headers for per-user usage attribution. These headers are set by an upstream auth layer and must not reach the upstream provider. reserved_headers in core only handles hardcoded x-praxis-* prefixes with no metadata capture and no configurable prefixes (core TODO #186).

Design

  • Configurable header prefix (default: x-tenant-)
  • Captured headers written to filter_metadata under a configurable namespace (prevents collision with verified auth metadata)
  • Matched headers marked for removal via request_headers_to_remove
  • ~120 lines of filter code

What's included

  • filters/src/identity_guard/ — filter, config, 11 unit tests + 1 doctest
  • tests/integration/tests/suite/examples/identity_header_guard.rs — 3 integration tests (config parse + header capture + strip)
  • examples/configs/identity-header-guard.yaml — example config
  • Generated filter docs and README updates

Test plan

  • 11 unit tests covering: prefix matching, case insensitivity, namespace isolation, no-match passthrough, empty/missing headers, multiple captures, strip verification
  • 1 doctest
  • 3 integration tests (config parse, header capture to metadata, upstream strip)
  • cargo xtask lint-example-tests — passes (example config has test coverage)
  • cargo xtask lint-filter-docs — passes (generated docs up to date)
  • cargo clippy -p praxis-ai-filters -- -D warnings — zero warnings
  • Validated end-to-end on OpenShift deployment with external_metering consuming captured identity

@yossiovadia
yossiovadia requested review from a team and aslakknutsen August 11, 2026 14:46

@leseb leseb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would core be more suitable for this instead of this repo? it's not so much "ai" related

@praxis-bot praxis-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean implementation — the filter logic is correct and the security-critical behavior (stripping before forwarding, namespaced metadata to avoid collision with verified auth) is well thought out. Three medium findings, all related to test coverage gaps.

Comment thread filters/src/identity_guard/tests.rs
Comment thread filters/src/register.rs
Comment thread filters/src/identity_guard/mod.rs
@jordigilh

jordigilh commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Flagging a likely conflict: this filter's x-tenant- prefix capture-and-strip overlaps with #581's external_metering filter, which expects the same x-tenant-* header convention (username/group/subscription/model) and does its own stripping of those headers. Neither PR references the other.

If both land as-is, the pipeline would end up with two filters independently capturing/stripping the same headers into different metadata shapes. Might be worth the two of you syncing on which owns the canonical filter_metadata namespace before either merges — cc @noyitz.

@praxis-bot praxis-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One new medium finding on duplicate-header handling. The previous review's three findings (misleading default-namespace test, missing registry assertion, non-UTF-8 edge case) still apply and are not repeated here.

Comment thread filters/src/identity_guard/mod.rs
@leseb

leseb commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

@yossiovadia please address the bot's review or dismiss with a reason

@yossiovadia

Copy link
Copy Markdown
Contributor Author

Good catch @jordigilh. This filter and #581 are designed to work together, not independently:

  • identity_header_guard runs before external_metering in the pipeline. It captures x-tenant-* headers into namespaced metadata (identity.x-tenant-username) and strips them.
  • external_metering (feat(metering): add external metering filter with balance checks and usage reporting #581) then reads identity from filter_metadata (where this filter put it) rather than from raw request headers. This is the trusted path — metadata written by an earlier filter is verified, headers are not.

The overlap in stripping is intentional redundancy: if identity_header_guard is present, it strips first. If it's absent (e.g., a deployment that doesn't need identity isolation), external_metering handles it as fallback.

The canonical metadata namespace is identity. (configurable via metadata_namespace). #581's metering filter reads from that namespace first, falling back to raw headers only when the guard isn't in the pipeline.

cc @noyitz — this is the same pattern as IPP's maas-headers-guardexternal-metering dependency.

@aslakknutsen

aslakknutsen commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Any reason why this feature couldn't be expressed by using the core filter Header? https://github.com/praxis-proxy/praxis/blob/main/filter/src/builtins/http/transformation/header/mod.rs#L82

Potentially adding wildcard and "move to metadata" as a feature?

@alexsnaps

@yossiovadia

Copy link
Copy Markdown
Contributor Author

Good question. Looked at the core HeaderFilter — it supports exact-name request_remove, request_set, etc. What this filter adds:

  1. Prefix matching — matches by configurable prefix (x-tenant-*), not exact header names. The set of headers isn't known at config time — an upstream auth layer (Authorino, Keycloak) can inject any x-tenant-* header.
  2. Capture to metadata — writes values to filter_metadata before stripping. The core filter strips headers but doesn't preserve them anywhere. Downstream filters (metering, audit) need the captured values.
  3. Namespaced metadata keys — writes to identity.x-tenant-username, not x-tenant-username, to prevent collision with auth-verified metadata from jwt_auth or api_key_auth.

Could these be added to the core filter? Yes — as a request_capture_and_remove option with a prefix field and metadata namespace. That's a valid design. The trade-off:

  • Extending core: more capable core filter, but adds identity/security semantics to a general-purpose transformation filter. The "capture to namespaced metadata then strip" behavior is a security invariant, not a transformation — getting the ordering wrong leaks identity to providers.
  • Separate filter: smaller (~80 lines), self-contained, security intent is explicit in the name and placement. Easier to audit.

I'm fine either way — if the team prefers this as a core headers extension, I can refactor. The filter_metadata write + namespace isolation are the non-negotiable parts regardless of where the code lives.

cc @alexsnaps

@jordigilh

Copy link
Copy Markdown
Contributor

CI's green but there's a merge conflict with main — mind rebasing? This blocks #130/#104.

Also still open: the standalone-vs-core-filter question from @aslakknutsen (Aug 13) — needs a call from @alexsnaps.

@yossiovadia
yossiovadia force-pushed the feat/identity-header-guard branch from ea02554 to dd29829 Compare August 19, 2026 22:40
@yossiovadia

Copy link
Copy Markdown
Contributor Author

Rebased onto latest main — conflicts were just the filter-registration list (register.rs import + the registry test's expected-filters array) and the integration examples/mod.rs module list, all from filters that landed on main since this branched. Resolved to the superset; cargo test, clippy, and fmt are clean locally and the net diff is unchanged in scope (11 files, identity_header_guard only). Should unblock #130/#104.

Intent is unchanged from the original PR: this stays the self-contained temporary bridge @leseb green-lit in #708, writing captured identity to filter_metadata (not upstream headers), with nothing else depending on it.

On the standalone-vs-core-filter question from @aslakknutsen (Aug 13): still needs a call from @alexsnaps. I'm happy to refactor into a core headers extension instead if that's the direction — the metadata-capture + strip behavior is the only non-negotiable; where the code lives is the maintainers' call.

@praxis-bot praxis-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review (3/3)

Previous findings #2 (registry assertion), #3 (non-UTF-8 test), and #4 (first-wins semantics) are resolved. One new finding from the fix commits.

Comment thread filters/src/identity_guard/tests.rs Outdated

@praxis-bot praxis-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One new medium finding on untested validation logic. All previous findings are resolved.

Comment thread filters/src/identity_guard/config.rs

@jordigilh jordigilh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the last-wins vs first-wins security gap is fixed (duplicate_headers_first_value_wins, a6bca42), matching the bot's original recommendation.

@yossiovadia
yossiovadia force-pushed the feat/identity-header-guard branch from 86c4dc9 to 7e85535 Compare August 25, 2026 03:51

@praxis-bot praxis-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review (5/5)

One new medium finding. The previous finding (#6, empty-namespace validation test) is still open and not repeated here.

Comment thread filters/src/register.rs Outdated
@yossiovadia
yossiovadia force-pushed the feat/identity-header-guard branch from 8428c38 to 0389131 Compare August 27, 2026 18:49
@shaneutt
shaneutt self-requested a review as a code owner August 28, 2026 17:11
shaneutt pushed a commit that referenced this pull request Aug 28, 2026
)

Signed-off-by: Sébastien Han <seb@redhat.com>
@leseb

leseb commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

@yossiovadia please rebase

@shaneutt shaneutt self-assigned this Sep 3, 2026
@shaneutt shaneutt moved this from Next to Review in AI Gateway - Model Serving Sep 3, 2026
@shaneutt shaneutt added this to the v0.3.0 milestone Sep 3, 2026
Captures request headers matching a configurable prefix into
filter_metadata and strips them before upstream forwarding.
Prevents identity headers (e.g. x-tenant-username, x-tenant-group)
from leaking to LLM providers while making them available to
downstream filters like external_metering.

Fixes praxis-proxy#698

Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
- Add registry assertion for identity_header_guard in
  build_ai_registry_includes_ai_and_builtin_filters test
- Add test for non-UTF-8 header values (stripped but not captured)
- Add test for duplicate headers (last-value-wins behavior)
- Clarify default namespace test with comment explaining the
  indirect verification via captures_matching_headers_to_metadata

Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
…filters

Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
Reflowed the use list after the rebase conflict resolution; the manual
wrap did not match nightly rustfmt's line-filling.

Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
@yossiovadia
yossiovadia force-pushed the feat/identity-header-guard branch from 0389131 to e44cc69 Compare September 8, 2026 17:53
@yossiovadia

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (post-#581). Conflict resolution was limited to the three registration sites (register.rs import list, examples/README.md table, integration suite mod.rs) — all both-sides-added-a-line merges, verified against the xtask sync checks. All gates green locally: filter unit + doctests + integration example, clippy (both feature sets), nightly fmt, machete, xtask suite.

One note for reviewers: this force-push also removes four previously stacked, now-superseded commits (MCP metadata block / credential redaction, #436/#438/#578, plus a chores commit) — upstream reimplemented that redaction/SSRF work in faa043f. The PR now contains only the identity_header_guard work.

@shaneutt fresh eyes when you get a chance — the rebase supersedes the earlier review.

@noyitz

noyitz commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

@leseb can you confirm and approve this one?

@leseb leseb modified the milestones: v0.3.0, v0.4.0 Sep 9, 2026
@leseb leseb assigned leseb and shaneutt and unassigned shaneutt and leseb Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Review

Development

Successfully merging this pull request may close these issues.

feat(filter): add identity header guard filter

7 participants